Re: [Qemu-devel] [PATCH v5 08/36] ppc/xive: introduce a simplified XIVE presenter

[Cédric Le Goater]

On 12/4/18 2:54 AM, David Gibson wrote:
> On Mon, Dec 03, 2018 at 06:05:12PM +0100, Cédric Le Goater wrote:
>> I forgot to reply to this one.
>>
>> On 11/29/18 1:47 AM, David Gibson wrote:
>>> On Wed, Nov 28, 2018 at 11:59:58AM +0100, Cédric Le Goater wrote:
>>>> On 11/28/18 12:49 AM, David Gibson wrote:
>>>>> On Fri, Nov 16, 2018 at 11:57:01AM +0100, Cédric Le Goater wrote:
>>>>>> The last sub-engine of the XIVE architecture is the Interrupt
>>>>>> Virtualization Presentation Engine (IVPE). On HW, they share elements,
>>>>>> the Power Bus interface (CQ), the routing table descriptors, and they
>>>>>> can be combined in the same HW logic. We do the same in QEMU and
>>>>>> combine both engines in the XiveRouter for simplicity.
>>>>>
>>>>> Ok, I'm not entirely convinced combining the IVPE and IVRE into a
>>>>> single object is a good idea, but we can probably discuss that once
>>>>> I've read further.
>>>>
>>>> We could introduce a simplified presenter for sPAPR but I am not even
>>>> sure of that as it will get more complex if we support the EBB
>>>> one day.
> 
> [snip]
>>>>>> +static inline uint32_t xive_tctx_cam_line(uint8_t nvt_blk, uint32_t 
>>>>>> nvt_idx)
>>>>>> +{
>>>>>> +    return (nvt_blk << 19) | nvt_idx;
>>>>>
>>>>> I'm guessing this formula is the standard way of combining the NVT
>>>>> block and index into a single word?  
>>>>
>>>> That number is the VP/NVT identifier which is written in the CAM value. 
>>>> The index is on 19 bits because of the NVT  definition in the END 
>>>> structure. It is being increased to 24 bits on Power10 
>>>>
>>>>> If so, I think we should
>>>>> standardize on passing a single word "nvt_id" around and only
>>>>> splitting it when we need to use the block separately.  
>>>>
>>>> This is really the only place where we concatenate the two NVT values,
>>>> block and index. 
>>>
>>> Hm, ok.  I know we don't model them (yet, maybe ever) but could
>>> combined values appear in the PowerBUS messages that handle remote
>>> notifications?
>>
>> They do. 
>>  
>>>>> Same goes for
>>>>> the end_id, assuming there's a standard way of putting that into a
>>>>> single word.  That will address the point I raised earlier about lisn
>>>>> being passed around as a single word, but these later stage ids being
>>>>> split.
>>>>
>>>> Hmm, I am not sure this is a good option. It is not how the PowerNV 
>>>> model would use it, skiboot is very much aware of these blocks and 
>>>> indexes and for remote accesses chips are identified using the block. 
>>>> I will take a look at it but I am not found of it. I can add helpers 
>>>> in some places though.    
>>>
>>> Hm, ok.  Do the block and index appear as an (effectively) single
>>> field in the EAS?
>>
>> no. In all XIVE structures, block and index are always distinct.
> 
> Hm.  Distinct in what sense?  I get that the fields are labelled
> separately in the documentation, but if the fields are adjacent, you
> could equally well treat them as one.

yes. Indeed. They are adjacent. The size of the index is subject to 
change in P10. 

I am not sure that treating them as one will be of any help because 
we need to extract them from their XIVE structure with the *_INDEX 
and *_BLOCK masks first. I will take a look. May be not in v6.


>>>>>> +    if (!match->tctx) {
>>>>>> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: NVT %x/%x is not 
>>>>>> dispatched\n",
>>>>>> +                      nvt_blk, nvt_idx);
>>>>>> +        return false;
>>>>>
>>>>> Hmm.. this isn't actually an error isn't it? At least not for powernv
>>>>
>>>> It is on sPAPR, it would mean the END was configured with an unknow CPU. 
>>>
>>> Right.
>>>
>>>> It is not error on PowerNV, when we support escalations.
>>>>
>>>>> - that just means the NVT isn't currently dispatched, so we'll need to
>>>>> trigger the escalation interrupt.  
>>>>
>>>> Yes.
>>>>
>>>>> Does this get changed later in the series?
>>>>
>>>> No.
>>>
>>> But this code is common to PAPR and powernv, yes, so it will need to?
>>
>> When we add support for escalations, yes, it will change. Would you rather
>> use an error_report() until then ?
> 
> Ah, I guess leaving an error until we implement escalation makes
> sense.  It shouldn't be LOG_GUEST_ERROR, though, the guest didn't do
> anything wrong, and error_report() doesn't really make sense for the
> same reason.
> 
> LOG_UNIMP, I guess?

OK. will do.

Thanks,

C.