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:[email protected]] On Behalf Of > Laszlo Ersek > Sent: Monday, September 11, 2017 2:47 PM > To: Ni, Ruiyu <[email protected]>; [email protected] > Cc: Dong Wei <[email protected]>; Benjamin Herrenschmidt > <[email protected]>; Andrew Fish <[email protected]>; Ard Biesheuvel > <[email protected]> > 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 <[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? > > 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 > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
