* [libvirt] [PATCH v2 1/4] vfio/mdev: Add new instances parameter for mdev create
2018-07-20 2:19 ` [libvirt] [PATCH v2 0/4] New mdev type handling " Zhenyu Wang
@ 2018-07-20 2:19 ` Zhenyu Wang
2018-07-26 15:37 ` Cornelia Huck
2018-07-20 2:19 ` [libvirt] [PATCH v2 2/4] vfio/mdev: Add mdev device instances attribute Zhenyu Wang
` (11 subsequent siblings)
12 siblings, 1 reply; 38+ messages in thread
From: Zhenyu Wang @ 2018-07-20 2:19 UTC (permalink / raw)
To: libvirt-list, kvm; +Cc: kevin.tian, intel-gvt-dev, kwankhede
For special mdev type which can aggregate instances for mdev device,
this extends mdev create interface by allowing extra "instances=xxx"
parameter, which is passed to mdev device model to be able to create
arbitrary bundled number of instances for target mdev device.
v2: create new create_with_instances operator for vendor driver
Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
drivers/vfio/mdev/mdev_core.c | 18 +++++++++++++----
drivers/vfio/mdev/mdev_private.h | 5 ++++-
drivers/vfio/mdev/mdev_sysfs.c | 34 ++++++++++++++++++++++++++------
include/linux/mdev.h | 10 ++++++++++
4 files changed, 56 insertions(+), 11 deletions(-)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index 0212f0ee8aea..e69db302a4f2 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -104,12 +104,16 @@ static inline void mdev_put_parent(struct mdev_parent *parent)
}
static int mdev_device_create_ops(struct kobject *kobj,
- struct mdev_device *mdev)
+ struct mdev_device *mdev,
+ unsigned int instances)
{
struct mdev_parent *parent = mdev->parent;
int ret;
- ret = parent->ops->create(kobj, mdev);
+ if (instances > 1)
+ ret = parent->ops->create_with_instances(kobj, mdev, instances);
+ else
+ ret = parent->ops->create(kobj, mdev);
if (ret)
return ret;
@@ -276,7 +280,8 @@ static void mdev_device_release(struct device *dev)
kfree(mdev);
}
-int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid)
+int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid,
+ unsigned int instances)
{
int ret;
struct mdev_device *mdev, *tmp;
@@ -287,6 +292,11 @@ int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid)
if (!parent)
return -EINVAL;
+ if (instances > 1 && !parent->ops->create_with_instances) {
+ ret = -EINVAL;
+ goto mdev_fail;
+ }
+
mutex_lock(&mdev_list_lock);
/* Check for duplicate */
@@ -323,7 +333,7 @@ int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid)
goto mdev_fail;
}
- ret = mdev_device_create_ops(kobj, mdev);
+ ret = mdev_device_create_ops(kobj, mdev, instances);
if (ret)
goto create_fail;
diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
index b5819b7d7ef7..c9d3fe04e273 100644
--- a/drivers/vfio/mdev/mdev_private.h
+++ b/drivers/vfio/mdev/mdev_private.h
@@ -52,13 +52,16 @@ struct mdev_type {
#define to_mdev_type(_kobj) \
container_of(_kobj, struct mdev_type, kobj)
+#define MDEV_CREATE_OPT_LEN 32
+
int parent_create_sysfs_files(struct mdev_parent *parent);
void parent_remove_sysfs_files(struct mdev_parent *parent);
int mdev_create_sysfs_files(struct device *dev, struct mdev_type *type);
void mdev_remove_sysfs_files(struct device *dev, struct mdev_type *type);
-int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid);
+int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid,
+ unsigned int instances);
int mdev_device_remove(struct device *dev, bool force_remove);
#endif /* MDEV_PRIVATE_H */
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
index 249472f05509..a06e5b7c69d3 100644
--- a/drivers/vfio/mdev/mdev_sysfs.c
+++ b/drivers/vfio/mdev/mdev_sysfs.c
@@ -54,11 +54,15 @@ static const struct sysfs_ops mdev_type_sysfs_ops = {
static ssize_t create_store(struct kobject *kobj, struct device *dev,
const char *buf, size_t count)
{
- char *str;
+ char *str, *opt = NULL;
uuid_le uuid;
int ret;
+ unsigned int instances = 1;
- if ((count < UUID_STRING_LEN) || (count > UUID_STRING_LEN + 1))
+ if (count < UUID_STRING_LEN)
+ return -EINVAL;
+
+ if (count > UUID_STRING_LEN + 1 + MDEV_CREATE_OPT_LEN)
return -EINVAL;
str = kstrndup(buf, count, GFP_KERNEL);
@@ -66,13 +70,31 @@ static ssize_t create_store(struct kobject *kobj, struct device *dev,
return -ENOMEM;
ret = uuid_le_to_bin(str, &uuid);
- kfree(str);
- if (ret)
+ if (ret) {
+ kfree(str);
return ret;
+ }
- ret = mdev_device_create(kobj, dev, uuid);
- if (ret)
+ if (count > UUID_STRING_LEN + 1) {
+ opt = str + UUID_STRING_LEN;
+ if (*opt++ != ',' ||
+ strncmp(opt, "instances=", 10)) {
+ kfree(str);
+ return -EINVAL;
+ }
+ opt += 10;
+ if (kstrtouint(opt, 10, &instances)) {
+ kfree(str);
+ return -EINVAL;
+ }
+ }
+
+ ret = mdev_device_create(kobj, dev, uuid, instances);
+ if (ret) {
+ kfree(str);
return ret;
+ }
+ kfree(str);
return count;
}
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index b6e048e1045f..cfb702600f95 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -30,6 +30,13 @@ struct mdev_device;
* @kobj: kobject of type for which 'create' is called.
* @mdev: mdev_device structure on of mediated device
* that is being created
+ * @create_with_instances: Allocate aggregated instances' resources in parent device's
+ * driver for a particular mediated device. It is optional
+ * if doesn't support aggregated resources.
+ * @kobj: kobject of type for which 'create' is called.
+ * @mdev: mdev_device structure on of mediated device
+ * that is being created
+ * @instances: number of instances to aggregate
* Returns integer: success (0) or error (< 0)
* @remove: Called to free resources in parent device's driver for a
* a mediated device. It is mandatory to provide 'remove'
@@ -71,6 +78,9 @@ struct mdev_parent_ops {
struct attribute_group **supported_type_groups;
int (*create)(struct kobject *kobj, struct mdev_device *mdev);
+ int (*create_with_instances)(struct kobject *kobj,
+ struct mdev_device *mdev,
+ unsigned int instances);
int (*remove)(struct mdev_device *mdev);
int (*open)(struct mdev_device *mdev);
void (*release)(struct mdev_device *mdev);
--
2.18.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [libvirt] [PATCH v2 1/4] vfio/mdev: Add new instances parameter for mdev create
2018-07-20 2:19 ` [libvirt] [PATCH v2 1/4] vfio/mdev: Add new instances parameter for mdev create Zhenyu Wang
@ 2018-07-26 15:37 ` Cornelia Huck
0 siblings, 0 replies; 38+ messages in thread
From: Cornelia Huck @ 2018-07-26 15:37 UTC (permalink / raw)
To: Zhenyu Wang; +Cc: kevin.tian, kvm, libvirt-list, kwankhede, intel-gvt-dev
On Fri, 20 Jul 2018 10:19:25 +0800
Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> For special mdev type which can aggregate instances for mdev device,
> this extends mdev create interface by allowing extra "instances=xxx"
> parameter, which is passed to mdev device model to be able to create
> arbitrary bundled number of instances for target mdev device.
>
> v2: create new create_with_instances operator for vendor driver
>
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> ---
> drivers/vfio/mdev/mdev_core.c | 18 +++++++++++++----
> drivers/vfio/mdev/mdev_private.h | 5 ++++-
> drivers/vfio/mdev/mdev_sysfs.c | 34 ++++++++++++++++++++++++++------
> include/linux/mdev.h | 10 ++++++++++
> 4 files changed, 56 insertions(+), 11 deletions(-)
>
(...)
> diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
> index 249472f05509..a06e5b7c69d3 100644
> --- a/drivers/vfio/mdev/mdev_sysfs.c
> +++ b/drivers/vfio/mdev/mdev_sysfs.c
> @@ -54,11 +54,15 @@ static const struct sysfs_ops mdev_type_sysfs_ops = {
> static ssize_t create_store(struct kobject *kobj, struct device *dev,
> const char *buf, size_t count)
> {
> - char *str;
> + char *str, *opt = NULL;
> uuid_le uuid;
> int ret;
> + unsigned int instances = 1;
>
> - if ((count < UUID_STRING_LEN) || (count > UUID_STRING_LEN + 1))
> + if (count < UUID_STRING_LEN)
> + return -EINVAL;
> +
> + if (count > UUID_STRING_LEN + 1 + MDEV_CREATE_OPT_LEN)
Do you plan to have other optional parameters? If you don't, you could
probably do a quick exit here if count is between UUID_STRING_LEN + 1
and UUID_STRING_LEN + 12 (for ",instances=<one digit>")?
> return -EINVAL;
>
> str = kstrndup(buf, count, GFP_KERNEL);
(...)
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index b6e048e1045f..cfb702600f95 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -30,6 +30,13 @@ struct mdev_device;
> * @kobj: kobject of type for which 'create' is called.
> * @mdev: mdev_device structure on of mediated device
> * that is being created
> + * @create_with_instances: Allocate aggregated instances' resources in parent device's
> + * driver for a particular mediated device. It is optional
> + * if doesn't support aggregated resources.
"Optional if aggregated resources are not supported"
> + * @kobj: kobject of type for which 'create' is called.
> + * @mdev: mdev_device structure on of mediated device
> + * that is being created
> + * @instances: number of instances to aggregate
> * Returns integer: success (0) or error (< 0)
You need that "Returns" line for both the old and the new ops.
> * @remove: Called to free resources in parent device's driver for a
> * a mediated device. It is mandatory to provide 'remove'
> @@ -71,6 +78,9 @@ struct mdev_parent_ops {
> struct attribute_group **supported_type_groups;
>
> int (*create)(struct kobject *kobj, struct mdev_device *mdev);
> + int (*create_with_instances)(struct kobject *kobj,
> + struct mdev_device *mdev,
> + unsigned int instances);
> int (*remove)(struct mdev_device *mdev);
> int (*open)(struct mdev_device *mdev);
> void (*release)(struct mdev_device *mdev);
^ permalink raw reply [flat|nested] 38+ messages in thread
* [libvirt] [PATCH v2 2/4] vfio/mdev: Add mdev device instances attribute
2018-07-20 2:19 ` [libvirt] [PATCH v2 0/4] New mdev type handling " Zhenyu Wang
2018-07-20 2:19 ` [libvirt] [PATCH v2 1/4] vfio/mdev: Add new instances parameter for mdev create Zhenyu Wang
@ 2018-07-20 2:19 ` Zhenyu Wang
2018-07-20 2:19 ` [libvirt] [PATCH v2 3/4] drm/i915/gvt: Add new aggregation type support Zhenyu Wang
` (10 subsequent siblings)
12 siblings, 0 replies; 38+ messages in thread
From: Zhenyu Wang @ 2018-07-20 2:19 UTC (permalink / raw)
To: libvirt-list, kvm; +Cc: kevin.tian, intel-gvt-dev, kwankhede
For mdev device, create new sysfs attribute "instances" to show
number of instances allocated for possible aggregation type.
For compatibility default or without aggregated allocation, the
number is 1.
Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
drivers/vfio/mdev/mdev_core.c | 1 +
drivers/vfio/mdev/mdev_private.h | 1 +
drivers/vfio/mdev/mdev_sysfs.c | 8 ++++++++
3 files changed, 10 insertions(+)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index e69db302a4f2..f0478fc372d8 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -326,6 +326,7 @@ int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid,
mdev->dev.bus = &mdev_bus_type;
mdev->dev.release = mdev_device_release;
dev_set_name(&mdev->dev, "%pUl", uuid.b);
+ mdev->type_instances = instances;
ret = device_register(&mdev->dev);
if (ret) {
diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
index c9d3fe04e273..aa0b4b64c503 100644
--- a/drivers/vfio/mdev/mdev_private.h
+++ b/drivers/vfio/mdev/mdev_private.h
@@ -33,6 +33,7 @@ struct mdev_device {
struct kref ref;
struct list_head next;
struct kobject *type_kobj;
+ unsigned int type_instances;
bool active;
};
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
index a06e5b7c69d3..5a417a20e774 100644
--- a/drivers/vfio/mdev/mdev_sysfs.c
+++ b/drivers/vfio/mdev/mdev_sysfs.c
@@ -268,10 +268,18 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
return count;
}
+static ssize_t instances_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct mdev_device *mdev = to_mdev_device(dev);
+ return sprintf(buf, "%u\n", mdev->type_instances);
+}
+
static DEVICE_ATTR_WO(remove);
+static DEVICE_ATTR_RO(instances);
static const struct attribute *mdev_device_attrs[] = {
&dev_attr_remove.attr,
+ &dev_attr_instances.attr,
NULL,
};
--
2.18.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [libvirt] [PATCH v2 3/4] drm/i915/gvt: Add new aggregation type support
2018-07-20 2:19 ` [libvirt] [PATCH v2 0/4] New mdev type handling " Zhenyu Wang
2018-07-20 2:19 ` [libvirt] [PATCH v2 1/4] vfio/mdev: Add new instances parameter for mdev create Zhenyu Wang
2018-07-20 2:19 ` [libvirt] [PATCH v2 2/4] vfio/mdev: Add mdev device instances attribute Zhenyu Wang
@ 2018-07-20 2:19 ` Zhenyu Wang
2018-07-20 2:19 ` [libvirt] [PATCH v2 4/4] Documentation/vfio-mediated-device.txt: update for aggregation attribute Zhenyu Wang
` (9 subsequent siblings)
12 siblings, 0 replies; 38+ messages in thread
From: Zhenyu Wang @ 2018-07-20 2:19 UTC (permalink / raw)
To: libvirt-list, kvm; +Cc: kevin.tian, intel-gvt-dev, kwankhede
New aggregation type is created for KVMGT which can be used
with new mdev create "instances=xxx" parameter to combine
minimal resource number for target instances, which can create
user defined number of resources. For KVMGT, aggregated resource
is determined by memory and fence resource allocation for target
number of instances.
Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
drivers/gpu/drm/i915/gvt/gvt.c | 26 ++++++++++++---
drivers/gpu/drm/i915/gvt/gvt.h | 14 +++++---
drivers/gpu/drm/i915/gvt/kvmgt.c | 30 ++++++++++++++---
drivers/gpu/drm/i915/gvt/vgpu.c | 56 ++++++++++++++++++++++++++++----
4 files changed, 104 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
index 712f9d14e720..21600447b029 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.c
+++ b/drivers/gpu/drm/i915/gvt/gvt.c
@@ -57,7 +57,7 @@ static struct intel_vgpu_type *intel_gvt_find_vgpu_type(struct intel_gvt *gvt,
for (i = 0; i < gvt->num_types; i++) {
t = &gvt->types[i];
if (!strncmp(t->name, name + strlen(driver_name) + 1,
- sizeof(t->name)))
+ strlen(t->name)))
return t;
}
@@ -105,9 +105,16 @@ static ssize_t description_show(struct kobject *kobj, struct device *dev,
type->weight);
}
+static ssize_t aggregation_show(struct kobject *kobj, struct device *dev,
+ char *buf)
+{
+ return sprintf(buf, "%s\n", "true");
+}
+
static MDEV_TYPE_ATTR_RO(available_instances);
static MDEV_TYPE_ATTR_RO(device_api);
static MDEV_TYPE_ATTR_RO(description);
+static MDEV_TYPE_ATTR_RO(aggregation);
static struct attribute *gvt_type_attrs[] = {
&mdev_type_attr_available_instances.attr,
@@ -116,14 +123,20 @@ static struct attribute *gvt_type_attrs[] = {
NULL,
};
+static struct attribute *gvt_type_aggregate_attrs[] = {
+ &mdev_type_attr_available_instances.attr,
+ &mdev_type_attr_device_api.attr,
+ &mdev_type_attr_description.attr,
+ &mdev_type_attr_aggregation.attr,
+ NULL,
+};
+
static struct attribute_group *gvt_vgpu_type_groups[] = {
[0 ... NR_MAX_INTEL_VGPU_TYPES - 1] = NULL,
};
-static bool intel_get_gvt_attrs(struct attribute ***type_attrs,
- struct attribute_group ***intel_vgpu_type_groups)
+static bool intel_get_gvt_attrs(struct attribute_group ***intel_vgpu_type_groups)
{
- *type_attrs = gvt_type_attrs;
*intel_vgpu_type_groups = gvt_vgpu_type_groups;
return true;
}
@@ -142,7 +155,10 @@ static bool intel_gvt_init_vgpu_type_groups(struct intel_gvt *gvt)
goto unwind;
group->name = type->name;
- group->attrs = gvt_type_attrs;
+ if (type->aggregation)
+ group->attrs = gvt_type_aggregate_attrs;
+ else
+ group->attrs = gvt_type_attrs;
gvt_vgpu_type_groups[i] = group;
}
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index 9a9671522774..8848587f638f 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -241,6 +241,9 @@ struct intel_vgpu {
struct intel_gvt_gm {
unsigned long vgpu_allocated_low_gm_size;
unsigned long vgpu_allocated_high_gm_size;
+ unsigned long low_avail;
+ unsigned long high_avail;
+ unsigned long fence_avail;
};
struct intel_gvt_fence {
@@ -292,13 +295,14 @@ struct intel_gvt_firmware {
#define NR_MAX_INTEL_VGPU_TYPES 20
struct intel_vgpu_type {
- char name[16];
+ char name[32];
unsigned int avail_instance;
unsigned int low_gm_size;
unsigned int high_gm_size;
unsigned int fence;
unsigned int weight;
enum intel_vgpu_edid resolution;
+ bool aggregation; /* fine-grained resource type with aggregation capability */
};
struct intel_gvt {
@@ -484,7 +488,8 @@ void intel_gvt_clean_vgpu_types(struct intel_gvt *gvt);
struct intel_vgpu *intel_gvt_create_idle_vgpu(struct intel_gvt *gvt);
void intel_gvt_destroy_idle_vgpu(struct intel_vgpu *vgpu);
struct intel_vgpu *intel_gvt_create_vgpu(struct intel_gvt *gvt,
- struct intel_vgpu_type *type);
+ struct intel_vgpu_type *type,
+ unsigned int);
void intel_gvt_destroy_vgpu(struct intel_vgpu *vgpu);
void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr,
unsigned int engine_mask);
@@ -562,15 +567,14 @@ struct intel_gvt_ops {
int (*emulate_mmio_write)(struct intel_vgpu *, u64, void *,
unsigned int);
struct intel_vgpu *(*vgpu_create)(struct intel_gvt *,
- struct intel_vgpu_type *);
+ struct intel_vgpu_type *, unsigned int);
void (*vgpu_destroy)(struct intel_vgpu *);
void (*vgpu_reset)(struct intel_vgpu *);
void (*vgpu_activate)(struct intel_vgpu *);
void (*vgpu_deactivate)(struct intel_vgpu *);
struct intel_vgpu_type *(*gvt_find_vgpu_type)(struct intel_gvt *gvt,
const char *name);
- bool (*get_gvt_attrs)(struct attribute ***type_attrs,
- struct attribute_group ***intel_vgpu_type_groups);
+ bool (*get_gvt_attrs)(struct attribute_group ***intel_vgpu_type_groups);
int (*vgpu_query_plane)(struct intel_vgpu *vgpu, void *);
int (*vgpu_get_dmabuf)(struct intel_vgpu *vgpu, unsigned int);
int (*write_protect_handler)(struct intel_vgpu *, u64, void *,
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 4d2f53ae9f0f..4f3a57b510fd 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -498,7 +498,9 @@ static void kvmgt_put_vfio_device(void *vgpu)
vfio_device_put(((struct intel_vgpu *)vgpu)->vdev.vfio_device);
}
-static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
+static int intel_vgpu_create_internal(struct kobject *kobj,
+ struct mdev_device *mdev,
+ unsigned int instances)
{
struct intel_vgpu *vgpu = NULL;
struct intel_vgpu_type *type;
@@ -517,7 +519,12 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
goto out;
}
- vgpu = intel_gvt_ops->vgpu_create(gvt, type);
+ if (instances > 1 && !type->aggregation) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ vgpu = intel_gvt_ops->vgpu_create(gvt, type, instances);
if (IS_ERR_OR_NULL(vgpu)) {
ret = vgpu == NULL ? -EFAULT : PTR_ERR(vgpu);
gvt_err("failed to create intel vgpu: %d\n", ret);
@@ -537,6 +544,20 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
return ret;
}
+static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
+{
+ return intel_vgpu_create_internal(kobj, mdev, 1);
+}
+
+static int intel_vgpu_create_with_instances(struct kobject *kobj,
+ struct mdev_device *mdev,
+ unsigned int instances)
+{
+ return intel_vgpu_create_internal(kobj, mdev, instances);
+}
+
+
+
static int intel_vgpu_remove(struct mdev_device *mdev)
{
struct intel_vgpu *vgpu = mdev_get_drvdata(mdev);
@@ -1430,6 +1451,7 @@ static const struct attribute_group *intel_vgpu_groups[] = {
static struct mdev_parent_ops intel_vgpu_ops = {
.mdev_attr_groups = intel_vgpu_groups,
.create = intel_vgpu_create,
+ .create_with_instances = intel_vgpu_create_with_instances,
.remove = intel_vgpu_remove,
.open = intel_vgpu_open,
@@ -1443,12 +1465,10 @@ static struct mdev_parent_ops intel_vgpu_ops = {
static int kvmgt_host_init(struct device *dev, void *gvt, const void *ops)
{
- struct attribute **kvm_type_attrs;
struct attribute_group **kvm_vgpu_type_groups;
intel_gvt_ops = ops;
- if (!intel_gvt_ops->get_gvt_attrs(&kvm_type_attrs,
- &kvm_vgpu_type_groups))
+ if (!intel_gvt_ops->get_gvt_attrs(&kvm_vgpu_type_groups))
return -EFAULT;
intel_vgpu_ops.supported_type_groups = kvm_vgpu_type_groups;
diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
index f6fa916517c3..5304b983a84b 100644
--- a/drivers/gpu/drm/i915/gvt/vgpu.c
+++ b/drivers/gpu/drm/i915/gvt/vgpu.c
@@ -125,11 +125,14 @@ int intel_gvt_init_vgpu_types(struct intel_gvt *gvt)
high_avail = gvt_hidden_sz(gvt) - HOST_HIGH_GM_SIZE;
num_types = sizeof(vgpu_types) / sizeof(vgpu_types[0]);
- gvt->types = kcalloc(num_types, sizeof(struct intel_vgpu_type),
+ gvt->types = kcalloc(num_types + 1, sizeof(struct intel_vgpu_type),
GFP_KERNEL);
if (!gvt->types)
return -ENOMEM;
+ gvt->gm.low_avail = low_avail;
+ gvt->gm.high_avail = high_avail;
+ gvt->gm.fence_avail = 32 - HOST_FENCE;
min_low = MB_TO_BYTES(32);
for (i = 0; i < num_types; ++i) {
if (low_avail / vgpu_types[i].low_mm == 0)
@@ -149,11 +152,11 @@ int intel_gvt_init_vgpu_types(struct intel_gvt *gvt)
high_avail / vgpu_types[i].high_mm);
if (IS_GEN8(gvt->dev_priv))
- sprintf(gvt->types[i].name, "GVTg_V4_%s",
- vgpu_types[i].name);
+ snprintf(gvt->types[i].name, sizeof(gvt->types[i].name),
+ "GVTg_V4_%s", vgpu_types[i].name);
else if (IS_GEN9(gvt->dev_priv))
- sprintf(gvt->types[i].name, "GVTg_V5_%s",
- vgpu_types[i].name);
+ snprintf(gvt->types[i].name, sizeof(gvt->types[i].name),
+ "GVTg_V5_%s", vgpu_types[i].name);
gvt_dbg_core("type[%d]: %s avail %u low %u high %u fence %u weight %u res %s\n",
i, gvt->types[i].name,
@@ -164,7 +167,32 @@ int intel_gvt_init_vgpu_types(struct intel_gvt *gvt)
vgpu_edid_str(gvt->types[i].resolution));
}
- gvt->num_types = i;
+ /* add aggregation type */
+ gvt->types[i].low_gm_size = MB_TO_BYTES(32);
+ gvt->types[i].high_gm_size = MB_TO_BYTES(192);
+ gvt->types[i].fence = 2;
+ gvt->types[i].weight = 16;
+ gvt->types[i].resolution = GVT_EDID_1024_768;
+ gvt->types[i].avail_instance = min(low_avail / gvt->types[i].low_gm_size,
+ high_avail / gvt->types[i].high_gm_size);
+ gvt->types[i].avail_instance = min(gvt->types[i].avail_instance,
+ (32 - HOST_FENCE) / gvt->types[i].fence);
+ if (IS_GEN8(gvt->dev_priv))
+ strcat(gvt->types[i].name, "GVTg_V4_aggregate");
+ else if (IS_GEN9(gvt->dev_priv))
+ strcat(gvt->types[i].name, "GVTg_V5_aggregate");
+
+ gvt_dbg_core("type[%d]: %s avail %u low %u high %u fence %u weight %u res %s\n",
+ i, gvt->types[i].name,
+ gvt->types[i].avail_instance,
+ gvt->types[i].low_gm_size,
+ gvt->types[i].high_gm_size, gvt->types[i].fence,
+ gvt->types[i].weight,
+ vgpu_edid_str(gvt->types[i].resolution));
+
+ gvt->types[i].aggregation = true;
+ gvt->num_types = ++i;
+
return 0;
}
@@ -444,7 +472,8 @@ static struct intel_vgpu *__intel_gvt_create_vgpu(struct intel_gvt *gvt,
* pointer to intel_vgpu, error pointer if failed.
*/
struct intel_vgpu *intel_gvt_create_vgpu(struct intel_gvt *gvt,
- struct intel_vgpu_type *type)
+ struct intel_vgpu_type *type,
+ unsigned int instances)
{
struct intel_vgpu_creation_params param;
struct intel_vgpu *vgpu;
@@ -461,6 +490,19 @@ struct intel_vgpu *intel_gvt_create_vgpu(struct intel_gvt *gvt,
param.low_gm_sz = BYTES_TO_MB(param.low_gm_sz);
param.high_gm_sz = BYTES_TO_MB(param.high_gm_sz);
+ if (type->aggregation && instances > 1) {
+ if (instances > type->avail_instance)
+ return ERR_PTR(-EINVAL);
+ param.low_gm_sz = min(param.low_gm_sz * instances,
+ (u64)BYTES_TO_MB(gvt->gm.low_avail));
+ param.high_gm_sz = min(param.high_gm_sz * instances,
+ (u64)BYTES_TO_MB(gvt->gm.high_avail));
+ param.fence_sz = min(param.fence_sz * instances,
+ (u64)gvt->gm.fence_avail);
+ gvt_dbg_core("instances %d, low %lluMB, high %lluMB, fence %llu\n",
+ instances, param.low_gm_sz, param.high_gm_sz, param.fence_sz);
+ }
+
mutex_lock(&gvt->lock);
vgpu = __intel_gvt_create_vgpu(gvt, ¶m);
if (!IS_ERR(vgpu))
--
2.18.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [libvirt] [PATCH v2 4/4] Documentation/vfio-mediated-device.txt: update for aggregation attribute
2018-07-20 2:19 ` [libvirt] [PATCH v2 0/4] New mdev type handling " Zhenyu Wang
` (2 preceding siblings ...)
2018-07-20 2:19 ` [libvirt] [PATCH v2 3/4] drm/i915/gvt: Add new aggregation type support Zhenyu Wang
@ 2018-07-20 2:19 ` Zhenyu Wang
2018-07-26 15:46 ` Cornelia Huck
2018-07-24 17:44 ` [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources Alex Williamson
` (8 subsequent siblings)
12 siblings, 1 reply; 38+ messages in thread
From: Zhenyu Wang @ 2018-07-20 2:19 UTC (permalink / raw)
To: libvirt-list, kvm; +Cc: kevin.tian, intel-gvt-dev, kwankhede
Update mdev doc on new aggregration attribute and instances attribute
for mdev.
Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
Documentation/vfio-mediated-device.txt | 39 ++++++++++++++++++++++----
1 file changed, 33 insertions(+), 6 deletions(-)
diff --git a/Documentation/vfio-mediated-device.txt b/Documentation/vfio-mediated-device.txt
index c3f69bcaf96e..9ec9495dcbe7 100644
--- a/Documentation/vfio-mediated-device.txt
+++ b/Documentation/vfio-mediated-device.txt
@@ -211,12 +211,20 @@ Directories and files under the sysfs for Each Physical Device
| | |--- description
| | |--- [devices]
| |--- [<type-id>]
- | |--- create
- | |--- name
- | |--- available_instances
- | |--- device_api
- | |--- description
- | |--- [devices]
+ | | |--- create
+ | | |--- name
+ | | |--- available_instances
+ | | |--- device_api
+ | | |--- description
+ | | |--- [devices]
+ | |--- [<type-id>]
+ | | |--- create
+ | | |--- name
+ | | |--- available_instances
+ | | |--- device_api
+ | | |--- description
+ | | |--- <aggregation>
+ | | |--- [devices]
* [mdev_supported_types]
@@ -260,6 +268,19 @@ Directories and files under the sysfs for Each Physical Device
This attribute should show brief features/description of the type. This is
optional attribute.
+* <aggregation>
+
+ The description is to show feature for one instance of the type. <aggregation>
+ is an optional attributes to show that [<type-id>]'s instances can be
+ aggregated to be assigned for one mdev device. Set number of instances by
+ appending "instances=N" parameter for create. Instances number can't exceed
+ available_instances number. Without "instances=N" parameter will be default
+ one instance to create.
+
+Example::
+
+ # echo "<uuid>,instances=N" > create
+
Directories and Files Under the sysfs for Each mdev Device
----------------------------------------------------------
@@ -268,6 +289,7 @@ Directories and Files Under the sysfs for Each mdev Device
|- [parent phy device]
|--- [$MDEV_UUID]
|--- remove
+ |--- instances
|--- mdev_type {link to its type}
|--- vendor-specific-attributes [optional]
@@ -281,6 +303,11 @@ Example::
# echo 1 > /sys/bus/mdev/devices/$mdev_UUID/remove
+* instances
+
+For aggregation type show number of instances assigned for this mdev. For normal
+type or default will just show one instance.
+
Mediated device Hot plug
------------------------
--
2.18.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [libvirt] [PATCH v2 4/4] Documentation/vfio-mediated-device.txt: update for aggregation attribute
2018-07-20 2:19 ` [libvirt] [PATCH v2 4/4] Documentation/vfio-mediated-device.txt: update for aggregation attribute Zhenyu Wang
@ 2018-07-26 15:46 ` Cornelia Huck
2018-07-27 2:16 ` Zhenyu Wang
0 siblings, 1 reply; 38+ messages in thread
From: Cornelia Huck @ 2018-07-26 15:46 UTC (permalink / raw)
To: Zhenyu Wang; +Cc: kevin.tian, kvm, libvirt-list, kwankhede, intel-gvt-dev
On Fri, 20 Jul 2018 10:19:28 +0800
Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> Update mdev doc on new aggregration attribute and instances attribute
> for mdev.
>
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> ---
> Documentation/vfio-mediated-device.txt | 39 ++++++++++++++++++++++----
> 1 file changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/vfio-mediated-device.txt b/Documentation/vfio-mediated-device.txt
> index c3f69bcaf96e..9ec9495dcbe7 100644
> --- a/Documentation/vfio-mediated-device.txt
> +++ b/Documentation/vfio-mediated-device.txt
> @@ -211,12 +211,20 @@ Directories and files under the sysfs for Each Physical Device
> | | |--- description
> | | |--- [devices]
> | |--- [<type-id>]
> - | |--- create
> - | |--- name
> - | |--- available_instances
> - | |--- device_api
> - | |--- description
> - | |--- [devices]
> + | | |--- create
> + | | |--- name
> + | | |--- available_instances
> + | | |--- device_api
> + | | |--- description
> + | | |--- [devices]
> + | |--- [<type-id>]
> + | | |--- create
> + | | |--- name
> + | | |--- available_instances
> + | | |--- device_api
> + | | |--- description
> + | | |--- <aggregation>
> + | | |--- [devices]
>
> * [mdev_supported_types]
>
> @@ -260,6 +268,19 @@ Directories and files under the sysfs for Each Physical Device
> This attribute should show brief features/description of the type. This is
> optional attribute.
>
> +* <aggregation>
> +
> + The description is to show feature for one instance of the type. <aggregation>
You are talking about "one instance" here. Can this be different for
the same type with different physical devices?
> + is an optional attributes to show that [<type-id>]'s instances can be
> + aggregated to be assigned for one mdev device. Set number of instances by
> + appending "instances=N" parameter for create. Instances number can't exceed
> + available_instances number. Without "instances=N" parameter will be default
> + one instance to create.
Could there be a case where available_instances is n, but aggregation
is only supported for a value m < n? If yes, should m be discoverable
via the "aggregation" attribute?
> +
> +Example::
> +
> + # echo "<uuid>,instances=N" > create
> +
> Directories and Files Under the sysfs for Each mdev Device
> ----------------------------------------------------------
>
> @@ -268,6 +289,7 @@ Directories and Files Under the sysfs for Each mdev Device
> |- [parent phy device]
> |--- [$MDEV_UUID]
> |--- remove
> + |--- instances
> |--- mdev_type {link to its type}
> |--- vendor-specific-attributes [optional]
>
> @@ -281,6 +303,11 @@ Example::
>
> # echo 1 > /sys/bus/mdev/devices/$mdev_UUID/remove
>
> +* instances
> +
> +For aggregation type show number of instances assigned for this mdev. For normal
> +type or default will just show one instance.
> +
> Mediated device Hot plug
> ------------------------
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [libvirt] [PATCH v2 4/4] Documentation/vfio-mediated-device.txt: update for aggregation attribute
2018-07-26 15:46 ` Cornelia Huck
@ 2018-07-27 2:16 ` Zhenyu Wang
2018-07-27 3:30 ` Alex Williamson
2018-07-27 11:49 ` Cornelia Huck
0 siblings, 2 replies; 38+ messages in thread
From: Zhenyu Wang @ 2018-07-27 2:16 UTC (permalink / raw)
To: Cornelia Huck; +Cc: kevin.tian, kvm, libvirt-list, kwankhede, intel-gvt-dev
[-- Attachment #1.1: Type: text/plain, Size: 4211 bytes --]
On 2018.07.26 17:46:40 +0200, Cornelia Huck wrote:
> On Fri, 20 Jul 2018 10:19:28 +0800
> Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
>
> > Update mdev doc on new aggregration attribute and instances attribute
> > for mdev.
> >
> > Cc: Kirti Wankhede <kwankhede@nvidia.com>
> > Cc: Alex Williamson <alex.williamson@redhat.com>
> > Cc: Kevin Tian <kevin.tian@intel.com>
> > Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> > ---
> > Documentation/vfio-mediated-device.txt | 39 ++++++++++++++++++++++----
> > 1 file changed, 33 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/vfio-mediated-device.txt b/Documentation/vfio-mediated-device.txt
> > index c3f69bcaf96e..9ec9495dcbe7 100644
> > --- a/Documentation/vfio-mediated-device.txt
> > +++ b/Documentation/vfio-mediated-device.txt
> > @@ -211,12 +211,20 @@ Directories and files under the sysfs for Each Physical Device
> > | | |--- description
> > | | |--- [devices]
> > | |--- [<type-id>]
> > - | |--- create
> > - | |--- name
> > - | |--- available_instances
> > - | |--- device_api
> > - | |--- description
> > - | |--- [devices]
> > + | | |--- create
> > + | | |--- name
> > + | | |--- available_instances
> > + | | |--- device_api
> > + | | |--- description
> > + | | |--- [devices]
> > + | |--- [<type-id>]
> > + | | |--- create
> > + | | |--- name
> > + | | |--- available_instances
> > + | | |--- device_api
> > + | | |--- description
> > + | | |--- <aggregation>
> > + | | |--- [devices]
> >
> > * [mdev_supported_types]
> >
> > @@ -260,6 +268,19 @@ Directories and files under the sysfs for Each Physical Device
> > This attribute should show brief features/description of the type. This is
> > optional attribute.
> >
> > +* <aggregation>
> > +
> > + The description is to show feature for one instance of the type. <aggregation>
>
> You are talking about "one instance" here. Can this be different for
> the same type with different physical devices?
>
I would expect for normal mdev types, driver might expose like x2, x4, x8 types
which split hw resource equally. But for type with aggregation feature, it can
set user wanted number of instances. Sorry maybe my use of word was not clear, how
about "one example of type"?
> > + is an optional attributes to show that [<type-id>]'s instances can be
> > + aggregated to be assigned for one mdev device. Set number of instances by
> > + appending "instances=N" parameter for create. Instances number can't exceed
> > + available_instances number. Without "instances=N" parameter will be default
> > + one instance to create.
>
> Could there be a case where available_instances is n, but aggregation
> is only supported for a value m < n? If yes, should m be discoverable
> via the "aggregation" attribute?
>
I don't think there could be a case for that. As for aggregation type,
it represents minimal resource allocated for a singleton, I can't see
any reason for possible m < n.
Thanks.
> > +
> > +Example::
> > +
> > + # echo "<uuid>,instances=N" > create
> > +
> > Directories and Files Under the sysfs for Each mdev Device
> > ----------------------------------------------------------
> >
> > @@ -268,6 +289,7 @@ Directories and Files Under the sysfs for Each mdev Device
> > |- [parent phy device]
> > |--- [$MDEV_UUID]
> > |--- remove
> > + |--- instances
> > |--- mdev_type {link to its type}
> > |--- vendor-specific-attributes [optional]
> >
> > @@ -281,6 +303,11 @@ Example::
> >
> > # echo 1 > /sys/bus/mdev/devices/$mdev_UUID/remove
> >
> > +* instances
> > +
> > +For aggregation type show number of instances assigned for this mdev. For normal
> > +type or default will just show one instance.
> > +
> > Mediated device Hot plug
> > ------------------------
> >
>
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [libvirt] [PATCH v2 4/4] Documentation/vfio-mediated-device.txt: update for aggregation attribute
2018-07-27 2:16 ` Zhenyu Wang
@ 2018-07-27 3:30 ` Alex Williamson
2018-07-27 11:49 ` Cornelia Huck
1 sibling, 0 replies; 38+ messages in thread
From: Alex Williamson @ 2018-07-27 3:30 UTC (permalink / raw)
To: Zhenyu Wang
Cc: kevin.tian, kvm, Cornelia Huck, libvirt-list, kwankhede, intel-gvt-dev
On Fri, 27 Jul 2018 10:16:58 +0800
Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> On 2018.07.26 17:46:40 +0200, Cornelia Huck wrote:
> > On Fri, 20 Jul 2018 10:19:28 +0800
> > Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> >
> > > Update mdev doc on new aggregration attribute and instances attribute
> > > for mdev.
> > >
> > > Cc: Kirti Wankhede <kwankhede@nvidia.com>
> > > Cc: Alex Williamson <alex.williamson@redhat.com>
> > > Cc: Kevin Tian <kevin.tian@intel.com>
> > > Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> > > ---
> > > Documentation/vfio-mediated-device.txt | 39 ++++++++++++++++++++++----
> > > 1 file changed, 33 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/Documentation/vfio-mediated-device.txt b/Documentation/vfio-mediated-device.txt
> > > index c3f69bcaf96e..9ec9495dcbe7 100644
> > > --- a/Documentation/vfio-mediated-device.txt
> > > +++ b/Documentation/vfio-mediated-device.txt
> > > @@ -211,12 +211,20 @@ Directories and files under the sysfs for Each Physical Device
> > > | | |--- description
> > > | | |--- [devices]
> > > | |--- [<type-id>]
> > > - | |--- create
> > > - | |--- name
> > > - | |--- available_instances
> > > - | |--- device_api
> > > - | |--- description
> > > - | |--- [devices]
> > > + | | |--- create
> > > + | | |--- name
> > > + | | |--- available_instances
> > > + | | |--- device_api
> > > + | | |--- description
> > > + | | |--- [devices]
> > > + | |--- [<type-id>]
> > > + | | |--- create
> > > + | | |--- name
> > > + | | |--- available_instances
> > > + | | |--- device_api
> > > + | | |--- description
> > > + | | |--- <aggregation>
> > > + | | |--- [devices]
> > >
> > > * [mdev_supported_types]
> > >
> > > @@ -260,6 +268,19 @@ Directories and files under the sysfs for Each Physical Device
> > > This attribute should show brief features/description of the type. This is
> > > optional attribute.
> > >
> > > +* <aggregation>
> > > +
> > > + The description is to show feature for one instance of the type. <aggregation>
> >
> > You are talking about "one instance" here. Can this be different for
> > the same type with different physical devices?
> >
>
> I would expect for normal mdev types, driver might expose like x2, x4, x8 types
> which split hw resource equally. But for type with aggregation feature, it can
> set user wanted number of instances. Sorry maybe my use of word was not clear, how
> about "one example of type"?
>
> > > + is an optional attributes to show that [<type-id>]'s instances can be
> > > + aggregated to be assigned for one mdev device. Set number of instances by
> > > + appending "instances=N" parameter for create. Instances number can't exceed
> > > + available_instances number. Without "instances=N" parameter will be default
> > > + one instance to create.
> >
> > Could there be a case where available_instances is n, but aggregation
> > is only supported for a value m < n? If yes, should m be discoverable
> > via the "aggregation" attribute?
> >
>
> I don't think there could be a case for that. As for aggregation type,
> it represents minimal resource allocated for a singleton, I can't see
> any reason for possible m < n.
Let's think about the interface beyond the immediate needs. It seems
perfectly reasonable to me to think that a vendor driver might have a
large number of resources available, the ability to aggregate resources
beyond what's practical to enumerate with discrete types, yet have an
upper bound of how many resources can be aggregated for a single
instance. Thanks,
Alex
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [libvirt] [PATCH v2 4/4] Documentation/vfio-mediated-device.txt: update for aggregation attribute
2018-07-27 2:16 ` Zhenyu Wang
2018-07-27 3:30 ` Alex Williamson
@ 2018-07-27 11:49 ` Cornelia Huck
1 sibling, 0 replies; 38+ messages in thread
From: Cornelia Huck @ 2018-07-27 11:49 UTC (permalink / raw)
To: Zhenyu Wang; +Cc: kevin.tian, kvm, libvirt-list, kwankhede, intel-gvt-dev
On Fri, 27 Jul 2018 10:16:58 +0800
Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> On 2018.07.26 17:46:40 +0200, Cornelia Huck wrote:
> > On Fri, 20 Jul 2018 10:19:28 +0800
> > Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> >
> > > Update mdev doc on new aggregration attribute and instances attribute
> > > for mdev.
> > >
> > > Cc: Kirti Wankhede <kwankhede@nvidia.com>
> > > Cc: Alex Williamson <alex.williamson@redhat.com>
> > > Cc: Kevin Tian <kevin.tian@intel.com>
> > > Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> > > ---
> > > Documentation/vfio-mediated-device.txt | 39 ++++++++++++++++++++++----
> > > 1 file changed, 33 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/Documentation/vfio-mediated-device.txt b/Documentation/vfio-mediated-device.txt
> > > index c3f69bcaf96e..9ec9495dcbe7 100644
> > > --- a/Documentation/vfio-mediated-device.txt
> > > +++ b/Documentation/vfio-mediated-device.txt
> > > @@ -211,12 +211,20 @@ Directories and files under the sysfs for Each Physical Device
> > > | | |--- description
> > > | | |--- [devices]
> > > | |--- [<type-id>]
> > > - | |--- create
> > > - | |--- name
> > > - | |--- available_instances
> > > - | |--- device_api
> > > - | |--- description
> > > - | |--- [devices]
> > > + | | |--- create
> > > + | | |--- name
> > > + | | |--- available_instances
> > > + | | |--- device_api
> > > + | | |--- description
> > > + | | |--- [devices]
> > > + | |--- [<type-id>]
> > > + | | |--- create
> > > + | | |--- name
> > > + | | |--- available_instances
> > > + | | |--- device_api
> > > + | | |--- description
> > > + | | |--- <aggregation>
> > > + | | |--- [devices]
> > >
> > > * [mdev_supported_types]
> > >
> > > @@ -260,6 +268,19 @@ Directories and files under the sysfs for Each Physical Device
> > > This attribute should show brief features/description of the type. This is
> > > optional attribute.
> > >
> > > +* <aggregation>
> > > +
> > > + The description is to show feature for one instance of the type. <aggregation>
> >
> > You are talking about "one instance" here. Can this be different for
> > the same type with different physical devices?
> >
>
> I would expect for normal mdev types, driver might expose like x2, x4, x8 types
> which split hw resource equally. But for type with aggregation feature, it can
> set user wanted number of instances. Sorry maybe my use of word was not clear, how
> about "one example of type"?
Maybe my question was confusing as well...
<aggregation> is an attribute that is exposed for a particular type
under a particular physical device.
- If <aggregation> is always the same for that particular type,
regardless of which physical device we're dealing with, let's just
drop the "one instance" sentence.
- If it instead depends on what physical device we're handling, I'd
write something like "The contents of this attribute depend both on
the type and on the particular instance."
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources
2018-07-20 2:19 ` [libvirt] [PATCH v2 0/4] New mdev type handling " Zhenyu Wang
` (3 preceding siblings ...)
2018-07-20 2:19 ` [libvirt] [PATCH v2 4/4] Documentation/vfio-mediated-device.txt: update for aggregation attribute Zhenyu Wang
@ 2018-07-24 17:44 ` Alex Williamson
2018-07-26 13:50 ` Erik Skultety
2018-07-27 2:01 ` Zhenyu Wang
2018-10-08 3:19 ` Tian, Kevin
` (7 subsequent siblings)
12 siblings, 2 replies; 38+ messages in thread
From: Alex Williamson @ 2018-07-24 17:44 UTC (permalink / raw)
To: Zhenyu Wang; +Cc: kevin.tian, kwankhede, intel-gvt-dev, libvirt-list, kvm
On Fri, 20 Jul 2018 10:19:24 +0800
Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> Current mdev device create interface depends on fixed mdev type, which get uuid
> from user to create instance of mdev device. If user wants to use customized
> number of resource for mdev device, then only can create new mdev type for that
> which may not be flexible. This requirement comes not only from to be able to
> allocate flexible resources for KVMGT, but also from Intel scalable IO
> virtualization which would use vfio/mdev to be able to allocate arbitrary
> resources on mdev instance. More info on [1] [2] [3].
>
> To allow to create user defined resources for mdev, it trys to extend mdev
> create interface by adding new "instances=xxx" parameter following uuid, for
> target mdev type if aggregation is supported, it can create new mdev device
> which contains resources combined by number of instances, e.g
>
> echo "<uuid>,instances=10" > create
>
> VM manager e.g libvirt can check mdev type with "aggregation" attribute which
> can support this setting. If no "aggregation" attribute found for mdev type,
> previous behavior is still kept for one instance allocation. And new sysfs
> attribute "instances" is created for each mdev device to show allocated number.
>
> This trys to create new KVMGT type with minimal vGPU resources which can be
> combined with "instances=x" setting to allocate for user wanted resources.
"instances" makes me think this is arg helps to create multiple mdev
instances rather than consuming multiple instances for a single mdev.
You're already exposing the "aggregation" attribute, so doesn't
"aggregate" perhaps make more sense as the create option? We're asking
the driver to aggregate $NUM instances into a single mdev. The mdev
attribute should then perhaps also be "aggregated_instances".
The next user question for the interface might be what aspect of the
device gets multiplied by this aggregation? In i915 I see you're
multiplying the memory sizes by the instance, but clearly the
resolution doesn't change. I assume this is sort of like mdev types
themselves, ie. some degree of what a type means is buried in the
implementation and some degree of what some number of those types
aggregated together means is impossible to describe generically.
We're also going to need to add aggregation to the checklist for device
compatibility for migration, for example 1) is it the same mdev_type,
1a) are the aggregation counts the same (new), 2) is the host driver
compatible (TBD).
The count handling in create_store(), particularly MDEV_CREATE_OPT_LEN
looks a little suspicious. I think we should just be validating that
the string before the ',' or the entire string if there is no comma is
UUID length. Pass only that to uuid_le_to_bin(). We can then strncmp
as you have for "instances=" (or "aggregate=") but then let's be sure
to end the string we pass to kstrtouint(), ie. assume there could be
further args.
Documentation/ABI/testing/sysfs-bus-vfio-mdev also needs to be updated
with this series.
I'm curious what libvirt folks and Kirti think of this, it looks like
it has a nice degree of backwards compatibility, both in the sysfs
interface and the vendor driver interface. Thanks,
Alex
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources
2018-07-24 17:44 ` [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources Alex Williamson
@ 2018-07-26 13:50 ` Erik Skultety
2018-07-26 14:29 ` Alex Williamson
2018-07-26 15:30 ` Cornelia Huck
2018-07-27 2:01 ` Zhenyu Wang
1 sibling, 2 replies; 38+ messages in thread
From: Erik Skultety @ 2018-07-26 13:50 UTC (permalink / raw)
To: Alex Williamson
Cc: kevin.tian, kvm, libvirt-list, Zhenyu Wang, kwankhede, intel-gvt-dev
On Tue, Jul 24, 2018 at 11:44:40AM -0600, Alex Williamson wrote:
> On Fri, 20 Jul 2018 10:19:24 +0800
> Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
>
> > Current mdev device create interface depends on fixed mdev type, which get uuid
> > from user to create instance of mdev device. If user wants to use customized
> > number of resource for mdev device, then only can create new mdev type for that
> > which may not be flexible. This requirement comes not only from to be able to
> > allocate flexible resources for KVMGT, but also from Intel scalable IO
> > virtualization which would use vfio/mdev to be able to allocate arbitrary
> > resources on mdev instance. More info on [1] [2] [3].
> >
> > To allow to create user defined resources for mdev, it trys to extend mdev
> > create interface by adding new "instances=xxx" parameter following uuid, for
> > target mdev type if aggregation is supported, it can create new mdev device
> > which contains resources combined by number of instances, e.g
> >
> > echo "<uuid>,instances=10" > create
> >
> > VM manager e.g libvirt can check mdev type with "aggregation" attribute which
> > can support this setting. If no "aggregation" attribute found for mdev type,
> > previous behavior is still kept for one instance allocation. And new sysfs
> > attribute "instances" is created for each mdev device to show allocated number.
> >
> > This trys to create new KVMGT type with minimal vGPU resources which can be
> > combined with "instances=x" setting to allocate for user wanted resources.
>
> "instances" makes me think this is arg helps to create multiple mdev
> instances rather than consuming multiple instances for a single mdev.
> You're already exposing the "aggregation" attribute, so doesn't
> "aggregate" perhaps make more sense as the create option? We're asking
> the driver to aggregate $NUM instances into a single mdev. The mdev
> attribute should then perhaps also be "aggregated_instances".
>
> The next user question for the interface might be what aspect of the
> device gets multiplied by this aggregation? In i915 I see you're
> multiplying the memory sizes by the instance, but clearly the
> resolution doesn't change. I assume this is sort of like mdev types
> themselves, ie. some degree of what a type means is buried in the
> implementation and some degree of what some number of those types
> aggregated together means is impossible to describe generically.
I don't seem to clearly see the benefit here, so I have to ask, how is this
better and/or different from allowing a heterogeneous setup if one needs a more
performant instance in terms of more resources? Because to me, once you're able
to aggregate instances, I would assume that a simple "echo `uuid`" with a
different type should succeed as well and provide me (from user's perspective)
with the same results. Could you please clarify this to me, as well as what
resources/parameters are going to be impacted by aggregation?
...
>
> I'm curious what libvirt folks and Kirti think of this, it looks like
> it has a nice degree of backwards compatibility, both in the sysfs
> interface and the vendor driver interface. Thanks,
Since libvirt doesn't have an API to create mdevs yet, this doesn't pose an
issue for us at the moment. I see this adds new optional sysfs attributes which
we could expose within our device capabilities XML, provided it doesn't use a
free form text, like the description attribute does.
Erik
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources
2018-07-26 13:50 ` Erik Skultety
@ 2018-07-26 14:29 ` Alex Williamson
2018-07-26 16:00 ` Erik Skultety
2018-07-26 15:30 ` Cornelia Huck
1 sibling, 1 reply; 38+ messages in thread
From: Alex Williamson @ 2018-07-26 14:29 UTC (permalink / raw)
To: Erik Skultety
Cc: kevin.tian, kvm, libvirt-list, Zhenyu Wang, kwankhede, intel-gvt-dev
On Thu, 26 Jul 2018 15:50:28 +0200
Erik Skultety <eskultet@redhat.com> wrote:
> On Tue, Jul 24, 2018 at 11:44:40AM -0600, Alex Williamson wrote:
> > On Fri, 20 Jul 2018 10:19:24 +0800
> > Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> >
> > > Current mdev device create interface depends on fixed mdev type, which get uuid
> > > from user to create instance of mdev device. If user wants to use customized
> > > number of resource for mdev device, then only can create new mdev type for that
> > > which may not be flexible. This requirement comes not only from to be able to
> > > allocate flexible resources for KVMGT, but also from Intel scalable IO
> > > virtualization which would use vfio/mdev to be able to allocate arbitrary
> > > resources on mdev instance. More info on [1] [2] [3].
> > >
> > > To allow to create user defined resources for mdev, it trys to extend mdev
> > > create interface by adding new "instances=xxx" parameter following uuid, for
> > > target mdev type if aggregation is supported, it can create new mdev device
> > > which contains resources combined by number of instances, e.g
> > >
> > > echo "<uuid>,instances=10" > create
> > >
> > > VM manager e.g libvirt can check mdev type with "aggregation" attribute which
> > > can support this setting. If no "aggregation" attribute found for mdev type,
> > > previous behavior is still kept for one instance allocation. And new sysfs
> > > attribute "instances" is created for each mdev device to show allocated number.
> > >
> > > This trys to create new KVMGT type with minimal vGPU resources which can be
> > > combined with "instances=x" setting to allocate for user wanted resources.
> >
> > "instances" makes me think this is arg helps to create multiple mdev
> > instances rather than consuming multiple instances for a single mdev.
> > You're already exposing the "aggregation" attribute, so doesn't
> > "aggregate" perhaps make more sense as the create option? We're asking
> > the driver to aggregate $NUM instances into a single mdev. The mdev
> > attribute should then perhaps also be "aggregated_instances".
> >
> > The next user question for the interface might be what aspect of the
> > device gets multiplied by this aggregation? In i915 I see you're
> > multiplying the memory sizes by the instance, but clearly the
> > resolution doesn't change. I assume this is sort of like mdev types
> > themselves, ie. some degree of what a type means is buried in the
> > implementation and some degree of what some number of those types
> > aggregated together means is impossible to describe generically.
>
> I don't seem to clearly see the benefit here, so I have to ask, how is this
> better and/or different from allowing a heterogeneous setup if one needs a more
> performant instance in terms of more resources? Because to me, once you're able
> to aggregate instances, I would assume that a simple "echo `uuid`" with a
> different type should succeed as well and provide me (from user's perspective)
> with the same results. Could you please clarify this to me, as well as what
> resources/parameters are going to be impacted by aggregation?
I think you're suggesting that we could simply define new mdev types to
account for these higher aggregate instances, for example we can
define discrete types that are 2x, 3x, 4x, etc. the resource count of a
single instance. What I think we're trying to address with this
proposal is what happens when the resources available are exceptionally
large and they can be combined in arbitrary ways. For example if a
parent device can expose 10,000 resources and the granularity with
which we can create and mdev instance is 1, is it practical to create
10,000 mdev types or does it make more sense to expose a granularity
and method of aggregation.
Using graphics here perhaps falls a little short of the intention of
the interface because the possible types are easily enumerable and it
would be entirely practical to create discrete types for each. vGPUs
also have a lot of variables, so defining which attribute of the device
is multiplied by the number of instances is a little more fuzzy.
Thanks,
Alex
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources
2018-07-26 14:29 ` Alex Williamson
@ 2018-07-26 16:00 ` Erik Skultety
0 siblings, 0 replies; 38+ messages in thread
From: Erik Skultety @ 2018-07-26 16:00 UTC (permalink / raw)
To: Alex Williamson
Cc: kevin.tian, kvm, libvirt-list, Zhenyu Wang, kwankhede, intel-gvt-dev
On Thu, Jul 26, 2018 at 08:29:45AM -0600, Alex Williamson wrote:
> On Thu, 26 Jul 2018 15:50:28 +0200
> Erik Skultety <eskultet@redhat.com> wrote:
>
> > On Tue, Jul 24, 2018 at 11:44:40AM -0600, Alex Williamson wrote:
> > > On Fri, 20 Jul 2018 10:19:24 +0800
> > > Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> > >
> > > > Current mdev device create interface depends on fixed mdev type, which get uuid
> > > > from user to create instance of mdev device. If user wants to use customized
> > > > number of resource for mdev device, then only can create new mdev type for that
> > > > which may not be flexible. This requirement comes not only from to be able to
> > > > allocate flexible resources for KVMGT, but also from Intel scalable IO
> > > > virtualization which would use vfio/mdev to be able to allocate arbitrary
> > > > resources on mdev instance. More info on [1] [2] [3].
> > > >
> > > > To allow to create user defined resources for mdev, it trys to extend mdev
> > > > create interface by adding new "instances=xxx" parameter following uuid, for
> > > > target mdev type if aggregation is supported, it can create new mdev device
> > > > which contains resources combined by number of instances, e.g
> > > >
> > > > echo "<uuid>,instances=10" > create
> > > >
> > > > VM manager e.g libvirt can check mdev type with "aggregation" attribute which
> > > > can support this setting. If no "aggregation" attribute found for mdev type,
> > > > previous behavior is still kept for one instance allocation. And new sysfs
> > > > attribute "instances" is created for each mdev device to show allocated number.
> > > >
> > > > This trys to create new KVMGT type with minimal vGPU resources which can be
> > > > combined with "instances=x" setting to allocate for user wanted resources.
> > >
> > > "instances" makes me think this is arg helps to create multiple mdev
> > > instances rather than consuming multiple instances for a single mdev.
> > > You're already exposing the "aggregation" attribute, so doesn't
> > > "aggregate" perhaps make more sense as the create option? We're asking
> > > the driver to aggregate $NUM instances into a single mdev. The mdev
> > > attribute should then perhaps also be "aggregated_instances".
> > >
> > > The next user question for the interface might be what aspect of the
> > > device gets multiplied by this aggregation? In i915 I see you're
> > > multiplying the memory sizes by the instance, but clearly the
> > > resolution doesn't change. I assume this is sort of like mdev types
> > > themselves, ie. some degree of what a type means is buried in the
> > > implementation and some degree of what some number of those types
> > > aggregated together means is impossible to describe generically.
> >
> > I don't seem to clearly see the benefit here, so I have to ask, how is this
> > better and/or different from allowing a heterogeneous setup if one needs a more
> > performant instance in terms of more resources? Because to me, once you're able
> > to aggregate instances, I would assume that a simple "echo `uuid`" with a
> > different type should succeed as well and provide me (from user's perspective)
> > with the same results. Could you please clarify this to me, as well as what
> > resources/parameters are going to be impacted by aggregation?
>
> I think you're suggesting that we could simply define new mdev types to
> account for these higher aggregate instances, for example we can
> define discrete types that are 2x, 3x, 4x, etc. the resource count of a
> single instance. What I think we're trying to address with this
> proposal is what happens when the resources available are exceptionally
> large and they can be combined in arbitrary ways. For example if a
> parent device can expose 10,000 resources and the granularity with
> which we can create and mdev instance is 1, is it practical to create
> 10,000 mdev types or does it make more sense to expose a granularity
> and method of aggregation.
Okay, I got a much better picture now, thanks for the clarification.
The granularity you mentioned definitely does give users more power and control
(in terms of provisioning) over the devices.
Erik
>
> Using graphics here perhaps falls a little short of the intention of
> the interface because the possible types are easily enumerable and it
> would be entirely practical to create discrete types for each. vGPUs
> also have a lot of variables, so defining which attribute of the device
> is multiplied by the number of instances is a little more fuzzy.
> Thanks,
>
> Alex
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources
2018-07-26 13:50 ` Erik Skultety
2018-07-26 14:29 ` Alex Williamson
@ 2018-07-26 15:30 ` Cornelia Huck
2018-07-26 15:43 ` Erik Skultety
2018-07-26 15:51 ` Alex Williamson
1 sibling, 2 replies; 38+ messages in thread
From: Cornelia Huck @ 2018-07-26 15:30 UTC (permalink / raw)
To: Erik Skultety
Cc: kevin.tian, kvm, libvirt-list, Zhenyu Wang, kwankhede, intel-gvt-dev
On Thu, 26 Jul 2018 15:50:28 +0200
Erik Skultety <eskultet@redhat.com> wrote:
> On Tue, Jul 24, 2018 at 11:44:40AM -0600, Alex Williamson wrote:
> > On Fri, 20 Jul 2018 10:19:24 +0800
> > Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> >
> > > Current mdev device create interface depends on fixed mdev type, which get uuid
> > > from user to create instance of mdev device. If user wants to use customized
> > > number of resource for mdev device, then only can create new mdev type for that
> > > which may not be flexible. This requirement comes not only from to be able to
> > > allocate flexible resources for KVMGT, but also from Intel scalable IO
> > > virtualization which would use vfio/mdev to be able to allocate arbitrary
> > > resources on mdev instance. More info on [1] [2] [3].
> > >
> > > To allow to create user defined resources for mdev, it trys to extend mdev
> > > create interface by adding new "instances=xxx" parameter following uuid, for
> > > target mdev type if aggregation is supported, it can create new mdev device
> > > which contains resources combined by number of instances, e.g
> > >
> > > echo "<uuid>,instances=10" > create
> > >
> > > VM manager e.g libvirt can check mdev type with "aggregation" attribute which
> > > can support this setting. If no "aggregation" attribute found for mdev type,
> > > previous behavior is still kept for one instance allocation. And new sysfs
> > > attribute "instances" is created for each mdev device to show allocated number.
> > >
> > > This trys to create new KVMGT type with minimal vGPU resources which can be
> > > combined with "instances=x" setting to allocate for user wanted resources.
> >
> > "instances" makes me think this is arg helps to create multiple mdev
> > instances rather than consuming multiple instances for a single mdev.
> > You're already exposing the "aggregation" attribute, so doesn't
> > "aggregate" perhaps make more sense as the create option? We're asking
> > the driver to aggregate $NUM instances into a single mdev. The mdev
> > attribute should then perhaps also be "aggregated_instances".
I agree, that seems like a better naming scheme.
(...)
> > I'm curious what libvirt folks and Kirti think of this, it looks like
> > it has a nice degree of backwards compatibility, both in the sysfs
> > interface and the vendor driver interface. Thanks,
>
> Since libvirt doesn't have an API to create mdevs yet, this doesn't pose an
> issue for us at the moment. I see this adds new optional sysfs attributes which
> we could expose within our device capabilities XML, provided it doesn't use a
> free form text, like the description attribute does.
One thing I noticed is that we have seem to have an optional (?)
vendor-driver created "aggregation" attribute (which always prints
"true" in the Intel driver). Would it be better or worse for libvirt if
it contained some kind of upper boundary or so? Additionally, would it
be easier if the "create" attribute always accepted
",instances=1" (calling the .create ops in that case?)
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources
2018-07-26 15:30 ` Cornelia Huck
@ 2018-07-26 15:43 ` Erik Skultety
2018-07-26 16:04 ` Cornelia Huck
2018-07-26 15:51 ` Alex Williamson
1 sibling, 1 reply; 38+ messages in thread
From: Erik Skultety @ 2018-07-26 15:43 UTC (permalink / raw)
To: Cornelia Huck
Cc: kevin.tian, kvm, libvirt-list, Zhenyu Wang, kwankhede, intel-gvt-dev
On Thu, Jul 26, 2018 at 05:30:07PM +0200, Cornelia Huck wrote:
> On Thu, 26 Jul 2018 15:50:28 +0200
> Erik Skultety <eskultet@redhat.com> wrote:
>
> > On Tue, Jul 24, 2018 at 11:44:40AM -0600, Alex Williamson wrote:
> > > On Fri, 20 Jul 2018 10:19:24 +0800
> > > Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> > >
> > > > Current mdev device create interface depends on fixed mdev type, which get uuid
> > > > from user to create instance of mdev device. If user wants to use customized
> > > > number of resource for mdev device, then only can create new mdev type for that
> > > > which may not be flexible. This requirement comes not only from to be able to
> > > > allocate flexible resources for KVMGT, but also from Intel scalable IO
> > > > virtualization which would use vfio/mdev to be able to allocate arbitrary
> > > > resources on mdev instance. More info on [1] [2] [3].
> > > >
> > > > To allow to create user defined resources for mdev, it trys to extend mdev
> > > > create interface by adding new "instances=xxx" parameter following uuid, for
> > > > target mdev type if aggregation is supported, it can create new mdev device
> > > > which contains resources combined by number of instances, e.g
> > > >
> > > > echo "<uuid>,instances=10" > create
> > > >
> > > > VM manager e.g libvirt can check mdev type with "aggregation" attribute which
> > > > can support this setting. If no "aggregation" attribute found for mdev type,
> > > > previous behavior is still kept for one instance allocation. And new sysfs
> > > > attribute "instances" is created for each mdev device to show allocated number.
> > > >
> > > > This trys to create new KVMGT type with minimal vGPU resources which can be
> > > > combined with "instances=x" setting to allocate for user wanted resources.
> > >
> > > "instances" makes me think this is arg helps to create multiple mdev
> > > instances rather than consuming multiple instances for a single mdev.
> > > You're already exposing the "aggregation" attribute, so doesn't
> > > "aggregate" perhaps make more sense as the create option? We're asking
> > > the driver to aggregate $NUM instances into a single mdev. The mdev
> > > attribute should then perhaps also be "aggregated_instances".
>
> I agree, that seems like a better naming scheme.
>
> (...)
>
> > > I'm curious what libvirt folks and Kirti think of this, it looks like
> > > it has a nice degree of backwards compatibility, both in the sysfs
> > > interface and the vendor driver interface. Thanks,
> >
> > Since libvirt doesn't have an API to create mdevs yet, this doesn't pose an
> > issue for us at the moment. I see this adds new optional sysfs attributes which
> > we could expose within our device capabilities XML, provided it doesn't use a
> > free form text, like the description attribute does.
>
> One thing I noticed is that we have seem to have an optional (?)
> vendor-driver created "aggregation" attribute (which always prints
> "true" in the Intel driver). Would it be better or worse for libvirt if
> it contained some kind of upper boundary or so? Additionally, would it
Can you be more specific? Although, I wouldn't argue against data that conveys
some information, since I would treat the mere presence of the optional
attribute as a supported feature that we can expose. Therefore, additional
*structured* data which sets clear limits to a certain feature is only a plus
that we can expose to the users/management layer.
> be easier if the "create" attribute always accepted
> ",instances=1" (calling the .create ops in that case?)
Is ^this libvirt related question? If so, then by accepting such syntax you'll
definitely save us a few lines of code ;). However, until we have a clear
vision on how to tackle the mdev migration, I'd like to avoid proposing a
libvirt "create" API until then.
Erik
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources
2018-07-26 15:43 ` Erik Skultety
@ 2018-07-26 16:04 ` Cornelia Huck
2018-07-27 14:45 ` Erik Skultety
0 siblings, 1 reply; 38+ messages in thread
From: Cornelia Huck @ 2018-07-26 16:04 UTC (permalink / raw)
To: Erik Skultety
Cc: kevin.tian, kvm, libvirt-list, Zhenyu Wang, kwankhede, intel-gvt-dev
On Thu, 26 Jul 2018 17:43:45 +0200
Erik Skultety <eskultet@redhat.com> wrote:
> On Thu, Jul 26, 2018 at 05:30:07PM +0200, Cornelia Huck wrote:
> > One thing I noticed is that we have seem to have an optional (?)
> > vendor-driver created "aggregation" attribute (which always prints
> > "true" in the Intel driver). Would it be better or worse for libvirt if
> > it contained some kind of upper boundary or so? Additionally, would it
>
> Can you be more specific? Although, I wouldn't argue against data that conveys
> some information, since I would treat the mere presence of the optional
> attribute as a supported feature that we can expose. Therefore, additional
> *structured* data which sets clear limits to a certain feature is only a plus
> that we can expose to the users/management layer.
My question is what would be easiest for libvirt:
- "aggregation" attribute only present when driver supports aggregation
(possibly containing max number of resources to be aggregated)
- "aggregation" attribute always present; contains '1' if driver does
not support aggregation and 'm' if driver can aggregate 'm' resources
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources
2018-07-26 16:04 ` Cornelia Huck
@ 2018-07-27 14:45 ` Erik Skultety
2018-07-30 2:11 ` Zhenyu Wang
0 siblings, 1 reply; 38+ messages in thread
From: Erik Skultety @ 2018-07-27 14:45 UTC (permalink / raw)
To: Cornelia Huck
Cc: kevin.tian, kvm, libvirt-list, Zhenyu Wang, kwankhede, intel-gvt-dev
On Thu, Jul 26, 2018 at 06:04:10PM +0200, Cornelia Huck wrote:
> On Thu, 26 Jul 2018 17:43:45 +0200
> Erik Skultety <eskultet@redhat.com> wrote:
>
> > On Thu, Jul 26, 2018 at 05:30:07PM +0200, Cornelia Huck wrote:
> > > One thing I noticed is that we have seem to have an optional (?)
> > > vendor-driver created "aggregation" attribute (which always prints
> > > "true" in the Intel driver). Would it be better or worse for libvirt if
> > > it contained some kind of upper boundary or so? Additionally, would it
> >
> > Can you be more specific? Although, I wouldn't argue against data that conveys
> > some information, since I would treat the mere presence of the optional
> > attribute as a supported feature that we can expose. Therefore, additional
> > *structured* data which sets clear limits to a certain feature is only a plus
> > that we can expose to the users/management layer.
>
> My question is what would be easiest for libvirt:
>
> - "aggregation" attribute only present when driver supports aggregation
> (possibly containing max number of resources to be aggregated)
> - "aggregation" attribute always present; contains '1' if driver does
> not support aggregation and 'm' if driver can aggregate 'm' resources
Both are fine from libvirt's POV, but IMHO the former makes a bit more sense
and I'm in favour of that one, IOW the presence of an attribute denotes a new
functionality which we can report, if it's missing, the feature is clearly
lacking- I don't think we (libvirt) should be reporting the value 1 explicitly
in the XML if the feature is missing, we can assume 1 as the default.
Erik
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources
2018-07-27 14:45 ` Erik Skultety
@ 2018-07-30 2:11 ` Zhenyu Wang
0 siblings, 0 replies; 38+ messages in thread
From: Zhenyu Wang @ 2018-07-30 2:11 UTC (permalink / raw)
To: Erik Skultety
Cc: kevin.tian, kvm, Cornelia Huck, kwankhede, Zhenyu Wang,
libvirt-list, intel-gvt-dev
[-- Attachment #1.1: Type: text/plain, Size: 1898 bytes --]
On 2018.07.27 16:45:55 +0200, Erik Skultety wrote:
> On Thu, Jul 26, 2018 at 06:04:10PM +0200, Cornelia Huck wrote:
> > On Thu, 26 Jul 2018 17:43:45 +0200
> > Erik Skultety <eskultet@redhat.com> wrote:
> >
> > > On Thu, Jul 26, 2018 at 05:30:07PM +0200, Cornelia Huck wrote:
> > > > One thing I noticed is that we have seem to have an optional (?)
> > > > vendor-driver created "aggregation" attribute (which always prints
> > > > "true" in the Intel driver). Would it be better or worse for libvirt if
> > > > it contained some kind of upper boundary or so? Additionally, would it
> > >
> > > Can you be more specific? Although, I wouldn't argue against data that conveys
> > > some information, since I would treat the mere presence of the optional
> > > attribute as a supported feature that we can expose. Therefore, additional
> > > *structured* data which sets clear limits to a certain feature is only a plus
> > > that we can expose to the users/management layer.
> >
> > My question is what would be easiest for libvirt:
> >
> > - "aggregation" attribute only present when driver supports aggregation
> > (possibly containing max number of resources to be aggregated)
> > - "aggregation" attribute always present; contains '1' if driver does
> > not support aggregation and 'm' if driver can aggregate 'm' resources
>
> Both are fine from libvirt's POV, but IMHO the former makes a bit more sense
> and I'm in favour of that one, IOW the presence of an attribute denotes a new
> functionality which we can report, if it's missing, the feature is clearly
> lacking- I don't think we (libvirt) should be reporting the value 1 explicitly
> in the XML if the feature is missing, we can assume 1 as the default.
>
Good I'll adhere to that, thanks!
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources
2018-07-26 15:30 ` Cornelia Huck
2018-07-26 15:43 ` Erik Skultety
@ 2018-07-26 15:51 ` Alex Williamson
2018-07-26 15:59 ` Cornelia Huck
1 sibling, 1 reply; 38+ messages in thread
From: Alex Williamson @ 2018-07-26 15:51 UTC (permalink / raw)
To: Cornelia Huck
Cc: kevin.tian, kvm, Erik Skultety, libvirt-list, Zhenyu Wang,
kwankhede, intel-gvt-dev
On Thu, 26 Jul 2018 17:30:07 +0200
Cornelia Huck <cohuck@redhat.com> wrote:
> On Thu, 26 Jul 2018 15:50:28 +0200
> Erik Skultety <eskultet@redhat.com> wrote:
>
> > On Tue, Jul 24, 2018 at 11:44:40AM -0600, Alex Williamson wrote:
> > > On Fri, 20 Jul 2018 10:19:24 +0800
> > > Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> > >
> > > > Current mdev device create interface depends on fixed mdev type, which get uuid
> > > > from user to create instance of mdev device. If user wants to use customized
> > > > number of resource for mdev device, then only can create new mdev type for that
> > > > which may not be flexible. This requirement comes not only from to be able to
> > > > allocate flexible resources for KVMGT, but also from Intel scalable IO
> > > > virtualization which would use vfio/mdev to be able to allocate arbitrary
> > > > resources on mdev instance. More info on [1] [2] [3].
> > > >
> > > > To allow to create user defined resources for mdev, it trys to extend mdev
> > > > create interface by adding new "instances=xxx" parameter following uuid, for
> > > > target mdev type if aggregation is supported, it can create new mdev device
> > > > which contains resources combined by number of instances, e.g
> > > >
> > > > echo "<uuid>,instances=10" > create
> > > >
> > > > VM manager e.g libvirt can check mdev type with "aggregation" attribute which
> > > > can support this setting. If no "aggregation" attribute found for mdev type,
> > > > previous behavior is still kept for one instance allocation. And new sysfs
> > > > attribute "instances" is created for each mdev device to show allocated number.
> > > >
> > > > This trys to create new KVMGT type with minimal vGPU resources which can be
> > > > combined with "instances=x" setting to allocate for user wanted resources.
> > >
> > > "instances" makes me think this is arg helps to create multiple mdev
> > > instances rather than consuming multiple instances for a single mdev.
> > > You're already exposing the "aggregation" attribute, so doesn't
> > > "aggregate" perhaps make more sense as the create option? We're asking
> > > the driver to aggregate $NUM instances into a single mdev. The mdev
> > > attribute should then perhaps also be "aggregated_instances".
>
> I agree, that seems like a better naming scheme.
>
> (...)
>
> > > I'm curious what libvirt folks and Kirti think of this, it looks like
> > > it has a nice degree of backwards compatibility, both in the sysfs
> > > interface and the vendor driver interface. Thanks,
> >
> > Since libvirt doesn't have an API to create mdevs yet, this doesn't pose an
> > issue for us at the moment. I see this adds new optional sysfs attributes which
> > we could expose within our device capabilities XML, provided it doesn't use a
> > free form text, like the description attribute does.
>
> One thing I noticed is that we have seem to have an optional (?)
> vendor-driver created "aggregation" attribute (which always prints
> "true" in the Intel driver). Would it be better or worse for libvirt if
> it contained some kind of upper boundary or so?
Ultimately the aggregation value should be fully specified in
Documentation/ABI, but doesn't the kernel generally use 'Y' or 'N' for
boolean attributes in sysfs? Maybe mdev core can handle the attribute
since it should exist any time the create_with_instances callback is
provided.
> Additionally, would it
> be easier if the "create" attribute always accepted
> ",instances=1" (calling the .create ops in that case?)
Unless I misunderstand the code or the question, I think this is
exactly what happens:
+ if (instances > 1)
+ ret = parent->ops->create_with_instances(kobj, mdev, instances);
+ else
+ ret = parent->ops->create(kobj, mdev);
And elsewhere:
+ if (instances > 1 && !parent->ops->create_with_instances) {
+ ret = -EINVAL;
+ goto mdev_fail;
+ }
If the mdev core supplied the aggregation attribute, then the presence
of the attribute could indicate to userspace whether it can always
provide an instance (aggregate) count, even if limited to '1' when 'N',
for that mdev type. Thanks,
Alex
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources
2018-07-26 15:51 ` Alex Williamson
@ 2018-07-26 15:59 ` Cornelia Huck
0 siblings, 0 replies; 38+ messages in thread
From: Cornelia Huck @ 2018-07-26 15:59 UTC (permalink / raw)
To: Alex Williamson
Cc: kevin.tian, kvm, Erik Skultety, libvirt-list, Zhenyu Wang,
kwankhede, intel-gvt-dev
On Thu, 26 Jul 2018 09:51:26 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:
> On Thu, 26 Jul 2018 17:30:07 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
>
> > On Thu, 26 Jul 2018 15:50:28 +0200
> > Erik Skultety <eskultet@redhat.com> wrote:
> > > Since libvirt doesn't have an API to create mdevs yet, this doesn't pose an
> > > issue for us at the moment. I see this adds new optional sysfs attributes which
> > > we could expose within our device capabilities XML, provided it doesn't use a
> > > free form text, like the description attribute does.
> >
> > One thing I noticed is that we have seem to have an optional (?)
> > vendor-driver created "aggregation" attribute (which always prints
> > "true" in the Intel driver). Would it be better or worse for libvirt if
> > it contained some kind of upper boundary or so?
>
> Ultimately the aggregation value should be fully specified in
> Documentation/ABI, but doesn't the kernel generally use 'Y' or 'N' for
> boolean attributes in sysfs? Maybe mdev core can handle the attribute
> since it should exist any time the create_with_instances callback is
> provided.
It might make sense to print a number if the driver allows a number of
resources to be aggregated which is not the same as available_instances
(see my reply to the documentation patch).
>
> > Additionally, would it
> > be easier if the "create" attribute always accepted
> > ",instances=1" (calling the .create ops in that case?)
>
> Unless I misunderstand the code or the question, I think this is
> exactly what happens:
Indeed, it does. Let me blame the weather ;)
> If the mdev core supplied the aggregation attribute, then the presence
> of the attribute could indicate to userspace whether it can always
> provide an instance (aggregate) count, even if limited to '1' when 'N',
> for that mdev type. Thanks,
>
> Alex
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources
2018-07-24 17:44 ` [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources Alex Williamson
2018-07-26 13:50 ` Erik Skultety
@ 2018-07-27 2:01 ` Zhenyu Wang
1 sibling, 0 replies; 38+ messages in thread
From: Zhenyu Wang @ 2018-07-27 2:01 UTC (permalink / raw)
To: Alex Williamson
Cc: kevin.tian, kvm, libvirt-list, kwankhede, intel-gvt-dev, Zhi Wang
[-- Attachment #1.1: Type: text/plain, Size: 4052 bytes --]
On 2018.07.24 11:44:40 -0600, Alex Williamson wrote:
> On Fri, 20 Jul 2018 10:19:24 +0800
> Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
>
> > Current mdev device create interface depends on fixed mdev type, which get uuid
> > from user to create instance of mdev device. If user wants to use customized
> > number of resource for mdev device, then only can create new mdev type for that
> > which may not be flexible. This requirement comes not only from to be able to
> > allocate flexible resources for KVMGT, but also from Intel scalable IO
> > virtualization which would use vfio/mdev to be able to allocate arbitrary
> > resources on mdev instance. More info on [1] [2] [3].
> >
> > To allow to create user defined resources for mdev, it trys to extend mdev
> > create interface by adding new "instances=xxx" parameter following uuid, for
> > target mdev type if aggregation is supported, it can create new mdev device
> > which contains resources combined by number of instances, e.g
> >
> > echo "<uuid>,instances=10" > create
> >
> > VM manager e.g libvirt can check mdev type with "aggregation" attribute which
> > can support this setting. If no "aggregation" attribute found for mdev type,
> > previous behavior is still kept for one instance allocation. And new sysfs
> > attribute "instances" is created for each mdev device to show allocated number.
> >
> > This trys to create new KVMGT type with minimal vGPU resources which can be
> > combined with "instances=x" setting to allocate for user wanted resources.
>
> "instances" makes me think this is arg helps to create multiple mdev
> instances rather than consuming multiple instances for a single mdev.
> You're already exposing the "aggregation" attribute, so doesn't
> "aggregate" perhaps make more sense as the create option? We're asking
> the driver to aggregate $NUM instances into a single mdev. The mdev
> attribute should then perhaps also be "aggregated_instances".
yeah that seems better.
>
> The next user question for the interface might be what aspect of the
> device gets multiplied by this aggregation? In i915 I see you're
> multiplying the memory sizes by the instance, but clearly the
> resolution doesn't change. I assume this is sort of like mdev types
> themselves, ie. some degree of what a type means is buried in the
> implementation and some degree of what some number of those types
> aggregated together means is impossible to describe generically.
>
yeah, the purpose was to increase memory resource only, but due to current
free formatted 'description', can't have a meaningful expression for that,
and not sure if libvirt likes to understand vendor specific behavior e.g
for intel vGPU?
> We're also going to need to add aggregation to the checklist for device
> compatibility for migration, for example 1) is it the same mdev_type,
> 1a) are the aggregation counts the same (new), 2) is the host driver
> compatible (TBD).
>
Right, will check with Zhi on that.
> The count handling in create_store(), particularly MDEV_CREATE_OPT_LEN
> looks a little suspicious. I think we should just be validating that
> the string before the ',' or the entire string if there is no comma is
> UUID length. Pass only that to uuid_le_to_bin(). We can then strncmp
> as you have for "instances=" (or "aggregate=") but then let's be sure
> to end the string we pass to kstrtouint(), ie. assume there could be
> further args.
Original purpose was to limit the length of string to accept, but can take
this way without issue I think.
>
> Documentation/ABI/testing/sysfs-bus-vfio-mdev also needs to be updated
> with this series.
Ok.
Thanks
>
> I'm curious what libvirt folks and Kirti think of this, it looks like
> it has a nice degree of backwards compatibility, both in the sysfs
> interface and the vendor driver interface. Thanks,
>
> Alex
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources
2018-07-20 2:19 ` [libvirt] [PATCH v2 0/4] New mdev type handling " Zhenyu Wang
` (4 preceding siblings ...)
2018-07-24 17:44 ` [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources Alex Williamson
@ 2018-10-08 3:19 ` Tian, Kevin
2018-10-08 5:08 ` Zhenyu Wang
2018-10-17 9:00 ` [libvirt] [PATCH v3 0/6] VFIO mdev aggregated resources handling Zhenyu Wang
` (6 subsequent siblings)
12 siblings, 1 reply; 38+ messages in thread
From: Tian, Kevin @ 2018-10-08 3:19 UTC (permalink / raw)
To: Zhenyu Wang, libvirt-list, kvm; +Cc: intel-gvt-dev, kwankhede
Hi, Zhenyu,
curious about the progress of this series. Is there still some open remaining
or a new version coming soon?
Thanks
Kevin
> From: Zhenyu Wang [mailto:zhenyuw@linux.intel.com]
> Sent: Friday, July 20, 2018 10:19 AM
>
> Current mdev device create interface depends on fixed mdev type, which
> get uuid
> from user to create instance of mdev device. If user wants to use
> customized
> number of resource for mdev device, then only can create new mdev type
> for that
> which may not be flexible. This requirement comes not only from to be
> able to
> allocate flexible resources for KVMGT, but also from Intel scalable IO
> virtualization which would use vfio/mdev to be able to allocate arbitrary
> resources on mdev instance. More info on [1] [2] [3].
>
> To allow to create user defined resources for mdev, it trys to extend mdev
> create interface by adding new "instances=xxx" parameter following uuid,
> for
> target mdev type if aggregation is supported, it can create new mdev device
> which contains resources combined by number of instances, e.g
>
> echo "<uuid>,instances=10" > create
>
> VM manager e.g libvirt can check mdev type with "aggregation" attribute
> which
> can support this setting. If no "aggregation" attribute found for mdev type,
> previous behavior is still kept for one instance allocation. And new sysfs
> attribute "instances" is created for each mdev device to show allocated
> number.
>
> This trys to create new KVMGT type with minimal vGPU resources which
> can be
> combined with "instances=x" setting to allocate for user wanted resources.
>
> References:
> [1] https://software.intel.com/en-us/download/intel-virtualization-
> technology-for-directed-io-architecture-specification
> [2] https://software.intel.com/en-us/download/intel-scalable-io-
> virtualization-technical-specification
> [3] https://schd.ws/hosted_files/lc32018/00/LC3-SIOV-final.pdf
>
> v2:
> - Add new create_with_instances driver hook
> - Update doc for new attributes
>
> Zhenyu Wang (4):
> vfio/mdev: Add new instances parameter for mdev create
> vfio/mdev: Add mdev device instances attribute
> drm/i915/gvt: Add new aggregation type support
> Documentation/vfio-mediated-device.txt: update for aggregation
> attribute
>
> Documentation/vfio-mediated-device.txt | 39 +++++++++++++++---
> drivers/gpu/drm/i915/gvt/gvt.c | 26 +++++++++---
> drivers/gpu/drm/i915/gvt/gvt.h | 14 ++++---
> drivers/gpu/drm/i915/gvt/kvmgt.c | 30 +++++++++++---
> drivers/gpu/drm/i915/gvt/vgpu.c | 56 ++++++++++++++++++++++----
> drivers/vfio/mdev/mdev_core.c | 19 +++++++--
> drivers/vfio/mdev/mdev_private.h | 6 ++-
> drivers/vfio/mdev/mdev_sysfs.c | 42 ++++++++++++++++---
> include/linux/mdev.h | 10 +++++
> 9 files changed, 203 insertions(+), 39 deletions(-)
>
> --
> 2.18.0
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources
2018-10-08 3:19 ` Tian, Kevin
@ 2018-10-08 5:08 ` Zhenyu Wang
0 siblings, 0 replies; 38+ messages in thread
From: Zhenyu Wang @ 2018-10-08 5:08 UTC (permalink / raw)
To: Tian, Kevin; +Cc: intel-gvt-dev, libvirt-list, kvm, kwankhede
[-- Attachment #1.1: Type: text/plain, Size: 483 bytes --]
On 2018.10.08 03:19:25 +0000, Tian, Kevin wrote:
> Hi, Zhenyu,
>
> curious about the progress of this series. Is there still some open remaining
> or a new version coming soon?
>
I had new version almost ready before my vacation, planned to send after be back.
So will refresh this later. Sorry for any delay as was trying to close several
gvt issues.
Thanks.
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* [libvirt] [PATCH v3 0/6] VFIO mdev aggregated resources handling
2018-07-20 2:19 ` [libvirt] [PATCH v2 0/4] New mdev type handling " Zhenyu Wang
` (5 preceding siblings ...)
2018-10-08 3:19 ` Tian, Kevin
@ 2018-10-17 9:00 ` Zhenyu Wang
2018-10-17 9:00 ` [libvirt] [PATCH v3 1/6] vfio/mdev: Add new "aggregate" parameter for mdev create Zhenyu Wang
` (5 subsequent siblings)
12 siblings, 0 replies; 38+ messages in thread
From: Zhenyu Wang @ 2018-10-17 9:00 UTC (permalink / raw)
To: libvirt-list, kvm; +Cc: intel-gvt-dev
Current mdev device create interface depends on fixed mdev type, which get uuid
from user to create instance of mdev device. If user wants to use customized
number of resource for mdev device, then only can create new mdev type for that
which may not be flexible. This requirement comes not only from to be able to
allocate flexible resources for KVMGT, but also from Intel scalable IO
virtualization which would use vfio/mdev to be able to allocate arbitrary
resources on mdev instance. More info on [1] [2] [3].
To allow to create user defined resources for mdev, it trys to extend mdev
create interface by adding new "aggregate=xxx" parameter following UUID, for
target mdev type if aggregation is supported, it can create new mdev device
which contains resources combined by number of instances, e.g
echo "<uuid>,aggregate=10" > create
VM manager e.g libvirt can check mdev type with "aggregation" attribute which
can support this setting. If no "aggregation" attribute found for mdev type,
previous behavior is still kept for one instance allocation. And new sysfs
attribute "aggregated_instances" is created for each mdev device to show allocated number.
References:
[1] https://software.intel.com/en-us/download/intel-virtualization-technology-for-directed-io-architecture-specification
[2] https://software.intel.com/en-us/download/intel-scalable-io-virtualization-technical-specification
[3] https://schd.ws/hosted_files/lc32018/00/LC3-SIOV-final.pdf
v2:
- Add new create_with_instances driver hook
- Update doc for new attributes
v3:
- Rename parameter and attribute names from review
- Make "aggregation" attribute to show maxium resource number
for aggregation
- Check driver hooks for attribute create validation
- Update doc and ABI spec
Zhenyu Wang (6):
vfio/mdev: Add new "aggregate" parameter for mdev create
vfio/mdev: Add "aggregation" attribute for supported mdev type
vfio/mdev: Add "aggregated_instances" attribute for supported mdev
device
Documentation/vfio-mediated-device.txt: Update for vfio/mdev
aggregation support
Documentation/ABI/testing/sysfs-bus-vfio-mdev: Update for vfio/mdev
aggregation support
drm/i915/gvt: Add new type with aggregation support
Documentation/ABI/testing/sysfs-bus-vfio-mdev | 25 +++++++
Documentation/vfio-mediated-device.txt | 44 +++++++++--
drivers/gpu/drm/i915/gvt/gvt.h | 11 ++-
drivers/gpu/drm/i915/gvt/kvmgt.c | 53 ++++++++++++-
drivers/gpu/drm/i915/gvt/vgpu.c | 56 +++++++++++++-
drivers/vfio/mdev/mdev_core.c | 40 +++++++++-
drivers/vfio/mdev/mdev_private.h | 6 +-
drivers/vfio/mdev/mdev_sysfs.c | 74 ++++++++++++++++++-
include/linux/mdev.h | 19 +++++
9 files changed, 305 insertions(+), 23 deletions(-)
--
2.19.1
^ permalink raw reply [flat|nested] 38+ messages in thread* [libvirt] [PATCH v3 1/6] vfio/mdev: Add new "aggregate" parameter for mdev create
2018-07-20 2:19 ` [libvirt] [PATCH v2 0/4] New mdev type handling " Zhenyu Wang
` (6 preceding siblings ...)
2018-10-17 9:00 ` [libvirt] [PATCH v3 0/6] VFIO mdev aggregated resources handling Zhenyu Wang
@ 2018-10-17 9:00 ` Zhenyu Wang
2018-10-17 9:00 ` [libvirt] [PATCH v3 2/6] vfio/mdev: Add "aggregation" attribute for supported mdev type Zhenyu Wang
` (4 subsequent siblings)
12 siblings, 0 replies; 38+ messages in thread
From: Zhenyu Wang @ 2018-10-17 9:00 UTC (permalink / raw)
To: libvirt-list, kvm
Cc: Kevin Tian, Kirti Wankhede, Cornelia Huck, intel-gvt-dev
For special mdev type which can aggregate instances for mdev device,
this extends mdev create interface by allowing extra "aggregate=xxx"
parameter, which is passed to mdev device model to be able to create
bundled number of instances for target mdev device.
v2: create new create_with_instances operator for vendor driver
v3:
- Change parameter name as "aggregate="
- Fix new interface comments.
- Parameter checking for new option, pass UUID string only to
parse and properly end parameter for kstrtouint() conversion.
Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
drivers/vfio/mdev/mdev_core.c | 21 +++++++++++++++++----
drivers/vfio/mdev/mdev_private.h | 4 +++-
drivers/vfio/mdev/mdev_sysfs.c | 32 ++++++++++++++++++++++++++++----
include/linux/mdev.h | 11 +++++++++++
4 files changed, 59 insertions(+), 9 deletions(-)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index 0212f0ee8aea..545c52ec7618 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -104,12 +104,17 @@ static inline void mdev_put_parent(struct mdev_parent *parent)
}
static int mdev_device_create_ops(struct kobject *kobj,
- struct mdev_device *mdev)
+ struct mdev_device *mdev,
+ unsigned int instances)
{
struct mdev_parent *parent = mdev->parent;
int ret;
- ret = parent->ops->create(kobj, mdev);
+ if (instances > 1) {
+ ret = parent->ops->create_with_instances(kobj, mdev,
+ instances);
+ } else
+ ret = parent->ops->create(kobj, mdev);
if (ret)
return ret;
@@ -276,7 +281,8 @@ static void mdev_device_release(struct device *dev)
kfree(mdev);
}
-int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid)
+int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid,
+ unsigned int instances)
{
int ret;
struct mdev_device *mdev, *tmp;
@@ -287,6 +293,12 @@ int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid)
if (!parent)
return -EINVAL;
+ if (instances > 1 &&
+ !parent->ops->create_with_instances) {
+ ret = -EINVAL;
+ goto mdev_fail;
+ }
+
mutex_lock(&mdev_list_lock);
/* Check for duplicate */
@@ -316,6 +328,7 @@ int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid)
mdev->dev.bus = &mdev_bus_type;
mdev->dev.release = mdev_device_release;
dev_set_name(&mdev->dev, "%pUl", uuid.b);
+ mdev->type_instances = instances;
ret = device_register(&mdev->dev);
if (ret) {
@@ -323,7 +336,7 @@ int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid)
goto mdev_fail;
}
- ret = mdev_device_create_ops(kobj, mdev);
+ ret = mdev_device_create_ops(kobj, mdev, instances);
if (ret)
goto create_fail;
diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
index b5819b7d7ef7..e90d295d3927 100644
--- a/drivers/vfio/mdev/mdev_private.h
+++ b/drivers/vfio/mdev/mdev_private.h
@@ -33,6 +33,7 @@ struct mdev_device {
struct kref ref;
struct list_head next;
struct kobject *type_kobj;
+ unsigned int type_instances;
bool active;
};
@@ -58,7 +59,8 @@ void parent_remove_sysfs_files(struct mdev_parent *parent);
int mdev_create_sysfs_files(struct device *dev, struct mdev_type *type);
void mdev_remove_sysfs_files(struct device *dev, struct mdev_type *type);
-int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid);
+int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid,
+ unsigned int instances);
int mdev_device_remove(struct device *dev, bool force_remove);
#endif /* MDEV_PRIVATE_H */
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
index 249472f05509..aefed0c8891b 100644
--- a/drivers/vfio/mdev/mdev_sysfs.c
+++ b/drivers/vfio/mdev/mdev_sysfs.c
@@ -54,14 +54,21 @@ static const struct sysfs_ops mdev_type_sysfs_ops = {
static ssize_t create_store(struct kobject *kobj, struct device *dev,
const char *buf, size_t count)
{
- char *str;
+ char *str, *param, *opt = NULL;
uuid_le uuid;
int ret;
+ unsigned int instances = 1;
- if ((count < UUID_STRING_LEN) || (count > UUID_STRING_LEN + 1))
+ if (count < UUID_STRING_LEN)
return -EINVAL;
- str = kstrndup(buf, count, GFP_KERNEL);
+ if ((param = strnchr(buf, count, ',')) == NULL) {
+ if (count > UUID_STRING_LEN + 1)
+ return -EINVAL;
+ } else if (param - buf != UUID_STRING_LEN)
+ return -EINVAL;
+
+ str = kstrndup(buf, UUID_STRING_LEN, GFP_KERNEL);
if (!str)
return -ENOMEM;
@@ -70,7 +77,24 @@ static ssize_t create_store(struct kobject *kobj, struct device *dev,
if (ret)
return ret;
- ret = mdev_device_create(kobj, dev, uuid);
+ if (param) {
+ opt = kstrndup(param + 1, count - UUID_STRING_LEN - 1,
+ GFP_KERNEL);
+ if (!opt)
+ return -ENOMEM;
+ if (strncmp(opt, "aggregate=", 10)) {
+ kfree(opt);
+ return -EINVAL;
+ }
+ opt += 10;
+ if (kstrtouint(opt, 10, &instances)) {
+ kfree(opt);
+ return -EINVAL;
+ }
+ kfree(opt);
+ }
+
+ ret = mdev_device_create(kobj, dev, uuid, instances);
if (ret)
return ret;
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index b6e048e1045f..c12c0bfc5598 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -31,6 +31,14 @@ struct mdev_device;
* @mdev: mdev_device structure on of mediated device
* that is being created
* Returns integer: success (0) or error (< 0)
+ * @create_with_instances: Allocate aggregated instances' resources in parent device's
+ * driver for a particular mediated device. Optional if aggregated
+ * resources are not supported.
+ * @kobj: kobject of type for which 'create' is called.
+ * @mdev: mdev_device structure on of mediated device
+ * that is being created
+ * @instances: number of instances to aggregate
+ * Returns integer: success (0) or error (< 0)
* @remove: Called to free resources in parent device's driver for a
* a mediated device. It is mandatory to provide 'remove'
* ops.
@@ -71,6 +79,9 @@ struct mdev_parent_ops {
struct attribute_group **supported_type_groups;
int (*create)(struct kobject *kobj, struct mdev_device *mdev);
+ int (*create_with_instances)(struct kobject *kobj,
+ struct mdev_device *mdev,
+ unsigned int instances);
int (*remove)(struct mdev_device *mdev);
int (*open)(struct mdev_device *mdev);
void (*release)(struct mdev_device *mdev);
--
2.19.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [libvirt] [PATCH v3 2/6] vfio/mdev: Add "aggregation" attribute for supported mdev type
2018-07-20 2:19 ` [libvirt] [PATCH v2 0/4] New mdev type handling " Zhenyu Wang
` (7 preceding siblings ...)
2018-10-17 9:00 ` [libvirt] [PATCH v3 1/6] vfio/mdev: Add new "aggregate" parameter for mdev create Zhenyu Wang
@ 2018-10-17 9:00 ` Zhenyu Wang
2018-10-17 9:00 ` [libvirt] [PATCH v3 3/6] vfio/mdev: Add "aggregated_instances" attribute for supported mdev device Zhenyu Wang
` (3 subsequent siblings)
12 siblings, 0 replies; 38+ messages in thread
From: Zhenyu Wang @ 2018-10-17 9:00 UTC (permalink / raw)
To: libvirt-list, kvm
Cc: Kevin Tian, Kirti Wankhede, Cornelia Huck, intel-gvt-dev
For supported mdev driver to create aggregated device, this creates
new "aggregation" attribute for target type, which will show maximum
number of instance resources that can be aggregated.
Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
drivers/vfio/mdev/mdev_core.c | 19 +++++++++++++++++++
drivers/vfio/mdev/mdev_private.h | 2 ++
drivers/vfio/mdev/mdev_sysfs.c | 22 ++++++++++++++++++++++
include/linux/mdev.h | 8 ++++++++
4 files changed, 51 insertions(+)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index 545c52ec7618..8f8bbb72e5d9 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -161,6 +161,25 @@ static int mdev_device_remove_cb(struct device *dev, void *data)
return mdev_device_remove(dev, data ? *(bool *)data : true);
}
+int mdev_max_aggregated_instances(struct kobject *kobj, struct device *dev,
+ unsigned int *m)
+{
+ struct mdev_parent *parent;
+ struct mdev_type *type = to_mdev_type(kobj);
+ int ret;
+
+ parent = mdev_get_parent(type->parent);
+ if (!parent)
+ return -EINVAL;
+
+ if (parent->ops->max_aggregated_instances) {
+ ret = parent->ops->max_aggregated_instances(kobj, dev, m);
+ } else
+ ret = -EINVAL;
+ mdev_put_parent(parent);
+ return ret;
+}
+
/*
* mdev_register_device : Register a device
* @dev: device structure representing parent device.
diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
index e90d295d3927..f1289db75884 100644
--- a/drivers/vfio/mdev/mdev_private.h
+++ b/drivers/vfio/mdev/mdev_private.h
@@ -63,4 +63,6 @@ int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid,
unsigned int instances);
int mdev_device_remove(struct device *dev, bool force_remove);
+int mdev_max_aggregated_instances(struct kobject *kobj, struct device *dev,
+ unsigned int *m);
#endif /* MDEV_PRIVATE_H */
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
index aefed0c8891b..a329d6ab6cb9 100644
--- a/drivers/vfio/mdev/mdev_sysfs.c
+++ b/drivers/vfio/mdev/mdev_sysfs.c
@@ -103,6 +103,18 @@ static ssize_t create_store(struct kobject *kobj, struct device *dev,
MDEV_TYPE_ATTR_WO(create);
+static ssize_t
+aggregation_show(struct kobject *kobj, struct device *dev, char *buf)
+{
+ unsigned int m;
+
+ if (mdev_max_aggregated_instances(kobj, dev, &m))
+ return sprintf(buf, "1\n");
+ else
+ return sprintf(buf, "%u\n", m);
+}
+MDEV_TYPE_ATTR_RO(aggregation);
+
static void mdev_type_release(struct kobject *kobj)
{
struct mdev_type *type = to_mdev_type(kobj);
@@ -145,6 +157,14 @@ struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent,
if (ret)
goto attr_create_failed;
+ if (parent->ops->create_with_instances &&
+ parent->ops->max_aggregated_instances) {
+ ret = sysfs_create_file(&type->kobj,
+ &mdev_type_attr_aggregation.attr);
+ if (ret)
+ goto attr_aggregate_failed;
+ }
+
type->devices_kobj = kobject_create_and_add("devices", &type->kobj);
if (!type->devices_kobj) {
ret = -ENOMEM;
@@ -165,6 +185,8 @@ struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent,
attrs_failed:
kobject_put(type->devices_kobj);
attr_devices_failed:
+ sysfs_remove_file(&type->kobj, &mdev_type_attr_aggregation.attr);
+attr_aggregate_failed:
sysfs_remove_file(&type->kobj, &mdev_type_attr_create.attr);
attr_create_failed:
kobject_del(&type->kobj);
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index c12c0bfc5598..66cfdb0bf0d6 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -39,6 +39,11 @@ struct mdev_device;
* that is being created
* @instances: number of instances to aggregate
* Returns integer: success (0) or error (< 0)
+ * @max_aggregated_instances: Return max number for aggregated resources
+ * @kobj: kobject of type
+ * @dev: mdev parent device for target type
+ * @max: return max number of instances which can aggregate
+ * Returns integer: success (0) or error (< 0)
* @remove: Called to free resources in parent device's driver for a
* a mediated device. It is mandatory to provide 'remove'
* ops.
@@ -82,6 +87,9 @@ struct mdev_parent_ops {
int (*create_with_instances)(struct kobject *kobj,
struct mdev_device *mdev,
unsigned int instances);
+ int (*max_aggregated_instances)(struct kobject *kobj,
+ struct device *dev,
+ unsigned int *max);
int (*remove)(struct mdev_device *mdev);
int (*open)(struct mdev_device *mdev);
void (*release)(struct mdev_device *mdev);
--
2.19.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [libvirt] [PATCH v3 3/6] vfio/mdev: Add "aggregated_instances" attribute for supported mdev device
2018-07-20 2:19 ` [libvirt] [PATCH v2 0/4] New mdev type handling " Zhenyu Wang
` (8 preceding siblings ...)
2018-10-17 9:00 ` [libvirt] [PATCH v3 2/6] vfio/mdev: Add "aggregation" attribute for supported mdev type Zhenyu Wang
@ 2018-10-17 9:00 ` Zhenyu Wang
2018-10-17 9:00 ` [libvirt] [PATCH v3 4/6] Documentation/vfio-mediated-device.txt: Update for vfio/mdev aggregation support Zhenyu Wang
` (2 subsequent siblings)
12 siblings, 0 replies; 38+ messages in thread
From: Zhenyu Wang @ 2018-10-17 9:00 UTC (permalink / raw)
To: libvirt-list, kvm
Cc: Kevin Tian, Kirti Wankhede, Cornelia Huck, intel-gvt-dev
For mdev device created by "aggregate" parameter, this creates new mdev
device attribute "aggregated_instances" to show number of aggregated
instances allocated.
v2:
- change attribute name as "aggregated_instances"
v3:
- create only for aggregated allocation
Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
drivers/vfio/mdev/mdev_sysfs.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
index a329d6ab6cb9..f03bdfbf5a42 100644
--- a/drivers/vfio/mdev/mdev_sysfs.c
+++ b/drivers/vfio/mdev/mdev_sysfs.c
@@ -292,7 +292,17 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
return count;
}
+static ssize_t
+aggregated_instances_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct mdev_device *mdev = to_mdev_device(dev);
+ return sprintf(buf, "%u\n", mdev->type_instances);
+}
+
static DEVICE_ATTR_WO(remove);
+static DEVICE_ATTR_RO(aggregated_instances);
static const struct attribute *mdev_device_attrs[] = {
&dev_attr_remove.attr,
@@ -301,6 +311,7 @@ static const struct attribute *mdev_device_attrs[] = {
int mdev_create_sysfs_files(struct device *dev, struct mdev_type *type)
{
+ struct mdev_device *mdev = to_mdev_device(dev);
int ret;
ret = sysfs_create_link(type->devices_kobj, &dev->kobj, dev_name(dev));
@@ -315,8 +326,17 @@ int mdev_create_sysfs_files(struct device *dev, struct mdev_type *type)
if (ret)
goto create_files_failed;
+ if (mdev->type_instances > 1) {
+ ret = sysfs_create_file(&dev->kobj,
+ &dev_attr_aggregated_instances.attr);
+ if (ret)
+ goto create_aggregated_failed;
+ }
+
return ret;
+create_aggregated_failed:
+ sysfs_remove_files(&dev->kobj, mdev_device_attrs);
create_files_failed:
sysfs_remove_link(&dev->kobj, "mdev_type");
type_link_failed:
--
2.19.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [libvirt] [PATCH v3 4/6] Documentation/vfio-mediated-device.txt: Update for vfio/mdev aggregation support
2018-07-20 2:19 ` [libvirt] [PATCH v2 0/4] New mdev type handling " Zhenyu Wang
` (9 preceding siblings ...)
2018-10-17 9:00 ` [libvirt] [PATCH v3 3/6] vfio/mdev: Add "aggregated_instances" attribute for supported mdev device Zhenyu Wang
@ 2018-10-17 9:00 ` Zhenyu Wang
2018-10-17 9:00 ` [libvirt] [PATCH v3 5/6] Documentation/ABI/testing/sysfs-bus-vfio-mdev: " Zhenyu Wang
2018-10-17 9:00 ` [libvirt] [PATCH v3 6/6] drm/i915/gvt: Add new type with " Zhenyu Wang
12 siblings, 0 replies; 38+ messages in thread
From: Zhenyu Wang @ 2018-10-17 9:00 UTC (permalink / raw)
To: libvirt-list, kvm
Cc: Kevin Tian, Kirti Wankhede, Cornelia Huck, intel-gvt-dev
Update vfio/mdev doc on new "aggregate" create parameter, new "aggregation"
attribute and "aggregated_instances" attribute for mdev device.
Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
Documentation/vfio-mediated-device.txt | 44 ++++++++++++++++++++++----
1 file changed, 38 insertions(+), 6 deletions(-)
diff --git a/Documentation/vfio-mediated-device.txt b/Documentation/vfio-mediated-device.txt
index c3f69bcaf96e..cf4849a34c9f 100644
--- a/Documentation/vfio-mediated-device.txt
+++ b/Documentation/vfio-mediated-device.txt
@@ -211,12 +211,20 @@ Directories and files under the sysfs for Each Physical Device
| | |--- description
| | |--- [devices]
| |--- [<type-id>]
- | |--- create
- | |--- name
- | |--- available_instances
- | |--- device_api
- | |--- description
- | |--- [devices]
+ | | |--- create
+ | | |--- name
+ | | |--- available_instances
+ | | |--- device_api
+ | | |--- description
+ | | |--- [devices]
+ | |--- [<type-id>]
+ | | |--- create
+ | | |--- name
+ | | |--- available_instances
+ | | |--- device_api
+ | | |--- description
+ | | |--- <aggregation>
+ | | |--- [devices]
* [mdev_supported_types]
@@ -260,6 +268,23 @@ Directories and files under the sysfs for Each Physical Device
This attribute should show brief features/description of the type. This is
optional attribute.
+* <aggregation>
+
+ <aggregation> is an optional attributes to show max number that the
+ instance resources of [<type-id>] can be aggregated to be assigned
+ for one mdev device. No <aggregation> attribute means driver doesn't
+ support to aggregate instance resoures for one mdev device.
+ <aggregation> may be less than available_instances which depends on
+ driver. <aggregation> number can't exceed available_instances.
+
+ Set number of instances by appending "aggregate=N" parameter for
+ create attribute. By default without "aggregate=N" parameter it
+ will create one instance as normal.
+
+Example::
+
+ # echo "<uuid>,aggregate=N" > create
+
Directories and Files Under the sysfs for Each mdev Device
----------------------------------------------------------
@@ -268,6 +293,7 @@ Directories and Files Under the sysfs for Each mdev Device
|- [parent phy device]
|--- [$MDEV_UUID]
|--- remove
+ |--- <aggregated_instances>
|--- mdev_type {link to its type}
|--- vendor-specific-attributes [optional]
@@ -281,6 +307,12 @@ Example::
# echo 1 > /sys/bus/mdev/devices/$mdev_UUID/remove
+* <aggregated_instances> (read only)
+
+For mdev created with aggregate parameter, this shows number of
+instances assigned for this mdev. For normal type this attribute will
+not exist.
+
Mediated device Hot plug
------------------------
--
2.19.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [libvirt] [PATCH v3 5/6] Documentation/ABI/testing/sysfs-bus-vfio-mdev: Update for vfio/mdev aggregation support
2018-07-20 2:19 ` [libvirt] [PATCH v2 0/4] New mdev type handling " Zhenyu Wang
` (10 preceding siblings ...)
2018-10-17 9:00 ` [libvirt] [PATCH v3 4/6] Documentation/vfio-mediated-device.txt: Update for vfio/mdev aggregation support Zhenyu Wang
@ 2018-10-17 9:00 ` Zhenyu Wang
2018-10-17 9:00 ` [libvirt] [PATCH v3 6/6] drm/i915/gvt: Add new type with " Zhenyu Wang
12 siblings, 0 replies; 38+ messages in thread
From: Zhenyu Wang @ 2018-10-17 9:00 UTC (permalink / raw)
To: libvirt-list, kvm
Cc: Kevin Tian, Kirti Wankhede, Cornelia Huck, intel-gvt-dev
Update vfio/mdev ABI description for new aggregation attributes.
Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
Documentation/ABI/testing/sysfs-bus-vfio-mdev | 25 +++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-bus-vfio-mdev b/Documentation/ABI/testing/sysfs-bus-vfio-mdev
index 452dbe39270e..192fe06e60d0 100644
--- a/Documentation/ABI/testing/sysfs-bus-vfio-mdev
+++ b/Documentation/ABI/testing/sysfs-bus-vfio-mdev
@@ -85,6 +85,24 @@ Users:
a particular <type-id> that can help in understanding the
features provided by that type of mediated device.
+What: /sys/.../mdev_supported_types/<type-id>/aggregation
+Date: October 2018
+Contact: Zhenyu Wang <zhenyuw@linux.intel.com>
+Description:
+ Reading this attribute will show number of mdev instances
+ that can be aggregated to assign for one mdev device.
+ This is optional attribute. If this attribute exists that
+ means driver supports to aggregate target mdev type's
+ resources assigned for one mdev device.
+Users:
+ Userspace applications interested in creating mediated
+ device with aggregated type instances. Userspace application
+ should check the number of aggregation instances that could
+ be created before creating mediated device by applying this,
+ e.g
+ # echo "83b8f4f2-509f-382f-3c1e-e6bfe0fa1001,aggregate=XX" > \
+ /sys/devices/foo/mdev_supported_types/foo-1/create
+
What: /sys/.../<device>/<UUID>/
Date: October 2016
Contact: Kirti Wankhede <kwankhede@nvidia.com>
@@ -109,3 +127,10 @@ Description:
is active and the vendor driver doesn't support hot unplug.
Example:
# echo 1 > /sys/bus/mdev/devices/<UUID>/remove
+
+What: /sys/.../<device>/<UUID>/aggregated_instances
+Date: October 2018
+Contact: Zhenyu Wang <zhenyuw@linux.intel.com>
+Description:
+ This attributes shows number of aggregated instances if this
+ mediated device was created with "aggregate" parameter.
\ No newline at end of file
--
2.19.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [libvirt] [PATCH v3 6/6] drm/i915/gvt: Add new type with aggregation support
2018-07-20 2:19 ` [libvirt] [PATCH v2 0/4] New mdev type handling " Zhenyu Wang
` (11 preceding siblings ...)
2018-10-17 9:00 ` [libvirt] [PATCH v3 5/6] Documentation/ABI/testing/sysfs-bus-vfio-mdev: " Zhenyu Wang
@ 2018-10-17 9:00 ` Zhenyu Wang
12 siblings, 0 replies; 38+ messages in thread
From: Zhenyu Wang @ 2018-10-17 9:00 UTC (permalink / raw)
To: libvirt-list, kvm
Cc: Kevin Tian, Kirti Wankhede, Cornelia Huck, intel-gvt-dev
New aggregation type is created for KVMGT which can be used
to combine minimal resource number for target instances, to create
user defined number of resources. For KVMGT, aggregated resource
is determined by memory and fence resource allocation for target
number of instances.
v2:
- apply for new hooks
Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
drivers/gpu/drm/i915/gvt/gvt.h | 11 +++++--
drivers/gpu/drm/i915/gvt/kvmgt.c | 53 ++++++++++++++++++++++++++++--
drivers/gpu/drm/i915/gvt/vgpu.c | 56 ++++++++++++++++++++++++++++++--
3 files changed, 112 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index 31f6cdbe5c42..cb318079fa74 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -241,6 +241,9 @@ struct intel_vgpu {
struct intel_gvt_gm {
unsigned long vgpu_allocated_low_gm_size;
unsigned long vgpu_allocated_high_gm_size;
+ unsigned long low_avail;
+ unsigned long high_avail;
+ unsigned long fence_avail;
};
struct intel_gvt_fence {
@@ -292,13 +295,15 @@ struct intel_gvt_firmware {
#define NR_MAX_INTEL_VGPU_TYPES 20
struct intel_vgpu_type {
- char name[16];
+ const char *drv_name;
+ char name[32];
unsigned int avail_instance;
unsigned int low_gm_size;
unsigned int high_gm_size;
unsigned int fence;
unsigned int weight;
enum intel_vgpu_edid resolution;
+ unsigned int aggregation;
};
struct intel_gvt {
@@ -484,7 +489,7 @@ void intel_gvt_clean_vgpu_types(struct intel_gvt *gvt);
struct intel_vgpu *intel_gvt_create_idle_vgpu(struct intel_gvt *gvt);
void intel_gvt_destroy_idle_vgpu(struct intel_vgpu *vgpu);
struct intel_vgpu *intel_gvt_create_vgpu(struct intel_gvt *gvt,
- struct intel_vgpu_type *type);
+ struct intel_vgpu_type *type, unsigned int);
void intel_gvt_destroy_vgpu(struct intel_vgpu *vgpu);
void intel_gvt_release_vgpu(struct intel_vgpu *vgpu);
void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr,
@@ -563,7 +568,7 @@ struct intel_gvt_ops {
int (*emulate_mmio_write)(struct intel_vgpu *, u64, void *,
unsigned int);
struct intel_vgpu *(*vgpu_create)(struct intel_gvt *,
- struct intel_vgpu_type *);
+ struct intel_vgpu_type *, unsigned int);
void (*vgpu_destroy)(struct intel_vgpu *vgpu);
void (*vgpu_release)(struct intel_vgpu *vgpu);
void (*vgpu_reset)(struct intel_vgpu *);
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index c1072143da1d..841ad5437c4a 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -501,7 +501,9 @@ static void kvmgt_put_vfio_device(void *vgpu)
vfio_device_put(((struct intel_vgpu *)vgpu)->vdev.vfio_device);
}
-static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
+static int intel_vgpu_create_internal(struct kobject *kobj,
+ struct mdev_device *mdev,
+ unsigned int instances)
{
struct intel_vgpu *vgpu = NULL;
struct intel_vgpu_type *type;
@@ -520,7 +522,14 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
goto out;
}
- vgpu = intel_gvt_ops->vgpu_create(gvt, type);
+ if (instances > type->aggregation) {
+ gvt_vgpu_err("wrong aggregation specified for type %s\n",
+ kobject_name(kobj));
+ ret = -EINVAL;
+ goto out;
+ }
+
+ vgpu = intel_gvt_ops->vgpu_create(gvt, type, instances);
if (IS_ERR_OR_NULL(vgpu)) {
ret = vgpu == NULL ? -EFAULT : PTR_ERR(vgpu);
gvt_err("failed to create intel vgpu: %d\n", ret);
@@ -540,6 +549,44 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
return ret;
}
+static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
+{
+ return intel_vgpu_create_internal(kobj, mdev, 1);
+}
+
+static int intel_vgpu_create_with_instances(struct kobject *kobj,
+ struct mdev_device *mdev,
+ unsigned int instances)
+{
+ return intel_vgpu_create_internal(kobj, mdev, instances);
+}
+
+static int intel_vgpu_max_aggregated_instances(struct kobject *kobj,
+ struct device *dev,
+ unsigned int *max)
+{
+ struct intel_vgpu_type *type;
+ struct intel_gvt *gvt;
+ int ret = 0;
+
+ gvt = kdev_to_i915(dev)->gvt;
+
+ type = intel_gvt_ops->gvt_find_vgpu_type(gvt, kobject_name(kobj));
+ if (!type) {
+ gvt_err("failed to find type %s to create\n",
+ kobject_name(kobj));
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (type->aggregation <= 1)
+ *max = 1;
+ else
+ *max = type->aggregation;
+out:
+ return ret;
+}
+
static int intel_vgpu_remove(struct mdev_device *mdev)
{
struct intel_vgpu *vgpu = mdev_get_drvdata(mdev);
@@ -1442,6 +1489,8 @@ static const struct attribute_group *intel_vgpu_groups[] = {
static struct mdev_parent_ops intel_vgpu_ops = {
.mdev_attr_groups = intel_vgpu_groups,
.create = intel_vgpu_create,
+ .create_with_instances = intel_vgpu_create_with_instances,
+ .max_aggregated_instances = intel_vgpu_max_aggregated_instances,
.remove = intel_vgpu_remove,
.open = intel_vgpu_open,
diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
index c628be05fbfe..d36f017c740b 100644
--- a/drivers/gpu/drm/i915/gvt/vgpu.c
+++ b/drivers/gpu/drm/i915/gvt/vgpu.c
@@ -108,6 +108,7 @@ int intel_gvt_init_vgpu_types(struct intel_gvt *gvt)
unsigned int num_types;
unsigned int i, low_avail, high_avail;
unsigned int min_low;
+ const char *driver_name = dev_driver_string(&gvt->dev_priv->drm.pdev->dev);
/* vGPU type name is defined as GVTg_Vx_y which contains
* physical GPU generation type (e.g V4 as BDW server, V5 as
@@ -125,11 +126,15 @@ int intel_gvt_init_vgpu_types(struct intel_gvt *gvt)
high_avail = gvt_hidden_sz(gvt) - HOST_HIGH_GM_SIZE;
num_types = sizeof(vgpu_types) / sizeof(vgpu_types[0]);
- gvt->types = kcalloc(num_types, sizeof(struct intel_vgpu_type),
+ gvt->types = kcalloc(num_types + 1, sizeof(struct intel_vgpu_type),
GFP_KERNEL);
if (!gvt->types)
return -ENOMEM;
+ gvt->gm.low_avail = low_avail;
+ gvt->gm.high_avail = high_avail;
+ gvt->gm.fence_avail = 32 - HOST_FENCE;
+
min_low = MB_TO_BYTES(32);
for (i = 0; i < num_types; ++i) {
if (low_avail / vgpu_types[i].low_mm == 0)
@@ -147,6 +152,7 @@ int intel_gvt_init_vgpu_types(struct intel_gvt *gvt)
gvt->types[i].resolution = vgpu_types[i].edid;
gvt->types[i].avail_instance = min(low_avail / vgpu_types[i].low_mm,
high_avail / vgpu_types[i].high_mm);
+ gvt->types[i].aggregation = 1;
if (IS_GEN8(gvt->dev_priv))
sprintf(gvt->types[i].name, "GVTg_V4_%s",
@@ -154,6 +160,7 @@ int intel_gvt_init_vgpu_types(struct intel_gvt *gvt)
else if (IS_GEN9(gvt->dev_priv))
sprintf(gvt->types[i].name, "GVTg_V5_%s",
vgpu_types[i].name);
+ gvt->types[i].drv_name = driver_name;
gvt_dbg_core("type[%d]: %s avail %u low %u high %u fence %u weight %u res %s\n",
i, gvt->types[i].name,
@@ -164,7 +171,32 @@ int intel_gvt_init_vgpu_types(struct intel_gvt *gvt)
vgpu_edid_str(gvt->types[i].resolution));
}
- gvt->num_types = i;
+ /* add aggregation type */
+ gvt->types[i].low_gm_size = MB_TO_BYTES(32);
+ gvt->types[i].high_gm_size = MB_TO_BYTES(192);
+ gvt->types[i].fence = 2;
+ gvt->types[i].weight = 16;
+ gvt->types[i].resolution = GVT_EDID_1024_768;
+ gvt->types[i].avail_instance = min(low_avail / gvt->types[i].low_gm_size,
+ high_avail / gvt->types[i].high_gm_size);
+ gvt->types[i].avail_instance = min(gvt->types[i].avail_instance,
+ (32 - HOST_FENCE) / gvt->types[i].fence);
+ if (IS_GEN8(gvt->dev_priv))
+ strcat(gvt->types[i].name, "GVTg_V4_aggregate");
+ else if (IS_GEN9(gvt->dev_priv))
+ strcat(gvt->types[i].name, "GVTg_V5_aggregate");
+ gvt->types[i].drv_name = driver_name;
+
+ gvt_dbg_core("type[%d]: %s avail %u low %u high %u fence %u weight %u res %s\n",
+ i, gvt->types[i].name,
+ gvt->types[i].avail_instance,
+ gvt->types[i].low_gm_size,
+ gvt->types[i].high_gm_size, gvt->types[i].fence,
+ gvt->types[i].weight,
+ vgpu_edid_str(gvt->types[i].resolution));
+
+ gvt->types[i].aggregation = gvt->types[i].avail_instance;
+ gvt->num_types = ++i;
return 0;
}
@@ -195,6 +227,8 @@ static void intel_gvt_update_vgpu_types(struct intel_gvt *gvt)
fence_min = fence_avail / gvt->types[i].fence;
gvt->types[i].avail_instance = min(min(low_gm_min, high_gm_min),
fence_min);
+ if (gvt->types[i].aggregation > 1)
+ gvt->types[i].aggregation = gvt->types[i].avail_instance;
gvt_dbg_core("update type[%d]: %s avail %u low %u high %u fence %u\n",
i, gvt->types[i].name,
@@ -464,7 +498,8 @@ static struct intel_vgpu *__intel_gvt_create_vgpu(struct intel_gvt *gvt,
* pointer to intel_vgpu, error pointer if failed.
*/
struct intel_vgpu *intel_gvt_create_vgpu(struct intel_gvt *gvt,
- struct intel_vgpu_type *type)
+ struct intel_vgpu_type *type,
+ unsigned int instances)
{
struct intel_vgpu_creation_params param;
struct intel_vgpu *vgpu;
@@ -481,6 +516,21 @@ struct intel_vgpu *intel_gvt_create_vgpu(struct intel_gvt *gvt,
param.low_gm_sz = BYTES_TO_MB(param.low_gm_sz);
param.high_gm_sz = BYTES_TO_MB(param.high_gm_sz);
+ if (instances > 1 && instances <= type->aggregation) {
+ if (instances > type->avail_instance)
+ return ERR_PTR(-EINVAL);
+ param.low_gm_sz = min(param.low_gm_sz * instances,
+ (u64)BYTES_TO_MB(gvt->gm.low_avail));
+ param.high_gm_sz = min(param.high_gm_sz * instances,
+ (u64)BYTES_TO_MB(gvt->gm.high_avail));
+ param.fence_sz = min(param.fence_sz * instances,
+ (u64)gvt->gm.fence_avail);
+ type->aggregation -= instances;
+ gvt_dbg_core("instances %d, low %lluMB, high %lluMB, fence %llu, left %u\n",
+ instances, param.low_gm_sz, param.high_gm_sz, param.fence_sz,
+ type->aggregation);
+ }
+
mutex_lock(&gvt->lock);
vgpu = __intel_gvt_create_vgpu(gvt, ¶m);
if (!IS_ERR(vgpu))
--
2.19.1
^ permalink raw reply related [flat|nested] 38+ messages in thread