* [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support multiple devices with 0 HardwareInstance @ 2022-12-12 19:27 Jeff Brasen 2023-02-01 16:21 ` Jeff Brasen 0 siblings, 1 reply; 11+ messages in thread From: Jeff Brasen @ 2022-12-12 19:27 UTC (permalink / raw) To: devel; +Cc: jian.j.wang, gaoliming, guomin.jiang, Jeff Brasen 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support multiple devices with 0 HardwareInstance 2022-12-12 19:27 [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support multiple devices with 0 HardwareInstance Jeff Brasen @ 2023-02-01 16:21 ` Jeff Brasen 2023-02-09 22:02 ` FW: " Jeff Brasen 0 siblings, 1 reply; 11+ messages in thread From: Jeff Brasen @ 2023-02-01 16:21 UTC (permalink / raw) To: devel@edk2.groups.io Cc: jian.j.wang@intel.com, gaoliming@byosoft.com.cn, guomin.jiang@intel.com 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* FW: [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support multiple devices with 0 HardwareInstance 2023-02-01 16:21 ` Jeff Brasen @ 2023-02-09 22:02 ` Jeff Brasen 2023-02-10 3:42 ` Michael D Kinney 0 siblings, 1 reply; 11+ messages in thread From: Jeff Brasen @ 2023-02-09 22:02 UTC (permalink / raw) To: miki.demeter@intel.com; +Cc: devel@edk2.groups.io 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support multiple devices with 0 HardwareInstance 2023-02-09 22:02 ` FW: " Jeff Brasen @ 2023-02-10 3:42 ` Michael D Kinney 2023-02-10 16:44 ` Jeff Brasen 0 siblings, 1 reply; 11+ messages in thread From: Michael D Kinney @ 2023-02-10 3:42 UTC (permalink / raw) To: devel@edk2.groups.io, jbrasen@nvidia.com, Demeter, Miki; +Cc: Kinney, Michael D 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 > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support multiple devices with 0 HardwareInstance 2023-02-10 3:42 ` Michael D Kinney @ 2023-02-10 16:44 ` Jeff Brasen 2023-02-10 19:47 ` [edk2-devel] " Michael D Kinney 0 siblings, 1 reply; 11+ messages in thread From: Jeff Brasen @ 2023-02-10 16:44 UTC (permalink / raw) To: Kinney, Michael D, devel@edk2.groups.io, Demeter, Miki 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 > > > > > > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support multiple devices with 0 HardwareInstance 2023-02-10 16:44 ` Jeff Brasen @ 2023-02-10 19:47 ` Michael D Kinney 2023-02-10 20:43 ` Jeff Brasen 0 siblings, 1 reply; 11+ messages in thread From: Michael D Kinney @ 2023-02-10 19:47 UTC (permalink / raw) To: devel@edk2.groups.io, jbrasen@nvidia.com, Demeter, Miki; +Cc: Kinney, Michael D 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.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 > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support multiple devices with 0 HardwareInstance 2023-02-10 19:47 ` [edk2-devel] " Michael D Kinney @ 2023-02-10 20:43 ` Jeff Brasen 2023-02-10 21:17 ` Michael D Kinney 0 siblings, 1 reply; 11+ messages in thread From: Jeff Brasen @ 2023-02-10 20:43 UTC (permalink / raw) To: Kinney, Michael D, devel@edk2.groups.io, Demeter, Miki 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.htm > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support multiple devices with 0 HardwareInstance 2023-02-10 20:43 ` Jeff Brasen @ 2023-02-10 21:17 ` Michael D Kinney 2023-02-10 21:20 ` Jeff Brasen 0 siblings, 1 reply; 11+ messages in thread From: Michael D Kinney @ 2023-02-10 21:17 UTC (permalink / raw) To: devel@edk2.groups.io, jbrasen@nvidia.com, Demeter, Miki; +Cc: Kinney, Michael D 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.htm > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support multiple devices with 0 HardwareInstance 2023-02-10 21:17 ` Michael D Kinney @ 2023-02-10 21:20 ` Jeff Brasen 2023-02-14 23:50 ` Michael D Kinney 0 siblings, 1 reply; 11+ messages in thread From: Jeff Brasen @ 2023-02-10 21:20 UTC (permalink / raw) To: Kinney, Michael D, devel@edk2.groups.io, Demeter, Miki 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support multiple devices with 0 HardwareInstance 2023-02-10 21:20 ` Jeff Brasen @ 2023-02-14 23:50 ` Michael D Kinney 2023-02-15 1:59 ` Michael D Kinney 0 siblings, 1 reply; 11+ messages in thread From: Michael D Kinney @ 2023-02-14 23:50 UTC (permalink / raw) To: Jeff Brasen, devel@edk2.groups.io, Demeter, Miki; +Cc: Kinney, Michael D 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support multiple devices with 0 HardwareInstance 2023-02-14 23:50 ` Michael D Kinney @ 2023-02-15 1:59 ` Michael D Kinney 0 siblings, 0 replies; 11+ messages in thread From: Michael D Kinney @ 2023-02-15 1:59 UTC (permalink / raw) To: Jeff Brasen, devel@edk2.groups.io, Demeter, Miki; +Cc: Kinney, Michael D Merged PR: https://github.com/tianocore/edk2/pull/4043 Commit: https://github.com/tianocore/edk2/commit/090642db7ac124c336da72e1954e1fb09e816fb0 Mike > -----Original Message----- > From: Kinney, Michael D <michael.d.kinney@intel.com> > Sent: Tuesday, February 14, 2023 3:50 PM > To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io; 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 > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-02-15 1:59 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-12-12 19:27 [PATCH v2] MdeModulePkg/EsrtFmpDxe: Support multiple devices with 0 HardwareInstance Jeff Brasen 2023-02-01 16:21 ` Jeff Brasen 2023-02-09 22:02 ` FW: " Jeff Brasen 2023-02-10 3:42 ` Michael D Kinney 2023-02-10 16:44 ` Jeff Brasen 2023-02-10 19:47 ` [edk2-devel] " Michael D Kinney 2023-02-10 20:43 ` Jeff Brasen 2023-02-10 21:17 ` Michael D Kinney 2023-02-10 21:20 ` Jeff Brasen 2023-02-14 23:50 ` Michael D Kinney 2023-02-15 1:59 ` Michael D Kinney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox