From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web10.994.1588167607151285268 for ; Wed, 29 Apr 2020 06:40:07 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=iju4lTuI; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1588167606; 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=XmhnOse4W/qdaCM8R1gwW4rvXnmUp3kUQneWOS7/am0=; b=iju4lTuIeS5DltKTrpa0LyS8ZqgIA6R1KEGSit6gTMjR9MpHVjq4sbSqJVqc78CvyJi+6W IzjAskZyG4zld1YVnSeuQ8jm13T2Kp0M4UaOKe/0qlYsDpjB6fM8xLYeLNOLvYR2H9Nxqu 7bKcBaC/fMKthGCFCAAvXe9WPoE+I3s= 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-81-iDL7zbnWO6G0viZPqxYpig-1; Wed, 29 Apr 2020 09:39:59 -0400 X-MC-Unique: iDL7zbnWO6G0viZPqxYpig-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 50BBD107ACF4; Wed, 29 Apr 2020 13:39:58 +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 6625460DB4; Wed, 29 Apr 2020 13:39:56 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v5 06/12] OvmfPkg/MptScsiDxe: Report targets and one LUN From: "Laszlo Ersek" To: devel@edk2.groups.io, nikita.leshchenko@oracle.com, liran.alon@oracle.com Cc: aaron.young@oracle.com, Jordan Justen , Ard Biesheuvel References: <20200424175927.41210-1-nikita.leshchenko@oracle.com> <20200424175927.41210-7-nikita.leshchenko@oracle.com> <4f6278f1-f04d-ce38-38f6-8d45401d0bf1@redhat.com> Message-ID: Date: Wed, 29 Apr 2020 15:39:55 +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: <4f6278f1-f04d-ce38-38f6-8d45401d0bf1@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 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/29/20 15:38, Laszlo Ersek wrote: > 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 > ... BTW I missed the same in the PvScsiDxe driver :/ Liran, can you post a patch for that, please? Thanks, Laszlo