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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 23 August 2016 at 23:23, Laszlo Ersek <lersek@redhat.com> wrote:
> 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.
>

Yes, that makes sense.

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

Thanks