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

[Cédric Le Goater]

On 12/3/18 2:14 AM, David Gibson wrote:
> 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 <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.
>>
>> 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. 

We don't really care for PAPR. We can use 0 but the model is common
with PowerNV.

> Why can't you configure the end_block value directly in the Xive 
> components, 

That is what I am proposing to add a "chip-id" property to XiveENDSource 

> then just set it equal to the chip_id when you build the powernv 
> machine?

yes. that's how it's more or less built. It's a little more complex
on PowerNV because there are low level settings on how the chip_id is
used in the PC and VC, but the modeling is minimal.

C.