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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5085 bytes --]

On Tue, Dec 04, 2018 at 06:04:13PM +0100, Cédric Le Goater wrote:
> 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. 

Ah, ok.  If the boundary might change in P10 that is indeed a reason
to keep them separate.

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

Ok.

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]