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.web12.11488.1582880644705866818 for ; Fri, 28 Feb 2020 01:04:04 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=LIZxfTer; 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=1582880643; 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=2N6nD365VxGoAdUuj5D9Qe2LaOieHQFwnwyR4ZtbFVk=; b=LIZxfTerCP3rezC9TyLFEjavk6INq1ViTTnHLx0fcAI4w+l+yCU3qZF8Qk9K2T1MRdgh+G lamtaMDCQYHRg8esQ/x8zTce//Tnxz058M1U2NHJ/Pl6neB5G2ra5TLNf9oYRMLGS1j9yC SOBUg++hyEdTJC0gjtQ6aJY61sSnxtU= 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-156-MKwU9AjnPHGHqbWPZZznxQ-1; Fri, 28 Feb 2020 04:03:55 -0500 X-MC-Unique: MKwU9AjnPHGHqbWPZZznxQ-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 99A77DB20; Fri, 28 Feb 2020 09:03:54 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-243.ams2.redhat.com [10.36.116.243]) by smtp.corp.redhat.com (Postfix) with ESMTP id 256549299D; Fri, 28 Feb 2020 09:03:52 +0000 (UTC) Subject: Re: [PATCH v2 07/13] OvmfPkg/MptScsiDxe: Build DevicePath for discovered devices To: Nikita Leshenko , devel@edk2.groups.io Cc: liran.alon@oracle.com, aaron.young@oracle.com, jordan.l.justen@intel.com, ard.biesheuvel@linaro.org References: <20200226164151.125182-1-nikita.leshchenko@oracle.com> <20200226164151.125182-8-nikita.leshchenko@oracle.com> From: "Laszlo Ersek" Message-ID: <6f916738-86a5-b1a1-fbc1-6959169dfd6e@redhat.com> Date: Fri, 28 Feb 2020 10:03:52 +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: <20200226164151.125182-8-nikita.leshchenko@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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 02/26/20 17:41, Nikita Leshenko wrote: > Used to identify the individual disks in the hardware tree > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Nikita Leshenko > Reviewed-by: Konrad Rzeszutek Wilk > Reviewed-by: Aaron Young > Reviewed-by: Liran Alon > --- > OvmfPkg/MptScsiDxe/MptScsi.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c > index 76f0515b52..593cf30f6b 100644 > --- a/OvmfPkg/MptScsiDxe/MptScsi.c > +++ b/OvmfPkg/MptScsiDxe/MptScsi.c > @@ -128,7 +128,22 @@ MptScsiBuildDevicePath ( > IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath > ) > { > - return EFI_UNSUPPORTED; > + SCSI_DEVICE_PATH *ScsiDevicePath; > + > + ScsiDevicePath = AllocateZeroPool (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; (1) This does not cause ill behavior in practice, but the value makes no sense. You are first casting the result of the sizeof operator to UINT8, and then right-shifting that by 8 bits. That is guaranteed to produce zero, regardless of what sizeof returns. You should use (UINT8)(sizeof (*ScsiDevicePath) >> 8) > + ScsiDevicePath->Pun = *Target; (2) Aha, OK. I was a bit surprised by the previous patch, but it is certainly valid. You want to use target identifiers that are all 0xFF bytes, except the very first byte. That's fine (and "PcdMptScsiMaxTargetLimit" is also consistent with that, having type UINT8), but please announce this decision more visibly. Please append: ---- This driver uses the first byte (out of TARGET_MAX_BYTES) in Target IDs for representing the target, the other bytes are always 0xFF in Target IDs. ---- to the commit messages of: - OvmfPkg/MptScsiDxe: Report one Target and one LUN - OvmfPkg/MptScsiDxe: Build DevicePath for discovered devices - OvmfPkg/MptScsiDxe: Implement GetTargetLun - OvmfPkg/MptScsiDxe: Report multiple targets > + ScsiDevicePath->Lun = (UINT16)Lun; This funcion is missing the target / lun validity check at the top (the UEFI spec describes that related to the EFI_NOT_FOUND return status -- "The SCSI devices specified by Target and Lun does not exist on the SCSI channel"). My understanding is that the driver only supports (*Target=0 && Lun=0) at this point. (3) So please express that at the start of the function. (Return EFI_NOT_FOUND otherwise.) (4) And, in patch "OvmfPkg/MptScsiDxe: Report multiple targets", please also update this function -- MptScsiBuildDevicePath() --, so that it accept (*Target) values up to and including "Dev->MaxTarget". > + > + *DevicePath = &ScsiDevicePath->Header; > + return EFI_SUCCESS; > } > > STATIC > Thanks Laszlo