From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750935AbdA1Axc (ORCPT ); Fri, 27 Jan 2017 19:53:32 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:39566 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750987AbdA1AxZ (ORCPT ); Fri, 27 Jan 2017 19:53:25 -0500 Subject: Re: ibmvtpm byteswapping inconsistency To: Michal Suchanek , Benjamin Herrenschmidt References: <20170126212248.3f3e9103@kitsune.suse.cz> <1485481819.2980.82.camel@kernel.crashing.org> Cc: Marcel Selhorst , Linux Kernel Mailing List , Jarkko Sakkinen , Jason Gunthorpe , tpmdd-devel@lists.sourceforge.net, Paul Mackerras , Ashley Lai , Peter Huewe , =?UTF-8?Q?Michal_Such=c3=a1nek?= , linuxppc-dev@lists.ozlabs.org From: Tyrel Datwyler Date: Fri, 27 Jan 2017 13:19:13 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 17012721-0028-0000-0000-000006E5FBFB X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006508; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000201; SDB=6.00813609; UDB=6.00396921; IPR=6.00591025; BA=6.00005093; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00014078; XFM=3.00000011; UTC=2017-01-27 21:19:19 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17012721-0029-0000-0000-00003316AAA9 Message-Id: <73c1e5be-0820-8dca-c86a-8cf3ffeb5efe@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-01-27_15:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1612050000 definitions=main-1701270207 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/27/2017 01:03 AM, Michal Suchanek wrote: > 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. 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))); > >>>> >>>> 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. Unfortunately, vtpm doesn't appear to be covered in the publicly available LoPAPR. -Tyrel > > Thanks > > Michal >