diff mbox series

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

Message ID 1519282474-94811-3-git-send-email-heyi.guo@linaro.org
State New
Headers show
Series Add translation support to generic PCIHostBridge | expand

Commit Message

gary guo Feb. 22, 2018, 6:54 a.m. UTC 
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

Comments

Laszlo Ersek Feb. 22, 2018, 10:41 a.m. UTC | #1 
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
gary guo Feb. 23, 2018, 1:10 a.m. UTC | #2 
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 mbox series

Patch

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;