Re: [PATCH 1/5] ArmVirtPkg/PciHostBridgeDxe: don't set linux, pci-probe-only DT property

[Laszlo Ersek <lersek@redhat.com>]

On 08/22/16 02:35, Ard Biesheuvel wrote:
> Setting the linux,pci-probe-only was intended to align OSes booting via
> DT with OSes booting via ACPI in the way they honor the PCI configuration
> performed by the firmware. However, ACPI on arm64 does not currently honor
> the firmware's PCI configuration, and the linux,pci-probe-only completely
> prevents any PCI reconfiguration from occurring under the OS, including
> what is needed to support PCI hotplug.
> 
> Since the primary use case was OS access to the GOP framebuffer (which
> breaks when the framebuffer BAR is moved when the OS reconfigures the
> PCI), we can undo this change now that ArmVirtQemu has moved to a GOP
> implementation that does not expose a raw framebuffer in the first place.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c | 38 --------------------
>  1 file changed, 38 deletions(-)
> 
> diff --git a/ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c b/ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c
> index 5063782bb392..669c90355889 100644
> --- a/ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c
> +++ b/ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c
> @@ -79,42 +79,6 @@ PCI_HOST_BRIDGE_INSTANCE mPciHostBridgeInstanceTemplate = {
>  // Implementation
>  //
>  
> -STATIC
> -VOID
> -SetLinuxPciProbeOnlyProperty (
> -  IN FDT_CLIENT_PROTOCOL         *FdtClient
> -  )
> -{
> -  INT32         Node;
> -  UINT32        Tmp;
> -  EFI_STATUS    Status;
> -
> -  if (!FeaturePcdGet (PcdPureAcpiBoot)) {
> -    //
> -    // 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 typically.
> -    //
> -    // 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.
> -    //
> -    Status = FdtClient->GetOrInsertChosenNode (FdtClient, &Node);
> -
> -    if (!EFI_ERROR (Status)) {
> -      Tmp = SwapBytes32 (1);
> -      Status = FdtClient->SetNodeProperty (FdtClient, Node,
> -                            "linux,pci-probe-only", &Tmp, sizeof (Tmp));
> -    }
> -    if (EFI_ERROR (Status)) {
> -      DEBUG ((EFI_D_WARN,
> -        "Failed to set /chosen/linux,pci-probe-only property\n"));
> -    }
> -  }
> -}
> -
>  //
>  // We expect the "ranges" property of "pci-host-ecam-generic" to consist of
>  // records like this.
> @@ -293,8 +257,6 @@ ProcessPciHost (
>    //
>    ASSERT (PcdGet64 (PcdPciExpressBaseAddress) == ConfigBase);
>  
> -  SetLinuxPciProbeOnlyProperty (FdtClient);
> -
>    DEBUG ((EFI_D_INFO, "%a: Config[0x%Lx+0x%Lx) Bus[0x%x..0x%x] "
>      "Io[0x%Lx+0x%Lx)@0x%Lx Mem[0x%Lx+0x%Lx)@0x%Lx\n", __FUNCTION__, ConfigBase,
>      ConfigSize, *BusMin, *BusMax, *IoBase, *IoSize, *IoTranslation, *MmioBase,
> 

I suggest to note in the commit message that this patch effectively
undoes commit 8b816c624dd407e1590d7c399c69ab307af3ef33. Can be done
without a resend, if you agree.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>0

(Further review feedback will likely be very slow, for the known reasons
-- I try to process my email backlog whenever I find a reasonable time
slot. For larger patches, larger slots are necessary.)

Thanks!
Laszlo