From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web11.12440.1594112671258654966 for ; Tue, 07 Jul 2020 02:04:31 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=A0khHXDt; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1594112670; 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=xFN6UKdAW+S3AlsYmOQct+S714wDM7F+2cN+TXQx4ww=; b=A0khHXDtOWX71hR3vrBB0OPOXIjCWJnpU6dgWXFXua0oCBHAH0MUsjn0UwRvZIpmvO66YH Y2Mb8cbGLtZ6/iXe7JK4JrihbGwndUJoNN4SXchfKrC7J6jPSl9KgtEz8UX7eOtdSRIQ8Y jFZG3Nk+dnEyww/9Veci7LA/txsrf40= 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-326-OEsVuAPqPsaFZRtzphblYg-1; Tue, 07 Jul 2020 05:04:28 -0400 X-MC-Unique: OEsVuAPqPsaFZRtzphblYg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id EF8D9EC1A0; Tue, 7 Jul 2020 09:04:23 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-90.ams2.redhat.com [10.36.114.90]) by smtp.corp.redhat.com (Postfix) with ESMTP id A8BA511A036; Tue, 7 Jul 2020 09:04:22 +0000 (UTC) Subject: Re: [PATCH 06/11] OvmfPkg/LsiScsiDxe: Report Targets and LUNs To: Gary Lin , devel@edk2.groups.io Cc: Jordan Justen , Ard Biesheuvel References: <20200701040448.14871-1-glin@suse.com> <20200701040448.14871-7-glin@suse.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 7 Jul 2020 11:04:21 +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: <20200701040448.14871-7-glin@suse.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com 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/01/20 06:04, 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. > > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Signed-off-by: Gary Lin > --- > OvmfPkg/LsiScsiDxe/LsiScsi.c | 143 +++++++++++++++++++++++++++++- > OvmfPkg/LsiScsiDxe/LsiScsi.h | 3 + > OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf | 6 ++ > OvmfPkg/OvmfPkg.dec | 5 ++ > 4 files changed, 155 insertions(+), 2 deletions(-) > > diff --git a/OvmfPkg/LsiScsiDxe/LsiScsi.c b/OvmfPkg/LsiScsiDxe/LsiScsi.c > index f633c6793298..e10a81a5f9f7 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,36 @@ 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; > + } > + > + // > + // This device support 8 targets only, so it's enough to set the LSB > + // of Target. > + // > + Target = *TargetPointer; > + *Target = (UINT8)ScsiDevicePath->Pun; I don't like this shortcut. The first reason is that the shortcut is not consistent with the rest of the functions implemented in this patch. The other functions handle target numbers that extend up to two bytes; there are no single-byte shortcuts in them. (1) So please either rewrite all of those functions to use a similar shortcut like here, or please remove this shortcut, and stick with a 2-byte sized CopyMem() here too. I prefer the latter. (2) Whether you use the single-byte shortcut or not, please zero out the bytes in (*Target) that you do not set otherwise (from Pun). (3) If the device supports only 8 targets, then please add an according STATIC_ASSERT() on PcdLsiScsiMaxTargetLimit somewhere. For example: STATIC_ASSERT ( FixedPcdGet8 (PcdLsiScsiMaxTargetLimit) < 8, "LSI 53C895A supports targets [0..7]" ); It's probably best to add the assert in LsiScsiControllerStart() below, where you fetch the PCD anyway. > + *Lun = ScsiDevicePath->Lun; > + > + return EFI_SUCCESS; > } > > EFI_STATUS > @@ -107,6 +207,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 +325,9 @@ LsiScsiControllerStart ( > > Dev->Signature = LSI_SCSI_DEV_SIGNATURE; > > + 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 fca1007f9b98..a3d51d8f2386 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. (4) Spurious newline, please drop it. (Or squash it in the previous patch -- for example --, where you define the LSI_SCSI_FROM_PASS_THRU macro.) > diff --git a/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf b/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf > index 8660513e2ffd..68844c6772e3 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 65bb2bb0eb4c..ae7d1d648d22 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|0x40 > > + ## 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 > Commit 093cceaf79b5 ("OvmfPkg/MptScsiDxe: Report targets and one LUN", 2020-05-05) introduced PcdMptScsiMaxTargetLimit with token value 0x39. That was OK. Commit 505812ae1d2d ("OvmfPkg/MptScsiDxe: Implement the PassThru method", 2020-05-05) then introduced PcdMptScsiStallPerPollUsec with token value 0x40. That was not OK -- but I missed it. (The token value should have been 0x3a; as 0x39 is not succeeded by 0x40 but by 0x3a.) Token value 0x3a remains unused at this point. (5) So please introduce the new PCDs with token values 0x3a and 0x3b, to keep the token space dense. Thanks Laszlo