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

[David Gibson]

On Fri, Nov 23, 2018 at 08:28:42AM +0100, Cédric Le Goater wrote:
> On 11/23/18 5:36 AM, David Gibson wrote:
> > On Thu, Nov 22, 2018 at 10:58:56PM +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?
> >>
> >> yes, it should probably be under the XiveRouter you are right because
> >> there is a direct link with the ENDT which is in the XiverRouter. 
> >>
> >> But if I remove the chip_id field from the XiveRouter, it becomes a QOM
> >> interface. something to ponder.
> > 
> > Huh?  I really don't understand what you're saying here.  What does
> > chip_id have to do with anything?
> 
> I am quoting a comment of yours :
> 
>       > +/*
>       > + * XIVE Router
>       > + */
>       > +
>       > +typedef struct XiveRouter {
>       > +    SysBusDevice    parent;
>       > +
>       > +    uint32_t        chip_id;
> 
>       I don't think this belongs in the base class.  The PowerNV specific
>       variants will need it, but it doesn't make sense for the PAPR version.
> 
> 
> If we remove 'chip_id' from XiveRouter, it can become a QOM interface 
> without state, like the XiveFabric is.

Hm, not really.  At this stage the object does't have any state, but
at least the pnv variants will have state for the registers which
point it to the EAST and ENDT, so a "real" object still makes more
sense than an interface.  If it were an interface it's not clear what
real object it would be attached to.

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