Message ID | 1519282474-94811-3-git-send-email-heyi.guo@linaro.org |
---|---|
State | New |
Headers | show |
Series | Add translation support to generic PCIHostBridge | expand |
On 02/22/18 07:54, Heyi Guo wrote: > PciIo::GetBarAttributes should return CPU view address according to > UEFI spec 2.7, so we change the implementation to follow the spec. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Heyi Guo <heyi.guo@linaro.org> > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Star Zeng <star.zeng@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > > --- > MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c > index 190f4b0..0aafcba 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c > @@ -1814,8 +1814,8 @@ GetMmioAddressTranslationOffset ( > > while (Configuration->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) { > if ((Configuration->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) && > - (Configuration->AddrRangeMin <= AddrRangeMin) && > - (Configuration->AddrRangeMin + Configuration->AddrLen >= AddrRangeMin + AddrLen) > + (Configuration->AddrRangeMin + Configuration->AddrTranslationOffset <= AddrRangeMin) && > + (Configuration->AddrRangeMin + Configuration->AddrLen + Configuration->AddrTranslationOffset >= AddrRangeMin + AddrLen) > ) { > return Configuration->AddrTranslationOffset; > } > @@ -1968,6 +1968,11 @@ PciIoGetBarAttributes ( > return EFI_UNSUPPORTED; > } > } > + > + // According to UEFI spec 2.7, we need return CPU view address for PciIo::GetBarAttributes, > + // and PCI view = CPU view + translation > + Descriptor->AddrRangeMin -= Descriptor->AddrTranslationOffset; > + Descriptor->AddrRangeMax -= Descriptor->AddrTranslationOffset; > } > > return EFI_SUCCESS; > According to your blurb -- which should be instead part of the commit message of patch #1, as discussed before --, we have the following interpretations: * internal: host = device + ATO * external: device = host + ATO The GetMmioAddressTranslationOffset() change looks correct, because (according to your blurb) RootBridgeIo->Configuration() returns a host (CPU) view. Adding the ATO we get the device view, and then we can do the comparison against the BAR values read from the device. OK. In PciIoGetBarAttributes(), Descriptor->AddrRangeMin is first set from PciIoDevice, so it's a device view. I think the subtraction is correct; the caller will re-add the ATO if it wants to return to the device view. In PciIoGetBarAttributes(), I think the AddrRangeMax manipulation is incorrect (possibly due to a preexistent bug in PciBusDxe). Namely, Descriptor->AddrRangeMax is first set to the Alignment of the BAR, from PciIoDevice, not the end address of the BAR. In order to output the value required by the UEFI spec, we have to calculate the end address using AddrLen. Is that right? ... To repeat myself, I find it extremely hard to reason about this feature while using different internal and external ATO interpretations. Can we pick one formula and stick with it everywhere? (I don't insist, but without it, I don't think I can sensibly review future iterations of this set.) Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Thu, Feb 22, 2018 at 11:41:50AM +0100, Laszlo Ersek wrote: > On 02/22/18 07:54, Heyi Guo wrote: > > PciIo::GetBarAttributes should return CPU view address according to > > UEFI spec 2.7, so we change the implementation to follow the spec. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Heyi Guo <heyi.guo@linaro.org> > > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Cc: Star Zeng <star.zeng@intel.com> > > Cc: Eric Dong <eric.dong@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Cc: Michael D Kinney <michael.d.kinney@intel.com> > > > > --- > > MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c > > index 190f4b0..0aafcba 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c > > @@ -1814,8 +1814,8 @@ GetMmioAddressTranslationOffset ( > > > > while (Configuration->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) { > > if ((Configuration->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) && > > - (Configuration->AddrRangeMin <= AddrRangeMin) && > > - (Configuration->AddrRangeMin + Configuration->AddrLen >= AddrRangeMin + AddrLen) > > + (Configuration->AddrRangeMin + Configuration->AddrTranslationOffset <= AddrRangeMin) && > > + (Configuration->AddrRangeMin + Configuration->AddrLen + Configuration->AddrTranslationOffset >= AddrRangeMin + AddrLen) > > ) { > > return Configuration->AddrTranslationOffset; > > } > > @@ -1968,6 +1968,11 @@ PciIoGetBarAttributes ( > > return EFI_UNSUPPORTED; > > } > > } > > + > > + // According to UEFI spec 2.7, we need return CPU view address for PciIo::GetBarAttributes, > > + // and PCI view = CPU view + translation > > + Descriptor->AddrRangeMin -= Descriptor->AddrTranslationOffset; > > + Descriptor->AddrRangeMax -= Descriptor->AddrTranslationOffset; > > } > > > > return EFI_SUCCESS; > > > > According to your blurb -- which should be instead part of the commit > message of patch #1, as discussed before --, we have the following > interpretations: > > * internal: host = device + ATO > * external: device = host + ATO > > The GetMmioAddressTranslationOffset() change looks correct, because > (according to your blurb) RootBridgeIo->Configuration() returns a host > (CPU) view. Adding the ATO we get the device view, and then we can do > the comparison against the BAR values read from the device. OK. > > In PciIoGetBarAttributes(), Descriptor->AddrRangeMin is first set from > PciIoDevice, so it's a device view. I think the subtraction is correct; > the caller will re-add the ATO if it wants to return to the device view. > > In PciIoGetBarAttributes(), I think the AddrRangeMax manipulation is > incorrect (possibly due to a preexistent bug in PciBusDxe). Namely, > Descriptor->AddrRangeMax is first set to the Alignment of the BAR, from > PciIoDevice, not the end address of the BAR. In order to output the > value required by the UEFI spec, we have to calculate the end address > using AddrLen. Is that right? Will double check what it really is. > > ... To repeat myself, I find it extremely hard to reason about this > feature while using different internal and external ATO interpretations. > Can we pick one formula and stick with it everywhere? (I don't insist, > but without it, I don't think I can sensibly review future iterations of > this set.) I made the patch according to the conclusion here: https://lists.01.org/pipermail/edk2-devel/2018-February/020960.html if I understood that correctly :) However, if it turns out so confusing in the code, especially to someone who didn't participate in the discussions, I agree it may worth keeping all the definitions consistent in EDK2 code, while being different from what in ACPI ASL code. Thanks, Gary (Heyi Guo) > > Thanks > Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c index 190f4b0..0aafcba 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c @@ -1814,8 +1814,8 @@ GetMmioAddressTranslationOffset ( while (Configuration->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) { if ((Configuration->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) && - (Configuration->AddrRangeMin <= AddrRangeMin) && - (Configuration->AddrRangeMin + Configuration->AddrLen >= AddrRangeMin + AddrLen) + (Configuration->AddrRangeMin + Configuration->AddrTranslationOffset <= AddrRangeMin) && + (Configuration->AddrRangeMin + Configuration->AddrLen + Configuration->AddrTranslationOffset >= AddrRangeMin + AddrLen) ) { return Configuration->AddrTranslationOffset; } @@ -1968,6 +1968,11 @@ PciIoGetBarAttributes ( return EFI_UNSUPPORTED; } } + + // According to UEFI spec 2.7, we need return CPU view address for PciIo::GetBarAttributes, + // and PCI view = CPU view + translation + Descriptor->AddrRangeMin -= Descriptor->AddrTranslationOffset; + Descriptor->AddrRangeMax -= Descriptor->AddrTranslationOffset; } return EFI_SUCCESS;
PciIo::GetBarAttributes should return CPU view address according to UEFI spec 2.7, so we change the implementation to follow the spec. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Heyi Guo <heyi.guo@linaro.org> Cc: Ruiyu Ni <ruiyu.ni@intel.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Star Zeng <star.zeng@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> --- MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel