Re: [edk2] [PATCH v3 44/52] OvmfPkg: QemuFlashFvbServicesRuntimeDxe: no dual addressing needed

[Andrew Fish]

> On Oct 18, 2015, at 12:29 AM, Jordan Justen  wrote:
> 
> I can't find a fault with your argument, but something tells me it
> might be good to get some input from Mike or Andrew (or others on the
> list) in the form of a history lesson to know why the two modes might
> have been supported.
> 

This pattern traces back to Itanium, and is only required for that environment. 

The SAL (Itanium specific platform firmware API) has different calling 
convention rules (supports virtual and physical at runtime). Also on Itanium a 
pointer to a function is a pointer to a data structure (called PLABLE) that 
contains a register value, and the function pointer. 

Thanks,

Andrew Fish

> I've always seen it done this way, but likely this has been due to a
> lot of "copy the old version and modify."
> 
> -Jordan
> 
> On 2015-10-14 15:26:40, Laszlo Ersek wrote:
>> Currently the EFI_FW_VOL_INSTANCE and ESAL_FWB_GLOBAL structures declare
>> the following entries as arrays, with two entries each:
>> 
>> - EFI_FW_VOL_INSTANCE.FvBase[2]
>> - ESAL_FWB_GLOBAL.FvInstance[2]
>> 
>> In every case, the entry at subscript zero is meant as "physical address",
>> while the entry at subscript one is meant as "virtual address" -- a
>> pointer to the same object. The virtual address entry is originally
>> initialized to the physical address, and then it is converted to the
>> virtual mapping in FvbVirtualddressChangeEvent().
>> 
>> Functions that (a) read the listed fields and (b) run both before and
>> after the virtual address change event -- since this is a runtime DXE
>> driver -- derive the correct array subscript by calling the
>> EfiGoneVirtual() function from UefiRuntimeLib.
>> 
>> The problem with the above infrastructure is that it's entirely
>> superfluous.
>> 
>> EfiGoneVirtual() "knows" whether EFI has gone virtual only because the
>> UefiRuntimeLib constructor registers the exact same kind of virtual
>> address change callback, and the callback flips a static variabe to TRUE,
>> and EfiGoneVirtual() queries that static variable.
>> 
>> In effect this means for QemuFlashFvbServicesRuntimeDxe: "when there is a
>> virtual address change, convert the entries with subscript one from
>> physical to virtual, and from then on use the entries with subscript one".
>> 
>> This would only make sense if QemuFlashFvbServicesRuntimeDxe ever needed
>> the original (physical) addresses (ie. the entries with subscript zero)
>> after the virtual address change, but that is not the case.
>> 
>> Replace the arrays with single elements. The subscript zero elements
>> simply disappear, and the single elements take the role of the prior
>> subscript one elements.
>> 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek 
>> ---
>> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h | 22 ++
>> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c | 77 
>> ++--
>> 2 files changed, 30 insertions(+), 69 deletions(-)
>> 
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h 
>> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
>> index 7e4ff1e..5e8c2c7 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
>> @@ -23,21 +23,15 @@
>> #ifndef _FW_BLOCK_SERVICE_H
>> #define _FW_BLOCK_SERVICE_H
>> 
>> -//
>> -// BugBug: Add documentation here for data structure
>> -//
>> -#define FVB_PHYSICAL  0
>> -#define FVB_VIRTUAL   1
>> -
>> typedef struct {
>> -  UINTN   FvBase[2];
>> +  UINTN   FvBase;
>>   UINTN   NumOfBlocks;
>>   EFI_FIRMWARE_VOLUME_HEADER  VolumeHeader;
>> } EFI_FW_VOL_INSTANCE;
>> 
>> typedef struct {
>>   UINT32  NumFv;
>> -  EFI_FW_VOL_INSTANCE *FvInstance[2];
>> +  EFI_FW_VOL_INSTANCE *FvInstance;
>> } ESAL_FWB_GLOBAL;
>> 
>> //
>> @@ -78,24 +72,21 @@ EFI_STATUS
>> FvbSetVolumeAttributes (
>>   IN UINTNInstance,
>>   IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes,
>> -  IN ESAL_FWB_GLOBAL  *Global,
>> -  IN BOOLEAN  Virtual
>> +  IN ESAL_FWB_GLOBAL  *Global
>>   );
>> 
>> EFI_STATUS
>> FvbGetVolumeAttributes (
>>   IN UINTNInstance,
>>   OUT EFI_FVB_ATTRIBUTES_2*Attributes,
>> -  IN ESAL_FWB_GLOBAL  *Global,
>> -  IN BOOLEAN  Virtual
>> +  IN ESAL_FWB_GLOBAL  *Global
>>   );
>> 
>> EFI_STATUS
>> FvbGetPhysicalAddress (
>>   IN UINTNInstance,
>>   OUT EFI_PHYSICAL_ADDRESS*Address,
>> -  IN ESAL_FWB_GLOBAL  *Global,
>> -  IN BOOLEAN  Virtual
>> +  IN ESAL_FWB_GLOBAL  *Global
>>   );
>> 
>> EFI_STATUS
>> @@ -120,8 +111,7 

Re: [edk2] [PATCH v3 44/52] OvmfPkg: QemuFlashFvbServicesRuntimeDxe: no dual addressing needed

[Jordan Justen]

I can't find a fault with your argument, but something tells me it
might be good to get some input from Mike or Andrew (or others on the
list) in the form of a history lesson to know why the two modes might
have been supported.

I've always seen it done this way, but likely this has been due to a
lot of "copy the old version and modify."

-Jordan

On 2015-10-14 15:26:40, Laszlo Ersek wrote:
> Currently the EFI_FW_VOL_INSTANCE and ESAL_FWB_GLOBAL structures declare
> the following entries as arrays, with two entries each:
> 
> - EFI_FW_VOL_INSTANCE.FvBase[2]
> - ESAL_FWB_GLOBAL.FvInstance[2]
> 
> In every case, the entry at subscript zero is meant as "physical address",
> while the entry at subscript one is meant as "virtual address" -- a
> pointer to the same object. The virtual address entry is originally
> initialized to the physical address, and then it is converted to the
> virtual mapping in FvbVirtualddressChangeEvent().
> 
> Functions that (a) read the listed fields and (b) run both before and
> after the virtual address change event -- since this is a runtime DXE
> driver -- derive the correct array subscript by calling the
> EfiGoneVirtual() function from UefiRuntimeLib.
> 
> The problem with the above infrastructure is that it's entirely
> superfluous.
> 
> EfiGoneVirtual() "knows" whether EFI has gone virtual only because the
> UefiRuntimeLib constructor registers the exact same kind of virtual
> address change callback, and the callback flips a static variabe to TRUE,
> and EfiGoneVirtual() queries that static variable.
> 
> In effect this means for QemuFlashFvbServicesRuntimeDxe: "when there is a
> virtual address change, convert the entries with subscript one from
> physical to virtual, and from then on use the entries with subscript one".
> 
> This would only make sense if QemuFlashFvbServicesRuntimeDxe ever needed
> the original (physical) addresses (ie. the entries with subscript zero)
> after the virtual address change, but that is not the case.
> 
> Replace the arrays with single elements. The subscript zero elements
> simply disappear, and the single elements take the role of the prior
> subscript one elements.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> ---
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h | 22 ++
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c | 77 
> ++--
>  2 files changed, 30 insertions(+), 69 deletions(-)
> 
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h 
> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
> index 7e4ff1e..5e8c2c7 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
> @@ -23,21 +23,15 @@
>  #ifndef _FW_BLOCK_SERVICE_H
>  #define _FW_BLOCK_SERVICE_H
>  
> -//
> -// BugBug: Add documentation here for data structure
> -//
> -#define FVB_PHYSICAL  0
> -#define FVB_VIRTUAL   1
> -
>  typedef struct {
> -  UINTN   FvBase[2];
> +  UINTN   FvBase;
>UINTN   NumOfBlocks;
>EFI_FIRMWARE_VOLUME_HEADER  VolumeHeader;
>  } EFI_FW_VOL_INSTANCE;
>  
>  typedef struct {
>UINT32  NumFv;
> -  EFI_FW_VOL_INSTANCE *FvInstance[2];
> +  EFI_FW_VOL_INSTANCE *FvInstance;
>  } ESAL_FWB_GLOBAL;
>  
>  //
> @@ -78,24 +72,21 @@ EFI_STATUS
>  FvbSetVolumeAttributes (
>IN UINTNInstance,
>IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes,
> -  IN ESAL_FWB_GLOBAL  *Global,
> -  IN BOOLEAN  Virtual
> +  IN ESAL_FWB_GLOBAL  *Global
>);
>  
>  EFI_STATUS
>  FvbGetVolumeAttributes (
>IN UINTNInstance,
>OUT EFI_FVB_ATTRIBUTES_2*Attributes,
> -  IN ESAL_FWB_GLOBAL  *Global,
> -  IN BOOLEAN  Virtual
> +  IN ESAL_FWB_GLOBAL  *Global
>);
>  
>  EFI_STATUS
>  FvbGetPhysicalAddress (
>IN UINTNInstance,
>OUT EFI_PHYSICAL_ADDRESS*Address,
> -  IN ESAL_FWB_GLOBAL  *Global,
> -  IN BOOLEAN  Virtual
> +  IN ESAL_FWB_GLOBAL  *Global
>);
>  
>  EFI_STATUS
> @@ -120,8 +111,7 @@ FvbGetLbaAddress (
>OUT UINTN   *LbaAddress,
>OUT UINTN   *LbaLength,
>OUT UINTN   *NumOfBlocks,
> -  IN  ESAL_FWB_GLOBAL *Global,
> -  IN  BOOLEAN Virtual
> +  IN  ESAL_FWB_GLOBAL *Global
>);
>  
>  //
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c 
> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> index 95ae8cc..3eccb1f 100644
> ---