On 02/19/16 03:18, Ruiyu Ni wrote:
> Some platform doesn't use CPU(HOST)/Device 1:1 mapping for PCI Bus.
> But PCI IO doesn't have interface to tell caller (device driver)
> whether the address returned by GetBarAttributes() is HOST address
> or device address.
> UEFI Spec 2.6 addresses this issue by clarifying the address returned
> is HOST address and caller can use AddrTranslationOffset to calculate
> the device address.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Andrew Fish <[email protected]>
> Cc: Jeff Fan <[email protected]>
> ---
> MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c | 62
> ++++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> index 50ed866..55c9ce2 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> @@ -1744,6 +1744,53 @@ PciIoAttributes (
> }
>
> /**
> + Retrieve the AddrTranslationOffset from RootBridgeIo for the
> + specified range.
> +
> + @param RootBridgeIo Root Bridge IO instance.
> + @param AddrRangeMin The base address of the MMIO.
> + @param AddrLen The length of the MMIO.
> +
> + @retval The AddrTranslationOffset from RootBridgeIo for the
> + specified range, or (UINT64) -1 if the range is not
> + found in RootBridgeIo.
> +**/
> +UINT64
> +GetMmioAddressTranslationOffset (
> + EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL *RootBridgeIo,
> + UINT64 AddrRangeMin,
> + UINT64 AddrLen
> + )
> +{
> + EFI_STATUS Status;
> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Configuration;
> +
> + Status = RootBridgeIo->Configuration (
> + RootBridgeIo,
> + (VOID **) &Configuration
> + );
> + if (EFI_ERROR (Status)) {
> + return (UINT64) -1;
> + }
> +
> + while (Configuration->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) {
> + if ((Configuration->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) &&
> + (Configuration->AddrRangeMin <= AddrRangeMin) &&
> + (Configuration->AddrRangeMin + Configuration->AddrLen >=
> AddrRangeMin + AddrLen)
> + ) {
> + return Configuration->AddrTranslationOffset;
> + }
> + Configuration++;
> + }
> +
> + //
> + // The resource occupied by BAR should be in the range reported by
> RootBridge.
> + //
> + ASSERT (Configuration->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR);
I don't understand this assertion. If we reach this spot, then the while
loop terminated, which implies
Configuration->Desc != ACPI_ADDRESS_SPACE_DESCRIPTOR
So it seems like this ASSERT() will always fail.
What am I missing?
According to the comment, I think the intent is probably to enforce that
the loop always finds a match, and hence never completes. If that's the
case, can't you just add:
ASSERT (FALSE);
? I think that would me much clearer.
> + return (UINT64) -1;
> +}
> +
> +/**
> Gets the attributes that this PCI controller supports setting on a BAR
> using
> SetBarAttributes(), and retrieves the list of resource descriptors for a
> BAR.
>
> @@ -1863,6 +1910,21 @@ PciIoGetBarAttributes (
> End = (EFI_ACPI_END_TAG_DESCRIPTOR *) (Descriptor + 1);
> End->Desc = ACPI_END_TAG_DESCRIPTOR;
> End->Checksum = 0;
> +
> + //
> + // Get the Address Translation Offset
> + //
> + if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
> + Descriptor->AddrTranslationOffset = GetMmioAddressTranslationOffset (
> + PciIoDevice->PciRootBridgeIo,
> + Descriptor->AddrRangeMin,
> + Descriptor->AddrLen
> + );
> + if (Descriptor->AddrTranslationOffset == (UINT64) -1) {
> + FreePool (Descriptor);
> + return EFI_UNSUPPORTED;
> + }
> + }
> }
>
> return EFI_SUCCESS;
>
Otherwise the patch seems good to me.
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
