Re: [PATCH 09/20] spapr: Clarify and fix handling of nr_irqs

[Cédric Le Goater]

On 26/09/2019 03:03, David Gibson wrote:
> On Wed, Sep 25, 2019 at 09:05:34AM +0200, Cédric Le Goater wrote:
>> On 25/09/2019 08:45, David Gibson wrote:
>>> Both the XICS and XIVE interrupt backends have a "nr-irqs" property, but
>>> it means slightly different things.  For XICS (or, strictly, the ICS) it
>>> indicates the number of "real" external IRQs.  Those start at XICS_IRQ_BASE
>>> (0x1000) and don't include the special IPI vector.  For XIVE, however, it
>>> includes the whole IRQ space, including XIVE's many IPI vectors.
>>>
>>> The spapr code currently doesn't handle this sensibly, with the nr_irqs
>>> value in SpaprIrq having different meanings depending on the backend.
>>> We fix this by renaming nr_irqs to nr_xirqs and making it always indicate
>>> just the number of external irqs, adjusting the value we pass to XIVE
>>> accordingly.  We also use move to using common constants in most of the
>>> irq configurations, to make it clearer that the IRQ space looks the same
>>> to the guest (and emulated devices), even if the backend is different.
>>>
>>> Signed-off-by: David Gibson <address@hidden>
>>
>> Reviewed-by: Cédric Le Goater <address@hidden>
>>
>> one comment below,
>>
>>> ---
>>>  hw/ppc/spapr_irq.c         | 48 +++++++++++++++-----------------------
>>>  include/hw/ppc/spapr_irq.h | 19 +++++++++------
>>>  2 files changed, 31 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>>> index 8c26fa2d1e..5190a33e08 100644
>>> --- a/hw/ppc/spapr_irq.c
>>> +++ b/hw/ppc/spapr_irq.c
>>> @@ -92,7 +92,7 @@ static void spapr_irq_init_kvm(SpaprMachineState *spapr,
>>>   * XICS IRQ backend.
>>>   */
>>>  
>>> -static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs,
>>> +static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_xirqs,
>>>                                  Error **errp)
>>>  {
>>>      Object *obj;
>>> @@ -102,7 +102,7 @@ static void spapr_irq_init_xics(SpaprMachineState 
>>> *spapr, int nr_irqs,
>>>      object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
>>>      object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
>>>                                     &error_fatal);
>>> -    object_property_set_int(obj, nr_irqs, "nr-irqs",  &error_fatal);
>>> +    object_property_set_int(obj, nr_xirqs, "nr-irqs",  &error_fatal);
>>>      object_property_set_bool(obj, true, "realized", &local_err);
>>>      if (local_err) {
>>>          error_propagate(errp, local_err);
>>> @@ -234,13 +234,9 @@ static void spapr_irq_init_kvm_xics(SpaprMachineState 
>>> *spapr, Error **errp)
>>>      }
>>>  }
>>>  
>>> -#define SPAPR_IRQ_XICS_NR_IRQS     0x1000
>>> -#define SPAPR_IRQ_XICS_NR_MSIS     \
>>> -    (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)
>>> -
>>>  SpaprIrq spapr_irq_xics = {
>>> -    .nr_irqs     = SPAPR_IRQ_XICS_NR_IRQS,
>>> -    .nr_msis     = SPAPR_IRQ_XICS_NR_MSIS,
>>> +    .nr_xirqs    = SPAPR_NR_XIRQS,
>>> +    .nr_msis     = SPAPR_NR_MSIS,
>>>      .ov5         = SPAPR_OV5_XIVE_LEGACY,
>>>  
>>>      .init        = spapr_irq_init_xics,
>>> @@ -260,7 +256,7 @@ SpaprIrq spapr_irq_xics = {
>>>  /*
>>>   * XIVE IRQ backend.
>>>   */
>>> -static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_irqs,
>>> +static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_xirqs,
>>>                                  Error **errp)
>>>  {
>>>      uint32_t nr_servers = spapr_max_server_number(spapr);
>>> @@ -268,7 +264,7 @@ static void spapr_irq_init_xive(SpaprMachineState 
>>> *spapr, int nr_irqs,
>>>      int i;
>>>  
>>>      dev = qdev_create(NULL, TYPE_SPAPR_XIVE);
>>> -    qdev_prop_set_uint32(dev, "nr-irqs", nr_irqs);
>>> +    qdev_prop_set_uint32(dev, "nr-irqs", nr_xirqs + SPAPR_XIRQ_BASE);
>>>      /*
>>>       * 8 XIVE END structures per CPU. One for each available priority
>>>       */
>>> @@ -308,7 +304,7 @@ static qemu_irq spapr_qirq_xive(SpaprMachineState 
>>> *spapr, int irq)
>>>  {
>>>      SpaprXive *xive = spapr->xive;
>>>  
>>> -    if (irq >= xive->nr_irqs) {
>>> +    if ((irq < SPAPR_XIRQ_BASE) || (irq >= xive->nr_irqs)) {
>>
>> So IPIs cannot be pulsed ? I think that is OK in QEMU.
> 
> They can be pulsed, they just can't be retrieved via the spapr_qirq()
> interface.  Since that interface basically exists for the spapr root
> devices (VIO and PHBs) to find the qemu_irqs to wire themselves up to,
> I think that's fine.
> 
> If we discover some reason we need to grab IPI qirqs by global number
> then we can revisit this.

Not from QEMU I think. This is fine.

C. 

> 
> I'll add a comment to clarify this in the later patch where I unify
> the qirq implementations.
> 
>> XIVE unifies all the interrupts at the controller level. Any one can trigger 
>> an interrupt with a store on the associate ESB page.
> 
> Absolutely, and nothing's stopping them.
>>
>>>          return NULL;
>>>      }
>>>  
>>> @@ -409,12 +405,9 @@ static void spapr_irq_init_kvm_xive(SpaprMachineState 
>>> *spapr, Error **errp)
>>>   * with XICS.
>>>   */
>>>  
>>> -#define SPAPR_IRQ_XIVE_NR_IRQS     0x2000
>>> -#define SPAPR_IRQ_XIVE_NR_MSIS     (SPAPR_IRQ_XIVE_NR_IRQS - SPAPR_IRQ_MSI)
>>> -
>>>  SpaprIrq spapr_irq_xive = {
>>> -    .nr_irqs     = SPAPR_IRQ_XIVE_NR_IRQS,
>>> -    .nr_msis     = SPAPR_IRQ_XIVE_NR_MSIS,
>>> +    .nr_xirqs    = SPAPR_NR_XIRQS,
>>> +    .nr_msis     = SPAPR_NR_MSIS,
>>>      .ov5         = SPAPR_OV5_XIVE_EXPLOIT,
>>>  
>>>      .init        = spapr_irq_init_xive,
>>> @@ -450,18 +443,18 @@ static SpaprIrq *spapr_irq_current(SpaprMachineState 
>>> *spapr)
>>>          &spapr_irq_xive : &spapr_irq_xics;
>>>  }
>>>  
>>> -static void spapr_irq_init_dual(SpaprMachineState *spapr, int nr_irqs,
>>> +static void spapr_irq_init_dual(SpaprMachineState *spapr, int nr_xirqs,
>>>                                  Error **errp)
>>>  {
>>>      Error *local_err = NULL;
>>>  
>>> -    spapr_irq_xics.init(spapr, spapr_irq_xics.nr_irqs, &local_err);
>>> +    spapr_irq_xics.init(spapr, spapr_irq_xics.nr_xirqs, &local_err);
>>>      if (local_err) {
>>>          error_propagate(errp, local_err);
>>>          return;
>>>      }
>>>  
>>> -    spapr_irq_xive.init(spapr, spapr_irq_xive.nr_irqs, &local_err);
>>> +    spapr_irq_xive.init(spapr, spapr_irq_xive.nr_xirqs, &local_err);
>>>      if (local_err) {
>>>          error_propagate(errp, local_err);
>>>          return;
>>> @@ -586,12 +579,9 @@ static const char 
>>> *spapr_irq_get_nodename_dual(SpaprMachineState *spapr)
>>>  /*
>>>   * 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,
>>> +    .nr_xirqs    = SPAPR_NR_XIRQS,
>>> +    .nr_msis     = SPAPR_NR_MSIS,
>>>      .ov5         = SPAPR_OV5_XIVE_BOTH,
>>>  
>>>      .init        = spapr_irq_init_dual,
>>> @@ -693,10 +683,10 @@ void spapr_irq_init(SpaprMachineState *spapr, Error 
>>> **errp)
>>>          spapr_irq_msi_init(spapr, spapr->irq->nr_msis);
>>>      }
>>>  
>>> -    spapr->irq->init(spapr, spapr->irq->nr_irqs, errp);
>>> +    spapr->irq->init(spapr, spapr->irq->nr_xirqs, errp);
>>>  
>>>      spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr,
>>> -                                      spapr->irq->nr_irqs);
>>> +                                      spapr->irq->nr_xirqs + 
>>> SPAPR_XIRQ_BASE);
>>>  }
>>>  
>>>  int spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error 
>>> **errp)
>>> @@ -804,11 +794,11 @@ int spapr_irq_find(SpaprMachineState *spapr, int num, 
>>> bool align, Error **errp)
>>>      return first + ics->offset;
>>>  }
>>>  
>>> -#define SPAPR_IRQ_XICS_LEGACY_NR_IRQS     0x400
>>> +#define SPAPR_IRQ_XICS_LEGACY_NR_XIRQS     0x400
>>>  
>>>  SpaprIrq spapr_irq_xics_legacy = {
>>> -    .nr_irqs     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
>>> -    .nr_msis     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
>>> +    .nr_xirqs    = SPAPR_IRQ_XICS_LEGACY_NR_XIRQS,
>>> +    .nr_msis     = SPAPR_IRQ_XICS_LEGACY_NR_XIRQS,
>>>      .ov5         = SPAPR_OV5_XIVE_LEGACY,
>>>  
>>>      .init        = spapr_irq_init_xics,
>>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>>> index 5db305165c..a8f9a2ab11 100644
>>> --- a/include/hw/ppc/spapr_irq.h
>>> +++ b/include/hw/ppc/spapr_irq.h
>>> @@ -16,13 +16,18 @@
>>>   * IRQ range offsets per device type
>>>   */
>>>  #define SPAPR_IRQ_IPI        0x0
>>> -#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 */
>>> +#define SPAPR_XIRQ_BASE      XICS_IRQ_BASE /* 0x1000 */
>>> +#define SPAPR_IRQ_EPOW       (SPAPR_XIRQ_BASE + 0x0000)
>>> +#define SPAPR_IRQ_HOTPLUG    (SPAPR_XIRQ_BASE + 0x0001)
>>> +#define SPAPR_IRQ_VIO        (SPAPR_XIRQ_BASE + 0x0100)  /* 256 VIO 
>>> devices */
>>> +#define SPAPR_IRQ_PCI_LSI    (SPAPR_XIRQ_BASE + 0x0200)  /* 32+ PHBs 
>>> devices */
>>> +
>>> +/* Offset of the dynamic range covered by the bitmap allocator */
>>> +#define SPAPR_IRQ_MSI        (SPAPR_XIRQ_BASE + 0x0300)
>>> +
>>> +#define SPAPR_NR_XIRQS       0x1000
>>> +#define SPAPR_NR_MSIS        (SPAPR_XIRQ_BASE + SPAPR_NR_XIRQS - 
>>> SPAPR_IRQ_MSI)
>>>  
>>>  typedef struct SpaprMachineState SpaprMachineState;
>>>  
>>> @@ -32,7 +37,7 @@ int spapr_irq_msi_alloc(SpaprMachineState *spapr, 
>>> uint32_t num, bool align,
>>>  void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num);
>>>  
>>>  typedef struct SpaprIrq {
>>> -    uint32_t    nr_irqs;
>>> +    uint32_t    nr_xirqs;
>>>      uint32_t    nr_msis;
>>>      uint8_t     ov5;
>>>  
>>>
>>
>