[edk2] MdeModulePkg/PciBusDxe: don't create bogus descriptor if no resources needed

Message ID 1461858688-31517-1-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit b59e2427c2d92cfee0238d9bde7372691c2af17c
Headers show

Commit Message

Ard Biesheuvel April 28, 2016, 3:51 p.m.
If the current PCI configuration requires no resources to be allocated at
all (i.e., unpopulated bus), the PCI enumeration code creates a single
ACPI_ADDRESS_SPACE_DESCRIPTOR memory descriptor with all fields cleared.
This is rejected by the SubmitResources() implementation of the generic
PciHostBridgeDxe in the following way:

  PciHostBridge: SubmitResources for PcieRoot(0x0)
   Mem: Granularity/SpecificFlag = 0 / 00
        Length/Alignment = 0x0 / 0x0
  PciBus: HostBridge->SubmitResources() - Invalid Parameter

  ASSERT_EFI_ERROR (Status = Invalid Parameter)
  ASSERT [PciBusDxe] .../PciBusDxe/PciLib.c(561): !EFI_ERROR (Status)

So instead, create the empty configuration as a single entry of type
EFI_ACPI_END_TAG_DESCRIPTOR.

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

---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

-- 
2.7.4

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

Comments

Laszlo Ersek April 28, 2016, 4:06 p.m. | #1
On 04/28/16 17:51, Ard Biesheuvel wrote:
> If the current PCI configuration requires no resources to be allocated at

> all (i.e., unpopulated bus), the PCI enumeration code creates a single

> ACPI_ADDRESS_SPACE_DESCRIPTOR memory descriptor with all fields cleared.

> This is rejected by the SubmitResources() implementation of the generic

> PciHostBridgeDxe in the following way:

> 

>   PciHostBridge: SubmitResources for PcieRoot(0x0)

>    Mem: Granularity/SpecificFlag = 0 / 00

>         Length/Alignment = 0x0 / 0x0

>   PciBus: HostBridge->SubmitResources() - Invalid Parameter

> 

>   ASSERT_EFI_ERROR (Status = Invalid Parameter)

>   ASSERT [PciBusDxe] .../PciBusDxe/PciLib.c(561): !EFI_ERROR (Status)

> 

> So instead, create the empty configuration as a single entry of type

> EFI_ACPI_END_TAG_DESCRIPTOR.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

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

> ---

>  MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c | 7 ++-----

>  1 file changed, 2 insertions(+), 5 deletions(-)


Is this error related to the other thread?

http://thread.gmane.org/gmane.comp.bios.edk2.devel/11135

In particular, to PcdPciDisableBusEnumeration?

Thanks
Laszlo

> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c

> index 597c0834e0f5..469a2ddb8ac0 100644

> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c

> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c

> @@ -1307,15 +1307,12 @@ ConstructAcpiResourceRequestor (

>      //

>      // If there is no resource request

>      //

> -    Configuration = AllocateZeroPool (sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) + sizeof (EFI_ACPI_END_TAG_DESCRIPTOR));

> +    Configuration = AllocateZeroPool (sizeof (EFI_ACPI_END_TAG_DESCRIPTOR));

>      if (Configuration == NULL) {

>        return EFI_OUT_OF_RESOURCES;

>      }

>  

> -    Ptr               = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) (Configuration);

> -    Ptr->Desc         = ACPI_ADDRESS_SPACE_DESCRIPTOR;

> -

> -    PtrEnd            = (EFI_ACPI_END_TAG_DESCRIPTOR *) (Ptr + 1);

> +    PtrEnd            = (EFI_ACPI_END_TAG_DESCRIPTOR *) (Configuration);

>      PtrEnd->Desc      = ACPI_END_TAG_DESCRIPTOR;

>      PtrEnd->Checksum  = 0;

>    }

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 28, 2016, 4:09 p.m. | #2
On 28 April 2016 at 18:06, Laszlo Ersek <lersek@redhat.com> wrote:
> On 04/28/16 17:51, Ard Biesheuvel wrote:

>> If the current PCI configuration requires no resources to be allocated at

>> all (i.e., unpopulated bus), the PCI enumeration code creates a single

>> ACPI_ADDRESS_SPACE_DESCRIPTOR memory descriptor with all fields cleared.

>> This is rejected by the SubmitResources() implementation of the generic

>> PciHostBridgeDxe in the following way:

>>

>>   PciHostBridge: SubmitResources for PcieRoot(0x0)

>>    Mem: Granularity/SpecificFlag = 0 / 00

>>         Length/Alignment = 0x0 / 0x0

>>   PciBus: HostBridge->SubmitResources() - Invalid Parameter

>>

>>   ASSERT_EFI_ERROR (Status = Invalid Parameter)

>>   ASSERT [PciBusDxe] .../PciBusDxe/PciLib.c(561): !EFI_ERROR (Status)

>>

>> So instead, create the empty configuration as a single entry of type

>> EFI_ACPI_END_TAG_DESCRIPTOR.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.0

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

>> ---

>>  MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c | 7 ++-----

>>  1 file changed, 2 insertions(+), 5 deletions(-)

>

> Is this error related to the other thread?

>

> http://thread.gmane.org/gmane.comp.bios.edk2.devel/11135

>

> In particular, to PcdPciDisableBusEnumeration?

>


No, it is completely separate, as far as I can tell. It simply creates
an incorrect descriptor list if none of the PCI devices behind a root
bridge require any resources at all, and this is probably a code path
that you are very unlikely to hit on x86.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek April 28, 2016, 4:13 p.m. | #3
On 04/28/16 18:09, Ard Biesheuvel wrote:
> On 28 April 2016 at 18:06, Laszlo Ersek <lersek@redhat.com> wrote:

>> On 04/28/16 17:51, Ard Biesheuvel wrote:

>>> If the current PCI configuration requires no resources to be allocated at

>>> all (i.e., unpopulated bus), the PCI enumeration code creates a single

>>> ACPI_ADDRESS_SPACE_DESCRIPTOR memory descriptor with all fields cleared.

>>> This is rejected by the SubmitResources() implementation of the generic

>>> PciHostBridgeDxe in the following way:

>>>

>>>   PciHostBridge: SubmitResources for PcieRoot(0x0)

>>>    Mem: Granularity/SpecificFlag = 0 / 00

>>>         Length/Alignment = 0x0 / 0x0

>>>   PciBus: HostBridge->SubmitResources() - Invalid Parameter

>>>

>>>   ASSERT_EFI_ERROR (Status = Invalid Parameter)

>>>   ASSERT [PciBusDxe] .../PciBusDxe/PciLib.c(561): !EFI_ERROR (Status)

>>>

>>> So instead, create the empty configuration as a single entry of type

>>> EFI_ACPI_END_TAG_DESCRIPTOR.

>>>

>>> Contributed-under: TianoCore Contribution Agreement 1.0

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

>>> ---

>>>  MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c | 7 ++-----

>>>  1 file changed, 2 insertions(+), 5 deletions(-)

>>

>> Is this error related to the other thread?

>>

>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/11135

>>

>> In particular, to PcdPciDisableBusEnumeration?

>>

> 

> No, it is completely separate, as far as I can tell. It simply creates

> an incorrect descriptor list if none of the PCI devices behind a root

> bridge require any resources at all, and this is probably a code path

> that you are very unlikely to hit on x86.


Ah, very interesting. So only devices with PCI config space, right? How
curious. :)

Thank you for the explanation!
Laszlo

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 29, 2016, 7:21 a.m. | #4
On 29 April 2016 at 09:19, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> Ard,

> At first glance of this patch, I immediately checked the PI spec SubResources() description,

> it says when a root bridge doesn't require any resource, "a zero-length resource request

> must explicitly be submitted."

> "zero-length resource" can be interpreted in two ways:

> 1. only END descriptor

> 2. MEM resource with 0-length, plus IO resource with 0-length, plus END descriptor

>

> I believe that both ways should be accepted by PciHostBridge driver.

> But without your patch, the zero descriptor cannot be accepted by PciHostBridge driver.

> I think it fails in the check:

>

> case ACPI_ADDRESS_SPACE_TYPE_MEM /* 0 */:

>   if (Descriptor->AddrSpaceGranularity != 32 && Descriptor->AddrSpaceGranularity != 64) {

>     return EFI_INVALID_PARAMETER;

>   }

>


I agree with your analysis. The NULL entry is misidentified as a
ACPI_ADDRESS_SPACE_TYPE_MEM resource descriptor because the constant
is defined as 0

> I agree with your patch.

>

> Reviewed-by: Ruiyu Ni <Ruiyu.ni@intel.com>

>


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

Patch

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
index 597c0834e0f5..469a2ddb8ac0 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
@@ -1307,15 +1307,12 @@  ConstructAcpiResourceRequestor (
     //
     // If there is no resource request
     //
-    Configuration = AllocateZeroPool (sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) + sizeof (EFI_ACPI_END_TAG_DESCRIPTOR));
+    Configuration = AllocateZeroPool (sizeof (EFI_ACPI_END_TAG_DESCRIPTOR));
     if (Configuration == NULL) {
       return EFI_OUT_OF_RESOURCES;
     }
 
-    Ptr               = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) (Configuration);
-    Ptr->Desc         = ACPI_ADDRESS_SPACE_DESCRIPTOR;
-
-    PtrEnd            = (EFI_ACPI_END_TAG_DESCRIPTOR *) (Ptr + 1);
+    PtrEnd            = (EFI_ACPI_END_TAG_DESCRIPTOR *) (Configuration);
     PtrEnd->Desc      = ACPI_END_TAG_DESCRIPTOR;
     PtrEnd->Checksum  = 0;
   }