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

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

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

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 <clg@kaod.org>
> ---
>  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);
> +        }
>          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.

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?

>      /* 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.

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

> @@ -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 */

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