On Mon, Dec 03, 2018 at 05:49:37PM +0100, Cédric Le Goater wrote:
> >>>>>>>> + }
> >>>>>>>> +
> >>>>>>>> + switch (qsize) {
> >>>>>>>> + case 12:
> >>>>>>>> + case 16:
> >>>>>>>> + case 21:
> >>>>>>>> + case 24:
> >>>>>>>> + end.w3 = ((uint64_t)qpage) & 0xffffffff;
> >>>>>>>
> >>>>>>> It just occurred to me that I haven't been looking for this across any
> >>>>>>> of these reviews. Don't you need byteswaps when accessing these
> >>>>>>> in-memory structures?
> >>>>>>
> >>>>>> yes this is done when some event data is enqueued in the EQ.
> >>>>>
> >>>>> I'm not talking about the data in the EQ itself, but the fields in the
> >>>>> END (and the NVT).
> >>>>
> >>>> XIVE is all BE.
> >>>
> >>> Yes... the qemu host might not be, which is why you need byteswaps.
> >>
> >> ok. I understand.
> >>
> >>> I realized eventually you have the swaps in your pnv get/set
> >>> accessors.
> >>
> >> Yes. because skiboot is BE, like the XIVE structures.
> >
> > skiboot's endiannness isn't really relevant, because we're modelling
> > below that level.
> >
> >>> I don't like that at all for a couple of reasons:
> >>>
> >>> 1) Although the END structure is made up of word-sized fields because
> >>> that's convenient, the END really is made of a bunch of subfields of
> >>> different sizes. Knowing that it wouldn't be unreasonable for people
> >>> to expect they can look into the XIVE by byte offsets;
> >>
> >> These structures should be accessed with GETFIELD and SETFIELD macros
> >> using the XIVE definitions in the xive_regs.h header file. I would want
> >> to keep that common with skiboot for sure.
> >
> > Right. It might make sense to make some helper macros or inlines that
> > include both the GETFIELD/SETFIELD and the byteswap.
>
> ah. I have to evaluate the added complexity, because we don't really
> have a struct. it's just an array of BE words.
You're still treating as a struct which reflects an in-memory layout,
even if all the fields are words of the same size.
> So for each field or bit we are interested in, we would have an helper
> routine picking the correct word from the XIVE structure, doing the
> byteswap and extracting the value ?
>
> sigh.
Oh, no, I was just thinking a version of GETFIELD that byteswaps the
value before extracting the given field. Likewise a SETFIELD variant
that swaps, deposits the field, then swaps back.
--
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
