| Message ID | 20241015095818.357915-1-kirill.shutemov@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/kvm: Override default caching mode for SEV-SNP and TDX | expand |
On 15.10.24 11:58, Kirill A. Shutemov wrote: > AMD SEV-SNP and Intel TDX have limited access to MTRR: either it is not > advertised in CPUID or it cannot be programmed (on TDX, due to #VE on > CR0.CD clear). > > This results in guests using uncached mappings where it shouldn't and > pmd/pud_set_huge() failures due to non-uniform memory type reported by > mtrr_type_lookup(). > > Override MTRR state, making it WB by default as the kernel does for > Hyper-V guests. > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Suggested-by: Binbin Wu <binbin.wu@intel.com> > Cc: Juergen Gross <jgross@suse.com> > Cc: Tom Lendacky <thomas.lendacky@amd.com> > --- > arch/x86/kernel/kvm.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c > index 263f8aed4e2c..21e9e4845354 100644 > --- a/arch/x86/kernel/kvm.c > +++ b/arch/x86/kernel/kvm.c > @@ -37,6 +37,7 @@ > #include <asm/apic.h> > #include <asm/apicdef.h> > #include <asm/hypervisor.h> > +#include <asm/mtrr.h> > #include <asm/tlb.h> > #include <asm/cpuidle_haltpoll.h> > #include <asm/ptrace.h> > @@ -980,6 +981,9 @@ static void __init kvm_init_platform(void) > } > kvmclock_init(); > x86_platform.apic_post_init = kvm_apic_init; > + > + /* Set WB as the default cache mode for SEV-SNP and TDX */ > + mtrr_overwrite_state(NULL, 0, MTRR_TYPE_WRBACK); Do you really want to do this for _all_ KVM guests? I'd expect this call to be conditional on TDX or SEV-SNP. Juergen
On Tue, Oct 15, 2024 at 12:12:51PM +0200, Jürgen Groß wrote: > On 15.10.24 11:58, Kirill A. Shutemov wrote: > > AMD SEV-SNP and Intel TDX have limited access to MTRR: either it is not > > advertised in CPUID or it cannot be programmed (on TDX, due to #VE on > > CR0.CD clear). > > > > This results in guests using uncached mappings where it shouldn't and > > pmd/pud_set_huge() failures due to non-uniform memory type reported by > > mtrr_type_lookup(). > > > > Override MTRR state, making it WB by default as the kernel does for > > Hyper-V guests. > > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > > Suggested-by: Binbin Wu <binbin.wu@intel.com> > > Cc: Juergen Gross <jgross@suse.com> > > Cc: Tom Lendacky <thomas.lendacky@amd.com> > > --- > > arch/x86/kernel/kvm.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c > > index 263f8aed4e2c..21e9e4845354 100644 > > --- a/arch/x86/kernel/kvm.c > > +++ b/arch/x86/kernel/kvm.c > > @@ -37,6 +37,7 @@ > > #include <asm/apic.h> > > #include <asm/apicdef.h> > > #include <asm/hypervisor.h> > > +#include <asm/mtrr.h> > > #include <asm/tlb.h> > > #include <asm/cpuidle_haltpoll.h> > > #include <asm/ptrace.h> > > @@ -980,6 +981,9 @@ static void __init kvm_init_platform(void) > > } > > kvmclock_init(); > > x86_platform.apic_post_init = kvm_apic_init; > > + > > + /* Set WB as the default cache mode for SEV-SNP and TDX */ > > + mtrr_overwrite_state(NULL, 0, MTRR_TYPE_WRBACK); > > Do you really want to do this for _all_ KVM guests? > > I'd expect this call to be conditional on TDX or SEV-SNP. mtrr_overwrite_state() checks it internally.
On 15.10.24 14:31, Kirill A. Shutemov wrote: > On Tue, Oct 15, 2024 at 12:12:51PM +0200, Jürgen Groß wrote: >> On 15.10.24 11:58, Kirill A. Shutemov wrote: >>> AMD SEV-SNP and Intel TDX have limited access to MTRR: either it is not >>> advertised in CPUID or it cannot be programmed (on TDX, due to #VE on >>> CR0.CD clear). >>> >>> This results in guests using uncached mappings where it shouldn't and >>> pmd/pud_set_huge() failures due to non-uniform memory type reported by >>> mtrr_type_lookup(). >>> >>> Override MTRR state, making it WB by default as the kernel does for >>> Hyper-V guests. >>> >>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> >>> Suggested-by: Binbin Wu <binbin.wu@intel.com> >>> Cc: Juergen Gross <jgross@suse.com> >>> Cc: Tom Lendacky <thomas.lendacky@amd.com> >>> --- >>> arch/x86/kernel/kvm.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c >>> index 263f8aed4e2c..21e9e4845354 100644 >>> --- a/arch/x86/kernel/kvm.c >>> +++ b/arch/x86/kernel/kvm.c >>> @@ -37,6 +37,7 @@ >>> #include <asm/apic.h> >>> #include <asm/apicdef.h> >>> #include <asm/hypervisor.h> >>> +#include <asm/mtrr.h> >>> #include <asm/tlb.h> >>> #include <asm/cpuidle_haltpoll.h> >>> #include <asm/ptrace.h> >>> @@ -980,6 +981,9 @@ static void __init kvm_init_platform(void) >>> } >>> kvmclock_init(); >>> x86_platform.apic_post_init = kvm_apic_init; >>> + >>> + /* Set WB as the default cache mode for SEV-SNP and TDX */ >>> + mtrr_overwrite_state(NULL, 0, MTRR_TYPE_WRBACK); >> >> Do you really want to do this for _all_ KVM guests? >> >> I'd expect this call to be conditional on TDX or SEV-SNP. > > mtrr_overwrite_state() checks it internally. Ah, right, I forgot I added that check on request by Boris. :-) Reviewed-by: Juergen Gross <jgross@suse.com> Juergen
On 10/15/24 03:12, Jürgen Groß wrote: >> >> + /* Set WB as the default cache mode for SEV-SNP and TDX */ >> + mtrr_overwrite_state(NULL, 0, MTRR_TYPE_WRBACK); > > Do you really want to do this for _all_ KVM guests? > > I'd expect this call to be conditional on TDX or SEV-SNP. I was confused by this as well. Shouldn't mtrr_overwrite_state() be named something more like: guest_force_mtrr_state() or something? The mtrr_overwrite_state() comment is pretty good, but it looks quite confusing from the caller.
On Tue, Oct 15, 2024 at 06:14:29AM -0700, Dave Hansen wrote: > On 10/15/24 03:12, Jürgen Groß wrote: > >> > >> + /* Set WB as the default cache mode for SEV-SNP and TDX */ > >> + mtrr_overwrite_state(NULL, 0, MTRR_TYPE_WRBACK); > > > > Do you really want to do this for _all_ KVM guests? > > > > I'd expect this call to be conditional on TDX or SEV-SNP. > > I was confused by this as well. > > Shouldn't mtrr_overwrite_state() be named something more like: > > guest_force_mtrr_state() > > or something? > > The mtrr_overwrite_state() comment is pretty good, but it looks quite > confusing from the caller. I can submit a following up patch with rename if it is fine.
On 15.10.24 15:54, Kirill A. Shutemov wrote: > On Tue, Oct 15, 2024 at 06:14:29AM -0700, Dave Hansen wrote: >> On 10/15/24 03:12, Jürgen Groß wrote: >>>> >>>> + /* Set WB as the default cache mode for SEV-SNP and TDX */ >>>> + mtrr_overwrite_state(NULL, 0, MTRR_TYPE_WRBACK); >>> >>> Do you really want to do this for _all_ KVM guests? >>> >>> I'd expect this call to be conditional on TDX or SEV-SNP. >> >> I was confused by this as well. >> >> Shouldn't mtrr_overwrite_state() be named something more like: >> >> guest_force_mtrr_state() >> >> or something? >> >> The mtrr_overwrite_state() comment is pretty good, but it looks quite >> confusing from the caller. > > I can submit a following up patch with rename if it is fine. > Fine with me. Juergen
On 10/15/24 11:58, Kirill A. Shutemov wrote: > AMD SEV-SNP and Intel TDX have limited access to MTRR: either it is not > advertised in CPUID or it cannot be programmed (on TDX, due to #VE on > CR0.CD clear). > > This results in guests using uncached mappings where it shouldn't and > pmd/pud_set_huge() failures due to non-uniform memory type reported by > mtrr_type_lookup(). > > Override MTRR state, making it WB by default as the kernel does for > Hyper-V guests. > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Suggested-by: Binbin Wu <binbin.wu@intel.com> > Cc: Juergen Gross <jgross@suse.com> > Cc: Tom Lendacky <thomas.lendacky@amd.com> Queued, thanks. I'll leave the follow up to the owners of the tip tree. Paolo > --- > arch/x86/kernel/kvm.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c > index 263f8aed4e2c..21e9e4845354 100644 > --- a/arch/x86/kernel/kvm.c > +++ b/arch/x86/kernel/kvm.c > @@ -37,6 +37,7 @@ > #include <asm/apic.h> > #include <asm/apicdef.h> > #include <asm/hypervisor.h> > +#include <asm/mtrr.h> > #include <asm/tlb.h> > #include <asm/cpuidle_haltpoll.h> > #include <asm/ptrace.h> > @@ -980,6 +981,9 @@ static void __init kvm_init_platform(void) > } > kvmclock_init(); > x86_platform.apic_post_init = kvm_apic_init; > + > + /* Set WB as the default cache mode for SEV-SNP and TDX */ > + mtrr_overwrite_state(NULL, 0, MTRR_TYPE_WRBACK); > } > > #if defined(CONFIG_AMD_MEM_ENCRYPT)
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c index 263f8aed4e2c..21e9e4845354 100644 --- a/arch/x86/kernel/kvm.c +++ b/arch/x86/kernel/kvm.c @@ -37,6 +37,7 @@ #include <asm/apic.h> #include <asm/apicdef.h> #include <asm/hypervisor.h> +#include <asm/mtrr.h> #include <asm/tlb.h> #include <asm/cpuidle_haltpoll.h> #include <asm/ptrace.h> @@ -980,6 +981,9 @@ static void __init kvm_init_platform(void) } kvmclock_init(); x86_platform.apic_post_init = kvm_apic_init; + + /* Set WB as the default cache mode for SEV-SNP and TDX */ + mtrr_overwrite_state(NULL, 0, MTRR_TYPE_WRBACK); } #if defined(CONFIG_AMD_MEM_ENCRYPT)
AMD SEV-SNP and Intel TDX have limited access to MTRR: either it is not advertised in CPUID or it cannot be programmed (on TDX, due to #VE on CR0.CD clear). This results in guests using uncached mappings where it shouldn't and pmd/pud_set_huge() failures due to non-uniform memory type reported by mtrr_type_lookup(). Override MTRR state, making it WB by default as the kernel does for Hyper-V guests. Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Suggested-by: Binbin Wu <binbin.wu@intel.com> Cc: Juergen Gross <jgross@suse.com> Cc: Tom Lendacky <thomas.lendacky@amd.com> --- arch/x86/kernel/kvm.c | 4 ++++ 1 file changed, 4 insertions(+)