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 4d37a93c57a..4280acfc074 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3832,67 +3832,50 @@ 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 243c41fb13a..20591c4a44f 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -589,8 +589,7 @@ 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 82c55f49aea..eda99e268d1 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -803,8 +803,7 @@ 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.0On 11/03/2026 2:27 pm, Alejandro Vallejo wrote: > 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. These two things are at odds. Two patches please. The first which strips out is_cross_vendor() and initialises should_emulate to false, to be this patch in conjunction with the changes for the UD intercept. Then a subsequent patch to rearrange hvm_ud_intercept() to DCE some more in the !FEP case, which is no-functional-change. In fact, I've got half a mind to suggest 3 patches, with the middle patch being a strict un-indent of the the current "if ( FEP )" clause. I think that will make a very a surprisingly legible patch 3. The result will be much more coherent for future archaeologists to follow. ~Andrew
On 11.03.2026 15:27, Alejandro Vallejo wrote:
> 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
"Revert" as in "move it even farther away from the original". As said, you
want re-indentation, so please do just that, nothing else that isn't
explicitly justified (like the moving of hvm_emulate_init_once() is). With
this put back in its original shape (can do while committing, I suppose):
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3832,67 +3832,50 @@ 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:
I'm inclined to suggest to indent this the same as the case labels.
Jan
On Wed Mar 11, 2026 at 3:59 PM CET, Jan Beulich wrote:
> On 11.03.2026 15:27, Alejandro Vallejo wrote:
>> 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
>
> "Revert" as in "move it even farther away from the original".
Revert as in not split the assignment and restore the orignal syntax _of the
assignment_, which was the main focus of the prior discussion.
It's hardly my intention to add unrequested changes, but I can't address that
which isn't explicitly requested.
> As said, you want re-indentation,
This is an ambiguous piece of advice.
Of what? That can mean moving the prior logic back to its original location and
crate a minimal diff (1) or simply collapsing the indentation of the block (2).
(1) can't be done with hvm context initialiser moving after the early exit,
which I explicitly mentioned in the commit message I wanted to do.
(2) can't happen because declarations and statements cannot be mixed (though I
really wish we dropped that rule).
There's a third option of keeping a silly { ... } around just for indentation
purposes, but that's worse than either of the other 2 options.
Maybe there's a fourth code arrangement in your head that does all this in a
way you find less intrusive and I just don't see it. If so, feel free to send
a patch I can review. It'll be faster for the both of us. Or tell me precisely
what's at fault here.
If it's the diff, I'll go for option (1) above. I don't care enough about it to
argue.
> so please do just that, nothing else that isn't
> explicitly justified (like the moving of hvm_emulate_init_once() is).
I'm not sure if you're fine with that motion because it's in the commit message
or not because it's a refactor that shouldn't be in the patch. This statement
can be read either way.
> With
> this put back in its original shape (can do while committing, I suppose):
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
I don't think it's very obvious what you mean to do on commit, so it wouldn't be
appropriate to agree to your adjustments, seeing how I just don't know what they
are. I'm happy to send a v4.5 on this particular patch with whatever else needs
modifying. Or a full v5 even. Or review whatever you wish to send as a v4.5 of
this patch.
Your pick.
>> + reinject:
>
> I'm inclined to suggest to indent this the same as the case labels.
I didn't notice the extra statement in CODING_STYLE for labels inside switches.
I tend to do that myself, but thought it wasn't in Xen's style. Sounds good
then.
Cheers,
Alejandro
On 11.03.2026 19:01, Alejandro Vallejo wrote:
> On Wed Mar 11, 2026 at 3:59 PM CET, Jan Beulich wrote:
>> On 11.03.2026 15:27, Alejandro Vallejo wrote:
>>> 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
>>
>> "Revert" as in "move it even farther away from the original".
>
> Revert as in not split the assignment and restore the orignal syntax _of the
> assignment_, which was the main focus of the prior discussion.
>
> It's hardly my intention to add unrequested changes, but I can't address that
> which isn't explicitly requested.
>
>> As said, you want re-indentation,
>
> This is an ambiguous piece of advice.
>
> Of what? That can mean moving the prior logic back to its original location and
> crate a minimal diff (1) or simply collapsing the indentation of the block (2).
>
> (1) can't be done with hvm context initialiser moving after the early exit,
> which I explicitly mentioned in the commit message I wanted to do.
>
> (2) can't happen because declarations and statements cannot be mixed (though I
> really wish we dropped that rule).
>
> There's a third option of keeping a silly { ... } around just for indentation
> purposes, but that's worse than either of the other 2 options.
>
> Maybe there's a fourth code arrangement in your head that does all this in a
> way you find less intrusive and I just don't see it. If so, feel free to send
> a patch I can review. It'll be faster for the both of us. Or tell me precisely
> what's at fault here.
>
> If it's the diff, I'll go for option (1) above. I don't care enough about it to
> argue.
>
>> so please do just that, nothing else that isn't
>> explicitly justified (like the moving of hvm_emulate_init_once() is).
>
> I'm not sure if you're fine with that motion because it's in the commit message
> or not because it's a refactor that shouldn't be in the patch. This statement
> can be read either way.
You justify that movement in the description, and I agree with that justification.
>> With
>> this put back in its original shape (can do while committing, I suppose):
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> I don't think it's very obvious what you mean to do on commit, so it wouldn't be
> appropriate to agree to your adjustments, seeing how I just don't know what they
> are. I'm happy to send a v4.5 on this particular patch with whatever else needs
> modifying. Or a full v5 even. Or review whatever you wish to send as a v4.5 of
> this patch.
The variable had an initializer, and mere re-indentation wants to keep it so.
(There's no question that declarations may need to move, for the result to still
compile.)
Jan
© 2016 - 2026 Red Hat, Inc.