[edk2-devel] [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support multiple devices with 0 HardwareInstance
Michael D Kinney
michael.d.kinney at intel.com
Tue Feb 14 23:50:23 UTC 2023
Hi Jeff,
I have been studying the code for side effects from Version >= 3 and HardwareInstance = 0.
I think the only side effect with your patch is extra entries in HardwareIntances with the
same GUID value and HardwareInstance value of 0.
IN OUT GUID_HARDWAREINSTANCE_PAIR *HardwareInstances,
IN OUT UINT32 *NumberOfDescriptors,
This table is only used to look for duplicate GUID/HardwareInstance pairs and is not used
anywhere else and is freed after ESRT entries are determined. The same number of ESRT entries
are be created even if these extra HardwareInstances entries are present.
I have also not seen any comments or concerns from anyone else on this topic.
Reviewed-by: Michael D Kinney <michael.d.kinney at intel.com>
Best regards,
Mike
> -----Original Message-----
> From: Jeff Brasen <jbrasen at nvidia.com>
> Sent: Friday, February 10, 2023 1:21 PM
> To: Kinney, Michael D <michael.d.kinney at intel.com>; devel at edk2.groups.io; Demeter, Miki <miki.demeter at intel.com>
> Subject: RE: [edk2-devel] [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support multiple devices with 0 HardwareInstance
>
> This is unclear in my opinion. The spec does have
> This number must be unique within the namespace of the ImageTypeId GUID and ImageIndex. And
> For implementations that will never have more than one instance a zero can be used.
>
> But it also states the parameter is optional and states that zero can be used when if it is not able to determine a unique
> hardware instance number or a hardware instance number is not needed.
>
> I could see reasonable arguments on both sides.
>
> -Jeff
>
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney at intel.com>
> > Sent: Friday, February 10, 2023 2:17 PM
> > To: devel at edk2.groups.io; Jeff Brasen <jbrasen at nvidia.com>; Demeter,
> > Miki <miki.demeter at intel.com>
> > Cc: Kinney, Michael D <michael.d.kinney at intel.com>
> > Subject: RE: [edk2-devel] [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support
> > multiple devices with 0 HardwareInstance
> >
> > External email: Use caution opening links or attachments
> >
> >
> > Based on UEFI Spec, would you consider those option roms to be non-
> > conformant?
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Jeff
> > > Brasen via groups.io
> > > Sent: Friday, February 10, 2023 12:43 PM
> > > To: Kinney, Michael D <michael.d.kinney at intel.com>;
> > > devel at edk2.groups.io; Demeter, Miki <miki.demeter at intel.com>
> > > Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/EsrtFmpDxe:
> > Support
> > > multiple devices with 0 HardwareInstance
> > >
> > > It was not as we were seeing devices expose FMP via an option rom
> > > driver with Version == 3 and HardwareInstance = 0. If there are multiple of
> > these device it would hit the error in the EsrtCode. The patch included does
> > resolve the issues we were seeing.
> > >
> > > -Jeff
> > >
> > >
> > > > -----Original Message-----
> > > > From: Kinney, Michael D <michael.d.kinney at intel.com>
> > > > Sent: Friday, February 10, 2023 12:48 PM
> > > > To: devel at edk2.groups.io; Jeff Brasen <jbrasen at nvidia.com>; Demeter,
> > > > Miki <miki.demeter at intel.com>
> > > > Cc: Kinney, Michael D <michael.d.kinney at intel.com>
> > > > Subject: RE: [edk2-devel] [PATCH v2] MdeModulePkg/EsrtFmpDxe:
> > > > Support multiple devices with 0 HardwareInstance
> > > >
> > > > External email: Use caution opening links or attachments
> > > >
> > > >
> > > > Did your original approach work for all your use cases?
> > > >
> > > > Mike
> > > >
> > > > > -----Original Message-----
> > > > > From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
> > > > > Jeff Brasen via groups.io
> > > > > Sent: Friday, February 10, 2023 8:45 AM
> > > > > To: Kinney, Michael D <michael.d.kinney at intel.com>;
> > > > > devel at edk2.groups.io; Demeter, Miki <miki.demeter at intel.com>
> > > > > Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/EsrtFmpDxe:
> > > > Support
> > > > > multiple devices with 0 HardwareInstance
> > > > >
> > > > > That was my original approach when I saw this but I was seeing
> > > > > this occur on a device with FmpVersion == 3. The UEFI spec isn't
> > > > > completely
> > > > clear but could be read that 0 is legal if this part isn't being used.
> > > > >
> > > > >
> > > >
> > https://uefi.org/specs/UEFI/2.10/23_Firmware_Update_and_Reporting.ht
> > > > m
> > > > l
> > > > > #efi-firmware-management-protocol-getimageinfo
> > > > >
> > > > > It does state that this is Optional, and has a statement "A zero
> > > > > means the FMP provider is not able to determine a unique hardware
> > > > > instance
> > > > number or a hardware instance number is not needed."
> > > > >
> > > > > Also, our ESRT implementation just merges all instances to one
> > > > > anyways so it doesn't currently consume the hardware instance
> > > > > except for this
> > > > check.
> > > > >
> > > > > Thanks,
> > > > > Jeff
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Kinney, Michael D <michael.d.kinney at intel.com>
> > > > > > Sent: Thursday, February 9, 2023 8:43 PM
> > > > > > To: devel at edk2.groups.io; Jeff Brasen <jbrasen at nvidia.com>;
> > > > > > Demeter, Miki <miki.demeter at intel.com>
> > > > > > Cc: Kinney, Michael D <michael.d.kinney at intel.com>
> > > > > > Subject: RE: [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support
> > > > > > multiple devices with 0 HardwareInstance
> > > > > >
> > > > > > External email: Use caution opening links or attachments
> > > > > >
> > > > > >
> > > > > > Hi Jeff,
> > > > > >
> > > > > > Thank you for the reminder.
> > > > > >
> > > > > > I am wondering if the check should be for FmpVersion < 3 and not
> > > > > > FmpHardwareInstance != 0.
> > > > > >
> > > > > > It is possible for an FmpInfoBuffer to have an FmpVersion >= 3
> > > > > > and a HardwareInstance of 0. If more than one of these it
> > > > > > found, then that would be an EFI_UNSUPPORTED condition.
> > > > > >
> > > > > > If FmpVersion >= 3, then the HardwareInstance must be filled in
> > > > > > with a unique value. 0 can be used if there is at most 1
> > > > > > instance, so the duplicate check would never be triggered. If
> > > > > > FmpVersion >= 3 and there can be more then one instance, then
> > > > > > the HardwareInstance must be a non-zero unique value.
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > Mike
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
> > > > > > > Jeff Brasen via groups.io
> > > > > > > Sent: Thursday, February 9, 2023 2:03 PM
> > > > > > > To: Demeter, Miki <miki.demeter at intel.com>
> > > > > > > Cc: devel at edk2.groups.io
> > > > > > > Subject: [edk2-devel] FW: [PATCH v2]
> > MdeModulePkg/EsrtFmpDxe:
> > > > > > Support
> > > > > > > multiple devices with 0 HardwareInstance
> > > > > > >
> > > > > > > Here is a patch that has been pending for a bit
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Jeff
> > > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Jeff Brasen
> > > > > > > Sent: Wednesday, February 1, 2023 9:21 AM
> > > > > > > To: devel at edk2.groups.io
> > > > > > > Cc: jian.j.wang at intel.com; gaoliming at byosoft.com.cn;
> > > > > > > guomin.jiang at intel.com
> > > > > > > Subject: RE: [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support
> > > > > > > multiple devices with 0 HardwareInstance
> > > > > > >
> > > > > > > Any updates on this would be great to get this in for the next
> > > > > > > stable release
> > > > > > if possible.
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Jeff Brasen <jbrasen at nvidia.com>
> > > > > > > > Sent: Monday, December 12, 2022 12:27 PM
> > > > > > > > To: devel at edk2.groups.io
> > > > > > > > Cc: jian.j.wang at intel.com; gaoliming at byosoft.com.cn;
> > > > > > > > guomin.jiang at intel.com; Jeff Brasen <jbrasen at nvidia.com>
> > > > > > > > Subject: [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support
> > > > > > > > multiple devices with 0 HardwareInstance
> > > > > > > >
> > > > > > > > Skip error check if HardwareInstance is 0 as this either
> > > > > > > > means that FmpVersion < 3 and not supported or, "A zero
> > > > > > > > means the FMP provider is not able to determine a unique
> > > > > > > > hardware instance number or a hardware instance number is
> > > > > > > > not needed." per UEFI
> > > > specification.
> > > > > > > >
> > > > > > > > As the FmpInstances are merged and HardwareInstance is not
> > > > > > > > used remove error check in this case.
> > > > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Jeff Brasen <jbrasen at nvidia.com>
> > > > > > > > ---
> > > > > > > > MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c | 22
> > > > ++++++++++++--
> > > > > > ----
> > > > > > > > ---
> > > > > > > > 1 file changed, 13 insertions(+), 9 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> > > > > > > > b/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> > > > > > > > index 4f47c55cce..5bc627461d 100644
> > > > > > > > --- a/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> > > > > > > > +++ b/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> > > > > > > > @@ -153,16 +153,20 @@ CreateEsrtEntry (
> > > > > > > >
> > > > > > > > //
> > > > > > > > // Check to see of FmpImageInfoBuf GUID/HardwareInstance
> > > > > > > > is unique
> > > > > > > > + // Skip if HardwareInstance is 0 as this is the case if
> > > > > > > > + FmpVersion <
> > > > > > > > + 3 // or the device can not create a unique ID per UEFI
> > > > > > > > + specification
> > > > > > > > //
> > > > > > > > - for (Index = 0; Index < *NumberOfDescriptors; Index++) {
> > > > > > > > - if (CompareGuid (&HardwareInstances[Index].ImageTypeGuid,
> > > > > > > > &FmpImageInfoBuf->ImageTypeId)) {
> > > > > > > > - if (HardwareInstances[Index].HardwareInstance ==
> > > > > > > > FmpHardwareInstance) {
> > > > > > > > - DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Duplicate firmware
> > > > image
> > > > > > > > descriptor with GUID %g HardwareInstance:0x%x\n",
> > > > > > &FmpImageInfoBuf-
> > > > > > > > >ImageTypeId, FmpHardwareInstance));
> > > > > > > > - ASSERT (
> > > > > > > > - !CompareGuid
> > (&HardwareInstances[Index].ImageTypeGuid,
> > > > > > > > &FmpImageInfoBuf->ImageTypeId) ||
> > > > > > > > - HardwareInstances[Index].HardwareInstance !=
> > > > > > > > FmpHardwareInstance
> > > > > > > > - );
> > > > > > > > - return EFI_UNSUPPORTED;
> > > > > > > > + if (FmpHardwareInstance != 0) {
> > > > > > > > + for (Index = 0; Index < *NumberOfDescriptors; Index++) {
> > > > > > > > + if (CompareGuid
> > > > > > > > + (&HardwareInstances[Index].ImageTypeGuid,
> > > > > > > > &FmpImageInfoBuf->ImageTypeId)) {
> > > > > > > > + if (HardwareInstances[Index].HardwareInstance ==
> > > > > > > > FmpHardwareInstance) {
> > > > > > > > + DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Duplicate
> > > > > > > > + firmware image
> > > > > > > > descriptor with GUID %g HardwareInstance:0x%x\n",
> > > > > > &FmpImageInfoBuf-
> > > > > > > > >ImageTypeId, FmpHardwareInstance));
> > > > > > > > + ASSERT (
> > > > > > > > + !CompareGuid
> > > > > > > > + (&HardwareInstances[Index].ImageTypeGuid,
> > > > > > > > &FmpImageInfoBuf->ImageTypeId) ||
> > > > > > > > + HardwareInstances[Index].HardwareInstance !=
> > > > > > > > FmpHardwareInstance
> > > > > > > > + );
> > > > > > > > + return EFI_UNSUPPORTED;
> > > > > > > > + }
> > > > > > > > }
> > > > > > > > }
> > > > > > > > }
> > > > > > > > --
> > > > > > > > 2.25.1
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > >
> > >
> > >
> > >
> > >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100202): https://edk2.groups.io/g/devel/message/100202
Mute This Topic: https://groups.io/mt/96869771/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3943202/1813853/130120423/xyzzy [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list