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.
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;
>
>
--
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
