From mboxrd@z Thu Jan 1 00:00:00 1970 From: "tiejun.chen" Subject: Re: [RFC PATCH v2 1/6] kvm: add device control API Date: Tue, 2 Apr 2013 14:59:57 +0800 Message-ID: <515A81ED.9060106@windriver.com> References: <1360820960-12537-1-git-send-email-scottwood@freescale.com> <1364856473-25245-1-git-send-email-scottwood@freescale.com> <1364856473-25245-2-git-send-email-scottwood@freescale.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: Alexander Graf , , , To: Scott Wood Return-path: In-Reply-To: <1364856473-25245-2-git-send-email-scottwood@freescale.com> Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 04/02/2013 06:47 AM, 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 > --- > Documentation/virtual/kvm/api.txt | 70 ++++++++++++++++++++++++++++++ > Documentation/virtual/kvm/devices/README | 1 + > arch/powerpc/include/asm/kvm_host.h | 6 +++ > arch/powerpc/include/asm/kvm_ppc.h | 2 + > arch/powerpc/kvm/powerpc.c | 7 +++ > include/uapi/linux/kvm.h | 27 ++++++++++++ > virt/kvm/kvm_main.c | 31 +++++++++++++ > 7 files changed, 144 insertions(+) > create mode 100644 Documentation/virtual/kvm/devices/README > > 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 > +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 > + > + Other error conditions may be defined by individual device types. > + > +Creates an emulated device in the kernel. The file descriptor returned > +in fd can be used with KVM_SET/GET/HAS_DEVICE_ATTR. > + > +If the KVM_CREATE_DEVICE_TEST flag is set, only test whether the > +device type is supported (not necessarily whether it can be created > +in the current vm). > + > +Individual devices should not define flags. Attributes should be used > +for specifying any behavior that is not implied by the device type > +number. > + > +struct kvm_create_device { > + __u32 type; /* in: KVM_DEV_TYPE_xxx */ > + __u32 fd; /* out: device handle */ > + __u32 flags; /* in: KVM_CREATE_DEVICE_xxx */ > +}; > + > +4.80 KVM_SET_DEVICE_ATTR/KVM_GET_DEVICE_ATTR > + > +Capability: KVM_CAP_DEVICE_CTRL > +Type: device ioctl > +Parameters: struct kvm_device_attr > +Returns: 0 on success, -1 on error > +Errors: > + ENXIO: The group or attribute is unknown/unsupported for this device > + EPERM: The attribute cannot (currently) be accessed this way > + (e.g. read-only attribute, or attribute that only makes > + sense when the device is in a different state) > + > + Other error conditions may be defined by individual device types. > + > +Gets/sets a specified piece of device configuration and/or state. The > +semantics are device-specific. See individual device documentation in > +the "devices" directory. As with ONE_REG, the size of the data > +transferred is defined by the particular attribute. > + > +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 */ > +}; > + > +4.81 KVM_HAS_DEVICE_ATTR > + > +Capability: KVM_CAP_DEVICE_CTRL > +Type: device ioctl > +Parameters: struct kvm_device_attr > +Returns: 0 on success, -1 on error > +Errors: > + ENXIO: The group or attribute is unknown/unsupported for this device > + > +Tests whether a device supports a particular attribute. A successful > +return indicates the attribute is implemented. It does not necessarily > +indicate that the attribute can be read or written in the device's > +current state. "addr" is ignored. > > 4.77 KVM_ARM_VCPU_INIT > > diff --git a/Documentation/virtual/kvm/devices/README b/Documentation/virtual/kvm/devices/README > new file mode 100644 > index 0000000..34a6983 > --- /dev/null > +++ b/Documentation/virtual/kvm/devices/README > @@ -0,0 +1 @@ > +This directory contains specific device bindings for KVM_CAP_DEVICE_CTRL. > diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h > index e34f8fe..e0caae2 100644 > --- 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 > + > 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; > + > #ifdef CONFIG_KVM_BOOK3S_64_HV > struct kvm_vcpu_arch_shared shregs; > > diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h > index f589307..f44932c 100644 > --- a/arch/powerpc/include/asm/kvm_ppc.h > +++ b/arch/powerpc/include/asm/kvm_ppc.h > @@ -323,4 +323,6 @@ static inline ulong kvmppc_get_ea_indexed(struct kvm_vcpu *vcpu, int ra, int rb) > return ea; > } > > +void mpic_put(void *priv); > + > #endif /* __POWERPC_KVM_PPC_H__ */ > 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; > + } > + > kvmppc_core_vcpu_free(vcpu); > } > > 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) > + > +/* ioctls for fds returned by KVM_CREATE_DEVICE */ > +#define KVM_SET_DEVICE_ATTR _IOW(KVMIO, 0xe1, struct kvm_device_attr) > +#define KVM_GET_DEVICE_ATTR _IOW(KVMIO, 0xe2, struct kvm_device_attr) > +#define KVM_HAS_DEVICE_ATTR _IOW(KVMIO, 0xe3, struct kvm_device_attr) > + > +/* > * ioctls for vcpu fds > */ > #define KVM_RUN _IO(KVMIO, 0x80) > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index ff71541..ed033c0 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -2158,6 +2158,17 @@ out: > } > #endif > > +static int kvm_ioctl_create_device(struct kvm *kvm, > + struct kvm_create_device *cd) > +{ > + bool test = cd->flags & KVM_CREATE_DEVICE_TEST; > + > + switch (cd->type) { > + default: > + return -ENODEV; > + } Even after apply patch 5, looks here still misses something like: if (test) WARN_ON_ONCE(!cd->type); return -ENODEV; Tiejun > +} > + > static long kvm_vm_ioctl(struct file *filp, > unsigned int ioctl, unsigned long arg) > { > @@ -2272,6 +2283,26 @@ static long kvm_vm_ioctl(struct file *filp, > break; > } > #endif > + case KVM_CREATE_DEVICE: { > + struct kvm_create_device cd; > + > + r = -EFAULT; > + if (copy_from_user(&cd, argp, sizeof(cd))) > + goto out; > + > + mutex_lock(&kvm->lock); > + r = kvm_ioctl_create_device(kvm, &cd); > + mutex_unlock(&kvm->lock); > + if (r) > + goto out; > + > + r = -EFAULT; > + if (copy_to_user(argp, &cd, sizeof(cd))) > + goto out; > + > + r = 0; > + break; > + } > default: > r = kvm_arch_vm_ioctl(filp, ioctl, arg); > if (r == -ENOTTY) >