[PATCH 0/4] Add rudimentary Hyper-V guest support v2

[PATCH 0/4] Add rudimentary Hyper-V guest support v2

[Alexander Graf]

Now that we have nested SVM in place, let's make use of it and virtualize
something non-kvm.
The first interesting target that came to my mind here was Hyper-V.

This patchset makes Windows Server 2008 boot with Hyper-V, which runs
the "dom0" in virtualized mode already. It hangs somewhere in IDE code when
booted, so I haven't been able to run a second VM within for now yet.

Please keep in mind that Hyper-V won't work unless you apply the userspace
patches too and the PAT bit patch

v2 changes:
  - remove reserved PAT check patch (Avi will do this)
  - remove #PF inject on emulated_read
  - take comments from v1 into account (listed individually)

Alexander Graf (4):
  Add definition for IGNNE MSR
  Implement Hyper-V MSRs v2
  Nested SVM: Implement INVLPGA v2
  Nested SVM: Improve interrupt injection v2

 arch/x86/include/asm/msr-index.h |    1 +
 arch/x86/kvm/svm.c               |   59 +++++++++++++++++++++++++++----------
 2 files changed, 44 insertions(+), 16 deletions(-)

[PATCH 1/4] Add definition for IGNNE MSR

[Alexander Graf]

Hyper-V tried to access MSR_IGNNE, so let's at least have a definition
for it in our headers.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/x86/include/asm/msr-index.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index ec41fc1..e273549 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -372,6 +372,7 @@
 /* AMD-V MSRs */
 
 #define MSR_VM_CR                       0xc0010114
+#define MSR_VM_IGNNE                    0xc0010115
 #define MSR_VM_HSAVE_PA                 0xc0010117
 
 #endif /* _ASM_X86_MSR_INDEX_H */
-- 
1.6.0.2

[PATCH 2/4] Implement Hyper-V MSRs v2

[Alexander Graf]

Hyper-V uses some MSRs, some of which are actually reserved for BIOS usage.

But let's be nice today and have it its way, because otherwise it fails
terribly.

v2 changes:
  - remove the 0x40000081 MSR definition
  - add pr_unimpl() on unimplemented writes

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/x86/kvm/svm.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index ef43a18..4b4eadd 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2034,6 +2034,11 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 data)
 	case MSR_VM_HSAVE_PA:
 		svm->hsave_msr = data;
 		break;
+	case MSR_VM_CR:
+	case MSR_VM_IGNNE:
+	case MSR_K8_HWCR:
+		pr_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx, data);
+		break;
 	default:
 		return kvm_set_msr_common(vcpu, ecx, data);
 	}
-- 
1.6.0.2

[PATCH 3/4] Nested SVM: Implement INVLPGA v2

[Alexander Graf]

SVM adds another way to do INVLPG by ASID which Hyper-V makes use of,
so let's implement it!

For now we just do the same thing invlpg does, as asid switching
means we flush the mmu anyways. That might change one day though.

v2 makes invlpga do the same as invlpg, not flush the whole mmu

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/x86/kvm/svm.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 4b4eadd..fa2a710 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1785,6 +1785,19 @@ static int clgi_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
 	return 1;
 }
 
+static int invlpga_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
+{
+	struct kvm_vcpu *vcpu = &svm->vcpu;
+	nsvm_printk("INVLPGA\n");
+
+	/* Let's treat INVLPGA the same as INVLPG */
+	kvm_mmu_invlpg(vcpu, vcpu->arch.regs[VCPU_REGS_RAX]);
+
+	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
+	skip_emulated_instruction(&svm->vcpu);
+	return 1;
+}
+
 static int invalid_op_interception(struct vcpu_svm *svm,
 				   struct kvm_run *kvm_run)
 {
@@ -2130,7 +2143,7 @@ static int (*svm_exit_handlers[])(struct vcpu_svm *svm,
 	[SVM_EXIT_INVD]                         = emulate_on_interception,
 	[SVM_EXIT_HLT]				= halt_interception,
 	[SVM_EXIT_INVLPG]			= invlpg_interception,
-	[SVM_EXIT_INVLPGA]			= invalid_op_interception,
+	[SVM_EXIT_INVLPGA]			= invlpga_interception,
 	[SVM_EXIT_IOIO] 		  	= io_interception,
 	[SVM_EXIT_MSR]				= msr_interception,
 	[SVM_EXIT_TASK_SWITCH]			= task_switch_interception,
-- 
1.6.0.2

[PATCH 4/4] Nested SVM: Improve interrupt injection v2

[Alexander Graf]

While trying to get Hyper-V running, I realized that the interrupt injection
mechanisms that are in place right now are not 100% correct.

This patch makes nested SVM's interrupt injection behave more like on a
real machine.

v2 calls BUG_ON when svm_set_irq is called with GIF=0

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/x86/kvm/svm.c |   39 ++++++++++++++++++++++++---------------
 1 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index fa2a710..5b14c9d 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1517,7 +1517,8 @@ static int nested_svm_vmexit_real(struct vcpu_svm *svm, void *arg1,
 	/* Kill any pending exceptions */
 	if (svm->vcpu.arch.exception.pending == true)
 		nsvm_printk("WARNING: Pending Exception\n");
-	svm->vcpu.arch.exception.pending = false;
+	kvm_clear_exception_queue(&svm->vcpu);
+	kvm_clear_interrupt_queue(&svm->vcpu);
 
 	/* Restore selected save entries */
 	svm->vmcb->save.es = hsave->save.es;
@@ -1585,7 +1586,8 @@ static int nested_svm_vmrun(struct vcpu_svm *svm, void *arg1,
 	svm->nested_vmcb = svm->vmcb->save.rax;
 
 	/* Clear internal status */
-	svm->vcpu.arch.exception.pending = false;
+	kvm_clear_exception_queue(&svm->vcpu);
+	kvm_clear_interrupt_queue(&svm->vcpu);
 
 	/* Save the old vmcb, so we don't need to pick what we save, but
 	   can restore everything when a VMEXIT occurs */
@@ -2277,21 +2279,14 @@ static inline void svm_inject_irq(struct vcpu_svm *svm, int irq)
 		((/*control->int_vector >> 4*/ 0xf) << V_INTR_PRIO_SHIFT);
 }
 
-static void svm_queue_irq(struct kvm_vcpu *vcpu, unsigned nr)
-{
-	struct vcpu_svm *svm = to_svm(vcpu);
-
-	svm->vmcb->control.event_inj = nr |
-		SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR;
-}
-
 static void svm_set_irq(struct kvm_vcpu *vcpu, int irq)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
-	nested_svm_intr(svm);
+	BUG_ON(!(svm->vcpu.arch.hflags & HF_GIF_MASK));
 
-	svm_queue_irq(vcpu, irq);
+	svm->vmcb->control.event_inj = irq |
+		SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR;
 }
 
 static void update_cr8_intercept(struct kvm_vcpu *vcpu, int tpr, int irr)
@@ -2319,13 +2314,25 @@ static int svm_interrupt_allowed(struct kvm_vcpu *vcpu)
 	struct vmcb *vmcb = svm->vmcb;
 	return (vmcb->save.rflags & X86_EFLAGS_IF) &&
 		!(vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) &&
-		(svm->vcpu.arch.hflags & HF_GIF_MASK);
+		(svm->vcpu.arch.hflags & HF_GIF_MASK) &&
+		!is_nested(svm);
 }
 
 static void enable_irq_window(struct kvm_vcpu *vcpu)
 {
-	svm_set_vintr(to_svm(vcpu));
-	svm_inject_irq(to_svm(vcpu), 0x0);
+	struct vcpu_svm *svm = to_svm(vcpu);
+	nsvm_printk("Trying to open IRQ window\n");
+
+	nested_svm_intr(svm);
+
+	/* In case GIF=0 we can't rely on the CPU to tell us when
+	 * GIF becomes 1, because that's a separate STGI/VMRUN intercept.
+	 * The next time we get that intercept, this function will be
+	 * called again though and we'll get the vintr intercept. */
+	if (svm->vcpu.arch.hflags & HF_GIF_MASK) {
+		svm_set_vintr(svm);
+		svm_inject_irq(svm, 0x0);
+	}
 }
 
 static void enable_nmi_window(struct kvm_vcpu *vcpu)
@@ -2393,6 +2400,8 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
 	case SVM_EXITINTINFO_TYPE_EXEPT:
 		/* In case of software exception do not reinject an exception
 		   vector, but re-execute and instruction instead */
+		if (is_nested(svm))
+			break;
 		if (vector == BP_VECTOR || vector == OF_VECTOR)
 			break;
 		if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
-- 
1.6.0.2

Re: [PATCH 2/4] Implement Hyper-V MSRs v2

[Avi Kivity]

Alexander Graf wrote:
> Hyper-V uses some MSRs, some of which are actually reserved for BIOS usage.
>
> But let's be nice today and have it its way, because otherwise it fails
> terribly.
>
> v2 changes:
>   - remove the 0x40000081 MSR definition
>   - add pr_unimpl() on unimplemented writes
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/x86/kvm/svm.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index ef43a18..4b4eadd 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2034,6 +2034,11 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 data)
>  	case MSR_VM_HSAVE_PA:
>  		svm->hsave_msr = data;
>  		break;
> +	case MSR_VM_CR:
> +	case MSR_VM_IGNNE:
> +	case MSR_K8_HWCR:
> +		pr_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx, data);
> +		break;
>   

We can be nicer, if the write doesn't set bits which we don't implement, 
we can let it proceed silently.  See for example MSR_IA32_DEBUGCTLMSR.  
Most likely the values written are already correctly implemented (by 
doing nothing).

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

Re: [PATCH 3/4] Nested SVM: Implement INVLPGA v2

[Avi Kivity]

Alexander Graf wrote:
> SVM adds another way to do INVLPG by ASID which Hyper-V makes use of,
> so let's implement it!
>
> For now we just do the same thing invlpg does, as asid switching
> means we flush the mmu anyways. That might change one day though.
>
> v2 makes invlpga do the same as invlpg, not flush the whole mmu
>
>  
> +static int invlpga_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
> +{
> +	struct kvm_vcpu *vcpu = &svm->vcpu;
> +	nsvm_printk("INVLPGA\n");
> +
> +	/* Let's treat INVLPGA the same as INVLPG */
> +	kvm_mmu_invlpg(vcpu, vcpu->arch.regs[VCPU_REGS_RAX]);
> +
> +	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
> +	skip_emulated_instruction(&svm->vcpu);
> +	return 1;
> +}
>   

I think that for ASID!=0 you can actually do nothing.  The guest entry 
is a cr3 switch, so we'll both get a tlb flush and a resync on any 
modified ptes.

For ASID==0 you can do the invlpg thing.

Marcelo?

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

Re: [PATCH 3/4] Nested SVM: Implement INVLPGA v2

[Alexander Graf]

On 19.05.2009, at 14:58, Avi Kivity wrote:

> Alexander Graf wrote:
>> SVM adds another way to do INVLPG by ASID which Hyper-V makes use of,
>> so let's implement it!
>>
>> For now we just do the same thing invlpg does, as asid switching
>> means we flush the mmu anyways. That might change one day though.
>>
>> v2 makes invlpga do the same as invlpg, not flush the whole mmu
>>
>> +static int invlpga_interception(struct vcpu_svm *svm, struct  
>> kvm_run *kvm_run)
>> +{
>> +	struct kvm_vcpu *vcpu = &svm->vcpu;
>> +	nsvm_printk("INVLPGA\n");
>> +
>> +	/* Let's treat INVLPGA the same as INVLPG */
>> +	kvm_mmu_invlpg(vcpu, vcpu->arch.regs[VCPU_REGS_RAX]);
>> +
>> +	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
>> +	skip_emulated_instruction(&svm->vcpu);
>> +	return 1;
>> +}
>>
>
> I think that for ASID!=0 you can actually do nothing.  The guest  
> entry is a cr3 switch, so we'll both get a tlb flush and a resync on  
> any modified ptes.

Right, the only situation I can imagine this isn't fulfilled is when  
INVLPGA isn't trapped in the 1st level guest, but issued in the 2nd  
level one. That should be rather rare though ;-).

Alex

Re: [PATCH 3/4] Nested SVM: Implement INVLPGA v2

[Avi Kivity]

Alexander Graf wrote:
>> I think that for ASID!=0 you can actually do nothing.  The guest 
>> entry is a cr3 switch, so we'll both get a tlb flush and a resync on 
>> any modified ptes.
>
> Right, the only situation I can imagine this isn't fulfilled is when 
> INVLPGA isn't trapped in the 1st level guest, but issued in the 2nd 
> level one. That should be rather rare though ;-).

Good catch.

Would be better to get it right; changing the test to asid != 
current_asid should suffice.

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

Re: [PATCH 4/4] Nested SVM: Improve interrupt injection v2

[Gleb Natapov]

On Tue, May 19, 2009 at 12:54:03PM +0200, Alexander Graf wrote:
> While trying to get Hyper-V running, I realized that the interrupt injection
> mechanisms that are in place right now are not 100% correct.
> 
> This patch makes nested SVM's interrupt injection behave more like on a
> real machine.
> 
> v2 calls BUG_ON when svm_set_irq is called with GIF=0
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/x86/kvm/svm.c |   39 ++++++++++++++++++++++++---------------
>  1 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index fa2a710..5b14c9d 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1517,7 +1517,8 @@ static int nested_svm_vmexit_real(struct vcpu_svm *svm, void *arg1,
>  	/* Kill any pending exceptions */
>  	if (svm->vcpu.arch.exception.pending == true)
>  		nsvm_printk("WARNING: Pending Exception\n");
> -	svm->vcpu.arch.exception.pending = false;
> +	kvm_clear_exception_queue(&svm->vcpu);
> +	kvm_clear_interrupt_queue(&svm->vcpu);
>  
What about pending NMI here?
 
>  	/* Restore selected save entries */
>  	svm->vmcb->save.es = hsave->save.es;
> @@ -1585,7 +1586,8 @@ static int nested_svm_vmrun(struct vcpu_svm *svm, void *arg1,
>  	svm->nested_vmcb = svm->vmcb->save.rax;
>  
>  	/* Clear internal status */
> -	svm->vcpu.arch.exception.pending = false;
> +	kvm_clear_exception_queue(&svm->vcpu);
> +	kvm_clear_interrupt_queue(&svm->vcpu);
>  
And here.

>  	/* Save the old vmcb, so we don't need to pick what we save, but
>  	   can restore everything when a VMEXIT occurs */
> @@ -2277,21 +2279,14 @@ static inline void svm_inject_irq(struct vcpu_svm *svm, int irq)
>  		((/*control->int_vector >> 4*/ 0xf) << V_INTR_PRIO_SHIFT);
>  }
>  
> -static void svm_queue_irq(struct kvm_vcpu *vcpu, unsigned nr)
> -{
> -	struct vcpu_svm *svm = to_svm(vcpu);
> -
> -	svm->vmcb->control.event_inj = nr |
> -		SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR;
> -}
> -
>  static void svm_set_irq(struct kvm_vcpu *vcpu, int irq)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  
> -	nested_svm_intr(svm);
> +	BUG_ON(!(svm->vcpu.arch.hflags & HF_GIF_MASK));
>  
> -	svm_queue_irq(vcpu, irq);
> +	svm->vmcb->control.event_inj = irq |
> +		SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR;
>  }
>  
>  static void update_cr8_intercept(struct kvm_vcpu *vcpu, int tpr, int irr)
> @@ -2319,13 +2314,25 @@ static int svm_interrupt_allowed(struct kvm_vcpu *vcpu)
>  	struct vmcb *vmcb = svm->vmcb;
>  	return (vmcb->save.rflags & X86_EFLAGS_IF) &&
>  		!(vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) &&
> -		(svm->vcpu.arch.hflags & HF_GIF_MASK);
> +		(svm->vcpu.arch.hflags & HF_GIF_MASK) &&
> +		!is_nested(svm);
>  }
>  
>  static void enable_irq_window(struct kvm_vcpu *vcpu)
>  {
> -	svm_set_vintr(to_svm(vcpu));
> -	svm_inject_irq(to_svm(vcpu), 0x0);
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +	nsvm_printk("Trying to open IRQ window\n");
> +
> +	nested_svm_intr(svm);
> +
> +	/* In case GIF=0 we can't rely on the CPU to tell us when
> +	 * GIF becomes 1, because that's a separate STGI/VMRUN intercept.
> +	 * The next time we get that intercept, this function will be
> +	 * called again though and we'll get the vintr intercept. */
> +	if (svm->vcpu.arch.hflags & HF_GIF_MASK) {
> +		svm_set_vintr(svm);
> +		svm_inject_irq(svm, 0x0);
> +	}
>  }
>  
>  static void enable_nmi_window(struct kvm_vcpu *vcpu)
> @@ -2393,6 +2400,8 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
>  	case SVM_EXITINTINFO_TYPE_EXEPT:
>  		/* In case of software exception do not reinject an exception
>  		   vector, but re-execute and instruction instead */
> +		if (is_nested(svm))
> +			break;
>  		if (vector == BP_VECTOR || vector == OF_VECTOR)
>  			break;
>  		if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
> -- 
> 1.6.0.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: [PATCH 3/4] Nested SVM: Implement INVLPGA v2

[Marcelo Tosatti]

On Tue, May 19, 2009 at 03:58:52PM +0300, Avi Kivity wrote:
> Alexander Graf wrote:
>> SVM adds another way to do INVLPG by ASID which Hyper-V makes use of,
>> so let's implement it!
>>
>> For now we just do the same thing invlpg does, as asid switching
>> means we flush the mmu anyways. That might change one day though.
>>
>> v2 makes invlpga do the same as invlpg, not flush the whole mmu
>>
>>  +static int invlpga_interception(struct vcpu_svm *svm, struct kvm_run 
>> *kvm_run)
>> +{
>> +	struct kvm_vcpu *vcpu = &svm->vcpu;
>> +	nsvm_printk("INVLPGA\n");
>> +
>> +	/* Let's treat INVLPGA the same as INVLPG */
>> +	kvm_mmu_invlpg(vcpu, vcpu->arch.regs[VCPU_REGS_RAX]);
>> +
>> +	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
>> +	skip_emulated_instruction(&svm->vcpu);
>> +	return 1;
>> +}
>>   
>
> I think that for ASID!=0 you can actually do nothing.  The guest entry  
> is a cr3 switch, so we'll both get a tlb flush and a resync on any  
> modified ptes.
>
> For ASID==0 you can do the invlpg thing.
>
> Marcelo?

kvm_mmu_invlpg is cheap, better just invalidate the entry. If hyper-v
uses invlpga to invalidate TLB entries which it has updated pte's in
memory for, and you skip the invalidation now and somehow later use an
unsync spte, you're toast.

Re: [PATCH 3/4] Nested SVM: Implement INVLPGA v2

[Avi Kivity]

Marcelo Tosatti wrote:
>> I think that for ASID!=0 you can actually do nothing.  The guest entry  
>> is a cr3 switch, so we'll both get a tlb flush and a resync on any  
>> modified ptes.
>>
>> For ASID==0 you can do the invlpg thing.
>>
>> Marcelo?
>>     
>
> kvm_mmu_invlpg is cheap, better just invalidate the entry. If hyper-v
> uses invlpga to invalidate TLB entries which it has updated pte's in
> memory for, and you skip the invalidation now and somehow later use an
> unsync spte, you're toast.
>   

But won't the guest entry cause a resync?

Doing nothing is even cheaper.

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

Re: [PATCH 3/4] Nested SVM: Implement INVLPGA v2

[Marcelo Tosatti]

On Tue, May 19, 2009 at 04:56:48PM +0300, Avi Kivity wrote:
> Marcelo Tosatti wrote:
>>> I think that for ASID!=0 you can actually do nothing.  The guest 
>>> entry  is a cr3 switch, so we'll both get a tlb flush and a resync on 
>>> any  modified ptes.
>>>
>>> For ASID==0 you can do the invlpg thing.
>>>
>>> Marcelo?
>>>     
>>
>> kvm_mmu_invlpg is cheap, better just invalidate the entry. If hyper-v
>> uses invlpga to invalidate TLB entries which it has updated pte's in
>> memory for, and you skip the invalidation now and somehow later use an
>> unsync spte, you're toast.
>>   
>
> But won't the guest entry cause a resync?

If its a cr3/cr4 exit, yes. 

> Doing nothing is even cheaper.

My brain is nested.

Re: [PATCH 3/4] Nested SVM: Implement INVLPGA v2

[Alexander Graf]

On 19.05.2009, at 15:58, Marcelo Tosatti wrote:

> On Tue, May 19, 2009 at 04:56:48PM +0300, Avi Kivity wrote:
>> Marcelo Tosatti wrote:
>>>> I think that for ASID!=0 you can actually do nothing.  The guest
>>>> entry  is a cr3 switch, so we'll both get a tlb flush and a  
>>>> resync on
>>>> any  modified ptes.
>>>>
>>>> For ASID==0 you can do the invlpg thing.
>>>>
>>>> Marcelo?
>>>>
>>>
>>> kvm_mmu_invlpg is cheap, better just invalidate the entry. If  
>>> hyper-v
>>> uses invlpga to invalidate TLB entries which it has updated pte's in
>>> memory for, and you skip the invalidation now and somehow later  
>>> use an
>>> unsync spte, you're toast.
>>>
>>
>> But won't the guest entry cause a resync?
>
> If its a cr3/cr4 exit, yes.

Well it has to be. Either we're switching from one NPT to the other  
(todo) or do a normal cr3+cr4 switch.

So I guess we can optimize here. Is it worth it?

Alex

Re: [PATCH 3/4] Nested SVM: Implement INVLPGA v2

[Marcelo Tosatti]

On Tue, May 19, 2009 at 05:18:07PM +0200, Alexander Graf wrote:
>
> On 19.05.2009, at 15:58, Marcelo Tosatti wrote:
>
>> On Tue, May 19, 2009 at 04:56:48PM +0300, Avi Kivity wrote:
>>> Marcelo Tosatti wrote:
>>>>> I think that for ASID!=0 you can actually do nothing.  The guest
>>>>> entry  is a cr3 switch, so we'll both get a tlb flush and a  
>>>>> resync on
>>>>> any  modified ptes.
>>>>>
>>>>> For ASID==0 you can do the invlpg thing.
>>>>>
>>>>> Marcelo?
>>>>>
>>>>
>>>> kvm_mmu_invlpg is cheap, better just invalidate the entry. If  
>>>> hyper-v
>>>> uses invlpga to invalidate TLB entries which it has updated pte's in
>>>> memory for, and you skip the invalidation now and somehow later  
>>>> use an
>>>> unsync spte, you're toast.
>>>>
>>>
>>> But won't the guest entry cause a resync?
>>
>> If its a cr3/cr4 exit, yes.
>
> Well it has to be. Either we're switching from one NPT to the other  
> (todo) or do a normal cr3+cr4 switch.
>
> So I guess we can optimize here. Is it worth it?

IMHO better leave it the way it is, perhaps add a comment that 
the optimization is possible, and do it later if worthwhile.

Re: [PATCH 3/4] Nested SVM: Implement INVLPGA v2

[Avi Kivity]

Alexander Graf wrote:
>>>> kvm_mmu_invlpg is cheap, better just invalidate the entry. If hyper-v
>>>> uses invlpga to invalidate TLB entries which it has updated pte's in
>>>> memory for, and you skip the invalidation now and somehow later use an
>>>> unsync spte, you're toast.
>>>>
>>>
>>> But won't the guest entry cause a resync?
>>
>> If its a cr3/cr4 exit, yes.
>
> Well it has to be. Either we're switching from one NPT to the other 
> (todo) or do a normal cr3+cr4 switch.
>
> So I guess we can optimize here. Is it worth it?
>

I think so.  We also need to make sure the entry causes a resync, even 
if cr3 doesn't change.

Oh, exit needs to force a resync as well, in case the guest foolishly 
let its guest touch its page tables and issue invlpga asid=0.

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

Re: [PATCH 2/4] Implement Hyper-V MSRs v2

[Alexander Graf]

On 19.05.2009, at 14:52, Avi Kivity wrote:

> Alexander Graf wrote:
>> Hyper-V uses some MSRs, some of which are actually reserved for  
>> BIOS usage.
>>
>> But let's be nice today and have it its way, because otherwise it  
>> fails
>> terribly.
>>
>> v2 changes:
>>  - remove the 0x40000081 MSR definition
>>  - add pr_unimpl() on unimplemented writes
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> arch/x86/kvm/svm.c |    5 +++++
>> 1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index ef43a18..4b4eadd 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -2034,6 +2034,11 @@ static int svm_set_msr(struct kvm_vcpu  
>> *vcpu, unsigned ecx, u64 data)
>> 	case MSR_VM_HSAVE_PA:
>> 		svm->hsave_msr = data;
>> 		break;
>> +	case MSR_VM_CR:
>> +	case MSR_VM_IGNNE:
>> +	case MSR_K8_HWCR:
>> +		pr_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx,  
>> data);
>> +		break;
>>
>
> We can be nicer, if the write doesn't set bits which we don't  
> implement, we can let it proceed silently.  See for example  
> MSR_IA32_DEBUGCTLMSR.  Most likely the values written are already  
> correctly implemented (by doing nothing).

Actually we implement very little of the bits. See http://support.amd.com/us/Processor_TechDocs/31116-Public-GH-BKDG_3-28_5-28-09.pdf 
  for what we're missing ;-).

So it might make sense to always warn for now, see what OSs use and  
only filter out those bits they actually try to access (and maybe  
implement them).

Alex

Re: [PATCH 4/4] Nested SVM: Improve interrupt injection v2

[Alexander Graf]

On 19.05.2009, at 15:22, Gleb Natapov wrote:

> On Tue, May 19, 2009 at 12:54:03PM +0200, Alexander Graf wrote:
>> While trying to get Hyper-V running, I realized that the interrupt  
>> injection
>> mechanisms that are in place right now are not 100% correct.
>>
>> This patch makes nested SVM's interrupt injection behave more like  
>> on a
>> real machine.
>>
>> v2 calls BUG_ON when svm_set_irq is called with GIF=0
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> arch/x86/kvm/svm.c |   39 ++++++++++++++++++++++++---------------
>> 1 files changed, 24 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index fa2a710..5b14c9d 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -1517,7 +1517,8 @@ static int nested_svm_vmexit_real(struct  
>> vcpu_svm *svm, void *arg1,
>> 	/* Kill any pending exceptions */
>> 	if (svm->vcpu.arch.exception.pending == true)
>> 		nsvm_printk("WARNING: Pending Exception\n");
>> -	svm->vcpu.arch.exception.pending = false;
>> +	kvm_clear_exception_queue(&svm->vcpu);
>> +	kvm_clear_interrupt_queue(&svm->vcpu);
>>
> What about pending NMI here?

NMI injected to the guest? That should have triggered by now and  
caused an #NMI exit, no?

Alex

Re: [PATCH 4/4] Nested SVM: Improve interrupt injection v2

[Gleb Natapov]

On Mon, Jun 15, 2009 at 01:47:08PM +0200, Alexander Graf wrote:
>
> On 19.05.2009, at 15:22, Gleb Natapov wrote:
>
>> On Tue, May 19, 2009 at 12:54:03PM +0200, Alexander Graf wrote:
>>> While trying to get Hyper-V running, I realized that the interrupt  
>>> injection
>>> mechanisms that are in place right now are not 100% correct.
>>>
>>> This patch makes nested SVM's interrupt injection behave more like  
>>> on a
>>> real machine.
>>>
>>> v2 calls BUG_ON when svm_set_irq is called with GIF=0
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>> arch/x86/kvm/svm.c |   39 ++++++++++++++++++++++++---------------
>>> 1 files changed, 24 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>>> index fa2a710..5b14c9d 100644
>>> --- a/arch/x86/kvm/svm.c
>>> +++ b/arch/x86/kvm/svm.c
>>> @@ -1517,7 +1517,8 @@ static int nested_svm_vmexit_real(struct  
>>> vcpu_svm *svm, void *arg1,
>>> 	/* Kill any pending exceptions */
>>> 	if (svm->vcpu.arch.exception.pending == true)
>>> 		nsvm_printk("WARNING: Pending Exception\n");
>>> -	svm->vcpu.arch.exception.pending = false;
>>> +	kvm_clear_exception_queue(&svm->vcpu);
>>> +	kvm_clear_interrupt_queue(&svm->vcpu);
>>>
>> What about pending NMI here?
>
> NMI injected to the guest? That should have triggered by now and caused 
> an #NMI exit, no?
>
I don't really understand what this code is doing, but there are three
types of events exception/interrupt/nmi you clear only two of them. If
you are sure this is correct then OK.

--
			Gleb.