From: Krystian Hebel <krystian.hebel@3mdeb.com>
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@3mdeb.com>
Signed-off-by: Michał Żygowski <michal.zygowski@3mdeb.com>
Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
---
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);
+
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);
+}
--
2.49.0
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
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
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
On 5/30/25 6:17 AM, Sergii Dmytruk wrote:
> From: Krystian Hebel <krystian.hebel@3mdeb.com>
>
> 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@3mdeb.com>
> Signed-off-by: Michał Żygowski <michal.zygowski@3mdeb.com>
> Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
> ---
> 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);
> +}
On Tue, Jun 03, 2025 at 12:43:30PM -0700, ross.philipson@oracle.com wrote: > On 5/30/25 6:17 AM, Sergii Dmytruk wrote: > > From: Krystian Hebel <krystian.hebel@3mdeb.com> > > > > 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@3mdeb.com> > > Signed-off-by: Michał Żygowski <michal.zygowski@3mdeb.com> > > Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com> > > --- > > 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 Original MTRR values are in SLRT which is deliberately measured before being used by e820 code. I'm not sure if anything precludes moving that part (mapping memory, interacting with TPM) earlier in __start_xen(). Could be worth trying moving it up. Regards
© 2016 - 2025 Red Hat, Inc.