All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andre Przywara <andre.przywara@arm.com>
To: Christoffer Dall <christoffer.dall@linaro.org>
Cc: "kvmarm@lists.cs.columbia.edu" <kvmarm@lists.cs.columbia.edu>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	Marc Zyngier <Marc.Zyngier@arm.com>
Subject: Re: [PATCH v2 05/15] arm/arm64: KVM: introduce per-VM ops
Date: Fri, 31 Oct 2014 13:59:05 +0000	[thread overview]
Message-ID: <545395A9.9040305@arm.com> (raw)
In-Reply-To: <20141015162715.GF14272@lvm>

Hi Christoffer,

On 15/10/14 17:27, Christoffer Dall wrote:
> On Thu, Aug 21, 2014 at 02:06:46PM +0100, Andre Przywara wrote:
>> Currently we only have one virtual GIC model supported, so all guests
>> use the same emulation code. With the addition of another model we
>> end up with different guests using potentially different vGIC models,
>> so we have to split up some functions to be per VM.
>> Introduce a vgic_vm_ops struct to hold function pointers for those
>> functions that are different and provide the necessary code to
>> initialize them.
>> This includes functions that depend on the emulated GIC model only
>> and functions that depend on the combination of host and guest GIC.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  include/kvm/arm_vgic.h |   18 ++++++++--
>>  virt/kvm/arm/vgic-v2.c |   17 +++++++--
>>  virt/kvm/arm/vgic-v3.c |   16 +++++++--
>>  virt/kvm/arm/vgic.c    |   92 +++++++++++++++++++++++++++++++++++-------------
>>  4 files changed, 113 insertions(+), 30 deletions(-)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index 4feac9a..7e7c99e 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -99,8 +99,6 @@ struct vgic_vmcr {
>>  };
>>
>>  struct vgic_ops {
>> -     struct vgic_lr  (*get_lr)(const struct kvm_vcpu *, int);
>> -     void    (*set_lr)(struct kvm_vcpu *, int, struct vgic_lr);
>>       void    (*sync_lr_elrsr)(struct kvm_vcpu *, int, struct vgic_lr);
>>       u64     (*get_elrsr)(const struct kvm_vcpu *vcpu);
>>       u64     (*get_eisr)(const struct kvm_vcpu *vcpu);
>> @@ -123,6 +121,17 @@ struct vgic_params {
>>       unsigned int    maint_irq;
>>       /* Virtual control interface base address */
>>       void __iomem    *vctrl_base;
>> +     bool (*init_emul)(struct kvm *kvm, int type);
>> +};
>> +
>> +struct vgic_vm_ops {
>> +     struct vgic_lr  (*get_lr)(const struct kvm_vcpu *, int);
>> +     void    (*set_lr)(struct kvm_vcpu *, int, struct vgic_lr);
>> +     bool    (*handle_mmio)(struct kvm_vcpu *, struct kvm_run *,
>> +                            struct kvm_exit_mmio *);
>> +     bool    (*queue_sgi)(struct kvm_vcpu *vcpu, int irq);
>> +     void    (*unqueue_sgi)(struct kvm_vcpu *vcpu, int irq, int source);
>> +     int     (*vgic_init)(struct kvm *kvm, const struct vgic_params *params);
>>  };
>>
>>  struct vgic_dist {
>> @@ -131,6 +140,9 @@ struct vgic_dist {
>>       bool                    in_kernel;
>>       bool                    ready;
>>
>> +     /* vGIC model the kernel emulates for the guest (GICv2 or GICv3) */
>> +     u32                     vgic_model;
>> +
>>       int                     nr_cpus;
>>       int                     nr_irqs;
>>
>> @@ -168,6 +180,8 @@ struct vgic_dist {
>>
>>       /* Bitmap indicating which CPU has something pending */
>>       unsigned long           irq_pending_on_cpu;
>> +
>> +     struct vgic_vm_ops      vm_ops;
>>  #endif
>>  };
>>
>> diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
>> index 01124ef..90947c6 100644
>> --- a/virt/kvm/arm/vgic-v2.c
>> +++ b/virt/kvm/arm/vgic-v2.c
>> @@ -161,8 +161,6 @@ static void vgic_v2_enable(struct kvm_vcpu *vcpu)
>>  }
>>
>>  static const struct vgic_ops vgic_v2_ops = {
>> -     .get_lr                 = vgic_v2_get_lr,
>> -     .set_lr                 = vgic_v2_set_lr,
>>       .sync_lr_elrsr          = vgic_v2_sync_lr_elrsr,
>>       .get_elrsr              = vgic_v2_get_elrsr,
>>       .get_eisr               = vgic_v2_get_eisr,
>> @@ -176,6 +174,20 @@ static const struct vgic_ops vgic_v2_ops = {
>>
>>  static struct vgic_params vgic_v2_params;
>>
>> +static bool vgic_v2_init_emul(struct kvm *kvm, int type)
>> +{
>> +     struct vgic_vm_ops *vm_ops = &kvm->arch.vgic.vm_ops;
>> +
>> +     switch (type) {
>> +     case KVM_DEV_TYPE_ARM_VGIC_V2:
>> +             vm_ops->get_lr = vgic_v2_get_lr;
>> +             vm_ops->set_lr = vgic_v2_set_lr;
>> +             return true;
>> +     }
>> +
>> +     return false;
>> +}
>> +
>>  /**
>>   * vgic_v2_probe - probe for a GICv2 compatible interrupt controller in DT
>>   * @node:    pointer to the DT node
>> @@ -214,6 +226,7 @@ int vgic_v2_probe(struct device_node *vgic_node,
>>               ret = -ENOMEM;
>>               goto out;
>>       }
>> +     vgic->init_emul = vgic_v2_init_emul;
>
> this feels overly complicated, can't each host-support function just
> export this function and we call it directly from the vgic creation
> code?
>
> That or just keep the host-specific functions the way they are and
> handle the split within these functions?
>
> Bah, I'm not sure, this patch is just terrible to review...  Perhaps
> it's because we're doing some combination of introducing infrastructure
> and also changing random bits and naming to be version-specific.

OK, I split up the patch to do one thing at a time. This is now 2 bigger
and 2 smaller patches in the new series, and it looks more
review-friendly, I hope.

>>
>>       vgic->nr_lr = readl_relaxed(vgic->vctrl_base + GICH_VTR);
>>       vgic->nr_lr = (vgic->nr_lr & 0x3f) + 1;
>> diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
>> index 1c2c8ee..a38339e 100644
>> --- a/virt/kvm/arm/vgic-v3.c
>> +++ b/virt/kvm/arm/vgic-v3.c
>> @@ -157,8 +157,6 @@ static void vgic_v3_enable(struct kvm_vcpu *vcpu)
>>  }
>>
>>  static const struct vgic_ops vgic_v3_ops = {
>> -     .get_lr                 = vgic_v3_get_lr,
>> -     .set_lr                 = vgic_v3_set_lr,
>>       .sync_lr_elrsr          = vgic_v3_sync_lr_elrsr,
>>       .get_elrsr              = vgic_v3_get_elrsr,
>>       .get_eisr               = vgic_v3_get_eisr,
>> @@ -170,6 +168,19 @@ static const struct vgic_ops vgic_v3_ops = {
>>       .enable                 = vgic_v3_enable,
>>  };
>>
>> +static bool vgic_v3_init_emul_compat(struct kvm *kvm, int type)
>
> why _compat?

Because if covers GICv3 implementations which support GICv2
compatibility (which is all we support at this point).

>> +{
>> +     struct vgic_vm_ops *vm_ops = &kvm->arch.vgic.vm_ops;
>> +
>> +     switch (type) {
>> +     case KVM_DEV_TYPE_ARM_VGIC_V2:
>> +             vm_ops->get_lr = vgic_v3_get_lr;
>> +             vm_ops->set_lr = vgic_v3_set_lr;
>> +             return true;
>> +     }
>> +     return false;
>> +}
>> +
>>  static struct vgic_params vgic_v3_params;
>>
>>  /**
>> @@ -231,6 +242,7 @@ int vgic_v3_probe(struct device_node *vgic_node,
>>               goto out;
>>       }
>>
>> +     vgic->init_emul = vgic_v3_init_emul_compat;
>>       vgic->vcpu_base = vcpu_res.start;
>>       vgic->vctrl_base = NULL;
>>       vgic->type = VGIC_V3;
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index bef9aa0..5e0bc24 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -99,6 +99,8 @@ static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
>>  static const struct vgic_ops *vgic_ops;
>>  static const struct vgic_params *vgic;
>>
>> +#define vgic_vm_op(kvm, fn) ((kvm)->arch.vgic.vm_ops.fn)
>
> yuck, we have only 5 of these, can't we create a define for each or
> create small wrapper functions instead?  That would make ctags behave
> nicely too.
>
>> +
>>  /*
>>   * struct vgic_bitmap contains a bitmap of unsigned longs, but
>>   * extracts u32s out of them.
>> @@ -668,11 +670,16 @@ static bool handle_mmio_sgi_reg(struct kvm_vcpu *vcpu,
>>   * to the distributor but the active state stays in the LRs, because we don't
>>   * track the active state on the distributor side.
>>   */
>> -static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
>> +
>
> extra whitespace and misplaced comment

Indeed, looks like a rebase artifact.

>> +static void vgic_v2_unqueue_sgi(struct kvm_vcpu *vcpu, int irq, int source)
>
> not sure about this name, it's not really unqueueing anything, it's just
> setting the source for an SGI you're setting on the distributor...

Right, I changed it to add_sgi_source, which is admittedly rather
technical, but still better than the current one.

>>  {
>>       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>> +
>> +     *vgic_get_sgi_sources(dist, vcpu->vcpu_id, irq) |= 1 << source;
>> +}
>
> missing whitespace
>
>> +static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
>> +{
>>       struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>> -     int vcpu_id = vcpu->vcpu_id;
>>       int i;
>>
>>       for_each_set_bit(i, vgic_cpu->lr_used, vgic_cpu->nr_lr) {
>> @@ -699,7 +706,8 @@ static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
>>                */
>>               vgic_dist_irq_set(vcpu, lr.irq);
>>               if (lr.irq < VGIC_NR_SGIS)
>> -                     *vgic_get_sgi_sources(dist, vcpu_id, lr.irq) |= 1 << lr.source;
>> +                     vgic_vm_op(vcpu->kvm, unqueue_sgi)(vcpu, lr.irq,
>> +                                                        lr.source);
>>               lr.state &= ~LR_STATE_PENDING;
>>               vgic_set_lr(vcpu, i, lr);
>>
>> @@ -1050,7 +1058,7 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>       if (!irqchip_in_kernel(vcpu->kvm))
>>               return false;
>>
>> -     return vgic_v2_handle_mmio(vcpu, run, mmio);
>> +     return vgic_vm_op(vcpu->kvm, handle_mmio)(vcpu, run, mmio);
>>  }
>>
>>  static u8 *vgic_get_sgi_sources(struct vgic_dist *dist, int vcpu_id, int sgi)
>> @@ -1158,13 +1166,13 @@ static void vgic_update_state(struct kvm *kvm)
>>
>>  static struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr)
>>  {
>> -     return vgic_ops->get_lr(vcpu, lr);
>> +     return vgic_vm_op(vcpu->kvm, get_lr)(vcpu, lr);
>>  }
>>
>>  static void vgic_set_lr(struct kvm_vcpu *vcpu, int lr,
>>                              struct vgic_lr vlr)
>>  {
>> -     vgic_ops->set_lr(vcpu, lr, vlr);
>> +     return vgic_vm_op(vcpu->kvm, set_lr)(vcpu, lr, vlr);
>>  }
>>
>>  static void vgic_sync_lr_elrsr(struct kvm_vcpu *vcpu, int lr,
>> @@ -1302,7 +1310,7 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
>>       return true;
>>  }
>>
>> -static bool vgic_queue_sgi(struct kvm_vcpu *vcpu, int irq)
>> +static bool vgic_v2_queue_sgi(struct kvm_vcpu *vcpu, int irq)
>>  {
>>       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>       unsigned long sources;
>> @@ -1377,7 +1385,7 @@ static void __kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
>>
>>       /* SGIs */
>>       for_each_set_bit(i, vgic_cpu->pending_percpu, VGIC_NR_SGIS) {
>> -             if (!vgic_queue_sgi(vcpu, i))
>> +             if (!vgic_vm_op(vcpu->kvm, queue_sgi)(vcpu, i))
>>                       overflow = 1;
>>       }
>>
>> @@ -1873,9 +1881,6 @@ static int vgic_init_maps(struct kvm *kvm)
>>               }
>>       }
>>
>> -     for (i = VGIC_NR_PRIVATE_IRQS; i < dist->nr_irqs; i += 4)
>> -             vgic_set_target_reg(kvm, 0, i);
>> -
>>  out:
>>       if (ret)
>>               kvm_vgic_destroy(kvm);
>> @@ -1883,6 +1888,31 @@ out:
>>       return ret;
>>  }
>>
>> +static int vgic_v2_init(struct kvm *kvm, const struct vgic_params *params)
>> +{
>> +     struct vgic_dist *dist = &kvm->arch.vgic;
>> +     int ret, i;
>> +
>> +     if (IS_VGIC_ADDR_UNDEF(dist->vgic_dist_base) ||
>> +         IS_VGIC_ADDR_UNDEF(dist->vgic_cpu_base)) {
>> +             kvm_err("Need to set vgic distributor addresses first\n");
>> +             return -ENXIO;
>> +     }
>> +
>> +     ret = kvm_phys_addr_ioremap(kvm, dist->vgic_cpu_base,
>> +                                 params->vcpu_base,
>> +                                 KVM_VGIC_V2_CPU_SIZE);
>> +     if (ret) {
>> +             kvm_err("Unable to remap VGIC CPU to VCPU\n");
>> +             return ret;
>> +     }
>> +
>> +     for (i = VGIC_NR_PRIVATE_IRQS; i < dist->nr_irqs; i += 4)
>> +             vgic_set_target_reg(kvm, 0, i);
>> +
>> +     return 0;
>> +}
>> +
>>  /**
>>   * kvm_vgic_init - Initialize global VGIC state before running any VCPUs
>>   * @kvm: pointer to the kvm struct
>> @@ -1905,25 +1935,15 @@ int kvm_vgic_init(struct kvm *kvm)
>>       if (vgic_initialized(kvm))
>>               goto out;
>>
>> -     if (IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_dist_base) ||
>> -         IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_cpu_base)) {
>> -             kvm_err("Need to set vgic cpu and dist addresses first\n");
>> -             ret = -ENXIO;
>> -             goto out;
>> -     }
>> -
>>       ret = vgic_init_maps(kvm);
>>       if (ret) {
>>               kvm_err("Unable to allocate maps\n");
>>               goto out;
>>       }
>>
>> -     ret = kvm_phys_addr_ioremap(kvm, kvm->arch.vgic.vgic_cpu_base,
>> -                                 vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE);
>> -     if (ret) {
>> -             kvm_err("Unable to remap VGIC CPU to VCPU\n");
>> +     ret = vgic_vm_op(kvm, vgic_init)(kvm, vgic);
>> +     if (ret)
>>               goto out;
>> -     }
>>
>>       kvm_for_each_vcpu(i, vcpu, kvm)
>>               kvm_vgic_vcpu_init(vcpu);
>> @@ -1936,6 +1956,21 @@ out:
>>       return ret;
>>  }
>>
>> +static bool init_emulation_ops(struct kvm *kvm, int type)
>> +{
>> +     struct vgic_dist *dist = &kvm->arch.vgic;
>> +
>> +     switch (type) {
>> +     case KVM_DEV_TYPE_ARM_VGIC_V2:
>> +             dist->vm_ops.handle_mmio = vgic_v2_handle_mmio;
>> +             dist->vm_ops.queue_sgi = vgic_v2_queue_sgi;
>> +             dist->vm_ops.unqueue_sgi = vgic_v2_unqueue_sgi;
>> +             dist->vm_ops.vgic_init = vgic_v2_init;
>> +             return true;
>> +     }
>> +     return false;
>> +}
>> +
>>  int kvm_vgic_create(struct kvm *kvm, u32 type)
>>  {
>>       int i, vcpu_lock_idx = -1, ret = 0;
>> @@ -1943,7 +1978,7 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
>>
>>       mutex_lock(&kvm->lock);
>>
>> -     if (kvm->arch.vgic.vctrl_base) {
>> +     if (kvm->arch.vgic.in_kernel) {
>
> unrelated change?

Kind of. I couldn't find a better patch to stuff this into, so it's now
a patch on it's own. Should be easy to review now ;-)

Thanks for the comments!
Andre.

>
>>               ret = -EEXIST;
>>               goto out;
>>       }
>> @@ -1966,12 +2001,21 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
>>               }
>>       }
>>
>> +     if (!vgic->init_emul(kvm, type)) {
>> +             ret = -ENODEV;
>> +             goto out_unlock;
>> +     }
>> +
>>       spin_lock_init(&kvm->arch.vgic.lock);
>>       kvm->arch.vgic.in_kernel = true;
>> +     kvm->arch.vgic.vgic_model = type;
>>       kvm->arch.vgic.vctrl_base = vgic->vctrl_base;
>>       kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
>>       kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
>>
>> +     if (!init_emulation_ops(kvm, type))
>> +             ret = -ENODEV;
>> +
>>  out_unlock:
>>       for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) {
>>               vcpu = kvm_get_vcpu(kvm, vcpu_lock_idx);
>> --
>> 1.7.9.5
>>
>
> Thanks,
> -Christoffer
>

WARNING: multiple messages have this Message-ID (diff)
From: andre.przywara@arm.com (Andre Przywara)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2 05/15] arm/arm64: KVM: introduce per-VM ops
Date: Fri, 31 Oct 2014 13:59:05 +0000	[thread overview]
Message-ID: <545395A9.9040305@arm.com> (raw)
In-Reply-To: <20141015162715.GF14272@lvm>

Hi Christoffer,

On 15/10/14 17:27, Christoffer Dall wrote:
> On Thu, Aug 21, 2014 at 02:06:46PM +0100, Andre Przywara wrote:
>> Currently we only have one virtual GIC model supported, so all guests
>> use the same emulation code. With the addition of another model we
>> end up with different guests using potentially different vGIC models,
>> so we have to split up some functions to be per VM.
>> Introduce a vgic_vm_ops struct to hold function pointers for those
>> functions that are different and provide the necessary code to
>> initialize them.
>> This includes functions that depend on the emulated GIC model only
>> and functions that depend on the combination of host and guest GIC.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  include/kvm/arm_vgic.h |   18 ++++++++--
>>  virt/kvm/arm/vgic-v2.c |   17 +++++++--
>>  virt/kvm/arm/vgic-v3.c |   16 +++++++--
>>  virt/kvm/arm/vgic.c    |   92 +++++++++++++++++++++++++++++++++++-------------
>>  4 files changed, 113 insertions(+), 30 deletions(-)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index 4feac9a..7e7c99e 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -99,8 +99,6 @@ struct vgic_vmcr {
>>  };
>>
>>  struct vgic_ops {
>> -     struct vgic_lr  (*get_lr)(const struct kvm_vcpu *, int);
>> -     void    (*set_lr)(struct kvm_vcpu *, int, struct vgic_lr);
>>       void    (*sync_lr_elrsr)(struct kvm_vcpu *, int, struct vgic_lr);
>>       u64     (*get_elrsr)(const struct kvm_vcpu *vcpu);
>>       u64     (*get_eisr)(const struct kvm_vcpu *vcpu);
>> @@ -123,6 +121,17 @@ struct vgic_params {
>>       unsigned int    maint_irq;
>>       /* Virtual control interface base address */
>>       void __iomem    *vctrl_base;
>> +     bool (*init_emul)(struct kvm *kvm, int type);
>> +};
>> +
>> +struct vgic_vm_ops {
>> +     struct vgic_lr  (*get_lr)(const struct kvm_vcpu *, int);
>> +     void    (*set_lr)(struct kvm_vcpu *, int, struct vgic_lr);
>> +     bool    (*handle_mmio)(struct kvm_vcpu *, struct kvm_run *,
>> +                            struct kvm_exit_mmio *);
>> +     bool    (*queue_sgi)(struct kvm_vcpu *vcpu, int irq);
>> +     void    (*unqueue_sgi)(struct kvm_vcpu *vcpu, int irq, int source);
>> +     int     (*vgic_init)(struct kvm *kvm, const struct vgic_params *params);
>>  };
>>
>>  struct vgic_dist {
>> @@ -131,6 +140,9 @@ struct vgic_dist {
>>       bool                    in_kernel;
>>       bool                    ready;
>>
>> +     /* vGIC model the kernel emulates for the guest (GICv2 or GICv3) */
>> +     u32                     vgic_model;
>> +
>>       int                     nr_cpus;
>>       int                     nr_irqs;
>>
>> @@ -168,6 +180,8 @@ struct vgic_dist {
>>
>>       /* Bitmap indicating which CPU has something pending */
>>       unsigned long           irq_pending_on_cpu;
>> +
>> +     struct vgic_vm_ops      vm_ops;
>>  #endif
>>  };
>>
>> diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
>> index 01124ef..90947c6 100644
>> --- a/virt/kvm/arm/vgic-v2.c
>> +++ b/virt/kvm/arm/vgic-v2.c
>> @@ -161,8 +161,6 @@ static void vgic_v2_enable(struct kvm_vcpu *vcpu)
>>  }
>>
>>  static const struct vgic_ops vgic_v2_ops = {
>> -     .get_lr                 = vgic_v2_get_lr,
>> -     .set_lr                 = vgic_v2_set_lr,
>>       .sync_lr_elrsr          = vgic_v2_sync_lr_elrsr,
>>       .get_elrsr              = vgic_v2_get_elrsr,
>>       .get_eisr               = vgic_v2_get_eisr,
>> @@ -176,6 +174,20 @@ static const struct vgic_ops vgic_v2_ops = {
>>
>>  static struct vgic_params vgic_v2_params;
>>
>> +static bool vgic_v2_init_emul(struct kvm *kvm, int type)
>> +{
>> +     struct vgic_vm_ops *vm_ops = &kvm->arch.vgic.vm_ops;
>> +
>> +     switch (type) {
>> +     case KVM_DEV_TYPE_ARM_VGIC_V2:
>> +             vm_ops->get_lr = vgic_v2_get_lr;
>> +             vm_ops->set_lr = vgic_v2_set_lr;
>> +             return true;
>> +     }
>> +
>> +     return false;
>> +}
>> +
>>  /**
>>   * vgic_v2_probe - probe for a GICv2 compatible interrupt controller in DT
>>   * @node:    pointer to the DT node
>> @@ -214,6 +226,7 @@ int vgic_v2_probe(struct device_node *vgic_node,
>>               ret = -ENOMEM;
>>               goto out;
>>       }
>> +     vgic->init_emul = vgic_v2_init_emul;
>
> this feels overly complicated, can't each host-support function just
> export this function and we call it directly from the vgic creation
> code?
>
> That or just keep the host-specific functions the way they are and
> handle the split within these functions?
>
> Bah, I'm not sure, this patch is just terrible to review...  Perhaps
> it's because we're doing some combination of introducing infrastructure
> and also changing random bits and naming to be version-specific.

OK, I split up the patch to do one thing at a time. This is now 2 bigger
and 2 smaller patches in the new series, and it looks more
review-friendly, I hope.

>>
>>       vgic->nr_lr = readl_relaxed(vgic->vctrl_base + GICH_VTR);
>>       vgic->nr_lr = (vgic->nr_lr & 0x3f) + 1;
>> diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
>> index 1c2c8ee..a38339e 100644
>> --- a/virt/kvm/arm/vgic-v3.c
>> +++ b/virt/kvm/arm/vgic-v3.c
>> @@ -157,8 +157,6 @@ static void vgic_v3_enable(struct kvm_vcpu *vcpu)
>>  }
>>
>>  static const struct vgic_ops vgic_v3_ops = {
>> -     .get_lr                 = vgic_v3_get_lr,
>> -     .set_lr                 = vgic_v3_set_lr,
>>       .sync_lr_elrsr          = vgic_v3_sync_lr_elrsr,
>>       .get_elrsr              = vgic_v3_get_elrsr,
>>       .get_eisr               = vgic_v3_get_eisr,
>> @@ -170,6 +168,19 @@ static const struct vgic_ops vgic_v3_ops = {
>>       .enable                 = vgic_v3_enable,
>>  };
>>
>> +static bool vgic_v3_init_emul_compat(struct kvm *kvm, int type)
>
> why _compat?

Because if covers GICv3 implementations which support GICv2
compatibility (which is all we support at this point).

>> +{
>> +     struct vgic_vm_ops *vm_ops = &kvm->arch.vgic.vm_ops;
>> +
>> +     switch (type) {
>> +     case KVM_DEV_TYPE_ARM_VGIC_V2:
>> +             vm_ops->get_lr = vgic_v3_get_lr;
>> +             vm_ops->set_lr = vgic_v3_set_lr;
>> +             return true;
>> +     }
>> +     return false;
>> +}
>> +
>>  static struct vgic_params vgic_v3_params;
>>
>>  /**
>> @@ -231,6 +242,7 @@ int vgic_v3_probe(struct device_node *vgic_node,
>>               goto out;
>>       }
>>
>> +     vgic->init_emul = vgic_v3_init_emul_compat;
>>       vgic->vcpu_base = vcpu_res.start;
>>       vgic->vctrl_base = NULL;
>>       vgic->type = VGIC_V3;
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index bef9aa0..5e0bc24 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -99,6 +99,8 @@ static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
>>  static const struct vgic_ops *vgic_ops;
>>  static const struct vgic_params *vgic;
>>
>> +#define vgic_vm_op(kvm, fn) ((kvm)->arch.vgic.vm_ops.fn)
>
> yuck, we have only 5 of these, can't we create a define for each or
> create small wrapper functions instead?  That would make ctags behave
> nicely too.
>
>> +
>>  /*
>>   * struct vgic_bitmap contains a bitmap of unsigned longs, but
>>   * extracts u32s out of them.
>> @@ -668,11 +670,16 @@ static bool handle_mmio_sgi_reg(struct kvm_vcpu *vcpu,
>>   * to the distributor but the active state stays in the LRs, because we don't
>>   * track the active state on the distributor side.
>>   */
>> -static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
>> +
>
> extra whitespace and misplaced comment

Indeed, looks like a rebase artifact.

>> +static void vgic_v2_unqueue_sgi(struct kvm_vcpu *vcpu, int irq, int source)
>
> not sure about this name, it's not really unqueueing anything, it's just
> setting the source for an SGI you're setting on the distributor...

Right, I changed it to add_sgi_source, which is admittedly rather
technical, but still better than the current one.

>>  {
>>       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>> +
>> +     *vgic_get_sgi_sources(dist, vcpu->vcpu_id, irq) |= 1 << source;
>> +}
>
> missing whitespace
>
>> +static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
>> +{
>>       struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>> -     int vcpu_id = vcpu->vcpu_id;
>>       int i;
>>
>>       for_each_set_bit(i, vgic_cpu->lr_used, vgic_cpu->nr_lr) {
>> @@ -699,7 +706,8 @@ static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
>>                */
>>               vgic_dist_irq_set(vcpu, lr.irq);
>>               if (lr.irq < VGIC_NR_SGIS)
>> -                     *vgic_get_sgi_sources(dist, vcpu_id, lr.irq) |= 1 << lr.source;
>> +                     vgic_vm_op(vcpu->kvm, unqueue_sgi)(vcpu, lr.irq,
>> +                                                        lr.source);
>>               lr.state &= ~LR_STATE_PENDING;
>>               vgic_set_lr(vcpu, i, lr);
>>
>> @@ -1050,7 +1058,7 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>       if (!irqchip_in_kernel(vcpu->kvm))
>>               return false;
>>
>> -     return vgic_v2_handle_mmio(vcpu, run, mmio);
>> +     return vgic_vm_op(vcpu->kvm, handle_mmio)(vcpu, run, mmio);
>>  }
>>
>>  static u8 *vgic_get_sgi_sources(struct vgic_dist *dist, int vcpu_id, int sgi)
>> @@ -1158,13 +1166,13 @@ static void vgic_update_state(struct kvm *kvm)
>>
>>  static struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr)
>>  {
>> -     return vgic_ops->get_lr(vcpu, lr);
>> +     return vgic_vm_op(vcpu->kvm, get_lr)(vcpu, lr);
>>  }
>>
>>  static void vgic_set_lr(struct kvm_vcpu *vcpu, int lr,
>>                              struct vgic_lr vlr)
>>  {
>> -     vgic_ops->set_lr(vcpu, lr, vlr);
>> +     return vgic_vm_op(vcpu->kvm, set_lr)(vcpu, lr, vlr);
>>  }
>>
>>  static void vgic_sync_lr_elrsr(struct kvm_vcpu *vcpu, int lr,
>> @@ -1302,7 +1310,7 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
>>       return true;
>>  }
>>
>> -static bool vgic_queue_sgi(struct kvm_vcpu *vcpu, int irq)
>> +static bool vgic_v2_queue_sgi(struct kvm_vcpu *vcpu, int irq)
>>  {
>>       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>       unsigned long sources;
>> @@ -1377,7 +1385,7 @@ static void __kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
>>
>>       /* SGIs */
>>       for_each_set_bit(i, vgic_cpu->pending_percpu, VGIC_NR_SGIS) {
>> -             if (!vgic_queue_sgi(vcpu, i))
>> +             if (!vgic_vm_op(vcpu->kvm, queue_sgi)(vcpu, i))
>>                       overflow = 1;
>>       }
>>
>> @@ -1873,9 +1881,6 @@ static int vgic_init_maps(struct kvm *kvm)
>>               }
>>       }
>>
>> -     for (i = VGIC_NR_PRIVATE_IRQS; i < dist->nr_irqs; i += 4)
>> -             vgic_set_target_reg(kvm, 0, i);
>> -
>>  out:
>>       if (ret)
>>               kvm_vgic_destroy(kvm);
>> @@ -1883,6 +1888,31 @@ out:
>>       return ret;
>>  }
>>
>> +static int vgic_v2_init(struct kvm *kvm, const struct vgic_params *params)
>> +{
>> +     struct vgic_dist *dist = &kvm->arch.vgic;
>> +     int ret, i;
>> +
>> +     if (IS_VGIC_ADDR_UNDEF(dist->vgic_dist_base) ||
>> +         IS_VGIC_ADDR_UNDEF(dist->vgic_cpu_base)) {
>> +             kvm_err("Need to set vgic distributor addresses first\n");
>> +             return -ENXIO;
>> +     }
>> +
>> +     ret = kvm_phys_addr_ioremap(kvm, dist->vgic_cpu_base,
>> +                                 params->vcpu_base,
>> +                                 KVM_VGIC_V2_CPU_SIZE);
>> +     if (ret) {
>> +             kvm_err("Unable to remap VGIC CPU to VCPU\n");
>> +             return ret;
>> +     }
>> +
>> +     for (i = VGIC_NR_PRIVATE_IRQS; i < dist->nr_irqs; i += 4)
>> +             vgic_set_target_reg(kvm, 0, i);
>> +
>> +     return 0;
>> +}
>> +
>>  /**
>>   * kvm_vgic_init - Initialize global VGIC state before running any VCPUs
>>   * @kvm: pointer to the kvm struct
>> @@ -1905,25 +1935,15 @@ int kvm_vgic_init(struct kvm *kvm)
>>       if (vgic_initialized(kvm))
>>               goto out;
>>
>> -     if (IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_dist_base) ||
>> -         IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_cpu_base)) {
>> -             kvm_err("Need to set vgic cpu and dist addresses first\n");
>> -             ret = -ENXIO;
>> -             goto out;
>> -     }
>> -
>>       ret = vgic_init_maps(kvm);
>>       if (ret) {
>>               kvm_err("Unable to allocate maps\n");
>>               goto out;
>>       }
>>
>> -     ret = kvm_phys_addr_ioremap(kvm, kvm->arch.vgic.vgic_cpu_base,
>> -                                 vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE);
>> -     if (ret) {
>> -             kvm_err("Unable to remap VGIC CPU to VCPU\n");
>> +     ret = vgic_vm_op(kvm, vgic_init)(kvm, vgic);
>> +     if (ret)
>>               goto out;
>> -     }
>>
>>       kvm_for_each_vcpu(i, vcpu, kvm)
>>               kvm_vgic_vcpu_init(vcpu);
>> @@ -1936,6 +1956,21 @@ out:
>>       return ret;
>>  }
>>
>> +static bool init_emulation_ops(struct kvm *kvm, int type)
>> +{
>> +     struct vgic_dist *dist = &kvm->arch.vgic;
>> +
>> +     switch (type) {
>> +     case KVM_DEV_TYPE_ARM_VGIC_V2:
>> +             dist->vm_ops.handle_mmio = vgic_v2_handle_mmio;
>> +             dist->vm_ops.queue_sgi = vgic_v2_queue_sgi;
>> +             dist->vm_ops.unqueue_sgi = vgic_v2_unqueue_sgi;
>> +             dist->vm_ops.vgic_init = vgic_v2_init;
>> +             return true;
>> +     }
>> +     return false;
>> +}
>> +
>>  int kvm_vgic_create(struct kvm *kvm, u32 type)
>>  {
>>       int i, vcpu_lock_idx = -1, ret = 0;
>> @@ -1943,7 +1978,7 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
>>
>>       mutex_lock(&kvm->lock);
>>
>> -     if (kvm->arch.vgic.vctrl_base) {
>> +     if (kvm->arch.vgic.in_kernel) {
>
> unrelated change?

Kind of. I couldn't find a better patch to stuff this into, so it's now
a patch on it's own. Should be easy to review now ;-)

Thanks for the comments!
Andre.

>
>>               ret = -EEXIST;
>>               goto out;
>>       }
>> @@ -1966,12 +2001,21 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
>>               }
>>       }
>>
>> +     if (!vgic->init_emul(kvm, type)) {
>> +             ret = -ENODEV;
>> +             goto out_unlock;
>> +     }
>> +
>>       spin_lock_init(&kvm->arch.vgic.lock);
>>       kvm->arch.vgic.in_kernel = true;
>> +     kvm->arch.vgic.vgic_model = type;
>>       kvm->arch.vgic.vctrl_base = vgic->vctrl_base;
>>       kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
>>       kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
>>
>> +     if (!init_emulation_ops(kvm, type))
>> +             ret = -ENODEV;
>> +
>>  out_unlock:
>>       for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) {
>>               vcpu = kvm_get_vcpu(kvm, vcpu_lock_idx);
>> --
>> 1.7.9.5
>>
>
> Thanks,
> -Christoffer
>

  reply	other threads:[~2014-10-31 13:59 UTC|newest]

Thread overview: 69+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-08-21 13:06 [PATCH v2 00/15] KVM GICv3 emulation Andre Przywara
2014-08-21 13:06 ` Andre Przywara
2014-08-21 13:06 ` [PATCH v2 01/15] arm/arm64: KVM: rework MPIDR assignment and add accessors Andre Przywara
2014-08-21 13:06   ` Andre Przywara
2014-10-15 16:25   ` Christoffer Dall
2014-10-15 16:25     ` Christoffer Dall
2014-10-31 14:06     ` Andre Przywara
2014-10-31 14:06       ` Andre Przywara
2014-08-21 13:06 ` [PATCH v2 02/15] arm/arm64: KVM: pass down user space provided GIC type into vGIC code Andre Przywara
2014-08-21 13:06   ` Andre Przywara
2014-10-15 16:25   ` Christoffer Dall
2014-10-15 16:25     ` Christoffer Dall
2014-08-21 13:06 ` [PATCH v2 03/15] arm/arm64: KVM: refactor vgic_handle_mmio() function Andre Przywara
2014-08-21 13:06   ` Andre Przywara
2014-09-04  7:52   ` wanghaibin
2014-10-15 16:25   ` Christoffer Dall
2014-10-15 16:25     ` Christoffer Dall
2014-10-31 13:42     ` Andre Przywara
2014-10-31 13:42       ` Andre Przywara
2014-08-21 13:06 ` [PATCH v2 04/15] arm/arm64: KVM: wrap 64 bit MMIO accesses with two 32 bit ones Andre Przywara
2014-08-21 13:06   ` Andre Przywara
2014-10-15 16:26   ` Christoffer Dall
2014-10-15 16:26     ` Christoffer Dall
2014-10-31 13:49     ` Andre Przywara
2014-10-31 13:49       ` Andre Przywara
2014-11-03  9:54       ` Christoffer Dall
2014-11-03  9:54         ` Christoffer Dall
2014-08-21 13:06 ` [PATCH v2 05/15] arm/arm64: KVM: introduce per-VM ops Andre Przywara
2014-08-21 13:06   ` Andre Przywara
2014-10-15 16:27   ` Christoffer Dall
2014-10-15 16:27     ` Christoffer Dall
2014-10-31 13:59     ` Andre Przywara [this message]
2014-10-31 13:59       ` Andre Przywara
2014-08-21 13:06 ` [PATCH v2 06/15] arm/arm64: KVM: make the maximum number of vCPUs a per-VM value Andre Przywara
2014-08-21 13:06   ` Andre Przywara
2014-10-15 16:27   ` Christoffer Dall
2014-10-15 16:27     ` Christoffer Dall
2014-10-31 14:10     ` Andre Przywara
2014-10-31 14:10       ` Andre Przywara
2014-08-21 13:06 ` [PATCH v2 07/15] arm/arm64: KVM: make the value of ICC_SRE_EL1 a per-VM variable Andre Przywara
2014-08-21 13:06   ` Andre Przywara
2014-10-15 16:27   ` Christoffer Dall
2014-10-15 16:27     ` Christoffer Dall
2014-08-21 13:06 ` [PATCH v2 08/15] arm/arm64: KVM: refactor MMIO accessors Andre Przywara
2014-08-21 13:06   ` Andre Przywara
2014-08-21 13:06 ` [PATCH v2 09/15] arm/arm64: KVM: refactor/wrap vgic_set/get_attr() Andre Przywara
2014-08-21 13:06   ` Andre Przywara
2014-08-21 13:06 ` [PATCH v2 10/15] arm/arm64: KVM: split GICv2 specific emulation code from vgic.c Andre Przywara
2014-08-21 13:06   ` Andre Przywara
2014-08-21 13:06 ` [PATCH v2 11/15] arm/arm64: KVM: add opaque private pointer to MMIO accessors Andre Przywara
2014-08-21 13:06   ` Andre Przywara
2014-08-21 13:06 ` [PATCH v2 12/15] arm/arm64: KVM: add virtual GICv3 distributor emulation Andre Przywara
2014-08-21 13:06   ` Andre Przywara
2014-09-05  3:28   ` wanghaibin
2014-09-05  3:28     ` wanghaibin
2014-09-05  8:13     ` Andre Przywara
2014-09-05  8:13       ` Andre Przywara
2014-08-21 13:06 ` [PATCH v2 13/15] arm/arm64: KVM: add SGI system register trapping Andre Przywara
2014-08-21 13:06   ` Andre Przywara
2014-08-21 13:06 ` [PATCH v2 14/15] arm/arm64: KVM: enable kernel side of GICv3 emulation Andre Przywara
2014-08-21 13:06   ` Andre Przywara
2014-08-21 13:06 ` [PATCH v2 15/15] arm/arm64: KVM: allow userland to request a virtual GICv3 Andre Przywara
2014-08-21 13:06   ` Andre Przywara
2014-09-20  1:15   ` wanghaibin
2014-09-20  1:15     ` wanghaibin
2014-10-08  4:08 ` [PATCH v2 00/15] KVM GICv3 emulation wanghaibin
2014-10-08  4:08   ` wanghaibin
2014-10-08  8:41   ` Andre Przywara
2014-10-08  8:41     ` Andre Przywara

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=545395A9.9040305@arm.com \
    --to=andre.przywara@arm.com \
    --cc=Marc.Zyngier@arm.com \
    --cc=christoffer.dall@linaro.org \
    --cc=kvm@vger.kernel.org \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.