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

[Brijesh Singh <brijesh.ksingh@gmail.com>]

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.