Re: [qemu-s390x] [Qemu-devel] [PATCH v3 1/7] s390x/pci: factor out endianess conversion

[Yi Min Zhao]

在 2017/11/23 下午6:33, Cornelia Huck 写道:
> On Thu, 23 Nov 2017 11:25:10 +0100
> Thomas Huth <address@hidden> wrote:
>
> > On 23.11.2017 11:08, Cornelia Huck wrote:
> > > On Thu, 23 Nov 2017 11:01:23 +0100
> > > Thomas Huth <address@hidden> wrote:
> > >    
> > > > On 23.11.2017 10:49, Cornelia Huck wrote:
> > > > > On Thu, 23 Nov 2017 09:48:41 +0100
> > > > > Thomas Huth <address@hidden> wrote:
> > > > > > On 22.11.2017 23:05, Pierre Morel wrote:
> > [...]
> > > > > > > +/**
> > > > > > > + * Swap data contained in s390x big endian registers to little endian
> > > > > > > + * PCI bars.
> > > > > > > + *
> > > > > > > + * @ptr: a pointer to a uint64_t data field
> > > > > > > + * @len: the length of the valid data, must be 1,2,4 or 8
> > > > > > > + */
> > > > > > > +static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
> > > > > > > +{
> > > > > > > +    uint64_t data = *ptr;
> > > > > > > +
> > > > > > > +    switch (len) {
> > > > > > > +    case 1:
> > > > > > > +        break;
> > > > > > > +    case 2:
> > > > > > > +        data = bswap16(data);
> > > > > > > +        break;
> > > > > > > +    case 4:
> > > > > > > +        data = bswap32(data);
> > > > > > > +        break;
> > > > > > > +    case 8:
> > > > > > > +        data = bswap64(data);
> > > > > > > +        break;
> > > > > > > +    default:
> > > > > > > +        return -EINVAL;
> > > > > > > +    }
> > > > > > > +    *ptr = data;
> > > > > > > +    return 0;
> > > > > > > +}
> > > > > > While you're at it, I think that should rather be leXX_to_cpu() instead
> > > > > > of bswapXX() here,
> > > > > I don't think that's correct, as this is supposed to swap BE registers
> > > > > to LE PCI bars.
> > > > Yes, but for the CPU emulation, the registers are stored in the host's
> > > > endianness in the CPUS390XState structure. Or why do we byte-swap them
> > > > again with cpu_to_be64() during s390_store_status(), for example?
> > > Gah, endian conversion is eating my brain...
> > >
> > > So, is the content we get BE or not? I thought in our last discussion
> > > we came to the conclusion that it is.
> > data is read from / written to env->regs[r1], so this is host endian, as
> > far as I know. PCI is little endian, so using le32_to_cpu() /
> > cpu_to_le32() should IMHO be the right way to go here.
> >
> > By the way, if we want to use both, cpu_to_le and le_to_cpu, depending
> > on whether we read from or write to PCI, we should maybe *not* put this
> > code into a separate function?
> Yes, if your assessment is correct, we need two functions (I think this
> conversion is used in other places in later patches as well). Or are
> there mechanisms for that already available?
I have a question, is the data in cpu->regs the guest's endianess?
In our case, the guest is S390. Although the arch is big-endian, the data in
pcilg/stg instructions is little-endian.

Another question, does 'cpu' in cpu_to_le**() or le**_to_cpu() mean the 
host endianess?

If the answers to upper two questions are yes, we actually need handle 
two cases.
1) For pcilg, we need to translate the data to little-endian, thus 
cpu_to_le**().
2) For pcistg, we need to translate the data to host endianess, thus 
le**_to_cpu().
> > > [I really need to continue working on wiring up zpci in tcg, but I keep
> > > getting sidetracked.]
> > Maybe best if you get it running on a big endian host first ... if it is
> > then not working on a little endian host, you know that you have to look
> > for things like these "bswapXX()" statements...
> That was exactly my reasoning behind getting tcg to run... but getting
> it to run at all is the hard part :)