Re: [PATCH v14] i2c: virtio: add a virtio i2c frontend driver
On 09-07-21, 10:25, Jie Deng wrote:
> Add an I2C bus driver for virtio para-virtualization.
>
> The controller can be emulated by the backend driver in
> any device model software by following the virtio protocol.
>
> The device specification can be found on
> https://lists.oasis-open.org/archives/virtio-comment/202101/msg8.html.
>
> By following the specification, people may implement different
> backend drivers to emulate different controllers according to
> their needs.
>
> Co-developed-by: Conghui Chen
> Signed-off-by: Conghui Chen
> Signed-off-by: Jie Deng
> ---
> Changes v13 -> v14
> - Put the headers in virtio_i2c.h in alphabetical order.
> - Dropped I2C_FUNC_SMBUS_QUICK support.
> - Dropped few unnecessary variables and checks.
> - Use "num" everywhere instead of num or nr, to be consistent.
> - Added few comments which make the design more clear.
Thanks a lot for following this up so far :)
> +static int virtio_i2c_prepare_reqs(struct virtqueue *vq,
> +struct virtio_i2c_req *reqs,
> +struct i2c_msg *msgs, int num)
> +{
> + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
> + int i;
> +
> + for (i = 0; i < num; i++) {
> + int outcnt = 0, incnt = 0;
> +
> + /*
> + * We don't support 0 length messages and so masked out
> + * I2C_FUNC_SMBUS_QUICK in virtio_i2c_func().
> + */
> + if (!msgs[i].len)
> + break;
> +
> + /*
> + * Only 7-bit mode supported for this moment. For the address
> + * format, Please check the Virtio I2C Specification.
> + */
> + reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
> +
> + if (i != num - 1)
> + reqs[i].out_hdr.flags =
> cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT);
> +
> + sg_init_one(&out_hdr, &reqs[i].out_hdr,
> sizeof(reqs[i].out_hdr));
> + sgs[outcnt++] = &out_hdr;
> +
> + reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1);
> + if (!reqs[i].buf)
> + break;
> +
> + sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
> +
> + if (msgs[i].flags & I2C_M_RD)
> + sgs[outcnt + incnt++] = &msg_buf;
> + else
> + sgs[outcnt++] = &msg_buf;
> +
> + sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr));
> + sgs[outcnt + incnt++] = &in_hdr;
> +
> + if (virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i],
> GFP_KERNEL)) {
> + i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], false);
> + break;
> + }
> + }
> +
> + return i;
> +}
Wolfram, in case you wonder why we don't error out early as discussed earlier,
then ...
> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> +int num)
> +{
...
> + /*
> + * For the case where count < num, i.e. we weren't able to queue all the
> + * msgs, ideally we should abort right away and return early, but some
> + * of the messages are already sent to the remote I2C controller and the
> + * virtqueue will be left in undefined state in that case. We kick the
> + * remote here to clear the virtqueue, so we can try another set of
> + * messages later on.
> + */
... here is the reasoning for that.
Please see if you can still get it merged into 5.14-rc1/2. Thanks.
Reviewed-by: Viresh Kumar
Tested-by: Viresh Kumar
--
viresh
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V3 2/2] vDPA/ifcvf: implement management netlink framework for ifcvf
在 2021/7/6 上午10:36, Zhu Lingshan 写道:
This commit implements the management netlink framework for ifcvf,
including register and add / remove a device
It works with iproute2:
[root@localhost lszhu]# vdpa mgmtdev show -jp
{
"mgmtdev": {
"pci/:01:00.5": {
"supported_classes": [ "net" ]
},
"pci/:01:00.6": {
"supported_classes": [ "net" ]
}
}
}
[root@localhost lszhu]# vdpa dev add mgmtdev pci/:01:00.5 name vdpa0
[root@localhost lszhu]# vdpa dev add mgmtdev pci/:01:00.6 name vdpa1
Signed-off-by: Zhu Lingshan
Acked-by: Jason Wang
---
drivers/vdpa/ifcvf/ifcvf_base.h | 6 ++
drivers/vdpa/ifcvf/ifcvf_main.c | 154
2 files changed, 124 insertions(+), 36 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index ded1b1b5fb13..e5251fcbb200 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -104,6 +104,12 @@ struct ifcvf_lm_cfg {
struct ifcvf_vring_lm_cfg vring_lm_cfg[IFCVF_MAX_QUEUE_PAIRS];
};
+struct ifcvf_vdpa_mgmt_dev {
+ struct vdpa_mgmt_dev mdev;
+ struct ifcvf_adapter *adapter;
+ struct pci_dev *pdev;
+};
+
int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *dev);
int ifcvf_start_hw(struct ifcvf_hw *hw);
void ifcvf_stop_hw(struct ifcvf_hw *hw);
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 5f70ab1283a0..c72d9b36e4a0 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -218,7 +218,7 @@ static void ifcvf_vdpa_set_status(struct vdpa_device
*vdpa_dev, u8 status)
int ret;
vf = vdpa_to_vf(vdpa_dev);
- adapter = dev_get_drvdata(vdpa_dev->dev.parent);
+ adapter = vdpa_to_adapter(vdpa_dev);
status_old = ifcvf_get_status(vf);
if (status_old == status)
@@ -442,6 +442,16 @@ static const struct vdpa_config_ops ifc_vdpa_ops = {
.set_config_cb = ifcvf_vdpa_set_config_cb,
};
+static struct virtio_device_id id_table_net[] = {
+ {VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID},
+ {0},
+};
+
+static struct virtio_device_id id_table_blk[] = {
+ {VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID},
+ {0},
+};
+
static u32 get_dev_type(struct pci_dev *pdev)
{
u32 dev_type;
@@ -462,48 +472,30 @@ static u32 get_dev_type(struct pci_dev *pdev)
return dev_type;
}
-static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name)
{
- struct device *dev = &pdev->dev;
+ struct ifcvf_vdpa_mgmt_dev *ifcvf_mgmt_dev;
struct ifcvf_adapter *adapter;
+ struct pci_dev *pdev;
struct ifcvf_hw *vf;
+ struct device *dev;
int ret, i;
- ret = pcim_enable_device(pdev);
- if (ret) {
- IFCVF_ERR(pdev, "Failed to enable device\n");
- return ret;
- }
-
- ret = pcim_iomap_regions(pdev, BIT(0) | BIT(2) | BIT(4),
-IFCVF_DRIVER_NAME);
- if (ret) {
- IFCVF_ERR(pdev, "Failed to request MMIO region\n");
- return ret;
- }
-
- ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
- if (ret) {
- IFCVF_ERR(pdev, "No usable DMA configuration\n");
- return ret;
- }
-
- ret = devm_add_action_or_reset(dev, ifcvf_free_irq_vectors, pdev);
- if (ret) {
- IFCVF_ERR(pdev,
- "Failed for adding devres for freeing irq vectors\n");
- return ret;
- }
+ ifcvf_mgmt_dev = container_of(mdev, struct ifcvf_vdpa_mgmt_dev, mdev);
+ if (ifcvf_mgmt_dev->adapter)
+ return -EOPNOTSUPP;
+ pdev = ifcvf_mgmt_dev->pdev;
+ dev = &pdev->dev;
adapter = vdpa_alloc_device(struct ifcvf_adapter, vdpa,
- dev, &ifc_vdpa_ops, NULL);
- if (adapter == NULL) {
+ dev, &ifc_vdpa_ops, name);
+ if (!adapter) {
IFCVF_ERR(pdev, "Failed to allocate vDPA structure");
return -ENOMEM;
}
- pci_set_master(pdev);
- pci_set_drvdata(pdev, adapter);
+ ifcvf_mgmt_dev->adapter = adapter;
+ pci_set_drvdata(pdev, ifcvf_mgmt_dev);
vf = &adapter->vf;
vf->dev_type = get_dev_type(pdev);
@@ -523,9 +515,10 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
vf->hw_features = ifcvf_get_hw_features(vf);
- ret = vdpa_register_device(&adapter->vdpa, IFCVF_MAX_QUEUE_PAIRS * 2);
+ adapter->vdpa.mdev = &ifcvf_mgmt_dev->mdev;
+ ret = _vdpa_register_device(&adapter->vdpa, IFCVF_MAX_QUEUE_PAIRS * 2);
if (ret) {
- IFCVF_ERR(pdev, "Failed to register ifcvf to vdpa bus");
+ IFCVF_ERR(pdev, "Failed to
Re: [PATCH V3 1/2] vDPA/ifcvf: introduce get_dev_type() which returns virtio dev id
在 2021/7/6 上午10:36, Zhu Lingshan 写道:
This commit introduces a new function get_dev_type() which returns
the virtio device id of a device, to avoid duplicated code.
Signed-off-by: Zhu Lingshan
Acked-by: Jason Wang
---
drivers/vdpa/ifcvf/ifcvf_main.c | 34 -
1 file changed, 21 insertions(+), 13 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index bc1d59f316d1..5f70ab1283a0 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -442,6 +442,26 @@ static const struct vdpa_config_ops ifc_vdpa_ops = {
.set_config_cb = ifcvf_vdpa_set_config_cb,
};
+static u32 get_dev_type(struct pci_dev *pdev)
+{
+ u32 dev_type;
+
+ /* This drirver drives both modern virtio devices and transitional
+* devices in modern mode.
+* vDPA requires feature bit VIRTIO_F_ACCESS_PLATFORM,
+* so legacy devices and transitional devices in legacy
+* mode will not work for vDPA, this driver will not
+* drive devices with legacy interface.
+*/
+
+ if (pdev->device < 0x1040)
+ dev_type = pdev->subsystem_device;
+ else
+ dev_type = pdev->device - 0x1040;
+
+ return dev_type;
+}
+
static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
{
struct device *dev = &pdev->dev;
@@ -486,19 +506,7 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
pci_set_drvdata(pdev, adapter);
vf = &adapter->vf;
-
- /* This drirver drives both modern virtio devices and transitional
-* devices in modern mode.
-* vDPA requires feature bit VIRTIO_F_ACCESS_PLATFORM,
-* so legacy devices and transitional devices in legacy
-* mode will not work for vDPA, this driver will not
-* drive devices with legacy interface.
-*/
- if (pdev->device < 0x1040)
- vf->dev_type = pdev->subsystem_device;
- else
- vf->dev_type = pdev->device - 0x1040;
-
+ vf->dev_type = get_dev_type(pdev);
vf->base = pcim_iomap_table(pdev);
adapter->pdev = pdev;
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v14] i2c: virtio: add a virtio i2c frontend driver
Add an I2C bus driver for virtio para-virtualization.
The controller can be emulated by the backend driver in
any device model software by following the virtio protocol.
The device specification can be found on
https://lists.oasis-open.org/archives/virtio-comment/202101/msg8.html.
By following the specification, people may implement different
backend drivers to emulate different controllers according to
their needs.
Co-developed-by: Conghui Chen
Signed-off-by: Conghui Chen
Signed-off-by: Jie Deng
---
Changes v13 -> v14
- Put the headers in virtio_i2c.h in alphabetical order.
- Dropped I2C_FUNC_SMBUS_QUICK support.
- Dropped few unnecessary variables and checks.
- Use "num" everywhere instead of num or nr, to be consistent.
- Added few comments which make the design more clear.
Changes v12 -> v13
- Use _BITUL() instead of BIT().
- Rename "virtio_i2c_send_reqs" to "virtio_i2c_prepare_reqs".
- Optimize the return value of "virtio_i2c_complete_reqs".
Changes v11 -> v12
- Do not sent msg_buf for zero-length request.
- Send requests to host only if all the number of transfers requested
prepared successfully.
- Remove the line #include in virtio_i2c.h.
Changes v10 -> v11
- Remove vi->adap.class = I2C_CLASS_DEPRECATED.
- Use #ifdef CONFIG_PM_SLEEP to replace the "__maybe_unused".
- Remove "struct mutex lock" in "struct virtio_i2c".
- Support zero-length request.
- Remove unnecessary logs.
- Remove vi->adap.timeout = HZ / 10, just use the default value.
- Use BIT(0) to define VIRTIO_I2C_FLAGS_FAIL_NEXT.
- Add the virtio_device index to adapter's naming mechanism.
drivers/i2c/busses/Kconfig | 11 ++
drivers/i2c/busses/Makefile | 3 +
drivers/i2c/busses/i2c-virtio.c | 285
include/uapi/linux/virtio_i2c.h | 41 ++
include/uapi/linux/virtio_ids.h | 1 +
5 files changed, 341 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-virtio.c
create mode 100644 include/uapi/linux/virtio_i2c.h
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 10acece..e47616a 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -21,6 +21,17 @@ config I2C_ALI1535
This driver can also be built as a module. If so, the module
will be called i2c-ali1535.
+config I2C_VIRTIO
+ tristate "Virtio I2C Adapter"
+ select VIRTIO
+ help
+ If you say yes to this option, support will be included for the virtio
+ I2C adapter driver. The hardware can be emulated by any device model
+ software according to the virtio protocol.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-virtio.
+
config I2C_ALI1563
tristate "ALI 1563"
depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 69e9963..9843756 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -147,4 +147,7 @@ obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
obj-$(CONFIG_I2C_FSI) += i2c-fsi.o
+# VIRTIO I2C host controller driver
+obj-$(CONFIG_I2C_VIRTIO) += i2c-virtio.o
+
ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
new file mode 100644
index 000..0139cdc
--- /dev/null
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -0,0 +1,285 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Virtio I2C Bus Driver
+ *
+ * The Virtio I2C Specification:
+ *
https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex
+ *
+ * Copyright (c) 2021 Intel Corporation. All rights reserved.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/**
+ * struct virtio_i2c - virtio I2C data
+ * @vdev: virtio device for this controller
+ * @completion: completion of virtio I2C message
+ * @adap: I2C adapter for this controller
+ * @vq: the virtio virtqueue for communication
+ */
+struct virtio_i2c {
+ struct virtio_device *vdev;
+ struct completion completion;
+ struct i2c_adapter adap;
+ struct virtqueue *vq;
+};
+
+/**
+ * struct virtio_i2c_req - the virtio I2C request structure
+ * @out_hdr: the OUT header of the virtio I2C message
+ * @buf: the buffer into which data is read, or from which it's written
+ * @in_hdr: the IN header of the virtio I2C message
+ */
+struct virtio_i2c_req {
+ struct virtio_i2c_out_hdr out_hdr cacheline_aligned;
+ uint8_t *bufcacheline_aligned;
+ struct virtio_i2c_in_hdr in_hdr cacheline_aligned;
+};
+
+static void virtio_i2c_msg_done(struct virtqueue *vq)
+{
+ struct virtio_i2c *vi = vq->vdev->priv;
+
+
Re: [PATCH v2 4/4] bus: Make remove callback return void
On Tue, 6 Jul 2021 at 17:53, Uwe Kleine-König wrote: > > The driver core ignores the return value of this callback because there > is only little it can do when a device disappears. > > This is the final bit of a long lasting cleanup quest where several > buses were converted to also return void from their remove callback. > Additionally some resource leaks were fixed that were caused by drivers > returning an error code in the expectation that the driver won't go > away. > > With struct bus_type::remove returning void it's prevented that newly > implemented buses return an ignored error code and so don't anticipate > wrong expectations for driver authors. > > Acked-by: Russell King (Oracle) (For ARM, Amba > and related parts) > Acked-by: Mark Brown > Acked-by: Chen-Yu Tsai (for drivers/bus/sunxi-rsb.c) > Acked-by: Pali Rohár > Acked-by: Mauro Carvalho Chehab (for drivers/media) > Acked-by: Hans de Goede (For drivers/platform) > Acked-by: Alexandre Belloni > Acked-By: Vinod Koul > Acked-by: Juergen Gross (For Xen) > Acked-by: Lee Jones (For drivers/mfd) > Acked-by: Johannes Thumshirn (For drivers/mcb) > Acked-by: Johan Hovold > Acked-by: Srinivas Kandagatla (For > drivers/slimbus) > Acked-by: Kirti Wankhede (For drivers/vfio) > Acked-by: Maximilian Luz > Acked-by: Heikki Krogerus (For ulpi and > typec) > Acked-by: Samuel Iglesias Gonsálvez (For ipack) > Reviewed-by: Tom Rix (For fpga) > Acked-by: Geoff Levand (For ps3) > Signed-off-by: Uwe Kleine-König Acked-by: Ulf Hansson # For MMC [...] Kind regards Uffe ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v8 10/10] Documentation: Add documentation for VDUSE
On Wed, Jul 07, 2021 at 05:09:13PM +0800, Yongji Xie wrote: > On Tue, Jul 6, 2021 at 6:22 PM Stefan Hajnoczi wrote: > > > > On Tue, Jul 06, 2021 at 11:04:18AM +0800, Yongji Xie wrote: > > > On Mon, Jul 5, 2021 at 8:50 PM Stefan Hajnoczi > > > wrote: > > > > > > > > On Mon, Jul 05, 2021 at 11:36:15AM +0800, Jason Wang wrote: > > > > > > > > > > 在 2021/7/4 下午5:49, Yongji Xie 写道: > > > > > > > > OK, I get you now. Since the VIRTIO specification says "Device > > > > > > > > configuration space is generally used for rarely-changing or > > > > > > > > initialization-time parameters". I assume the > > > > > > > > VDUSE_DEV_SET_CONFIG > > > > > > > > ioctl should not be called frequently. > > > > > > > The spec uses MUST and other terms to define the precise > > > > > > > requirements. > > > > > > > Here the language (especially the word "generally") is weaker and > > > > > > > means > > > > > > > there may be exceptions. > > > > > > > > > > > > > > Another type of access that doesn't work with the > > > > > > > VDUSE_DEV_SET_CONFIG > > > > > > > approach is reads that have side-effects. For example, imagine a > > > > > > > field > > > > > > > containing an error code if the device encounters a problem > > > > > > > unrelated to > > > > > > > a specific virtqueue request. Reading from this field resets the > > > > > > > error > > > > > > > code to 0, saving the driver an extra configuration space write > > > > > > > access > > > > > > > and possibly race conditions. It isn't possible to implement those > > > > > > > semantics suing VDUSE_DEV_SET_CONFIG. It's another corner case, > > > > > > > but it > > > > > > > makes me think that the interface does not allow full VIRTIO > > > > > > > semantics. > > > > > > > > > > > > > > > Note that though you're correct, my understanding is that config > > > > > space is > > > > > not suitable for this kind of error propagating. And it would be very > > > > > hard > > > > > to implement such kind of semantic in some transports. Virtqueue > > > > > should be > > > > > much better. As Yong Ji quoted, the config space is used for > > > > > "rarely-changing or intialization-time parameters". > > > > > > > > > > > > > > > > Agreed. I will use VDUSE_DEV_GET_CONFIG in the next version. And to > > > > > > handle the message failure, I'm going to add a return value to > > > > > > virtio_config_ops.get() and virtio_cread_* API so that the error can > > > > > > be propagated to the virtio device driver. Then the virtio-blk > > > > > > device > > > > > > driver can be modified to handle that. > > > > > > > > > > > > Jason and Stefan, what do you think of this way? > > > > > > > > Why does VDUSE_DEV_GET_CONFIG need to support an error return value? > > > > > > > > > > We add a timeout and return error in case userspace never replies to > > > the message. > > > > > > > The VIRTIO spec provides no way for the device to report errors from > > > > config space accesses. > > > > > > > > The QEMU virtio-pci implementation returns -1 from invalid > > > > virtio_config_read*() and silently discards virtio_config_write*() > > > > accesses. > > > > > > > > VDUSE can take the same approach with > > > > VDUSE_DEV_GET_CONFIG/VDUSE_DEV_SET_CONFIG. > > > > > > > > > > I noticed that virtio_config_read*() only returns -1 when we access a > > > invalid field. But in the VDUSE case, VDUSE_DEV_GET_CONFIG might fail > > > when we access a valid field. Not sure if it's ok to silently ignore > > > this kind of error. > > > > That's a good point but it's a general VIRTIO issue. Any device > > implementation (QEMU userspace, hardware vDPA, etc) can fail, so the > > VIRTIO specification needs to provide a way for the driver to detect > > this. > > > > If userspace violates the contract then VDUSE needs to mark the device > > broken. QEMU's device emulation does something similar with the > > vdev->broken flag. > > > > The VIRTIO Device Status field DEVICE_NEEDS_RESET bit can be set by > > vDPA/VDUSE to indicate that the device is not operational and must be > > reset. > > > > It might be a solution. But DEVICE_NEEDS_RESET is not implemented > currently. So I'm thinking whether it's ok to add a check of > DEVICE_NEEDS_RESET status bit in probe function of virtio device > driver (e.g. virtio-blk driver). Then VDUSE can make use of it to fail > device initailization when configuration space access failed. Okay. Stefan signature.asc Description: PGP signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v8 10/10] Documentation: Add documentation for VDUSE
On Thu, Jul 08, 2021 at 12:17:56PM +0800, Jason Wang wrote: > > 在 2021/7/7 下午11:54, Stefan Hajnoczi 写道: > > On Wed, Jul 07, 2021 at 05:24:08PM +0800, Jason Wang wrote: > > > 在 2021/7/7 下午4:55, Stefan Hajnoczi 写道: > > > > On Wed, Jul 07, 2021 at 11:43:28AM +0800, Jason Wang wrote: > > > > > 在 2021/7/7 上午1:11, Stefan Hajnoczi 写道: > > > > > > On Tue, Jul 06, 2021 at 09:08:26PM +0800, Jason Wang wrote: > > > > > > > On Tue, Jul 6, 2021 at 6:15 PM Stefan Hajnoczi > > > > > > > wrote: > > > > > > > > On Tue, Jul 06, 2021 at 10:34:33AM +0800, Jason Wang wrote: > > > > > > > > > 在 2021/7/5 下午8:49, Stefan Hajnoczi 写道: > > > > > > > > > > On Mon, Jul 05, 2021 at 11:36:15AM +0800, Jason Wang wrote: > > > > > > > > > > > 在 2021/7/4 下午5:49, Yongji Xie 写道: > > > > > > > > > > > > > > OK, I get you now. Since the VIRTIO specification > > > > > > > > > > > > > > says "Device > > > > > > > > > > > > > > configuration space is generally used for > > > > > > > > > > > > > > rarely-changing or > > > > > > > > > > > > > > initialization-time parameters". I assume the > > > > > > > > > > > > > > VDUSE_DEV_SET_CONFIG > > > > > > > > > > > > > > ioctl should not be called frequently. > > > > > > > > > > > > > The spec uses MUST and other terms to define the > > > > > > > > > > > > > precise requirements. > > > > > > > > > > > > > Here the language (especially the word "generally") > > > > > > > > > > > > > is weaker and means > > > > > > > > > > > > > there may be exceptions. > > > > > > > > > > > > > > > > > > > > > > > > > > Another type of access that doesn't work with the > > > > > > > > > > > > > VDUSE_DEV_SET_CONFIG > > > > > > > > > > > > > approach is reads that have side-effects. For > > > > > > > > > > > > > example, imagine a field > > > > > > > > > > > > > containing an error code if the device encounters a > > > > > > > > > > > > > problem unrelated to > > > > > > > > > > > > > a specific virtqueue request. Reading from this field > > > > > > > > > > > > > resets the error > > > > > > > > > > > > > code to 0, saving the driver an extra configuration > > > > > > > > > > > > > space write access > > > > > > > > > > > > > and possibly race conditions. It isn't possible to > > > > > > > > > > > > > implement those > > > > > > > > > > > > > semantics suing VDUSE_DEV_SET_CONFIG. It's another > > > > > > > > > > > > > corner case, but it > > > > > > > > > > > > > makes me think that the interface does not allow full > > > > > > > > > > > > > VIRTIO semantics. > > > > > > > > > > > Note that though you're correct, my understanding is that > > > > > > > > > > > config space is > > > > > > > > > > > not suitable for this kind of error propagating. And it > > > > > > > > > > > would be very hard > > > > > > > > > > > to implement such kind of semantic in some transports. > > > > > > > > > > > Virtqueue should be > > > > > > > > > > > much better. As Yong Ji quoted, the config space is used > > > > > > > > > > > for > > > > > > > > > > > "rarely-changing or intialization-time parameters". > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Agreed. I will use VDUSE_DEV_GET_CONFIG in the next > > > > > > > > > > > > version. And to > > > > > > > > > > > > handle the message failure, I'm going to add a return > > > > > > > > > > > > value to > > > > > > > > > > > > virtio_config_ops.get() and virtio_cread_* API so that > > > > > > > > > > > > the error can > > > > > > > > > > > > be propagated to the virtio device driver. Then the > > > > > > > > > > > > virtio-blk device > > > > > > > > > > > > driver can be modified to handle that. > > > > > > > > > > > > > > > > > > > > > > > > Jason and Stefan, what do you think of this way? > > > > > > > > > > Why does VDUSE_DEV_GET_CONFIG need to support an error > > > > > > > > > > return value? > > > > > > > > > > > > > > > > > > > > The VIRTIO spec provides no way for the device to report > > > > > > > > > > errors from > > > > > > > > > > config space accesses. > > > > > > > > > > > > > > > > > > > > The QEMU virtio-pci implementation returns -1 from invalid > > > > > > > > > > virtio_config_read*() and silently discards > > > > > > > > > > virtio_config_write*() > > > > > > > > > > accesses. > > > > > > > > > > > > > > > > > > > > VDUSE can take the same approach with > > > > > > > > > > VDUSE_DEV_GET_CONFIG/VDUSE_DEV_SET_CONFIG. > > > > > > > > > > > > > > > > > > > > > I'd like to stick to the current assumption thich > > > > > > > > > > > get_config won't fail. > > > > > > > > > > > That is to say, > > > > > > > > > > > > > > > > > > > > > > 1) maintain a config in the kernel, make sure the config > > > > > > > > > > > space read can > > > > > > > > > > > always succeed > > > > > > > > > > > 2) introduce an ioctl for the vduse usersapce to update > > > > > > > > > > > the config space. > > > > > > > > > > > 3) we can synchronize with the vduse userspace during > > > > > > > > > > > set_config > > > > > > > > > > > > > > > > > >
