[edk2,1/2] ArmVirtPkg/VirtFdtDxe: make installation of FDT as config table option

Message ID 1459423216-2415-2-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit 7a63d29151ae9fec47e409298999cd9a1ee4b5c1
Headers show

Commit Message

Ard Biesheuvel March 31, 2016, 11:20 a.m.
The arm64 kernel is hardwired to prefer DT over ACPI, unless 'acpi=force'
is passed on the kernel command line. The only other way to force the
kernel to use ACPI is not to pass an FDT to it in the first place. So
introduce a PCD that inhibits the installation of the QEMU supplied FDT
as a configuration table.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 ArmVirtPkg/ArmVirtPkg.dec            |  9 +++
 ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c   | 67 +++++++++++---------
 ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf |  3 +
 3 files changed, 48 insertions(+), 31 deletions(-)

-- 
2.5.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek March 31, 2016, 12:38 p.m. | #1
(1) please replace the word "option" with "optional" in the subject

On 03/31/16 13:20, Ard Biesheuvel wrote:
> The arm64 kernel is hardwired to prefer DT over ACPI, unless 'acpi=force'

> is passed on the kernel command line. The only other way to force the

> kernel to use ACPI is not to pass an FDT to it in the first place. So

> introduce a PCD that inhibits the installation of the QEMU supplied FDT

> as a configuration table.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  ArmVirtPkg/ArmVirtPkg.dec            |  9 +++

>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c   | 67 +++++++++++---------

>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf |  3 +

>  3 files changed, 48 insertions(+), 31 deletions(-)

> 

> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec

> index 89e8448a8443..c67d7aa678e1 100644

> --- a/ArmVirtPkg/ArmVirtPkg.dec

> +++ b/ArmVirtPkg/ArmVirtPkg.dec

> @@ -98,3 +98,12 @@ [PcdsFeatureFlag]

>    # The default is to turn off the kludge; DSC's can selectively enable it.

>    #

>    gArmVirtTokenSpaceGuid.PcdKludgeMapPciMmioAsCached|FALSE|BOOLEAN|0x00000006

> +

> +  #

> +  # Pure ACPI boot

> +  #

> +  # Inhibit installation of the FDT as a configuration table if this feature

> +  # PCD is TRUE. Otherwise, the OS is presented with both a DT and an ACPI

> +  # description of the platform, and it is up to the OS to choose.

> +  #

> +  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|FALSE|BOOLEAN|0x00000010


(2) Since we don't do BCD in edk2, I suggest you replace the token value
0x00000010 with 0x0000000a. ;)

(3) The rest of the patch looks good. If you wish, you can add a small
hint to the commit message: "this patch is best viewed with 'git show
-b'". (That's how I'm reviewing it at the second look.) Not important
though.

The above points themselves don't require a repost (you can fix them up
at commit); but let me first see patch #2 as well.

BTW this series is very welcome.

Thanks
Laszlo

> diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c

> index 437b2a879216..d3043fa9b877 100644

> --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c

> +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c

> @@ -317,9 +317,6 @@ InitializeVirtFdtDxe (

>      return EFI_NOT_FOUND;

>    }

>  

> -  Status = gBS->InstallConfigurationTable (&gFdtTableGuid, DeviceTreeBase);

> -  ASSERT_EFI_ERROR (Status);

> -

>    DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, DeviceTreeBase));

>  

>    RtcNode = -1;

> @@ -571,39 +568,47 @@ InitializeVirtFdtDxe (

>      }

>    }

>  

> -  //

> -  // UEFI takes ownership of the RTC hardware, and exposes its functionality

> -  // through the UEFI Runtime Services GetTime, SetTime, etc. This means we

> -  // need to disable it in the device tree to prevent the OS from attaching its

> -  // device driver as well.

> -  //

> -  if ((RtcNode != -1) &&

> -      fdt_setprop_string (DeviceTreeBase, RtcNode, "status",

> -        "disabled") != 0) {

> -    DEBUG ((EFI_D_WARN, "Failed to set PL031 status to 'disabled'\n"));

> -  }

> -

> -  if (HavePci) {

> +  if (!FeaturePcdGet (PcdPureAcpiBoot)) {

> +    //

> +    // Only install the FDT as a configuration table if we want to leave it up

> +    // to the OS to decide whether it prefers ACPI over DT.

>      //

> -    // Set the /chosen/linux,pci-probe-only property to 1, so that the PCI

> -    // setup we will perform in the firmware is honored by the Linux OS,

> -    // rather than torn down and done from scratch. This is generally a more

> -    // sensible approach, and aligns with what ACPI based OSes do in general.

> +    Status = gBS->InstallConfigurationTable (&gFdtTableGuid, DeviceTreeBase);

> +    ASSERT_EFI_ERROR (Status);

> +

>      //

> -    // In case we are exposing an emulated VGA PCI device to the guest, which

> -    // may subsequently get exposed via the Graphics Output protocol and

> -    // driven as an efifb by Linux, we need this setting to prevent the

> -    // framebuffer from becoming unresponsive.

> +    // UEFI takes ownership of the RTC hardware, and exposes its functionality

> +    // through the UEFI Runtime Services GetTime, SetTime, etc. This means we

> +    // need to disable it in the device tree to prevent the OS from attaching its

> +    // device driver as well.

>      //

> -    Node = fdt_path_offset (DeviceTreeBase, "/chosen");

> -    if (Node < 0) {

> -      Node = fdt_add_subnode (DeviceTreeBase, 0, "/chosen");

> +    if ((RtcNode != -1) &&

> +        fdt_setprop_string (DeviceTreeBase, RtcNode, "status",

> +          "disabled") != 0) {

> +      DEBUG ((EFI_D_WARN, "Failed to set PL031 status to 'disabled'\n"));

>      }

> -    if (Node < 0 ||

> -        fdt_setprop_u32 (DeviceTreeBase, Node, "linux,pci-probe-only", 1) < 0) {

> -      DEBUG ((EFI_D_WARN, "Failed to set /chosen/linux,pci-probe-only property\n"));

> +

> +    if (HavePci) {

> +      //

> +      // Set the /chosen/linux,pci-probe-only property to 1, so that the PCI

> +      // setup we will perform in the firmware is honored by the Linux OS,

> +      // rather than torn down and done from scratch. This is generally a more

> +      // sensible approach, and aligns with what ACPI based OSes do in general.

> +      //

> +      // In case we are exposing an emulated VGA PCI device to the guest, which

> +      // may subsequently get exposed via the Graphics Output protocol and

> +      // driven as an efifb by Linux, we need this setting to prevent the

> +      // framebuffer from becoming unresponsive.

> +      //

> +      Node = fdt_path_offset (DeviceTreeBase, "/chosen");

> +      if (Node < 0) {

> +        Node = fdt_add_subnode (DeviceTreeBase, 0, "/chosen");

> +      }

> +      if (Node < 0 ||

> +          fdt_setprop_u32 (DeviceTreeBase, Node, "linux,pci-probe-only", 1) < 0) {

> +        DEBUG ((EFI_D_WARN, "Failed to set /chosen/linux,pci-probe-only property\n"));

> +      }

>      }

>    }

> -

>    return EFI_SUCCESS;

>  }

> diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf

> index ee2503ac4a7e..f807bf76bb4b 100644

> --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf

> +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf

> @@ -73,6 +73,9 @@ [Pcd]

>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress

>    gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration

>  

> +[FeaturePcd]

> +  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot

> +

>  [Protocols]

>    gEfiDevicePathProtocolGuid

>  

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
index 89e8448a8443..c67d7aa678e1 100644
--- a/ArmVirtPkg/ArmVirtPkg.dec
+++ b/ArmVirtPkg/ArmVirtPkg.dec
@@ -98,3 +98,12 @@  [PcdsFeatureFlag]
   # The default is to turn off the kludge; DSC's can selectively enable it.
   #
   gArmVirtTokenSpaceGuid.PcdKludgeMapPciMmioAsCached|FALSE|BOOLEAN|0x00000006
+
+  #
+  # Pure ACPI boot
+  #
+  # Inhibit installation of the FDT as a configuration table if this feature
+  # PCD is TRUE. Otherwise, the OS is presented with both a DT and an ACPI
+  # description of the platform, and it is up to the OS to choose.
+  #
+  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|FALSE|BOOLEAN|0x00000010
diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
index 437b2a879216..d3043fa9b877 100644
--- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
+++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
@@ -317,9 +317,6 @@  InitializeVirtFdtDxe (
     return EFI_NOT_FOUND;
   }
 
-  Status = gBS->InstallConfigurationTable (&gFdtTableGuid, DeviceTreeBase);
-  ASSERT_EFI_ERROR (Status);
-
   DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, DeviceTreeBase));
 
   RtcNode = -1;
@@ -571,39 +568,47 @@  InitializeVirtFdtDxe (
     }
   }
 
-  //
-  // UEFI takes ownership of the RTC hardware, and exposes its functionality
-  // through the UEFI Runtime Services GetTime, SetTime, etc. This means we
-  // need to disable it in the device tree to prevent the OS from attaching its
-  // device driver as well.
-  //
-  if ((RtcNode != -1) &&
-      fdt_setprop_string (DeviceTreeBase, RtcNode, "status",
-        "disabled") != 0) {
-    DEBUG ((EFI_D_WARN, "Failed to set PL031 status to 'disabled'\n"));
-  }
-
-  if (HavePci) {
+  if (!FeaturePcdGet (PcdPureAcpiBoot)) {
+    //
+    // Only install the FDT as a configuration table if we want to leave it up
+    // to the OS to decide whether it prefers ACPI over DT.
     //
-    // Set the /chosen/linux,pci-probe-only property to 1, so that the PCI
-    // setup we will perform in the firmware is honored by the Linux OS,
-    // rather than torn down and done from scratch. This is generally a more
-    // sensible approach, and aligns with what ACPI based OSes do in general.
+    Status = gBS->InstallConfigurationTable (&gFdtTableGuid, DeviceTreeBase);
+    ASSERT_EFI_ERROR (Status);
+
     //
-    // In case we are exposing an emulated VGA PCI device to the guest, which
-    // may subsequently get exposed via the Graphics Output protocol and
-    // driven as an efifb by Linux, we need this setting to prevent the
-    // framebuffer from becoming unresponsive.
+    // UEFI takes ownership of the RTC hardware, and exposes its functionality
+    // through the UEFI Runtime Services GetTime, SetTime, etc. This means we
+    // need to disable it in the device tree to prevent the OS from attaching its
+    // device driver as well.
     //
-    Node = fdt_path_offset (DeviceTreeBase, "/chosen");
-    if (Node < 0) {
-      Node = fdt_add_subnode (DeviceTreeBase, 0, "/chosen");
+    if ((RtcNode != -1) &&
+        fdt_setprop_string (DeviceTreeBase, RtcNode, "status",
+          "disabled") != 0) {
+      DEBUG ((EFI_D_WARN, "Failed to set PL031 status to 'disabled'\n"));
     }
-    if (Node < 0 ||
-        fdt_setprop_u32 (DeviceTreeBase, Node, "linux,pci-probe-only", 1) < 0) {
-      DEBUG ((EFI_D_WARN, "Failed to set /chosen/linux,pci-probe-only property\n"));
+
+    if (HavePci) {
+      //
+      // Set the /chosen/linux,pci-probe-only property to 1, so that the PCI
+      // setup we will perform in the firmware is honored by the Linux OS,
+      // rather than torn down and done from scratch. This is generally a more
+      // sensible approach, and aligns with what ACPI based OSes do in general.
+      //
+      // In case we are exposing an emulated VGA PCI device to the guest, which
+      // may subsequently get exposed via the Graphics Output protocol and
+      // driven as an efifb by Linux, we need this setting to prevent the
+      // framebuffer from becoming unresponsive.
+      //
+      Node = fdt_path_offset (DeviceTreeBase, "/chosen");
+      if (Node < 0) {
+        Node = fdt_add_subnode (DeviceTreeBase, 0, "/chosen");
+      }
+      if (Node < 0 ||
+          fdt_setprop_u32 (DeviceTreeBase, Node, "linux,pci-probe-only", 1) < 0) {
+        DEBUG ((EFI_D_WARN, "Failed to set /chosen/linux,pci-probe-only property\n"));
+      }
     }
   }
-
   return EFI_SUCCESS;
 }
diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
index ee2503ac4a7e..f807bf76bb4b 100644
--- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
+++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
@@ -73,6 +73,9 @@  [Pcd]
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
   gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
 
+[FeaturePcd]
+  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot
+
 [Protocols]
   gEfiDevicePathProtocolGuid