From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [216.205.24.74]) by mx.groups.io with SMTP id smtpd.web12.10716.1585313726343692963 for ; Fri, 27 Mar 2020 05:55:26 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Ga6DSOa0; spf=pass (domain: redhat.com, ip: 216.205.24.74, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585313725; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=U+1QCdpE55IiYnbf5BXVVZU37MLiGalrApdCfdVnJmM=; b=Ga6DSOa0Wl40Fwm/GbzHbGuYpCCBFfV7lSG544Tb9dh2pQLhejpS3bsMAgJmLsGLchKxHi VrMowh16+trjQ/yUCPTkQbLg+LuDfES3rRoJIUZZgncoJCCGHBrvh05yFbCIkkOZ8US9Ja m0PTrqw/Opa4OlJ4EH916W0WsXqqZqk= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-159-bFMdhUzyOQeKii7vxgXtSg-1; Fri, 27 Mar 2020 08:55:21 -0400 X-MC-Unique: bFMdhUzyOQeKii7vxgXtSg-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B8F011005512; Fri, 27 Mar 2020 12:55:19 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-36.ams2.redhat.com [10.36.114.36]) by smtp.corp.redhat.com (Postfix) with ESMTP id 327B519757; Fri, 27 Mar 2020 12:55:18 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init To: Liran Alon , 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> <7c1b4c39-e588-5547-883b-ad3b06a39bba@oracle.com> From: "Laszlo Ersek" Message-ID: <32e19272-f077-c37a-ccec-526707b78d50@redhat.com> Date: Fri, 27 Mar 2020 13:55:16 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <7c1b4c39-e588-5547-883b-ad3b06a39bba@oracle.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable On 03/25/20 18:13, Liran Alon wrote: >=20 > 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 EDK2 >>>> 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 agree >>> 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 i= nterface >>> =A0=A0=A0=A0 definitions, and Table 6 lists their modifiers. Unless oth= erwise >>> =A0=A0=A0=A0 specified all data types are naturally aligned. Structures= 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 achie= ve >>> 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 would >>> normally guarantee the proper alignment. *But*, because the structure i= s >>> packed, I don't think we can rely on the spec's description (cf. "unles= s >>> otherwise specified"). >>> >>> So, in theory, there are two options: >>> >>> - drop the packing (and rely on the natural alignment providing what yo= u >>> =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 documentatio= n >>> 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 >> to do this change for v3 as-well. >> >> Thanks. >> > Actually, I'm not sure I understand how this union approach helps with > anything. > Isn't the PVSCSI_CMD_DESC_SETUP_RINGS structure already aligned because > it have only UINT32 and UINT64 fields? Yes, that would be sufficient, due to the UEFI spec, and due to edk2 conforming to the UEFI spec the best it can. Except, the structure is packed. > And if alignment is not guaranteed, how does putting it in a union > together with another UINT32 provides the required alignment it didn't > had before? > Because the union itself is not marked with packed(), in contrast to > PVSCSI_CMD_DESC_SETUP_RINGS? Exactly. Laszlo