I agree to put ecr number in commit message. Thanks, Ray
> 在 2016年5月18日，下午7:35，Laszlo Ersek <[email protected]> 写道： > > 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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
