Re: [Qemu-devel] [PATCH 20/21] ppc/pnv: Add model for Power8 PHB3 PCIe Host bridge

[David Gibson]

On Tue, Apr 11, 2017 at 01:05:51PM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2017-04-10 at 18:14 +1000, David Gibson wrote:
> > > +    switch (size) {
> > > +    case 1:
> > > +        break;
> > > +    case 2:
> > > +        val = bswap16(val);
> > 
> > An unconditional bswap() seems unlikely to be correct.  What's the
> > purpose of thise?
> 
> Though it is :-) I *think* ... I did test with both LE and BE hosts I
> believe but Cedric can double check.
> 
> >From memory it comes from the fact that the PHB register space is
> declared to be big endian since 99% of it actually is.
> 
> However the CONFIG_DATA register used to issue config space accesses is
> "special" and expect little endian accesses.
> 
> However because the whole region is marked BE, qemu will have done
> the necessary swapping for BE, which thus needs to be undone for
> that specific register unconditionally:
> 
>  - BE host will not swap the value, so we get the LE value written by
> the guest which needs to be swapped to BE.
> 
>  - LE host will have swapped the value as if it was BE except it isn't
> so we need to re-swap it.
> 
> IE. Another option would be to create a specific memory region just for
> that one register but it's easier to just unconditionally swap.

Ok, that makes sense.  A comment saying that this one register is LE
whereas most are BE would be helpful.

> > > +        break;
> > > +    case 4:
> > > +        val = bswap32(val);
> > > +        break;
> > 
> > No 64-bit config registers?
> 
> No, they don't exist. (Which is also why an assert isn't a good
> idea as other registers do support 64-bit accesses).

Ah, ok.

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