Re: [PATCH v4 0/3] KVM: arm/arm64: Clean up some obsolete code

[Peter Maydell]

On 5 November 2015 at 06:50, Pavel Fedin 
>  You know, since we are talking about this...  This definitely
> has something to do with the reset, and... Looks like nobody
> resets vGIC/vTimer, unless the userland does it explicitly by
> resetting every register by hand.

This is how KVM in-kernel device reset is supposed to work, yes.

thanks
-- PMM
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v4 0/3] KVM: arm/arm64: Clean up some obsolete code

[Pavel Fedin]

 Hello!

> Actually, I seem to have been just incredibly unlucky with my test
> cycles, because I eventually reproduced the bug without your patches.

 Or lucky, without "un" :)

> I'm going to take this version of the series because that's what I
> reviewed and tested.

 It's OK, as i wrote, v5 is no different from v4 actually, just 0001 bisected. 
And making it was useful because it helped me to make
sure once again that i haven't messed anything up.

> Sorry for the noise.

 It's OK, thank you very much for putting efforts into testing and cooperation.
 You know, since we are talking about this...  This definitely has something to 
do with the reset, and... Looks like nobody resets
vGIC/vTimer, unless the userland does it explicitly by resetting every register 
by hand.
 I know, there is no global "reset" function for the whole VM. But, at least we 
have reset ioctl for vCPU. What if we hook up
vGIC/vTimer there, and reset at least per-CPU objects (CPU interface + redist + 
timer) at this point?

 P.S. I've seen your PULL, and it is missing a little thing that could be good 
for 4.4 too. I've fixed one more bug recently, it
reproduces on CP15-timer-less boards like Exynos: 
http://www.spinics.net/lists/kvm/msg122746.html. Just to make sure that you 
don't
miss it.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/3] KVM: arm/arm64: Clean up some obsolete code

[Christoffer Dall]

On Tue, Nov 03, 2015 at 12:44:54PM +0300, Pavel Fedin wrote:
>  Hello!
> 
> >  By this time i'll make a very minimal version of patch 0001, for you to 
> > test it. If we have
> > problems with current 0001, which we
> > cannot solve quickly, we could stick to that version then, which will 
> > provide the necessary
> > changes to plug in LPIs, yet with
> > minimal changes (it will only remove vgic_irq_lr_map).
> >  I guess i should have done it before. Or, i could even respin v5, with 
> > current 0001 split up.
> > This should make it easier to bisect
> > the problem.
> 
>  So, i have just sent v5, conditions are the same as before. It is OK to stop 
> at any point, and actually you should be able to
> easily throw away 0003 and apply just 1, 2, 4. The minimum needed thing for 
> LPIs introduction is 0001.
>  You can also stick to v4 if the problem does not get triggered by its first 
> patch, if you prefer reduced commit log.
> 
Actually, I seem to have been just incredibly unlucky with my test
cycles, because I eventually reproduced the bug without your patches.

I'm going to take this version of the series because that's what I
reviewed and tested.

Sorry for the noise.

-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v4 0/3] KVM: arm/arm64: Clean up some obsolete code

[Pavel Fedin]

 Hello!

>  By this time i'll make a very minimal version of patch 0001, for you to test 
> it. If we have
> problems with current 0001, which we
> cannot solve quickly, we could stick to that version then, which will provide 
> the necessary
> changes to plug in LPIs, yet with
> minimal changes (it will only remove vgic_irq_lr_map).
>  I guess i should have done it before. Or, i could even respin v5, with 
> current 0001 split up.
> This should make it easier to bisect
> the problem.

 So, i have just sent v5, conditions are the same as before. It is OK to stop 
at any point, and actually you should be able to
easily throw away 0003 and apply just 1, 2, 4. The minimum needed thing for 
LPIs introduction is 0001.
 You can also stick to v4 if the problem does not get triggered by its first 
patch, if you prefer reduced commit log.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v4 0/3] KVM: arm/arm64: Clean up some obsolete code

[Pavel Fedin]

 Hello!

> I ran this through my test scripts and I'm now quite sure that there's
> some breakage in here.
> 
> One of my tests is running two VMs in parallel, each booting up, running
> hackbench, and then doing reboot (from within the guest), and just
> repeating like that.
> 
> I've run your patches in the above config 100 times, and every time,
> the rebooting VMs got stuck before 50 reboots.
> 
> Without these patches, I could run the above config 100 times, and every
> time, the rebooting VMs passed 200 reboots.

 Huh, the description looks like some problem with vgic_retire_disabled_irqs(). 
By the way, during reboot, who does call it? The
only call i see is in vgic_handle_enable_reg(), which obviously just processes 
emulated register accesses...
 And the only thing i know is that in case of GICv2 the userland resets vGIC 
manually by resetting each register to its default
value (therefore all ENABLER are set to 0). At least qemu does this, and i'm 
not sure about kvmtool. And in case of vGICv3 nobody
can do this because there's no API to set registers yet. So, could we be 
rebooting with interrupts enabled or something like that?
 So: what kind of container are you running and what vGIC version? Does this 
problem reproduce with both vGICv2 and vGICv3?

 By this time i'll make a very minimal version of patch 0001, for you to test 
it. If we have problems with current 0001, which we
cannot solve quickly, we could stick to that version then, which will provide 
the necessary changes to plug in LPIs, yet with
minimal changes (it will only remove vgic_irq_lr_map).
 I guess i should have done it before. Or, i could even respin v5, with current 
0001 split up. This should make it easier to bisect
the problem.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/3] KVM: arm/arm64: Clean up some obsolete code

[Christoffer Dall]

On Tue, Oct 27, 2015 at 11:37:28AM +0300, Pavel Fedin wrote:
> Current KVM code has lots of old redundancies, which can be cleaned up.
> This patchset is actually a better alternative to
> http://www.spinics.net/lists/arm-kernel/msg430726.html, which allows to
> keep piggy-backed LRs. The idea is based on the fact that our code also
> maintains LR state in elrsr, and this information is enough to track LR
> usage.
> 
> In case of problems this series can be applied partially, each patch is
> a complete refactoring step on its own.
> 
> Thanks to Andre Przywara for pinpointing some 4.3+ specifics.
> 
> This version has been tested on SMDK5410 development board
> (Exynos5410 SoC).

I ran this through my test scripts and I'm now quite sure that there's
some breakage in here.

One of my tests is running two VMs in parallel, each booting up, running
hackbench, and then doing reboot (from within the guest), and just
repeating like that.

I've run your patches in the above config 100 times, and every time,
the rebooting VMs got stuck before 50 reboots.

Without these patches, I could run the above config 100 times, and every
time, the rebooting VMs passed 200 reboots.

I'll try to test each patch individually soon.

-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 0/3] KVM: arm/arm64: Clean up some obsolete code

[Pavel Fedin]

Current KVM code has lots of old redundancies, which can be cleaned up.
This patchset is actually a better alternative to
http://www.spinics.net/lists/arm-kernel/msg430726.html, which allows to
keep piggy-backed LRs. The idea is based on the fact that our code also
maintains LR state in elrsr, and this information is enough to track LR
usage.

In case of problems this series can be applied partially, each patch is
a complete refactoring step on its own.

Thanks to Andre Przywara for pinpointing some 4.3+ specifics.

This version has been tested on SMDK5410 development board
(Exynos5410 SoC).

v3 => v4:
- Reordered changes for purpose of better understanding and bisection. All
  changes related to vgic_retire_lr() are gathered in one patch now.

v2 => v3:
- Removed two unused variables in __kvm_vgic_flush_hwstate(), overlooked
  leftover from v1.

v1 => v2:
- Rebased to kvmarm/next of 23.10.2015.
- Do not use vgic_retire_lr() for initializing ELRSR bitmask, because now
  it also handles pushback of PENDING state, use direct initialization
  instead (copied from Andre's patchset).
- Took more care about vgic_retire_lr(), which has deserved own patch.

Pavel Fedin (3):
  KVM: arm/arm64: Optimize away redundant LR tracking
  KVM: arm/arm64: Clean up vgic_retire_lr() and surroundings
  KVM: arm/arm64: Merge vgic_set_lr() and vgic_sync_lr_elrsr()

 include/kvm/arm_vgic.h |   7 
 virt/kvm/arm/vgic-v2.c |   6 +--
 virt/kvm/arm/vgic-v3.c |   6 +--
 virt/kvm/arm/vgic.c| 104 +
 4 files changed, 29 insertions(+), 94 deletions(-)

-- 
2.4.4

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html