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

[Cédric Le Goater]

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.

C.