Breaking change issue with NetworkPkg/Ip6Dxe/Ip6ConfigImlp.[c, h]

Breaking change issue with NetworkPkg/Ip6Dxe/Ip6ConfigImlp.[c, h]

[Larry Cleeton]

This commit (fdc4b0b147b386e966e99893526181dfae9eaeef) changed a data structure that is stored in an NVRAM variable. 
See NetworkPkg/Ip6Dxe/Ip6ConfigImpl.[c,h]

This data structure:

typedef struct {
  UINT16                    Offset;
  UINTN                     DataSize;
  EFI_IP6_CONFIG_DATA_TYPE  DataType;
} IP6_CONFIG_DATA_RECORD;

Is now:

typedef struct {
  UINT16                    Offset;
  UINT32                     DataSize;    <---------------- changed size in 64bit environments
  EFI_IP6_CONFIG_DATA_TYPE  DataType;
} IP6_CONFIG_DATA_RECORD;

Unfortunately with a 64bit implementation this current structure is now *not* compatible with an existing NVRAM variable written with the previous version of the structure. It's causing me considerable grief so I'm just sharing the discovery.  It would only impact you if you update some 64bit machine's firmware with a new version containing this change.

--Larry

Re: Breaking change issue with NetworkPkg/Ip6Dxe/Ip6ConfigImlp.[c, h]

[Ye, Ting]

Hi Larry,

We are very sorry about the impact you suffered today. We made the change in early 2013 to support the existing NVRAM variable when firmware image was updated from IA32 to X64. Unfortunately we introduced an incompatibility issue as you raised. Now we prefer to keep the existing definition, since if we change it back that would introduce another similar incompatibility issue. What do you think about this?

Best Regards,
Ye Ting

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Larry Cleeton
Sent: Wednesday, August 03, 2016 4:55 AM
To: edk2-devel@lists.01.org
Subject: [edk2] Breaking change issue with NetworkPkg/Ip6Dxe/Ip6ConfigImlp.[c, h]

This commit (fdc4b0b147b386e966e99893526181dfae9eaeef) changed a data structure that is stored in an NVRAM variable. 
See NetworkPkg/Ip6Dxe/Ip6ConfigImpl.[c,h]

This data structure:

typedef struct {
  UINT16                    Offset;
  UINTN                     DataSize;
  EFI_IP6_CONFIG_DATA_TYPE  DataType;
} IP6_CONFIG_DATA_RECORD;

Is now:

typedef struct {
  UINT16                    Offset;
  UINT32                     DataSize;    <---------------- changed size in 64bit environments
  EFI_IP6_CONFIG_DATA_TYPE  DataType;
} IP6_CONFIG_DATA_RECORD;

Unfortunately with a 64bit implementation this current structure is now *not* compatible with an existing NVRAM variable written with the previous version of the structure. It's causing me considerable grief so I'm just sharing the discovery.  It would only impact you if you update some 64bit machine's firmware with a new version containing this change.

--Larry
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: Breaking change issue with NetworkPkg/Ip6Dxe/Ip6ConfigImlp.[c, h]

[Larry Cleeton]

I agree with your assessment about leaving the data structure as it is.   I just wanted to highlight it as it may impact others.  

The bottom line is my development group is entirely responsible for vetting any changes coming from the EDK2 into our product.  This one slipped by us.

--Larry

-----Original Message-----
From: Ye, Ting [mailto:ting.ye@intel.com] 
Sent: Thursday, August 4, 2016 8:25 PM
To: Larry Cleeton <Larry.Cleeton@microsoft.com>; edk2-devel@lists.01.org
Subject: RE: Breaking change issue with NetworkPkg/Ip6Dxe/Ip6ConfigImlp.[c, h]

Hi Larry,

We are very sorry about the impact you suffered today. We made the change in early 2013 to support the existing NVRAM variable when firmware image was updated from IA32 to X64. Unfortunately we introduced an incompatibility issue as you raised. Now we prefer to keep the existing definition, since if we change it back that would introduce another similar incompatibility issue. What do you think about this?

Best Regards,
Ye Ting

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Larry Cleeton
Sent: Wednesday, August 03, 2016 4:55 AM
To: edk2-devel@lists.01.org
Subject: [edk2] Breaking change issue with NetworkPkg/Ip6Dxe/Ip6ConfigImlp.[c, h]

This commit (fdc4b0b147b386e966e99893526181dfae9eaeef) changed a data structure that is stored in an NVRAM variable. 
See NetworkPkg/Ip6Dxe/Ip6ConfigImpl.[c,h]

This data structure:

typedef struct {
  UINT16                    Offset;
  UINTN                     DataSize;
  EFI_IP6_CONFIG_DATA_TYPE  DataType;
} IP6_CONFIG_DATA_RECORD;

Is now:

typedef struct {
  UINT16                    Offset;
  UINT32                     DataSize;    <---------------- changed size in 64bit environments
  EFI_IP6_CONFIG_DATA_TYPE  DataType;
} IP6_CONFIG_DATA_RECORD;

Unfortunately with a 64bit implementation this current structure is now *not* compatible with an existing NVRAM variable written with the previous version of the structure. It's causing me considerable grief so I'm just sharing the discovery.  It would only impact you if you update some 64bit machine's firmware with a new version containing this change.

--Larry
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: Breaking change issue with NetworkPkg/Ip6Dxe/Ip6ConfigImlp.[c, h]

[Cohen, Eugene]

We've been hit by this same kind of issue and it's really painful, especially as it affects shipping systems.

Long term I think we need an extensible/revisioned data format so we can get forwards and backwards compatibility between NVRAM data and FW.

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Larry Cleeton
> Sent: Friday, August 05, 2016 9:35 AM
> To: Ye, Ting <ting.ye@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] Breaking change issue with
> NetworkPkg/Ip6Dxe/Ip6ConfigImlp.[c, h]
> 
> I agree with your assessment about leaving the data structure as it is.   I just
> wanted to highlight it as it may impact others.
> 
> The bottom line is my development group is entirely responsible for vetting
> any changes coming from the EDK2 into our product.  This one slipped by us.
> 
> --Larry
> 
> -----Original Message-----
> From: Ye, Ting [mailto:ting.ye@intel.com]
> Sent: Thursday, August 4, 2016 8:25 PM
> To: Larry Cleeton <Larry.Cleeton@microsoft.com>; edk2-devel@lists.01.org
> Subject: RE: Breaking change issue with
> NetworkPkg/Ip6Dxe/Ip6ConfigImlp.[c, h]
> 
> Hi Larry,
> 
> We are very sorry about the impact you suffered today. We made the
> change in early 2013 to support the existing NVRAM variable when firmware
> image was updated from IA32 to X64. Unfortunately we introduced an
> incompatibility issue as you raised. Now we prefer to keep the existing
> definition, since if we change it back that would introduce another similar
> incompatibility issue. What do you think about this?
> 
> Best Regards,
> Ye Ting
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Larry Cleeton
> Sent: Wednesday, August 03, 2016 4:55 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] Breaking change issue with
> NetworkPkg/Ip6Dxe/Ip6ConfigImlp.[c, h]
> 
> This commit (fdc4b0b147b386e966e99893526181dfae9eaeef) changed a data
> structure that is stored in an NVRAM variable.
> See NetworkPkg/Ip6Dxe/Ip6ConfigImpl.[c,h]
> 
> This data structure:
> 
> typedef struct {
>   UINT16                    Offset;
>   UINTN                     DataSize;
>   EFI_IP6_CONFIG_DATA_TYPE  DataType;
> } IP6_CONFIG_DATA_RECORD;
> 
> Is now:
> 
> typedef struct {
>   UINT16                    Offset;
>   UINT32                     DataSize;    <---------------- changed size in 64bit
> environments
>   EFI_IP6_CONFIG_DATA_TYPE  DataType;
> } IP6_CONFIG_DATA_RECORD;
> 
> Unfortunately with a 64bit implementation this current structure is now
> *not* compatible with an existing NVRAM variable written with the previous
> version of the structure. It's causing me considerable grief so I'm just sharing
> the discovery.  It would only impact you if you update some 64bit machine's
> firmware with a new version containing this change.
> 
> --Larry
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: Breaking change issue with NetworkPkg/Ip6Dxe/Ip6ConfigImlp.[c, h]

[Laszlo Ersek]

On 08/05/16 18:12, Cohen, Eugene wrote:
> We've been hit by this same kind of issue and it's really painful, especially as it affects shipping systems.
> 
> Long term I think we need an extensible/revisioned data format so we can get forwards and backwards compatibility between NVRAM data and FW.

In OVMF we have a (very rudimentary) implementation for this, see
PlatformConfigLoad() in "OvmfPkg/PlatformDxe/PlatformConfig.c".

The idea is shamelessly stolen from UEFI protocols:
- we store the config structure in some NV variable under some
  namespace GUID,
- new fields can only be added after existing fields to the structure,
- if a new field would change the meaning of a preexistent field, or
  some preexistent fields would have to be dropped / replaced, then the
  change is deemed incompatible, and a new variable name or namespace
  GUID is required.

The number of bytes the config reader function reads from the variable
is the minimum of:
(a) the size of the variable,
(b) the size of largest structure version known to the config reader.

If a==b, then it's a version match. If a>b, then it's a firmware
downgrade (unknown fields are ignored). If a<b, then it's a firmware
upgrade, fields missing from the NV data are initialized to default
values (or some such).

The implementation in OVMF is of course just a "stem" for future
extensions. We've never needed such an extension yet :), but the idea is
the one above, FWIW.

Thanks
Laszlo