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
