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

[Thomas Huth]

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?

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

 Thomas