The following commits will complete the implementation of
device initialization.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/PvScsiDxe/PvScsi.c | 77 ++++++++++++++++++++++++++++++++++++++
1 file changed, 77 insertions(+)
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index ff6b50b7020f..fb2407d2adb2 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -29,6 +29,76 @@
// Ext SCSI Pass Thru utilities
//
+
+//
+// Writes a 32-bit value into BAR0 using MMIO
+//
+STATIC
+EFI_STATUS
+PvScsiMmioWrite32 (
+ IN CONST PVSCSI_DEV *Dev,
+ IN UINT64 Offset,
+ IN UINT32 Value
+ )
+{
+ return Dev->PciIo->Mem.Write(
+ Dev->PciIo,
+ EfiPciIoWidthUint32,
+ 0, // BarIndex
+ Offset,
+ 1, // Count
+ &Value
+ );
+}
+
+//
+// Send PVSCSI command to device
+//
+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);
+
+ 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
+ //
+ 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;
+}
//
// Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and
// EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized
@@ -348,6 +418,13 @@ PvScsiInit (
return Status;
}
+ //
+ // Reset adapter
+ //
+ Status = PvScsiWriteCmdDesc (Dev, PVSCSI_CMD_ADAPTER_RESET, NULL, 0);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
//
// Populate the exported interface's attributes
//
--
2.20.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55878): https://edk2.groups.io/g/devel/message/55878
Mute This Topic: https://groups.io/mt/72001280/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
a bit more superficial comments, for now:
On 03/16/20 16:01, Liran Alon wrote:
> The following commits will complete the implementation of
> device initialization.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
> OvmfPkg/PvScsiDxe/PvScsi.c | 77 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 77 insertions(+)
>
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index ff6b50b7020f..fb2407d2adb2 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -29,6 +29,76 @@
> // Ext SCSI Pass Thru utilities
> //
>
> +
> +//
> +// Writes a 32-bit value into BAR0 using MMIO
> +//
(1) Please use the /** **/ comment style for top-level function comment
blocks.
> +STATIC
> +EFI_STATUS
> +PvScsiMmioWrite32 (
> + IN CONST PVSCSI_DEV *Dev,
> + IN UINT64 Offset,
> + IN UINT32 Value
> + )
> +{
> + return Dev->PciIo->Mem.Write(
> + Dev->PciIo,
> + EfiPciIoWidthUint32,
> + 0, // BarIndex
(2) Style improvement: please use the PCI_BAR_IDX0 macro.
> + Offset,
> + 1, // Count
> + &Value
> + );
> +}
> +
> +//
> +// Send PVSCSI command to device
> +//
(3) Same as (1).
> +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)?
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). But, for this function, it really is unclear.
> +
> + 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 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.)
> //
> // Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and
> // EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized
> @@ -348,6 +418,13 @@ PvScsiInit (
> return Status;
> }
>
> + //
> + // Reset adapter
> + //
> + Status = PvScsiWriteCmdDesc (Dev, PVSCSI_CMD_ADAPTER_RESET, NULL, 0);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> //
> // Populate the exported interface's attributes
> //
>
OK, so this ties back to my comments on resource management, under
patch#9.
Let me quote the PvScsiInit() function in full, after the present patch
is applied:
> STATIC
> EFI_STATUS
> PvScsiInit (
> IN OUT PVSCSI_DEV *Dev
> )
> {
> EFI_STATUS Status;
>
> //
> // Init configuration
> //
> Dev->MaxTarget = PcdGet8 (PcdPvScsiMaxTargetLimit);
> Dev->MaxLun = PcdGet8 (PcdPvScsiMaxLunLimit);
>
> //
> // Set PCI Attributes
> //
> Status = PvScsiSetPCIAttributes (Dev);
> if (EFI_ERROR (Status)) {
> return Status;
> }
>
> //
> // Reset adapter
> //
> Status = PvScsiWriteCmdDesc (Dev, PVSCSI_CMD_ADAPTER_RESET, NULL, 0);
> if (EFI_ERROR (Status)) {
> return Status;
> }
(7) So this is precisely the spot where we have to jump to a new error
handling label -- called "RestorePciAttributes" --, to be introduced at
the end of the function. And we need to call
PvScsiRestorePciAttributes() there.
Otherwise, the original PCI attributes will never be restored.
Thanks!
Laszlo
On 03/16/20 16:01, Liran Alon wrote:
> //
> // Populate the exported interface's attributes
> //
> Dev->PassThru.Mode = &Dev->PassThruMode;
> Dev->PassThru.PassThru = &PvScsiPassThru;
> Dev->PassThru.GetNextTargetLun = &PvScsiGetNextTargetLun;
> Dev->PassThru.BuildDevicePath = &PvScsiBuildDevicePath;
> Dev->PassThru.GetTargetLun = &PvScsiGetTargetLun;
> Dev->PassThru.ResetChannel = &PvScsiResetChannel;
> Dev->PassThru.ResetTargetLun = &PvScsiResetTargetLun;
> Dev->PassThru.GetNextTarget = &PvScsiGetNextTarget;
>
> //
> // AdapterId is a target for which no handle will be created during bus scan.
> // Prevent any conflict with real devices.
> //
> Dev->PassThruMode.AdapterId = MAX_UINT32;
>
> //
> // Set both physical and logical attributes for non-RAID SCSI channel
> //
> Dev->PassThruMode.Attributes = EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL |
> EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_LOGICAL;
>
> //
> // No restriction on transfer buffer alignment
> //
> Dev->PassThruMode.IoAlign = 0;
>
> return EFI_SUCCESS;
> }
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56162): https://edk2.groups.io/g/devel/message/56162
Mute This Topic: https://groups.io/mt/72001280/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
)
>> +
>> + 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. :)
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?
It's easy to document with one-line comment at end of Desc parameter
deceleration that it must be aligned.
It's also not difficult to modify callers as only a single caller
actually pass a descriptor (The caller PvScsiInitRings()).
To avoid further style comments, what is the coding convention in EDK2
to align the "PVSCSI_CMD_DESC_SETUP_RINGS Cmd;" var properly?
In addition, I assume I don't need to add any validation of alignment to
PvScsiWriteCmdDesc().
-Liran
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56227): https://edk2.groups.io/g/devel/message/56227
Mute This Topic: https://groups.io/mt/72001280/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56326): https://edk2.groups.io/g/devel/message/56326
Mute This Topic: https://groups.io/mt/72001280/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56328): https://edk2.groups.io/g/devel/message/56328
Mute This Topic: https://groups.io/mt/72001280/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 {
>> 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.
>
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?
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?
-Liran
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56331): https://edk2.groups.io/g/devel/message/56331
Mute This Topic: https://groups.io/mt/72001280/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 03/25/20 18:13, Liran Alon wrote:
>
> 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 {
>>> 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.
>>
> 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56489): https://edk2.groups.io/g/devel/message/56489
Mute This Topic: https://groups.io/mt/72001280/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.