[PATCH 1/5] arm/arm64: KVM: vgic: move reset initialization into vgic_init_maps()

[christoffer.dall@linaro.org (Christoffer Dall)]

On Wed, Dec 10, 2014 at 11:11:41AM +0100, Eric Auger wrote:
> Hi Christoffer,
> Reviewed-by: Eric Auger <eric.auger@linaro.org>
> see few comments below.
> On 12/09/2014 04:44 PM, Christoffer Dall wrote:
> > From: Peter Maydell <peter.maydell@linaro.org>
> > 
> > VGIC initialization currently happens in three phases:
> >  (1) kvm_vgic_create() (triggered by userspace GIC creation)
> >  (2) vgic_init_maps() (triggered by userspace GIC register read/write
> >      requests, or from kvm_vgic_init() if not already run)
> >  (3) kvm_vgic_init() (triggered by first VM run)
> > 
> > We were doing initialization of some state to correspond with the
> > state of a freshly-reset GIC in kvm_vgic_init(); this is too late,
> > since it will overwrite changes made by userspace using the
> > register access APIs before the VM is run. Move this initialization
> > earlier, into the vgic_init_maps() phase.
> > 
> > This fixes a bug where QEMU could successfully restore a saved
> > VM state snapshot into a VM that had already been run, but could
> > not restore it "from cold" using the -loadvm command line option
> > (the symptoms being that the restored VM would run but interrupts
> > were ignored).
> > 
> > Finally rename vgic_init_maps to vgic_init and renamed kvm_vgic_init to
> > kvm_vgic_map_resources.
> > 
> >   [ This patch is originally written by Peter Maydell, but I have
> >     modified it somewhat heavily, renaming various bits and moving code
> >     around.  If something is broken, I am to be blamed. - Christoffer ]
> > 
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> > This patch was originally named "vgic: move reset initialization into
> > vgic_init_maps()" but I renamed it slightly to match the other vgic
> > patches in the kernel.  I also did the additional changes since the
> > original patch:
> >  - Renamed kvm_vgic_init to kvm_vgic_map_resources
> >  - Renamed vgic_init_maps to vgic_init
> >  - Moved vgic_enable call into existing vcpu loop in vgic_init
> >  - Moved ITARGETSRn initializtion above vcpu loop in vgic_init (the idea
> typo
> >    is to init global state first, then vcpu state).
> 
> kvm_vgic_vcpu_init also has disappeared and PPI settings of
> dist->irq_enabled and dist->irq_cfg now are in former vgic_init_maps.
> 
> Maybe it would be simpler to review if there were 2 patches: one for
> init redistribution from kvm_vgic_init to vgic_init_maps and one for the
> renaming.

Maybe, but if you apply this patch and review it that way, it becomes
much easier.  I'd really like to get this in soon, so given you already
gave me your reviewed-by, I'm going to wait and see what Marc says and
only respin if he finds it necessary.

> 
> kvm_vgic_map_resources: difficult to understand it also inits the
> internal states. Wouldn't kvm_vgic_set_ready be aligned with ready
> terminology?
> 
So I thought about that, but really, the fact that we now call
vgic_init() from all sorts of places shouldn't require us to rename all
those functions, just because they call init.  So I ended up naming this
function for what it really does, and then just added a comment about
the on-demand approach.

I think that's cleanest.

Thanks,
-Christoffer