From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web10.17367.1594911859473176634 for ; Thu, 16 Jul 2020 08:04:19 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=BUO88S/j; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1594911858; 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=CetkIiYAprWnJ16ip+ddUwHSigQ3hrb5LHv8k9/N9Ks=; b=BUO88S/jfnvUqW3XYm0ilSzMbtXlNFRMhBAhhBP+13Ar183VabD3+wa1x4fLrsWDamnnJs w+MzA6m6Z2cjo8i4ZLwygf+nXCkTIIkeM2u5GE94k/Pj/MVG4jz/d+N07+f86KEEXqngHb BSXYUd9y4xn7Q7v5gS7975jSte1D+Tk= 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-311-Na9NhLJ1P2W1yeDol0XQPA-1; Thu, 16 Jul 2020 11:04:16 -0400 X-MC-Unique: Na9NhLJ1P2W1yeDol0XQPA-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AF1A01800D42; Thu, 16 Jul 2020 15:04:15 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-123.ams2.redhat.com [10.36.112.123]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7174A19D61; Thu, 16 Jul 2020 15:04:14 +0000 (UTC) Subject: Re: [PATCH v2 06/12] OvmfPkg/LsiScsiDxe: Report Targets and LUNs To: Gary Lin , devel@edk2.groups.io Cc: Jordan Justen , Ard Biesheuvel References: <20200716074607.18048-1-glin@suse.com> <20200716074607.18048-7-glin@suse.com> From: "Laszlo Ersek" Message-ID: <1868905d-0420-2cc4-e692-1c48af4aa712@redhat.com> Date: Thu, 16 Jul 2020 17:04:13 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200716074607.18048-7-glin@suse.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 07/16/20 09:46, Gary Lin wrote: > Implement LsiScsiGetNextTargetLun(), LsiScsiBuildDevicePath(), > LsiScsiGetTargetLun(), and LsiScsiGetNextTarget() to report Targets and > LUNs and build the device path. > > This commit also introduces two PCD value: PcdLsiScsiMaxTargetLimit and > PcdLsiScsiMaxLunLimit as the limits for Targets and LUNs. > > v2: > - Zero out (*Target) in LsiScsiGetTargetLun() > - Use CopyMem() instead of the one-byte shortcut to copy target from > ScsiDevicePath->Pun > - Add asserts for PcdLsiScsiMaxTargetLimit and PcdLsiScsiMaxLunLimit > > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Signed-off-by: Gary Lin > --- > OvmfPkg/LsiScsiDxe/LsiScsi.c | 148 +++++++++++++++++++++++++++++- > OvmfPkg/LsiScsiDxe/LsiScsi.h | 3 + > OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf | 6 ++ > OvmfPkg/OvmfPkg.dec | 5 + > 4 files changed, 160 insertions(+), 2 deletions(-) > > diff --git a/OvmfPkg/LsiScsiDxe/LsiScsi.c b/OvmfPkg/LsiScsiDxe/LsiScsi.c > index 67fadb411e85..989bda88d209 100644 > --- a/OvmfPkg/LsiScsiDxe/LsiScsi.c > +++ b/OvmfPkg/LsiScsiDxe/LsiScsi.c > @@ -15,6 +15,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -53,6 +54,49 @@ LsiScsiGetNextTargetLun ( > IN OUT UINT64 *Lun > ) > { > + LSI_SCSI_DEV *Dev; > + UINTN Idx; > + UINT8 *Target; > + UINT16 LastTarget; > + > + // > + // the TargetPointer input parameter is unnecessarily a pointer-to-pointer > + // > + Target = *TargetPointer; > + > + // > + // Search for first non-0xFF byte. If not found, return first target & LUN. > + // > + for (Idx = 0; Idx < TARGET_MAX_BYTES && Target[Idx] == 0xFF; ++Idx) > + ; > + if (Idx == TARGET_MAX_BYTES) { > + SetMem (Target, TARGET_MAX_BYTES, 0x00); > + *Lun = 0; > + return EFI_SUCCESS; > + } > + > + CopyMem (&LastTarget, Target, sizeof LastTarget); > + > + // > + // increment (target, LUN) pair if valid on input > + // > + Dev = LSI_SCSI_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; > + CopyMem (Target, &LastTarget, sizeof LastTarget); > + return EFI_SUCCESS; > + } > + > return EFI_NOT_FOUND; > } > > @@ -65,7 +109,34 @@ LsiScsiBuildDevicePath ( > IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath > ) > { > - return EFI_NOT_FOUND; > + UINT16 TargetValue; > + LSI_SCSI_DEV *Dev; > + SCSI_DEVICE_PATH *ScsiDevicePath; > + > + if (DevicePath == NULL) { > + return EFI_INVALID_PARAMETER; > + } > + > + CopyMem (&TargetValue, Target, sizeof TargetValue); > + Dev = LSI_SCSI_FROM_PASS_THRU (This); > + if (TargetValue > Dev->MaxTarget || Lun > Dev->MaxLun || Lun > 0xFFFF) { > + return EFI_NOT_FOUND; > + } > + > + ScsiDevicePath = AllocatePool (sizeof *ScsiDevicePath); > + if (ScsiDevicePath == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + ScsiDevicePath->Header.Type = MESSAGING_DEVICE_PATH; > + ScsiDevicePath->Header.SubType = MSG_SCSI_DP; > + ScsiDevicePath->Header.Length[0] = (UINT8) sizeof *ScsiDevicePath; > + ScsiDevicePath->Header.Length[1] = (UINT8) (sizeof *ScsiDevicePath >> 8); > + ScsiDevicePath->Pun = TargetValue; > + ScsiDevicePath->Lun = (UINT16) Lun; > + > + *DevicePath = &ScsiDevicePath->Header; > + return EFI_SUCCESS; > } > > EFI_STATUS > @@ -77,7 +148,33 @@ LsiScsiGetTargetLun ( > OUT UINT64 *Lun > ) > { > - return EFI_UNSUPPORTED; > + SCSI_DEVICE_PATH *ScsiDevicePath; > + LSI_SCSI_DEV *Dev; > + UINT8 *Target; > + > + if (DevicePath == NULL || TargetPointer == NULL || *TargetPointer == NULL || > + Lun == NULL) { > + return EFI_INVALID_PARAMETER; > + } > + > + if (DevicePath->Type != MESSAGING_DEVICE_PATH || > + DevicePath->SubType != MSG_SCSI_DP) { > + return EFI_UNSUPPORTED; > + } > + > + ScsiDevicePath = (SCSI_DEVICE_PATH *) DevicePath; > + Dev = LSI_SCSI_FROM_PASS_THRU (This); > + if (ScsiDevicePath->Pun > Dev->MaxTarget || > + ScsiDevicePath->Lun > Dev->MaxLun) { > + return EFI_NOT_FOUND; > + } > + > + Target = *TargetPointer; > + ZeroMem (Target, TARGET_MAX_BYTES); > + CopyMem (Target, &ScsiDevicePath->Pun, sizeof ScsiDevicePath->Pun); > + *Lun = ScsiDevicePath->Lun; > + > + return EFI_SUCCESS; > } > > EFI_STATUS > @@ -107,6 +204,42 @@ LsiScsiGetNextTarget ( > IN OUT UINT8 **TargetPointer > ) > { > + LSI_SCSI_DEV *Dev; > + UINTN Idx; > + UINT8 *Target; > + UINT16 LastTarget; > + > + // > + // the TargetPointer input parameter is unnecessarily a pointer-to-pointer > + // > + Target = *TargetPointer; > + > + // > + // Search for first non-0xFF byte. If not found, return first target. > + // > + for (Idx = 0; Idx < TARGET_MAX_BYTES && Target[Idx] == 0xFF; ++Idx) > + ; > + if (Idx == TARGET_MAX_BYTES) { > + SetMem (Target, TARGET_MAX_BYTES, 0x00); > + return EFI_SUCCESS; > + } > + > + CopyMem (&LastTarget, Target, sizeof LastTarget); > + > + // > + // increment target if valid on input > + // > + Dev = LSI_SCSI_FROM_PASS_THRU (This); > + if (LastTarget > Dev->MaxTarget) { > + return EFI_INVALID_PARAMETER; > + } > + > + if (LastTarget < Dev->MaxTarget) { > + ++LastTarget; > + CopyMem (Target, &LastTarget, sizeof LastTarget); > + return EFI_SUCCESS; > + } > + > return EFI_NOT_FOUND; > } > > @@ -189,6 +322,17 @@ LsiScsiControllerStart ( > > Dev->Signature = LSI_SCSI_DEV_SIGNATURE; > > + STATIC_ASSERT ( > + FixedPcdGet8 (PcdLsiScsiMaxTargetLimit) < 8, > + "LSI 53C895A supports targets [0..7]" > + ); > + STATIC_ASSERT ( > + FixedPcdGet8 (PcdLsiScsiMaxLunLimit) < 128, > + "LSI 53C895A supports LUNs [0..128]" > + ); (1) The condition on the LUN limit, and the error message for the same, are not consistent. The PCD is an inclusive limit. We assert that the PCD is strictly smaller than 128. Therefore the highest permitted PCD value is 127. Therefore the highest permitted LUN (because the PCD is inclusive) is 127. Therefore the error message should advertize "[0..127]", not "[0..128]". In other words, LUN 128 itself is not valid. If LUN 128 *is* valid, and so PCD=128 too is valid, then the condition should be updated, to say "< 129". > + Dev->MaxTarget = PcdGet8 (PcdLsiScsiMaxTargetLimit); > + Dev->MaxLun = PcdGet8 (PcdLsiScsiMaxLunLimit); > + > // > // Host adapter channel, doesn't exist > // > diff --git a/OvmfPkg/LsiScsiDxe/LsiScsi.h b/OvmfPkg/LsiScsiDxe/LsiScsi.h > index c194dfbf3347..6c6ed25f1c33 100644 > --- a/OvmfPkg/LsiScsiDxe/LsiScsi.h > +++ b/OvmfPkg/LsiScsiDxe/LsiScsi.h > @@ -14,6 +14,8 @@ > > typedef struct { > UINT32 Signature; > + UINT8 MaxTarget; > + UINT8 MaxLun; > EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode; > EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru; > } LSI_SCSI_DEV; > @@ -23,6 +25,7 @@ typedef struct { > #define LSI_SCSI_FROM_PASS_THRU(PassThruPtr) \ > CR (PassThruPtr, LSI_SCSI_DEV, PassThru, LSI_SCSI_DEV_SIGNATURE) > > + > // > // Probe, start and stop functions of this driver, called by the DXE core for > // specific devices. (2) You missed point (4) in my v1 review -- the above newline is spurious, so it should be removed from this patch. (It should either be removed completely, or squashed into the patch that introduces the LSI_SCSI_FROM_PASS_THRU macro.) With (1) and (2) fixed: Reviewed-by: Laszlo Ersek Thanks Laszlo > diff --git a/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf b/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf > index 14f9c68308cc..6111449577a8 100644 > --- a/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf > +++ b/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf > @@ -27,7 +27,9 @@ [Packages] > [LibraryClasses] > BaseLib > BaseMemoryLib > + DebugLib > MemoryAllocationLib > + PcdLib > UefiBootServicesTableLib > UefiDriverEntryPoint > UefiLib > @@ -35,3 +37,7 @@ [LibraryClasses] > [Protocols] > gEfiExtScsiPassThruProtocolGuid ## BY_START > gEfiPciIoProtocolGuid ## TO_START > + > +[FixedPcd] > + gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxTargetLimit ## CONSUMES > + gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxLunLimit ## CONSUMES > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec > index 2b0f137cbcce..ca13a3cb11d3 100644 > --- a/OvmfPkg/OvmfPkg.dec > +++ b/OvmfPkg/OvmfPkg.dec > @@ -174,6 +174,11 @@ [PcdsFixedAtBuild] > ## Microseconds to stall between polling for MptScsi request result > gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec|5|UINT32|0x3a > > + ## Set the *inclusive* number of targets and LUNs that LsiScsi exposes for > + # scan by ScsiBusDxe. > + gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxTargetLimit|7|UINT8|0x3b > + gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxLunLimit|0|UINT8|0x3c > + > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8 > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9 > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa >