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

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

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

On Thu, Nov 22, 2018 at 08:39:41AM +0100, Cédric Le Goater wrote:
> 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 <clg@kaod.org>
> > 
> > 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.

Yes, I see that.  But the result is that while the MSI logic is
encapsulated in the MSI trigger function, this leaves the LSI logic
split across the trigger function and set_irq() itself.  I think it
would be better to have assert and deassert helpers instead, which
handle both the trigger/notification and also the updating of the
ASSERTED 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()
> >>      },
> >>  };
> > 
> 

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