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 <[email protected]> > --- > 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 UINTN Instance, > IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes, > - IN ESAL_FWB_GLOBAL *Global, > - IN BOOLEAN Virtual > + IN ESAL_FWB_GLOBAL *Global > ); > > EFI_STATUS > FvbGetVolumeAttributes ( > IN UINTN Instance, > OUT EFI_FVB_ATTRIBUTES_2 *Attributes, > - IN ESAL_FWB_GLOBAL *Global, > - IN BOOLEAN Virtual > + IN ESAL_FWB_GLOBAL *Global > ); > > EFI_STATUS > FvbGetPhysicalAddress ( > IN UINTN Instance, > 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 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c > @@ -132,11 +132,6 @@ FvbVirtualddressChangeEvent ( > Call the passed in Child Notify event and convert the mFvbModuleGlobal > date items to there virtual address. > > - mFvbModuleGlobal->FvInstance[FVB_PHYSICAL] - Physical copy of instance > - data > - mFvbModuleGlobal->FvInstance[FVB_VIRTUAL] - Virtual pointer to common > - instance data. > - > Arguments: > > (Standard EFI notify event - EFI_EVENT_NOTIFY) > @@ -150,16 +145,15 @@ FvbVirtualddressChangeEvent ( > EFI_FW_VOL_INSTANCE *FwhInstance; > UINTN Index; > > - EfiConvertPointer (0x0, > - (VOID **) &mFvbModuleGlobal->FvInstance[FVB_VIRTUAL]); > + FwhInstance = mFvbModuleGlobal->FvInstance; > + EfiConvertPointer (0x0, (VOID **) &mFvbModuleGlobal->FvInstance); > > // > // Convert the base address of all the instances > // > Index = 0; > - FwhInstance = mFvbModuleGlobal->FvInstance[FVB_PHYSICAL]; > while (Index < mFvbModuleGlobal->NumFv) { > - EfiConvertPointer (0x0, (VOID **) &FwhInstance->FvBase[FVB_VIRTUAL]); > + EfiConvertPointer (0x0, (VOID **) &FwhInstance->FvBase); > FwhInstance = (EFI_FW_VOL_INSTANCE *) > ( > (UINTN) ((UINT8 *) FwhInstance) + > @@ -177,8 +171,7 @@ EFI_STATUS > GetFvbInstance ( > IN UINTN Instance, > IN ESAL_FWB_GLOBAL *Global, > - OUT EFI_FW_VOL_INSTANCE **FwhInstance, > - IN BOOLEAN Virtual > + OUT EFI_FW_VOL_INSTANCE **FwhInstance > ) > /*++ > > @@ -191,7 +184,6 @@ GetFvbInstance ( > Global - Pointer to ESAL_FWB_GLOBAL that contains all > instance data > FwhInstance - The EFI_FW_VOL_INSTANCE fimrware instance > structure > - Virtual - Whether CPU is in virtual or physical mode > > Returns: > EFI_SUCCESS - Successfully returns > @@ -208,7 +200,7 @@ GetFvbInstance ( > // > // Find the right instance of the FVB private data > // > - FwhRecord = Global->FvInstance[Virtual]; > + FwhRecord = Global->FvInstance; > while (Instance > 0) { > FwhRecord = (EFI_FW_VOL_INSTANCE *) > ( > @@ -227,8 +219,7 @@ EFI_STATUS > FvbGetPhysicalAddress ( > IN UINTN Instance, > OUT EFI_PHYSICAL_ADDRESS *Address, > - IN ESAL_FWB_GLOBAL *Global, > - IN BOOLEAN Virtual > + IN ESAL_FWB_GLOBAL *Global > ) > /*++ > > @@ -243,7 +234,6 @@ FvbGetPhysicalAddress ( > address of the firmware volume. > Global - Pointer to ESAL_FWB_GLOBAL that contains all > instance data > - Virtual - Whether CPU is in virtual or physical mode > > Returns: > EFI_SUCCESS - Successfully returns > @@ -257,9 +247,9 @@ FvbGetPhysicalAddress ( > // > // Find the right instance of the FVB private data > // > - Status = GetFvbInstance (Instance, Global, &FwhInstance, Virtual); > + Status = GetFvbInstance (Instance, Global, &FwhInstance); > ASSERT_EFI_ERROR (Status); > - *Address = FwhInstance->FvBase[Virtual]; > + *Address = FwhInstance->FvBase; > > return EFI_SUCCESS; > } > @@ -268,8 +258,7 @@ EFI_STATUS > FvbGetVolumeAttributes ( > IN UINTN Instance, > OUT EFI_FVB_ATTRIBUTES_2 *Attributes, > - IN ESAL_FWB_GLOBAL *Global, > - IN BOOLEAN Virtual > + IN ESAL_FWB_GLOBAL *Global > ) > /*++ > > @@ -283,7 +272,6 @@ FvbGetVolumeAttributes ( > Attributes - Output buffer which contains attributes > Global - Pointer to ESAL_FWB_GLOBAL that contains all > instance data > - Virtual - Whether CPU is in virtual or physical mode > > Returns: > EFI_SUCCESS - Successfully returns > @@ -297,7 +285,7 @@ FvbGetVolumeAttributes ( > // > // Find the right instance of the FVB private data > // > - Status = GetFvbInstance (Instance, Global, &FwhInstance, Virtual); > + Status = GetFvbInstance (Instance, Global, &FwhInstance); > ASSERT_EFI_ERROR (Status); > *Attributes = FwhInstance->VolumeHeader.Attributes; > > @@ -311,8 +299,7 @@ FvbGetLbaAddress ( > OUT UINTN *LbaAddress, > OUT UINTN *LbaLength, > OUT UINTN *NumOfBlocks, > - IN ESAL_FWB_GLOBAL *Global, > - IN BOOLEAN Virtual > + IN ESAL_FWB_GLOBAL *Global > ) > /*++ > > @@ -331,7 +318,6 @@ FvbGetLbaAddress ( > BlockSize > Global - Pointer to ESAL_FWB_GLOBAL that contains all > instance data > - Virtual - Whether CPU is in virtual or physical mode > > Returns: > EFI_SUCCESS - Successfully returns > @@ -351,7 +337,7 @@ FvbGetLbaAddress ( > // > // Find the right instance of the FVB private data > // > - Status = GetFvbInstance (Instance, Global, &FwhInstance, Virtual); > + Status = GetFvbInstance (Instance, Global, &FwhInstance); > ASSERT_EFI_ERROR (Status); > > StartLba = 0; > @@ -377,7 +363,7 @@ FvbGetLbaAddress ( > if (Lba >= StartLba && Lba < NextLba) { > Offset = Offset + (UINTN) MultU64x32 ((Lba - StartLba), BlockLength); > if (LbaAddress != NULL) { > - *LbaAddress = FwhInstance->FvBase[Virtual] + Offset; > + *LbaAddress = FwhInstance->FvBase + Offset; > } > > if (LbaLength != NULL) { > @@ -401,8 +387,7 @@ EFI_STATUS > FvbSetVolumeAttributes ( > IN UINTN Instance, > IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes, > - IN ESAL_FWB_GLOBAL *Global, > - IN BOOLEAN Virtual > + IN ESAL_FWB_GLOBAL *Global > ) > /*++ > > @@ -419,7 +404,6 @@ FvbSetVolumeAttributes ( > of the firmware volume > Global - Pointer to ESAL_FWB_GLOBAL that contains all > instance data > - Virtual - Whether CPU is in virtual or physical mode > > Returns: > EFI_SUCCESS - Successfully returns > @@ -442,7 +426,7 @@ FvbSetVolumeAttributes ( > // > // Find the right instance of the FVB private data > // > - Status = GetFvbInstance (Instance, Global, &FwhInstance, Virtual); > + Status = GetFvbInstance (Instance, Global, &FwhInstance); > ASSERT_EFI_ERROR (Status); > > AttribPtr = > @@ -562,7 +546,7 @@ FvbProtocolGetPhysicalAddress ( > FvbDevice = FVB_DEVICE_FROM_THIS (This); > > return FvbGetPhysicalAddress (FvbDevice->Instance, Address, > - mFvbModuleGlobal, EfiGoneVirtual ()); > + mFvbModuleGlobal); > } > > EFI_STATUS > @@ -604,8 +588,7 @@ FvbProtocolGetBlockSize ( > NULL, > BlockSize, > NumOfBlocks, > - mFvbModuleGlobal, > - EfiGoneVirtual () > + mFvbModuleGlobal > ); > } > > @@ -634,7 +617,7 @@ FvbProtocolGetAttributes ( > FvbDevice = FVB_DEVICE_FROM_THIS (This); > > return FvbGetVolumeAttributes (FvbDevice->Instance, Attributes, > - mFvbModuleGlobal, EfiGoneVirtual ()); > + mFvbModuleGlobal); > } > > EFI_STATUS > @@ -662,7 +645,7 @@ FvbProtocolSetAttributes ( > FvbDevice = FVB_DEVICE_FROM_THIS (This); > > return FvbSetVolumeAttributes (FvbDevice->Instance, Attributes, > - mFvbModuleGlobal, EfiGoneVirtual ()); > + mFvbModuleGlobal); > } > > EFI_STATUS > @@ -708,7 +691,7 @@ FvbProtocolEraseBlocks ( > FvbDevice = FVB_DEVICE_FROM_THIS (This); > > Status = GetFvbInstance (FvbDevice->Instance, mFvbModuleGlobal, > - &FwhInstance, EfiGoneVirtual ()); > + &FwhInstance); > ASSERT_EFI_ERROR (Status); > > NumOfBlocks = FwhInstance->NumOfBlocks; > @@ -1088,21 +1071,10 @@ FvbInitialize ( > FwVolHeader->HeaderLength - > sizeof (EFI_FIRMWARE_VOLUME_HEADER) > ); > + mFvbModuleGlobal->FvInstance = AllocateRuntimePool (BufferSize); > + ASSERT (mFvbModuleGlobal->FvInstance != NULL); > > - // > - // Only need to allocate once. There is only one copy of physical memory > for > - // the private data of each FV instance. But in virtual mode or in physical > - // mode, the address of the the physical memory may be different. > - // > - mFvbModuleGlobal->FvInstance[FVB_PHYSICAL] = AllocateRuntimePool ( > - BufferSize); > - ASSERT (mFvbModuleGlobal->FvInstance[FVB_PHYSICAL] != NULL); > - > - // > - // Make a virtual copy of the FvInstance pointer. > - // > - FwhInstance = mFvbModuleGlobal->FvInstance[FVB_PHYSICAL]; > - mFvbModuleGlobal->FvInstance[FVB_VIRTUAL] = FwhInstance; > + FwhInstance = mFvbModuleGlobal->FvInstance; > > mFvbModuleGlobal->NumFv = 0; > MaxLbaSize = 0; > @@ -1111,8 +1083,7 @@ FvbInitialize ( > (EFI_FIRMWARE_VOLUME_HEADER *) (UINTN) > PcdGet32 (PcdOvmfFlashNvStorageVariableBase); > > - FwhInstance->FvBase[FVB_PHYSICAL] = (UINTN) BaseAddress; > - FwhInstance->FvBase[FVB_VIRTUAL] = (UINTN) BaseAddress; > + FwhInstance->FvBase = (UINTN) BaseAddress; > > CopyMem ((UINTN *) &(FwhInstance->VolumeHeader), (UINTN *) FwVolHeader, > FwVolHeader->HeaderLength); > -- > 1.8.3.1 > > > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
