| Message ID | 1364954273-18196-2-git-send-email-scottwood@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03.04.2013, at 03:57, 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> > --- > v3: remove some changes that were merged into this patch by accident, > and fix the error documentation for KVM_CREATE_DEVICE. > > NOTE: I had some difficulty figuring out what ioctl numbers I should > assign... it seems that at one point care was taken to keep vcpu and > vm ioctls separate, but some overlap exists now (despite not exhausing > the ioctl space). Some of that was my fault, but not all of it. :-) > I moved to a new ioctl range for device control -- please let me know > if there's something else you'd prefer I do. > --- > Documentation/virtual/kvm/api.txt | 70 ++++++++++++++++++++++++++++++ > Documentation/virtual/kvm/devices/README | 1 + > include/uapi/linux/kvm.h | 27 ++++++++++++ > virt/kvm/kvm_main.c | 31 +++++++++++++ > 4 files changed, 129 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..d52f3f9 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 > + > + Other error conditions may be defined by individual device types or > + have their standard meanings. > + > +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/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 */ > +}; Please move these above the ioctl number definitions, where all the other structs already are. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 02, 2013 at 08:57: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> > --- > v3: remove some changes that were merged into this patch by accident, > and fix the error documentation for KVM_CREATE_DEVICE. > > NOTE: I had some difficulty figuring out what ioctl numbers I should > assign... it seems that at one point care was taken to keep vcpu and > vm ioctls separate, but some overlap exists now (despite not exhausing > the ioctl space). Some of that was my fault, but not all of it. :-) > I moved to a new ioctl range for device control -- please let me know > if there's something else you'd prefer I do. > --- > Documentation/virtual/kvm/api.txt | 70 ++++++++++++++++++++++++++++++ > Documentation/virtual/kvm/devices/README | 1 + > include/uapi/linux/kvm.h | 27 ++++++++++++ > virt/kvm/kvm_main.c | 31 +++++++++++++ > 4 files changed, 129 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..d52f3f9 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 > + > + Other error conditions may be defined by individual device types or > + have their standard meanings. > + > +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 */ > +}; > + Since now each device has its own fd is it an advantage to enforce common interface between different devices? If we do so though why not handle file creation, ioctl and file descriptor lifetime in the common code. Common code will have "struct kvm_device" with "struct kvm_device_arch" and "struct kvm_device_ops" members. Instead of kvm_mpic_ioctl there will be kvm_device_ioctl which will despatch ioctls to a device using kvm_device->ops->(set|get|has)_attr pointers. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 04, 2013 at 01:41:35PM +0300, Gleb Natapov wrote: > Since now each device has its own fd is it an advantage to enforce > common interface between different devices? If we do so though why > not handle file creation, ioctl and file descriptor lifetime in the > common code. Common code will have "struct kvm_device" with "struct > kvm_device_arch" and "struct kvm_device_ops" members. Instead of > kvm_mpic_ioctl there will be kvm_device_ioctl which will despatch ioctls > to a device using kvm_device->ops->(set|get|has)_attr pointers. I thought about making the same request, but when I looked at it, the amount of code that could be made common in this way is pretty tiny, and doing that involves a bit of extra complexity, so I thought that on the whole it wouldn't be worthwhile. Paul. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 02, 2013 at 08:57:48PM -0500, Scott Wood wrote: [snip] > +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; > + } > +} This gives a compile error saying "error: unused variable `test'", which is fatal since this gets compiled under arch/powerpc/kvm, and we treat all warnings as errors there. This still gives a compile error at the end of your series if you try to compile with CONFIG_KVM_MPIC=n. Paul. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 04, 2013 at 06:47:45PM -0500, Scott Wood wrote: > On 04/04/2013 05:41:35 AM, Gleb Natapov wrote: > >On Tue, Apr 02, 2013 at 08:57:48PM -0500, Scott Wood wrote: > >> +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 */ > >> +}; > >> + > >Since now each device has its own fd is it an advantage to enforce > >common interface between different devices? > > I think so, even if only to avoid repeating the various pains > surrounding adding ioctls. Not necessarily "enforce", just enable. > If a device has some sort of command that does not fit neatly into > the "set or get" model, it could still add a new ioctl. > Make sense. > >If we do so though why not handle file creation, ioctl and file > >descriptor lifetime in the > >common code. Common code will have "struct kvm_device" with "struct > >kvm_device_arch" and "struct kvm_device_ops" members. Instead of > >kvm_mpic_ioctl there will be kvm_device_ioctl which will despatch > >ioctls > >to a device using kvm_device->ops->(set|get|has)_attr pointers. > > So make it more like the pre-fd version, except for the actual fd > usage? It would make destruction a bit simpler (assuming there's no > need for vcpu destruction code to access a device). Hopefully > nobody asks me to change it back again, though. :-) > -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 05, 2013 at 12:02:06PM +1100, Paul Mackerras wrote: > On Thu, Apr 04, 2013 at 01:41:35PM +0300, Gleb Natapov wrote: > > > Since now each device has its own fd is it an advantage to enforce > > common interface between different devices? If we do so though why > > not handle file creation, ioctl and file descriptor lifetime in the > > common code. Common code will have "struct kvm_device" with "struct > > kvm_device_arch" and "struct kvm_device_ops" members. Instead of > > kvm_mpic_ioctl there will be kvm_device_ioctl which will despatch ioctls > > to a device using kvm_device->ops->(set|get|has)_attr pointers. > > I thought about making the same request, but when I looked at it, the > amount of code that could be made common in this way is pretty tiny, > and doing that involves a bit of extra complexity, so I thought that > on the whole it wouldn't be worthwhile. > The value of doing so is not only in making some code common, but also moving fd lifetime management into the common code where it can be debugged once and for all potential users. I also expect the amount of shared code to grow when interface will be used by more architectures. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt index 976eb65..d52f3f9 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 + + Other error conditions may be defined by individual device types or + have their standard meanings. + +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/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; + } +} + 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)
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> --- v3: remove some changes that were merged into this patch by accident, and fix the error documentation for KVM_CREATE_DEVICE. NOTE: I had some difficulty figuring out what ioctl numbers I should assign... it seems that at one point care was taken to keep vcpu and vm ioctls separate, but some overlap exists now (despite not exhausing the ioctl space). Some of that was my fault, but not all of it. :-) I moved to a new ioctl range for device control -- please let me know if there's something else you'd prefer I do. --- Documentation/virtual/kvm/api.txt | 70 ++++++++++++++++++++++++++++++ Documentation/virtual/kvm/devices/README | 1 + include/uapi/linux/kvm.h | 27 ++++++++++++ virt/kvm/kvm_main.c | 31 +++++++++++++ 4 files changed, 129 insertions(+) create mode 100644 Documentation/virtual/kvm/devices/README