1. Patchew
  2. EDK2
  3. [edk2] [RFC PATCH v1 0/5] x86: Secure Encrypted Virtualization (AMD)
  4. View patch

[edk2] [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package

Brijesh Singh posted 5 patches 7 years, 8 months ago
There is a newer version of this series
[edk2] [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
Posted by Brijesh Singh 7 years, 8 months ago 
Imports IoLib into OvmfPkg to make the changes to support SEV guest.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 .../BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf      |    0 
 .../BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni      |    0 
 .../BaseIoLibIntrinsicInternal.h                   |    0 
 OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm |    0 
 .../Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm    |    0 
 OvmfPkg/Library/BaseIoLibIntrinsic/IoHighLevel.c   |    0 
 OvmfPkg/Library/BaseIoLibIntrinsic/IoLib.c         |    0 
 OvmfPkg/Library/BaseIoLibIntrinsic/IoLibArm.c      |    0 
 OvmfPkg/Library/BaseIoLibIntrinsic/IoLibEbc.c      |    0 
 OvmfPkg/Library/BaseIoLibIntrinsic/IoLibGcc.c      |    0 
 OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIcc.c      |    0 
 OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIpf.c      |    0 
 .../Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c   |    0 
 OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMsc.c      |    0 
 OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm  |    0 
 OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm |    0 
 OvmfPkg/OvmfPkgIa32X64.dsc                         |    2 +-
 OvmfPkg/OvmfPkgX64.dsc                             |    2 +-
 18 files changed, 2 insertions(+), 2 deletions(-)
 copy MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf => OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf (100%)
 copy MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni => OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni (100%)
 copy MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h => OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h (100%)
 copy MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm => OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm (100%)
 copy MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm => OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm (100%)
 copy MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoHighLevel.c (100%)
 copy MdePkg/Library/BaseIoLibIntrinsic/IoLib.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLib.c (100%)
 copy MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibArm.c (100%)
 copy MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibEbc.c (100%)
 copy MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibGcc.c (100%)
 copy MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIcc.c (100%)
 copy MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIpf.c (100%)
 copy MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c (100%)
 copy MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMsc.c (100%)
 copy MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm => OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm (100%)
 copy MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm => OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm (100%)

diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf b/OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
copy to OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni b/OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
copy to OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h b/OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
copy to OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm b/OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
copy to OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm b/OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
copy to OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLib.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLib.c
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLib.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibArm.c
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c
copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibArm.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm b/OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
copy to OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm b/OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
copy to OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index a35e1d2..fd89518 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -106,7 +106,7 @@
   PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
   PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
   PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
-  IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
+  IoLib|OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
   OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
   SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
   MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 5d853d6..ce77170 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -106,7 +106,7 @@
   PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
   PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
   PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
-  IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
+  IoLib|OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
   OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
   SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
   MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
Posted by Laszlo Ersek 7 years, 8 months ago 
On 03/07/17 00:27, Brijesh Singh wrote:
> Imports IoLib into OvmfPkg to make the changes to support SEV guest.

Ugh, this looks terrible.

$ wc -l $(git ls-files MdePkg/Library/BaseIoLibIntrinsic/)
    82 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
    24 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
    26 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
   141 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
   137 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
  2356 MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
   317 MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
   599 MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c
   342 MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
   196 MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
   214 MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
   736 MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
   411 MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
   228 MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
   127 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
   126 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
  6062 total

Jordan, Liming, if I recall correctly, you guys were leading the
IoFifoLib discussion a few weeks back. At that time, I would have
preferred to add those functions to a separate IoFifoLib class (like
Brijesh originally suggested), but seeing the consensus on adding the
Fifo primitives to IoLib instead, I didn't speak up.

So now that the Fifo primitives have to be customized (unrolled), and
the selection should be made dynamically (at runtime), what do you guys
suggest for the implementation, without importing six thousand lines
into OvmfPkg?

I think this patch should be dropped, and the next patch (#5) should be
applied straight to MdePkg. SEV detection happens via the CPUID
instruction, and it is specified by a public industry standard, so
adding the code to MdePkg looks appropriate to me.

If even the CPUID check should be omitted in the default case, then we
should use a new FeaturePCD.

Thanks,
Laszlo

> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  .../BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf      |    0 
>  .../BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni      |    0 
>  .../BaseIoLibIntrinsicInternal.h                   |    0 
>  OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm |    0 
>  .../Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm    |    0 
>  OvmfPkg/Library/BaseIoLibIntrinsic/IoHighLevel.c   |    0 
>  OvmfPkg/Library/BaseIoLibIntrinsic/IoLib.c         |    0 
>  OvmfPkg/Library/BaseIoLibIntrinsic/IoLibArm.c      |    0 
>  OvmfPkg/Library/BaseIoLibIntrinsic/IoLibEbc.c      |    0 
>  OvmfPkg/Library/BaseIoLibIntrinsic/IoLibGcc.c      |    0 
>  OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIcc.c      |    0 
>  OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIpf.c      |    0 
>  .../Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c   |    0 
>  OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMsc.c      |    0 
>  OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm  |    0 
>  OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm |    0 
>  OvmfPkg/OvmfPkgIa32X64.dsc                         |    2 +-
>  OvmfPkg/OvmfPkgX64.dsc                             |    2 +-
>  18 files changed, 2 insertions(+), 2 deletions(-)
>  copy MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf => OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf (100%)
>  copy MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni => OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni (100%)
>  copy MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h => OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h (100%)
>  copy MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm => OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm (100%)
>  copy MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm => OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm (100%)
>  copy MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoHighLevel.c (100%)
>  copy MdePkg/Library/BaseIoLibIntrinsic/IoLib.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLib.c (100%)
>  copy MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibArm.c (100%)
>  copy MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibEbc.c (100%)
>  copy MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibGcc.c (100%)
>  copy MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIcc.c (100%)
>  copy MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIpf.c (100%)
>  copy MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c (100%)
>  copy MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMsc.c (100%)
>  copy MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm => OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm (100%)
>  copy MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm => OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm (100%)
> 
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf b/OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni b/OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h b/OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm b/OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm b/OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLib.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLib.c
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLib.c
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibArm.c
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibArm.c
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm b/OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm b/OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index a35e1d2..fd89518 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -106,7 +106,7 @@
>    PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
>    PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
>    PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
> -  IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
> +  IoLib|OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
>    OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
>    SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
>    MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 5d853d6..ce77170 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -106,7 +106,7 @@
>    PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
>    PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
>    PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
> -  IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
> +  IoLib|OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
>    OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
>    SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
>    MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
Posted by Jordan Justen 7 years, 8 months ago 
On 2017-03-07 09:20:14, Laszlo Ersek wrote:
> On 03/07/17 00:27, Brijesh Singh wrote:
> > Imports IoLib into OvmfPkg to make the changes to support SEV guest.
> 
> Ugh, this looks terrible.
> 
> $ wc -l $(git ls-files MdePkg/Library/BaseIoLibIntrinsic/)
>     82 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
>     24 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
>     26 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
>    141 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
>    137 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
>   2356 MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
>    317 MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
>    599 MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c
>    342 MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
>    196 MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
>    214 MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
>    736 MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
>    411 MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
>    228 MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
>    127 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
>    126 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
>   6062 total
> 
> Jordan, Liming, if I recall correctly, you guys were leading the
> IoFifoLib discussion a few weeks back. At that time, I would have
> preferred to add those functions to a separate IoFifoLib class (like
> Brijesh originally suggested), but seeing the consensus on adding the
> Fifo primitives to IoLib instead, I didn't speak up.
> 
> So now that the Fifo primitives have to be customized (unrolled), and
> the selection should be made dynamically (at runtime), what do you guys
> suggest for the implementation, without importing six thousand lines
> into OvmfPkg?
> 
> I think this patch should be dropped, and the next patch (#5) should be
> applied straight to MdePkg. SEV detection happens via the CPUID
> instruction, and it is specified by a public industry standard, so
> adding the code to MdePkg looks appropriate to me.

Yeah, I agree. (Not sure if Liming and Mike agree though. :)

Additionally, it would be nice to have a spec citation for the "Public
Industry Standard" in the commit message.

> If even the CPUID check should be omitted in the default case, then we
> should use a new FeaturePCD.

Apparently we don't mind terribly about adding a cpuid call straight
into the normal flow of commonly used functions
(881813d7a93d9009c873515b043c41c4554779e4). :)

I would say that I don't quite agree with that. And, further, it could
be that once per I/O operation has more of a perf impact than once per
flush. Do we know that cpuid time is so far down in the noise compared
to I/O that it won't matter?

One other thought is, should we consider a DxeSmm alternative .inf for
BaseIoLibIntrinsic.inf? In that case we could use a global variable to
help out. Maybe this could prevent the concern that might drive a new
PCD to be added?

-Jordan
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
Posted by Laszlo Ersek 7 years, 8 months ago 
On 03/07/17 21:06, Jordan Justen wrote:
> On 2017-03-07 09:20:14, Laszlo Ersek wrote:
>> On 03/07/17 00:27, Brijesh Singh wrote:
>>> Imports IoLib into OvmfPkg to make the changes to support SEV guest.
>>
>> Ugh, this looks terrible.
>>
>> $ wc -l $(git ls-files MdePkg/Library/BaseIoLibIntrinsic/)
>>     82 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
>>     24 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
>>     26 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
>>    141 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
>>    137 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
>>   2356 MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
>>    317 MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
>>    599 MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c
>>    342 MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
>>    196 MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
>>    214 MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
>>    736 MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
>>    411 MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
>>    228 MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
>>    127 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
>>    126 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
>>   6062 total
>>
>> Jordan, Liming, if I recall correctly, you guys were leading the
>> IoFifoLib discussion a few weeks back. At that time, I would have
>> preferred to add those functions to a separate IoFifoLib class (like
>> Brijesh originally suggested), but seeing the consensus on adding the
>> Fifo primitives to IoLib instead, I didn't speak up.
>>
>> So now that the Fifo primitives have to be customized (unrolled), and
>> the selection should be made dynamically (at runtime), what do you guys
>> suggest for the implementation, without importing six thousand lines
>> into OvmfPkg?
>>
>> I think this patch should be dropped, and the next patch (#5) should be
>> applied straight to MdePkg. SEV detection happens via the CPUID
>> instruction, and it is specified by a public industry standard, so
>> adding the code to MdePkg looks appropriate to me.
> 
> Yeah, I agree. (Not sure if Liming and Mike agree though. :)
> 
> Additionally, it would be nice to have a spec citation for the "Public
> Industry Standard" in the commit message.
> 
>> If even the CPUID check should be omitted in the default case, then we
>> should use a new FeaturePCD.
> 
> Apparently we don't mind terribly about adding a cpuid call straight
> into the normal flow of commonly used functions
> (881813d7a93d9009c873515b043c41c4554779e4). :)
> 
> I would say that I don't quite agree with that.

Well, I'm neutral on it. If keeping the CPUID for SEV feature detection
is okay as far as the coding style is concerned, I"m all for it.

> And, further, it could
> be that once per I/O operation has more of a perf impact than once per
> flush. Do we know that cpuid time is so far down in the noise compared
> to I/O that it won't matter?

Well, for fifo ops at least, the overhead of CPUID shouldn't be large.

> 
> One other thought is, should we consider a DxeSmm alternative .inf for
> BaseIoLibIntrinsic.inf?

I sort of dislike that, unless Brijesh can solve it with another INF
file within the same directory, and minimal code duplication. (I.e.,
with most source files reused.)

> In that case we could use a global variable to
> help out. Maybe this could prevent the concern that might drive a new
> PCD to be added?

This crossed my mind (and then the CPUID would be executed only once per
library constructor invocation), but I was (somewhat inexplicably?)
worried that such a library instance would not be suitable for SEC code,
and for PEI code running on different platforms (i.e., XIP from flash).
Those restrictions don't seem to apply to OVMF though, so I guess the
idea is worth exploring!

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
Posted by Gao, Liming 7 years, 8 months ago 

>-----Original Message-----
>From: Justen, Jordan L
>Sent: Wednesday, March 08, 2017 4:06 AM
>To: Gao, Liming <liming.gao@intel.com>; Brijesh Singh
><brijesh.ksingh@gmail.com>; Laszlo Ersek <lersek@redhat.com>; edk2-
>devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
>Cc: Thomas.Lendacky@amd.com; leo.duran@amd.com;
>brijesh.sing@amd.com
>Subject: Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import
>BaseIoLibIntrinsic package
>
>On 2017-03-07 09:20:14, Laszlo Ersek wrote:
>> On 03/07/17 00:27, Brijesh Singh wrote:
>> > Imports IoLib into OvmfPkg to make the changes to support SEV guest.
>>
>> Ugh, this looks terrible.
>>
>> $ wc -l $(git ls-files MdePkg/Library/BaseIoLibIntrinsic/)
>>     82 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
>>     24 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
>>     26 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
>>    141 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
>>    137 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
>>   2356 MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
>>    317 MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
>>    599 MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c
>>    342 MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
>>    196 MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
>>    214 MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
>>    736 MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
>>    411 MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
>>    228 MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
>>    127 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
>>    126 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
>>   6062 total
>>
>> Jordan, Liming, if I recall correctly, you guys were leading the
>> IoFifoLib discussion a few weeks back. At that time, I would have
>> preferred to add those functions to a separate IoFifoLib class (like
>> Brijesh originally suggested), but seeing the consensus on adding the
>> Fifo primitives to IoLib instead, I didn't speak up.
>>
>> So now that the Fifo primitives have to be customized (unrolled), and
>> the selection should be made dynamically (at runtime), what do you guys
>> suggest for the implementation, without importing six thousand lines
>> into OvmfPkg?
>>
>> I think this patch should be dropped, and the next patch (#5) should be
>> applied straight to MdePkg. SEV detection happens via the CPUID
>> instruction, and it is specified by a public industry standard, so
>> adding the code to MdePkg looks appropriate to me.
>
>Yeah, I agree. (Not sure if Liming and Mike agree though. :)
I agree. If SEV is the public industry standard, it can be added into MdePkg Library implementation. I suggest to add spec refer in file header. 

>
>Additionally, it would be nice to have a spec citation for the "Public
>Industry Standard" in the commit message.
>
>> If even the CPUID check should be omitted in the default case, then we
>> should use a new FeaturePCD.
>
>Apparently we don't mind terribly about adding a cpuid call straight
>into the normal flow of commonly used functions
>(881813d7a93d9009c873515b043c41c4554779e4). :)
>
>I would say that I don't quite agree with that. And, further, it could
>be that once per I/O operation has more of a perf impact than once per
>flush. Do we know that cpuid time is so far down in the noise compared
>to I/O that it won't matter?
>
I worry about functionality. Dose CheckSevFeature() work on Intel X86 CPU? If Intel X86 CPU doesn't support SEV, will CheckSevFeature() return 0? If Intel X86 CPU doesn't work, we need to add new PCD to control its logic. 

>One other thought is, should we consider a DxeSmm alternative .inf for
>BaseIoLibIntrinsic.inf? In that case we could use a global variable to
>help out. Maybe this could prevent the concern that might drive a new
>PCD to be added?
>
>-Jordan
Current patch has stored the check state into data section. In PEI phase, the data section can't be written. So, every call will check CpuId. In DXE and SMM phase, the data section can be written. The first call will cache the check state. So, no DxeSmm INF is required. 


Thanks
Liming
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
Posted by Brijesh Singh 7 years, 8 months ago 
On Wed, Mar 8, 2017 at 9:41 AM, Gao, Liming <liming.gao@intel.com> wrote:

>
>
> >-----Original Message-----
> >From: Justen, Jordan L
> >Sent: Wednesday, March 08, 2017 4:06 AM
> >To: Gao, Liming <liming.gao@intel.com>; Brijesh Singh
> ><brijesh.ksingh@gmail.com>; Laszlo Ersek <lersek@redhat.com>; edk2-
> >devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
> >Cc: Thomas.Lendacky@amd.com; leo.duran@amd.com;
> >brijesh.sing@amd.com
> >Subject: Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import
> >BaseIoLibIntrinsic package
> >
> >On 2017-03-07 09:20:14, Laszlo Ersek wrote:
> >> On 03/07/17 00:27, Brijesh Singh wrote:
> >> > Imports IoLib into OvmfPkg to make the changes to support SEV guest.
> >>
> >> Ugh, this looks terrible.
> >>
> >> $ wc -l $(git ls-files MdePkg/Library/BaseIoLibIntrinsic/)
> >>     82 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
> >>     24 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
> >>     26 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
> >>    141 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
> >>    137 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
> >>   2356 MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
> >>    317 MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
> >>    599 MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c
> >>    342 MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
> >>    196 MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
> >>    214 MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
> >>    736 MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
> >>    411 MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
> >>    228 MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
> >>    127 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
> >>    126 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
> >>   6062 total
> >>
> >> Jordan, Liming, if I recall correctly, you guys were leading the
> >> IoFifoLib discussion a few weeks back. At that time, I would have
> >> preferred to add those functions to a separate IoFifoLib class (like
> >> Brijesh originally suggested), but seeing the consensus on adding the
> >> Fifo primitives to IoLib instead, I didn't speak up.
> >>
> >> So now that the Fifo primitives have to be customized (unrolled), and
> >> the selection should be made dynamically (at runtime), what do you guys
> >> suggest for the implementation, without importing six thousand lines
> >> into OvmfPkg?
> >>
> >> I think this patch should be dropped, and the next patch (#5) should be
> >> applied straight to MdePkg. SEV detection happens via the CPUID
> >> instruction, and it is specified by a public industry standard, so
> >> adding the code to MdePkg looks appropriate to me.
> >
> >Yeah, I agree. (Not sure if Liming and Mike agree though. :)
> I agree. If SEV is the public industry standard, it can be added into
> MdePkg Library implementation. I suggest to add spec refer in file header.
>
> >
> >Additionally, it would be nice to have a spec citation for the "Public
> >Industry Standard" in the commit message.
> >
> >> If even the CPUID check should be omitted in the default case, then we
> >> should use a new FeaturePCD.
> >
> >Apparently we don't mind terribly about adding a cpuid call straight
> >into the normal flow of commonly used functions
> >(881813d7a93d9009c873515b043c41c4554779e4). :)
> >
> >I would say that I don't quite agree with that. And, further, it could
> >be that once per I/O operation has more of a perf impact than once per
> >flush. Do we know that cpuid time is so far down in the noise compared
> >to I/O that it won't matter?
> >
> I worry about functionality. Dose CheckSevFeature() work on Intel X86 CPU?
> If Intel X86 CPU doesn't support SEV, will CheckSevFeature() return 0? If
> Intel X86 CPU doesn't work, we need to add new PCD to control its logic.
>
>

The CheckSevFeature() should return 0 on non SEV platform. I have tried
booting a virtual guest on Intel X86 CPU and it seems to be work fine (
defaults to rep string I/O).  Both Intel and AMD have reserved CPUID
4000_0000 - 4000_00FF for software usage. So if we are booting in non
virtualized environment then CPUID 4000_0001 should return 0 and in
virtualized environment the BIT 8  should be 1 if hypervisor support SEV.
So far I have using KVM hypervisor to test my code but I will investigate
more to ensure that the check works accross multiple hypervisors.


>One other thought is, should we consider a DxeSmm alternative .inf for
> >BaseIoLibIntrinsic.inf? In that case we could use a global variable to
> >help out. Maybe this could prevent the concern that might drive a new
> >PCD to be added?
> >
> >-Jordan
> Current patch has stored the check state into data section. In PEI phase,
> the data section can't be written. So, every call will check CpuId. In DXE
> and SMM phase, the data section can be written. The first call will cache
> the check state. So, no DxeSmm INF is required.
>
>
> Thanks
> Liming
>



-- 
Confusion is always the most honest response.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
Posted by Gao, Liming 7 years, 8 months ago 

From: Brijesh Singh [mailto:brijesh.ksingh@gmail.com]
Sent: Thursday, March 9, 2017 12:27 AM
To: Gao, Liming <liming.gao@intel.com>
Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>; Thomas.Lendacky@amd.com; leo.duran@amd.com; brijesh.singh@amd.com
Subject: Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package



On Wed, Mar 8, 2017 at 9:41 AM, Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>> wrote:


>-----Original Message-----
>From: Justen, Jordan L
>Sent: Wednesday, March 08, 2017 4:06 AM
>To: Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Brijesh Singh
><brijesh.ksingh@gmail.com<mailto:brijesh.ksingh@gmail.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; edk2-
>devel@lists.01.org<mailto:devel@lists.01.org>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
>Cc: Thomas.Lendacky@amd.com<mailto:Thomas.Lendacky@amd.com>; leo.duran@amd.com<mailto:leo.duran@amd.com>;
>brijesh.sing@amd.com<mailto:brijesh.sing@amd.com>
>Subject: Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import
>BaseIoLibIntrinsic package
>
>On 2017-03-07 09:20:14, Laszlo Ersek wrote:
>> On 03/07/17 00:27, Brijesh Singh wrote:
>> > Imports IoLib into OvmfPkg to make the changes to support SEV guest.
>>
>> Ugh, this looks terrible.
>>
>> $ wc -l $(git ls-files MdePkg/Library/BaseIoLibIntrinsic/)
>>     82 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
>>     24 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
>>     26 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
>>    141 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
>>    137 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
>>   2356 MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
>>    317 MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
>>    599 MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c
>>    342 MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
>>    196 MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
>>    214 MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
>>    736 MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
>>    411 MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
>>    228 MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
>>    127 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
>>    126 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
>>   6062 total
>>
>> Jordan, Liming, if I recall correctly, you guys were leading the
>> IoFifoLib discussion a few weeks back. At that time, I would have
>> preferred to add those functions to a separate IoFifoLib class (like
>> Brijesh originally suggested), but seeing the consensus on adding the
>> Fifo primitives to IoLib instead, I didn't speak up.
>>
>> So now that the Fifo primitives have to be customized (unrolled), and
>> the selection should be made dynamically (at runtime), what do you guys
>> suggest for the implementation, without importing six thousand lines
>> into OvmfPkg?
>>
>> I think this patch should be dropped, and the next patch (#5) should be
>> applied straight to MdePkg. SEV detection happens via the CPUID
>> instruction, and it is specified by a public industry standard, so
>> adding the code to MdePkg looks appropriate to me.
>
>Yeah, I agree. (Not sure if Liming and Mike agree though. :)
I agree. If SEV is the public industry standard, it can be added into MdePkg Library implementation. I suggest to add spec refer in file header.

>
>Additionally, it would be nice to have a spec citation for the "Public
>Industry Standard" in the commit message.
>
>> If even the CPUID check should be omitted in the default case, then we
>> should use a new FeaturePCD.
>
>Apparently we don't mind terribly about adding a cpuid call straight
>into the normal flow of commonly used functions
>(881813d7a93d9009c873515b043c41c4554779e4). :)
>
>I would say that I don't quite agree with that. And, further, it could
>be that once per I/O operation has more of a perf impact than once per
>flush. Do we know that cpuid time is so far down in the noise compared
>to I/O that it won't matter?
>
I worry about functionality. Dose CheckSevFeature() work on Intel X86 CPU? If Intel X86 CPU doesn't support SEV, will CheckSevFeature() return 0? If Intel X86 CPU doesn't work, we need to add new PCD to control its logic.


The CheckSevFeature() should return 0 on non SEV platform. I have tried booting a virtual guest on Intel X86 CPU and it seems to be work fine ( defaults to rep string I/O).  Both Intel and AMD have reserved CPUID 4000_0000 - 4000_00FF for software usage. So if we are booting in non virtualized environment then CPUID 4000_0001 should return 0 and in virtualized environment the BIT 8  should be 1 if hypervisor support SEV. So far I have using KVM hypervisor to test my code but I will investigate more to ensure that the check works accross multiple hypervisors.

[Liming] As you say CPUID 4000_0001 is reserved, AMD uses it for SEV. Intel may use it for other purpose. If so, we can’t make sure CheckSevFeature() always work. Then, I suggest to introduce Pcd to enable/disable SEV checker.

>One other thought is, should we consider a DxeSmm alternative .inf for
>BaseIoLibIntrinsic.inf? In that case we could use a global variable to
>help out. Maybe this could prevent the concern that might drive a new
>PCD to be added?
>
>-Jordan
Current patch has stored the check state into data section. In PEI phase, the data section can't be written. So, every call will check CpuId. In DXE and SMM phase, the data section can be written. The first call will cache the check state. So, no DxeSmm INF is required.


Thanks
Liming



--
Confusion is always the most honest response.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
Posted by Jordan Justen 7 years, 8 months ago 
On 2017-03-08 07:41:58, Gao, Liming wrote:
> 
> >-----Original Message-----
> >From: Justen, Jordan L
> 
> >One other thought is, should we consider a DxeSmm alternative .inf for
> >BaseIoLibIntrinsic.inf? In that case we could use a global variable to
> >help out. Maybe this could prevent the concern that might drive a new
> >PCD to be added?
> >
> >-Jordan
> Current patch has stored the check state into data section. In PEI
> phase, the data section can't be written. So, every call will check
> CpuId. In DXE and SMM phase, the data section can be written. The
> first call will cache the check state. So, no DxeSmm INF is
> required.
> 

I don't think we can attempt to write a variable to memory in generic
SEC/PEI code. Some flash memory treat memory writes as an I/O for
programming purposes. I think we added
PcdGuidedExtractHandlerTableAddress for this reason. This is why I
suggested that maybe we could add a DXE/SMM .inf where we could assume
writeable global variables exit.

-Jordan
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
Posted by Gao, Liming 7 years, 8 months ago 

> -----Original Message-----
> From: Justen, Jordan L
> Sent: Thursday, March 9, 2017 2:59 AM
> To: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Brijesh Singh
> <brijesh.ksingh@gmail.com>; Laszlo Ersek <lersek@redhat.com>
> Cc: Thomas.Lendacky@amd.com; leo.duran@amd.com; brijesh.sing@amd.com
> Subject: RE: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
> 
> On 2017-03-08 07:41:58, Gao, Liming wrote:
> >
> > >-----Original Message-----
> > >From: Justen, Jordan L
> >
> > >One other thought is, should we consider a DxeSmm alternative .inf for
> > >BaseIoLibIntrinsic.inf? In that case we could use a global variable to
> > >help out. Maybe this could prevent the concern that might drive a new
> > >PCD to be added?
> > >
> > >-Jordan
> > Current patch has stored the check state into data section. In PEI
> > phase, the data section can't be written. So, every call will check
> > CpuId. In DXE and SMM phase, the data section can be written. The
> > first call will cache the check state. So, no DxeSmm INF is
> > required.
> >
> 
> I don't think we can attempt to write a variable to memory in generic
> SEC/PEI code. Some flash memory treat memory writes as an I/O for
> programming purposes. I think we added
> PcdGuidedExtractHandlerTableAddress for this reason. This is why I
> suggested that maybe we could add a DXE/SMM .inf where we could assume
> writeable global variables exit.
> 
> -Jordan

I get your point. So, I suggest to always check SEV in BaseIoLib. If people meet with the performance issue, DXE/SMM version IoLib can be added later. 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
Posted by Duran, Leo 7 years, 8 months ago 
OK, what I'm hearing is:
- Let's leave the Fifo routines in BaseIoLib (as we currently have)
- And let's add the SEV checks to BaseIoLib, so that OvmPkg (et al) can consume it as-is

If so, I could put a patch-set together for that... Please confirm.
BTW, I'd try to minimize the need for CPUID (e.g., check it once and cache the result)

Leo

> -----Original Message-----
> From: Gao, Liming [mailto:liming.gao@intel.com]
> Sent: Wednesday, March 08, 2017 7:48 PM
> To: Justen, Jordan L <jordan.l.justen@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Brijesh Singh
> <brijesh.ksingh@gmail.com>; Laszlo Ersek <lersek@redhat.com>
> Cc: Lendacky, Thomas <Thomas.Lendacky@amd.com>; Duran, Leo
> <leo.duran@amd.com>; brijesh.sing@amd.com
> Subject: RE: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import
> BaseIoLibIntrinsic package
> 
> 
> 
> > -----Original Message-----
> > From: Justen, Jordan L
> > Sent: Thursday, March 9, 2017 2:59 AM
> > To: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Brijesh Singh
> > <brijesh.ksingh@gmail.com>; Laszlo Ersek <lersek@redhat.com>
> > Cc: Thomas.Lendacky@amd.com; leo.duran@amd.com;
> brijesh.sing@amd.com
> > Subject: RE: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import
> > BaseIoLibIntrinsic package
> >
> > On 2017-03-08 07:41:58, Gao, Liming wrote:
> > >
> > > >-----Original Message-----
> > > >From: Justen, Jordan L
> > >
> > > >One other thought is, should we consider a DxeSmm alternative .inf
> > > >for BaseIoLibIntrinsic.inf? In that case we could use a global
> > > >variable to help out. Maybe this could prevent the concern that
> > > >might drive a new PCD to be added?
> > > >
> > > >-Jordan
> > > Current patch has stored the check state into data section. In PEI
> > > phase, the data section can't be written. So, every call will check
> > > CpuId. In DXE and SMM phase, the data section can be written. The
> > > first call will cache the check state. So, no DxeSmm INF is
> > > required.
> > >
> >
> > I don't think we can attempt to write a variable to memory in generic
> > SEC/PEI code. Some flash memory treat memory writes as an I/O for
> > programming purposes. I think we added
> > PcdGuidedExtractHandlerTableAddress for this reason. This is why I
> > suggested that maybe we could add a DXE/SMM .inf where we could
> assume
> > writeable global variables exit.
> >
> > -Jordan
> 
> I get your point. So, I suggest to always check SEV in BaseIoLib. If people
> meet with the performance issue, DXE/SMM version IoLib can be added
> later.
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
Posted by Laszlo Ersek 7 years, 8 months ago 
On 03/09/17 16:36, Duran, Leo wrote:
> OK, what I'm hearing is:
> - Let's leave the Fifo routines in BaseIoLib (as we currently have)
> - And let's add the SEV checks to BaseIoLib, so that OvmPkg (et al) can consume it as-is
> 
> If so, I could put a patch-set together for that... Please confirm.

Confirmed from my side.

> BTW, I'd try to minimize the need for CPUID (e.g., check it once and cache the result)

In a general purpose BASE library instance, you can't rely on memory
(writeable memory) being available. The library instance could be linked
into SEC or PEI phase modules that (generally speaking) have no
writeable global variables.

It's only an OvmfPkg specialty that PEI has writeable global variables.
(And it comes with a non-intuitive downside: in the SMM-less build of
OVMF, on S3 resume, the PEI global variables are not re-initialized to
the values that the C language would require. This is something we
always have to keep in mind.)

I don't think the CPUID checks will have a huge performance impact,
especially because (IIUC) you only have to customize the Fifo routines.
The cost of the CPUID will be amortized over all the individual IO port
accesses (--> traps) that you will perform individually anyway.

IMO the FeaturePCD check (which will be evaluated at compile time) is a
valid requirement, the CPUID result caching is not -- not until you can
measure the CPUID's impact to be "grave" under "general" workloads. Even
then, the separate DxeSmm instance can be added incrementally (as
suggested by others).

Thanks!
Laszlo

> 
> Leo
> 
>> -----Original Message-----
>> From: Gao, Liming [mailto:liming.gao@intel.com]
>> Sent: Wednesday, March 08, 2017 7:48 PM
>> To: Justen, Jordan L <jordan.l.justen@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Brijesh Singh
>> <brijesh.ksingh@gmail.com>; Laszlo Ersek <lersek@redhat.com>
>> Cc: Lendacky, Thomas <Thomas.Lendacky@amd.com>; Duran, Leo
>> <leo.duran@amd.com>; brijesh.sing@amd.com
>> Subject: RE: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import
>> BaseIoLibIntrinsic package
>>
>>
>>
>>> -----Original Message-----
>>> From: Justen, Jordan L
>>> Sent: Thursday, March 9, 2017 2:59 AM
>>> To: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
>>> <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Brijesh Singh
>>> <brijesh.ksingh@gmail.com>; Laszlo Ersek <lersek@redhat.com>
>>> Cc: Thomas.Lendacky@amd.com; leo.duran@amd.com;
>> brijesh.sing@amd.com
>>> Subject: RE: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import
>>> BaseIoLibIntrinsic package
>>>
>>> On 2017-03-08 07:41:58, Gao, Liming wrote:
>>>>
>>>>> -----Original Message-----
>>>>> From: Justen, Jordan L
>>>>
>>>>> One other thought is, should we consider a DxeSmm alternative .inf
>>>>> for BaseIoLibIntrinsic.inf? In that case we could use a global
>>>>> variable to help out. Maybe this could prevent the concern that
>>>>> might drive a new PCD to be added?
>>>>>
>>>>> -Jordan
>>>> Current patch has stored the check state into data section. In PEI
>>>> phase, the data section can't be written. So, every call will check
>>>> CpuId. In DXE and SMM phase, the data section can be written. The
>>>> first call will cache the check state. So, no DxeSmm INF is
>>>> required.
>>>>
>>>
>>> I don't think we can attempt to write a variable to memory in generic
>>> SEC/PEI code. Some flash memory treat memory writes as an I/O for
>>> programming purposes. I think we added
>>> PcdGuidedExtractHandlerTableAddress for this reason. This is why I
>>> suggested that maybe we could add a DXE/SMM .inf where we could
>> assume
>>> writeable global variables exit.
>>>
>>> -Jordan
>>
>> I get your point. So, I suggest to always check SEV in BaseIoLib. If people
>> meet with the performance issue, DXE/SMM version IoLib can be added
>> later.
>>
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.