Re: [PATCH V2 2/2] IntelSiliconPkg/VtdPeiSample: Add premem support.

["Yao, Jiewen" <jiewen.yao@intel.com>]

Good feedback. I fixed all these in V3.

Why the wrong size does not cause problem?
The reason is that the garbage data is skipped in the while loop of DRHD or RMRR structure.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, October 27, 2017 4:47 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH V2 2/2] IntelSiliconPkg/VtdPeiSample: Add premem
> support.
> 
> Another comment.
> 
> sizeof(MY_VTD_INFO_PPI) is used in mPlatformVTdNoIgdSample, that seems
> wrong and should be sizeof(MY_VTD_INFO_NO_IGD_PPI), right?
> Why it does not cause problem with the code.
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, October 27, 2017 2:56 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH V2 2/2] IntelSiliconPkg/VtdPeiSample: Add premem
> support.
> 
> Could you update the function comments of InitDmar() and
> SiliconInitializedPpiNotifyCallback()?
> There are two lines of " DEBUG ((DEBUG_INFO, "MchBar - %x\n", MchBar)) " in
> InitDmar(), are they added on purpose?
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jiewen
> Yao
> Sent: Friday, October 27, 2017 1:41 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>
> Subject: [edk2] [PATCH V2 2/2] IntelSiliconPkg/VtdPeiSample: Add premem
> support.
> 
> Before memory is ready, this sample produces one VTd engine.
> After memory and silicon is initialized, this sample produces both IGD VTd engine
> and all-rest VTd engine by reinstall the FV_INFO_PPI.
> 
> This update is to demonstrate how to support pre-mem VTd usage.
> 
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> ---
> 
> IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSample
> Pei.c   | 234 +++++++++++++++++---
> 
> IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSample
> Pei.inf |   2 +-
>  2 files changed, 202 insertions(+), 34 deletions(-)
> 
> diff --git
> a/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSam
> plePei.c
> b/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSam
> plePei.c
> index 6267da7..921daef 100644
> ---
> a/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSam
> plePei.c
> +++ b/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdIn
> +++ foSamplePei.c
> @@ -20,6 +20,7 @@
>  #include <Library/DebugLib.h>
>  #include <Library/PciLib.h>
>  #include <Library/IoLib.h>
> +#include <Library/MemoryAllocationLib.h>
> 
>  #define R_SA_MCHBAR               (0x48)
>  #define R_SA_GGC                  (0x50)
> @@ -33,6 +34,8 @@
>  #define R_SA_MCHBAR_VTD1_OFFSET  0x5400  ///< HW UNIT for IGD
> #define R_SA_MCHBAR_VTD2_OFFSET  0x5410  ///< HW UNIT for all other -
> PEG, USB, SATA etc
> 
> +EFI_GUID gEdkiiSiliconInitializedPpiGuid = {0x82a72dc8, 0x61ec, 0x403e,
> +{0xb1, 0x5a, 0x8d, 0x7a, 0x3a, 0x71, 0x84, 0x98}};
> +
>  typedef struct {
>    EFI_ACPI_DMAR_HEADER                         DmarHeader;
>    //
> @@ -131,50 +134,190 @@ EFI_PEI_PPI_DESCRIPTOR
> mPlatformVTdInfoSampleDesc = {
>    &mPlatformVTdSample
>  };
> 
> +typedef struct {
> +  EFI_ACPI_DMAR_HEADER                         DmarHeader;
> +  //
> +  // VTd engine 2 - all rest
> +  //
> +  EFI_ACPI_DMAR_DRHD_HEADER                    Drhd2;
> +} MY_VTD_INFO_NO_IGD_PPI;
> +
> +MY_VTD_INFO_NO_IGD_PPI  mPlatformVTdNoIgdSample = {
> +  { // DmarHeader
> +    { // Header
> +      EFI_ACPI_4_0_DMA_REMAPPING_TABLE_SIGNATURE,
> +      sizeof(MY_VTD_INFO_PPI),
> +      EFI_ACPI_DMAR_REVISION,
> +    },
> +    0x26, // HostAddressWidth
> +  },
> +
> +  { // Drhd2
> +    { // Header
> +      EFI_ACPI_DMAR_TYPE_DRHD,
> +      sizeof(EFI_ACPI_DMAR_DRHD_HEADER)
> +    },
> +    EFI_ACPI_DMAR_DRHD_FLAGS_INCLUDE_PCI_ALL, // Flags
> +    0, // Reserved
> +    0, // SegmentNumber
> +    0xFED91000 // RegisterBaseAddress -- TO BE PATCHED
> +  },
> +};
> +
> +EFI_PEI_PPI_DESCRIPTOR mPlatformVTdNoIgdInfoSampleDesc = {
> +  (EFI_PEI_PPI_DESCRIPTOR_PPI |
> EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
> +  &gEdkiiVTdInfoPpiGuid,
> +  &mPlatformVTdNoIgdSample
> +};
> +
>  /**
>    Patch Graphic UMA address in RMRR and base address.
>  **/
>  VOID
> -PatchDmar (
> +InitDmar (
>    VOID
>    )
>  {
>    UINT32              MchBar;
> -  UINT16              IgdMode;
> -  UINT16              GttMode;
> -  UINT32              IgdMemSize;
> -  UINT32              GttMemSize;
> -
> -  ///
> -  /// Calculate IGD memsize
> -  ///
> -  IgdMode = ((PciRead16 (PCI_LIB_ADDRESS(0, 0, 0, R_SA_GGC)) &
> B_SKL_SA_GGC_GMS_MASK) >> N_SKL_SA_GGC_GMS_OFFSET) & 0xFF;
> -  if (IgdMode < 0xF0) {
> -    IgdMemSize = IgdMode * 32 * (1024) * (1024);
> +
> +  DEBUG ((DEBUG_INFO, "InitDmar\n"));
> +
> +  MchBar = PciRead32 (PCI_LIB_ADDRESS(0, 0, 0, R_SA_MCHBAR)) & ~BIT0;
> + DEBUG ((DEBUG_INFO, "MchBar - %x\n", MchBar));
> +
> +  PciWrite32 (PCI_LIB_ADDRESS(0, 0, 0, R_SA_MCHBAR), 0xFED10000 |
> + BIT0);  DEBUG ((DEBUG_INFO, "MchBar - %x\n", MchBar));
> +
> +  DEBUG ((DEBUG_INFO, "VTd2 - %x\n", (MmioRead32 (MchBar +
> +R_SA_MCHBAR_VTD2_OFFSET) &~1)));
> +  MmioWrite32 (MchBar + R_SA_MCHBAR_VTD2_OFFSET,
> +(UINT32)mPlatformVTdSample.Drhd2.RegisterBaseAddress | 1);
> +  DEBUG ((DEBUG_INFO, "VTd2 - %x\n", (MmioRead32 (MchBar +
> +R_SA_MCHBAR_VTD2_OFFSET) &~1))); }
> +
> +/**
> +  Patch Graphic UMA address in RMRR and base address.
> +**/
> +EFI_PEI_PPI_DESCRIPTOR *
> +PatchDmar (
> +  VOID
> +  )
> +{
> +  UINT32                  MchBar;
> +  UINT16                  IgdMode;
> +  UINT16                  GttMode;
> +  UINT32                  IgdMemSize;
> +  UINT32                  GttMemSize;
> +  MY_VTD_INFO_PPI         *PlatformVTdSample;
> +  EFI_PEI_PPI_DESCRIPTOR  *PlatformVTdInfoSampleDesc;
> +  MY_VTD_INFO_NO_IGD_PPI  *PlatformVTdNoIgdSample;
> +  EFI_PEI_PPI_DESCRIPTOR  *PlatformVTdNoIgdInfoSampleDesc;
> +
> +  DEBUG ((DEBUG_INFO, "PatchDmar\n"));
> +
> +  if (PciRead16 (PCI_LIB_ADDRESS(0, 2, 0, 0)) != 0xFFFF) {
> +    PlatformVTdSample = AllocateCopyPool (sizeof(MY_VTD_INFO_PPI),
> &mPlatformVTdSample);
> +    ASSERT(PlatformVTdSample != NULL);
> +    PlatformVTdInfoSampleDesc = AllocateCopyPool
> (sizeof(EFI_PEI_PPI_DESCRIPTOR), &mPlatformVTdInfoSampleDesc);
> +    ASSERT(PlatformVTdInfoSampleDesc != NULL);
> +    PlatformVTdInfoSampleDesc->Ppi = PlatformVTdSample;
> +
> +    ///
> +    /// Calculate IGD memsize
> +    ///
> +    IgdMode = ((PciRead16 (PCI_LIB_ADDRESS(0, 0, 0, R_SA_GGC)) &
> B_SKL_SA_GGC_GMS_MASK) >> N_SKL_SA_GGC_GMS_OFFSET) & 0xFF;
> +    if (IgdMode < 0xF0) {
> +      IgdMemSize = IgdMode * 32 * (1024) * (1024);
> +    } else {
> +      IgdMemSize = 4 * (IgdMode - 0xF0 + 1) * (1024) * (1024);
> +    }
> +
> +    ///
> +    /// Calculate GTT mem size
> +    ///
> +    GttMemSize = 0;
> +    GttMode = (PciRead16 (PCI_LIB_ADDRESS(0, 0, 0, R_SA_GGC)) &
> B_SKL_SA_GGC_GGMS_MASK) >> N_SKL_SA_GGC_GGMS_OFFSET;
> +    if (GttMode <= V_SKL_SA_GGC_GGMS_8MB) {
> +      GttMemSize = (1 << GttMode) * (1024) * (1024);
> +    }
> +
> +    PlatformVTdSample->Rmrr1.ReservedMemoryRegionBaseAddress  =
> (PciRead32 (PCI_LIB_ADDRESS(0, 0, 0, R_SA_TOLUD)) & ~(0x01)) - IgdMemSize -
> GttMemSize;
> +    PlatformVTdSample->Rmrr1.ReservedMemoryRegionLimitAddress =
> + PlatformVTdSample->Rmrr1.ReservedMemoryRegionBaseAddress +
> IgdMemSize
> + + GttMemSize - 1;
> +
> +    ///
> +    /// Update DRHD structures of DmarTable
> +    ///
> +    MchBar = PciRead32 (PCI_LIB_ADDRESS(0, 0, 0, R_SA_MCHBAR)) & ~BIT0;
> +
> +    DEBUG ((DEBUG_INFO, "VTd1 - %x\n", (MmioRead32 (MchBar +
> R_SA_MCHBAR_VTD1_OFFSET))));
> +    if ((MmioRead32 (MchBar + R_SA_MCHBAR_VTD1_OFFSET) &~1) != 0) {
> +      PlatformVTdSample->Drhd1.RegisterBaseAddress = (MmioRead32
> (MchBar + R_SA_MCHBAR_VTD1_OFFSET) &~1);
> +    } else {
> +      MmioWrite32 (MchBar + R_SA_MCHBAR_VTD1_OFFSET,
> (UINT32)PlatformVTdSample->Drhd1.RegisterBaseAddress | 1);
> +    }
> +
> +    DEBUG ((DEBUG_INFO, "VTd2 - %x\n", (MmioRead32 (MchBar +
> R_SA_MCHBAR_VTD2_OFFSET))));
> +    if ((MmioRead32 (MchBar + R_SA_MCHBAR_VTD2_OFFSET) &~1) != 0) {
> +      PlatformVTdSample->Drhd2.RegisterBaseAddress = (MmioRead32
> (MchBar + R_SA_MCHBAR_VTD2_OFFSET) &~1);
> +    } else {
> +      MmioWrite32 (MchBar + R_SA_MCHBAR_VTD2_OFFSET,
> (UINT32)PlatformVTdSample->Drhd2.RegisterBaseAddress | 1);
> +    }
> +
> +    return PlatformVTdInfoSampleDesc;
>    } else {
> -    IgdMemSize = 4 * (IgdMode - 0xF0 + 1) * (1024) * (1024);
> -  }
> +    PlatformVTdNoIgdSample = AllocateCopyPool
> (sizeof(MY_VTD_INFO_NO_IGD_PPI), &mPlatformVTdNoIgdSample);
> +    ASSERT(PlatformVTdNoIgdSample != NULL);
> +    PlatformVTdNoIgdInfoSampleDesc = AllocateCopyPool
> (sizeof(EFI_PEI_PPI_DESCRIPTOR), &mPlatformVTdNoIgdInfoSampleDesc);
> +    ASSERT(PlatformVTdNoIgdInfoSampleDesc != NULL);
> +    PlatformVTdNoIgdInfoSampleDesc->Ppi = PlatformVTdNoIgdSample;
> +
> +    ///
> +    /// Update DRHD structures of DmarTable
> +    ///
> +    MchBar = PciRead32 (PCI_LIB_ADDRESS(0, 0, 0, R_SA_MCHBAR)) & ~BIT0;
> 
> -  ///
> -  /// Calculate GTT mem size
> -  ///
> -  GttMemSize = 0;
> -  GttMode = (PciRead16 (PCI_LIB_ADDRESS(0, 0, 0, R_SA_GGC)) &
> B_SKL_SA_GGC_GGMS_MASK) >> N_SKL_SA_GGC_GGMS_OFFSET;
> -  if (GttMode <= V_SKL_SA_GGC_GGMS_8MB) {
> -    GttMemSize = (1 << GttMode) * (1024) * (1024);
> +    DEBUG ((DEBUG_INFO, "VTd2 - %x\n", (MmioRead32 (MchBar +
> R_SA_MCHBAR_VTD2_OFFSET))));
> +    if ((MmioRead32 (MchBar + R_SA_MCHBAR_VTD2_OFFSET) &~1) != 0) {
> +      PlatformVTdNoIgdSample->Drhd2.RegisterBaseAddress = (MmioRead32
> (MchBar + R_SA_MCHBAR_VTD2_OFFSET) &~1);
> +    } else {
> +      MmioWrite32 (MchBar + R_SA_MCHBAR_VTD2_OFFSET,
> (UINT32)PlatformVTdNoIgdSample->Drhd2.RegisterBaseAddress | 1);
> +    }
> +
> +    return PlatformVTdNoIgdInfoSampleDesc;
>    }
> +}
> 
> -  mPlatformVTdSample.Rmrr1.ReservedMemoryRegionBaseAddress  =
> (PciRead32 (PCI_LIB_ADDRESS(0, 0, 0, R_SA_TOLUD)) & ~(0x01)) - IgdMemSize -
> GttMemSize;
> -  mPlatformVTdSample.Rmrr1.ReservedMemoryRegionLimitAddress =
> mPlatformVTdSample.Rmrr1.ReservedMemoryRegionBaseAddress +
> IgdMemSize + GttMemSize - 1;
> +/**
> +  Install Firmware Volume Hob's once there is main memory
> 
> -  ///
> -  /// Update DRHD structures of DmarTable
> -  ///
> -  MchBar = PciRead32 (PCI_LIB_ADDRESS(0, 0, 0, R_SA_MCHBAR)) & ~BIT0;
> -  mPlatformVTdSample.Drhd1.RegisterBaseAddress = (MmioRead32 (MchBar +
> R_SA_MCHBAR_VTD1_OFFSET) &~1);
> -  mPlatformVTdSample.Drhd2.RegisterBaseAddress = (MmioRead32 (MchBar +
> R_SA_MCHBAR_VTD2_OFFSET) &~1);
> +  @param[in]  PeiServices       General purpose services available to every
> PEIM.
> +  @param[in]  NotifyDescriptor  Notify that this module published.
> +  @param[in]  Ppi               PPI that was installed.
> +
> +  @retval     EFI_SUCCESS       The function completed successfully.
> +**/
> +EFI_STATUS
> +EFIAPI
> +SiliconInitializedPpiNotifyCallback (
> +  IN CONST EFI_PEI_SERVICES     **PeiServices,
> +  IN EFI_PEI_NOTIFY_DESCRIPTOR  *NotifyDescriptor,
> +  IN VOID                       *Ppi
> +  )
> +{
> +  EFI_STATUS               Status;
> +  EFI_PEI_PPI_DESCRIPTOR   *PpiDesc;
> +
> +  PpiDesc = PatchDmar ();
> +
> +  Status = PeiServicesReInstallPpi (&mPlatformVTdNoIgdInfoSampleDesc,
> + PpiDesc);  ASSERT_EFI_ERROR (Status);  return EFI_SUCCESS;
>  }
> 
> +EFI_PEI_NOTIFY_DESCRIPTOR mSiliconInitializedNotifyList = {
> +  (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_CALLBACK |
> +EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
> +  &gEdkiiSiliconInitializedPpiGuid,
> +  (EFI_PEIM_NOTIFY_ENTRY_POINT) SiliconInitializedPpiNotifyCallback
> +};
> +
>  /**
>    Platform VTd Info sample driver.
> 
> @@ -190,12 +333,37 @@ PlatformVTdInfoSampleInitialize (
>    IN CONST EFI_PEI_SERVICES     **PeiServices
>    )
>  {
> -  EFI_STATUS  Status;
> +  EFI_STATUS               Status;
> +  BOOLEAN                  SiliconInitialized;
> +  VOID                     *SiliconInitializedPpi;
> +  EFI_PEI_PPI_DESCRIPTOR   *PpiDesc;
> +
> +  SiliconInitialized = FALSE;
> +  //
> +  // Check if silicon is initialized.
> +  //
> +  Status = PeiServicesLocatePpi (
> +             &gEdkiiSiliconInitializedPpiGuid,
> +             0,
> +             NULL,
> +             &SiliconInitializedPpi
> +             );
> +  if (!EFI_ERROR(Status)) {
> +    SiliconInitialized = TRUE;
> +  }
> +  DEBUG ((DEBUG_INFO, "SiliconInitialized - %x\n",
> + SiliconInitialized));  if (!SiliconInitialized) {
> +    Status = PeiServicesNotifyPpi (&mSiliconInitializedNotifyList);
> +    InitDmar ();
> 
> -  PatchDmar ();
> +    Status = PeiServicesInstallPpi (&mPlatformVTdNoIgdInfoSampleDesc);
> +    ASSERT_EFI_ERROR (Status);
> +  } else {
> +    PpiDesc = PatchDmar ();
> 
> -  Status = PeiServicesInstallPpi (&mPlatformVTdInfoSampleDesc);
> -  ASSERT_EFI_ERROR (Status);
> +    Status = PeiServicesInstallPpi (PpiDesc);
> +    ASSERT_EFI_ERROR (Status);
> +  }
> 
>    return Status;
>  }
> diff --git
> a/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSam
> plePei.inf
> b/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSam
> plePei.inf
> index 96adb70..a4ffe51 100644
> ---
> a/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSam
> plePei.inf
> +++ b/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdIn
> +++ foSamplePei.inf
> @@ -47,7 +47,7 @@
>    gEdkiiVTdInfoPpiGuid         ## PRODUCES
> 
>  [Depex]
> -  gEfiPeiMemoryDiscoveredPpiGuid
> +  gEfiPeiMasterBootModePpiGuid
> 
>  [UserExtensions.TianoCore."ExtraFiles"]
>    PlatformVTdInfoSamplePeiExtra.uni
> --
> 2.7.4.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel