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