On Thu, 2017-01-26 at 17:42 -0800, Tyrel Datwyler wrote:

On 01/26/2017 12:22 PM, Michal Suchánek wrote:

quoted

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

quoted

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

quoted

Thanks

Michal