Re: [PATCH v2] KVM: x86: Rework INIT and SIPI handling

[Gleb Natapov <gleb@redhat.com>]

On Thu, Mar 14, 2013 at 01:25:04PM +0100, Jan Kiszka wrote:
> On 2013-03-14 13:18, Gleb Natapov wrote:
> > On Thu, Mar 14, 2013 at 01:16:41PM +0100, Jan Kiszka wrote:
> >> On 2013-03-14 13:12, Gleb Natapov wrote:
> >>> On Thu, Mar 14, 2013 at 12:29:48PM +0100, Jan Kiszka wrote:
> >>>> On 2013-03-14 11:15, Jan Kiszka wrote:
> >>>>>>>  
> >>>>>>> -	if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED)) {
> >>>>>>> -		pr_debug("vcpu %d received sipi with vector # %x\n",
> >>>>>>> -			 vcpu->vcpu_id, vcpu->arch.sipi_vector);
> >>>>>>> -		kvm_lapic_reset(vcpu);
> >>>>>>> -		kvm_vcpu_reset(vcpu);
> >>>>>>> -		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
> >>>>>>> -	}
> >>>>>>> -
> >>>>>>>  	vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
> >>>>>>>  	r = vapic_enter(vcpu);
> >>>>>>
> >>>>>> vmx_vcpu_reset overwrites vcpu->srcu_idx if ->vcpu_reset is called from
> >>>>>> within srcu section.
> >>>>>
> >>>>> Indeed.
> >>>>>
> >>>>> Do you know what the look over vmx_set_cr0 actually protects?
> >>>>
> >>>> Found it: It's not actually protecting anything. enter_rmode is called,
> >>>> and that assumes that lock to be held. If enter_rmode faces an
> >>>> uninitialized tss, it drops the lock before calling vmx_set_tss_addr.
> >>>>
> >>>> Well, I wonder if that is a good place to fix the TSS issue. Why not
> >>>> make that special case (lacking KVM_SET_TSS_ADDR before first KVM_RUN) a
> >>>> static jump key and check for it on KVM_RUN?
> >>>>
> >>> Or finally break userspace that does not set it before calling kvm_run.
> >>> I haven't seen people complain about "kvm: KVM_SET_TSS_ADDR need to be
> >>> called before entering vcpu" warning in dmesg. Or create TSS mem slot at
> >>> 0xfeffd000 during VM creation and destroy it if userspace overwrites it.
> >>
> >> Whatever is preferred, I'm not able to decide (about ABI "breakage"
> >> specifically). I just think any of them would be better than pulling
> >> vcpu_reset out of the inner loop again just to fulfill the locking
> >> requirements.
> >>
> > I agree. Lets try second approach. Can you write a patch?
> 
> Task queued for later today or tomorrow. I suppose you hold back this
> patch here for now anyway.
> 
The patch is pushed to queue already, I prefer to apply fix on top but
if it will take time I can rebase queue for now.

--
			Gleb.