From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [63.128.21.74]) by mx.groups.io with SMTP id smtpd.web11.5544.1585055604387951327 for ; Tue, 24 Mar 2020 06:13:24 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=HV2D1MMb; spf=pass (domain: redhat.com, ip: 63.128.21.74, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585055603; 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=hbUChtSghvZ5AYwwgBO9GYqYiDZ2kdZn/JV1J4UzEyc=; b=HV2D1MMb+g6nF1voAy9/mJXSRfiPmAxIvgaQrfR7VRKeLra19GcYBhinGqIsIVuUcJcCIC ydGNNX/o+A5G2cC8DF9V0kGLUB1mHNalL/2L2vF6+BiLXSc1OmJck1GW5jC985NfgpT24q v2ymXwPxHLGyMKbWBTdg111r2pJM+N0= 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-2-1XI5xkKlNOu0alWNd50trg-1; Tue, 24 Mar 2020 09:13:13 -0400 X-MC-Unique: 1XI5xkKlNOu0alWNd50trg-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AB9AD477; Tue, 24 Mar 2020 13:13:11 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-139.ams2.redhat.com [10.36.115.139]) by smtp.corp.redhat.com (Postfix) with ESMTP id BDC755D9C5; Tue, 24 Mar 2020 13:13:06 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 06/17] OvmfPkg/PvScsiDxe: Report the number of targets and LUNs To: devel@edk2.groups.io, liran.alon@oracle.com 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-7-liran.alon@oracle.com> From: "Laszlo Ersek" Message-ID: <2b94cb7e-2c67-c27d-38ad-d5d070044d60@redhat.com> Date: Tue, 24 Mar 2020 14:12:52 +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: <20200316150113.104630-7-liran.alon@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 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/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 > Signed-off-by: Liran Alon > --- > 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 > #include > +#include > #include > #include > #include > @@ -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 Thanks, Laszlo