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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

> @@ -111,3 +113,76 @@ void __init txt_reserve_mem_regions(void)
>                       E820_UNUSABLE);
>      BUG_ON(rc == 0);
>  }
> +
> +void __init txt_restore_mtrrs(bool e820_verbose)
> +{
> +    struct slr_entry_intel_info *intel_info;
> +    uint64_t mtrr_cap, mtrr_def, base, mask;
> +    unsigned int i;
> +    uint64_t def_type;
> +    struct mtrr_pausing_state pausing_state;
> +
> +    rdmsrl(MSR_MTRRcap, mtrr_cap);
> +    rdmsrl(MSR_MTRRdefType, mtrr_def);
> +
> +    if ( e820_verbose )
> +    {
> +        printk("MTRRs set previously for SINIT ACM:\n");
> +        printk(" MTRR cap: %"PRIx64" type: %"PRIx64"\n", mtrr_cap, mtrr_def);
> +
> +        for ( i = 0; i < (uint8_t)mtrr_cap; i++ )
> +        {
> +            rdmsrl(MSR_IA32_MTRR_PHYSBASE(i), base);
> +            rdmsrl(MSR_IA32_MTRR_PHYSMASK(i), mask);
> +
> +            printk(" MTRR[%d]: base %"PRIx64" mask %"PRIx64"\n",
> +                   i, base, mask);
> +        }
> +    }
> +
> +    intel_info = (struct slr_entry_intel_info *)
> +        slr_next_entry_by_tag(slaunch_get_slrt(), NULL, 
> SLR_ENTRY_INTEL_INFO);
> +
> +    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.

> +    {
> +        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