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
signature.asc
Description: PGP signature
