Re: [PATCH v2 07/13] OvmfPkg/MptScsiDxe: Build DevicePath for discovered devices

["Laszlo Ersek" <lersek@redhat.com>]

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 <nikita.leshchenko@oracle.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Aaron Young <aaron.young@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> ---
>  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