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

[ross.philipson@xxxxxxxxxx]

On 5/30/25 6:17 AM, Sergii Dmytruk wrote:
> From: Krystian Hebel <krystian.hebel@xxxxxxxxx>
>
> In preparation for TXT SENTER call, GRUB had to modify MTRR settings
> to be UC for everything except SINIT ACM. Old values are restored
> from SLRT where they were saved by the bootloader.
>
> Signed-off-by: Krystian Hebel <krystian.hebel@xxxxxxxxx>
> Signed-off-by: Michał Żygowski <michal.zygowski@xxxxxxxxx>
> Signed-off-by: Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>
> ---
>   xen/arch/x86/e820.c                  |  5 ++
>   xen/arch/x86/include/asm/intel-txt.h |  3 ++
>   xen/arch/x86/intel-txt.c             | 75 ++++++++++++++++++++++++++++
>   3 files changed, 83 insertions(+)
>
> diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
> index ca577c0bde..60f00e5259 100644
> --- a/xen/arch/x86/e820.c
> +++ b/xen/arch/x86/e820.c
> @@ -11,6 +11,8 @@
>   #include <asm/mtrr.h>
>   #include <asm/msr.h>
>   #include <asm/guest.h>
> +#include <asm/intel-txt.h>
> +#include <asm/slaunch.h>
>   
>   /*
>    * opt_mem: Limit maximum address of physical RAM.
> @@ -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);
> +
I was just curious why they are being restored here in the e820 code? It 
seems that could be restored earlier. Until they are restored, most of 
RAM is set UC as you know. I also don't have an exact idea how early in 
Xen boot cycle this is occurring so maybe this is fine but obviously for 
performance reasons it should be done as early as possible.
Thanks,
Ross

>       rdmsrl(MSR_MTRRcap, mtrr_cap);
>       rdmsrl(MSR_MTRRdefType, mtrr_def);
>   
> diff --git a/xen/arch/x86/include/asm/intel-txt.h b/xen/arch/x86/include/asm/intel-txt.h
> index ad3c41d86c..0b0bdc1bb2 100644
> --- 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);
> +
>   #endif /* __ASSEMBLY__ */
>   
>   #endif /* X86_INTEL_TXT_H */
> diff --git a/xen/arch/x86/intel-txt.c b/xen/arch/x86/intel-txt.c
> index 163383b262..0c14d84486 100644
> --- a/xen/arch/x86/intel-txt.c
> +++ b/xen/arch/x86/intel-txt.c
> @@ -10,6 +10,8 @@
>   #include <xen/types.h>
>   #include <asm/e820.h>
>   #include <asm/intel-txt.h>
> +#include <asm/msr.h>
> +#include <asm/mtrr.h>
>   #include <asm/slaunch.h>
>   
>   static uint64_t __initdata txt_heap_base, txt_heap_size;
> @@ -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 )
> +    {
> +        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 ?
> +                   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++ )
> +    {
> +        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(). 
> */
> +
> +        /* If MTRRs are not enabled or WB is not a default type, MTRRs won't 
> be printed */
> +        if ( !test_bit(11, &def_type) || ((uint8_t)def_type == X86_MT_WB) )
> +        {
> +            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);
> +            }
> +        }
> +    }
> +
> +    /* Restore IA32_MISC_ENABLES */
> +    wrmsrl(MSR_IA32_MISC_ENABLE, intel_info->saved_misc_enable_msr);
> +}