I am going to hold the patch. Unless some real issue is exposed using current PciBus driver, I will not make any change to current implementation.
Thanks/Ray > -----Original Message----- > From: Benjamin Herrenschmidt [mailto:b...@au1.ibm.com] > Sent: Wednesday, September 13, 2017 7:38 AM > To: Laszlo Ersek <ler...@redhat.com>; Ard Biesheuvel > <ard.biesheu...@linaro.org> > Cc: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org; Dong Wei > <dong....@arm.com>; Andrew Fish <af...@apple.com> > Subject: Re: [edk2] [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() > returns Host address > > On Tue, 2017-09-12 at 18:15 +0200, Laszlo Ersek wrote: > > > So IIUC, if we were to decide that AddrRangeMin contains the raw BAR > > > value, and the translation offset that needs to be applied to > > > produce the CPU address is added to it, we are quite close to the > > > intent of the definition of QWord, and our PCI I/O code is correct. > > > Only in this case, we need to fix all users of the protocol (i.e., > > > GOP producers) > > > > I'd be totally OK with that... > > Which happens to be the approach I took in my original port looking back at > my code. > > > > Given the low likelihood that this ever worked correctly for cases > > > where the translation offset != 0, I think that is perhaps the best > > > course of action. > > > > ...as long as the USWG agreed to invert the sense of the fields in the > > UEFI spec, based on which the GOPs should be updated. > > I think we had that conversation back then ;-) I remember a number of > inconsistencies in some of the resource fields including existing cases of > UEFI > not quite following the ACPI spec. > > However: > > > In practice this would mean reverting > > <https://urldefense.proofpoint.com/v2/url?u=https-3A__mantis.uefi.org_ > > mantis_view.php-3Fid-3D1502&d=DwICaQ&c=jf_iaSHvJObTbx- > siA1ZOg&r=ilASlO > > > u_ee2uzSGEkp3JMw&m=Y8F47uHCO5ZLYtvPhj45KfJDaIOCvYcJVK7HG2h7PC > M&s=klaAU > > FJYXQhUuF3Ad3BNs3HfM_8TpbgsENeLHRlI-vc&e== >. By now the fix for > > Mantis#1502 has been in three released versions of the spec (one of > > the > > 2.5 Errata, 2.6 and 2.7). > > > > I'm fine both ways, as long as code and spec are consistent. From a > > development perspective though, I think the spec is harder to change > > than the code, no matter how ugly the code ends up. > > That's a bit annoying. We need to ensure nobody already implemented > something based on the above and rleased it... > > Note there's another problem in EDK implementation. The > AddressTranslationOffset field is hijacked in GetProposedResources return > code iirc (at least back then, this might have changed since). > > I had this patch to the header (and corresponding implementation > changes) in my tree in > MdePkg/Include/Protocol/PciHostBridgeResourceAllocation.h: > > /// > -/// A UINT64 value that contains the status of a PCI resource requested -/// > in the Configuration parameter returned by GetProposedResources() -/// > The legal values are EFI_RESOURCE_SATISFIED and > EFI_RESOURCE_NOT_SATISFIED > +/// If this bit is set, then the PCI Root Bridge doesn't support /// IO > +space > /// > -typedef UINT64 EFI_RESOURCE_ALLOCATION_STATUS; > +#define EFI_PCI_HOST_BRIDGE_NO_IO_DECODE 4 > > /// > -/// The request of this resource type could be fulfilled. Used in the -/// > Configuration parameter returned by GetProposedResources() to identify - > /// a PCI resources request that can be satisfied. > +/// The "Address Translation Offset" field of the ACPI descriptor /// > +returned by GetProposedResources() can contain either of these /// > +ranges of *unsigned* values: > +/// > +/// EFI_RESOURCE_NONEXISTENT : The resource request cannot be > +satisfied /// > +/// EFI_RESOURCE_LESS : The resource wasn't satisified but a small > +/// request could be > /// > -#define EFI_RESOURCE_SATISFIED 0x0000000000000000ULL > +/// Value < EFI_RESOURCE_OFFSET_LIMIT > +/// : An offset representing the translation from > +/// PCI addresses to CPU addresses > +// > +#define EFI_RESOURCE_NONEXISTENT 0xFFFFFFFFFFFFFFFFULL > +#define EFI_RESOURCE_LESS 0xFFFFFFFFFFFFFFFEULL > +#define EFI_RESOURCE_OFFSET_LIMIT 0xFFFFFFFFFFFFFFF0ULL > + > +// Helper for internal use of PciBusDxe > +#define EFI_RESOURCE_SATISFIED 0 > > /// > -/// The request of this resource type could not be fulfilled for its -/// > absence in the host bridge resource pool. Used in the Configuration > parameter -/// returned by GetProposedResources() to identify a PCI > resources request that -/// can not be satisfied. > +/// The request of this resource type could be fulfilled. Used in the > +/// Configuration parameter returned by GetProposedResources() to > +identify /// a PCI resources request that can be satisfied. > /// > -#define EFI_RESOURCE_NOT_SATISFIED 0xFFFFFFFFFFFFFFFFULL > +#define EFI_RESOURCE_IS_SATISFIED(Offset) \ > + (((UINT64)Offset) < EFI_RESOURCE_OFFSET_LIMIT) > > Along with making PciEnumerator.c use EFI_RESOURCE_IS_SATISFIED() > rather than comparing against EFI_RESOURCE_NOT_SATISFIED > > Cheers, > Ben. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
