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

[Thomas Huth]

On 23.11.2017 11:33, Cornelia Huck wrote:
> 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?

Well, leXX_to_cpu() is the very same as cpu_to_leXX(), so we could cheat
here and still use one wrapper function. Depends on whether we want to
be more explicit about the data flow or not (and maybe whether we want
to use sparse one day to statically check the endianness automatically).

 Thomas