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.16190.1585154439745768664 for ; Wed, 25 Mar 2020 09:40:39 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=sEZzf6Q0; 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 02PGdThG156734; Wed, 25 Mar 2020 16:40:39 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : to : cc : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=corp-2020-01-29; bh=97acxpMizZOWAoD4UO/41Ip4W3EzmtpOjquXDvq3bmc=; b=sEZzf6Q08GIYrREz9wbb/MkbD6gtWCQ0SaGPYBY1sA6EHmjzlE+C/EG2Qnyh6lx/LZCy JaviVcEhmfzt38eqEFI0HkhX7QzvgPEUbE4GlRL1BAcX/RIHdSINTWzSU39+y8lLbIV9 670ncRbBVz7Gx5Azc/+fBfmUpLosP78FJeAurXgCArM0jyfk6+ybJAxnq+L8j37Q7Gwv ZIpuTBJAXDQVn3IKg4FUs9VFVV3w46VLgKsHfBywP/klZurIM88sDxoNEGH4AmtDN9j2 jtarAbpY4O2WR8raOeGlIUK/wS+Gs+dMYlNTse7OfeqGClSAO+Soc4WdUflgT491JF4G KA== Received: from aserp3020.oracle.com (aserp3020.oracle.com [141.146.126.70]) by userp2130.oracle.com with ESMTP id 2ywabraug2-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 25 Mar 2020 16:40:39 +0000 Received: from pps.filterd (aserp3020.oracle.com [127.0.0.1]) by aserp3020.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 02PGcZi6043269; Wed, 25 Mar 2020 16:40:38 GMT Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by aserp3020.oracle.com with ESMTP id 30073ambtw-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 25 Mar 2020 16:40:38 +0000 Received: from abhmp0005.oracle.com (abhmp0005.oracle.com [141.146.116.11]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id 02PGeasj018849; Wed, 25 Mar 2020 16:40:36 GMT Received: from Lirans-MacBook-Pro.local (/213.57.127.2) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 25 Mar 2020 09:40:36 -0700 Subject: Re: [edk2-devel] [PATCH 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init 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> From: "Liran Alon" Message-ID: Date: Wed, 25 Mar 2020 18:40:32 +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: <5833e06c-bb10-5c8c-1827-25e723b5834e@redhat.com> X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9571 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 adultscore=0 suspectscore=0 mlxscore=0 phishscore=0 bulkscore=0 spamscore=0 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003250133 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-2003250133 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US On 25/03/2020 18:31, Laszlo Ersek wrote: > On 03/25/20 02:11, Liran Alon wrote: >> On 24/03/2020 18:00, Laszlo Ersek wrote: >>> On 03/16/20 16:01, Liran Alon wrote: >>>> +STATIC >>>> +EFI_STATUS >>>> +PvScsiWriteCmdDesc ( >>>> + IN CONST PVSCSI_DEV *Dev, >>>> + IN UINT32 Cmd, >>>> + IN VOID *Desc, >>>> + IN UINTN Length >>>> + ) >>>> +{ >>>> + EFI_STATUS Status; >>>> + UINTN LengthInWords; >>>> + UINT8 *WordPtr; >>>> + UINT8 *DescEndPtr; >>>> + UINT32 Word; >>>> + >>>> + LengthInWords = Length / sizeof (UINT32); >>> (4) What guarantees that "Length" is a whole multiple of sizeof >>> (UINT32)? >> Nothing. >> Besides the fact that all commands passed to this function are indeed >> multiple of sizeof (UINT32). >> >>> In this review I have not insisted on including full-blown interface >>> contracts in the top-level function comment blocks (with @param[in] >>> and @retval etc). >> Thanks for that. I think too it would be an overkill with little >> value. >>> But, for this function, it really is unclear. >> Will it be sufficient for you if I just replace the prototype with >> something like the following? >> >> /** >> Send PVSCSI command to device >> **/ >> STATIC >> EFI_STATUS >> PvScsiWriteCmdDesc ( >> IN CONST PVSCSI_DEV *Dev, >> IN UINT32 Cmd, >> IN VOID *Desc, >> IN UINTN LengthInWords // Note: Word is UINT32 >> ) > The comment "// Note: ..." on a particular parameter is really > non-idiomatic. So please either (a) rename "LengthInWords" so that > "UINT32" is obvious from it, or (for this one particular function) (b) > please do add a full-blown function-level comment, with @param[in] and > @return / @retval markers. I took a different approach eventually in v2 which I think you will like. See v2 patch-series for more information. > >>>> + >>>> + if (LengthInWords > PVSCSI_MAX_CMD_DATA_WORDS) { >>>> + return EFI_INVALID_PARAMETER; >>>> + } >>>> + >>>> + Status = PvScsiMmioWrite32 (Dev, PVSCSI_REG_OFFSET_COMMAND, Cmd); >>>> + if (EFI_ERROR (Status)) { >>>> + return Status; >>>> + } >>>> + >>>> + WordPtr = Desc; >>>> + DescEndPtr = WordPtr + Length; >>>> + >>>> + while (WordPtr != DescEndPtr) { >>>> + // >>>> + // CopyMem() is used to avoid strict-aliasing issues >>>> + // >>> (5) In edk2, we -- completely intentionally -- disable the >>> enforcement of the effective type rules / strict aliasing rules. See >>> "-fno-strict-aliasing" in "BaseTools/Conf/tools_def.template". >>>> + CopyMem (&Word, WordPtr, sizeof (UINT32)); >>>> + >>>> + Status = PvScsiMmioWrite32 (Dev, PVSCSI_REG_OFFSET_COMMAND_DATA, >>>> Word); >>>> + if (EFI_ERROR (Status)) { >>>> + return Status; >>>> + } >>>> + >>>> + WordPtr += sizeof (UINT32); >>>> + } >>>> + >>>> + return EFI_SUCCESS; >>>> +} >>> (6) I think the open-coded loop is suboptimal -- the PciIo protocol >>> seems to offer EfiPciIoWidthFifoUint32 for exactly this purpose (= >>> advance in the memory buffer, while accessing the same offset in the >>> BAR). >>> >>> Have you perhaps tried that? >> I actually haven't noticed EfiPciIoWidthFifoUint32 until you mentioned >> it. >> As it seems there isn't even a single line of code in EDK2 that use >> it. :) > "MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c" seems to use it, > doesn't it? From commit 9bfaa3da1ee5 ("MdeModulePkg/SdMmcPciHcDxe: Fix > PIO transfer mode", 2020-03-05). > >> In fact, the only code that use one of the EfiPciIoWidthFifo* is >> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c. >>> (I can imagine that you ruled it out, due to "Desc" being unaligned. >>> The UEFI spec does say, "The caller is responsible for any alignment >>> and I/O width issues which the bus, device, platform, or type of I/O >>> might require.) >> Why is this an issue? > If it's not an issue for you, it's definitely not an issue for me. :) I > didn't know how difficult it would be for you to satisfy such an > alignment requirement, at all the call sites. > >> It's easy to document with one-line comment at end of Desc parameter >> deceleration that it must be aligned. > Documenting the requirement is good, but *if* you do that, you can > *only* do it with a @param[in] marker. > >> It's also not difficult to modify callers as only a single caller >> actually pass a descriptor (The caller PvScsiInitRings()). > OK. > >> 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 { > PVSCSI_CMD_DESC_SETUP_RINGS Cmd; > UINT32 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: > > 2.3.1 Data Types > > Table 5 lists the common data types that are used in the interface > definitions, and Table 6 lists their modifiers. Unless otherwise > specified all data types are naturally aligned. Structures are > aligned on boundaries equal to the largest internal datum of the > structure and internal data are implicitly padded to achieve natural > 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 is > packed, I don't think we can rely on the spec's description (cf. "unless > otherwise specified"). > > So, in theory, there are two options: > > - drop the packing (and rely on the natural alignment providing what you > need anyway), > > - keep the packing, and use other methods to guarantee struct-level > alignment (such as the above union). > > I prefer keeping the packing, if for nothing else then for documentation > 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. > >> In addition, I assume I don't need to add any validation of alignment >> to PvScsiWriteCmdDesc(). > No, I don't think that's necessary. I think the generic edk2 machinery > rejects a misaligned memory buffer explicitly, as follows: > > (a) The PciIo->Mem.Read/Write members are implemented in PciIoMemRead() > / PciIoMemWrite() [MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c]. > > Two comments about them: > > - These functions break up unaligned requests into byte-size > requests if "PcdUnalignedPciIoEnable" is TRUE. (Note: it is FALSE > for OvmfPkg, and we should not change that.) > > - These functions do not actually check the alignment of either > "Buffer" or the MMIO "Offset" within the BAR. They just delegate > to PciRootBridgeIo->Mem.Read() and PciRootBridgeIo->Mem.Write(). > > (b) For those, we need to look at RootBridgeIoMemRead() and > RootBridgeIoMemWrite(), in > "MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c". > > Both of these functions call RootBridgeIoCheckParameter(), > RootBridgeIoCheckParameter() -- which checks the alignment on the > MMIO "Address" within the BAR (--> EFI_UNSUPPORTED), but still > doesn't check the alignment of "Buffer". > > So, let's continue analyzing RootBridgeIoMemRead() and > RootBridgeIoMemWrite()... Once RootBridgeIoCheckParameter() is > happy, the work is delegated to CpuIo->Mem.Read/Write. > > (c) For those, we need to look at CpuMemoryServiceRead() and > CpuMemoryServiceWrite(), in "UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.c". > > These functions call CpuIoCheckParameter(). And that function > finally does seem to check the alignment of "Buffer": > > // > // Check to see if Buffer is aligned > // (IA-32 allows UINT64 and INT64 data types to be 32-bit aligned.) > // > if (((UINTN)Buffer & ((MIN (sizeof (UINTN), mInStride[Width]) - 1))) != 0) { > return EFI_UNSUPPORTED; > } > > (See related commit 36de860619c2, "Update the check condition to > allows UINT64 and INT64 data types to be 32-bit aligned on IA32 > system.", 2011-12-01.) > > Thus I believe any mis-alignment in "Desc" would be caught for you. That's what I thought as-well. Thanks for all the details. :) -Liran