KVM: VMX: Do not overwrite vcpu->srcu_idx in vmx_vcpu_reset

PATCH Superseded Jan Kiszka 5 participants 12 messages Mar 14, 2013 – Mar 15, 2013

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Jan Kiszka

author

2013-03-14 14:52 UTC #0 lore raw

PATCH 1/1 KVM: VMX: Do not overwrite vcpu->srcu_idx in vmx_vcpu_reset

vmx_vcpu_reset may now be called while already holding the srcu lock, so we may overwrite what was already saved there. Also, we lock and unlock in the same context, thus there was no need to save to the vcpu anyway.

Signed-off-by Jan Kiszka <[email protected]>

---

Marcelo just suggested this as the simplest fix for the issue caused by the INIT/SIPI patch. Avoiding srcu lock for TSS handling might still be possible but more tricky.

 arch/x86/kvm/vmx.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

arch/x86/kvm/vmx.c +3 -2

index 958ac3a..be5b1dc 100644
@@ -4117,6 +4117,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	u64 msr;
+	int idx;
 
 	vmx->rmode.vm86_active = 0;

@@ -4190,9 +4191,9 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 		vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
 
 	vmx->vcpu.arch.cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
-	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
+	idx = srcu_read_lock(&vcpu->kvm->srcu);
 	vmx_set_cr0(&vmx->vcpu, kvm_read_cr0(vcpu)); /* enter rmode */
-	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
+	srcu_read_unlock(&vcpu->kvm->srcu, idx);
 	vmx_set_cr4(&vmx->vcpu, 0);
 	vmx_set_efer(&vmx->vcpu, 0);
 	vmx_fpu_activate(&vmx->vcpu);

Signature · 1 line

1.7.3.4

Gleb Natapov

2013-03-14 15:00 UTC #1 lore raw

Quoted 27 lines · Jan Kiszka

vmx_vcpu_reset may now be called while already holding the srcu lock, so we may overwrite what was already saved there. Also, we lock and unlock in the same context, thus there was no need to save to the vcpu anyway.
Signed-off-by Jan Kiszka <[email protected]>
---
Marcelo just suggested this as the simplest fix for the issue caused by the INIT/SIPI patch. Avoiding srcu lock for TSS handling might still be possible but more tricky.
 arch/x86/kvm/vmx.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
arch/x86/kvm/vmx.c +1 -0
index 958ac3a..be5b1dc 100644
@@ -4117,6 +4117,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	u64 msr;
+	int idx;
 
 	vmx->rmode.vm86_active = 0;
 

arch/x86/kvm/vmx.c static void vmx_vcpu_reset(struct kvm_vcpu *vcpu) · line 4196

Quoted 8 lines

arch/x86/kvm/vmx.c +2 -2

@@ -4190,9 +4191,9 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 		vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
 
 	vmx->vcpu.arch.cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
-	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
+	idx = srcu_read_lock(&vcpu->kvm->srcu);
 	vmx_set_cr0(&vmx->vcpu, kvm_read_cr0(vcpu)); /* enter rmode */
-	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
+	srcu_read_unlock(&vcpu->kvm->srcu, idx);

vmx_set_cr0() does:

                srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
                vmx_set_tss_addr(vcpu->kvm, 0xfeffd000);
                vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);

So with this change the sequence will be:

vcpu->srcu_idx = srcu_read_lock()
idx = srcu_read_lock(&vcpu->kvm->srcu);
srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx); vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
srcu_read_unlock(&vcpu->kvm->srcu, idx);
srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);

Not sure this is valid.

Quoted 5 lines

arch/x86/kvm/vmx.c +0 -0

 	vmx_set_cr4(&vmx->vcpu, 0);
 	vmx_set_efer(&vmx->vcpu, 0);
 	vmx_fpu_activate(&vmx->vcpu);

Signature · 1 line

1.7.3.4

--
Gleb.

Jan Kiszka

author

2013-03-14 15:03 UTC #2 lore raw

Quoted 42 lines · Gleb Natapov

Quoted 27 lines · Jan Kiszka
vmx_vcpu_reset may now be called while already holding the srcu lock, so we may overwrite what was already saved there. Also, we lock and unlock in the same context, thus there was no need to save to the vcpu anyway.
Signed-off-by Jan Kiszka <[email protected]>
---
Marcelo just suggested this as the simplest fix for the issue caused by the INIT/SIPI patch. Avoiding srcu lock for TSS handling might still be possible but more tricky.
 arch/x86/kvm/vmx.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
arch/x86/kvm/vmx.c +1 -0
index 958ac3a..be5b1dc 100644
@@ -4117,6 +4117,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	u64 msr;
+	int idx;
 
 	vmx->rmode.vm86_active = 0;
 
arch/x86/kvm/vmx.c static void vmx_vcpu_reset(struct kvm_vcpu *vcpu) · line 4196
Quoted 8 lines
arch/x86/kvm/vmx.c +2 -2
@@ -4190,9 +4191,9 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 		vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
 
 	vmx->vcpu.arch.cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
-	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
+	idx = srcu_read_lock(&vcpu->kvm->srcu);
 	vmx_set_cr0(&vmx->vcpu, kvm_read_cr0(vcpu)); /* enter rmode */
-	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
+	srcu_read_unlock(&vcpu->kvm->srcu, idx);
vmx_set_cr0() does:
                srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
                vmx_set_tss_addr(vcpu->kvm, 0xfeffd000);
                vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
So with this change the sequence will be:

Quoted 8 lines

vcpu->srcu_idx = srcu_read_lock()
  idx = srcu_read_lock(&vcpu->kvm->srcu); 
   srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
   vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
srcu_read_unlock(&vcpu->kvm->srcu, idx);
srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
Not sure this is valid.

Grmbl, likely not.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Paolo Bonzini

maintainer

2013-03-14 15:08 UTC #3 lore raw

Il 14/03/2013 16:03, Jan Kiszka ha scritto:

Quoted 9 lines

Quoted 8 lines
Quoted 8 lines
vcpu->srcu_idx = srcu_read_lock()
  idx = srcu_read_lock(&vcpu->kvm->srcu); 
   srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
   vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
srcu_read_unlock(&vcpu->kvm->srcu, idx);
srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
Not sure this is valid.
Grmbl, likely not.

It might be.

Isn't it the same as two different CPUs doing

CPU 1 CPU 2
------------------------------------------------------------------------------------------------

vcpu->srcu_idx = srcu_read_lock()

                                                      idx = srcu_read_lock(&vcpu->kvm->srcu); 
srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
                                                      srcu_read_unlock(&vcpu->kvm->srcu, idx);

srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);

Paolo

Gleb Natapov

2013-03-14 15:11 UTC #4 lore raw

Quoted 18 lines · Paolo Bonzini

Il 14/03/2013 16:03, Jan Kiszka ha scritto:
Quoted 9 lines
Quoted 8 lines
Quoted 8 lines
vcpu->srcu_idx = srcu_read_lock()
  idx = srcu_read_lock(&vcpu->kvm->srcu); 
   srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
   vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
srcu_read_unlock(&vcpu->kvm->srcu, idx);
srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
Not sure this is valid.
Grmbl, likely not.
It might be.
Isn't it the same as two different CPUs doing
CPU 1 CPU 2
------------------------------------------------------------------------------------------------

Quoted 9 lines

vcpu->srcu_idx = srcu_read_lock()

                                                      idx = srcu_read_lock(&vcpu->kvm->srcu); 
srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
                                                      srcu_read_unlock(&vcpu->kvm->srcu, idx);

srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);

Srcu may have per cpu state. We can always ask Paul.

--
Gleb.

Paolo Bonzini

maintainer

2013-03-14 15:45 UTC #5 lore raw

Il 14/03/2013 16:11, Gleb Natapov ha scritto:

Quoted 26 lines · Paolo Bonzini

Quoted 25 lines
Il 14/03/2013 16:03, Jan Kiszka ha scritto:
Quoted 9 lines
Quoted 8 lines
Quoted 8 lines
vcpu->srcu_idx = srcu_read_lock()
  idx = srcu_read_lock(&vcpu->kvm->srcu); 
   srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
   vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
srcu_read_unlock(&vcpu->kvm->srcu, idx);
srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
Not sure this is valid.
Grmbl, likely not.
It might be.
Isn't it the same as two different CPUs doing
CPU 1 CPU 2
------------------------------------------------------------------------------------------------
vcpu->srcu_idx = srcu_read_lock()
                                                      idx = srcu_read_lock(&vcpu->kvm->srcu); 
srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
                                                      srcu_read_unlock(&vcpu->kvm->srcu, idx);
srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);

Quoted 3 lines

Quoted 2 lines
?
Srcu may have per cpu state. We can always ask Paul.

There is per-CPU state but it is only used as an optimization. synchronize_srcu only uses the sum of all values.

In fact, SRCU critical sections are preemptable so there's not even a guarantee that srcu_read_lock() and srcu_read_unlock() run on the same CPU.

Paolo

Jan Kiszka

author

2013-03-14 15:11 UTC #6 lore raw

In reply to Paolo Bonzini

Quoted 18 lines · Paolo Bonzini

Il 14/03/2013 16:03, Jan Kiszka ha scritto:
Quoted 9 lines
Quoted 8 lines
Quoted 8 lines
vcpu->srcu_idx = srcu_read_lock()
  idx = srcu_read_lock(&vcpu->kvm->srcu); 
   srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
   vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
srcu_read_unlock(&vcpu->kvm->srcu, idx);
srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
Not sure this is valid.
Grmbl, likely not.
It might be.
Isn't it the same as two different CPUs doing
CPU 1 CPU 2
------------------------------------------------------------------------------------------------

Quoted 8 lines

vcpu->srcu_idx = srcu_read_lock()

                                                      idx = srcu_read_lock(&vcpu->kvm->srcu); 
srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
                                                      srcu_read_unlock(&vcpu->kvm->srcu, idx);

srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);

Isn't there any per-cpu info encoded in idx?

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Marcelo Tosatti

2013-03-14 19:14 UTC #7 lore raw

In reply to Gleb Natapov

Quoted 42 lines · Gleb Natapov

Quoted 27 lines · Jan Kiszka
vmx_vcpu_reset may now be called while already holding the srcu lock, so we may overwrite what was already saved there. Also, we lock and unlock in the same context, thus there was no need to save to the vcpu anyway.
Signed-off-by Jan Kiszka <[email protected]>
---
Marcelo just suggested this as the simplest fix for the issue caused by the INIT/SIPI patch. Avoiding srcu lock for TSS handling might still be possible but more tricky.
 arch/x86/kvm/vmx.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
arch/x86/kvm/vmx.c +1 -0
index 958ac3a..be5b1dc 100644
@@ -4117,6 +4117,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	u64 msr;
+	int idx;
 
 	vmx->rmode.vm86_active = 0;
 
arch/x86/kvm/vmx.c static void vmx_vcpu_reset(struct kvm_vcpu *vcpu) · line 4196
Quoted 8 lines
arch/x86/kvm/vmx.c +2 -2
@@ -4190,9 +4191,9 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 		vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
 
 	vmx->vcpu.arch.cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
-	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
+	idx = srcu_read_lock(&vcpu->kvm->srcu);
 	vmx_set_cr0(&vmx->vcpu, kvm_read_cr0(vcpu)); /* enter rmode */
-	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
+	srcu_read_unlock(&vcpu->kvm->srcu, idx);
vmx_set_cr0() does:
                srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
                vmx_set_tss_addr(vcpu->kvm, 0xfeffd000);
                vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
So with this change the sequence will be:

Quoted 8 lines

vcpu->srcu_idx = srcu_read_lock()
  idx = srcu_read_lock(&vcpu->kvm->srcu); 
   srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
   vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
srcu_read_unlock(&vcpu->kvm->srcu, idx);
srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
Not sure this is valid.

The lock/unlocks must be paired.

Pass parameters around to make that happen?

Jan Kiszka

author

2013-03-14 19:20 UTC #8 lore raw

Quoted 50 lines · Marcelo Tosatti

Quoted 49 lines · Gleb Natapov
Quoted 27 lines · Jan Kiszka
vmx_vcpu_reset may now be called while already holding the srcu lock, so we may overwrite what was already saved there. Also, we lock and unlock in the same context, thus there was no need to save to the vcpu anyway.
Signed-off-by Jan Kiszka <[email protected]>
---
Marcelo just suggested this as the simplest fix for the issue caused by the INIT/SIPI patch. Avoiding srcu lock for TSS handling might still be possible but more tricky.
 arch/x86/kvm/vmx.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
arch/x86/kvm/vmx.c +1 -0
index 958ac3a..be5b1dc 100644
@@ -4117,6 +4117,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	u64 msr;
+	int idx;
 
 	vmx->rmode.vm86_active = 0;
 
arch/x86/kvm/vmx.c static void vmx_vcpu_reset(struct kvm_vcpu *vcpu) · line 4196
Quoted 8 lines
arch/x86/kvm/vmx.c +2 -2
@@ -4190,9 +4191,9 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 		vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
 
 	vmx->vcpu.arch.cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
-	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
+	idx = srcu_read_lock(&vcpu->kvm->srcu);
 	vmx_set_cr0(&vmx->vcpu, kvm_read_cr0(vcpu)); /* enter rmode */
-	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
+	srcu_read_unlock(&vcpu->kvm->srcu, idx);
vmx_set_cr0() does:
                srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
                vmx_set_tss_addr(vcpu->kvm, 0xfeffd000);
                vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
So with this change the sequence will be:
vcpu->srcu_idx = srcu_read_lock()
  idx = srcu_read_lock(&vcpu->kvm->srcu); 
   srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
   vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
srcu_read_unlock(&vcpu->kvm->srcu, idx);
srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);

Quoted 3 lines

Quoted 1 line
Not sure this is valid.
The lock/unlocks must be paired.

Did you find out more than what Paolo reported?

Quoted 2 lines

Pass parameters around to make that happen?

Or we save/restore srcu_idx when overwriting.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Jan Kiszka

author

2013-03-14 19:39 UTC #9 lore raw

PATCH v2 1/1 KVM: VMX: Do not overwrite vcpu->srcu_idx in vmx_vcpu_reset

vmx_vcpu_reset may now be called while already holding the srcu lock, so we may overwrite what was already saved there. Save and restore it.

Signed-off-by Jan Kiszka <[email protected]>

---

Even if this should be unneeded, it looks more consistent. In any case, all versions on the table, pick what you prefer.

 arch/x86/kvm/vmx.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

arch/x86/kvm/vmx.c +3 -0

index 958ac3a..7bc49ca 100644
@@ -4117,6 +4117,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	u64 msr;
+	int idx;
 
 	vmx->rmode.vm86_active = 0;

@@ -4190,9 +4191,11 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 		vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
 
 	vmx->vcpu.arch.cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
+	idx = vcpu->srcu_idx;
 	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
 	vmx_set_cr0(&vmx->vcpu, kvm_read_cr0(vcpu)); /* enter rmode */
 	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
+	vcpu->srcu_idx = idx;
 	vmx_set_cr4(&vmx->vcpu, 0);
 	vmx_set_efer(&vmx->vcpu, 0);
 	vmx_fpu_activate(&vmx->vcpu);

Signature · 1 line

1.7.3.4

Jan Kiszka

2013-03-15 07:09 UTC #10 lore raw

Quoted 27 lines · Jan Kiszka

vmx_vcpu_reset may now be called while already holding the srcu lock, so we may overwrite what was already saved there. Save and restore it.
Signed-off-by Jan Kiszka <[email protected]>
---
Even if this should be unneeded, it looks more consistent. In any case, all versions on the table, pick what you prefer.
 arch/x86/kvm/vmx.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
arch/x86/kvm/vmx.c +1 -0
index 958ac3a..7bc49ca 100644
@@ -4117,6 +4117,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	u64 msr;
+	int idx;
 
 	vmx->rmode.vm86_active = 0;
 
@@ -4190,9 +4191,11 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
 		vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);

arch/x86/kvm/vmx.c static void vmx_vcpu_reset(struct kvm_vcpu *vcpu) · line 4201

Quoted 10 lines

arch/x86/kvm/vmx.c +2 -0

 
 	vmx->vcpu.arch.cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
+	idx = vcpu->srcu_idx;
 	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
 	vmx_set_cr0(&vmx->vcpu, kvm_read_cr0(vcpu)); /* enter rmode */
 	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
+	vcpu->srcu_idx = idx;
 	vmx_set_cr4(&vmx->vcpu, 0);
 	vmx_set_efer(&vmx->vcpu, 0);
 	vmx_fpu_activate(&vmx->vcpu);

This cannot work either: I think we really need to drop the srcu lock before calling vmx_set_tss_addr. But if we nest the lock, we may only drop it once now in enter_rmode.

OK, I'll propose a patch to remove that TSS bug workaround from enter_rmode. Will dig a bit in the archives as well to check which version of qemu-kvm was actually exposing this.

Jan

Marcelo Tosatti

2013-03-14 21:06 UTC #11 lore raw

In reply to Jan Kiszka

Quoted 5 lines · Jan Kiszka

Quoted 3 lines
Quoted 1 line
Not sure this is valid.
The lock/unlocks must be paired.
Did you find out more than what Paolo reported?

/**
* srcu_read_unlock - unregister a old reader from an SRCU-protected
* structure.
* @sp: srcu_struct in which to unregister the old reader.
* @idx: return value from corresponding srcu_read_lock(). *
* Exit an SRCU read-side critical section.
*/
static inline void srcu_read_unlock(struct srcu_struct *sp, int idx)

        __releases(sp)
{
        rcu_lock_release(&(sp)->dep_map);
        __srcu_read_unlock(sp, idx);
}