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: 4289 bytes --]

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.

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

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