Re: [PATCH v3 08/22] x86/slaunch: restore boot MTRRs after Intel TXT DRTM

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.07.2025 23:55, Sergii Dmytruk wrote:
> On Wed, Jul 02, 2025 at 05:11:26PM +0200, Jan Beulich wrote:
>> On 30.05.2025 15:17, Sergii Dmytruk wrote:
>>> @@ -442,6 +444,9 @@ static uint64_t __init mtrr_top_of_ram(void)
>>>      ASSERT(paddr_bits);
>>>      addr_mask = ((1ULL << paddr_bits) - 1) & PAGE_MASK;
>>>
>>> +    if ( slaunch_active )
>>> +        txt_restore_mtrrs(e820_verbose);
>>
>> How did you pick this call site? Besides it being unrelated to the purpose of
>> the function, we may not even make it here (e.g. when "e820-mtrr-clip=no" on
>> the command line). Imo this wants to go in the caller of this function,
>> machine_specific_memory_setup(). Or you want to reason about this placement 
>> in
>> the description.
> 
> I think the motivation behind using this location was related to
> printing debug information in a fitting place.  The possible early exit
> definitely ruins everything here, thanks.  Will move at least to the
> caller.

Of course debug info logging will want to remain somewhat sensible. So, just
to mention, besides moving the call site there is also the option of simply
explaining why it needs to be here.

>>> --- a/xen/arch/x86/include/asm/intel-txt.h
>>> +++ b/xen/arch/x86/include/asm/intel-txt.h
>>> @@ -426,6 +426,9 @@ void txt_map_mem_regions(void);
>>>  /* Marks TXT-specific memory as used to avoid its corruption. */
>>>  void txt_reserve_mem_regions(void);
>>>
>>> +/* Restores original MTRR values saved by a bootloader before starting 
>>> DRTM. */
>>> +void txt_restore_mtrrs(bool e820_verbose);
>>
>> This parameter name is, when the header is used from e820.c, going to shadow 
>> the
>> static variable of the same name there. Misra objects to such. But the 
>> parameter
>> doesn't really have a need for having the e820_ prefix, does it?
> 
> I don't think it's possible for a parameter in a declaration to shadow
> anything, but the prefix is indeed unnecessary.

Considering that without a prior forward decl

void test(struct s);

gains a private declaration of struct s (scope being just the function decl),
I expect the same applies to the shadowing caused by parameter names.

Jan