Re: [PATCH 1/2] IntelSiliconPkg/Vtd: Support CSM usage.

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

Good suggestion. It can make error message more readable.

> -----Original Message-----
> From: Zeng, Star
> Sent: Wednesday, September 6, 2017 10:14 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH 1/2] IntelSiliconPkg/Vtd: Support CSM usage.
> 
> How about we also enhance the debug message like below?
> 
>     if (HasError) {
>       DEBUG((DEBUG_INFO, "#### ERROR ####\n"));
>       DumpVtdRegs (Num);
>       DEBUG((DEBUG_INFO, "#### ERROR ####\n"));
>     }
> 
> ->
> 
>     if (HasError) {
>       DEBUG((DEBUG_INFO, "\n#### ERROR ####\n"));
>       DumpVtdRegs (Num);
>       DEBUG((DEBUG_INFO, "#### ERROR ####\n\n"));
>     }
> 
> Then the error message could be at a separated block for easy understanding.
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Yao, Jiewen
> Sent: Tuesday, September 5, 2017 11:30 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 1/2] IntelSiliconPkg/Vtd: Support CSM usage.
> 
> Remove zero address check in IoMmuMap.
> The reason is that a CSM legacy driver may use legacy memory for DMA.
> As such, the legacyBios need allow below 1M to the legacy device.
> 
> This patch also fixed some typo.
> 
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  IntelSiliconPkg/IntelVTdDxe/BmDma.c            | 8 ++++----
>  IntelSiliconPkg/IntelVTdDxe/TranslationTable.c | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/IntelSiliconPkg/IntelVTdDxe/BmDma.c
> b/IntelSiliconPkg/IntelVTdDxe/BmDma.c
> index 5dcee00..7a5f361 100644
> --- a/IntelSiliconPkg/IntelVTdDxe/BmDma.c
> +++ b/IntelSiliconPkg/IntelVTdDxe/BmDma.c
> @@ -77,14 +77,14 @@ IoMmuMap (
>    EFI_PHYSICAL_ADDRESS                              DmaMemoryTop;
>    BOOLEAN                                           NeedRemap;
> 
> -  DEBUG ((DEBUG_VERBOSE, "IoMmuMap: ==> 0x%08x - 0x%08x (%x)\n",
> HostAddress, NumberOfBytes, Operation));
> -
> -  if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress ==
> NULL ||
> +  if (NumberOfBytes == NULL || DeviceAddress == NULL ||
>        Mapping == NULL) {
>      DEBUG ((DEBUG_ERROR, "IoMmuMap: %r\n", EFI_INVALID_PARAMETER));
>      return EFI_INVALID_PARAMETER;
>    }
> 
> +  DEBUG ((DEBUG_VERBOSE, "IoMmuMap: ==> 0x%08x - 0x%08x (%x)\n",
> + HostAddress, *NumberOfBytes, Operation));
> +
>    //
>    // Make sure that Operation is valid
>    //
> @@ -135,7 +135,7 @@ IoMmuMap (
>      if (NeedRemap) {
>        //
>        // Common Buffer operations can not be remapped.  If the common
> buffer
> -      // if above 4GB, then it is not possible to generate a mapping, so return
> +      // is above 4GB, then it is not possible to generate a mapping,
> + so return
>        // an error.
>        //
>        DEBUG ((DEBUG_ERROR, "IoMmuMap: %r\n", EFI_UNSUPPORTED)); diff
> --git a/IntelSiliconPkg/IntelVTdDxe/TranslationTable.c
> b/IntelSiliconPkg/IntelVTdDxe/TranslationTable.c
> index cd3111c..ccecc95 100644
> --- a/IntelSiliconPkg/IntelVTdDxe/TranslationTable.c
> +++ b/IntelSiliconPkg/IntelVTdDxe/TranslationTable.c
> @@ -891,7 +891,7 @@ SetAccessAttribute (
> 
>    SecondLevelPagingEntry = NULL;
> 
> -  DEBUG ((DEBUG_INFO,"SetAccessAttribute (S%04x B%02x D%02x F%02x)
> (0x%016lx - 0x%08x, %x)\n", Segment, SourceId.Bits.Bus, SourceId.Bits.Device,
> SourceId.Bits.Function, BaseAddress, (UINTN)Length, IoMmuAccess));
> +  DEBUG ((DEBUG_VERBOSE,"SetAccessAttribute (S%04x B%02x D%02x F%02x)
> + (0x%016lx - 0x%08x, %x)\n", Segment, SourceId.Bits.Bus,
> + SourceId.Bits.Device, SourceId.Bits.Function, BaseAddress,
> + (UINTN)Length, IoMmuAccess));
> 
>    VtdIndex = FindVtdIndexByPciDevice (Segment, SourceId, &ExtContextEntry,
> &ContextEntry);
>    if (VtdIndex == (UINTN)-1) {
> --
> 2.7.4.windows.1