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

[Cédric Le Goater]

On 2/14/19 4:35 AM, David Gibson wrote:
> On Wed, Feb 13, 2019 at 11:07:49AM +0100, Greg Kurz wrote:
>> On Wed, 13 Feb 2019 09:22:46 +0100
>> Cédric Le Goater <address@hidden> wrote:
>>
>>> 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.
>>>>>  
>>>>>>> +     */
>>>>>>> +    if (xive->fd != -1) {
>>>>>>> +        return;
>>>>>>> +    }
>>>>>>>  
>>>>>>>      if (!kvm_enabled() || !kvmppc_has_cap_xive()) {
>>>>>>>          error_setg(errp, "IRQ_XIVE capability must be present for 
>>>>>>> KVM");
>>>>>>> @@ -800,6 +809,24 @@ void kvmppc_xive_connect(sPAPRXive *xive, Error 
>>>>>>> **errp)
>>>>>>>      xive->change = qemu_add_vm_change_state_handler(
>>>>>>>          kvmppc_xive_change_state_handler, xive);
>>>>>>>  
>>>>>>> +    /* Connect the presenters to the initial VCPUs of the machine */
>>>>>>> +    CPU_FOREACH(cs) {
>>>>>>> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
>>>>>>> +
>>>>>>> +        kvmppc_xive_cpu_connect(cpu->tctx, &local_err);
>>>>>>> +        if (local_err) {
>>>>>>> +            error_propagate(errp, local_err);
>>>>>>> +            return;
>>>>>>> +        }
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    /* Update the KVM sources */
>>>>>>> +    kvmppc_xive_source_reset(xsrc, &local_err);
>>>>>>> +    if (local_err) {
>>>>>>> +            error_propagate(errp, local_err);
>>>>>>> +            return;
>>>>>>> +    }
>>>>>>> +
>>>>>>>      kvm_kernel_irqchip = true;
>>>>>>>      kvm_msi_via_irqfd_allowed = true;
>>>>>>>      kvm_gsi_direct_mapping = true;
>>>>>>> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
>>>>>>> index 1d21ff217b82..bfc35d71df7f 100644
>>>>>>> --- a/hw/intc/xics_kvm.c
>>>>>>> +++ b/hw/intc/xics_kvm.c
>>>>>>> @@ -448,6 +448,16 @@ static void rtas_dummy(PowerPCCPU *cpu, 
>>>>>>> sPAPRMachineState *spapr,
>>>>>>>  int xics_kvm_init(sPAPRMachineState *spapr, Error **errp)
>>>>>>>  {
>>>>>>>      int rc;
>>>>>>> +    CPUState *cs;
>>>>>>> +    Error *local_err = NULL;
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * The KVM XICS device already in use. This is the case when
>>>>>>> +     * rebooting XICS -> XICS
>>>>>>> +     */
>>>>>>> +    if (kernel_xics_fd != -1) {
>>>>>>> +        return 0;
>>>>>>> +    }
>>>>>>>  
>>>>>>>      if (!kvm_enabled() || !kvm_check_extension(kvm_state, 
>>>>>>> KVM_CAP_IRQ_XICS)) {
>>>>>>>          error_setg(errp,
>>>>>>> @@ -496,6 +506,21 @@ int xics_kvm_init(sPAPRMachineState *spapr, Error 
>>>>>>> **errp)
>>>>>>>      kvm_msi_via_irqfd_allowed = true;
>>>>>>>      kvm_gsi_direct_mapping = true;
>>>>>>>  
>>>>>>> +    /* Connect the presenters to the initial VCPUs of the machine */
>>>>>>> +    CPU_FOREACH(cs) {
>>>>>>> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
>>>>>>> +
>>>>>>> +        icp_kvm_connect(cpu->icp, &local_err);
>>>>>>> +        if (local_err) {
>>>>>>> +            error_propagate(errp, local_err);
>>>>>>> +            goto fail;
>>>>>>> +        }
>>>>>>> +        icp_set_kvm_state(cpu->icp, 1);
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    /* Update the KVM sources */
>>>>>>> +    ics_set_kvm_state(ICS_KVM(spapr->ics), 1);
>>>>>>> +
>>>>>>>      return 0;
>>>>>>>  
>>>>>>>  fail:
>>>>>>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>>>>>>> index c5c2fbc3f8bc..c166eab5b210 100644
>>>>>>> --- a/hw/intc/xive.c
>>>>>>> +++ b/hw/intc/xive.c
>>>>>>> @@ -932,10 +932,6 @@ static void xive_source_reset(void *dev)
>>>>>>>  
>>>>>>>      /* PQs are initialized to 0b01 (Q=1) which corresponds to "ints 
>>>>>>> off" */
>>>>>>>      memset(xsrc->status, XIVE_ESB_OFF, xsrc->nr_irqs);
>>>>>>> -
>>>>>>> -    if (kvmppc_xive_enabled()) {
>>>>>>> -        kvmppc_xive_source_reset(xsrc, &error_fatal);
>>>>>>> -    }
>>>>>>>  }
>>>>>>>  
>>>>>>>  static void xive_source_realize(DeviceState *dev, Error **errp)
>>>>>>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>>>>>>> index ba27d9d8e972..5592eec3787b 100644
>>>>>>> --- a/hw/ppc/spapr_irq.c
>>>>>>> +++ b/hw/ppc/spapr_irq.c
>>>>>>> @@ -98,20 +98,14 @@ static void spapr_irq_init_xics(sPAPRMachineState 
>>>>>>> *spapr, Error **errp)
>>>>>>>      int nr_irqs = spapr->irq->nr_irqs;
>>>>>>>      Error *local_err = NULL;
>>>>>>>  
>>>>>>> -    if (kvm_enabled()) {
>>>>>>> -        if (machine_kernel_irqchip_allowed(machine) &&
>>>>>>> -            !xics_kvm_init(spapr, &local_err)) {
>>>>>>> -            spapr->icp_type = TYPE_KVM_ICP;
>>>>>>> -            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
>>>>>>> -                                          &local_err);
>>>>>>> -        }
>>>>>>> -        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
>>>>>>> -            error_prepend(&local_err,
>>>>>>> -                          "kernel_irqchip requested but unavailable: 
>>>>>>> ");
>>>>>>> -            goto error;  
>>>>>>
>>>>>> I don't see anything that replaces the irqchip_required logic, which
>>>>>> doesn't seem right.  
>>>>>
>>>>> Yes. We do loose the ability to fall back to the emulated device in case
>>>>> of failure. It is not impossible to do but it will require more changes
>>>>> to check what are the KVM capabilities before starting the machine.  
>>>>
>>>> Uh... it seems more like it's the other way around.  We'll always fall
>>>> back to emulated, even if we've explicitly said on the command line
>>>> that we don't want that.  
>>>
>>> Ah yes. The init function might be also broken. 
>>>
>>> XICS mode is a bit more difficult to handle than XIVE because we have 
>>> different object type for the KVM device and the QEMU emulated device, 
>>
>> This is indeed a bit unfortunate, but I think there's still room for
>> improvement. Let's look at the base classes:
>>
>> struct ICPStateClass {
>>     DeviceClass parent_class;
>>
>>     DeviceRealize parent_realize;
>>     DeviceReset parent_reset;
>>
>>     void (*pre_save)(ICPState *icp);
>>     int (*post_load)(ICPState *icp, int version_id);
>>     void (*synchronize_state)(ICPState *icp);
>> };
>>
>> struct ICSStateClass {
>>     DeviceClass parent_class;
>>
>>     DeviceRealize parent_realize;
>>     DeviceReset parent_reset;
>>
>>     void (*pre_save)(ICSState *s);
>>     int (*post_load)(ICSState *s, int version_id);
>>     void (*reject)(ICSState *s, uint32_t irq);
>>     void (*resend)(ICSState *s);
>>     void (*eoi)(ICSState *s, uint32_t irq);
>>     void (*synchronize_state)(ICSState *s);
>> };
>>
>> The pre_save and post_load callbacks are only used with
>> the KVM device. They could be explicitely called from
>> the corresponding VMStateDescription callbacks with a
>> kvm_enabled() && kvm_irqchip_in_kernel() check.

yes.
>> Same goes for the synchronize_state callbacks, which are only
>> needed for 'info pic'.

yes, like we do for XIVE.

>> The reject, resend and eoi callbacks are only called by code that
>> belongs to the QEMU emulated device. Either the RTAS/hypercalls
>> or from the machine code with explicit checks like:
>>
>> static void spapr_irq_set_irq_xics(void *opaque, int srcno, int val)
>> {
>>     sPAPRMachineState *spapr = opaque;
>>     MachineState *machine = MACHINE(opaque);
>>
>>     if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {
>>         ics_kvm_set_irq(spapr->ics, srcno, val);
>>     } else {
>>         ics_simple_set_irq(spapr->ics, srcno, val);
>>     }
>> }
>>
>> or
>>
>> static int spapr_irq_post_load_xics(sPAPRMachineState *spapr, int version_id)
>> {
>>     if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
>>         CPUState *cs;
>>         CPU_FOREACH(cs) {
>>             PowerPCCPU *cpu = POWERPC_CPU(cs);
>>             icp_resend(spapr_cpu_state(cpu)->icp);
>>         }
>>     }
>>     return 0;
>> }
>>
>> Unless I'm missing something, the reject, resend and eoi callbacks could
>> simply be removed. This would allow to unify KVM and QEMU emulation in
>> the same ICP and ICS object types.

yes.  

>> If this makes sense to you, I can have a look (already started actually ;-)
> 
> Please do.  The use of different object types was something that
> seemed like a good idea at the time, but in hindsight, wasn't.  In
> general different device types should represent guest-visibly
> different objects, not just implementation differences.

yes. The direction we took to add KVM support in XIVE proved to be 
much simpler.

>>> and with the 'dual' mode, we activate the device at CAS reset time.
>>>
>>> Failures being handled at reset time, should we keep the same logic and  
>>> abort the machine at reset if the kernel irqchip is required ? 
>>>
>>
>> If the user passed ic-mode=dual,kernel-irqchip=on, we should at least make
>> sure KVM supports both XICS and XIVE devices during machine init. Then
>> during reset if something goes wrong with KVM, it seems ok to abort.
>>
>> If the user didn't pass kernel-irqchip, ie, kernel_irqchip_allowed is true
>> and kernel_irqchip_required is false, the current behavior for XICS is
>> to try KVM first and fallback to QEMU emulation. I guess it could be the
>> same for XIVE.
> 
> Yes, I think that's the behaviour we want, on all counts.
 
Thanks,

C.