Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1570104

Re: ibmvtpm byteswapping inconsistency

Show all headers | View raw

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
> 

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

ibmvtpm byteswapping inconsistency Michal Suchánek <msuchanek@suse.de> - 2017-01-26 21:30 +0100
  Re: ibmvtpm byteswapping inconsistency Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-26 23:10 +0100
    Re: ibmvtpm byteswapping inconsistency Michal Suchanek <hramrach@gmail.com> - 2017-01-26 23:50 +0100
    Re: ibmvtpm byteswapping inconsistency Ashley Lai <ashleydlai@gmail.com> - 2017-01-27 00:00 +0100
      Re: ibmvtpm byteswapping inconsistency Vicky <honclo@linux.vnet.ibm.com> - 2017-02-02 05:50 +0100
        Re: ibmvtpm byteswapping inconsistency Michael Ellerman <mpe@ellerman.id.au> - 2017-02-02 12:00 +0100
        Re: ibmvtpm byteswapping inconsistency Michal Suchánek <msuchanek@suse.de> - 2017-02-02 12:50 +0100
          RE: ibmvtpm byteswapping inconsistency David Laight <David.Laight@ACULAB.COM> - 2017-02-02 16:20 +0100
  Re: ibmvtpm byteswapping inconsistency Tyrel Datwyler <tyreld@linux.vnet.ibm.com> - 2017-01-27 02:50 +0100
    Re: ibmvtpm byteswapping inconsistency Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2017-01-27 03:50 +0100
      Re: ibmvtpm byteswapping inconsistency Michal Suchanek <hramrach@gmail.com> - 2017-01-27 11:10 +0100
        Re: ibmvtpm byteswapping inconsistency Tyrel Datwyler <tyreld@linux.vnet.ibm.com> - 2017-01-28 02:00 +0100
          Re: ibmvtpm byteswapping inconsistency Michael Ellerman <mpe@ellerman.id.au> - 2017-01-30 05:40 +0100
            Re: ibmvtpm byteswapping inconsistency Tyrel Datwyler <tyreld@linux.vnet.ibm.com> - 2017-01-30 21:40 +0100
              Re: ibmvtpm byteswapping inconsistency Michael Ellerman <mpe@ellerman.id.au> - 2017-01-31 09:40 +0100
      Re: ibmvtpm byteswapping inconsistency Tyrel Datwyler <tyreld@linux.vnet.ibm.com> - 2017-01-27 21:30 +0100
        Re: ibmvtpm byteswapping inconsistency Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2017-01-27 21:50 +0100
          Re: ibmvtpm byteswapping inconsistency Tyrel Datwyler <tyreld@linux.vnet.ibm.com> - 2017-01-28 00:50 +0100
            Re: ibmvtpm byteswapping inconsistency msuchanek <msuchanek@suse.de> - 2017-01-28 01:40 +0100
            Re: ibmvtpm byteswapping inconsistency Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2017-01-28 05:40 +0100
        RE: ibmvtpm byteswapping inconsistency David Laight <David.Laight@ACULAB.COM> - 2017-01-30 15:50 +0100
  RE: ibmvtpm byteswapping inconsistency David Laight <David.Laight@ACULAB.COM> - 2017-01-27 12:50 +0100

csiph-web