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
