On 11/12/14 11:48, Christoffer Dall wrote: > On Wed, Dec 10, 2014 at 11:11:41AM +0100, Eric Auger wrote: >> Hi Christoffer, >> Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx> >> see few comments below. >> On 12/09/2014 04:44 PM, Christoffer Dall wrote: >>> From: Peter Maydell <peter.maydell@xxxxxxxxxx> >>> >>> 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@xxxxxxxxxx> >>> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx> >>> --- >>> 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. No, I think this is fine as it is. This is a good cleanup, and it seems to simplify the whole thing (and yes, we could do with simplicity in this file...). Acked-by: Marc Zyngier <marc.zyngier@xxxxxxx> M. -- Jazz is not dead. It just smells funny... -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
