Re: [Qemu-devel] [PATCH] spapr/irq: Add XIVE sanity checks on non-P9 machines

[David Gibson]

On Thu, Mar 28, 2019 at 11:00:44AM +0100, Cédric Le Goater wrote:
> On non-P9 machines, the XIVE interrupt mode is not advertised, see
> spapr_dt_ov5_platform_support(). Add a couple of checks on the machine
> configuration to filter bogus setups and prevent OS failures :
> 
>                      Interrupt modes
> 
>   CPU/Compat      XICS    XIVE                dual
> 
>    P8/P8          OK      QEMU failure (1)    OK (3)
>    P9/P8          OK      QEMU failure (2)    OK (3)
>    P9/P9          OK      OK                  OK
> 
>   (1) CPU exception model is incompatible with XIVE and the presenters
>       will fail to realize.
> 
>   (2) CPU exception model is compatible with XIVE, but the XIVE CAS
>       advertisement is dropped when in POWER8 mode. So we could ended up
>       booting with the XIVE DT properties but without the HCALLs. Avoid
>       confusing Linux with such settings and fail under QEMU.
> 
>   (3) force XICS in machine init
> 
> Remove the check on XIVE-only machines in spapr_machine_init(), which
> has now become redundant.
> 
> Signed-off-by: Cédric Le Goater <address@hidden>

Applied, thanks.

> ---
>  hw/ppc/spapr.c     |  8 +-------
>  hw/ppc/spapr_irq.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index adde36a01da3..33ab7b683ba2 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2806,13 +2806,7 @@ static void spapr_machine_init(MachineState *machine)
>  
>      /* advertise XIVE on POWER9 machines */
>      if (spapr->irq->ov5 & (SPAPR_OV5_XIVE_EXPLOIT | SPAPR_OV5_XIVE_BOTH)) {
> -        if (ppc_type_check_compat(machine->cpu_type, 
> CPU_POWERPC_LOGICAL_3_00,
> -                                  0, spapr->max_compat_pvr)) {
> -            spapr_ovec_set(spapr->ov5, OV5_XIVE_EXPLOIT);
> -        } else if (spapr->irq->ov5 & SPAPR_OV5_XIVE_EXPLOIT) {
> -            error_report("XIVE-only machines require a POWER9 CPU");
> -            exit(1);
> -        }
> +        spapr_ovec_set(spapr->ov5, OV5_XIVE_EXPLOIT);
>      }
>  
>      /* init CPUs */
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 5809c955501c..b1f79ea9def6 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -16,6 +16,7 @@
>  #include "hw/ppc/spapr_xive.h"
>  #include "hw/ppc/xics.h"
>  #include "hw/ppc/xics_spapr.h"
> +#include "cpu-models.h"
>  #include "sysemu/kvm.h"
>  
>  #include "trace.h"
> @@ -566,12 +567,55 @@ SpaprIrq spapr_irq_dual = {
>      .get_nodename = spapr_irq_get_nodename_dual,
>  };
>  
> +
> +static void spapr_irq_check(SpaprMachineState *spapr, Error **errp)
> +{
> +    MachineState *machine = MACHINE(spapr);
> +
> +    /*
> +     * Sanity checks on non-P9 machines. On these, XIVE is not
> +     * advertised, see spapr_dt_ov5_platform_support()
> +     */
> +    if (!ppc_type_check_compat(machine->cpu_type, CPU_POWERPC_LOGICAL_3_00,
> +                               0, spapr->max_compat_pvr)) {
> +        /*
> +         * If the 'dual' interrupt mode is selected, force XICS as CAS
> +         * negotiation is useless.
> +         */
> +        if (spapr->irq == &spapr_irq_dual) {
> +            spapr->irq = &spapr_irq_xics;
> +            return;
> +        }
> +
> +        /*
> +         * Non-P9 machines using only XIVE is a bogus setup. We have two
> +         * scenarios to take into account because of the compat mode:
> +         *
> +         * 1. POWER7/8 machines should fail to init later on when creating
> +         *    the XIVE interrupt presenters because a POWER9 exception
> +         *    model is required.
> +
> +         * 2. POWER9 machines using the POWER8 compat mode won't fail and
> +         *    will let the OS boot with a partial XIVE setup : DT
> +         *    properties but no hcalls.
> +         *
> +         * To cover both and not confuse the OS, add an early failure in
> +         * QEMU.
> +         */
> +        if (spapr->irq == &spapr_irq_xive) {
> +            error_setg(errp, "XIVE-only machines require a POWER9 CPU");
> +            return;
> +        }
> +    }
> +}
> +
>  /*
>   * sPAPR IRQ frontend routines for devices
>   */
>  void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
>  {
>      MachineState *machine = MACHINE(spapr);
> +    Error *local_err = NULL;
>  
>      if (machine_kernel_irqchip_split(machine)) {
>          error_setg(errp, "kernel_irqchip split mode not supported on 
> pseries");
> @@ -584,6 +628,12 @@ void spapr_irq_init(SpaprMachineState *spapr, Error 
> **errp)
>          return;
>      }
>  
> +    spapr_irq_check(spapr, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
>      /* Initialize the MSI IRQ allocator. */
>      if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
>          spapr_irq_msi_init(spapr, spapr->irq->nr_msis);

-- 
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