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.web11.19206.1594916370546186468 for ; Thu, 16 Jul 2020 09:19:30 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=IasvWaX2; 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=1594916369; 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=c8MP6f9WhL3hLoNYZS+wquTWuImZGaVMbigP27aceHU=; b=IasvWaX26okDMLnWGtxY4Nf/gwPosI14cF8u4P1kUGhO2jeOJ09vdpFxrYdtIgmkEz5zUz KxGANz3Q5kOHJTTTGLhvVesMgCy4fmPvhyHQy2P8QBZwom9237mqawxh7hBgt+kVoEovAx l6WLau7rfMMfoLJw/L4YF7XZxgsbW+8= 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-479-v7qpGtBoPTixbxfQu1KpGA-1; Thu, 16 Jul 2020 12:19:27 -0400 X-MC-Unique: v7qpGtBoPTixbxfQu1KpGA-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B26A8A30C3; Thu, 16 Jul 2020 16:19:26 +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 90E5C6FED1; Thu, 16 Jul 2020 16:19:25 +0000 (UTC) Subject: Re: [PATCH v2 06/12] OvmfPkg/LsiScsiDxe: Report Targets and LUNs From: "Laszlo Ersek" 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> <1868905d-0420-2cc4-e692-1c48af4aa712@redhat.com> Message-ID: <1053bd63-8c06-d15d-28f3-2090f9130f35@redhat.com> Date: Thu, 16 Jul 2020 18:19:24 +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: <1868905d-0420-2cc4-e692-1c48af4aa712@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 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 17:04, Laszlo Ersek wrote: > 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". OK, reading through my v1 review, I've realized (again) that 127 is the maximum valid LUN (and so the max PCD value), therefore the problem is in the message, not in the check. Please modify the message to "[0..127]". Thanks Laszlo > >> + 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 >> >