From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk0-x230.google.com (mail-qk0-x230.google.com [IPv6:2607:f8b0:400d:c09::230]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 433838032B for ; Wed, 8 Mar 2017 08:27:00 -0800 (PST) Received: by mail-qk0-x230.google.com with SMTP id v125so72875104qkh.2 for ; Wed, 08 Mar 2017 08:27:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=eTH40p87kOTY2mXOhuNc9MZ/MVpOtP2MuuPqZ2JuxiU=; b=f1nQMhyi3Fc1gjXK09KomgkiwOamaR+Dd1aenWYT1CGDnjBNwQh3Jwnel1WKZkm78K 0d5j+jQep07dTsEljoEVB8V/jS8QHCF/qsedsdlBWuyvdb1QZse2bt5siz+k05GcIpi6 75XJCn6YxfEoMdEFCXbFSbhICA8Cfs8mx2sfmdNaUIl/+H1EiqsnqkipL0Y3nqFMX+8s MeTfCALeR1zA712M8F4EInBnI2i1fY0iQM41rcksUJfD6RJ3U83yyhomXwR92WY+eYBY vqinn6Vxsolm3V2Rn+3yTW2N1kKFm2Ox8wRoQML61hQw/mqvLHFSt2V2bVtsTC/xo263 ad0w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=eTH40p87kOTY2mXOhuNc9MZ/MVpOtP2MuuPqZ2JuxiU=; b=Cayo0qRa7D+tpfol6UTmmMA0CKVuSSFuijgJmZydovy5YeZZAW0NFlo2dpy6/Igv4S E+0gC2KtDRSR0Ss6yIlU/ZI9GU7ZJOc6Ex2o7x/8nyBjf5QkTyalMOz4BCV+iPavn5Bx j9emsVbEtG/zhFPkm8bML3LzOwWnYLR3Ek+6Z/WMcATonWNV9sRstX15m5ewzzsnnR8K GTN7+AyBQz4XjCUMG2fjoVlgkpBJE0JTiZH3OWgMPLrQjRS9BYuzuMJwTFVYiw0QudkW Wkz6y5gNAnmnAXn+Rh5AcnFG1doWY0C8drfbe2ZztrY7AaXl4UCADDtBea2M7PM1fP0N e+bw== X-Gm-Message-State: AFeK/H0vivssXzV5A5ad6w7+1ye2ywKR8BGA4KKXRjxq3+diw4sGL4h+/2JTfcZYFmEq2DLkvHFGHIxKJlKWTg== X-Received: by 10.55.122.130 with SMTP id v124mr7604596qkc.19.1488990418831; Wed, 08 Mar 2017 08:26:58 -0800 (PST) MIME-Version: 1.0 Received: by 10.12.182.65 with HTTP; Wed, 8 Mar 2017 08:26:58 -0800 (PST) In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14D6ECFA9@shsmsx102.ccr.corp.intel.com> References: <148884284887.29188.7643544710695103939.stgit@brijesh-build-machine> <148884287496.29188.5155874233993236979.stgit@brijesh-build-machine> <2c6593dd-12f5-9277-0c36-ffd2d6c2cc55@redhat.com> <148891718851.27104.2018366977522352345@jljusten-skl> <4A89E2EF3DFEDB4C8BFDE51014F606A14D6ECFA9@shsmsx102.ccr.corp.intel.com> From: Brijesh Singh Date: Wed, 8 Mar 2017 10:26:58 -0600 Message-ID: To: "Gao, Liming" Cc: "Justen, Jordan L" , Laszlo Ersek , "edk2-devel@lists.01.org" , "Kinney, Michael D" , "Thomas.Lendacky@amd.com" , "leo.duran@amd.com" , brijesh.singh@amd.com X-Content-Filtered-By: Mailman/MimeDel 2.1.21 Subject: Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Mar 2017 16:27:00 -0000 Content-Type: text/plain; charset=UTF-8 On Wed, Mar 8, 2017 at 9:41 AM, Gao, Liming wrote: > > > >-----Original Message----- > >From: Justen, Jordan L > >Sent: Wednesday, March 08, 2017 4:06 AM > >To: Gao, Liming ; Brijesh Singh > >; Laszlo Ersek ; edk2- > >devel@lists.01.org; Kinney, Michael D > >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.