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

Groups > linux.kernel > #1572140 > unrolled thread

Re: ibmvtpm byteswapping inconsistency

Started byVicky <honclo@linux.vnet.ibm.com>
First post2017-02-02 05:50 +0100
Last post2017-02-02 16:20 +0100
Articles 4 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  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

#1572140 — Re: ibmvtpm byteswapping inconsistency

FromVicky <honclo@linux.vnet.ibm.com>
Date2017-02-02 05:50 +0100
SubjectRe: ibmvtpm byteswapping inconsistency
Message-ID<t6hvj-7m6-9@gated-at.bofh.it>
> On Jan 26, 2017, at 5:58 PM, Ashley Lai <ashleydlai@gmail.com> wrote:
> 
> 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
> 

I don’t think we want ‘word' to be changed back to be of type ‘u64’.   Please see commit 62dfd912ab3b5405b6fe72d0135c37e9648071f1


Vicky

[toc] | [next] | [standalone]

#1572277

FromMichael Ellerman <mpe@ellerman.id.au>
Date2017-02-02 12:00 +0100
Message-ID<t6nho-2Sm-13@gated-at.bofh.it>
In reply to#1572140
Vicky <honclo@linux.vnet.ibm.com> writes:

>> On Jan 26, 2017, at 5:58 PM, Ashley Lai <ashleydlai@gmail.com> wrote:
>> 
>> 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
>> 
>
> I don’t think we want ‘word' to be changed back to be of type ‘u64’.   Please see commit 62dfd912ab3b5405b6fe72d0135c37e9648071f1

The type of word is basically irrelevant. Unless you're running sparse
and actually checking the errors, which it seems you're not doing:

  drivers/char/tpm/tpm_ibmvtpm.c:90:30: warning: cast removes address space of expression
  drivers/char/tpm/tpm_ibmvtpm.c:91:23: warning: incorrect type in argument 1 (different address spaces)
  drivers/char/tpm/tpm_ibmvtpm.c:91:23:    expected void *<noident>
  drivers/char/tpm/tpm_ibmvtpm.c:91:23:    got void [noderef] <asn:2>*rtce_buf
  drivers/char/tpm/tpm_ibmvtpm.c:136:17: warning: cast removes address space of expression
  drivers/char/tpm/tpm_ibmvtpm.c:188:46: warning: incorrect type in argument 2 (different base types)
  drivers/char/tpm/tpm_ibmvtpm.c:188:46:    expected unsigned long long [unsigned] [usertype] w1
  drivers/char/tpm/tpm_ibmvtpm.c:188:46:    got restricted __be64 [usertype] <noident>
  drivers/char/tpm/tpm_ibmvtpm.c:189:31: warning: incorrect type in argument 3 (different base types)
  drivers/char/tpm/tpm_ibmvtpm.c:189:31:    expected unsigned long long [unsigned] [usertype] w2
  drivers/char/tpm/tpm_ibmvtpm.c:189:31:    got restricted __be64 [usertype] <noident>
  drivers/char/tpm/tpm_ibmvtpm.c:215:46: warning: incorrect type in argument 2 (different base types)
  drivers/char/tpm/tpm_ibmvtpm.c:215:46:    expected unsigned long long [unsigned] [usertype] w1
  drivers/char/tpm/tpm_ibmvtpm.c:215:46:    got restricted __be64 [usertype] <noident>
  drivers/char/tpm/tpm_ibmvtpm.c:216:31: warning: incorrect type in argument 3 (different base types)
  drivers/char/tpm/tpm_ibmvtpm.c:216:31:    expected unsigned long long [unsigned] [usertype] w2
  drivers/char/tpm/tpm_ibmvtpm.c:216:31:    got restricted __be64 [usertype] <noident>
  drivers/char/tpm/tpm_ibmvtpm.c:294:30: warning: incorrect type in argument 1 (different address spaces)
  drivers/char/tpm/tpm_ibmvtpm.c:294:30:    expected void const *<noident>
  drivers/char/tpm/tpm_ibmvtpm.c:294:30:    got void [noderef] <asn:2>*rtce_buf
  drivers/char/tpm/tpm_ibmvtpm.c:342:46: warning: incorrect type in argument 2 (different base types)
  drivers/char/tpm/tpm_ibmvtpm.c:342:46:    expected unsigned long long [unsigned] [usertype] w1
  drivers/char/tpm/tpm_ibmvtpm.c:342:46:    got restricted __be64 [usertype] <noident>
  drivers/char/tpm/tpm_ibmvtpm.c:343:31: warning: incorrect type in argument 3 (different base types)
  drivers/char/tpm/tpm_ibmvtpm.c:343:31:    expected unsigned long long [unsigned] [usertype] w2
  drivers/char/tpm/tpm_ibmvtpm.c:343:31:    got restricted __be64 [usertype] <noident>
  drivers/char/tpm/tpm_ibmvtpm.c:494:43: warning: incorrect type in assignment (different address spaces)
  drivers/char/tpm/tpm_ibmvtpm.c:494:43:    expected void [noderef] <asn:2>*rtce_buf
  drivers/char/tpm/tpm_ibmvtpm.c:494:43:    got void *
  drivers/char/tpm/tpm_ibmvtpm.c:501:52: warning: incorrect type in argument 2 (different address spaces)
  drivers/char/tpm/tpm_ibmvtpm.c:501:52:    expected void *ptr
  drivers/char/tpm/tpm_ibmvtpm.c:501:52:    got void [noderef] <asn:2>*rtce_buf
  drivers/char/tpm/tpm_ibmvtpm.c:507:46: warning: incorrect type in argument 1 (different address spaces)
  drivers/char/tpm/tpm_ibmvtpm.c:507:46:    expected void const *<noident>
  drivers/char/tpm/tpm_ibmvtpm.c:507:46:    got void [noderef] <asn:2>*rtce_buf


What matters is how you actually do the byte swaps.

cheers

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

#1572301

FromMichal Suchánek <msuchanek@suse.de>
Date2017-02-02 12:50 +0100
Message-ID<t6o3L-3p1-5@gated-at.bofh.it>
In reply to#1572140
On Wed, 1 Feb 2017 23:40:33 -0500
Vicky <honclo@linux.vnet.ibm.com> wrote:

> > On Jan 26, 2017, at 5:58 PM, Ashley Lai <ashleydlai@gmail.com>
> > wrote:
> > 
> > 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  
> >   
> 
> I don’t think we want ‘word' to be changed back to be of type
> ‘u64’.   Please see commit 62dfd912ab3b5405b6fe72d0135c37e9648071f1

The word is marked correctly as __be64 in that patch because count and
handle are swapped to BE when saved to it and the whole word is then
swapped again when loaded. If you just load ((u64)IBMVTPM_VALID_CMD <<
56 | ((u64)VTPM_TPM_COMMAND << 48) | ((u64)count << 32) |
ibmvtpm->rtce_dma_handle in a register it works equally well
without any __be and swaps involved.

Note however that __be64 and u64 are all the same to the compiler. It's
just a note for the reader and analysis tools.

Thanks

Michal

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

#1572459

FromDavid Laight <David.Laight@ACULAB.COM>
Date2017-02-02 16:20 +0100
Message-ID<t6rl0-5HE-23@gated-at.bofh.it>
In reply to#1572301
From: Michal Suchánek
> Sent: 02 February 2017 11:30
...
> The word is marked correctly as __be64 in that patch because count and
> handle are swapped to BE when saved to it and the whole word is then
> swapped again when loaded. If you just load ((u64)IBMVTPM_VALID_CMD <<
> 56 | ((u64)VTPM_TPM_COMMAND << 48) | ((u64)count << 32) |
> ibmvtpm->rtce_dma_handle in a register it works equally well
> without any __be and swaps involved.

And that version will almost certainly generate much better code.

	David

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web