From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from userp2130.oracle.com (userp2130.oracle.com [156.151.31.86]) by mx.groups.io with SMTP id smtpd.web11.17206.1585156398448088597 for ; Wed, 25 Mar 2020 10:13:18 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@oracle.com header.s=corp-2020-01-29 header.b=pR2I/VjT; spf=pass (domain: oracle.com, ip: 156.151.31.86, mailfrom: liran.alon@oracle.com) Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 02PH9KWk165205; Wed, 25 Mar 2020 17:13:18 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : from : to : cc : references : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=corp-2020-01-29; bh=EpqN3OQ+VBnAlYquFBZoVBLLfDys8cw8NRW0sjEzNZA=; b=pR2I/VjT4xHikAeXmwkW2YQ71EGNAtNPWqHZ+6SVLc/u7EzyvTenRwegZzh8p9xO8+9a LxSZFOZ/tVfNNTU48DGcytWr2exp3oOr6gxzRiioSJwaEhhFkUUzcFu9WvxTZ6tfTITv Pa1Dz0rPnWeu+jmDaxYlCfj51xd+krHdxw3/leUTlsU8DJom8aKPqnMK+6Z/0hQgXWxT /OiutPPMup7f9CjmcK8Ckie9a9jDiosHt88c8hBO7wzeUNzuYxNS9XG8suba1yUHveyN 9OFoIRyR9y5++IuXqo6OrfVgfx3FDQUvPVYcNmhiRW1GuGBG1anMY2tqRVzwcFZfiknU zw== Received: from aserp3030.oracle.com (aserp3030.oracle.com [141.146.126.71]) by userp2130.oracle.com with ESMTP id 2ywabrb12g-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 25 Mar 2020 17:13:15 +0000 Received: from pps.filterd (aserp3030.oracle.com [127.0.0.1]) by aserp3030.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 02PH7pw6096924; Wed, 25 Mar 2020 17:13:15 GMT Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by aserp3030.oracle.com with ESMTP id 3006r6xe97-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 25 Mar 2020 17:13:14 +0000 Received: from abhmp0016.oracle.com (abhmp0016.oracle.com [141.146.116.22]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 02PHDDJw031978; Wed, 25 Mar 2020 17:13:13 GMT Received: from Lirans-MacBook-Pro.local (/213.57.127.2) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 25 Mar 2020 10:13:13 -0700 Subject: Re: [edk2-devel] [PATCH 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init From: "Liran Alon" To: Laszlo Ersek , devel@edk2.groups.io Cc: nikita.leshchenko@oracle.com, aaron.young@oracle.com, jordan.l.justen@intel.com, ard.biesheuvel@linaro.org References: <20200316150113.104630-1-liran.alon@oracle.com> <20200316150113.104630-13-liran.alon@oracle.com> <5833e06c-bb10-5c8c-1827-25e723b5834e@redhat.com> Message-ID: <7c1b4c39-e588-5547-883b-ad3b06a39bba@oracle.com> Date: Wed, 25 Mar 2020 19:13:10 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9571 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 adultscore=0 suspectscore=0 phishscore=0 spamscore=0 mlxscore=0 mlxlogscore=999 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003250138 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9571 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0 lowpriorityscore=0 malwarescore=0 phishscore=0 priorityscore=1501 clxscore=1015 adultscore=0 mlxscore=0 mlxlogscore=999 bulkscore=0 impostorscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003250138 X-MIME-Autoconverted: from 8bit to quoted-printable by userp2130.oracle.com id 02PH9KWk165205 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 25/03/2020 18:40, Liran Alon wrote: > > On 25/03/2020 18:31, Laszlo Ersek wrote: >> On 03/25/20 02:11, Liran Alon wrote: >>> To avoid further style comments, what is the coding convention in EDK= 2 >>> to align the "PVSCSI_CMD_DESC_SETUP_RINGS Cmd;" var properly? >> The best I can recommend off-hand is: >> >> union { >> =A0=A0 PVSCSI_CMD_DESC_SETUP_RINGS Cmd; >> =A0=A0 UINT32=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0 Uint32; >> } AlignAtUint32; >> >> Perhaps someone else can recommend something less awkward. >> >> Note: PVSCSI_CMD_DESC_SETUP_RINGS is a packed structure (and I do agre= e >> that's good, if at least for documentation purposes). If it weren't >> packed, then the following passage from the UEFI spec would apply: >> >> =A0=A0=A0=A0 2.3.1 Data Types >> >> =A0=A0=A0=A0 Table 5 lists the common data types that are used in the = interface >> =A0=A0=A0=A0 definitions, and Table 6 lists their modifiers. Unless ot= herwise >> =A0=A0=A0=A0 specified all data types are naturally aligned. Structure= s are >> =A0=A0=A0=A0 aligned on boundaries equal to the largest internal datum= of the >> =A0=A0=A0=A0 structure and internal data are implicitly padded to achi= eve=20 >> natural >> =A0=A0=A0=A0 alignment. >> >> Because PVSCSI_CMD_DESC_SETUP_RINGS only contains members with types >> listed in Table 5 (namely, UINT32 and UINT64), the above language woul= d >> normally guarantee the proper alignment. *But*, because the structure = is >> packed, I don't think we can rely on the spec's description (cf. "unle= ss >> otherwise specified"). >> >> So, in theory, there are two options: >> >> - drop the packing (and rely on the natural alignment providing what y= ou >> =A0=A0 need anyway), >> >> - keep the packing, and use other methods to guarantee struct-level >> =A0=A0 alignment (such as the above union). >> >> I prefer keeping the packing, if for nothing else then for documentati= on >> purposes (it says "wire format" loud and clear). If you use the union >> above, I'll be OK with it. > Ok. I will use this union approach. > Unfortunately, I have seen this comment only after submitting v2. > So I will wait for the rest of your v2 review comments and make sure=20 > to do this change for v3 as-well. > > Thanks. > Actually, I'm not sure I understand how this union approach helps with=20 anything. Isn't the PVSCSI_CMD_DESC_SETUP_RINGS structure already aligned because=20 it have only UINT32 and UINT64 fields? And if alignment is not guaranteed, how does putting it in a union=20 together with another UINT32 provides the required alignment it didn't=20 had before? Because the union itself is not marked with packed(), in contrast to=20 PVSCSI_CMD_DESC_SETUP_RINGS? -Liran