On 05/18/16 17:08, Ni, Ruiyu wrote: > I agree to put ecr number in commit message.
Series Tested-by: Laszlo Ersek <ler...@redhat.com> I'll soon submit a patch that enables this new functionality for OvmfPkg. Thank you! Laszlo >> 在 2016年5月18日，下午7:35，Laszlo Ersek <ler...@redhat.com> 写道： >> >> On 05/18/16 10:16, Ni, Ruiyu wrote: >>>> -----Original Message----- >>>> From: Laszlo Ersek [mailto:ler...@redhat.com] >>>> Sent: Tuesday, May 17, 2016 8:23 PM >>>> To: Ni, Ruiyu <ruiyu...@intel.com> >>>> Cc: edk2-de...@ml01.01.org >>>> 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 <ler...@redhat.com> >> >> 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 <ler...@redhat.com> >> >> 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 >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
