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@xxxxxxx>
>> ---
>> 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
>
--
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
