Re: EFI_FIRMWARE_IMAGE_DESCRIPTOR v1/v2/v3: MdePkg and ShellPkg

["Shah, Tapan" <tapandshah@hpe.com>]

[-- Attachment #1: Type: text/plain, Size: 3274 bytes --]

There is no rule that says consumer of the code cannot define their own structures (in this case UefiHandleParsingLib in ShellPkg needs to consume V1 and V2). Having V1, V2 structures make it easy to decode the data in a structural way instead of hard-coded method to parse the data.

Liming and Jaben agreed on this idea of having V1, V2 in UefiHandleParsingLib when we decided to add FMP V1,V2,V3 decode support in 'dh' command. See attached old discussion on this agreement.
From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
Sent: Thursday, February 16, 2017 5:13 PM
To: Shah, Tapan <tapandshah@hpe.com>; Rebecca Cran <rebecca@bluestop.org>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>
Subject: RE: [edk2] EFI_FIRMWARE_IMAGE_DESCRIPTOR v1/v2/v3: MdePkg and ShellPkg

Yes, I agree that SHELL pkg should dump V1 only and V2 only data structure.

I just think there is no need to add new data structure there.
The shell pkg can just skip LowestSupportedImageVersion in V1 and skip LastAttemptVersion/ LastAttemptStatus/ HardwareInstance in V2.


Thank you
Yao Jiewen

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Shah,
> Tapan
> Sent: Thursday, February 16, 2017 3:00 PM
> To: Rebecca Cran <rebecca@bluestop.org<mailto:rebecca@bluestop.org>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Gao, Liming
> <liming.gao@intel.com<mailto:liming.gao@intel.com>>
> Subject: Re: [edk2] EFI_FIRMWARE_IMAGE_DESCRIPTOR v1/v2/v3: MdePkg and
> ShellPkg
>
> UEFI Spec does not have old FMP image descriptor structures V1 and V2 defined.
>
> MdePkg only follows the spec, so it contains the latest version # 3. But there are
> still drivers using old V1, V2 revisions and Shell 'dh' command needs to support
> decoding all revisions and needs to remain in ShellPkg.
>
>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Rebecca Cran
> Sent: Thursday, February 16, 2017 4:48 PM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Subject: [edk2] EFI_FIRMWARE_IMAGE_DESCRIPTOR v1/v2/v3: MdePkg and
> ShellPkg
>
> I'm a bit confused about why Firmware Management Protocol image descriptor
> structures are split between MdePkg and ShellPkg:
>
> In MdePkg/Include/Protocol/FirmwareInformation.h there's the definition of
> EFI_FIRMWARE_IMAGE_DESCRIPTOR (version 3).  But then the
> EFI_FIRMWARE_IMAGE_DESCRIPTOR_V1 and
> EFI_FIRMWARE_IMAGE_DESCRIPTOR_V2 struct definitions are in
> ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.h along with some
> seemingly-unrelated stuff - and that file appears to have a ridiculous number of
> #include's!
>
>
> Is there a reasoning behind putting the older structures in ShellPkg, or should
> they be moved to FirmwareInformation.h?
>
>
> --
>
> Rebecca
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel

[-- Attachment #2: Type: message/rfc822, Size: 16584 bytes --]

From: "Carsey, Jaben" <jaben.carsey@intel.com>
To: "Gao, Liming" <liming.gao@intel.com>, "El-Haj-Mahmoud, Samer" <samer.el-haj-mahmoud@hpe.com>, "Shah, Tapan" <tapandshah@hpe.com>, "Kinney, Michael D" <michael.d.kinney@intel.com>, "Rothman, Michael A" <michael.a.rothman@intel.com>
Cc: "Qiu, Shumin" <shumin.qiu@intel.com>, "Ni, Ruiyu" <ruiyu.ni@intel.com>, "Carsey, Jaben" <jaben.carsey@intel.com>
Subject: RE: [PATCH] MdePkg: Add EFI_FIRMWARE_IMAGE_DESCRIPTOR V1 and V2 structure definition.
Date: Fri, 18 Mar 2016 14:55:07 +0000
Message-ID: <CB6E33457884FA40993F35157061515C548886FA@FMSMSX103.amr.corp.intel.com>

I am ok.

From: Gao, Liming
Sent: Friday, March 18, 2016 7:25 AM
To: El-Haj-Mahmoud, Samer <samer.el-haj-mahmoud@hpe.com>; Shah, Tapan <tapandshah@hpe.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Carsey, Jaben <jaben.carsey@intel.com>; Rothman, Michael A <michael.a.rothman@intel.com>
Cc: Qiu, Shumin <shumin.qiu@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: RE: [PATCH] MdePkg: Add EFI_FIRMWARE_IMAGE_DESCRIPTOR V1 and V2 structure definition.
Importance: High

+ Ruiyu and Shumin

I am OK to add them into ShellPkg.

Thanks
Liming
From: El-Haj-Mahmoud, Samer [mailto:samer.el-haj-mahmoud@hpe.com]
Sent: Friday, March 18, 2016 10:22 PM
To: Shah, Tapan <tapandshah@hpe.com<mailto:tapandshah@hpe.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Carsey, Jaben <jaben.carsey@intel.com<mailto:jaben.carsey@intel.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Rothman, Michael A <michael.a.rothman@intel.com<mailto:michael.a.rothman@intel.com>>
Cc: El-Haj-Mahmoud, Samer <samer.el-haj-mahmoud@hpe.com<mailto:samer.el-haj-mahmoud@hpe.com>>
Subject: RE: [PATCH] MdePkg: Add EFI_FIRMWARE_IMAGE_DESCRIPTOR V1 and V2 structure definition.

+ Liming and Mike Rothman

We need the Shell DH command to be able to dump the FMP information from previous as well as most recent UEFI spec implementations. There are a wide range of IHV drivers in the wild that support a mix of V1, V2, and the latest V3.

I think putting these definitions in the ShellPkg UefiHandleParsingLib should be acceptable.

Jaben?

Thanks,
--Samer

-----Original Message-----
From: Shah, Tapan
Sent: Friday, March 18, 2016 8:42 AM
To: Gao, Liming ; Kinney, Michael D ; Carsey, Jaben
Cc: El-Haj-Mahmoud, Samer
Subject: RE: [PATCH] MdePkg: Add EFI_FIRMWARE_IMAGE_DESCRIPTOR V1 and V2 structure definition.

Jaben,
UefiHandleParsingLib library of ShellPkg is the consumer of this V1/V2 structure for 'dh' command. Is it OK for you if we add these definitions in UefiHandleParsingLib.h file?

Thanks,
Tapan

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Gao, Liming
Sent: Friday, March 18, 2016 12:22 AM
To: Shah, Tapan ; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Kinney, Michael D
Cc: Carsey, Jaben
Subject: Re: [edk2] [PATCH] MdePkg: Add EFI_FIRMWARE_IMAGE_DESCRIPTOR V1 and V2 structure definition.

Tapan:
UEFI spec doesn't define EFI_FIRMWARE_IMAGE_DESCRIPTOR_V1 and EFI_FIRMWARE_IMAGE_DESCRIPTOR_V2. MdePkg definitions follows UEFI/PI/IndustryStandard specification. For this case, the consumer code can base on current definition to use the different version structure. So, I don't think we need to add V1 and V2 structure into UEFI spec and MdePkg. But, they can still be added into your local driver for your development.

Thanks
Liming
From: Shah, Tapan [mailto:tapandshah@hpe.com]
Sent: Thursday, March 17, 2016 10:30 PM
To: Gao, Liming; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Kinney, Michael D
Cc: Carsey, Jaben; El-Haj-Mahmoud, Samer
Subject: RE: [PATCH] MdePkg: Add EFI_FIRMWARE_IMAGE_DESCRIPTOR V1 and V2 structure definition.

Liming,
Having old version structure definition makes it clean for consumer to decode structure without being deterministic for which fields exists in structure based on its version and size. The only structure definition exist in .h file does not provide a clean way to decode the data.

Tapan

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Gao, Liming
Sent: Wednesday, March 16, 2016 8:53 PM
To: Shah, Tapan ; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Kinney, Michael D
Cc: Carsey, Jaben
Subject: Re: [edk2] [PATCH] MdePkg: Add EFI_FIRMWARE_IMAGE_DESCRIPTOR V1 and V2 structure definition.

Tapan:
Consumer can base on version value to know the structure size and date. We don't need to add old version structure definition.

Thanks
Liming
From: Shah, Tapan [mailto:tapandshah@hpe.com]
Sent: Thursday, March 17, 2016 3:54 AM
To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Kinney, Michael D ; Gao, Liming
Cc: Carsey, Jaben ; El-Haj-Mahmoud, Samer
Subject: RE: [PATCH] MdePkg: Add EFI_FIRMWARE_IMAGE_DESCRIPTOR V1 and V2 structure definition.

Michael / Liming,
Can you please help review this change?

Thanks,
Tapan

-----Original Message-----
From: Carsey, Jaben [mailto:jaben.carsey@intel.com]
Sent: Wednesday, March 16, 2016 12:58 PM
To: Shah, Tapan ; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: El-Haj-Mahmoud, Samer ; Carsey, Jaben
Subject: RE: [PATCH] MdePkg: Add EFI_FIRMWARE_IMAGE_DESCRIPTOR V1 and V2 structure definition.

Looks good.

Reviewed-by: Jaben Carsey

> -----Original Message-----
> From: Tapan Shah [mailto:tapandshah@hpe.com]
> Sent: Wednesday, March 16, 2016 10:31 AM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: samer.el-haj-mahmoud@hpe.com<mailto:samer.el-haj-mahmoud@hpe.com>;
> Carsey, Jaben ; Tapan Shah
> Subject: [PATCH] MdePkg: Add EFI_FIRMWARE_IMAGE_DESCRIPTOR V1 and
> V2 structure definition.
>
> Add V1 and V2 structure definitions for EFI_FIRMWARE_IMAGE_DESCRIPTOR
> structure. Consumer of the protocol needs correct structure definition
> to decode structure fields correctly.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Tapan Shah
> ---
> MdePkg/Include/Protocol/FirmwareManagement.h | 116
> +++++++++++++++++++++++++++
> 1 file changed, 116 insertions(+)
>
> diff --git a/MdePkg/Include/Protocol/FirmwareManagement.h
> b/MdePkg/Include/Protocol/FirmwareManagement.h
> index e615573..67f19fd 100644
> --- a/MdePkg/Include/Protocol/FirmwareManagement.h
> +++ b/MdePkg/Include/Protocol/FirmwareManagement.h
> @@ -10,6 +10,7 @@
>
> Copyright (c) 2009 - 2015, Intel Corporation. All rights reserved.

> Copyright (c) 2013 - 2014, Hewlett-Packard Development Company, L.P.

> + (C) Copyright 2016 Hewlett Packard Enterprise Development LP

> This program and the accompanying materials are licensed and made
> available under the terms and conditions of the BSD License which
> accompanies this distribution. The full text of the license may be
> found at @@ -119,6 +120,121 @@ typedef struct {
> UINT64 HardwareInstance;
> } EFI_FIRMWARE_IMAGE_DESCRIPTOR;
>
> +#define EFI_FIRMWARE_IMAGE_DESCRIPTOR_VERSION_V1 1 #define
> +EFI_FIRMWARE_IMAGE_DESCRIPTOR_VERSION_V2 2
> +
> +///
> +/// EFI_FIRMWARE_IMAGE_DESCRIPTOR in UEFI spec < 2.4a /// typedef
> +struct { /// /// A unique number identifying the firmware image
> +within the device. The
> number is
> + /// between 1 and DescriptorCount.
> + ///
> + UINT8 ImageIndex;
> + ///
> + /// A unique number identifying the firmware image type.
> + ///
> + EFI_GUID ImageTypeId;
> + ///
> + /// A unique number identifying the firmware image.
> + ///
> + UINT64 ImageId;
> + ///
> + /// A pointer to a null-terminated string representing the firmware
> + image
> name.
> + ///
> + CHAR16 *ImageIdName;
> + ///
> + /// Identifies the version of the device firmware. The format is
> + vendor
> specific and new
> + /// version must have a greater value than an old version.
> + ///
> + UINT32 Version;
> + ///
> + /// A pointer to a null-terminated string representing the firmware
> + image
> version name.
> + ///
> + CHAR16 *VersionName;
> + ///
> + /// Size of the image in bytes. If size=0, then only ImageIndex and
> ImageTypeId are valid.
> + ///
> + UINTN Size;
> + ///
> + /// Image attributes that are supported by this device. See 'Image
> + Attribute
> Definitions'
> + /// for possible returned values of this parameter. A value of 1
> + indicates
> the attribute is
> + /// supported and the current setting value is indicated in AttributesSetting.
> A
> + /// value of 0 indicates the attribute is not supported and the
> + current
> setting value in
> + /// AttributesSetting is meaningless.
> + ///
> + UINT64 AttributesSupported;
> + ///
> + /// Image attributes. See 'Image Attribute Definitions' for possible
> + returned
> values of
> + /// this parameter.
> + ///
> + UINT64 AttributesSetting;
> + ///
> + /// Image compatibilities. See 'Image Compatibility Definitions'
> + for possible
> returned
> + /// values of this parameter.
> + ///
> + UINT64 Compatibilities;
> +} EFI_FIRMWARE_IMAGE_DESCRIPTOR_V1;
> +
> +
> +///
> +/// EFI_FIRMWARE_IMAGE_DESCRIPTOR in UEFI spec > 2.4a and < 2.5 ///
> +typedef struct { /// /// A unique number identifying the firmware
> +image within the device. The
> number is
> + /// between 1 and DescriptorCount.
> + ///
> + UINT8 ImageIndex;
> + ///
> + /// A unique number identifying the firmware image type.
> + ///
> + EFI_GUID ImageTypeId;
> + ///
> + /// A unique number identifying the firmware image.
> + ///
> + UINT64 ImageId;
> + ///
> + /// A pointer to a null-terminated string representing the firmware
> + image
> name.
> + ///
> + CHAR16 *ImageIdName;
> + ///
> + /// Identifies the version of the device firmware. The format is
> + vendor
> specific and new
> + /// version must have a greater value than an old version.
> + ///
> + UINT32 Version;
> + ///
> + /// A pointer to a null-terminated string representing the firmware
> + image
> version name.
> + ///
> + CHAR16 *VersionName;
> + ///
> + /// Size of the image in bytes. If size=0, then only ImageIndex and
> ImageTypeId are valid.
> + ///
> + UINTN Size;
> + ///
> + /// Image attributes that are supported by this device. See 'Image
> + Attribute
> Definitions'
> + /// for possible returned values of this parameter. A value of 1
> + indicates
> the attribute is
> + /// supported and the current setting value is indicated in AttributesSetting.
> A
> + /// value of 0 indicates the attribute is not supported and the
> + current
> setting value in
> + /// AttributesSetting is meaningless.
> + ///
> + UINT64 AttributesSupported;
> + ///
> + /// Image attributes. See 'Image Attribute Definitions' for possible
> + returned
> values of
> + /// this parameter.
> + ///
> + UINT64 AttributesSetting;
> + ///
> + /// Image compatibilities. See 'Image Compatibility Definitions'
> + for possible
> returned
> + /// values of this parameter.
> + ///
> + UINT64 Compatibilities;
> + ///
> + /// Describes the lowest ImageDescriptor version that the device
> + will
> accept. Only
> + /// present in version 2 or higher.
> + UINT32 LowestSupportedImageVersion;
> +} EFI_FIRMWARE_IMAGE_DESCRIPTOR_V2;
>
> //
> // Image Attribute Definitions
> --
> 2.6.2.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel