Stefano Garzarella
2021-Jan-27 08:57 UTC
[RFC v3 03/11] vdpa: Remove the restriction that only supports virtio-net devices
On Wed, Jan 27, 2021 at 11:33:03AM +0800, Jason Wang wrote:> >On 2021/1/20 ??7:08, Stefano Garzarella wrote: >>On Wed, Jan 20, 2021 at 11:46:38AM +0800, Jason Wang wrote: >>> >>>On 2021/1/19 ??12:59, Xie Yongji wrote: >>>>With VDUSE, we should be able to support all kinds of virtio devices. >>>> >>>>Signed-off-by: Xie Yongji <xieyongji at bytedance.com> >>>>--- >>>>?drivers/vhost/vdpa.c | 29 +++-------------------------- >>>>?1 file changed, 3 insertions(+), 26 deletions(-) >>>> >>>>diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c >>>>index 29ed4173f04e..448be7875b6d 100644 >>>>--- a/drivers/vhost/vdpa.c >>>>+++ b/drivers/vhost/vdpa.c >>>>@@ -22,6 +22,7 @@ >>>>?#include <linux/nospec.h> >>>>?#include <linux/vhost.h> >>>>?#include <linux/virtio_net.h> >>>>+#include <linux/virtio_blk.h> >>>>?#include "vhost.h" >>>>@@ -185,26 +186,6 @@ static long vhost_vdpa_set_status(struct >>>>vhost_vdpa *v, u8 __user *statusp) >>>>???? return 0; >>>>?} >>>>-static int vhost_vdpa_config_validate(struct vhost_vdpa *v, >>>>-????????????????????? struct vhost_vdpa_config *c) >>>>-{ >>>>-??? long size = 0; >>>>- >>>>-??? switch (v->virtio_id) { >>>>-??? case VIRTIO_ID_NET: >>>>-??????? size = sizeof(struct virtio_net_config); >>>>-??????? break; >>>>-??? } >>>>- >>>>-??? if (c->len == 0) >>>>-??????? return -EINVAL; >>>>- >>>>-??? if (c->len > size - c->off) >>>>-??????? return -E2BIG; >>>>- >>>>-??? return 0; >>>>-} >>> >>> >>>I think we should use a separate patch for this. >> >>For the vdpa-blk simulator I had the same issues and I'm adding a >>.get_config_size() callback to vdpa devices. >> >>Do you think make sense or is better to remove this check in >>vhost/vdpa, delegating the boundaries checks to >>get_config/set_config callbacks. > > >A question here. How much value could we gain from get_config_size() >consider we can let vDPA parent to validate the length in its >get_config(). >I agree, most of the implementations already validate the length, the only gain is an error returned since get_config() is void, but eventually we can add a return value to it. Thanks, Stefano
Jason Wang
2021-Jan-28 03:11 UTC
[RFC v3 03/11] vdpa: Remove the restriction that only supports virtio-net devices
On 2021/1/27 ??4:57, Stefano Garzarella wrote:> On Wed, Jan 27, 2021 at 11:33:03AM +0800, Jason Wang wrote: >> >> On 2021/1/20 ??7:08, Stefano Garzarella wrote: >>> On Wed, Jan 20, 2021 at 11:46:38AM +0800, Jason Wang wrote: >>>> >>>> On 2021/1/19 ??12:59, Xie Yongji wrote: >>>>> With VDUSE, we should be able to support all kinds of virtio devices. >>>>> >>>>> Signed-off-by: Xie Yongji <xieyongji at bytedance.com> >>>>> --- >>>>> ?drivers/vhost/vdpa.c | 29 +++-------------------------- >>>>> ?1 file changed, 3 insertions(+), 26 deletions(-) >>>>> >>>>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c >>>>> index 29ed4173f04e..448be7875b6d 100644 >>>>> --- a/drivers/vhost/vdpa.c >>>>> +++ b/drivers/vhost/vdpa.c >>>>> @@ -22,6 +22,7 @@ >>>>> ?#include <linux/nospec.h> >>>>> ?#include <linux/vhost.h> >>>>> ?#include <linux/virtio_net.h> >>>>> +#include <linux/virtio_blk.h> >>>>> ?#include "vhost.h" >>>>> @@ -185,26 +186,6 @@ static long vhost_vdpa_set_status(struct >>>>> vhost_vdpa *v, u8 __user *statusp) >>>>> ???? return 0; >>>>> ?} >>>>> -static int vhost_vdpa_config_validate(struct vhost_vdpa *v, >>>>> -????????????????????? struct vhost_vdpa_config *c) >>>>> -{ >>>>> -??? long size = 0; >>>>> - >>>>> -??? switch (v->virtio_id) { >>>>> -??? case VIRTIO_ID_NET: >>>>> -??????? size = sizeof(struct virtio_net_config); >>>>> -??????? break; >>>>> -??? } >>>>> - >>>>> -??? if (c->len == 0) >>>>> -??????? return -EINVAL; >>>>> - >>>>> -??? if (c->len > size - c->off) >>>>> -??????? return -E2BIG; >>>>> - >>>>> -??? return 0; >>>>> -} >>>> >>>> >>>> I think we should use a separate patch for this. >>> >>> For the vdpa-blk simulator I had the same issues and I'm adding a >>> .get_config_size() callback to vdpa devices. >>> >>> Do you think make sense or is better to remove this check in >>> vhost/vdpa, delegating the boundaries checks to >>> get_config/set_config callbacks. >> >> >> A question here. How much value could we gain from get_config_size() >> consider we can let vDPA parent to validate the length in its >> get_config(). >> > > I agree, most of the implementations already validate the length, the > only gain is an error returned since get_config() is void, but > eventually we can add a return value to it.Right, one problem here is that. For the virito path, its get_config() returns void. So we can not propagate error to virtio drivers. But it might not be a big issue since we trust kernel virtio driver. So I think it makes sense to change the return value in the vdpa config ops. Thanks> > Thanks, > Stefano >