On 02/26/14 22:24, Jordan Justen wrote:
> On Fri, Feb 21, 2014 at 3:58 PM, Laszlo Ersek <ler...@redhat.com> wrote:
>> This small library allows the saving and loading of platform configuration
>> to/from the non-volatile variable store.
>>
>> It provides a backend for EFI_HII_CONFIG_ACCESS_PROTOCOL (ie. setup forms)
>> and for various DXE and UEFI drivers that depend on the platform
>> configuration.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>> ---
>>  .../PlatformConfigLib/PlatformConfigLib.inf        |  36 +++++++
>>  OvmfPkg/Include/Library/PlatformConfigLib.h        |  54 +++++++++++
>>  .../Library/PlatformConfigLib/PlatformConfigLib.c  | 106 
>> +++++++++++++++++++++
>>  OvmfPkg/OvmfPkgIa32.dsc                            |   2 +
>>  OvmfPkg/OvmfPkgIa32X64.dsc                         |   2 +
>>  OvmfPkg/OvmfPkgX64.dsc                             |   2 +
>>  6 files changed, 202 insertions(+)
>>  create mode 100644 OvmfPkg/Library/PlatformConfigLib/PlatformConfigLib.inf
>>  create mode 100644 OvmfPkg/Include/Library/PlatformConfigLib.h
>>  create mode 100644 OvmfPkg/Library/PlatformConfigLib/PlatformConfigLib.c
>>
>> diff --git a/OvmfPkg/Library/PlatformConfigLib/PlatformConfigLib.inf 
>> b/OvmfPkg/Library/PlatformConfigLib/PlatformConfigLib.inf
>> new file mode 100644
>> index 0000000..cffb60c
>> --- /dev/null
>> +++ b/OvmfPkg/Library/PlatformConfigLib/PlatformConfigLib.inf
>> @@ -0,0 +1,36 @@
>> +## @file
>> +# Library for serializing (persistently storing) and deserializing OVMF's
>> +# platform configuration.
>> +#
>> +# Copyright (C) 2014, Red Hat, Inc.
>> +#
>> +# This program and the accompanying materials are licensed and made 
>> available
>> +# under the terms and conditions of the BSD License which accompanies this
>> +# distribution. The full text of the license may be found at
>> +# http://opensource.org/licenses/bsd-license.php
>> +#
>> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, 
>> WITHOUT
>> +# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +#
>> +##
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x00010005
>> +  BASE_NAME                      = PlatformConfigLib
>> +  FILE_GUID                      = 30F126DF-CD18-4DDC-BE0A-1159540BCADF
>> +  MODULE_TYPE                    = DXE_DRIVER
>> +  VERSION_STRING                 = 1.0
>> +  LIBRARY_CLASS                  = PlatformConfigLib|DXE_DRIVER UEFI_DRIVER
>> +
>> +[Sources]
>> +  PlatformConfigLib.c
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +  OvmfPkg/OvmfPkg.dec
>> +
>> +[LibraryClasses]
>> +  UefiRuntimeServicesTableLib
>> +
>> +[Guids]
>> +  gOvmfPlatformConfigGuid
>> diff --git a/OvmfPkg/Include/Library/PlatformConfigLib.h 
>> b/OvmfPkg/Include/Library/PlatformConfigLib.h
>> new file mode 100644
>> index 0000000..d2c11a5
>> --- /dev/null
>> +++ b/OvmfPkg/Include/Library/PlatformConfigLib.h
>> @@ -0,0 +1,54 @@
>> +/** @file
>> +
>> +  Library for serializing (persistently storing) and deserializing OVMF's
>> +  platform configuration.
>> +
>> +  Copyright (C) 2014, Red Hat, Inc.
>> +
>> +  This program and the accompanying materials are licensed and made 
>> available
>> +  under the terms and conditions of the BSD License which accompanies this
>> +  distribution. The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, 
>> WITHOUT
>> +  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#ifndef _PLATFORM_CONFIG_LIB_H_
>> +#define _PLATFORM_CONFIG_LIB_H_
>> +
>> +#include <Base.h>
>> +
>> +//
>> +// This structure participates in driver configuration and in communication
>> +// with HII. It does not (necessarily) reflect the wire format in the
>> +// persistent store.
>> +//
>> +#pragma pack(1)
>> +typedef struct {
>> +  //
>> +  // preferred graphics console resolution when booting
>> +  //
>> +  UINT32 HorizontalResolution;
>> +  UINT32 VerticalResolution;
>> +} PLATFORM_CONFIG;
>> +#pragma pack()
>> +
>> +//
>> +// Please see the API documentation near the function definitions.
>> +//
>> +EFI_STATUS
>> +EFIAPI
>> +PlatformConfigSave (
>> +  IN PLATFORM_CONFIG *PlatformConfig
>> +  );
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +PlatformConfigLoad (
>> +  OUT PLATFORM_CONFIG *PlatformConfig,
>> +  OUT UINT64          *OptionalElements
>> +  );
>> +
>> +#endif // _PLATFORM_CONFIG_LIB_H_
>> diff --git a/OvmfPkg/Library/PlatformConfigLib/PlatformConfigLib.c 
>> b/OvmfPkg/Library/PlatformConfigLib/PlatformConfigLib.c
>> new file mode 100644
>> index 0000000..73c4bfc
>> --- /dev/null
>> +++ b/OvmfPkg/Library/PlatformConfigLib/PlatformConfigLib.c
>> @@ -0,0 +1,106 @@
>> +/** @file
>> +
>> +  Library for serializing (persistently storing) and deserializing OVMF's
>> +  platform configuration.
>> +
>> +  Copyright (C) 2014, Red Hat, Inc.
>> +
>> +  This program and the accompanying materials are licensed and made 
>> available
>> +  under the terms and conditions of the BSD License which accompanies this
>> +  distribution. The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, 
>> WITHOUT
>> +  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#include <Library/PlatformConfigLib.h>
>> +#include <Library/UefiRuntimeServicesTableLib.h>
>> +#include <Guid/OvmfPlatformConfig.h>
>> +
>> +
>> +//
>> +// Name of the UEFI variable that we use for persistent storage.
>> +//
>> +STATIC CHAR16 mVariableName[] = L"PlatformConfig";
>> +
>> +
>> +/**
>> +  Serialize and persistently save platform configuration.
>> +
>> +  @param[in] PlatformConfig  The platform configuration to serialize and 
>> save.
>> +
>> +  @return  Status codes returned by gRT->SetVariable().
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +PlatformConfigSave (
>> +  IN PLATFORM_CONFIG *PlatformConfig
>> +  )
>> +{
>> +  EFI_STATUS Status;
>> +
>> +  //
>> +  // We could implement any kind of translation here, as part of 
>> serialization.
>> +  // For example, we could expose the platform configuration in separate
>> +  // variables with human-readable contents, allowing other tools to access
>> +  // them more easily. For now, just save a binary dump.
>> +  //
>> +  Status = gRT->SetVariable (mVariableName, &gOvmfPlatformConfigGuid,
>> +                  EFI_VARIABLE_NON_VOLATILE | 
>> EFI_VARIABLE_BOOTSERVICE_ACCESS |
>> +                    EFI_VARIABLE_RUNTIME_ACCESS,
>> +                  sizeof *PlatformConfig, PlatformConfig);
>> +  return Status;
>> +}
>> +
>> +
>> +/**
>> +  Load and deserialize platform configuration.
>> +
>> +  When the function fails, output parameters are indeterminate.
>> +
>> +  @param[out] PlatformConfig    The platform configuration to receive the
>> +                                loaded data.
>> +
>> +  @param[out] OptionalElements  This bitmap describes the presence of 
>> optional
>> +                                configuration elements.
>> +
>> +  @retval  EFI_SUCCESS         Loading & deserialization successful.
>> +  @retval  EFI_PROTOCOL_ERROR  Invalid contents in persistent store.
>> +  @return                      Error codes returned by gRT->GetVariable().
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +PlatformConfigLoad (
>> +  OUT PLATFORM_CONFIG *PlatformConfig,
>> +  OUT UINT64          *OptionalElements
>> +  )
>> +{
>> +  UINTN      DataSize;
>> +  EFI_STATUS Status;
>> +
>> +  //
>> +  // Any translation done in PlatformConfigSave() would have to be mirrored
>> +  // here. For now, just load the binary dump.
>> +  //
>> +  // Versioning of the binary wire format can be implemented later on, 
>> based on
>> +  // size (only incremental changes, ie. new fields), and on GUID.
>> +  // (Incompatible changes require a GUID change.)
>> +  //
>> +  DataSize = sizeof *PlatformConfig;
>> +  Status = gRT->GetVariable (mVariableName, &gOvmfPlatformConfigGuid,
>> +                  NULL /* Attributes */, &DataSize, PlatformConfig);
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  //
>> +  // No optional configuration elements are supported for the time being.
>> +  //
>> +  if (DataSize < sizeof *PlatformConfig) {
>> +    return EFI_PROTOCOL_ERROR;
>> +  }
>> +  *OptionalElements = 0;
>> +  return EFI_SUCCESS;
>> +}
> 
> It seems like these functions don't take into account firmware
> 'upgrades' which might cause the size of PLATFORM_CONFIG to grow
> compared to what is in the variable.

It was decidedly my intent to support that. I had the following in mind,
for when we extend the PLATFORM_CONFIG variable for the first time:
- Change this function to accept smaller buffers. (Note the comment
saying "for the time being" in the source.)
- Introduce new flags as bits in OptionalElements, describing the
presence of the new fields, and set them if the NvVar size warrants that
(ie. new fields are available).

> Unfortunately, dealing with that can be complicated. There are ways of
> getting default values from HII to help out with this. Or, one can
> pre-fill the structure with zeros and hope those are safe values.

The idea was that callers of PlatformConfigLoad() would check the
OptionalElements output parameter. When we introduce a new knob in
PLATFORM_CONFIG, most existent callers of PlatformConfigLoad() in the
tree wouldn't care. The few callers that do care about the new fields
would check the bits in OptionalElements.

PLATFORM_CONFIG is not an incomplete type. IOW its size is known to all
callers. Whenever we introduce a new field, the auto (=stack)
allocations in all callers would be automatically updated during the build.

I think this covers backward compat.

> Another potential concern: firmware downgrade, which can cause the
> variable size to be larger than expected.

This is something that I expressly didn't want to support. When you
downgrade your firmware, it's OK to lose platform configuration. In my
opinion, bidirectional compatibility is not something we need.

Backward compatibility is important (new code can parse data from old
code). Forward compatibility (old code can sensibly parse a subset of
data generated by new code) -- I think you don't even get that with
config files of UNIX daemons.

In any case, if you think that forward compat would be well served by
simply loading the leading part of the NvVar into PLATFORM_CONFIG (ie. a
special handling of EFI_BUFFER_TOO_SMALL), I can do that.

Thanks,
Laszlo

------------------------------------------------------------------------------
Flow-based real-time traffic analytics software. Cisco certified tool.
Monitor traffic, SLAs, QoS, Medianet, WAAS etc. with NetFlow Analyzer
Customize your own dashboards, set traffic alerts and generate reports.
Network behavioral analysis & security monitoring. All-in-one tool.
http://pubads.g.doubleclick.net/gampad/clk?id=126839071&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Reply via email to