From patchwork Mon Apr 1 14:01:13 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Andrew Cooper X-Patchwork-Id: 10880043 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id D8CDF922 for ; Mon, 1 Apr 2019 14:03:34 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C367D285D9 for ; Mon, 1 Apr 2019 14:03:34 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B76CF286F8; Mon, 1 Apr 2019 14:03:34 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 1AEFB285D9 for ; Mon, 1 Apr 2019 14:03:34 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1hAxVC-00039e-7o; Mon, 01 Apr 2019 14:01:30 +0000 Received: from us1-rack-dfw2.inumbo.com ([104.130.134.6]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1hAxVA-000397-Li for xen-devel@lists.xen.org; Mon, 01 Apr 2019 14:01:28 +0000 X-Inumbo-ID: a40023ac-5486-11e9-bc90-bc764e045a96 Received: from SMTP03.CITRIX.COM (unknown [162.221.156.55]) by us1-rack-dfw2.inumbo.com (Halon) with ESMTPS id a40023ac-5486-11e9-bc90-bc764e045a96; Mon, 01 Apr 2019 14:01:27 +0000 (UTC) X-IronPort-AV: E=Sophos;i="5.60,296,1549929600"; d="scan'208";a="82382462" From: Andrew Cooper To: Xen-devel Date: Mon, 1 Apr 2019 15:01:13 +0100 Message-ID: <1554127273-18663-1-git-send-email-andrew.cooper3@citrix.com> X-Mailer: git-send-email 2.1.4 MIME-Version: 1.0 Subject: [Xen-devel] [PATCH] x86/msr: Fix handling of MSR_AMD_PATCHLEVEL/MSR_IA32_UCODE_REV X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Andrew Cooper , David Wang , Wei Liu , Jan Beulich , =?utf-8?q?Roger_Pau_Monn=C3=A9?= Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" X-Virus-Scanned: ClamAV using ClamSMTP There are a number of bugs. There are no read/write hooks on the HVM side, so guest accesses fall into the "read/write-discard" defaults, which bypass the correct faulting behaviour and the Intel special case. For the PV side, writes are discarded (again, bypassing proper faulting), except for a pinned dom0, which is permitted to actually write the values other than 0. This is pointless with read hook implementing the Intel special case. However, implementing the Intel special case is pointless. First of all, OS software can't guarentee to read back 0 in the first place, because a) this behaviour isn't guarenteed in the SDM, and b) there are SMM handlers which use the CPUID instruction. Secondly, when a guest executes CPUID, this doesn't typically result in Xen executing a CPUID instruction in practice. With the dom0 special case removed, there are now no writes to this MSR other than Xen's microcode loading facilities, which means that the value held in the MSR will be properly up-to-date. Forward it directly, without jumping through any hoops. Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich --- CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné CC: David Wang This patch texturally (but not functionally) interacts with "xen/sched: Remove d->is_pinned" but rebasing either is easy. It would also be best applied with Sergey's "x86/microcode: always collect_cpu_info() during boot". The migration case is complicated. A guest which re-evaluates its idea of the world may find something completely different, but this is probably less bad letting it see the wrong microcode version. The cross-vendor case is even worse, because Intel and AMD report the version in different 32bit words in this MSR. The result here is fairly close to current behaviour. David: Do Shanghai processors do microcode loading, and if so, how? I can't find any documentation (at all), and don't see any support in Xen or Linux. --- xen/arch/x86/msr.c | 35 +++++++++++++++++++++++++++++++++++ xen/arch/x86/pv/emul-priv-op.c | 22 ---------------------- 2 files changed, 35 insertions(+), 22 deletions(-) diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c index 4df4a59..59b4298 100644 --- a/xen/arch/x86/msr.c +++ b/xen/arch/x86/msr.c @@ -135,6 +135,28 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val) /* Not offered to guests. */ goto gp_fault; + case MSR_AMD_PATCHLEVEL: + BUILD_BUG_ON(MSR_IA32_UCODE_REV != MSR_AMD_PATCHLEVEL); + /* + * AMD and Intel use the same MSR for the current microcode version. + * + * There is no need to jump through the SDM-provided hoops for Intel. + * A guest might itself perform the "write 0, CPUID, read" sequence, + * but servicing the CPUID for the guest typically wont result in + * actually executing a CPUID instruction. + * + * However, as a guest can't influence the value of this MSR, the + * value will be from Xen's last microcode load, which can be + * forwarded straight to the guest. + */ + if ( (d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL && + d->arch.cpuid->x86_vendor != X86_VENDOR_AMD) || + (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL && + boot_cpu_data.x86_vendor != X86_VENDOR_AMD) || + rdmsr_safe(MSR_AMD_PATCHLEVEL, *val) ) + goto gp_fault; + break; + case MSR_SPEC_CTRL: if ( !cp->feat.ibrsb ) goto gp_fault; @@ -236,6 +258,19 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val) /* Not offered to guests. */ goto gp_fault; + case MSR_AMD_PATCHLEVEL: + BUILD_BUG_ON(MSR_IA32_UCODE_REV != MSR_AMD_PATCHLEVEL); + /* + * AMD and Intel use the same MSR for the current microcode version. + * + * Both document it as read-only. However Intel also document that, + * for backwards compatiblity, the OS should write 0 to it before + * trying to access the current microcode version. + */ + if ( d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL || val != 0 ) + goto gp_fault; + break; + case MSR_AMD_PATCHLOADER: /* * See note on MSR_IA32_UCODE_WRITE below, which may or may not apply diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c index 84ce67c..a4f3ffc 100644 --- a/xen/arch/x86/pv/emul-priv-op.c +++ b/xen/arch/x86/pv/emul-priv-op.c @@ -893,17 +893,6 @@ static int read_msr(unsigned int reg, uint64_t *val, *val = 0; return X86EMUL_OKAY; - case MSR_IA32_UCODE_REV: - BUILD_BUG_ON(MSR_IA32_UCODE_REV != MSR_AMD_PATCHLEVEL); - if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) - { - if ( wrmsr_safe(MSR_IA32_UCODE_REV, 0) ) - break; - /* As documented in the SDM: Do a CPUID 1 here */ - cpuid_eax(1); - } - goto normal; - case MSR_IA32_MISC_ENABLE: rdmsrl(reg, *val); *val = guest_misc_enable(*val); @@ -1047,17 +1036,6 @@ static int write_msr(unsigned int reg, uint64_t val, return X86EMUL_OKAY; break; - case MSR_IA32_UCODE_REV: - if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ) - break; - if ( !is_hwdom_pinned_vcpu(curr) ) - return X86EMUL_OKAY; - if ( rdmsr_safe(reg, temp) ) - break; - if ( val ) - goto invalid; - return X86EMUL_OKAY; - case MSR_IA32_MISC_ENABLE: rdmsrl(reg, temp); if ( val != guest_misc_enable(temp) )