Re: [edk2-devel] [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support multiple devices with 0 HardwareInstance

["Michael D Kinney" <michael.d.kinney@intel.com>]

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@intel.com>



Best regards,

Mike

> -----Original Message-----
> From: Jeff Brasen <jbrasen@nvidia.com>
> Sent: Friday, February 10, 2023 1:21 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; Demeter, Miki <miki.demeter@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@intel.com>
> > Sent: Friday, February 10, 2023 2:17 PM
> > To: devel@edk2.groups.io; Jeff Brasen <jbrasen@nvidia.com>; Demeter,
> > Miki <miki.demeter@intel.com>
> > Cc: Kinney, Michael D <michael.d.kinney@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@edk2.groups.io <devel@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@intel.com>;
> > > devel@edk2.groups.io; Demeter, Miki <miki.demeter@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@intel.com>
> > > > Sent: Friday, February 10, 2023 12:48 PM
> > > > To: devel@edk2.groups.io; Jeff Brasen <jbrasen@nvidia.com>; Demeter,
> > > > Miki <miki.demeter@intel.com>
> > > > Cc: Kinney, Michael D <michael.d.kinney@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@edk2.groups.io <devel@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@intel.com>;
> > > > > devel@edk2.groups.io; Demeter, Miki <miki.demeter@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@intel.com>
> > > > > > Sent: Thursday, February 9, 2023 8:43 PM
> > > > > > To: devel@edk2.groups.io; Jeff Brasen <jbrasen@nvidia.com>;
> > > > > > Demeter, Miki <miki.demeter@intel.com>
> > > > > > Cc: Kinney, Michael D <michael.d.kinney@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > > > > Jeff Brasen via groups.io
> > > > > > > Sent: Thursday, February 9, 2023 2:03 PM
> > > > > > > To: Demeter, Miki <miki.demeter@intel.com>
> > > > > > > Cc: devel@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@edk2.groups.io
> > > > > > > Cc: jian.j.wang@intel.com; gaoliming@byosoft.com.cn;
> > > > > > > guomin.jiang@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@nvidia.com>
> > > > > > > > Sent: Monday, December 12, 2022 12:27 PM
> > > > > > > > To: devel@edk2.groups.io
> > > > > > > > Cc: jian.j.wang@intel.com; gaoliming@byosoft.com.cn;
> > > > > > > > guomin.jiang@intel.com; Jeff Brasen <jbrasen@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@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
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > >
> > >
> > >
> > > 
> > >