[RFC PATCH 13/45] KVM: arm/arm64: vgic-new: Export register access interface

[Christoffer Dall]

On Mon, Apr 11, 2016 at 12:09:49PM +0100, Andre Przywara wrote:
> Hej,
> 
> On 31/03/16 10:24, Christoffer Dall wrote:
> > On Fri, Mar 25, 2016 at 02:04:36AM +0000, Andre Przywara wrote:
> >> Userland can access the emulated GIC to save and restore its state
> >> for initialization or migration purposes.
> >> The kvm_io_bus API requires an absolute gpa, which does not fit the
> >> KVM_DEV_ARM_VGIC_GRP_DIST_REGS user API, that only provides relative
> >> offsets. So we explicitly iterate our register list to connect
> >> userland to the VGIC.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> >> Reviewed-by: Eric Auger <eric.auger at linaro.org>
> >> ---
> >>  virt/kvm/arm/vgic/vgic.h      |  2 ++
> >>  virt/kvm/arm/vgic/vgic_mmio.c | 45 +++++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 47 insertions(+)
> >>
> >> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> >> index 53730ba..a462e2b 100644
> >> --- a/virt/kvm/arm/vgic/vgic.h
> >> +++ b/virt/kvm/arm/vgic/vgic.h
> >> @@ -25,6 +25,8 @@ void vgic_v2_process_maintenance(struct kvm_vcpu *vcpu);
> >>  void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu);
> >>  void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr);
> >>  void vgic_v2_set_underflow(struct kvm_vcpu *vcpu);
> >> +int vgic_v2_dist_access(struct kvm_vcpu *vcpu, bool is_write,
> >> +			int offset, int len, void *val);
> >>  
> >>  #ifdef CONFIG_KVM_ARM_VGIC_V3
> >>  void vgic_v3_irq_change_affinity(struct kvm *kvm, u32 intid, u64 mpidr);
> >> diff --git a/virt/kvm/arm/vgic/vgic_mmio.c b/virt/kvm/arm/vgic/vgic_mmio.c
> >> index 26c46e7..e1fd17f 100644
> >> --- a/virt/kvm/arm/vgic/vgic_mmio.c
> >> +++ b/virt/kvm/arm/vgic/vgic_mmio.c
> >> @@ -113,6 +113,51 @@ struct vgic_register_region vgic_v2_dist_registers[] = {
> >>  		vgic_mmio_read_nyi, vgic_mmio_write_nyi, 16),
> >>  };
> >>  
> >> +/*
> >> + * Using kvm_io_bus_* to access GIC registers directly from userspace does
> >> + * not work, since we would need the absolute IPA address of the register
> >> + * in question, but the userland interface only provides relative offsets.
> >> + * So we provide our own dispatcher function for that purpose here.
> >> + */
> >> +static int vgic_mmio_access(struct kvm_vcpu *vcpu,
> >> +			    struct vgic_register_region *region, int nr_regions,
> >> +			    bool is_write, int offset, int len, void *val)
> >> +{
> > 
> > I suspect this is going to get rewored based on previous discussions
> > with the IO api...
> > 
> >> +	int i;
> >> +	struct vgic_io_device dev;
> >> +
> >> +	for (i = 0; i < nr_regions; i++) {
> >> +		int reg_size = region[i].len;
> >> +
> >> +		if (!reg_size)
> >> +			reg_size = (region[i].bits_per_irq * 1024) / 8;
> >> +
> >> +		if ((offset < region[i].reg_offset) ||
> >> +		    (offset + len > region[i].reg_offset + reg_size))
> >> +			continue;
> >> +
> >> +		dev.base_addr	= region[i].reg_offset;
> >> +		dev.redist_vcpu	= vcpu;
> >> +
> >> +		if (is_write)
> >> +			return region[i].ops.write(vcpu, &dev.dev,
> >> +						   offset, len, val);
> >> +		else
> >> +			return region[i].ops.read(vcpu, &dev.dev,
> >> +						  offset, len, val);
> >> +	}
> >> +
> >> +	return -ENODEV;
> >> +}
> >> +
> >> +int vgic_v2_dist_access(struct kvm_vcpu *vcpu, bool is_write,
> >> +			int offset, int len, void *val)
> >> +{
> >> +	return vgic_mmio_access(vcpu, vgic_v2_dist_registers,
> >> +				ARRAY_SIZE(vgic_v2_dist_registers),
> >> +				is_write, offset, len, val);
> >> +}
> >> +
> > 
> > this makes me wonder if the v2 region declarations and functions like
> > this should go in a separate file?  vgic/mmio-v2.c ?
> 
> But we actually share a lot of functions like all the PENDING, ENABLED,
> ACTIVE, PRIORITY, IGROUP register handlers. So we would need to export
> them in order to be used by both the v2 and v3 emulation. This is what I
> did for the existing v3 emulation, but I didn't like it very much.
> Now we keep all of this private and static, both the handler functions
> and the structures declaring the respective register layout.
> I found this more appropriate now that v2 and v3 emulation are developed
> hand in hand.
> I see that the file gets pretty big, but it's still only roughly half
> the size of the old vgic.c and also smaller than the combined old
> vgic-v2-emul.c and vgic-v3-emul.c.
> 
> So I guess those 37K are just the price we need to pay for the beast
> that the GIC actually is.
> 
ok, that's a convincing argument.

Thanks,
-Christoffer