Re: [PATCH V2 vfio 2/9] virtio-pci: Introduce admin virtqueue
On Tue, Oct 31, 2023 at 03:11:57AM +, Parav Pandit wrote:
>
>
> > From: Michael S. Tsirkin
> > Sent: Tuesday, October 31, 2023 5:02 AM
> >
> > On Mon, Oct 30, 2023 at 06:10:06PM +, Parav Pandit wrote:
> > >
> > >
> > > > From: Michael S. Tsirkin
> > > > Sent: Monday, October 30, 2023 9:29 PM On Mon, Oct 30, 2023 at
> > > > 03:51:40PM +, Parav Pandit wrote:
> > > > >
> > > > >
> > > > > > From: Michael S. Tsirkin
> > > > > > Sent: Monday, October 30, 2023 1:53 AM
> > > > > >
> > > > > > On Sun, Oct 29, 2023 at 05:59:45PM +0200, Yishai Hadas wrote:
> > > > > > > From: Feng Liu
> > > > > > >
> > > > > > > Introduce support for the admin virtqueue. By negotiating
> > > > > > > VIRTIO_F_ADMIN_VQ feature, driver detects capability and
> > > > > > > creates one administration virtqueue. Administration virtqueue
> > > > > > > implementation in virtio pci generic layer, enables multiple
> > > > > > > types of upper layer drivers such as vfio, net, blk to utilize it.
> > > > > > >
> > > > > > > Signed-off-by: Feng Liu
> > > > > > > Reviewed-by: Parav Pandit
> > > > > > > Reviewed-by: Jiri Pirko
> > > > > > > Signed-off-by: Yishai Hadas
> > > > > > > ---
> > > > > > > drivers/virtio/virtio.c| 37 ++--
> > > > > > > drivers/virtio/virtio_pci_common.c | 3 ++
> > > > > > > drivers/virtio/virtio_pci_common.h | 15 ++-
> > > > > > > drivers/virtio/virtio_pci_modern.c | 61
> > +-
> > > > > > > drivers/virtio/virtio_pci_modern_dev.c | 18
> > > > > > > include/linux/virtio_config.h | 4 ++
> > > > > > > include/linux/virtio_pci_modern.h | 5 +++
> > > > > > > 7 files changed, 137 insertions(+), 6 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > > > > index
> > > > > > > 3893dc29eb26..f4080692b351 100644
> > > > > > > --- a/drivers/virtio/virtio.c
> > > > > > > +++ b/drivers/virtio/virtio.c
> > > > > > > @@ -302,9 +302,15 @@ static int virtio_dev_probe(struct device
> > > > > > > *_d)
> > > > > > > if (err)
> > > > > > > goto err;
> > > > > > >
> > > > > > > + if (dev->config->create_avq) {
> > > > > > > + err = dev->config->create_avq(dev);
> > > > > > > + if (err)
> > > > > > > + goto err;
> > > > > > > + }
> > > > > > > +
> > > > > > > err = drv->probe(dev);
> > > > > > > if (err)
> > > > > > > - goto err;
> > > > > > > + goto err_probe;
> > > > > > >
> > > > > > > /* If probe didn't do it, mark device DRIVER_OK ourselves. */
> > > > > > > if (!(dev->config->get_status(dev) &
> > > > > > > VIRTIO_CONFIG_S_DRIVER_OK))
> > > > > >
> > > > > > Hmm I am not all that happy that we are just creating avq
> > unconditionally.
> > > > > > Can't we do it on demand to avoid wasting resources if no one uses
> > > > > > it?
> > > > > >
> > > > > Virtio queues must be enabled before driver_ok as we discussed in
> > > > F_DYNAMIC bit exercise.
> > > > > So creating AQ when first legacy command is invoked, would be too
> > > > > late.
> > > >
> > > > Well we didn't release the spec with AQ so I am pretty sure there
> > > > are no devices using the feature. Do we want to already make an
> > > > exception for AQ and allow creating AQs after DRIVER_OK even without
> > F_DYNAMIC?
> > > >
> > > No. it would abuse the init time config registers for the dynamic things
> > > like
> > this.
> > > For flow filters and others there is need for dynamic q creation with
> > > multiple
> > physical address anyway.
> >
> > That seems like a completely unrelated issue.
> >
> It isn't.
> Driver requirements are:
> 1. Driver needs to dynamically create vqs
> 2. Sometimes this VQ needs to have multiple physical addresses
> 3. Driver needs to create them after driver is fully running, past the
> bootstrap stage using tiny config registers
>
> Device requirements are:
> 1. Not to keep growing 64K VQs *(8+8+8) bytes of address registers + enable
> bit
> 2. Ability to return appropriate error code when fail to create queue
> 3. Above #2
>
> Users of this new infrastructure are eth tx,rx queues, flow filter queues,
> aq, blk rq per cpu.
> AQs are just one of those.
> When a generic infrastructure for this will be built in the spec as we
> started that, all above use cases will be handled.
>
> > > So creating virtqueues dynamically using a generic scheme is desired with
> > new feature bit.
Reducing config registers and returning errors should be handled by
a new transport.
VQ with multiple addresses - I can see how you would maybe only
support that with that new transport?
I think I can guess why you are tying multiple addresses to dynamic VQs -
you suspect that allocating huge half-megabyte VQs dynamically will fail if
triggered on a busy system. Is that the point?
In that case I feel it's a good argument to special-case
admin VQs because there's no real need to make them
huge at the moment - for
Re: [PATCH V2 vfio 6/9] virtio-pci: Introduce APIs to execute legacy IO admin commands
On Sun, Oct 29, 2023 at 05:59:49PM +0200, Yishai Hadas wrote:
> Introduce APIs to execute legacy IO admin commands.
>
> It includes: io_legacy_read/write for both common and the device
> registers, io_legacy_notify_info.
>
> In addition, exposing an API to check whether the legacy IO commands are
> supported. (i.e. virtio_pci_admin_has_legacy_io()).
>
> Those APIs will be used by the next patches from this series.
>
> Signed-off-by: Yishai Hadas
> ---
> drivers/virtio/virtio_pci_common.c | 11 ++
> drivers/virtio/virtio_pci_common.h | 2 +
> drivers/virtio/virtio_pci_modern.c | 241 +
> include/linux/virtio_pci_admin.h | 21 +++
> 4 files changed, 275 insertions(+)
> create mode 100644 include/linux/virtio_pci_admin.h
>
> diff --git a/drivers/virtio/virtio_pci_common.c
> b/drivers/virtio/virtio_pci_common.c
> index 6b4766d5abe6..212d68401d2c 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -645,6 +645,17 @@ static struct pci_driver virtio_pci_driver = {
> .sriov_configure = virtio_pci_sriov_configure,
> };
>
> +struct virtio_device *virtio_pci_vf_get_pf_dev(struct pci_dev *pdev)
> +{
> + struct virtio_pci_device *pf_vp_dev;
> +
> + pf_vp_dev = pci_iov_get_pf_drvdata(pdev, &virtio_pci_driver);
> + if (IS_ERR(pf_vp_dev))
> + return NULL;
> +
> + return &pf_vp_dev->vdev;
> +}
> +
> module_pci_driver(virtio_pci_driver);
>
> MODULE_AUTHOR("Anthony Liguori ");
> diff --git a/drivers/virtio/virtio_pci_common.h
> b/drivers/virtio/virtio_pci_common.h
> index 9e07e556a51a..07d4f863ac44 100644
> --- a/drivers/virtio/virtio_pci_common.h
> +++ b/drivers/virtio/virtio_pci_common.h
> @@ -156,4 +156,6 @@ static inline void virtio_pci_legacy_remove(struct
> virtio_pci_device *vp_dev)
> int virtio_pci_modern_probe(struct virtio_pci_device *);
> void virtio_pci_modern_remove(struct virtio_pci_device *);
>
> +struct virtio_device *virtio_pci_vf_get_pf_dev(struct pci_dev *pdev);
> +
> #endif
> diff --git a/drivers/virtio/virtio_pci_modern.c
> b/drivers/virtio/virtio_pci_modern.c
> index 25e27aa79cab..def0f2de6091 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -15,6 +15,7 @@
> */
>
> #include
> +#include
> #define VIRTIO_PCI_NO_LEGACY
> #define VIRTIO_RING_NO_LEGACY
> #include "virtio_pci_common.h"
> @@ -794,6 +795,246 @@ static void vp_modern_destroy_avq(struct virtio_device
> *vdev)
> vp_dev->del_vq(&vp_dev->admin_vq.info);
> }
>
> +#define VIRTIO_LEGACY_ADMIN_CMD_BITMAP \
> + (BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE) | \
> + BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ) | \
> + BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE) | \
> + BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ) | \
> + BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO))
> +
> +/*
> + * virtio_pci_admin_has_legacy_io - Checks whether the legacy IO
> + * commands are supported
> + * @dev: VF pci_dev
> + *
> + * Returns true on success.
> + */
> +bool virtio_pci_admin_has_legacy_io(struct pci_dev *pdev)
> +{
> + struct virtio_device *virtio_dev = virtio_pci_vf_get_pf_dev(pdev);
> + struct virtio_pci_device *vp_dev;
> +
> + if (!virtio_dev)
> + return false;
> +
> + if (!virtio_has_feature(virtio_dev, VIRTIO_F_ADMIN_VQ))
> + return false;
> +
> + vp_dev = to_vp_device(virtio_dev);
> +
> + if ((vp_dev->admin_vq.supported_cmds & VIRTIO_LEGACY_ADMIN_CMD_BITMAP)
> ==
> + VIRTIO_LEGACY_ADMIN_CMD_BITMAP)
> + return true;
> + return false;
> +}
> +EXPORT_SYMBOL_GPL(virtio_pci_admin_has_legacy_io);
> +
> +static int virtio_pci_admin_legacy_io_write(struct pci_dev *pdev, u16 opcode,
> + u8 offset, u8 size, u8 *buf)
> +{
> + struct virtio_device *virtio_dev = virtio_pci_vf_get_pf_dev(pdev);
> + struct virtio_admin_cmd_legacy_wr_data *data;
> + struct virtio_admin_cmd cmd = {};
> + struct scatterlist data_sg;
> + int vf_id;
> + int ret;
> +
> + if (!virtio_dev)
> + return -ENODEV;
> +
> + vf_id = pci_iov_vf_id(pdev);
> + if (vf_id < 0)
> + return vf_id;
> +
> + data = kzalloc(sizeof(*data) + size, GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->offset = offset;
> + memcpy(data->registers, buf, size);
> + sg_init_one(&data_sg, data, sizeof(*data) + size);
> + cmd.opcode = cpu_to_le16(opcode);
> + cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SRIOV);
> + cmd.group_member_id = cpu_to_le64(vf_id + 1);
> + cmd.data_sg = &data_sg;
> + ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd);
> +
> + kfree(data);
> + return ret;
> +}
> +
> +/*
> + * virtio_pci_admin_legacy_io_write_common - Write common legacy registers
> + * of a member device
> + * @dev: VF pci_dev
> + * @offset: starting byte
Re: [PATCH V2 vfio 9/9] vfio/virtio: Introduce a vfio driver over virtio devices
On Tue, Oct 31, 2023 at 04:17:45PM +0800, Yi Liu wrote: > a dumb question. Is it common between all virtio devices that the legay > interface is in BAR0? It has to be, that is where the legacy driver is looking for it. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 vfio 6/9] virtio-pci: Introduce APIs to execute legacy IO admin commands
On 31/10/2023 10:09, Michael S. Tsirkin wrote:
On Sun, Oct 29, 2023 at 05:59:49PM +0200, Yishai Hadas wrote:
Introduce APIs to execute legacy IO admin commands.
It includes: io_legacy_read/write for both common and the device
registers, io_legacy_notify_info.
In addition, exposing an API to check whether the legacy IO commands are
supported. (i.e. virtio_pci_admin_has_legacy_io()).
Those APIs will be used by the next patches from this series.
Signed-off-by: Yishai Hadas
---
drivers/virtio/virtio_pci_common.c | 11 ++
drivers/virtio/virtio_pci_common.h | 2 +
drivers/virtio/virtio_pci_modern.c | 241 +
include/linux/virtio_pci_admin.h | 21 +++
4 files changed, 275 insertions(+)
create mode 100644 include/linux/virtio_pci_admin.h
diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
index 6b4766d5abe6..212d68401d2c 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -645,6 +645,17 @@ static struct pci_driver virtio_pci_driver = {
.sriov_configure = virtio_pci_sriov_configure,
};
+struct virtio_device *virtio_pci_vf_get_pf_dev(struct pci_dev *pdev)
+{
+ struct virtio_pci_device *pf_vp_dev;
+
+ pf_vp_dev = pci_iov_get_pf_drvdata(pdev, &virtio_pci_driver);
+ if (IS_ERR(pf_vp_dev))
+ return NULL;
+
+ return &pf_vp_dev->vdev;
+}
+
module_pci_driver(virtio_pci_driver);
MODULE_AUTHOR("Anthony Liguori ");
diff --git a/drivers/virtio/virtio_pci_common.h
b/drivers/virtio/virtio_pci_common.h
index 9e07e556a51a..07d4f863ac44 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -156,4 +156,6 @@ static inline void virtio_pci_legacy_remove(struct
virtio_pci_device *vp_dev)
int virtio_pci_modern_probe(struct virtio_pci_device *);
void virtio_pci_modern_remove(struct virtio_pci_device *);
+struct virtio_device *virtio_pci_vf_get_pf_dev(struct pci_dev *pdev);
+
#endif
diff --git a/drivers/virtio/virtio_pci_modern.c
b/drivers/virtio/virtio_pci_modern.c
index 25e27aa79cab..def0f2de6091 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -15,6 +15,7 @@
*/
#include
+#include
#define VIRTIO_PCI_NO_LEGACY
#define VIRTIO_RING_NO_LEGACY
#include "virtio_pci_common.h"
@@ -794,6 +795,246 @@ static void vp_modern_destroy_avq(struct virtio_device
*vdev)
vp_dev->del_vq(&vp_dev->admin_vq.info);
}
+#define VIRTIO_LEGACY_ADMIN_CMD_BITMAP \
+ (BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE) | \
+BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ) | \
+BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE) | \
+BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ) | \
+BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO))
+
+/*
+ * virtio_pci_admin_has_legacy_io - Checks whether the legacy IO
+ * commands are supported
+ * @dev: VF pci_dev
+ *
+ * Returns true on success.
+ */
+bool virtio_pci_admin_has_legacy_io(struct pci_dev *pdev)
+{
+ struct virtio_device *virtio_dev = virtio_pci_vf_get_pf_dev(pdev);
+ struct virtio_pci_device *vp_dev;
+
+ if (!virtio_dev)
+ return false;
+
+ if (!virtio_has_feature(virtio_dev, VIRTIO_F_ADMIN_VQ))
+ return false;
+
+ vp_dev = to_vp_device(virtio_dev);
+
+ if ((vp_dev->admin_vq.supported_cmds & VIRTIO_LEGACY_ADMIN_CMD_BITMAP)
==
+ VIRTIO_LEGACY_ADMIN_CMD_BITMAP)
+ return true;
+ return false;
+}
+EXPORT_SYMBOL_GPL(virtio_pci_admin_has_legacy_io);
+
+static int virtio_pci_admin_legacy_io_write(struct pci_dev *pdev, u16 opcode,
+ u8 offset, u8 size, u8 *buf)
+{
+ struct virtio_device *virtio_dev = virtio_pci_vf_get_pf_dev(pdev);
+ struct virtio_admin_cmd_legacy_wr_data *data;
+ struct virtio_admin_cmd cmd = {};
+ struct scatterlist data_sg;
+ int vf_id;
+ int ret;
+
+ if (!virtio_dev)
+ return -ENODEV;
+
+ vf_id = pci_iov_vf_id(pdev);
+ if (vf_id < 0)
+ return vf_id;
+
+ data = kzalloc(sizeof(*data) + size, GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->offset = offset;
+ memcpy(data->registers, buf, size);
+ sg_init_one(&data_sg, data, sizeof(*data) + size);
+ cmd.opcode = cpu_to_le16(opcode);
+ cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SRIOV);
+ cmd.group_member_id = cpu_to_le64(vf_id + 1);
+ cmd.data_sg = &data_sg;
+ ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd);
+
+ kfree(data);
+ return ret;
+}
+
+/*
+ * virtio_pci_admin_legacy_io_write_common - Write common legacy registers
+ * of a member device
+ * @dev: VF pci_dev
+ * @offset: starting byte offset within the registers to write to
+ * @size: size of the data to write
+ * @buf: buffer which holds the data
+ *
Re: [PATCH V2 vfio 6/9] virtio-pci: Introduce APIs to execute legacy IO admin commands
On Tue, Oct 31, 2023 at 10:30:41AM +0200, Yishai Hadas wrote: > > And further, is caller expected not to invoke several of these > > in parallel on the same device? If yes this needs to be > > documented. I don't see where does vfio enforce this if yes. > Please have a look at virtiovf_issue_legacy_rw_cmd() from patch #9. > > It has a lock on its VF device to serialize access to the bar, it includes > calling this API. > > Yishai OK so if caller must serialize accesses then please document this assumption. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH V2 vfio 2/9] virtio-pci: Introduce admin virtqueue
> From: Michael S. Tsirkin
> Sent: Tuesday, October 31, 2023 1:29 PM
>
> On Tue, Oct 31, 2023 at 03:11:57AM +, Parav Pandit wrote:
> >
> >
> > > From: Michael S. Tsirkin
> > > Sent: Tuesday, October 31, 2023 5:02 AM
> > >
> > > On Mon, Oct 30, 2023 at 06:10:06PM +, Parav Pandit wrote:
> > > >
> > > >
> > > > > From: Michael S. Tsirkin
> > > > > Sent: Monday, October 30, 2023 9:29 PM On Mon, Oct 30, 2023 at
> > > > > 03:51:40PM +, Parav Pandit wrote:
> > > > > >
> > > > > >
> > > > > > > From: Michael S. Tsirkin
> > > > > > > Sent: Monday, October 30, 2023 1:53 AM
> > > > > > >
> > > > > > > On Sun, Oct 29, 2023 at 05:59:45PM +0200, Yishai Hadas wrote:
> > > > > > > > From: Feng Liu
> > > > > > > >
> > > > > > > > Introduce support for the admin virtqueue. By negotiating
> > > > > > > > VIRTIO_F_ADMIN_VQ feature, driver detects capability and
> > > > > > > > creates one administration virtqueue. Administration
> > > > > > > > virtqueue implementation in virtio pci generic layer,
> > > > > > > > enables multiple types of upper layer drivers such as vfio,
> > > > > > > > net, blk
> to utilize it.
> > > > > > > >
> > > > > > > > Signed-off-by: Feng Liu
> > > > > > > > Reviewed-by: Parav Pandit
> > > > > > > > Reviewed-by: Jiri Pirko
> > > > > > > > Signed-off-by: Yishai Hadas
> > > > > > > > ---
> > > > > > > > drivers/virtio/virtio.c| 37 ++--
> > > > > > > > drivers/virtio/virtio_pci_common.c | 3 ++
> > > > > > > > drivers/virtio/virtio_pci_common.h | 15 ++-
> > > > > > > > drivers/virtio/virtio_pci_modern.c | 61
> > > +-
> > > > > > > > drivers/virtio/virtio_pci_modern_dev.c | 18
> > > > > > > > include/linux/virtio_config.h | 4 ++
> > > > > > > > include/linux/virtio_pci_modern.h | 5 +++
> > > > > > > > 7 files changed, 137 insertions(+), 6 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/virtio/virtio.c
> > > > > > > > b/drivers/virtio/virtio.c index
> > > > > > > > 3893dc29eb26..f4080692b351 100644
> > > > > > > > --- a/drivers/virtio/virtio.c
> > > > > > > > +++ b/drivers/virtio/virtio.c
> > > > > > > > @@ -302,9 +302,15 @@ static int virtio_dev_probe(struct device
> *_d)
> > > > > > > > if (err)
> > > > > > > > goto err;
> > > > > > > >
> > > > > > > > + if (dev->config->create_avq) {
> > > > > > > > + err = dev->config->create_avq(dev);
> > > > > > > > + if (err)
> > > > > > > > + goto err;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > err = drv->probe(dev);
> > > > > > > > if (err)
> > > > > > > > - goto err;
> > > > > > > > + goto err_probe;
> > > > > > > >
> > > > > > > > /* If probe didn't do it, mark device DRIVER_OK
> > > > > > > > ourselves. */
> > > > > > > > if (!(dev->config->get_status(dev) &
> > > > > > > > VIRTIO_CONFIG_S_DRIVER_OK))
> > > > > > >
> > > > > > > Hmm I am not all that happy that we are just creating avq
> > > unconditionally.
> > > > > > > Can't we do it on demand to avoid wasting resources if no one uses
> it?
> > > > > > >
> > > > > > Virtio queues must be enabled before driver_ok as we discussed
> > > > > > in
> > > > > F_DYNAMIC bit exercise.
> > > > > > So creating AQ when first legacy command is invoked, would be too
> late.
> > > > >
> > > > > Well we didn't release the spec with AQ so I am pretty sure
> > > > > there are no devices using the feature. Do we want to already
> > > > > make an exception for AQ and allow creating AQs after DRIVER_OK
> > > > > even without
> > > F_DYNAMIC?
> > > > >
> > > > No. it would abuse the init time config registers for the dynamic
> > > > things like
> > > this.
> > > > For flow filters and others there is need for dynamic q creation
> > > > with multiple
> > > physical address anyway.
> > >
> > > That seems like a completely unrelated issue.
> > >
> > It isn't.
> > Driver requirements are:
> > 1. Driver needs to dynamically create vqs 2. Sometimes this VQ needs
> > to have multiple physical addresses 3. Driver needs to create them
> > after driver is fully running, past the bootstrap stage using tiny
> > config registers
> >
> > Device requirements are:
> > 1. Not to keep growing 64K VQs *(8+8+8) bytes of address registers +
> > enable bit 2. Ability to return appropriate error code when fail to
> > create queue 3. Above #2
> >
> > Users of this new infrastructure are eth tx,rx queues, flow filter queues,
> > aq, blk
> rq per cpu.
> > AQs are just one of those.
> > When a generic infrastructure for this will be built in the spec as we
> > started
> that, all above use cases will be handled.
> >
> > > > So creating virtqueues dynamically using a generic scheme is
> > > > desired with
> > > new feature bit.
>
> Reducing config registers and returning errors should be handled by a new
> transport.
> VQ with multiple addresses - I can see
[PATCH] vdpa_sim_blk: allocate the buffer zeroed
Deleting and recreating a device can lead to having the same
content as the old device, so let's always allocate buffers
completely zeroed out.
Fixes: abebb16254b3 ("vdpa_sim_blk: support shared backend")
Suggested-by: Qing Wang
Signed-off-by: Stefano Garzarella
---
drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
index b3a3cb165795..b137f3679343 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -437,7 +437,7 @@ static int vdpasim_blk_dev_add(struct vdpa_mgmt_dev *mdev,
const char *name,
if (blk->shared_backend) {
blk->buffer = shared_buffer;
} else {
- blk->buffer = kvmalloc(VDPASIM_BLK_CAPACITY << SECTOR_SHIFT,
+ blk->buffer = kvzalloc(VDPASIM_BLK_CAPACITY << SECTOR_SHIFT,
GFP_KERNEL);
if (!blk->buffer) {
ret = -ENOMEM;
@@ -495,7 +495,7 @@ static int __init vdpasim_blk_init(void)
goto parent_err;
if (shared_backend) {
- shared_buffer = kvmalloc(VDPASIM_BLK_CAPACITY << SECTOR_SHIFT,
+ shared_buffer = kvzalloc(VDPASIM_BLK_CAPACITY << SECTOR_SHIFT,
GFP_KERNEL);
if (!shared_buffer) {
ret = -ENOMEM;
--
2.41.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio_pci: move structure to a header
These are guest/host interfaces so belong in the header
where e.g. qemu will know to find them.
Note: we added a new structure as opposed to extending existing one
because someone might be relying on the size of the existing structure
staying unchanged. Add a warning to avoid using sizeof.
Signed-off-by: Michael S. Tsirkin
---
drivers/virtio/virtio_pci_modern_dev.c | 7 ---
include/linux/virtio_pci_modern.h | 7 ---
include/uapi/linux/virtio_pci.h| 11 +++
3 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/virtio/virtio_pci_modern_dev.c
b/drivers/virtio/virtio_pci_modern_dev.c
index e2a1fe7bb66c..7de8b1ebabac 100644
--- a/drivers/virtio/virtio_pci_modern_dev.c
+++ b/drivers/virtio/virtio_pci_modern_dev.c
@@ -294,9 +294,10 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
err = -EINVAL;
mdev->common = vp_modern_map_capability(mdev, common,
- sizeof(struct virtio_pci_common_cfg), 4,
- 0, sizeof(struct
virtio_pci_modern_common_cfg),
- &mdev->common_len, NULL);
+ sizeof(struct virtio_pci_common_cfg), 4, 0,
+ offsetofend(struct virtio_pci_modern_common_cfg,
+ queue_reset),
+ &mdev->common_len, NULL);
if (!mdev->common)
goto err_map_common;
mdev->isr = vp_modern_map_capability(mdev, isr, sizeof(u8), 1,
diff --git a/include/linux/virtio_pci_modern.h
b/include/linux/virtio_pci_modern.h
index d0f2797420f7..a09e13a577a9 100644
--- a/include/linux/virtio_pci_modern.h
+++ b/include/linux/virtio_pci_modern.h
@@ -5,13 +5,6 @@
#include
#include
-struct virtio_pci_modern_common_cfg {
- struct virtio_pci_common_cfg cfg;
-
- __le16 queue_notify_data; /* read-write */
- __le16 queue_reset; /* read-write */
-};
-
/**
* struct virtio_pci_modern_device - info for modern PCI virtio
* @pci_dev: Ptr to the PCI device struct
diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
index f703afc7ad31..44f4dd2add18 100644
--- a/include/uapi/linux/virtio_pci.h
+++ b/include/uapi/linux/virtio_pci.h
@@ -166,6 +166,17 @@ struct virtio_pci_common_cfg {
__le32 queue_used_hi; /* read-write */
};
+/*
+ * Warning: do not use sizeof on this: use offsetofend for
+ * specific fields you need.
+ */
+struct virtio_pci_modern_common_cfg {
+ struct virtio_pci_common_cfg cfg;
+
+ __le16 queue_notify_data; /* read-write */
+ __le16 queue_reset; /* read-write */
+};
+
/* Fields in VIRTIO_PCI_CAP_PCI_CFG: */
struct virtio_pci_cfg_cap {
struct virtio_pci_cap cap;
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4] ALSA: virtio: use ack callback
On Fri, Oct 27, 2023 at 10:10:30AM -0400, Michael S. Tsirkin wrote:
On Fri, Oct 27, 2023 at 12:18:00PM +0200, Stefano Garzarella wrote:
On Fri, Oct 27, 2023 at 11:27:40AM +0200, Takashi Iwai wrote:
> On Wed, 25 Oct 2023 11:49:19 +0200,
> Matias Ezequiel Vara Larsen wrote:
> >
> > This commit uses the ack() callback to determine when a buffer has been
> > updated, then exposes it to guest.
> >
> > The current mechanism splits a dma buffer into descriptors that are
> > exposed to the device. This dma buffer is shared with the user
> > application. When the device consumes a buffer, the driver moves the
> > request from the used ring to available ring.
> >
> > The driver exposes the buffer to the device without knowing if the
> > content has been updated from the user. The section 2.8.21.1 of the
> > virtio spec states that: "The device MAY access the descriptor chains
> > the driver created and the memory they refer to immediately". If the
> > device picks up buffers from the available ring just after it is
> > notified, it happens that the content may be old.
> >
> > When the ack() callback is invoked, the driver exposes only the buffers
> > that have already been updated, i.e., enqueued in the available ring.
> > Thus, the device always picks up a buffer that is updated.
> >
> > For capturing, the driver starts by exposing all the available buffers
> > to device. After device updates the content of a buffer, it enqueues it
> > in the used ring. It is only after the ack() for capturing is issued
> > that the driver re-enqueues the buffer in the available ring.
> >
> > Co-developed-by: Anton Yakovlev
> > Signed-off-by: Anton Yakovlev
> > Signed-off-by: Matias Ezequiel Vara Larsen
>
> Applied now to for-next branch.
Cool, thanks for that!
I just wonder if we should CC stable since we are fixing a virtio
specification violation.
@Michael what do you think?
Thanks,
Stefano
Acked-by: Michael S. Tsirkin
Fixes: de3a9980d8c3 ("ALSA: virtio: add virtio sound driver")
The patch is too big for stable - more than 100 lines added. See:
Documentation/process/stable-kernel-rules.rst
Yeah, I see, thanks for sharing!
Stefano
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vdpa_sim_blk: allocate the buffer zeroed
On Wed, Nov 1, 2023 at 1:52 AM Eugenio Perez Martin wrote:
>
> On Tue, Oct 31, 2023 at 3:44 PM Stefano Garzarella
> wrote:
> >
> > Deleting and recreating a device can lead to having the same
> > content as the old device, so let's always allocate buffers
> > completely zeroed out.
> >
> > Fixes: abebb16254b3 ("vdpa_sim_blk: support shared backend")
> > Suggested-by: Qing Wang
>
> Acked-by: Eugenio Pérez
Acked-by: Jason Wang
Thanks
>
> > Signed-off-by: Stefano Garzarella
> > ---
> > drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > index b3a3cb165795..b137f3679343 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > @@ -437,7 +437,7 @@ static int vdpasim_blk_dev_add(struct vdpa_mgmt_dev
> > *mdev, const char *name,
> > if (blk->shared_backend) {
> > blk->buffer = shared_buffer;
> > } else {
> > - blk->buffer = kvmalloc(VDPASIM_BLK_CAPACITY << SECTOR_SHIFT,
> > + blk->buffer = kvzalloc(VDPASIM_BLK_CAPACITY << SECTOR_SHIFT,
> >GFP_KERNEL);
> > if (!blk->buffer) {
> > ret = -ENOMEM;
> > @@ -495,7 +495,7 @@ static int __init vdpasim_blk_init(void)
> > goto parent_err;
> >
> > if (shared_backend) {
> > - shared_buffer = kvmalloc(VDPASIM_BLK_CAPACITY <<
> > SECTOR_SHIFT,
> > + shared_buffer = kvzalloc(VDPASIM_BLK_CAPACITY <<
> > SECTOR_SHIFT,
> > GFP_KERNEL);
> > if (!shared_buffer) {
> > ret = -ENOMEM;
> > --
> > 2.41.0
> >
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_pci: move structure to a header
On Wed, Nov 1, 2023 at 12:20 AM Michael S. Tsirkin wrote:
>
> These are guest/host interfaces so belong in the header
> where e.g. qemu will know to find them.
> Note: we added a new structure as opposed to extending existing one
> because someone might be relying on the size of the existing structure
> staying unchanged. Add a warning to avoid using sizeof.
>
> Signed-off-by: Michael S. Tsirkin
Acked-by: Jason Wang
Thanks
> ---
> drivers/virtio/virtio_pci_modern_dev.c | 7 ---
> include/linux/virtio_pci_modern.h | 7 ---
> include/uapi/linux/virtio_pci.h| 11 +++
> 3 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_modern_dev.c
> b/drivers/virtio/virtio_pci_modern_dev.c
> index e2a1fe7bb66c..7de8b1ebabac 100644
> --- a/drivers/virtio/virtio_pci_modern_dev.c
> +++ b/drivers/virtio/virtio_pci_modern_dev.c
> @@ -294,9 +294,10 @@ int vp_modern_probe(struct virtio_pci_modern_device
> *mdev)
>
> err = -EINVAL;
> mdev->common = vp_modern_map_capability(mdev, common,
> - sizeof(struct virtio_pci_common_cfg), 4,
> - 0, sizeof(struct
> virtio_pci_modern_common_cfg),
> - &mdev->common_len, NULL);
> + sizeof(struct virtio_pci_common_cfg), 4, 0,
> + offsetofend(struct virtio_pci_modern_common_cfg,
> + queue_reset),
> + &mdev->common_len, NULL);
> if (!mdev->common)
> goto err_map_common;
> mdev->isr = vp_modern_map_capability(mdev, isr, sizeof(u8), 1,
> diff --git a/include/linux/virtio_pci_modern.h
> b/include/linux/virtio_pci_modern.h
> index d0f2797420f7..a09e13a577a9 100644
> --- a/include/linux/virtio_pci_modern.h
> +++ b/include/linux/virtio_pci_modern.h
> @@ -5,13 +5,6 @@
> #include
> #include
>
> -struct virtio_pci_modern_common_cfg {
> - struct virtio_pci_common_cfg cfg;
> -
> - __le16 queue_notify_data; /* read-write */
> - __le16 queue_reset; /* read-write */
> -};
> -
> /**
> * struct virtio_pci_modern_device - info for modern PCI virtio
> * @pci_dev: Ptr to the PCI device struct
> diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
> index f703afc7ad31..44f4dd2add18 100644
> --- a/include/uapi/linux/virtio_pci.h
> +++ b/include/uapi/linux/virtio_pci.h
> @@ -166,6 +166,17 @@ struct virtio_pci_common_cfg {
> __le32 queue_used_hi; /* read-write */
> };
>
> +/*
> + * Warning: do not use sizeof on this: use offsetofend for
> + * specific fields you need.
> + */
> +struct virtio_pci_modern_common_cfg {
> + struct virtio_pci_common_cfg cfg;
> +
> + __le16 queue_notify_data; /* read-write */
> + __le16 queue_reset; /* read-write */
> +};
> +
> /* Fields in VIRTIO_PCI_CAP_PCI_CFG: */
> struct virtio_pci_cfg_cap {
> struct virtio_pci_cap cap;
> --
> MST
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_pci: move structure to a header
On Tue, 31 Oct 2023 12:19:54 -0400, "Michael S. Tsirkin"
wrote:
> These are guest/host interfaces so belong in the header
> where e.g. qemu will know to find them.
> Note: we added a new structure as opposed to extending existing one
> because someone might be relying on the size of the existing structure
> staying unchanged. Add a warning to avoid using sizeof.
>
> Signed-off-by: Michael S. Tsirkin
Reviewed-by: Xuan Zhuo
> ---
> drivers/virtio/virtio_pci_modern_dev.c | 7 ---
> include/linux/virtio_pci_modern.h | 7 ---
> include/uapi/linux/virtio_pci.h| 11 +++
> 3 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_modern_dev.c
> b/drivers/virtio/virtio_pci_modern_dev.c
> index e2a1fe7bb66c..7de8b1ebabac 100644
> --- a/drivers/virtio/virtio_pci_modern_dev.c
> +++ b/drivers/virtio/virtio_pci_modern_dev.c
> @@ -294,9 +294,10 @@ int vp_modern_probe(struct virtio_pci_modern_device
> *mdev)
>
> err = -EINVAL;
> mdev->common = vp_modern_map_capability(mdev, common,
> - sizeof(struct virtio_pci_common_cfg), 4,
> - 0, sizeof(struct
> virtio_pci_modern_common_cfg),
> - &mdev->common_len, NULL);
> + sizeof(struct virtio_pci_common_cfg), 4, 0,
> + offsetofend(struct virtio_pci_modern_common_cfg,
> + queue_reset),
> + &mdev->common_len, NULL);
> if (!mdev->common)
> goto err_map_common;
> mdev->isr = vp_modern_map_capability(mdev, isr, sizeof(u8), 1,
> diff --git a/include/linux/virtio_pci_modern.h
> b/include/linux/virtio_pci_modern.h
> index d0f2797420f7..a09e13a577a9 100644
> --- a/include/linux/virtio_pci_modern.h
> +++ b/include/linux/virtio_pci_modern.h
> @@ -5,13 +5,6 @@
> #include
> #include
>
> -struct virtio_pci_modern_common_cfg {
> - struct virtio_pci_common_cfg cfg;
> -
> - __le16 queue_notify_data; /* read-write */
> - __le16 queue_reset; /* read-write */
> -};
> -
> /**
> * struct virtio_pci_modern_device - info for modern PCI virtio
> * @pci_dev: Ptr to the PCI device struct
> diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
> index f703afc7ad31..44f4dd2add18 100644
> --- a/include/uapi/linux/virtio_pci.h
> +++ b/include/uapi/linux/virtio_pci.h
> @@ -166,6 +166,17 @@ struct virtio_pci_common_cfg {
> __le32 queue_used_hi; /* read-write */
> };
>
> +/*
> + * Warning: do not use sizeof on this: use offsetofend for
> + * specific fields you need.
> + */
> +struct virtio_pci_modern_common_cfg {
> + struct virtio_pci_common_cfg cfg;
> +
> + __le16 queue_notify_data; /* read-write */
> + __le16 queue_reset; /* read-write */
> +};
> +
> /* Fields in VIRTIO_PCI_CAP_PCI_CFG: */
> struct virtio_pci_cfg_cap {
> struct virtio_pci_cap cap;
> --
> MST
>
> ___
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-XXX] vhost-vdpa: fix use after free in vhost_vdpa_probe()
On Fri, Oct 27, 2023 at 8:13 PM Dan Carpenter wrote:
>
> The put_device() calls vhost_vdpa_release_dev() which calls
> ida_simple_remove() and frees "v". So this call to
> ida_simple_remove() is a use after free and a double free.
>
> Fixes: ebe6a354fa7e ("vhost-vdpa: Call ida_simple_remove() when failed")
> Signed-off-by: Dan Carpenter
Acked-by: Jason Wang
Thanks
> ---
> drivers/vhost/vdpa.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 9a2343c45df0..1aa67729e188 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -1511,7 +1511,6 @@ static int vhost_vdpa_probe(struct vdpa_device *vdpa)
>
> err:
> put_device(&v->dev);
> - ida_simple_remove(&vhost_vdpa_ida, v->minor);
> return r;
> }
>
> --
> 2.42.0
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-XXX] vhost-vdpa: fix use after free in vhost_vdpa_probe()
On Fri, Oct 27, 2023 at 03:12:54PM +0300, Dan Carpenter wrote:
> The put_device() calls vhost_vdpa_release_dev() which calls
> ida_simple_remove() and frees "v". So this call to
> ida_simple_remove() is a use after free and a double free.
>
> Fixes: ebe6a354fa7e ("vhost-vdpa: Call ida_simple_remove() when failed")
> Signed-off-by: Dan Carpenter
queued, thanks!
> ---
> drivers/vhost/vdpa.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 9a2343c45df0..1aa67729e188 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -1511,7 +1511,6 @@ static int vhost_vdpa_probe(struct vdpa_device *vdpa)
>
> err:
> put_device(&v->dev);
> - ida_simple_remove(&vhost_vdpa_ida, v->minor);
> return r;
> }
>
> --
> 2.42.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
