From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web10.973.1588167532371766833 for ; Wed, 29 Apr 2020 06:38:52 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=GjRd4DxB; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1588167531; 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=grulNQddD4Ay8leAUc+g3og2WPV7Ggjmg0luLloqvEE=; b=GjRd4DxBu9CH+aCBTLWlThh9ex1hKYu9m3Atw5vhHZWEGkIEIvyw6oRFWldeJz5tTcnGVK jDAHByYGXE4VMrhmty8qBL87FdnMduGEcESTyvadHw1acNlwoIYnEmGE7fLcquUzX1hCvK Hx5R77bmqgep7QCh126JBAuMGI2o5Eo= 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-390-Bfd5KnheMgqumxXmQGGgGA-1; Wed, 29 Apr 2020 09:38:45 -0400 X-MC-Unique: Bfd5KnheMgqumxXmQGGgGA-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 884E0107ACF2; Wed, 29 Apr 2020 13:38:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-1.ams2.redhat.com [10.36.114.1]) by smtp.corp.redhat.com (Postfix) with ESMTP id F180F50FDC; Wed, 29 Apr 2020 13:38:41 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v5 06/12] OvmfPkg/MptScsiDxe: Report targets and one LUN To: devel@edk2.groups.io, nikita.leshchenko@oracle.com Cc: liran.alon@oracle.com, aaron.young@oracle.com, Jordan Justen , Ard Biesheuvel References: <20200424175927.41210-1-nikita.leshchenko@oracle.com> <20200424175927.41210-7-nikita.leshchenko@oracle.com> From: "Laszlo Ersek" Message-ID: <4f6278f1-f04d-ce38-38f6-8d45401d0bf1@redhat.com> Date: Wed, 29 Apr 2020 15:38:41 +0200 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: <20200424175927.41210-7-nikita.leshchenko@oracle.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=windows-1252 Content-Transfer-Encoding: 7bit On 04/24/20 19:59, Nikita Leshenko wrote: > The controller supports up to 8 targets in practice (Not reported by the > controller, but based on the implementation of the virtual device), > report them in GetNextTarget and GetNextTargetLun. The firmware will > then try to communicate with them and create a block device for each one > that responds. > > Support for multiple LUNs will be implemented in another series. > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390 > Signed-off-by: Nikita Leshenko > --- > OvmfPkg/MptScsiDxe/MptScsi.c | 62 ++++++++++++++++++++++++++++++- > OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 4 ++ > OvmfPkg/OvmfPkg.dec | 4 ++ > 3 files changed, 68 insertions(+), 2 deletions(-) > > diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c > index 40d392c2346f..5e0544c8d9d2 100644 > --- a/OvmfPkg/MptScsiDxe/MptScsi.c > +++ b/OvmfPkg/MptScsiDxe/MptScsi.c > @@ -11,6 +11,7 @@ > > #include > #include > +#include > #include > #include > #include > @@ -34,6 +35,7 @@ typedef struct { > UINT32 Signature; > EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru; > EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode; > + UINT8 MaxTarget; > } MPT_SCSI_DEV; > > #define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \ > @@ -57,6 +59,22 @@ MptScsiPassThru ( > return EFI_UNSUPPORTED; > } > > +STATIC > +BOOLEAN > +IsTargetInitialized ( > + IN UINT8 *Target > + ) > +{ > + UINTN Idx; > + > + for (Idx = 0; Idx < TARGET_MAX_BYTES; ++Idx) { > + if (Target[Idx] != 0xFF) { > + return TRUE; > + } > + } > + return FALSE; > +} > + > STATIC > EFI_STATUS > EFIAPI > @@ -66,7 +84,28 @@ MptScsiGetNextTargetLun ( > IN OUT UINT64 *Lun > ) > { > - return EFI_UNSUPPORTED; > + MPT_SCSI_DEV *Dev; > + > + Dev = MPT_SCSI_FROM_PASS_THRU (This); > + // > + // Currently support only LUN 0, so hardcode it > + // > + if (!IsTargetInitialized (*Target)) { > + ZeroMem (*Target, TARGET_MAX_BYTES); > + *Lun = 0; > + } else if (**Target > Dev->MaxTarget || *Lun > 0) { > + return EFI_INVALID_PARAMETER; > + } else if (**Target < Dev->MaxTarget) { > + // > + // This device interface support 256 targets only, so it's enough to > + // increment the LSB of Target, as it will never overflow. > + // > + **Target += 1; > + } else { > + return EFI_NOT_FOUND; > + } > + > + return EFI_SUCCESS; > } > > STATIC > @@ -77,7 +116,24 @@ MptScsiGetNextTarget ( > IN OUT UINT8 **Target > ) > { > - return EFI_UNSUPPORTED; > + MPT_SCSI_DEV *Dev; > + > + Dev = MPT_SCSI_FROM_PASS_THRU (This); > + if (!IsTargetInitialized (*Target)) { > + ZeroMem (*Target, TARGET_MAX_BYTES); > + } else if (**Target > Dev->MaxTarget) { > + return EFI_INVALID_PARAMETER; > + } else if (**Target < Dev->MaxTarget) { > + // > + // This device interface support 256 targets only, so it's enough to > + // increment the LSB of Target, as it will never overflow. > + // > + **Target += 1; > + } else { > + return EFI_NOT_FOUND; > + } > + > + return EFI_SUCCESS; > } > > STATIC > @@ -206,6 +262,8 @@ MptScsiControllerStart ( > > Dev->Signature = MPT_SCSI_DEV_SIGNATURE; > > + Dev->MaxTarget = PcdGet8 (PcdMptScsiMaxTargetLimit); > + > // > // Host adapter channel, doesn't exist > // > diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf > index 9f7c98829ee1..4862ff9dd497 100644 > --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf > +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf > @@ -24,6 +24,7 @@ [Packages] > OvmfPkg/OvmfPkg.dec > > [LibraryClasses] > + BaseMemoryLib > DebugLib > MemoryAllocationLib > UefiBootServicesTableLib > @@ -33,3 +34,6 @@ [LibraryClasses] > [Protocols] > gEfiExtScsiPassThruProtocolGuid ## BY_START > gEfiPciIoProtocolGuid ## TO_START > + > +[FixedPcd] > + gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiMaxTargetLimit ## CONSUMES > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec > index 28030391cff2..2d09444bbb16 100644 > --- a/OvmfPkg/OvmfPkg.dec > +++ b/OvmfPkg/OvmfPkg.dec > @@ -163,6 +163,10 @@ [PcdsFixedAtBuild] > # polling loop iteration. > gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiWaitForCmpStallInUsecs|5|UINT32|0x38 > > + ## Set the *inclusive* number of targets that MptScsi exposes for scan > + # by ScsiBusDxe. > + gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiMaxTargetLimit|7|UINT8|0x39 > + > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8 > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9 > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa > (1) should be #include'd in this patch, and PcdLib should be listed under [LibraryClasses]. On one hand, that's a minor wart. The driver (and the OVMF platform(s)) build OK at this stage, in practice. (I tested that.) So this, per se, does not justify a v6. On the other hand, even at the end of the series, the module does not spell out the PcdLib dependency (neither #include nor [LibraryClasses]). That's not so nice. But, I'll fix that up for you, if there's not going to be another reason for a v6. With (1) addressed (by you, or by me): Reviewed-by: Laszlo Ersek Thanks, Laszlo