[PATCH v3 00/17] virtio pci support VIRTIO_F_RING_RESET

[PATCH v3 00/17] virtio pci support VIRTIO_F_RING_RESET

[Xuan Zhuo]

================================================================================
The virtio spec already supports the virtio queue reset function. This patch set
is to add this function to the kernel. The relevant virtio spec information is
here:

    https://github.com/oasis-tcs/virtio-spec/issues/124

Also regarding MMIO support for queue reset, I plan to support it after this
patch is passed.

#14-#17 is the disable/enable function of rx/tx pair implemented by virtio-net
using the new helper. This function is not currently referenced by other
functions. It is more to show the usage of the new helper, I not sure if they
are going to be merged together.

Please review. Thanks.

v3:
  1. keep vq, irq unreleased

Xuan Zhuo (17):
  virtio_pci: struct virtio_pci_common_cfg add queue_notify_data
  virtio: queue_reset: add VIRTIO_F_RING_RESET
  virtio: queue_reset: struct virtio_config_ops add callbacks for
    queue_reset
  virtio: queue_reset: add helper
  vritio_ring: queue_reset: extract the release function of the vq ring
  virtio_ring: queue_reset: split: add __vring_init_virtqueue()
  virtio_ring: queue_reset: split: support enable reset queue
  virtio_ring: queue_reset: packed: support enable reset queue
  virtio_ring: queue_reset: add vring_reset_virtqueue()
  virtio_pci: queue_reset: update struct virtio_pci_common_cfg and
    option functions
  virtio_pci: queue_reset: release vq by vp_dev->vqs
  virtio_pci: queue_reset: setup_vq use vring_setup_virtqueue()
  virtio_pci: queue_reset: support VIRTIO_F_RING_RESET
  virtio_net: virtnet_tx_timeout() fix style
  virtio_net: virtnet_tx_timeout() stop ref sq->vq
  virtio_net: split free_unused_bufs()
  virtio_net: support pair disable/enable

 drivers/net/virtio_net.c               | 220 ++++++++++++++++++++++---
 drivers/virtio/virtio_pci_common.c     |  62 ++++---
 drivers/virtio/virtio_pci_common.h     |  11 +-
 drivers/virtio/virtio_pci_legacy.c     |   5 +-
 drivers/virtio/virtio_pci_modern.c     | 120 +++++++++++++-
 drivers/virtio/virtio_pci_modern_dev.c |  28 ++++
 drivers/virtio/virtio_ring.c           | 144 +++++++++++-----
 include/linux/virtio.h                 |   1 +
 include/linux/virtio_config.h          |  75 ++++++++-
 include/linux/virtio_pci_modern.h      |   2 +
 include/linux/virtio_ring.h            |  42 +++--
 include/uapi/linux/virtio_config.h     |   7 +-
 include/uapi/linux/virtio_pci.h        |   2 +
 13 files changed, 618 insertions(+), 101 deletions(-)

--
2.31.0

[PATCH v3 01/17] virtio_pci: struct virtio_pci_common_cfg add queue_notify_data

[Xuan Zhuo]

Add queue_notify_data in struct virtio_pci_common_cfg, which comes from
here https://github.com/oasis-tcs/virtio-spec/issues/89

Since I want to add queue_reset after it, I submitted this patch first.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 include/uapi/linux/virtio_pci.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
index 3a86f36d7e3d..492c89f56c6a 100644
--- a/include/uapi/linux/virtio_pci.h
+++ b/include/uapi/linux/virtio_pci.h
@@ -164,6 +164,7 @@ struct virtio_pci_common_cfg {
 	__le32 queue_avail_hi;		/* read-write */
 	__le32 queue_used_lo;		/* read-write */
 	__le32 queue_used_hi;		/* read-write */
+	__le16 queue_notify_data;	/* read-write */
 };
 
 /* Fields in VIRTIO_PCI_CAP_PCI_CFG: */
-- 
2.31.0

Re: [PATCH v3 01/17] virtio_pci: struct virtio_pci_common_cfg add queue_notify_data

[Jason Wang]

在 2022/1/26 下午3:35, Xuan Zhuo 写道:
> Add queue_notify_data in struct virtio_pci_common_cfg, which comes from
> here https://github.com/oasis-tcs/virtio-spec/issues/89
>
> Since I want to add queue_reset after it, I submitted this patch first.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   include/uapi/linux/virtio_pci.h | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
> index 3a86f36d7e3d..492c89f56c6a 100644
> --- a/include/uapi/linux/virtio_pci.h
> +++ b/include/uapi/linux/virtio_pci.h
> @@ -164,6 +164,7 @@ struct virtio_pci_common_cfg {
>   	__le32 queue_avail_hi;		/* read-write */
>   	__le32 queue_used_lo;		/* read-write */
>   	__le32 queue_used_hi;		/* read-write */
> +	__le16 queue_notify_data;	/* read-write */
>   };


So I had the same concern as previous version.

This breaks uABI where program may try to use sizeof(struct 
virtio_pci_common_cfg).

We probably need a container structure here.

THanks


>   
>   /* Fields in VIRTIO_PCI_CAP_PCI_CFG: */

Re: [PATCH v3 01/17] virtio_pci: struct virtio_pci_common_cfg add queue_notify_data

[Jason Wang]

On Mon, Feb 7, 2022 at 2:07 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Mon, 7 Feb 2022 11:41:06 +0800, Jason Wang <jasowang@redhat.com> wrote:
> >
> > 在 2022/1/26 下午3:35, Xuan Zhuo 写道:
> > > Add queue_notify_data in struct virtio_pci_common_cfg, which comes from
> > > here https://github.com/oasis-tcs/virtio-spec/issues/89
> > >
> > > Since I want to add queue_reset after it, I submitted this patch first.
> > >
> > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > ---
> > >   include/uapi/linux/virtio_pci.h | 1 +
> > >   1 file changed, 1 insertion(+)
> > >
> > > diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
> > > index 3a86f36d7e3d..492c89f56c6a 100644
> > > --- a/include/uapi/linux/virtio_pci.h
> > > +++ b/include/uapi/linux/virtio_pci.h
> > > @@ -164,6 +164,7 @@ struct virtio_pci_common_cfg {
> > >     __le32 queue_avail_hi;          /* read-write */
> > >     __le32 queue_used_lo;           /* read-write */
> > >     __le32 queue_used_hi;           /* read-write */
> > > +   __le16 queue_notify_data;       /* read-write */
> > >   };
> >
> >
> > So I had the same concern as previous version.
> >
> > This breaks uABI where program may try to use sizeof(struct
> > virtio_pci_common_cfg).
> >
> > We probably need a container structure here.
>
> I see, I plan to add a struct like this, do you think it's appropriate?
>
> struct virtio_pci_common_cfg_v1 {
>         struct virtio_pci_common_cfg cfg;
>         __le16 queue_notify_data;       /* read-write */
> }

Something like this but we probably need a better name.

Thanks

>
> Thanks.
>
> >
> > THanks
> >
> >
> > >
> > >   /* Fields in VIRTIO_PCI_CAP_PCI_CFG: */
> >
>

Re: [PATCH v3 01/17] virtio_pci: struct virtio_pci_common_cfg add queue_notify_data

[Jason Wang]

On Tue, Feb 8, 2022 at 10:17 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Mon, 7 Feb 2022 16:06:15 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Mon, Feb 7, 2022 at 2:07 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >
> > > On Mon, 7 Feb 2022 11:41:06 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > 在 2022/1/26 下午3:35, Xuan Zhuo 写道:
> > > > > Add queue_notify_data in struct virtio_pci_common_cfg, which comes from
> > > > > here https://github.com/oasis-tcs/virtio-spec/issues/89
> > > > >
> > > > > Since I want to add queue_reset after it, I submitted this patch first.
> > > > >
> > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > ---
> > > > >   include/uapi/linux/virtio_pci.h | 1 +
> > > > >   1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
> > > > > index 3a86f36d7e3d..492c89f56c6a 100644
> > > > > --- a/include/uapi/linux/virtio_pci.h
> > > > > +++ b/include/uapi/linux/virtio_pci.h
> > > > > @@ -164,6 +164,7 @@ struct virtio_pci_common_cfg {
> > > > >     __le32 queue_avail_hi;          /* read-write */
> > > > >     __le32 queue_used_lo;           /* read-write */
> > > > >     __le32 queue_used_hi;           /* read-write */
> > > > > +   __le16 queue_notify_data;       /* read-write */
> > > > >   };
> > > >
> > > >
> > > > So I had the same concern as previous version.
> > > >
> > > > This breaks uABI where program may try to use sizeof(struct
> > > > virtio_pci_common_cfg).
> > > >
> > > > We probably need a container structure here.
> > >
> > > I see, I plan to add a struct like this, do you think it's appropriate?
> > >
> > > struct virtio_pci_common_cfg_v1 {
> > >         struct virtio_pci_common_cfg cfg;
> > >         __le16 queue_notify_data;       /* read-write */
> > > }
> >
> > Something like this but we probably need a better name.
>
>
> how about this?
>
>         /* Ext Fields in VIRTIO_PCI_CAP_COMMON_CFG: */
>         struct virtio_pci_common_cfg_ext {
>                 struct virtio_pci_common_cfg cfg;
>
>                 __le16 queue_notify_data;       /* read-write */
>
>                 __le16 reserved0;
>                 __le16 reserved1;
>                 __le16 reserved2;
>                 __le16 reserved3;
>                 __le16 reserved4;
>                 __le16 reserved5;
>                 __le16 reserved6;
>                 __le16 reserved7;
>                 __le16 reserved8;
>                 __le16 reserved9;
>                 __le16 reserved10;
>                 __le16 reserved11;
>                 __le16 reserved12;
>                 __le16 reserved13;
>                 __le16 reserved14;
>         };

I still think the container without padding is better. Otherwise
userspace needs to use offset_of() trick instead of sizeof().

Thanks

>
> Thanks
>
> >
> > Thanks
> >
> > >
> > > Thanks.
> > >
> > > >
> > > > THanks
> > > >
> > > >
> > > > >
> > > > >   /* Fields in VIRTIO_PCI_CAP_PCI_CFG: */
> > > >
> > >
> >
>

Re: [PATCH v3 01/17] virtio_pci: struct virtio_pci_common_cfg add queue_notify_data

[Jason Wang]

On Tue, Feb 8, 2022 at 11:20 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Tue, 8 Feb 2022 11:03:17 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Tue, Feb 8, 2022 at 10:17 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >
> > > On Mon, 7 Feb 2022 16:06:15 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > On Mon, Feb 7, 2022 at 2:07 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > >
> > > > > On Mon, 7 Feb 2022 11:41:06 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > >
> > > > > > 在 2022/1/26 下午3:35, Xuan Zhuo 写道:
> > > > > > > Add queue_notify_data in struct virtio_pci_common_cfg, which comes from
> > > > > > > here https://github.com/oasis-tcs/virtio-spec/issues/89
> > > > > > >
> > > > > > > Since I want to add queue_reset after it, I submitted this patch first.
> > > > > > >
> > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > > > ---
> > > > > > >   include/uapi/linux/virtio_pci.h | 1 +
> > > > > > >   1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
> > > > > > > index 3a86f36d7e3d..492c89f56c6a 100644
> > > > > > > --- a/include/uapi/linux/virtio_pci.h
> > > > > > > +++ b/include/uapi/linux/virtio_pci.h
> > > > > > > @@ -164,6 +164,7 @@ struct virtio_pci_common_cfg {
> > > > > > >     __le32 queue_avail_hi;          /* read-write */
> > > > > > >     __le32 queue_used_lo;           /* read-write */
> > > > > > >     __le32 queue_used_hi;           /* read-write */
> > > > > > > +   __le16 queue_notify_data;       /* read-write */
> > > > > > >   };
> > > > > >
> > > > > >
> > > > > > So I had the same concern as previous version.
> > > > > >
> > > > > > This breaks uABI where program may try to use sizeof(struct
> > > > > > virtio_pci_common_cfg).
> > > > > >
> > > > > > We probably need a container structure here.
> > > > >
> > > > > I see, I plan to add a struct like this, do you think it's appropriate?
> > > > >
> > > > > struct virtio_pci_common_cfg_v1 {
> > > > >         struct virtio_pci_common_cfg cfg;
> > > > >         __le16 queue_notify_data;       /* read-write */
> > > > > }
> > > >
> > > > Something like this but we probably need a better name.
> > >
> > >
> > > how about this?
> > >
> > >         /* Ext Fields in VIRTIO_PCI_CAP_COMMON_CFG: */
> > >         struct virtio_pci_common_cfg_ext {
> > >                 struct virtio_pci_common_cfg cfg;
> > >
> > >                 __le16 queue_notify_data;       /* read-write */
> > >
> > >                 __le16 reserved0;
> > >                 __le16 reserved1;
> > >                 __le16 reserved2;
> > >                 __le16 reserved3;
> > >                 __le16 reserved4;
> > >                 __le16 reserved5;
> > >                 __le16 reserved6;
> > >                 __le16 reserved7;
> > >                 __le16 reserved8;
> > >                 __le16 reserved9;
> > >                 __le16 reserved10;
> > >                 __le16 reserved11;
> > >                 __le16 reserved12;
> > >                 __le16 reserved13;
> > >                 __le16 reserved14;
> > >         };
> >
> > I still think the container without padding is better. Otherwise
> > userspace needs to use offset_of() trick instead of sizeof().
>
> In this case, as virtio_pci_common_cfg_ext adds new members in the future, we
> will add more container structures.
>
> In that case, I think virtio_pci_common_cfg_v1 is a good name instead.

Something like "virtio_pci_common_cfg_notify" might be a little bit better.

Thanks

>
> Thanks.
>
>
> >
> > Thanks
> >
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > Thanks.
> > > > >
> > > > > >
> > > > > > THanks
> > > > > >
> > > > > >
> > > > > > >
> > > > > > >   /* Fields in VIRTIO_PCI_CAP_PCI_CFG: */
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [PATCH v3 01/17] virtio_pci: struct virtio_pci_common_cfg add queue_notify_data

[Jason Wang]

On Tue, Feb 8, 2022 at 11:29 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Tue, 8 Feb 2022 11:24:13 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Tue, Feb 8, 2022 at 11:20 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >
> > > On Tue, 8 Feb 2022 11:03:17 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > On Tue, Feb 8, 2022 at 10:17 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > >
> > > > > On Mon, 7 Feb 2022 16:06:15 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > > On Mon, Feb 7, 2022 at 2:07 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > > > >
> > > > > > > On Mon, 7 Feb 2022 11:41:06 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > >
> > > > > > > > 在 2022/1/26 下午3:35, Xuan Zhuo 写道:
> > > > > > > > > Add queue_notify_data in struct virtio_pci_common_cfg, which comes from
> > > > > > > > > here https://github.com/oasis-tcs/virtio-spec/issues/89
> > > > > > > > >
> > > > > > > > > Since I want to add queue_reset after it, I submitted this patch first.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > > > > > ---
> > > > > > > > >   include/uapi/linux/virtio_pci.h | 1 +
> > > > > > > > >   1 file changed, 1 insertion(+)
> > > > > > > > >
> > > > > > > > > diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
> > > > > > > > > index 3a86f36d7e3d..492c89f56c6a 100644
> > > > > > > > > --- a/include/uapi/linux/virtio_pci.h
> > > > > > > > > +++ b/include/uapi/linux/virtio_pci.h
> > > > > > > > > @@ -164,6 +164,7 @@ struct virtio_pci_common_cfg {
> > > > > > > > >     __le32 queue_avail_hi;          /* read-write */
> > > > > > > > >     __le32 queue_used_lo;           /* read-write */
> > > > > > > > >     __le32 queue_used_hi;           /* read-write */
> > > > > > > > > +   __le16 queue_notify_data;       /* read-write */
> > > > > > > > >   };
> > > > > > > >
> > > > > > > >
> > > > > > > > So I had the same concern as previous version.
> > > > > > > >
> > > > > > > > This breaks uABI where program may try to use sizeof(struct
> > > > > > > > virtio_pci_common_cfg).
> > > > > > > >
> > > > > > > > We probably need a container structure here.
> > > > > > >
> > > > > > > I see, I plan to add a struct like this, do you think it's appropriate?
> > > > > > >
> > > > > > > struct virtio_pci_common_cfg_v1 {
> > > > > > >         struct virtio_pci_common_cfg cfg;
> > > > > > >         __le16 queue_notify_data;       /* read-write */
> > > > > > > }
> > > > > >
> > > > > > Something like this but we probably need a better name.
> > > > >
> > > > >
> > > > > how about this?
> > > > >
> > > > >         /* Ext Fields in VIRTIO_PCI_CAP_COMMON_CFG: */
> > > > >         struct virtio_pci_common_cfg_ext {
> > > > >                 struct virtio_pci_common_cfg cfg;
> > > > >
> > > > >                 __le16 queue_notify_data;       /* read-write */
> > > > >
> > > > >                 __le16 reserved0;
> > > > >                 __le16 reserved1;
> > > > >                 __le16 reserved2;
> > > > >                 __le16 reserved3;
> > > > >                 __le16 reserved4;
> > > > >                 __le16 reserved5;
> > > > >                 __le16 reserved6;
> > > > >                 __le16 reserved7;
> > > > >                 __le16 reserved8;
> > > > >                 __le16 reserved9;
> > > > >                 __le16 reserved10;
> > > > >                 __le16 reserved11;
> > > > >                 __le16 reserved12;
> > > > >                 __le16 reserved13;
> > > > >                 __le16 reserved14;
> > > > >         };
> > > >
> > > > I still think the container without padding is better. Otherwise
> > > > userspace needs to use offset_of() trick instead of sizeof().
> > >
> > > In this case, as virtio_pci_common_cfg_ext adds new members in the future, we
> > > will add more container structures.
> > >
> > > In that case, I think virtio_pci_common_cfg_v1 is a good name instead.
> >
> > Something like "virtio_pci_common_cfg_notify" might be a little bit better.
>
> Although there is only one notify_data in this patch, I plan to look like this
> after my patch set:
>
>         struct virtio_pci_common_cfg_v1 {
>                 struct virtio_pci_common_cfg cfg;
>
>                 __le16 queue_notify_data;       /* read-write */
>                 __le16 queue_reset;       /* read-write */
>         }
>
> If we use virtio_pci_common_cfg_notify, then we will get two structures after
> this patch set:
>
>         struct virtio_pci_common_cfg_notify {
>                 struct virtio_pci_common_cfg cfg;
>
>                 __le16 queue_notify_data;       /* read-write */
>         }
>
>         struct virtio_pci_common_cfg_reset {
>                 struct virtio_pci_common_cfg_notify cfg;
>
>                 __le16 queue_reset;       /* read-write */
>         }

Right, this is sub-optimal, and we need padding in cfg_notify
probably. But I couldn't think of a better idea currently, maybe we
can listen from others opinion

But we use something like this for vnet_header extension

struct virtio_net_hdr_v1{
};

struct virtio_net_hdr_v1_hash{
struct virtio_net_hdr_v1;
__le32 XXX;
...
__le16 padding;
};

And it's not hard to imagine there would be another container for
struct virtio_net_hdr_v1_hash in the future if we want to extend vnet
header.

Thanks

>
>
> Thanks.
>
> >
> > Thanks
> >
> > >
> > > Thanks.
> > >
> > >
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >
> > > > > > > Thanks.
> > > > > > >
> > > > > > > >
> > > > > > > > THanks
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > >   /* Fields in VIRTIO_PCI_CAP_PCI_CFG: */
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

[PATCH v3 02/17] virtio: queue_reset: add VIRTIO_F_RING_RESET

[Xuan Zhuo]

Added VIRTIO_F_RING_RESET, it came from here
https://github.com/oasis-tcs/virtio-spec/issues/124

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 include/uapi/linux/virtio_config.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
index b5eda06f0d57..0862be802ff8 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -52,7 +52,7 @@
  * rest are per-device feature bits.
  */
 #define VIRTIO_TRANSPORT_F_START	28
-#define VIRTIO_TRANSPORT_F_END		38
+#define VIRTIO_TRANSPORT_F_END		41
 
 #ifndef VIRTIO_CONFIG_NO_LEGACY
 /* Do we get callbacks when the ring is completely used, even if we've
@@ -92,4 +92,9 @@
  * Does the device support Single Root I/O Virtualization?
  */
 #define VIRTIO_F_SR_IOV			37
+
+/*
+ * This feature indicates that the driver can reset a queue individually.
+ */
+#define VIRTIO_F_RING_RESET		40
 #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */
-- 
2.31.0

[PATCH v3 03/17] virtio: queue_reset: struct virtio_config_ops add callbacks for queue_reset

[Xuan Zhuo]

Performing reset on a queue is divided into two steps:

1. reset_vq: reset one vq
2. enable_reset_vq: re-enable the reset queue

In the first step, these tasks will be completed:
    1. notify the hardware queue to reset
    2. recycle the buffer from vq
    3. release the ring of the vq

The second step is similar to find vqs, passing parameters callback and
name, etc. Based on the original vq, the ring is re-allocated and
configured to the backend.

So add two callbacks reset_vq, enable_reset_vq to struct
virtio_config_ops.

Add a structure for passing parameters. This will facilitate subsequent
expansion of the parameters of enable reset vq.
There is currently only one default extended parameter ring_num.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 include/linux/virtio_config.h | 43 ++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 4d107ad31149..51dd8461d1b6 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -16,6 +16,44 @@ struct virtio_shm_region {
 	u64 len;
 };
 
+typedef void vq_callback_t(struct virtqueue *);
+
+/* virtio_reset_vq: specify parameters for queue_reset
+ *
+ *	vdev: the device
+ *	queue_index: the queue index
+ *
+ *	free_unused_cb: callback to free unused bufs
+ *	data: used by free_unused_cb
+ *
+ *	callback: callback for the virtqueue, NULL for vq that do not need a
+ *	          callback
+ *	name: virtqueue names (mainly for debugging), NULL for vq unused by
+ *	      driver
+ *	ctx: ctx
+ *
+ *	ring_num: specify ring num for the vq to be re-enabled. 0 means use the
+ *	          default value. MUST be a power of 2.
+ */
+struct virtio_reset_vq;
+typedef void vq_reset_callback_t(struct virtio_reset_vq *param, void *buf);
+struct virtio_reset_vq {
+	struct virtio_device *vdev;
+	u16 queue_index;
+
+	/* reset vq param */
+	vq_reset_callback_t *free_unused_cb;
+	void *data;
+
+	/* enable reset vq param */
+	vq_callback_t *callback;
+	const char *name;
+	const bool *ctx;
+
+	/* ext enable reset vq param */
+	u16 ring_num;
+};
+
 /**
  * virtio_config_ops - operations for configuring a virtio device
  * Note: Do not assume that a transport implements all of the operations
@@ -74,8 +112,9 @@ struct virtio_shm_region {
  * @set_vq_affinity: set the affinity for a virtqueue (optional).
  * @get_vq_affinity: get the affinity for a virtqueue (optional).
  * @get_shm_region: get a shared memory region based on the index.
+ * @reset_vq: reset a queue individually
+ * @enable_reset_vq: enable a reset queue
  */
-typedef void vq_callback_t(struct virtqueue *);
 struct virtio_config_ops {
 	void (*enable_cbs)(struct virtio_device *vdev);
 	void (*get)(struct virtio_device *vdev, unsigned offset,
@@ -100,6 +139,8 @@ struct virtio_config_ops {
 			int index);
 	bool (*get_shm_region)(struct virtio_device *vdev,
 			       struct virtio_shm_region *region, u8 id);
+	int (*reset_vq)(struct virtio_reset_vq *param);
+	struct virtqueue *(*enable_reset_vq)(struct virtio_reset_vq *param);
 };
 
 /* If driver didn't advertise the feature, it will never appear. */
-- 
2.31.0

Re: [PATCH v3 03/17] virtio: queue_reset: struct virtio_config_ops add callbacks for queue_reset

[Jason Wang]

在 2022/1/26 下午3:35, Xuan Zhuo 写道:
> Performing reset on a queue is divided into two steps:
>
> 1. reset_vq: reset one vq
> 2. enable_reset_vq: re-enable the reset queue
>
> In the first step, these tasks will be completed:
>      1. notify the hardware queue to reset
>      2. recycle the buffer from vq
>      3. release the ring of the vq
>
> The second step is similar to find vqs,


Not sure, since find_vqs will usually try to allocate interrupts.


>   passing parameters callback and
> name, etc. Based on the original vq, the ring is re-allocated and
> configured to the backend.


I wonder whether we really have such requirement.

For example, do we really have a use case that may change:

vq callback, ctx, ring_num or even re-create the virtqueue?

Thanks


>
> So add two callbacks reset_vq, enable_reset_vq to struct
> virtio_config_ops.
>
> Add a structure for passing parameters. This will facilitate subsequent
> expansion of the parameters of enable reset vq.
> There is currently only one default extended parameter ring_num.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   include/linux/virtio_config.h | 43 ++++++++++++++++++++++++++++++++++-
>   1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 4d107ad31149..51dd8461d1b6 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -16,6 +16,44 @@ struct virtio_shm_region {
>   	u64 len;
>   };
>   
> +typedef void vq_callback_t(struct virtqueue *);
> +
> +/* virtio_reset_vq: specify parameters for queue_reset
> + *
> + *	vdev: the device
> + *	queue_index: the queue index
> + *
> + *	free_unused_cb: callback to free unused bufs
> + *	data: used by free_unused_cb
> + *
> + *	callback: callback for the virtqueue, NULL for vq that do not need a
> + *	          callback
> + *	name: virtqueue names (mainly for debugging), NULL for vq unused by
> + *	      driver
> + *	ctx: ctx
> + *
> + *	ring_num: specify ring num for the vq to be re-enabled. 0 means use the
> + *	          default value. MUST be a power of 2.
> + */
> +struct virtio_reset_vq;
> +typedef void vq_reset_callback_t(struct virtio_reset_vq *param, void *buf);
> +struct virtio_reset_vq {
> +	struct virtio_device *vdev;
> +	u16 queue_index;
> +
> +	/* reset vq param */
> +	vq_reset_callback_t *free_unused_cb;
> +	void *data;
> +
> +	/* enable reset vq param */
> +	vq_callback_t *callback;
> +	const char *name;
> +	const bool *ctx;
> +
> +	/* ext enable reset vq param */
> +	u16 ring_num;
> +};
> +
>   /**
>    * virtio_config_ops - operations for configuring a virtio device
>    * Note: Do not assume that a transport implements all of the operations
> @@ -74,8 +112,9 @@ struct virtio_shm_region {
>    * @set_vq_affinity: set the affinity for a virtqueue (optional).
>    * @get_vq_affinity: get the affinity for a virtqueue (optional).
>    * @get_shm_region: get a shared memory region based on the index.
> + * @reset_vq: reset a queue individually
> + * @enable_reset_vq: enable a reset queue
>    */
> -typedef void vq_callback_t(struct virtqueue *);
>   struct virtio_config_ops {
>   	void (*enable_cbs)(struct virtio_device *vdev);
>   	void (*get)(struct virtio_device *vdev, unsigned offset,
> @@ -100,6 +139,8 @@ struct virtio_config_ops {
>   			int index);
>   	bool (*get_shm_region)(struct virtio_device *vdev,
>   			       struct virtio_shm_region *region, u8 id);
> +	int (*reset_vq)(struct virtio_reset_vq *param);
> +	struct virtqueue *(*enable_reset_vq)(struct virtio_reset_vq *param);
>   };
>   
>   /* If driver didn't advertise the feature, it will never appear. */

[PATCH v3 04/17] virtio: queue_reset: add helper

[Xuan Zhuo]

Add helper for virtio queue reset.

* virtio_reset_vq: reset a queue individually
* virtio_enable_resetq: enable a reset queue

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 include/linux/virtio_config.h | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 51dd8461d1b6..3c971d9a0a59 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -260,6 +260,38 @@ int virtio_find_vqs_ctx(struct virtio_device *vdev, unsigned nvqs,
 				      desc);
 }
 
+/**
+ * virtio_reset_vq - reset a queue individually
+ * @param: struct virtio_reset_vq
+ *
+ * returns 0 on success or error status
+ *
+ */
+static inline
+int virtio_reset_vq(struct virtio_reset_vq *param)
+{
+	if (!param->vdev->config->reset_vq)
+		return -ENOENT;
+
+	return param->vdev->config->reset_vq(param);
+}
+
+/**
+ * virtio_enable_resetq - enable a reset queue
+ * @param: struct virtio_reset_vq
+ *
+ * returns vq on success or error status
+ *
+ */
+static inline
+struct virtqueue *virtio_enable_resetq(struct virtio_reset_vq *param)
+{
+	if (!param->vdev->config->enable_reset_vq)
+		return ERR_PTR(-ENOENT);
+
+	return param->vdev->config->enable_reset_vq(param);
+}
+
 /**
  * virtio_device_ready - enable vq use in probe function
  * @vdev: the device
-- 
2.31.0

[PATCH v3 05/17] vritio_ring: queue_reset: extract the release function of the vq ring

[Xuan Zhuo]

Extract a function __vring_del_virtqueue() from vring_del_virtqueue() to
handle releasing vq's ring.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_ring.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 028b05d44546..b6434e36c447 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2300,12 +2300,10 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
 }
 EXPORT_SYMBOL_GPL(vring_new_virtqueue);
 
-void vring_del_virtqueue(struct virtqueue *_vq)
+static void __vring_del_virtqueue(struct vring_virtqueue *vq)
 {
-	struct vring_virtqueue *vq = to_vvq(_vq);
-
 	spin_lock(&vq->vq.vdev->vqs_list_lock);
-	list_del(&_vq->list);
+	list_del(&vq->vq.list);
 	spin_unlock(&vq->vq.vdev->vqs_list_lock);
 
 	if (vq->we_own_ring) {
@@ -2338,6 +2336,13 @@ void vring_del_virtqueue(struct virtqueue *_vq)
 		kfree(vq->split.desc_state);
 		kfree(vq->split.desc_extra);
 	}
+}
+
+void vring_del_virtqueue(struct virtqueue *_vq)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+
+	__vring_del_virtqueue(vq);
 	kfree(vq);
 }
 EXPORT_SYMBOL_GPL(vring_del_virtqueue);
-- 
2.31.0

[PATCH v3 06/17] virtio_ring: queue_reset: split: add __vring_init_virtqueue()

[Xuan Zhuo]

Extract vq's initialization function __vring_init_virtqueue() from
__vring_new_virtqueue()

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_ring.c | 61 +++++++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 19 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index b6434e36c447..4e1d345b0557 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2167,23 +2167,17 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
 EXPORT_SYMBOL_GPL(vring_interrupt);
 
 /* Only available for split ring */
-struct virtqueue *__vring_new_virtqueue(unsigned int index,
-					struct vring vring,
-					struct virtio_device *vdev,
-					bool weak_barriers,
-					bool context,
-					bool (*notify)(struct virtqueue *),
-					void (*callback)(struct virtqueue *),
-					const char *name)
+static int __vring_init_virtqueue(struct virtqueue *_vq,
+				  unsigned int index,
+				  struct vring vring,
+				  struct virtio_device *vdev,
+				  bool weak_barriers,
+				  bool context,
+				  bool (*notify)(struct virtqueue *),
+				  void (*callback)(struct virtqueue *),
+				  const char *name)
 {
-	struct vring_virtqueue *vq;
-
-	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
-		return NULL;
-
-	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
-	if (!vq)
-		return NULL;
+	struct vring_virtqueue *vq = to_vvq(_vq);
 
 	vq->packed_ring = false;
 	vq->vq.callback = callback;
@@ -2243,13 +2237,42 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 	spin_lock(&vdev->vqs_list_lock);
 	list_add_tail(&vq->vq.list, &vdev->vqs);
 	spin_unlock(&vdev->vqs_list_lock);
-	return &vq->vq;
+	return 0;
 
 err_extra:
 	kfree(vq->split.desc_state);
 err_state:
-	kfree(vq);
-	return NULL;
+	return -ENOMEM;
+}
+
+struct virtqueue *__vring_new_virtqueue(unsigned int index,
+					struct vring vring,
+					struct virtio_device *vdev,
+					bool weak_barriers,
+					bool context,
+					bool (*notify)(struct virtqueue *),
+					void (*callback)(struct virtqueue *),
+					const char *name)
+{
+	struct vring_virtqueue *vq;
+	int err;
+
+	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
+		return NULL;
+
+	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
+	if (!vq)
+		return NULL;
+
+	err = __vring_init_virtqueue(&vq->vq, index, vring, vdev, weak_barriers,
+				     context, notify, callback, name);
+
+	if (err) {
+		kfree(vq);
+		return NULL;
+	}
+
+	return &vq->vq;
 }
 EXPORT_SYMBOL_GPL(__vring_new_virtqueue);
 
-- 
2.31.0

[PATCH v3 07/17] virtio_ring: queue_reset: split: support enable reset queue

[Xuan Zhuo]

The purpose of this patch is to make vring split support re-enable reset
vq.

The core idea is that when calling vring_create_virtqueue_split, use the
existing vq instead of re-allocating vq. vq is passed in through
function parameters.

Modify vring_create_virtqueue() to vring_setup_virtqueue(), and add a
new parameter vq. At the same time, add a new inline function, the
definition of the function is exactly the same as the original
vring_create_virtqueue().

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_ring.c | 41 ++++++++++++++++++++++++++----------
 include/linux/virtio.h       |  1 +
 include/linux/virtio_ring.h  | 37 +++++++++++++++++++++++---------
 3 files changed, 58 insertions(+), 21 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 4e1d345b0557..a93a677008ba 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -198,6 +198,15 @@ struct vring_virtqueue {
 #endif
 };
 
+static int __vring_init_virtqueue(struct virtqueue *_vq,
+				  unsigned int index,
+				  struct vring vring,
+				  struct virtio_device *vdev,
+				  bool weak_barriers,
+				  bool context,
+				  bool (*notify)(struct virtqueue *),
+				  void (*callback)(struct virtqueue *),
+				  const char *name);
 
 /*
  * Helpers.
@@ -925,9 +934,9 @@ static struct virtqueue *vring_create_virtqueue_split(
 	bool context,
 	bool (*notify)(struct virtqueue *),
 	void (*callback)(struct virtqueue *),
-	const char *name)
+	const char *name,
+	struct virtqueue *vq)
 {
-	struct virtqueue *vq;
 	void *queue = NULL;
 	dma_addr_t dma_addr;
 	size_t queue_size_in_bytes;
@@ -964,12 +973,17 @@ static struct virtqueue *vring_create_virtqueue_split(
 	queue_size_in_bytes = vring_size(num, vring_align);
 	vring_init(&vring, num, queue, vring_align);
 
-	vq = __vring_new_virtqueue(index, vring, vdev, weak_barriers, context,
-				   notify, callback, name);
 	if (!vq) {
-		vring_free_queue(vdev, queue_size_in_bytes, queue,
-				 dma_addr);
-		return NULL;
+		vq = __vring_new_virtqueue(index, vring, vdev, weak_barriers,
+					   context, notify, callback, name);
+		if (!vq)
+			goto err;
+
+	} else {
+		if (__vring_init_virtqueue(vq, index, vring, vdev,
+					   weak_barriers, context, notify,
+					   callback, name))
+			goto err;
 	}
 
 	to_vvq(vq)->split.queue_dma_addr = dma_addr;
@@ -977,6 +991,9 @@ static struct virtqueue *vring_create_virtqueue_split(
 	to_vvq(vq)->we_own_ring = true;
 
 	return vq;
+err:
+	vring_free_queue(vdev, queue_size_in_bytes, queue, dma_addr);
+	return NULL;
 }
 
 
@@ -2185,6 +2202,7 @@ static int __vring_init_virtqueue(struct virtqueue *_vq,
 	vq->vq.name = name;
 	vq->vq.num_free = vring.num;
 	vq->vq.index = index;
+	vq->vq.reset = false;
 	vq->we_own_ring = false;
 	vq->notify = notify;
 	vq->weak_barriers = weak_barriers;
@@ -2276,7 +2294,7 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 }
 EXPORT_SYMBOL_GPL(__vring_new_virtqueue);
 
-struct virtqueue *vring_create_virtqueue(
+struct virtqueue *vring_setup_virtqueue(
 	unsigned int index,
 	unsigned int num,
 	unsigned int vring_align,
@@ -2286,7 +2304,8 @@ struct virtqueue *vring_create_virtqueue(
 	bool context,
 	bool (*notify)(struct virtqueue *),
 	void (*callback)(struct virtqueue *),
-	const char *name)
+	const char *name,
+	struct virtqueue *vq)
 {
 
 	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
@@ -2296,9 +2315,9 @@ struct virtqueue *vring_create_virtqueue(
 
 	return vring_create_virtqueue_split(index, num, vring_align,
 			vdev, weak_barriers, may_reduce_num,
-			context, notify, callback, name);
+			context, notify, callback, name, vq);
 }
-EXPORT_SYMBOL_GPL(vring_create_virtqueue);
+EXPORT_SYMBOL_GPL(vring_setup_virtqueue);
 
 /* Only available for split ring */
 struct virtqueue *vring_new_virtqueue(unsigned int index,
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 41edbc01ffa4..98c89cbeafa1 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -32,6 +32,7 @@ struct virtqueue {
 	unsigned int index;
 	unsigned int num_free;
 	void *priv;
+	bool reset;
 };
 
 int virtqueue_add_outbuf(struct virtqueue *vq,
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index b485b13fa50b..e90323fce4bf 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -65,16 +65,33 @@ struct virtqueue;
  * expected.  The caller should query virtqueue_get_vring_size to learn
  * the actual size of the ring.
  */
-struct virtqueue *vring_create_virtqueue(unsigned int index,
-					 unsigned int num,
-					 unsigned int vring_align,
-					 struct virtio_device *vdev,
-					 bool weak_barriers,
-					 bool may_reduce_num,
-					 bool ctx,
-					 bool (*notify)(struct virtqueue *vq),
-					 void (*callback)(struct virtqueue *vq),
-					 const char *name);
+struct virtqueue *vring_setup_virtqueue(unsigned int index,
+					unsigned int num,
+					unsigned int vring_align,
+					struct virtio_device *vdev,
+					bool weak_barriers,
+					bool may_reduce_num,
+					bool ctx,
+					bool (*notify)(struct virtqueue *vq),
+					void (*callback)(struct virtqueue *vq),
+					const char *name,
+					struct virtqueue *vq);
+
+static inline struct virtqueue *vring_create_virtqueue(unsigned int index,
+						       unsigned int num,
+						       unsigned int vring_align,
+						       struct virtio_device *vdev,
+						       bool weak_barriers,
+						       bool may_reduce_num,
+						       bool ctx,
+						       bool (*notify)(struct virtqueue *vq),
+						       void (*callback)(struct virtqueue *vq),
+						       const char *name)
+{
+	return vring_setup_virtqueue(index, num, vring_align, vdev,
+				     weak_barriers, may_reduce_num, ctx,
+				     notify, callback, name, NULL);
+}
 
 /* Creates a virtqueue with a custom layout. */
 struct virtqueue *__vring_new_virtqueue(unsigned int index,
-- 
2.31.0

[PATCH v3 08/17] virtio_ring: queue_reset: packed: support enable reset queue

[Xuan Zhuo]

The purpose of this patch is to make vring packed support re-enable reset
vq.

The core idea is that when calling vring_create_virtqueue_packed, use the
existing vq instead of re-allocating vq. vq is passed in through
function parameters.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_ring.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index a93a677008ba..4beb7c7127c1 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1680,7 +1680,8 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	bool context,
 	bool (*notify)(struct virtqueue *),
 	void (*callback)(struct virtqueue *),
-	const char *name)
+	const char *name,
+	struct virtqueue *_vq)
 {
 	struct vring_virtqueue *vq;
 	struct vring_packed_desc *ring;
@@ -1710,15 +1711,20 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	if (!device)
 		goto err_device;
 
-	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
-	if (!vq)
-		goto err_vq;
+	if (_vq) {
+		vq = to_vvq(_vq);
+	} else {
+		vq = kmalloc(sizeof(*vq), GFP_KERNEL);
+		if (!vq)
+			goto err_vq;
+	}
 
 	vq->vq.callback = callback;
 	vq->vq.vdev = vdev;
 	vq->vq.name = name;
 	vq->vq.num_free = num;
 	vq->vq.index = index;
+	vq->vq.reset = false;
 	vq->we_own_ring = true;
 	vq->notify = notify;
 	vq->weak_barriers = weak_barriers;
@@ -1789,7 +1795,8 @@ static struct virtqueue *vring_create_virtqueue_packed(
 err_desc_extra:
 	kfree(vq->packed.desc_state);
 err_desc_state:
-	kfree(vq);
+	if (!_vq)
+		kfree(vq);
 err_vq:
 	vring_free_queue(vdev, event_size_in_bytes, device, device_event_dma_addr);
 err_device:
@@ -2311,7 +2318,7 @@ struct virtqueue *vring_setup_virtqueue(
 	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
 		return vring_create_virtqueue_packed(index, num, vring_align,
 				vdev, weak_barriers, may_reduce_num,
-				context, notify, callback, name);
+				context, notify, callback, name, vq);
 
 	return vring_create_virtqueue_split(index, num, vring_align,
 			vdev, weak_barriers, may_reduce_num,
-- 
2.31.0

[PATCH v3 09/17] virtio_ring: queue_reset: add vring_reset_virtqueue()

[Xuan Zhuo]

Added vring_reset_virtqueue() for reset vring_virtqueue.

In this process, vq is removed from the vdev->vqs queue. And the memory
of the ring is released

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_ring.c | 12 +++++++++++-
 include/linux/virtio_ring.h  |  5 +++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 4beb7c7127c1..bba9f3c67b33 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2391,11 +2391,21 @@ void vring_del_virtqueue(struct virtqueue *_vq)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 
-	__vring_del_virtqueue(vq);
+	if (!_vq->reset)
+		__vring_del_virtqueue(vq);
 	kfree(vq);
 }
 EXPORT_SYMBOL_GPL(vring_del_virtqueue);
 
+void vring_reset_virtqueue(struct virtqueue *_vq)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+
+	__vring_del_virtqueue(vq);
+	_vq->reset = true;
+}
+EXPORT_SYMBOL_GPL(vring_reset_virtqueue);
+
 /* Manipulates transport-specific feature bits. */
 void vring_transport_features(struct virtio_device *vdev)
 {
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index e90323fce4bf..84b55fb8686d 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -124,6 +124,11 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
  */
 void vring_del_virtqueue(struct virtqueue *vq);
 
+/*
+ * Resets a virtqueue. Just frees the ring, not free vq.
+ */
+void vring_reset_virtqueue(struct virtqueue *vq);
+
 /* Filter out transport-specific feature bits. */
 void vring_transport_features(struct virtio_device *vdev);
 
-- 
2.31.0

[PATCH v3 10/17] virtio_pci: queue_reset: update struct virtio_pci_common_cfg and option functions

[Xuan Zhuo]

Add queue_reset in virtio_pci_common_cfg, and add related operation
functions.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_pci_modern_dev.c | 28 ++++++++++++++++++++++++++
 include/linux/virtio_pci_modern.h      |  2 ++
 include/uapi/linux/virtio_pci.h        |  1 +
 3 files changed, 31 insertions(+)

diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
index e11ed748e661..be02f812a2f6 100644
--- a/drivers/virtio/virtio_pci_modern_dev.c
+++ b/drivers/virtio/virtio_pci_modern_dev.c
@@ -463,6 +463,34 @@ void vp_modern_set_status(struct virtio_pci_modern_device *mdev,
 }
 EXPORT_SYMBOL_GPL(vp_modern_set_status);
 
+/*
+ * vp_modern_get_queue_reset - get the queue reset status
+ * @mdev: the modern virtio-pci device
+ * @index: queue index
+ */
+int vp_modern_get_queue_reset(struct virtio_pci_modern_device *mdev, u16 index)
+{
+	struct virtio_pci_common_cfg __iomem *cfg = mdev->common;
+
+	vp_iowrite16(index, &cfg->queue_select);
+	return vp_ioread16(&cfg->queue_reset);
+}
+EXPORT_SYMBOL_GPL(vp_modern_get_queue_reset);
+
+/*
+ * vp_modern_set_queue_reset - reset the queue
+ * @mdev: the modern virtio-pci device
+ * @index: queue index
+ */
+void vp_modern_set_queue_reset(struct virtio_pci_modern_device *mdev, u16 index)
+{
+	struct virtio_pci_common_cfg __iomem *cfg = mdev->common;
+
+	vp_iowrite16(index, &cfg->queue_select);
+	vp_iowrite16(1, &cfg->queue_reset);
+}
+EXPORT_SYMBOL_GPL(vp_modern_set_queue_reset);
+
 /*
  * vp_modern_queue_vector - set the MSIX vector for a specific virtqueue
  * @mdev: the modern virtio-pci device
diff --git a/include/linux/virtio_pci_modern.h b/include/linux/virtio_pci_modern.h
index eb2bd9b4077d..cc4154dd7b28 100644
--- a/include/linux/virtio_pci_modern.h
+++ b/include/linux/virtio_pci_modern.h
@@ -106,4 +106,6 @@ void __iomem * vp_modern_map_vq_notify(struct virtio_pci_modern_device *mdev,
 				       u16 index, resource_size_t *pa);
 int vp_modern_probe(struct virtio_pci_modern_device *mdev);
 void vp_modern_remove(struct virtio_pci_modern_device *mdev);
+int vp_modern_get_queue_reset(struct virtio_pci_modern_device *mdev, u16 index);
+void vp_modern_set_queue_reset(struct virtio_pci_modern_device *mdev, u16 index);
 #endif
diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
index 492c89f56c6a..bf10f349087f 100644
--- a/include/uapi/linux/virtio_pci.h
+++ b/include/uapi/linux/virtio_pci.h
@@ -165,6 +165,7 @@ struct virtio_pci_common_cfg {
 	__le32 queue_used_lo;		/* read-write */
 	__le32 queue_used_hi;		/* read-write */
 	__le16 queue_notify_data;	/* read-write */
+	__le16 queue_reset;		/* read-write */
 };
 
 /* Fields in VIRTIO_PCI_CAP_PCI_CFG: */
-- 
2.31.0

[PATCH v3 11/17] virtio_pci: queue_reset: release vq by vp_dev->vqs

[Xuan Zhuo]

In the process of queue reset, vq leaves vdev->vqs, so the original
processing logic may miss some vq. So modify the processing method of
releasing vq. Release vq by listing vqs.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_pci_common.c | 22 ++++++++++++++++++----
 drivers/virtio/virtio_pci_common.h |  2 ++
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index fdbde1db5ec5..6b2573ec1ae8 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -260,12 +260,20 @@ static void vp_del_vq(struct virtqueue *vq)
 void vp_del_vqs(struct virtio_device *vdev)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	struct virtqueue *vq, *n;
-	int i;
+	struct virtio_pci_vq_info *info;
+	struct virtqueue *vq;
+	int i, v;
+
+	for (i = 0; i < vp_dev->nvqs; ++i) {
+
+		info = vp_dev->vqs[i];
+		if (!info)
+			continue;
+
+		vq = info->vq;
 
-	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
 		if (vp_dev->per_vq_vectors) {
-			int v = vp_dev->vqs[vq->index]->msix_vector;
+			v = info->msix_vector;
 
 			if (v != VIRTIO_MSI_NO_VECTOR) {
 				int irq = pci_irq_vector(vp_dev->pci_dev, v);
@@ -275,6 +283,7 @@ void vp_del_vqs(struct virtio_device *vdev)
 			}
 		}
 		vp_del_vq(vq);
+		vp_dev->vqs[i] = NULL;
 	}
 	vp_dev->per_vq_vectors = false;
 
@@ -308,6 +317,7 @@ void vp_del_vqs(struct virtio_device *vdev)
 	vp_dev->msix_affinity_masks = NULL;
 	kfree(vp_dev->vqs);
 	vp_dev->vqs = NULL;
+	vp_dev->nvqs = 0;
 }
 
 static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
@@ -324,6 +334,8 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
 	if (!vp_dev->vqs)
 		return -ENOMEM;
 
+	vp_dev->nvqs = nvqs;
+
 	if (per_vq_vectors) {
 		/* Best option: one for change interrupt, one per vq. */
 		nvectors = 1;
@@ -395,6 +407,8 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned nvqs,
 	if (!vp_dev->vqs)
 		return -ENOMEM;
 
+	vp_dev->nvqs = nvqs;
+
 	err = request_irq(vp_dev->pci_dev->irq, vp_interrupt, IRQF_SHARED,
 			dev_name(&vdev->dev), vp_dev);
 	if (err)
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 23f6c5c678d5..392d990b7c73 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -60,6 +60,8 @@ struct virtio_pci_device {
 	/* array of all queues for house-keeping */
 	struct virtio_pci_vq_info **vqs;
 
+	u32 nvqs;
+
 	/* MSI-X support */
 	int msix_enabled;
 	int intx_enabled;
-- 
2.31.0

[PATCH v3 12/17] virtio_pci: queue_reset: setup_vq use vring_setup_virtqueue()

[Xuan Zhuo]

setup_vq replaces vring_create_virtuque() with vring_setup_virtqueue() to
support the need to enable reset vq.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_pci_common.c |  8 ++++----
 drivers/virtio/virtio_pci_common.h |  4 +++-
 drivers/virtio/virtio_pci_legacy.c |  5 +++--
 drivers/virtio/virtio_pci_modern.c | 20 ++++++++++++++------
 4 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 6b2573ec1ae8..c02936d29a31 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -209,7 +209,7 @@ static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
 				     void (*callback)(struct virtqueue *vq),
 				     const char *name,
 				     bool ctx,
-				     u16 msix_vec)
+				     u16 msix_vec, u16 num)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtio_pci_vq_info *info = kmalloc(sizeof *info, GFP_KERNEL);
@@ -221,7 +221,7 @@ static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
 		return ERR_PTR(-ENOMEM);
 
 	vq = vp_dev->setup_vq(vp_dev, info, index, callback, name, ctx,
-			      msix_vec);
+			      msix_vec, NULL, num);
 	if (IS_ERR(vq))
 		goto out_info;
 
@@ -368,7 +368,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
 			msix_vec = VP_MSIX_VQ_VECTOR;
 		vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
 				     ctx ? ctx[i] : false,
-				     msix_vec);
+				     msix_vec, 0);
 		if (IS_ERR(vqs[i])) {
 			err = PTR_ERR(vqs[i]);
 			goto error_find;
@@ -423,7 +423,7 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned nvqs,
 		}
 		vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
 				     ctx ? ctx[i] : false,
-				     VIRTIO_MSI_NO_VECTOR);
+				     VIRTIO_MSI_NO_VECTOR, 0);
 		if (IS_ERR(vqs[i])) {
 			err = PTR_ERR(vqs[i]);
 			goto out_del_vqs;
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 392d990b7c73..65db92245e41 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -84,7 +84,9 @@ struct virtio_pci_device {
 				      void (*callback)(struct virtqueue *vq),
 				      const char *name,
 				      bool ctx,
-				      u16 msix_vec);
+				      u16 msix_vec,
+				      struct virtqueue *vq,
+				      u16 num);
 	void (*del_vq)(struct virtio_pci_vq_info *info);
 
 	u16 (*config_vector)(struct virtio_pci_device *vp_dev, u16 vector);
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index b3f8128b7983..9bc41b764624 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -113,9 +113,10 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 				  void (*callback)(struct virtqueue *vq),
 				  const char *name,
 				  bool ctx,
-				  u16 msix_vec)
+				  u16 msix_vec,
+				  struct virtqueue *vq,
+				  u16 ring_num)
 {
-	struct virtqueue *vq;
 	u16 num;
 	int err;
 	u64 q_pfn;
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 5455bc041fb6..2ce58de549de 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -187,11 +187,12 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 				  void (*callback)(struct virtqueue *vq),
 				  const char *name,
 				  bool ctx,
-				  u16 msix_vec)
+				  u16 msix_vec,
+				  struct virtqueue *vq,
+				  u16 ring_num)
 {
 
 	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
-	struct virtqueue *vq;
 	u16 num;
 	int err;
 
@@ -203,6 +204,13 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 	if (!num || vp_modern_get_queue_enable(mdev, index))
 		return ERR_PTR(-ENOENT);
 
+	if (ring_num) {
+		if (ring_num > num)
+			return ERR_PTR(-ENOENT);
+
+		num = ring_num;
+	}
+
 	if (num & (num - 1)) {
 		dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u", num);
 		return ERR_PTR(-EINVAL);
@@ -211,10 +219,10 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 	info->msix_vector = msix_vec;
 
 	/* create the vring */
-	vq = vring_create_virtqueue(index, num,
-				    SMP_CACHE_BYTES, &vp_dev->vdev,
-				    true, true, ctx,
-				    vp_notify, callback, name);
+	vq = vring_setup_virtqueue(index, num,
+				   SMP_CACHE_BYTES, &vp_dev->vdev,
+				   true, true, ctx,
+				   vp_notify, callback, name, vq);
 	if (!vq)
 		return ERR_PTR(-ENOMEM);
 
-- 
2.31.0

[PATCH v3 13/17] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET

[Xuan Zhuo]

This patch implements virtio pci support for QUEUE RESET.

Performing reset on a queue is divided into two steps:

1. reset_vq: reset one vq
2. enable_reset_vq: re-enable the reset queue

In the first step, these tasks will be completed:
   1. notify the hardware queue to reset
   2. recycle the buffer from vq
   3. release the ring of the vq

The process of enable reset vq:
    vp_modern_enable_reset_vq()
    vp_enable_reset_vq()
    __vp_setup_vq()
    setup_vq()
    vring_setup_virtqueue()

In this process, we added two parameters, vq and num, and finally passed
them to vring_setup_virtqueue().  And reuse the original info and vq.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_pci_common.c |  36 +++++++----
 drivers/virtio/virtio_pci_common.h |   5 ++
 drivers/virtio/virtio_pci_modern.c | 100 +++++++++++++++++++++++++++++
 3 files changed, 128 insertions(+), 13 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index c02936d29a31..ad21638fbf66 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -205,23 +205,28 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
 	return err;
 }
 
-static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
-				     void (*callback)(struct virtqueue *vq),
-				     const char *name,
-				     bool ctx,
-				     u16 msix_vec, u16 num)
+struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
+			      void (*callback)(struct virtqueue *vq),
+			      const char *name,
+			      bool ctx,
+			      u16 msix_vec, u16 num)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	struct virtio_pci_vq_info *info = kmalloc(sizeof *info, GFP_KERNEL);
+	struct virtio_pci_vq_info *info;
 	struct virtqueue *vq;
 	unsigned long flags;
 
-	/* fill out our structure that represents an active queue */
-	if (!info)
-		return ERR_PTR(-ENOMEM);
+	info = vp_dev->vqs[index];
+	if (!info) {
+		info = kzalloc(sizeof *info, GFP_KERNEL);
+
+		/* fill out our structure that represents an active queue */
+		if (!info)
+			return ERR_PTR(-ENOMEM);
+	}
 
 	vq = vp_dev->setup_vq(vp_dev, info, index, callback, name, ctx,
-			      msix_vec, NULL, num);
+			      msix_vec, info->vq, num);
 	if (IS_ERR(vq))
 		goto out_info;
 
@@ -238,6 +243,9 @@ static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
 	return vq;
 
 out_info:
+	if (info->vq && info->vq->reset)
+		return vq;
+
 	kfree(info);
 	return vq;
 }
@@ -248,9 +256,11 @@ static void vp_del_vq(struct virtqueue *vq)
 	struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
 	unsigned long flags;
 
-	spin_lock_irqsave(&vp_dev->lock, flags);
-	list_del(&info->node);
-	spin_unlock_irqrestore(&vp_dev->lock, flags);
+	if (!vq->reset) {
+		spin_lock_irqsave(&vp_dev->lock, flags);
+		list_del(&info->node);
+		spin_unlock_irqrestore(&vp_dev->lock, flags);
+	}
 
 	vp_dev->del_vq(info);
 	kfree(info);
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 65db92245e41..c1d15f7c0be4 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -119,6 +119,11 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 		struct virtqueue *vqs[], vq_callback_t *callbacks[],
 		const char * const names[], const bool *ctx,
 		struct irq_affinity *desc);
+struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
+			      void (*callback)(struct virtqueue *vq),
+			      const char *name,
+			      bool ctx,
+			      u16 msix_vec, u16 num);
 const char *vp_bus_name(struct virtio_device *vdev);
 
 /* Setup the affinity for a virtqueue:
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 2ce58de549de..6789411169e4 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -34,6 +34,9 @@ static void vp_transport_features(struct virtio_device *vdev, u64 features)
 	if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
 			pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
 		__virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
+
+	if (features & BIT_ULL(VIRTIO_F_RING_RESET))
+		__virtio_set_bit(vdev, VIRTIO_F_RING_RESET);
 }
 
 /* virtio config->finalize_features() implementation */
@@ -176,6 +179,94 @@ static void vp_reset(struct virtio_device *vdev)
 	vp_disable_cbs(vdev);
 }
 
+static int vp_modern_reset_vq(struct virtio_reset_vq *param)
+{
+	struct virtio_pci_device *vp_dev = to_vp_device(param->vdev);
+	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
+	struct virtio_pci_vq_info *info;
+	u16 msix_vec, queue_index;
+	unsigned long flags;
+	void *buf;
+
+	if (!virtio_has_feature(param->vdev, VIRTIO_F_RING_RESET))
+		return -ENOENT;
+
+	queue_index = param->queue_index;
+
+	vp_modern_set_queue_reset(mdev, queue_index);
+
+	/* After write 1 to queue reset, the driver MUST wait for a read of
+	 * queue reset to return 1.
+	 */
+	while (vp_modern_get_queue_reset(mdev, queue_index) != 1)
+		msleep(1);
+
+	info = vp_dev->vqs[queue_index];
+	msix_vec = info->msix_vector;
+
+	/* Disable VQ callback. */
+	if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
+		disable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
+
+	while ((buf = virtqueue_detach_unused_buf(info->vq)) != NULL)
+		param->free_unused_cb(param, buf);
+
+	/* delete vq */
+	spin_lock_irqsave(&vp_dev->lock, flags);
+	list_del(&info->node);
+	spin_unlock_irqrestore(&vp_dev->lock, flags);
+
+	INIT_LIST_HEAD(&info->node);
+
+	if (vp_dev->msix_enabled)
+		vp_modern_queue_vector(mdev, info->vq->index,
+				       VIRTIO_MSI_NO_VECTOR);
+
+	if (!mdev->notify_base)
+		pci_iounmap(mdev->pci_dev,
+			    (void __force __iomem *)info->vq->priv);
+
+	vring_reset_virtqueue(info->vq);
+
+	return 0;
+}
+
+static struct virtqueue *vp_modern_enable_reset_vq(struct virtio_reset_vq *param)
+{
+	struct virtio_pci_device *vp_dev = to_vp_device(param->vdev);
+	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
+	struct virtio_pci_vq_info *info;
+	u16 msix_vec, queue_index;
+	struct virtqueue *vq;
+
+	if (!virtio_has_feature(param->vdev, VIRTIO_F_RING_RESET))
+		return ERR_PTR(-ENOENT);
+
+	queue_index = param->queue_index;
+
+	info = vp_dev->vqs[queue_index];
+
+	if (!info->vq->reset)
+		return ERR_PTR(-EPERM);
+
+	/* check queue reset status */
+	if (vp_modern_get_queue_reset(mdev, queue_index) != 1)
+		return ERR_PTR(-EBUSY);
+
+	vq = vp_setup_vq(param->vdev, queue_index, param->callback, param->name,
+			 param->ctx, info->msix_vector, param->ring_num);
+	if (IS_ERR(vq))
+		return vq;
+
+	vp_modern_set_queue_enable(&vp_dev->mdev, vq->index, true);
+
+	msix_vec = vp_dev->vqs[queue_index]->msix_vector;
+	if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
+		enable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
+
+	return vq;
+}
+
 static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)
 {
 	return vp_modern_config_vector(&vp_dev->mdev, vector);
@@ -284,6 +375,11 @@ static void del_vq(struct virtio_pci_vq_info *info)
 	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
 	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
 
+	if (vq->reset) {
+		vring_del_virtqueue(vq);
+		return;
+	}
+
 	if (vp_dev->msix_enabled)
 		vp_modern_queue_vector(mdev, vq->index,
 				       VIRTIO_MSI_NO_VECTOR);
@@ -403,6 +499,8 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
 	.set_vq_affinity = vp_set_vq_affinity,
 	.get_vq_affinity = vp_get_vq_affinity,
 	.get_shm_region  = vp_get_shm_region,
+	.reset_vq	 = vp_modern_reset_vq,
+	.enable_reset_vq = vp_modern_enable_reset_vq,
 };
 
 static const struct virtio_config_ops virtio_pci_config_ops = {
@@ -421,6 +519,8 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
 	.set_vq_affinity = vp_set_vq_affinity,
 	.get_vq_affinity = vp_get_vq_affinity,
 	.get_shm_region  = vp_get_shm_region,
+	.reset_vq	 = vp_modern_reset_vq,
+	.enable_reset_vq = vp_modern_enable_reset_vq,
 };
 
 /* the PCI probing function */
-- 
2.31.0

Re: [PATCH v3 13/17] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET

[Jason Wang]

在 2022/1/26 下午3:35, Xuan Zhuo 写道:
> This patch implements virtio pci support for QUEUE RESET.
>
> Performing reset on a queue is divided into two steps:
>
> 1. reset_vq: reset one vq
> 2. enable_reset_vq: re-enable the reset queue
>
> In the first step, these tasks will be completed:
>     1. notify the hardware queue to reset
>     2. recycle the buffer from vq
>     3. release the ring of the vq
>
> The process of enable reset vq:
>      vp_modern_enable_reset_vq()
>      vp_enable_reset_vq()
>      __vp_setup_vq()
>      setup_vq()
>      vring_setup_virtqueue()
>
> In this process, we added two parameters, vq and num, and finally passed
> them to vring_setup_virtqueue().  And reuse the original info and vq.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   drivers/virtio/virtio_pci_common.c |  36 +++++++----
>   drivers/virtio/virtio_pci_common.h |   5 ++
>   drivers/virtio/virtio_pci_modern.c | 100 +++++++++++++++++++++++++++++
>   3 files changed, 128 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index c02936d29a31..ad21638fbf66 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -205,23 +205,28 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
>   	return err;
>   }
>   
> -static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
> -				     void (*callback)(struct virtqueue *vq),
> -				     const char *name,
> -				     bool ctx,
> -				     u16 msix_vec, u16 num)
> +struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
> +			      void (*callback)(struct virtqueue *vq),
> +			      const char *name,
> +			      bool ctx,
> +			      u16 msix_vec, u16 num)
>   {
>   	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> -	struct virtio_pci_vq_info *info = kmalloc(sizeof *info, GFP_KERNEL);
> +	struct virtio_pci_vq_info *info;
>   	struct virtqueue *vq;
>   	unsigned long flags;
>   
> -	/* fill out our structure that represents an active queue */
> -	if (!info)
> -		return ERR_PTR(-ENOMEM);
> +	info = vp_dev->vqs[index];
> +	if (!info) {
> +		info = kzalloc(sizeof *info, GFP_KERNEL);
> +
> +		/* fill out our structure that represents an active queue */
> +		if (!info)
> +			return ERR_PTR(-ENOMEM);
> +	}
>   
>   	vq = vp_dev->setup_vq(vp_dev, info, index, callback, name, ctx,
> -			      msix_vec, NULL, num);
> +			      msix_vec, info->vq, num);
>   	if (IS_ERR(vq))
>   		goto out_info;
>   
> @@ -238,6 +243,9 @@ static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
>   	return vq;
>   
>   out_info:
> +	if (info->vq && info->vq->reset)
> +		return vq;
> +
>   	kfree(info);
>   	return vq;
>   }
> @@ -248,9 +256,11 @@ static void vp_del_vq(struct virtqueue *vq)
>   	struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
>   	unsigned long flags;
>   
> -	spin_lock_irqsave(&vp_dev->lock, flags);
> -	list_del(&info->node);
> -	spin_unlock_irqrestore(&vp_dev->lock, flags);
> +	if (!vq->reset) {
> +		spin_lock_irqsave(&vp_dev->lock, flags);
> +		list_del(&info->node);
> +		spin_unlock_irqrestore(&vp_dev->lock, flags);
> +	}
>   
>   	vp_dev->del_vq(info);
>   	kfree(info);
> diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
> index 65db92245e41..c1d15f7c0be4 100644
> --- a/drivers/virtio/virtio_pci_common.h
> +++ b/drivers/virtio/virtio_pci_common.h
> @@ -119,6 +119,11 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>   		struct virtqueue *vqs[], vq_callback_t *callbacks[],
>   		const char * const names[], const bool *ctx,
>   		struct irq_affinity *desc);
> +struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
> +			      void (*callback)(struct virtqueue *vq),
> +			      const char *name,
> +			      bool ctx,
> +			      u16 msix_vec, u16 num);
>   const char *vp_bus_name(struct virtio_device *vdev);
>   
>   /* Setup the affinity for a virtqueue:
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 2ce58de549de..6789411169e4 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -34,6 +34,9 @@ static void vp_transport_features(struct virtio_device *vdev, u64 features)
>   	if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
>   			pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
>   		__virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
> +
> +	if (features & BIT_ULL(VIRTIO_F_RING_RESET))
> +		__virtio_set_bit(vdev, VIRTIO_F_RING_RESET);
>   }
>   
>   /* virtio config->finalize_features() implementation */
> @@ -176,6 +179,94 @@ static void vp_reset(struct virtio_device *vdev)
>   	vp_disable_cbs(vdev);
>   }
>   
> +static int vp_modern_reset_vq(struct virtio_reset_vq *param)
> +{
> +	struct virtio_pci_device *vp_dev = to_vp_device(param->vdev);
> +	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> +	struct virtio_pci_vq_info *info;
> +	u16 msix_vec, queue_index;
> +	unsigned long flags;
> +	void *buf;
> +
> +	if (!virtio_has_feature(param->vdev, VIRTIO_F_RING_RESET))
> +		return -ENOENT;
> +
> +	queue_index = param->queue_index;
> +
> +	vp_modern_set_queue_reset(mdev, queue_index);
> +
> +	/* After write 1 to queue reset, the driver MUST wait for a read of
> +	 * queue reset to return 1.
> +	 */
> +	while (vp_modern_get_queue_reset(mdev, queue_index) != 1)
> +		msleep(1);


Is this better to move this logic into vp_modern_set_queue_reset()?


> +
> +	info = vp_dev->vqs[queue_index];
> +	msix_vec = info->msix_vector;
> +
> +	/* Disable VQ callback. */
> +	if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
> +		disable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));


How about the INTX case where irq is shared? I guess we need to disable 
and enable the irq as well.


> +
> +	while ((buf = virtqueue_detach_unused_buf(info->vq)) != NULL)
> +		param->free_unused_cb(param, buf);


Any reason that we can't leave this logic to driver? (Or is there any 
operations that must be done before the following operations?)


> +
> +	/* delete vq */
> +	spin_lock_irqsave(&vp_dev->lock, flags);
> +	list_del(&info->node);
> +	spin_unlock_irqrestore(&vp_dev->lock, flags);
> +
> +	INIT_LIST_HEAD(&info->node);
> +
> +	if (vp_dev->msix_enabled)
> +		vp_modern_queue_vector(mdev, info->vq->index,
> +				       VIRTIO_MSI_NO_VECTOR);


I wonder if this is a must.


> +
> +	if (!mdev->notify_base)
> +		pci_iounmap(mdev->pci_dev,
> +			    (void __force __iomem *)info->vq->priv);


Is this a must? what happens if we simply don't do this?


> +
> +	vring_reset_virtqueue(info->vq);
> +
> +	return 0;
> +}
> +
> +static struct virtqueue *vp_modern_enable_reset_vq(struct virtio_reset_vq *param)
> +{
> +	struct virtio_pci_device *vp_dev = to_vp_device(param->vdev);
> +	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> +	struct virtio_pci_vq_info *info;
> +	u16 msix_vec, queue_index;
> +	struct virtqueue *vq;
> +
> +	if (!virtio_has_feature(param->vdev, VIRTIO_F_RING_RESET))
> +		return ERR_PTR(-ENOENT);
> +
> +	queue_index = param->queue_index;
> +
> +	info = vp_dev->vqs[queue_index];
> +
> +	if (!info->vq->reset)
> +		return ERR_PTR(-EPERM);
> +
> +	/* check queue reset status */
> +	if (vp_modern_get_queue_reset(mdev, queue_index) != 1)
> +		return ERR_PTR(-EBUSY);
> +
> +	vq = vp_setup_vq(param->vdev, queue_index, param->callback, param->name,
> +			 param->ctx, info->msix_vector, param->ring_num);
> +	if (IS_ERR(vq))
> +		return vq;
> +
> +	vp_modern_set_queue_enable(&vp_dev->mdev, vq->index, true);
> +
> +	msix_vec = vp_dev->vqs[queue_index]->msix_vector;
> +	if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
> +		enable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));


How about the INT-X case?

Thanks


> +
> +	return vq;
> +}
> +
>   static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)
>   {
>   	return vp_modern_config_vector(&vp_dev->mdev, vector);
> @@ -284,6 +375,11 @@ static void del_vq(struct virtio_pci_vq_info *info)
>   	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
>   	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
>   
> +	if (vq->reset) {
> +		vring_del_virtqueue(vq);
> +		return;
> +	}
> +
>   	if (vp_dev->msix_enabled)
>   		vp_modern_queue_vector(mdev, vq->index,
>   				       VIRTIO_MSI_NO_VECTOR);
> @@ -403,6 +499,8 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
>   	.set_vq_affinity = vp_set_vq_affinity,
>   	.get_vq_affinity = vp_get_vq_affinity,
>   	.get_shm_region  = vp_get_shm_region,
> +	.reset_vq	 = vp_modern_reset_vq,
> +	.enable_reset_vq = vp_modern_enable_reset_vq,
>   };
>   
>   static const struct virtio_config_ops virtio_pci_config_ops = {
> @@ -421,6 +519,8 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
>   	.set_vq_affinity = vp_set_vq_affinity,
>   	.get_vq_affinity = vp_get_vq_affinity,
>   	.get_shm_region  = vp_get_shm_region,
> +	.reset_vq	 = vp_modern_reset_vq,
> +	.enable_reset_vq = vp_modern_enable_reset_vq,
>   };
>   
>   /* the PCI probing function */

Re: [PATCH v3 13/17] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET

[Jason Wang]

On Mon, Feb 7, 2022 at 4:19 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Mon, 7 Feb 2022 14:57:13 +0800, Jason Wang <jasowang@redhat.com> wrote:
> >
> > 在 2022/1/26 下午3:35, Xuan Zhuo 写道:
> > > This patch implements virtio pci support for QUEUE RESET.
> > >
> > > Performing reset on a queue is divided into two steps:
> > >
> > > 1. reset_vq: reset one vq
> > > 2. enable_reset_vq: re-enable the reset queue
> > >
> > > In the first step, these tasks will be completed:
> > >     1. notify the hardware queue to reset
> > >     2. recycle the buffer from vq
> > >     3. release the ring of the vq
> > >
> > > The process of enable reset vq:
> > >      vp_modern_enable_reset_vq()
> > >      vp_enable_reset_vq()
> > >      __vp_setup_vq()
> > >      setup_vq()
> > >      vring_setup_virtqueue()
> > >
> > > In this process, we added two parameters, vq and num, and finally passed
> > > them to vring_setup_virtqueue().  And reuse the original info and vq.
> > >
> > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > ---
> > >   drivers/virtio/virtio_pci_common.c |  36 +++++++----
> > >   drivers/virtio/virtio_pci_common.h |   5 ++
> > >   drivers/virtio/virtio_pci_modern.c | 100 +++++++++++++++++++++++++++++
> > >   3 files changed, 128 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > index c02936d29a31..ad21638fbf66 100644
> > > --- a/drivers/virtio/virtio_pci_common.c
> > > +++ b/drivers/virtio/virtio_pci_common.c
> > > @@ -205,23 +205,28 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
> > >     return err;
> > >   }
> > >
> > > -static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
> > > -                                void (*callback)(struct virtqueue *vq),
> > > -                                const char *name,
> > > -                                bool ctx,
> > > -                                u16 msix_vec, u16 num)
> > > +struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
> > > +                         void (*callback)(struct virtqueue *vq),
> > > +                         const char *name,
> > > +                         bool ctx,
> > > +                         u16 msix_vec, u16 num)
> > >   {
> > >     struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > -   struct virtio_pci_vq_info *info = kmalloc(sizeof *info, GFP_KERNEL);
> > > +   struct virtio_pci_vq_info *info;
> > >     struct virtqueue *vq;
> > >     unsigned long flags;
> > >
> > > -   /* fill out our structure that represents an active queue */
> > > -   if (!info)
> > > -           return ERR_PTR(-ENOMEM);
> > > +   info = vp_dev->vqs[index];
> > > +   if (!info) {
> > > +           info = kzalloc(sizeof *info, GFP_KERNEL);
> > > +
> > > +           /* fill out our structure that represents an active queue */
> > > +           if (!info)
> > > +                   return ERR_PTR(-ENOMEM);
> > > +   }
> > >
> > >     vq = vp_dev->setup_vq(vp_dev, info, index, callback, name, ctx,
> > > -                         msix_vec, NULL, num);
> > > +                         msix_vec, info->vq, num);
> > >     if (IS_ERR(vq))
> > >             goto out_info;
> > >
> > > @@ -238,6 +243,9 @@ static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
> > >     return vq;
> > >
> > >   out_info:
> > > +   if (info->vq && info->vq->reset)
> > > +           return vq;
> > > +
> > >     kfree(info);
> > >     return vq;
> > >   }
> > > @@ -248,9 +256,11 @@ static void vp_del_vq(struct virtqueue *vq)
> > >     struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
> > >     unsigned long flags;
> > >
> > > -   spin_lock_irqsave(&vp_dev->lock, flags);
> > > -   list_del(&info->node);
> > > -   spin_unlock_irqrestore(&vp_dev->lock, flags);
> > > +   if (!vq->reset) {
> > > +           spin_lock_irqsave(&vp_dev->lock, flags);
> > > +           list_del(&info->node);
> > > +           spin_unlock_irqrestore(&vp_dev->lock, flags);
> > > +   }
> > >
> > >     vp_dev->del_vq(info);
> > >     kfree(info);
> > > diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
> > > index 65db92245e41..c1d15f7c0be4 100644
> > > --- a/drivers/virtio/virtio_pci_common.h
> > > +++ b/drivers/virtio/virtio_pci_common.h
> > > @@ -119,6 +119,11 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> > >             struct virtqueue *vqs[], vq_callback_t *callbacks[],
> > >             const char * const names[], const bool *ctx,
> > >             struct irq_affinity *desc);
> > > +struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
> > > +                         void (*callback)(struct virtqueue *vq),
> > > +                         const char *name,
> > > +                         bool ctx,
> > > +                         u16 msix_vec, u16 num);
> > >   const char *vp_bus_name(struct virtio_device *vdev);
> > >
> > >   /* Setup the affinity for a virtqueue:
> > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > > index 2ce58de549de..6789411169e4 100644
> > > --- a/drivers/virtio/virtio_pci_modern.c
> > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > @@ -34,6 +34,9 @@ static void vp_transport_features(struct virtio_device *vdev, u64 features)
> > >     if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
> > >                     pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
> > >             __virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
> > > +
> > > +   if (features & BIT_ULL(VIRTIO_F_RING_RESET))
> > > +           __virtio_set_bit(vdev, VIRTIO_F_RING_RESET);
> > >   }
> > >
> > >   /* virtio config->finalize_features() implementation */
> > > @@ -176,6 +179,94 @@ static void vp_reset(struct virtio_device *vdev)
> > >     vp_disable_cbs(vdev);
> > >   }
> > >
> > > +static int vp_modern_reset_vq(struct virtio_reset_vq *param)
> > > +{
> > > +   struct virtio_pci_device *vp_dev = to_vp_device(param->vdev);
> > > +   struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> > > +   struct virtio_pci_vq_info *info;
> > > +   u16 msix_vec, queue_index;
> > > +   unsigned long flags;
> > > +   void *buf;
> > > +
> > > +   if (!virtio_has_feature(param->vdev, VIRTIO_F_RING_RESET))
> > > +           return -ENOENT;
> > > +
> > > +   queue_index = param->queue_index;
> > > +
> > > +   vp_modern_set_queue_reset(mdev, queue_index);
> > > +
> > > +   /* After write 1 to queue reset, the driver MUST wait for a read of
> > > +    * queue reset to return 1.
> > > +    */
> > > +   while (vp_modern_get_queue_reset(mdev, queue_index) != 1)
> > > +           msleep(1);
> >
> >
> > Is this better to move this logic into vp_modern_set_queue_reset()?
> >
>
> OK.
>
> >
> > > +
> > > +   info = vp_dev->vqs[queue_index];
> > > +   msix_vec = info->msix_vector;
> > > +
> > > +   /* Disable VQ callback. */
> > > +   if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
> > > +           disable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
> >
> >
> > How about the INTX case where irq is shared? I guess we need to disable
> > and enable the irq as well.
>
> For the INTX scenario, I don't think we need to disable/enable the irq. But I do
> have a mistake, I should put the following list_del(&info->node) here, so that
> when the irq comes, it will no longer operate this vq.
>
> In fact, for no INTX case, the disable_irq here is not necessary, according to
> the requirements of the spec, after reset, the device should not generate irq
> anymore.

I may miss something but I don't think INTX differs from MSI from the
vq handler perspective.

> Here just to prevent accidents.

The problem is that if you don't disable/sync IRQ there could be a
race between the vq irq handler and the virtqueue_detach_unused_buf()?

>
> >
> >
> > > +
> > > +   while ((buf = virtqueue_detach_unused_buf(info->vq)) != NULL)
> > > +           param->free_unused_cb(param, buf);
> >
> >
> > Any reason that we can't leave this logic to driver? (Or is there any
> > operations that must be done before the following operations?)
>
> As you commented before, we merged free unused buf and reset queue.
>
> I think it's a good note, otherwise, we're going to
>
>         1. reset vq
>         2. free unused(by driver)
>         3. free ring of vq

Rethink about this, I'd prefer to leave it to the driver for consistency.

E.g the virtqueue_detach_unused_buf() is called by each driver nowdays.

>
>
> >
> >
> > > +
> > > +   /* delete vq */
> > > +   spin_lock_irqsave(&vp_dev->lock, flags);
> > > +   list_del(&info->node);
> > > +   spin_unlock_irqrestore(&vp_dev->lock, flags);
> > > +
> > > +   INIT_LIST_HEAD(&info->node);
> > > +
> > > +   if (vp_dev->msix_enabled)
> > > +           vp_modern_queue_vector(mdev, info->vq->index,
> > > +                                  VIRTIO_MSI_NO_VECTOR);
> >
> >
> > I wonder if this is a must.
> >
> >
> > > +
> > > +   if (!mdev->notify_base)
> > > +           pci_iounmap(mdev->pci_dev,
> > > +                       (void __force __iomem *)info->vq->priv);
> >
> >
> > Is this a must? what happens if we simply don't do this?
> >
>
> The purpose of these two operations is mainly to be symmetrical with del_vq().

This is another question I want to ask. Any reason for calling
del_vq()? Is it because you need to exclude a vq from the vq handler?

For any case, the MSI and notification stuff seems unnecessary.

Thanks

>
>
> >
> > > +
> > > +   vring_reset_virtqueue(info->vq);
> > > +
> > > +   return 0;
> > > +}
> > > +
> > > +static struct virtqueue *vp_modern_enable_reset_vq(struct virtio_reset_vq *param)
> > > +{
> > > +   struct virtio_pci_device *vp_dev = to_vp_device(param->vdev);
> > > +   struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> > > +   struct virtio_pci_vq_info *info;
> > > +   u16 msix_vec, queue_index;
> > > +   struct virtqueue *vq;
> > > +
> > > +   if (!virtio_has_feature(param->vdev, VIRTIO_F_RING_RESET))
> > > +           return ERR_PTR(-ENOENT);
> > > +
> > > +   queue_index = param->queue_index;
> > > +
> > > +   info = vp_dev->vqs[queue_index];
> > > +
> > > +   if (!info->vq->reset)
> > > +           return ERR_PTR(-EPERM);
> > > +
> > > +   /* check queue reset status */
> > > +   if (vp_modern_get_queue_reset(mdev, queue_index) != 1)
> > > +           return ERR_PTR(-EBUSY);
> > > +
> > > +   vq = vp_setup_vq(param->vdev, queue_index, param->callback, param->name,
> > > +                    param->ctx, info->msix_vector, param->ring_num);
> > > +   if (IS_ERR(vq))
> > > +           return vq;
> > > +
> > > +   vp_modern_set_queue_enable(&vp_dev->mdev, vq->index, true);
> > > +
> > > +   msix_vec = vp_dev->vqs[queue_index]->msix_vector;
> > > +   if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
> > > +           enable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
> >
> >
> > How about the INT-X case?
>
> Explained above.
>
> Thanks.
>
> >
> > Thanks
> >
> >
> > > +
> > > +   return vq;
> > > +}
> > > +
> > >   static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)
> > >   {
> > >     return vp_modern_config_vector(&vp_dev->mdev, vector);
> > > @@ -284,6 +375,11 @@ static void del_vq(struct virtio_pci_vq_info *info)
> > >     struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> > >     struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> > >
> > > +   if (vq->reset) {
> > > +           vring_del_virtqueue(vq);
> > > +           return;
> > > +   }
> > > +
> > >     if (vp_dev->msix_enabled)
> > >             vp_modern_queue_vector(mdev, vq->index,
> > >                                    VIRTIO_MSI_NO_VECTOR);
> > > @@ -403,6 +499,8 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
> > >     .set_vq_affinity = vp_set_vq_affinity,
> > >     .get_vq_affinity = vp_get_vq_affinity,
> > >     .get_shm_region  = vp_get_shm_region,
> > > +   .reset_vq        = vp_modern_reset_vq,
> > > +   .enable_reset_vq = vp_modern_enable_reset_vq,
> > >   };
> > >
> > >   static const struct virtio_config_ops virtio_pci_config_ops = {
> > > @@ -421,6 +519,8 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
> > >     .set_vq_affinity = vp_set_vq_affinity,
> > >     .get_vq_affinity = vp_get_vq_affinity,
> > >     .get_shm_region  = vp_get_shm_region,
> > > +   .reset_vq        = vp_modern_reset_vq,
> > > +   .enable_reset_vq = vp_modern_enable_reset_vq,
> > >   };
> > >
> > >   /* the PCI probing function */
> >
>

Re: [PATCH v3 13/17] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET

[xuanzhuo]

On 2022/2/8 上午10:55, Jason Wang wrote:
> On Mon, Feb 7, 2022 at 4:19 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>> On Mon, 7 Feb 2022 14:57:13 +0800, Jason Wang <jasowang@redhat.com> wrote:
>>> 在 2022/1/26 下午3:35, Xuan Zhuo 写道:
>>>> This patch implements virtio pci support for QUEUE RESET.
>>>>
>>>> Performing reset on a queue is divided into two steps:
>>>>
>>>> 1. reset_vq: reset one vq
>>>> 2. enable_reset_vq: re-enable the reset queue
>>>>
>>>> In the first step, these tasks will be completed:
>>>>      1. notify the hardware queue to reset
>>>>      2. recycle the buffer from vq
>>>>      3. release the ring of the vq
>>>>
>>>> The process of enable reset vq:
>>>>       vp_modern_enable_reset_vq()
>>>>       vp_enable_reset_vq()
>>>>       __vp_setup_vq()
>>>>       setup_vq()
>>>>       vring_setup_virtqueue()
>>>>
>>>> In this process, we added two parameters, vq and num, and finally passed
>>>> them to vring_setup_virtqueue().  And reuse the original info and vq.
>>>>
>>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>> ---
>>>>    drivers/virtio/virtio_pci_common.c |  36 +++++++----
>>>>    drivers/virtio/virtio_pci_common.h |   5 ++
>>>>    drivers/virtio/virtio_pci_modern.c | 100 +++++++++++++++++++++++++++++
>>>>    3 files changed, 128 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
>>>> index c02936d29a31..ad21638fbf66 100644
>>>> --- a/drivers/virtio/virtio_pci_common.c
>>>> +++ b/drivers/virtio/virtio_pci_common.c
>>>> @@ -205,23 +205,28 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
>>>>      return err;
>>>>    }
>>>>
>>>> -static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
>>>> -                                void (*callback)(struct virtqueue *vq),
>>>> -                                const char *name,
>>>> -                                bool ctx,
>>>> -                                u16 msix_vec, u16 num)
>>>> +struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
>>>> +                         void (*callback)(struct virtqueue *vq),
>>>> +                         const char *name,
>>>> +                         bool ctx,
>>>> +                         u16 msix_vec, u16 num)
>>>>    {
>>>>      struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>>>> -   struct virtio_pci_vq_info *info = kmalloc(sizeof *info, GFP_KERNEL);
>>>> +   struct virtio_pci_vq_info *info;
>>>>      struct virtqueue *vq;
>>>>      unsigned long flags;
>>>>
>>>> -   /* fill out our structure that represents an active queue */
>>>> -   if (!info)
>>>> -           return ERR_PTR(-ENOMEM);
>>>> +   info = vp_dev->vqs[index];
>>>> +   if (!info) {
>>>> +           info = kzalloc(sizeof *info, GFP_KERNEL);
>>>> +
>>>> +           /* fill out our structure that represents an active queue */
>>>> +           if (!info)
>>>> +                   return ERR_PTR(-ENOMEM);
>>>> +   }
>>>>
>>>>      vq = vp_dev->setup_vq(vp_dev, info, index, callback, name, ctx,
>>>> -                         msix_vec, NULL, num);
>>>> +                         msix_vec, info->vq, num);
>>>>      if (IS_ERR(vq))
>>>>              goto out_info;
>>>>
>>>> @@ -238,6 +243,9 @@ static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
>>>>      return vq;
>>>>
>>>>    out_info:
>>>> +   if (info->vq && info->vq->reset)
>>>> +           return vq;
>>>> +
>>>>      kfree(info);
>>>>      return vq;
>>>>    }
>>>> @@ -248,9 +256,11 @@ static void vp_del_vq(struct virtqueue *vq)
>>>>      struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
>>>>      unsigned long flags;
>>>>
>>>> -   spin_lock_irqsave(&vp_dev->lock, flags);
>>>> -   list_del(&info->node);
>>>> -   spin_unlock_irqrestore(&vp_dev->lock, flags);
>>>> +   if (!vq->reset) {
>>>> +           spin_lock_irqsave(&vp_dev->lock, flags);
>>>> +           list_del(&info->node);
>>>> +           spin_unlock_irqrestore(&vp_dev->lock, flags);
>>>> +   }
>>>>
>>>>      vp_dev->del_vq(info);
>>>>      kfree(info);
>>>> diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
>>>> index 65db92245e41..c1d15f7c0be4 100644
>>>> --- a/drivers/virtio/virtio_pci_common.h
>>>> +++ b/drivers/virtio/virtio_pci_common.h
>>>> @@ -119,6 +119,11 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>>>>              struct virtqueue *vqs[], vq_callback_t *callbacks[],
>>>>              const char * const names[], const bool *ctx,
>>>>              struct irq_affinity *desc);
>>>> +struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
>>>> +                         void (*callback)(struct virtqueue *vq),
>>>> +                         const char *name,
>>>> +                         bool ctx,
>>>> +                         u16 msix_vec, u16 num);
>>>>    const char *vp_bus_name(struct virtio_device *vdev);
>>>>
>>>>    /* Setup the affinity for a virtqueue:
>>>> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
>>>> index 2ce58de549de..6789411169e4 100644
>>>> --- a/drivers/virtio/virtio_pci_modern.c
>>>> +++ b/drivers/virtio/virtio_pci_modern.c
>>>> @@ -34,6 +34,9 @@ static void vp_transport_features(struct virtio_device *vdev, u64 features)
>>>>      if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
>>>>                      pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
>>>>              __virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
>>>> +
>>>> +   if (features & BIT_ULL(VIRTIO_F_RING_RESET))
>>>> +           __virtio_set_bit(vdev, VIRTIO_F_RING_RESET);
>>>>    }
>>>>
>>>>    /* virtio config->finalize_features() implementation */
>>>> @@ -176,6 +179,94 @@ static void vp_reset(struct virtio_device *vdev)
>>>>      vp_disable_cbs(vdev);
>>>>    }
>>>>
>>>> +static int vp_modern_reset_vq(struct virtio_reset_vq *param)
>>>> +{
>>>> +   struct virtio_pci_device *vp_dev = to_vp_device(param->vdev);
>>>> +   struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
>>>> +   struct virtio_pci_vq_info *info;
>>>> +   u16 msix_vec, queue_index;
>>>> +   unsigned long flags;
>>>> +   void *buf;
>>>> +
>>>> +   if (!virtio_has_feature(param->vdev, VIRTIO_F_RING_RESET))
>>>> +           return -ENOENT;
>>>> +
>>>> +   queue_index = param->queue_index;
>>>> +
>>>> +   vp_modern_set_queue_reset(mdev, queue_index);
>>>> +
>>>> +   /* After write 1 to queue reset, the driver MUST wait for a read of
>>>> +    * queue reset to return 1.
>>>> +    */
>>>> +   while (vp_modern_get_queue_reset(mdev, queue_index) != 1)
>>>> +           msleep(1);
>>>
>>> Is this better to move this logic into vp_modern_set_queue_reset()?
>>>
>> OK.
>>
>>>> +
>>>> +   info = vp_dev->vqs[queue_index];
>>>> +   msix_vec = info->msix_vector;
>>>> +
>>>> +   /* Disable VQ callback. */
>>>> +   if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
>>>> +           disable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
>>>
>>> How about the INTX case where irq is shared? I guess we need to disable
>>> and enable the irq as well.
>> For the INTX scenario, I don't think we need to disable/enable the irq. But I do
>> have a mistake, I should put the following list_del(&info->node) here, so that
>> when the irq comes, it will no longer operate this vq.
>>
>> In fact, for no INTX case, the disable_irq here is not necessary, according to
>> the requirements of the spec, after reset, the device should not generate irq
>> anymore.
> I may miss something but I don't think INTX differs from MSI from the
> vq handler perspective.
>
>> Here just to prevent accidents.
> The problem is that if you don't disable/sync IRQ there could be a
> race between the vq irq handler and the virtqueue_detach_unused_buf()?

I understand that under INTX, the irq handler (vp_vring_interrupt) will 
check
the list vp_dev->virtqueues to call the vq callback. Then if we have 
deleted it
from vp_dev->virtqueues, then there is no need to disable irq. And this 
process
is locked by vp_dev->lock.
>>>
>>>> +
>>>> +   while ((buf = virtqueue_detach_unused_buf(info->vq)) != NULL)
>>>> +           param->free_unused_cb(param, buf);
>>>
>>> Any reason that we can't leave this logic to driver? (Or is there any
>>> operations that must be done before the following operations?)
>> As you commented before, we merged free unused buf and reset queue.
>>
>> I think it's a good note, otherwise, we're going to
>>
>>          1. reset vq
>>          2. free unused(by driver)
>>          3. free ring of vq
> Rethink about this, I'd prefer to leave it to the driver for consistency.
>
> E.g the virtqueue_detach_unused_buf() is called by each driver nowdays.
>
OK
>>
>>>
>>>> +
>>>> +   /* delete vq */
>>>> +   spin_lock_irqsave(&vp_dev->lock, flags);
>>>> +   list_del(&info->node);
>>>> +   spin_unlock_irqrestore(&vp_dev->lock, flags);
>>>> +
>>>> +   INIT_LIST_HEAD(&info->node);
>>>> +
>>>> +   if (vp_dev->msix_enabled)
>>>> +           vp_modern_queue_vector(mdev, info->vq->index,
>>>> +                                  VIRTIO_MSI_NO_VECTOR);
>>>
>>> I wonder if this is a must.
>>>
>>>
>>>> +
>>>> +   if (!mdev->notify_base)
>>>> +           pci_iounmap(mdev->pci_dev,
>>>> +                       (void __force __iomem *)info->vq->priv);
>>>
>>> Is this a must? what happens if we simply don't do this?
>>>
>> The purpose of these two operations is mainly to be symmetrical with del_vq().
> This is another question I want to ask. Any reason for calling
> del_vq()? Is it because you need to exclude a vq from the vq handler?
I didn't actually call del_vq(), but I think the process is similar to 
it, so
I'd like to be symmetric with del_vq() in implementation.

The separation from the vq handler is achieved by the following code:

   spin_lock_irqsave(&vp_dev->lock, flags);
   list_del(&info->node);
   spin_unlock_irqrestore(&vp_dev->lock, flags);
> For any case, the MSI and notification stuff seems unnecessary.

I will remove these in next version.

Thanks.
> Thanks
>
>>
>>>> +
>>>> +   vring_reset_virtqueue(info->vq);
>>>> +
>>>> +   return 0;
>>>> +}
>>>> +
>>>> +static struct virtqueue *vp_modern_enable_reset_vq(struct virtio_reset_vq *param)
>>>> +{
>>>> +   struct virtio_pci_device *vp_dev = to_vp_device(param->vdev);
>>>> +   struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
>>>> +   struct virtio_pci_vq_info *info;
>>>> +   u16 msix_vec, queue_index;
>>>> +   struct virtqueue *vq;
>>>> +
>>>> +   if (!virtio_has_feature(param->vdev, VIRTIO_F_RING_RESET))
>>>> +           return ERR_PTR(-ENOENT);
>>>> +
>>>> +   queue_index = param->queue_index;
>>>> +
>>>> +   info = vp_dev->vqs[queue_index];
>>>> +
>>>> +   if (!info->vq->reset)
>>>> +           return ERR_PTR(-EPERM);
>>>> +
>>>> +   /* check queue reset status */
>>>> +   if (vp_modern_get_queue_reset(mdev, queue_index) != 1)
>>>> +           return ERR_PTR(-EBUSY);
>>>> +
>>>> +   vq = vp_setup_vq(param->vdev, queue_index, param->callback, param->name,
>>>> +                    param->ctx, info->msix_vector, param->ring_num);
>>>> +   if (IS_ERR(vq))
>>>> +           return vq;
>>>> +
>>>> +   vp_modern_set_queue_enable(&vp_dev->mdev, vq->index, true);
>>>> +
>>>> +   msix_vec = vp_dev->vqs[queue_index]->msix_vector;
>>>> +   if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
>>>> +           enable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
>>>
>>> How about the INT-X case?
>> Explained above.
>>
>> Thanks.
>>
>>> Thanks
>>>
>>>
>>>> +
>>>> +   return vq;
>>>> +}
>>>> +
>>>>    static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)
>>>>    {
>>>>      return vp_modern_config_vector(&vp_dev->mdev, vector);
>>>> @@ -284,6 +375,11 @@ static void del_vq(struct virtio_pci_vq_info *info)
>>>>      struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
>>>>      struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
>>>>
>>>> +   if (vq->reset) {
>>>> +           vring_del_virtqueue(vq);
>>>> +           return;
>>>> +   }
>>>> +
>>>>      if (vp_dev->msix_enabled)
>>>>              vp_modern_queue_vector(mdev, vq->index,
>>>>                                     VIRTIO_MSI_NO_VECTOR);
>>>> @@ -403,6 +499,8 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
>>>>      .set_vq_affinity = vp_set_vq_affinity,
>>>>      .get_vq_affinity = vp_get_vq_affinity,
>>>>      .get_shm_region  = vp_get_shm_region,
>>>> +   .reset_vq        = vp_modern_reset_vq,
>>>> +   .enable_reset_vq = vp_modern_enable_reset_vq,
>>>>    };
>>>>
>>>>    static const struct virtio_config_ops virtio_pci_config_ops = {
>>>> @@ -421,6 +519,8 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
>>>>      .set_vq_affinity = vp_set_vq_affinity,
>>>>      .get_vq_affinity = vp_get_vq_affinity,
>>>>      .get_shm_region  = vp_get_shm_region,
>>>> +   .reset_vq        = vp_modern_reset_vq,
>>>> +   .enable_reset_vq = vp_modern_enable_reset_vq,
>>>>    };
>>>>
>>>>    /* the PCI probing function */

Re: [PATCH v3 13/17] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET

[Jason Wang]

On Tue, Feb 8, 2022 at 3:44 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Tue, 8 Feb 2022 10:55:37 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Mon, Feb 7, 2022 at 4:19 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >
> > > On Mon, 7 Feb 2022 14:57:13 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > 在 2022/1/26 下午3:35, Xuan Zhuo 写道:
> > > > > This patch implements virtio pci support for QUEUE RESET.
> > > > >
> > > > > Performing reset on a queue is divided into two steps:
> > > > >
> > > > > 1. reset_vq: reset one vq
> > > > > 2. enable_reset_vq: re-enable the reset queue
> > > > >
> > > > > In the first step, these tasks will be completed:
> > > > >     1. notify the hardware queue to reset
> > > > >     2. recycle the buffer from vq
> > > > >     3. release the ring of the vq
> > > > >
> > > > > The process of enable reset vq:
> > > > >      vp_modern_enable_reset_vq()
> > > > >      vp_enable_reset_vq()
> > > > >      __vp_setup_vq()
> > > > >      setup_vq()
> > > > >      vring_setup_virtqueue()
> > > > >
> > > > > In this process, we added two parameters, vq and num, and finally passed
> > > > > them to vring_setup_virtqueue().  And reuse the original info and vq.
> > > > >
> > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > ---
> > > > >   drivers/virtio/virtio_pci_common.c |  36 +++++++----
> > > > >   drivers/virtio/virtio_pci_common.h |   5 ++
> > > > >   drivers/virtio/virtio_pci_modern.c | 100 +++++++++++++++++++++++++++++
> > > > >   3 files changed, 128 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > > index c02936d29a31..ad21638fbf66 100644
> > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > @@ -205,23 +205,28 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
> > > > >     return err;
> > > > >   }
> > > > >
> > > > > -static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
> > > > > -                                void (*callback)(struct virtqueue *vq),
> > > > > -                                const char *name,
> > > > > -                                bool ctx,
> > > > > -                                u16 msix_vec, u16 num)
> > > > > +struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
> > > > > +                         void (*callback)(struct virtqueue *vq),
> > > > > +                         const char *name,
> > > > > +                         bool ctx,
> > > > > +                         u16 msix_vec, u16 num)
> > > > >   {
> > > > >     struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > -   struct virtio_pci_vq_info *info = kmalloc(sizeof *info, GFP_KERNEL);
> > > > > +   struct virtio_pci_vq_info *info;
> > > > >     struct virtqueue *vq;
> > > > >     unsigned long flags;
> > > > >
> > > > > -   /* fill out our structure that represents an active queue */
> > > > > -   if (!info)
> > > > > -           return ERR_PTR(-ENOMEM);
> > > > > +   info = vp_dev->vqs[index];
> > > > > +   if (!info) {
> > > > > +           info = kzalloc(sizeof *info, GFP_KERNEL);
> > > > > +
> > > > > +           /* fill out our structure that represents an active queue */
> > > > > +           if (!info)
> > > > > +                   return ERR_PTR(-ENOMEM);
> > > > > +   }
> > > > >
> > > > >     vq = vp_dev->setup_vq(vp_dev, info, index, callback, name, ctx,
> > > > > -                         msix_vec, NULL, num);
> > > > > +                         msix_vec, info->vq, num);
> > > > >     if (IS_ERR(vq))
> > > > >             goto out_info;
> > > > >
> > > > > @@ -238,6 +243,9 @@ static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
> > > > >     return vq;
> > > > >
> > > > >   out_info:
> > > > > +   if (info->vq && info->vq->reset)
> > > > > +           return vq;
> > > > > +
> > > > >     kfree(info);
> > > > >     return vq;
> > > > >   }
> > > > > @@ -248,9 +256,11 @@ static void vp_del_vq(struct virtqueue *vq)
> > > > >     struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
> > > > >     unsigned long flags;
> > > > >
> > > > > -   spin_lock_irqsave(&vp_dev->lock, flags);
> > > > > -   list_del(&info->node);
> > > > > -   spin_unlock_irqrestore(&vp_dev->lock, flags);
> > > > > +   if (!vq->reset) {
> > > > > +           spin_lock_irqsave(&vp_dev->lock, flags);
> > > > > +           list_del(&info->node);
> > > > > +           spin_unlock_irqrestore(&vp_dev->lock, flags);
> > > > > +   }
> > > > >
> > > > >     vp_dev->del_vq(info);
> > > > >     kfree(info);
> > > > > diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
> > > > > index 65db92245e41..c1d15f7c0be4 100644
> > > > > --- a/drivers/virtio/virtio_pci_common.h
> > > > > +++ b/drivers/virtio/virtio_pci_common.h
> > > > > @@ -119,6 +119,11 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> > > > >             struct virtqueue *vqs[], vq_callback_t *callbacks[],
> > > > >             const char * const names[], const bool *ctx,
> > > > >             struct irq_affinity *desc);
> > > > > +struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
> > > > > +                         void (*callback)(struct virtqueue *vq),
> > > > > +                         const char *name,
> > > > > +                         bool ctx,
> > > > > +                         u16 msix_vec, u16 num);
> > > > >   const char *vp_bus_name(struct virtio_device *vdev);
> > > > >
> > > > >   /* Setup the affinity for a virtqueue:
> > > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > > > > index 2ce58de549de..6789411169e4 100644
> > > > > --- a/drivers/virtio/virtio_pci_modern.c
> > > > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > > > @@ -34,6 +34,9 @@ static void vp_transport_features(struct virtio_device *vdev, u64 features)
> > > > >     if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
> > > > >                     pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
> > > > >             __virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
> > > > > +
> > > > > +   if (features & BIT_ULL(VIRTIO_F_RING_RESET))
> > > > > +           __virtio_set_bit(vdev, VIRTIO_F_RING_RESET);
> > > > >   }
> > > > >
> > > > >   /* virtio config->finalize_features() implementation */
> > > > > @@ -176,6 +179,94 @@ static void vp_reset(struct virtio_device *vdev)
> > > > >     vp_disable_cbs(vdev);
> > > > >   }
> > > > >
> > > > > +static int vp_modern_reset_vq(struct virtio_reset_vq *param)
> > > > > +{
> > > > > +   struct virtio_pci_device *vp_dev = to_vp_device(param->vdev);
> > > > > +   struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> > > > > +   struct virtio_pci_vq_info *info;
> > > > > +   u16 msix_vec, queue_index;
> > > > > +   unsigned long flags;
> > > > > +   void *buf;
> > > > > +
> > > > > +   if (!virtio_has_feature(param->vdev, VIRTIO_F_RING_RESET))
> > > > > +           return -ENOENT;
> > > > > +
> > > > > +   queue_index = param->queue_index;
> > > > > +
> > > > > +   vp_modern_set_queue_reset(mdev, queue_index);
> > > > > +
> > > > > +   /* After write 1 to queue reset, the driver MUST wait for a read of
> > > > > +    * queue reset to return 1.
> > > > > +    */
> > > > > +   while (vp_modern_get_queue_reset(mdev, queue_index) != 1)
> > > > > +           msleep(1);
> > > >
> > > >
> > > > Is this better to move this logic into vp_modern_set_queue_reset()?
> > > >
> > >
> > > OK.
> > >
> > > >
> > > > > +
> > > > > +   info = vp_dev->vqs[queue_index];
> > > > > +   msix_vec = info->msix_vector;
> > > > > +
> > > > > +   /* Disable VQ callback. */
> > > > > +   if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
> > > > > +           disable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
> > > >
> > > >
> > > > How about the INTX case where irq is shared? I guess we need to disable
> > > > and enable the irq as well.
> > >
> > > For the INTX scenario, I don't think we need to disable/enable the irq. But I do
> > > have a mistake, I should put the following list_del(&info->node) here, so that
> > > when the irq comes, it will no longer operate this vq.
> > >
> > > In fact, for no INTX case, the disable_irq here is not necessary, according to
> > > the requirements of the spec, after reset, the device should not generate irq
> > > anymore.
> >
> > I may miss something but I don't think INTX differs from MSI from the
> > vq handler perspective.
> >
> > > Here just to prevent accidents.
> >
> > The problem is that if you don't disable/sync IRQ there could be a
> > race between the vq irq handler and the virtqueue_detach_unused_buf()?
> >
> > >
> > > >
> > > >
> > > > > +
> > > > > +   while ((buf = virtqueue_detach_unused_buf(info->vq)) != NULL)
> > > > > +           param->free_unused_cb(param, buf);
> > > >
> > > >
> > > > Any reason that we can't leave this logic to driver? (Or is there any
> > > > operations that must be done before the following operations?)
> > >
> > > As you commented before, we merged free unused buf and reset queue.
> > >
> > > I think it's a good note, otherwise, we're going to
> > >
> > >         1. reset vq
> > >         2. free unused(by driver)
> > >         3. free ring of vq
> >
> > Rethink about this, I'd prefer to leave it to the driver for consistency.
> >
> > E.g the virtqueue_detach_unused_buf() is called by each driver nowdays.
>
> In this case, go back to my previous design and add three helpers:
>
>         int (*reset_vq)();
>         int (*free_reset_vq)();

So this is only needed if there are any transport specific operations.
But I don't see there's any.

Thanks

>         int (*enable_reset_vq)();
>
> Thanks.
>
> >
> > >
> > >
> > > >
> > > >
> > > > > +
> > > > > +   /* delete vq */
> > > > > +   spin_lock_irqsave(&vp_dev->lock, flags);
> > > > > +   list_del(&info->node);
> > > > > +   spin_unlock_irqrestore(&vp_dev->lock, flags);
> > > > > +
> > > > > +   INIT_LIST_HEAD(&info->node);
> > > > > +
> > > > > +   if (vp_dev->msix_enabled)
> > > > > +           vp_modern_queue_vector(mdev, info->vq->index,
> > > > > +                                  VIRTIO_MSI_NO_VECTOR);
> > > >
> > > >
> > > > I wonder if this is a must.
> > > >
> > > >
> > > > > +
> > > > > +   if (!mdev->notify_base)
> > > > > +           pci_iounmap(mdev->pci_dev,
> > > > > +                       (void __force __iomem *)info->vq->priv);
> > > >
> > > >
> > > > Is this a must? what happens if we simply don't do this?
> > > >
> > >
> > > The purpose of these two operations is mainly to be symmetrical with del_vq().
> >
> > This is another question I want to ask. Any reason for calling
> > del_vq()? Is it because you need to exclude a vq from the vq handler?
> >
> > For any case, the MSI and notification stuff seems unnecessary.
> >
> > Thanks
> >
> > >
> > >
> > > >
> > > > > +
> > > > > +   vring_reset_virtqueue(info->vq);
> > > > > +
> > > > > +   return 0;
> > > > > +}
> > > > > +
> > > > > +static struct virtqueue *vp_modern_enable_reset_vq(struct virtio_reset_vq *param)
> > > > > +{
> > > > > +   struct virtio_pci_device *vp_dev = to_vp_device(param->vdev);
> > > > > +   struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> > > > > +   struct virtio_pci_vq_info *info;
> > > > > +   u16 msix_vec, queue_index;
> > > > > +   struct virtqueue *vq;
> > > > > +
> > > > > +   if (!virtio_has_feature(param->vdev, VIRTIO_F_RING_RESET))
> > > > > +           return ERR_PTR(-ENOENT);
> > > > > +
> > > > > +   queue_index = param->queue_index;
> > > > > +
> > > > > +   info = vp_dev->vqs[queue_index];
> > > > > +
> > > > > +   if (!info->vq->reset)
> > > > > +           return ERR_PTR(-EPERM);
> > > > > +
> > > > > +   /* check queue reset status */
> > > > > +   if (vp_modern_get_queue_reset(mdev, queue_index) != 1)
> > > > > +           return ERR_PTR(-EBUSY);
> > > > > +
> > > > > +   vq = vp_setup_vq(param->vdev, queue_index, param->callback, param->name,
> > > > > +                    param->ctx, info->msix_vector, param->ring_num);
> > > > > +   if (IS_ERR(vq))
> > > > > +           return vq;
> > > > > +
> > > > > +   vp_modern_set_queue_enable(&vp_dev->mdev, vq->index, true);
> > > > > +
> > > > > +   msix_vec = vp_dev->vqs[queue_index]->msix_vector;
> > > > > +   if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
> > > > > +           enable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
> > > >
> > > >
> > > > How about the INT-X case?
> > >
> > > Explained above.
> > >
> > > Thanks.
> > >
> > > >
> > > > Thanks
> > > >
> > > >
> > > > > +
> > > > > +   return vq;
> > > > > +}
> > > > > +
> > > > >   static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)
> > > > >   {
> > > > >     return vp_modern_config_vector(&vp_dev->mdev, vector);
> > > > > @@ -284,6 +375,11 @@ static void del_vq(struct virtio_pci_vq_info *info)
> > > > >     struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> > > > >     struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> > > > >
> > > > > +   if (vq->reset) {
> > > > > +           vring_del_virtqueue(vq);
> > > > > +           return;
> > > > > +   }
> > > > > +
> > > > >     if (vp_dev->msix_enabled)
> > > > >             vp_modern_queue_vector(mdev, vq->index,
> > > > >                                    VIRTIO_MSI_NO_VECTOR);
> > > > > @@ -403,6 +499,8 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
> > > > >     .set_vq_affinity = vp_set_vq_affinity,
> > > > >     .get_vq_affinity = vp_get_vq_affinity,
> > > > >     .get_shm_region  = vp_get_shm_region,
> > > > > +   .reset_vq        = vp_modern_reset_vq,
> > > > > +   .enable_reset_vq = vp_modern_enable_reset_vq,
> > > > >   };
> > > > >
> > > > >   static const struct virtio_config_ops virtio_pci_config_ops = {
> > > > > @@ -421,6 +519,8 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
> > > > >     .set_vq_affinity = vp_set_vq_affinity,
> > > > >     .get_vq_affinity = vp_get_vq_affinity,
> > > > >     .get_shm_region  = vp_get_shm_region,
> > > > > +   .reset_vq        = vp_modern_reset_vq,
> > > > > +   .enable_reset_vq = vp_modern_enable_reset_vq,
> > > > >   };
> > > > >
> > > > >   /* the PCI probing function */
> > > >
> > >
> >
>

Re: [PATCH v3 13/17] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET

[Xuan Zhuo]

On Wed, 9 Feb 2022 13:44:06 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Tue, Feb 8, 2022 at 3:44 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Tue, 8 Feb 2022 10:55:37 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > On Mon, Feb 7, 2022 at 4:19 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > >
> > > > On Mon, 7 Feb 2022 14:57:13 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > 在 2022/1/26 下午3:35, Xuan Zhuo 写道:
> > > > > > This patch implements virtio pci support for QUEUE RESET.
> > > > > >
> > > > > > Performing reset on a queue is divided into two steps:
> > > > > >
> > > > > > 1. reset_vq: reset one vq
> > > > > > 2. enable_reset_vq: re-enable the reset queue
> > > > > >
> > > > > > In the first step, these tasks will be completed:
> > > > > >     1. notify the hardware queue to reset
> > > > > >     2. recycle the buffer from vq
> > > > > >     3. release the ring of the vq
> > > > > >
> > > > > > The process of enable reset vq:
> > > > > >      vp_modern_enable_reset_vq()
> > > > > >      vp_enable_reset_vq()
> > > > > >      __vp_setup_vq()
> > > > > >      setup_vq()
> > > > > >      vring_setup_virtqueue()
> > > > > >
> > > > > > In this process, we added two parameters, vq and num, and finally passed
> > > > > > them to vring_setup_virtqueue().  And reuse the original info and vq.
> > > > > >
> > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > > ---
> > > > > >   drivers/virtio/virtio_pci_common.c |  36 +++++++----
> > > > > >   drivers/virtio/virtio_pci_common.h |   5 ++
> > > > > >   drivers/virtio/virtio_pci_modern.c | 100 +++++++++++++++++++++++++++++
> > > > > >   3 files changed, 128 insertions(+), 13 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > > > index c02936d29a31..ad21638fbf66 100644
> > > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > > @@ -205,23 +205,28 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
> > > > > >     return err;
> > > > > >   }
> > > > > >
> > > > > > -static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
> > > > > > -                                void (*callback)(struct virtqueue *vq),
> > > > > > -                                const char *name,
> > > > > > -                                bool ctx,
> > > > > > -                                u16 msix_vec, u16 num)
> > > > > > +struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
> > > > > > +                         void (*callback)(struct virtqueue *vq),
> > > > > > +                         const char *name,
> > > > > > +                         bool ctx,
> > > > > > +                         u16 msix_vec, u16 num)
> > > > > >   {
> > > > > >     struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > > -   struct virtio_pci_vq_info *info = kmalloc(sizeof *info, GFP_KERNEL);
> > > > > > +   struct virtio_pci_vq_info *info;
> > > > > >     struct virtqueue *vq;
> > > > > >     unsigned long flags;
> > > > > >
> > > > > > -   /* fill out our structure that represents an active queue */
> > > > > > -   if (!info)
> > > > > > -           return ERR_PTR(-ENOMEM);
> > > > > > +   info = vp_dev->vqs[index];
> > > > > > +   if (!info) {
> > > > > > +           info = kzalloc(sizeof *info, GFP_KERNEL);
> > > > > > +
> > > > > > +           /* fill out our structure that represents an active queue */
> > > > > > +           if (!info)
> > > > > > +                   return ERR_PTR(-ENOMEM);
> > > > > > +   }
> > > > > >
> > > > > >     vq = vp_dev->setup_vq(vp_dev, info, index, callback, name, ctx,
> > > > > > -                         msix_vec, NULL, num);
> > > > > > +                         msix_vec, info->vq, num);
> > > > > >     if (IS_ERR(vq))
> > > > > >             goto out_info;
> > > > > >
> > > > > > @@ -238,6 +243,9 @@ static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
> > > > > >     return vq;
> > > > > >
> > > > > >   out_info:
> > > > > > +   if (info->vq && info->vq->reset)
> > > > > > +           return vq;
> > > > > > +
> > > > > >     kfree(info);
> > > > > >     return vq;
> > > > > >   }
> > > > > > @@ -248,9 +256,11 @@ static void vp_del_vq(struct virtqueue *vq)
> > > > > >     struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
> > > > > >     unsigned long flags;
> > > > > >
> > > > > > -   spin_lock_irqsave(&vp_dev->lock, flags);
> > > > > > -   list_del(&info->node);
> > > > > > -   spin_unlock_irqrestore(&vp_dev->lock, flags);
> > > > > > +   if (!vq->reset) {
> > > > > > +           spin_lock_irqsave(&vp_dev->lock, flags);
> > > > > > +           list_del(&info->node);
> > > > > > +           spin_unlock_irqrestore(&vp_dev->lock, flags);
> > > > > > +   }
> > > > > >
> > > > > >     vp_dev->del_vq(info);
> > > > > >     kfree(info);
> > > > > > diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
> > > > > > index 65db92245e41..c1d15f7c0be4 100644
> > > > > > --- a/drivers/virtio/virtio_pci_common.h
> > > > > > +++ b/drivers/virtio/virtio_pci_common.h
> > > > > > @@ -119,6 +119,11 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> > > > > >             struct virtqueue *vqs[], vq_callback_t *callbacks[],
> > > > > >             const char * const names[], const bool *ctx,
> > > > > >             struct irq_affinity *desc);
> > > > > > +struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
> > > > > > +                         void (*callback)(struct virtqueue *vq),
> > > > > > +                         const char *name,
> > > > > > +                         bool ctx,
> > > > > > +                         u16 msix_vec, u16 num);
> > > > > >   const char *vp_bus_name(struct virtio_device *vdev);
> > > > > >
> > > > > >   /* Setup the affinity for a virtqueue:
> > > > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > > > > > index 2ce58de549de..6789411169e4 100644
> > > > > > --- a/drivers/virtio/virtio_pci_modern.c
> > > > > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > > > > @@ -34,6 +34,9 @@ static void vp_transport_features(struct virtio_device *vdev, u64 features)
> > > > > >     if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
> > > > > >                     pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
> > > > > >             __virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
> > > > > > +
> > > > > > +   if (features & BIT_ULL(VIRTIO_F_RING_RESET))
> > > > > > +           __virtio_set_bit(vdev, VIRTIO_F_RING_RESET);
> > > > > >   }
> > > > > >
> > > > > >   /* virtio config->finalize_features() implementation */
> > > > > > @@ -176,6 +179,94 @@ static void vp_reset(struct virtio_device *vdev)
> > > > > >     vp_disable_cbs(vdev);
> > > > > >   }
> > > > > >
> > > > > > +static int vp_modern_reset_vq(struct virtio_reset_vq *param)
> > > > > > +{
> > > > > > +   struct virtio_pci_device *vp_dev = to_vp_device(param->vdev);
> > > > > > +   struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> > > > > > +   struct virtio_pci_vq_info *info;
> > > > > > +   u16 msix_vec, queue_index;
> > > > > > +   unsigned long flags;
> > > > > > +   void *buf;
> > > > > > +
> > > > > > +   if (!virtio_has_feature(param->vdev, VIRTIO_F_RING_RESET))
> > > > > > +           return -ENOENT;
> > > > > > +
> > > > > > +   queue_index = param->queue_index;
> > > > > > +
> > > > > > +   vp_modern_set_queue_reset(mdev, queue_index);
> > > > > > +
> > > > > > +   /* After write 1 to queue reset, the driver MUST wait for a read of
> > > > > > +    * queue reset to return 1.
> > > > > > +    */
> > > > > > +   while (vp_modern_get_queue_reset(mdev, queue_index) != 1)
> > > > > > +           msleep(1);
> > > > >
> > > > >
> > > > > Is this better to move this logic into vp_modern_set_queue_reset()?
> > > > >
> > > >
> > > > OK.
> > > >
> > > > >
> > > > > > +
> > > > > > +   info = vp_dev->vqs[queue_index];
> > > > > > +   msix_vec = info->msix_vector;
> > > > > > +
> > > > > > +   /* Disable VQ callback. */
> > > > > > +   if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
> > > > > > +           disable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
> > > > >
> > > > >
> > > > > How about the INTX case where irq is shared? I guess we need to disable
> > > > > and enable the irq as well.
> > > >
> > > > For the INTX scenario, I don't think we need to disable/enable the irq. But I do
> > > > have a mistake, I should put the following list_del(&info->node) here, so that
> > > > when the irq comes, it will no longer operate this vq.
> > > >
> > > > In fact, for no INTX case, the disable_irq here is not necessary, according to
> > > > the requirements of the spec, after reset, the device should not generate irq
> > > > anymore.
> > >
> > > I may miss something but I don't think INTX differs from MSI from the
> > > vq handler perspective.
> > >
> > > > Here just to prevent accidents.
> > >
> > > The problem is that if you don't disable/sync IRQ there could be a
> > > race between the vq irq handler and the virtqueue_detach_unused_buf()?
> > >
> > > >
> > > > >
> > > > >
> > > > > > +
> > > > > > +   while ((buf = virtqueue_detach_unused_buf(info->vq)) != NULL)
> > > > > > +           param->free_unused_cb(param, buf);
> > > > >
> > > > >
> > > > > Any reason that we can't leave this logic to driver? (Or is there any
> > > > > operations that must be done before the following operations?)
> > > >
> > > > As you commented before, we merged free unused buf and reset queue.
> > > >
> > > > I think it's a good note, otherwise, we're going to
> > > >
> > > >         1. reset vq
> > > >         2. free unused(by driver)
> > > >         3. free ring of vq
> > >
> > > Rethink about this, I'd prefer to leave it to the driver for consistency.
> > >
> > > E.g the virtqueue_detach_unused_buf() is called by each driver nowdays.
> >
> > In this case, go back to my previous design and add three helpers:
> >
> >         int (*reset_vq)();
> >         int (*free_reset_vq)();
>
> So this is only needed if there are any transport specific operations.
> But I don't see there's any.

Yes, you are right.

Performing reset on a queue is divided into four steps
    1. virtio_config_ops->reset_vq(): reset one vq
    2. recycle the buffer from vq by virtqueue_detach_unused_buf()
    3. release the ring of the vq by vring_release_virtqueue() (new function)
    4. virtio_config_ops->enable_reset_vq(): re-enable the reset queue

Thanks.

>
> Thanks
>
> >         int (*enable_reset_vq)();
> >
> > Thanks.
> >
> > >
> > > >
> > > >
> > > > >
> > > > >
> > > > > > +
> > > > > > +   /* delete vq */
> > > > > > +   spin_lock_irqsave(&vp_dev->lock, flags);
> > > > > > +   list_del(&info->node);
> > > > > > +   spin_unlock_irqrestore(&vp_dev->lock, flags);
> > > > > > +
> > > > > > +   INIT_LIST_HEAD(&info->node);
> > > > > > +
> > > > > > +   if (vp_dev->msix_enabled)
> > > > > > +           vp_modern_queue_vector(mdev, info->vq->index,
> > > > > > +                                  VIRTIO_MSI_NO_VECTOR);
> > > > >
> > > > >
> > > > > I wonder if this is a must.
> > > > >
> > > > >
> > > > > > +
> > > > > > +   if (!mdev->notify_base)
> > > > > > +           pci_iounmap(mdev->pci_dev,
> > > > > > +                       (void __force __iomem *)info->vq->priv);
> > > > >
> > > > >
> > > > > Is this a must? what happens if we simply don't do this?
> > > > >
> > > >
> > > > The purpose of these two operations is mainly to be symmetrical with del_vq().
> > >
> > > This is another question I want to ask. Any reason for calling
> > > del_vq()? Is it because you need to exclude a vq from the vq handler?
> > >
> > > For any case, the MSI and notification stuff seems unnecessary.
> > >
> > > Thanks
> > >
> > > >
> > > >
> > > > >
> > > > > > +
> > > > > > +   vring_reset_virtqueue(info->vq);
> > > > > > +
> > > > > > +   return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static struct virtqueue *vp_modern_enable_reset_vq(struct virtio_reset_vq *param)
> > > > > > +{
> > > > > > +   struct virtio_pci_device *vp_dev = to_vp_device(param->vdev);
> > > > > > +   struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> > > > > > +   struct virtio_pci_vq_info *info;
> > > > > > +   u16 msix_vec, queue_index;
> > > > > > +   struct virtqueue *vq;
> > > > > > +
> > > > > > +   if (!virtio_has_feature(param->vdev, VIRTIO_F_RING_RESET))
> > > > > > +           return ERR_PTR(-ENOENT);
> > > > > > +
> > > > > > +   queue_index = param->queue_index;
> > > > > > +
> > > > > > +   info = vp_dev->vqs[queue_index];
> > > > > > +
> > > > > > +   if (!info->vq->reset)
> > > > > > +           return ERR_PTR(-EPERM);
> > > > > > +
> > > > > > +   /* check queue reset status */
> > > > > > +   if (vp_modern_get_queue_reset(mdev, queue_index) != 1)
> > > > > > +           return ERR_PTR(-EBUSY);
> > > > > > +
> > > > > > +   vq = vp_setup_vq(param->vdev, queue_index, param->callback, param->name,
> > > > > > +                    param->ctx, info->msix_vector, param->ring_num);
> > > > > > +   if (IS_ERR(vq))
> > > > > > +           return vq;
> > > > > > +
> > > > > > +   vp_modern_set_queue_enable(&vp_dev->mdev, vq->index, true);
> > > > > > +
> > > > > > +   msix_vec = vp_dev->vqs[queue_index]->msix_vector;
> > > > > > +   if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
> > > > > > +           enable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
> > > > >
> > > > >
> > > > > How about the INT-X case?
> > > >
> > > > Explained above.
> > > >
> > > > Thanks.
> > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > >
> > > > > > +
> > > > > > +   return vq;
> > > > > > +}
> > > > > > +
> > > > > >   static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)
> > > > > >   {
> > > > > >     return vp_modern_config_vector(&vp_dev->mdev, vector);
> > > > > > @@ -284,6 +375,11 @@ static void del_vq(struct virtio_pci_vq_info *info)
> > > > > >     struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> > > > > >     struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> > > > > >
> > > > > > +   if (vq->reset) {
> > > > > > +           vring_del_virtqueue(vq);
> > > > > > +           return;
> > > > > > +   }
> > > > > > +
> > > > > >     if (vp_dev->msix_enabled)
> > > > > >             vp_modern_queue_vector(mdev, vq->index,
> > > > > >                                    VIRTIO_MSI_NO_VECTOR);
> > > > > > @@ -403,6 +499,8 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
> > > > > >     .set_vq_affinity = vp_set_vq_affinity,
> > > > > >     .get_vq_affinity = vp_get_vq_affinity,
> > > > > >     .get_shm_region  = vp_get_shm_region,
> > > > > > +   .reset_vq        = vp_modern_reset_vq,
> > > > > > +   .enable_reset_vq = vp_modern_enable_reset_vq,
> > > > > >   };
> > > > > >
> > > > > >   static const struct virtio_config_ops virtio_pci_config_ops = {
> > > > > > @@ -421,6 +519,8 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
> > > > > >     .set_vq_affinity = vp_set_vq_affinity,
> > > > > >     .get_vq_affinity = vp_get_vq_affinity,
> > > > > >     .get_shm_region  = vp_get_shm_region,
> > > > > > +   .reset_vq        = vp_modern_reset_vq,
> > > > > > +   .enable_reset_vq = vp_modern_enable_reset_vq,
> > > > > >   };
> > > > > >
> > > > > >   /* the PCI probing function */
> > > > >
> > > >
> > >
> >
>

[PATCH v3 14/17] virtio_net: virtnet_tx_timeout() fix style

[Xuan Zhuo]

Replace priv with vi which is commonly used in the virtio-net module.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 569eecfbc2cd..97eb4dddba1f 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2683,8 +2683,8 @@ static int virtnet_set_features(struct net_device *dev,
 
 static void virtnet_tx_timeout(struct net_device *dev, unsigned int txqueue)
 {
-	struct virtnet_info *priv = netdev_priv(dev);
-	struct send_queue *sq = &priv->sq[txqueue];
+	struct virtnet_info *vi = netdev_priv(dev);
+	struct send_queue *sq = &vi->sq[txqueue];
 	struct netdev_queue *txq = netdev_get_tx_queue(dev, txqueue);
 
 	u64_stats_update_begin(&sq->stats.syncp);
-- 
2.31.0

[PATCH v3 15/17] virtio_net: virtnet_tx_timeout() stop ref sq->vq

[Xuan Zhuo]

Make virtnet_tx_timeout() no longer refer to vq directly. Because sq->vq
may be equal to NULL after implementing rx/tx queue disable/enable.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 97eb4dddba1f..2add7fe749b8 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2692,7 +2692,7 @@ static void virtnet_tx_timeout(struct net_device *dev, unsigned int txqueue)
 	u64_stats_update_end(&sq->stats.syncp);
 
 	netdev_err(dev, "TX timeout on queue: %u, sq: %s, vq: 0x%x, name: %s, %u usecs ago\n",
-		   txqueue, sq->name, sq->vq->index, sq->vq->name,
+		   txqueue, sq->name, txq2vq(sq - vi->sq), sq->name,
 		   jiffies_to_usecs(jiffies - READ_ONCE(txq->trans_start)));
 }
 
-- 
2.31.0

[PATCH v3 16/17] virtio_net: split free_unused_bufs()

[Xuan Zhuo]

This patch separates two functions for freeing sq buf and rq buf from
free_unused_bufs().

When supporting the enable/disable tx/rq queue in the future, it is
necessary to support separate recovery of a sq buf or a rq buf.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 46 ++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 18 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 2add7fe749b8..ea90a1a57c9e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2804,33 +2804,43 @@ static void free_receive_page_frags(struct virtnet_info *vi)
 			put_page(vi->rq[i].alloc_frag.page);
 }
 
+static void virtnet_sq_free_unused_buf(struct virtnet_info *vi,
+				       struct send_queue *sq, void *buf)
+{
+	if (!is_xdp_frame(buf))
+		dev_kfree_skb(buf);
+	else
+		xdp_return_frame(ptr_to_xdp(buf));
+}
+
+static void virtnet_rq_free_unused_buf(struct virtnet_info *vi,
+				       struct receive_queue *rq, void *buf)
+{
+	if (vi->mergeable_rx_bufs)
+		put_page(virt_to_head_page(buf));
+	else if (vi->big_packets)
+		give_pages(rq, buf);
+	else
+		put_page(virt_to_head_page(buf));
+}
+
 static void free_unused_bufs(struct virtnet_info *vi)
 {
+	struct receive_queue *rq;
+	struct send_queue *sq;
 	void *buf;
 	int i;
 
 	for (i = 0; i < vi->max_queue_pairs; i++) {
-		struct virtqueue *vq = vi->sq[i].vq;
-		while ((buf = virtqueue_detach_unused_buf(vq)) != NULL) {
-			if (!is_xdp_frame(buf))
-				dev_kfree_skb(buf);
-			else
-				xdp_return_frame(ptr_to_xdp(buf));
-		}
+		sq = &vi->sq[i];
+		while ((buf = virtqueue_detach_unused_buf(sq->vq)) != NULL)
+			virtnet_sq_free_unused_buf(vi, sq, buf);
 	}
 
 	for (i = 0; i < vi->max_queue_pairs; i++) {
-		struct virtqueue *vq = vi->rq[i].vq;
-
-		while ((buf = virtqueue_detach_unused_buf(vq)) != NULL) {
-			if (vi->mergeable_rx_bufs) {
-				put_page(virt_to_head_page(buf));
-			} else if (vi->big_packets) {
-				give_pages(&vi->rq[i], buf);
-			} else {
-				put_page(virt_to_head_page(buf));
-			}
-		}
+		rq = &vi->rq[i];
+		while ((buf = virtqueue_detach_unused_buf(rq->vq)) != NULL)
+			virtnet_rq_free_unused_buf(vi, rq, buf);
 	}
 }
 
-- 
2.31.0

[PATCH v3 17/17] virtio_net: support pair disable/enable

[Xuan Zhuo]

This patch implements virtio-net rx/tx pair disable/enable functionality
based on virtio queue reset. The purpose of the current implementation
is to quickly recycle the buffer submitted to vq.

In the process of pair disable, in theory, as long as virtio supports
queue reset, there will be no exceptions.

However, in the process of pari enable, there may be exceptions due to
memory allocation. In this case, vq == NULL, but napi will still
be enabled. Because napi_disable is similar to a lock, napi_enable must
be called after calling napi_disable.

Since enable fails, the driver will not receive an interrupt from the
device to wake up napi, so the driver is safe. But we still need to add
vq checks in some places to ensure safety, such as refill_work().

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 168 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 168 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ea90a1a57c9e..cf77ef1bad1c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1369,6 +1369,9 @@ static void virtnet_napi_enable(struct virtqueue *vq, struct napi_struct *napi)
 {
 	napi_enable(napi);
 
+	if (!vq)
+		return;
+
 	/* If all buffers were filled by other side before we napi_enabled, we
 	 * won't get another interrupt, so process any outstanding packets now.
 	 * Call local_bh_enable after to trigger softIRQ processing.
@@ -1413,6 +1416,10 @@ static void refill_work(struct work_struct *work)
 		struct receive_queue *rq = &vi->rq[i];
 
 		napi_disable(&rq->napi);
+		if (!rq->vq) {
+			virtnet_napi_enable(rq->vq, &rq->napi);
+			continue;
+		}
 		still_empty = !try_fill_recv(vi, rq, GFP_KERNEL);
 		virtnet_napi_enable(rq->vq, &rq->napi);
 
@@ -2871,6 +2878,167 @@ static unsigned int mergeable_min_buf_len(struct virtnet_info *vi, struct virtqu
 		   (unsigned int)GOOD_PACKET_LEN);
 }
 
+static void virtnet_rq_free_unused_buf_cb(struct virtio_reset_vq *param,
+					  void *buf)
+{
+	virtnet_rq_free_unused_buf(param->vdev->priv, param->data, buf);
+}
+
+static void virtnet_sq_free_unused_buf_cb(struct virtio_reset_vq *param,
+					  void *buf)
+{
+	virtnet_rq_free_unused_buf(param->vdev->priv, param->data, buf);
+}
+
+static int __virtnet_rx_vq_disable(struct virtnet_info *vi,
+				   struct receive_queue *rq)
+{
+	struct virtio_reset_vq param = {0};
+	int err, qnum;
+
+	qnum = rxq2vq(rq - vi->rq);
+
+	napi_disable(&rq->napi);
+
+	param.vdev = vi->vdev;
+	param.queue_index = qnum;
+	param.free_unused_cb = virtnet_rq_free_unused_buf_cb;
+	param.data = rq;
+
+	err = virtio_reset_vq(&param);
+	if (err) {
+		virtnet_napi_enable(rq->vq, &rq->napi);
+		return err;
+	}
+
+	rq->vq = NULL;
+
+	return err;
+}
+
+static int __virtnet_tx_vq_disable(struct virtnet_info *vi,
+				   struct send_queue *sq)
+{
+	struct virtio_reset_vq param = {0};
+	struct netdev_queue *txq;
+	int err, qnum;
+
+	qnum = txq2vq(sq - vi->sq);
+
+	netif_stop_subqueue(vi->dev, sq - vi->sq);
+	virtnet_napi_tx_disable(&sq->napi);
+
+	/* wait xmit done */
+	txq = netdev_get_tx_queue(vi->dev, qnum);
+	__netif_tx_lock(txq, raw_smp_processor_id());
+	__netif_tx_unlock(txq);
+
+	param.vdev = vi->vdev;
+	param.queue_index = qnum;
+	param.free_unused_cb = virtnet_sq_free_unused_buf_cb;
+	param.data = sq;
+
+	err = virtio_reset_vq(&param);
+	if (err) {
+		virtnet_napi_tx_enable(vi, sq->vq, &sq->napi);
+		netif_start_subqueue(vi->dev, sq - vi->sq);
+		return err;
+	}
+
+	sq->vq = NULL;
+
+	return err;
+}
+
+static int virtnet_pair_disable(struct virtnet_info *vi, int i)
+{
+	int err;
+
+	err = __virtnet_rx_vq_disable(vi, vi->rq + i);
+	if (err)
+		return err;
+
+	return __virtnet_tx_vq_disable(vi, vi->sq + i);
+}
+
+static int virtnet_enable_resetq(struct virtnet_info *vi,
+				 struct receive_queue *rq,
+				 struct send_queue *sq)
+{
+	struct virtio_reset_vq param = {0};
+	vq_callback_t *callback;
+	struct virtqueue *vq;
+	const char *name;
+	int vq_idx;
+	bool ctx;
+
+	if (rq) {
+		vq = rq->vq;
+		vq_idx = rxq2vq(rq - vi->rq);
+		callback = skb_recv_done;
+		name = rq->name;
+
+	} else {
+		vq = sq->vq;
+		vq_idx = txq2vq(sq - vi->sq);
+		callback = skb_xmit_done;
+		name = sq->name;
+	}
+
+	if (vq)
+		return -EBUSY;
+
+	if (!vi->big_packets || vi->mergeable_rx_bufs)
+		ctx = true;
+	else
+		ctx = false;
+
+	param.vdev = vi->vdev;
+	param.queue_index = vq_idx;
+	param.callback = callback;
+	param.name = name;
+	param.ctx = &ctx;
+	param.ring_num = 0;
+
+	vq = virtio_enable_resetq(&param);
+	if (IS_ERR(vq))
+		return PTR_ERR(vq);
+
+	if (rq)
+		rq->vq = vq;
+	else
+		sq->vq = vq;
+
+	return 0;
+}
+
+static int virtnet_pair_enable(struct virtnet_info *vi, int i)
+{
+	struct receive_queue *rq;
+	struct send_queue *sq;
+	int err;
+
+	rq = vi->rq + i;
+	sq = vi->sq + i;
+
+	/* tx */
+	err = virtnet_enable_resetq(vi, NULL, sq);
+	if (err)
+		goto err;
+	else
+		netif_start_subqueue(vi->dev, sq - vi->sq);
+
+	/* rx */
+	err = virtnet_enable_resetq(vi, rq, NULL);
+	if (err)
+		return err;
+
+err:
+	virtnet_napi_tx_enable(vi, sq->vq, &sq->napi);
+	virtnet_napi_enable(rq->vq, &rq->napi);
+	return 0;
+}
+
 static int virtnet_find_vqs(struct virtnet_info *vi)
 {
 	vq_callback_t **callbacks;
-- 
2.31.0

Re: [PATCH v3 00/17] virtio pci support VIRTIO_F_RING_RESET

[Jason Wang]

On Wed, Jan 26, 2022 at 3:35 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> ================================================================================
> The virtio spec already supports the virtio queue reset function. This patch set
> is to add this function to the kernel. The relevant virtio spec information is
> here:
>
>     https://github.com/oasis-tcs/virtio-spec/issues/124
>
> Also regarding MMIO support for queue reset, I plan to support it after this
> patch is passed.
>
> #14-#17 is the disable/enable function of rx/tx pair implemented by virtio-net
> using the new helper.

One thing that came to mind is the steering. E.g if we disable an RX,
should we stop steering packets to that queue?

Thanks

> This function is not currently referenced by other
> functions. It is more to show the usage of the new helper, I not sure if they
> are going to be merged together.
>
> Please review. Thanks.
>
> v3:
>   1. keep vq, irq unreleased
>
> Xuan Zhuo (17):
>   virtio_pci: struct virtio_pci_common_cfg add queue_notify_data
>   virtio: queue_reset: add VIRTIO_F_RING_RESET
>   virtio: queue_reset: struct virtio_config_ops add callbacks for
>     queue_reset
>   virtio: queue_reset: add helper
>   vritio_ring: queue_reset: extract the release function of the vq ring
>   virtio_ring: queue_reset: split: add __vring_init_virtqueue()
>   virtio_ring: queue_reset: split: support enable reset queue
>   virtio_ring: queue_reset: packed: support enable reset queue
>   virtio_ring: queue_reset: add vring_reset_virtqueue()
>   virtio_pci: queue_reset: update struct virtio_pci_common_cfg and
>     option functions
>   virtio_pci: queue_reset: release vq by vp_dev->vqs
>   virtio_pci: queue_reset: setup_vq use vring_setup_virtqueue()
>   virtio_pci: queue_reset: support VIRTIO_F_RING_RESET
>   virtio_net: virtnet_tx_timeout() fix style
>   virtio_net: virtnet_tx_timeout() stop ref sq->vq
>   virtio_net: split free_unused_bufs()
>   virtio_net: support pair disable/enable
>
>  drivers/net/virtio_net.c               | 220 ++++++++++++++++++++++---
>  drivers/virtio/virtio_pci_common.c     |  62 ++++---
>  drivers/virtio/virtio_pci_common.h     |  11 +-
>  drivers/virtio/virtio_pci_legacy.c     |   5 +-
>  drivers/virtio/virtio_pci_modern.c     | 120 +++++++++++++-
>  drivers/virtio/virtio_pci_modern_dev.c |  28 ++++
>  drivers/virtio/virtio_ring.c           | 144 +++++++++++-----
>  include/linux/virtio.h                 |   1 +
>  include/linux/virtio_config.h          |  75 ++++++++-
>  include/linux/virtio_pci_modern.h      |   2 +
>  include/linux/virtio_ring.h            |  42 +++--
>  include/uapi/linux/virtio_config.h     |   7 +-
>  include/uapi/linux/virtio_pci.h        |   2 +
>  13 files changed, 618 insertions(+), 101 deletions(-)
>
> --
> 2.31.0
>

Re: [PATCH v3 00/17] virtio pci support VIRTIO_F_RING_RESET

[Jason Wang]

在 2022/2/7 下午2:02, Xuan Zhuo 写道:
> On Mon, 7 Feb 2022 11:39:36 +0800, Jason Wang <jasowang@redhat.com> wrote:
>> On Wed, Jan 26, 2022 at 3:35 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>>> ================================================================================
>>> The virtio spec already supports the virtio queue reset function. This patch set
>>> is to add this function to the kernel. The relevant virtio spec information is
>>> here:
>>>
>>>      https://github.com/oasis-tcs/virtio-spec/issues/124
>>>
>>> Also regarding MMIO support for queue reset, I plan to support it after this
>>> patch is passed.
>>>
>>> #14-#17 is the disable/enable function of rx/tx pair implemented by virtio-net
>>> using the new helper.
>> One thing that came to mind is the steering. E.g if we disable an RX,
>> should we stop steering packets to that queue?
> Yes, we should reselect a queue.
>
> Thanks.


Maybe a spec patch for that?

Thanks


>
>> Thanks
>>
>>> This function is not currently referenced by other
>>> functions. It is more to show the usage of the new helper, I not sure if they
>>> are going to be merged together.
>>>
>>> Please review. Thanks.
>>>
>>> v3:
>>>    1. keep vq, irq unreleased
>>>
>>> Xuan Zhuo (17):
>>>    virtio_pci: struct virtio_pci_common_cfg add queue_notify_data
>>>    virtio: queue_reset: add VIRTIO_F_RING_RESET
>>>    virtio: queue_reset: struct virtio_config_ops add callbacks for
>>>      queue_reset
>>>    virtio: queue_reset: add helper
>>>    vritio_ring: queue_reset: extract the release function of the vq ring
>>>    virtio_ring: queue_reset: split: add __vring_init_virtqueue()
>>>    virtio_ring: queue_reset: split: support enable reset queue
>>>    virtio_ring: queue_reset: packed: support enable reset queue
>>>    virtio_ring: queue_reset: add vring_reset_virtqueue()
>>>    virtio_pci: queue_reset: update struct virtio_pci_common_cfg and
>>>      option functions
>>>    virtio_pci: queue_reset: release vq by vp_dev->vqs
>>>    virtio_pci: queue_reset: setup_vq use vring_setup_virtqueue()
>>>    virtio_pci: queue_reset: support VIRTIO_F_RING_RESET
>>>    virtio_net: virtnet_tx_timeout() fix style
>>>    virtio_net: virtnet_tx_timeout() stop ref sq->vq
>>>    virtio_net: split free_unused_bufs()
>>>    virtio_net: support pair disable/enable
>>>
>>>   drivers/net/virtio_net.c               | 220 ++++++++++++++++++++++---
>>>   drivers/virtio/virtio_pci_common.c     |  62 ++++---
>>>   drivers/virtio/virtio_pci_common.h     |  11 +-
>>>   drivers/virtio/virtio_pci_legacy.c     |   5 +-
>>>   drivers/virtio/virtio_pci_modern.c     | 120 +++++++++++++-
>>>   drivers/virtio/virtio_pci_modern_dev.c |  28 ++++
>>>   drivers/virtio/virtio_ring.c           | 144 +++++++++++-----
>>>   include/linux/virtio.h                 |   1 +
>>>   include/linux/virtio_config.h          |  75 ++++++++-
>>>   include/linux/virtio_pci_modern.h      |   2 +
>>>   include/linux/virtio_ring.h            |  42 +++--
>>>   include/uapi/linux/virtio_config.h     |   7 +-
>>>   include/uapi/linux/virtio_pci.h        |   2 +
>>>   13 files changed, 618 insertions(+), 101 deletions(-)
>>>
>>> --
>>> 2.31.0
>>>

Re: [PATCH v3 00/17] virtio pci support VIRTIO_F_RING_RESET

[Xuan Zhuo]

On Tue, 08 Feb 2022 11:14:45 +0800, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> On Tue, 8 Feb 2022 10:59:48 +0800, Jason Wang <jasowang@redhat.com> wrote:
> >
> > 在 2022/2/7 下午2:02, Xuan Zhuo 写道:
> > > On Mon, 7 Feb 2022 11:39:36 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > >> On Wed, Jan 26, 2022 at 3:35 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >>> ================================================================================
> > >>> The virtio spec already supports the virtio queue reset function. This patch set
> > >>> is to add this function to the kernel. The relevant virtio spec information is
> > >>> here:
> > >>>
> > >>>      https://github.com/oasis-tcs/virtio-spec/issues/124
> > >>>
> > >>> Also regarding MMIO support for queue reset, I plan to support it after this
> > >>> patch is passed.
> > >>>
> > >>> #14-#17 is the disable/enable function of rx/tx pair implemented by virtio-net
> > >>> using the new helper.
> > >> One thing that came to mind is the steering. E.g if we disable an RX,
> > >> should we stop steering packets to that queue?

Regarding this spec, if there are multiple queues disabled at the same time, it
will be a troublesome problem for the backend to select the queue, so I want to
directly define that only one queue is allowed to reset at the same time, do you
think this is appropriate?

In terms of the implementation of backend queue reselection, it would be more
convenient to implement if we drop packets directly. Do you think we must
implement this reselection function?

Thanks.

> > > Yes, we should reselect a queue.
> > >
> > > Thanks.
> >
> >
> > Maybe a spec patch for that?
>
> Yes, I also realized this. Although virtio-net's disable/enable is implemented
> based on queue reset, virtio-net still has to define its own flag and define
> some more detailed implementations.
>
> I'll think about it and post a spec patch.
>
> Thanks.
>
> >
> > Thanks
> >
> >
> > >
> > >> Thanks
> > >>
> > >>> This function is not currently referenced by other
> > >>> functions. It is more to show the usage of the new helper, I not sure if they
> > >>> are going to be merged together.
> > >>>
> > >>> Please review. Thanks.
> > >>>
> > >>> v3:
> > >>>    1. keep vq, irq unreleased
> > >>>
> > >>> Xuan Zhuo (17):
> > >>>    virtio_pci: struct virtio_pci_common_cfg add queue_notify_data
> > >>>    virtio: queue_reset: add VIRTIO_F_RING_RESET
> > >>>    virtio: queue_reset: struct virtio_config_ops add callbacks for
> > >>>      queue_reset
> > >>>    virtio: queue_reset: add helper
> > >>>    vritio_ring: queue_reset: extract the release function of the vq ring
> > >>>    virtio_ring: queue_reset: split: add __vring_init_virtqueue()
> > >>>    virtio_ring: queue_reset: split: support enable reset queue
> > >>>    virtio_ring: queue_reset: packed: support enable reset queue
> > >>>    virtio_ring: queue_reset: add vring_reset_virtqueue()
> > >>>    virtio_pci: queue_reset: update struct virtio_pci_common_cfg and
> > >>>      option functions
> > >>>    virtio_pci: queue_reset: release vq by vp_dev->vqs
> > >>>    virtio_pci: queue_reset: setup_vq use vring_setup_virtqueue()
> > >>>    virtio_pci: queue_reset: support VIRTIO_F_RING_RESET
> > >>>    virtio_net: virtnet_tx_timeout() fix style
> > >>>    virtio_net: virtnet_tx_timeout() stop ref sq->vq
> > >>>    virtio_net: split free_unused_bufs()
> > >>>    virtio_net: support pair disable/enable
> > >>>
> > >>>   drivers/net/virtio_net.c               | 220 ++++++++++++++++++++++---
> > >>>   drivers/virtio/virtio_pci_common.c     |  62 ++++---
> > >>>   drivers/virtio/virtio_pci_common.h     |  11 +-
> > >>>   drivers/virtio/virtio_pci_legacy.c     |   5 +-
> > >>>   drivers/virtio/virtio_pci_modern.c     | 120 +++++++++++++-
> > >>>   drivers/virtio/virtio_pci_modern_dev.c |  28 ++++
> > >>>   drivers/virtio/virtio_ring.c           | 144 +++++++++++-----
> > >>>   include/linux/virtio.h                 |   1 +
> > >>>   include/linux/virtio_config.h          |  75 ++++++++-
> > >>>   include/linux/virtio_pci_modern.h      |   2 +
> > >>>   include/linux/virtio_ring.h            |  42 +++--
> > >>>   include/uapi/linux/virtio_config.h     |   7 +-
> > >>>   include/uapi/linux/virtio_pci.h        |   2 +
> > >>>   13 files changed, 618 insertions(+), 101 deletions(-)
> > >>>
> > >>> --
> > >>> 2.31.0
> > >>>
> >
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 00/17] virtio pci support VIRTIO_F_RING_RESET

[Jason Wang]

On Tue, Feb 8, 2022 at 3:56 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Tue, 08 Feb 2022 11:14:45 +0800, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > On Tue, 8 Feb 2022 10:59:48 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > 在 2022/2/7 下午2:02, Xuan Zhuo 写道:
> > > > On Mon, 7 Feb 2022 11:39:36 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > >> On Wed, Jan 26, 2022 at 3:35 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > >>> ================================================================================
> > > >>> The virtio spec already supports the virtio queue reset function. This patch set
> > > >>> is to add this function to the kernel. The relevant virtio spec information is
> > > >>> here:
> > > >>>
> > > >>>      https://github.com/oasis-tcs/virtio-spec/issues/124
> > > >>>
> > > >>> Also regarding MMIO support for queue reset, I plan to support it after this
> > > >>> patch is passed.
> > > >>>
> > > >>> #14-#17 is the disable/enable function of rx/tx pair implemented by virtio-net
> > > >>> using the new helper.
> > > >> One thing that came to mind is the steering. E.g if we disable an RX,
> > > >> should we stop steering packets to that queue?
>
> Regarding this spec, if there are multiple queues disabled at the same time, it
> will be a troublesome problem for the backend to select the queue,

I don't understand this, for such a kind of backend or device it can
simply claim not to support this feature.

> so I want to
> directly define that only one queue is allowed to reset at the same time, do you
> think this is appropriate?

This sounds very complicated. E.g how can we define an API that can
reset more than one queues? (currently PCI only support MMIO access).

>
> In terms of the implementation of backend queue reselection, it would be more
> convenient to implement if we drop packets directly. Do you think we must
> implement this reselection function?

Rethink of this. It should be fine and much simpler.

Thanks

>
> Thanks.
>
> > > > Yes, we should reselect a queue.
> > > >
> > > > Thanks.
> > >
> > >
> > > Maybe a spec patch for that?
> >
> > Yes, I also realized this. Although virtio-net's disable/enable is implemented
> > based on queue reset, virtio-net still has to define its own flag and define
> > some more detailed implementations.
> >
> > I'll think about it and post a spec patch.
> >
> > Thanks.
> >
> > >
> > > Thanks
> > >
> > >
> > > >
> > > >> Thanks
> > > >>
> > > >>> This function is not currently referenced by other
> > > >>> functions. It is more to show the usage of the new helper, I not sure if they
> > > >>> are going to be merged together.
> > > >>>
> > > >>> Please review. Thanks.
> > > >>>
> > > >>> v3:
> > > >>>    1. keep vq, irq unreleased
> > > >>>
> > > >>> Xuan Zhuo (17):
> > > >>>    virtio_pci: struct virtio_pci_common_cfg add queue_notify_data
> > > >>>    virtio: queue_reset: add VIRTIO_F_RING_RESET
> > > >>>    virtio: queue_reset: struct virtio_config_ops add callbacks for
> > > >>>      queue_reset
> > > >>>    virtio: queue_reset: add helper
> > > >>>    vritio_ring: queue_reset: extract the release function of the vq ring
> > > >>>    virtio_ring: queue_reset: split: add __vring_init_virtqueue()
> > > >>>    virtio_ring: queue_reset: split: support enable reset queue
> > > >>>    virtio_ring: queue_reset: packed: support enable reset queue
> > > >>>    virtio_ring: queue_reset: add vring_reset_virtqueue()
> > > >>>    virtio_pci: queue_reset: update struct virtio_pci_common_cfg and
> > > >>>      option functions
> > > >>>    virtio_pci: queue_reset: release vq by vp_dev->vqs
> > > >>>    virtio_pci: queue_reset: setup_vq use vring_setup_virtqueue()
> > > >>>    virtio_pci: queue_reset: support VIRTIO_F_RING_RESET
> > > >>>    virtio_net: virtnet_tx_timeout() fix style
> > > >>>    virtio_net: virtnet_tx_timeout() stop ref sq->vq
> > > >>>    virtio_net: split free_unused_bufs()
> > > >>>    virtio_net: support pair disable/enable
> > > >>>
> > > >>>   drivers/net/virtio_net.c               | 220 ++++++++++++++++++++++---
> > > >>>   drivers/virtio/virtio_pci_common.c     |  62 ++++---
> > > >>>   drivers/virtio/virtio_pci_common.h     |  11 +-
> > > >>>   drivers/virtio/virtio_pci_legacy.c     |   5 +-
> > > >>>   drivers/virtio/virtio_pci_modern.c     | 120 +++++++++++++-
> > > >>>   drivers/virtio/virtio_pci_modern_dev.c |  28 ++++
> > > >>>   drivers/virtio/virtio_ring.c           | 144 +++++++++++-----
> > > >>>   include/linux/virtio.h                 |   1 +
> > > >>>   include/linux/virtio_config.h          |  75 ++++++++-
> > > >>>   include/linux/virtio_pci_modern.h      |   2 +
> > > >>>   include/linux/virtio_ring.h            |  42 +++--
> > > >>>   include/uapi/linux/virtio_config.h     |   7 +-
> > > >>>   include/uapi/linux/virtio_pci.h        |   2 +
> > > >>>   13 files changed, 618 insertions(+), 101 deletions(-)
> > > >>>
> > > >>> --
> > > >>> 2.31.0
> > > >>>
> > >
> > _______________________________________________
> > Virtualization mailing list
> > Virtualization@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>

Re: [PATCH v3 03/17] virtio: queue_reset: struct virtio_config_ops add callbacks for queue_reset

[Jason Wang]

在 2022/2/7 下午3:19, Xuan Zhuo 写道:
> On Mon, 7 Feb 2022 14:45:02 +0800, Jason Wang <jasowang@redhat.com> wrote:
>> 在 2022/1/26 下午3:35, Xuan Zhuo 写道:
>>> Performing reset on a queue is divided into two steps:
>>>
>>> 1. reset_vq: reset one vq
>>> 2. enable_reset_vq: re-enable the reset queue
>>>
>>> In the first step, these tasks will be completed:
>>>       1. notify the hardware queue to reset
>>>       2. recycle the buffer from vq
>>>       3. release the ring of the vq
>>>
>>> The second step is similar to find vqs,
>>
>> Not sure, since find_vqs will usually try to allocate interrupts.
>>
>>
> Yes.
>
>
>>>    passing parameters callback and
>>> name, etc. Based on the original vq, the ring is re-allocated and
>>> configured to the backend.
>>
>> I wonder whether we really have such requirement.
>>
>> For example, do we really have a use case that may change:
>>
>> vq callback, ctx, ring_num or even re-create the virtqueue?
> 1. virtqueue is not recreated
> 2. ring_num can be used to modify ring num by ethtool -G


It looks to me we don't support this right now.


>
> There is really no scene to modify vq callback, ctx, name.
>
> Do you mean we still use the old one instead of adding these parameters?


Yes, I think for driver we need to implement the function that is needed 
for the first user (e.g AF_XDP). If there's no use case, we can leave 
those extension for the future.

Thanks


>
> Thanks.
>
>> Thanks
>>
>>
>>> So add two callbacks reset_vq, enable_reset_vq to struct
>>> virtio_config_ops.
>>>
>>> Add a structure for passing parameters. This will facilitate subsequent
>>> expansion of the parameters of enable reset vq.
>>> There is currently only one default extended parameter ring_num.
>>>
>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>> ---
>>>    include/linux/virtio_config.h | 43 ++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 42 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
>>> index 4d107ad31149..51dd8461d1b6 100644
>>> --- a/include/linux/virtio_config.h
>>> +++ b/include/linux/virtio_config.h
>>> @@ -16,6 +16,44 @@ struct virtio_shm_region {
>>>    	u64 len;
>>>    };
>>>
>>> +typedef void vq_callback_t(struct virtqueue *);
>>> +
>>> +/* virtio_reset_vq: specify parameters for queue_reset
>>> + *
>>> + *	vdev: the device
>>> + *	queue_index: the queue index
>>> + *
>>> + *	free_unused_cb: callback to free unused bufs
>>> + *	data: used by free_unused_cb
>>> + *
>>> + *	callback: callback for the virtqueue, NULL for vq that do not need a
>>> + *	          callback
>>> + *	name: virtqueue names (mainly for debugging), NULL for vq unused by
>>> + *	      driver
>>> + *	ctx: ctx
>>> + *
>>> + *	ring_num: specify ring num for the vq to be re-enabled. 0 means use the
>>> + *	          default value. MUST be a power of 2.
>>> + */
>>> +struct virtio_reset_vq;
>>> +typedef void vq_reset_callback_t(struct virtio_reset_vq *param, void *buf);
>>> +struct virtio_reset_vq {
>>> +	struct virtio_device *vdev;
>>> +	u16 queue_index;
>>> +
>>> +	/* reset vq param */
>>> +	vq_reset_callback_t *free_unused_cb;
>>> +	void *data;
>>> +
>>> +	/* enable reset vq param */
>>> +	vq_callback_t *callback;
>>> +	const char *name;
>>> +	const bool *ctx;
>>> +
>>> +	/* ext enable reset vq param */
>>> +	u16 ring_num;
>>> +};
>>> +
>>>    /**
>>>     * virtio_config_ops - operations for configuring a virtio device
>>>     * Note: Do not assume that a transport implements all of the operations
>>> @@ -74,8 +112,9 @@ struct virtio_shm_region {
>>>     * @set_vq_affinity: set the affinity for a virtqueue (optional).
>>>     * @get_vq_affinity: get the affinity for a virtqueue (optional).
>>>     * @get_shm_region: get a shared memory region based on the index.
>>> + * @reset_vq: reset a queue individually
>>> + * @enable_reset_vq: enable a reset queue
>>>     */
>>> -typedef void vq_callback_t(struct virtqueue *);
>>>    struct virtio_config_ops {
>>>    	void (*enable_cbs)(struct virtio_device *vdev);
>>>    	void (*get)(struct virtio_device *vdev, unsigned offset,
>>> @@ -100,6 +139,8 @@ struct virtio_config_ops {
>>>    			int index);
>>>    	bool (*get_shm_region)(struct virtio_device *vdev,
>>>    			       struct virtio_shm_region *region, u8 id);
>>> +	int (*reset_vq)(struct virtio_reset_vq *param);
>>> +	struct virtqueue *(*enable_reset_vq)(struct virtio_reset_vq *param);
>>>    };
>>>
>>>    /* If driver didn't advertise the feature, it will never appear. */