On 05/30/16 11:24, Gary Lin wrote:
> When OVMF tried to load the file-based NvVars, it checked all the PCI
> instances and connected the drivers to the mass storage device. However,
> Xen registered its PCI device with a special class id (0xFF80), so
> ConnectRecursivelyIfPciMassStorage() couldn't recognize it and skipped the
> driver connecting for Xen PCI devices. In the end, the Xen block device
> wasn't initialized until EfiBootManagerConnectAll() was called, and it's
> already too late to load NvVars.
>
> This commit connects the Xen drivers in ConnectRecursivelyIfPciMassStorage()
> so that Xen can use the file-based NvVars.
>
> Cc: Jordan Justen <[email protected]>
> Cc: Laszlo Ersek <[email protected]>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Gary Lin <[email protected]>
> ---
> OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 9
> +++++++--
> .../Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 +
> 2 files changed, 8 insertions(+), 2 deletions(-)
The idea looks good to me, but I'd like to see a few cosmetic changes:
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> index befcc57..8b09956 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> @@ -1049,8 +1049,12 @@ ConnectRecursivelyIfPciMassStorage (
> EFI_STATUS Status;
> EFI_DEVICE_PATH_PROTOCOL *DevicePath;
> CHAR16 *DevPathStr;
> + BOOLEAN isXenPci;
>
> - if (IS_CLASS1 (PciHeader, PCI_CLASS_MASS_STORAGE)) {
> + isXenPci = PcdGetBool (PcdPciDisableBusEnumeration) && \
> + IS_CLASS2 (PciHeader, 0xFF, 0x80);
> +
> + if (IS_CLASS1 (PciHeader, PCI_CLASS_MASS_STORAGE) || isXenPci) {
What I dislike about this is:
- the runaway backslash after the "&&" :)
- and the fact that isXenPci is computed unnecessarily in some cases.
Instead, I recommend the following:
BOOLEAN IsPciMassStorage;
IsPciMassStorage = IS_CLASS1 (PciHeader, PCI_CLASS_MASS_STORAGE);
//
// Recognize PCI Mass Storage, and Xen PCI devices
//
if (IsPciMassStorage ||
(PcdGetBool (PcdPciDisableBusEnumeration) &&
IS_CLASS2 (PciHeader, 0xFF, 0x80))) {
> DevicePath = NULL;
> Status = gBS->HandleProtocol (
> Handle,
> @@ -1068,7 +1072,8 @@ ConnectRecursivelyIfPciMassStorage (
> if (DevPathStr != NULL) {
> DEBUG((
> EFI_D_INFO,
> - "Found Mass Storage device: %s\n",
> + "Found %s device: %s\n",
> + !isXenPci ? L"Mass Storage" : L"Xen",
And here, please replace (!isXenPci) with IsPciMassStorage.
> DevPathStr
> ));
> FreePool(DevPathStr);
> diff --git
> a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> index 5fcee3c..cd28be0 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> @@ -62,6 +62,7 @@ [Pcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
> gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut
> gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile
> + gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
>
> [Pcd.IA32, Pcd.X64]
> gEfiMdePkgTokenSpaceGuid.PcdFSBClock
>
Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
