Re: [PATCH v3 2/4] x86/hvm: Disable cross-vendor handling in #UD handler

[Jan Beulich <jbeulich@xxxxxxxx>]

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