Re: [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access

[Laszlo Ersek <lersek@redhat.com>]

On 09/21/18 09:25, Ruiyu Ni wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1196
> 
> RootBridgeIoCheckParameter() verifies that the requested MMIO access
> can fit in any of the MEM/PMEM 32/64 ranges. But today's logic
> somehow only checks the requested access against MEM 32/64 ranges.
> 
> It should also check the requested access against PMEM 32/64 ranges.
> 
> The patch fixes this issue.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Kirkendall, Garrett <Garrett.Kirkendall@amd.com>
> ---
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index 0b6b56f846..f6234b5d11 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -411,12 +411,18 @@ RootBridgeIoCheckParameter (
>      // By comparing the Address against Limit we know which range to be used
>      // for checking
>      //
> -    if (Address + Length <= RootBridge->Mem.Limit + 1) {
> -      Base = RootBridge->Mem.Base;
> +    if ((Address >= RootBridge->Mem.Base) && (Address + Length <= RootBridge->Mem.Limit + 1)) {
> +      Base  = RootBridge->Mem.Base;
>        Limit = RootBridge->Mem.Limit;
> -    } else {
> -      Base = RootBridge->MemAbove4G.Base;
> +    } else if ((Address >= RootBridge->PMem.Base) && (Address + Length <= RootBridge->PMem.Limit + 1)) {
> +      Base  = RootBridge->PMem.Base;
> +      Limit = RootBridge->PMem.Limit;
> +    } else if ((Address >= RootBridge->MemAbove4G.Base) && (Address + Length <= RootBridge->MemAbove4G.Limit + 1)) {
> +      Base  = RootBridge->MemAbove4G.Base;
>        Limit = RootBridge->MemAbove4G.Limit;
> +    } else {
> +      Base  = RootBridge->PMemAbove4G.Base;
> +      Limit = RootBridge->PMemAbove4G.Limit;
>      }
>    } else {
>      PciRbAddr = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS*) &Address;
> 

The interesting thing about this patch is that, if any one of the first
three branches is taken, then the final checks will automatically pass.
That's because, on the first three branches, we select the base & the
limit *because* the access falls between them. Therefore, in the end,
when we check whether the access falls between base and end, they
miraculously happen to do so. :)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo