Re: [PATCH v2 1/2] UefiCpuPkg/CpuDxe: Copy two functions from PciHostBridge

["Fan, Jeff" <jeff.fan@intel.com>]

Laszlo,

For 1), I will update this typo when pushing the serial of patches. Thanks!

For2), I will do clean up on EFI_D to DEBUG_ in CpuDxe driver after Jiewen & my patches pushed.

Thanks!
Jeff

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Tuesday, February 21, 2017 6:57 PM
To: Fan, Jeff; edk2-devel@ml01.01.org
Cc: Yao, Jiewen; Tian, Feng; Kinney, Michael D
Subject: Re: [PATCH v2 1/2] UefiCpuPkg/CpuDxe: Copy two functions from PciHostBridge

I have two suggestions for this patch:

On 02/21/17 04:08, Jeff Fan wrote:
> Copy AddMemoryMappedIoSpace() and AddMemoryMappedIoSpace() from 
> MdeModulePkg\Bus\Pci\PciHostBridgeDxe\PciHostBridge.c.

(1) One of the two "AddMemoryMappedIoSpace" strings should be replaced with "IntersectMemoryDescriptor".

This can be done when you commit / push the patch; no need to repost just because of it.

(2) This is more of a "followup" suggestion: since this is new code, I recommend that the EFI_D_ macros be replced with DEBUG_ macros.

However, I see value in keeping the first patch as a pristine copy from the PciHostBridgeDxe driver, so I don't recommend doing the above change in patch #1. I think it can be patch #3, or a completely separate patch (later).

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

Thanks!
Laszlo

> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=390
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan <jeff.fan@intel.com>
> ---
>  UefiCpuPkg/CpuDxe/CpuDxe.c | 148 
> +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 148 insertions(+)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c 
> index 9fb6d76..925c50f 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.c
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
> @@ -887,6 +887,154 @@ IdleLoopEventCallback (
>    CpuSleep ();
>  }
>  
> +/**
> +  Ensure the compatibility of a memory space descriptor with the MMIO aperture.
> +
> +  The memory space descriptor can come from the GCD memory space map, 
> + or it can  represent a gap between two neighboring memory space 
> + descriptors. In the  latter case, the GcdMemoryType field is 
> + expected to be  EfiGcdMemoryTypeNonExistent.
> +
> +  If the memory space descriptor already has type  
> + EfiGcdMemoryTypeMemoryMappedIo, and its capabilities are a superset 
> + of the  required capabilities, then no action is taken -- it is by 
> + definition  compatible with the aperture.
> +
> +  Otherwise, the intersection of the memory space descriptor is 
> + calculated with  the aperture. If the intersection is the empty set 
> + (no overlap), no action is  taken; the memory space descriptor is compatible with the aperture.
> +
> +  Otherwise, the type of the descriptor is investigated again. If the 
> + type is  EfiGcdMemoryTypeNonExistent (representing a gap, or a 
> + genuine descriptor with  such a type), then an attempt is made to 
> + add the intersection as MMIO space  to the GCD memory space map, 
> + with the specified capabilities. This ensures  continuity for the 
> + aperture, and the descriptor is deemed compatible with the  aperture.
> +
> +  Otherwise, the memory space descriptor is incompatible with the 
> + MMIO  aperture.
> +
> +  @param[in] Base         Base address of the aperture.
> +  @param[in] Length       Length of the aperture.
> +  @param[in] Capabilities Capabilities required by the aperture.
> +  @param[in] Descriptor   The descriptor to ensure compatibility with the
> +                          aperture for.
> +
> +  @retval EFI_SUCCESS            The descriptor is compatible. The GCD memory
> +                                 space map may have been updated, for
> +                                 continuity within the aperture.
> +  @retval EFI_INVALID_PARAMETER  The descriptor is incompatible.
> +  @return                        Error codes from gDS->AddMemorySpace().
> +**/
> +EFI_STATUS
> +IntersectMemoryDescriptor (
> +  IN  UINT64                                Base,
> +  IN  UINT64                                Length,
> +  IN  UINT64                                Capabilities,
> +  IN  CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Descriptor
> +  )
> +{
> +  UINT64                                    IntersectionBase;
> +  UINT64                                    IntersectionEnd;
> +  EFI_STATUS                                Status;
> +
> +  if (Descriptor->GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo &&
> +      (Descriptor->Capabilities & Capabilities) == Capabilities) {
> +    return EFI_SUCCESS;
> +  }
> +
> +  IntersectionBase = MAX (Base, Descriptor->BaseAddress);  
> + IntersectionEnd = MIN (Base + Length,
> +                      Descriptor->BaseAddress + Descriptor->Length);  
> + if (IntersectionBase >= IntersectionEnd) {
> +    //
> +    // The descriptor and the aperture don't overlap.
> +    //
> +    return EFI_SUCCESS;
> +  }
> +
> +  if (Descriptor->GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
> +    Status = gDS->AddMemorySpace (EfiGcdMemoryTypeMemoryMappedIo,
> +                    IntersectionBase, IntersectionEnd - IntersectionBase,
> +                    Capabilities);
> +
> +    DEBUG ((EFI_ERROR (Status) ? EFI_D_ERROR : EFI_D_VERBOSE,
> +      "%a: %a: add [%Lx, %Lx): %r\n", gEfiCallerBaseName, __FUNCTION__,
> +      IntersectionBase, IntersectionEnd, Status));
> +    return Status;
> +  }
> +
> +  DEBUG ((EFI_D_ERROR, "%a: %a: desc [%Lx, %Lx) type %u cap %Lx conflicts "
> +    "with aperture [%Lx, %Lx) cap %Lx\n", gEfiCallerBaseName, __FUNCTION__,
> +    Descriptor->BaseAddress, Descriptor->BaseAddress + Descriptor->Length,
> +    (UINT32)Descriptor->GcdMemoryType, Descriptor->Capabilities,
> +    Base, Base + Length, Capabilities));
> +  return EFI_INVALID_PARAMETER;
> +}
> +
> +/**
> +  Add MMIO space to GCD.
> +  The routine checks the GCD database and only adds those which are
> +  not added in the specified range to GCD.
> +
> +  @param Base         Base address of the MMIO space.
> +  @param Length       Length of the MMIO space.
> +  @param Capabilities Capabilities of the MMIO space.
> +
> +  @retval EFI_SUCCES The MMIO space was added successfully.
> +**/
> +EFI_STATUS
> +AddMemoryMappedIoSpace (
> +  IN  UINT64                            Base,
> +  IN  UINT64                            Length,
> +  IN  UINT64                            Capabilities
> +  )
> +{
> +  EFI_STATUS                            Status;
> +  UINTN                                 Index;
> +  UINTN                                 NumberOfDescriptors;
> +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR       *MemorySpaceMap;
> +
> +  Status = gDS->GetMemorySpaceMap (&NumberOfDescriptors, 
> + &MemorySpaceMap);  if (EFI_ERROR (Status)) {
> +    DEBUG ((EFI_D_ERROR, "%a: %a: GetMemorySpaceMap(): %r\n",
> +      gEfiCallerBaseName, __FUNCTION__, Status));
> +    return Status;
> +  }
> +
> +  for (Index = 0; Index < NumberOfDescriptors; Index++) {
> +    Status = IntersectMemoryDescriptor (Base, Length, Capabilities,
> +               &MemorySpaceMap[Index]);
> +    if (EFI_ERROR (Status)) {
> +      goto FreeMemorySpaceMap;
> +    }
> +  }
> +
> +  DEBUG_CODE (
> +    //
> +    // Make sure there are adjacent descriptors covering [Base, Base + Length).
> +    // It is possible that they have not been merged; merging can be prevented
> +    // by allocation and different capabilities.
> +    //
> +    UINT64                          CheckBase;
> +    EFI_STATUS                      CheckStatus;
> +    EFI_GCD_MEMORY_SPACE_DESCRIPTOR Descriptor;
> +
> +    for (CheckBase = Base;
> +         CheckBase < Base + Length;
> +         CheckBase = Descriptor.BaseAddress + Descriptor.Length) {
> +      CheckStatus = gDS->GetMemorySpaceDescriptor (CheckBase, &Descriptor);
> +      ASSERT_EFI_ERROR (CheckStatus);
> +      ASSERT (Descriptor.GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo);
> +      ASSERT ((Descriptor.Capabilities & Capabilities) == Capabilities);
> +    }
> +    );
> +
> +FreeMemorySpaceMap:
> +  FreePool (MemorySpaceMap);
> +
> +  return Status;
> +}
>  
>  /**
>    Initialize the state information for the CPU Architectural Protocol.
>