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

[Cédric Le Goater]

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

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


Thanks, 

C.

 
>>> +        spapr_ovec_set(spapr->ov5, OV5_XIVE_EXPLOIT);
>>> +    }
>>> +
>>>      /* init CPUs */
>>>      spapr_init_cpus(spapr);
>>>  
>>> @@ -3971,6 +3982,21 @@ static void 
>>> spapr_machine_3_1_class_options(MachineClass *mc)
>>>  
>>>  DEFINE_SPAPR_MACHINE(3_1, "3.1", true);
>>>  
>>> +static void spapr_machine_3_1_xive_instance_options(MachineState *machine)
>>> +{
>>> +    spapr_machine_3_1_instance_options(machine);
>>> +}
>>> +
>>> +static void spapr_machine_3_1_xive_class_options(MachineClass *mc)
>>> +{
>>> +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
>>> +
>>> +    spapr_machine_3_1_class_options(mc);
>>> +    smc->irq = &spapr_irq_xive;
>>> +}
>>> +
>>> +DEFINE_SPAPR_MACHINE(3_1_xive, "3.1-xive", false);
>>> +
>>>  /*
>>>   * pseries-3.0
>>>   */
>>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>>> index 253abc10e780..42e73851b174 100644
>>> --- a/hw/ppc/spapr_irq.c
>>> +++ b/hw/ppc/spapr_irq.c
>>> @@ -210,6 +210,7 @@ static Object 
>>> *spapr_irq_cpu_intc_create_xics(sPAPRMachineState *spapr,
>>>  sPAPRIrq spapr_irq_xics = {
>>>      .nr_irqs     = SPAPR_IRQ_XICS_NR_IRQS,
>>>      .nr_msis     = SPAPR_IRQ_XICS_NR_MSIS,
>>> +    .ov5         = 0x0, /* XICS only */
>>>  
>>>      .init        = spapr_irq_init_xics,
>>>      .claim       = spapr_irq_claim_xics,
>>> @@ -341,6 +342,7 @@ static Object 
>>> *spapr_irq_cpu_intc_create_xive(sPAPRMachineState *spapr,
>>>  sPAPRIrq spapr_irq_xive = {
>>>      .nr_irqs     = SPAPR_IRQ_XIVE_NR_IRQS,
>>>      .nr_msis     = SPAPR_IRQ_XIVE_NR_MSIS,
>>> +    .ov5         = 0x40, /* XIVE exploitation mode only */
>>>  
>>>      .init        = spapr_irq_init_xive,
>>>      .claim       = spapr_irq_claim_xive,
>>> @@ -447,6 +449,7 @@ int spapr_irq_find(sPAPRMachineState *spapr, int num, 
>>> bool align, Error **errp)
>>>  sPAPRIrq spapr_irq_xics_legacy = {
>>>      .nr_irqs     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
>>>      .nr_msis     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
>>> +    .ov5         = 0x0, /* XICS only */
>>>  
>>>      .init        = spapr_irq_init_xics,
>>>      .claim       = spapr_irq_claim_xics,
>>
>