Re: [Qemu-devel] [PATCH v5 06/36] ppc/xive: add support for the END Event State buffers

[David Gibson]

On Thu, Nov 29, 2018 at 11:06:13PM +0100, Cédric Le Goater wrote:
> On 11/22/18 6:13 AM, David Gibson wrote:
> > On Fri, Nov 16, 2018 at 11:56:59AM +0100, Cédric Le Goater wrote:
> >> The Event Notification Descriptor also contains two Event State
> >> Buffers providing further coalescing of interrupts, one for the
> >> notification event (ESn) and one for the escalation events (ESe). A
> >> MMIO page is assigned for each to control the EOI through loads
> >> only. Stores are not allowed.
> >>
> >> The END ESBs are modeled through an object resembling the 'XiveSource'
> >> It is stateless as the END state bits are backed into the XiveEND
> >> structure under the XiveRouter and the MMIO accesses follow the same
> >> rules as for the standard source ESBs.
> >>
> >> END ESBs are not supported by the Linux drivers neither on OPAL nor on
> >> sPAPR. Nevetherless, it provides a mean to study the question in the
> >> future and validates a bit more the XIVE model.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >>  include/hw/ppc/xive.h |  20 ++++++
> >>  hw/intc/xive.c        | 160 +++++++++++++++++++++++++++++++++++++++++-
> >>  2 files changed, 178 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> >> index ce62aaf28343..24301bf2076d 100644
> >> --- a/include/hw/ppc/xive.h
> >> +++ b/include/hw/ppc/xive.h
> >> @@ -208,6 +208,26 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t 
> >> end_blk, uint32_t end_idx,
> >>  int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t 
> >> end_idx,
> >>                          XiveEND *end);
> >>  
> >> +/*
> >> + * XIVE END ESBs
> >> + */
> >> +
> >> +#define TYPE_XIVE_END_SOURCE "xive-end-source"
> >> +#define XIVE_END_SOURCE(obj) \
> >> +    OBJECT_CHECK(XiveENDSource, (obj), TYPE_XIVE_END_SOURCE)
> > 
> > Is there a particular reason to make this a full QOM object, rather
> > than just embedding it in the XiveRouter?
> 
> Coming back on this question because removing the chip_id from the
> router is a problem for the END triggering. At least with the current
> design. See below for the comment.
> 
> >> +typedef struct XiveENDSource {
> >> +    SysBusDevice parent;
> >> +
> >> +    uint32_t        nr_ends;
> >> +
> >> +    /* ESB memory region */
> >> +    uint32_t        esb_shift;
> >> +    MemoryRegion    esb_mmio;
> >> +
> >> +    XiveRouter      *xrtr;
> >> +} XiveENDSource;
> >> +
> >>  /*
> >>   * For legacy compatibility, the exceptions define up to 256 different
> >>   * priorities. P9 implements only 9 levels : 8 active levels [0 - 7]
> >> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> >> index 9cb001e7b540..5a8882d47a98 100644
> >> --- a/hw/intc/xive.c
> >> +++ b/hw/intc/xive.c
> >> @@ -622,8 +622,18 @@ static void xive_router_end_notify(XiveRouter *xrtr, 
> >> uint8_t end_blk,
> >>       * even futher coalescing in the Router
> >>       */
> >>      if (!(end.w0 & END_W0_UCOND_NOTIFY)) {
> >> -        qemu_log_mask(LOG_UNIMP, "XIVE: !UCOND_NOTIFY not implemented\n");
> >> -        return;
> >> +        uint8_t pq = GETFIELD(END_W1_ESn, end.w1);
> >> +        bool notify = xive_esb_trigger(&pq);
> >> +
> >> +        if (pq != GETFIELD(END_W1_ESn, end.w1)) {
> >> +            end.w1 = SETFIELD(END_W1_ESn, end.w1, pq);
> >> +            xive_router_set_end(xrtr, end_blk, end_idx, &end);
> >> +        }
> >> +
> >> +        /* ESn[Q]=1 : end of notification */
> >> +        if (!notify) {
> >> +            return;
> >> +        }
> >>      }
> >>  
> >>      /*
> >> @@ -706,6 +716,151 @@ void xive_eas_pic_print_info(XiveEAS *eas, uint32_t 
> >> lisn, Monitor *mon)
> >>                     (uint32_t) GETFIELD(EAS_END_DATA, eas->w));
> >>  }
> >>  
> >> +/*
> >> + * END ESB MMIO loads
> >> + */
> >> +static uint64_t xive_end_source_read(void *opaque, hwaddr addr, unsigned 
> >> size)
> >> +{
> >> +    XiveENDSource *xsrc = XIVE_END_SOURCE(opaque);
> >> +    XiveRouter *xrtr = xsrc->xrtr;
> >> +    uint32_t offset = addr & 0xFFF;
> >> +    uint8_t end_blk;
> >> +    uint32_t end_idx;
> >> +    XiveEND end;
> >> +    uint32_t end_esmask;
> >> +    uint8_t pq;
> >> +    uint64_t ret = -1;
> >> +
> >> +    end_blk = xrtr->chip_id;
> >> +    end_idx = addr >> (xsrc->esb_shift + 1);
> >> +    if (xive_router_get_end(xrtr, end_blk, end_idx, &end)) {
> 
> The current END accessors require a block identifier, hence xrtr->chip_id, 
> but in this case, we don't really need it because we are using the ENDT 
> local to the router/chip. 

> I don't know how to handle simply this case without keeping chip_id :/

I don't really follow how chip_id is relevant here.  AFAICT the END
accessors take a block id and the back end is responsible for
interpreting them.  The ponwernv one will map it to chip id, but the
PAPR one can just ignore it or only use block 0.

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