[PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem Limit Checks

[PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem Limit Checks

[Wasim Khan]

With Address Translation Support, it is possible and
also correct that Mem and Pmem Limit cross the 4GB boundary.
Update the checks so that Mem/PMem Limit should not cross 4GB
from the Mem/PMem Base address.

Signed-off-by: Wasim Khan <wasim.khan@nxp.com>
---
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index d304fae..9cf7e98 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -117,8 +117,8 @@ CreateRootBridge (
   // Make sure Mem and MemAbove4G apertures are valid
   //
   if (RESOURCE_VALID (&Bridge->Mem)) {
-    ASSERT (Bridge->Mem.Limit < SIZE_4GB);
-    if (Bridge->Mem.Limit >= SIZE_4GB) {
+    ASSERT (Bridge->Mem.Limit < (Bridge->Mem.Base + SIZE_4GB));
+    if (Bridge->Mem.Limit >= (Bridge->Mem.Base + SIZE_4GB)) {
       return NULL;
     }
   }
@@ -129,8 +129,8 @@ CreateRootBridge (
     }
   }
   if (RESOURCE_VALID (&Bridge->PMem)) {
-    ASSERT (Bridge->PMem.Limit < SIZE_4GB);
-    if (Bridge->PMem.Limit >= SIZE_4GB) {
+    ASSERT (Bridge->PMem.Limit < (Bridge->PMem.Base + SIZE_4GB));
+    if (Bridge->PMem.Limit >= (Bridge->PMem.Base + SIZE_4GB)) {
       return NULL;
     }
   }
-- 
2.7.4

Re: [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem Limit Checks

[Ni, Ray]

Thanks for fixing the check.

PCI_ROOT_BRIDGE_APERTURE.Base/Limit are device address while the memory space
in GCD belongs to host domain.
So, host address for Mem/Pmem should be below 4GB while device address can across
4GB.

Can you enhance the check as below?
  ASSERT (TO_HOST_ADDRESS (Bridge->Mem.Limit, Bridge->Mem.Translation) < SIZE_4GB);
  if (TO_HOST_ADDRESS (Bridge->Mem.Limit, Bridge->Mem.Translation) >= SIZE_4GB) {
    return NULL;
  }

It will look more precise and can detect invalid Mem/Pmem resource.

> -----Original Message-----
> From: Wasim Khan <wasim.khan@nxp.com>
> Sent: Thursday, April 23, 2020 6:44 PM
> To: devel@edk2.groups.io
> Cc: ard.biesheuvel@linaro.org; v.sethi@nxp.com; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Wasim
> Khan <wasim.khan@nxp.com>
> Subject: [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem Limit Checks
> 
> With Address Translation Support, it is possible and
> also correct that Mem and Pmem Limit cross the 4GB boundary.
> Update the checks so that Mem/PMem Limit should not cross 4GB
> from the Mem/PMem Base address.
> 
> Signed-off-by: Wasim Khan <wasim.khan@nxp.com>
> ---
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index d304fae..9cf7e98 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -117,8 +117,8 @@ CreateRootBridge (
>    // Make sure Mem and MemAbove4G apertures are valid
>    //
>    if (RESOURCE_VALID (&Bridge->Mem)) {
> -    ASSERT (Bridge->Mem.Limit < SIZE_4GB);
> -    if (Bridge->Mem.Limit >= SIZE_4GB) {
> +    ASSERT (Bridge->Mem.Limit < (Bridge->Mem.Base + SIZE_4GB));
> +    if (Bridge->Mem.Limit >= (Bridge->Mem.Base + SIZE_4GB)) {
>        return NULL;
>      }
>    }
> @@ -129,8 +129,8 @@ CreateRootBridge (
>      }
>    }
>    if (RESOURCE_VALID (&Bridge->PMem)) {
> -    ASSERT (Bridge->PMem.Limit < SIZE_4GB);
> -    if (Bridge->PMem.Limit >= SIZE_4GB) {
> +    ASSERT (Bridge->PMem.Limit < (Bridge->PMem.Base + SIZE_4GB));
> +    if (Bridge->PMem.Limit >= (Bridge->PMem.Base + SIZE_4GB)) {
>        return NULL;
>      }
>    }
> --
> 2.7.4

Re: [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem Limit Checks

[Wasim Khan]

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Thursday, April 23, 2020 5:07 PM
> To: Wasim Khan <wasim.khan@nxp.com>; devel@edk2.groups.io
> Cc: ard.biesheuvel@linaro.org; Varun Sethi <V.Sethi@nxp.com>; Wu, Hao A
> <hao.a.wu@intel.com>
> Subject: RE: [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem
> Limit Checks
> 
> Thanks for fixing the check.
> 
> PCI_ROOT_BRIDGE_APERTURE.Base/Limit are device address while the memory
> space in GCD belongs to host domain.
> So, host address for Mem/Pmem should be below 4GB while device address can
> across 4GB.
> 


Hi Ray,
Thank you for the review. 
There are cases when we don't have PCIe host address below 4GB, and the PCIe HOST Address space is only available above 4GB. 
For Example, Mem: 40000000 - FFFFFFFF Translation=FFFFFF6000000000 will result in HOST Address = 0xA0FFFFFFFF . This is a valid use case, but below check will report ASSERT for this HOST ADDRESS. 


> Can you enhance the check as below?
>   ASSERT (TO_HOST_ADDRESS (Bridge->Mem.Limit, Bridge->Mem.Translation) <
> SIZE_4GB);
>   if (TO_HOST_ADDRESS (Bridge->Mem.Limit, Bridge->Mem.Translation) >=
> SIZE_4GB) {
>     return NULL;
>   }
> 
> It will look more precise and can detect invalid Mem/Pmem resource.
> 
> > -----Original Message-----
> > From: Wasim Khan <wasim.khan@nxp.com>
> > Sent: Thursday, April 23, 2020 6:44 PM
> > To: devel@edk2.groups.io
> > Cc: ard.biesheuvel@linaro.org; v.sethi@nxp.com; Wu, Hao A
> > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Wasim Khan
> > <wasim.khan@nxp.com>
> > Subject: [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem
> Limit
> > Checks
> >
> > With Address Translation Support, it is possible and also correct that
> > Mem and Pmem Limit cross the 4GB boundary.
> > Update the checks so that Mem/PMem Limit should not cross 4GB from the
> > Mem/PMem Base address.
> >
> > Signed-off-by: Wasim Khan <wasim.khan@nxp.com>
> > ---
> >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> > b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> > index d304fae..9cf7e98 100644
> > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> > @@ -117,8 +117,8 @@ CreateRootBridge (
> >    // Make sure Mem and MemAbove4G apertures are valid
> >    //
> >    if (RESOURCE_VALID (&Bridge->Mem)) {
> > -    ASSERT (Bridge->Mem.Limit < SIZE_4GB);
> > -    if (Bridge->Mem.Limit >= SIZE_4GB) {
> > +    ASSERT (Bridge->Mem.Limit < (Bridge->Mem.Base + SIZE_4GB));
> > +    if (Bridge->Mem.Limit >= (Bridge->Mem.Base + SIZE_4GB)) {
> >        return NULL;
> >      }
> >    }
> > @@ -129,8 +129,8 @@ CreateRootBridge (
> >      }
> >    }
> >    if (RESOURCE_VALID (&Bridge->PMem)) {
> > -    ASSERT (Bridge->PMem.Limit < SIZE_4GB);
> > -    if (Bridge->PMem.Limit >= SIZE_4GB) {
> > +    ASSERT (Bridge->PMem.Limit < (Bridge->PMem.Base + SIZE_4GB));
> > +    if (Bridge->PMem.Limit >= (Bridge->PMem.Base + SIZE_4GB)) {
> >        return NULL;
> >      }
> >    }
> > --
> > 2.7.4

Re: [edk2-devel] [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem Limit Checks

[Ni, Ray]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wasim Khan
> Sent: Thursday, April 23, 2020 9:52 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: ard.biesheuvel@linaro.org; Varun Sethi <V.Sethi@nxp.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem Limit Checks
> 
> 
> 
> > -----Original Message-----
> > From: Ni, Ray <ray.ni@intel.com>
> > Sent: Thursday, April 23, 2020 5:07 PM
> > To: Wasim Khan <wasim.khan@nxp.com>; devel@edk2.groups.io
> > Cc: ard.biesheuvel@linaro.org; Varun Sethi <V.Sethi@nxp.com>; Wu, Hao A
> > <hao.a.wu@intel.com>
> > Subject: RE: [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem
> > Limit Checks
> >
> > Thanks for fixing the check.
> >
> > PCI_ROOT_BRIDGE_APERTURE.Base/Limit are device address while the memory
> > space in GCD belongs to host domain.
> > So, host address for Mem/Pmem should be below 4GB while device address can
> > across 4GB.
> >
> 
> 
> Hi Ray,
> Thank you for the review.
> There are cases when we don't have PCIe host address below 4GB, and the PCIe HOST Address space is only available above
> 4GB.
> For Example, Mem: 40000000 - FFFFFFFF Translation=FFFFFF6000000000 will result in HOST Address = 0xA0FFFFFFFF . This is
> a valid use case, but below check will report ASSERT for this HOST ADDRESS.

OK. Now I remember that "Mem" reports the 32bit memory space (device address) and "MemAbove4GB" reports the 64bit memory space (device address).

Then if "Mem" reports memory range that across 4GB, it means the range above 4GB should be reported through "MemAbove4GB".

Re: [edk2-devel] [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem Limit Checks

[Wasim Khan]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray via
> groups.io
> Sent: Thursday, April 23, 2020 7:58 PM
> To: devel@edk2.groups.io; Wasim Khan <wasim.khan@nxp.com>
> Cc: ard.biesheuvel@linaro.org; Varun Sethi <V.Sethi@nxp.com>; Wu, Hao A
> <hao.a.wu@intel.com>
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PciHostBridge: Update Mem
> and PMem Limit Checks
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wasim
> > Khan
> > Sent: Thursday, April 23, 2020 9:52 PM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> > Cc: ard.biesheuvel@linaro.org; Varun Sethi <V.Sethi@nxp.com>; Wu, Hao
> > A <hao.a.wu@intel.com>
> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PciHostBridge: Update
> > Mem and PMem Limit Checks
> >
> >
> >
> > > -----Original Message-----
> > > From: Ni, Ray <ray.ni@intel.com>
> > > Sent: Thursday, April 23, 2020 5:07 PM
> > > To: Wasim Khan <wasim.khan@nxp.com>; devel@edk2.groups.io
> > > Cc: ard.biesheuvel@linaro.org; Varun Sethi <V.Sethi@nxp.com>; Wu,
> > > Hao A <hao.a.wu@intel.com>
> > > Subject: RE: [PATCH] MdeModulePkg/PciHostBridge: Update Mem and
> PMem
> > > Limit Checks
> > >
> > > Thanks for fixing the check.
> > >
> > > PCI_ROOT_BRIDGE_APERTURE.Base/Limit are device address while the
> > > memory space in GCD belongs to host domain.
> > > So, host address for Mem/Pmem should be below 4GB while device
> > > address can across 4GB.
> > >
> >
> >
> > Hi Ray,
> > Thank you for the review.
> > There are cases when we don't have PCIe host address below 4GB, and
> > the PCIe HOST Address space is only available above 4GB.
> > For Example, Mem: 40000000 - FFFFFFFF Translation=FFFFFF6000000000
> > will result in HOST Address = 0xA0FFFFFFFF . This is a valid use case, but below
> check will report ASSERT for this HOST ADDRESS.
> 
> OK. Now I remember that "Mem" reports the 32bit memory space (device
> address) and "MemAbove4GB" reports the 64bit memory space (device address).
> 
> Then if "Mem" reports memory range that across 4GB, it means the range above
> 4GB should be reported through "MemAbove4GB".
> 
Yes this is true, but some devices needs MMIO 32bit space only as per their BAR property, including E1000 EP.

> 
> 

Re: [edk2-devel] [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem Limit Checks

[Ni, Ray]

> -----Original Message-----
> From: Wasim Khan <wasim.khan@nxp.com>
> Sent: Thursday, April 23, 2020 10:54 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: ard.biesheuvel@linaro.org; Varun Sethi <V.Sethi@nxp.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem Limit Checks
> > > >
> > > > Thanks for fixing the check.
> > > >
> > > > PCI_ROOT_BRIDGE_APERTURE.Base/Limit are device address while the
> > > > memory space in GCD belongs to host domain.
> > > > So, host address for Mem/Pmem should be below 4GB while device
> > > > address can across 4GB.
> > > >
> > >
> > >
> > > Hi Ray,
> > > Thank you for the review.
> > > There are cases when we don't have PCIe host address below 4GB, and
> > > the PCIe HOST Address space is only available above 4GB.
> > > For Example, Mem: 40000000 - FFFFFFFF Translation=FFFFFF6000000000
> > > will result in HOST Address = 0xA0FFFFFFFF . This is a valid use case, but below
> > check will report ASSERT for this HOST ADDRESS.
> >
> > OK. Now I remember that "Mem" reports the 32bit memory space (device
> > address) and "MemAbove4GB" reports the 64bit memory space (device address).
> >
> > Then if "Mem" reports memory range that across 4GB, it means the range above
> > 4GB should be reported through "MemAbove4GB".
> >
> Yes this is true, but some devices needs MMIO 32bit space only as per their BAR property, including E1000 EP.

I understand some devices contain only 32bit MMIO BAR so only 32bit memory space (device address) can be assigned to them.
Can you tell me the value of Mem/MemAbove4GB/Pmem/PmemAbove4GB in your real case?
Can you also tell me the PCI(e) device BAR information you want to initialize through the EDKII PCI stack?

Re: [edk2-devel] [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem Limit Checks

[Wasim Khan]

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Thursday, April 23, 2020 8:47 PM
> To: Wasim Khan <wasim.khan@nxp.com>; devel@edk2.groups.io
> Cc: ard.biesheuvel@linaro.org; Varun Sethi <V.Sethi@nxp.com>; Wu, Hao A
> <hao.a.wu@intel.com>
> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/PciHostBridge: Update Mem
> and PMem Limit Checks
> 
> 
> 
> > -----Original Message-----
> > From: Wasim Khan <wasim.khan@nxp.com>
> > Sent: Thursday, April 23, 2020 10:54 PM
> > To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> > Cc: ard.biesheuvel@linaro.org; Varun Sethi <V.Sethi@nxp.com>; Wu, Hao
> > A <hao.a.wu@intel.com>
> > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/PciHostBridge: Update
> > Mem and PMem Limit Checks
> > > > >
> > > > > Thanks for fixing the check.
> > > > >
> > > > > PCI_ROOT_BRIDGE_APERTURE.Base/Limit are device address while the
> > > > > memory space in GCD belongs to host domain.
> > > > > So, host address for Mem/Pmem should be below 4GB while device
> > > > > address can across 4GB.
> > > > >
> > > >
> > > >
> > > > Hi Ray,
> > > > Thank you for the review.
> > > > There are cases when we don't have PCIe host address below 4GB,
> > > > and the PCIe HOST Address space is only available above 4GB.
> > > > For Example, Mem: 40000000 - FFFFFFFF Translation=FFFFFF6000000000
> > > > will result in HOST Address = 0xA0FFFFFFFF . This is a valid use
> > > > case, but below
> > > check will report ASSERT for this HOST ADDRESS.
> > >
> > > OK. Now I remember that "Mem" reports the 32bit memory space (device
> > > address) and "MemAbove4GB" reports the 64bit memory space (device
> address).
> > >
> > > Then if "Mem" reports memory range that across 4GB, it means the
> > > range above 4GB should be reported through "MemAbove4GB".
> > >
> > Yes this is true, but some devices needs MMIO 32bit space only as per their
> BAR property, including E1000 EP.
> 
> I understand some devices contain only 32bit MMIO BAR so only 32bit memory
> space (device address) can be assigned to them.
> Can you tell me the value of Mem/MemAbove4GB/Pmem/PmemAbove4GB in
> your real case?
> Can you also tell me the PCI(e) device BAR information you want to initialize
> through the EDKII PCI stack?

So as mentioned that in our case, we do not have Host region below 4GB. So we use MMIO + Translation.
For a PCIe device which needs 32 Bit, Non-prefetchable memory , below are the two experiments I could run.

With MMIO + translation , and a MMIO64 , for a device : (Works fine)
 
Mem: 40000000 - FFFFFFFF Translation=FFFFFF7000000000
 MemAbove4G: 9100000000 - 91FFFFFFFF Translation=0
 PMem: FFFFFFFFFFFFFFFF - 0 Translation=0
 PMemAbove4G: FFFFFFFFFFFFFFFF - 0 Translation=0

PciHostBridge: SubmitResources for PcieRoot(0x2)
Mem: Granularity/SpecificFlag = 32 / 00
      Length/Alignment = 0x6100000 / 0x3FFFFFF

PciHostBridge: NotifyPhase (AllocateResources)
 RootBridge: PcieRoot(0x2)
  Mem: Base/Length/Alignment = 9040000000/6100000/3FFFFFF - Success

EP's BAR0 : 0x46080000


With Only MMIO64: (Does not work)
 Mem: FFFFFFFFFFFFFFFF - 0 Translation=0
 MemAbove4G: 9100000000 - 91FFFFFFFF Translation=0
 PMem: FFFFFFFFFFFFFFFF - 0 Translation=0
 PMemAbove4G: FFFFFFFFFFFFFFFF - 0 Translation=0

PciHostBridge: SubmitResources for PcieRoot(0x2)
Mem: Granularity/SpecificFlag = 32 / 00
      Length/Alignment = 0x6100000 / 0x3FFFFFF

PciHostBridge: NotifyPhase (AllocateResources)
 RootBridge: PcieRoot(0x2)
  Mem: Base/Length/Alignment = FFFFFFFFFFFFFFFF/6100000/3FFFFFF - Out Of Resource!

EP's BAR0: 0xFFFE0000

Re: [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem Limit Checks

[Ard Biesheuvel]

On Thu, 23 Apr 2020 at 12:43, Wasim Khan <wasim.khan@nxp.com> wrote:
>
> With Address Translation Support, it is possible and
> also correct that Mem and Pmem Limit cross the 4GB boundary.
> Update the checks so that Mem/PMem Limit should not cross 4GB
> from the Mem/PMem Base address.
>
> Signed-off-by: Wasim Khan <wasim.khan@nxp.com>
> ---
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index d304fae..9cf7e98 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -117,8 +117,8 @@ CreateRootBridge (
>    // Make sure Mem and MemAbove4G apertures are valid
>    //
>    if (RESOURCE_VALID (&Bridge->Mem)) {
> -    ASSERT (Bridge->Mem.Limit < SIZE_4GB);
> -    if (Bridge->Mem.Limit >= SIZE_4GB) {
> +    ASSERT (Bridge->Mem.Limit < (Bridge->Mem.Base + SIZE_4GB));
> +    if (Bridge->Mem.Limit >= (Bridge->Mem.Base + SIZE_4GB)) {
>        return NULL;
>      }
>    }
> @@ -129,8 +129,8 @@ CreateRootBridge (
>      }
>    }
>    if (RESOURCE_VALID (&Bridge->PMem)) {
> -    ASSERT (Bridge->PMem.Limit < SIZE_4GB);
> -    if (Bridge->PMem.Limit >= SIZE_4GB) {
> +    ASSERT (Bridge->PMem.Limit < (Bridge->PMem.Base + SIZE_4GB));
> +    if (Bridge->PMem.Limit >= (Bridge->PMem.Base + SIZE_4GB)) {
>        return NULL;
>      }
>    }
> --
> 2.7.4
>

This is not the right fix.

The translation offset should be taken into account for these checks

Re: [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem Limit Checks

[Wasim Khan]

> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Sent: Friday, April 24, 2020 12:27 AM
> To: Wasim Khan <wasim.khan@nxp.com>
> Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Varun Sethi
> <V.Sethi@nxp.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> <ray.ni@intel.com>
> Subject: Re: [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem
> Limit Checks
> 
> On Thu, 23 Apr 2020 at 12:43, Wasim Khan <wasim.khan@nxp.com> wrote:
> >
> > With Address Translation Support, it is possible and also correct that
> > Mem and Pmem Limit cross the 4GB boundary.
> > Update the checks so that Mem/PMem Limit should not cross 4GB from the
> > Mem/PMem Base address.
> >
> > Signed-off-by: Wasim Khan <wasim.khan@nxp.com>
> > ---
> >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> > b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> > index d304fae..9cf7e98 100644
> > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> > @@ -117,8 +117,8 @@ CreateRootBridge (
> >    // Make sure Mem and MemAbove4G apertures are valid
> >    //
> >    if (RESOURCE_VALID (&Bridge->Mem)) {
> > -    ASSERT (Bridge->Mem.Limit < SIZE_4GB);
> > -    if (Bridge->Mem.Limit >= SIZE_4GB) {
> > +    ASSERT (Bridge->Mem.Limit < (Bridge->Mem.Base + SIZE_4GB));
> > +    if (Bridge->Mem.Limit >= (Bridge->Mem.Base + SIZE_4GB)) {
> >        return NULL;
> >      }
> >    }
> > @@ -129,8 +129,8 @@ CreateRootBridge (
> >      }
> >    }
> >    if (RESOURCE_VALID (&Bridge->PMem)) {
> > -    ASSERT (Bridge->PMem.Limit < SIZE_4GB);
> > -    if (Bridge->PMem.Limit >= SIZE_4GB) {
> > +    ASSERT (Bridge->PMem.Limit < (Bridge->PMem.Base + SIZE_4GB));
> > +    if (Bridge->PMem.Limit >= (Bridge->PMem.Base + SIZE_4GB)) {
> >        return NULL;
> >      }
> >    }
> > --
> > 2.7.4
> >
> 
> This is not the right fix.
> 
> The translation offset should be taken into account for these checks

Thanks for the review Ard. 
device address = host address + translation offset.
Mem and Pmem represents "device address" , so that are already taking translation offset into account.

Re: [edk2-devel] [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem Limit Checks

[Ard Biesheuvel]

On 4/24/20 6:35 AM, Wasim Khan via groups.io wrote:
> 
> 
>> -----Original Message-----
>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Sent: Friday, April 24, 2020 12:27 AM
>> To: Wasim Khan <wasim.khan@nxp.com>
>> Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Varun Sethi
>> <V.Sethi@nxp.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
>> <ray.ni@intel.com>
>> Subject: Re: [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem
>> Limit Checks
>>
>> On Thu, 23 Apr 2020 at 12:43, Wasim Khan <wasim.khan@nxp.com> wrote:
>>>
>>> With Address Translation Support, it is possible and also correct that
>>> Mem and Pmem Limit cross the 4GB boundary.
>>> Update the checks so that Mem/PMem Limit should not cross 4GB from the
>>> Mem/PMem Base address.
>>>
>>> Signed-off-by: Wasim Khan <wasim.khan@nxp.com>
>>> ---
>>>   MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> index d304fae..9cf7e98 100644
>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> @@ -117,8 +117,8 @@ CreateRootBridge (
>>>     // Make sure Mem and MemAbove4G apertures are valid
>>>     //
>>>     if (RESOURCE_VALID (&Bridge->Mem)) {
>>> -    ASSERT (Bridge->Mem.Limit < SIZE_4GB);
>>> -    if (Bridge->Mem.Limit >= SIZE_4GB) {
>>> +    ASSERT (Bridge->Mem.Limit < (Bridge->Mem.Base + SIZE_4GB));
>>> +    if (Bridge->Mem.Limit >= (Bridge->Mem.Base + SIZE_4GB)) {
>>>         return NULL;
>>>       }
>>>     }
>>> @@ -129,8 +129,8 @@ CreateRootBridge (
>>>       }
>>>     }
>>>     if (RESOURCE_VALID (&Bridge->PMem)) {
>>> -    ASSERT (Bridge->PMem.Limit < SIZE_4GB);
>>> -    if (Bridge->PMem.Limit >= SIZE_4GB) {
>>> +    ASSERT (Bridge->PMem.Limit < (Bridge->PMem.Base + SIZE_4GB));
>>> +    if (Bridge->PMem.Limit >= (Bridge->PMem.Base + SIZE_4GB)) {
>>>         return NULL;
>>>       }
>>>     }
>>> --
>>> 2.7.4
>>>
>>
>> This is not the right fix.
>>
>> The translation offset should be taken into account for these checks
> 
> Thanks for the review Ard.
> device address = host address + translation offset.
> Mem and Pmem represents "device address" , so that are already taking translation offset into account.
> 

OK, apparently I am missing something.

For the MMIO32 window, the limit has to be < 4 GB, since the whole 
region needs to be 32-bit addressable. Otherwise, how are you going to 
allocate a 32-bit BAR from the part of the window that is > 4 GB ?

Re: [edk2-devel] [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem Limit Checks

[Wasim Khan]

> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Sent: Friday, April 24, 2020 11:38 AM
> To: devel@edk2.groups.io; Wasim Khan <wasim.khan@nxp.com>; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Varun Sethi <V.Sethi@nxp.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PciHostBridge: Update Mem
> and PMem Limit Checks
> 
> On 4/24/20 6:35 AM, Wasim Khan via groups.io wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> Sent: Friday, April 24, 2020 12:27 AM
> >> To: Wasim Khan <wasim.khan@nxp.com>
> >> Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Varun Sethi
> >> <V.Sethi@nxp.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> >> <ray.ni@intel.com>
> >> Subject: Re: [PATCH] MdeModulePkg/PciHostBridge: Update Mem and PMem
> >> Limit Checks
> >>
> >> On Thu, 23 Apr 2020 at 12:43, Wasim Khan <wasim.khan@nxp.com> wrote:
> >>>
> >>> With Address Translation Support, it is possible and also correct
> >>> that Mem and Pmem Limit cross the 4GB boundary.
> >>> Update the checks so that Mem/PMem Limit should not cross 4GB from
> >>> the Mem/PMem Base address.
> >>>
> >>> Signed-off-by: Wasim Khan <wasim.khan@nxp.com>
> >>> ---
> >>>   MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 8 ++++----
> >>>   1 file changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> >>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> >>> index d304fae..9cf7e98 100644
> >>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> >>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> >>> @@ -117,8 +117,8 @@ CreateRootBridge (
> >>>     // Make sure Mem and MemAbove4G apertures are valid
> >>>     //
> >>>     if (RESOURCE_VALID (&Bridge->Mem)) {
> >>> -    ASSERT (Bridge->Mem.Limit < SIZE_4GB);
> >>> -    if (Bridge->Mem.Limit >= SIZE_4GB) {
> >>> +    ASSERT (Bridge->Mem.Limit < (Bridge->Mem.Base + SIZE_4GB));
> >>> +    if (Bridge->Mem.Limit >= (Bridge->Mem.Base + SIZE_4GB)) {
> >>>         return NULL;
> >>>       }
> >>>     }
> >>> @@ -129,8 +129,8 @@ CreateRootBridge (
> >>>       }
> >>>     }
> >>>     if (RESOURCE_VALID (&Bridge->PMem)) {
> >>> -    ASSERT (Bridge->PMem.Limit < SIZE_4GB);
> >>> -    if (Bridge->PMem.Limit >= SIZE_4GB) {
> >>> +    ASSERT (Bridge->PMem.Limit < (Bridge->PMem.Base + SIZE_4GB));
> >>> +    if (Bridge->PMem.Limit >= (Bridge->PMem.Base + SIZE_4GB)) {
> >>>         return NULL;
> >>>       }
> >>>     }
> >>> --
> >>> 2.7.4
> >>>
> >>
> >> This is not the right fix.
> >>
> >> The translation offset should be taken into account for these checks
> >
> > Thanks for the review Ard.
> > device address = host address + translation offset.
> > Mem and Pmem represents "device address" , so that are already taking
> translation offset into account.
> >
> 
> OK, apparently I am missing something.
> 
> For the MMIO32 window, the limit has to be < 4 GB, since the whole region
> needs to be 32-bit addressable. Otherwise, how are you going to allocate a 32-
> bit BAR from the part of the window that is > 4 GB ?
> 

OK, it is correct that we can not allocate 32-bit BAR from the part of window that is > 4GB.
Thanks for catching it.
Thanks Ard, Ray for the good discussion.