On 11 March 2016 at 18:47, Laszlo Ersek <ler...@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.biesheu...@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
