Re: [Qemu-devel] [PATCH v5 19/36] spapr: add a 'pseries-3.1-xive' machine type

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

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

On Tue, Dec 04, 2018 at 04:14:12PM +0100, Cédric Le Goater wrote:
> On 11/28/18 11:37 PM, Cédric Le Goater wrote:
> > On 11/28/18 5:42 AM, David Gibson wrote:
> >> On Fri, Nov 16, 2018 at 11:57:12AM +0100, Cédric Le Goater wrote:
> >>> The interrupt mode is statically defined to XIVE only for this machine.
> >>> The guest OS is required to have support for the XIVE exploitation
> >>> mode of the POWER9 interrupt controller.
> >>>
> >>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >>> ---
> >>>  include/hw/ppc/spapr_irq.h |  1 +
> >>>  hw/ppc/spapr.c             | 36 +++++++++++++++++++++++++++++++-----
> >>>  hw/ppc/spapr_irq.c         |  3 +++
> >>>  3 files changed, 35 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> >>> index c3b4c38145eb..b299dd794bff 100644
> >>> --- a/include/hw/ppc/spapr_irq.h
> >>> +++ b/include/hw/ppc/spapr_irq.h
> >>> @@ -33,6 +33,7 @@ void spapr_irq_msi_reset(sPAPRMachineState *spapr);
> >>>  typedef struct sPAPRIrq {
> >>>      uint32_t    nr_irqs;
> >>>      uint32_t    nr_msis;
> >>> +    uint8_t     ov5;
> >>
> >> I'm a bit confused as to what exactly this represents..
> > 
> > The option vector 5 bits advertised by CAS for the platform. What the
> > hypervisor supports.
> 
> 0x80 both mode
> 0x40 XIVE only
> 0x00 XICS only

Yes....

> 
> >>
> >>>      void (*init)(sPAPRMachineState *spapr, int nr_irqs, int nr_servers,
> >>>                   Error **errp);
> >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>> index ad1692cdcd0f..8fbb743769db 100644
> >>> --- a/hw/ppc/spapr.c
> >>> +++ b/hw/ppc/spapr.c
> >>> @@ -1097,12 +1097,14 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr, void *fdt)
> >>>      spapr_dt_rtas_tokens(fdt, rtas);
> >>>  }
> >>>  
> >>> -/* Prepare ibm,arch-vec-5-platform-support, which indicates the MMU features
> >>> - * that the guest may request and thus the valid values for bytes 24..26 of
> >>> - * option vector 5: */
> >>> -static void spapr_dt_ov5_platform_support(void *fdt, int chosen)
> >>> +/* Prepare ibm,arch-vec-5-platform-support, which indicates the MMU
> >>> + * and the XIVE features that the guest may request and thus the valid
> >>> + * values for bytes 23..26 of option vector 5: */
> >>> +static void spapr_dt_ov5_platform_support(sPAPRMachineState *spapr, void *fdt,
> >>> +                                          int chosen)
> >>>  {
> >>>      PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> >>> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> >>>  
> >>>      char val[2 * 4] = {
> >>>          23, 0x00, /* Xive mode, filled in below. */
> >>> @@ -1123,7 +1125,11 @@ static void spapr_dt_ov5_platform_support(void *fdt, int chosen)
> >>>          } else {
> >>>              val[3] = 0x00; /* Hash */
> >>>          }
> >>> +        /* TODO: test KVM support */
> >>> +        val[1] = smc->irq->ov5;
> >>>      } else {
> >>> +        val[1] = smc->irq->ov5;
> >>
> >> ..here it seems to be a specific value for this OV5 byte, indicating the
> >> supported intc...
> > 
> > yes.> 
> >>
> >>> +
> >>>          /* V3 MMU supports both hash and radix in tcg (with dynamic switching) */
> >>>          val[3] = 0xC0;
> >>>      }
> >>> @@ -1191,7 +1197,7 @@ static void spapr_dt_chosen(sPAPRMachineState *spapr, void *fdt)
> >>>          _FDT(fdt_setprop_string(fdt, chosen, "stdout-path", stdout_path));
> >>>      }
> >>>  
> >>> -    spapr_dt_ov5_platform_support(fdt, chosen);
> >>> +    spapr_dt_ov5_platform_support(spapr, fdt, chosen);
> >>>  
> >>>      g_free(stdout_path);
> >>>      g_free(bootlist);
> >>> @@ -2622,6 +2628,11 @@ static void spapr_machine_init(MachineState *machine)
> >>>      /* advertise support for ibm,dyamic-memory-v2 */
> >>>      spapr_ovec_set(spapr->ov5, OV5_DRMEM_V2);
> >>>  
> >>> +    /* advertise XIVE */
> >>> +    if (smc->irq->ov5) {
> >>
> >> ..but here it seems to be a bool indicating XIVE support specifically.
> > 
> > ah. yes. I need to check this part. That was a while ago.
> 
> This is advertising XIVE again if the machine supports it. We need to 
> populate the DT node "ibm,arch-vec-5-platform-support" in routine
> spapr_dt_ov5_platform_support() *and* also to update the machine field 
> spapr->ov5. But it seems redundant to me. 
> 
> spapr->ov5 should be used to build the DT. Shouldn't it ? Or I really 
> missed something.

Possibly, but we are talking PAPR here, which is the king of putting
the same information in multiple places, differently encoded.  You'll
need to check it.

Regardless please don't use if (smc->irq->ov5) as a shortcut for if
(smc->irq->ov5 != XICS_ONLY).  The latter is much clearer and doesn't
mislead as to the type of ...->ov5.

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