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

["Jeff Brasen" <jbrasen@nvidia.com>]

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.html#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
> >
> >
> >
> > 
> >