From patchwork Tue Apr 9 17:53:47 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Andrew Cooper X-Patchwork-Id: 10891911 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 2D48B1708 for ; Tue, 9 Apr 2019 17:56:30 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1A289288D1 for ; Tue, 9 Apr 2019 17:56:30 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 0DCCA2890C; Tue, 9 Apr 2019 17:56:30 +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 698F2288D1 for ; Tue, 9 Apr 2019 17:56:28 +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 1hDuwU-0006aj-Tb; Tue, 09 Apr 2019 17:53:54 +0000 Received: from us1-rack-dfw2.inumbo.com ([104.130.134.6]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1hDuwT-0006ae-JK for xen-devel@lists.xen.org; Tue, 09 Apr 2019 17:53:53 +0000 X-Inumbo-ID: 6ec78931-5af0-11e9-92d7-bc764e045a96 Received: from SMTP03.CITRIX.COM (unknown [162.221.156.55]) by us1-rack-dfw2.inumbo.com (Halon) with ESMTPS id 6ec78931-5af0-11e9-92d7-bc764e045a96; Tue, 09 Apr 2019 17:53:51 +0000 (UTC) X-IronPort-AV: E=Sophos;i="5.60,330,1549929600"; d="scan'208";a="83145568" From: Andrew Cooper To: Xen-devel Date: Tue, 9 Apr 2019 18:53:47 +0100 Message-ID: <1554832427-30567-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 fallout from mostly c/s 832c180 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: Kevin Tian , Wei Liu , Jan Beulich , Andrew Cooper , Paul Durrant , Jun Nakajima , =?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 The series 832c1803^..f61685a6 was committed without adequate review. * Fix the shim build by providing a !CONFIG_HVM declaration for hvm_get_guest_bndcfgs() * Revert the bogus de-const'ing of the vcpu pointer in vmx_get_guest_bndcfgs(). vmx_vmcs_enter() really does mutate the vcpu, and may cause it to undergo a full de/reschedule, which is in violation of the ABI described by hvm_get_guest_bndcfgs(). guest_rdmsr() was always going to need to lose its const parameter, and this was the correct time for it to happen. * Remove the introduced ASSERT(is_hvm_domain(d)) and check the predicate directly. While we expect it to be true, the result is potential type confusion in release builds based on several subtle aspects of the CPUID feature derivation logic with no other safety checks. This also fixes the a linker error in the release build of the shim, again for !CONFIG_HVM reasons. * The MSRs in vcpu_msrs are in numeric order. Re-position XSS to match. Signed-off-by: Andrew Cooper Reviewed-by: Paul Durrant --- CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné CC: Paul Durrant CC: Jun Nakajima CC: Kevin Tian There is a further bug which I haven't got a clean solution for yet. hvm_set_guest_bndcfgs() isn't always safe to use out of current context, and will fail the migration rather than making a remote adjustment to %xcr0. I'm leaning towards dropping this "just in case" logic, but ideally with clarification of the silicon behaviour. --- xen/arch/x86/hvm/vmx/vmx.c | 5 +---- xen/arch/x86/msr.c | 18 +++++------------- xen/arch/x86/pv/emul-priv-op.c | 2 +- xen/include/asm-x86/hvm/hvm.h | 5 +++-- xen/include/asm-x86/msr.h | 12 ++++++------ 5 files changed, 16 insertions(+), 26 deletions(-) diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index c46e05b..283eb7b 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -1150,11 +1150,8 @@ static bool vmx_set_guest_bndcfgs(struct vcpu *v, u64 val) return true; } -static bool vmx_get_guest_bndcfgs(const struct vcpu *cv, u64 *val) +static bool vmx_get_guest_bndcfgs(struct vcpu *v, u64 *val) { - /* Get a non-const pointer for vmx_vmcs_enter() */ - struct vcpu *v = cv->domain->vcpu[cv->vcpu_id]; - ASSERT(cpu_has_mpx && cpu_has_vmx_mpx); vmx_vmcs_enter(v); diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c index 815d599..0049a73 100644 --- a/xen/arch/x86/msr.c +++ b/xen/arch/x86/msr.c @@ -115,7 +115,7 @@ int init_vcpu_msr_policy(struct vcpu *v) return 0; } -int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val) +int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val) { const struct vcpu *curr = current; const struct domain *d = v->domain; @@ -182,13 +182,9 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val) break; case MSR_IA32_BNDCFGS: - if ( !cp->feat.mpx ) + if ( !cp->feat.mpx || !is_hvm_domain(d) || + !hvm_get_guest_bndcfgs(v, val) ) goto gp_fault; - - ASSERT(is_hvm_domain(d)); - if (!hvm_get_guest_bndcfgs(v, val) ) - goto gp_fault; - break; case MSR_IA32_XSS: @@ -375,13 +371,9 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val) break; case MSR_IA32_BNDCFGS: - if ( !cp->feat.mpx ) + if ( !cp->feat.mpx || !is_hvm_domain(d) || + !hvm_set_guest_bndcfgs(v, val) ) goto gp_fault; - - ASSERT(is_hvm_domain(d)); - if ( !hvm_set_guest_bndcfgs(v, val) ) - goto gp_fault; - break; case MSR_IA32_XSS: diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c index a55a400..af74f50 100644 --- a/xen/arch/x86/pv/emul-priv-op.c +++ b/xen/arch/x86/pv/emul-priv-op.c @@ -819,7 +819,7 @@ static inline bool is_cpufreq_controller(const struct domain *d) static int read_msr(unsigned int reg, uint64_t *val, struct x86_emulate_ctxt *ctxt) { - const struct vcpu *curr = current; + struct vcpu *curr = current; const struct domain *currd = curr->domain; bool vpmu_msr = false; int ret; diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h index c811fa9..157f0de 100644 --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -145,7 +145,7 @@ struct hvm_function_table { int (*get_guest_pat)(struct vcpu *v, u64 *); int (*set_guest_pat)(struct vcpu *v, u64); - bool (*get_guest_bndcfgs)(const struct vcpu *v, u64 *); + bool (*get_guest_bndcfgs)(struct vcpu *v, u64 *); bool (*set_guest_bndcfgs)(struct vcpu *v, u64); void (*set_tsc_offset)(struct vcpu *v, u64 offset, u64 at_tsc); @@ -444,7 +444,7 @@ static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v) return hvm_funcs.get_shadow_gs_base(v); } -static inline bool hvm_get_guest_bndcfgs(const struct vcpu *v, u64 *val) +static inline bool hvm_get_guest_bndcfgs(struct vcpu *v, u64 *val) { return hvm_funcs.get_guest_bndcfgs && hvm_funcs.get_guest_bndcfgs(v, val); @@ -692,6 +692,7 @@ unsigned long hvm_get_shadow_gs_base(struct vcpu *v); void hvm_set_info_guest(struct vcpu *v); void hvm_cpuid_policy_changed(struct vcpu *v); void hvm_set_tsc_offset(struct vcpu *v, uint64_t offset, uint64_t at_tsc); +bool hvm_get_guest_bndcfgs(struct vcpu *v, uint64_t *val); /* End of prototype list */ diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h index 0d52c08..3cbbc65 100644 --- a/xen/include/asm-x86/msr.h +++ b/xen/include/asm-x86/msr.h @@ -296,6 +296,11 @@ struct vcpu_msrs }; } misc_features_enables; + /* 0x00000da0 - MSR_IA32_XSS */ + struct { + uint64_t raw; + } xss; + /* * 0xc0000103 - MSR_TSC_AUX * @@ -313,11 +318,6 @@ struct vcpu_msrs * values here may be stale in current context. */ uint32_t dr_mask[4]; - - /* 0x00000da0 - MSR_IA32_XSS */ - struct { - uint64_t raw; - } xss; }; void init_guest_msr_policy(void); @@ -333,7 +333,7 @@ int init_vcpu_msr_policy(struct vcpu *v); * These functions are also used by the migration logic, so need to cope with * being used outside of v's context. */ -int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val); +int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val); int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val); #endif /* !__ASSEMBLY__ */