On 11 September 2017 at 07:46, Laszlo Ersek <[email protected]> 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 <[email protected]> >> Cc: Ard Biesheuvel <[email protected]> >> Cc: Benjamin Herrenschmidt <[email protected]> >> Cc: Andrew Fish <[email protected]> >> Cc: Dong Wei <[email protected]> >> Cc: Laszlo Ersek <[email protected]> >> Cc: Bartosz Szczepanek <[email protected]> >> --- >> 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. _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
