From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754369AbdA3Ue4 (ORCPT ); Mon, 30 Jan 2017 15:34:56 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:32856 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751062AbdA3Uez (ORCPT ); Mon, 30 Jan 2017 15:34:55 -0500 Subject: Re: ibmvtpm byteswapping inconsistency To: Michael Ellerman , Tyrel Datwyler , Michal Suchanek , Benjamin Herrenschmidt References: <20170126212248.3f3e9103@kitsune.suse.cz> <1485481819.2980.82.camel@kernel.crashing.org> <73c1e5be-0820-8dca-c86a-8cf3ffeb5efe@linux.vnet.ibm.com> <87tw8hw4k7.fsf@concordia.ellerman.id.au> 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: Mon, 30 Jan 2017 12:34:45 -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: <87tw8hw4k7.fsf@concordia.ellerman.id.au> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 17013020-0040-0000-0000-0000028018BD X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006526; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000201; SDB=6.00814883; UDB=6.00397762; IPR=6.00592324; BA=6.00005100; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00014111; XFM=3.00000011; UTC=2017-01-30 20:34:52 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17013020-0041-0000-0000-000006731EB3 Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-01-30_11:,, 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-1701300192 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/29/2017 08:32 PM, Michael Ellerman wrote: > Tyrel Datwyler writes: > >> 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))); > > 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 I should have been explicit that PAPR uses Big Endian bit and byte numbering throughout the spec unless otherwise noted. -Tyrel > field. > > cheers >