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

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

[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 <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_cr4(&vmx->vcpu, 0);
 	vmx_set_efer(&vmx->vcpu, 0);
 	vmx_fpu_activate(&vmx->vcpu);
-- 
1.7.3.4

Re: [PATCH] KVM: VMX: Do not overwrite vcpu->srcu_idx in vmx_vcpu_reset

[Gleb Natapov]

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.

Re: [PATCH] KVM: VMX: Do not overwrite vcpu->srcu_idx in vmx_vcpu_reset

[Jan Kiszka]

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

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

Re: [PATCH] KVM: VMX: Do not overwrite vcpu->srcu_idx in vmx_vcpu_reset

[Paolo Bonzini]

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

Re: [PATCH] KVM: VMX: Do not overwrite vcpu->srcu_idx in vmx_vcpu_reset

[Gleb Natapov]

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.

Re: [PATCH] KVM: VMX: Do not overwrite vcpu->srcu_idx in vmx_vcpu_reset

[Paolo Bonzini]

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

Re: [PATCH] KVM: VMX: Do not overwrite vcpu->srcu_idx in vmx_vcpu_reset

[Jan Kiszka]

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

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

Re: [PATCH] KVM: VMX: Do not overwrite vcpu->srcu_idx in vmx_vcpu_reset

[Marcelo Tosatti]

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?

Re: [PATCH] KVM: VMX: Do not overwrite vcpu->srcu_idx in vmx_vcpu_reset

[Jan Kiszka]

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

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

[PATCH v2] KVM: VMX: Do not overwrite vcpu->srcu_idx in vmx_vcpu_reset

[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 <jan.kiszka@siemens.com>
---

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(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 958ac3a..7bc49ca 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,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);
-- 
1.7.3.4

Re: [PATCH v2] KVM: VMX: Do not overwrite vcpu->srcu_idx in vmx_vcpu_reset

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1729 bytes --]

On 2013-03-14 20:39, 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. Save and restore it.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> 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(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 958ac3a..7bc49ca 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,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);
> 

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH] KVM: VMX: Do not overwrite vcpu->srcu_idx in vmx_vcpu_reset

[Marcelo Tosatti]

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);
}