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>
---
v3:
* No changes
v2:
* Fix bug introduced in v1: Don't emulate instructions when they
shouldn't be emulated.
* Refactor the function so it's simpler.
---
xen/arch/x86/hvm/hvm.c | 77 +++++++++++++++-----------------------
xen/arch/x86/hvm/svm/svm.c | 3 +-
xen/arch/x86/hvm/vmx/vmx.c | 3 +-
3 files changed, 32 insertions(+), 51 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 4d37a93c57..8708af9425 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3832,69 +3832,47 @@ 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 = PFEC_insn_fetch;
+ 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 )
+ goto reinject;
- 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;
+ hvm_emulate_init_once(&ctxt, NULL, regs);
- /* 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;
+ if ( ctxt.seg_reg[x86_seg_ss].dpl == 3 )
+ walk |= PFEC_user_mode;
- add_taint(TAINT_HVM_FEP);
+ 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;
- should_emulate = true;
- }
- }
+ /* 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;
- if ( !should_emulate )
- {
- hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC);
- return;
+ add_taint(TAINT_HVM_FEP);
}
+ else
+ goto reinject;
switch ( hvm_emulate_one(&ctxt, VIO_no_completion) )
{
case X86EMUL_UNHANDLEABLE:
case X86EMUL_UNIMPLEMENTED:
- hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC);
- break;
+ goto reinject;
case X86EMUL_EXCEPTION:
hvm_inject_event(&ctxt.ctxt.event);
/* fall through */
@@ -3902,6 +3880,11 @@ void hvm_ud_intercept(struct cpu_user_regs *regs)
hvm_emulate_writeback(&ctxt);
break;
}
+
+ return;
+
+ reinject:
+ hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC);
}
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 18ba837738..10d1bf350c 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 82c55f49ae..eda99e268d 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 13.02.2026 12:42, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3832,69 +3832,47 @@ 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 = PFEC_insn_fetch;
> + 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 )
> + goto reinject;
Is this possible at all, i.e. shouldn't there be ASSERT_UNREACHABLE() in
addition if already the check is kept?
> - 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;
Why is this initializer not retained?
> - 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;
> + hvm_emulate_init_once(&ctxt, NULL, regs);
>
> - /* 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;
> + if ( ctxt.seg_reg[x86_seg_ss].dpl == 3 )
> + walk |= PFEC_user_mode;
>
> - add_taint(TAINT_HVM_FEP);
> + 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;
>
> - should_emulate = true;
> - }
> - }
> + /* 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;
>
> - if ( !should_emulate )
> - {
> - hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC);
> - return;
> + add_taint(TAINT_HVM_FEP);
> }
> + else
> + goto reinject;
>
> switch ( hvm_emulate_one(&ctxt, VIO_no_completion) )
> {
> case X86EMUL_UNHANDLEABLE:
> case X86EMUL_UNIMPLEMENTED:
> - hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC);
> - break;
> + goto reinject;
How about placing the reinject label here, along with the two case one?
Jan
> case X86EMUL_EXCEPTION:
> hvm_inject_event(&ctxt.ctxt.event);
> /* fall through */
> @@ -3902,6 +3880,11 @@ void hvm_ud_intercept(struct cpu_user_regs *regs)
> hvm_emulate_writeback(&ctxt);
> break;
> }
> +
> + return;
> +
> + reinject:
> + hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC);
> }
>
> enum hvm_intblk hvm_interrupt_blocked(struct vcpu *v, struct hvm_intack intack)
On Wed Mar 11, 2026 at 9:35 AM CET, Jan Beulich wrote:
> On 13.02.2026 12:42, Alejandro Vallejo wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3832,69 +3832,47 @@ 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 = PFEC_insn_fetch;
>> + 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 )
>> + goto reinject;
>
> Is this possible at all, i.e. shouldn't there be ASSERT_UNREACHABLE() in
> addition if already the check is kept?
It isn't.
v2 used to BUG_ON() at VMEXIT when !HVM_FEP and compile out this handler
altogether, but Andrew was unhappy with it because he uses it occasionally and
it'd be more of a PITA to undo the removal or force a HVM_FEP-enabled hypervisor
for the #UD handler to be present at all.
I have no strong views on the ASSERT. It's not expected to happen, but I don't
expect the existing conditions to change either, and if they do that will warrant
a change in the handler too.
If you want it I can add it, but if we're not killing the handler in release I
don't think it's very helpful to assert/bug_on.
>
>> - 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;
>
> Why is this initializer not retained?
It is, it's just that the diff is terrible. An unfortunate side effect of the
removal of the braces. The scope collapsing forces it on top of the function,
before the emulation context is initialised.
It's set up in steps. walk is unconditionally initialised as isnsn_fetch, and
later (after emulate_init_once()), OR'd with PFEC_user_mode for DPL == 3. See...
>
>> - 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;
>> + hvm_emulate_init_once(&ctxt, NULL, regs);
>>
>> - /* 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;
>> + if ( ctxt.seg_reg[x86_seg_ss].dpl == 3 )
>> + walk |= PFEC_user_mode;
... here.
>>
>> - add_taint(TAINT_HVM_FEP);
>> + 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;
>>
>> - should_emulate = true;
>> - }
>> - }
>> + /* 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;
>>
>> - if ( !should_emulate )
>> - {
>> - hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC);
>> - return;
>> + add_taint(TAINT_HVM_FEP);
>> }
>> + else
>> + goto reinject;
>>
>> switch ( hvm_emulate_one(&ctxt, VIO_no_completion) )
>> {
>> case X86EMUL_UNHANDLEABLE:
>> case X86EMUL_UNIMPLEMENTED:
>> - hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC);
>> - break;
>> + goto reinject;
>
> How about placing the reinject label here, along with the two case one?
>
> Jan
Sure. That works too.
Cheers,
Alejandro
On 11.03.2026 10:25, Alejandro Vallejo wrote:
> On Wed Mar 11, 2026 at 9:35 AM CET, Jan Beulich wrote:
>> On 13.02.2026 12:42, Alejandro Vallejo wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -3832,69 +3832,47 @@ 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 = PFEC_insn_fetch;
>>> + 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 )
>>> + goto reinject;
>>
>> Is this possible at all, i.e. shouldn't there be ASSERT_UNREACHABLE() in
>> addition if already the check is kept?
>
> It isn't.
>
> v2 used to BUG_ON() at VMEXIT when !HVM_FEP and compile out this handler
> altogether, but Andrew was unhappy with it because he uses it occasionally and
> it'd be more of a PITA to undo the removal or force a HVM_FEP-enabled hypervisor
> for the #UD handler to be present at all.
>
> I have no strong views on the ASSERT. It's not expected to happen, but I don't
> expect the existing conditions to change either, and if they do that will warrant
> a change in the handler too.
>
> If you want it I can add it, but if we're not killing the handler in release I
> don't think it's very helpful to assert/bug_on.
I see two options: Drop the if() or add ASSERT_UNREACHABLE() to its body.
>>> - 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;
>>
>> Why is this initializer not retained?
>
> It is, it's just that the diff is terrible. An unfortunate side effect of the
> removal of the braces. The scope collapsing forces it on top of the function,
> before the emulation context is initialised.
>
> It's set up in steps. walk is unconditionally initialised as isnsn_fetch, and
> later (after emulate_init_once()), OR'd with PFEC_user_mode for DPL == 3. See...
>
>>
>>> - 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;
>>> + hvm_emulate_init_once(&ctxt, NULL, regs);
>>>
>>> - /* 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;
>>> + if ( ctxt.seg_reg[x86_seg_ss].dpl == 3 )
>>> + walk |= PFEC_user_mode;
>
> ... here.
But that's the point of my question: Why did you split it? All you mean to
do is re-indentation.
Jan
On Wed Mar 11, 2026 at 10:30 AM CET, Jan Beulich wrote:
> On 11.03.2026 10:25, Alejandro Vallejo wrote:
>> On Wed Mar 11, 2026 at 9:35 AM CET, Jan Beulich wrote:
>>> On 13.02.2026 12:42, Alejandro Vallejo wrote:
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -3832,69 +3832,47 @@ 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 = PFEC_insn_fetch;
>>>> + 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 )
>>>> + goto reinject;
>>>
>>> Is this possible at all, i.e. shouldn't there be ASSERT_UNREACHABLE() in
>>> addition if already the check is kept?
>>
>> It isn't.
>>
>> v2 used to BUG_ON() at VMEXIT when !HVM_FEP and compile out this handler
>> altogether, but Andrew was unhappy with it because he uses it occasionally and
>> it'd be more of a PITA to undo the removal or force a HVM_FEP-enabled hypervisor
>> for the #UD handler to be present at all.
>>
>> I have no strong views on the ASSERT. It's not expected to happen, but I don't
>> expect the existing conditions to change either, and if they do that will warrant
>> a change in the handler too.
>>
>> If you want it I can add it, but if we're not killing the handler in release I
>> don't think it's very helpful to assert/bug_on.
>
> I see two options: Drop the if() or add ASSERT_UNREACHABLE() to its body.
I'll go for that second option then.
>
>>>> - 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;
>>>
>>> Why is this initializer not retained?
>>
>> It is, it's just that the diff is terrible. An unfortunate side effect of the
>> removal of the braces. The scope collapsing forces it on top of the function,
>> before the emulation context is initialised.
>>
>> It's set up in steps. walk is unconditionally initialised as isnsn_fetch, and
>> later (after emulate_init_once()), OR'd with PFEC_user_mode for DPL == 3. See...
>>
>>>
>>>> - 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;
>>>> + hvm_emulate_init_once(&ctxt, NULL, regs);
>>>>
>>>> - /* 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;
>>>> + if ( ctxt.seg_reg[x86_seg_ss].dpl == 3 )
>>>> + walk |= PFEC_user_mode;
>>
>> ... here.
>
> But that's the point of my question: Why did you split it? All you mean to
> do is re-indentation.
Because I need to declare "walk" ahead of the statements. Thus this...
uint32_t walk = ((ctxt.seg_reg[x86_seg_ss].dpl == 3)
? PFEC_user_mode : 0) | PFEC_insn_fetch;
must (by necessity) have the declaration placed on top before the emulator
context initialisation. The options are...
uint32_t walk;
[... lines ...]
walk = ((ctxt.seg_reg[x86_seg_ss].dpl == 3)
? PFEC_user_mode : 0) | PFEC_insn_fetch;
... or...
uint32_t walk = PFEC_insn_fetch;
[... lines ...]
if ( ctxt.seg_reg[x86_seg_ss].dpl == 3 )
walk |= PFEC_user_mode;
Line count remains at 3 in both cases, but in the former case there's a
comparison, a ternary operator and an OR all adding cognitive load to the
same statement. In the latter case there's an assignment in the 1st statement,
an if+comparison in a separate line, and a separate OR in the final statement.
It's just simpler to meantally parse because the complexity is evenly
distributed.
I can see how the current form was preferred to avoid a third line (and
then a forth due to the required newline, doubling the total). But with the
rearrangement that's no longer relevant.
If you have a very strong preference for the prior form I could keep it, though
I do have a preference myself for the latter out of improved readability.
Cheers,
Alejandro
On 11.03.2026 11:21, Alejandro Vallejo wrote:
> On Wed Mar 11, 2026 at 10:30 AM CET, Jan Beulich wrote:
>> On 11.03.2026 10:25, Alejandro Vallejo wrote:
>>> On Wed Mar 11, 2026 at 9:35 AM CET, Jan Beulich wrote:
>>>> On 13.02.2026 12:42, Alejandro Vallejo wrote:
>>>>> - 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;
>>>>
>>>> Why is this initializer not retained?
>>>
>>> It is, it's just that the diff is terrible. An unfortunate side effect of the
>>> removal of the braces. The scope collapsing forces it on top of the function,
>>> before the emulation context is initialised.
>>>
>>> It's set up in steps. walk is unconditionally initialised as isnsn_fetch, and
>>> later (after emulate_init_once()), OR'd with PFEC_user_mode for DPL == 3. See...
>>>
>>>>
>>>>> - 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;
>>>>> + hvm_emulate_init_once(&ctxt, NULL, regs);
>>>>>
>>>>> - /* 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;
>>>>> + if ( ctxt.seg_reg[x86_seg_ss].dpl == 3 )
>>>>> + walk |= PFEC_user_mode;
>>>
>>> ... here.
>>
>> But that's the point of my question: Why did you split it? All you mean to
>> do is re-indentation.
>
> Because I need to declare "walk" ahead of the statements. Thus this...
>
> uint32_t walk = ((ctxt.seg_reg[x86_seg_ss].dpl == 3)
> ? PFEC_user_mode : 0) | PFEC_insn_fetch;
>
> must (by necessity) have the declaration placed on top before the emulator
> context initialisation. The options are...
>
> uint32_t walk;
> [... lines ...]
> walk = ((ctxt.seg_reg[x86_seg_ss].dpl == 3)
> ? PFEC_user_mode : 0) | PFEC_insn_fetch;
>
> ... or...
>
> uint32_t walk = PFEC_insn_fetch;
> [... lines ...]
> if ( ctxt.seg_reg[x86_seg_ss].dpl == 3 )
> walk |= PFEC_user_mode;
>
> Line count remains at 3 in both cases, but in the former case there's a
> comparison, a ternary operator and an OR all adding cognitive load to the
> same statement. In the latter case there's an assignment in the 1st statement,
> an if+comparison in a separate line, and a separate OR in the final statement.
> It's just simpler to meantally parse because the complexity is evenly
> distributed.
>
> I can see how the current form was preferred to avoid a third line (and
> then a forth due to the required newline, doubling the total). But with the
> rearrangement that's no longer relevant.
>
> If you have a very strong preference for the prior form I could keep it, though
> I do have a preference myself for the latter out of improved readability.
Strong preference or not - readability is subjective. I prefer the present
form, where the variable obtains it final value right away. More generally,
with subjective aspects it may often be better to leave mechanical changes
(here: re-indentation) as purely mechanical. Things are different with
objective aspects, like style violations which of course can (and imo
preferably should) be corrected on such occasions.
Jan
On Wed Mar 11, 2026 at 12:06 PM CET, Jan Beulich wrote:
> On 11.03.2026 11:21, Alejandro Vallejo wrote:
>> On Wed Mar 11, 2026 at 10:30 AM CET, Jan Beulich wrote:
>>> On 11.03.2026 10:25, Alejandro Vallejo wrote:
>>>> On Wed Mar 11, 2026 at 9:35 AM CET, Jan Beulich wrote:
>>>>> On 13.02.2026 12:42, Alejandro Vallejo wrote:
>>>>>> - 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;
>>>>>
>>>>> Why is this initializer not retained?
>>>>
>>>> It is, it's just that the diff is terrible. An unfortunate side effect of the
>>>> removal of the braces. The scope collapsing forces it on top of the function,
>>>> before the emulation context is initialised.
>>>>
>>>> It's set up in steps. walk is unconditionally initialised as isnsn_fetch, and
>>>> later (after emulate_init_once()), OR'd with PFEC_user_mode for DPL == 3. See...
>>>>
>>>>>
>>>>>> - 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;
>>>>>> + hvm_emulate_init_once(&ctxt, NULL, regs);
>>>>>>
>>>>>> - /* 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;
>>>>>> + if ( ctxt.seg_reg[x86_seg_ss].dpl == 3 )
>>>>>> + walk |= PFEC_user_mode;
>>>>
>>>> ... here.
>>>
>>> But that's the point of my question: Why did you split it? All you mean to
>>> do is re-indentation.
>>
>> Because I need to declare "walk" ahead of the statements. Thus this...
>>
>> uint32_t walk = ((ctxt.seg_reg[x86_seg_ss].dpl == 3)
>> ? PFEC_user_mode : 0) | PFEC_insn_fetch;
>>
>> must (by necessity) have the declaration placed on top before the emulator
>> context initialisation. The options are...
>>
>> uint32_t walk;
>> [... lines ...]
>> walk = ((ctxt.seg_reg[x86_seg_ss].dpl == 3)
>> ? PFEC_user_mode : 0) | PFEC_insn_fetch;
>>
>> ... or...
>>
>> uint32_t walk = PFEC_insn_fetch;
>> [... lines ...]
>> if ( ctxt.seg_reg[x86_seg_ss].dpl == 3 )
>> walk |= PFEC_user_mode;
>>
>> Line count remains at 3 in both cases, but in the former case there's a
>> comparison, a ternary operator and an OR all adding cognitive load to the
>> same statement. In the latter case there's an assignment in the 1st statement,
>> an if+comparison in a separate line, and a separate OR in the final statement.
>> It's just simpler to meantally parse because the complexity is evenly
>> distributed.
>>
>> I can see how the current form was preferred to avoid a third line (and
>> then a forth due to the required newline, doubling the total). But with the
>> rearrangement that's no longer relevant.
>>
>> If you have a very strong preference for the prior form I could keep it, though
>> I do have a preference myself for the latter out of improved readability.
>
> Strong preference or not - readability is subjective. I prefer the present
> form, where the variable obtains it final value right away. More generally,
> with subjective aspects it may often be better to leave mechanical changes
> (here: re-indentation) as purely mechanical. Things are different with
> objective aspects, like style violations which of course can (and imo
> preferably should) be corrected on such occasions.
Ack
Cheers,
Alejandro
© 2016 - 2026 Red Hat, Inc.