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

[Laurent Vivier]

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 <address@hidden>
> 
> 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 <address@hidden>
> ---
>  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).

Thanks,
Laurent