| Message ID | 1457698773-26840-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 8b816c624dd407e1590d7c399c69ab307af3ef33 |
| Headers | show |
On 03/11/16 13:19, 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 | 27 +++++++++++++++++++- > 1 file changed, 26 insertions(+), 1 deletion(-) > > diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c > index 74f80d1d2b78..4e4989751455 100644 > --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c > +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c > @@ -304,6 +304,7 @@ InitializeVirtFdtDxe ( > UINT64 FwCfgDataSize; > UINT64 FwCfgDmaAddress; > UINT64 FwCfgDmaSize; > + BOOLEAN HavePci; > > Hob = GetFirstGuidHob(&gFdtHobGuid); > if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) { > @@ -322,6 +323,7 @@ InitializeVirtFdtDxe ( > DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, DeviceTreeBase)); > > RtcNode = -1; > + HavePci = FALSE; > // > // Now enumerate the nodes and install peripherals that we are interested in, > // i.e., GIC, RTC and virtio MMIO nodes > @@ -356,8 +358,8 @@ InitializeVirtFdtDxe ( > ASSERT (Len == 2 * sizeof (UINT64)); > Status = ProcessPciHost (DeviceTreeBase, Node, RegProp); > ASSERT_EFI_ERROR (Status); > + HavePci = TRUE; > break; > - Any particular reason to remove this empty line? I think it should be kept. > case PropertyTypeFwCfg: > ASSERT (Len == 2 * sizeof (UINT64)); > > @@ -579,5 +581,28 @@ InitializeVirtFdtDxe ( > "disabled") != 0) { > DEBUG ((EFI_D_WARN, "Failed to set PL031 status to 'disabled'\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")); Another nit (sorry I did notice this in v1, but ultimately forgot to mention it): please add a "\n" at the end of the warning. Please fix these up before you commit the patch. Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks! Laszlo > + } > + } > + > return EFI_SUCCESS; > } > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 11 March 2016 at 19:25, Laszlo Ersek <lersek@redhat.com> wrote: > On 03/11/16 13:19, 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 | 27 +++++++++++++++++++- >> 1 file changed, 26 insertions(+), 1 deletion(-) >> >> diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c >> index 74f80d1d2b78..4e4989751455 100644 >> --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c >> +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c >> @@ -304,6 +304,7 @@ InitializeVirtFdtDxe ( >> UINT64 FwCfgDataSize; >> UINT64 FwCfgDmaAddress; >> UINT64 FwCfgDmaSize; >> + BOOLEAN HavePci; >> >> Hob = GetFirstGuidHob(&gFdtHobGuid); >> if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) { >> @@ -322,6 +323,7 @@ InitializeVirtFdtDxe ( >> DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, DeviceTreeBase)); >> >> RtcNode = -1; >> + HavePci = FALSE; >> // >> // Now enumerate the nodes and install peripherals that we are interested in, >> // i.e., GIC, RTC and virtio MMIO nodes >> @@ -356,8 +358,8 @@ InitializeVirtFdtDxe ( >> ASSERT (Len == 2 * sizeof (UINT64)); >> Status = ProcessPciHost (DeviceTreeBase, Node, RegProp); >> ASSERT_EFI_ERROR (Status); >> + HavePci = TRUE; >> break; >> - > > Any particular reason to remove this empty line? I think it should be kept. > Post-conference exhaustion syndrome, mostly, and the fact that the red '-' does not stand out in my grey-on-black terminal >> case PropertyTypeFwCfg: >> ASSERT (Len == 2 * sizeof (UINT64)); >> >> @@ -579,5 +581,28 @@ InitializeVirtFdtDxe ( >> "disabled") != 0) { >> DEBUG ((EFI_D_WARN, "Failed to set PL031 status to 'disabled'\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")); > > Another nit (sorry I did notice this in v1, but ultimately forgot to > mention it): please add a "\n" at the end of the warning. > > Please fix these up before you commit the patch. > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > Thanks. Committed as 8b816c624dd4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c index 74f80d1d2b78..4e4989751455 100644 --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c @@ -304,6 +304,7 @@ InitializeVirtFdtDxe ( UINT64 FwCfgDataSize; UINT64 FwCfgDmaAddress; UINT64 FwCfgDmaSize; + BOOLEAN HavePci; Hob = GetFirstGuidHob(&gFdtHobGuid); if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) { @@ -322,6 +323,7 @@ InitializeVirtFdtDxe ( DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, DeviceTreeBase)); RtcNode = -1; + HavePci = FALSE; // // Now enumerate the nodes and install peripherals that we are interested in, // i.e., GIC, RTC and virtio MMIO nodes @@ -356,8 +358,8 @@ InitializeVirtFdtDxe ( ASSERT (Len == 2 * sizeof (UINT64)); Status = ProcessPciHost (DeviceTreeBase, Node, RegProp); ASSERT_EFI_ERROR (Status); + HavePci = TRUE; break; - case PropertyTypeFwCfg: ASSERT (Len == 2 * sizeof (UINT64)); @@ -579,5 +581,28 @@ InitializeVirtFdtDxe ( "disabled") != 0) { DEBUG ((EFI_D_WARN, "Failed to set PL031 status to 'disabled'\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")); + } + } + return EFI_SUCCESS; }
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 | 27 +++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) -- 1.9.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel