On Wed, Jul 02, 2014 at 10:17:20AM +0100, Cornelia Huck wrote:
> On Tue, 1 Jul 2014 15:45:15 +0100
> Will Deacon <will.deacon@xxxxxxx> wrote:
>
> > kvm_ioctl_create_device currently has knowledge of all the device types
> > and their associated ops. This is fairly inflexible when adding support
> > for new in-kernel device emulations, so move what we currently have out
> > into a table, which can support dynamic registration of ops by new
> > drivers for virtual hardware.
> >
> > I didn't try to port all current drivers over, as it's not always clear
> > which initialisation hook the ops should be registered from.
>
> I think that last paragraph should rather go into a cover letter :)
>
> >
> > Cc: Cornelia Huck <cornelia.huck@xxxxxxxxxx>
> > Cc: Alex Williamson <Alex.Williamson@xxxxxxxxxx>
> > Cc: Alex Graf <agraf@xxxxxxx>
> > Cc: Gleb Natapov <gleb@xxxxxxxxxx>
> > Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > Cc: Marc Zyngier <marc.zyngier@xxxxxxx>
> > Cc: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> > Signed-off-by: Will Deacon <will.deacon@xxxxxxx>
> > ---
> >
> > v1 -> v2: Added enum for KVM_DEV_TYPE* IDs, changed limits to ARRAY_SIZE,
> > removed stray semicolon, had a crack at porting VFIO, included
> > Cornelia's s390 FLIC patch.
>
> ...and the changelog as well (or keep changelogs for individual
> patches).
Yeah... this has grown to be bigger than one patch now. I can include that
for v3.
> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index e11d8f170a62..6875cc225dff 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -940,15 +940,25 @@ struct kvm_device_attr {
> > __u64 addr; /* userspace address of attr data */
> > };
> >
> > -#define KVM_DEV_TYPE_FSL_MPIC_20 1
> > -#define KVM_DEV_TYPE_FSL_MPIC_42 2
> > -#define KVM_DEV_TYPE_XICS 3
> > -#define KVM_DEV_TYPE_VFIO 4
> > #define KVM_DEV_VFIO_GROUP 1
> > #define KVM_DEV_VFIO_GROUP_ADD 1
> > #define KVM_DEV_VFIO_GROUP_DEL 2
> > -#define KVM_DEV_TYPE_ARM_VGIC_V2 5
> > -#define KVM_DEV_TYPE_FLIC 6
> > +
> > +enum kvm_device_type {
> > + KVM_DEV_TYPE_FSL_MPIC_20 = 1,
> > +#define KVM_DEV_TYPE_FSL_MPIC_20 KVM_DEV_TYPE_FSL_MPIC_20
> > + KVM_DEV_TYPE_FSL_MPIC_42,
> > +#define KVM_DEV_TYPE_FSL_MPIC_42 KVM_DEV_TYPE_FSL_MPIC_42
> > + KVM_DEV_TYPE_XICS,
> > +#define KVM_DEV_TYPE_XICS KVM_DEV_TYPE_XICS
> > + KVM_DEV_TYPE_VFIO,
> > +#define KVM_DEV_TYPE_VFIO KVM_DEV_TYPE_VFIO
> > + KVM_DEV_TYPE_ARM_VGIC_V2,
> > +#define KVM_DEV_TYPE_ARM_VGIC_V2 KVM_DEV_TYPE_ARM_VGIC_V2
> > + KVM_DEV_TYPE_FLIC,
> > +#define KVM_DEV_TYPE_FLIC KVM_DEV_TYPE_FLIC
> > + KVM_DEV_TYPE_MAX,
>
> Do you want to add the individual values to the enum? A removal of a
> type would be an API break (so we should be safe against renumbering),
> but it's sometimes helpful if one can get the numeric value at a glance.
Could do, but then I think the advantage of the enum is questionable over
the #defines, since you could just as easily have two entries in the enum
with the same ID value as forgetting to update a KVM_DEV_TYPE_MAX #define
(which was the reason for the enum in the first place).
So I'd be inclined to keep the patch as-is, unless you have really strong
objections?
Will
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
