Re: [Qemu-ppc] [PATCH v3 02/35] ppc/xive: add support for the LSI interrupt sources

[Cédric Le Goater]

On 04/23/2018 08:44 AM, David Gibson wrote:
> On Thu, Apr 19, 2018 at 02:42:58PM +0200, 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>
>> ---
>>  hw/intc/xive.c        | 54 
>> +++++++++++++++++++++++++++++++++++++++++++++++----
>>  include/hw/ppc/xive.h | 16 +++++++++++++++
>>  2 files changed, 66 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index c70578759d02..060976077dd7 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -104,6 +104,21 @@ static void xive_source_notify(XiveSource *xsrc, int 
>> srcno)
>>  
>>  }
>>  
>> +/*
>> + * LSI interrupt sources use the P bit and a custom assertion flag
>> + */
>> +static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t srcno)
>> +{
>> +    uint8_t old_pq = xive_source_pq_get(xsrc, srcno);
>> +
>> +    if  (old_pq == XIVE_ESB_RESET &&
>> +         xsrc->status[srcno] & XIVE_STATUS_ASSERTED) {
>> +        xive_source_pq_set(xsrc, srcno, XIVE_ESB_PENDING);
>> +        return true;
>> +    }
>> +    return false;
>> +}
>> +
>>  /* In a two pages ESB MMIO setting, even page is the trigger page, odd
>>   * page is for management */
>>  static inline bool xive_source_is_trigger_page(hwaddr addr)
>> @@ -133,6 +148,13 @@ static uint64_t xive_source_esb_read(void *opaque, 
>> hwaddr addr, unsigned size)
>>           */
>>          ret = xive_source_pq_eoi(xsrc, srcno);
>>  
>> +        /* If the LSI source is still asserted, forward a new source
>> +         * event notification */
>> +        if (xive_source_irq_is_lsi(xsrc, srcno)) {
>> +            if (xive_source_lsi_trigger(xsrc, srcno)) {
>> +                xive_source_notify(xsrc, srcno);
>> +            }
>> +        }
>>          break;
>>  
>>      case XIVE_ESB_GET:
>> @@ -183,6 +205,14 @@ static void xive_source_esb_write(void *opaque, hwaddr 
>> addr,
>>           * notification
>>           */
>>          notify = xive_source_pq_eoi(xsrc, srcno);
>> +
>> +        /* LSI sources do not set the Q bit but they can still be
>> +         * asserted, in which case we should forward a new source
>> +         * event notification
>> +         */
>> +        if (xive_source_irq_is_lsi(xsrc, srcno)) {
>> +            notify = xive_source_lsi_trigger(xsrc, srcno);
>> +        }

FYI, I have moved that common test under xive_source_pq_eoi()

>>          break;
>>  
>>      default:
>> @@ -216,8 +246,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_pq_trigger(xsrc, srcno);
>> +    if (xive_source_irq_is_lsi(xsrc, srcno)) {
>> +        if (val) {
>> +            xsrc->status[srcno] |= XIVE_STATUS_ASSERTED;
>> +        } else {
>> +            xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED;
>> +        }
>> +        notify = xive_source_lsi_trigger(xsrc, srcno);
>> +    } else {
>> +        if (val) {
>> +            notify = xive_source_pq_trigger(xsrc, srcno);
>> +        }
>>      }
>>  
>>      /* Forward the source event notification for routing */
>> @@ -234,13 +273,13 @@ void xive_source_pic_print_info(XiveSource *xsrc, 
>> Monitor *mon)
>>                     xsrc->offset, xsrc->offset + xsrc->nr_irqs - 1);
>>      for (i = 0; i < xsrc->nr_irqs; i++) {
>>          uint8_t pq = xive_source_pq_get(xsrc, i);
>> -        uint32_t lisn = i  + xsrc->offset;
>>  
>>          if (pq == XIVE_ESB_OFF) {
>>              continue;
>>          }
>>  
>> -        monitor_printf(mon, "  %4x %c%c\n", lisn,
>> +        monitor_printf(mon, "  %4x %s %c%c\n", i + xsrc->offset,
>> +                       xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI",
>>                         pq & XIVE_ESB_VAL_P ? 'P' : '-',
>>                         pq & XIVE_ESB_VAL_Q ? 'Q' : '-');
>>      }
>> @@ -249,6 +288,12 @@ void xive_source_pic_print_info(XiveSource *xsrc, 
>> Monitor *mon)
>>  static void xive_source_reset(DeviceState *dev)
>>  {
>>      XiveSource *xsrc = XIVE_SOURCE(dev);
>> +    int i;
>> +
>> +    /* Keep the IRQ type */
>> +    for (i = 0; i < xsrc->nr_irqs; i++) {
>> +        xsrc->status[i] &= ~XIVE_STATUS_ASSERTED;
>> +    }
>>  
>>      /* SBEs are initialized to 0b01 which corresponds to "ints off" */
>>      memset(xsrc->sbe, 0x55, xsrc->sbe_size);
>> @@ -273,6 +318,7 @@ static void xive_source_realize(DeviceState *dev, Error 
>> **errp)
>>  
>>      xsrc->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc,
>>                                       xsrc->nr_irqs);
>> +    xsrc->status = g_malloc0(xsrc->nr_irqs);
>>  
>>      /* Allocate the SBEs (State Bit Entry). 2 bits, so 4 entries per byte */
>>      xsrc->sbe_size = DIV_ROUND_UP(xsrc->nr_irqs, 4);
>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>> index d92a50519edf..0b76dd278d9b 100644
>> --- a/include/hw/ppc/xive.h
>> +++ b/include/hw/ppc/xive.h
>> @@ -33,6 +33,9 @@ typedef struct XiveSource {
>>      uint32_t     nr_irqs;
>>      uint32_t     offset;
>>      qemu_irq     *qirqs;
>> +#define XIVE_STATUS_LSI         0x1
>> +#define XIVE_STATUS_ASSERTED    0x2
>> +    uint8_t      *status;
> 
> I don't love the idea of mixing configuration information (STATUS_LSI)
> with runtime state information (ASSERTED) in the same field.  Any
> reason not to have these as parallel bitmaps.

none. I can change that. 
 
> Come to that.. is there a compelling reason to allow any individual
> irq to be marked LSI or MSI, rather than using separate XiveSource
> objects for MSIs and LSIs?

yes. I would have preferred two distinct interrupt source objects but 
this is to be compatible with XICS, which uses only one. If we want
to be able to change interrupt mode, the IRQ number space should be
organized in the exact same way. Or we should change XICS also.

Also, the change (a bitmap) is really small.

>>      /* PQ bits */
>>      uint8_t      *sbe;
> 
> .. and come to that is there a reason to keep the ASSERTED bit in a
> separate array from sbe?  AFAICT the actual 2-bit-per-irq layout is
> never exposed to the guests.

indeed. we always use the xive_source_pq_get/set() helpers to 
manipulate the PQ bits. So we could add an extra bit for the ASSERT 
without too much changes. Could also we put the type there or would 
you still prefer a bitmap ?  

> Or, even re-use the Q bit for asserted in LSIs (but report it as
> always 0 in the register read/write path).

I would prefer to add extra status bits. It is easier to debug.

Thanks,

C.

>> @@ -127,4 +130,17 @@ uint8_t xive_source_pq_set(XiveSource *xsrc, uint32_t 
>> srcno, uint8_t pq);
>>  
>>  void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon);
>>  
>> +static inline bool xive_source_irq_is_lsi(XiveSource *xsrc, uint32_t srcno)
>> +{
>> +    assert(srcno < xsrc->nr_irqs);
>> +    return xsrc->status[srcno] & XIVE_STATUS_LSI;
>> +}
>> +
>> +static inline void xive_source_irq_set(XiveSource *xsrc, uint32_t srcno,
>> +                                       bool lsi)
>> +{
>> +    assert(srcno < xsrc->nr_irqs);
>> +    xsrc->status[srcno] |= lsi ? XIVE_STATUS_LSI : 0;
>> +}
>> +
>>  #endif /* PPC_XIVE_H */
>