Recent changes to EsrtFmp causing ASSERTs

Recent changes to EsrtFmp causing ASSERTs

[jason.spottswood]

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

In EsrtFmp.c, function CreateEsrtEntry, line 196, the code asserts if the FMP image hardware instance matches that of an existing instance.  This is fine if the hardware instance is supported.  The field is optional though.  In the UEFI spec, "a zero hardware instance means the FMP provider is not able to determine a unique hardware instance number or a hardware instance number is not needed."  The code below needs to also check if the hardware instance is supported (by comparing it to zero) before checking it against existing entries.

 //
 // Check to see of FmpImageInfoBuf GUID/HardwareInstance is unique
 //
 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;
     }
   }
 }

[-- Attachment #2: Type: text/html, Size: 1683 bytes --]

Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

[Rothman, Michael A]

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

Jason, Agreed - though that image you sent was challenging for these old eyes. Black on dark grey? Ack!

Thanks,
Michael A. Rothman
---------------------------------------------------------------
Let no excuse be a barrier to your success.

On Sep 30, 2019, at 1:48 PM, "jason.spottswood@hpe.com<mailto:jason.spottswood@hpe.com>" <jason.spottswood@hpe.com<mailto:jason.spottswood@hpe.com>> wrote:

In EsrtFmp.c, function CreateEsrtEntry, line 196, the code asserts if the FMP image hardware instance matches that of an existing instance.  This is fine if the hardware instance is supported.  The field is optional though.  In the UEFI spec, "a zero hardware instance means the FMP provider is not able to determine a unique hardware instance number or a hardware instance number is not needed."  The code below needs to also check if the hardware instance is supported (by comparing it to zero) before checking it against existing entries.


  //
  // Check to see of FmpImageInfoBuf GUID/HardwareInstance is unique
  //
  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;
      }
    }
  }




[-- Attachment #2: Type: text/html, Size: 2768 bytes --]

Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

[jason.spottswood]

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

Cut-n-paste problem. My apologies.

Copying again in plain text:

  //
  // Check to see of FmpImageInfoBuf GUID/HardwareInstance is unique
  //
  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;
      }
    }
  }

-Jason

From: Rothman, Michael A <michael.a.rothman@intel.com>
Sent: Monday, September 30, 2019 3:53 PM
To: devel@edk2.groups.io; Spottswood, Jason <jason.spottswood@hpe.com>
Subject: Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

Jason, Agreed - though that image you sent was challenging for these old eyes. Black on dark grey? Ack!
Thanks,
Michael A. Rothman
---------------------------------------------------------------
Let no excuse be a barrier to your success.

On Sep 30, 2019, at 1:48 PM, "jason.spottswood@hpe.com<mailto:jason.spottswood@hpe.com>" <jason.spottswood@hpe.com<mailto:jason.spottswood@hpe.com>> wrote:
In EsrtFmp.c, function CreateEsrtEntry, line 196, the code asserts if the FMP image hardware instance matches that of an existing instance.  This is fine if the hardware instance is supported.  The field is optional though.  In the UEFI spec, "a zero hardware instance means the FMP provider is not able to determine a unique hardware instance number or a hardware instance number is not needed."  The code below needs to also check if the hardware instance is supported (by comparing it to zero) before checking it against existing entries.

  //

  // Check to see of FmpImageInfoBuf GUID/HardwareInstance is unique

  //

  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;

      }

    }

  }


[-- Attachment #2: Type: text/html, Size: 10726 bytes --]

Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

[Sean]

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

Jason/Mike,

What would you propose is done if the hw instance in 0 but an entry with same guid and instance 0 already exists?
Ignore it and first in wins?

It asserts because i am not aware of the right answer for the ESRT and it creates a situation with unpredictable results for capsule processing using ESRT.

Thanks
Sean

[-- Attachment #2: Type: text/html, Size: 397 bytes --]

Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

[Michael D Kinney]

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

Hi Jason,

I believe the logic to check for uniqueness of FMP Descriptor is correct.

The UEFI Spec has 2 structs with HardwareInstance.  One is FMP Descriptor and the other is UEFI Capsule for FMP.

The HardwareInstance in FMP must be unique and not 0 unless there is a guarantee there is only one instance.

  ///
  /// An optional number to identify the unique hardware instance within the system for
  /// devices that may have multiple instances (Example: a plug in pci network card). This
  /// number must be unique within the namespace of the ImageTypeId GUID and
  /// ImageIndex. For FMP instances that have multiple descriptors for a single
  /// hardware instance, all descriptors must have the same HardwareInstance value.
  /// This number must be consistent between boots and should be based on some sort of
  /// hardware identified unique id (serial number, etc) whenever possible. If a hardware
  /// based number is not available the FMP provider may use some other characteristic
  /// such as device path, bus/dev/function, slot num, etc for generating the
  /// HardwareInstance. For implementations that will never have more than one
  /// instance a zero can be used. A zero means the FMP provider is not able to determine a
  /// unique hardware instance number or a hardware instance number is not needed. Only
  /// present in version 3 or higher.
  ///
  UINT64                           HardwareInstance;
} EFI_FIRMWARE_IMAGE_DESCRIPTOR;

The UEFI Capsule one can be 0 to specify match any.

  ///
  /// The HardwareInstance to target with this update. If value is zero it means match all
  /// HardwareInstances. This field allows update software to target only a single device in
  /// cases where there are more than one device with the same ImageTypeId GUID.
  /// This header is outside the signed data of the Authentication Info structure and
  /// therefore can be modified without changing the Auth data.
  ///
  UINT64   UpdateHardwareInstance;
} EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER;

Best regards,

Mike


From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Spottswood, Jason
Sent: Monday, September 30, 2019 2:24 PM
To: Rothman, Michael A <michael.a.rothman@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

Cut-n-paste problem. My apologies.

Copying again in plain text:

  //
  // Check to see of FmpImageInfoBuf GUID/HardwareInstance is unique
  //
  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;
      }
    }
  }

-Jason

From: Rothman, Michael A <michael.a.rothman@intel.com<mailto:michael.a.rothman@intel.com>>
Sent: Monday, September 30, 2019 3:53 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Spottswood, Jason <jason.spottswood@hpe.com<mailto:jason.spottswood@hpe.com>>
Subject: Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

Jason, Agreed - though that image you sent was challenging for these old eyes. Black on dark grey? Ack!
Thanks,
Michael A. Rothman
---------------------------------------------------------------
Let no excuse be a barrier to your success.

On Sep 30, 2019, at 1:48 PM, "jason.spottswood@hpe.com<mailto:jason.spottswood@hpe.com>" <jason.spottswood@hpe.com<mailto:jason.spottswood@hpe.com>> wrote:
In EsrtFmp.c, function CreateEsrtEntry, line 196, the code asserts if the FMP image hardware instance matches that of an existing instance.  This is fine if the hardware instance is supported.  The field is optional though.  In the UEFI spec, "a zero hardware instance means the FMP provider is not able to determine a unique hardware instance number or a hardware instance number is not needed."  The code below needs to also check if the hardware instance is supported (by comparing it to zero) before checking it against existing entries.

  //

  // Check to see of FmpImageInfoBuf GUID/HardwareInstance is unique

  //

  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;

      }

    }

  }


[-- Attachment #2: Type: text/html, Size: 58753 bytes --]

Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

[Spottswood, Jason]

[-- Attachment #1.1: Type: text/plain, Size: 7146 bytes --]

Hey Mike,

Specifically, my team has run into ASSERTs on systems loaded with many identical model HDDs.  Each HDD provides an FMP where the type GUID is identical, and the HW instance is not provided (zero).  Attached serial log with additional debug output to print the FMP version. (We commented out the ASSERT to gather the log.)

Given the current text in the UEFI spec, I do not believe the device vendor is necessarily at fault here, even if I would like for them to implement the HW instance.


  1.  First sentence from the FW image HW instance description says this field is optional.
 ///
  /// An optional number to identify the unique hardware instance within the system for
  /// devices that may have multiple instances (Example: a plug in pci network card). This


  1.  Near the end of the FW image HW instance description says that an FMP can use zero to mean uniqueness cannot be determined.
 /// instance a zero can be used. A zero means the FMP provider is not able to determine a
  /// unique hardware instance number or a hardware instance number is not needed. Only
  /// present in version 3 or higher.

Another point is that the ESRT can only have one entry for each given type GUID.  There is also no HW instance field for entries in the ESRT.  As it relates to building the ESRT, checking for duplicate type GUID/IDs is the only requirement, not HW instance.

-Jason

From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Tuesday, October 1, 2019 12:31 PM
To: devel@edk2.groups.io; Spottswood, Jason <jason.spottswood@hpe.com>; Rothman, Michael A <michael.a.rothman@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

Hi Jason,

I believe the logic to check for uniqueness of FMP Descriptor is correct.

The UEFI Spec has 2 structs with HardwareInstance.  One is FMP Descriptor and the other is UEFI Capsule for FMP.

The HardwareInstance in FMP must be unique and not 0 unless there is a guarantee there is only one instance.

  ///
  /// An optional number to identify the unique hardware instance within the system for
  /// devices that may have multiple instances (Example: a plug in pci network card). This
  /// number must be unique within the namespace of the ImageTypeId GUID and
  /// ImageIndex. For FMP instances that have multiple descriptors for a single
  /// hardware instance, all descriptors must have the same HardwareInstance value.
  /// This number must be consistent between boots and should be based on some sort of
  /// hardware identified unique id (serial number, etc) whenever possible. If a hardware
  /// based number is not available the FMP provider may use some other characteristic
  /// such as device path, bus/dev/function, slot num, etc for generating the
  /// HardwareInstance. For implementations that will never have more than one
  /// instance a zero can be used. A zero means the FMP provider is not able to determine a
 /// unique hardware instance number or a hardware instance number is not needed. Only
  /// present in version 3 or higher.
  ///
  UINT64                           HardwareInstance;
} EFI_FIRMWARE_IMAGE_DESCRIPTOR;

The UEFI Capsule one can be 0 to specify match any.

  ///
  /// The HardwareInstance to target with this update. If value is zero it means match all
  /// HardwareInstances. This field allows update software to target only a single device in
  /// cases where there are more than one device with the same ImageTypeId GUID.
  /// This header is outside the signed data of the Authentication Info structure and
  /// therefore can be modified without changing the Auth data.
  ///
  UINT64   UpdateHardwareInstance;
} EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER;

Best regards,

Mike


From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Spottswood, Jason
Sent: Monday, September 30, 2019 2:24 PM
To: Rothman, Michael A <michael.a.rothman@intel.com<mailto:michael.a.rothman@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

Cut-n-paste problem. My apologies.

Copying again in plain text:

  //
  // Check to see of FmpImageInfoBuf GUID/HardwareInstance is unique
  //
  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;
      }
    }
  }

-Jason

From: Rothman, Michael A <michael.a.rothman@intel.com<mailto:michael.a.rothman@intel.com>>
Sent: Monday, September 30, 2019 3:53 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Spottswood, Jason <jason.spottswood@hpe.com<mailto:jason.spottswood@hpe.com>>
Subject: Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

Jason, Agreed - though that image you sent was challenging for these old eyes. Black on dark grey? Ack!
Thanks,
Michael A. Rothman
---------------------------------------------------------------
Let no excuse be a barrier to your success.

On Sep 30, 2019, at 1:48 PM, "jason.spottswood@hpe.com<mailto:jason.spottswood@hpe.com>" <jason.spottswood@hpe.com<mailto:jason.spottswood@hpe.com>> wrote:
In EsrtFmp.c, function CreateEsrtEntry, line 196, the code asserts if the FMP image hardware instance matches that of an existing instance.  This is fine if the hardware instance is supported.  The field is optional though.  In the UEFI spec, "a zero hardware instance means the FMP provider is not able to determine a unique hardware instance number or a hardware instance number is not needed."  The code below needs to also check if the hardware instance is supported (by comparing it to zero) before checking it against existing entries.

  //

  // Check to see of FmpImageInfoBuf GUID/HardwareInstance is unique

  //

  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;

      }

    }

  }


[-- Attachment #1.2: Type: text/html, Size: 21770 bytes --]

[-- Attachment #2: serial.txt --]
[-- Type: text/plain, Size: 2539 bytes --]

EsrtFmpDxe:  FmpVersion: 0x3,  HardwareInstance:0x0
EsrtFmpDxe: Add new image descriptor with GUID 164FFEF2-15D3-4F7C-BAE6-72097DE8AF78 HardwareInstance:0x0
EsrtFmpDxe:  FmpVersion: 0x3,  HardwareInstance:0x0
EsrtFmpDxe: Duplicate firmware image descriptor with GUID 164FFEF2-15D3-4F7C-BAE6-72097DE8AF78 HardwareInstance:0x0
EsrtFmpDxe:  FmpVersion: 0x3,  HardwareInstance:0x0
EsrtFmpDxe: Duplicate firmware image descriptor with GUID 164FFEF2-15D3-4F7C-BAE6-72097DE8AF78 HardwareInstance:0x0
EsrtFmpDxe:  FmpVersion: 0x3,  HardwareInstance:0x0
EsrtFmpDxe: Duplicate firmware image descriptor with GUID 164FFEF2-15D3-4F7C-BAE6-72097DE8AF78 HardwareInstance:0x0
EsrtFmpDxe:  FmpVersion: 0x3,  HardwareInstance:0x0
EsrtFmpDxe: Duplicate firmware image descriptor with GUID 164FFEF2-15D3-4F7C-BAE6-72097DE8AF78 HardwareInstance:0x0
EsrtFmpDxe:  FmpVersion: 0x3,  HardwareInstance:0x0
EsrtFmpDxe: Duplicate firmware image descriptor with GUID 164FFEF2-15D3-4F7C-BAE6-72097DE8AF78 HardwareInstance:0x0
EsrtFmpDxe:  FmpVersion: 0x3,  HardwareInstance:0x0
EsrtFmpDxe: Duplicate firmware image descriptor with GUID 164FFEF2-15D3-4F7C-BAE6-72097DE8AF78 HardwareInstance:0x0
EsrtFmpDxe:  FmpVersion: 0x3,  HardwareInstance:0x0
EsrtFmpDxe: Duplicate firmware image descriptor with GUID 164FFEF2-15D3-4F7C-BAE6-72097DE8AF78 HardwareInstance:0x0
EsrtFmpDxe:  FmpVersion: 0x3,  HardwareInstance:0x0
EsrtFmpDxe: Duplicate firmware image descriptor with GUID 164FFEF2-15D3-4F7C-BAE6-72097DE8AF78 HardwareInstance:0x0
EsrtFmpDxe:  FmpVersion: 0x3,  HardwareInstance:0x0
EsrtFmpDxe: Duplicate firmware image descriptor with GUID 164FFEF2-15D3-4F7C-BAE6-72097DE8AF78 HardwareInstance:0x0
EsrtFmpDxe:  FmpVersion: 0x3,  HardwareInstance:0x0
EsrtFmpDxe: Duplicate firmware image descriptor with GUID 164FFEF2-15D3-4F7C-BAE6-72097DE8AF78 HardwareInstance:0x0
EsrtFmpDxe:  FmpVersion: 0x3,  HardwareInstance:0x0
EsrtFmpDxe: Duplicate firmware image descriptor with GUID 164FFEF2-15D3-4F7C-BAE6-72097DE8AF78 HardwareInstance:0x0
EsrtFmpDxe:  FmpVersion: 0x3,  HardwareInstance:0x0
EsrtFmpDxe: Duplicate firmware image descriptor with GUID 164FFEF2-15D3-4F7C-BAE6-72097DE8AF78 HardwareInstance:0x0
EsrtFmpDxe:  FmpVersion: 0x3,  HardwareInstance:0x0
EsrtFmpDxe: Duplicate firmware image descriptor with GUID 164FFEF2-15D3-4F7C-BAE6-72097DE8AF78 HardwareInstance:0x0
EsrtFmpDxe:  FmpVersion: 0x3,  HardwareInstance:0x0
EsrtFmpDxe: Duplicate firmware image descriptor with GUID 164FFEF2-15D3-4F7C-BAE6-72097DE8AF78 HardwareInstance:0x0

Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

[Michael D Kinney]

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

Jason,

The only time the HardwareInstance is optional (and set to 0) is if the system can guarantee that there is at most one instance of the device in the system.  This can only be guaranteed for an integrated device.  Any devices that an end user can add/remove from the system through slots or ports may potentially have multiple instances and for those FMP use cases the FMP driver must provide a unique HardwareInstance value.

You did not quote all the text that applies in point (2) below.  You need the additional sentence in front to know that 0 can only be used if there is a guarantee there will never be more than 1 instance.

For implementations that will never have more than one
instance a zero can be used. A zero means the FMP provider is not able to determine a
unique hardware instance number or a hardware instance number is not needed. Only
present in version 3 or higher.

Mike

From: Spottswood, Jason <jason.spottswood@hpe.com>
Sent: Tuesday, October 1, 2019 2:21 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; Rothman, Michael A <michael.a.rothman@intel.com>
Subject: RE: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

Hey Mike,

Specifically, my team has run into ASSERTs on systems loaded with many identical model HDDs.  Each HDD provides an FMP where the type GUID is identical, and the HW instance is not provided (zero).  Attached serial log with additional debug output to print the FMP version. (We commented out the ASSERT to gather the log.)

Given the current text in the UEFI spec, I do not believe the device vendor is necessarily at fault here, even if I would like for them to implement the HW instance.


  1.  First sentence from the FW image HW instance description says this field is optional.
 ///
  /// An optional number to identify the unique hardware instance within the system for
  /// devices that may have multiple instances (Example: a plug in pci network card). This


  1.  Near the end of the FW image HW instance description says that an FMP can use zero to mean uniqueness cannot be determined.
 /// instance a zero can be used. A zero means the FMP provider is not able to determine a
  /// unique hardware instance number or a hardware instance number is not needed. Only
  /// present in version 3 or higher.

Another point is that the ESRT can only have one entry for each given type GUID.  There is also no HW instance field for entries in the ESRT.  As it relates to building the ESRT, checking for duplicate type GUID/IDs is the only requirement, not HW instance.

-Jason

From: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Sent: Tuesday, October 1, 2019 12:31 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Spottswood, Jason <jason.spottswood@hpe.com<mailto:jason.spottswood@hpe.com>>; Rothman, Michael A <michael.a.rothman@intel.com<mailto:michael.a.rothman@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Subject: RE: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

Hi Jason,

I believe the logic to check for uniqueness of FMP Descriptor is correct.

The UEFI Spec has 2 structs with HardwareInstance.  One is FMP Descriptor and the other is UEFI Capsule for FMP.

The HardwareInstance in FMP must be unique and not 0 unless there is a guarantee there is only one instance.

  ///
  /// An optional number to identify the unique hardware instance within the system for
  /// devices that may have multiple instances (Example: a plug in pci network card). This
  /// number must be unique within the namespace of the ImageTypeId GUID and
  /// ImageIndex. For FMP instances that have multiple descriptors for a single
  /// hardware instance, all descriptors must have the same HardwareInstance value.
  /// This number must be consistent between boots and should be based on some sort of
  /// hardware identified unique id (serial number, etc) whenever possible. If a hardware
  /// based number is not available the FMP provider may use some other characteristic
  /// such as device path, bus/dev/function, slot num, etc for generating the
  /// HardwareInstance. For implementations that will never have more than one
  /// instance a zero can be used. A zero means the FMP provider is not able to determine a
 /// unique hardware instance number or a hardware instance number is not needed. Only
  /// present in version 3 or higher.
  ///
  UINT64                           HardwareInstance;
} EFI_FIRMWARE_IMAGE_DESCRIPTOR;

The UEFI Capsule one can be 0 to specify match any.

  ///
  /// The HardwareInstance to target with this update. If value is zero it means match all
  /// HardwareInstances. This field allows update software to target only a single device in
  /// cases where there are more than one device with the same ImageTypeId GUID.
  /// This header is outside the signed data of the Authentication Info structure and
  /// therefore can be modified without changing the Auth data.
  ///
  UINT64   UpdateHardwareInstance;
} EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER;

Best regards,

Mike


From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Spottswood, Jason
Sent: Monday, September 30, 2019 2:24 PM
To: Rothman, Michael A <michael.a.rothman@intel.com<mailto:michael.a.rothman@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

Cut-n-paste problem. My apologies.

Copying again in plain text:

  //
  // Check to see of FmpImageInfoBuf GUID/HardwareInstance is unique
  //
  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;
      }
    }
  }

-Jason

From: Rothman, Michael A <michael.a.rothman@intel.com<mailto:michael.a.rothman@intel.com>>
Sent: Monday, September 30, 2019 3:53 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Spottswood, Jason <jason.spottswood@hpe.com<mailto:jason.spottswood@hpe.com>>
Subject: Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

Jason, Agreed - though that image you sent was challenging for these old eyes. Black on dark grey? Ack!
Thanks,
Michael A. Rothman
---------------------------------------------------------------
Let no excuse be a barrier to your success.

On Sep 30, 2019, at 1:48 PM, "jason.spottswood@hpe.com<mailto:jason.spottswood@hpe.com>" <jason.spottswood@hpe.com<mailto:jason.spottswood@hpe.com>> wrote:
In EsrtFmp.c, function CreateEsrtEntry, line 196, the code asserts if the FMP image hardware instance matches that of an existing instance.  This is fine if the hardware instance is supported.  The field is optional though.  In the UEFI spec, "a zero hardware instance means the FMP provider is not able to determine a unique hardware instance number or a hardware instance number is not needed."  The code below needs to also check if the hardware instance is supported (by comparing it to zero) before checking it against existing entries.

  //

  // Check to see of FmpImageInfoBuf GUID/HardwareInstance is unique

  //

  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;

      }

    }

  }


[-- Attachment #2: Type: text/html, Size: 62926 bytes --]

Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

[scott.wiginton]

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

Hi Michael,

I'm not sure that I agree with your last statement.  The says that HardwareInstance is "Only present in version 3 or higher" in reference to the EFI_FIRMWARE_IMAGE_DESCRIPTOR.  Isn't that the point of the DescriptorVersion parameter in FMP.GetImageInfo?  To know if that field is event valid or not.  The function in question even looks at this passed in FW image descriptor version (though the parameter is called FmpVersion).  The code just sets it to 0 if the FmpVersion is < 3.  If one or more devices producing FMPs in the system do not support FW image descriptor 3 or later, then this will cause an ASSERT.

Thanks,
SWig

[-- Attachment #2: Type: text/html, Size: 710 bytes --]

Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

[Michael D Kinney]

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

Devices with FMP instances below version 3 do not support a HardwareInstance so they can only support single device for each GUID.  FMP implementations with versions below 3 that want to support multiple devices need to use multiple GUIDs.

The ASSERT() is still be correct if there are multiple FMP instances with the same GUID and their version is below 3 that uses an assumed HardwareInstance of 0.

Mike

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of scott.wiginton@hpe.com
Sent: Friday, October 18, 2019 2:52 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

Hi Michael,

I'm not sure that I agree with your last statement.  The says that HardwareInstance is "Only present in version 3 or higher" in reference to the EFI_FIRMWARE_IMAGE_DESCRIPTOR.  Isn't that the point of the DescriptorVersion parameter in FMP.GetImageInfo?  To know if that field is event valid or not.  The function in question even looks at this passed in FW image descriptor version (though the parameter is called FmpVersion).  The code just sets it to 0 if the FmpVersion is < 3.  If one or more devices producing FMPs in the system do not support FW image descriptor 3 or later, then this will cause an ASSERT.

Thanks,
SWig


[-- Attachment #2: Type: text/html, Size: 40702 bytes --]

Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

[scott.wiginton]

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

Michael,

Wouldn't the current implementation ASSERT if there are different plug-in cards by the same vendor using the same GUID in their FMP?  They are managing different hardware and have no knowledge of the existence of the other FMP instance.  Each FMP is only aware of the specific device on whose handle they are installed.  If they use FW image descriptors below version 2 (or they cannot determine a unique HW instance), this would always be 0.  Why would we want to ASSERT in this case?  What error condition is being caught?

Thanks,
SWig

[-- Attachment #2: Type: text/html, Size: 600 bytes --]

Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

[Michael D Kinney]

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

Hi Scott,

I would agree that the current logic that checks for uniqueness applies to V3+ of an EFI_FIRMWARE_IMAGE_DESCRITOR.

Below V3, the HardwareInstance field does not exist, so I agree that the uniqueness check and ASSERT can not be done.  The language around V3 that introduced fields like HardwareInstance sites examples like PCI add-in cards as to why the field was added.  So I would not expect to see an FMP instance from a PCI add-in card that is below V3.

If we remove the uniqueness check and ASSERT at versions below V2, then we need to also review the capsule processing to determine how capsules are processed.  If the capsule has a non zero HardwareInstance and the FMP with the matching GUID is below V3, is the capsule ignored?  Perhaps only capsules with a zero HardwareInstance value can be dispatched to an FMP with version below V3?

Thanks,

Mike

From: scott.wiginton@hpe.com <scott.wiginton@hpe.com>
Sent: Monday, October 21, 2019 6:12 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

Michael,

Wouldn't the current implementation ASSERT if there are different plug-in cards by the same vendor using the same GUID in their FMP?  They are managing different hardware and have no knowledge of the existence of the other FMP instance.  Each FMP is only aware of the specific device on whose handle they are installed.  If they use FW image descriptors below version 2 (or they cannot determine a unique HW instance), this would always be 0.  Why would we want to ASSERT in this case?  What error condition is being caught?

Thanks,
SWig

[-- Attachment #2: Type: text/html, Size: 41996 bytes --]