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
