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

Groups > linux.kernel > #1567688 > unrolled thread

ibmvtpm byteswapping inconsistency

Back to article view | Back to linux.kernel

Contents

  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 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-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

#1567688 — ibmvtpm byteswapping inconsistency

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]));

which means that the second word indeed contains purely garbage.

This is repeated a few times in the driver so I added memset to quiet
gcc and make behavior deterministic in case the unused fields get some
meaning in the future.

However, in tpm_ibmvtpm_send the structure is initialized as

	struct ibmvtpm_crq crq;
        __be64 *word = (__be64 *)&crq;
...
        crq.valid = (u8)IBMVTPM_VALID_CMD;
        crq.msg = (u8)VTPM_TPM_COMMAND;
        crq.len = cpu_to_be16(count);
        crq.data = cpu_to_be32(ibmvtpm->rtce_dma_handle);

and submitted with

	rc = ibmvtpm_send_crq(ibmvtpm->vdev, be64_to_cpu(word[0]),
                              be64_to_cpu(word[1]));
meaning it is swapped twice.


Where is the interface defined? Are the command arguments passed as BE
subfields (the second case was correct before adding the extra whole
word swap) or BE words (the first case doing whole word swap is
correct)?

Thanks

Michal

[toc] | [next] | [standalone]

#1567749

On Thu, Jan 26, 2017 at 09:22:48PM +0100, Michal Such??nek wrote:

> This is repeated a few times in the driver so I added memset to quiet
> gcc and make behavior deterministic in case the unused fields get some
> meaning in the future.

Yep, reserved certainly needs to be zeroed.. Can you send a patch?
memset is overkill...

> However, in tpm_ibmvtpm_send the structure is initialized as
> 
> 	struct ibmvtpm_crq crq;
>         __be64 *word = (__be64 *)&crq;
> ...
>         crq.valid = (u8)IBMVTPM_VALID_CMD;
>         crq.msg = (u8)VTPM_TPM_COMMAND;
>         crq.len = cpu_to_be16(count);
>         crq.data = cpu_to_be32(ibmvtpm->rtce_dma_handle);
> 
> and submitted with
> 
> 	rc = ibmvtpm_send_crq(ibmvtpm->vdev, be64_to_cpu(word[0]),
>                               be64_to_cpu(word[1]));
> meaning it is swapped twice.

No idea, Nayna may know.

My guess is that '__be64 *word' should be 'u64 *word'...

Jason

[toc] | [prev] | [next] | [standalone]

#1567778

On 26 January 2017 at 23:05, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Thu, Jan 26, 2017 at 09:22:48PM +0100, Michal Such??nek wrote:
>
>> This is repeated a few times in the driver so I added memset to quiet
>> gcc and make behavior deterministic in case the unused fields get some
>> meaning in the future.
>
> Yep, reserved certainly needs to be zeroed.. Can you send a patch?

And len and data as well..

> memset is overkill...

Does not look so.

Michal

[toc] | [prev] | [next] | [standalone]

#1567791

Adding Vicky from IBM.


On 01/26/2017 04:05 PM, Jason Gunthorpe wrote:
> On Thu, Jan 26, 2017 at 09:22:48PM +0100, Michal Such??nek wrote:
>
>> This is repeated a few times in the driver so I added memset to quiet
>> gcc and make behavior deterministic in case the unused fields get some
>> meaning in the future.
> Yep, reserved certainly needs to be zeroed.. Can you send a patch?
> memset is overkill...
>
>> However, in tpm_ibmvtpm_send the structure is initialized as
>>
>> 	struct ibmvtpm_crq crq;
>>          __be64 *word = (__be64 *)&crq;
>> ...
>>          crq.valid = (u8)IBMVTPM_VALID_CMD;
>>          crq.msg = (u8)VTPM_TPM_COMMAND;
>>          crq.len = cpu_to_be16(count);
>>          crq.data = cpu_to_be32(ibmvtpm->rtce_dma_handle);
>>
>> and submitted with
>>
>> 	rc = ibmvtpm_send_crq(ibmvtpm->vdev, be64_to_cpu(word[0]),
>>                                be64_to_cpu(word[1]));
>> meaning it is swapped twice.
> No idea, Nayna may know.
>
> My guess is that '__be64 *word' should be 'u64 *word'...
>
> Jason

[toc] | [prev] | [next] | [standalone]

#1567860

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.

> 
> which means that the second word indeed contains purely garbage.
> 
> This is repeated a few times in the driver so I added memset to quiet
> gcc and make behavior deterministic in case the unused fields get some
> meaning in the future.
> 
> However, in tpm_ibmvtpm_send the structure is initialized as
> 
> 	struct ibmvtpm_crq crq;
>         __be64 *word = (__be64 *)&crq;
> ...
>         crq.valid = (u8)IBMVTPM_VALID_CMD;
>         crq.msg = (u8)VTPM_TPM_COMMAND;
>         crq.len = cpu_to_be16(count);
>         crq.data = cpu_to_be32(ibmvtpm->rtce_dma_handle);
> 
> and submitted with
> 
> 	rc = ibmvtpm_send_crq(ibmvtpm->vdev, be64_to_cpu(word[0]),
>                               be64_to_cpu(word[1]));
> meaning it is swapped twice.
> 
> 
> Where is the interface defined? Are the command arguments passed as BE
> subfields (the second case was correct before adding the extra whole
> word swap) or BE words (the first case doing whole word swap is
> correct)?

The interface is defined in PAPR. The crq format is defined in BE terms.
However, when we break the crq apart into high and low words they need
to be in cpu endian as mentioned above.

-Tyrel

> 
> Thanks
> 
> Michal
> 

[toc] | [prev] | [next] | [standalone]

#1567874

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).
> > 
> > which means that the second word indeed contains purely garbage.
> > 
> > This is repeated a few times in the driver so I added memset to
> > quiet
> > gcc and make behavior deterministic in case the unused fields get
> > some
> > meaning in the future.
> > 
> > However, in tpm_ibmvtpm_send the structure is initialized as
> > 
> > 	struct ibmvtpm_crq crq;
> >         __be64 *word = (__be64 *)&crq;
> > ...
> >         crq.valid = (u8)IBMVTPM_VALID_CMD;
> >         crq.msg = (u8)VTPM_TPM_COMMAND;
> >         crq.len = cpu_to_be16(count);
> >         crq.data = cpu_to_be32(ibmvtpm->rtce_dma_handle);
> > 
> > and submitted with
> > 
> > 	rc = ibmvtpm_send_crq(ibmvtpm->vdev, be64_to_cpu(word[0]),
> >                               be64_to_cpu(word[1]));
> > meaning it is swapped twice.
> > 
> > 
> > Where is the interface defined? Are the command arguments passed as
> > BE
> > subfields (the second case was correct before adding the extra
> > whole
> > word swap) or BE words (the first case doing whole word swap is
> > correct)?
> 
> The interface is defined in PAPR. The crq format is defined in BE
> terms.
> However, when we break the crq apart into high and low words they
> need
> to be in cpu endian as mentioned above.
> 
> -Tyrel
> 
> > 
> > Thanks
> > 
> > Michal
> > 

[toc] | [prev] | [next] | [standalone]

#1568011

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.

>> >
>> > which means that the second word indeed contains purely garbage.
>> >
>> > This is repeated a few times in the driver so I added memset to
>> > quiet
>> > gcc and make behavior deterministic in case the unused fields get
>> > some
>> > meaning in the future.
>> >
>> > However, in tpm_ibmvtpm_send the structure is initialized as
>> >
>> >     struct ibmvtpm_crq crq;
>> >         __be64 *word = (__be64 *)&crq;
>> > ...
>> >         crq.valid = (u8)IBMVTPM_VALID_CMD;
>> >         crq.msg = (u8)VTPM_TPM_COMMAND;
>> >         crq.len = cpu_to_be16(count);
>> >         crq.data = cpu_to_be32(ibmvtpm->rtce_dma_handle);
>> >
>> > and submitted with
>> >
>> >     rc = ibmvtpm_send_crq(ibmvtpm->vdev, be64_to_cpu(word[0]),
>> >                               be64_to_cpu(word[1]));
>> > meaning it is swapped twice.
>> >
>> >
>> > Where is the interface defined? Are the command arguments passed as
>> > BE
>> > subfields (the second case was correct before adding the extra
>> > whole
>> > word swap) or BE words (the first case doing whole word swap is
>> > correct)?
>>
>> The interface is defined in PAPR. The crq format is defined in BE
>> terms.

Which exact PAPR? Where can I get it?
The PAPR document I found does not say anything about vtpm.

Thanks

Michal

[toc] | [prev] | [next] | [standalone]

#1568807

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)));

> 
>>>>
>>>> which means that the second word indeed contains purely garbage.
>>>>
>>>> This is repeated a few times in the driver so I added memset to
>>>> quiet
>>>> gcc and make behavior deterministic in case the unused fields get
>>>> some
>>>> meaning in the future.
>>>>
>>>> However, in tpm_ibmvtpm_send the structure is initialized as
>>>>
>>>>     struct ibmvtpm_crq crq;
>>>>         __be64 *word = (__be64 *)&crq;
>>>> ...
>>>>         crq.valid = (u8)IBMVTPM_VALID_CMD;
>>>>         crq.msg = (u8)VTPM_TPM_COMMAND;
>>>>         crq.len = cpu_to_be16(count);
>>>>         crq.data = cpu_to_be32(ibmvtpm->rtce_dma_handle);
>>>>
>>>> and submitted with
>>>>
>>>>     rc = ibmvtpm_send_crq(ibmvtpm->vdev, be64_to_cpu(word[0]),
>>>>                               be64_to_cpu(word[1]));
>>>> meaning it is swapped twice.
>>>>
>>>>
>>>> Where is the interface defined? Are the command arguments passed as
>>>> BE
>>>> subfields (the second case was correct before adding the extra
>>>> whole
>>>> word swap) or BE words (the first case doing whole word swap is
>>>> correct)?
>>>
>>> The interface is defined in PAPR. The crq format is defined in BE
>>> terms.
> 
> Which exact PAPR? Where can I get it?
> The PAPR document I found does not say anything about vtpm. 

Unfortunately, vtpm doesn't appear to be covered in the publicly
available LoPAPR.

-Tyrel

> 
> Thanks
> 
> Michal
> 

[toc] | [prev] | [next] | [standalone]

#1569377

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
field.

cheers

[toc] | [prev] | [next] | [standalone]

#1568661

On 01/26/2017 05:50 PM, Benjamin Herrenschmidt 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.

I wasn't suggesting that they do. However, I still believe my point is
valid that the arguments need to be loaded into the registers according
to the endianness of the cpu. We had several bugs during LE porting
where assumptions were made that parameters should be loaded BE
regardless of cpu endian. For example:

commit 3df76a9dcc74d5f012b94ea01ed6e7aaf8362c5a
Author: Cyril Bur <cyrilbur@gmail.com>
Date:   Wed Jan 21 13:32:00 2015 +1100

    powerpc/pseries: Fix endian problems with LE migration

    RTAS events require arguments be passed in big endian while
    hypercalls have their arguments passed in registers and the values
    should therefore be in CPU endian.

> 
> 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.

This is only the case if the cpu is BE. If the cpu is LE, regardless of
the fact that our in memory structure is laid out BE, when we break it
into 2 words each of those words needs to be loaded LE.

-Tyrel

> 
> 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).
>>>
>>> which means that the second word indeed contains purely garbage.
>>>
>>> This is repeated a few times in the driver so I added memset to
>>> quiet
>>> gcc and make behavior deterministic in case the unused fields get
>>> some
>>> meaning in the future.
>>>
>>> However, in tpm_ibmvtpm_send the structure is initialized as
>>>
>>> 	struct ibmvtpm_crq crq;
>>>         __be64 *word = (__be64 *)&crq;
>>> ...
>>>         crq.valid = (u8)IBMVTPM_VALID_CMD;
>>>         crq.msg = (u8)VTPM_TPM_COMMAND;
>>>         crq.len = cpu_to_be16(count);
>>>         crq.data = cpu_to_be32(ibmvtpm->rtce_dma_handle);
>>>
>>> and submitted with
>>>
>>> 	rc = ibmvtpm_send_crq(ibmvtpm->vdev, be64_to_cpu(word[0]),
>>>                               be64_to_cpu(word[1]));
>>> meaning it is swapped twice.
>>>
>>>
>>> Where is the interface defined? Are the command arguments passed as
>>> BE
>>> subfields (the second case was correct before adding the extra
>>> whole
>>> word swap) or BE words (the first case doing whole word swap is
>>> correct)?
>>
>> The interface is defined in PAPR. The crq format is defined in BE
>> terms.
>> However, when we break the crq apart into high and low words they
>> need
>> to be in cpu endian as mentioned above.
>>
>> -Tyrel
>>
>>>
>>> Thanks
>>>
>>> Michal
>>>
> 

[toc] | [prev] | [next] | [standalone]

#1568678

On Fri, 2017-01-27 at 10:02 -0800, Tyrel Datwyler wrote:
> > 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.
> 
> This is only the case if the cpu is BE. If the cpu is LE, regardless of
> the fact that our in memory structure is laid out BE, when we break it
> into 2 words each of those words needs to be loaded LE.

That doesn't make sense and doesn't match the code... The structure
needs to always have the same in-register layout regardless of the
endianness of the CPU, especially since the underlying hypervisor
will most likely be BE :-)

Thta's why the code does a be64_to_cpu() when loading it, this in
effect performs a "BE" load, which on a BE CPU is just a normal load
and on LE is a swap to compensate for the CPU loading it the "wrong way
around".

Cheers,
Ben.

[toc] | [prev] | [next] | [standalone]

#1568786

On 01/27/2017 11:58 AM, Benjamin Herrenschmidt wrote:
> On Fri, 2017-01-27 at 10:02 -0800, Tyrel Datwyler wrote:
>>> 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.
>>
>> This is only the case if the cpu is BE. If the cpu is LE, regardless of
>> the fact that our in memory structure is laid out BE, when we break it
>> into 2 words each of those words needs to be loaded LE.
> 
> That doesn't make sense and doesn't match the code... The structure
> needs to always have the same in-register layout regardless of the
> endianness of the CPU, especially since the underlying hypervisor
> will most likely be BE :-)
> 
> Thta's why the code does a be64_to_cpu() when loading it, this in
> effect performs a "BE" load, which on a BE CPU is just a normal load
> and on LE is a swap to compensate for the CPU loading it the "wrong way
> around".

Its possible being the end of the week I'm just a little dense, but
wouldn't be64_to_cpu() imply that we are byte-swapping something that is
already, or supposedly already, in BE format to cpu endianness? Which on
a BE cpu I would expect a no-op, and on a LE cpu the 64bit word to have
been swapped from BE --> LE?

In my eyes the code does seem to support what I've argued. The same
thing is done in the scsi VIO drivers. The CRQ structure is laid out and
annotated BE. We use cpu_to_be() calls to load any non 8bit field.
Finally, each word is swapped to cpu endian when we hand it off for the
hcall.

from ibmvfc_send_event():

        __be64 *crq_as_u64 = (__be64 *) &evt->crq;

	<..snip..>

        if ((rc = ibmvfc_send_crq(vhost, be64_to_cpu(crq_as_u64[0]),
                                  be64_to_cpu(crq_as_u64[1])))) {

Again, maybe I'm missing something.

-Tyrel

> 
> Cheers,
> Ben.
> 

[toc] | [prev] | [next] | [standalone]

#1568802

Hello,

On 2017-01-27 21:32, Tyrel Datwyler wrote:
> On 01/27/2017 11:58 AM, Benjamin Herrenschmidt wrote:
>> On Fri, 2017-01-27 at 10:02 -0800, Tyrel Datwyler wrote:
>>>> 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.
>>> 
>>> This is only the case if the cpu is BE. If the cpu is LE, regardless 
>>> of
>>> the fact that our in memory structure is laid out BE, when we break 
>>> it
>>> into 2 words each of those words needs to be loaded LE.
>> 
>> That doesn't make sense and doesn't match the code... The structure
>> needs to always have the same in-register layout regardless of the
>> endianness of the CPU, especially since the underlying hypervisor
>> will most likely be BE :-)
>> 
>> Thta's why the code does a be64_to_cpu() when loading it, this in
>> effect performs a "BE" load, which on a BE CPU is just a normal load
>> and on LE is a swap to compensate for the CPU loading it the "wrong 
>> way
>> around".
> 
> Its possible being the end of the week I'm just a little dense, but
> wouldn't be64_to_cpu() imply that we are byte-swapping something that 
> is
> already, or supposedly already, in BE format to cpu endianness? Which 
> on
> a BE cpu I would expect a no-op, and on a LE cpu the 64bit word to have
> been swapped from BE --> LE?
> 
> In my eyes the code does seem to support what I've argued. The same
> thing is done in the scsi VIO drivers. The CRQ structure is laid out 
> and
> annotated BE. We use cpu_to_be() calls to load any non 8bit field.
> Finally, each word is swapped to cpu endian when we hand it off for the
> hcall.
> 
> from ibmvfc_send_event():
> 
>         __be64 *crq_as_u64 = (__be64 *) &evt->crq;
> 
> 	<..snip..>
> 
>         if ((rc = ibmvfc_send_crq(vhost, be64_to_cpu(crq_as_u64[0]),
>                                   be64_to_cpu(crq_as_u64[1])))) {
> 
> Again, maybe I'm missing something.
> 

Ok, so you perform really difficult operation for no good reason. You 
say
that the ppc dual-endian works like this: there is an internal in-cpu
representation of numbers which is always the same. What is affected by
switching endian is how memory loads and stores work.

If you pass these two words in registers you never need to swap 
anything.

> 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)));

If under BE valid is first byte then it is MSB and you would get value
to pass in word 0 as (valid << 56) | (type << 48) | (length << 32 ) | 
data.

No swaps involved.

To achieve same with structure and swaps you would indeed first swap the
members and then the whole word. Much harder to read code that way, 
though.

Thanks

Michal

[toc] | [prev] | [next] | [standalone]

#1568837

On Fri, 2017-01-27 at 12:32 -0800, Tyrel Datwyler wrote:
> Its possible being the end of the week I'm just a little dense, but
> wouldn't be64_to_cpu() imply that we are byte-swapping something that is
> already, or supposedly already, in BE format to cpu endianness? Which on
> a BE cpu I would expect a no-op, and on a LE cpu the 64bit word to have
> been swapped from BE --> LE?

It's in BE format in memory. In LE mode, loading it into a register will
get it the wrong way around, thus we have to swap it again. Once in a
register it has no "endianness" per-se, what matters is that the act
of loading from memory to a register would have loaded it the wrong
way around in LE.

> In my eyes the code does seem to support what I've argued. The same
> thing is done in the scsi VIO drivers. The CRQ structure is laid out and
> annotated BE. We use cpu_to_be() calls to load any non 8bit field.
> Finally, each word is swapped to cpu endian when we hand it off for the
> hcall.
> 
> from ibmvfc_send_event():
> 
>         __be64 *crq_as_u64 = (__be64 *) &evt->crq;
> 
>         <..snip..>
> 
>         if ((rc = ibmvfc_send_crq(vhost, be64_to_cpu(crq_as_u64[0]),
>                                   be64_to_cpu(crq_as_u64[1])))) {
> 
> Again, maybe I'm missing something.

[toc] | [prev] | [next] | [standalone]

#1569797

From: Tyrel Datwyler
> Sent: 27 January 2017 18:03
> On 01/26/2017 05:50 PM, Benjamin Herrenschmidt wrote:
> > On Thu, 2017-01-26 at 17:42 -0800, Tyrel Datwyler wrote:
> >> On 01/26/2017 12:22 PM, Michal Suchnek 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]));

Isn't the real fubar here the use of that memory layout structure at all?
It would probably all be better if the call looked like:
	rc = ibmvtpm_send_crq(ibmvtpm->vdev, MAKE_REQ(IBMVTPM_VALID_CMD,
		VTPM_PREPARE_TO_SUSPEND, xxx_len, xxx_data), 0);
and MAKE_REQ() did all the required endian independant shifts to generate
the correct 32bit value.

	David

[toc] | [prev] | [next] | [standalone]

#1568254

From: Michal Suchánek
> 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;
...

Hrummfff....
What is that attribute for, seems pretty confusing and pointless to me.

I also suspect that if you want to access it as two 64bit words it
ought to be a union.

	David

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web