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

[Yi Min Zhao <zyimin@linux.vnet.ibm.com>]

在 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 <thuth@redhat.com> wrote:
>>>>>>
>>>>>>> On 23.11.2017 11:08, Cornelia Huck wrote:
>>>>>>>> On Thu, 23 Nov 2017 11:01:23 +0100
>>>>>>>> Thomas Huth <thuth@redhat.com> wrote:
>>>>>>>>> On 23.11.2017 10:49, Cornelia Huck wrote:
>>>>>>>>>> On Thu, 23 Nov 2017 09:48:41 +0100
>>>>>>>>>> Thomas Huth <thuth@redhat.com> 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?
> Now I'm pretty much completely confused ... sorry for the noise if I was
> wrong... I think it's best you ignore my comment for now (i.e. go with
> bswapXX() instead of le_to_cpuXX()), and if we later wire up zPCI with
> TCG, we still can fix this if necessary.
>
>   Thomas
>
>