Re: [Qemu-devel] [PATCH v5 02/36] ppc/xive: add support for the LSI interrupt sources

[Cédric Le Goater]

On 11/22/18 4:19 AM, David Gibson wrote:
> On Fri, Nov 16, 2018 at 11:56:55AM +0100, Cédric Le Goater wrote:
>> The 'sent' status of the LSI interrupt source is modeled with the 'P'
>> bit of the ESB and the assertion status of the source is maintained in
>> an array under the main sPAPRXive object. The type of the source is
>> stored in the same array for practical reasons.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
> 
> Looks good except for some minor details.
> 
>> ---
>>  include/hw/ppc/xive.h | 20 ++++++++++++-
>>  hw/intc/xive.c        | 68 +++++++++++++++++++++++++++++++++++++++----
>>  2 files changed, 81 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>> index 5fec4b08705d..e118acd59f1e 100644
>> --- a/include/hw/ppc/xive.h
>> +++ b/include/hw/ppc/xive.h
>> @@ -32,8 +32,10 @@ typedef struct XiveSource {
>>      /* IRQs */
>>      uint32_t        nr_irqs;
>>      qemu_irq        *qirqs;
>> +    unsigned long   *lsi_map;
>> +    int32_t         lsi_map_size; /* for VMSTATE_BITMAP */
> 
> At some point it's possible we'll want XiveSource subclasses that just
> know which irqs are LSI and which aren't without an explicit map.  But
> this detail isn't exposed in the migration stream or the user
> interface, so we can tweak it later as ncessary.
> 
>> -    /* PQ bits */
>> +    /* PQ bits and LSI assertion bit */
>>      uint8_t         *status;
>>  
>>      /* ESB memory region */
>> @@ -89,6 +91,7 @@ static inline hwaddr xive_source_esb_mgmt(XiveSource 
>> *xsrc, int srcno)
>>   * When doing an EOI, the Q bit will indicate if the interrupt
>>   * needs to be re-triggered.
>>   */
>> +#define XIVE_STATUS_ASSERTED  0x4  /* Extra bit for LSI */
>>  #define XIVE_ESB_VAL_P        0x2
>>  #define XIVE_ESB_VAL_Q        0x1
>>  
>> @@ -127,4 +130,19 @@ static inline qemu_irq xive_source_qirq(XiveSource 
>> *xsrc, uint32_t srcno)
>>      return xsrc->qirqs[srcno];
>>  }
>>  
>> +static inline bool xive_source_irq_is_lsi(XiveSource *xsrc, uint32_t srcno)
>> +{
>> +    assert(srcno < xsrc->nr_irqs);
>> +    return test_bit(srcno, xsrc->lsi_map);
>> +}
>> +
>> +static inline void xive_source_irq_set(XiveSource *xsrc, uint32_t srcno,
>> +                                       bool lsi)
> 
> The function name isn't obvious about this being controlling LSI
> configuration. '..._irq_set_lsi' maybe?

yes.


>> +{
>> +    assert(srcno < xsrc->nr_irqs);
>> +    if (lsi) {
>> +        bitmap_set(xsrc->lsi_map, srcno, 1);
>> +    }
>> +}
>> +
>>  #endif /* PPC_XIVE_H */
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index f7621f84828c..ac4605fee8b7 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -88,14 +88,40 @@ uint8_t xive_source_esb_set(XiveSource *xsrc, uint32_t 
>> srcno, uint8_t pq)
>>      return xive_esb_set(&xsrc->status[srcno], pq);
>>  }
>>  
>> +/*
>> + * Returns whether the event notification should be forwarded.
>> + */
>> +static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t
>> srcno)
> 
> What exactly "trigger" means isn't entirely obvious for an LSI.  Might
> be clearer to have "lsi_assert" and "lsi_deassert" helpers instead.

This is called only when the interrupt is asserted. So it is a 
simplified LSI trigger depending only on the 'P' bit.
 
> 
>> +{
>> +    uint8_t old_pq = xive_source_esb_get(xsrc, srcno);
>> +
>> +    switch (old_pq) {
>> +    case XIVE_ESB_RESET:
>> +        xive_source_esb_set(xsrc, srcno, XIVE_ESB_PENDING);
>> +        return true;
>> +    default:
>> +        return false;
>> +    }
>> +}
>> +
>>  /*
>>   * Returns whether the event notification should be forwarded.
>>   */
>>  static bool xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno)
>>  {
>> +    bool ret;
>> +
>>      assert(srcno < xsrc->nr_irqs);
>>  
>> -    return xive_esb_trigger(&xsrc->status[srcno]);
>> +    ret = xive_esb_trigger(&xsrc->status[srcno]);
>> +
>> +    if (xive_source_irq_is_lsi(xsrc, srcno) &&
>> +        xive_source_esb_get(xsrc, srcno) == XIVE_ESB_QUEUED) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "XIVE: queued an event on LSI IRQ %d\n", srcno);
>> +    }
>> +
>> +    return ret;
>>  }
>>  
>>  /*
>> @@ -103,9 +129,22 @@ static bool xive_source_esb_trigger(XiveSource *xsrc, 
>> uint32_t srcno)
>>   */
>>  static bool xive_source_esb_eoi(XiveSource *xsrc, uint32_t srcno)
>>  {
>> +    bool ret;
>> +
>>      assert(srcno < xsrc->nr_irqs);
>>  
>> -    return xive_esb_eoi(&xsrc->status[srcno]);
>> +    ret = xive_esb_eoi(&xsrc->status[srcno]);
>> +
>> +    /* LSI sources do not set the Q bit but they can still be
>> +     * asserted, in which case we should forward a new event
>> +     * notification
>> +     */
>> +    if (xive_source_irq_is_lsi(xsrc, srcno) &&
>> +        xsrc->status[srcno] & XIVE_STATUS_ASSERTED) {
>> +        ret = xive_source_lsi_trigger(xsrc, srcno);
>> +    }
>> +
>> +    return ret;
>>  }
>>  
>>  /*
>> @@ -268,8 +307,17 @@ static void xive_source_set_irq(void *opaque, int 
>> srcno, int val)
>>      XiveSource *xsrc = XIVE_SOURCE(opaque);
>>      bool notify = false;
>>  
>> -    if (val) {
>> -        notify = xive_source_esb_trigger(xsrc, srcno);
>> +    if (xive_source_irq_is_lsi(xsrc, srcno)) {
>> +        if (val) {
>> +            xsrc->status[srcno] |= XIVE_STATUS_ASSERTED;
>> +            notify = xive_source_lsi_trigger(xsrc, srcno);
>> +        } else {
>> +            xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED;
>> +        }
>> +    } else {
>> +        if (val) {
>> +            notify = xive_source_esb_trigger(xsrc, srcno);
>> +        }
>>      }
>>  
>>      /* Forward the source event notification for routing */
>> @@ -289,9 +337,11 @@ void xive_source_pic_print_info(XiveSource *xsrc, 
>> uint32_t offset, Monitor *mon)
>>              continue;
>>          }
>>  
>> -        monitor_printf(mon, "  %08x %c%c\n", i + offset,
>> +        monitor_printf(mon, "  %08x %s %c%c%c\n", i + offset,
>> +                       xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI",
>>                         pq & XIVE_ESB_VAL_P ? 'P' : '-',
>> -                       pq & XIVE_ESB_VAL_Q ? 'Q' : '-');
>> +                       pq & XIVE_ESB_VAL_Q ? 'Q' : '-',
>> +                       xsrc->status[i] & XIVE_STATUS_ASSERTED ? 'A' : ' ');
>>      }
>>  }
>>  
>> @@ -299,6 +349,8 @@ static void xive_source_reset(DeviceState *dev)
>>  {
>>      XiveSource *xsrc = XIVE_SOURCE(dev);
>>  
>> +    /* Do not clear the LSI bitmap */
>> +
>>      /* PQs are initialized to 0b01 which corresponds to "ints off" */
>>      memset(xsrc->status, 0x1, xsrc->nr_irqs);
>>  }
>> @@ -325,6 +377,9 @@ static void xive_source_realize(DeviceState *dev, Error 
>> **errp)
>>  
>>      xsrc->status = g_malloc0(xsrc->nr_irqs);
>>  
>> +    xsrc->lsi_map = bitmap_new(xsrc->nr_irqs);
>> +    xsrc->lsi_map_size = xsrc->nr_irqs;
>> +
>>      memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc),
>>                            &xive_source_esb_ops, xsrc, "xive.esb",
>>                            (1ull << xsrc->esb_shift) * xsrc->nr_irqs);
>> @@ -338,6 +393,7 @@ static const VMStateDescription vmstate_xive_source = {
>>      .fields = (VMStateField[]) {
>>          VMSTATE_UINT32_EQUAL(nr_irqs, XiveSource, NULL),
>>          VMSTATE_VBUFFER_UINT32(status, XiveSource, 1, NULL, nr_irqs),
>> +        VMSTATE_BITMAP(lsi_map, XiveSource, 1, lsi_map_size),
> 
> This shouldn't be here.  The lsi_map is all set up at machine
> configuration time and then static, so it doesn't need to be migrated.

yes. of course ... I will get rid of it.

Thanks,

C. 
> 
>>          VMSTATE_END_OF_LIST()
>>      },
>>  };
>