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