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.15828.1585153897592743539 for ; Wed, 25 Mar 2020 09:31:37 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=bykMqOjg; 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=1585153896; 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=jDXoEQGKpw8LHEwFKlOTIGxMmpCeuEA880a3mT7ENdg=; b=bykMqOjgL23KLhA3tE/kuUJeO9C0XScsy6CT7ixQ4PGuUqQibfR1wT8xcVns0qc2Cp6Tc+ PbxQ4OL0VsGOPgWW81M/lUScbrxS+UXmFdHpx3d21iOyiOo3yS2FPGKysmOR2o5fg0XQx9 Yqdw237g0oTmSQgrKHlR8Ij6Dy6fx1s= 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-215-nKDS-wLaMJmaJ-ikkkjn6A-1; Wed, 25 Mar 2020 12:31:33 -0400 X-MC-Unique: nKDS-wLaMJmaJ-ikkkjn6A-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 464401005509; Wed, 25 Mar 2020 16:31:32 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-153.ams2.redhat.com [10.36.113.153]) by smtp.corp.redhat.com (Postfix) with ESMTP id A9C1260BE0; Wed, 25 Mar 2020 16:31:30 +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> From: "Laszlo Ersek" Message-ID: <5833e06c-bb10-5c8c-1827-25e723b5834e@redhat.com> Date: Wed, 25 Mar 2020 17:31:29 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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. >>> + >>> + 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. > 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. Thanks, Laszlo