[edk2] ArmVirtPkg/VirtFdtDxe: set /chosen/linux, pci-probe-only to 1 in DTB

Message ID 1457664818-12175-1-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel March 11, 2016, 2:53 a.m.
Unlike Linux on x86, which typically honors the PCI configuration performed
by the firmware, Linux on ARM assumes that the PCI subsystem needs to be
configured from scratch. This is not entirely unreasonable given the
historical background of embedded systems using very basic bootloaders,
but is no longer tenable with Linux on arm64 moving to UEFI and ACPI in the
server space. For this reason, PCI support in the arm64 kernel running under
ACPI is likely to move to the x86 model of honoring the PCI configuration
done by the firmware.

So let's align with that in our DT based configuration as well, and set the
/chosen/linux,pci-probe-only property to 1 in the Device Tree before we
hand it to the OS.

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 the OS, this ensures the PCI resource allocations for the framebuffer
are not overridden, since that would cause the framebuffer to stop working.

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

---
 ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c | 23 +++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

-- 
1.9.1

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

Comments

Laszlo Ersek March 11, 2016, 11:47 a.m. | #1
On 03/11/16 03:53, Ard Biesheuvel wrote:
> Unlike Linux on x86, which typically honors the PCI configuration performed

> by the firmware, Linux on ARM assumes that the PCI subsystem needs to be

> configured from scratch. This is not entirely unreasonable given the

> historical background of embedded systems using very basic bootloaders,

> but is no longer tenable with Linux on arm64 moving to UEFI and ACPI in the

> server space. For this reason, PCI support in the arm64 kernel running under

> ACPI is likely to move to the x86 model of honoring the PCI configuration

> done by the firmware.

> 

> So let's align with that in our DT based configuration as well, and set the

> /chosen/linux,pci-probe-only property to 1 in the Device Tree before we

> hand it to the OS.

> 

> 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 the OS, this ensures the PCI resource allocations for the framebuffer

> are not overridden, since that would cause the framebuffer to stop working.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

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

> ---

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

>  1 file changed, 22 insertions(+), 1 deletion(-)


The commit message is very clear, thanks for that.

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

> index 74f80d1d2b78..36484a0bbb7e 100644

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

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

> @@ -286,6 +286,7 @@ InitializeVirtFdtDxe (

>    VOID                           *DeviceTreeBase;

>    INT32                          Node, Prev;

>    INT32                          RtcNode;

> +  INT32                          ChosenNode;

>    EFI_STATUS                     Status;

>    CONST CHAR8                    *Type;

>    INT32                          Len;

> @@ -356,8 +357,28 @@ InitializeVirtFdtDxe (

>        ASSERT (Len == 2 * sizeof (UINT64));

>        Status = ProcessPciHost (DeviceTreeBase, Node, RegProp);

>        ASSERT_EFI_ERROR (Status);

> -      break;

>  

> +      //

> +      // 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.

> +      //

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

> +      if (ChosenNode < 0) {

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

> +      }

> +      if (ChosenNode < 0) {

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

> +        break;

> +      }

> +      fdt_setprop_u32 (DeviceTreeBase, ChosenNode, "linux,pci-probe-only", 1);

> +      break;

>      case PropertyTypeFwCfg:

>        ASSERT (Len == 2 * sizeof (UINT64));


Can you please move this logic out of the loop? I reviewed the
documentation for fdt_add_subnode() and fdt_setprop_u32(), and they can
both insert new data in the device tree, changing the offsets of some
existing nodes.

Theoretically, this could mean that the offset of the current node
changes, and then the Prev = Node; fdt_next_node (... Prev ... )
sequence might not work. Without the libfdt header giving more specific
guarantees, I think we should keep all modifications of the device tree
out of the loop body -- the iteration should be read only.

There is already an example for this, the modification of the RTC node
near the end of the function. In order to constrain the logic to the
case when a PCI host node is present, you could introduce a boolean flag
(to be set under the case label), or even compare
PcdPciExpressBaseAddress against zero (it is set by ProcessPciHost()).

... Actually, since the loop caches the RtcNode offset as well, that's
another thing that could break if we inserted nodes or properties during
the iteration. So this manipulation has to happen after disabling the
RTC -- looking up /chosen from scratch will work fine even then.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 11, 2016, 12:08 p.m. | #2
On 11 March 2016 at 18:47, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/11/16 03:53, Ard Biesheuvel wrote:

>> Unlike Linux on x86, which typically honors the PCI configuration performed

>> by the firmware, Linux on ARM assumes that the PCI subsystem needs to be

>> configured from scratch. This is not entirely unreasonable given the

>> historical background of embedded systems using very basic bootloaders,

>> but is no longer tenable with Linux on arm64 moving to UEFI and ACPI in the

>> server space. For this reason, PCI support in the arm64 kernel running under

>> ACPI is likely to move to the x86 model of honoring the PCI configuration

>> done by the firmware.

>>

>> So let's align with that in our DT based configuration as well, and set the

>> /chosen/linux,pci-probe-only property to 1 in the Device Tree before we

>> hand it to the OS.

>>

>> 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 the OS, this ensures the PCI resource allocations for the framebuffer

>> are not overridden, since that would cause the framebuffer to stop working.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.0

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

>> ---

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

>>  1 file changed, 22 insertions(+), 1 deletion(-)

>

> The commit message is very clear, thanks for that.

>

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

>> index 74f80d1d2b78..36484a0bbb7e 100644

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

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

>> @@ -286,6 +286,7 @@ InitializeVirtFdtDxe (

>>    VOID                           *DeviceTreeBase;

>>    INT32                          Node, Prev;

>>    INT32                          RtcNode;

>> +  INT32                          ChosenNode;

>>    EFI_STATUS                     Status;

>>    CONST CHAR8                    *Type;

>>    INT32                          Len;

>> @@ -356,8 +357,28 @@ InitializeVirtFdtDxe (

>>        ASSERT (Len == 2 * sizeof (UINT64));

>>        Status = ProcessPciHost (DeviceTreeBase, Node, RegProp);

>>        ASSERT_EFI_ERROR (Status);

>> -      break;

>>

>> +      //

>> +      // 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.

>> +      //

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

>> +      if (ChosenNode < 0) {

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

>> +      }

>> +      if (ChosenNode < 0) {

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

>> +        break;

>> +      }

>> +      fdt_setprop_u32 (DeviceTreeBase, ChosenNode, "linux,pci-probe-only", 1);

>> +      break;

>>      case PropertyTypeFwCfg:

>>        ASSERT (Len == 2 * sizeof (UINT64));

>

> Can you please move this logic out of the loop? I reviewed the

> documentation for fdt_add_subnode() and fdt_setprop_u32(), and they can

> both insert new data in the device tree, changing the offsets of some

> existing nodes.

>

> Theoretically, this could mean that the offset of the current node

> changes, and then the Prev = Node; fdt_next_node (... Prev ... )

> sequence might not work. Without the libfdt header giving more specific

> guarantees, I think we should keep all modifications of the device tree

> out of the loop body -- the iteration should be read only.

>

> There is already an example for this, the modification of the RTC node

> near the end of the function. In order to constrain the logic to the

> case when a PCI host node is present, you could introduce a boolean flag

> (to be set under the case label), or even compare

> PcdPciExpressBaseAddress against zero (it is set by ProcessPciHost()).

>

> ... Actually, since the loop caches the RtcNode offset as well, that's

> another thing that could break if we inserted nodes or properties during

> the iteration. So this manipulation has to happen after disabling the

> RTC -- looking up /chosen from scratch will work fine even then.

>


Ah yes, how sloppy of me. I will just add a boolean, and move this
code to the end of the function.

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

Patch

diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
index 74f80d1d2b78..36484a0bbb7e 100644
--- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
+++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
@@ -286,6 +286,7 @@  InitializeVirtFdtDxe (
   VOID                           *DeviceTreeBase;
   INT32                          Node, Prev;
   INT32                          RtcNode;
+  INT32                          ChosenNode;
   EFI_STATUS                     Status;
   CONST CHAR8                    *Type;
   INT32                          Len;
@@ -356,8 +357,28 @@  InitializeVirtFdtDxe (
       ASSERT (Len == 2 * sizeof (UINT64));
       Status = ProcessPciHost (DeviceTreeBase, Node, RegProp);
       ASSERT_EFI_ERROR (Status);
-      break;
 
+      //
+      // 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.
+      //
+      ChosenNode = fdt_path_offset (DeviceTreeBase, "/chosen");
+      if (ChosenNode < 0) {
+        ChosenNode = fdt_add_subnode (DeviceTreeBase, 0, "/chosen");
+      }
+      if (ChosenNode < 0) {
+        DEBUG ((EFI_D_WARN, "Failed to set /chosen/linux,pci-probe-only property"));
+        break;
+      }
+      fdt_setprop_u32 (DeviceTreeBase, ChosenNode, "linux,pci-probe-only", 1);
+      break;
     case PropertyTypeFwCfg:
       ASSERT (Len == 2 * sizeof (UINT64));