| Message ID | 1418139844-27892-2-git-send-email-christoffer.dall@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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. kvm_vgic_map_resources: difficult to understand it also inits the internal states. Wouldn't kvm_vgic_set_ready be aligned with ready terminology? Best Regards Eric > - Added comment in kvm_vgic_map_resources > > arch/arm/kvm/arm.c | 6 ++-- > include/kvm/arm_vgic.h | 4 +-- > virt/kvm/arm/vgic.c | 77 +++++++++++++++++++++----------------------------- > 3 files changed, 37 insertions(+), 50 deletions(-) > > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > index 9e193c8..a56cbb5 100644 > --- a/arch/arm/kvm/arm.c > +++ b/arch/arm/kvm/arm.c > @@ -427,11 +427,11 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) > vcpu->arch.has_run_once = true; > > /* > - * Initialize the VGIC before running a vcpu the first time on > - * this VM. > + * Map the VGIC hardware resources before running a vcpu the first > + * time on this VM. > */ > if (unlikely(!vgic_initialized(vcpu->kvm))) { > - ret = kvm_vgic_init(vcpu->kvm); > + ret = kvm_vgic_map_resources(vcpu->kvm); > if (ret) > return ret; > } > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h > index 206dcc3..fe9783b 100644 > --- a/include/kvm/arm_vgic.h > +++ b/include/kvm/arm_vgic.h > @@ -274,7 +274,7 @@ struct kvm_exit_mmio; > #ifdef CONFIG_KVM_ARM_VGIC > int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write); > int kvm_vgic_hyp_init(void); > -int kvm_vgic_init(struct kvm *kvm); > +int kvm_vgic_map_resources(struct kvm *kvm); > int kvm_vgic_create(struct kvm *kvm); > void kvm_vgic_destroy(struct kvm *kvm); > void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu); > @@ -321,7 +321,7 @@ static inline int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, > return -ENXIO; > } > > -static inline int kvm_vgic_init(struct kvm *kvm) > +static inline int kvm_vgic_map_resources(struct kvm *kvm) > { > return 0; > } > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c > index aacdb59..91e6bfc 100644 > --- a/virt/kvm/arm/vgic.c > +++ b/virt/kvm/arm/vgic.c > @@ -91,6 +91,7 @@ > #define ACCESS_WRITE_VALUE (3 << 1) > #define ACCESS_WRITE_MASK(x) ((x) & (3 << 1)) > > +static int vgic_init(struct kvm *kvm); > static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu); > static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu); > static void vgic_update_state(struct kvm *kvm); > @@ -1726,39 +1727,14 @@ static int vgic_vcpu_init_maps(struct kvm_vcpu *vcpu, int nr_irqs) > > int sz = (nr_irqs - VGIC_NR_PRIVATE_IRQS) / 8; > vgic_cpu->pending_shared = kzalloc(sz, GFP_KERNEL); > - vgic_cpu->vgic_irq_lr_map = kzalloc(nr_irqs, GFP_KERNEL); > + vgic_cpu->vgic_irq_lr_map = kmalloc(nr_irqs, GFP_KERNEL); > > if (!vgic_cpu->pending_shared || !vgic_cpu->vgic_irq_lr_map) { > kvm_vgic_vcpu_destroy(vcpu); > return -ENOMEM; > } > > - return 0; > -} > - > -/** > - * kvm_vgic_vcpu_init - Initialize per-vcpu VGIC state > - * @vcpu: pointer to the vcpu struct > - * > - * Initialize the vgic_cpu struct and vgic_dist struct fields pertaining to > - * this vcpu and enable the VGIC for this VCPU > - */ > -static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu) > -{ > - struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; > - struct vgic_dist *dist = &vcpu->kvm->arch.vgic; > - int i; > - > - for (i = 0; i < dist->nr_irqs; i++) { > - if (i < VGIC_NR_PPIS) > - vgic_bitmap_set_irq_val(&dist->irq_enabled, > - vcpu->vcpu_id, i, 1); > - if (i < VGIC_NR_PRIVATE_IRQS) > - vgic_bitmap_set_irq_val(&dist->irq_cfg, > - vcpu->vcpu_id, i, VGIC_CFG_EDGE); > - > - vgic_cpu->vgic_irq_lr_map[i] = LR_EMPTY; > - } > + memset(vgic_cpu->vgic_irq_lr_map, LR_EMPTY, nr_irqs); > > /* > * Store the number of LRs per vcpu, so we don't have to go > @@ -1767,7 +1743,7 @@ static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu) > */ > vgic_cpu->nr_lr = vgic->nr_lr; > > - vgic_enable(vcpu); > + return 0; > } > > void kvm_vgic_destroy(struct kvm *kvm) > @@ -1804,12 +1780,12 @@ void kvm_vgic_destroy(struct kvm *kvm) > * Allocate and initialize the various data structures. Must be called > * with kvm->lock held! > */ > -static int vgic_init_maps(struct kvm *kvm) > +static int vgic_init(struct kvm *kvm) > { > struct vgic_dist *dist = &kvm->arch.vgic; > struct kvm_vcpu *vcpu; > int nr_cpus, nr_irqs; > - int ret, i; > + int ret, i, vcpu_id; > > if (dist->nr_cpus) /* Already allocated */ > return 0; > @@ -1859,16 +1835,28 @@ static int vgic_init_maps(struct kvm *kvm) > if (ret) > goto out; > > - kvm_for_each_vcpu(i, vcpu, kvm) { > + for (i = VGIC_NR_PRIVATE_IRQS; i < dist->nr_irqs; i += 4) > + vgic_set_target_reg(kvm, 0, i); > + > + kvm_for_each_vcpu(vcpu_id, vcpu, kvm) { > ret = vgic_vcpu_init_maps(vcpu, nr_irqs); > if (ret) { > kvm_err("VGIC: Failed to allocate vcpu memory\n"); > break; > } > - } > > - for (i = VGIC_NR_PRIVATE_IRQS; i < dist->nr_irqs; i += 4) > - vgic_set_target_reg(kvm, 0, i); > + for (i = 0; i < dist->nr_irqs; i++) { > + if (i < VGIC_NR_PPIS) > + vgic_bitmap_set_irq_val(&dist->irq_enabled, > + vcpu->vcpu_id, i, 1); > + if (i < VGIC_NR_PRIVATE_IRQS) > + vgic_bitmap_set_irq_val(&dist->irq_cfg, > + vcpu->vcpu_id, i, > + VGIC_CFG_EDGE); > + } > + > + vgic_enable(vcpu); > + } > > out: > if (ret) > @@ -1878,18 +1866,16 @@ out: > } > > /** > - * kvm_vgic_init - Initialize global VGIC state before running any VCPUs > + * kvm_vgic_map_resources - Configure global VGIC state before running any VCPUs > * @kvm: pointer to the kvm struct > * > * Map the virtual CPU interface into the VM before running any VCPUs. We > * can't do this at creation time, because user space must first set the > - * virtual CPU interface address in the guest physical address space. Also > - * initialize the ITARGETSRn regs to 0 on the emulated distributor. > + * virtual CPU interface address in the guest physical address space. > */ > -int kvm_vgic_init(struct kvm *kvm) > +int kvm_vgic_map_resources(struct kvm *kvm) > { > - struct kvm_vcpu *vcpu; > - int ret = 0, i; > + int ret = 0; > > if (!irqchip_in_kernel(kvm)) > return 0; > @@ -1906,7 +1892,11 @@ int kvm_vgic_init(struct kvm *kvm) > goto out; > } > > - ret = vgic_init_maps(kvm); > + /* > + * Initialize the vgic if this hasn't already been done on demand by > + * accessing the vgic state from userspace. > + */ > + ret = vgic_init(kvm); > if (ret) { > kvm_err("Unable to allocate maps\n"); > goto out; > @@ -1920,9 +1910,6 @@ int kvm_vgic_init(struct kvm *kvm) > goto out; > } > > - kvm_for_each_vcpu(i, vcpu, kvm) > - kvm_vgic_vcpu_init(vcpu); > - > kvm->arch.vgic.ready = true; > out: > if (ret) > @@ -2167,7 +2154,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev, > > mutex_lock(&dev->kvm->lock); > > - ret = vgic_init_maps(dev->kvm); > + ret = vgic_init(dev->kvm); > if (ret) > goto out; > >
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
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index 9e193c8..a56cbb5 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -427,11 +427,11 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) vcpu->arch.has_run_once = true; /* - * Initialize the VGIC before running a vcpu the first time on - * this VM. + * Map the VGIC hardware resources before running a vcpu the first + * time on this VM. */ if (unlikely(!vgic_initialized(vcpu->kvm))) { - ret = kvm_vgic_init(vcpu->kvm); + ret = kvm_vgic_map_resources(vcpu->kvm); if (ret) return ret; } diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h index 206dcc3..fe9783b 100644 --- a/include/kvm/arm_vgic.h +++ b/include/kvm/arm_vgic.h @@ -274,7 +274,7 @@ struct kvm_exit_mmio; #ifdef CONFIG_KVM_ARM_VGIC int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write); int kvm_vgic_hyp_init(void); -int kvm_vgic_init(struct kvm *kvm); +int kvm_vgic_map_resources(struct kvm *kvm); int kvm_vgic_create(struct kvm *kvm); void kvm_vgic_destroy(struct kvm *kvm); void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu); @@ -321,7 +321,7 @@ static inline int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, return -ENXIO; } -static inline int kvm_vgic_init(struct kvm *kvm) +static inline int kvm_vgic_map_resources(struct kvm *kvm) { return 0; } diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index aacdb59..91e6bfc 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -91,6 +91,7 @@ #define ACCESS_WRITE_VALUE (3 << 1) #define ACCESS_WRITE_MASK(x) ((x) & (3 << 1)) +static int vgic_init(struct kvm *kvm); static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu); static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu); static void vgic_update_state(struct kvm *kvm); @@ -1726,39 +1727,14 @@ static int vgic_vcpu_init_maps(struct kvm_vcpu *vcpu, int nr_irqs) int sz = (nr_irqs - VGIC_NR_PRIVATE_IRQS) / 8; vgic_cpu->pending_shared = kzalloc(sz, GFP_KERNEL); - vgic_cpu->vgic_irq_lr_map = kzalloc(nr_irqs, GFP_KERNEL); + vgic_cpu->vgic_irq_lr_map = kmalloc(nr_irqs, GFP_KERNEL); if (!vgic_cpu->pending_shared || !vgic_cpu->vgic_irq_lr_map) { kvm_vgic_vcpu_destroy(vcpu); return -ENOMEM; } - return 0; -} - -/** - * kvm_vgic_vcpu_init - Initialize per-vcpu VGIC state - * @vcpu: pointer to the vcpu struct - * - * Initialize the vgic_cpu struct and vgic_dist struct fields pertaining to - * this vcpu and enable the VGIC for this VCPU - */ -static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu) -{ - struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; - struct vgic_dist *dist = &vcpu->kvm->arch.vgic; - int i; - - for (i = 0; i < dist->nr_irqs; i++) { - if (i < VGIC_NR_PPIS) - vgic_bitmap_set_irq_val(&dist->irq_enabled, - vcpu->vcpu_id, i, 1); - if (i < VGIC_NR_PRIVATE_IRQS) - vgic_bitmap_set_irq_val(&dist->irq_cfg, - vcpu->vcpu_id, i, VGIC_CFG_EDGE); - - vgic_cpu->vgic_irq_lr_map[i] = LR_EMPTY; - } + memset(vgic_cpu->vgic_irq_lr_map, LR_EMPTY, nr_irqs); /* * Store the number of LRs per vcpu, so we don't have to go @@ -1767,7 +1743,7 @@ static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu) */ vgic_cpu->nr_lr = vgic->nr_lr; - vgic_enable(vcpu); + return 0; } void kvm_vgic_destroy(struct kvm *kvm) @@ -1804,12 +1780,12 @@ void kvm_vgic_destroy(struct kvm *kvm) * Allocate and initialize the various data structures. Must be called * with kvm->lock held! */ -static int vgic_init_maps(struct kvm *kvm) +static int vgic_init(struct kvm *kvm) { struct vgic_dist *dist = &kvm->arch.vgic; struct kvm_vcpu *vcpu; int nr_cpus, nr_irqs; - int ret, i; + int ret, i, vcpu_id; if (dist->nr_cpus) /* Already allocated */ return 0; @@ -1859,16 +1835,28 @@ static int vgic_init_maps(struct kvm *kvm) if (ret) goto out; - kvm_for_each_vcpu(i, vcpu, kvm) { + for (i = VGIC_NR_PRIVATE_IRQS; i < dist->nr_irqs; i += 4) + vgic_set_target_reg(kvm, 0, i); + + kvm_for_each_vcpu(vcpu_id, vcpu, kvm) { ret = vgic_vcpu_init_maps(vcpu, nr_irqs); if (ret) { kvm_err("VGIC: Failed to allocate vcpu memory\n"); break; } - } - for (i = VGIC_NR_PRIVATE_IRQS; i < dist->nr_irqs; i += 4) - vgic_set_target_reg(kvm, 0, i); + for (i = 0; i < dist->nr_irqs; i++) { + if (i < VGIC_NR_PPIS) + vgic_bitmap_set_irq_val(&dist->irq_enabled, + vcpu->vcpu_id, i, 1); + if (i < VGIC_NR_PRIVATE_IRQS) + vgic_bitmap_set_irq_val(&dist->irq_cfg, + vcpu->vcpu_id, i, + VGIC_CFG_EDGE); + } + + vgic_enable(vcpu); + } out: if (ret) @@ -1878,18 +1866,16 @@ out: } /** - * kvm_vgic_init - Initialize global VGIC state before running any VCPUs + * kvm_vgic_map_resources - Configure global VGIC state before running any VCPUs * @kvm: pointer to the kvm struct * * Map the virtual CPU interface into the VM before running any VCPUs. We * can't do this at creation time, because user space must first set the - * virtual CPU interface address in the guest physical address space. Also - * initialize the ITARGETSRn regs to 0 on the emulated distributor. + * virtual CPU interface address in the guest physical address space. */ -int kvm_vgic_init(struct kvm *kvm) +int kvm_vgic_map_resources(struct kvm *kvm) { - struct kvm_vcpu *vcpu; - int ret = 0, i; + int ret = 0; if (!irqchip_in_kernel(kvm)) return 0; @@ -1906,7 +1892,11 @@ int kvm_vgic_init(struct kvm *kvm) goto out; } - ret = vgic_init_maps(kvm); + /* + * Initialize the vgic if this hasn't already been done on demand by + * accessing the vgic state from userspace. + */ + ret = vgic_init(kvm); if (ret) { kvm_err("Unable to allocate maps\n"); goto out; @@ -1920,9 +1910,6 @@ int kvm_vgic_init(struct kvm *kvm) goto out; } - kvm_for_each_vcpu(i, vcpu, kvm) - kvm_vgic_vcpu_init(vcpu); - kvm->arch.vgic.ready = true; out: if (ret) @@ -2167,7 +2154,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev, mutex_lock(&dev->kvm->lock); - ret = vgic_init_maps(dev->kvm); + ret = vgic_init(dev->kvm); if (ret) goto out;