From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [216.205.24.74]) by mx.groups.io with SMTP id smtpd.web10.6261.1585056980941030487 for ; Tue, 24 Mar 2020 06:36:21 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Ao2Xek0c; spf=pass (domain: redhat.com, ip: 216.205.24.74, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585056979; 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=ib8U1iH/AllZ9ovX5FicGzQXs2Fn1pUUAW1rPE4Nlxc=; b=Ao2Xek0cPE/+3IzdVl04kxksYGSw8xLIejaQsg6pJpwY1klrqHqJ/3/PDgyKPPQN0JgjIn ah/jEKhN2nh7TyC+jZWhLL12jpuK6Ghqb3WNvT9VFwwIoN4ZDqtAl5pRqnmhfSKpKfexYN VWjNOLcLG+jrMZXTL3fgeoWJ+v0Z9VE= 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-293-5Q-9S-LpOBmNoo10JTbjaw-1; Tue, 24 Mar 2020 09:36:16 -0400 X-MC-Unique: 5Q-9S-LpOBmNoo10JTbjaw-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id A5BC319057AC; Tue, 24 Mar 2020 13:36:14 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-139.ams2.redhat.com [10.36.115.139]) by smtp.corp.redhat.com (Postfix) with ESMTP id 009A81001B3F; Tue, 24 Mar 2020 13:36:11 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 07/17] OvmfPkg/PvScsiDxe: Translate Target & LUN to/from DevicePath To: devel@edk2.groups.io, liran.alon@oracle.com Cc: nikita.leshchenko@oracle.com, aaron.young@oracle.com, jordan.l.justen@intel.com, ard.biesheuvel@linaro.org References: <20200316150113.104630-1-liran.alon@oracle.com> <20200316150113.104630-8-liran.alon@oracle.com> From: "Laszlo Ersek" Message-ID: <9fb13978-eeaf-3d6a-2343-186de7515fde@redhat.com> Date: Tue, 24 Mar 2020 14:36:11 +0100 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: <20200316150113.104630-8-liran.alon@oracle.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 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 03/16/20 16:01, Liran Alon wrote: > Implement EXT_SCSI_PASS_THRU.BuildDevicePath() and > EXT_SCSI_PASS_THRU.GetTargetLun(). > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567 > Reviewed-by: Nikita Leshenko > Signed-off-by: Liran Alon > --- > OvmfPkg/PvScsiDxe/PvScsi.c | 60 ++++++++++++++++++++++++++++++++++++-- > 1 file changed, 58 insertions(+), 2 deletions(-) > > diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c > index 76bb361c7c94..f613870e80f2 100644 > --- a/OvmfPkg/PvScsiDxe/PvScsi.c > +++ b/OvmfPkg/PvScsiDxe/PvScsi.c > @@ -136,7 +136,38 @@ PvScsiBuildDevicePath ( > IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath > ) > { > - return EFI_UNSUPPORTED; > + UINT8 TargetValue; > + PVSCSI_DEV *Dev; > + SCSI_DEVICE_PATH *ScsiDevicePath; > + > + if (DevicePath == NULL) { > + return EFI_INVALID_PARAMETER; > + } > + > + // > + // We only use first byte of target identifer > + // > + TargetValue = *Target; > + > + Dev = PVSCSI_FROM_PASS_THRU (This); > + if (TargetValue > Dev->MaxTarget || Lun > Dev->MaxLun) { > + 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; > } > > STATIC > @@ -149,7 +180,32 @@ PvScsiGetTargetLun ( > OUT UINT64 *Lun > ) > { > - return EFI_UNSUPPORTED; > + SCSI_DEVICE_PATH *ScsiDevicePath; > + PVSCSI_DEV *Dev; > + > + if (DevicePath == NULL || Target == NULL || *Target == 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 = PVSCSI_FROM_PASS_THRU (This); > + if (ScsiDevicePath->Pun > Dev->MaxTarget || > + ScsiDevicePath->Lun > Dev->MaxLun) { > + return EFI_NOT_FOUND; > + } > + > + // > + // We only use first byte of target identifer > + // > + **Target = (UINT8)ScsiDevicePath->Pun; (1) Please add a ZeroMem() call here, for nulling all the other (TARGET_MAX_BYTES - 1) bytes in the target array. I think: ZeroMem (*Target + 1, TARGET_MAX_BYTES - 1); For two reasons: - We should be consistent in the target arrays that we output, and the functions implemented in the previous patch zero out the unused bytes (which is nice). We should produce similar target arrays here. - When investigating SCSI problems, sometimes debug patches have to be written for the generic (device model-independent) SCSI drivers in edk2. In those cases, we may hexdump the entire target array. And indeterminate byte values in the target array should not leak into such logs -- they could complicate textual comparisons etc. > + *Lun = ScsiDevicePath->Lun; > + > + return EFI_SUCCESS; > } > > STATIC > With (1) addressed: Reviewed-by: Laszlo Ersek Thanks Laszlo