Re: [Qemu-devel] [Qemu-ppc] [PULL 1/2] spapr: Reset CAS & IRQ subsystem after devices

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 7460 bytes --]

On Fri, Aug 23, 2019 at 03:47:52PM +0200, Laurent Vivier wrote:
> On 23/08/2019 07:39, David Gibson wrote:
> > On Thu, Aug 22, 2019 at 10:07:09PM +0200, Laurent Vivier wrote:
> >> On 13/08/2019 08:59, David Gibson wrote:
> >>> This fixes a nasty regression in qemu-4.1 for the 'pseries' machine,
> >>> caused by the new "dual" interrupt controller model.  Specifically,
> >>> qemu can crash when used with KVM if a 'system_reset' is requested
> >>> while there's active I/O in the guest.
> >>>
> >>> The problem is that in spapr_machine_reset() we:
> >>>
> >>> 1. Reset the CAS vector state
> >>> 	spapr_ovec_cleanup(spapr->ov5_cas);
> >>>
> >>> 2. Reset all devices
> >>> 	qemu_devices_reset()
> >>>
> >>> 3. Reset the irq subsystem
> >>> 	spapr_irq_reset();
> >>>
> >>> However (1) implicitly changes the interrupt delivery mode, because
> >>> whether we're using XICS or XIVE depends on the CAS state.  We don't
> >>> properly initialize the new irq mode until (3) though - in particular
> >>> setting up the KVM devices.
> >>>
> >>> During (2), we can temporarily drop the BQL allowing some irqs to be
> >>> delivered which will go to an irq system that's not properly set up.
> >>>
> >>> Specifically, if the previous guest was in (KVM) XIVE mode, the CAS
> >>> reset will put us back in XICS mode.  kvm_kernel_irqchip() still
> >>> returns true, because XIVE was using KVM, however XICs doesn't have
> >>> its KVM components intialized and kernel_xics_fd == -1.  When the irq
> >>> is delivered it goes via ics_kvm_set_irq() which assert()s that
> >>> kernel_xics_fd != -1.
> >>>
> >>> This change addresses the problem by delaying the CAS reset until
> >>> after the devices reset.  The device reset should quiesce all the
> >>> devices so we won't get irqs delivered while we mess around with the
> >>> IRQ.  The CAS reset and irq re-initialize should also now be under the
> >>> same BQL critical section so nothing else should be able to interrupt
> >>> it either.
> >>>
> >>> We also move the spapr_irq_msi_reset() used in one of the legacy irq
> >>> modes, since it logically makes sense at the same point as the
> >>> spapr_irq_reset() (it's essentially an equivalent operation for older
> >>> machine types).  Since we don't need to switch between different
> >>> interrupt controllers for those old machine types it shouldn't
> >>> actually be broken in those cases though.
> >>>
> >>> Cc: Cédric Le Goater <clg@kaod.org>
> >>>
> >>> Fixes: b2e22477 "spapr: add a 'reset' method to the sPAPR IRQ backend"
> >>> Fixes: 13db0cd9 "spapr: introduce a new sPAPR IRQ backend supporting
> >>>                  XIVE and XICS"
> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >>> ---
> >>>  hw/ppc/spapr.c | 24 ++++++++++++------------
> >>>  1 file changed, 12 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>> index 821f0d4a49..12ed4b065c 100644
> >>> --- a/hw/ppc/spapr.c
> >>> +++ b/hw/ppc/spapr.c
> >>> @@ -1726,6 +1726,18 @@ static void spapr_machine_reset(MachineState *machine)
> >>>          spapr_setup_hpt_and_vrma(spapr);
> >>>      }
> >>>  
> >>> +    /*
> >>> +     * NVLink2-connected GPU RAM needs to be placed on a separate NUMA node.
> >>> +     * We assign a new numa ID per GPU in spapr_pci_collect_nvgpu() which is
> >>> +     * called from vPHB reset handler so we initialize the counter here.
> >>> +     * If no NUMA is configured from the QEMU side, we start from 1 as GPU RAM
> >>> +     * must be equally distant from any other node.
> >>> +     * The final value of spapr->gpu_numa_id is going to be written to
> >>> +     * max-associativity-domains in spapr_build_fdt().
> >>> +     */
> >>> +    spapr->gpu_numa_id = MAX(1, nb_numa_nodes);
> >>> +    qemu_devices_reset();
> >>> +
> >>>      /*
> >>>       * If this reset wasn't generated by CAS, we should reset our
> >>>       * negotiated options and start from scratch
> >>> @@ -1741,18 +1753,6 @@ static void spapr_machine_reset(MachineState *machine)
> >>>          spapr_irq_msi_reset(spapr);
> >>>      }
> >>>  
> >>> -    /*
> >>> -     * NVLink2-connected GPU RAM needs to be placed on a separate NUMA node.
> >>> -     * We assign a new numa ID per GPU in spapr_pci_collect_nvgpu() which is
> >>> -     * called from vPHB reset handler so we initialize the counter here.
> >>> -     * If no NUMA is configured from the QEMU side, we start from 1 as GPU RAM
> >>> -     * must be equally distant from any other node.
> >>> -     * The final value of spapr->gpu_numa_id is going to be written to
> >>> -     * max-associativity-domains in spapr_build_fdt().
> >>> -     */
> >>> -    spapr->gpu_numa_id = MAX(1, nb_numa_nodes);
> >>> -    qemu_devices_reset();
> >>> -
> >>>      /*
> >>>       * This is fixing some of the default configuration of the XIVE
> >>>       * devices. To be called after the reset of the machine devices.
> >>>
> >>
> >> This commit breaks migration between POWER8 <-> POWER9 hosts:
> >>
> >> qemu-system-ppc64: error while loading state for instance 0x1 of device 'cpu'
> >> qemu-system-ppc64: load of migration failed: Operation not permitted
> >>
> >> Using a guest with a running 4.18 kernel (RHEL 8) and "-M pseries,max-cpu-compat=power8" on both sides.
> >>
> >> There is no problem if both hosts are of the same kind ( P8 <-> P8 or P9 <-> P9).
> > 
> > Crud, I was afraid there might be some subtle dependency on the
> > reverse order.
> 
> It seems the side effect of patch in comment 5 is to add a supplementary field compat_pvr in CPUs in the migration stream:
> 
>         {
>             "name": "cpu",
>             "instance_id": 0,
>             "vmsd_name": "cpu",
>             "version": 5,
> ...
>             "subsections": [
> ...
>                 {
>                     "vmsd_name": "cpu/compat",
>                     "version": 1,
>                     "fields": [
>                         {
>                             "name": "compat_pvr",
>                             "type": "uint32",
>                             "size": 4
>                         }
>                     ]
>                 }
>             ]
>         },
> ...
> 
> What seems to happen is compat_pvr is not propagated correctly to all
>  CPUs.
> 
> Originally, spapr_machine_reset() calls ppc_set_compat() to set the 
> value to max_compat_pvr for the first cpu and this was propagated to all 
> CPUs by spapr_cpu_reset().
> Now, as spapr_cpu_reset() is called before that, the value is not 
> propagated to all CPUs.
> 
> A simple fix seems to be:
> 
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1752,7 +1752,7 @@ static void spapr_machine_reset(MachineState *machine)
>          spapr_ovec_cleanup(spapr->ov5_cas);
>          spapr->ov5_cas = spapr_ovec_new();
>  
> -        ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
> +        ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
>      }
>  
>      /*
> 

As discussed on our call, I think this is a good fix at least in the
medium term.  If you can add in some explanatory comments and post it
formally ASAP.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]