Re: [PATCH v2 4/5] vdpa: add new vdpa attribute VDPA_ATTR_DEV_F_VERSION_1
On Fri, Sep 17, 2021 at 10:35 AM Wu Zongyong
wrote:
>
> On Thu, Sep 16, 2021 at 09:05:58AM +0800, Jason Wang wrote:
> > On Wed, Sep 15, 2021 at 7:09 PM Michael S. Tsirkin wrote:
> > >
> > > On Wed, Sep 15, 2021 at 04:06:57PM +0800, Jason Wang wrote:
> > > > On Wed, Sep 15, 2021 at 3:38 PM Michael S. Tsirkin
> > > > wrote:
> > > > >
> > > > > On Wed, Sep 15, 2021 at 11:18:06AM +0800, Jason Wang wrote:
> > > > > > On Tue, Sep 14, 2021 at 8:58 PM Michael S. Tsirkin
> > > > > > wrote:
> > > > > > >
> > > > > > > On Tue, Sep 14, 2021 at 08:24:51PM +0800, Wu Zongyong wrote:
> > > > > > > > This new attribute advertises whether the vdpa device is legacy
> > > > > > > > or not.
> > > > > > > > Users can pick right virtqueue size if the vdpa device is
> > > > > > > > legacy which
> > > > > > > > doesn't support to change virtqueue size.
> > > > > > > >
> > > > > > > > Signed-off-by: Wu Zongyong
> > > > > > >
> > > > > > > So if we are bothering with legacy,
> > > > > >
> > > > > > I think we'd better not. I guess the following may work:
> > > > > >
> > > > > > 1) disable the driver on BE host
> > > > > > 2) present VERSION_1 with ACCESS_PLATFORM in get_features()
> > > > > > 3) extend the management to advertise max_queue_size and
> > > > > > min_queue_size, for ENI they are the same so management layer knows
> > > > > > it
> > > > > > needs to set the queue_size correctly during launching qemu
> > > > > >
> > > > > > Thoughts?
> > > > > >
> > > > > > Thanks
> > > > >
> > > > > There are other subtle differences such as header size without
> > > > > mergeable buffers for net.
> > > >
> > > > This can be solved by mandating the feature of a mergeable buffer?
> > > >
> > > > Thanks
> > >
> > > PXE and some dpdk versions are only some of the guests that
> > > disable mergeable buffers feature.
> >
> > True, but consider
> >
> > 1) the legacy stuffs requires changes in several software layers
> > 2) it is how virtio 1.0 works e.g device can fail the feature negotiation
> > 3) it is not supported since day 0
> > 4) management API can be extended to advertise the mandated features
>
> So let me confirm what I should do in next revision:
> 1) disable the driver on BE host like that:
>
>#ifdef __LITTE_ENDIAN
>int eni_vdpa_probe()
>{
>...
>}
>#else
>int eni_vdpa_probe()
>{
>return -ENODEV;
>}
>#endif
This might work but I wonder if we can disable it via Kconfig.
>
> 2) report F_VERSION_1 and F_ACCESS_PLATFORM in get_features()
> 3) introduce a new cb get_vq_num_min in vdpa_config_ops
>
> Does I miss something?
And we need this as well.
Fail the feature negotiation if mrg rxbuf is not negotiated. Otherwise
we can meet the 1.0 requirement of header length. Or the hardware can
still preset the mergeable header if the mrg rx buffer is not
negotiated?
Thanks
>
> > It looks affordable.
> >
> > Thanks
> >
> > >
> > > > >
> > > > >
> > > > > > > I think there are
> > > > > > > several things to do when building the interface
> > > > > > > - support transitional devices, that is allow userspace
> > > > > > > to tell device it's in legacy mode
> > > > > > > - support reporting/setting supporting endian-ness
> > > > > > >
> > > > > > > > ---
> > > > > > > > drivers/vdpa/vdpa.c | 6 ++
> > > > > > > > drivers/virtio/virtio_vdpa.c | 7 ++-
> > > > > > > > include/uapi/linux/vdpa.h| 1 +
> > > > > > > > 3 files changed, 13 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > > > > > > > index 1dc121a07a93..533d7f589eee 100644
> > > > > > > > --- a/drivers/vdpa/vdpa.c
> > > > > > > > +++ b/drivers/vdpa/vdpa.c
> > > > > > > > @@ -12,6 +12,7 @@
> > > > > > > > #include
> > > > > > > > #include
> > > > > > > > #include
> > > > > > > > +#include
> > > > > > > > #include
> > > > > > > > #include
> > > > > > > >
> > > > > > > > @@ -494,6 +495,7 @@ vdpa_dev_fill(struct vdpa_device *vdev,
> > > > > > > > struct sk_buff *msg, u32 portid, u32 seq
> > > > > > > > u16 max_vq_size;
> > > > > > > > u32 device_id;
> > > > > > > > u32 vendor_id;
> > > > > > > > + u64 features;
> > > > > > > > void *hdr;
> > > > > > > > int err;
> > > > > > > >
> > > > > > > > @@ -508,6 +510,7 @@ vdpa_dev_fill(struct vdpa_device *vdev,
> > > > > > > > struct sk_buff *msg, u32 portid, u32 seq
> > > > > > > > device_id = vdev->config->get_device_id(vdev);
> > > > > > > > vendor_id = vdev->config->get_vendor_id(vdev);
> > > > > > > > max_vq_size = vdev->config->get_vq_num_max(vdev);
> > > > > > > > + features = vdev->config->get_features(vdev);
> > > > > > > >
> > > > > > > > err = -EMSGSIZE;
> > > > > > > > if (nla_put_string(msg, VDPA_ATTR_DEV_NAME,
> > > > > > > > dev_name(>dev)))
> > > > > > > > @@ -520,6 +523,9 @@ vdpa_dev_fill(struct vdpa_device *vdev,
> > > > > > > > struct sk_buff *msg, u32 portid, u32 seq
> > > > > > > >
Re: [PATCH v2 4/5] vdpa: add new vdpa attribute VDPA_ATTR_DEV_F_VERSION_1
On Wed, Sep 15, 2021 at 8:12 PM Wu Zongyong wrote: > > On Wed, Sep 15, 2021 at 07:08:51AM -0400, Michael S. Tsirkin wrote: > > On Wed, Sep 15, 2021 at 04:06:57PM +0800, Jason Wang wrote: > > > On Wed, Sep 15, 2021 at 3:38 PM Michael S. Tsirkin > > > wrote: > > > > > > > > On Wed, Sep 15, 2021 at 11:18:06AM +0800, Jason Wang wrote: > > > > > On Tue, Sep 14, 2021 at 8:58 PM Michael S. Tsirkin > > > > > wrote: > > > > > > > > > > > > On Tue, Sep 14, 2021 at 08:24:51PM +0800, Wu Zongyong wrote: > > > > > > > This new attribute advertises whether the vdpa device is legacy > > > > > > > or not. > > > > > > > Users can pick right virtqueue size if the vdpa device is legacy > > > > > > > which > > > > > > > doesn't support to change virtqueue size. > > > > > > > > > > > > > > Signed-off-by: Wu Zongyong > > > > > > > > > > > > So if we are bothering with legacy, > > > > > > > > > > I think we'd better not. I guess the following may work: > > > > > > > > > > 1) disable the driver on BE host > > > > > 2) present VERSION_1 with ACCESS_PLATFORM in get_features() > > > > > 3) extend the management to advertise max_queue_size and > > > > > min_queue_size, for ENI they are the same so management layer knows it > > > > > needs to set the queue_size correctly during launching qemu > > > > > > > > > > Thoughts? > > > > > > > > > > Thanks > > > > > > > > There are other subtle differences such as header size without > > > > mergeable buffers for net. > > > > > > This can be solved by mandating the feature of a mergeable buffer? > > > > > > Thanks > > > > PXE and some dpdk versions are only some of the guests that > > disable mergeable buffers feature. > > > So what about this: > > 1) disable the driver on BE host >AFAIK, there are no use cases for ENI to be used in a BE machine. So >just disable the driver on BE machine, it will make things simper. > 2) present ACCESS_PLATFORM but not VERSION_1 in get_features() This sounds like a violation of the virtio spec. ACCESS_PLATFORM depends on VERSION_1. > 3) extend the management to advertise min_queue_size >min_queue_size is the same as with max_queue_size for ENI. > > Another choice for 3): >extend the management to advertise the flag F_VERSION_1 just like >this patch This will bring a lot of trouble, notice that the legacy/transitional device doesn't work in several layers (both vdpa kernel and qemu). If we can afford mandating mergeable rx buffers in the driver, it's the most simple way. I guess you should have the plan for the next generation ENI which should support VERSION_1 and RING_PACKED. Thanks > > > > > > > > > > > > > > I think there are > > > > > > several things to do when building the interface > > > > > > - support transitional devices, that is allow userspace > > > > > > to tell device it's in legacy mode > > > > > > - support reporting/setting supporting endian-ness > > > > > > > > > > > > > --- > > > > > > > drivers/vdpa/vdpa.c | 6 ++ > > > > > > > drivers/virtio/virtio_vdpa.c | 7 ++- > > > > > > > include/uapi/linux/vdpa.h| 1 + > > > > > > > 3 files changed, 13 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > > > > > > > index 1dc121a07a93..533d7f589eee 100644 > > > > > > > --- a/drivers/vdpa/vdpa.c > > > > > > > +++ b/drivers/vdpa/vdpa.c > > > > > > > @@ -12,6 +12,7 @@ > > > > > > > #include > > > > > > > #include > > > > > > > #include > > > > > > > +#include > > > > > > > #include > > > > > > > #include > > > > > > > > > > > > > > @@ -494,6 +495,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, > > > > > > > struct sk_buff *msg, u32 portid, u32 seq > > > > > > > u16 max_vq_size; > > > > > > > u32 device_id; > > > > > > > u32 vendor_id; > > > > > > > + u64 features; > > > > > > > void *hdr; > > > > > > > int err; > > > > > > > > > > > > > > @@ -508,6 +510,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, > > > > > > > struct sk_buff *msg, u32 portid, u32 seq > > > > > > > device_id = vdev->config->get_device_id(vdev); > > > > > > > vendor_id = vdev->config->get_vendor_id(vdev); > > > > > > > max_vq_size = vdev->config->get_vq_num_max(vdev); > > > > > > > + features = vdev->config->get_features(vdev); > > > > > > > > > > > > > > err = -EMSGSIZE; > > > > > > > if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, > > > > > > > dev_name(>dev))) > > > > > > > @@ -520,6 +523,9 @@ vdpa_dev_fill(struct vdpa_device *vdev, > > > > > > > struct sk_buff *msg, u32 portid, u32 seq > > > > > > > goto msg_err; > > > > > > > if (nla_put_u16(msg, VDPA_ATTR_DEV_MAX_VQ_SIZE, > > > > > > > max_vq_size)) > > > > > > > goto msg_err; > > > > > > > + if (features & BIT_ULL(VIRTIO_F_VERSION_1) && > > > > > > > + nla_put_flag(msg, VDPA_ATTR_DEV_VERSION_1)) > > > > > > > + goto msg_err; > > > > > > > > > > > > > > genlmsg_end(msg,
Re: [PATCH v2 4/5] vdpa: add new vdpa attribute VDPA_ATTR_DEV_F_VERSION_1
On Wed, Sep 15, 2021 at 7:09 PM Michael S. Tsirkin wrote:
>
> On Wed, Sep 15, 2021 at 04:06:57PM +0800, Jason Wang wrote:
> > On Wed, Sep 15, 2021 at 3:38 PM Michael S. Tsirkin wrote:
> > >
> > > On Wed, Sep 15, 2021 at 11:18:06AM +0800, Jason Wang wrote:
> > > > On Tue, Sep 14, 2021 at 8:58 PM Michael S. Tsirkin
> > > > wrote:
> > > > >
> > > > > On Tue, Sep 14, 2021 at 08:24:51PM +0800, Wu Zongyong wrote:
> > > > > > This new attribute advertises whether the vdpa device is legacy or
> > > > > > not.
> > > > > > Users can pick right virtqueue size if the vdpa device is legacy
> > > > > > which
> > > > > > doesn't support to change virtqueue size.
> > > > > >
> > > > > > Signed-off-by: Wu Zongyong
> > > > >
> > > > > So if we are bothering with legacy,
> > > >
> > > > I think we'd better not. I guess the following may work:
> > > >
> > > > 1) disable the driver on BE host
> > > > 2) present VERSION_1 with ACCESS_PLATFORM in get_features()
> > > > 3) extend the management to advertise max_queue_size and
> > > > min_queue_size, for ENI they are the same so management layer knows it
> > > > needs to set the queue_size correctly during launching qemu
> > > >
> > > > Thoughts?
> > > >
> > > > Thanks
> > >
> > > There are other subtle differences such as header size without
> > > mergeable buffers for net.
> >
> > This can be solved by mandating the feature of a mergeable buffer?
> >
> > Thanks
>
> PXE and some dpdk versions are only some of the guests that
> disable mergeable buffers feature.
True, but consider
1) the legacy stuffs requires changes in several software layers
2) it is how virtio 1.0 works e.g device can fail the feature negotiation
3) it is not supported since day 0
4) management API can be extended to advertise the mandated features
It looks affordable.
Thanks
>
> > >
> > >
> > > > > I think there are
> > > > > several things to do when building the interface
> > > > > - support transitional devices, that is allow userspace
> > > > > to tell device it's in legacy mode
> > > > > - support reporting/setting supporting endian-ness
> > > > >
> > > > > > ---
> > > > > > drivers/vdpa/vdpa.c | 6 ++
> > > > > > drivers/virtio/virtio_vdpa.c | 7 ++-
> > > > > > include/uapi/linux/vdpa.h| 1 +
> > > > > > 3 files changed, 13 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > > > > > index 1dc121a07a93..533d7f589eee 100644
> > > > > > --- a/drivers/vdpa/vdpa.c
> > > > > > +++ b/drivers/vdpa/vdpa.c
> > > > > > @@ -12,6 +12,7 @@
> > > > > > #include
> > > > > > #include
> > > > > > #include
> > > > > > +#include
> > > > > > #include
> > > > > > #include
> > > > > >
> > > > > > @@ -494,6 +495,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > > > > sk_buff *msg, u32 portid, u32 seq
> > > > > > u16 max_vq_size;
> > > > > > u32 device_id;
> > > > > > u32 vendor_id;
> > > > > > + u64 features;
> > > > > > void *hdr;
> > > > > > int err;
> > > > > >
> > > > > > @@ -508,6 +510,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > > > > sk_buff *msg, u32 portid, u32 seq
> > > > > > device_id = vdev->config->get_device_id(vdev);
> > > > > > vendor_id = vdev->config->get_vendor_id(vdev);
> > > > > > max_vq_size = vdev->config->get_vq_num_max(vdev);
> > > > > > + features = vdev->config->get_features(vdev);
> > > > > >
> > > > > > err = -EMSGSIZE;
> > > > > > if (nla_put_string(msg, VDPA_ATTR_DEV_NAME,
> > > > > > dev_name(>dev)))
> > > > > > @@ -520,6 +523,9 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > > > > sk_buff *msg, u32 portid, u32 seq
> > > > > > goto msg_err;
> > > > > > if (nla_put_u16(msg, VDPA_ATTR_DEV_MAX_VQ_SIZE, max_vq_size))
> > > > > > goto msg_err;
> > > > > > + if (features & BIT_ULL(VIRTIO_F_VERSION_1) &&
> > > > > > + nla_put_flag(msg, VDPA_ATTR_DEV_VERSION_1))
> > > > > > + goto msg_err;
> > > > > >
> > > > > > genlmsg_end(msg, hdr);
> > > > > > return 0;
> > > > > > diff --git a/drivers/virtio/virtio_vdpa.c
> > > > > > b/drivers/virtio/virtio_vdpa.c
> > > > > > index 72eaef2caeb1..1cba957c4cdc 100644
> > > > > > --- a/drivers/virtio/virtio_vdpa.c
> > > > > > +++ b/drivers/virtio/virtio_vdpa.c
> > > > > > @@ -7,6 +7,7 @@
> > > > > > *
> > > > > > */
> > > > > >
> > > > > > +#include "linux/virtio_config.h"
> > > > > > #include
> > > > > > #include
> > > > > > #include
> > > > > > @@ -145,6 +146,7 @@ virtio_vdpa_setup_vq(struct virtio_device
> > > > > > *vdev, unsigned int index,
> > > > > > /* Assume split virtqueue, switch to packed if necessary */
> > > > > > struct vdpa_vq_state state = {0};
> > > > > > unsigned long flags;
> > > > > > + bool may_reduce_num = false;
> > > > > > u32 align, num;
> > > > > > int err;
> > > > > >
> > > > > > @@ -169,10 +171,13 @@
Re: [PATCH v2 4/5] vdpa: add new vdpa attribute VDPA_ATTR_DEV_F_VERSION_1
On Wed, Sep 15, 2021 at 04:05:49PM +0800, Jason Wang wrote:
> On Wed, Sep 15, 2021 at 3:31 PM Michael S. Tsirkin wrote:
> >
> > On Wed, Sep 15, 2021 at 11:24:53AM +0800, Wu Zongyong wrote:
> > > On Tue, Sep 14, 2021 at 08:58:28AM -0400, Michael S. Tsirkin wrote:
> > > > On Tue, Sep 14, 2021 at 08:24:51PM +0800, Wu Zongyong wrote:
> > > > > This new attribute advertises whether the vdpa device is legacy or
> > > > > not.
> > > > > Users can pick right virtqueue size if the vdpa device is legacy which
> > > > > doesn't support to change virtqueue size.
> > > > >
> > > > > Signed-off-by: Wu Zongyong
> > > >
> > > > So if we are bothering with legacy, I think there are
> > > > several things to do when building the interface
> > > > - support transitional devices, that is allow userspace
> > > > to tell device it's in legacy mode
> > > > - support reporting/setting supporting endian-ness
> > >
> > > It's true if we try to implement a general drvier for legacy.
> > > But this series is dedicated to implement a driver only for ENI. Is it
> > > necessary to implement what you said here in this series?
> >
> > To a certain degree, yes.
> >
> > I am thinking about the UAPI here. The new attribute is part of that.
> > E.g. userspace consuming this needs to be more or less hardware agnostic
> > and not depend on specifics of ENI.
> >
> > Otherwise if userspace assumes legacy==eni then it will break with
> > other hardware.
> >
> > One way to test how generic it all is would be adding legacy support in
> > the simulator.
>
> I don't get why we need to support legacy devices e.g it doesn't have
> ACCESS_PLATFORM support. I think we should re-consider to mandate 1.0
> devices.
>
> https://lore.kernel.org/lkml/20210408082648.20145-1-jasow...@redhat.com/
>
> And it will complicate all of the different layers.
>
> Thanks
It's not that we have to, it's just that imho, if we do
it's easier to just do it all in the kernel rather than spreading
parts of code around.
> >
> > > >
> > > > > ---
> > > > > drivers/vdpa/vdpa.c | 6 ++
> > > > > drivers/virtio/virtio_vdpa.c | 7 ++-
> > > > > include/uapi/linux/vdpa.h| 1 +
> > > > > 3 files changed, 13 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > > > > index 1dc121a07a93..533d7f589eee 100644
> > > > > --- a/drivers/vdpa/vdpa.c
> > > > > +++ b/drivers/vdpa/vdpa.c
> > > > > @@ -12,6 +12,7 @@
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > +#include
> > > > > #include
> > > > > #include
> > > > >
> > > > > @@ -494,6 +495,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > > > sk_buff *msg, u32 portid, u32 seq
> > > > > u16 max_vq_size;
> > > > > u32 device_id;
> > > > > u32 vendor_id;
> > > > > + u64 features;
> > > > > void *hdr;
> > > > > int err;
> > > > >
> > > > > @@ -508,6 +510,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > > > sk_buff *msg, u32 portid, u32 seq
> > > > > device_id = vdev->config->get_device_id(vdev);
> > > > > vendor_id = vdev->config->get_vendor_id(vdev);
> > > > > max_vq_size = vdev->config->get_vq_num_max(vdev);
> > > > > + features = vdev->config->get_features(vdev);
> > > > >
> > > > > err = -EMSGSIZE;
> > > > > if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(>dev)))
> > > > > @@ -520,6 +523,9 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > > > sk_buff *msg, u32 portid, u32 seq
> > > > > goto msg_err;
> > > > > if (nla_put_u16(msg, VDPA_ATTR_DEV_MAX_VQ_SIZE, max_vq_size))
> > > > > goto msg_err;
> > > > > + if (features & BIT_ULL(VIRTIO_F_VERSION_1) &&
> > > > > + nla_put_flag(msg, VDPA_ATTR_DEV_VERSION_1))
> > > > > + goto msg_err;
> > > > >
> > > > > genlmsg_end(msg, hdr);
> > > > > return 0;
> > > > > diff --git a/drivers/virtio/virtio_vdpa.c
> > > > > b/drivers/virtio/virtio_vdpa.c
> > > > > index 72eaef2caeb1..1cba957c4cdc 100644
> > > > > --- a/drivers/virtio/virtio_vdpa.c
> > > > > +++ b/drivers/virtio/virtio_vdpa.c
> > > > > @@ -7,6 +7,7 @@
> > > > > *
> > > > > */
> > > > >
> > > > > +#include "linux/virtio_config.h"
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > @@ -145,6 +146,7 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev,
> > > > > unsigned int index,
> > > > > /* Assume split virtqueue, switch to packed if necessary */
> > > > > struct vdpa_vq_state state = {0};
> > > > > unsigned long flags;
> > > > > + bool may_reduce_num = false;
> > > > > u32 align, num;
> > > > > int err;
> > > > >
> > > > > @@ -169,10 +171,13 @@ virtio_vdpa_setup_vq(struct virtio_device
> > > > > *vdev, unsigned int index,
> > > > > goto error_new_virtqueue;
> > > > > }
> > > > >
> > > > > + if (ops->get_features(vdpa) & BIT_ULL(VIRTIO_F_VERSION_1))
> > > > > + may_reduce_num = true;
> > > > > +
> > > > > /* Create the vring */
> > > > > align = ops->get_vq_align(vdpa);
> > > > > vq =
Re: [PATCH v2 4/5] vdpa: add new vdpa attribute VDPA_ATTR_DEV_F_VERSION_1
On Wed, Sep 15, 2021 at 04:06:57PM +0800, Jason Wang wrote:
> On Wed, Sep 15, 2021 at 3:38 PM Michael S. Tsirkin wrote:
> >
> > On Wed, Sep 15, 2021 at 11:18:06AM +0800, Jason Wang wrote:
> > > On Tue, Sep 14, 2021 at 8:58 PM Michael S. Tsirkin
> > > wrote:
> > > >
> > > > On Tue, Sep 14, 2021 at 08:24:51PM +0800, Wu Zongyong wrote:
> > > > > This new attribute advertises whether the vdpa device is legacy or
> > > > > not.
> > > > > Users can pick right virtqueue size if the vdpa device is legacy which
> > > > > doesn't support to change virtqueue size.
> > > > >
> > > > > Signed-off-by: Wu Zongyong
> > > >
> > > > So if we are bothering with legacy,
> > >
> > > I think we'd better not. I guess the following may work:
> > >
> > > 1) disable the driver on BE host
> > > 2) present VERSION_1 with ACCESS_PLATFORM in get_features()
> > > 3) extend the management to advertise max_queue_size and
> > > min_queue_size, for ENI they are the same so management layer knows it
> > > needs to set the queue_size correctly during launching qemu
> > >
> > > Thoughts?
> > >
> > > Thanks
> >
> > There are other subtle differences such as header size without
> > mergeable buffers for net.
>
> This can be solved by mandating the feature of a mergeable buffer?
>
> Thanks
PXE and some dpdk versions are only some of the guests that
disable mergeable buffers feature.
> >
> >
> > > > I think there are
> > > > several things to do when building the interface
> > > > - support transitional devices, that is allow userspace
> > > > to tell device it's in legacy mode
> > > > - support reporting/setting supporting endian-ness
> > > >
> > > > > ---
> > > > > drivers/vdpa/vdpa.c | 6 ++
> > > > > drivers/virtio/virtio_vdpa.c | 7 ++-
> > > > > include/uapi/linux/vdpa.h| 1 +
> > > > > 3 files changed, 13 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > > > > index 1dc121a07a93..533d7f589eee 100644
> > > > > --- a/drivers/vdpa/vdpa.c
> > > > > +++ b/drivers/vdpa/vdpa.c
> > > > > @@ -12,6 +12,7 @@
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > +#include
> > > > > #include
> > > > > #include
> > > > >
> > > > > @@ -494,6 +495,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > > > sk_buff *msg, u32 portid, u32 seq
> > > > > u16 max_vq_size;
> > > > > u32 device_id;
> > > > > u32 vendor_id;
> > > > > + u64 features;
> > > > > void *hdr;
> > > > > int err;
> > > > >
> > > > > @@ -508,6 +510,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > > > sk_buff *msg, u32 portid, u32 seq
> > > > > device_id = vdev->config->get_device_id(vdev);
> > > > > vendor_id = vdev->config->get_vendor_id(vdev);
> > > > > max_vq_size = vdev->config->get_vq_num_max(vdev);
> > > > > + features = vdev->config->get_features(vdev);
> > > > >
> > > > > err = -EMSGSIZE;
> > > > > if (nla_put_string(msg, VDPA_ATTR_DEV_NAME,
> > > > > dev_name(>dev)))
> > > > > @@ -520,6 +523,9 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > > > sk_buff *msg, u32 portid, u32 seq
> > > > > goto msg_err;
> > > > > if (nla_put_u16(msg, VDPA_ATTR_DEV_MAX_VQ_SIZE, max_vq_size))
> > > > > goto msg_err;
> > > > > + if (features & BIT_ULL(VIRTIO_F_VERSION_1) &&
> > > > > + nla_put_flag(msg, VDPA_ATTR_DEV_VERSION_1))
> > > > > + goto msg_err;
> > > > >
> > > > > genlmsg_end(msg, hdr);
> > > > > return 0;
> > > > > diff --git a/drivers/virtio/virtio_vdpa.c
> > > > > b/drivers/virtio/virtio_vdpa.c
> > > > > index 72eaef2caeb1..1cba957c4cdc 100644
> > > > > --- a/drivers/virtio/virtio_vdpa.c
> > > > > +++ b/drivers/virtio/virtio_vdpa.c
> > > > > @@ -7,6 +7,7 @@
> > > > > *
> > > > > */
> > > > >
> > > > > +#include "linux/virtio_config.h"
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > @@ -145,6 +146,7 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev,
> > > > > unsigned int index,
> > > > > /* Assume split virtqueue, switch to packed if necessary */
> > > > > struct vdpa_vq_state state = {0};
> > > > > unsigned long flags;
> > > > > + bool may_reduce_num = false;
> > > > > u32 align, num;
> > > > > int err;
> > > > >
> > > > > @@ -169,10 +171,13 @@ virtio_vdpa_setup_vq(struct virtio_device
> > > > > *vdev, unsigned int index,
> > > > > goto error_new_virtqueue;
> > > > > }
> > > > >
> > > > > + if (ops->get_features(vdpa) & BIT_ULL(VIRTIO_F_VERSION_1))
> > > > > + may_reduce_num = true;
> > > > > +
> > > > > /* Create the vring */
> > > > > align = ops->get_vq_align(vdpa);
> > > > > vq = vring_create_virtqueue(index, num, align, vdev,
> > > > > - true, true, ctx,
> > > > > + true, may_reduce_num, ctx,
> > > > >
Re: [PATCH v2 4/5] vdpa: add new vdpa attribute VDPA_ATTR_DEV_F_VERSION_1
On Wed, Sep 15, 2021 at 3:38 PM Michael S. Tsirkin wrote:
>
> On Wed, Sep 15, 2021 at 11:18:06AM +0800, Jason Wang wrote:
> > On Tue, Sep 14, 2021 at 8:58 PM Michael S. Tsirkin wrote:
> > >
> > > On Tue, Sep 14, 2021 at 08:24:51PM +0800, Wu Zongyong wrote:
> > > > This new attribute advertises whether the vdpa device is legacy or not.
> > > > Users can pick right virtqueue size if the vdpa device is legacy which
> > > > doesn't support to change virtqueue size.
> > > >
> > > > Signed-off-by: Wu Zongyong
> > >
> > > So if we are bothering with legacy,
> >
> > I think we'd better not. I guess the following may work:
> >
> > 1) disable the driver on BE host
> > 2) present VERSION_1 with ACCESS_PLATFORM in get_features()
> > 3) extend the management to advertise max_queue_size and
> > min_queue_size, for ENI they are the same so management layer knows it
> > needs to set the queue_size correctly during launching qemu
> >
> > Thoughts?
> >
> > Thanks
>
> There are other subtle differences such as header size without
> mergeable buffers for net.
This can be solved by mandating the feature of a mergeable buffer?
Thanks
>
>
> > > I think there are
> > > several things to do when building the interface
> > > - support transitional devices, that is allow userspace
> > > to tell device it's in legacy mode
> > > - support reporting/setting supporting endian-ness
> > >
> > > > ---
> > > > drivers/vdpa/vdpa.c | 6 ++
> > > > drivers/virtio/virtio_vdpa.c | 7 ++-
> > > > include/uapi/linux/vdpa.h| 1 +
> > > > 3 files changed, 13 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > > > index 1dc121a07a93..533d7f589eee 100644
> > > > --- a/drivers/vdpa/vdpa.c
> > > > +++ b/drivers/vdpa/vdpa.c
> > > > @@ -12,6 +12,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > >
> > > > @@ -494,6 +495,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > > sk_buff *msg, u32 portid, u32 seq
> > > > u16 max_vq_size;
> > > > u32 device_id;
> > > > u32 vendor_id;
> > > > + u64 features;
> > > > void *hdr;
> > > > int err;
> > > >
> > > > @@ -508,6 +510,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > > sk_buff *msg, u32 portid, u32 seq
> > > > device_id = vdev->config->get_device_id(vdev);
> > > > vendor_id = vdev->config->get_vendor_id(vdev);
> > > > max_vq_size = vdev->config->get_vq_num_max(vdev);
> > > > + features = vdev->config->get_features(vdev);
> > > >
> > > > err = -EMSGSIZE;
> > > > if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(>dev)))
> > > > @@ -520,6 +523,9 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > > sk_buff *msg, u32 portid, u32 seq
> > > > goto msg_err;
> > > > if (nla_put_u16(msg, VDPA_ATTR_DEV_MAX_VQ_SIZE, max_vq_size))
> > > > goto msg_err;
> > > > + if (features & BIT_ULL(VIRTIO_F_VERSION_1) &&
> > > > + nla_put_flag(msg, VDPA_ATTR_DEV_VERSION_1))
> > > > + goto msg_err;
> > > >
> > > > genlmsg_end(msg, hdr);
> > > > return 0;
> > > > diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> > > > index 72eaef2caeb1..1cba957c4cdc 100644
> > > > --- a/drivers/virtio/virtio_vdpa.c
> > > > +++ b/drivers/virtio/virtio_vdpa.c
> > > > @@ -7,6 +7,7 @@
> > > > *
> > > > */
> > > >
> > > > +#include "linux/virtio_config.h"
> > > > #include
> > > > #include
> > > > #include
> > > > @@ -145,6 +146,7 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev,
> > > > unsigned int index,
> > > > /* Assume split virtqueue, switch to packed if necessary */
> > > > struct vdpa_vq_state state = {0};
> > > > unsigned long flags;
> > > > + bool may_reduce_num = false;
> > > > u32 align, num;
> > > > int err;
> > > >
> > > > @@ -169,10 +171,13 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev,
> > > > unsigned int index,
> > > > goto error_new_virtqueue;
> > > > }
> > > >
> > > > + if (ops->get_features(vdpa) & BIT_ULL(VIRTIO_F_VERSION_1))
> > > > + may_reduce_num = true;
> > > > +
> > > > /* Create the vring */
> > > > align = ops->get_vq_align(vdpa);
> > > > vq = vring_create_virtqueue(index, num, align, vdev,
> > > > - true, true, ctx,
> > > > + true, may_reduce_num, ctx,
> > > > virtio_vdpa_notify, callback, name);
> > > > if (!vq) {
> > > > err = -ENOMEM;
> > > > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> > > > index 66a41e4ec163..ce0b74276a5b 100644
> > > > --- a/include/uapi/linux/vdpa.h
> > > > +++ b/include/uapi/linux/vdpa.h
> > > > @@ -32,6 +32,7 @@ enum vdpa_attr {
> > > > VDPA_ATTR_DEV_VENDOR_ID,/* u32 */
> > > >
Re: [PATCH v2 4/5] vdpa: add new vdpa attribute VDPA_ATTR_DEV_F_VERSION_1
On Wed, Sep 15, 2021 at 3:31 PM Michael S. Tsirkin wrote:
>
> On Wed, Sep 15, 2021 at 11:24:53AM +0800, Wu Zongyong wrote:
> > On Tue, Sep 14, 2021 at 08:58:28AM -0400, Michael S. Tsirkin wrote:
> > > On Tue, Sep 14, 2021 at 08:24:51PM +0800, Wu Zongyong wrote:
> > > > This new attribute advertises whether the vdpa device is legacy or not.
> > > > Users can pick right virtqueue size if the vdpa device is legacy which
> > > > doesn't support to change virtqueue size.
> > > >
> > > > Signed-off-by: Wu Zongyong
> > >
> > > So if we are bothering with legacy, I think there are
> > > several things to do when building the interface
> > > - support transitional devices, that is allow userspace
> > > to tell device it's in legacy mode
> > > - support reporting/setting supporting endian-ness
> >
> > It's true if we try to implement a general drvier for legacy.
> > But this series is dedicated to implement a driver only for ENI. Is it
> > necessary to implement what you said here in this series?
>
> To a certain degree, yes.
>
> I am thinking about the UAPI here. The new attribute is part of that.
> E.g. userspace consuming this needs to be more or less hardware agnostic
> and not depend on specifics of ENI.
>
> Otherwise if userspace assumes legacy==eni then it will break with
> other hardware.
>
> One way to test how generic it all is would be adding legacy support in
> the simulator.
I don't get why we need to support legacy devices e.g it doesn't have
ACCESS_PLATFORM support. I think we should re-consider to mandate 1.0
devices.
https://lore.kernel.org/lkml/20210408082648.20145-1-jasow...@redhat.com/
And it will complicate all of the different layers.
Thanks
>
> > >
> > > > ---
> > > > drivers/vdpa/vdpa.c | 6 ++
> > > > drivers/virtio/virtio_vdpa.c | 7 ++-
> > > > include/uapi/linux/vdpa.h| 1 +
> > > > 3 files changed, 13 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > > > index 1dc121a07a93..533d7f589eee 100644
> > > > --- a/drivers/vdpa/vdpa.c
> > > > +++ b/drivers/vdpa/vdpa.c
> > > > @@ -12,6 +12,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > >
> > > > @@ -494,6 +495,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > > sk_buff *msg, u32 portid, u32 seq
> > > > u16 max_vq_size;
> > > > u32 device_id;
> > > > u32 vendor_id;
> > > > + u64 features;
> > > > void *hdr;
> > > > int err;
> > > >
> > > > @@ -508,6 +510,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > > sk_buff *msg, u32 portid, u32 seq
> > > > device_id = vdev->config->get_device_id(vdev);
> > > > vendor_id = vdev->config->get_vendor_id(vdev);
> > > > max_vq_size = vdev->config->get_vq_num_max(vdev);
> > > > + features = vdev->config->get_features(vdev);
> > > >
> > > > err = -EMSGSIZE;
> > > > if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(>dev)))
> > > > @@ -520,6 +523,9 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > > sk_buff *msg, u32 portid, u32 seq
> > > > goto msg_err;
> > > > if (nla_put_u16(msg, VDPA_ATTR_DEV_MAX_VQ_SIZE, max_vq_size))
> > > > goto msg_err;
> > > > + if (features & BIT_ULL(VIRTIO_F_VERSION_1) &&
> > > > + nla_put_flag(msg, VDPA_ATTR_DEV_VERSION_1))
> > > > + goto msg_err;
> > > >
> > > > genlmsg_end(msg, hdr);
> > > > return 0;
> > > > diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> > > > index 72eaef2caeb1..1cba957c4cdc 100644
> > > > --- a/drivers/virtio/virtio_vdpa.c
> > > > +++ b/drivers/virtio/virtio_vdpa.c
> > > > @@ -7,6 +7,7 @@
> > > > *
> > > > */
> > > >
> > > > +#include "linux/virtio_config.h"
> > > > #include
> > > > #include
> > > > #include
> > > > @@ -145,6 +146,7 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev,
> > > > unsigned int index,
> > > > /* Assume split virtqueue, switch to packed if necessary */
> > > > struct vdpa_vq_state state = {0};
> > > > unsigned long flags;
> > > > + bool may_reduce_num = false;
> > > > u32 align, num;
> > > > int err;
> > > >
> > > > @@ -169,10 +171,13 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev,
> > > > unsigned int index,
> > > > goto error_new_virtqueue;
> > > > }
> > > >
> > > > + if (ops->get_features(vdpa) & BIT_ULL(VIRTIO_F_VERSION_1))
> > > > + may_reduce_num = true;
> > > > +
> > > > /* Create the vring */
> > > > align = ops->get_vq_align(vdpa);
> > > > vq = vring_create_virtqueue(index, num, align, vdev,
> > > > - true, true, ctx,
> > > > + true, may_reduce_num, ctx,
> > > > virtio_vdpa_notify, callback, name);
> > > > if (!vq) {
> > > > err = -ENOMEM;
> > > > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> > > > index 66a41e4ec163..ce0b74276a5b 100644
> > > > --- a/include/uapi/linux/vdpa.h
> > > > +++
Re: [PATCH v2 4/5] vdpa: add new vdpa attribute VDPA_ATTR_DEV_F_VERSION_1
On Wed, Sep 15, 2021 at 11:18:06AM +0800, Jason Wang wrote:
> On Tue, Sep 14, 2021 at 8:58 PM Michael S. Tsirkin wrote:
> >
> > On Tue, Sep 14, 2021 at 08:24:51PM +0800, Wu Zongyong wrote:
> > > This new attribute advertises whether the vdpa device is legacy or not.
> > > Users can pick right virtqueue size if the vdpa device is legacy which
> > > doesn't support to change virtqueue size.
> > >
> > > Signed-off-by: Wu Zongyong
> >
> > So if we are bothering with legacy,
>
> I think we'd better not. I guess the following may work:
>
> 1) disable the driver on BE host
> 2) present VERSION_1 with ACCESS_PLATFORM in get_features()
> 3) extend the management to advertise max_queue_size and
> min_queue_size, for ENI they are the same so management layer knows it
> needs to set the queue_size correctly during launching qemu
>
> Thoughts?
>
> Thanks
There are other subtle differences such as header size without
mergeable buffers for net.
> > I think there are
> > several things to do when building the interface
> > - support transitional devices, that is allow userspace
> > to tell device it's in legacy mode
> > - support reporting/setting supporting endian-ness
> >
> > > ---
> > > drivers/vdpa/vdpa.c | 6 ++
> > > drivers/virtio/virtio_vdpa.c | 7 ++-
> > > include/uapi/linux/vdpa.h| 1 +
> > > 3 files changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > > index 1dc121a07a93..533d7f589eee 100644
> > > --- a/drivers/vdpa/vdpa.c
> > > +++ b/drivers/vdpa/vdpa.c
> > > @@ -12,6 +12,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > >
> > > @@ -494,6 +495,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > sk_buff *msg, u32 portid, u32 seq
> > > u16 max_vq_size;
> > > u32 device_id;
> > > u32 vendor_id;
> > > + u64 features;
> > > void *hdr;
> > > int err;
> > >
> > > @@ -508,6 +510,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > sk_buff *msg, u32 portid, u32 seq
> > > device_id = vdev->config->get_device_id(vdev);
> > > vendor_id = vdev->config->get_vendor_id(vdev);
> > > max_vq_size = vdev->config->get_vq_num_max(vdev);
> > > + features = vdev->config->get_features(vdev);
> > >
> > > err = -EMSGSIZE;
> > > if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(>dev)))
> > > @@ -520,6 +523,9 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > sk_buff *msg, u32 portid, u32 seq
> > > goto msg_err;
> > > if (nla_put_u16(msg, VDPA_ATTR_DEV_MAX_VQ_SIZE, max_vq_size))
> > > goto msg_err;
> > > + if (features & BIT_ULL(VIRTIO_F_VERSION_1) &&
> > > + nla_put_flag(msg, VDPA_ATTR_DEV_VERSION_1))
> > > + goto msg_err;
> > >
> > > genlmsg_end(msg, hdr);
> > > return 0;
> > > diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> > > index 72eaef2caeb1..1cba957c4cdc 100644
> > > --- a/drivers/virtio/virtio_vdpa.c
> > > +++ b/drivers/virtio/virtio_vdpa.c
> > > @@ -7,6 +7,7 @@
> > > *
> > > */
> > >
> > > +#include "linux/virtio_config.h"
> > > #include
> > > #include
> > > #include
> > > @@ -145,6 +146,7 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev,
> > > unsigned int index,
> > > /* Assume split virtqueue, switch to packed if necessary */
> > > struct vdpa_vq_state state = {0};
> > > unsigned long flags;
> > > + bool may_reduce_num = false;
> > > u32 align, num;
> > > int err;
> > >
> > > @@ -169,10 +171,13 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev,
> > > unsigned int index,
> > > goto error_new_virtqueue;
> > > }
> > >
> > > + if (ops->get_features(vdpa) & BIT_ULL(VIRTIO_F_VERSION_1))
> > > + may_reduce_num = true;
> > > +
> > > /* Create the vring */
> > > align = ops->get_vq_align(vdpa);
> > > vq = vring_create_virtqueue(index, num, align, vdev,
> > > - true, true, ctx,
> > > + true, may_reduce_num, ctx,
> > > virtio_vdpa_notify, callback, name);
> > > if (!vq) {
> > > err = -ENOMEM;
> > > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> > > index 66a41e4ec163..ce0b74276a5b 100644
> > > --- a/include/uapi/linux/vdpa.h
> > > +++ b/include/uapi/linux/vdpa.h
> > > @@ -32,6 +32,7 @@ enum vdpa_attr {
> > > VDPA_ATTR_DEV_VENDOR_ID,/* u32 */
> > > VDPA_ATTR_DEV_MAX_VQS, /* u32 */
> > > VDPA_ATTR_DEV_MAX_VQ_SIZE, /* u16 */
> > > + VDPA_ATTR_DEV_VERSION_1,/* flag */
> > >
> > > /* new attributes must be added above here */
> > > VDPA_ATTR_MAX,
> > > --
> > > 2.31.1
> >
___
Virtualization mailing list
Re: [PATCH v2 4/5] vdpa: add new vdpa attribute VDPA_ATTR_DEV_F_VERSION_1
On Wed, Sep 15, 2021 at 11:24:53AM +0800, Wu Zongyong wrote:
> On Tue, Sep 14, 2021 at 08:58:28AM -0400, Michael S. Tsirkin wrote:
> > On Tue, Sep 14, 2021 at 08:24:51PM +0800, Wu Zongyong wrote:
> > > This new attribute advertises whether the vdpa device is legacy or not.
> > > Users can pick right virtqueue size if the vdpa device is legacy which
> > > doesn't support to change virtqueue size.
> > >
> > > Signed-off-by: Wu Zongyong
> >
> > So if we are bothering with legacy, I think there are
> > several things to do when building the interface
> > - support transitional devices, that is allow userspace
> > to tell device it's in legacy mode
> > - support reporting/setting supporting endian-ness
>
> It's true if we try to implement a general drvier for legacy.
> But this series is dedicated to implement a driver only for ENI. Is it
> necessary to implement what you said here in this series?
To a certain degree, yes.
I am thinking about the UAPI here. The new attribute is part of that.
E.g. userspace consuming this needs to be more or less hardware agnostic
and not depend on specifics of ENI.
Otherwise if userspace assumes legacy==eni then it will break with
other hardware.
One way to test how generic it all is would be adding legacy support in
the simulator.
> >
> > > ---
> > > drivers/vdpa/vdpa.c | 6 ++
> > > drivers/virtio/virtio_vdpa.c | 7 ++-
> > > include/uapi/linux/vdpa.h| 1 +
> > > 3 files changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > > index 1dc121a07a93..533d7f589eee 100644
> > > --- a/drivers/vdpa/vdpa.c
> > > +++ b/drivers/vdpa/vdpa.c
> > > @@ -12,6 +12,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > >
> > > @@ -494,6 +495,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > sk_buff *msg, u32 portid, u32 seq
> > > u16 max_vq_size;
> > > u32 device_id;
> > > u32 vendor_id;
> > > + u64 features;
> > > void *hdr;
> > > int err;
> > >
> > > @@ -508,6 +510,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > sk_buff *msg, u32 portid, u32 seq
> > > device_id = vdev->config->get_device_id(vdev);
> > > vendor_id = vdev->config->get_vendor_id(vdev);
> > > max_vq_size = vdev->config->get_vq_num_max(vdev);
> > > + features = vdev->config->get_features(vdev);
> > >
> > > err = -EMSGSIZE;
> > > if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(>dev)))
> > > @@ -520,6 +523,9 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct
> > > sk_buff *msg, u32 portid, u32 seq
> > > goto msg_err;
> > > if (nla_put_u16(msg, VDPA_ATTR_DEV_MAX_VQ_SIZE, max_vq_size))
> > > goto msg_err;
> > > + if (features & BIT_ULL(VIRTIO_F_VERSION_1) &&
> > > + nla_put_flag(msg, VDPA_ATTR_DEV_VERSION_1))
> > > + goto msg_err;
> > >
> > > genlmsg_end(msg, hdr);
> > > return 0;
> > > diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> > > index 72eaef2caeb1..1cba957c4cdc 100644
> > > --- a/drivers/virtio/virtio_vdpa.c
> > > +++ b/drivers/virtio/virtio_vdpa.c
> > > @@ -7,6 +7,7 @@
> > > *
> > > */
> > >
> > > +#include "linux/virtio_config.h"
> > > #include
> > > #include
> > > #include
> > > @@ -145,6 +146,7 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev,
> > > unsigned int index,
> > > /* Assume split virtqueue, switch to packed if necessary */
> > > struct vdpa_vq_state state = {0};
> > > unsigned long flags;
> > > + bool may_reduce_num = false;
> > > u32 align, num;
> > > int err;
> > >
> > > @@ -169,10 +171,13 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev,
> > > unsigned int index,
> > > goto error_new_virtqueue;
> > > }
> > >
> > > + if (ops->get_features(vdpa) & BIT_ULL(VIRTIO_F_VERSION_1))
> > > + may_reduce_num = true;
> > > +
> > > /* Create the vring */
> > > align = ops->get_vq_align(vdpa);
> > > vq = vring_create_virtqueue(index, num, align, vdev,
> > > - true, true, ctx,
> > > + true, may_reduce_num, ctx,
> > > virtio_vdpa_notify, callback, name);
> > > if (!vq) {
> > > err = -ENOMEM;
> > > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> > > index 66a41e4ec163..ce0b74276a5b 100644
> > > --- a/include/uapi/linux/vdpa.h
> > > +++ b/include/uapi/linux/vdpa.h
> > > @@ -32,6 +32,7 @@ enum vdpa_attr {
> > > VDPA_ATTR_DEV_VENDOR_ID,/* u32 */
> > > VDPA_ATTR_DEV_MAX_VQS, /* u32 */
> > > VDPA_ATTR_DEV_MAX_VQ_SIZE, /* u16 */
> > > + VDPA_ATTR_DEV_VERSION_1,/* flag */
> > >
> > > /* new attributes must be added above here */
> > > VDPA_ATTR_MAX,
> > > --
> > > 2.31.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
Re: [PATCH v2 4/5] vdpa: add new vdpa attribute VDPA_ATTR_DEV_F_VERSION_1
On Tue, Sep 14, 2021 at 8:58 PM Michael S. Tsirkin wrote:
>
> On Tue, Sep 14, 2021 at 08:24:51PM +0800, Wu Zongyong wrote:
> > This new attribute advertises whether the vdpa device is legacy or not.
> > Users can pick right virtqueue size if the vdpa device is legacy which
> > doesn't support to change virtqueue size.
> >
> > Signed-off-by: Wu Zongyong
>
> So if we are bothering with legacy,
I think we'd better not. I guess the following may work:
1) disable the driver on BE host
2) present VERSION_1 with ACCESS_PLATFORM in get_features()
3) extend the management to advertise max_queue_size and
min_queue_size, for ENI they are the same so management layer knows it
needs to set the queue_size correctly during launching qemu
Thoughts?
Thanks
> I think there are
> several things to do when building the interface
> - support transitional devices, that is allow userspace
> to tell device it's in legacy mode
> - support reporting/setting supporting endian-ness
>
> > ---
> > drivers/vdpa/vdpa.c | 6 ++
> > drivers/virtio/virtio_vdpa.c | 7 ++-
> > include/uapi/linux/vdpa.h| 1 +
> > 3 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > index 1dc121a07a93..533d7f589eee 100644
> > --- a/drivers/vdpa/vdpa.c
> > +++ b/drivers/vdpa/vdpa.c
> > @@ -12,6 +12,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> >
> > @@ -494,6 +495,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff
> > *msg, u32 portid, u32 seq
> > u16 max_vq_size;
> > u32 device_id;
> > u32 vendor_id;
> > + u64 features;
> > void *hdr;
> > int err;
> >
> > @@ -508,6 +510,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff
> > *msg, u32 portid, u32 seq
> > device_id = vdev->config->get_device_id(vdev);
> > vendor_id = vdev->config->get_vendor_id(vdev);
> > max_vq_size = vdev->config->get_vq_num_max(vdev);
> > + features = vdev->config->get_features(vdev);
> >
> > err = -EMSGSIZE;
> > if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(>dev)))
> > @@ -520,6 +523,9 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff
> > *msg, u32 portid, u32 seq
> > goto msg_err;
> > if (nla_put_u16(msg, VDPA_ATTR_DEV_MAX_VQ_SIZE, max_vq_size))
> > goto msg_err;
> > + if (features & BIT_ULL(VIRTIO_F_VERSION_1) &&
> > + nla_put_flag(msg, VDPA_ATTR_DEV_VERSION_1))
> > + goto msg_err;
> >
> > genlmsg_end(msg, hdr);
> > return 0;
> > diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> > index 72eaef2caeb1..1cba957c4cdc 100644
> > --- a/drivers/virtio/virtio_vdpa.c
> > +++ b/drivers/virtio/virtio_vdpa.c
> > @@ -7,6 +7,7 @@
> > *
> > */
> >
> > +#include "linux/virtio_config.h"
> > #include
> > #include
> > #include
> > @@ -145,6 +146,7 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev,
> > unsigned int index,
> > /* Assume split virtqueue, switch to packed if necessary */
> > struct vdpa_vq_state state = {0};
> > unsigned long flags;
> > + bool may_reduce_num = false;
> > u32 align, num;
> > int err;
> >
> > @@ -169,10 +171,13 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev,
> > unsigned int index,
> > goto error_new_virtqueue;
> > }
> >
> > + if (ops->get_features(vdpa) & BIT_ULL(VIRTIO_F_VERSION_1))
> > + may_reduce_num = true;
> > +
> > /* Create the vring */
> > align = ops->get_vq_align(vdpa);
> > vq = vring_create_virtqueue(index, num, align, vdev,
> > - true, true, ctx,
> > + true, may_reduce_num, ctx,
> > virtio_vdpa_notify, callback, name);
> > if (!vq) {
> > err = -ENOMEM;
> > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> > index 66a41e4ec163..ce0b74276a5b 100644
> > --- a/include/uapi/linux/vdpa.h
> > +++ b/include/uapi/linux/vdpa.h
> > @@ -32,6 +32,7 @@ enum vdpa_attr {
> > VDPA_ATTR_DEV_VENDOR_ID,/* u32 */
> > VDPA_ATTR_DEV_MAX_VQS, /* u32 */
> > VDPA_ATTR_DEV_MAX_VQ_SIZE, /* u16 */
> > + VDPA_ATTR_DEV_VERSION_1,/* flag */
> >
> > /* new attributes must be added above here */
> > VDPA_ATTR_MAX,
> > --
> > 2.31.1
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 4/5] vdpa: add new vdpa attribute VDPA_ATTR_DEV_F_VERSION_1
On Tue, Sep 14, 2021 at 08:24:51PM +0800, Wu Zongyong wrote:
> This new attribute advertises whether the vdpa device is legacy or not.
> Users can pick right virtqueue size if the vdpa device is legacy which
> doesn't support to change virtqueue size.
>
> Signed-off-by: Wu Zongyong
So if we are bothering with legacy, I think there are
several things to do when building the interface
- support transitional devices, that is allow userspace
to tell device it's in legacy mode
- support reporting/setting supporting endian-ness
> ---
> drivers/vdpa/vdpa.c | 6 ++
> drivers/virtio/virtio_vdpa.c | 7 ++-
> include/uapi/linux/vdpa.h| 1 +
> 3 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 1dc121a07a93..533d7f589eee 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -12,6 +12,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -494,6 +495,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff
> *msg, u32 portid, u32 seq
> u16 max_vq_size;
> u32 device_id;
> u32 vendor_id;
> + u64 features;
> void *hdr;
> int err;
>
> @@ -508,6 +510,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff
> *msg, u32 portid, u32 seq
> device_id = vdev->config->get_device_id(vdev);
> vendor_id = vdev->config->get_vendor_id(vdev);
> max_vq_size = vdev->config->get_vq_num_max(vdev);
> + features = vdev->config->get_features(vdev);
>
> err = -EMSGSIZE;
> if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(>dev)))
> @@ -520,6 +523,9 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff
> *msg, u32 portid, u32 seq
> goto msg_err;
> if (nla_put_u16(msg, VDPA_ATTR_DEV_MAX_VQ_SIZE, max_vq_size))
> goto msg_err;
> + if (features & BIT_ULL(VIRTIO_F_VERSION_1) &&
> + nla_put_flag(msg, VDPA_ATTR_DEV_VERSION_1))
> + goto msg_err;
>
> genlmsg_end(msg, hdr);
> return 0;
> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> index 72eaef2caeb1..1cba957c4cdc 100644
> --- a/drivers/virtio/virtio_vdpa.c
> +++ b/drivers/virtio/virtio_vdpa.c
> @@ -7,6 +7,7 @@
> *
> */
>
> +#include "linux/virtio_config.h"
> #include
> #include
> #include
> @@ -145,6 +146,7 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned
> int index,
> /* Assume split virtqueue, switch to packed if necessary */
> struct vdpa_vq_state state = {0};
> unsigned long flags;
> + bool may_reduce_num = false;
> u32 align, num;
> int err;
>
> @@ -169,10 +171,13 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev,
> unsigned int index,
> goto error_new_virtqueue;
> }
>
> + if (ops->get_features(vdpa) & BIT_ULL(VIRTIO_F_VERSION_1))
> + may_reduce_num = true;
> +
> /* Create the vring */
> align = ops->get_vq_align(vdpa);
> vq = vring_create_virtqueue(index, num, align, vdev,
> - true, true, ctx,
> + true, may_reduce_num, ctx,
> virtio_vdpa_notify, callback, name);
> if (!vq) {
> err = -ENOMEM;
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> index 66a41e4ec163..ce0b74276a5b 100644
> --- a/include/uapi/linux/vdpa.h
> +++ b/include/uapi/linux/vdpa.h
> @@ -32,6 +32,7 @@ enum vdpa_attr {
> VDPA_ATTR_DEV_VENDOR_ID,/* u32 */
> VDPA_ATTR_DEV_MAX_VQS, /* u32 */
> VDPA_ATTR_DEV_MAX_VQ_SIZE, /* u16 */
> + VDPA_ATTR_DEV_VERSION_1,/* flag */
>
> /* new attributes must be added above here */
> VDPA_ATTR_MAX,
> --
> 2.31.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
