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.
