Re: [Qemu-devel] [PATCH 13/13] spapr: add KVM support to the 'dual' machine

[Cédric Le Goater]

On 2/13/19 2:32 AM, David Gibson wrote:
> On Tue, Feb 12, 2019 at 08:18:19AM +0100, Cédric Le Goater wrote:
>> On 2/12/19 2:11 AM, David Gibson wrote:
>>> On Mon, Jan 07, 2019 at 07:39:46PM +0100, Cédric Le Goater wrote:
>>>> The interrupt mode is chosen by the CAS negotiation process and
>>>> activated after a reset to take into account the required changes in
>>>> the machine. This brings new constraints on how the associated KVM IRQ
>>>> device is initialized.
>>>>
>>>> Currently, each model takes care of the initialization of the KVM
>>>> device in their realize method but this is not possible anymore as the
>>>> initialization needs to be done globaly when the interrupt mode is
>>>> known, i.e. when machine is reseted. It also means that we need a way
>>>> to delete a KVM device when another mode is chosen.
>>>>
>>>> Also, to support migration, the QEMU objects holding the state to
>>>> transfer should always be available but not necessarily activated.
>>>>
>>>> The overall approach of this proposal is to initialize both interrupt
>>>> mode at the QEMU level and keep the IRQ number space in sync to allow
>>>> switching from one mode to another. For the KVM side of things, the
>>>> whole initialization of the KVM device, sources and presenters, is
>>>> grouped in a single routine. The XICS and XIVE sPAPR IRQ reset
>>>> handlers are modified accordingly to handle the init and the delete
>>>> sequences of the KVM device.
>>>>
>>>> As KVM is now initialized at reset, we loose the possiblity to
>>>> fallback to the QEMU emulated mode in case of failure and failures
>>>> become fatal to the machine.
>>>>
>>>> Signed-off-by: Cédric Le Goater <address@hidden>
>>>> ---
>>>>  hw/intc/spapr_xive.c     |  8 +---
>>>>  hw/intc/spapr_xive_kvm.c | 27 ++++++++++++++
>>>>  hw/intc/xics_kvm.c       | 25 +++++++++++++
>>>>  hw/intc/xive.c           |  4 --
>>>>  hw/ppc/spapr_irq.c       | 79 ++++++++++++++++++++++++++++------------
>>>>  5 files changed, 109 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>>>> index 21f3c1ef0901..0661aca35900 100644
>>>> --- a/hw/intc/spapr_xive.c
>>>> +++ b/hw/intc/spapr_xive.c
>>>> @@ -330,13 +330,7 @@ static void spapr_xive_realize(DeviceState *dev, 
>>>> Error **errp)
>>>>      xive->eat = g_new0(XiveEAS, xive->nr_irqs);
>>>>      xive->endt = g_new0(XiveEND, xive->nr_ends);
>>>>  
>>>> -    if (kvmppc_xive_enabled()) {
>>>> -        kvmppc_xive_connect(xive, &local_err);
>>>> -        if (local_err) {
>>>> -            error_propagate(errp, local_err);
>>>> -            return;
>>>> -        }
>>>> -    } else {
>>>> +    if (!kvmppc_xive_enabled()) {
>>>>          /* TIMA initialization */
>>>>          memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &xive_tm_ops, 
>>>> xive,
>>>>                                "xive.tima", 4ull << TM_SHIFT);
>>>> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
>>>> index d35814c1992e..3ebc947f2be7 100644
>>>> --- a/hw/intc/spapr_xive_kvm.c
>>>> +++ b/hw/intc/spapr_xive_kvm.c
>>>> @@ -737,6 +737,15 @@ void kvmppc_xive_connect(sPAPRXive *xive, Error 
>>>> **errp)
>>>>      Error *local_err = NULL;
>>>>      size_t esb_len;
>>>>      size_t tima_len;
>>>> +    CPUState *cs;
>>>> +
>>>> +    /*
>>>> +     * The KVM XIVE device already in use. This is the case when
>>>> +     * rebooting XIVE -> XIVE
>>>
>>> Can this case actually occur?  Further down you appear to
>>> unconditionally destroy both KVM devices at reset time.
>>
>> I guess you are right. I will check.

So we have to keep it for ic-mode=xive.

The number of test combinations is exploding. We now have :

3 hypervisors              KVM, KVM nested, QEMU TCG
3 Interrupt modes          xics, xive, dual (xics+xive)
3 kernel irqchip modes     off, allow, required.


C.