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

[Laszlo Ersek <lersek@redhat.com>]

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