Re: [PATCH v3 09/13] KVM: x86: emulator/smm: use smram struct for 32 bit smram load/restore

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 09/13] KVM: x86: emulator/smm: use smram struct for 32 bit smram load/restore
Date: Wed, 24 Aug 2022 22:28:45 +0000	[thread overview]
Message-ID: <YwamHYLokbGX96cG@google.com> (raw)
In-Reply-To: <20220803155011.43721-10-mlevitsk@redhat.com>

On Wed, Aug 03, 2022, Maxim Levitsky wrote:
> Use kvm_smram_state_32 struct to save/restore 32 bit SMM state
> (used when X86_FEATURE_LM is not present in the guest CPUID).
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  arch/x86/kvm/emulate.c | 81 +++++++++++++++---------------------------
>  arch/x86/kvm/x86.c     | 75 +++++++++++++++++---------------------
>  2 files changed, 60 insertions(+), 96 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 610978d00b52b0..3339d542a25439 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -2371,25 +2371,17 @@ 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 void rsm_load_seg_32(struct x86_emulate_ctxt *ctxt,
> +			   const struct kvm_smm_seg_state_32 *state,

Alignment is off by one.

> +			   u16 selector,
>  			   int n)

These can go on a single line.

static void rsm_load_seg_32(struct x86_emulate_ctxt *ctxt,
			    const struct kvm_smm_seg_state_32 *state,
			    u16 selector, int n)

>  	struct desc_struct desc;
> -	int offset;
> -	u16 selector;
> -
> -	selector = GET_SMSTATE(u32, smstate, 0x7fa8 + n * 4);
> -
> -	if (n < 3)
> -		offset = 0x7f84 + n * 12;
> -	else
> -		offset = 0x7f2c + (n - 3) * 12;
>  
> -	set_desc_base(&desc,      GET_SMSTATE(u32, smstate, offset + 8));
> -	set_desc_limit(&desc,     GET_SMSTATE(u32, smstate, offset + 4));
> -	rsm_set_desc_flags(&desc, GET_SMSTATE(u32, smstate, offset));
> +	set_desc_base(&desc,      state->base);
> +	set_desc_limit(&desc,     state->limit);
> +	rsm_set_desc_flags(&desc, state->flags);
>  	ctxt->ops->set_segment(ctxt, selector, &desc, 0, n);
> -	return X86EMUL_CONTINUE;
>  }
>  

...

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index cbbe49bdc58787..6abe35f7687e2c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9833,22 +9833,18 @@ static u32 enter_smm_get_segment_flags(struct kvm_segment *seg)
>  	return flags;
>  }
>  
> -static void enter_smm_save_seg_32(struct kvm_vcpu *vcpu, char *buf, int n)
> +static void enter_smm_save_seg_32(struct kvm_vcpu *vcpu,
> +					 struct kvm_smm_seg_state_32 *state,
> +					 u32 *selector,
> +					 int n)

Similar issues here.

static void enter_smm_save_seg_32(struct kvm_vcpu *vcpu,
				  struct kvm_smm_seg_state_32 *state,
				  u32 *selector, int n)
>  {
>  	struct kvm_segment seg;
> -	int offset;
>  
>  	kvm_get_segment(vcpu, &seg, n);
> -	put_smstate(u32, buf, 0x7fa8 + n * 4, seg.selector);
> -
> -	if (n < 3)
> -		offset = 0x7f84 + n * 12;
> -	else
> -		offset = 0x7f2c + (n - 3) * 12;
> -
> -	put_smstate(u32, buf, offset + 8, seg.base);
> -	put_smstate(u32, buf, offset + 4, seg.limit);
> -	put_smstate(u32, buf, offset, enter_smm_get_segment_flags(&seg));
> +	*selector = seg.selector;
> +	state->base = seg.base;
> +	state->limit = seg.limit;
> +	state->flags = enter_smm_get_segment_flags(&seg);
>  }
>  
>  #ifdef CONFIG_X86_64
> @@ -9869,54 +9865,47 @@ static void enter_smm_save_seg_64(struct kvm_vcpu *vcpu, char *buf, int n)
>  }
>  #endif
>  
> -static void enter_smm_save_state_32(struct kvm_vcpu *vcpu, char *buf)
> +static void enter_smm_save_state_32(struct kvm_vcpu *vcpu, struct kvm_smram_state_32 *smram)

Please wrap, no reason to run long.

static void enter_smm_save_state_32(struct kvm_vcpu *vcpu,
				    struct kvm_smram_state_32 *smram)