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

[David Gibson]

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?

> +{
> +    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.

> +{
> +    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.

>          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

signature.asc
Description: PGP signature