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

2016-08-05 Thread 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

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

2016-08-05 Thread 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-boun...@lists.01.org] On Behalf Of
> Larry Cleeton
> Sent: Friday, August 05, 2016 9:35 AM
> To: Ye, Ting <ting...@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...@intel.com]
> Sent: Thursday, August 4, 2016 8:25 PM
> To: Larry Cleeton <larry.clee...@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-boun...@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 {
>   UINT16Offset;
>   UINTN DataSize;
>   EFI_IP6_CONFIG_DATA_TYPE  DataType;
> } IP6_CONFIG_DATA_RECORD;
> 
> Is now:
> 
> typedef struct {
>   UINT16Offset;
>   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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2016-08-05 Thread 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...@intel.com] 
Sent: Thursday, August 4, 2016 8:25 PM
To: Larry Cleeton ; 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-boun...@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 {
  UINT16Offset;
  UINTN DataSize;
  EFI_IP6_CONFIG_DATA_TYPE  DataType;
} IP6_CONFIG_DATA_RECORD;

Is now:

typedef struct {
  UINT16Offset;
  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