Re: [Qemu-ppc] [PATCH v4 03/15] spapr_irq: Set LSIs at interrupt controller init

[Greg Kurz]

On Wed, 13 Feb 2019 14:48:44 +1100
David Gibson <address@hidden> wrote:

> On Tue, Feb 12, 2019 at 07:24:13PM +0100, Greg Kurz wrote:
> > The pseries machine only uses LSIs to support legacy PCI devices. Every
> > PHB claims 4 LSIs at realize time. When using in-kernel XICS (or upcoming
> > in-kernel XIVE), QEMU synchronizes the state of all irqs, including these
> > LSIs, later on at machine reset.
> > 
> > In order to support PHB hotplug, we need a way to tell KVM about the LSIs
> > that doesn't require a machine reset.
> > 
> > Since recent machine types allocate all these LSIs in a fixed range for
> > the machine lifetime, identify them when initializing the interrupt
> > controller, long before they get passed to KVM.
> > 
> > In order to do that, first disintricate interrupt typing and allocation.
> > Since the vast majority of interrupts are MSIs, make that the default
> > and have only the LSI users to explicitely set the type.
> > 
> > It is rather straight forward for XIVE. XICS needs some extra care
> > though: allocation state and type are mixed up in the same bits of the
> > flags field within the interrupt state. Setting the LSI bit there at
> > init time would mean the interrupt is de facto allocated, even if no
> > device asked for it. Introduce a bitmap to track LSIs at the ICS level.
> > In order to keep the patch minimal, the bitmap is only used when writing
> > the source state to KVM and when the interrupt is claimed, so that the
> > code that checks the interrupt type through the flags stays untouched.
> > 
> > With older pseries machine using the XICS legacy IRQ allocation scheme,
> > all interrupt numbers come from a common pool and there's no such thing
> > as a fixed range for LSIs. Introduce an helper so that these older
> > machine types can continue to set the type when allocating the LSI.
> > 
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> >  hw/intc/spapr_xive.c        |    7 +------
> >  hw/intc/xics.c              |   10 ++++++++--
> >  hw/intc/xics_kvm.c          |    2 +-
> >  hw/ppc/pnv_psi.c            |    3 ++-
> >  hw/ppc/spapr_events.c       |    4 ++--
> >  hw/ppc/spapr_irq.c          |   42 
> > ++++++++++++++++++++++++++++++++----------
> >  hw/ppc/spapr_pci.c          |    6 ++++--
> >  hw/ppc/spapr_vio.c          |    2 +-
> >  include/hw/ppc/spapr_irq.h  |    5 +++--
> >  include/hw/ppc/spapr_xive.h |    2 +-
> >  include/hw/ppc/xics.h       |    4 +++-
> >  11 files changed, 58 insertions(+), 29 deletions(-)
> > 
> > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> > index 290a290e43a5..815263ca72ab 100644
> > --- a/hw/intc/spapr_xive.c
> > +++ b/hw/intc/spapr_xive.c
> > @@ -480,18 +480,13 @@ static void spapr_xive_register_types(void)
> >  
> >  type_init(spapr_xive_register_types)
> >  
> > -bool spapr_xive_irq_claim(sPAPRXive *xive, uint32_t lisn, bool lsi)
> > +bool spapr_xive_irq_claim(sPAPRXive *xive, uint32_t lisn)
> >  {
> > -    XiveSource *xsrc = &xive->source;
> > -
> >      if (lisn >= xive->nr_irqs) {
> >          return false;
> >      }
> >  
> >      xive->eat[lisn].w |= cpu_to_be64(EAS_VALID);
> > -    if (lsi) {
> > -        xive_source_irq_set_lsi(xsrc, lisn);
> > -    }
> >      return true;
> >  }
> >  
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index 7cac138067e2..26e8940d7329 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -636,6 +636,7 @@ static void ics_base_realize(DeviceState *dev, Error 
> > **errp)
> >          return;
> >      }
> >      ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> > +    ics->lsi_map = bitmap_new(ics->nr_irqs);
> >  }
> >  
> >  static int ics_base_dispatch_pre_save(void *opaque)
> > @@ -733,12 +734,17 @@ ICPState *xics_icp_get(XICSFabric *xi, int server)
> >      return xic->icp_get(xi, server);
> >  }
> >  
> > -void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
> > +void ics_set_lsi(ICSState *ics, int srcno)
> > +{
> > +    set_bit(srcno, ics->lsi_map);
> > +}
> > +
> > +void ics_claim_irq(ICSState *ics, int srcno)
> >  {
> >      assert(!(ics->irqs[srcno].flags & XICS_FLAGS_IRQ_MASK));
> >  
> >      ics->irqs[srcno].flags |=
> > -        lsi ? XICS_FLAGS_IRQ_LSI : XICS_FLAGS_IRQ_MSI;
> > +        test_bit(srcno, ics->lsi_map) ? XICS_FLAGS_IRQ_LSI : 
> > XICS_FLAGS_IRQ_MSI;  
> 
> I really don't like having the trigger type redundantly stored in the
> lsi_map and then again in the flags fields.
> 
> In a sense the natural way to do this would be more like the hardware
> - have two source objects, one for MSIs and one for LSIs, and make the
> trigger a per ICSState rather than per IRQState.  But that would make
> life hard for the legacy support.
> 
> But... thinking about it, isn't all this overkill anyway.  Can't we
> fix the problem by simply forcing an ics_set_kvm_state() (and the xive
> equivalent) at claim time.  It's not like it's a hot path.
> 

I had kinda followed this approach in earlier versions. I'll try
again.

> >  }
> >  
> >  static void xics_register_types(void)
> > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> > index dff13300504c..e63979abc7fc 100644
> > --- a/hw/intc/xics_kvm.c
> > +++ b/hw/intc/xics_kvm.c
> > @@ -271,7 +271,7 @@ static int ics_set_kvm_state(ICSState *ics, int 
> > version_id)
> >              state |= KVM_XICS_MASKED;
> >          }
> >  
> > -        if (ics->irqs[i].flags & XICS_FLAGS_IRQ_LSI) {
> > +        if (test_bit(i, ics->lsi_map)) {
> >              state |= KVM_XICS_LEVEL_SENSITIVE;
> >              if (irq->status & XICS_STATUS_ASSERTED) {
> >                  state |= KVM_XICS_PENDING;
> > diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> > index 8ced09506321..e6089e1035c0 100644
> > --- a/hw/ppc/pnv_psi.c
> > +++ b/hw/ppc/pnv_psi.c
> > @@ -487,7 +487,8 @@ static void pnv_psi_realize(DeviceState *dev, Error 
> > **errp)
> >      }
> >  
> >      for (i = 0; i < ics->nr_irqs; i++) {
> > -        ics_set_irq_type(ics, i, true);
> > +        ics_set_lsi(ics, i);
> > +        ics_claim_irq(ics, i);
> >      }
> >  
> >      psi->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs);
> > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> > index b9c7ecb9e987..559026d0981c 100644
> > --- a/hw/ppc/spapr_events.c
> > +++ b/hw/ppc/spapr_events.c
> > @@ -713,7 +713,7 @@ void spapr_events_init(sPAPRMachineState *spapr)
> >          epow_irq = spapr_irq_findone(spapr, &error_fatal);
> >      }
> >  
> > -    spapr_irq_claim(spapr, epow_irq, false, &error_fatal);
> > +    spapr_irq_claim(spapr, epow_irq, &error_fatal);
> >  
> >      QTAILQ_INIT(&spapr->pending_events);
> >  
> > @@ -737,7 +737,7 @@ void spapr_events_init(sPAPRMachineState *spapr)
> >              hp_irq = spapr_irq_findone(spapr, &error_fatal);
> >          }
> >  
> > -        spapr_irq_claim(spapr, hp_irq, false, &error_fatal);
> > +        spapr_irq_claim(spapr, hp_irq, &error_fatal);
> >  
> >          spapr_event_sources_register(spapr->event_sources, 
> > EVENT_CLASS_HOT_PLUG,
> >                                       hp_irq);
> > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> > index 8217e0215411..3fc34d7c8a43 100644
> > --- a/hw/ppc/spapr_irq.c
> > +++ b/hw/ppc/spapr_irq.c
> > @@ -16,10 +16,13 @@
> >  #include "hw/ppc/spapr_xive.h"
> >  #include "hw/ppc/xics.h"
> >  #include "hw/ppc/xics_spapr.h"
> > +#include "hw/pci-host/spapr.h"
> >  #include "sysemu/kvm.h"
> >  
> >  #include "trace.h"
> >  
> > +#define SPAPR_IRQ_PCI_LSI_NR     (SPAPR_MAX_PHBS * PCI_NUM_PINS)
> > +
> >  void spapr_irq_msi_init(sPAPRMachineState *spapr, uint32_t nr_msis)
> >  {
> >      spapr->irq_map_nr = nr_msis;
> > @@ -102,6 +105,7 @@ static void spapr_irq_init_xics(sPAPRMachineState 
> > *spapr, Error **errp)
> >      MachineState *machine = MACHINE(spapr);
> >      int nr_irqs = spapr->irq->nr_irqs;
> >      Error *local_err = NULL;
> > +    int i;
> >  
> >      if (kvm_enabled()) {
> >          if (machine_kernel_irqchip_allowed(machine) &&
> > @@ -128,6 +132,14 @@ static void spapr_irq_init_xics(sPAPRMachineState 
> > *spapr, Error **errp)
> >                                        &local_err);
> >      }
> >  
> > +    /* Identify the PCI LSIs */
> > +    if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
> > +        for (i = 0; i < SPAPR_IRQ_PCI_LSI_NR; ++i) {
> > +            ics_set_lsi(spapr->ics,
> > +                        i + SPAPR_IRQ_PCI_LSI - spapr->irq->xics_offset);
> > +        }
> > +    }
> > +
> >  error:
> >      error_propagate(errp, local_err);
> >  }
> > @@ -135,7 +147,7 @@ error:
> >  #define ICS_IRQ_FREE(ics, srcno)   \
> >      (!((ics)->irqs[(srcno)].flags & (XICS_FLAGS_IRQ_MASK)))
> >  
> > -static int spapr_irq_claim_xics(sPAPRMachineState *spapr, int irq, bool 
> > lsi,
> > +static int spapr_irq_claim_xics(sPAPRMachineState *spapr, int irq,
> >                                  Error **errp)
> >  {
> >      ICSState *ics = spapr->ics;
> > @@ -152,7 +164,7 @@ static int spapr_irq_claim_xics(sPAPRMachineState 
> > *spapr, int irq, bool lsi,
> >          return -1;
> >      }
> >  
> > -    ics_set_irq_type(ics, irq - ics->offset, lsi);
> > +    ics_claim_irq(ics, irq - ics->offset);
> >      return 0;
> >  }
> >  
> > @@ -296,16 +308,21 @@ static void spapr_irq_init_xive(sPAPRMachineState 
> > *spapr, Error **errp)
> >  
> >      /* Enable the CPU IPIs */
> >      for (i = 0; i < nr_servers; ++i) {
> > -        spapr_xive_irq_claim(spapr->xive, SPAPR_IRQ_IPI + i, false);
> > +        spapr_xive_irq_claim(spapr->xive, SPAPR_IRQ_IPI + i);
> > +    }
> > +
> > +    /* Identify the PCI LSIs */
> > +    for (i = 0; i < SPAPR_IRQ_PCI_LSI_NR; ++i) {
> > +        xive_source_irq_set_lsi(&spapr->xive->source, SPAPR_IRQ_PCI_LSI + 
> > i);
> >      }
> >  
> >      spapr_xive_hcall_init(spapr);
> >  }
> >  
> > -static int spapr_irq_claim_xive(sPAPRMachineState *spapr, int irq, bool 
> > lsi,
> > +static int spapr_irq_claim_xive(sPAPRMachineState *spapr, int irq,
> >                                  Error **errp)
> >  {
> > -    if (!spapr_xive_irq_claim(spapr->xive, irq, lsi)) {
> > +    if (!spapr_xive_irq_claim(spapr->xive, irq)) {
> >          error_setg(errp, "IRQ %d is invalid", irq);
> >          return -1;
> >      }
> > @@ -465,19 +482,19 @@ static void spapr_irq_init_dual(sPAPRMachineState 
> > *spapr, Error **errp)
> >      }
> >  }
> >  
> > -static int spapr_irq_claim_dual(sPAPRMachineState *spapr, int irq, bool 
> > lsi,
> > +static int spapr_irq_claim_dual(sPAPRMachineState *spapr, int irq,
> >                                  Error **errp)
> >  {
> >      Error *local_err = NULL;
> >      int ret;
> >  
> > -    ret = spapr_irq_xics.claim(spapr, irq, lsi, &local_err);
> > +    ret = spapr_irq_xics.claim(spapr, irq, &local_err);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> >          return ret;
> >      }
> >  
> > -    ret = spapr_irq_xive.claim(spapr, irq, lsi, &local_err);
> > +    ret = spapr_irq_xive.claim(spapr, irq, &local_err);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> >          return ret;
> > @@ -630,9 +647,9 @@ void spapr_irq_init(sPAPRMachineState *spapr, Error 
> > **errp)
> >                                        spapr->irq->nr_irqs);
> >  }
> >  
> > -int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error 
> > **errp)
> > +int spapr_irq_claim(sPAPRMachineState *spapr, int irq, Error **errp)
> >  {
> > -    return spapr->irq->claim(spapr, irq, lsi, errp);
> > +    return spapr->irq->claim(spapr, irq, errp);
> >  }
> >  
> >  void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num)
> > @@ -712,6 +729,11 @@ int spapr_irq_find(sPAPRMachineState *spapr, int num, 
> > bool align, Error **errp)
> >      return first + ics->offset;
> >  }
> >  
> > +void spapr_irq_set_lsi_legacy(sPAPRMachineState *spapr, int irq)
> > +{
> > +    ics_set_lsi(spapr->ics, irq - spapr->irq->xics_offset);
> > +}
> > +
> >  #define SPAPR_IRQ_XICS_LEGACY_NR_IRQS     0x400
> >  
> >  sPAPRIrq spapr_irq_xics_legacy = {
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index c3fb0ac884b0..d68595531d5a 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -391,7 +391,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, 
> > sPAPRMachineState *spapr,
> >      }
> >  
> >      for (i = 0; i < req_num; i++) {
> > -        spapr_irq_claim(spapr, irq + i, false, &err);
> > +        spapr_irq_claim(spapr, irq + i, &err);
> >          if (err) {
> >              if (i) {
> >                  spapr_irq_free(spapr, irq, i);
> > @@ -1742,9 +1742,11 @@ static void spapr_phb_realize(DeviceState *dev, 
> > Error **errp)
> >                                          "can't allocate LSIs: ");
> >                  return;
> >              }
> > +
> > +            spapr_irq_set_lsi_legacy(spapr, irq);
> >          }
> >  
> > -        spapr_irq_claim(spapr, irq, true, &local_err);
> > +        spapr_irq_claim(spapr, irq, &local_err);
> >          if (local_err) {
> >              error_propagate_prepend(errp, local_err, "can't allocate LSIs: 
> > ");
> >              return;
> > diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> > index 2b7e7ecac57f..b1beefc24be5 100644
> > --- a/hw/ppc/spapr_vio.c
> > +++ b/hw/ppc/spapr_vio.c
> > @@ -512,7 +512,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, 
> > Error **errp)
> >          }
> >      }
> >  
> > -    spapr_irq_claim(spapr, dev->irq, false, &local_err);
> > +    spapr_irq_claim(spapr, dev->irq, &local_err);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> >          return;
> > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> > index 5e30858dc22a..0e6c65d55430 100644
> > --- a/include/hw/ppc/spapr_irq.h
> > +++ b/include/hw/ppc/spapr_irq.h
> > @@ -37,7 +37,7 @@ typedef struct sPAPRIrq {
> >      uint32_t    xics_offset;
> >  
> >      void (*init)(sPAPRMachineState *spapr, Error **errp);
> > -    int (*claim)(sPAPRMachineState *spapr, int irq, bool lsi, Error 
> > **errp);
> > +    int (*claim)(sPAPRMachineState *spapr, int irq, Error **errp);
> >      void (*free)(sPAPRMachineState *spapr, int irq, int num);
> >      qemu_irq (*qirq)(sPAPRMachineState *spapr, int irq);
> >      void (*print_info)(sPAPRMachineState *spapr, Monitor *mon);
> > @@ -56,7 +56,7 @@ 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);
> > +int spapr_irq_claim(sPAPRMachineState *spapr, int irq, Error **errp);
> >  void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
> >  qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
> >  int spapr_irq_post_load(sPAPRMachineState *spapr, int version_id);
> > @@ -67,5 +67,6 @@ void spapr_irq_reset(sPAPRMachineState *spapr, Error 
> > **errp);
> >   */
> >  int spapr_irq_find(sPAPRMachineState *spapr, int num, bool align, Error 
> > **errp);
> >  #define spapr_irq_findone(spapr, errp) spapr_irq_find(spapr, 1, false, 
> > errp)
> > +void spapr_irq_set_lsi_legacy(sPAPRMachineState *spapr, int irq);
> >  
> >  #endif
> > diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> > index 9bec9192e4a0..885ca169cb29 100644
> > --- a/include/hw/ppc/spapr_xive.h
> > +++ b/include/hw/ppc/spapr_xive.h
> > @@ -37,7 +37,7 @@ typedef struct sPAPRXive {
> >      MemoryRegion  tm_mmio;
> >  } sPAPRXive;
> >  
> > -bool spapr_xive_irq_claim(sPAPRXive *xive, uint32_t lisn, bool lsi);
> > +bool spapr_xive_irq_claim(sPAPRXive *xive, uint32_t lisn);
> >  bool spapr_xive_irq_free(sPAPRXive *xive, uint32_t lisn);
> >  void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
> >  
> > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> > index fad786e8b22d..18b083fe2aec 100644
> > --- a/include/hw/ppc/xics.h
> > +++ b/include/hw/ppc/xics.h
> > @@ -133,6 +133,7 @@ struct ICSState {
> >      uint32_t offset;
> >      ICSIRQState *irqs;
> >      XICSFabric *xics;
> > +    unsigned long *lsi_map;
> >  };
> >  
> >  #define ICS_PROP_XICS "xics"
> > @@ -193,7 +194,8 @@ void ics_simple_write_xive(ICSState *ics, int nr, int 
> > server,
> >  void ics_simple_set_irq(void *opaque, int srcno, int val);
> >  void ics_kvm_set_irq(void *opaque, int srcno, int val);
> >  
> > -void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);
> > +void ics_set_lsi(ICSState *ics, int srcno);
> > +void ics_claim_irq(ICSState *ics, int srcno);
> >  void icp_pic_print_info(ICPState *icp, Monitor *mon);
> >  void ics_pic_print_info(ICSState *ics, Monitor *mon);
> >  
> >   
>

pgp2qwjjo_gET.pgp
Description: OpenPGP digital signature