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

[Thomas Huth]

On 27.11.2017 11:09, Yi Min Zhao wrote:
> 
> 
> 在 2017/11/27 下午2:59, Thomas Huth 写道:
>> On 25.11.2017 14:49, Pierre Morel wrote:
>>> On 24/11/2017 07:19, Yi Min Zhao wrote:
>>>>
>>>> 在 2017/11/23 下午8:18, 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).
>>> Here is my comprehension of this funny swapping:
>>>
>>> - TCG for a BE guest and a le host swap bytes because if we do (register
>>> & 0x01) in the zPCI interception code it must work what ever the
>>> endianess is.
>> Uhhh, I might have missed that the value has already been byte-swapped
>> once by TCG for env->regs[r1] ...
>
> I want to ask a question. For this case, BE guest and LE host, is
> env->regs[r1] in LE byte ordering?

Generally env->regs[] are in host byte order, so LE if the host is LE.
Not sure which byte-order is stored in the register by the guest,
though, since I don't have the zPCI spec ... so if the (BE) guest wrote
a LE value in the register, and TCG byte-swapped it again, the value is
suddenly BE again and thus we have to always byte-swap it again...?
Sorry, it's hard to say without having the spec available.

 Thomas