Agree. ASSERT(FALSE) is cleaner:) Regards, Ray
From: Laszlo Ersek [mailto:ler...@redhat.com] Sent: Friday, February 19, 2016 6:56 PM To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-de...@ml01.01.org Cc: Benjamin Herrenschmidt <b...@au1.ibm.com>; Andrew Fish <af...@apple.com>; Fan, Jeff <jeff....@intel.com> Subject: Re: [edk2] [Patch V2 2/2] MdeModuelPkg/PciBus: Return AddrTranslationOffset in GetBarAttributes 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 <ruiyu...@intel.com<mailto:ruiyu...@intel.com>> > Cc: Benjamin Herrenschmidt <b...@au1.ibm.com<mailto:b...@au1.ibm.com>> > Cc: Andrew Fish <af...@apple.com<mailto:af...@apple.com>> > Cc: Jeff Fan <jeff....@intel.com<mailto:jeff....@intel.com>> > --- > 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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
