[PATCH] deal with interrupt shadow state for emulated instruction

[PATCH] deal with interrupt shadow state for emulated instruction

[Glauber Costa]

we currently unblock shadow interrupt state when we skip an instruction,
but failing to do so when we actually emulate one. This blocks interrupts
in key instruction blocks, in particular sti; hlt; sequences

If the instruction emulated is an sti, we have to block shadow interrupts.
The same goes for mov ss. pop ss also needs it, but we don't currently
emulate it. For sequences of two or more instructions of the same type
among those instructions, only the first one has this effect.

Without this patch, I cannot boot gpxe option roms at vmx machines.
This is described at https://bugzilla.redhat.com/show_bug.cgi?id=494469

Signed-off-by: Glauber Costa <glommer@redhat.com>
CC: H. Peter Anvin <hpa@zytor.com>
CC: Avi Kivity <avi@redhat.com>
---
 arch/x86/include/asm/kvm_host.h        |    2 +
 arch/x86/include/asm/kvm_x86_emulate.h |    2 +-
 arch/x86/kvm/svm.c                     |   21 +++++++++++++++-
 arch/x86/kvm/vmx.c                     |   39 ++++++++++++++++++++++++-------
 arch/x86/kvm/x86.c                     |   10 ++++++-
 arch/x86/kvm/x86_emulate.c             |   16 ++++++++++++-
 6 files changed, 75 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 3fc4623..9853aa9 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -513,6 +513,8 @@ struct kvm_x86_ops {
 	void (*run)(struct kvm_vcpu *vcpu, struct kvm_run *run);
 	int (*handle_exit)(struct kvm_run *run, struct kvm_vcpu *vcpu);
 	void (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
+	void (*block_interrupt_shadow)(struct kvm_vcpu *vcpu);
+	void (*unblock_interrupt_shadow)(struct kvm_vcpu *vcpu);
 	void (*patch_hypercall)(struct kvm_vcpu *vcpu,
 				unsigned char *hypercall_addr);
 	int (*get_irq)(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/include/asm/kvm_x86_emulate.h b/arch/x86/include/asm/kvm_x86_emulate.h
index 6a15973..800c5b1 100644
--- a/arch/x86/include/asm/kvm_x86_emulate.h
+++ b/arch/x86/include/asm/kvm_x86_emulate.h
@@ -176,6 +176,6 @@ struct x86_emulate_ctxt {
 int x86_decode_insn(struct x86_emulate_ctxt *ctxt,
 		    struct x86_emulate_ops *ops);
 int x86_emulate_insn(struct x86_emulate_ctxt *ctxt,
-		     struct x86_emulate_ops *ops);
+		     struct x86_emulate_ops *ops, int *interruptibility);
 
 #endif /* _ASM_X86_KVM_X86_EMULATE_H */
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 3ffb695..86038a3 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -210,6 +210,22 @@ static int is_external_interrupt(u32 info)
 	return info == (SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR);
 }
 
+static void svm_block_interrupt_shadow(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	svm->vmcb->control.int_state |= SVM_INTERRUPT_SHADOW_MASK;
+	vcpu->arch.interrupt_window_open = 0;
+}
+
+static void svm_unblock_interrupt_shadow(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	svm->vmcb->control.int_state &= ~SVM_INTERRUPT_SHADOW_MASK;
+	vcpu->arch.interrupt_window_open = (svm->vcpu.arch.hflags & HF_GIF_MASK);
+}
+
 static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -223,9 +239,8 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 		       __func__, kvm_rip_read(vcpu), svm->next_rip);
 
 	kvm_rip_write(vcpu, svm->next_rip);
-	svm->vmcb->control.int_state &= ~SVM_INTERRUPT_SHADOW_MASK;
 
-	vcpu->arch.interrupt_window_open = (svm->vcpu.arch.hflags & HF_GIF_MASK);
+	svm_unblock_interrupt_shadow(vcpu);
 }
 
 static int has_svm(void)
@@ -2660,6 +2675,8 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.run = svm_vcpu_run,
 	.handle_exit = handle_exit,
 	.skip_emulated_instruction = skip_emulated_instruction,
+	.block_interrupt_shadow = svm_block_interrupt_shadow,
+	.unblock_interrupt_shadow = svm_unblock_interrupt_shadow,
 	.patch_hypercall = svm_patch_hypercall,
 	.get_irq = svm_get_irq,
 	.set_irq = svm_set_irq,
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c6997c0..5158c2b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -736,26 +736,45 @@ static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
 	vmcs_writel(GUEST_RFLAGS, rflags);
 }
 
-static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
+static void vmx_block_interrupt_shadow(struct kvm_vcpu *vcpu)
 {
-	unsigned long rip;
-	u32 interruptibility;
+	u32 interruptibility = vmcs_read32(GUEST_INTERRUPTIBILITY_INFO);
+	u32 interruptibility_mask = ((GUEST_INTR_STATE_STI | GUEST_INTR_STATE_MOV_SS));
 
-	rip = kvm_rip_read(vcpu);
-	rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
-	kvm_rip_write(vcpu, rip);
+	if (!(interruptibility & interruptibility_mask))
+		vmcs_write32(GUEST_INTERRUPTIBILITY_INFO,
+			     interruptibility | interruptibility_mask);
+	vcpu->arch.interrupt_window_open = 0;
+}
 
+static void vmx_unblock_interrupt_shadow(struct kvm_vcpu *vcpu)
+{
 	/*
 	 * We emulated an instruction, so temporary interrupt blocking
 	 * should be removed, if set.
 	 */
-	interruptibility = vmcs_read32(GUEST_INTERRUPTIBILITY_INFO);
-	if (interruptibility & 3)
+	u32 interruptibility = vmcs_read32(GUEST_INTERRUPTIBILITY_INFO);
+	u32 interruptibility_mask = ((GUEST_INTR_STATE_STI | GUEST_INTR_STATE_MOV_SS));
+
+	if (interruptibility & interruptibility_mask)
 		vmcs_write32(GUEST_INTERRUPTIBILITY_INFO,
-			     interruptibility & ~3);
+			     interruptibility & ~interruptibility_mask);
 	vcpu->arch.interrupt_window_open = 1;
 }
 
+static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
+{
+	unsigned long rip;
+
+	rip = kvm_rip_read(vcpu);
+	rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
+	kvm_rip_write(vcpu, rip);
+
+	/* skipping an emulated instruction also counts */
+	vmx_unblock_interrupt_shadow(vcpu);
+}
+
+
 static void vmx_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
 				bool has_error_code, u32 error_code)
 {
@@ -3727,6 +3746,8 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.run = vmx_vcpu_run,
 	.handle_exit = vmx_handle_exit,
 	.skip_emulated_instruction = skip_emulated_instruction,
+	.block_interrupt_shadow = vmx_block_interrupt_shadow,
+	.unblock_interrupt_shadow = vmx_unblock_interrupt_shadow,
 	.patch_hypercall = vmx_patch_hypercall,
 	.get_irq = vmx_get_irq,
 	.set_irq = vmx_inject_irq,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0bb4131..47daa23 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2364,7 +2364,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 			u16 error_code,
 			int emulation_type)
 {
-	int r;
+	int r, interruptibility;
 	struct decode_cache *c;
 
 	kvm_clear_exception_queue(vcpu);
@@ -2412,7 +2412,13 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 		}
 	}
 
-	r = x86_emulate_insn(&vcpu->arch.emulate_ctxt, &emulate_ops);
+	interruptibility = 0;
+	r = x86_emulate_insn(&vcpu->arch.emulate_ctxt, &emulate_ops, &interruptibility);
+
+	if (interruptibility)
+		kvm_x86_ops->block_interrupt_shadow(vcpu);
+	else
+		kvm_x86_ops->unblock_interrupt_shadow(vcpu);
 
 	if (vcpu->arch.pio.string)
 		return EMULATE_DO_MMIO;
diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index d7c9f6f..f3507ec 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -1350,7 +1350,8 @@ static inline int writeback(struct x86_emulate_ctxt *ctxt,
 }
 
 int
-x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
+x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops,
+						int *interruptibility)
 {
 	unsigned long memop = 0;
 	u64 msr_data;
@@ -1359,6 +1360,10 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 	unsigned int port;
 	int io_dir_in;
 	int rc = 0;
+	static int movss_int_flag, movss_int_flag_old;
+
+	movss_int_flag_old = movss_int_flag;
+	movss_int_flag = 0;
 
 	/* Shadow copy of register state. Committed on successful emulation.
 	 * NOTE: we can copy them from vcpu as x86_decode_insn() doesn't
@@ -1610,6 +1615,13 @@ special_insn:
 
 		sel = c->src.val;
 		if (c->modrm_reg <= 5) {
+			if (c->modrm_reg == VCPU_SREG_SS) {
+				if (movss_int_flag_old)
+					*interruptibility = 1;
+				else
+					movss_int_flag = 1;
+			}
+
 			type_bits = (c->modrm_reg == 1) ? 9 : 1;
 			err = kvm_load_segment_descriptor(ctxt->vcpu, sel,
 							  type_bits, c->modrm_reg);
@@ -1864,6 +1876,8 @@ special_insn:
 		c->dst.type = OP_NONE;	/* Disable writeback. */
 		break;
 	case 0xfb: /* sti */
+		if (!(ctxt->eflags & X86_EFLAGS_IF))
+			*interruptibility = 1;
 		ctxt->eflags |= X86_EFLAGS_IF;
 		c->dst.type = OP_NONE;	/* Disable writeback. */
 		break;
-- 
1.5.6.6

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[Avi Kivity]

Glauber Costa wrote:
> we currently unblock shadow interrupt state when we skip an instruction,
> but failing to do so when we actually emulate one. This blocks interrupts
> in key instruction blocks, in particular sti; hlt; sequences
>
> If the instruction emulated is an sti, we have to block shadow interrupts.
> The same goes for mov ss. pop ss also needs it, but we don't currently
> emulate it. For sequences of two or more instructions of the same type
> among those instructions, only the first one has this effect.
>
> Without this patch, I cannot boot gpxe option roms at vmx machines.
> This is described at https://bugzilla.redhat.com/show_bug.cgi?id=494469
>
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -513,6 +513,8 @@ struct kvm_x86_ops {
>  	void (*run)(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  	int (*handle_exit)(struct kvm_run *run, struct kvm_vcpu *vcpu);
>  	void (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
> +	void (*block_interrupt_shadow)(struct kvm_vcpu *vcpu);
> +	void (*unblock_interrupt_shadow)(struct kvm_vcpu *vcpu);
>   

set_interrupt_shadow(), clear_interrupt_shadow().  The interrupt shadow 
blocks interrupts, but what happens when you block the interrupt shadow?

Maybe better to fold into one callback with a parameter.

>  int x86_emulate_insn(struct x86_emulate_ctxt *ctxt,
> -		     struct x86_emulate_ops *ops);
> +		     struct x86_emulate_ops *ops, int *interruptibility);
>   

Add to x86_emulate_ctxt, there's already some

>  
> +static void svm_block_interrupt_shadow(struct kvm_vcpu *vcpu)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +
> +	svm->vmcb->control.int_state |= SVM_INTERRUPT_SHADOW_MASK;
> +	vcpu->arch.interrupt_window_open = 0;
> +}
> +
> +static void svm_unblock_interrupt_shadow(struct kvm_vcpu *vcpu)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +
> +	svm->vmcb->control.int_state &= ~SVM_INTERRUPT_SHADOW_MASK;
> +	vcpu->arch.interrupt_window_open = (svm->vcpu.arch.hflags & HF_GIF_MASK);
>   

If eflags.if = 0, the interrupt window is closed.

> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index c6997c0..5158c2b 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -736,26 +736,45 @@ static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
>  	vmcs_writel(GUEST_RFLAGS, rflags);
>  }
>  
> -static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
> +static void vmx_block_interrupt_shadow(struct kvm_vcpu *vcpu)
>  {
> -	unsigned long rip;
> -	u32 interruptibility;
> +	u32 interruptibility = vmcs_read32(GUEST_INTERRUPTIBILITY_INFO);
> +	u32 interruptibility_mask = ((GUEST_INTR_STATE_STI | GUEST_INTR_STATE_MOV_SS));
>  
> -	rip = kvm_rip_read(vcpu);
> -	rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
> -	kvm_rip_write(vcpu, rip);
> +	if (!(interruptibility & interruptibility_mask))
> +		vmcs_write32(GUEST_INTERRUPTIBILITY_INFO,
> +			     interruptibility | interruptibility_mask);
> +	vcpu->arch.interrupt_window_open = 0;
>   

Setting both _MOV_SS and _STI is wierd; can't happen on real hardware.

>  {
>  	unsigned long memop = 0;
>  	u64 msr_data;
> @@ -1359,6 +1360,10 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
>  	unsigned int port;
>  	int io_dir_in;
>  	int rc = 0;
> +	static int movss_int_flag, movss_int_flag_old;
>   

static, for shame.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[H. Peter Anvin]

Avi Kivity wrote:
>>  
>> -    rip = kvm_rip_read(vcpu);
>> -    rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
>> -    kvm_rip_write(vcpu, rip);
>> +    if (!(interruptibility & interruptibility_mask))
>> +        vmcs_write32(GUEST_INTERRUPTIBILITY_INFO,
>> +                 interruptibility | interruptibility_mask);
>> +    vcpu->arch.interrupt_window_open = 0;
>>   
> 
> Setting both _MOV_SS and _STI is wierd; can't happen on real hardware.
> 

Not at architecturally visible boundaries, for sure.  It can be an
implementation artifact internally to an instruction, though.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

[PATCH] deal with interrupt shadow state for emulated instruction

[Glauber Costa]

we currently unblock shadow interrupt state when we skip an instruction,
but failing to do so when we actually emulate one. This blocks interrupts
in key instruction blocks, in particular sti; hlt; sequences

If the instruction emulated is an sti, we have to block shadow interrupts.
The same goes for mov ss. pop ss also needs it, but we don't currently
emulate it. For sequences of two or more instructions of the same type
among those instructions, only the first one has this effect.

Without this patch, I cannot boot gpxe option roms at vmx machines.
This is described at https://bugzilla.redhat.com/show_bug.cgi?id=494469

Signed-off-by: Glauber Costa <glommer@redhat.com>
CC: H. Peter Anvin <hpa@zytor.com>
CC: Avi Kivity <avi@redhat.com>
---
 arch/x86/include/asm/kvm_host.h        |    1 +
 arch/x86/include/asm/kvm_x86_emulate.h |    7 +++++
 arch/x86/kvm/svm.c                     |   19 +++++++++++++-
 arch/x86/kvm/vmx.c                     |   42 ++++++++++++++++++++++++-------
 arch/x86/kvm/x86.c                     |    6 ++++-
 arch/x86/kvm/x86_emulate.c             |   14 ++++++++++
 6 files changed, 76 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 3fc4623..0db1be7 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -513,6 +513,7 @@ struct kvm_x86_ops {
 	void (*run)(struct kvm_vcpu *vcpu, struct kvm_run *run);
 	int (*handle_exit)(struct kvm_run *run, struct kvm_vcpu *vcpu);
 	void (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
+	void (*interrupt_shadow_mask)(struct kvm_vcpu *vcpu, int mask);
 	void (*patch_hypercall)(struct kvm_vcpu *vcpu,
 				unsigned char *hypercall_addr);
 	int (*get_irq)(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/include/asm/kvm_x86_emulate.h b/arch/x86/include/asm/kvm_x86_emulate.h
index 6a15973..6c15498 100644
--- a/arch/x86/include/asm/kvm_x86_emulate.h
+++ b/arch/x86/include/asm/kvm_x86_emulate.h
@@ -143,6 +143,9 @@ struct decode_cache {
 	struct fetch_cache fetch;
 };
 
+#define X86_SHADOW_INT_MOV_SS	1
+#define X86_SHADOW_INT_STI	2
+
 struct x86_emulate_ctxt {
 	/* Register state before/after emulation. */
 	struct kvm_vcpu *vcpu;
@@ -152,6 +155,10 @@ struct x86_emulate_ctxt {
 	int mode;
 	u32 cs_base;
 
+	/* interruptibility state, as a result of execution of STI or MOV SS */
+	int interruptibility;
+	int movss_int_flag, movss_int_flag_old;
+
 	/* decode cache */
 	struct decode_cache decode;
 };
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 3ffb695..f41cb08 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -210,6 +210,21 @@ static int is_external_interrupt(u32 info)
 	return info == (SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR);
 }
 
+static void svm_interrupt_shadow_mask(struct kvm_vcpu *vcpu, int mask)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	if (mask == 0)
+		svm->vmcb->control.int_state &= ~SVM_INTERRUPT_SHADOW_MASK;
+	else
+		svm->vmcb->control.int_state |= SVM_INTERRUPT_SHADOW_MASK;
+
+	svm->vcpu.arch.interrupt_window_open =
+		(!(svm->vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) &&
+		 (svm->vmcb->save.rflags & X86_EFLAGS_IF) &&
+		 (svm->vcpu.arch.hflags & HF_GIF_MASK));
+}
+
 static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -223,9 +238,8 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 		       __func__, kvm_rip_read(vcpu), svm->next_rip);
 
 	kvm_rip_write(vcpu, svm->next_rip);
-	svm->vmcb->control.int_state &= ~SVM_INTERRUPT_SHADOW_MASK;
 
-	vcpu->arch.interrupt_window_open = (svm->vcpu.arch.hflags & HF_GIF_MASK);
+	svm_interrupt_shadow_mask(vcpu, 0);
 }
 
 static int has_svm(void)
@@ -2660,6 +2674,7 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.run = svm_vcpu_run,
 	.handle_exit = handle_exit,
 	.skip_emulated_instruction = skip_emulated_instruction,
+	.interrupt_shadow_mask = svm_interrupt_shadow_mask,
 	.patch_hypercall = svm_patch_hypercall,
 	.get_irq = svm_get_irq,
 	.set_irq = svm_set_irq,
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c6997c0..07b0203 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -736,26 +736,47 @@ static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
 	vmcs_writel(GUEST_RFLAGS, rflags);
 }
 
+static void vmx_interrupt_shadow_mask(struct kvm_vcpu *vcpu, int mask)
+{
+	u32 interruptibility_old = vmcs_read32(GUEST_INTERRUPTIBILITY_INFO);
+	u32 interruptibility = interruptibility_old;
+
+	switch (mask) {
+	case 0:
+		interruptibility &= ~((GUEST_INTR_STATE_STI | GUEST_INTR_STATE_MOV_SS));
+		break;
+	case X86_SHADOW_INT_MOV_SS:
+		interruptibility |= GUEST_INTR_STATE_MOV_SS;
+		break;
+	case X86_SHADOW_INT_STI:
+		interruptibility |= GUEST_INTR_STATE_STI;
+		break;
+	default:
+		printk(KERN_ERR "Bogus mask for interrupt shadow!\n");
+	}
+
+	if ((interruptibility != interruptibility_old))
+		vmcs_write32(GUEST_INTERRUPTIBILITY_INFO, interruptibility);
+
+	vcpu->arch.interrupt_window_open =
+		((vmcs_readl(GUEST_RFLAGS) & X86_EFLAGS_IF) &&
+		 !(interruptibility & (GUEST_INTR_STATE_STI |
+				       GUEST_INTR_STATE_MOV_SS)));
+}
+
 static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 {
 	unsigned long rip;
-	u32 interruptibility;
 
 	rip = kvm_rip_read(vcpu);
 	rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
 	kvm_rip_write(vcpu, rip);
 
-	/*
-	 * We emulated an instruction, so temporary interrupt blocking
-	 * should be removed, if set.
-	 */
-	interruptibility = vmcs_read32(GUEST_INTERRUPTIBILITY_INFO);
-	if (interruptibility & 3)
-		vmcs_write32(GUEST_INTERRUPTIBILITY_INFO,
-			     interruptibility & ~3);
-	vcpu->arch.interrupt_window_open = 1;
+	/* skipping an emulated instruction also counts */
+	vmx_interrupt_shadow_mask(vcpu, 0);
 }
 
+
 static void vmx_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
 				bool has_error_code, u32 error_code)
 {
@@ -3727,6 +3748,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.run = vmx_vcpu_run,
 	.handle_exit = vmx_handle_exit,
 	.skip_emulated_instruction = skip_emulated_instruction,
+	.interrupt_shadow_mask = vmx_interrupt_shadow_mask,
 	.patch_hypercall = vmx_patch_hypercall,
 	.get_irq = vmx_get_irq,
 	.set_irq = vmx_inject_irq,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0bb4131..b1fc8b6 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2364,7 +2364,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 			u16 error_code,
 			int emulation_type)
 {
-	int r;
+	int r, shadow_mask;
 	struct decode_cache *c;
 
 	kvm_clear_exception_queue(vcpu);
@@ -2412,8 +2412,12 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 		}
 	}
 
+	vcpu->arch.emulate_ctxt.interruptibility = 0;
 	r = x86_emulate_insn(&vcpu->arch.emulate_ctxt, &emulate_ops);
 
+	shadow_mask = vcpu->arch.emulate_ctxt.interruptibility;
+	kvm_x86_ops->interrupt_shadow_mask(vcpu, shadow_mask);
+
 	if (vcpu->arch.pio.string)
 		return EMULATE_DO_MMIO;
 
diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index d7c9f6f..1369a2e 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -1360,6 +1360,10 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 	int io_dir_in;
 	int rc = 0;
 
+	ctxt->movss_int_flag_old = ctxt->movss_int_flag;
+
+	ctxt->movss_int_flag = 0;
+
 	/* Shadow copy of register state. Committed on successful emulation.
 	 * NOTE: we can copy them from vcpu as x86_decode_insn() doesn't
 	 * modify them.
@@ -1610,6 +1614,14 @@ special_insn:
 
 		sel = c->src.val;
 		if (c->modrm_reg <= 5) {
+			if (c->modrm_reg == VCPU_SREG_SS) {
+				if (ctxt->movss_int_flag_old)
+					ctxt->interruptibility |=
+						X86_SHADOW_INT_MOV_SS;
+				else
+					ctxt->movss_int_flag = 1;
+			}
+
 			type_bits = (c->modrm_reg == 1) ? 9 : 1;
 			err = kvm_load_segment_descriptor(ctxt->vcpu, sel,
 							  type_bits, c->modrm_reg);
@@ -1864,6 +1876,8 @@ special_insn:
 		c->dst.type = OP_NONE;	/* Disable writeback. */
 		break;
 	case 0xfb: /* sti */
+		if (!(ctxt->eflags & X86_EFLAGS_IF))
+			ctxt->interruptibility |= X86_SHADOW_INT_STI;
 		ctxt->eflags |= X86_EFLAGS_IF;
 		c->dst.type = OP_NONE;	/* Disable writeback. */
 		break;
-- 
1.5.6.6

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[Gleb Natapov]

On Mon, Apr 13, 2009 at 04:06:50PM -0400, Glauber Costa wrote:
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 3fc4623..0db1be7 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -513,6 +513,7 @@ struct kvm_x86_ops {
>  	void (*run)(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  	int (*handle_exit)(struct kvm_run *run, struct kvm_vcpu *vcpu);
>  	void (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
> +	void (*interrupt_shadow_mask)(struct kvm_vcpu *vcpu, int mask);

Note that this conflicts with my "Move interrupt injection logic to x86.c"
patch. It adds drop_interrupt_shadow callback. Your callback is more
general so it should replace mine when/if my patchset will go in.

--
			Gleb.

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[Avi Kivity]

Glauber Costa wrote:
> we currently unblock shadow interrupt state when we skip an instruction,
> but failing to do so when we actually emulate one. This blocks interrupts
> in key instruction blocks, in particular sti; hlt; sequences
>
> If the instruction emulated is an sti, we have to block shadow interrupts.
> The same goes for mov ss. pop ss also needs it, but we don't currently
> emulate it. For sequences of two or more instructions of the same type
> among those instructions, only the first one has this effect.
>
> Without this patch, I cannot boot gpxe option roms at vmx machines.
> This is described at https://bugzilla.redhat.com/show_bug.cgi?id=494469
>
>   

We'll defer this until after Gleb's patchset, since that's much bigger.

> +#define X86_SHADOW_INT_MOV_SS	1
> +#define X86_SHADOW_INT_STI	2
> +
>  struct x86_emulate_ctxt {
>  	/* Register state before/after emulation. */
>  	struct kvm_vcpu *vcpu;
> @@ -152,6 +155,10 @@ struct x86_emulate_ctxt {
>  	int mode;
>  	u32 cs_base;
>  
> +	/* interruptibility state, as a result of execution of STI or MOV SS */
> +	int interruptibility;
> +	int movss_int_flag, movss_int_flag_old;
> +
>   

bit masks are traditionally unsigned.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 0bb4131..b1fc8b6 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2364,7 +2364,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
>  			u16 error_code,
>  			int emulation_type)
>  {
> -	int r;
> +	int r, shadow_mask;
>  	struct decode_cache *c;
>  
>  	kvm_clear_exception_queue(vcpu);
> @@ -2412,8 +2412,12 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
>  		}
>  	}
>  
> +	vcpu->arch.emulate_ctxt.interruptibility = 0;
>  	r = x86_emulate_insn(&vcpu->arch.emulate_ctxt, &emulate_ops);
>  
> +	shadow_mask = vcpu->arch.emulate_ctxt.interruptibility;
> +	kvm_x86_ops->interrupt_shadow_mask(vcpu, shadow_mask);
> +
>   

Emulation may have failed, in which case you don't want to update the 
interrupt shadow mask.

>  	if (vcpu->arch.pio.string)
>  		return EMULATE_DO_MMIO;
>  
> diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
> index d7c9f6f..1369a2e 100644
> --- a/arch/x86/kvm/x86_emulate.c
> +++ b/arch/x86/kvm/x86_emulate.c
> @@ -1360,6 +1360,10 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
>  	int io_dir_in;
>  	int rc = 0;
>  
> +	ctxt->movss_int_flag_old = ctxt->movss_int_flag;
> +
> +	ctxt->movss_int_flag = 0;
>   

This seem to be internal to the emulator.  However, instructions may be 
executed outside the emulator, invalidating movss_int_flag.  But see below.

> @@ -1610,6 +1614,14 @@ special_insn:
>  
>  		sel = c->src.val;
>  		if (c->modrm_reg <= 5) {
> +			if (c->modrm_reg == VCPU_SREG_SS) {
> +				if (ctxt->movss_int_flag_old)
> +					ctxt->interruptibility |=
> +						X86_SHADOW_INT_MOV_SS;
> +				else
> +					ctxt->movss_int_flag = 1;
> +			}
>   

The comment about repeating 'mov ss' in the manual has that wonderful 
word in it, May.  That means we're perfectly allowed to ignore it and 
just set the flag unconditionally.

I doubt we'll ever see a repeated 'mov ss', once is more than enough.



-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[H. Peter Anvin]

Avi Kivity wrote:
> 
> The comment about repeating 'mov ss' in the manual has that wonderful
> word in it, May.  That means we're perfectly allowed to ignore it and
> just set the flag unconditionally.
> 

Realistically, though, this should only be done for a limited number of
sequential instructions.

> I doubt we'll ever see a repeated 'mov ss', once is more than enough.

True enough, except maliciously.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[Avi Kivity]

H. Peter Anvin wrote:
> Avi Kivity wrote:
>   
>> The comment about repeating 'mov ss' in the manual has that wonderful
>> word in it, May.  That means we're perfectly allowed to ignore it and
>> just set the flag unconditionally.
>>
>>     
>
> Realistically, though, this should only be done for a limited number of
> sequential instructions.
>
>   

Why?  Do you see a guest filling all of memory with 'mov ss' and 
expecting to break out of it via an interrupt?

>> I doubt we'll ever see a repeated 'mov ss', once is more than enough.
>>     
>
> True enough, except maliciously.
>   

Why do we care?  The guest can only harm itself, and if it wants to 
disable interrupts, it would be a lot easier for it to run a plain 'cli'.

I guess it would be a problem if we emulated 'mov ss' for ordinary 
userspace or vm86 mode, but we don't.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[H. Peter Anvin]

Avi Kivity wrote:
> 
> Why do we care?  The guest can only harm itself, and if it wants to
> disable interrupts, it would be a lot easier for it to run a plain 'cli'.
> 
> I guess it would be a problem if we emulated 'mov ss' for ordinary
> userspace or vm86 mode, but we don't.
> 

Well, the answer is that mov ss is an unprivileged instruction.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[Alan Cox]

> Why?  Do you see a guest filling all of memory with 'mov ss' and 
> expecting to break out of it via an interrupt?

Well I did try mapping a page of move ss all through memory on real
hardware long ago and seeing what happened on a 386 in real mode with
DOSEMU. I was disappointed ;)

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[H. Peter Anvin]

Alan Cox wrote:
>> Why?  Do you see a guest filling all of memory with 'mov ss' and 
>> expecting to break out of it via an interrupt?
> 
> Well I did try mapping a page of move ss all through memory on real
> hardware long ago and seeing what happened on a 386 in real mode with
> DOSEMU. I was disappointed ;)
> 

Heheheheh....

	-hpa

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[Avi Kivity]

H. Peter Anvin wrote:
> Avi Kivity wrote:
>   
>> Why do we care?  The guest can only harm itself, and if it wants to
>> disable interrupts, it would be a lot easier for it to run a plain 'cli'.
>>
>> I guess it would be a problem if we emulated 'mov ss' for ordinary
>> userspace or vm86 mode, but we don't.
>>
>>     
>
> Well, the answer is that mov ss is an unprivileged instruction.
>
>   

We don't emulate guest user mode.

Well, if guest userspace can convince its kernel to give it access to 
some memory mapped I/O register, I guess it can execute repeated 'mov 
ss, mmio' and starve the guest kernel.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[H. Peter Anvin]

Avi Kivity wrote:
> 
> We don't emulate guest user mode.
> 
> Well, if guest userspace can convince its kernel to give it access to 
> some memory mapped I/O register, I guess it can execute repeated 'mov 
> ss, mmio' and starve the guest kernel.
> 

It doesn't need a MMIO register to do that, even.

	-hpa

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[Avi Kivity]

H. Peter Anvin wrote:
> Avi Kivity wrote:
>>
>> We don't emulate guest user mode.
>>
>> Well, if guest userspace can convince its kernel to give it access to 
>> some memory mapped I/O register, I guess it can execute repeated 'mov 
>> ss, mmio' and starve the guest kernel.
>>
>
> It doesn't need a MMIO register to do that, even.
>

Can you explain how?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[PATCH] deal with interrupt shadow state for emulated instruction

[Glauber Costa]

we currently unblock shadow interrupt state when we skip an instruction,
but failing to do so when we actually emulate one. This blocks interrupts
in key instruction blocks, in particular sti; hlt; sequences

If the instruction emulated is an sti, we have to block shadow interrupts.
The same goes for mov ss. pop ss also needs it, but we don't currently
emulate it.

Without this patch, I cannot boot gpxe option roms at vmx machines.
This is described at https://bugzilla.redhat.com/show_bug.cgi?id=494469

Signed-off-by: Glauber Costa <glommer@redhat.com>
CC: H. Peter Anvin <hpa@zytor.com>
CC: Avi Kivity <avi@redhat.com>
---
 arch/x86/include/asm/kvm_host.h        |    2 +
 arch/x86/include/asm/kvm_x86_emulate.h |    6 +++
 arch/x86/kvm/svm.c                     |   31 ++++++++++++++++--
 arch/x86/kvm/vmx.c                     |   55 ++++++++++++++++++++++++++------
 arch/x86/kvm/x86.c                     |    7 +++-
 arch/x86/kvm/x86_emulate.c             |   25 ++++++++++++++-
 6 files changed, 111 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index cb306cf..9455a30 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -510,6 +510,8 @@ struct kvm_x86_ops {
 	void (*run)(struct kvm_vcpu *vcpu, struct kvm_run *run);
 	int (*handle_exit)(struct kvm_run *run, struct kvm_vcpu *vcpu);
 	void (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
+	void (*interrupt_shadow_mask)(struct kvm_vcpu *vcpu, int mask);
+	u32 (*get_interrupt_shadow)(struct kvm_vcpu *vcpu);
 	void (*patch_hypercall)(struct kvm_vcpu *vcpu,
 				unsigned char *hypercall_addr);
 	int (*get_irq)(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/include/asm/kvm_x86_emulate.h b/arch/x86/include/asm/kvm_x86_emulate.h
index 6a15973..b7ed2c4 100644
--- a/arch/x86/include/asm/kvm_x86_emulate.h
+++ b/arch/x86/include/asm/kvm_x86_emulate.h
@@ -143,6 +143,9 @@ struct decode_cache {
 	struct fetch_cache fetch;
 };
 
+#define X86_SHADOW_INT_MOV_SS  1
+#define X86_SHADOW_INT_STI     2
+
 struct x86_emulate_ctxt {
 	/* Register state before/after emulation. */
 	struct kvm_vcpu *vcpu;
@@ -152,6 +155,9 @@ struct x86_emulate_ctxt {
 	int mode;
 	u32 cs_base;
 
+	/* interruptibility state, as a result of execution of STI or MOV SS */
+	int interruptibility;
+
 	/* decode cache */
 	struct decode_cache decode;
 };
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 053f3c5..a6af946 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -210,6 +210,31 @@ static int is_external_interrupt(u32 info)
 	return info == (SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR);
 }
 
+static u32 svm_get_interrupt_shadow(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+	u32 ret = 0;
+
+	if (svm->vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK)
+		ret |= (X86_SHADOW_INT_STI && X86_SHADOW_INT_MOV_SS);
+	return ret;
+}
+
+static void svm_interrupt_shadow_mask(struct kvm_vcpu *vcpu, int mask)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	if (mask == 0)
+		svm->vmcb->control.int_state &= ~SVM_INTERRUPT_SHADOW_MASK;
+	else
+		svm->vmcb->control.int_state |= SVM_INTERRUPT_SHADOW_MASK;
+
+	svm->vcpu.arch.interrupt_window_open =
+	       (!(svm->vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) &&
+		(svm->vmcb->save.rflags & X86_EFLAGS_IF) &&
+		(svm->vcpu.arch.hflags & HF_GIF_MASK));
+}
+
 static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -223,9 +248,7 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 		       __func__, kvm_rip_read(vcpu), svm->next_rip);
 
 	kvm_rip_write(vcpu, svm->next_rip);
-	svm->vmcb->control.int_state &= ~SVM_INTERRUPT_SHADOW_MASK;
-
-	vcpu->arch.interrupt_window_open = (svm->vcpu.arch.hflags & HF_GIF_MASK);
+	svm_interrupt_shadow_mask(vcpu, 0);
 }
 
 static int has_svm(void)
@@ -2667,6 +2690,8 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.run = svm_vcpu_run,
 	.handle_exit = handle_exit,
 	.skip_emulated_instruction = skip_emulated_instruction,
+	.interrupt_shadow_mask = svm_interrupt_shadow_mask,
+	.get_interrupt_shadow = svm_get_interrupt_shadow,
 	.patch_hypercall = svm_patch_hypercall,
 	.get_irq = svm_get_irq,
 	.set_irq = svm_set_irq,
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c6997c0..87995ed 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -736,24 +736,57 @@ static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
 	vmcs_writel(GUEST_RFLAGS, rflags);
 }
 
+static u32 vmx_get_interrupt_shadow(struct kvm_vcpu *vcpu)
+{
+	u32 interruptibility = vmcs_read32(GUEST_INTERRUPTIBILITY_INFO);
+	int ret = 0;
+
+	if (interruptibility & GUEST_INTR_STATE_STI)
+		ret |= X86_SHADOW_INT_STI;
+	if (interruptibility & GUEST_INTR_STATE_MOV_SS)
+		ret |= X86_SHADOW_INT_MOV_SS;
+
+	return ret;
+}
+
+static void vmx_interrupt_shadow_mask(struct kvm_vcpu *vcpu, int mask)
+{
+	u32 interruptibility_old = vmcs_read32(GUEST_INTERRUPTIBILITY_INFO);
+	u32 interruptibility = interruptibility_old;
+
+	switch (mask) {
+	case 0:
+		interruptibility &= ~((GUEST_INTR_STATE_STI | GUEST_INTR_STATE_MOV_SS));
+		break;
+	case X86_SHADOW_INT_MOV_SS:
+		interruptibility |= GUEST_INTR_STATE_MOV_SS;
+		break;
+	case X86_SHADOW_INT_STI:
+		interruptibility |= GUEST_INTR_STATE_STI;
+		break;
+	default:
+		printk(KERN_ERR "Bogus mask for interrupt shadow!\n");
+	}
+
+	if ((interruptibility != interruptibility_old))
+		vmcs_write32(GUEST_INTERRUPTIBILITY_INFO, interruptibility);
+
+	vcpu->arch.interrupt_window_open =
+	       ((vmcs_readl(GUEST_RFLAGS) & X86_EFLAGS_IF) &&
+		!(interruptibility & (GUEST_INTR_STATE_STI |
+				      GUEST_INTR_STATE_MOV_SS)));
+}
+
 static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 {
 	unsigned long rip;
-	u32 interruptibility;
 
 	rip = kvm_rip_read(vcpu);
 	rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
 	kvm_rip_write(vcpu, rip);
 
-	/*
-	 * We emulated an instruction, so temporary interrupt blocking
-	 * should be removed, if set.
-	 */
-	interruptibility = vmcs_read32(GUEST_INTERRUPTIBILITY_INFO);
-	if (interruptibility & 3)
-		vmcs_write32(GUEST_INTERRUPTIBILITY_INFO,
-			     interruptibility & ~3);
-	vcpu->arch.interrupt_window_open = 1;
+	/* skipping an emulated instruction also counts */
+	vmx_interrupt_shadow_mask(vcpu, 0);
 }
 
 static void vmx_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
@@ -3727,6 +3760,8 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.run = vmx_vcpu_run,
 	.handle_exit = vmx_handle_exit,
 	.skip_emulated_instruction = skip_emulated_instruction,
+	.interrupt_shadow_mask = vmx_interrupt_shadow_mask,
+	.get_interrupt_shadow = vmx_get_interrupt_shadow,
 	.patch_hypercall = vmx_patch_hypercall,
 	.get_irq = vmx_get_irq,
 	.set_irq = vmx_inject_irq,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ab1fdac..fd58ccc 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2362,7 +2362,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 			u16 error_code,
 			int emulation_type)
 {
-	int r;
+	int r, shadow_mask;
 	struct decode_cache *c;
 
 	kvm_clear_exception_queue(vcpu);
@@ -2415,7 +2415,12 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 		return EMULATE_DONE;
 	}
 
+	vcpu->arch.emulate_ctxt.interruptibility = 0;
 	r = x86_emulate_insn(&vcpu->arch.emulate_ctxt, &emulate_ops);
+	shadow_mask = vcpu->arch.emulate_ctxt.interruptibility;
+
+	if (r == 0)
+		kvm_x86_ops->interrupt_shadow_mask(vcpu, shadow_mask);
 
 	if (vcpu->arch.pio.string)
 		return EMULATE_DO_MMIO;
diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index d2664fc..797d41f 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -1618,6 +1618,16 @@ special_insn:
 		int err;
 
 		sel = c->src.val;
+		if (c->modrm_reg == VCPU_SREG_SS) {
+			u32 int_shadow =
+				kvm_x86_ops->get_interrupt_shadow(ctxt->vcpu);
+			/* See sti emulation for an explanation of this */
+			if ((int_shadow & X86_SHADOW_INT_MOV_SS))
+				ctxt->interruptibility &= ~X86_SHADOW_INT_MOV_SS;
+			else
+				ctxt->interruptibility |= X86_SHADOW_INT_MOV_SS;
+		}
+
 		if (c->modrm_reg <= 5) {
 			type_bits = (c->modrm_reg == 1) ? 9 : 1;
 			err = kvm_load_segment_descriptor(ctxt->vcpu, sel,
@@ -1846,10 +1856,23 @@ special_insn:
 		ctxt->eflags &= ~X86_EFLAGS_IF;
 		c->dst.type = OP_NONE;	/* Disable writeback. */
 		break;
-	case 0xfb: /* sti */
+	case 0xfb: { /* sti */
+		u32 int_shadow = kvm_x86_ops->get_interrupt_shadow(ctxt->vcpu);
+		/*
+		 * an sti; sti; sequence only disable interrupts for the first
+		 * instruction. So, if the last instruction, be it emulated or
+		 * not, left the system with the INT_STI flag enabled, it
+		 * means that the last instruction is an sti. We should not
+		 * leave the flag on in this case
+		 */
+		if ((int_shadow & X86_SHADOW_INT_STI))
+			ctxt->interruptibility &= ~X86_SHADOW_INT_STI;
+		else
+			ctxt->interruptibility |= X86_SHADOW_INT_STI;
 		ctxt->eflags |= X86_EFLAGS_IF;
 		c->dst.type = OP_NONE;	/* Disable writeback. */
 		break;
+	}
 	case 0xfc: /* cld */
 		ctxt->eflags &= ~EFLG_DF;
 		c->dst.type = OP_NONE;	/* Disable writeback. */
-- 
1.5.6.6

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[Avi Kivity]

Glauber Costa wrote:
> we currently unblock shadow interrupt state when we skip an instruction,
> but failing to do so when we actually emulate one. This blocks interrupts
> in key instruction blocks, in particular sti; hlt; sequences
>
> If the instruction emulated is an sti, we have to block shadow interrupts.
> The same goes for mov ss. pop ss also needs it, but we don't currently
> emulate it.
>
> Without this patch, I cannot boot gpxe option roms at vmx machines.
> This is described at https://bugzilla.redhat.com/show_bug.cgi?id=494469
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index cb306cf..9455a30 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -510,6 +510,8 @@ struct kvm_x86_ops {
>  	void (*run)(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  	int (*handle_exit)(struct kvm_run *run, struct kvm_vcpu *vcpu);
>  	void (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
> +	void (*interrupt_shadow_mask)(struct kvm_vcpu *vcpu, int mask);
>   

Can you verb this function?  set_interrupt_shadow would make it nicely 
complement get_interrupt_shadow.

> +	u32 (*get_interrupt_shadow)(struct kvm_vcpu *vcpu);
>  	void (*patch_hypercall)(struct kvm_vcpu *vcpu,
>  				unsigned char *hypercall_addr);
>  	int (*get_irq)(struct kvm_vcpu *vcpu);
>  
> +static u32 svm_get_interrupt_shadow(struct kvm_vcpu *vcpu)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +	u32 ret = 0;
> +
> +	if (svm->vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK)
> +		ret |= (X86_SHADOW_INT_STI && X86_SHADOW_INT_MOV_SS);
> +	return ret;
> +}
>   

Hmm, if the guest runs an infinite emulated 'mov ss', it will keep 
toggling the MOV_SS bit, but STI will remain set, so we'll never allow 
an interrupt into the guest kernel.

> diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
> index d2664fc..797d41f 100644
> --- a/arch/x86/kvm/x86_emulate.c
> +++ b/arch/x86/kvm/x86_emulate.c
> @@ -1618,6 +1618,16 @@ special_insn:
>  		int err;
>  
>  		sel = c->src.val;
> +		if (c->modrm_reg == VCPU_SREG_SS) {
> +			u32 int_shadow =
> +				kvm_x86_ops->get_interrupt_shadow(ctxt->vcpu);
> +			/* See sti emulation for an explanation of this */
> +			if ((int_shadow & X86_SHADOW_INT_MOV_SS))
> +				ctxt->interruptibility &= ~X86_SHADOW_INT_MOV_SS;
> +			else
> +				ctxt->interruptibility |= X86_SHADOW_INT_MOV_SS;
> +		}
>   

^=

> @@ -1846,10 +1856,23 @@ special_insn:
>  		ctxt->eflags &= ~X86_EFLAGS_IF;
>  		c->dst.type = OP_NONE;	/* Disable writeback. */
>  		break;
> -	case 0xfb: /* sti */
> +	case 0xfb: { /* sti */
> +		u32 int_shadow = kvm_x86_ops->get_interrupt_shadow(ctxt->vcpu);
> +		/*
> +		 * an sti; sti; sequence only disable interrupts for the first
> +		 * instruction. So, if the last instruction, be it emulated or
> +		 * not, left the system with the INT_STI flag enabled, it
> +		 * means that the last instruction is an sti. We should not
> +		 * leave the flag on in this case
> +		 */
> +		if ((int_shadow & X86_SHADOW_INT_STI))
> +			ctxt->interruptibility &= ~X86_SHADOW_INT_STI;
> +		else
> +			ctxt->interruptibility |= X86_SHADOW_INT_STI;
>   

^=

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[Glauber Costa]

> Hmm, if the guest runs an infinite emulated 'mov ss', it will keep  
> toggling the MOV_SS bit, but STI will remain set, so we'll never allow  
> an interrupt into the guest kernel.
We have no choice but returning both flags, since svm does not differentiate
between them.

But see below for an alternative path that makes it a non-issue.

>
>> diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
>> index d2664fc..797d41f 100644
>> --- a/arch/x86/kvm/x86_emulate.c
>> +++ b/arch/x86/kvm/x86_emulate.c
>> @@ -1618,6 +1618,16 @@ special_insn:
>>  		int err;
>>   		sel = c->src.val;
>> +		if (c->modrm_reg == VCPU_SREG_SS) {
>> +			u32 int_shadow =
>> +				kvm_x86_ops->get_interrupt_shadow(ctxt->vcpu);
>> +			/* See sti emulation for an explanation of this */
>> +			if ((int_shadow & X86_SHADOW_INT_MOV_SS))
>> +				ctxt->interruptibility &= ~X86_SHADOW_INT_MOV_SS;
>> +			else
>> +				ctxt->interruptibility |= X86_SHADOW_INT_MOV_SS;
>> +		}
>>   
>
> ^=
 =p \o/

After re-reading this, masking the flags in here makes no sense.

I am moving to an approach in which I do

                        if (!(int_shadow & X86_SHADOW_INT_MOV_SS))
                                ctxt->interruptibility = X86_SHADOW_INT_MOV_SS;

Since if the next instruction is an sti, it is certainly not an sti; sti instruction
(the current is mov ss, after all). So we should mask it anyway. This also solves
nicely the problem you raised at svm.c.
>
>> @@ -1846,10 +1856,23 @@ special_insn:
>>  		ctxt->eflags &= ~X86_EFLAGS_IF;
>>  		c->dst.type = OP_NONE;	/* Disable writeback. */
>>  		break;
>> -	case 0xfb: /* sti */
>> +	case 0xfb: { /* sti */
>> +		u32 int_shadow = kvm_x86_ops->get_interrupt_shadow(ctxt->vcpu);
>> +		/*
>> +		 * an sti; sti; sequence only disable interrupts for the first
>> +		 * instruction. So, if the last instruction, be it emulated or
>> +		 * not, left the system with the INT_STI flag enabled, it
>> +		 * means that the last instruction is an sti. We should not
>> +		 * leave the flag on in this case
>> +		 */
>> +		if ((int_shadow & X86_SHADOW_INT_STI))
>> +			ctxt->interruptibility &= ~X86_SHADOW_INT_STI;
>> +		else
>> +			ctxt->interruptibility |= X86_SHADOW_INT_STI;
>>   
>
> ^=
ditto

[PATCH] deal with interrupt shadow state for emulated instruction

[Glauber Costa]

we currently unblock shadow interrupt state when we skip an instruction,
but failing to do so when we actually emulate one. This blocks interrupts
in key instruction blocks, in particular sti; hlt; sequences

If the instruction emulated is an sti, we have to block shadow interrupts.
The same goes for mov ss. pop ss also needs it, but we don't currently
emulate it.

Without this patch, I cannot boot gpxe option roms at vmx machines.
This is described at https://bugzilla.redhat.com/show_bug.cgi?id=494469

Signed-off-by: Glauber Costa <glommer@redhat.com>
CC: H. Peter Anvin <hpa@zytor.com>
CC: Avi Kivity <avi@redhat.com>
---
 arch/x86/include/asm/kvm_host.h        |    2 +
 arch/x86/include/asm/kvm_x86_emulate.h |    6 ++++
 arch/x86/kvm/svm.c                     |   25 +++++++++++++++-
 arch/x86/kvm/vmx.c                     |   49 ++++++++++++++++++++++++++------
 arch/x86/kvm/x86.c                     |    7 ++++-
 arch/x86/kvm/x86_emulate.c             |   21 +++++++++++++-
 6 files changed, 98 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 8e680c3..a49d07b 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -510,6 +510,8 @@ struct kvm_x86_ops {
 	void (*run)(struct kvm_vcpu *vcpu, struct kvm_run *run);
 	int (*handle_exit)(struct kvm_run *run, struct kvm_vcpu *vcpu);
 	void (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
+	void (*set_interrupt_shadow)(struct kvm_vcpu *vcpu, int mask);
+	u32 (*get_interrupt_shadow)(struct kvm_vcpu *vcpu);
 	void (*patch_hypercall)(struct kvm_vcpu *vcpu,
 				unsigned char *hypercall_addr);
 	void (*set_irq)(struct kvm_vcpu *vcpu, int vec);
diff --git a/arch/x86/include/asm/kvm_x86_emulate.h b/arch/x86/include/asm/kvm_x86_emulate.h
index 6a15973..b7ed2c4 100644
--- a/arch/x86/include/asm/kvm_x86_emulate.h
+++ b/arch/x86/include/asm/kvm_x86_emulate.h
@@ -143,6 +143,9 @@ struct decode_cache {
 	struct fetch_cache fetch;
 };
 
+#define X86_SHADOW_INT_MOV_SS  1
+#define X86_SHADOW_INT_STI     2
+
 struct x86_emulate_ctxt {
 	/* Register state before/after emulation. */
 	struct kvm_vcpu *vcpu;
@@ -152,6 +155,9 @@ struct x86_emulate_ctxt {
 	int mode;
 	u32 cs_base;
 
+	/* interruptibility state, as a result of execution of STI or MOV SS */
+	int interruptibility;
+
 	/* decode cache */
 	struct decode_cache decode;
 };
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index ef43a18..4941dea 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -202,6 +202,27 @@ static int is_external_interrupt(u32 info)
 	return info == (SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR);
 }
 
+static u32 svm_get_interrupt_shadow(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+	u32 ret = 0;
+
+	if (svm->vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK)
+		ret |= (X86_SHADOW_INT_STI && X86_SHADOW_INT_MOV_SS);
+	return ret;
+}
+
+static void svm_set_interrupt_shadow(struct kvm_vcpu *vcpu, int mask)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	if (mask == 0)
+		svm->vmcb->control.int_state &= ~SVM_INTERRUPT_SHADOW_MASK;
+	else
+		svm->vmcb->control.int_state |= SVM_INTERRUPT_SHADOW_MASK;
+
+}
+
 static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -215,7 +236,7 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 		       __func__, kvm_rip_read(vcpu), svm->next_rip);
 
 	kvm_rip_write(vcpu, svm->next_rip);
-	svm->vmcb->control.int_state &= ~SVM_INTERRUPT_SHADOW_MASK;
+	svm_set_interrupt_shadow(vcpu, 0);
 }
 
 static int has_svm(void)
@@ -2637,6 +2658,8 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.run = svm_vcpu_run,
 	.handle_exit = handle_exit,
 	.skip_emulated_instruction = skip_emulated_instruction,
+	.set_interrupt_shadow= svm_set_interrupt_shadow,
+	.get_interrupt_shadow = svm_get_interrupt_shadow,
 	.patch_hypercall = svm_patch_hypercall,
 	.set_irq = svm_set_irq,
 	.set_nmi = svm_inject_nmi,
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index e8a5649..bbfe894 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -736,23 +736,52 @@ static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
 	vmcs_writel(GUEST_RFLAGS, rflags);
 }
 
+static u32 vmx_get_interrupt_shadow(struct kvm_vcpu *vcpu)
+{
+	u32 interruptibility = vmcs_read32(GUEST_INTERRUPTIBILITY_INFO);
+	int ret = 0;
+
+	if (interruptibility & GUEST_INTR_STATE_STI)
+		ret |= X86_SHADOW_INT_STI;
+	if (interruptibility & GUEST_INTR_STATE_MOV_SS)
+		ret |= X86_SHADOW_INT_MOV_SS;
+
+	return ret;
+}
+
+static void vmx_set_interrupt_shadow(struct kvm_vcpu *vcpu, int mask)
+{
+	u32 interruptibility_old = vmcs_read32(GUEST_INTERRUPTIBILITY_INFO);
+	u32 interruptibility = interruptibility_old;
+
+	switch (mask) {
+	case 0:
+		interruptibility &= ~((GUEST_INTR_STATE_STI | GUEST_INTR_STATE_MOV_SS));
+		break;
+	case X86_SHADOW_INT_MOV_SS:
+		interruptibility |= GUEST_INTR_STATE_MOV_SS;
+		break;
+	case X86_SHADOW_INT_STI:
+		interruptibility |= GUEST_INTR_STATE_STI;
+		break;
+	default:
+		printk(KERN_ERR "Bogus mask for interrupt shadow!\n");
+	}
+
+	if ((interruptibility != interruptibility_old))
+		vmcs_write32(GUEST_INTERRUPTIBILITY_INFO, interruptibility);
+}
+
 static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 {
 	unsigned long rip;
-	u32 interruptibility;
 
 	rip = kvm_rip_read(vcpu);
 	rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
 	kvm_rip_write(vcpu, rip);
 
-	/*
-	 * We emulated an instruction, so temporary interrupt blocking
-	 * should be removed, if set.
-	 */
-	interruptibility = vmcs_read32(GUEST_INTERRUPTIBILITY_INFO);
-	if (interruptibility & 3)
-		vmcs_write32(GUEST_INTERRUPTIBILITY_INFO,
-			     interruptibility & ~3);
+	/* skipping an emulated instruction also counts */
+	vmx_set_interrupt_shadow(vcpu, 0);
 }
 
 static void vmx_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
@@ -3649,6 +3678,8 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.run = vmx_vcpu_run,
 	.handle_exit = vmx_handle_exit,
 	.skip_emulated_instruction = skip_emulated_instruction,
+	.set_interrupt_shadow = vmx_set_interrupt_shadow,
+	.get_interrupt_shadow = vmx_get_interrupt_shadow,
 	.patch_hypercall = vmx_patch_hypercall,
 	.set_irq = vmx_inject_irq,
 	.set_nmi = vmx_inject_nmi,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2d7082c..fc468cc 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2362,7 +2362,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 			u16 error_code,
 			int emulation_type)
 {
-	int r;
+	int r, shadow_mask;
 	struct decode_cache *c;
 
 	kvm_clear_exception_queue(vcpu);
@@ -2415,7 +2415,12 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 		return EMULATE_DONE;
 	}
 
+	vcpu->arch.emulate_ctxt.interruptibility = 0;
 	r = x86_emulate_insn(&vcpu->arch.emulate_ctxt, &emulate_ops);
+	shadow_mask = vcpu->arch.emulate_ctxt.interruptibility;
+
+	if (r == 0)
+		kvm_x86_ops->set_interrupt_shadow(vcpu, shadow_mask);
 
 	if (vcpu->arch.pio.string)
 		return EMULATE_DO_MMIO;
diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index d2664fc..1d042d4 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -1618,6 +1618,14 @@ special_insn:
 		int err;
 
 		sel = c->src.val;
+		if (c->modrm_reg == VCPU_SREG_SS) {
+			u32 int_shadow =
+				kvm_x86_ops->get_interrupt_shadow(ctxt->vcpu);
+			/* See sti emulation for an explanation of this */
+			if (!(int_shadow & X86_SHADOW_INT_MOV_SS))
+				ctxt->interruptibility = X86_SHADOW_INT_MOV_SS;
+		}
+
 		if (c->modrm_reg <= 5) {
 			type_bits = (c->modrm_reg == 1) ? 9 : 1;
 			err = kvm_load_segment_descriptor(ctxt->vcpu, sel,
@@ -1846,10 +1854,21 @@ special_insn:
 		ctxt->eflags &= ~X86_EFLAGS_IF;
 		c->dst.type = OP_NONE;	/* Disable writeback. */
 		break;
-	case 0xfb: /* sti */
+	case 0xfb: { /* sti */
+		u32 int_shadow = kvm_x86_ops->get_interrupt_shadow(ctxt->vcpu);
+		/*
+		 * an sti; sti; sequence only disable interrupts for the first
+		 * instruction. So, if the last instruction, be it emulated or
+		 * not, left the system with the INT_STI flag enabled, it
+		 * means that the last instruction is an sti. We should not
+		 * leave the flag on in this case
+		 */
+		if (!(int_shadow & X86_SHADOW_INT_STI))
+			ctxt->interruptibility = X86_SHADOW_INT_STI;
 		ctxt->eflags |= X86_EFLAGS_IF;
 		c->dst.type = OP_NONE;	/* Disable writeback. */
 		break;
+	}
 	case 0xfc: /* cld */
 		ctxt->eflags &= ~EFLG_DF;
 		c->dst.type = OP_NONE;	/* Disable writeback. */
-- 
1.5.6.6

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[Gleb Natapov]

On Tue, May 05, 2009 at 02:40:11PM -0400, Glauber Costa wrote:
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 8e680c3..a49d07b 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -510,6 +510,8 @@ struct kvm_x86_ops {
>  	void (*run)(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  	int (*handle_exit)(struct kvm_run *run, struct kvm_vcpu *vcpu);
>  	void (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
> +	void (*set_interrupt_shadow)(struct kvm_vcpu *vcpu, int mask);
There is .drop_interrupt_shadow() callback. The patch should remove it and
replace its use by set_interrupt_shadow().

--
			Gleb.

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[Avi Kivity]

Gleb Natapov wrote:
> On Tue, May 05, 2009 at 02:40:11PM -0400, Glauber Costa wrote:
>   
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 8e680c3..a49d07b 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -510,6 +510,8 @@ struct kvm_x86_ops {
>>  	void (*run)(struct kvm_vcpu *vcpu, struct kvm_run *run);
>>  	int (*handle_exit)(struct kvm_run *run, struct kvm_vcpu *vcpu);
>>  	void (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
>> +	void (*set_interrupt_shadow)(struct kvm_vcpu *vcpu, int mask);
>>     
> There is .drop_interrupt_shadow() callback. The patch should remove it and
> replace its use by set_interrupt_shadow().
>   

That would be [PATCH 1/2].

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[Glauber Costa]

On Wed, May 06, 2009 at 01:51:04PM +0300, Avi Kivity wrote:
> Gleb Natapov wrote:
>> On Tue, May 05, 2009 at 02:40:11PM -0400, Glauber Costa wrote:
>>   
>>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>>> index 8e680c3..a49d07b 100644
>>> --- a/arch/x86/include/asm/kvm_host.h
>>> +++ b/arch/x86/include/asm/kvm_host.h
>>> @@ -510,6 +510,8 @@ struct kvm_x86_ops {
>>>  	void (*run)(struct kvm_vcpu *vcpu, struct kvm_run *run);
>>>  	int (*handle_exit)(struct kvm_run *run, struct kvm_vcpu *vcpu);
>>>  	void (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
>>> +	void (*set_interrupt_shadow)(struct kvm_vcpu *vcpu, int mask);
>>>     
>> There is .drop_interrupt_shadow() callback. The patch should remove it and
>> replace its use by set_interrupt_shadow().
>>   
>
> That would be [PATCH 1/2].
[PATCH 2/2]. Otherwise we will break bisectability, as the pure removal of this
function would lead us to a non-functioning kernel for no reason.

Avi: if this patch is okay, please apply. I'll send another one later that replaces
the existing .drop_interrupt_shadow by the (then) in tree set_interrupt_shadow.

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[Gleb Natapov]

On Fri, May 08, 2009 at 02:25:11AM -0300, Glauber Costa wrote:
> On Wed, May 06, 2009 at 01:51:04PM +0300, Avi Kivity wrote:
> > Gleb Natapov wrote:
> >> On Tue, May 05, 2009 at 02:40:11PM -0400, Glauber Costa wrote:
> >>   
> >>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> >>> index 8e680c3..a49d07b 100644
> >>> --- a/arch/x86/include/asm/kvm_host.h
> >>> +++ b/arch/x86/include/asm/kvm_host.h
> >>> @@ -510,6 +510,8 @@ struct kvm_x86_ops {
> >>>  	void (*run)(struct kvm_vcpu *vcpu, struct kvm_run *run);
> >>>  	int (*handle_exit)(struct kvm_run *run, struct kvm_vcpu *vcpu);
> >>>  	void (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
> >>> +	void (*set_interrupt_shadow)(struct kvm_vcpu *vcpu, int mask);
> >>>     
> >> There is .drop_interrupt_shadow() callback. The patch should remove it and
> >> replace its use by set_interrupt_shadow().
> >>   
> >
> > That would be [PATCH 1/2].
> [PATCH 2/2]. Otherwise we will break bisectability, as the pure removal of this
> function would lead us to a non-functioning kernel for no reason.
> 
> Avi: if this patch is okay, please apply. I'll send another one later that replaces
> the existing .drop_interrupt_shadow by the (then) in tree set_interrupt_shadow.
> 
It is not always easy to understand what Avi means :) but my
interpretation was that patch 1/2 should replace drop_interrupt_shadow()
with set_interrupt_shadow() and 2/2 should be only emulation changes.

--
			Gleb.

Re: [PATCH] deal with interrupt shadow state for emulated instruction

[Glauber Costa]

On Fri, May 08, 2009 at 10:18:14AM +0300, Gleb Natapov wrote:
> On Fri, May 08, 2009 at 02:25:11AM -0300, Glauber Costa wrote:
> > On Wed, May 06, 2009 at 01:51:04PM +0300, Avi Kivity wrote:
> > > Gleb Natapov wrote:
> > >> On Tue, May 05, 2009 at 02:40:11PM -0400, Glauber Costa wrote:
> > >>   
> > >>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > >>> index 8e680c3..a49d07b 100644
> > >>> --- a/arch/x86/include/asm/kvm_host.h
> > >>> +++ b/arch/x86/include/asm/kvm_host.h
> > >>> @@ -510,6 +510,8 @@ struct kvm_x86_ops {
> > >>>  	void (*run)(struct kvm_vcpu *vcpu, struct kvm_run *run);
> > >>>  	int (*handle_exit)(struct kvm_run *run, struct kvm_vcpu *vcpu);
> > >>>  	void (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
> > >>> +	void (*set_interrupt_shadow)(struct kvm_vcpu *vcpu, int mask);
> > >>>     
> > >> There is .drop_interrupt_shadow() callback. The patch should remove it and
> > >> replace its use by set_interrupt_shadow().
> > >>   
> > >
> > > That would be [PATCH 1/2].
> > [PATCH 2/2]. Otherwise we will break bisectability, as the pure removal of this
> > function would lead us to a non-functioning kernel for no reason.
> > 
> > Avi: if this patch is okay, please apply. I'll send another one later that replaces
> > the existing .drop_interrupt_shadow by the (then) in tree set_interrupt_shadow.
> > 
> It is not always easy to understand what Avi means :) but my
> interpretation was that patch 1/2 should replace drop_interrupt_shadow()
> with set_interrupt_shadow() and 2/2 should be only emulation changes.
ok... I'll send an updated version.