lkml.org 
[lkml]   [2017]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: ibmvtpm byteswapping inconsistency
From
Date
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
> >

\
 
 \ /
  Last update: 2017-01-27 03:44    [W:0.479 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site