Tyrel Datwyler [off-list ref] writes:

On 01/27/2017 01:03 AM, Michal Suchanek wrote:

quoted

On 27 January 2017 at 02:50, Benjamin Herrenschmidt
[off-list ref] wrote:

quoted

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

quoted

On 01/26/2017 12:22 PM, Michal Such=C3=A1nek 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 =3D (u64 *) &crq;
...
        crq.valid =3D (u8)IBMVTPM_VALID_CMD;
        crq.msg =3D (u8)VTPM_PREPARE_TO_SUSPEND;

and submitted with

        rc =3D 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).

=20
If this should work then the below case that swaps the fields separately=

 is

quoted

broken.
=20
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.
=20
On the other hand, bitfields are part of an integer and the field should=

 be

quoted

swapped as part of the integer.
=20
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.
=20
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