* [PATCH] vdpa: add driver_override support
@ 2021-11-04 16:17 Stefano Garzarella
2021-11-05 3:01 ` Jason Wang
0 siblings, 1 reply; 8+ messages in thread
From: Stefano Garzarella @ 2021-11-04 16:17 UTC (permalink / raw)
To: virtualization; +Cc: Parav Pandit, Michael S. Tsirkin, Jason Wang, linux-kernel
`driver_override` allows to control which of the vDPA bus drivers
binds to a vDPA device.
If `driver_override` is not set, the previous behaviour is followed:
devices use the first vDPA bus driver loaded (unless auto binding
is disabled).
Tested on Fedora 34 with driverctl(8):
$ modprobe virtio-vdpa
$ modprobe vhost-vdpa
$ modprobe vdpa-sim-net
$ vdpa dev add mgmtdev vdpasim_net name dev1
# dev1 is attached to the first vDPA bus driver loaded
$ driverctl -b vdpa list-devices
dev1 virtio_vdpa
$ driverctl -b vdpa set-override dev1 vhost_vdpa
$ driverctl -b vdpa list-devices
dev1 vhost_vdpa [*]
Note: driverctl(8) integrates with udev so the binding is
preserved.
Suggested-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
include/linux/vdpa.h | 2 ++
drivers/vdpa/vdpa.c | 74 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 76 insertions(+)
diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index c3011ccda430..ae34015b37b7 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -64,6 +64,7 @@ struct vdpa_mgmt_dev;
* struct vdpa_device - representation of a vDPA device
* @dev: underlying device
* @dma_dev: the actual device that is performing DMA
+ * @driver_override: driver name to force a match
* @config: the configuration ops for this device.
* @cf_mutex: Protects get and set access to configuration layout.
* @index: device index
@@ -76,6 +77,7 @@ struct vdpa_mgmt_dev;
struct vdpa_device {
struct device dev;
struct device *dma_dev;
+ const char *driver_override;
const struct vdpa_config_ops *config;
struct mutex cf_mutex; /* Protects get/set config */
unsigned int index;
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 7332a74a4b00..659231bbfee8 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -52,8 +52,81 @@ static void vdpa_dev_remove(struct device *d)
drv->remove(vdev);
}
+static int vdpa_dev_match(struct device *dev, struct device_driver *drv)
+{
+ struct vdpa_device *vdev = dev_to_vdpa(dev);
+
+ /* Check override first, and if set, only use the named driver */
+ if (vdev->driver_override)
+ return strcmp(vdev->driver_override, drv->name) == 0;
+
+ /* Currently devices must be supported by all vDPA bus drivers */
+ return 1;
+}
+
+static ssize_t driver_override_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct vdpa_device *vdev = dev_to_vdpa(dev);
+ const char *driver_override, *old;
+ char *cp;
+
+ /* We need to keep extra room for a newline */
+ if (count >= (PAGE_SIZE - 1))
+ return -EINVAL;
+
+ driver_override = kstrndup(buf, count, GFP_KERNEL);
+ if (!driver_override)
+ return -ENOMEM;
+
+ cp = strchr(driver_override, '\n');
+ if (cp)
+ *cp = '\0';
+
+ device_lock(dev);
+ old = vdev->driver_override;
+ if (strlen(driver_override)) {
+ vdev->driver_override = driver_override;
+ } else {
+ kfree(driver_override);
+ vdev->driver_override = NULL;
+ }
+ device_unlock(dev);
+
+ kfree(old);
+
+ return count;
+}
+
+static ssize_t driver_override_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct vdpa_device *vdev = dev_to_vdpa(dev);
+ ssize_t len;
+
+ device_lock(dev);
+ len = snprintf(buf, PAGE_SIZE, "%s\n", vdev->driver_override);
+ device_unlock(dev);
+
+ return len;
+}
+static DEVICE_ATTR_RW(driver_override);
+
+static struct attribute *vdpa_dev_attrs[] = {
+ &dev_attr_driver_override.attr,
+ NULL,
+};
+
+static const struct attribute_group vdpa_dev_group = {
+ .attrs = vdpa_dev_attrs,
+};
+__ATTRIBUTE_GROUPS(vdpa_dev);
+
static struct bus_type vdpa_bus = {
.name = "vdpa",
+ .dev_groups = vdpa_dev_groups,
+ .match = vdpa_dev_match,
.probe = vdpa_dev_probe,
.remove = vdpa_dev_remove,
};
@@ -68,6 +141,7 @@ static void vdpa_release_dev(struct device *d)
ida_simple_remove(&vdpa_index_ida, vdev->index);
mutex_destroy(&vdev->cf_mutex);
+ kfree(vdev->driver_override);
kfree(vdev);
}
--
2.31.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] vdpa: add driver_override support
2021-11-04 16:17 [PATCH] vdpa: add driver_override support Stefano Garzarella
@ 2021-11-05 3:01 ` Jason Wang
2021-11-05 8:04 ` Stefano Garzarella
2021-11-08 17:05 ` Stefano Garzarella
0 siblings, 2 replies; 8+ messages in thread
From: Jason Wang @ 2021-11-05 3:01 UTC (permalink / raw)
To: Stefano Garzarella
Cc: virtualization, Parav Pandit, Michael S. Tsirkin, linux-kernel
On Fri, Nov 5, 2021 at 12:17 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> `driver_override` allows to control which of the vDPA bus drivers
> binds to a vDPA device.
>
> If `driver_override` is not set, the previous behaviour is followed:
> devices use the first vDPA bus driver loaded (unless auto binding
> is disabled).
>
> Tested on Fedora 34 with driverctl(8):
> $ modprobe virtio-vdpa
> $ modprobe vhost-vdpa
> $ modprobe vdpa-sim-net
>
> $ vdpa dev add mgmtdev vdpasim_net name dev1
>
> # dev1 is attached to the first vDPA bus driver loaded
> $ driverctl -b vdpa list-devices
> dev1 virtio_vdpa
>
> $ driverctl -b vdpa set-override dev1 vhost_vdpa
>
> $ driverctl -b vdpa list-devices
> dev1 vhost_vdpa [*]
>
> Note: driverctl(8) integrates with udev so the binding is
> preserved.
>
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> include/linux/vdpa.h | 2 ++
> drivers/vdpa/vdpa.c | 74 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 76 insertions(+)
>
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index c3011ccda430..ae34015b37b7 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -64,6 +64,7 @@ struct vdpa_mgmt_dev;
> * struct vdpa_device - representation of a vDPA device
> * @dev: underlying device
> * @dma_dev: the actual device that is performing DMA
> + * @driver_override: driver name to force a match
This seems useless?
> * @config: the configuration ops for this device.
> * @cf_mutex: Protects get and set access to configuration layout.
> * @index: device index
> @@ -76,6 +77,7 @@ struct vdpa_mgmt_dev;
> struct vdpa_device {
> struct device dev;
> struct device *dma_dev;
> + const char *driver_override;
> const struct vdpa_config_ops *config;
> struct mutex cf_mutex; /* Protects get/set config */
> unsigned int index;
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 7332a74a4b00..659231bbfee8 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -52,8 +52,81 @@ static void vdpa_dev_remove(struct device *d)
> drv->remove(vdev);
> }
>
> +static int vdpa_dev_match(struct device *dev, struct device_driver *drv)
> +{
> + struct vdpa_device *vdev = dev_to_vdpa(dev);
> +
> + /* Check override first, and if set, only use the named driver */
> + if (vdev->driver_override)
> + return strcmp(vdev->driver_override, drv->name) == 0;
> +
> + /* Currently devices must be supported by all vDPA bus drivers */
> + return 1;
> +}
> +
> +static ssize_t driver_override_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct vdpa_device *vdev = dev_to_vdpa(dev);
> + const char *driver_override, *old;
> + char *cp;
> +
> + /* We need to keep extra room for a newline */
> + if (count >= (PAGE_SIZE - 1))
> + return -EINVAL;
> +
> + driver_override = kstrndup(buf, count, GFP_KERNEL);
> + if (!driver_override)
> + return -ENOMEM;
> +
> + cp = strchr(driver_override, '\n');
> + if (cp)
> + *cp = '\0';
> +
> + device_lock(dev);
> + old = vdev->driver_override;
> + if (strlen(driver_override)) {
> + vdev->driver_override = driver_override;
> + } else {
> + kfree(driver_override);
> + vdev->driver_override = NULL;
> + }
> + device_unlock(dev);
> +
> + kfree(old);
> +
> + return count;
> +}
> +
> +static ssize_t driver_override_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct vdpa_device *vdev = dev_to_vdpa(dev);
> + ssize_t len;
> +
> + device_lock(dev);
> + len = snprintf(buf, PAGE_SIZE, "%s\n", vdev->driver_override);
> + device_unlock(dev);
> +
> + return len;
> +}
> +static DEVICE_ATTR_RW(driver_override);
> +
> +static struct attribute *vdpa_dev_attrs[] = {
> + &dev_attr_driver_override.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group vdpa_dev_group = {
> + .attrs = vdpa_dev_attrs,
> +};
> +__ATTRIBUTE_GROUPS(vdpa_dev);
> +
> static struct bus_type vdpa_bus = {
> .name = "vdpa",
> + .dev_groups = vdpa_dev_groups,
This reminds me that we probably need to document the sysfs interface
in Documentation/ABI/testing/sysfs-bus-vdpa.
But it's not the fault of this patch which look good.
Thanks
> + .match = vdpa_dev_match,
> .probe = vdpa_dev_probe,
> .remove = vdpa_dev_remove,
> };
> @@ -68,6 +141,7 @@ static void vdpa_release_dev(struct device *d)
>
> ida_simple_remove(&vdpa_index_ida, vdev->index);
> mutex_destroy(&vdev->cf_mutex);
> + kfree(vdev->driver_override);
> kfree(vdev);
> }
>
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] vdpa: add driver_override support
2021-11-05 3:01 ` Jason Wang
@ 2021-11-05 8:04 ` Stefano Garzarella
2021-11-05 8:26 ` Jason Wang
2021-11-08 17:05 ` Stefano Garzarella
1 sibling, 1 reply; 8+ messages in thread
From: Stefano Garzarella @ 2021-11-05 8:04 UTC (permalink / raw)
To: Jason Wang; +Cc: virtualization, Parav Pandit, Michael S. Tsirkin, linux-kernel
On Fri, Nov 05, 2021 at 11:01:30AM +0800, Jason Wang wrote:
>On Fri, Nov 5, 2021 at 12:17 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> `driver_override` allows to control which of the vDPA bus drivers
>> binds to a vDPA device.
>>
>> If `driver_override` is not set, the previous behaviour is followed:
>> devices use the first vDPA bus driver loaded (unless auto binding
>> is disabled).
>>
>> Tested on Fedora 34 with driverctl(8):
>> $ modprobe virtio-vdpa
>> $ modprobe vhost-vdpa
>> $ modprobe vdpa-sim-net
>>
>> $ vdpa dev add mgmtdev vdpasim_net name dev1
>>
>> # dev1 is attached to the first vDPA bus driver loaded
>> $ driverctl -b vdpa list-devices
>> dev1 virtio_vdpa
>>
>> $ driverctl -b vdpa set-override dev1 vhost_vdpa
>>
>> $ driverctl -b vdpa list-devices
>> dev1 vhost_vdpa [*]
>>
>> Note: driverctl(8) integrates with udev so the binding is
>> preserved.
>>
>> Suggested-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>> include/linux/vdpa.h | 2 ++
>> drivers/vdpa/vdpa.c | 74 ++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 76 insertions(+)
>>
>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>> index c3011ccda430..ae34015b37b7 100644
>> --- a/include/linux/vdpa.h
>> +++ b/include/linux/vdpa.h
>> @@ -64,6 +64,7 @@ struct vdpa_mgmt_dev;
>> * struct vdpa_device - representation of a vDPA device
>> * @dev: underlying device
>> * @dma_dev: the actual device that is performing DMA
>> + * @driver_override: driver name to force a match
>
>This seems useless?
I'm a bit lost, do you mean we should remove the documentation of
`driver_override`?
>
>> * @config: the configuration ops for this device.
>> * @cf_mutex: Protects get and set access to configuration layout.
>> * @index: device index
>> @@ -76,6 +77,7 @@ struct vdpa_mgmt_dev;
>> struct vdpa_device {
>> struct device dev;
>> struct device *dma_dev;
>> + const char *driver_override;
>> const struct vdpa_config_ops *config;
>> struct mutex cf_mutex; /* Protects get/set config */
>> unsigned int index;
>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>> index 7332a74a4b00..659231bbfee8 100644
>> --- a/drivers/vdpa/vdpa.c
>> +++ b/drivers/vdpa/vdpa.c
>> @@ -52,8 +52,81 @@ static void vdpa_dev_remove(struct device *d)
>> drv->remove(vdev);
>> }
>>
>> +static int vdpa_dev_match(struct device *dev, struct device_driver *drv)
>> +{
>> + struct vdpa_device *vdev = dev_to_vdpa(dev);
>> +
>> + /* Check override first, and if set, only use the named driver */
>> + if (vdev->driver_override)
>> + return strcmp(vdev->driver_override, drv->name) == 0;
>> +
>> + /* Currently devices must be supported by all vDPA bus
>> drivers */
>> + return 1;
>> +}
>> +
>> +static ssize_t driver_override_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct vdpa_device *vdev = dev_to_vdpa(dev);
>> + const char *driver_override, *old;
>> + char *cp;
>> +
>> + /* We need to keep extra room for a newline */
>> + if (count >= (PAGE_SIZE - 1))
>> + return -EINVAL;
>> +
>> + driver_override = kstrndup(buf, count, GFP_KERNEL);
>> + if (!driver_override)
>> + return -ENOMEM;
>> +
>> + cp = strchr(driver_override, '\n');
>> + if (cp)
>> + *cp = '\0';
>> +
>> + device_lock(dev);
>> + old = vdev->driver_override;
>> + if (strlen(driver_override)) {
>> + vdev->driver_override = driver_override;
>> + } else {
>> + kfree(driver_override);
>> + vdev->driver_override = NULL;
>> + }
>> + device_unlock(dev);
>> +
>> + kfree(old);
>> +
>> + return count;
>> +}
>> +
>> +static ssize_t driver_override_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct vdpa_device *vdev = dev_to_vdpa(dev);
>> + ssize_t len;
>> +
>> + device_lock(dev);
>> + len = snprintf(buf, PAGE_SIZE, "%s\n", vdev->driver_override);
>> + device_unlock(dev);
>> +
>> + return len;
>> +}
>> +static DEVICE_ATTR_RW(driver_override);
>> +
>> +static struct attribute *vdpa_dev_attrs[] = {
>> + &dev_attr_driver_override.attr,
>> + NULL,
>> +};
>> +
>> +static const struct attribute_group vdpa_dev_group = {
>> + .attrs = vdpa_dev_attrs,
>> +};
>> +__ATTRIBUTE_GROUPS(vdpa_dev);
>> +
>> static struct bus_type vdpa_bus = {
>> .name = "vdpa",
>> + .dev_groups = vdpa_dev_groups,
>
>This reminds me that we probably need to document the sysfs interface
>in Documentation/ABI/testing/sysfs-bus-vdpa.
>
Yeah, I'd like to add more documentation. We should also document the
device life cycle and management API.
It's on my to-do list, I'm prioritizing it!
>But it's not the fault of this patch which look good.
Thanks,
Stefano
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] vdpa: add driver_override support
2021-11-05 8:04 ` Stefano Garzarella
@ 2021-11-05 8:26 ` Jason Wang
2021-11-05 8:31 ` Stefano Garzarella
0 siblings, 1 reply; 8+ messages in thread
From: Jason Wang @ 2021-11-05 8:26 UTC (permalink / raw)
To: Stefano Garzarella
Cc: virtualization, Parav Pandit, Michael S. Tsirkin, linux-kernel
On Fri, Nov 5, 2021 at 4:05 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Nov 05, 2021 at 11:01:30AM +0800, Jason Wang wrote:
> >On Fri, Nov 5, 2021 at 12:17 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >>
> >> `driver_override` allows to control which of the vDPA bus drivers
> >> binds to a vDPA device.
> >>
> >> If `driver_override` is not set, the previous behaviour is followed:
> >> devices use the first vDPA bus driver loaded (unless auto binding
> >> is disabled).
> >>
> >> Tested on Fedora 34 with driverctl(8):
> >> $ modprobe virtio-vdpa
> >> $ modprobe vhost-vdpa
> >> $ modprobe vdpa-sim-net
> >>
> >> $ vdpa dev add mgmtdev vdpasim_net name dev1
> >>
> >> # dev1 is attached to the first vDPA bus driver loaded
> >> $ driverctl -b vdpa list-devices
> >> dev1 virtio_vdpa
> >>
> >> $ driverctl -b vdpa set-override dev1 vhost_vdpa
> >>
> >> $ driverctl -b vdpa list-devices
> >> dev1 vhost_vdpa [*]
> >>
> >> Note: driverctl(8) integrates with udev so the binding is
> >> preserved.
> >>
> >> Suggested-by: Jason Wang <jasowang@redhat.com>
> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> >> ---
> >> include/linux/vdpa.h | 2 ++
> >> drivers/vdpa/vdpa.c | 74 ++++++++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 76 insertions(+)
> >>
> >> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> >> index c3011ccda430..ae34015b37b7 100644
> >> --- a/include/linux/vdpa.h
> >> +++ b/include/linux/vdpa.h
> >> @@ -64,6 +64,7 @@ struct vdpa_mgmt_dev;
> >> * struct vdpa_device - representation of a vDPA device
> >> * @dev: underlying device
> >> * @dma_dev: the actual device that is performing DMA
> >> + * @driver_override: driver name to force a match
> >
> >This seems useless?
>
> I'm a bit lost, do you mean we should remove the documentation of
> `driver_override`?
I misread the code which was misled by vdpa_mgmt_dev above:(
The code should be fine.
So:
Acked-by: Jason Wang <jasowang@redhat.com>
>
> >
> >> * @config: the configuration ops for this device.
> >> * @cf_mutex: Protects get and set access to configuration layout.
> >> * @index: device index
> >> @@ -76,6 +77,7 @@ struct vdpa_mgmt_dev;
> >> struct vdpa_device {
> >> struct device dev;
> >> struct device *dma_dev;
> >> + const char *driver_override;
> >> const struct vdpa_config_ops *config;
> >> struct mutex cf_mutex; /* Protects get/set config */
> >> unsigned int index;
> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> >> index 7332a74a4b00..659231bbfee8 100644
> >> --- a/drivers/vdpa/vdpa.c
> >> +++ b/drivers/vdpa/vdpa.c
> >> @@ -52,8 +52,81 @@ static void vdpa_dev_remove(struct device *d)
> >> drv->remove(vdev);
> >> }
> >>
> >> +static int vdpa_dev_match(struct device *dev, struct device_driver *drv)
> >> +{
> >> + struct vdpa_device *vdev = dev_to_vdpa(dev);
> >> +
> >> + /* Check override first, and if set, only use the named driver */
> >> + if (vdev->driver_override)
> >> + return strcmp(vdev->driver_override, drv->name) == 0;
> >> +
> >> + /* Currently devices must be supported by all vDPA bus
> >> drivers */
> >> + return 1;
> >> +}
> >> +
> >> +static ssize_t driver_override_store(struct device *dev,
> >> + struct device_attribute *attr,
> >> + const char *buf, size_t count)
> >> +{
> >> + struct vdpa_device *vdev = dev_to_vdpa(dev);
> >> + const char *driver_override, *old;
> >> + char *cp;
> >> +
> >> + /* We need to keep extra room for a newline */
> >> + if (count >= (PAGE_SIZE - 1))
> >> + return -EINVAL;
> >> +
> >> + driver_override = kstrndup(buf, count, GFP_KERNEL);
> >> + if (!driver_override)
> >> + return -ENOMEM;
> >> +
> >> + cp = strchr(driver_override, '\n');
> >> + if (cp)
> >> + *cp = '\0';
> >> +
> >> + device_lock(dev);
> >> + old = vdev->driver_override;
> >> + if (strlen(driver_override)) {
> >> + vdev->driver_override = driver_override;
> >> + } else {
> >> + kfree(driver_override);
> >> + vdev->driver_override = NULL;
> >> + }
> >> + device_unlock(dev);
> >> +
> >> + kfree(old);
> >> +
> >> + return count;
> >> +}
> >> +
> >> +static ssize_t driver_override_show(struct device *dev,
> >> + struct device_attribute *attr, char *buf)
> >> +{
> >> + struct vdpa_device *vdev = dev_to_vdpa(dev);
> >> + ssize_t len;
> >> +
> >> + device_lock(dev);
> >> + len = snprintf(buf, PAGE_SIZE, "%s\n", vdev->driver_override);
> >> + device_unlock(dev);
> >> +
> >> + return len;
> >> +}
> >> +static DEVICE_ATTR_RW(driver_override);
> >> +
> >> +static struct attribute *vdpa_dev_attrs[] = {
> >> + &dev_attr_driver_override.attr,
> >> + NULL,
> >> +};
> >> +
> >> +static const struct attribute_group vdpa_dev_group = {
> >> + .attrs = vdpa_dev_attrs,
> >> +};
> >> +__ATTRIBUTE_GROUPS(vdpa_dev);
> >> +
> >> static struct bus_type vdpa_bus = {
> >> .name = "vdpa",
> >> + .dev_groups = vdpa_dev_groups,
> >
> >This reminds me that we probably need to document the sysfs interface
> >in Documentation/ABI/testing/sysfs-bus-vdpa.
> >
>
> Yeah, I'd like to add more documentation. We should also document the
> device life cycle and management API.
>
> It's on my to-do list, I'm prioritizing it!
>
> >But it's not the fault of this patch which look good.
>
> Thanks,
> Stefano
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] vdpa: add driver_override support
2021-11-05 8:26 ` Jason Wang
@ 2021-11-05 8:31 ` Stefano Garzarella
0 siblings, 0 replies; 8+ messages in thread
From: Stefano Garzarella @ 2021-11-05 8:31 UTC (permalink / raw)
To: Jason Wang; +Cc: virtualization, Parav Pandit, Michael S. Tsirkin, linux-kernel
On Fri, Nov 05, 2021 at 04:26:44PM +0800, Jason Wang wrote:
>On Fri, Nov 5, 2021 at 4:05 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Fri, Nov 05, 2021 at 11:01:30AM +0800, Jason Wang wrote:
>> >On Fri, Nov 5, 2021 at 12:17 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >>
>> >> `driver_override` allows to control which of the vDPA bus drivers
>> >> binds to a vDPA device.
>> >>
>> >> If `driver_override` is not set, the previous behaviour is followed:
>> >> devices use the first vDPA bus driver loaded (unless auto binding
>> >> is disabled).
>> >>
>> >> Tested on Fedora 34 with driverctl(8):
>> >> $ modprobe virtio-vdpa
>> >> $ modprobe vhost-vdpa
>> >> $ modprobe vdpa-sim-net
>> >>
>> >> $ vdpa dev add mgmtdev vdpasim_net name dev1
>> >>
>> >> # dev1 is attached to the first vDPA bus driver loaded
>> >> $ driverctl -b vdpa list-devices
>> >> dev1 virtio_vdpa
>> >>
>> >> $ driverctl -b vdpa set-override dev1 vhost_vdpa
>> >>
>> >> $ driverctl -b vdpa list-devices
>> >> dev1 vhost_vdpa [*]
>> >>
>> >> Note: driverctl(8) integrates with udev so the binding is
>> >> preserved.
>> >>
>> >> Suggested-by: Jason Wang <jasowang@redhat.com>
>> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> >> ---
>> >> include/linux/vdpa.h | 2 ++
>> >> drivers/vdpa/vdpa.c | 74 ++++++++++++++++++++++++++++++++++++++++++++
>> >> 2 files changed, 76 insertions(+)
>> >>
>> >> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>> >> index c3011ccda430..ae34015b37b7 100644
>> >> --- a/include/linux/vdpa.h
>> >> +++ b/include/linux/vdpa.h
>> >> @@ -64,6 +64,7 @@ struct vdpa_mgmt_dev;
>> >> * struct vdpa_device - representation of a vDPA device
>> >> * @dev: underlying device
>> >> * @dma_dev: the actual device that is performing DMA
>> >> + * @driver_override: driver name to force a match
>> >
>> >This seems useless?
>>
>> I'm a bit lost, do you mean we should remove the documentation of
>> `driver_override`?
>
>I misread the code which was misled by vdpa_mgmt_dev above:(
Yeah, the same thing happened to me now while double checking ;-)
>
>The code should be fine.
>
>So:
>
>Acked-by: Jason Wang <jasowang@redhat.com>
>
Thanks,
Stefano
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] vdpa: add driver_override support
2021-11-05 3:01 ` Jason Wang
2021-11-05 8:04 ` Stefano Garzarella
@ 2021-11-08 17:05 ` Stefano Garzarella
2021-11-09 13:10 ` Michael S. Tsirkin
1 sibling, 1 reply; 8+ messages in thread
From: Stefano Garzarella @ 2021-11-08 17:05 UTC (permalink / raw)
To: Jason Wang, Michael S. Tsirkin; +Cc: virtualization, Parav Pandit, linux-kernel
On Fri, Nov 5, 2021 at 4:01 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Nov 5, 2021 at 12:17 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >
> > `driver_override` allows to control which of the vDPA bus drivers
> > binds to a vDPA device.
> >
> > If `driver_override` is not set, the previous behaviour is followed:
> > devices use the first vDPA bus driver loaded (unless auto binding
> > is disabled).
> >
> > Tested on Fedora 34 with driverctl(8):
> > $ modprobe virtio-vdpa
> > $ modprobe vhost-vdpa
> > $ modprobe vdpa-sim-net
> >
> > $ vdpa dev add mgmtdev vdpasim_net name dev1
> >
> > # dev1 is attached to the first vDPA bus driver loaded
> > $ driverctl -b vdpa list-devices
> > dev1 virtio_vdpa
> >
> > $ driverctl -b vdpa set-override dev1 vhost_vdpa
> >
> > $ driverctl -b vdpa list-devices
> > dev1 vhost_vdpa [*]
> >
> > Note: driverctl(8) integrates with udev so the binding is
> > preserved.
> >
> > Suggested-by: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> > include/linux/vdpa.h | 2 ++
> > drivers/vdpa/vdpa.c | 74 ++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 76 insertions(+)
> >
> > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> > index c3011ccda430..ae34015b37b7 100644
> > --- a/include/linux/vdpa.h
> > +++ b/include/linux/vdpa.h
> > @@ -64,6 +64,7 @@ struct vdpa_mgmt_dev;
> > * struct vdpa_device - representation of a vDPA device
> > * @dev: underlying device
> > * @dma_dev: the actual device that is performing DMA
> > + * @driver_override: driver name to force a match
>
> This seems useless?
>
> > * @config: the configuration ops for this device.
> > * @cf_mutex: Protects get and set access to configuration layout.
> > * @index: device index
> > @@ -76,6 +77,7 @@ struct vdpa_mgmt_dev;
> > struct vdpa_device {
> > struct device dev;
> > struct device *dma_dev;
> > + const char *driver_override;
> > const struct vdpa_config_ops *config;
> > struct mutex cf_mutex; /* Protects get/set config */
> > unsigned int index;
> > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > index 7332a74a4b00..659231bbfee8 100644
> > --- a/drivers/vdpa/vdpa.c
> > +++ b/drivers/vdpa/vdpa.c
> > @@ -52,8 +52,81 @@ static void vdpa_dev_remove(struct device *d)
> > drv->remove(vdev);
> > }
> >
> > +static int vdpa_dev_match(struct device *dev, struct device_driver *drv)
> > +{
> > + struct vdpa_device *vdev = dev_to_vdpa(dev);
> > +
> > + /* Check override first, and if set, only use the named driver */
> > + if (vdev->driver_override)
> > + return strcmp(vdev->driver_override, drv->name) == 0;
> > +
> > + /* Currently devices must be supported by all vDPA bus drivers */
> > + return 1;
> > +}
> > +
> > +static ssize_t driver_override_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct vdpa_device *vdev = dev_to_vdpa(dev);
> > + const char *driver_override, *old;
> > + char *cp;
> > +
> > + /* We need to keep extra room for a newline */
> > + if (count >= (PAGE_SIZE - 1))
> > + return -EINVAL;
> > +
> > + driver_override = kstrndup(buf, count, GFP_KERNEL);
> > + if (!driver_override)
> > + return -ENOMEM;
> > +
> > + cp = strchr(driver_override, '\n');
> > + if (cp)
> > + *cp = '\0';
> > +
> > + device_lock(dev);
> > + old = vdev->driver_override;
> > + if (strlen(driver_override)) {
> > + vdev->driver_override = driver_override;
> > + } else {
> > + kfree(driver_override);
> > + vdev->driver_override = NULL;
> > + }
> > + device_unlock(dev);
> > +
> > + kfree(old);
> > +
> > + return count;
> > +}
> > +
> > +static ssize_t driver_override_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct vdpa_device *vdev = dev_to_vdpa(dev);
> > + ssize_t len;
> > +
> > + device_lock(dev);
> > + len = snprintf(buf, PAGE_SIZE, "%s\n", vdev->driver_override);
> > + device_unlock(dev);
> > +
> > + return len;
> > +}
> > +static DEVICE_ATTR_RW(driver_override);
> > +
> > +static struct attribute *vdpa_dev_attrs[] = {
> > + &dev_attr_driver_override.attr,
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group vdpa_dev_group = {
> > + .attrs = vdpa_dev_attrs,
> > +};
> > +__ATTRIBUTE_GROUPS(vdpa_dev);
> > +
> > static struct bus_type vdpa_bus = {
> > .name = "vdpa",
> > + .dev_groups = vdpa_dev_groups,
>
> This reminds me that we probably need to document the sysfs interface
> in Documentation/ABI/testing/sysfs-bus-vdpa.
>
> But it's not the fault of this patch which look good.
Michael, Jason, about Documentation/ABI/testing/sysfs-bus-vdpa, do you
think is better to send a follow up patch/series, maybe including also
others entries (e.g. bind, unbind) or a v2 including the documentation
of `driver_ovveride`?
Thanks,
Stefano
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] vdpa: add driver_override support
2021-11-08 17:05 ` Stefano Garzarella
@ 2021-11-09 13:10 ` Michael S. Tsirkin
2021-11-09 13:31 ` Stefano Garzarella
0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2021-11-09 13:10 UTC (permalink / raw)
To: Stefano Garzarella; +Cc: Jason Wang, virtualization, Parav Pandit, linux-kernel
On Mon, Nov 08, 2021 at 06:05:29PM +0100, Stefano Garzarella wrote:
> On Fri, Nov 5, 2021 at 4:01 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Fri, Nov 5, 2021 at 12:17 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > >
> > > `driver_override` allows to control which of the vDPA bus drivers
> > > binds to a vDPA device.
> > >
> > > If `driver_override` is not set, the previous behaviour is followed:
> > > devices use the first vDPA bus driver loaded (unless auto binding
> > > is disabled).
> > >
> > > Tested on Fedora 34 with driverctl(8):
> > > $ modprobe virtio-vdpa
> > > $ modprobe vhost-vdpa
> > > $ modprobe vdpa-sim-net
> > >
> > > $ vdpa dev add mgmtdev vdpasim_net name dev1
> > >
> > > # dev1 is attached to the first vDPA bus driver loaded
> > > $ driverctl -b vdpa list-devices
> > > dev1 virtio_vdpa
> > >
> > > $ driverctl -b vdpa set-override dev1 vhost_vdpa
> > >
> > > $ driverctl -b vdpa list-devices
> > > dev1 vhost_vdpa [*]
> > >
> > > Note: driverctl(8) integrates with udev so the binding is
> > > preserved.
> > >
> > > Suggested-by: Jason Wang <jasowang@redhat.com>
> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > ---
> > > include/linux/vdpa.h | 2 ++
> > > drivers/vdpa/vdpa.c | 74 ++++++++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 76 insertions(+)
> > >
> > > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> > > index c3011ccda430..ae34015b37b7 100644
> > > --- a/include/linux/vdpa.h
> > > +++ b/include/linux/vdpa.h
> > > @@ -64,6 +64,7 @@ struct vdpa_mgmt_dev;
> > > * struct vdpa_device - representation of a vDPA device
> > > * @dev: underlying device
> > > * @dma_dev: the actual device that is performing DMA
> > > + * @driver_override: driver name to force a match
> >
> > This seems useless?
> >
> > > * @config: the configuration ops for this device.
> > > * @cf_mutex: Protects get and set access to configuration layout.
> > > * @index: device index
> > > @@ -76,6 +77,7 @@ struct vdpa_mgmt_dev;
> > > struct vdpa_device {
> > > struct device dev;
> > > struct device *dma_dev;
> > > + const char *driver_override;
> > > const struct vdpa_config_ops *config;
> > > struct mutex cf_mutex; /* Protects get/set config */
> > > unsigned int index;
> > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > > index 7332a74a4b00..659231bbfee8 100644
> > > --- a/drivers/vdpa/vdpa.c
> > > +++ b/drivers/vdpa/vdpa.c
> > > @@ -52,8 +52,81 @@ static void vdpa_dev_remove(struct device *d)
> > > drv->remove(vdev);
> > > }
> > >
> > > +static int vdpa_dev_match(struct device *dev, struct device_driver *drv)
> > > +{
> > > + struct vdpa_device *vdev = dev_to_vdpa(dev);
> > > +
> > > + /* Check override first, and if set, only use the named driver */
> > > + if (vdev->driver_override)
> > > + return strcmp(vdev->driver_override, drv->name) == 0;
> > > +
> > > + /* Currently devices must be supported by all vDPA bus drivers */
> > > + return 1;
> > > +}
> > > +
> > > +static ssize_t driver_override_store(struct device *dev,
> > > + struct device_attribute *attr,
> > > + const char *buf, size_t count)
> > > +{
> > > + struct vdpa_device *vdev = dev_to_vdpa(dev);
> > > + const char *driver_override, *old;
> > > + char *cp;
> > > +
> > > + /* We need to keep extra room for a newline */
> > > + if (count >= (PAGE_SIZE - 1))
> > > + return -EINVAL;
> > > +
> > > + driver_override = kstrndup(buf, count, GFP_KERNEL);
> > > + if (!driver_override)
> > > + return -ENOMEM;
> > > +
> > > + cp = strchr(driver_override, '\n');
> > > + if (cp)
> > > + *cp = '\0';
> > > +
> > > + device_lock(dev);
> > > + old = vdev->driver_override;
> > > + if (strlen(driver_override)) {
> > > + vdev->driver_override = driver_override;
> > > + } else {
> > > + kfree(driver_override);
> > > + vdev->driver_override = NULL;
> > > + }
> > > + device_unlock(dev);
> > > +
> > > + kfree(old);
> > > +
> > > + return count;
> > > +}
> > > +
> > > +static ssize_t driver_override_show(struct device *dev,
> > > + struct device_attribute *attr, char *buf)
> > > +{
> > > + struct vdpa_device *vdev = dev_to_vdpa(dev);
> > > + ssize_t len;
> > > +
> > > + device_lock(dev);
> > > + len = snprintf(buf, PAGE_SIZE, "%s\n", vdev->driver_override);
> > > + device_unlock(dev);
> > > +
> > > + return len;
> > > +}
> > > +static DEVICE_ATTR_RW(driver_override);
> > > +
> > > +static struct attribute *vdpa_dev_attrs[] = {
> > > + &dev_attr_driver_override.attr,
> > > + NULL,
> > > +};
> > > +
> > > +static const struct attribute_group vdpa_dev_group = {
> > > + .attrs = vdpa_dev_attrs,
> > > +};
> > > +__ATTRIBUTE_GROUPS(vdpa_dev);
> > > +
> > > static struct bus_type vdpa_bus = {
> > > .name = "vdpa",
> > > + .dev_groups = vdpa_dev_groups,
> >
> > This reminds me that we probably need to document the sysfs interface
> > in Documentation/ABI/testing/sysfs-bus-vdpa.
> >
> > But it's not the fault of this patch which look good.
>
> Michael, Jason, about Documentation/ABI/testing/sysfs-bus-vdpa, do you
> think is better to send a follow up patch/series, maybe including also
> others entries (e.g. bind, unbind) or a v2 including the documentation
> of `driver_ovveride`?
>
> Thanks,
> Stefano
I'd include it as a patch in series.
--
MST
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] vdpa: add driver_override support
2021-11-09 13:10 ` Michael S. Tsirkin
@ 2021-11-09 13:31 ` Stefano Garzarella
0 siblings, 0 replies; 8+ messages in thread
From: Stefano Garzarella @ 2021-11-09 13:31 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jason Wang, virtualization, Parav Pandit, linux-kernel
On Tue, Nov 09, 2021 at 08:10:58AM -0500, Michael S. Tsirkin wrote:
>On Mon, Nov 08, 2021 at 06:05:29PM +0100, Stefano Garzarella wrote:
>> On Fri, Nov 5, 2021 at 4:01 AM Jason Wang <jasowang@redhat.com> wrote:
>> >
>> > On Fri, Nov 5, 2021 at 12:17 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> > >
>> > > `driver_override` allows to control which of the vDPA bus drivers
>> > > binds to a vDPA device.
>> > >
>> > > If `driver_override` is not set, the previous behaviour is followed:
>> > > devices use the first vDPA bus driver loaded (unless auto binding
>> > > is disabled).
>> > >
>> > > Tested on Fedora 34 with driverctl(8):
>> > > $ modprobe virtio-vdpa
>> > > $ modprobe vhost-vdpa
>> > > $ modprobe vdpa-sim-net
>> > >
>> > > $ vdpa dev add mgmtdev vdpasim_net name dev1
>> > >
>> > > # dev1 is attached to the first vDPA bus driver loaded
>> > > $ driverctl -b vdpa list-devices
>> > > dev1 virtio_vdpa
>> > >
>> > > $ driverctl -b vdpa set-override dev1 vhost_vdpa
>> > >
>> > > $ driverctl -b vdpa list-devices
>> > > dev1 vhost_vdpa [*]
>> > >
>> > > Note: driverctl(8) integrates with udev so the binding is
>> > > preserved.
>> > >
>> > > Suggested-by: Jason Wang <jasowang@redhat.com>
>> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> > > ---
>> > > include/linux/vdpa.h | 2 ++
>> > > drivers/vdpa/vdpa.c | 74 ++++++++++++++++++++++++++++++++++++++++++++
>> > > 2 files changed, 76 insertions(+)
>> > >
>> > > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>> > > index c3011ccda430..ae34015b37b7 100644
>> > > --- a/include/linux/vdpa.h
>> > > +++ b/include/linux/vdpa.h
>> > > @@ -64,6 +64,7 @@ struct vdpa_mgmt_dev;
>> > > * struct vdpa_device - representation of a vDPA device
>> > > * @dev: underlying device
>> > > * @dma_dev: the actual device that is performing DMA
>> > > + * @driver_override: driver name to force a match
>> >
>> > This seems useless?
>> >
>> > > * @config: the configuration ops for this device.
>> > > * @cf_mutex: Protects get and set access to configuration layout.
>> > > * @index: device index
>> > > @@ -76,6 +77,7 @@ struct vdpa_mgmt_dev;
>> > > struct vdpa_device {
>> > > struct device dev;
>> > > struct device *dma_dev;
>> > > + const char *driver_override;
>> > > const struct vdpa_config_ops *config;
>> > > struct mutex cf_mutex; /* Protects get/set config */
>> > > unsigned int index;
>> > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>> > > index 7332a74a4b00..659231bbfee8 100644
>> > > --- a/drivers/vdpa/vdpa.c
>> > > +++ b/drivers/vdpa/vdpa.c
>> > > @@ -52,8 +52,81 @@ static void vdpa_dev_remove(struct device *d)
>> > > drv->remove(vdev);
>> > > }
>> > >
>> > > +static int vdpa_dev_match(struct device *dev, struct device_driver *drv)
>> > > +{
>> > > + struct vdpa_device *vdev = dev_to_vdpa(dev);
>> > > +
>> > > + /* Check override first, and if set, only use the named driver */
>> > > + if (vdev->driver_override)
>> > > + return strcmp(vdev->driver_override, drv->name) == 0;
>> > > +
>> > > + /* Currently devices must be supported by all vDPA bus drivers */
>> > > + return 1;
>> > > +}
>> > > +
>> > > +static ssize_t driver_override_store(struct device *dev,
>> > > + struct device_attribute *attr,
>> > > + const char *buf, size_t count)
>> > > +{
>> > > + struct vdpa_device *vdev = dev_to_vdpa(dev);
>> > > + const char *driver_override, *old;
>> > > + char *cp;
>> > > +
>> > > + /* We need to keep extra room for a newline */
>> > > + if (count >= (PAGE_SIZE - 1))
>> > > + return -EINVAL;
>> > > +
>> > > + driver_override = kstrndup(buf, count, GFP_KERNEL);
>> > > + if (!driver_override)
>> > > + return -ENOMEM;
>> > > +
>> > > + cp = strchr(driver_override, '\n');
>> > > + if (cp)
>> > > + *cp = '\0';
>> > > +
>> > > + device_lock(dev);
>> > > + old = vdev->driver_override;
>> > > + if (strlen(driver_override)) {
>> > > + vdev->driver_override = driver_override;
>> > > + } else {
>> > > + kfree(driver_override);
>> > > + vdev->driver_override = NULL;
>> > > + }
>> > > + device_unlock(dev);
>> > > +
>> > > + kfree(old);
>> > > +
>> > > + return count;
>> > > +}
>> > > +
>> > > +static ssize_t driver_override_show(struct device *dev,
>> > > + struct device_attribute *attr, char *buf)
>> > > +{
>> > > + struct vdpa_device *vdev = dev_to_vdpa(dev);
>> > > + ssize_t len;
>> > > +
>> > > + device_lock(dev);
>> > > + len = snprintf(buf, PAGE_SIZE, "%s\n", vdev->driver_override);
>> > > + device_unlock(dev);
>> > > +
>> > > + return len;
>> > > +}
>> > > +static DEVICE_ATTR_RW(driver_override);
>> > > +
>> > > +static struct attribute *vdpa_dev_attrs[] = {
>> > > + &dev_attr_driver_override.attr,
>> > > + NULL,
>> > > +};
>> > > +
>> > > +static const struct attribute_group vdpa_dev_group = {
>> > > + .attrs = vdpa_dev_attrs,
>> > > +};
>> > > +__ATTRIBUTE_GROUPS(vdpa_dev);
>> > > +
>> > > static struct bus_type vdpa_bus = {
>> > > .name = "vdpa",
>> > > + .dev_groups = vdpa_dev_groups,
>> >
>> > This reminds me that we probably need to document the sysfs interface
>> > in Documentation/ABI/testing/sysfs-bus-vdpa.
>> >
>> > But it's not the fault of this patch which look good.
>>
>> Michael, Jason, about Documentation/ABI/testing/sysfs-bus-vdpa, do you
>> think is better to send a follow up patch/series, maybe including also
>> others entries (e.g. bind, unbind) or a v2 including the documentation
>> of `driver_ovveride`?
>>
>> Thanks,
>> Stefano
>
>I'd include it as a patch in series.
>
Okay, so I'll send the documentation in a followup series.
Thanks,
Stefano
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-11-09 13:32 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-04 16:17 [PATCH] vdpa: add driver_override support Stefano Garzarella
2021-11-05 3:01 ` Jason Wang
2021-11-05 8:04 ` Stefano Garzarella
2021-11-05 8:26 ` Jason Wang
2021-11-05 8:31 ` Stefano Garzarella
2021-11-08 17:05 ` Stefano Garzarella
2021-11-09 13:10 ` Michael S. Tsirkin
2021-11-09 13:31 ` Stefano Garzarella
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).