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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 11 September 2017 at 07:46, Laszlo Ersek <lersek@redhat.com> wrote:
> 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?
>

AddrRangeMin is indeed already defined to be a host address, which
means the code that returns it should apply the offset to the raw BAR
value.