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
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
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
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
>-----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
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
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
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
> -----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
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
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
© 2016 - 2024 Red Hat, Inc.