[PATCH] kvm: arm/arm64: vgic: Fix the sequence principle about vgic save/restore.

[PATCH] kvm: arm/arm64: vgic: Fix the sequence principle about vgic save/restore.

[wanghaibin]

At present, take GICv3 as as an example, our implementation is that, the operation
of the recovery ICH_HCR register is prior to the recovery of ICH_LRn registers in vgic
state restore. Thus, the ICH_LRn registers are 0, and if ICH_HCR.UIE is configured to 1,
a large number of unnecessary maintenance interrupts will be triggered.

So, We have to make sure the sequence that ICH_LRn early than ICH_HCR when restore state,
and the other way round when save state.

Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
---
 virt/kvm/arm/hyp/vgic-v2-sr.c | 5 ++---
 virt/kvm/arm/hyp/vgic-v3-sr.c | 4 ++--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/virt/kvm/arm/hyp/vgic-v2-sr.c b/virt/kvm/arm/hyp/vgic-v2-sr.c
index a3f18d3..2ed5df1 100644
--- a/virt/kvm/arm/hyp/vgic-v2-sr.c
+++ b/virt/kvm/arm/hyp/vgic-v2-sr.c
@@ -70,12 +70,11 @@ void __hyp_text __vgic_v2_save_state(struct kvm_vcpu *vcpu)
 		return;
 
 	if (used_lrs) {
+		writel_relaxed(0, base + GICH_HCR);
 		cpu_if->vgic_apr = readl_relaxed(base + GICH_APR);
 
 		save_elrsr(vcpu, base);
 		save_lrs(vcpu, base);
-
-		writel_relaxed(0, base + GICH_HCR);
 	} else {
 		cpu_if->vgic_elrsr = ~0UL;
 		cpu_if->vgic_apr = 0;
@@ -96,12 +95,12 @@ void __hyp_text __vgic_v2_restore_state(struct kvm_vcpu *vcpu)
 		return;
 
 	if (used_lrs) {
-		writel_relaxed(cpu_if->vgic_hcr, base + GICH_HCR);
 		writel_relaxed(cpu_if->vgic_apr, base + GICH_APR);
 		for (i = 0; i < used_lrs; i++) {
 			writel_relaxed(cpu_if->vgic_lr[i],
 				       base + GICH_LR0 + (i * 4));
 		}
+		writel_relaxed(cpu_if->vgic_hcr, base + GICH_HCR);
 	}
 }
 
diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
index 32c3295..b625884 100644
--- a/virt/kvm/arm/hyp/vgic-v3-sr.c
+++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
@@ -220,8 +220,6 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
 	nr_pre_bits = vtr_to_nr_pre_bits(val);
 
 	if (used_lrs) {
-		write_gicreg(cpu_if->vgic_hcr, ICH_HCR_EL2);
-
 		switch (nr_pre_bits) {
 		case 7:
 			write_gicreg(cpu_if->vgic_ap0r[3], ICH_AP0R3_EL2);
@@ -244,6 +242,8 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
 
 		for (i = 0; i < used_lrs; i++)
 			__gic_v3_set_lr(cpu_if->vgic_lr[i], i);
+
+		write_gicreg(cpu_if->vgic_hcr, ICH_HCR_EL2);
 	}
 
 	/*
-- 
1.8.3.1

Re: [PATCH] kvm: arm/arm64: vgic: Fix the sequence principle about vgic save/restore.

[Marc Zyngier]

On 05/06/17 11:30, wanghaibin wrote:
> At present, take GICv3 as as an example, our implementation is that, the operation
> of the recovery ICH_HCR register is prior to the recovery of ICH_LRn registers in vgic
> state restore. Thus, the ICH_LRn registers are 0, and if ICH_HCR.UIE is configured to 1,
> a large number of unnecessary maintenance interrupts will be triggered.

Is that a theoretical problem? Or something you've actually observed?

At the point where we restore the vgic state, interrupts are disabled.
And by the time we enter the guest, we fully expect the HW to be in a
stable state, and no spurious interrupt would be delivered.

I also dispute the "large number of unnecessary maintenance interrupts".
This large number is at most *one*.

Or am I missing something obvious?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] kvm: arm/arm64: vgic: Fix the sequence principle about vgic save/restore.

[wanghaibin]

On 2017/6/5 21:56, Marc Zyngier wrote:

> On 05/06/17 11:30, wanghaibin wrote:
>> At present, take GICv3 as as an example, our implementation is that, the operation
>> of the recovery ICH_HCR register is prior to the recovery of ICH_LRn registers in vgic
>> state restore. Thus, the ICH_LRn registers are 0, and if ICH_HCR.UIE is configured to 1,
>> a large number of unnecessary maintenance interrupts will be triggered.
> 
> Is that a theoretical problem? Or something you've actually observed?


I observed this problem with that boot a android vm (with 4 vcpus) on my hisilicon D03 board (4 LRs support).
Boot the android vm will failed because of any virtual interrupts can't deliver to the vm timely.

I watched the maintenance interrupt (/proc/interrupts, GICv3 hwirq:25), and the number can up to 200000+ per second.
(sorry for my express fault, the large number of unnecessary maintenance interrupts means this).

> 
> At the point where we restore the vgic state, interrupts are disabled.
> And by the time we enter the guest, we fully expect the HW to be in a
> stable state, and no spurious interrupt would be delivered.


At that point where restore the vgic state,  it's true that interrupts are disabled (local_irq_disable),
but in my opinion, here maybe a maintenance interrupt pending at physical redist (at that point it can be delivered).
and it will be delivered at the moment that current vcpu's ctxt restore and entry (eret, here, PSTATE.I maybe unmask).
Thus, the vcpu will be kicked out immediately. At the next vgic state restore point, go round and begin again.

Thanks.

> 
> I also dispute the "large number of unnecessary maintenance interrupts".
> This large number is at most *one*.
> 
> Or am I missing something obvious?



> 
> Thanks,
> 
> 	M.

Re: [PATCH] kvm: arm/arm64: vgic: Fix the sequence principle about vgic save/restore.

[Marc Zyngier]

On 06/06/17 04:23, wanghaibin wrote:
> On 2017/6/5 21:56, Marc Zyngier wrote:
> 
>> On 05/06/17 11:30, wanghaibin wrote:
>>> At present, take GICv3 as as an example, our implementation is that, the operation
>>> of the recovery ICH_HCR register is prior to the recovery of ICH_LRn registers in vgic
>>> state restore. Thus, the ICH_LRn registers are 0, and if ICH_HCR.UIE is configured to 1,
>>> a large number of unnecessary maintenance interrupts will be triggered.
>>
>> Is that a theoretical problem? Or something you've actually observed?
> 
> 
> I observed this problem with that boot a android vm (with 4 vcpus) on my hisilicon D03 board (4 LRs support).
> Boot the android vm will failed because of any virtual interrupts can't deliver to the vm timely.
> 
> I watched the maintenance interrupt (/proc/interrupts, GICv3 hwirq:25), and the number can up to 200000+ per second.
> (sorry for my express fault, the large number of unnecessary maintenance interrupts means this).

That's really odd, as we disable that interrupt before exiting HYP (you
should never see that counter increasing). So either your GIC is
incredibly slow (it fails to retire the interrupt in a timely manner),
or it is configured as an Edge interrupt (and thus cannot be retired).

Could you please investigate the last point? Also, do you see warnings
from the virtual timer (something about unexpected interrupts)?

> 
>>
>> At the point where we restore the vgic state, interrupts are disabled.
>> And by the time we enter the guest, we fully expect the HW to be in a
>> stable state, and no spurious interrupt would be delivered.
> 
> 
> At that point where restore the vgic state,  it's true that interrupts are disabled (local_irq_disable),
> but in my opinion, here maybe a maintenance interrupt pending at physical redist (at that point it can be delivered).
> and it will be delivered at the moment that current vcpu's ctxt restore and entry (eret, here, PSTATE.I maybe unmask).
> Thus, the vcpu will be kicked out immediately. At the next vgic state restore point, go round and begin again.

I understand what happens when the interrupt is observed, but I want to
understand why it is observed.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] kvm: arm/arm64: vgic: Fix the sequence principle about vgic save/restore.

[wanghaibin]

On 2017/6/6 16:34, Marc Zyngier wrote:

> On 06/06/17 04:23, wanghaibin wrote:
>> On 2017/6/5 21:56, Marc Zyngier wrote:
>>
>>> On 05/06/17 11:30, wanghaibin wrote:
>>>> At present, take GICv3 as as an example, our implementation is that, the operation
>>>> of the recovery ICH_HCR register is prior to the recovery of ICH_LRn registers in vgic
>>>> state restore. Thus, the ICH_LRn registers are 0, and if ICH_HCR.UIE is configured to 1,
>>>> a large number of unnecessary maintenance interrupts will be triggered.
>>>
>>> Is that a theoretical problem? Or something you've actually observed?
>>
>>
>> I observed this problem with that boot a android vm (with 4 vcpus) on my hisilicon D03 board (4 LRs support).
>> Boot the android vm will failed because of any virtual interrupts can't deliver to the vm timely.
>>
>> I watched the maintenance interrupt (/proc/interrupts, GICv3 hwirq:25), and the number can up to 200000+ per second.
>> (sorry for my express fault, the large number of unnecessary maintenance interrupts means this).
> 
> That's really odd, as we disable that interrupt before exiting HYP (you
> should never see that counter increasing). So either your GIC is
> incredibly slow (it fails to retire the interrupt in a timely manner),
> or it is configured as an Edge interrupt (and thus cannot be retired).
> 
> Could you please investigate the last point? Also, do you see warnings
> from the virtual timer (something about unexpected interrupts)?


Yes, just like your said, it's a known defect on hisilicon D03 board,
and the that counter of maintenance interrupt does not increase on hisilicon D05 board.

> 
>>
>>>
>>> At the point where we restore the vgic state, interrupts are disabled.
>>> And by the time we enter the guest, we fully expect the HW to be in a
>>> stable state, and no spurious interrupt would be delivered.
>>
>>
>> At that point where restore the vgic state,  it's true that interrupts are disabled (local_irq_disable),
>> but in my opinion, here maybe a maintenance interrupt pending at physical redist (at that point it can be delivered).
>> and it will be delivered at the moment that current vcpu's ctxt restore and entry (eret, here, PSTATE.I maybe unmask).
>> Thus, the vcpu will be kicked out immediately. At the next vgic state restore point, go round and begin again.
> 
> I understand what happens when the interrupt is observed, but I want to
> understand why it is observed.


The ICH_HCR_EL2.UIE, spec says that:
Underflow Interrupt Enable. Enables the signaling of a maintenance interrupt when the List
registers are empty, or hold only one valid entry.

After the commit (b40c4892) , we clear the ICH_LRn when save the vgic state.
At next vgic restore point, if ICH_HCR_EL2.UIE = 1,  and ICH_LRn is all clear,
I think here will be a maintenance interrupt triggered when ICH_HCR restore early than ICH_LRn restore.

Thanks.

> 
> Thanks,
> 
> 	M.

Re: [PATCH] kvm: arm/arm64: vgic: Fix the sequence principle about vgic save/restore.

[Marc Zyngier]

On Tue, Jun 06 2017 at  5:29:54 pm BST, wanghaibin <wanghaibin.wang@huawei.com> wrote:
> On 2017/6/6 16:34, Marc Zyngier wrote:
>
>> On 06/06/17 04:23, wanghaibin wrote:
>>> On 2017/6/5 21:56, Marc Zyngier wrote:
>>>
>>>> On 05/06/17 11:30, wanghaibin wrote:
>>>>> At present, take GICv3 as as an example, our implementation is
>>>>> that, the operation
>>>>> of the recovery ICH_HCR register is prior to the recovery of
>>>>> ICH_LRn registers in vgic
>>>>> state restore. Thus, the ICH_LRn registers are 0, and if
>>>>> ICH_HCR.UIE is configured to 1,
>>>>> a large number of unnecessary maintenance interrupts will be triggered.
>>>>
>>>> Is that a theoretical problem? Or something you've actually observed?
>>>
>>>
>>> I observed this problem with that boot a android vm (with 4 vcpus)
>>> on my hisilicon D03 board (4 LRs support).
>>> Boot the android vm will failed because of any virtual interrupts
>>> can't deliver to the vm timely.
>>>
>>> I watched the maintenance interrupt (/proc/interrupts, GICv3
>>> hwirq:25), and the number can up to 200000+ per second.
>>> (sorry for my express fault, the large number of unnecessary
>>> maintenance interrupts means this).
>> 
>> That's really odd, as we disable that interrupt before exiting HYP (you
>> should never see that counter increasing). So either your GIC is
>> incredibly slow (it fails to retire the interrupt in a timely manner),
>> or it is configured as an Edge interrupt (and thus cannot be retired).
>> 
>> Could you please investigate the last point? Also, do you see warnings
>> from the virtual timer (something about unexpected interrupts)?
>
>
> Yes, just like your said, it's a known defect on hisilicon D03 board,
> and the that counter of maintenance interrupt does not increase on
> hisilicon D05 board.

Can you please answer my question about the configuration (edge or
level) of the maintenance interrupt?

>
>> 
>>>
>>>>
>>>> At the point where we restore the vgic state, interrupts are disabled.
>>>> And by the time we enter the guest, we fully expect the HW to be in a
>>>> stable state, and no spurious interrupt would be delivered.
>>>
>>>
>>> At that point where restore the vgic state, it's true that
>>> interrupts are disabled (local_irq_disable),
>>> but in my opinion, here maybe a maintenance interrupt pending at
>>> physical redist (at that point it can be delivered).
>>> and it will be delivered at the moment that current vcpu's ctxt
>>> restore and entry (eret, here, PSTATE.I maybe unmask).
>>> Thus, the vcpu will be kicked out immediately. At the next vgic
>>> state restore point, go round and begin again.
>> 
>> I understand what happens when the interrupt is observed, but I want to
>> understand why it is observed.
>
>
> The ICH_HCR_EL2.UIE, spec says that:
> Underflow Interrupt Enable. Enables the signaling of a maintenance
> interrupt when the List
> registers are empty, or hold only one valid entry.
>
> After the commit (b40c4892) , we clear the ICH_LRn when save the vgic state.
> At next vgic restore point, if ICH_HCR_EL2.UIE = 1,  and ICH_LRn is all clear,
> I think here will be a maintenance interrupt triggered when ICH_HCR
> restore early than ICH_LRn restore.

I have a rather precise idea of how things work, and I've understood
your point from your first email. I'm trying to understand why you're
the only person having reported this. So can you please answer the above
question?

Thanks,

        M.
-- 
Jazz is not dead, it just smell funny.

Re: [PATCH] kvm: arm/arm64: vgic: Fix the sequence principle about vgic save/restore.

[wanghaibin]

On 2017/6/6 17:43, Marc Zyngier wrote:

> On Tue, Jun 06 2017 at  5:29:54 pm BST, wanghaibin <wanghaibin.wang@huawei.com> wrote:
>> On 2017/6/6 16:34, Marc Zyngier wrote:
>>
>>> On 06/06/17 04:23, wanghaibin wrote:
>>>> On 2017/6/5 21:56, Marc Zyngier wrote:
>>>>
>>>>> On 05/06/17 11:30, wanghaibin wrote:
>>>>>> At present, take GICv3 as as an example, our implementation is
>>>>>> that, the operation
>>>>>> of the recovery ICH_HCR register is prior to the recovery of
>>>>>> ICH_LRn registers in vgic
>>>>>> state restore. Thus, the ICH_LRn registers are 0, and if
>>>>>> ICH_HCR.UIE is configured to 1,
>>>>>> a large number of unnecessary maintenance interrupts will be triggered.
>>>>>
>>>>> Is that a theoretical problem? Or something you've actually observed?
>>>>
>>>>
>>>> I observed this problem with that boot a android vm (with 4 vcpus)
>>>> on my hisilicon D03 board (4 LRs support).
>>>> Boot the android vm will failed because of any virtual interrupts
>>>> can't deliver to the vm timely.
>>>>
>>>> I watched the maintenance interrupt (/proc/interrupts, GICv3
>>>> hwirq:25), and the number can up to 200000+ per second.
>>>> (sorry for my express fault, the large number of unnecessary
>>>> maintenance interrupts means this).
>>>
>>> That's really odd, as we disable that interrupt before exiting HYP (you
>>> should never see that counter increasing). So either your GIC is
>>> incredibly slow (it fails to retire the interrupt in a timely manner),
>>> or it is configured as an Edge interrupt (and thus cannot be retired).
>>>
>>> Could you please investigate the last point? Also, do you see warnings
>>> from the virtual timer (something about unexpected interrupts)?
>>
>>
>> Yes, just like your said, it's a known defect on hisilicon D03 board,
>> and the that counter of maintenance interrupt does not increase on
>> hisilicon D05 board.
> 
> Can you please answer my question about the configuration (edge or
> level) of the maintenance interrupt?


The dts file GIC node:
        interrupt-controller@6d000000 {
                compatible = "hisilicon,gic-v3";
                #interrupt-cells = <0x3>;
                #address-cells = <0x2>;
                #size-cells = <0x2>;
                interrupts = <0x1 0x9 0xff04>;

The interrupt configuration is level triggered.

Thanks.

> 
>>
>>>
>>>>
>>>>>
>>>>> At the point where we restore the vgic state, interrupts are disabled.
>>>>> And by the time we enter the guest, we fully expect the HW to be in a
>>>>> stable state, and no spurious interrupt would be delivered.
>>>>
>>>>
>>>> At that point where restore the vgic state, it's true that
>>>> interrupts are disabled (local_irq_disable),
>>>> but in my opinion, here maybe a maintenance interrupt pending at
>>>> physical redist (at that point it can be delivered).
>>>> and it will be delivered at the moment that current vcpu's ctxt
>>>> restore and entry (eret, here, PSTATE.I maybe unmask).
>>>> Thus, the vcpu will be kicked out immediately. At the next vgic
>>>> state restore point, go round and begin again.
>>>
>>> I understand what happens when the interrupt is observed, but I want to
>>> understand why it is observed.
>>
>>
>> The ICH_HCR_EL2.UIE, spec says that:
>> Underflow Interrupt Enable. Enables the signaling of a maintenance
>> interrupt when the List
>> registers are empty, or hold only one valid entry.
>>
>> After the commit (b40c4892) , we clear the ICH_LRn when save the vgic state.
>> At next vgic restore point, if ICH_HCR_EL2.UIE = 1,  and ICH_LRn is all clear,
>> I think here will be a maintenance interrupt triggered when ICH_HCR
>> restore early than ICH_LRn restore.
> 
> I have a rather precise idea of how things work, and I've understood
> your point from your first email. I'm trying to understand why you're
> the only person having reported this. So can you please answer the above
> question?
> 
> Thanks,
> 
>         M.