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

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

Laszlo,
Your understanding is: DeviceAddress = HostAddress + AddressTranslationOffset
But my patch assumes: HostAddress = DeviceAddress + AddressTranslationOffset

They are totally different. If I follow your understanding, the patch is wrong!
Since UEFI spec doesn't describe "apply to" in sentence " Offset to apply to the
Starting address of a BAR to convert it to a PCI address" very clearly, I quoted
the statement from ACPI spec.
Your understanding to "apply to" is "add", my understanding is "minus".

Thanks/Ray

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Monday, September 11, 2017 2:47 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Dong Wei <Dong.Wei@arm.com>; Benjamin Herrenschmidt
> <benh@au1.ibm.com>; Andrew Fish <afish@apple.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes()
> returns Host address
> 
> On 09/11/17 07:01, Ruiyu Ni wrote:
> > Per the UEFI Spec, GetBarAttributes() should return the Host address.
> > But current implementation returns the address read from the BAR,
> > which is the Device address.
> > Per the description of AddressTranslationOffset in ACPI spec:
> > "For bridges that translate addresses across the bridge, this is the
> > offset that must be added to the address on the secondary side to
> > obtain the address on the primary side."
> 
> The ACPI spec also says:
> 
> "Non-bridge devices must list 0 for all Address Translation offset bits."
> 
> However, the UEFI spec (v2.7) says, under
> EFI_PCI_IO_PROTOCOL.GetBarAttributes():
> 
> "The ACPI Specification does not define how to the use the Address
> Translation Offset for non-bridge devices. The UEFI Specification is extending
> the definition of Address Translation Offset to support systems that have
> different mapping for HostAddress and DeviceAddress.
> [...] Address Translation Offset. Offset to apply to the Starting address of a
> BAR to convert it to a PCI address. This value is zero unless the HostAddress
> and DeviceAddress for the BAR are different."
> 
> So, I think the patch is correct, but the commit message should not refer to
> the ACPI spec. It should refer to / quote the UEFI spec only.
> 
> > HostAddress = DeviceAddress + AddressTranslationOffset.
> 
> The sentences from the UEFI spec are "Address Translation Offset. Offset to
> apply to the Starting address of a BAR to convert it to a PCI address", and
> "Address Range Minimum. Starting address of BAR."
> 
> To me this seems to imply that AddrRangeMin is already a host address, and
> 
> DeviceAddress = AddrRangeMin + AddressTranslationOffset
> 
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Benjamin Herrenschmidt <benh@au1.ibm.com>
> > Cc: Andrew Fish <afish@apple.com>
> > Cc: Dong Wei <Dong.Wei@arm.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Bartosz Szczepanek <bsz@semihalf.com>
> > ---
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> > index cc7125e4fc..852d35d710 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> > @@ -1955,7 +1955,7 @@ PciIoGetBarAttributes (
> >      End->Checksum = 0;
> >
> >      //
> > -    // Get the Address Translation Offset
> > +    // Get the Address Translation Offset and convert the Device address to
> Host address.
> >      //
> >      if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
> >        Descriptor->AddrTranslationOffset =
> > GetMmioAddressTranslationOffset ( @@ -1967,6 +1967,7 @@
> PciIoGetBarAttributes (
> >          FreePool (Descriptor);
> >          return EFI_UNSUPPORTED;
> >        }
> > +      Descriptor->AddrRangeMin += Descriptor->AddrTranslationOffset;
> >      }
> >    }
> >
> >
> 
> Actually, let me circle back to the initial problem here (apologies if it's too late
> for that) -- why are we adding the offset inside the
> GetBarAttributes() function? Isn't it the caller's responsibility to do the
> addition after GetBarAttributes() returns?
> 
> I mean if a PCI driver author reads the UEFI 2.7 spec, the spec seems to give
> that impression.
> 
> (I'm sorry if I should have raised these questions last week -- I don't wish to
> block this patch. Please go ahead if I'm wrong.)
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel