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). 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
