On Tue, Mar 05, 2013 at 02:25:51PM +0100, Jan Kiszka wrote:
> On 2013-03-05 11:50, Paolo Bonzini wrote:
> > Il 05/03/2013 10:37, Gleb Natapov ha scritto:
> >>>>> Not at all. I'm keeping the state in a single place, mp_state. I just
> >>>>> have to make sure that I do not loose asynchronous events - what INIT
> >>>>> and SIPI are.
> >>>>
> >>>> As evident from this code:
> >>>> + if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED ||
> >>>> + test_bit(KVM_REQ_INIT, &vcpu->requests)) {
> >>>> the state is in two places.
> >>> That's just to protect the content of sipi_vector during delivery.
> >
> > Is that even needed? Can we just do an unconditional:
> >
> > vcpu->arch.sipi_vector = vector;
> > /* make sure sipi_vector is visible for the receiver */
> > smp_wmb();
> > kvm_make_request(KVM_REQ_SIPI, vcpu);
> > kvm_vcpu_kick(vcpu);
> >
> > and check in the request handler that we did get an INIT?
> >
> >>> We could drop the complete if clause if we protected that variable differently.
> >>
> >> I understand why the code is here. I am saying that this is the evidence
> >> that the state is in two places.
> >
> > I agree with Gleb here. The state is in two places. I'm not saying that
> > using requests is wrong, it sounds nice especially for nested INIT. But
> > there is something nasty in the patches so far.
> >
> > BTW checking the requests in kvm_apic_has_interrupt seems nicer than
> > synchronizing in kvm_arch_cpu_runnable; it is a bit ugly to have side
> > effects in kvm_arch_cpu_runnable. Then you can actually _process_
> > the requests only in vcpu_enter_guest and kvm_arch_vcpu_ioctl_get_mpstate().
> > In kvm_arch_vcpu_ioctl_put_mpstate(), KVM_MP_STATE_SIPI_RECEIVED has to
> > become KVM_MP_STATE_INIT_RECEIVED with KVM_REQ_SIPI set.
>
> An INIT or SIPI signal is not really an interrupt in the sense that
> kvm_cpu_has_interrupt or kvm_get_apic_interrupt expect (the current
> users of kvm_apic_has_interrupt). I think we would move some ugliness
> around this way, not yet resolve it.
>
> But it probably makes sense to refactor the kvm_check_init_and_sipi
> service to something that the APIC provide (is that what you are
> thinking of, Gleb?). That will not reduce the number of places where we
> have to check, but it should encapsulate things a bit cleaner.
>
Yes, that what I am thinking. And yes I do not think it will reduce
number of places we need to check it much (or at all) unfortunately.
But if we want to hold INIT signal pending (until vmxoff for instance)
we cannot do that in ->requests anyway.
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
