[edk2-devel] [PATCH 06/17] OvmfPkg/PvScsiDxe: Report the number of targets and LUNs
Laszlo Ersek
lersek at redhat.com
Tue Mar 24 13:12:52 UTC 2020
On 03/16/20 16:01, Liran Alon wrote:
> Implement EXT_SCSI_PASS_THRU.GetNextTarget() and
> EXT_SCSI_PASS_THRU.GetNextTargetLun().
>
> ScsiBusDxe scans all MaxTarget * MaxLun possible devices.
> This can take unnecessarily long for large number of targets.
> To deal with this, VirtioScsiDxe has defined PCDs to limit the
> MaxTarget & MaxLun to desired values which gives sufficient
> performance. It is very important in virtio-scsi as it can have
> very big MaxTarget & MaxLun.
> Even though a common PVSCSI device has a default MaxTarget=64 and
> MaxLun=0, we implement similar mechanism as virtio-scsi for completeness.
> This may be useful in the future when PVSCSI will have bigger values
> for MaxTarget and MaxLun.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Reviewed-by: Nikita Leshenko <nikita.leshchenko at oracle.com>
> Signed-off-by: Liran Alon <liran.alon at oracle.com>
> ---
> OvmfPkg/OvmfPkg.dec | 9 +++
> OvmfPkg/PvScsiDxe/PvScsi.c | 122 ++++++++++++++++++++++++++++++++++-
> OvmfPkg/PvScsiDxe/PvScsi.h | 2 +
> OvmfPkg/PvScsiDxe/PvScsi.inf | 5 ++
> 4 files changed, 136 insertions(+), 2 deletions(-)
>
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 4c5b6511cb97..76ce507e8bd0 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -121,6 +121,15 @@
> gUefiOvmfPkgTokenSpaceGuid.PcdVirtioScsiMaxTargetLimit|31|UINT16|6
> gUefiOvmfPkgTokenSpaceGuid.PcdVirtioScsiMaxLunLimit|7|UINT32|7
>
> + ## Sets the *inclusive* number of targets and LUNs that PvScsi exposes for
> + # scan by ScsiBusDxe.
> + # As specified above for VirtioScsi, ScsiBusDxe scans all MaxTarget * MaxLun
> + # possible devices, which can take extremely long. Thus, the blow constants
> + # are used so that scanning the number of devices given by their product
> + # is still acceptably fast.
> + gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit|64|UINT8|0x40
> + gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit|0|UINT8|0x41
> +
Three cosmetic requests:
(1) s/blow/below/
(2) in the comment block, starting with the second line, please use a
"# "
prefix, rather than
"# "
so that the actual text line up with the first line.
(3) As tokens for the new PCDs, please use 0x36 and 0x37, for keeping
the OVMF token space densely populated.
Regarding the UINT8 type for PcdPvScsiMaxTargetLimit -- the QEMU source
indeed seems to #define PVSCSI_MAX_DEVS as 64, so I guess a UINT8 should
suffice.
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 46b430a34a57..76bb361c7c94 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -11,6 +11,7 @@
>
> #include <IndustryStandard/Pci.h>
> #include <IndustryStandard/PvScsi.h>
> +#include <Library/BaseMemoryLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include <Library/UefiLib.h>
> @@ -24,6 +25,30 @@
> //
> #define PVSCSI_BINDING_VERSION 0x10
>
> +//
> +// Ext SCSI Pass Thru utilities
> +//
OK.
> +
> +//
> +// Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and
> +// EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized
> +//
(4) For leading function comment blocks, please use the following style:
/**
Blah.
**/
> +STATIC
> +BOOLEAN
> +IsTargetInitialized (
> + IN UINT8 *Target
> + )
> +{
> + UINTN Idx;
> +
> + for (Idx = 0; Idx < TARGET_MAX_BYTES; ++Idx) {
> + if (Target[Idx] != 0xFF) {
> + return TRUE;
> + }
> + }
> + return FALSE;
> +}
> +
> //
> // Ext SCSI Pass Thru
> //
> @@ -51,7 +76,54 @@ PvScsiGetNextTargetLun (
> IN OUT UINT64 *Lun
> )
> {
> - return EFI_UNSUPPORTED;
> + UINT8 *TargetPtr;
> + UINT8 LastTarget;
> + PVSCSI_DEV *Dev;
> +
> + if (Target == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + //
> + // The TargetPointer input parameter is unnecessarily a pointer-to-pointer
> + //
> + TargetPtr = *Target;
(5) Please update the comment: in this function, the subject parameter
is called "Target", not "TargetPointer".
> +
> + //
> + // If target not initialized, return first target & LUN
> + //
> + if (!IsTargetInitialized (TargetPtr)) {
> + ZeroMem (TargetPtr, TARGET_MAX_BYTES);
> + *Lun = 0;
> + return EFI_SUCCESS;
> + }
> +
> + //
> + // We only use first byte of target identifer
> + //
> + LastTarget = *TargetPtr;
> +
> + //
> + // Increment (target, LUN) pair if valid on input
> + //
> + Dev = PVSCSI_FROM_PASS_THRU (This);
> + if (LastTarget > Dev->MaxTarget || *Lun > Dev->MaxLun) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + if (*Lun < Dev->MaxLun) {
> + ++*Lun;
> + return EFI_SUCCESS;
> + }
> +
> + if (LastTarget < Dev->MaxTarget) {
> + *Lun = 0;
> + ++LastTarget;
> + *TargetPtr = LastTarget;
> + return EFI_SUCCESS;
> + }
> +
> + return EFI_NOT_FOUND;
> }
>
> STATIC
> @@ -110,7 +182,47 @@ PvScsiGetNextTarget (
> IN OUT UINT8 **Target
> )
> {
> - return EFI_UNSUPPORTED;
> + UINT8 *TargetPtr;
> + UINT8 LastTarget;
> + PVSCSI_DEV *Dev;
> +
> + if (Target == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + //
> + // The Target input parameter is unnecessarily a pointer-to-pointer
> + //
> + TargetPtr = *Target;
Got the comment right, here :)
> +
> + //
> + // If target not initialized, return first target
> + //
> + if (!IsTargetInitialized (TargetPtr)) {
> + ZeroMem (TargetPtr, TARGET_MAX_BYTES);
> + return EFI_SUCCESS;
> + }
> +
> + //
> + // We only use first byte of target identifer
> + //
> + LastTarget = *TargetPtr;
> +
> + //
> + // Increment target if valid on input
> + //
> + Dev = PVSCSI_FROM_PASS_THRU (This);
> + if (LastTarget > Dev->MaxTarget) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + if (LastTarget < Dev->MaxTarget) {
> + ++LastTarget;
> + *TargetPtr = LastTarget;
> + return EFI_SUCCESS;
> + }
> +
> + return EFI_NOT_FOUND;
> }
>
> STATIC
> @@ -119,6 +231,12 @@ PvScsiInit (
> IN OUT PVSCSI_DEV *Dev
> )
> {
> + //
> + // Init configuration
> + //
> + Dev->MaxTarget = PcdGet8 (PcdPvScsiMaxTargetLimit);
> + Dev->MaxLun = PcdGet8 (PcdPvScsiMaxLunLimit);
> +
> //
> // Populate the exported interface's attributes
> //
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
> index 3940b4c20019..dd3e0c68e6da 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
> @@ -19,6 +19,8 @@
>
> typedef struct {
> UINT32 Signature;
> + UINT8 MaxTarget;
> + UINT8 MaxLun;
> EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
> EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode;
> } PVSCSI_DEV;
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.inf b/OvmfPkg/PvScsiDxe/PvScsi.inf
> index 3a8b07872ba3..96bd4e4a9a8b 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.inf
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.inf
> @@ -25,6 +25,7 @@
> OvmfPkg/OvmfPkg.dec
>
> [LibraryClasses]
> + BaseMemoryLib
> DebugLib
> MemoryAllocationLib
> UefiBootServicesTableLib
> @@ -34,3 +35,7 @@
> [Protocols]
> gEfiPciIoProtocolGuid ## TO_START
> gEfiExtScsiPassThruProtocolGuid ## BY_START
> +
> +[Pcd]
> + gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit ## CONSUMES
> + gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit ## CONSUMES
>
(6) Please keep the list of PCDs alphabetically sorted, in this section.
With (1) through (6) addressed:
Reviewed-by: Laszlo Ersek <lersek at redhat.com>
Thanks,
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56150): https://edk2.groups.io/g/devel/message/56150
Mute This Topic: https://groups.io/mt/72001283/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list