Re: [Qemu-devel] [PATCH v7 16/19] spapr: introduce a new sPAPR IRQ backend supporting XIVE and XICS

[David Gibson]

On Tue, Dec 11, 2018 at 11:19:01AM +0100, Cédric Le Goater wrote:
> On 12/11/18 3:03 AM, David Gibson wrote:
> > On Sun, Dec 09, 2018 at 08:46:07PM +0100, Cédric Le Goater wrote:
> >> The interrupt mode is chosen by the CAS negotiation process and
> >> 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.
> >>
> >> This default interrupt mode for the machine is XICS.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >>  include/hw/ppc/spapr_irq.h |   1 +
> >>  hw/ppc/spapr.c             |   3 +-
> >>  hw/ppc/spapr_hcall.c       |  13 ++++
> >>  hw/ppc/spapr_irq.c         | 143 +++++++++++++++++++++++++++++++++++++
> >>  4 files changed, 159 insertions(+), 1 deletion(-)
> >>
> >> 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 5ef87a00f68b..fa41927d95dd 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -2631,7 +2631,8 @@ static void spapr_machine_init(MachineState *machine)
> >>      spapr_ovec_set(spapr->ov5, OV5_DRMEM_V2);
> >>  
> >>      /* advertise XIVE */
> >> -    if (smc->irq->ov5 == SPAPR_OV5_XIVE_EXPLOIT) {
> >> +    if (smc->irq->ov5 == SPAPR_OV5_XIVE_EXPLOIT ||
> >> +        smc->irq->ov5 == SPAPR_OV5_XIVE_BOTH) {
> >>          spapr_ovec_set(spapr->ov5, OV5_XIVE_EXPLOIT);
> >>      }
> >>  
> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> >> index ae913d070f50..186b6a65543f 100644
> >> --- a/hw/ppc/spapr_hcall.c
> >> +++ b/hw/ppc/spapr_hcall.c
> >> @@ -1654,6 +1654,19 @@ 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) {
> >> +        sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> >> +
> >> +        spapr->cas_reboot = spapr_ovec_test(ov5_updates, OV5_XIVE_EXPLOIT)
> >> +            && smc->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 a8e50725397c..7c34939f774a 100644
> >> --- a/hw/ppc/spapr_irq.c
> >> +++ b/hw/ppc/spapr_irq.c
> >> @@ -392,6 +392,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);
> > 
> > Urgh... I don't think this is going to work.  IIRC the various devices
> > (PHB, VIO, etc.)  are wired up to their qirqs at realize() time, so if
> > you reboot from a XIVE guest to XICS guest (or maybe the other way
> > around) the peripherals won't be able to signal irqs in the new
> > scheme.
> 
> It does. The IRQ numbers are claimed in both backends.

Yes, I realize that, but the two backends still have their own set of
qirqs, which have their own set_irq routines associated with them.

> This is the problem since the very beginning. For reset and migration
> to work, we need to keep in sync the IRQ number space of the machine 
> and the different interrupt controllers.

Sure, we have the numbers in sync, but that won't help if when the
peripherals do a qemu_irq_pulse() it goes to the wrong backend's
trigger routine.


> 
> C. 
> 
> 
> > I think instead we need a common set of qirqs, whose set_irq routine
> > looks at whether to signal XICS or XIVE.  FOr now I think the easiest
> > approach is to layer those on top of the existing XICS or XIVE
> > specific qirqs.  Later we might want to remove the (input) qirqs
> > entirely from the XICS and XIVE subsystems, instead having just
> > explicit trigger functions.  Then spapr will always supply the qirqs
> > which call into one or the other.
> > 
> >> +}
> >> +
> >> +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
> >>   */
> > 
> 

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