On 02/18/16 15:18, Benjamin Herrenschmidt wrote: > On Thu, 2016-02-18 at 06:55 +0000, Ni, Ruiyu wrote: >> Ben, >> I just had a look at your changes. Nice feeling that our codes are >> quite similar, except yours are in a separate function. >> I thought about separating it to a sub function but later didn't do >> that because this sub function is only called once >> and the logic after all is quite simple:) > > The logic is simple yes but I dislike large functions, I find it makes > the overall code flow harder to follow ;-) Just a matter of personal > taste. > >> Can you give a rb for this patch? or you think a sub function is >> better? I can make that change if you insist. > > I'd prefer testing before rb'ing but I won't be able to do that for a > couple of weeks at least (travelling). Did you manage to find a > platform to test your changes ? I'll give it a closer look tomorrow and > send a rb based on pure code review. (Ping me if I forget).
I briefly skimmed your enormous patch in your github repo -- I see that there are several changes to OvmfPkg/QemuVideoDxe. Some of them seem to relate directly to the PciIo change (= address translation), while other changes in the driver are presumably related to some new feature or platform. Assuming the latter kind of change can be easily postponed, I'm now only going to ask this: with Ray's change in the generic PCI bus driver, *must* all PciIo-based device drivers be updated? Is this a (bug-)compatible change, or does it regress existing drivers (perhaps by triggering their hidden bugs)? If this PciIo change has potential to expose bugs or invalid silent assumptions in other drivers, then I think the patch should not be committed in isolation. It should be part of a series that addresses said assumptions in all dependent drivers that are present in the open source edk2 tree. I believe, for QemuVideoDxe, running on - qemu-system-(i386|x86_64), i440fx and q35 machine types, or - qemu-system-(arm|aarch64), "virt" machine type, the translation offset between CPU and device MMIO is, in practice, zero, so I guess in practice QemuVideoDxe won't break. But, it would be nice to see confirmation (plus in the long term, QemuVideoDxe should be updated nonetheless). Thanks Laszlo > > Cheers, > Ben. > >> Regards, >> Ray >> >> -----Original Message----- >> From: Benjamin Herrenschmidt [mailto:[email protected]] >> Sent: Thursday, February 18, 2016 11:21 AM >> To: Ni, Ruiyu <[email protected]>; [email protected] >> Cc: Andrew Fish <[email protected]>; Fan, Jeff <[email protected]> >> Subject: Re: [Patch 2/2] MdeModuelPkg/PciBus: Return >> AddrTranslationOffset in GetBarAttributes >> >> On Thu, 2016-02-18 at 10:33 +0800, 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. >> >> Hi ! >> >> I had implemented that a bit differently mostly for code clarity >> (see below pasted from github) >> >> Ie, make it a sub function. Otherwise I think the principle is ok. >> >> Note that I haven't been able to work on my EDK2 port for a while now >> due to other commitments. I will eventually get back to it and test >> your patch. >> >> You can browse my monster-patch on github, I need to split/clean it >> into a proper patch series, it also contains some updates of a couple >> of drivers to actually use that offset properly >> >> https://github.com/ozbenh/edk2/commits/master >> >> + Finds the root bridge resource corresponding to the BAR whose >> address >> + space descritpor is passes as an argument and copy the >> translation offset >> + over from it into the BAR descriptor >> + >> + @param RootBridge A pointer to the >> EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL instance. >> + @param BarAddressSpace A pointer to the ACPI 2.0 resource >> descriptors that describe the current >> + configuration of this BAR of the >> PCI controller that is to be updated. >> + >> + @retval EFI_SUCCESS The descriptor was successfully >> updated. >> + @retval EFI_UNSUPPORTED No resource in the root bridge >> matches the BAR region >> + @retval EFI_OUT_OF_RESOURCES There are not enough resources >> available to allocate >> + Resources. >> + >> +**/ >> +STATIC >> +EFI_STATUS >> +PciBarUpdateTranslationOffset( >> + IN EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL *RootBridge, >> + IN OUT EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *BarAddressSpace) >> +{ >> + EFI_STATUS Status; >> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *RootDescriptors; >> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc; >> + UINT8 *Temp; >> + >> + Status = RootBridge->Configuration (RootBridge, (VOID **) >> &RootDescriptors); >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + Temp = (UINT8 *) RootDescriptors; >> + while (*Temp == ACPI_ADDRESS_SPACE_DESCRIPTOR) { >> + Desc = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Temp; >> + >> + // >> + // Do we have the right type of descriptor ? >> + // >> + if (Desc->ResType == BarAddressSpace->ResType) { >> + // >> + // Check if the BAR base fits >> + // >> + if (BarAddressSpace->AddrRangeMin >= Desc->AddrRangeMin && >> + BarAddressSpace->AddrRangeMin < (Desc->AddrRangeMin + >> Desc->AddrLen)) { >> + // >> + // Found it, update offset and return >> + // >> + BarAddressSpace->AddrTranslationOffset = Desc- >>> AddrTranslationOffset; >> + return EFI_SUCCESS; >> + } >> + } >> + Temp += sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR); >> + } >> + return EFI_UNSUPPORTED; >> +} >> + >> +/** >> Gets the attributes that this PCI controller supports setting on >> a BAR using >> SetBarAttributes(), and retrieves the list of resource >> descriptors for a BAR. >> >> @@ -1778,6 +1834,7 @@ PciIoGetBarAttributes ( >> PCI_IO_DEVICE *PciIoDevice; >> EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *AddressSpace; >> EFI_ACPI_END_TAG_DESCRIPTOR *End; >> + EFI_STATUS Status; >> >> PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This); >> >> @@ -1878,6 +1935,15 @@ PciIoGetBarAttributes ( >> } >> >> // >> + // Update the AddrTranslationOffset field >> + // >> + Status = PciBarUpdateTranslationOffset(PciIoDevice- >>> PciRootBridgeIo, AddressSpace); >> + if (EFI_ERROR (Status)) { >> + FreePool(AddressSpace); >> + return Status; >> + } >> + >> + // >> // put the checksum >> // >> End = (EFI_ACPI_END_TAG_DESCRIPTOR *) (AddressSpace + >> 1); >> View >> >>> 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 | 37 >>> ++++++++++++++++++++++++++++++++++ >>> 1 file changed, 37 insertions(+) >>> >>> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c >>> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c >>> index 50ed866..5cee8ca 100644 >>> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c >>> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c >>> @@ -1774,8 +1774,10 @@ PciIoGetBarAttributes ( >>> OUT VOID **Resources OPTIONAL >>> ) >>> { >>> + EFI_STATUS Status; >>> PCI_IO_DEVICE *PciIoDevice; >>> EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor; >>> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Configuration; >>> EFI_ACPI_END_TAG_DESCRIPTOR *End; >>> >>> PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This); >>> @@ -1863,6 +1865,41 @@ 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) { >>> + Status = PciIoDevice->PciRootBridgeIo->Configuration ( >>> + PciIoDevice- >>>> PciRootBridgeIo, >>> + (VOID **) >>> &Configuration >>> + ); >>> + if (!EFI_ERROR (Status)) { >>> + while (Configuration->Desc == Descriptor->Desc) { >>> + if ((Configuration->ResType == Descriptor->ResType) && >>> + (Configuration->AddrRangeMin <= Descriptor- >>>> AddrRangeMin) && >>> + (Configuration->AddrRangeMin + Configuration- >>>> AddrLen >>>> = Descriptor->AddrRangeMin + Descriptor->AddrLen) >>> + ) { >>> + Descriptor->AddrTranslationOffset = Configuration- >>>> AddrTranslationOffset; >>> + break; >>> + } >>> + Configuration++; >>> + } >>> + >>> + // >>> + // The resource occupied by BAR should be in the range >>> reported by RootBridge. >>> + // >>> + ASSERT (Configuration->Desc == Descriptor->Desc); >>> + if (Configuration->Desc != Descriptor->Desc) { >>> + Status = EFI_UNSUPPORTED; >>> + } >>> + } >>> + >>> + if (EFI_ERROR (Status)) { >>> + FreePool (Descriptor); >>> + return Status; >>> + } >>> + } >>> } >>> >>> return EFI_SUCCESS; > _______________________________________________ > 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
