search

tianocore / edk2

EDK II
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II
Other
4.52k stars 2.44k forks source link

MdeModulePkg/PciHostBridgeDxe: Ignore TypeIo for non X86/X64 architec… #5916

Open Javagedes opened 1 month ago

Javagedes commented 1 month ago

…tures.

Throughout PciHostBridgeDxe, functions iterate through all entries in the PCI_RESOURCE_TYPE enum. However TypeIo only applies for x86_64 platforms and on platforms such as Arm, the driver may exit early if resources are not available.

This changes the starting point of the PCI_RESOURCE_TYPE enum iteration from 0 to PCI_RESOURCE_TYPE_ENUM_START and conditionally sets that value to TypeIo or TypeMem32 based on whether the platform is x86_64 or not.

Description

<_Include a description of the change and why this change was made._> <_For each item, place an "x" in between `[` and `]` if true. Example: `[x]` (you can also check items in GitHub UI)_> <_Create the PR as a Draft PR if it is only created to run CI checks._> <_Delete lines in \<\> tags before creating the PR._> - [ ] Breaking change? - **Breaking change** - Does this PR cause a break in build or boot behavior? - Examples: Does it add a new library class or move a module to a different repo. - [ ] Impacts security? - **Security** - Does this PR have a direct security impact? - Examples: Crypto algorithm change or buffer overflow fix. - [ ] Includes tests? - **Tests** - Does this PR include any explicit test code? - Examples: Unit tests or integration tests. ## How This Was Tested <_Describe the test(s) that were run to verify the changes._> ## Integration Instructions <_Describe how these changes should be integrated. Use N/A if nothing is required._>
mdkinney commented 1 month ago

It looks like the logic is reversed in this PR. non X86 starts a TypeIo and the X86 starts at TypeMem32. Also, the 2 H files are almost identical except for the 1 #define. Can there be a common .h file and the arch specific ones only contain the one #define? I also wondering why this change is needed here. This is a generic module that interacts with Si specific APIs for resources. Can't the Si specific APIs return values if TypeIo is supposed to be ignored?

Javagedes commented 1 month ago

It looks like the logic is reversed in this PR. non X86 starts a TypeIo and the X86 starts at TypeMem32. Also, the 2 H files are almost identical except for the 1 #define. Can there be a common .h file and the arch specific ones only contain the one #define? I also wondering why this change is needed here. This is a generic module that interacts with Si specific APIs for resources. Can't the Si specific APIs return values if TypeIo is supposed to be ignored?

@mdkinney This is just me running my new changes through the pipelines. I will be making any permanent changes to #5853 if you wish to have this conversation there :)

However to answer your questions:

It looks like the logic is reversed in this PR. non X86 starts a TypeIo and the X86 starts at TypeMem32

Yes, Oops. I will update.

Can there be a common .h file and the arch specific ones only contain the one #define

Yes no problem

I also wondering why this change is needed here

The overarching issue is that for architectures that do not have TypeIO resources, when a PCI Driver requests TypeIO resources, we immediately bail, leaving us in a state such that the endpoint is no longer able to allocate any resources.