On 05/18/16 10:16, Ni, Ruiyu wrote:
>>-----Original Message-----
>>From: Laszlo Ersek [mailto:[email protected]]
>>Sent: Tuesday, May 17, 2016 8:23 PM
>>To: Ni, Ruiyu <[email protected]>
>>Cc: [email protected]
>>Subject: Re: [edk2] [Patch 0/4] MdeModulePkg/PciBus Do not improperly degrade
>>resource
>>
>>On 05/17/16 04:03, Ruiyu Ni wrote:
>>> The patch serials refined the PciBus code and adds a new feature following
>>> PI spec 1.4a to not improperly degrade resource.
>>>
>>> Ruiyu Ni (4):
>>> MdeModulePkg/PciBus: use better name for local variables.
>>> MdeModulePkg/PciBus: Remove unused fields in PCI_BAR
>>> MdeModulePkg/PciBus: Use shorter global variable name
>>> MdeModulePkg/PciBus: do not improperly degrade resource
>>>
>>> MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c | 6 +--
>>> MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h | 5 +-
>>> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 58
>>> +++++++++++++++++-----
>>> .../Bus/Pci/PciBusDxe/PciResourceSupport.c | 26 ++++++----
>>> 4 files changed, 65 insertions(+), 30 deletions(-)
>>>
>>
>>* Please add the following reference to patch #4:
>>
>>https://mantis.uefi.org/mantis/view.php?id=1529
>
> I think it might not be proper to include the member only
> web page in the commit message.
I disagree -- I think the mantis ticket number should always be included in a
commit message, if the patch is related to a Mantis ticket. Sharing just the
number of the mantis ticket does not disclose any information that is not also
publicly available from the UEFI spec itself (the UEFI spec starts with a long
changelog of Mantis tickets).
The contents of the mantis ticket *are* private, but for actually reading a
ticket, general UEFI membership and the appropriate WG membership are required
too. So, capturing the mantis ticket number in the commit message helps
contributors that have access to Mantis, without disclosing sensitive
information to those who haven't.
Personally I frequently go to Mantis to understand the background of UEFI or PI
spec changes better. The ECRs attached to Mantis tickets often describe use
cases that are not carried over to the spec, but are important background.
In this specific case, the PI spec 1.4a, Volume 5, has the following in its
changelog:
1529 address space granularity errata
>>* I'm interested in this work primarily due to GPU assignment to
>>QEMU/KVM virtual machines. So I built OVMF applied with these patches,
>>and checked if they made a difference for the PCI resource map, with an
>>Nvidia GTX750 assigned to the guest.
>>
>>There's no difference; the 256MB BAR is still allocated under 4GB.
>>
>>* Apparently, EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL must be a
>>singleton protocol; it should come from the platform firmware, not from
>>the option ROM. In that case, how could OVMF implement this protocol, so
>>that such a GPU BAR would be allocated high?
>
> Option ROM depends on the PCI resource assignment so it’s like a
> chicken-egg problem if we let option rom produce such protocol.
> Singleton is enough I think.
I agree.
>>Looking at the CheckDevice() spec, I guess we could hard-code some PCI
>>vendor and device IDs, and set AddrSpaceGranularity to 64. However,
>>that's the only thing we should do; I don't think we should do the rest
>>of the PCI BAR probing.
>
> PCI BAR probing is still needed. We do not change the granularity
> for 32bit BAR.
>>
>>... The only thing I'd like to say to the resource allocator is,
>>
>> If CSM is disabled, then please don't degrade the MMIO64 BARs of this
>> device, just because it has an oprom.
>>
>>(In fact, the device is not an incompatible PCI device.)
>
> I agree. But another rule is MdeModulePkg cannot depend
> on IntelFrameworkPkg. PciBusDxe is in MdeModulePkg; LegacyBios
> protocol is defined in IntelFrameworkPkg.
That's fine; I'm not suggesting that PciBusDxe itself look for the legacy BIOS
protocol. Instead, platform code in OvmfPkg could look for the legacy BIOS
protocol (or determine CSM presence in another way), and then instruct
PciBusDxe through a well-defined interface not to degrade the 64-bit MMIO BARs.
My interest is in such a well-defined interface.
>>* By following the links in Mantis #1529 (recursively), I arrived at a
>>message that points to the following driver:
>>
>> MdeModulePkg/Bus/Pci/IncompatiblePciDeviceSupportDxe
>>
>>OVMF does not include this driver at the moment. It is a small driver
>>and I guess we could fork it for OvmfPkg, but how should we fill in the
>>"mIncompatiblePciDeviceList" array?
>>
>
> Try to use below value to disable all MMIO degrade.
> Not tested.
>
> GLOBAL_REMOVE_IF_UNREFERENCED UINT64 mIncompatiblePciDeviceList[] = {
> //
> // DEVICE_INF_TAG,
> // PCI_DEVICE_ID (VendorID, DeviceID, Revision, SubVendorId, SubDeviceId),
> // DEVICE_RES_TAG,
> // ResType, GFlag , SFlag, Granularity, RangeMin,
> // RangeMax, Offset, AddrLen
> //
> DEVICE_INF_TAG,
> PCI_DEVICE_ID(DEVICE_ID_NOCARE, DEVICE_ID_NOCARE, DEVICE_ID_NOCARE,
> DEVICE_ID_NOCARE, DEVICE_ID_NOCARE),
> DEVICE_RES_TAG,
> PCI_BAR_TYPE_MEM,
> PCI_ACPI_UNUSED,
> PCI_ACPI_UNUSED,
> 64,
> PCI_ACPI_UNUSED,
> PCI_BAR_OLD_ALIGN,
> PCI_BAR_ALL,
> PCI_BAR_NOCHANGE,
Ah, thanks a lot! Now I have taken a much closer look at
Table 20. ACPI 2.0 & 3.0 QWORD Address Space Descriptor Usage
plus I reviewed the UpdatePciInfo() function with more context (including your
patch #4).
It looks like I can express exactly what I want.
I did my best to carefully review your patches. Patches 1-3 are mechanic and I
think they are correct.
For #1 through #3:
Reviewed-by: Laszlo Ersek <[email protected]>
For patch #4, I have the following comments:
- Please consider adding the reference to mantis ticket 1529.
- I think the patch does a little bit more than documented in the subject and
in the commit message. The main topic is of course the prevention of improper
degradation; however the patch also handles the other case, when a 64-bit MMIO
BAR is forced under the 4GB limit. I think it is correct and these things do
belong to the same patch, but maybe the commit message could mention it
briefly. I don't insist of course.
- Anyway I'll leave the above two points up to your consideration. For patch #4
too:
Reviewed-by: Laszlo Ersek <[email protected]>
Independently, I noticed that the interpretation of the
EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR.AddrTranslationOffset
field might not comply with the PI spec. Namely, in Table 20, the BAR index
wildcard is (UINT64)-1 (== MAX_UINT64). However, the value used for recognizing
the wildcard, in the UpdatePciInfo() function, is PCI_BAR_ALL, which is only
0xFF.
Similarly, the AddrRangeMax field ("Used to convey the alignment information")
should be zero if no special alignment is required. But, PCI_BAR_OLD_ALIGN has
value 0xFFFFFFFFFFFFFFFFULL.
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> index a6ade26e3a09..089919d59d9e 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> @@ -1327,8 +1327,8 @@ UpdatePciInfo (
> )
> {
> EFI_STATUS Status;
> - UINTN BarIndex;
> - UINTN BarEndIndex;
> + UINT64 BarIndex;
> + UINT64 BarEndIndex;
> BOOLEAN SetFlag;
> VOID *Configuration;
> EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Ptr;
> diff --git a/MdePkg/Include/IndustryStandard/Pci22.h
> b/MdePkg/Include/IndustryStandard/Pci22.h
> index 0b3a785ef1e6..3419ad4ce2cd 100644
> --- a/MdePkg/Include/IndustryStandard/Pci22.h
> +++ b/MdePkg/Include/IndustryStandard/Pci22.h
> @@ -763,7 +763,7 @@ typedef struct {
>
> #define PCI_ACPI_UNUSED 0
> #define PCI_BAR_NOCHANGE 0
> -#define PCI_BAR_OLD_ALIGN 0xFFFFFFFFFFFFFFFFULL
> +#define PCI_BAR_OLD_ALIGN 0
> #define PCI_BAR_EVEN_ALIGN 0xFFFFFFFFFFFFFFFEULL
> #define PCI_BAR_SQUAD_ALIGN 0xFFFFFFFFFFFFFFFDULL
> #define PCI_BAR_DQUAD_ALIGN 0xFFFFFFFFFFFFFFFCULL
> @@ -774,7 +774,7 @@ typedef struct {
> #define PCI_BAR_IDX3 0x03
> #define PCI_BAR_IDX4 0x04
> #define PCI_BAR_IDX5 0x05
> -#define PCI_BAR_ALL 0xFF
> +#define PCI_BAR_ALL 0xFFFFFFFFFFFFFFFFULL
>
> ///
> /// EFI PCI Option ROM definitions
What do you think? (In any case, this change would be separate from your
series.)
I'll seek to write a minimal EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL
implementation OvmfPkg that puts this series to use. As long as I use the same
macro names as the PCI bus driver, in order to fill in the ACPI resource
descriptor, there will be an understanding, regardless of the actual macro
values.
Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
