On Tue, Nov 04, 2014 at 03:58:27PM +0000, Andre Przywara wrote:
> Hi Christoffer,
>
> as hinted on IRC, an earlier reply on this one got lost on my machine,
> so please excuse my apparent ignorance on your previous comments.
>
> On 03/11/14 13:59, Christoffer Dall wrote:
> > On Fri, Oct 31, 2014 at 05:26:40PM +0000, 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.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> >> ---
> >> include/kvm/arm_vgic.h | 10 ++++++
> >> virt/kvm/arm/vgic.c | 81 +++++++++++++++++++++++++++++++++++-------------
> >> 2 files changed, 69 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> >> index dde5a00..bfb660a 100644
> >> --- a/include/kvm/arm_vgic.h
> >> +++ b/include/kvm/arm_vgic.h
> >> @@ -134,6 +134,14 @@ struct vgic_params {
> >> void __iomem *vctrl_base;
> >> };
> >>
> >> +struct vgic_vm_ops {
> >> + bool (*handle_mmio)(struct kvm_vcpu *, struct kvm_run *,
> >> + struct kvm_exit_mmio *);
> >> + bool (*queue_sgi)(struct kvm_vcpu *vcpu, int irq);
> >> + void (*add_sgi_source)(struct kvm_vcpu *vcpu, int irq, int source);
> >> + int (*vgic_init)(struct kvm *kvm, const struct vgic_params *params);
> >> +};
> >> +
> >> struct vgic_dist {
> >> #ifdef CONFIG_KVM_ARM_VGIC
> >> spinlock_t lock;
> >> @@ -215,6 +223,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.c b/virt/kvm/arm/vgic.c
> >> index 0cbdde9..2c16684 100644
> >> --- a/virt/kvm/arm/vgic.c
> >> +++ b/virt/kvm/arm/vgic.c
> >> @@ -105,6 +105,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)
> >> +
> >
> > another one? why did you simply ignore my comment from the last review?
> >
> > If it wasn't obvious last time around, YUCK, and no ;)
>
> OK, which version would you like?
>
> 1) Actually my first solution was to decode it on each call-site directly:
>
> vcpu->kvm->arch.vgic.vm_ops.add_sgi_source(vcpu, lr.irq, lr.source);
>
> However one reviewer suggested to wrap it with the macro you see above.
>
yeah, those lines did become very long.
> 2) Provide a static inline for each seems like overkill, since there is
> only one caller for each of them, but it would look like this:
>
> static void add_sgi_source(struct kvm_vcpu *vcpu, int irq, int source)
> {
> vcpu->kvm->arch.vgic.vm_ops.add_sgi_source(vcpu, irq, source);
> }
>
> Both don't look very convincing to me, so if you see other
> colors^Wsolutions, please let me know ;-)
I strongly prefer the static inline version, I don't think it's that
bad. You could also stick with a macro solution, but then you should do
something like:
#define add_sgu_source(vcpu, irq, source) \
vcpu->kvm->arch.vgic.vm_ops.add_sgi_source(vcpu, irq, source)
There's only a handful or so of these, right, so I really don't see a
big problem having a number of static inlines.
>
> We have to choose between them at _runtime_, because there could be two
> guests with different vGIC models running at the same time.
>
> >> /*
> >> * struct vgic_bitmap contains a bitmap made of unsigned longs, but
> >> * extracts u32s out of them.
> >> @@ -761,6 +763,13 @@ static bool handle_mmio_sgi_reg(struct kvm_vcpu *vcpu,
> >> return false;
> >> }
> >>
> >> +static void vgic_v2_add_sgi_source(struct kvm_vcpu *vcpu, int irq, int source)
> >> +{
> >> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> >> +
> >> + *vgic_get_sgi_sources(dist, vcpu->vcpu_id, irq) |= 1 << source;
> >> +}
> >> +
> >> /**
> >> * vgic_unqueue_irqs - move pending IRQs from LRs to the distributor
> >> * @vgic_cpu: Pointer to the vgic_cpu struct holding the LRs
> >> @@ -775,9 +784,7 @@ static bool handle_mmio_sgi_reg(struct kvm_vcpu *vcpu,
> >> */
> >> static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
> >> {
> >> - struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> >> 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) {
> >> @@ -804,7 +811,8 @@ static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
> >> */
> >> vgic_dist_irq_set_pending(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, add_sgi_source)(vcpu, lr.irq,
> >> + lr.source);
> >> lr.state &= ~LR_STATE_PENDING;
> >> vgic_set_lr(vcpu, i, lr);
> >>
> >> @@ -1162,7 +1170,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)
> >> @@ -1414,7 +1422,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;
> >> @@ -1489,7 +1497,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;
> >> }
> >>
> >> @@ -1944,9 +1952,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);
> >> -
> >
> > Remind me, why are we moving this chunk?
>
> The target registers are only valid for vGICv2 (we have other means for
> GICv3), so this belongs now into the vGICv2 specific code.
>
ah, right, obvious ;)
Thanks,
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
