Re: [RFC PATCH v2 1/6] kvm: add device control API

[Scott Wood <scottwood@freescale.com>]

On 04/02/2013 08:02:39 PM, Paul Mackerras wrote:
> On Mon, Apr 01, 2013 at 05:47:48PM -0500, Scott Wood wrote:
> > Currently, devices that are emulated inside KVM are configured in a
> > hardcoded manner based on an assumption that any given architecture
> > only has one way to do it.  If there's any need to access device  
> state,
> > it is done through inflexible one-purpose-only IOCTLs (e.g.
> > KVM_GET/SET_LAPIC).  Defining new IOCTLs for every little thing is
> > cumbersome and depletes a limited numberspace.
> >
> > This API provides a mechanism to instantiate a device of a certain
> > type, returning an ID that can be used to set/get attributes of the
> > device.  Attributes may include configuration parameters (e.g.
> > register base address), device state, operational commands, etc.  It
> > is similar to the ONE_REG API, except that it acts on devices rather
> > than vcpus.
> >
> > Both device types and individual attributes can be tested without  
> having
> > to create the device or get/set the attribute, without the need for
> > separately managing enumerated capabilities.
> >
> > Signed-off-by: Scott Wood <scottwood@freescale.com>
> 
> Some comments below...
> 
> > diff --git a/Documentation/virtual/kvm/api.txt  
> b/Documentation/virtual/kvm/api.txt
> > index 976eb65..77328aa 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -2173,6 +2173,76 @@ header; first `n_valid' valid entries with  
> contents from the data
> >  written, then `n_invalid' invalid entries, invalidating any  
> previously
> >  valid entries found.
> >
> > +4.79 KVM_CREATE_DEVICE
> > +
> > +Capability: KVM_CAP_DEVICE_CTRL
> 
> I notice this patch doesn't add this capability;

Yes, it does (see below).

> you add it in a later patch.

Maybe you're thinking of KVM_CAP_IRQ_MPIC?

> > +Type: vm ioctl
> > +Parameters: struct kvm_create_device (in/out)
> > +Returns: 0 on success, -1 on error
> > +Errors:
> > +  ENODEV: The device type is unknown or unsupported
> > +  EEXIST: Device already created, and this type of device may not
> > +          be instantiated multiple times
> > +  ENOSPC: Too many devices have been created
> 
> Is this still a possible error code?

If you mean ENOSPC, probably not -- it'd be replaced with whatever  
errors can come out of creating a file descriptor.

> > --- a/arch/powerpc/include/asm/kvm_host.h
> > +++ b/arch/powerpc/include/asm/kvm_host.h
> > @@ -370,6 +370,9 @@ struct kvmppc_booke_debug_reg {
> >  	u64 dac[KVMPPC_BOOKE_MAX_DAC];
> >  };
> >
> > +#define KVMPPC_IRQCHIP_NONE	0
> > +#define KVMPPC_IRQCHIP_MPIC	1
> 
> This define should go in the patch that adds the MPIC device.
> 
> >  struct kvm_vcpu_arch {
> >  	ulong host_stack;
> >  	u32 host_pid;
> > @@ -549,6 +552,9 @@ struct kvm_vcpu_arch {
> >  	unsigned long magic_page_pa; /* phys addr to map the magic page  
> to */
> >  	unsigned long magic_page_ea; /* effect. addr to map the magic  
> page to */
> >
> > +	int irqchip_type;
> > +	void *irqchip_priv;
> 
> Since you add this (irqchip_priv) only to remove it in a later patch
> and replace it by a device-specific pointer, why bother adding it
> here?  And why not give irqchip_type the name it ultimately ends up
> with?

Oops... These were patch shuffling accidents and will be removed from  
the next iteration.

> > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> > index 16b4595..bdfa526 100644
> > --- a/arch/powerpc/kvm/powerpc.c
> > +++ b/arch/powerpc/kvm/powerpc.c
> > @@ -459,6 +459,13 @@ void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu)
> >  	tasklet_kill(&vcpu->arch.tasklet);
> >
> >  	kvmppc_remove_vcpu_debugfs(vcpu);
> > +
> > +	switch (vcpu->arch.irqchip_type) {
> > +	case KVMPPC_IRQCHIP_MPIC:
> > +		mpic_put(vcpu->arch.irqchip_priv);
> > +		break;
> > +	}
> 
> This is going to break bisection, since you don't define mpic_put() in
> this patch.

Sigh.  Something got messed up; I'll try to sort it out and resubmit.

> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index 74d0ff3..20ce2d2 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -668,6 +668,7 @@ struct kvm_ppc_smmu_info {
> >  #define KVM_CAP_PPC_EPR 86
> >  #define KVM_CAP_ARM_PSCI 87
> >  #define KVM_CAP_ARM_SET_DEVICE_ADDR 88
> > +#define KVM_CAP_DEVICE_CTRL 89

See, here's the capability. :-)

> >  /*
> > + * Device control API, available with KVM_CAP_DEVICE_CTRL
> > + */
> > +#define KVM_CREATE_DEVICE_TEST		1
> > +
> > +struct kvm_create_device {
> > +	__u32	type;	/* in: KVM_DEV_TYPE_xxx */
> > +	__u32	fd;	/* out: device handle */
> > +	__u32	flags;	/* in: KVM_CREATE_DEVICE_xxx */
> > +};
> > +
> > +struct kvm_device_attr {
> > +	__u32	flags;		/* no flags currently defined */
> > +	__u32	group;		/* device-defined */
> > +	__u64	attr;		/* group-defined */
> > +	__u64	addr;		/* userspace address of attr data */
> > +};
> > +
> > +/* ioctl for vm fd */
> > +#define KVM_CREATE_DEVICE	  _IOWR(KVMIO,  0xe0, struct  
> kvm_create_device)
> 
> This define should go with the other VM ioctls, otherwise the next
> person to add a VM ioctl will probably miss it and reuse the 0xe0
> code.

That's actually why I moved it to a new section, with device control  
ioctls getting their own range, as the legacy "device model" and some  
other things did.  0xe0 is not the next ioctl that would be used for  
either vm or vcpu.  The ioctl numbering is actually already a mess,  
with sometimes care being taken to keep vcpu and vm ioctls from  
overlapping, but on other places overlapping does happen.  I'm not sure  
what exactly I should do here.

-Scott