Re: [Qemu-ppc] [PATCH v2 3/3] spapr: introduce a fixed IRQ number space

[David Gibson]

On Tue, Jun 19, 2018 at 07:00:18AM +0200, Cédric Le Goater wrote:
> On 06/19/2018 03:02 AM, David Gibson wrote:
> > On Mon, Jun 18, 2018 at 07:34:02PM +0200, Cédric Le Goater wrote:
> >> This proposal introduces a new IRQ number space layout using static
> >> numbers for all devices and a bitmap allocator for the MSI numbers
> >> which are negotiated by the guest at runtime.
> >>
> >> The previous layout is kept in machines raising the 'xics_legacy'
> >> flag.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >>  include/hw/ppc/spapr.h     |  4 ++++
> >>  include/hw/ppc/spapr_irq.h | 30 +++++++++++++++++++++++++
> >>  hw/ppc/spapr.c             | 31 +++++++++++++++++++++++++
> >>  hw/ppc/spapr_events.c      | 12 ++++++++--
> >>  hw/ppc/spapr_irq.c         | 56 
> >> ++++++++++++++++++++++++++++++++++++++++++++++
> >>  hw/ppc/spapr_pci.c         | 28 ++++++++++++++++++-----
> >>  hw/ppc/spapr_vio.c         | 19 ++++++++++++----
> >>  hw/ppc/Makefile.objs       |  2 +-
> >>  8 files changed, 169 insertions(+), 13 deletions(-)
> >>  create mode 100644 include/hw/ppc/spapr_irq.h
> >>  create mode 100644 hw/ppc/spapr_irq.c
> >>
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index 9decc66a1915..4c63b1fac13b 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -7,6 +7,7 @@
> >>  #include "hw/ppc/spapr_drc.h"
> >>  #include "hw/mem/pc-dimm.h"
> >>  #include "hw/ppc/spapr_ovec.h"
> >> +#include "hw/ppc/spapr_irq.h"
> >>  
> >>  struct VIOsPAPRBus;
> >>  struct sPAPRPHBState;
> >> @@ -164,6 +165,9 @@ struct sPAPRMachineState {
> >>      char *kvm_type;
> >>  
> >>      const char *icp_type;
> >> +    bool xics_legacy;
> > 
> > This flag can go in the class, rather than the instance.
> > 
> > And maybe call it 'legacy_irq_allocation'.  It assumes XICS, but
> > otherwise isn't strongly tied to it.
> 
> OK.
> 
> >> +    int32_t irq_map_nr;
> >> +    unsigned long *irq_map;
> > 
> > So, I don't love the fact that the new bitmap duplicates information
> > that's also in the intc backend (e.g. via ICS_IRQ_FREE()).  
> 
> Yes. I agree. new devices using MSI like interrupts will follow the
> same pattern for allocation. 
> 
> we have two layers of IRQ routines, one for the IRQ numbers and one 
> for the controller backend. May be we could call the backend handling 
> routing from the msi one ? 
> 
> > However
> > leaving the authoritative info in the backend also causes problems
> > when we have dynamic switching.  Not entirely sure what to do about
> > that.
> 
> yes, if we put it in the IRQ backend (the current IRQ controller model
> in use) we will have to synchronize the number spaces when the machine 
> switches interrupt mode. 
>  
> >>      bool cmd_line_caps[SPAPR_CAP_NUM];
> >>      sPAPRCapabilities def, eff, mig;
> >> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> >> new file mode 100644
> >> index 000000000000..345a42efd366
> >> --- /dev/null
> >> +++ b/include/hw/ppc/spapr_irq.h
> >> @@ -0,0 +1,30 @@
> >> +/*
> >> + * QEMU PowerPC sPAPR IRQ backend definitions
> >> + *
> >> + * Copyright (c) 2018, IBM Corporation.
> >> + *
> >> + * This code is licensed under the GPL version 2 or later. See the
> >> + * COPYING file in the top-level directory.
> >> + */
> >> +
> >> +#ifndef HW_SPAPR_IRQ_H
> >> +#define HW_SPAPR_IRQ_H
> >> +
> >> +/*
> >> + * IRQ range offsets per device type
> >> + */
> >> +#define SPAPR_IRQ_EPOW       0x1000  /* XICS_IRQ_BASE offset */
> >> +#define SPAPR_IRQ_HOTPLUG    0x1001
> >> +#define SPAPR_IRQ_VIO        0x1100  /* 256 VIO devices */
> >> +#define SPAPR_IRQ_PCI_LSI    0x1200  /* 32+ PHBs devices */
> >> +
> >> +#define SPAPR_IRQ_MSI        0x1300  /* Offset of the dynamic range 
> >> covered
> >> +                                      * by the bitmap allocator */
> > 
> > I'm a little confused by the MSI stuff.  It looks like you're going
> > for the option of one big pool for all dynamic irqs.  Except that I
> > thought in our discussion the other day you said each PHB advertised
> > its own separate MSI range, so we'd actually need to split this up
> > into ranges for each PHB.
> 
> Yes we can also, but we don't really need to and it might be too much
> constrained in fact.

Ok.

> As the IRQs are allocated dynamically, there is not a strong relation 
> between the device doing so and the IRQ numbers. The need for a well
> defined IRQ number range is weak. We should provision a certain number 
> of IRQs of course to size our IRQ number space but even that could be 
> done dynamically. We can resize the bitmap and allocate new source 
> blocks under the KVM XICS/XIVE device if needed. The resulting code 
> is quite simple and the IRQ number space is also less fragmented. 
> 
> I think we have all the requirements in hand, the current ones and the 
> new ones for hotplug PHBs, XIVE interrupt model, CAPI (which should be
> like the PHBs), XIVE user IRQs (like MSIs). The new ones are all 
> dynamic IRQ models.
> 
> 
> So I am more in favor of a single big bunch of dynamic IRQs.

Ok, sounds reasonable.

[snip]
> >> @@ -4093,7 +4121,10 @@ DEFINE_SPAPR_MACHINE(3_0, "3.0", true);
> >>  
> >>  static void spapr_machine_2_12_instance_options(MachineState *machine)
> >>  {
> >> +    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> >> +
> >>      spapr_machine_3_0_instance_options(machine);
> >> +    spapr->xics_legacy = true;
> >>  }
> >>  
> >>  static void spapr_machine_2_12_class_options(MachineClass *mc)
> >> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> >> index e4f5946a2188..c82dc40be0d5 100644
> >> --- a/hw/ppc/spapr_events.c
> >> +++ b/hw/ppc/spapr_events.c
> >> @@ -709,7 +709,11 @@ void spapr_events_init(sPAPRMachineState *spapr)
> >>  {
> >>      int epow_irq;
> >>  
> >> -    epow_irq = spapr_irq_findone(spapr, &error_fatal);
> >> +    if (spapr->xics_legacy) {
> >> +        epow_irq = spapr_irq_findone(spapr, &error_fatal);
> >> +    } else {
> >> +        epow_irq = SPAPR_IRQ_EPOW;
> > 
> > Can slightly improve brevity by just initializing epow_irq to this,
> > then overwriting it in the legacy case.
> 
> yes. I will do that. It will ease removal because we want to deprecate
> the legacy mode one day. 
> 
> That might in a long time though. We have to wait for the QEMU machine 
> number to no longer be in a maintained distro. correct?

Pretty much, yes.  And, yes, that will be a long ways off.


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