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 <c...@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 <da...@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. -- 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
signature.asc
Description: PGP signature