Re: [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() returns Host address

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

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:benh@au1.ibm.com]
> Sent: Wednesday, September 13, 2017 7:38 AM
> To: Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Dong Wei
> <Dong.Wei@arm.com>; Andrew Fish <afish@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.