pipeline: https://gitlab.com/xen-project/people/agvallejo/xen/-/pipelines/2277124833 (pipeline differs with the CHANGELOG patch being separate. Nothing functional) As discussed in a prior RFC (https://lore.kernel.org/xen-devel/dc68b9ce-38aa-4949-b3e7-a7c0a7ee9a25@citrix.com/) this series drops cross-vendor support. It includes the policy check that was there and adds this on top: * Eliminates #UD handler when HVM_FEP is disabled. * Removes the cross-vendor checks from MSR handlers. * Eliminate Intel-behaviour hacks for SYSENTER on AMD handlers and drop intercept for SYSENTER. Open question unrelated to the series: Does it make sense to conditionalise the MSR handlers for non intercepted MSRs on HVM_FEP? Cheers, Alejandro Alejandro Vallejo (4): x86: Reject CPU policies with vendors other than the host's x86/hvm: Disable non-FEP cross-vendor handling in #UD handler x86/hvm: Remove cross-vendor checks from MSR handlers. x86/svm: Drop emulation of Intel's SYSENTER behaviour CHANGELOG.md | 4 +++ xen/arch/x86/hvm/hvm.c | 25 +++---------- xen/arch/x86/hvm/svm/svm.c | 46 +++++++++++------------- xen/arch/x86/hvm/svm/vmcb.c | 3 ++ xen/arch/x86/hvm/vmx/vmx.c | 4 +-- xen/arch/x86/include/asm/hvm/svm-types.h | 10 ------ xen/arch/x86/msr.c | 6 ++-- xen/lib/x86/policy.c | 3 +- 8 files changed, 38 insertions(+), 63 deletions(-) base-commit: 3001d9a19592bb4f12dab33f161ab2148513e30a -- 2.43.0 While in principle it's possible to have a vendor virtualising another, this is fairly tricky in practice and comes with the world's supply of security issues. Reject any CPU policy with vendors not matching the host's. Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com> --- CHANGELOG.md | 4 ++++ xen/lib/x86/policy.c | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index XXXXXXX..XXXXXXX 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -XXX,XX +XXX,XX @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Xenoprofile support. Oprofile themselves removed support for Xen in 2014 prior to the version 1.0 release, and there has been no development since before then in Xen. + - Cross-vendor support. Refuse to start domains whose CPU vendor differs + from the host so that security mitigations stay consistent. Cross-vendor + setups have been unreliable and not practical since 2017 with the advent of + speculation security. - Removed xenpm tool on non-x86 platforms as it doesn't actually provide anything useful outside of x86. diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c index XXXXXXX..XXXXXXX 100644 --- a/xen/lib/x86/policy.c +++ b/xen/lib/x86/policy.c @@ -XXX,XX +XXX,XX @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host, #define FAIL_MSR(m) \ do { e.msr = (m); goto out; } while ( 0 ) - if ( guest->basic.max_leaf > host->basic.max_leaf ) + if ( (guest->basic.max_leaf > host->basic.max_leaf) || + (guest->x86_vendor != host->x86_vendor) ) FAIL_CPUID(0, NA); if ( guest->feat.max_subleaf > host->feat.max_subleaf ) -- 2.43.0 Remove cross-vendor support now that VMs can no longer have a different vendor than the host, leaving FEP as the sole raison-d'être for #UD interception. Not a functional change. Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com> --- xen/arch/x86/hvm/hvm.c | 25 ++++--------------------- xen/arch/x86/hvm/svm/svm.c | 4 ++-- xen/arch/x86/hvm/vmx/vmx.c | 4 ++-- 3 files changed, 8 insertions(+), 25 deletions(-) diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index XXXXXXX..XXXXXXX 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -XXX,XX +XXX,XX @@ int hvm_descriptor_access_intercept(uint64_t exit_info, return X86EMUL_OKAY; } -static bool cf_check is_cross_vendor( - const struct x86_emulate_state *state, const struct x86_emulate_ctxt *ctxt) -{ - switch ( ctxt->opcode ) - { - case X86EMUL_OPC(0x0f, 0x05): /* syscall */ - case X86EMUL_OPC(0x0f, 0x34): /* sysenter */ - case X86EMUL_OPC(0x0f, 0x35): /* sysexit */ - return true; - } - - return false; -} - +#ifdef CONFIG_HVM_FEP void hvm_ud_intercept(struct cpu_user_regs *regs) { struct vcpu *cur = current; - bool should_emulate = - cur->domain->arch.cpuid->x86_vendor != boot_cpu_data.x86_vendor; struct hvm_emulate_ctxt ctxt; - hvm_emulate_init_once(&ctxt, opt_hvm_fep ? NULL : is_cross_vendor, regs); + hvm_emulate_init_once(&ctxt, NULL, regs); if ( opt_hvm_fep ) { @@ -XXX,XX +XXX,XX @@ void hvm_ud_intercept(struct cpu_user_regs *regs) regs->rip = (uint32_t)regs->rip; add_taint(TAINT_HVM_FEP); - - should_emulate = true; } } - - if ( !should_emulate ) + else { hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC); return; @@ -XXX,XX +XXX,XX @@ void hvm_ud_intercept(struct cpu_user_regs *regs) break; } } +#endif /* CONFIG_HVM_FEP */ enum hvm_intblk hvm_interrupt_blocked(struct vcpu *v, struct hvm_intack intack) { diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index XXXXXXX..XXXXXXX 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -XXX,XX +XXX,XX @@ static void cf_check svm_cpuid_policy_changed(struct vcpu *v) const struct cpu_policy *cp = v->domain->arch.cpu_policy; u32 bitmap = vmcb_get_exception_intercepts(vmcb); - if ( opt_hvm_fep || - (v->domain->arch.cpuid->x86_vendor != boot_cpu_data.x86_vendor) ) + if ( opt_hvm_fep ) bitmap |= (1U << X86_EXC_UD); else bitmap &= ~(1U << X86_EXC_UD); @@ -XXX,XX +XXX,XX @@ void asmlinkage svm_vmexit_handler(void) break; case VMEXIT_EXCEPTION_UD: + BUG_ON(!IS_ENABLED(CONFIG_HVM_FEP)); hvm_ud_intercept(regs); break; diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index XXXXXXX..XXXXXXX 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -XXX,XX +XXX,XX @@ static void cf_check vmx_cpuid_policy_changed(struct vcpu *v) const struct cpu_policy *cp = v->domain->arch.cpu_policy; int rc = 0; - if ( opt_hvm_fep || - (v->domain->arch.cpuid->x86_vendor != boot_cpu_data.x86_vendor) ) + if ( opt_hvm_fep ) v->arch.hvm.vmx.exception_bitmap |= (1U << X86_EXC_UD); else v->arch.hvm.vmx.exception_bitmap &= ~(1U << X86_EXC_UD); @@ -XXX,XX +XXX,XX @@ void asmlinkage vmx_vmexit_handler(struct cpu_user_regs *regs) /* Already handled above. */ break; case X86_EXC_UD: + BUG_ON(!IS_ENABLED(CONFIG_HVM_FEP)); TRACE(TRC_HVM_TRAP, vector); hvm_ud_intercept(regs); break; -- 2.43.0 Not a functional change now that cross-vendor guests are not launchable. Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com> --- xen/arch/x86/msr.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c index XXXXXXX..XXXXXXX 100644 --- a/xen/arch/x86/msr.c +++ b/xen/arch/x86/msr.c @@ -XXX,XX +XXX,XX @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val) break; case MSR_IA32_PLATFORM_ID: - if ( !(cp->x86_vendor & X86_VENDOR_INTEL) || - !(boot_cpu_data.x86_vendor & X86_VENDOR_INTEL) ) + if ( cp->x86_vendor != X86_VENDOR_INTEL ) goto gp_fault; + rdmsrl(MSR_IA32_PLATFORM_ID, *val); break; @@ -XXX,XX +XXX,XX @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val) * the guest. */ if ( !(cp->x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_AMD)) || - !(boot_cpu_data.x86_vendor & - (X86_VENDOR_INTEL | X86_VENDOR_AMD)) || rdmsr_safe(MSR_AMD_PATCHLEVEL, val) ) goto gp_fault; break; -- 2.43.0 With cross-vendor support gone, it's no longer needed. Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com> --- xen/arch/x86/hvm/svm/svm.c | 42 +++++++++++------------- xen/arch/x86/hvm/svm/vmcb.c | 3 ++ xen/arch/x86/include/asm/hvm/svm-types.h | 10 ------ 3 files changed, 22 insertions(+), 33 deletions(-) diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index XXXXXXX..XXXXXXX 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -XXX,XX +XXX,XX @@ static int svm_vmcb_save(struct vcpu *v, struct hvm_hw_cpu *c) { struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb; - c->sysenter_cs = v->arch.hvm.svm.guest_sysenter_cs; - c->sysenter_esp = v->arch.hvm.svm.guest_sysenter_esp; - c->sysenter_eip = v->arch.hvm.svm.guest_sysenter_eip; - if ( vmcb->event_inj.v && hvm_event_needs_reinjection(vmcb->event_inj.type, vmcb->event_inj.vector) ) @@ -XXX,XX +XXX,XX @@ static int svm_vmcb_restore(struct vcpu *v, struct hvm_hw_cpu *c) svm_update_guest_cr(v, 0, 0); svm_update_guest_cr(v, 4, 0); - /* Load sysenter MSRs into both VMCB save area and VCPU fields. */ - vmcb->sysenter_cs = v->arch.hvm.svm.guest_sysenter_cs = c->sysenter_cs; - vmcb->sysenter_esp = v->arch.hvm.svm.guest_sysenter_esp = c->sysenter_esp; - vmcb->sysenter_eip = v->arch.hvm.svm.guest_sysenter_eip = c->sysenter_eip; - if ( paging_mode_hap(v->domain) ) { vmcb_set_np(vmcb, true); @@ -XXX,XX +XXX,XX @@ static void svm_save_cpu_state(struct vcpu *v, struct hvm_hw_cpu *data) { struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb; + data->sysenter_cs = vmcb->sysenter_cs; + data->sysenter_esp = vmcb->sysenter_esp; + data->sysenter_eip = vmcb->sysenter_eip; data->shadow_gs = vmcb->kerngsbase; data->msr_lstar = vmcb->lstar; data->msr_star = vmcb->star; @@ -XXX,XX +XXX,XX @@ static void svm_load_cpu_state(struct vcpu *v, struct hvm_hw_cpu *data) { struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb; - vmcb->kerngsbase = data->shadow_gs; - vmcb->lstar = data->msr_lstar; - vmcb->star = data->msr_star; - vmcb->cstar = data->msr_cstar; - vmcb->sfmask = data->msr_syscall_mask; + vmcb->sysenter_cs = data->sysenter_cs; + vmcb->sysenter_esp = data->sysenter_esp; + vmcb->sysenter_eip = data->sysenter_eip; + vmcb->kerngsbase = data->shadow_gs; + vmcb->lstar = data->msr_lstar; + vmcb->star = data->msr_star; + vmcb->cstar = data->msr_cstar; + vmcb->sfmask = data->msr_syscall_mask; v->arch.hvm.guest_efer = data->msr_efer; svm_update_guest_efer(v); } @@ -XXX,XX +XXX,XX @@ static int cf_check svm_msr_read_intercept( switch ( msr ) { - /* - * Sync not needed while the cross-vendor logic is in unilateral effect. case MSR_IA32_SYSENTER_CS: case MSR_IA32_SYSENTER_ESP: case MSR_IA32_SYSENTER_EIP: - */ case MSR_STAR: case MSR_LSTAR: case MSR_CSTAR: @@ -XXX,XX +XXX,XX @@ static int cf_check svm_msr_read_intercept( switch ( msr ) { case MSR_IA32_SYSENTER_CS: - *msr_content = v->arch.hvm.svm.guest_sysenter_cs; + *msr_content = vmcb->sysenter_cs; break; + case MSR_IA32_SYSENTER_ESP: - *msr_content = v->arch.hvm.svm.guest_sysenter_esp; + *msr_content = vmcb->sysenter_esp; break; + case MSR_IA32_SYSENTER_EIP: - *msr_content = v->arch.hvm.svm.guest_sysenter_eip; + *msr_content = vmcb->sysenter_eip; break; case MSR_STAR: @@ -XXX,XX +XXX,XX @@ static int cf_check svm_msr_write_intercept( switch ( msr ) { case MSR_IA32_SYSENTER_ESP: - vmcb->sysenter_esp = v->arch.hvm.svm.guest_sysenter_esp = msr_content; + vmcb->sysenter_esp = msr_content; break; case MSR_IA32_SYSENTER_EIP: - vmcb->sysenter_eip = v->arch.hvm.svm.guest_sysenter_eip = msr_content; + vmcb->sysenter_eip = msr_content; break; case MSR_LSTAR: @@ -XXX,XX +XXX,XX @@ static int cf_check svm_msr_write_intercept( break; case MSR_IA32_SYSENTER_CS: - vmcb->sysenter_cs = v->arch.hvm.svm.guest_sysenter_cs = msr_content; + vmcb->sysenter_cs = msr_content; break; case MSR_STAR: diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c index XXXXXXX..XXXXXXX 100644 --- a/xen/arch/x86/hvm/svm/vmcb.c +++ b/xen/arch/x86/hvm/svm/vmcb.c @@ -XXX,XX +XXX,XX @@ static int construct_vmcb(struct vcpu *v) svm_disable_intercept_for_msr(v, MSR_LSTAR); svm_disable_intercept_for_msr(v, MSR_STAR); svm_disable_intercept_for_msr(v, MSR_SYSCALL_MASK); + svm_disable_intercept_for_msr(v, MSR_IA32_SYSENTER_CS); + svm_disable_intercept_for_msr(v, MSR_IA32_SYSENTER_EIP); + svm_disable_intercept_for_msr(v, MSR_IA32_SYSENTER_ESP); vmcb->_msrpm_base_pa = virt_to_maddr(svm->msrpm); vmcb->_iopm_base_pa = __pa(v->domain->arch.hvm.io_bitmap); diff --git a/xen/arch/x86/include/asm/hvm/svm-types.h b/xen/arch/x86/include/asm/hvm/svm-types.h index XXXXXXX..XXXXXXX 100644 --- a/xen/arch/x86/include/asm/hvm/svm-types.h +++ b/xen/arch/x86/include/asm/hvm/svm-types.h @@ -XXX,XX +XXX,XX @@ struct svm_vcpu { /* VMCB has a cached instruction from #PF/#NPF Decode Assist? */ uint8_t cached_insn_len; /* Zero if no cached instruction. */ - - /* - * Upper four bytes are undefined in the VMCB, therefore we can't use the - * fields in the VMCB. Write a 64bit value and then read a 64bit value is - * fine unless there's a VMRUN/VMEXIT in between which clears the upper - * four bytes. - */ - uint64_t guest_sysenter_cs; - uint64_t guest_sysenter_esp; - uint64_t guest_sysenter_eip; }; struct nestedsvm { -- 2.43.0 Hi, Only patches 1 and 2 missing acks. v1: https://lore.kernel.org/xen-devel/20260122164943.20691-1-alejandro.garciavallejo@amd.com/ v2: https://lore.kernel.org/xen-devel/20260205170923.38425-1-alejandro.garciavallejo@amd.com/ v3: https://lore.kernel.org/xen-devel/20260213114232.42996-1-alejandro.garciavallejo@amd.com/ pipeline (green): https://gitlab.com/xen-project/people/agvallejo/xen/-/pipelines/2378378894 Cheers, Alejandro Alejandro Vallejo (4): x86: Reject CPU policies with vendors other than the host's x86/hvm: Disable cross-vendor handling in #UD handler x86/hvm: Remove cross-vendor checks from MSR handlers. x86/svm: Drop emulation of Intel's SYSENTER behaviour on AMD systems CHANGELOG.md | 5 ++ tools/tests/cpu-policy/test-cpu-policy.c | 27 +++++++++ xen/arch/x86/hvm/hvm.c | 73 +++++++++--------------- xen/arch/x86/hvm/svm/svm.c | 45 +++++++-------- xen/arch/x86/hvm/svm/vmcb.c | 3 + xen/arch/x86/hvm/vmx/vmx.c | 3 +- xen/arch/x86/include/asm/hvm/svm-types.h | 10 ---- xen/arch/x86/lib/cpu-policy/policy.c | 5 +- xen/arch/x86/msr.c | 8 +-- 9 files changed, 91 insertions(+), 88 deletions(-) base-commit: bfb33fa6d4eb4110cd7dd47ec71d2e550739e126 -- 2.43.0 While in principle it's possible to have a vendor virtualising another, this is fairly tricky in practice and comes with the world's supply of security issues. Reject any CPU policy with vendors not matching the host's. Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com> --- v4: * Adjusted CHANGELOG --- CHANGELOG.md | 5 +++++ tools/tests/cpu-policy/test-cpu-policy.c | 27 ++++++++++++++++++++++++ xen/arch/x86/lib/cpu-policy/policy.c | 5 ++++- 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index XXXXXXX..XXXXXXX 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -XXX,XX +XXX,XX @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Xenoprofile support. Oprofile themselves removed support for Xen in 2014 prior to the version 1.0 release, and there has been no development since before then in Xen. + - Domains can no longer run on a system with CPUs of a vendor different from + the one they were initially launched on. This affects live migrations and + save/restore workflows across mixed-vendor hosts. Cross-vendor emulation + has always been unreliable, but since 2017 with the advent of speculation + security it became unsustainably so. - Removed xenpm tool on non-x86 platforms as it doesn't actually provide anything useful outside of x86. diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c index XXXXXXX..XXXXXXX 100644 --- a/tools/tests/cpu-policy/test-cpu-policy.c +++ b/tools/tests/cpu-policy/test-cpu-policy.c @@ -XXX,XX +XXX,XX @@ static void test_is_compatible_success(void) .platform_info.cpuid_faulting = true, }, }, + { + .name = "Host CPU vendor == Guest CPU vendor (both unknown)", + .host = { + .basic.vendor_ebx = X86_VENDOR_AMD_EBX + 1, + .basic.vendor_ecx = X86_VENDOR_AMD_ECX, + .basic.vendor_edx = X86_VENDOR_AMD_EDX, + }, + .guest = { + .basic.vendor_ebx = X86_VENDOR_AMD_EBX + 1, + .basic.vendor_ecx = X86_VENDOR_AMD_ECX, + .basic.vendor_edx = X86_VENDOR_AMD_EDX, + }, + }, }; struct cpu_policy_errors no_errors = INIT_CPU_POLICY_ERRORS; @@ -XXX,XX +XXX,XX @@ static void test_is_compatible_failure(void) }, .e = { -1, -1, 0xce }, }, + { + .name = "Host CPU vendor != Guest CPU vendor (both unknown)", + .host = { + .basic.vendor_ebx = X86_VENDOR_AMD_EBX + 1, + .basic.vendor_ecx = X86_VENDOR_AMD_ECX, + .basic.vendor_edx = X86_VENDOR_AMD_EDX, + }, + .guest = { + .basic.vendor_ebx = X86_VENDOR_AMD_EBX + 2, + .basic.vendor_ecx = X86_VENDOR_AMD_ECX, + .basic.vendor_edx = X86_VENDOR_AMD_EDX, + }, + .e = { 0, -1, -1 }, + }, }; printf("Testing policy compatibility failure:\n"); diff --git a/xen/arch/x86/lib/cpu-policy/policy.c b/xen/arch/x86/lib/cpu-policy/policy.c index XXXXXXX..XXXXXXX 100644 --- a/xen/arch/x86/lib/cpu-policy/policy.c +++ b/xen/arch/x86/lib/cpu-policy/policy.c @@ -XXX,XX +XXX,XX @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host, #define FAIL_MSR(m) \ do { e.msr = (m); goto out; } while ( 0 ) - if ( guest->basic.max_leaf > host->basic.max_leaf ) + if ( (guest->basic.vendor_ebx != host->basic.vendor_ebx) || + (guest->basic.vendor_ecx != host->basic.vendor_ecx) || + (guest->basic.vendor_edx != host->basic.vendor_edx) || + (guest->basic.max_leaf > host->basic.max_leaf) ) FAIL_CPUID(0, NA); if ( guest->feat.max_subleaf > host->feat.max_subleaf ) -- 2.43.0 Remove cross-vendor support now that VMs can no longer have a different vendor than the host. While at it, refactor the function to exit early and skip initialising the emulation context when FEP is not enabled. No functional change intended. Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com> --- v4: * Reverted refactor of the `walk` variable assignment * Added ASSERT_UNREACHABLE() to the !hvm_fep path. * Moved the `reinject` label to the UNIMPLEMENTED case in the emulator result handler. --- xen/arch/x86/hvm/hvm.c | 73 +++++++++++++++----------------------- xen/arch/x86/hvm/svm/svm.c | 3 +- xen/arch/x86/hvm/vmx/vmx.c | 3 +- 3 files changed, 30 insertions(+), 49 deletions(-) diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index XXXXXXX..XXXXXXX 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -XXX,XX +XXX,XX @@ int hvm_descriptor_access_intercept(uint64_t exit_info, return X86EMUL_OKAY; } -static bool cf_check is_cross_vendor( - const struct x86_emulate_state *state, const struct x86_emulate_ctxt *ctxt) -{ - switch ( ctxt->opcode ) - { - case X86EMUL_OPC(0x0f, 0x05): /* syscall */ - case X86EMUL_OPC(0x0f, 0x34): /* sysenter */ - case X86EMUL_OPC(0x0f, 0x35): /* sysexit */ - return true; - } - - return false; -} - void hvm_ud_intercept(struct cpu_user_regs *regs) { struct vcpu *cur = current; - bool should_emulate = - cur->domain->arch.cpuid->x86_vendor != boot_cpu_data.x86_vendor; struct hvm_emulate_ctxt ctxt; + const struct segment_register *cs = &ctxt.seg_reg[x86_seg_cs]; + uint32_t walk; + unsigned long addr; + char sig[5]; /* ud2; .ascii "xen" */ - hvm_emulate_init_once(&ctxt, opt_hvm_fep ? NULL : is_cross_vendor, regs); - - if ( opt_hvm_fep ) + if ( !opt_hvm_fep ) { - const struct segment_register *cs = &ctxt.seg_reg[x86_seg_cs]; - uint32_t walk = ((ctxt.seg_reg[x86_seg_ss].dpl == 3) - ? PFEC_user_mode : 0) | PFEC_insn_fetch; - unsigned long addr; - char sig[5]; /* ud2; .ascii "xen" */ - - if ( hvm_virtual_to_linear_addr(x86_seg_cs, cs, regs->rip, - sizeof(sig), hvm_access_insn_fetch, - cs, &addr) && - (hvm_copy_from_guest_linear(sig, addr, sizeof(sig), - walk, NULL) == HVMTRANS_okay) && - (memcmp(sig, "\xf\xb" "xen", sizeof(sig)) == 0) ) - { - regs->rip += sizeof(sig); - regs->eflags &= ~X86_EFLAGS_RF; - - /* Zero the upper 32 bits of %rip if not in 64bit mode. */ - if ( !(hvm_long_mode_active(cur) && cs->l) ) - regs->rip = (uint32_t)regs->rip; + ASSERT_UNREACHABLE(); + goto reinject; + } - add_taint(TAINT_HVM_FEP); + hvm_emulate_init_once(&ctxt, NULL, regs); - should_emulate = true; - } - } + walk = ((ctxt.seg_reg[x86_seg_ss].dpl == 3) + ? PFEC_user_mode : 0) | PFEC_insn_fetch; - if ( !should_emulate ) + if ( hvm_virtual_to_linear_addr(x86_seg_cs, cs, regs->rip, + sizeof(sig), hvm_access_insn_fetch, + cs, &addr) && + (hvm_copy_from_guest_linear(sig, addr, sizeof(sig), + walk, NULL) == HVMTRANS_okay) && + (memcmp(sig, "\xf\xb" "xen", sizeof(sig)) == 0) ) { - hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC); - return; + regs->rip += sizeof(sig); + regs->eflags &= ~X86_EFLAGS_RF; + + /* Zero the upper 32 bits of %rip if not in 64bit mode. */ + if ( !(hvm_long_mode_active(cur) && cs->l) ) + regs->rip = (uint32_t)regs->rip; + + add_taint(TAINT_HVM_FEP); } + else + goto reinject; switch ( hvm_emulate_one(&ctxt, VIO_no_completion) ) { case X86EMUL_UNHANDLEABLE: case X86EMUL_UNIMPLEMENTED: + reinject: hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC); break; case X86EMUL_EXCEPTION: diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index XXXXXXX..XXXXXXX 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -XXX,XX +XXX,XX @@ static void cf_check svm_cpuid_policy_changed(struct vcpu *v) const struct cpu_policy *cp = v->domain->arch.cpu_policy; u32 bitmap = vmcb_get_exception_intercepts(vmcb); - if ( opt_hvm_fep || - (v->domain->arch.cpuid->x86_vendor != boot_cpu_data.x86_vendor) ) + if ( opt_hvm_fep ) bitmap |= (1U << X86_EXC_UD); else bitmap &= ~(1U << X86_EXC_UD); diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index XXXXXXX..XXXXXXX 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -XXX,XX +XXX,XX @@ static void cf_check vmx_cpuid_policy_changed(struct vcpu *v) const struct cpu_policy *cp = v->domain->arch.cpu_policy; int rc = 0; - if ( opt_hvm_fep || - (v->domain->arch.cpuid->x86_vendor != boot_cpu_data.x86_vendor) ) + if ( opt_hvm_fep ) v->arch.hvm.vmx.exception_bitmap |= (1U << X86_EXC_UD); else v->arch.hvm.vmx.exception_bitmap &= ~(1U << X86_EXC_UD); -- 2.43.0 Not a functional change now that cross-vendor guests are not launchable. Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com> Reviewed-by: Teddy Astie <teddy.astie@vates.tech> Acked-by: Jan Beulich <jbeulich@suse.com> --- xen/arch/x86/msr.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c index XXXXXXX..XXXXXXX 100644 --- a/xen/arch/x86/msr.c +++ b/xen/arch/x86/msr.c @@ -XXX,XX +XXX,XX @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val) break; case MSR_IA32_PLATFORM_ID: - if ( !(cp->x86_vendor & X86_VENDOR_INTEL) || - !(boot_cpu_data.vendor & X86_VENDOR_INTEL) ) + if ( boot_cpu_data.vendor != X86_VENDOR_INTEL ) goto gp_fault; + rdmsrl(MSR_IA32_PLATFORM_ID, *val); break; @@ -XXX,XX +XXX,XX @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val) * from Xen's last microcode load, which can be forwarded straight to * the guest. */ - if ( !(cp->x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_AMD)) || - !(boot_cpu_data.vendor & - (X86_VENDOR_INTEL | X86_VENDOR_AMD)) || + if ( !(boot_cpu_data.vendor & (X86_VENDOR_INTEL | X86_VENDOR_AMD)) || rdmsr_safe(MSR_AMD_PATCHLEVEL, val) ) goto gp_fault; break; -- 2.43.0 With cross-vendor support gone, it's no longer needed. AMD CPUs ignore the top 32 bits of the SYSENTER/SYSEXIT MSRs, which is not how this emulation worked due to the need for cross-vendor support. Any AMD VMs storing state in the top 32bits of the SEP MSRs will lose it. It's very unlikely to affect any production VM because having 64bit width just isn't how real AMD CPUs behave. Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com> Reviewed-by: Teddy Astie <teddy.astie@vates.tech> Acked-by: Jan Beulich <jbeulich@suse.com> --- v4: * Sorted assignments to the vmcb struct by dst address. --- xen/arch/x86/hvm/svm/svm.c | 42 +++++++++++------------- xen/arch/x86/hvm/svm/vmcb.c | 3 ++ xen/arch/x86/include/asm/hvm/svm-types.h | 10 ------ 3 files changed, 22 insertions(+), 33 deletions(-) diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index XXXXXXX..XXXXXXX 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -XXX,XX +XXX,XX @@ static int svm_vmcb_save(struct vcpu *v, struct hvm_hw_cpu *c) { struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb; - c->sysenter_cs = v->arch.hvm.svm.guest_sysenter_cs; - c->sysenter_esp = v->arch.hvm.svm.guest_sysenter_esp; - c->sysenter_eip = v->arch.hvm.svm.guest_sysenter_eip; - if ( vmcb->event_inj.v && hvm_event_needs_reinjection(vmcb->event_inj.type, vmcb->event_inj.vector) ) @@ -XXX,XX +XXX,XX @@ static int svm_vmcb_restore(struct vcpu *v, struct hvm_hw_cpu *c) svm_update_guest_cr(v, 0, 0); svm_update_guest_cr(v, 4, 0); - /* Load sysenter MSRs into both VMCB save area and VCPU fields. */ - vmcb->sysenter_cs = v->arch.hvm.svm.guest_sysenter_cs = c->sysenter_cs; - vmcb->sysenter_esp = v->arch.hvm.svm.guest_sysenter_esp = c->sysenter_esp; - vmcb->sysenter_eip = v->arch.hvm.svm.guest_sysenter_eip = c->sysenter_eip; - if ( paging_mode_hap(v->domain) ) { vmcb_set_np(vmcb, true); @@ -XXX,XX +XXX,XX @@ static void svm_save_cpu_state(struct vcpu *v, struct hvm_hw_cpu *data) { struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb; + data->sysenter_cs = vmcb->sysenter_cs; + data->sysenter_esp = vmcb->sysenter_esp; + data->sysenter_eip = vmcb->sysenter_eip; data->shadow_gs = vmcb->kerngsbase; data->msr_lstar = vmcb->lstar; data->msr_star = vmcb->star; @@ -XXX,XX +XXX,XX @@ static void svm_load_cpu_state(struct vcpu *v, struct hvm_hw_cpu *data) { struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb; - vmcb->kerngsbase = data->shadow_gs; - vmcb->lstar = data->msr_lstar; - vmcb->star = data->msr_star; - vmcb->cstar = data->msr_cstar; - vmcb->sfmask = data->msr_syscall_mask; + vmcb->lstar = data->msr_lstar; + vmcb->star = data->msr_star; + vmcb->cstar = data->msr_cstar; + vmcb->sfmask = data->msr_syscall_mask; + vmcb->kerngsbase = data->shadow_gs; + vmcb->sysenter_cs = data->sysenter_cs; + vmcb->sysenter_esp = data->sysenter_esp; + vmcb->sysenter_eip = data->sysenter_eip; v->arch.hvm.guest_efer = data->msr_efer; svm_update_guest_efer(v); } @@ -XXX,XX +XXX,XX @@ static int cf_check svm_msr_read_intercept( switch ( msr ) { - /* - * Sync not needed while the cross-vendor logic is in unilateral effect. case MSR_IA32_SYSENTER_CS: case MSR_IA32_SYSENTER_ESP: case MSR_IA32_SYSENTER_EIP: - */ case MSR_STAR: case MSR_LSTAR: case MSR_CSTAR: @@ -XXX,XX +XXX,XX @@ static int cf_check svm_msr_read_intercept( switch ( msr ) { case MSR_IA32_SYSENTER_CS: - *msr_content = v->arch.hvm.svm.guest_sysenter_cs; + *msr_content = vmcb->sysenter_cs; break; + case MSR_IA32_SYSENTER_ESP: - *msr_content = v->arch.hvm.svm.guest_sysenter_esp; + *msr_content = vmcb->sysenter_esp; break; + case MSR_IA32_SYSENTER_EIP: - *msr_content = v->arch.hvm.svm.guest_sysenter_eip; + *msr_content = vmcb->sysenter_eip; break; case MSR_STAR: @@ -XXX,XX +XXX,XX @@ static int cf_check svm_msr_write_intercept( switch ( msr ) { case MSR_IA32_SYSENTER_ESP: - vmcb->sysenter_esp = v->arch.hvm.svm.guest_sysenter_esp = msr_content; + vmcb->sysenter_esp = msr_content; break; case MSR_IA32_SYSENTER_EIP: - vmcb->sysenter_eip = v->arch.hvm.svm.guest_sysenter_eip = msr_content; + vmcb->sysenter_eip = msr_content; break; case MSR_LSTAR: @@ -XXX,XX +XXX,XX @@ static int cf_check svm_msr_write_intercept( break; case MSR_IA32_SYSENTER_CS: - vmcb->sysenter_cs = v->arch.hvm.svm.guest_sysenter_cs = msr_content; + vmcb->sysenter_cs = msr_content; break; case MSR_STAR: diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c index XXXXXXX..XXXXXXX 100644 --- a/xen/arch/x86/hvm/svm/vmcb.c +++ b/xen/arch/x86/hvm/svm/vmcb.c @@ -XXX,XX +XXX,XX @@ static int construct_vmcb(struct vcpu *v) svm_disable_intercept_for_msr(v, MSR_LSTAR); svm_disable_intercept_for_msr(v, MSR_STAR); svm_disable_intercept_for_msr(v, MSR_SYSCALL_MASK); + svm_disable_intercept_for_msr(v, MSR_IA32_SYSENTER_CS); + svm_disable_intercept_for_msr(v, MSR_IA32_SYSENTER_EIP); + svm_disable_intercept_for_msr(v, MSR_IA32_SYSENTER_ESP); vmcb->_msrpm_base_pa = virt_to_maddr(svm->msrpm); vmcb->_iopm_base_pa = __pa(v->domain->arch.hvm.io_bitmap); diff --git a/xen/arch/x86/include/asm/hvm/svm-types.h b/xen/arch/x86/include/asm/hvm/svm-types.h index XXXXXXX..XXXXXXX 100644 --- a/xen/arch/x86/include/asm/hvm/svm-types.h +++ b/xen/arch/x86/include/asm/hvm/svm-types.h @@ -XXX,XX +XXX,XX @@ struct svm_vcpu { /* VMCB has a cached instruction from #PF/#NPF Decode Assist? */ uint8_t cached_insn_len; /* Zero if no cached instruction. */ - - /* - * Upper four bytes are undefined in the VMCB, therefore we can't use the - * fields in the VMCB. Write a 64bit value and then read a 64bit value is - * fine unless there's a VMRUN/VMEXIT in between which clears the upper - * four bytes. - */ - uint64_t guest_sysenter_cs; - uint64_t guest_sysenter_esp; - uint64_t guest_sysenter_eip; }; struct nestedsvm { -- 2.43.0