From: Jason Wang <jasowang@redhat.com> To: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Cc: "Michael S. Tsirkin" <mst@redhat.com>, "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, Jesper Dangaard Brouer <hawk@kernel.org>, John Fastabend <john.fastabend@gmail.com>, bpf@vger.kernel.org, virtualization <virtualization@lists.linux-foundation.org>, netdev <netdev@vger.kernel.org> Subject: Re: [PATCH v3 01/17] virtio_pci: struct virtio_pci_common_cfg add queue_notify_data Date: Tue, 8 Feb 2022 11:36:23 +0800 [thread overview] Message-ID: <CACGkMEshTp8vSP9=pKj82y8+DDQFu9tFAk1EGhMZLvXUE-OSEA@mail.gmail.com> (raw) In-Reply-To: <1644290712.5535257-1-xuanzhuo@linux.alibaba.com> 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: */ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
WARNING: multiple messages have this Message-ID (diff)
From: Jason Wang <jasowang@redhat.com> To: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Cc: Jesper Dangaard Brouer <hawk@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, "Michael S. Tsirkin" <mst@redhat.com>, netdev <netdev@vger.kernel.org>, John Fastabend <john.fastabend@gmail.com>, Alexei Starovoitov <ast@kernel.org>, virtualization <virtualization@lists.linux-foundation.org>, Jakub Kicinski <kuba@kernel.org>, bpf@vger.kernel.org, "David S. Miller" <davem@davemloft.net> Subject: Re: [PATCH v3 01/17] virtio_pci: struct virtio_pci_common_cfg add queue_notify_data Date: Tue, 8 Feb 2022 11:36:23 +0800 [thread overview] Message-ID: <CACGkMEshTp8vSP9=pKj82y8+DDQFu9tFAk1EGhMZLvXUE-OSEA@mail.gmail.com> (raw) In-Reply-To: <1644290712.5535257-1-xuanzhuo@linux.alibaba.com> 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: */ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2022-02-08 3:36 UTC|newest] Thread overview: 78+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-01-26 7:35 [PATCH v3 00/17] virtio pci support VIRTIO_F_RING_RESET Xuan Zhuo 2022-01-26 7:35 ` Xuan Zhuo 2022-01-26 7:35 ` [PATCH v3 01/17] virtio_pci: struct virtio_pci_common_cfg add queue_notify_data Xuan Zhuo 2022-01-26 7:35 ` Xuan Zhuo 2022-02-07 3:41 ` Jason Wang 2022-02-07 3:41 ` Jason Wang 2022-02-07 6:04 ` Xuan Zhuo 2022-02-07 8:06 ` Jason Wang 2022-02-07 8:06 ` Jason Wang 2022-02-08 2:17 ` Xuan Zhuo 2022-02-08 3:03 ` Jason Wang 2022-02-08 3:03 ` Jason Wang 2022-02-08 3:18 ` Xuan Zhuo 2022-02-08 3:24 ` Jason Wang 2022-02-08 3:24 ` Jason Wang 2022-02-08 3:25 ` Xuan Zhuo 2022-02-08 3:36 ` Jason Wang [this message] 2022-02-08 3:36 ` Jason Wang 2022-01-26 7:35 ` [PATCH v3 02/17] virtio: queue_reset: add VIRTIO_F_RING_RESET Xuan Zhuo 2022-01-26 7:35 ` Xuan Zhuo 2022-01-26 7:35 ` [PATCH v3 03/17] virtio: queue_reset: struct virtio_config_ops add callbacks for queue_reset Xuan Zhuo 2022-01-26 7:35 ` Xuan Zhuo 2022-02-07 6:45 ` Jason Wang 2022-02-07 6:45 ` Jason Wang 2022-02-07 7:19 ` Xuan Zhuo 2022-02-08 2:58 ` Jason Wang 2022-02-08 2:58 ` Jason Wang 2022-02-08 3:00 ` Xuan Zhuo 2022-01-26 7:35 ` [PATCH v3 04/17] virtio: queue_reset: add helper Xuan Zhuo 2022-01-26 7:35 ` Xuan Zhuo 2022-01-26 7:35 ` [PATCH v3 05/17] vritio_ring: queue_reset: extract the release function of the vq ring Xuan Zhuo 2022-01-26 7:35 ` Xuan Zhuo 2022-01-26 7:35 ` [PATCH v3 06/17] virtio_ring: queue_reset: split: add __vring_init_virtqueue() Xuan Zhuo 2022-01-26 7:35 ` Xuan Zhuo 2022-01-26 7:35 ` [PATCH v3 07/17] virtio_ring: queue_reset: split: support enable reset queue Xuan Zhuo 2022-01-26 7:35 ` Xuan Zhuo 2022-01-26 7:35 ` [PATCH v3 08/17] virtio_ring: queue_reset: packed: " Xuan Zhuo 2022-01-26 7:35 ` Xuan Zhuo 2022-01-26 7:35 ` [PATCH v3 09/17] virtio_ring: queue_reset: add vring_reset_virtqueue() Xuan Zhuo 2022-01-26 7:35 ` Xuan Zhuo 2022-01-26 7:35 ` [PATCH v3 10/17] virtio_pci: queue_reset: update struct virtio_pci_common_cfg and option functions Xuan Zhuo 2022-01-26 7:35 ` Xuan Zhuo 2022-01-26 7:35 ` [PATCH v3 11/17] virtio_pci: queue_reset: release vq by vp_dev->vqs Xuan Zhuo 2022-01-26 7:35 ` Xuan Zhuo 2022-01-26 7:35 ` [PATCH v3 12/17] virtio_pci: queue_reset: setup_vq use vring_setup_virtqueue() Xuan Zhuo 2022-01-26 7:35 ` Xuan Zhuo 2022-01-26 7:35 ` [PATCH v3 13/17] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET Xuan Zhuo 2022-01-26 7:35 ` Xuan Zhuo 2022-02-07 6:57 ` Jason Wang 2022-02-07 6:57 ` Jason Wang 2022-02-07 7:59 ` Xuan Zhuo 2022-02-08 2:55 ` Jason Wang 2022-02-08 2:55 ` Jason Wang 2022-02-08 6:47 ` xuanzhuo 2022-02-08 6:47 ` xuanzhuo 2022-02-08 7:35 ` Xuan Zhuo 2022-02-09 5:44 ` Jason Wang 2022-02-09 5:44 ` Jason Wang 2022-02-09 6:05 ` Xuan Zhuo 2022-02-09 6:05 ` Xuan Zhuo 2022-01-26 7:35 ` [PATCH v3 14/17] virtio_net: virtnet_tx_timeout() fix style Xuan Zhuo 2022-01-26 7:35 ` Xuan Zhuo 2022-01-26 7:35 ` [PATCH v3 15/17] virtio_net: virtnet_tx_timeout() stop ref sq->vq Xuan Zhuo 2022-01-26 7:35 ` Xuan Zhuo 2022-01-26 7:35 ` [PATCH v3 16/17] virtio_net: split free_unused_bufs() Xuan Zhuo 2022-01-26 7:35 ` Xuan Zhuo 2022-01-26 7:35 ` [PATCH v3 17/17] virtio_net: support pair disable/enable Xuan Zhuo 2022-01-26 7:35 ` Xuan Zhuo 2022-02-07 3:39 ` [PATCH v3 00/17] virtio pci support VIRTIO_F_RING_RESET Jason Wang 2022-02-07 3:39 ` Jason Wang 2022-02-07 6:02 ` Xuan Zhuo 2022-02-08 2:59 ` Jason Wang 2022-02-08 2:59 ` Jason Wang 2022-02-08 3:14 ` Xuan Zhuo 2022-02-08 7:51 ` Xuan Zhuo 2022-02-08 7:51 ` Xuan Zhuo 2022-02-09 5:39 ` Jason Wang 2022-02-09 5:39 ` Jason Wang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CACGkMEshTp8vSP9=pKj82y8+DDQFu9tFAk1EGhMZLvXUE-OSEA@mail.gmail.com' \ --to=jasowang@redhat.com \ --cc=ast@kernel.org \ --cc=bpf@vger.kernel.org \ --cc=daniel@iogearbox.net \ --cc=davem@davemloft.net \ --cc=hawk@kernel.org \ --cc=john.fastabend@gmail.com \ --cc=kuba@kernel.org \ --cc=mst@redhat.com \ --cc=netdev@vger.kernel.org \ --cc=virtualization@lists.linux-foundation.org \ --cc=xuanzhuo@linux.alibaba.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.