From: "Liran Alon" <liran.alon@oracle.com>
To: Laszlo Ersek <lersek@redhat.com>, devel@edk2.groups.io
Cc: nikita.leshchenko@oracle.com, aaron.young@oracle.com,
jordan.l.justen@intel.com, ard.biesheuvel@linaro.org
Subject: Re: [edk2-devel] [PATCH 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init
Date: Wed, 25 Mar 2020 18:40:32 +0200 [thread overview]
Message-ID: <c09baa3d-4c02-4d7b-5f70-c3a1621d69be@oracle.com> (raw)
In-Reply-To: <5833e06c-bb10-5c8c-1827-25e723b5834e@redhat.com>
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
next prev parent reply other threads:[~2020-03-25 16:40 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-16 15:00 [PATCH 00/17]: OvmfPkg: Support booting from VMware PVSCSI controller liran.alon
2020-03-16 15:00 ` [PATCH 01/17] OvmfPkg/PvScsiDxe: Create empty driver Liran Alon
2020-03-24 11:15 ` [edk2-devel] " Laszlo Ersek
2020-03-16 15:00 ` [PATCH 02/17] OvmfPkg/PvScsiDxe: Install DriverBinding protocol Liran Alon
2020-03-24 11:23 ` [edk2-devel] " Laszlo Ersek
2020-03-16 15:00 ` [PATCH 03/17] OvmfPkg/PvScsiDxe: Report name of driver Liran Alon
2020-03-24 11:29 ` [edk2-devel] " Laszlo Ersek
2020-03-16 15:01 ` [PATCH 04/17] OvmfPkg/PvScsiDxe: Probe PCI devices and look for PvScsi Liran Alon
2020-03-24 11:41 ` [edk2-devel] " Laszlo Ersek
2020-03-16 15:01 ` [PATCH 05/17] OvmfPkg/PvScsiDxe: Install stubbed EXT_SCSI_PASS_THRU Liran Alon
2020-03-24 12:27 ` [edk2-devel] " Laszlo Ersek
2020-03-24 12:47 ` Laszlo Ersek
2020-03-16 15:01 ` [PATCH 06/17] OvmfPkg/PvScsiDxe: Report the number of targets and LUNs Liran Alon
2020-03-24 13:12 ` [edk2-devel] " Laszlo Ersek
2020-03-16 15:01 ` [PATCH 07/17] OvmfPkg/PvScsiDxe: Translate Target & LUN to/from DevicePath Liran Alon
2020-03-24 13:36 ` [edk2-devel] " Laszlo Ersek
2020-03-16 15:01 ` [PATCH 08/17] OvmfPkg/PvScsiDxe: Open PciIo protocol for later use Liran Alon
2020-03-24 13:47 ` [edk2-devel] " Laszlo Ersek
2020-03-16 15:01 ` [PATCH 09/17] OvmfPkg/PvScsiDxe: Backup/Restore PCI attributes on Init/UnInit Liran Alon
2020-03-24 15:14 ` [edk2-devel] " Laszlo Ersek
2020-03-24 15:35 ` Liran Alon
2020-03-25 1:48 ` Laszlo Ersek
2020-03-25 10:32 ` Liran Alon
2020-03-16 15:01 ` [PATCH 10/17] OvmfPkg/PvScsiDxe: Enable IOSpace & Bus-Mastering in PCI attributes Liran Alon
2020-03-24 15:22 ` [edk2-devel] " Laszlo Ersek
2020-03-16 15:01 ` [PATCH 11/17] OvmfPkg/PvScsiDxe: Define device interface structures and constants Liran Alon
2020-03-24 15:35 ` [edk2-devel] " Laszlo Ersek
2020-03-24 16:34 ` Laszlo Ersek
2020-03-16 15:01 ` [PATCH 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init Liran Alon
2020-03-24 16:00 ` [edk2-devel] " Laszlo Ersek
2020-03-25 1:11 ` Liran Alon
2020-03-25 16:31 ` Laszlo Ersek
2020-03-25 16:40 ` Liran Alon [this message]
2020-03-25 17:13 ` Liran Alon
2020-03-27 12:55 ` Laszlo Ersek
2020-03-16 15:01 ` [PATCH 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings Liran Alon
2020-03-24 16:11 ` [edk2-devel] " Laszlo Ersek
2020-03-16 15:01 ` [PATCH 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer Liran Alon
2020-03-24 16:13 ` [edk2-devel] " Laszlo Ersek
2020-03-16 15:01 ` [PATCH 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response Liran Alon
2020-03-24 16:43 ` [edk2-devel] " Laszlo Ersek
2020-03-25 1:17 ` Liran Alon
2020-03-16 15:01 ` [PATCH 16/17] OvmfPkg/PvScsiDxe: Reset device on ExitBootServices() Liran Alon
2020-03-24 17:04 ` [edk2-devel] " Laszlo Ersek
2020-03-16 15:01 ` [PATCH 17/17] OvmfPkg/PvScsiDxe: Enable device 64-bit DMA addresses Liran Alon
2020-03-24 15:26 ` [edk2-devel] " Laszlo Ersek
2020-03-23 16:33 ` [edk2-devel] [PATCH 00/17]: OvmfPkg: Support booting from VMware PVSCSI controller Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c09baa3d-4c02-4d7b-5f70-c3a1621d69be@oracle.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox