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

["Wasim Khan" <wasim.khan@nxp.com>]

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