Re: [Qemu-devel] [RFC PATCH 07/26] ppc/xive: add MMIO handlers to the XIVE interrupt source

[Alexey Kardashevskiy]

On 06/07/17 03:13, Cédric Le Goater wrote:
> Each interrupt source is associated with a 2-bit state machine called
> an Event State Buffer (ESB). It is controlled by MMIO to trigger
> events.
> 
> See code for more details on the states.
> 
> Signed-off-by: Cédric Le Goater <address@hidden>
> ---
>  hw/intc/xive.c        | 230 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/xive.h |   3 +
>  2 files changed, 233 insertions(+)
> 
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 9ff14c0da595..816031b8ac81 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -32,6 +32,226 @@ static void xive_icp_irq(XiveICSState *xs, int lisn)
>  }
>  
>  /*
> + * "magic" Event State Buffer (ESB) MMIO offsets.
> + *
> + * Each interrupt source has a 2-bit state machine called ESB
> + * which can be controlled by MMIO. It's made of 2 bits, P and
> + * Q. P indicates that an interrupt is pending (has been sent
> + * to a queue and is waiting for an EOI). Q indicates that the
> + * interrupt has been triggered while pending.
> + *
> + * This acts as a coalescing mechanism in order to guarantee
> + * that a given interrupt only occurs at most once in a queue.
> + *
> + * When doing an EOI, the Q bit will indicate if the interrupt
> + * needs to be re-triggered.
> + *
> + * The following offsets into the ESB MMIO allow to read or
> + * manipulate the PQ bits. They must be used with an 8-bytes
> + * load instruction. They all return the previous state of the
> + * interrupt (atomically).
> + *
> + * Additionally, some ESB pages support doing an EOI via a
> + * store at 0 and some ESBs support doing a trigger via a
> + * separate trigger page.
> + */
> +#define XIVE_ESB_GET            0x800
> +#define XIVE_ESB_SET_PQ_00      0xc00
> +#define XIVE_ESB_SET_PQ_01      0xd00
> +#define XIVE_ESB_SET_PQ_10      0xe00
> +#define XIVE_ESB_SET_PQ_11      0xf00
> +
> +#define XIVE_ESB_VAL_P          0x2
> +#define XIVE_ESB_VAL_Q          0x1


These are not used. I'd suggest defining the states below using these two.


> +
> +#define XIVE_ESB_RESET          0x0
> +#define XIVE_ESB_PENDING        0x2
> +#define XIVE_ESB_QUEUED         0x3
> +#define XIVE_ESB_OFF            0x1
> +
> +static uint8_t xive_pq_get(XIVE *x, uint32_t lisn)
> +{
> +    uint32_t idx = lisn;
> +    uint32_t byte = idx / 4;
> +    uint32_t bit  = (idx % 4) * 2;
> +    uint8_t* pqs = (uint8_t *) x->sbe;
> +
> +    return (pqs[byte] >> bit) & 0x3;
> +}
> +
> +static void xive_pq_set(XIVE *x, uint32_t lisn, uint8_t pq)
> +{
> +    uint32_t idx = lisn;
> +    uint32_t byte = idx / 4;
> +    uint32_t bit  = (idx % 4) * 2;
> +    uint8_t* pqs = (uint8_t *) x->sbe;
> +
> +    pqs[byte] &= ~(0x3 << bit);
> +    pqs[byte] |= (pq & 0x3) << bit;
> +}
> +
> +static bool xive_pq_eoi(XIVE *x, uint32_t lisn)


Should not it return uint8_t as well (like xive_pq_get() does)? The value
than returned from .read() is uint64_t (a binary value).




-- 
Alexey