From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BDAF9202E6152 for ; Mon, 16 Oct 2017 11:40:53 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E6F934E916; Mon, 16 Oct 2017 18:44:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com E6F934E916 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-250.rdu2.redhat.com [10.10.120.250]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6341F5C1A1; Mon, 16 Oct 2017 18:44:26 +0000 (UTC) To: "Duran, Leo" , "Yao, Jiewen" Cc: "edk2-devel@lists.01.org" , Paolo Bonzini References: <1507751131-32404-1-git-send-email-leo.duran@amd.com> <74D8A39837DF1E4DA445A8C0B3885C503A9E505E@SHSMSX104.ccr.corp.intel.com> <279D7F72-28F1-4DAD-B75E-780F2638BA5E@intel.com> From: Laszlo Ersek Message-ID: <3ef8c7f1-bb81-73e4-fcac-5427db379e7b@redhat.com> Date: Mon, 16 Oct 2017 20:44:25 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Mon, 16 Oct 2017 18:44:28 +0000 (UTC) Subject: Re: [PATCH v5 0/2] Enhanced SMM support for AMD-based x86 systems. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Oct 2017 18:40:53 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/16/17 19:31, Duran, Leo wrote: > > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Monday, October 16, 2017 12:06 PM >> To: Yao, Jiewen ; Duran, Leo >> >> Cc: edk2-devel@lists.01.org; Paolo Bonzini >> Subject: Re: [edk2] [PATCH v5 0/2] Enhanced SMM support for AMD-based >> x86 systems. >> >> On 10/15/17 02:58, Yao, Jiewen wrote: >>> for runtime test, I recommend using ovmf. You don't need real hardware. >> It can run both 32bit or 64bit. It can run in both Linux and windows. >>> >>> You need use -D SMM_REQUIRE option to build ovmf. >>> If you have any problem, Laszlo is the good contact. >> >> I don't have much context about this series, but looking at the blurb, I see >> that version 3 removed OvmfPkg patches: >> >>> Changes since v2: >>> The intent of this revision is to maintain compatibility with existing >>> packages. To that end, changes to OvmgfPkg and QuarkSocPkg are >>> reverted. Moreover, pertinent macros are replaced in the C code, >>> rather than on header files that are shared globally. >> >> Judged on the diffstat of patch #1 -- only >> "UefiCpuPkg/Library/SmmCpuFeaturesLib" files are modified -- I would say >> that changes in patch #1 are invisible to OVMF. The reason is that OVMF uses >> a separate SmmCpuFeaturesLib instance, namely >> >> OvmfPkg/Library/SmmCpuFeaturesLib >> >> This means two things: >> >> - changes from patch #1 cannot be tested with OVMF, simply because >> "UefiCpuPkg/Library/SmmCpuFeaturesLib" is never built for OVMF; >> >> - changes from patch #2 may or may not break SMM in OVMF, dependent on >> whether patch #2 is tied closely to patch #1. >> >> In order to see why OvmfPkg has a separate SmmCpuFeaturesLib instance, >> please review the commit log: >> >> git log --reverse -- OvmfPkg/Library/SmmCpuFeaturesLib >> >> At this point I cannot determine if this patch set should ignore OvmfPkg >> completely, or else patch #1 should be duplicated for >> "OvmfPkg/Library/SmmCpuFeaturesLib" as well. (I guess I don't understand >> the goal of the patch set -- I've read the blurb, but the problem has not been >> stated well enough for me to understand. Or maybe it was stated long ago, >> and I've forgotten it :) ) >> > > Lazlo, > I purposely left out changes to OVMF and Quark, consistent with previous feedback. I've found my previous comments: http://mid.mail-archive.com/2d3efa5a-ad72-bb35-1e6a-b9b78379337c@redhat.com There I only suggested a different (more telling) subject for the OvmfPkg patch, and wrote, > (Of course I realize the patch might entirely be replaced in the next > version, based on Jiewen's and Mike's feedback -- that's OK with me, I > just wanted to give an example.) I didn't try to validate Jiewen's / Mike's feedback; I just stated *if*, according to them, patching OvmfPkg was not necessary, I'd be OK with that. Since we're talking anyway, can you (and/or Jiewen & Mike) please state the problem being solved here, and explain why patching the SmmCpuFeaturesLib instance in OvmfPkg is, or is not, necessary to update? Hmmm... Is it the case that UefiCpuPkg/Library/SmmCpuFeaturesLib runs correctly on Intel *hosts* only at the moment (so it needs fixing, for AMD *hosts*), while OvmfPkg/Library/SmmCpuFeaturesLib deals with AMD-looking *guests* anyway, so it needs no fixing, for AMD compatibility? If this is correct, then I agree patch #1 does not need to be duplicated for OvmfPkg. *However*, in turn, patch #2 (for PiSmmCpuDxeSmm) might be necessary to update for QEMU. PiSmmCpuDxeSmm runs on both bare metal and on QEMU. And, as Paolo says, a pure CPUID / manufacturer check (for determining the state save layout) is wrong on QEMU, even if the same would work on bare metal: @@ -547,6 +595,20 @@ PiCpuSmmEntry ( ); // + // Override SMRAM offsets for AMD + // + if (StandardSignatureIsAuthenticAMD ()) { + gSmmSmramStateMapOffset = AMD_SMRAM_SAVE_STATE_MAP_OFFSET; + gSmmPsdOffset = AMD_SMM_PSD_OFFSET; + } + If patch v5 2/2 is merely a refactoring (i.e., it causes PiSmmCpuDxeSmm to behave exactly the same as before, just with an improved implementation), then I agree a CPUID-based check is not necessarily a bug (regression). Instead, it might be called a missed opportunity (or, more nicely put, a "basis") for bringing PiSmmCpuDxeSmm closer to QEMU. My apologies if I'm confused. Thanks Laszlo