Re: AVIC related warning in enable_irq_window
On 5/5/20 7:12 PM, Paolo Bonzini wrote:
On 05/05/20 09:55, Suravee Suthikulpanit wrote:
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?
Yes, that's also a possibility. It's not too much extra complication if
we add a new argument to kvm_make_vcpus_request_mask, like this:
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 74bdb7bf3295..8f9dadb1ef42 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -258,7 +258,7 @@ static inline bool kvm_kick_many_cpus(const struct cpumask
*cpus, bool wait)
return true;
}
-bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req,
+bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req, struct
kvm_vcpu *except,
unsigned long *vcpu_bitmap, cpumask_var_t tmp)
{
int i, cpu, me;
@@ -270,6 +270,8 @@ bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned
int req,
kvm_for_each_vcpu(i, vcpu, kvm) {
if (vcpu_bitmap && !test_bit(i, vcpu_bitmap))
continue;
+ if (vcpu == except)
+ continue;
kvm_make_request(req, vcpu);
cpu = vcpu->cpu;
Paolo
Sounds good. I'll take care of this today.
Suravee
Re: AVIC related warning in enable_irq_window
On 05/05/20 09:55, Suravee Suthikulpanit wrote:
>> 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.
Indeed this is even documented:
NOTE: This value is written back to the VMCB at #VMEXIT.
This field is ignored on VMRUN when AVIC is
enabled.
> 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?
Yes, that's also a possibility. It's not too much extra complication if
we add a new argument to kvm_make_vcpus_request_mask, like this:
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 74bdb7bf3295..8f9dadb1ef42 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -258,7 +258,7 @@ static inline bool kvm_kick_many_cpus(const struct cpumask
*cpus, bool wait)
return true;
}
-bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req,
+bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req, struct
kvm_vcpu *except,
unsigned long *vcpu_bitmap, cpumask_var_t tmp)
{
int i, cpu, me;
@@ -270,6 +270,8 @@ bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned
int req,
kvm_for_each_vcpu(i, vcpu, kvm) {
if (vcpu_bitmap && !test_bit(i, vcpu_bitmap))
continue;
+ if (vcpu == except)
+ continue;
kvm_make_request(req, vcpu);
cpu = vcpu->cpu;
Paolo
Re: AVIC related warning in enable_irq_window
On Tue, 2020-05-05 at 14:55 +0700, Suravee Suthikulpanit wrote: > 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 corpus 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. I currently am waiting for the decision on how to we are going to fix this. I don't have a strong opinion on how to fix this, but at least I think that we know what is going on. Initially I was thinking that something was broken in AVIC, especially when I noticed that guest would hang when I did LatencyMon benchmark in it. Luckily the other fix that I tested and reviewed seems to fix those hangs. In a few days I plan to do some nvme passthrough stress testing as I used to do when I was developing my nvme-mdev driver with AVIC. I am very curios on how this will turn out. Best regards, Maxim Levitsky > > Thanks, > Suravee > > > > Paolo > > > >
Re: AVIC related warning in enable_irq_window
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
Re: AVIC related warning in enable_irq_window
On 04/05/20 12:37, Suravee Suthikulpanit wrote:
> Paolo / Maxim,
>
> 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:
Paolo / Maxim,
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));
>>
>> Paolo
>>
>
> Quick update. I tried your suggestion as following, and it's showing the
> warning still.
> I'll look further into this.
Ok, thanks. 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.
Paolo
> arch/x86/kvm/svm/svm.c | 20 +---
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 2f379ba..142c4b9 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1368,9 +1368,6 @@ static inline void svm_enable_vintr(struct
> vcpu_svm *svm)
> {
> struct vmcb_control_area *control;
>
> - /* The following fields are ignored when AVIC is enabled */
> - WARN_ON(kvm_vcpu_apicv_active(&svm->vcpu));
> -
> /*
> * This is just a dummy VINTR to actually cause a vmexit to happen.
> * Actual injection of virtual interrupts happens through EVENTINJ.
> @@ -3322,6 +3319,11 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
> vcpu->arch.apic->lapic_timer.timer_advance_ns)
> kvm_wait_lapic_expire(vcpu);
>
> +//SURAVEE
> + WARN_ON((svm->vmcb->control.int_ctl &
> + (AVIC_ENABLE_MASK | V_IRQ_MASK))
> + == (AVIC_ENABLE_MASK | V_IRQ_MASK));
> +
>
> Suravee
>
Re: AVIC related warning in enable_irq_window
Paolo / Maxim,
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:
Paolo / Maxim,
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));
Paolo
Quick update. I tried your suggestion as following, and it's showing the
warning still.
I'll look further into this.
arch/x86/kvm/svm/svm.c | 20 +---
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 2f379ba..142c4b9 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1368,9 +1368,6 @@ static inline void svm_enable_vintr(struct vcpu_svm *svm)
{
struct vmcb_control_area *control;
- /* The following fields are ignored when AVIC is enabled */
- WARN_ON(kvm_vcpu_apicv_active(&svm->vcpu));
-
/*
* This is just a dummy VINTR to actually cause a vmexit to happen.
* Actual injection of virtual interrupts happens through EVENTINJ.
@@ -3322,6 +3319,11 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
vcpu->arch.apic->lapic_timer.timer_advance_ns)
kvm_wait_lapic_expire(vcpu);
+//SURAVEE
+ WARN_ON((svm->vmcb->control.int_ctl &
+(AVIC_ENABLE_MASK | V_IRQ_MASK))
+== (AVIC_ENABLE_MASK | V_IRQ_MASK));
+
Suravee
Re: AVIC related warning in enable_irq_window
On 04/05/20 11:13, Maxim Levitsky wrote: > On Mon, 2020-05-04 at 15:46 +0700, Suravee Suthikulpanit wrote: >> Paolo / Maxim, >> >> 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)); Paolo
Re: AVIC related warning in enable_irq_window
On Mon, 2020-05-04 at 15:46 +0700, Suravee Suthikulpanit wrote: > Paolo / Maxim, > > 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()? > > Thanks, > Suravee > 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. Best regards, Maxim Levitsky
Re: AVIC related warning in enable_irq_window
Paolo / Maxim, 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()? Thanks, Suravee
Re: AVIC related warning in enable_irq_window
Maxim / Paolo,
On 5/2/20 11:43 PM, Maxim Levitsky wrote:
On Sat, 2020-05-02 at 18:42 +0200, 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?
svm_set_vintr also has a rather silly
static void svm_set_vintr(struct vcpu_svm *svm)
{
set_intercept(svm, INTERCEPT_VINTR);
if (is_intercept(svm, INTERCEPT_VINTR))
svm_enable_vintr(svm);
}
so I'm thinking of just inlining svm_enable_vintr and renaming
svm_{set,clear}_vintr to svm_{enable,disable}_vintr_window. Would you
like to send two patches for this, the first to remove the WARN_ON and
the second to do the cleanup?
Absolutely! I will send a patch very soon.
I have been debugging this and I have a patch that is supposed to fix this
(instead of removing the WARN ON). Please do not remove the warn on just yet.
Thanks,
Suravee
Re: AVIC related warning in enable_irq_window
On Sat, 2020-05-02 at 18:42 +0200, 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?
>
> svm_set_vintr also has a rather silly
>
> static void svm_set_vintr(struct vcpu_svm *svm)
> {
>set_intercept(svm, INTERCEPT_VINTR);
>if (is_intercept(svm, INTERCEPT_VINTR))
>svm_enable_vintr(svm);
> }
>
> so I'm thinking of just inlining svm_enable_vintr and renaming
> svm_{set,clear}_vintr to svm_{enable,disable}_vintr_window. Would you
> like to send two patches for this, the first to remove the WARN_ON and
> the second to do the cleanup?
Absolutely! I will send a patch very soon.
Best regards,
Maxim Levitsky
>
> Thanks,
>
> Paolo
>
> > The code containing this warning was added in commit
> >
> > 64b5bd27042639dfcc1534f01771b7b871a02ffe
> > KVM: nSVM: ignore L1 interrupt window while running L2 with V_INTR_MASKING=1
Re: AVIC related warning in enable_irq_window
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?
svm_set_vintr also has a rather silly
static void svm_set_vintr(struct vcpu_svm *svm)
{
set_intercept(svm, INTERCEPT_VINTR);
if (is_intercept(svm, INTERCEPT_VINTR))
svm_enable_vintr(svm);
}
so I'm thinking of just inlining svm_enable_vintr and renaming
svm_{set,clear}_vintr to svm_{enable,disable}_vintr_window. Would you
like to send two patches for this, the first to remove the WARN_ON and
the second to do the cleanup?
Thanks,
Paolo
> The code containing this warning was added in commit
>
> 64b5bd27042639dfcc1534f01771b7b871a02ffe
> KVM: nSVM: ignore L1 interrupt window while running L2 with V_INTR_MASKING=1
AVIC related warning in enable_irq_window
Hi! On kernel 5.7-rc3 I get the following warning when I boot a windows 10 VM with AVIC enabled [ 6702.706124] WARNING: CPU: 0 PID: 118232 at arch/x86/kvm/svm/svm.c:1372 enable_irq_window+0x6a/0xa0 [kvm_amd] [ 6702.706124] Modules linked in: vfio_pci vfio_virqfd vfio_iommu_type1 vfio vhost_net vhost vhost_iotlb tap kvm_amd kvm irqbypass hfsplus ntfs msdos xfs hidp ccm rfcomm xt_MASQUERADE xt_conntrack ipt_REJECT iptable_mangle iptable_nat nf_nat ebtable_filter ebtables ip6table_filter ip6_tables tun bridge pmbus ee1004 pmbus_core jc42 cmac bnep sunrpc nls_iso8859_1 nls_cp437 vfat fat dm_mirror dm_region_hash dm_log nvidia_uvm(O) ucsi_ccg typec_ucsi typec wmi_bmof mxm_wmi iwlmvm mac80211 edac_mce_amd edac_core libarc4 joydev nvidia(PO) iwlwifi btusb btrtl input_leds btbcm btintel snd_hda_codec_hdmi bluetooth snd_usb_audio cdc_acm xpad snd_hda_intel cfg80211 snd_usbmidi_lib ff_memless snd_intel_dspcfg snd_rawmidi snd_hda_codec mc ecdh_generic snd_hwdep snd_seq ecc snd_hda_core thunderbolt rfkill i2c_nvidia_gpu pcspkr efi_pstore snd_seq_device bfq snd_pcm snd_timer snd zenpower i2c_piix4 rtc_cmos tpm_crb tpm_tis tpm_tis_core wmi tpm button binfmt_misc ext4 mbcache jbd2 dm_crypt sd_mod uas [ 6702.706146] usb_storage hid_generic usbhid hid crc32_pclmul amdgpu crc32c_intel gpu_sched ttm drm_kms_helper cfbfillrect ahci syscopyarea libahci cfbimgblt sysfillrect libata sysimgblt igb fb_sys_fops ccp cfbcopyarea i2c_algo_bit cec rng_core xhci_pci nvme xhci_hcd drm nvme_core drm_panel_orientation_quirks t10_pi it87 hwmon_vid fuse i2c_dev i2c_core ipv6 autofs4 [last unloaded: irqbypass] [ 6702.706640] CPU: 0 PID: 118232 Comm: CPU 0/KVM Tainted: PW O 5.7.0-rc3+ #30 [ 6702.706667] Hardware name: Gigabyte Technology Co., Ltd. TRX40 DESIGNARE/TRX40 DESIGNARE, BIOS F4c 03/05/2020 [ 6702.706712] RIP: 0010:enable_irq_window+0x6a/0xa0 [kvm_amd] [ 6702.706759] Code: 0c 10 48 89 df e8 56 3c 00 00 48 8b 83 c8 2d 00 00 f6 40 0c 10 74 31 48 83 bb f0 02 00 00 00 74 0b 80 bb f8 02 00 00 00 74 02 <0f> 0b 81 48 60 00 01 0f 00 c7 40 64 00 00 00 00 48 8b 83 c8 2d 00 [ 6702.706804] RSP: 0018:c900073fbd48 EFLAGS: 00010202 [ 6702.706847] RAX: 889f56a51000 RBX: 889f6c4f RCX: [ 6702.706871] RDX: 60603ee37040 RSI: 00fb RDI: 889f6c4f [ 6702.706913] RBP: c900073fbd50 R08: 0001 R09: [ 6702.706959] R10: 029f R11: 0018 R12: 0001 [ 6702.706983] R13: 8000 R14: 889f6c4f R15: c9000729e1a0 [ 6702.707034] FS: 7f8fa96f9700() GS:889fbe00() knlGS: [ 6702.707090] CS: 0010 DS: ES: CR0: 80050033 [ 6702.707114] CR2: CR3: 001f5d236000 CR4: 00340ef0 [ 6702.707173] Call Trace: [ 6702.707217] kvm_arch_vcpu_ioctl_run+0x6e3/0x1b50 [kvm] [ 6702.707273] ? kvm_vm_ioctl_irq_line+0x27/0x40 [kvm] [ 6702.707298] ? _copy_to_user+0x26/0x30 [ 6702.707332] ? kvm_vm_ioctl+0xb3e/0xd90 [kvm] [ 6702.707374] ? set_next_entity+0x78/0xc0 [ 6702.707407] kvm_vcpu_ioctl+0x236/0x610 [kvm] [ 6702.707431] ksys_ioctl+0x8a/0xc0 [ 6702.707492] __x64_sys_ioctl+0x1a/0x20 [ 6702.707528] do_syscall_64+0x58/0x210 [ 6702.707553] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 6702.707596] RIP: 0033:0x7f8fb35e235b [ 6702.707637] Code: 0f 1e fa 48 8b 05 2d 9b 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d fd 9a 0c 00 f7 d8 64 89 01 48 [ 6702.707722] RSP: 002b:7f8fa96f7728 EFLAGS: 0246 ORIG_RAX: 0010 [ 6702.707747] RAX: ffda RBX: 556b7c859b90 RCX: 7f8fb35e235b [ 6702.707788] RDX: RSI: ae80 RDI: 0022 [ 6702.707845] RBP: 7f8fa96f7820 R08: 556b7a6207f0 R09: 00ff [ 6702.707868] R10: 556b79f86ecd R11: 0246 R12: 7ffd9a4264de [ 6702.707908] R13: 7ffd9a4264df R14: 7ffd9a4265a0 R15: 7f8fa96f7a40 [ 6702.707951] ---[ end trace d8146ba85c79e2a9 ]--- It looks like what happening is that enable_irq_window sometimes disables AVIC and then calls the svm_enable_vintr to enable intercept on the moment when guest enables interrupts. 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. The code containing this warning was added in commit 64b5bd27042639dfcc1534f01771b7b871a02ffe KVM: nSVM: ignore L1 interrupt window while running L2 with V_IN
