| Message ID | 5141E41B.8080804@siemens.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 14, 2013 at 03:52:11PM +0100, Jan Kiszka wrote: > 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 <jan.kiszka@siemens.com> > --- > > 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(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 958ac3a..be5b1dc 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -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_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. > vmx_set_cr4(&vmx->vcpu, 0); > vmx_set_efer(&vmx->vcpu, 0); > vmx_fpu_activate(&vmx->vcpu); > -- > 1.7.3.4 -- Gleb. -- 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
On 2013-03-14 16:00, Gleb Natapov wrote: > On Thu, Mar 14, 2013 at 03:52:11PM +0100, Jan Kiszka wrote: >> 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 <jan.kiszka@siemens.com> >> --- >> >> 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(-) >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index 958ac3a..be5b1dc 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -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_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. Grmbl, likely not. Jan
Il 14/03/2013 16:03, Jan Kiszka ha scritto: >> > 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 -- 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
On Thu, Mar 14, 2013 at 04:08:42PM +0100, Paolo Bonzini wrote: > Il 14/03/2013 16:03, Jan Kiszka ha scritto: > >> > 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); > > ? > Srcu may have per cpu state. We can always ask Paul. -- Gleb. -- 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
On 2013-03-14 16:08, Paolo Bonzini wrote: > Il 14/03/2013 16:03, Jan Kiszka ha scritto: >>>> 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); > > ? Isn't there any per-cpu info encoded in idx? Jan
Il 14/03/2013 16:11, Gleb Natapov ha scritto: > On Thu, Mar 14, 2013 at 04:08:42PM +0100, Paolo Bonzini wrote: >> Il 14/03/2013 16:03, Jan Kiszka ha scritto: >>>>> 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); >> >> ? >> > 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 -- 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
On Thu, Mar 14, 2013 at 05:00:04PM +0200, Gleb Natapov wrote: > On Thu, Mar 14, 2013 at 03:52:11PM +0100, Jan Kiszka wrote: > > 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 <jan.kiszka@siemens.com> > > --- > > > > 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(-) > > > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > > index 958ac3a..be5b1dc 100644 > > --- a/arch/x86/kvm/vmx.c > > +++ b/arch/x86/kvm/vmx.c > > @@ -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_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. The lock/unlocks must be paired. Pass parameters around to make that happen? -- 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
On 2013-03-14 20:14, Marcelo Tosatti wrote: > On Thu, Mar 14, 2013 at 05:00:04PM +0200, Gleb Natapov wrote: >> On Thu, Mar 14, 2013 at 03:52:11PM +0100, Jan Kiszka wrote: >>> 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 <jan.kiszka@siemens.com> >>> --- >>> >>> 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(-) >>> >>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >>> index 958ac3a..be5b1dc 100644 >>> --- a/arch/x86/kvm/vmx.c >>> +++ b/arch/x86/kvm/vmx.c >>> @@ -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_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. > > The lock/unlocks must be paired. Did you find out more than what Paolo reported? > > Pass parameters around to make that happen? Or we save/restore srcu_idx when overwriting. Jan
On Thu, Mar 14, 2013 at 08:20:26PM +0100, Jan Kiszka wrote: > >> 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); } -- 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
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 958ac3a..be5b1dc 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -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);
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 <jan.kiszka@siemens.com> --- 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(-)