Re: AVIC related warning in enable_irq_window

[Suravee Suthikulpanit]

Paolo / Maxim,

On 5/4/20 5:49 PM, Paolo Bonzini wrote:
> On 04/05/20 12:37, Suravee Suthikulpanit wrote:
> > On 5/4/20 4:25 PM, Paolo Bonzini wrote:
> > > On 04/05/20 11:13, Maxim Levitsky wrote:
> > > > On Mon, 2020-05-04 at 15:46 +0700, Suravee Suthikulpanit wrote:
> > > > > On 5/2/20 11:42 PM, Paolo Bonzini wrote:
> > > > > > On 02/05/20 15:58, Maxim Levitsky wrote:
> > > > > > > The AVIC is disabled by svm_toggle_avic_for_irq_window, which calls
> > > > > > > kvm_request_apicv_update, which broadcasts the
> > > > > > > KVM_REQ_APICV_UPDATE vcpu request,
> > > > > > > however it doesn't broadcast it to CPU on which now we are
> > > > > > > running, which seems OK,
> > > > > > > because the code that handles that broadcast runs on each VCPU
> > > > > > > entry, thus when this CPU will enter guest mode it will notice 
> > > > > > > and disable the AVIC. >>>>>>>
> > > > > > > However later in svm_enable_vintr, there is test
> > > > > > > 'WARN_ON(kvm_vcpu_apicv_active(&svm->vcpu));'
> > > > > > > which is still true on current CPU because of the above.
> > > > > >
> > > > > > Good point! We can just remove the WARN_ON I think. Can you send
> > > > > > a patch?
> > > > > Instead, as an alternative to remove the WARN_ON(), would it be
> > > > > better to just explicitly calling kvm_vcpu_update_apicv(vcpu) 
> > > > > to update the apicv_active flag right after kvm_request_apicv_update()?
> > > > This should work IMHO, other that the fact kvm_vcpu_update_apicv will
> > > > be called again, when this vcpu is entered since the KVM_REQ_APICV_UPDATE
> > > > will still be pending on it.
> > > > It shoudn't be a problem, and we can even add a check to do nothing
> > > > when it is called while avic is already in target enable state.
> > >
> > > I thought about that but I think it's a bit confusing. If we want to
> > > keep the WARN_ON, Maxim can add an equivalent one to svm_vcpu_run, which
> > > is even better because the invariant is clearer.
> > >
> > > WARN_ON((vmcb->control.int_ctl & (AVIC_ENABLE_MASK | V_IRQ_MASK))
> > >  ÂÂÂÂ== (AVIC_ENABLE_MASK | V_IRQ_MASK));

Based on my experiment, it seems that the hardware sets the V_IRQ_MASK bit
when #VMEXIT despite this bit being ignored when AVIC is enabled.
(I'll double check w/ HW team on this.) In this case, I don't think we can
use the WARN_ON() as suggested above.

I think we should keep the warning in the svm_set_vintr() since we want to know
if the V_IRQ, V_INTR_PRIO, V_IGN_TPR, and V_INTR_VECTOR are ignored when calling
svm_set_vintr().

Instead, I would consider explicitly call kvm_vcpu_update_apicv() since it would
be benefit from not having to wait for the next vcpu_enter_guest for this vcpu to process
the request. This is less confusing to me. In this case, we would need to
kvm_clear_request(KVM_REQ_APICV_UPDATE) for this vcpu as well.

On the other hand, would be it useful to implement kvm_make_all_cpus_request_but_self(),
which sends request to all other vcpus excluding itself?

> By the way, there is another possible cleanup: the clearing
> of V_IRQ_MASK can be removed from interrupt_window_interception since it
> has already called svm_clear_vintr.

Maxim, I can help with the clean up patches if you would prefer.

Thanks,
Suravee


> Paolo