diff mbox series

[v2,02/11] KVM: x86: emulator: introduce update_emulation_mode

Message ID 20220621150902.46126-3-mlevitsk@redhat.com (mailing list archive)
State New, archived
Headers show
Series SMM emulation and interrupt shadow fixes | expand

Commit Message

Maxim Levitsky June 21, 2022, 3:08 p.m. UTC 
Some instructions update the cpu execution mode, which needs
to update the emulation mode.

Extract this code, and make assign_eip_far use it.

assign_eip_far now reads CS, instead of getting it via a parameter,
which is ok, because callers always assign CS to the
same value before calling it.

No functional change is intended.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/emulate.c | 85 ++++++++++++++++++++++++++++--------------
 1 file changed, 57 insertions(+), 28 deletions(-)

Comments

Sean Christopherson July 20, 2022, 11:44 p.m. UTC | #1 
On Tue, Jun 21, 2022, Maxim Levitsky wrote:
> +static inline int update_emulation_mode(struct x86_emulate_ctxt *ctxt)

Maybe emulator_recalc_and_set_mode()?  It took me a second to understand that
"update" also involves determining the "new" mode, e.g. I was trying to figure
out where @mode was :-)

> +{
> +	u64 efer;
> +	struct desc_struct cs;
> +	u16 selector;
> +	u32 base3;
> +
> +	ctxt->ops->get_msr(ctxt, MSR_EFER, &efer);
> +
> +	if (!ctxt->ops->get_cr(ctxt, 0) & X86_CR0_PE) {
> +		/* Real mode. cpu must not have long mode active */
> +		if (efer & EFER_LMA)
> +			return X86EMUL_UNHANDLEABLE;

If we hit this, is there any hope of X86EMUL_UNHANDLEABLE doing the right thing?
Ah, SMM and the ability to swizzle SMRAM state.  Bummer.  I was hoping we could
just bug the VM.

> +		ctxt->mode = X86EMUL_MODE_REAL;
> +		return X86EMUL_CONTINUE;
> +	}
Maxim Levitsky July 21, 2022, 11:52 a.m. UTC | #2 
On Wed, 2022-07-20 at 23:44 +0000, Sean Christopherson wrote:
> On Tue, Jun 21, 2022, Maxim Levitsky wrote:
> > +static inline int update_emulation_mode(struct x86_emulate_ctxt *ctxt)
> 
> Maybe emulator_recalc_and_set_mode()?  It took me a second to understand that
> "update" also involves determining the "new" mode, e.g. I was trying to figure
> out where @mode was :-)

I don't mind at all, will update in v3.

> 
> > +{
> > +	u64 efer;
> > +	struct desc_struct cs;
> > +	u16 selector;
> > +	u32 base3;
> > +
> > +	ctxt->ops->get_msr(ctxt, MSR_EFER, &efer);
> > +
> > +	if (!ctxt->ops->get_cr(ctxt, 0) & X86_CR0_PE) {
> > +		/* Real mode. cpu must not have long mode active */
> > +		if (efer & EFER_LMA)
> > +			return X86EMUL_UNHANDLEABLE;
> 
> If we hit this, is there any hope of X86EMUL_UNHANDLEABLE doing the right thing?
> Ah, SMM and the ability to swizzle SMRAM state.  Bummer.  I was hoping we could
> just bug the VM.

I just tried to be a good citizen here, it is probably impossible to hit this case.
(RSM ignores LMA bit in the EFER in the SMRAM).

Best regards,
	Maxim Levitsky



> 
> > +		ctxt->mode = X86EMUL_MODE_REAL;
> > +		return X86EMUL_CONTINUE;
> > +	}
Sean Christopherson July 21, 2022, 2:23 p.m. UTC | #3 
On Thu, Jul 21, 2022, Maxim Levitsky wrote:
> On Wed, 2022-07-20 at 23:44 +0000, Sean Christopherson wrote:
> > On Tue, Jun 21, 2022, Maxim Levitsky wrote:
> > > +	if (!ctxt->ops->get_cr(ctxt, 0) & X86_CR0_PE) {
> > > +		/* Real mode. cpu must not have long mode active */
> > > +		if (efer & EFER_LMA)
> > > +			return X86EMUL_UNHANDLEABLE;
> > 
> > If we hit this, is there any hope of X86EMUL_UNHANDLEABLE doing the right thing?
> > Ah, SMM and the ability to swizzle SMRAM state.  Bummer.  I was hoping we could
> > just bug the VM.
> 
> I just tried to be a good citizen here, it is probably impossible to hit this case.
> (RSM ignores LMA bit in the EFER in the SMRAM).

The reason I asked is because if all of the X86EMUL_UNHANDLEABLE paths are impossible
then my preference would be to refactor this slightly to:

	static int emulator_calc_cpu_mode(const struct x86_emulate_ctxt *ctxt)

and return the mode instead of success/failure, and turn those checks into:

	KVM_EMULATOR_BUG_ON(efer & EFER_LMA);

with the callers being:

	ctxt->mode = emulator_calc_cpu_mode(ctxt);

But I think this one:

	if (!ctxt->ops->get_segment(ctxt, &selector, &cs, &base3, VCPU_SREG_CS))
		return X86EMUL_UNHANDLEABLE;

is reachable in the em_rsm() case :-/
diff mbox series

Patch

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 5aeb343ca8b007..2c0087df2d7e6a 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -804,8 +804,7 @@  static int linearize(struct x86_emulate_ctxt *ctxt,
 			   ctxt->mode, linear);
 }
 
-static inline int assign_eip(struct x86_emulate_ctxt *ctxt, ulong dst,
-			     enum x86emul_mode mode)
+static inline int assign_eip(struct x86_emulate_ctxt *ctxt, ulong dst)
 {
 	ulong linear;
 	int rc;
@@ -815,41 +814,71 @@  static inline int assign_eip(struct x86_emulate_ctxt *ctxt, ulong dst,
 
 	if (ctxt->op_bytes != sizeof(unsigned long))
 		addr.ea = dst & ((1UL << (ctxt->op_bytes << 3)) - 1);
-	rc = __linearize(ctxt, addr, &max_size, 1, false, true, mode, &linear);
+	rc = __linearize(ctxt, addr, &max_size, 1, false, true, ctxt->mode, &linear);
 	if (rc == X86EMUL_CONTINUE)
 		ctxt->_eip = addr.ea;
 	return rc;
 }
 
+static inline int update_emulation_mode(struct x86_emulate_ctxt *ctxt)
+{
+	u64 efer;
+	struct desc_struct cs;
+	u16 selector;
+	u32 base3;
+
+	ctxt->ops->get_msr(ctxt, MSR_EFER, &efer);
+
+	if (!ctxt->ops->get_cr(ctxt, 0) & X86_CR0_PE) {
+		/* Real mode. cpu must not have long mode active */
+		if (efer & EFER_LMA)
+			return X86EMUL_UNHANDLEABLE;
+		ctxt->mode = X86EMUL_MODE_REAL;
+		return X86EMUL_CONTINUE;
+	}
+
+	if (ctxt->eflags & X86_EFLAGS_VM) {
+		/* Protected/VM86 mode. cpu must not have long mode active */
+		if (efer & EFER_LMA)
+			return X86EMUL_UNHANDLEABLE;
+		ctxt->mode = X86EMUL_MODE_VM86;
+		return X86EMUL_CONTINUE;
+	}
+
+	if (!ctxt->ops->get_segment(ctxt, &selector, &cs, &base3, VCPU_SREG_CS))
+		return X86EMUL_UNHANDLEABLE;
+
+	if (efer & EFER_LMA) {
+		if (cs.l) {
+			/* Proper long mode */
+			ctxt->mode = X86EMUL_MODE_PROT64;
+		} else if (cs.d) {
+			/* 32 bit compatibility mode*/
+			ctxt->mode = X86EMUL_MODE_PROT32;
+		} else {
+			ctxt->mode = X86EMUL_MODE_PROT16;
+		}
+	} else {
+		/* Legacy 32 bit / 16 bit mode */
+		ctxt->mode = cs.d ? X86EMUL_MODE_PROT32 : X86EMUL_MODE_PROT16;
+	}
+
+	return X86EMUL_CONTINUE;
+}
+
 static inline int assign_eip_near(struct x86_emulate_ctxt *ctxt, ulong dst)
 {
-	return assign_eip(ctxt, dst, ctxt->mode);
+	return assign_eip(ctxt, dst);
 }
 
-static int assign_eip_far(struct x86_emulate_ctxt *ctxt, ulong dst,
-			  const struct desc_struct *cs_desc)
+static int assign_eip_far(struct x86_emulate_ctxt *ctxt, ulong dst)
 {
-	enum x86emul_mode mode = ctxt->mode;
-	int rc;
+	int rc = update_emulation_mode(ctxt);
 
-#ifdef CONFIG_X86_64
-	if (ctxt->mode >= X86EMUL_MODE_PROT16) {
-		if (cs_desc->l) {
-			u64 efer = 0;
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
 
-			ctxt->ops->get_msr(ctxt, MSR_EFER, &efer);
-			if (efer & EFER_LMA)
-				mode = X86EMUL_MODE_PROT64;
-		} else
-			mode = X86EMUL_MODE_PROT32; /* temporary value */
-	}
-#endif
-	if (mode == X86EMUL_MODE_PROT16 || mode == X86EMUL_MODE_PROT32)
-		mode = cs_desc->d ? X86EMUL_MODE_PROT32 : X86EMUL_MODE_PROT16;
-	rc = assign_eip(ctxt, dst, mode);
-	if (rc == X86EMUL_CONTINUE)
-		ctxt->mode = mode;
-	return rc;
+	return assign_eip(ctxt, dst);
 }
 
 static inline int jmp_rel(struct x86_emulate_ctxt *ctxt, int rel)
@@ -2184,7 +2213,7 @@  static int em_jmp_far(struct x86_emulate_ctxt *ctxt)
 	if (rc != X86EMUL_CONTINUE)
 		return rc;
 
-	rc = assign_eip_far(ctxt, ctxt->src.val, &new_desc);
+	rc = assign_eip_far(ctxt, ctxt->src.val);
 	/* Error handling is not implemented. */
 	if (rc != X86EMUL_CONTINUE)
 		return X86EMUL_UNHANDLEABLE;
@@ -2262,7 +2291,7 @@  static int em_ret_far(struct x86_emulate_ctxt *ctxt)
 				       &new_desc);
 	if (rc != X86EMUL_CONTINUE)
 		return rc;
-	rc = assign_eip_far(ctxt, eip, &new_desc);
+	rc = assign_eip_far(ctxt, eip);
 	/* Error handling is not implemented. */
 	if (rc != X86EMUL_CONTINUE)
 		return X86EMUL_UNHANDLEABLE;
@@ -3482,7 +3511,7 @@  static int em_call_far(struct x86_emulate_ctxt *ctxt)
 	if (rc != X86EMUL_CONTINUE)
 		return rc;
 
-	rc = assign_eip_far(ctxt, ctxt->src.val, &new_desc);
+	rc = assign_eip_far(ctxt, ctxt->src.val);
 	if (rc != X86EMUL_CONTINUE)
 		goto fail;