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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 16213 bytes --]

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 <groug@kaod.org>
> ---
>  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.

>  }
>  
>  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);
>  
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]