From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754827AbdA0KDp (ORCPT ); Fri, 27 Jan 2017 05:03:45 -0500 Received: from mail-ua0-f178.google.com ([209.85.217.178]:36859 "EHLO mail-ua0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754564AbdA0KC1 (ORCPT ); Fri, 27 Jan 2017 05:02:27 -0500 MIME-Version: 1.0 In-Reply-To: <1485481819.2980.82.camel@kernel.crashing.org> References: <20170126212248.3f3e9103@kitsune.suse.cz> <1485481819.2980.82.camel@kernel.crashing.org> From: Michal Suchanek Date: Fri, 27 Jan 2017 10:03:20 +0100 Message-ID: Subject: Re: ibmvtpm byteswapping inconsistency To: Benjamin Herrenschmidt Cc: Tyrel Datwyler , =?UTF-8?Q?Michal_Such=C3=A1nek?= , Ashley Lai , Paul Mackerras , Michael Ellerman , Peter Huewe , Marcel Selhorst , Jarkko Sakkinen , Jason Gunthorpe , tpmdd-devel@lists.sourceforge.net, linuxppc-dev@lists.ozlabs.org, Linux Kernel Mailing List Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v0RA3sSB030595 On 27 January 2017 at 02:50, Benjamin Herrenschmidt wrote: > 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). If this should work then the below case that swaps the fields separately is broken. 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. On the other hand, bitfields are part of an integer and the field should be swapped as part of the integer. 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. 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. >> > >> > 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. Which exact PAPR? Where can I get it? The PAPR document I found does not say anything about vtpm. Thanks Michal From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ua0-x243.google.com (mail-ua0-x243.google.com [IPv6:2607:f8b0:400c:c08::243]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3v8tBX31QyzDqGy for ; Fri, 27 Jan 2017 20:04:04 +1100 (AEDT) Received: by mail-ua0-x243.google.com with SMTP id d5so24221355uag.0 for ; Fri, 27 Jan 2017 01:04:04 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1485481819.2980.82.camel@kernel.crashing.org> References: <20170126212248.3f3e9103@kitsune.suse.cz> <1485481819.2980.82.camel@kernel.crashing.org> From: Michal Suchanek Date: Fri, 27 Jan 2017 10:03:20 +0100 Message-ID: Subject: Re: ibmvtpm byteswapping inconsistency To: Benjamin Herrenschmidt Cc: Tyrel Datwyler , =?UTF-8?Q?Michal_Such=C3=A1nek?= , Ashley Lai , Paul Mackerras , Michael Ellerman , Peter Huewe , Marcel Selhorst , Jarkko Sakkinen , Jason Gunthorpe , tpmdd-devel@lists.sourceforge.net, linuxppc-dev@lists.ozlabs.org, Linux Kernel Mailing List Content-Type: text/plain; charset=UTF-8 List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On 27 January 2017 at 02:50, Benjamin Herrenschmidt wrote: > On Thu, 2017-01-26 at 17:42 -0800, Tyrel Datwyler wrote: >> On 01/26/2017 12:22 PM, Michal Such=C3=A1nek 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 =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). If this should work then the below case that swaps the fields separately is broken. 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. On the other hand, bitfields are part of an integer and the field should be swapped as part of the integer. 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. 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. >> > >> > 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 =3D (__be64 *)&crq; >> > ... >> > crq.valid =3D (u8)IBMVTPM_VALID_CMD; >> > crq.msg =3D (u8)VTPM_TPM_COMMAND; >> > crq.len =3D cpu_to_be16(count); >> > crq.data =3D cpu_to_be32(ibmvtpm->rtce_dma_handle); >> > >> > and submitted with >> > >> > rc =3D 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. Which exact PAPR? Where can I get it? The PAPR document I found does not say anything about vtpm. Thanks Michal From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Suchanek Subject: Re: ibmvtpm byteswapping inconsistency Date: Fri, 27 Jan 2017 10:03:20 +0100 Message-ID: References: <20170126212248.3f3e9103@kitsune.suse.cz> <1485481819.2980.82.camel@kernel.crashing.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <1485481819.2980.82.camel@kernel.crashing.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@lists.ozlabs.org Sender: "Linuxppc-dev" To: Benjamin Herrenschmidt Cc: Marcel Selhorst , Linux Kernel Mailing List , Jarkko Sakkinen , Jason Gunthorpe , tpmdd-devel@lists.sourceforge.net, Paul Mackerras , Tyrel Datwyler , Ashley Lai , Peter Huewe , =?UTF-8?Q?Michal_Such=C3=A1nek?= , linuxppc-dev@lists.ozlabs.org List-Id: tpmdd-devel@lists.sourceforge.net On 27 January 2017 at 02:50, Benjamin Herrenschmidt wrote: > On Thu, 2017-01-26 at 17:42 -0800, Tyrel Datwyler wrote: >> On 01/26/2017 12:22 PM, Michal Such=C3=A1nek 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 =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). If this should work then the below case that swaps the fields separately is broken. 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. On the other hand, bitfields are part of an integer and the field should be swapped as part of the integer. 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. 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. >> > >> > 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 =3D (__be64 *)&crq; >> > ... >> > crq.valid =3D (u8)IBMVTPM_VALID_CMD; >> > crq.msg =3D (u8)VTPM_TPM_COMMAND; >> > crq.len =3D cpu_to_be16(count); >> > crq.data =3D cpu_to_be32(ibmvtpm->rtce_dma_handle); >> > >> > and submitted with >> > >> > rc =3D 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. Which exact PAPR? Where can I get it? The PAPR document I found does not say anything about vtpm. Thanks Michal