Re: [Qemu-devel] [PATCH v8 11/12] spapr: introduce a new sPAPR IRQ backend supporting XIVE and XICS

[Cédric Le Goater]

On 12/17/18 7:07 AM, David Gibson wrote:
> On Tue, Dec 11, 2018 at 11:38:22PM +0100, Cédric Le Goater wrote:
>> The 'dual' sPAPR IRQ backend supports both interrupt mode, XIVE
>> exploitation mode and the legacy compatibility mode (XICS). both modes
>> are not supported at the same time.
>>
>> The machine starts with the legacy mode and a new interrupt mode can
>> then be negotiated by the CAS process. In this case, the new mode is
>> activated after a reset to take into account the required changes in
>> the machine. These impact the device tree layout, the interrupt
>> presenter object and the exposed MMIO regions in the case of XIVE.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>
>>  Changes since v7:
>>
>>  - usage of 'ic-mode' machine option
>>
>>  include/hw/ppc/spapr_irq.h |   1 +
>>  hw/ppc/spapr.c             |  10 ++-
>>  hw/ppc/spapr_hcall.c       |  11 +++
>>  hw/ppc/spapr_irq.c         | 143 +++++++++++++++++++++++++++++++++++++
>>  4 files changed, 162 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>> index b34d5a00381b..29936498dbc8 100644
>> --- a/include/hw/ppc/spapr_irq.h
>> +++ b/include/hw/ppc/spapr_irq.h
>> @@ -51,6 +51,7 @@ typedef struct sPAPRIrq {
>>  extern sPAPRIrq spapr_irq_xics;
>>  extern sPAPRIrq spapr_irq_xics_legacy;
>>  extern sPAPRIrq spapr_irq_xive;
>> +extern sPAPRIrq spapr_irq_dual;
>>  
>>  void spapr_irq_init(sPAPRMachineState *spapr, Error **errp);
>>  int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error 
>> **errp);
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 5d985e38a598..97a5e3c9929f 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -2636,11 +2636,11 @@ static void spapr_machine_init(MachineState *machine)
>>      spapr_ovec_set(spapr->ov5, OV5_DRMEM_V2);
>>  
>>      /* advertise XIVE on POWER9 machines */
>> -    if (spapr->irq->ov5 & SPAPR_OV5_XIVE_EXPLOIT) {
>> +    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 {
>> +        } else if (spapr->irq->ov5 & SPAPR_OV5_XIVE_EXPLOIT) {
>>              error_report("XIVE-only machines require a POWER9 CPU");
>>              exit(1);
>>          }
>> @@ -3066,6 +3066,8 @@ static char *spapr_get_ic_mode(Object *obj, Error 
>> **errp)
>>          return g_strdup("xics");
>>      } else if (spapr->irq == &spapr_irq_xive) {
>>          return g_strdup("xive");
>> +    } else if (spapr->irq == &spapr_irq_dual) {
>> +        return g_strdup("dual");
>>      }
>>      g_assert_not_reached();
>>  }
>> @@ -3079,6 +3081,8 @@ static void spapr_set_ic_mode(Object *obj, const char 
>> *value, Error **errp)
>>          spapr->irq = &spapr_irq_xics;
>>      } else if (strcmp(value, "xive") == 0) {
>>          spapr->irq = &spapr_irq_xive;
>> +    } else if (strcmp(value, "dual") == 0) {
>> +        spapr->irq = &spapr_irq_dual;
>>      } else {
>>          error_setg(errp, "Bad value for \"ic-mode\" property");
>>      }
>> @@ -3127,7 +3131,7 @@ static void spapr_instance_init(Object *obj)
>>      object_property_add_str(obj, "ic-mode", spapr_get_ic_mode,
>>                              spapr_set_ic_mode, NULL);
>>      object_property_set_description(obj, "ic-mode",
>> -                 "Specifies the interrupt controller mode (xics, xive)",
>> +                 "Specifies the interrupt controller mode (xics, xive, 
>> dual)",
>>                   NULL);
>>  }
>>  
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index ae913d070f50..09386458f267 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1654,6 +1654,17 @@ static target_ulong 
>> h_client_architecture_support(PowerPCCPU *cpu,
>>              (spapr_h_cas_compose_response(spapr, args[1], args[2],
>>                                            ov5_updates) != 0);
>>      }
>> +
>> +    /*
>> +     * Generate a machine reset when we have an update of the
>> +     * interrupt mode. Only required on the machine supporting both
>> +     * mode.
>> +     */
>> +    if (!spapr->cas_reboot) {
>> +        spapr->cas_reboot = spapr_ovec_test(ov5_updates, OV5_XIVE_EXPLOIT)
>> +            && spapr->irq->ov5 & SPAPR_OV5_XIVE_BOTH;
>> +    }
>> +
>>      spapr_ovec_cleanup(ov5_updates);
>>  
>>      if (spapr->cas_reboot) {
>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>> index b1319905327f..31d195b08952 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -375,6 +375,149 @@ sPAPRIrq spapr_irq_xive = {
>>      .reset       = spapr_irq_reset_xive,
>>  };
>>  
>> +/*
>> + * Dual XIVE and XICS IRQ backend.
>> + *
>> + * Both interrupt mode, XIVE and XICS, objects are created but the
>> + * machine starts in legacy interrupt mode (XICS). It can be changed
>> + * by the CAS negotiation process and, in that case, the new mode is
>> + * activated after extra machine reset.
>> + */
>> +
>> +/*
>> + * Returns the sPAPR IRQ backend negotiated by CAS. XICS is the
>> + * default.
>> + */
>> +static sPAPRIrq *spapr_irq_current(sPAPRMachineState *spapr)
>> +{
>> +    return spapr_ovec_test(spapr->ov5_cas, OV5_XIVE_EXPLOIT) ?
>> +        &spapr_irq_xive : &spapr_irq_xics;
>> +}
>> +
>> +static void spapr_irq_init_dual(sPAPRMachineState *spapr, Error **errp)
>> +{
>> +    MachineState *machine = MACHINE(spapr);
>> +    Error *local_err = NULL;
>> +
>> +    if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {
>> +        error_setg(errp, "No KVM support for the 'dual' machine");
>> +        return;
>> +    }
>> +
>> +    spapr_irq_xics.init(spapr, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>> +    spapr_irq_xive.init(spapr, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +}
>> +
>> +static int spapr_irq_claim_dual(sPAPRMachineState *spapr, int irq, bool lsi,
>> +                                Error **errp)
>> +{
>> +    int ret;
>> +    Error *local_err = NULL;
>> +
>> +    ret = spapr_irq_xive.claim(spapr, irq, lsi, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return ret;
>> +    }
>> +
>> +    ret = spapr_irq_xics.claim(spapr, irq, lsi, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static void spapr_irq_free_dual(sPAPRMachineState *spapr, int irq, int num)
>> +{
>> +    spapr_irq_xive.free(spapr, irq, num);
>> +    spapr_irq_xics.free(spapr, irq, num);
>> +}
>> +
>> +static qemu_irq spapr_qirq_dual(sPAPRMachineState *spapr, int irq)
>> +{
>> +    return spapr_irq_current(spapr)->qirq(spapr, irq);
>> +}
> 
> This still makes me really nervous - I'd really prefer to have qirqs
> independent of the backend, rather than relying on *every* irq using
> device never looking up qirqs in advance.

I will take a look. This is a large rework I won't have time to address 
this year. I have removed the dual machine from v9.

You would move the qirq array at the machine level ?  

Thanks,

C.


> 
>> +static void spapr_irq_print_info_dual(sPAPRMachineState *spapr, Monitor 
>> *mon)
>> +{
>> +    spapr_irq_current(spapr)->print_info(spapr, mon);
>> +}
>> +
>> +static void spapr_irq_dt_populate_dual(sPAPRMachineState *spapr,
>> +                                       uint32_t nr_servers, void *fdt,
>> +                                       uint32_t phandle)
>> +{
>> +    spapr_irq_current(spapr)->dt_populate(spapr, nr_servers, fdt, phandle);
>> +}
>> +
>> +static Object *spapr_irq_cpu_intc_create_dual(sPAPRMachineState *spapr,
>> +                                              Object *cpu, Error **errp)
>> +{
>> +    Error *local_err = NULL;
>> +
>> +    spapr_irq_xive.cpu_intc_create(spapr, cpu, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return NULL;
>> +    }
>> +
>> +    /* Default to XICS interrupt mode */
>> +    return spapr_irq_xics.cpu_intc_create(spapr, cpu, errp);
>> +}
>> +
>> +static int spapr_irq_post_load_dual(sPAPRMachineState *spapr, int 
>> version_id)
>> +{
>> +    /*
>> +     * Force a reset of the XIVE backend after migration. The machine
>> +     * defaults to XICS at startup.
>> +     */
>> +    if (spapr_ovec_test(spapr->ov5_cas, OV5_XIVE_EXPLOIT)) {
>> +        spapr_irq_xive.reset(spapr, &error_fatal);
>> +    }
>> +
>> +    return spapr_irq_current(spapr)->post_load(spapr, version_id);
>> +}
>> +
>> +static void spapr_irq_reset_dual(sPAPRMachineState *spapr, Error **errp)
>> +{
>> +    /*
>> +     * Reset the interrupt mode selected by CAS.
>> +     */
>> +    spapr_irq_current(spapr)->reset(spapr, errp);
>> +}
>> +
>> +/*
>> + * Define values in sync with the XIVE and XICS backend
>> + */
>> +#define SPAPR_IRQ_DUAL_NR_IRQS     0x2000
>> +#define SPAPR_IRQ_DUAL_NR_MSIS     (SPAPR_IRQ_DUAL_NR_IRQS - SPAPR_IRQ_MSI)
>> +
>> +sPAPRIrq spapr_irq_dual = {
>> +    .nr_irqs     = SPAPR_IRQ_DUAL_NR_IRQS,
>> +    .nr_msis     = SPAPR_IRQ_DUAL_NR_MSIS,
>> +    .ov5         = SPAPR_OV5_XIVE_BOTH,
>> +
>> +    .init        = spapr_irq_init_dual,
>> +    .claim       = spapr_irq_claim_dual,
>> +    .free        = spapr_irq_free_dual,
>> +    .qirq        = spapr_qirq_dual,
>> +    .print_info  = spapr_irq_print_info_dual,
>> +    .dt_populate = spapr_irq_dt_populate_dual,
>> +    .cpu_intc_create = spapr_irq_cpu_intc_create_dual,
>> +    .post_load   = spapr_irq_post_load_dual,
>> +    .reset       = spapr_irq_reset_dual,
>> +};
>> +
>>  /*
>>   * sPAPR IRQ frontend routines for devices
>>   */
>