Re: [Qemu-ppc] [PATCH 09/10] spapr: introduce a new sPAPR IRQ backend supporting XIVE and XICS

[Cédric Le Goater]

On 1/3/19 5:35 AM, David Gibson wrote:
> On Wed, Jan 02, 2019 at 06:57:42AM +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>
>> ---
>>  include/hw/ppc/spapr_irq.h |   1 +
>>  hw/ppc/spapr.c             |  10 ++-
>>  hw/ppc/spapr_hcall.c       |  11 +++
>>  hw/ppc/spapr_irq.c         | 179 +++++++++++++++++++++++++++++++++++++
>>  4 files changed, 198 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>> index 283bb5002c16..14b02c3aca33 100644
>> --- a/include/hw/ppc/spapr_irq.h
>> +++ b/include/hw/ppc/spapr_irq.h
>> @@ -52,6 +52,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 5e8ffda47372..eb8ef741d860 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -2633,11 +2633,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);
>>          }
>> @@ -3063,6 +3063,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();
>>  }
>> @@ -3076,6 +3078,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");
>>      }
>> @@ -3124,7 +3128,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..45c35d41fac6 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 when the machine supports both
>> +     * modes.
>> +     */
>> +    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 d23914887ac0..d110b8cdeec7 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -230,6 +230,11 @@ static void spapr_irq_set_irq_xics(void *opaque, int 
>> srcno, int val)
>>      }
>>  }
>>  
>> +static void spapr_irq_reset_xics(sPAPRMachineState *spapr, Error **errp)
>> +{
>> +    /* TODO: create the KVM XICS device */
>> +}
>> +
>>  #define SPAPR_IRQ_XICS_NR_IRQS     0x1000
>>  #define SPAPR_IRQ_XICS_NR_MSIS     \
>>      (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)
>> @@ -247,6 +252,7 @@ sPAPRIrq spapr_irq_xics = {
>>      .dt_populate = spapr_dt_xics,
>>      .cpu_intc_create = spapr_irq_cpu_intc_create_xics,
>>      .post_load   = spapr_irq_post_load_xics,
>> +    .reset       = spapr_irq_reset_xics,
>>      .set_irq     = spapr_irq_set_irq_xics,
>>  };
>>  
>> @@ -403,6 +409,179 @@ sPAPRIrq spapr_irq_xive = {
>>      .set_irq     = spapr_irq_set_irq_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 an 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;
>> +    }
>> +
>> +    /*
>> +     * Align the XICS and the XIVE IRQ number space under QEMU.
>> +     *
>> +     * However, the XICS KVM device still considers that the IRQ
>> +     * numbers should start at XICS_IRQ_BASE (0x1000). Either we
>> +     * should introduce a KVM device ioctl to set the offset or ignore
>> +     * the lower 4K numbers when using the get/set ioctl of the XICS
>> +     * KVM device. The second option seems the least intrusive.
>> +     */
>> +    spapr->ics->offset = 0;
>> +
>> +    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)
>> +{
>> +    Error *local_err = NULL;
>> +    int ret;
>> +
>> +    ret = spapr_irq_xics.claim(spapr, irq, lsi, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return ret;
>> +    }
>> +
>> +    ret = spapr_irq_xive.claim(spapr, irq, lsi, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return ret;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static void spapr_irq_free_dual(sPAPRMachineState *spapr, int irq, int num)
>> +{
>> +    spapr_irq_xics.free(spapr, irq, num);
>> +    spapr_irq_xive.free(spapr, irq, num);
>> +}
>> +
>> +static qemu_irq spapr_qirq_dual(sPAPRMachineState *spapr, int irq)
> 
> If the qirq array is now at the machine level, you shouldn't need an
> irq backend version of the qirq function should you?  Just a backend
> specific version of set_irq.

The qirq function of the 'xics' backend takes into account the IRQ number 
offset which is different (0x1000 vs. 0x0) and the qirq function of the 
'dual' backend adds some extra checks on the IRQ number which should have 
been claimed by both XICS and XIVE interrupt modes. This check might be a 
little over kill. 

We could improve things if we could find a way to get rid of the ICSState 
offset which is spread all over the ics_* routine. I haven't found an 
obvious way to do so.

C.  

> 
>> +{
>> +    sPAPRXive *xive = spapr->xive;
>> +    ICSState *ics = spapr->ics;
>> +
>> +    if (irq >= spapr->irq->nr_irqs) {
>> +        return NULL;
>> +    }
>> +
>> +    /*
>> +     * The IRQ number should have been claimed under both interrupt
>> +     * controllers.
>> +     */
>> +    assert(!ICS_IRQ_FREE(ics, irq - ics->offset));
>> +    assert(xive_eas_is_valid(&xive->eat[irq]));
>> +
>> +    return spapr->qirqs[irq];
>> +}
>> +
>> +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 void spapr_irq_cpu_intc_create_dual(sPAPRMachineState *spapr,
>> +                                           PowerPCCPU *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;
>> +    }
>> +
>> +    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)
>> +{
>> +    spapr_irq_current(spapr)->reset(spapr, errp);
>> +}
>> +
>> +static void spapr_irq_set_irq_dual(void *opaque, int srcno, int val)
>> +{
>> +    sPAPRMachineState *spapr = opaque;
>> +
>> +    spapr_irq_current(spapr)->set_irq(spapr, srcno, val);
>> +}
>> +
>> +/*
>> + * 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,
>> +    .set_irq     = spapr_irq_set_irq_dual
>> +};
>> +
>>  /*
>>   * sPAPR IRQ frontend routines for devices
>>   */
>