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

[Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>]

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.

> > --- 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.

> > +        for ( i = 0; i < (uint8_t)mtrr_cap; i++ )
> ...
> > +    if ( (mtrr_cap & 0xFF) != intel_info->saved_bsp_mtrrs.mtrr_vcnt )
>
> Seeing this and ...
>
> > +    {
> > +        printk("Bootloader saved %ld MTRR values, but there should be 
> > %ld\n",
> > +               intel_info->saved_bsp_mtrrs.mtrr_vcnt, mtrr_cap & 0xFF);
> > +        /* Choose the smaller one to be on the safe side. */
> > +        mtrr_cap = (mtrr_cap & 0xFF) > 
> > intel_info->saved_bsp_mtrrs.mtrr_vcnt ?
>
> ... this vs ...
>
> > +                   intel_info->saved_bsp_mtrrs.mtrr_vcnt : mtrr_cap;
> > +    }
> > +
> > +    def_type = intel_info->saved_bsp_mtrrs.default_mem_type;
> > +    pausing_state = mtrr_pause_caching();
> > +
> > +    for ( i = 0; i < (uint8_t)mtrr_cap; i++ )
>
> ... this (and others): Please be consistent in how you access the same piece
> of data.

Will make it consistent.

> > +    {
> > +        base = intel_info->saved_bsp_mtrrs.mtrr_pair[i].mtrr_physbase;
> > +        mask = intel_info->saved_bsp_mtrrs.mtrr_pair[i].mtrr_physmask;
> > +        wrmsrl(MSR_IA32_MTRR_PHYSBASE(i), base);
> > +        wrmsrl(MSR_IA32_MTRR_PHYSMASK(i), mask);
> > +    }
> > +
> > +    pausing_state.def_type = def_type;
> > +    mtrr_resume_caching(pausing_state);
> > +
> > +    if ( e820_verbose )
> > +    {
> > +        printk("Restored MTRRs:\n"); /* Printed by caller, 
> > mtrr_top_of_ram(). */
>
> What's the comment supposed to be telling the reader? Perhaps this is related 
> to ...
>
> > +        /* If MTRRs are not enabled or WB is not a default type, MTRRs 
> > won't be printed */
>
> ... what this comment says, but then the earlier comment is still confusing 
> (to me
> at least). This latter comment says all that's needed, I would say.
>
> Jan

That comment is why I think output was meant to nest into existing debug
output, not sure about its intended meaning though.  Will drop.

Regards