Re: ibmvtpm byteswapping inconsistency

[Tyrel Datwyler <tyreld@linux.vnet.ibm.com>]

On 01/29/2017 08:32 PM, Michael Ellerman wrote:
> Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes:
> 
>> On 01/27/2017 01:03 AM, Michal Suchanek wrote:
>>> On 27 January 2017 at 02:50, Benjamin Herrenschmidt
>>> <benh@kernel.crashing.org> wrote:
>>>> On Thu, 2017-01-26 at 17:42 -0800, Tyrel Datwyler wrote:
>>>>> On 01/26/2017 12:22 PM, Michal Suchánek wrote:
>>>>>> Hello,
>>>>>>
>>>>>> building ibmvtpm I noticed gcc warning complaining that second word
>>>>>> of
>>>>>> struct ibmvtpm_crq in tpm_ibmvtpm_suspend is uninitialized.
>>>>>>
>>>>>> The structure is defined as
>>>>>>
>>>>>> struct ibmvtpm_crq {
>>>>>>         u8 valid;
>>>>>>         u8 msg;
>>>>>>         __be16 len;
>>>>>>         __be32 data;
>>>>>>         __be64 reserved;
>>>>>> } __attribute__((packed, aligned(8)));
>>>>>>
>>>>>> initialized as
>>>>>>
>>>>>>         struct ibmvtpm_crq crq;
>>>>>>         u64 *buf = (u64 *) &crq;
>>>>>> ...
>>>>>>         crq.valid = (u8)IBMVTPM_VALID_CMD;
>>>>>>         crq.msg = (u8)VTPM_PREPARE_TO_SUSPEND;
>>>>>>
>>>>>> and submitted with
>>>>>>
>>>>>>         rc = ibmvtpm_send_crq(ibmvtpm->vdev, cpu_to_be64(buf[0]),
>>>>>>                               cpu_to_be64(buf[1]));
>>>>>
>>>>> These should be be64_to_cpu() here. The underlying hcall made by
>>>>> ibmvtpm_send_crq() requires parameters to be in cpu endian unlike the
>>>>> RTAS interface which requires data in BE.
>>>>
>>>> Hrm... an hcall takes register arguments. Register arguments don't have
>>>> an endianness.
>>>>
>>>> The problem is that we are packing an in-memory structure into 2
>>>> registers and it's expected that this structure is laid out in the
>>>> registers as if it had been loaded by a BE CPU.
>>>>
>>>> So we have two things at play here:
>>>>
>>>>   - The >8-bit fields should be laid out BE in the memory image
>>>>   - That whole 128-bit structure should be loaded into 2 64-bit
>>>> registers MSB first.
>>>>
>>>> So the "double" swap is somewhat needed. The uglyness comes from the
>>>> passing-by-register of the h-call but it should work.
>>>>
>>>> That said, be64_to_cpup(buf) and be64_to_cpup(buf+1) might give you
>>>> better result (though recent gcc's might not make a difference).
>>>
>>> If this should work then the below case that swaps the fields separately is
>>> broken.
>>>
>>> Anyway, structures have no endianess so when they start with a byte they
>>> start with that byte no matter the host endian.
>>> crq.valid is the first byte always. And then each field is to be swapped
>>> separately.
>>>
>>> On the other hand, bitfields are part of an integer and the field should be
>>> swapped as part of the integer.
>>>
>>> That is,
>>> #define CRQ_VALID ((buf[0] >> 56) & 0xff)
>>> CRQ_VALID is part of an integer in buf and would be laid out differently
>>> on start or end depending on the host being BE or LE.
>>>
>>> And the question is what the PAPR actually defines because both ways are
>>> used in the code. You can describe an in-memory layout either way.
>>
>> Byte  |   0   |   1   |   2   |   3   |   4   |   5   |   6   |   7
>> -----------------------------------------------------------------------
>> Word0 | Valid | Type  |     Length    |              Data
>> -----------------------------------------------------------------------
>> Word1 |				Reserved
>> -----------------------------------------------------------------------
>>
>> The following definition looks to match:
>>
>> struct ibmvtpm_crq {
>>         u8 valid;
>>         u8 msg;
>>         __be16 len;
>>         __be32 data;
>>         __be64 reserved;
>> } __attribute__((packed, aligned(8)));
> 
> Well it's a partial match.
> 
> Your layout above doesn't define which byte of Length or Data is the MSB
> or LSB. So going by that we still don't know the endianness of either

I should have been explicit that PAPR uses Big Endian bit and byte
numbering throughout the spec unless otherwise noted.

-Tyrel

> field.
> 
> cheers
>