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

[Paul Mackerras <paulus@samba.org>]

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; you add it in a later
patch.  Since this patch adds the KVM_CREATE_DEVICE ioctl, it probably
should add the KVM_CAP_DEVICE_CTRL capability too.


> +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?

> --- 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?

> 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.

> 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
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> @@ -909,6 +910,32 @@ struct kvm_s390_ucas_mapping {
>  #define KVM_ARM_SET_DEVICE_ADDR	  _IOW(KVMIO,  0xab, struct kvm_arm_device_addr)
>  
>  /*
> + * 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.

Paul.