From: Sean Christopherson <seanjc@google.com>
To: Maxim Levitsky <mlevitsk@redhat.com>
Cc: kvm@vger.kernel.org, Borislav Petkov <bp@alien8.de>,
Dave Hansen <dave.hansen@linux.intel.com>,
linux-kernel@vger.kernel.org, Wanpeng Li <wanpengli@tencent.com>,
Ingo Molnar <mingo@redhat.com>,
x86@kernel.org, Jim Mattson <jmattson@google.com>,
Kees Cook <keescook@chromium.org>,
Thomas Gleixner <tglx@linutronix.de>,
"H. Peter Anvin" <hpa@zytor.com>, Joerg Roedel <joro@8bytes.org>,
Vitaly Kuznetsov <vkuznets@redhat.com>,
Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v3 08/13] KVM: x86: emulator/smm: use smram structs in the common code
Date: Wed, 24 Aug 2022 22:25:26 +0000 [thread overview]
Message-ID: <YwalVvBbfL1u3H8b@google.com> (raw)
In-Reply-To: <20220803155011.43721-9-mlevitsk@redhat.com>
On Wed, Aug 03, 2022, Maxim Levitsky wrote:
> Switch from using a raw array to 'union kvm_smram'.
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
> arch/x86/include/asm/kvm_host.h | 5 +++--
> arch/x86/kvm/emulate.c | 12 +++++++-----
> arch/x86/kvm/kvm_emulate.h | 3 ++-
> arch/x86/kvm/svm/svm.c | 8 ++++++--
> arch/x86/kvm/vmx/vmx.c | 4 ++--
> arch/x86/kvm/x86.c | 16 ++++++++--------
> 6 files changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index e8281d64a4315a..d752fabde94ad2 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -204,6 +204,7 @@ typedef enum exit_fastpath_completion fastpath_t;
>
> struct x86_emulate_ctxt;
> struct x86_exception;
> +union kvm_smram;
> enum x86_intercept;
> enum x86_intercept_stage;
>
> @@ -1600,8 +1601,8 @@ struct kvm_x86_ops {
> void (*setup_mce)(struct kvm_vcpu *vcpu);
>
> int (*smi_allowed)(struct kvm_vcpu *vcpu, bool for_injection);
> - int (*enter_smm)(struct kvm_vcpu *vcpu, char *smstate);
> - int (*leave_smm)(struct kvm_vcpu *vcpu, const char *smstate);
> + int (*enter_smm)(struct kvm_vcpu *vcpu, union kvm_smram *smram);
> + int (*leave_smm)(struct kvm_vcpu *vcpu, const union kvm_smram *smram);
> void (*enable_smi_window)(struct kvm_vcpu *vcpu);
>
> int (*mem_enc_ioctl)(struct kvm *kvm, void __user *argp);
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 55d9328e6074a2..610978d00b52b0 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -2594,16 +2594,18 @@ static int rsm_load_state_64(struct x86_emulate_ctxt *ctxt,
> static int em_rsm(struct x86_emulate_ctxt *ctxt)
> {
> unsigned long cr0, cr4, efer;
> - char buf[512];
> + const union kvm_smram smram;
This is blatantly wrong, ctxt->ops->read_phys() writes to the buffer. I assume
you did this to make it more difficult to modify the buffer after reading from
guest memory, but IMO that's not worth misleading readers.
> u64 smbase;
> int ret;
>
> + BUILD_BUG_ON(sizeof(smram) != 512);
> +
> if ((ctxt->ops->get_hflags(ctxt) & X86EMUL_SMM_MASK) == 0)
> return emulate_ud(ctxt);
>
> smbase = ctxt->ops->get_smbase(ctxt);
>
> - ret = ctxt->ops->read_phys(ctxt, smbase + 0xfe00, buf, sizeof(buf));
> + ret = ctxt->ops->read_phys(ctxt, smbase + 0xfe00, (void *)&smram, sizeof(smram));
The point of the union + bytes is so that KVM doesn't have to cast.
kvm_vcpu_write_guest(vcpu, vcpu->arch.smbase + 0xfe00,
smram.bytes, sizeof(smram));
> if (ret != X86EMUL_CONTINUE)
> return X86EMUL_UNHANDLEABLE;
>
> @@ -2653,15 +2655,15 @@ static int em_rsm(struct x86_emulate_ctxt *ctxt)
> * state (e.g. enter guest mode) before loading state from the SMM
> * state-save area.
> */
> - if (ctxt->ops->leave_smm(ctxt, buf))
> + if (ctxt->ops->leave_smm(ctxt, &smram))
> goto emulate_shutdown;
>
> #ifdef CONFIG_X86_64
> if (emulator_has_longmode(ctxt))
> - ret = rsm_load_state_64(ctxt, buf);
> + ret = rsm_load_state_64(ctxt, (const char *)&smram);
> else
> #endif
> - ret = rsm_load_state_32(ctxt, buf);
> + ret = rsm_load_state_32(ctxt, (const char *)&smram);
Same thing here, though this is temporary. And it's kinda silly, but I think it
makes sense to avoid the cast here by tweaking the rsm_load_state_*() helpers to
take "u8 *" instead of "char *".
> if (ret != X86EMUL_CONTINUE)
> goto emulate_shutdown;
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 38f873cb6f2c14..688315d1dfabd1 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4433,12 +4433,14 @@ static int svm_smi_allowed(struct kvm_vcpu *vcpu, bool for_injection)
> return 1;
> }
>
> -static int svm_enter_smm(struct kvm_vcpu *vcpu, char *smstate)
> +static int svm_enter_smm(struct kvm_vcpu *vcpu, union kvm_smram *smram)
> {
> struct vcpu_svm *svm = to_svm(vcpu);
> struct kvm_host_map map_save;
> int ret;
>
> + char *smstate = (char *)smram;
Again temporary, but since this is new code, just make it
u8 *smstate = smram->bytes;
> +
> if (!is_guest_mode(vcpu))
> return 0;
>
> @@ -4480,7 +4482,7 @@ static int svm_enter_smm(struct kvm_vcpu *vcpu, char *smstate)
> return 0;
> }
>
> -static int svm_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
> +static int svm_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram)
> {
> struct vcpu_svm *svm = to_svm(vcpu);
> struct kvm_host_map map, map_save;
> @@ -4488,6 +4490,8 @@ static int svm_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
> struct vmcb *vmcb12;
> int ret;
>
> + const char *smstate = (const char *)smram;
> +
And here.
> if (!guest_cpuid_has(vcpu, X86_FEATURE_LM))
> return 0;
>
E.g. this compiles cleanly on top
---
arch/x86/kvm/emulate.c | 17 +++++++++--------
arch/x86/kvm/svm/svm.c | 4 ++--
arch/x86/kvm/x86.c | 7 ++++---
3 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index dd0a08af1dd9..b2ef63cf6cff 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2357,7 +2357,7 @@ static void rsm_set_desc_flags(struct desc_struct *desc, u32 flags)
desc->type = (flags >> 8) & 15;
}
-static int rsm_load_seg_32(struct x86_emulate_ctxt *ctxt, const char *smstate,
+static int rsm_load_seg_32(struct x86_emulate_ctxt *ctxt, const u8 *smstate,
int n)
{
struct desc_struct desc;
@@ -2379,7 +2379,7 @@ static int rsm_load_seg_32(struct x86_emulate_ctxt *ctxt, const char *smstate,
}
#ifdef CONFIG_X86_64
-static int rsm_load_seg_64(struct x86_emulate_ctxt *ctxt, const char *smstate,
+static int rsm_load_seg_64(struct x86_emulate_ctxt *ctxt, const u8 *smstate,
int n)
{
struct desc_struct desc;
@@ -2446,7 +2446,7 @@ static int rsm_enter_protected_mode(struct x86_emulate_ctxt *ctxt,
}
static int rsm_load_state_32(struct x86_emulate_ctxt *ctxt,
- const char *smstate)
+ const u8 *smstate)
{
struct desc_struct desc;
struct desc_ptr dt;
@@ -2507,7 +2507,7 @@ static int rsm_load_state_32(struct x86_emulate_ctxt *ctxt,
#ifdef CONFIG_X86_64
static int rsm_load_state_64(struct x86_emulate_ctxt *ctxt,
- const char *smstate)
+ const u8 *smstate)
{
struct desc_struct desc;
struct desc_ptr dt;
@@ -2580,7 +2580,7 @@ static int rsm_load_state_64(struct x86_emulate_ctxt *ctxt,
static int em_rsm(struct x86_emulate_ctxt *ctxt)
{
unsigned long cr0, cr4, efer;
- const union kvm_smram smram;
+ union kvm_smram smram;
u64 smbase;
int ret;
@@ -2591,7 +2591,8 @@ static int em_rsm(struct x86_emulate_ctxt *ctxt)
smbase = ctxt->ops->get_smbase(ctxt);
- ret = ctxt->ops->read_phys(ctxt, smbase + 0xfe00, (void *)&smram, sizeof(smram));
+ ret = ctxt->ops->read_phys(ctxt, smbase + 0xfe00,
+ smram.bytes, sizeof(smram));
if (ret != X86EMUL_CONTINUE)
return X86EMUL_UNHANDLEABLE;
@@ -2646,10 +2647,10 @@ static int em_rsm(struct x86_emulate_ctxt *ctxt)
#ifdef CONFIG_X86_64
if (emulator_has_longmode(ctxt))
- ret = rsm_load_state_64(ctxt, (const char *)&smram);
+ ret = rsm_load_state_64(ctxt, smram.bytes);
else
#endif
- ret = rsm_load_state_32(ctxt, (const char *)&smram);
+ ret = rsm_load_state_32(ctxt, smram.bytes);
if (ret != X86EMUL_CONTINUE)
goto emulate_shutdown;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 5d748b10c5be..ecf11c8a052e 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4439,7 +4439,7 @@ static int svm_enter_smm(struct kvm_vcpu *vcpu, union kvm_smram *smram)
struct kvm_host_map map_save;
int ret;
- char *smstate = (char *)smram;
+ u8 *smstate = smram->bytes;
if (!is_guest_mode(vcpu))
return 0;
@@ -4490,7 +4490,7 @@ static int svm_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram)
struct vmcb *vmcb12;
int ret;
- const char *smstate = (const char *)smram;
+ const char *smstate = smram->bytes;
if (!guest_cpuid_has(vcpu, X86_FEATURE_LM))
return 0;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ca558674b07b..09268c2335a8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9985,10 +9985,10 @@ static void enter_smm(struct kvm_vcpu *vcpu)
memset(smram.bytes, 0, sizeof(smram.bytes));
#ifdef CONFIG_X86_64
if (guest_cpuid_has(vcpu, X86_FEATURE_LM))
- enter_smm_save_state_64(vcpu, (char *)&smram);
+ enter_smm_save_state_64(vcpu, smram.bytes);
else
#endif
- enter_smm_save_state_32(vcpu, (char *)&smram);
+ enter_smm_save_state_32(vcpu, smram.bytes);
/*
* Give enter_smm() a chance to make ISA-specific changes to the vCPU
@@ -9998,7 +9998,8 @@ static void enter_smm(struct kvm_vcpu *vcpu)
static_call(kvm_x86_enter_smm)(vcpu, &smram);
kvm_smm_changed(vcpu, true);
- kvm_vcpu_write_guest(vcpu, vcpu->arch.smbase + 0xfe00, &smram, sizeof(smram));
+ kvm_vcpu_write_guest(vcpu, vcpu->arch.smbase + 0xfe00,
+ smram.bytes, sizeof(smram));
if (static_call(kvm_x86_get_nmi_mask)(vcpu))
vcpu->arch.hflags |= HF_SMM_INSIDE_NMI_MASK;
base-commit: 0708faef18ff51a2b2dba546961d843223331138
--
next prev parent reply other threads:[~2022-08-24 22:25 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-03 15:49 [PATCH v3 00/13] SMM emulation and interrupt shadow fixes Maxim Levitsky
2022-08-03 15:49 ` [PATCH v3 01/13] bug: introduce ASSERT_STRUCT_OFFSET Maxim Levitsky
2022-08-03 15:50 ` [PATCH v3 02/13] KVM: x86: emulator: em_sysexit should update ctxt->mode Maxim Levitsky
2022-08-03 15:50 ` [PATCH v3 03/13] KVM: x86: emulator: introduce emulator_recalc_and_set_mode Maxim Levitsky
2022-08-11 15:33 ` Yang, Weijiang
2022-08-12 6:25 ` Maxim Levitsky
2022-08-17 14:42 ` Maxim Levitsky
2022-08-03 15:50 ` [PATCH v3 04/13] KVM: x86: emulator: update the emulation mode after rsm Maxim Levitsky
2022-08-24 21:50 ` Sean Christopherson
2022-08-03 15:50 ` [PATCH v3 05/13] KVM: x86: emulator: update the emulation mode after CR0 write Maxim Levitsky
2022-08-24 21:57 ` Sean Christopherson
2022-08-03 15:50 ` [PATCH v3 06/13] KVM: x86: emulator/smm: number of GPRs in the SMRAM image depends on the image format Maxim Levitsky
2022-08-03 15:50 ` [PATCH v3 07/13] KVM: x86: emulator/smm: add structs for KVM's smram layout Maxim Levitsky
2022-08-24 22:06 ` Sean Christopherson
2022-08-03 15:50 ` [PATCH v3 08/13] KVM: x86: emulator/smm: use smram structs in the common code Maxim Levitsky
2022-08-24 22:25 ` Sean Christopherson [this message]
2022-08-03 15:50 ` [PATCH v3 09/13] KVM: x86: emulator/smm: use smram struct for 32 bit smram load/restore Maxim Levitsky
2022-08-24 22:28 ` Sean Christopherson
2022-08-03 15:50 ` [PATCH v3 10/13] KVM: x86: emulator/smm: use smram struct for 64 " Maxim Levitsky
2022-08-24 22:34 ` Sean Christopherson
2022-08-03 15:50 ` [PATCH v3 11/13] KVM: x86: SVM: use smram structs Maxim Levitsky
2022-08-24 22:42 ` Sean Christopherson
2022-08-03 15:50 ` [PATCH v3 12/13] KVM: x86: SVM: don't save SVM state to SMRAM when VM is not long mode capable Maxim Levitsky
2022-08-24 22:58 ` Sean Christopherson
2022-08-03 15:50 ` [PATCH v3 13/13] KVM: x86: emulator/smm: preserve interrupt shadow in SMRAM Maxim Levitsky
2022-08-24 23:50 ` Sean Christopherson
2022-08-25 10:13 ` Maxim Levitsky
2022-08-25 15:44 ` Sean Christopherson
2022-08-10 12:00 ` [PATCH v3 00/13] SMM emulation and interrupt shadow fixes Thomas Lamprecht
2022-08-10 13:25 ` Maxim Levitsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YwalVvBbfL1u3H8b@google.com \
--to=seanjc@google.com \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=hpa@zytor.com \
--cc=jmattson@google.com \
--cc=joro@8bytes.org \
--cc=keescook@chromium.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=mlevitsk@redhat.com \
--cc=pbonzini@redhat.com \
--cc=tglx@linutronix.de \
--cc=vkuznets@redhat.com \
--cc=wanpengli@tencent.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.