Re: [Qemu-devel] [PATCH v5 16/36] spapr: add hcalls support for the XIVE exploitation interrupt mode

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

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

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

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