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

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

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

On Fri, Nov 30, 2018 at 07:41:33AM +0100, Cédric Le Goater wrote:
> On 11/30/18 2:04 AM, David Gibson wrote:
> > 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 <clg@kaod.org>
> >>>> ---
> >>>>  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.
> 
> Yes. But the block value comes from the xrtr->chip_id today, on PAPR and
> PowerNV, even if it's block 0. 
> 
> What I could do is add a "chip-id" property to XiveENDSource possibly.

This still seems wrong for the PAPR model.  Why can't you configure
the end_block value directly in the Xive components, then just set it
equal to the chip_id when you build the powernv machine?

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