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

[Gleb Natapov <gleb@redhat.com>]

On Thu, Mar 14, 2013 at 04:32:16PM +0100, Jan Kiszka wrote:
> On 2013-03-14 13:28, Gleb Natapov wrote:
> > 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.
> 
> OK, tried this approach, but I don't think it easily works: If we
> unconditionally set the TSS, we can create conflicts with userland's
> mapping if that is different, no?
> 
Yes, you are right. It is possible to add hack that removes the TSS slot
if userspace add overlapping slot, but this will push the hack to far
into the common code.

> Leaves us with dropping the fixup, just leaving the warning and let the
> guest crash. Or my static key idea.
> 
The warning was there for a long time. I say lets drop the fixup leaving
the warning. Static key will be a back up plan. Marcelo, Paolo what do you think?

--
			Gleb.