1. Patchew
  2. EDK2
  3. [edk2] [RFC v2 0/2] Add translation support to generic PCIHostBridge
  4. View patch

[edk2] [RFC v2 2/2] MdeModulePkg/PciBus: return CPU address for GetBarAttributes

Heyi Guo posted 2 patches 6 years, 10 months ago
[edk2] [RFC v2 2/2] MdeModulePkg/PciBus: return CPU address for GetBarAttributes
Posted by Heyi Guo 6 years, 10 months ago 
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;
-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC v2 2/2] MdeModulePkg/PciBus: return CPU address for GetBarAttributes
Posted by Laszlo Ersek 6 years, 10 months ago 
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
Re: [edk2] [RFC v2 2/2] MdeModulePkg/PciBus: return CPU address for GetBarAttributes
Posted by Guo Heyi 6 years, 10 months ago 
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

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.