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

[Thomas Huth]

On 23.11.2017 13:07, Yi Min Zhao wrote:
> 
> 
> 在 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?

As far as I know, it's host endianness, so on x86 with TCG emulation,
it's little endian.

> In our case, the guest is S390. Although the arch is big-endian, the
> data in
> pcilg/stg instructions is little-endian.

PCI memory is always little endian, right.

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

Yes, the "cpu" in cpu_to_le or le_to_cpu means the host, indeed. It's
confusing :-/

> 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 think we've got to byte-swap if the host is big endian (s390x), but
not if the host is little endian (x86 with TCG).

 Thomas