Re: [PATCH v2 04/10] drm/aperture: Add infrastructure for aperture ownership
Hi
Am 09.04.21 um 11:22 schrieb Daniel Vetter:
Is it that easy? simepldrm's detach function has code to synchronize with
concurrent hotplug removals. If we can use drm_dev_unplug() for everything,
I'm all for it.
Uh, I should have looked at the code instead of just asking silly
questions :-)
Now I'm even more scared, and also more convinced that we're recreating
a
bad version of some of the core driver model concepts.
I think the ideal option here would be if drm_aperture could unload
(unbind really) the platform driver for us, through the driver model. Then
there's only one place that keeps track whether the driver is unbound or
not. I'm not sure whether this can be done fully generic on a struct
device, or whether we need special code for each type. Since atm we only
have simpledrm we can just specialize on platform_device and it's good
enough.
I meanwhile found that calling platform_device_unregister() is the right
thing to do. It is like a hot-unplug event. It's simple to implement and
removes the generic device as well. Any memory ranges for the generic
device are gone as well. Only the native driver's native device will
remain. That's better than the existing simplefb driver.
Which unregister function to call still driver-specific, so I kept the
callback.
Best regards
Thomas
I think best here would be to Cc: gregkh on this patch and the simpledrm
->detach implementatation, and ask for his feedback as driver model
maintainer. Maybe if you could hack together the platform_device unbind
path as proof of concept would be even better.
Either way, this is really tricky.
-Daniel
Best regards
Thomas
Or maybe we should tie this more into the struct device mode and force an
unload that way? That way devm cleanup would work as one expects, and
avoid the need for anything specific (hopefully) in this detach callback.
Just feels a bit like we're reinventing half of the driver model here,
badly.
+ * };
+ *
+ * static int acquire_framebuffers(struct drm_device *dev, struct pci_dev
*pdev)
+ * {
+ * resource_size_t start, len;
+ * struct drm_aperture *ap;
+ *
+ * base = pci_resource_start(pdev, 0);
+ * size = pci_resource_len(pdev, 0);
+ *
+ * ap = devm_acquire_aperture(dev, base, size, &ap_funcs);
+ * if (IS_ERR(ap))
+ * return PTR_ERR(ap);
+ *
+ * return 0;
+ * }
+ *
+ * static int probe(struct pci_dev *pdev)
+ * {
+ * struct drm_device *dev;
+ * int ret;
+ *
+ * // ... Initialize the device...
+ * dev = devm_drm_dev_alloc();
+ * ...
+ *
+ * // ... and acquire ownership of the framebuffer.
+ * ret = acquire_framebuffers(dev, pdev);
+ * if (ret)
+ * return ret;
+ *
+ * drm_dev_register();
+ *
+ * return 0;
+ * }
+ *
+ * The generic driver is now subject to forced removal by other drivers. This
+ * is when the detach function in struct &drm_aperture_funcs comes into play.
+ * When a driver calls drm_fb_helper_remove_conflicting_framebuffers() et al
+ * for the registered framebuffer range, the DRM core calls struct
+ * &drm_aperture_funcs.detach and the generic driver has to onload itself. It
+ * may not access the device's registers, framebuffer memory, ROM, etc after
+ * detach returned. If the driver supports hotplugging, detach can be treated
+ * like an unplug event.
+ *
+ * .. code-block:: c
+ *
+ * static void detach_from_device(struct drm_device *dev,
+ *resource_size_t base,
+ *resource_size_t size)
+ * {
+ * // Signal unplug
+ * drm_dev_unplug(dev);
+ *
+ * // Maybe do other clean-up operations
+ * ...
+ * }
+ *
+ * static struct drm_aperture_funcs ap_funcs = {
+ * .detach = detach_from_device,
+ * };
+ */
+
+/**
+ * struct drm_aperture - Represents a DRM framebuffer aperture
+ *
+ * This structure has no public fields.
+ */
+struct drm_aperture {
+ struct drm_device *dev;
+ resource_size_t base;
+ resource_size_t size;
+
+ const struct drm_aperture_funcs *funcs;
+
+ struct list_head lh;
+};
+
+static LIST_HEAD(drm_apertures);
+
+static DEFINE_MUTEX(drm_apertures_lock);
+
+static bool overlap(resource_size_t base1, resource_size_t end1,
+ resource_size_t base2, resource_size_t end2)
+{
+ return (base1 < end2) && (end1 > base2);
+}
+
+static void devm_aperture_acquire_release(void *data)
+{
+ struct drm_aperture *ap = data;
+ bool detached = !ap->dev;
+
+ if (!detached)
Uh this needs a comment that if ap->dev is NULL then we're called from
drm_aperture_detach_drivers() and hence the lock is already held.
+ mutex_lock(&drm_apertures_lock);
and an
else
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 2021/4/15 14:45, Viresh Kumar wrote: On 23-03-21, 10:27, Arnd Bergmann wrote: I usually recommend the use of __maybe_unused for the suspend/resume callbacks for drivers that use SIMPLE_DEV_PM_OPS() or similar helpers that hide the exact conditions under which the functions get called. In this driver, there is an explicit #ifdef in the reference to the functions, so it would make sense to use the same #ifdef around the definition. Jie, I was talking about this comment when I said I was expecting a new version. I think you still need to make this change. I didn't forget this. It is a very small change. I'm not sure if the maintainer Wolfram has any comments so that I can address them together in one version. Thanks. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/3] vDPA/ifcvf: enable Intel C5000X-PL virtio-block for vDPA
On 4/15/2021 2:31 PM, Jason Wang wrote:
在 2021/4/15 下午1:55, Zhu Lingshan 写道:
On 4/15/2021 11:34 AM, Jason Wang wrote:
在 2021/4/14 下午5:18, Zhu Lingshan 写道:
This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-block
for vDPA.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_base.h | 17 -
drivers/vdpa/ifcvf/ifcvf_main.c | 10 +-
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
b/drivers/vdpa/ifcvf/ifcvf_base.h
index 1c04cd256fa7..8b403522bf06 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
@@ -28,7 +29,12 @@
#define C5000X_PL_SUBSYS_VENDOR_ID 0x8086
#define C5000X_PL_SUBSYS_DEVICE_ID 0x0001
-#define IFCVF_SUPPORTED_FEATURES \
+#define C5000X_PL_BLK_VENDOR_ID 0x1AF4
+#define C5000X_PL_BLK_DEVICE_ID 0x1001
+#define C5000X_PL_BLK_SUBSYS_VENDOR_ID 0x8086
+#define C5000X_PL_BLK_SUBSYS_DEVICE_ID 0x0002
+
+#define IFCVF_NET_SUPPORTED_FEATURES \
((1ULL << VIRTIO_NET_F_MAC) | \
(1ULL << VIRTIO_F_ANY_LAYOUT) | \
(1ULL << VIRTIO_F_VERSION_1) | \
@@ -37,6 +43,15 @@
(1ULL << VIRTIO_F_ACCESS_PLATFORM) | \
(1ULL << VIRTIO_NET_F_MRG_RXBUF))
+#define IFCVF_BLK_SUPPORTED_FEATURES \
+ ((1ULL << VIRTIO_BLK_F_SIZE_MAX) | \
+ (1ULL << VIRTIO_BLK_F_SEG_MAX) | \
+ (1ULL << VIRTIO_BLK_F_BLK_SIZE) | \
+ (1ULL << VIRTIO_BLK_F_TOPOLOGY) | \
+ (1ULL << VIRTIO_BLK_F_MQ) | \
+ (1ULL << VIRTIO_F_VERSION_1) | \
+ (1ULL << VIRTIO_F_ACCESS_PLATFORM))
I think we've discussed this sometime in the past but what's the
reason for such whitelist consider there's already a get_features()
implemention?
E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or
VIRTIO_F_RING_PACKED?
Thanks
The reason is some feature bits are supported in the device but not
supported by the driver, e.g, for virtio-net, mq & cq implementation
is not ready in the driver.
I understand the case of virtio-net but I wonder why we need this for
block where we don't vq cvq.
Thanks
This is still a subset of the feature bits read from hardware, I leave
it here to code consistently, and indicate what we support clearly.
Are you suggesting remove this feature bits list and just use what we
read from hardware?
Thansk
Thanks!
+
/* Only one queue pair for now. */
#define IFCVF_MAX_QUEUE_PAIRS 1
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index 99b0a6b4c227..9b6a38b798fa 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -171,7 +171,11 @@ static u64 ifcvf_vdpa_get_features(struct
vdpa_device *vdpa_dev)
struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
u64 features;
- features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
+ if (vf->dev_type == VIRTIO_ID_NET)
+ features = ifcvf_get_features(vf) &
IFCVF_NET_SUPPORTED_FEATURES;
+
+ if (vf->dev_type == VIRTIO_ID_BLOCK)
+ features = ifcvf_get_features(vf) &
IFCVF_BLK_SUPPORTED_FEATURES;
return features;
}
@@ -509,6 +513,10 @@ static struct pci_device_id ifcvf_pci_ids[] = {
C5000X_PL_DEVICE_ID,
C5000X_PL_SUBSYS_VENDOR_ID,
C5000X_PL_SUBSYS_DEVICE_ID) },
+ { PCI_DEVICE_SUB(C5000X_PL_BLK_VENDOR_ID,
+ C5000X_PL_BLK_DEVICE_ID,
+ C5000X_PL_BLK_SUBSYS_VENDOR_ID,
+ C5000X_PL_BLK_SUBSYS_DEVICE_ID) },
{ 0 },
};
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] vDPA/ifcvf: deduce VIRTIO device ID when probe
On 4/15/2021 2:30 PM, Jason Wang wrote:
在 2021/4/15 下午1:52, Zhu Lingshan 写道:
On 4/15/2021 11:30 AM, Jason Wang wrote:
在 2021/4/14 下午5:18, Zhu Lingshan 写道:
This commit deduces VIRTIO device ID as device type when probe,
then ifcvf_vdpa_get_device_id() can simply return the ID.
ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
can work properly based on the device ID.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
b/drivers/vdpa/ifcvf/ifcvf_base.h
index b2eeb16b9c2c..1c04cd256fa7 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -84,6 +84,7 @@ struct ifcvf_hw {
u32 notify_off_multiplier;
u64 req_features;
u64 hw_features;
+ u32 dev_type;
struct virtio_pci_common_cfg __iomem *common_cfg;
void __iomem *net_cfg;
struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index 44d7586019da..99b0a6b4c227 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct
vdpa_device *vdpa_dev)
static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
{
- struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
- struct pci_dev *pdev = adapter->pdev;
- u32 ret = -ENODEV;
-
- if (pdev->device < 0x1000 || pdev->device > 0x107f)
- return ret;
-
- if (pdev->device < 0x1040)
- ret = pdev->subsystem_device;
- else
- ret = pdev->device -0x1040;
+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
- return ret;
+ return vf->dev_type;
}
static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
@@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
pci_set_drvdata(pdev, adapter);
vf = &adapter->vf;
+ if (pdev->device < 0x1000 || pdev->device > 0x107f)
+ return -EOPNOTSUPP;
+
+ if (pdev->device < 0x1040)
+ vf->dev_type = pdev->subsystem_device;
+ else
+ vf->dev_type = pdev->device - 0x1040;
So a question here, is the device a transtional device or modern one?
If it's a transitonal one, can it swtich endianess automatically or
not?
Thanks
Hi Jason,
This driver should drive both modern and transitional devices as we
discussed before.
If it's a transitional one, it will act as a modern device by
default, legacy mode is a fail-over path.
Note that legacy driver use native endian, support legacy driver
requires the device to know native endian which I'm not sure your
device can do that.
Thanks
Yes, legacy requires guest native endianess, I think we don't need to
worry about this because our transitional device should work in modern
mode by
default(legacy mode is the failover path we will never reach,
get_features will fail if no ACCESS_PLATFORM), we don't support legacy
device in vDPA.
Thanks
For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it must
in modern mode.
I think we don't need to worry about endianess for legacy mode.
Thanks
Zhu Lingshan
+
vf->base = pcim_iomap_table(pdev);
adapter->pdev = pdev;
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/3] vDPA/ifcvf: enable Intel C5000X-PL virtio-block for vDPA
在 2021/4/15 下午1:55, Zhu Lingshan 写道:
On 4/15/2021 11:34 AM, Jason Wang wrote:
在 2021/4/14 下午5:18, Zhu Lingshan 写道:
This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-block
for vDPA.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_base.h | 17 -
drivers/vdpa/ifcvf/ifcvf_main.c | 10 +-
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
b/drivers/vdpa/ifcvf/ifcvf_base.h
index 1c04cd256fa7..8b403522bf06 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
@@ -28,7 +29,12 @@
#define C5000X_PL_SUBSYS_VENDOR_ID 0x8086
#define C5000X_PL_SUBSYS_DEVICE_ID 0x0001
-#define IFCVF_SUPPORTED_FEATURES \
+#define C5000X_PL_BLK_VENDOR_ID 0x1AF4
+#define C5000X_PL_BLK_DEVICE_ID 0x1001
+#define C5000X_PL_BLK_SUBSYS_VENDOR_ID 0x8086
+#define C5000X_PL_BLK_SUBSYS_DEVICE_ID 0x0002
+
+#define IFCVF_NET_SUPPORTED_FEATURES \
((1ULL << VIRTIO_NET_F_MAC) | \
(1ULL << VIRTIO_F_ANY_LAYOUT) | \
(1ULL << VIRTIO_F_VERSION_1) | \
@@ -37,6 +43,15 @@
(1ULL << VIRTIO_F_ACCESS_PLATFORM) | \
(1ULL << VIRTIO_NET_F_MRG_RXBUF))
+#define IFCVF_BLK_SUPPORTED_FEATURES \
+ ((1ULL << VIRTIO_BLK_F_SIZE_MAX) | \
+ (1ULL << VIRTIO_BLK_F_SEG_MAX) | \
+ (1ULL << VIRTIO_BLK_F_BLK_SIZE) | \
+ (1ULL << VIRTIO_BLK_F_TOPOLOGY) | \
+ (1ULL << VIRTIO_BLK_F_MQ) | \
+ (1ULL << VIRTIO_F_VERSION_1) | \
+ (1ULL << VIRTIO_F_ACCESS_PLATFORM))
I think we've discussed this sometime in the past but what's the
reason for such whitelist consider there's already a get_features()
implemention?
E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or
VIRTIO_F_RING_PACKED?
Thanks
The reason is some feature bits are supported in the device but not
supported by the driver, e.g, for virtio-net, mq & cq implementation
is not ready in the driver.
I understand the case of virtio-net but I wonder why we need this for
block where we don't vq cvq.
Thanks
Thanks!
+
/* Only one queue pair for now. */
#define IFCVF_MAX_QUEUE_PAIRS 1
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index 99b0a6b4c227..9b6a38b798fa 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -171,7 +171,11 @@ static u64 ifcvf_vdpa_get_features(struct
vdpa_device *vdpa_dev)
struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
u64 features;
- features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
+ if (vf->dev_type == VIRTIO_ID_NET)
+ features = ifcvf_get_features(vf) &
IFCVF_NET_SUPPORTED_FEATURES;
+
+ if (vf->dev_type == VIRTIO_ID_BLOCK)
+ features = ifcvf_get_features(vf) &
IFCVF_BLK_SUPPORTED_FEATURES;
return features;
}
@@ -509,6 +513,10 @@ static struct pci_device_id ifcvf_pci_ids[] = {
C5000X_PL_DEVICE_ID,
C5000X_PL_SUBSYS_VENDOR_ID,
C5000X_PL_SUBSYS_DEVICE_ID) },
+ { PCI_DEVICE_SUB(C5000X_PL_BLK_VENDOR_ID,
+ C5000X_PL_BLK_DEVICE_ID,
+ C5000X_PL_BLK_SUBSYS_VENDOR_ID,
+ C5000X_PL_BLK_SUBSYS_DEVICE_ID) },
{ 0 },
};
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] vDPA/ifcvf: deduce VIRTIO device ID when probe
在 2021/4/15 下午1:52, Zhu Lingshan 写道:
On 4/15/2021 11:30 AM, Jason Wang wrote:
在 2021/4/14 下午5:18, Zhu Lingshan 写道:
This commit deduces VIRTIO device ID as device type when probe,
then ifcvf_vdpa_get_device_id() can simply return the ID.
ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
can work properly based on the device ID.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
b/drivers/vdpa/ifcvf/ifcvf_base.h
index b2eeb16b9c2c..1c04cd256fa7 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -84,6 +84,7 @@ struct ifcvf_hw {
u32 notify_off_multiplier;
u64 req_features;
u64 hw_features;
+ u32 dev_type;
struct virtio_pci_common_cfg __iomem *common_cfg;
void __iomem *net_cfg;
struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index 44d7586019da..99b0a6b4c227 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct
vdpa_device *vdpa_dev)
static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
{
- struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
- struct pci_dev *pdev = adapter->pdev;
- u32 ret = -ENODEV;
-
- if (pdev->device < 0x1000 || pdev->device > 0x107f)
- return ret;
-
- if (pdev->device < 0x1040)
- ret = pdev->subsystem_device;
- else
- ret = pdev->device -0x1040;
+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
- return ret;
+ return vf->dev_type;
}
static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
@@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
pci_set_drvdata(pdev, adapter);
vf = &adapter->vf;
+ if (pdev->device < 0x1000 || pdev->device > 0x107f)
+ return -EOPNOTSUPP;
+
+ if (pdev->device < 0x1040)
+ vf->dev_type = pdev->subsystem_device;
+ else
+ vf->dev_type = pdev->device - 0x1040;
So a question here, is the device a transtional device or modern one?
If it's a transitonal one, can it swtich endianess automatically or not?
Thanks
Hi Jason,
This driver should drive both modern and transitional devices as we
discussed before.
If it's a transitional one, it will act as a modern device by default,
legacy mode is a fail-over path.
Note that legacy driver use native endian, support legacy driver
requires the device to know native endian which I'm not sure your device
can do that.
Thanks
For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it must
in modern mode.
I think we don't need to worry about endianess for legacy mode.
Thanks
Zhu Lingshan
+
vf->base = pcim_iomap_table(pdev);
adapter->pdev = pdev;
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 2021/4/14 11:52, Viresh Kumar wrote: Is i2c/for-next the right tree to merge it ? It should be. Thanks Viresh. Hi Wolfram, Do you have any comments for this patch ? Your opinion will be important to improve this patch since you are the maintainer of I2C. Thanks, Jie ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 2021/4/15 11:51, Jason Wang wrote:
+ for (i = 0; i < nr; i++) {
+ /* Detach the ith request from the vq */
+ req = virtqueue_get_buf(vq, &len);
+
+ /*
+ * Condition (req && req == &reqs[i]) should always meet since
+ * we have total nr requests in the vq.
So this assumes the requests are completed in order. Is this mandated
in the spec?
This is a mandatory device requirements in spec.
+ */
+ if (!failed && (WARN_ON(!(req && req == &reqs[i])) ||
+ (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))
+ failed = true;
+
+ i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
+ if (!failed)
+ ++j;
+ }
+
+ return (timeout ? -ETIMEDOUT : j);
Checking timeout is fragile, what happens if the request are completed
after wait_for_completion() but before virtio_i2c_complete_reqs()?
We have discussed this before in v6.
https://lists.linuxfoundation.org/pipermail/virtualization/2021-March/053093.html.
If timeout happens, we don't need to care about the requests whether
really completed by "HW" or not.
Just return error and let the i2c core to decide whether to resend. And
currently, the timeout is statically configured in driver.
We may extend a device timeout value configuration in spec for driver to
use if there is actual need in the future.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/3] vDPA/ifcvf: enable Intel C5000X-PL virtio-block for vDPA
On 4/15/2021 11:34 AM, Jason Wang wrote:
在 2021/4/14 下午5:18, Zhu Lingshan 写道:
This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-block
for vDPA.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_base.h | 17 -
drivers/vdpa/ifcvf/ifcvf_main.c | 10 +-
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
b/drivers/vdpa/ifcvf/ifcvf_base.h
index 1c04cd256fa7..8b403522bf06 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
@@ -28,7 +29,12 @@
#define C5000X_PL_SUBSYS_VENDOR_ID 0x8086
#define C5000X_PL_SUBSYS_DEVICE_ID 0x0001
-#define IFCVF_SUPPORTED_FEATURES \
+#define C5000X_PL_BLK_VENDOR_ID 0x1AF4
+#define C5000X_PL_BLK_DEVICE_ID 0x1001
+#define C5000X_PL_BLK_SUBSYS_VENDOR_ID 0x8086
+#define C5000X_PL_BLK_SUBSYS_DEVICE_ID 0x0002
+
+#define IFCVF_NET_SUPPORTED_FEATURES \
((1ULL << VIRTIO_NET_F_MAC) | \
(1ULL << VIRTIO_F_ANY_LAYOUT) | \
(1ULL << VIRTIO_F_VERSION_1) | \
@@ -37,6 +43,15 @@
(1ULL << VIRTIO_F_ACCESS_PLATFORM) | \
(1ULL << VIRTIO_NET_F_MRG_RXBUF))
+#define IFCVF_BLK_SUPPORTED_FEATURES \
+ ((1ULL << VIRTIO_BLK_F_SIZE_MAX) | \
+ (1ULL << VIRTIO_BLK_F_SEG_MAX) | \
+ (1ULL << VIRTIO_BLK_F_BLK_SIZE) | \
+ (1ULL << VIRTIO_BLK_F_TOPOLOGY) | \
+ (1ULL << VIRTIO_BLK_F_MQ) | \
+ (1ULL << VIRTIO_F_VERSION_1) | \
+ (1ULL << VIRTIO_F_ACCESS_PLATFORM))
I think we've discussed this sometime in the past but what's the
reason for such whitelist consider there's already a get_features()
implemention?
E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or VIRTIO_F_RING_PACKED?
Thanks
The reason is some feature bits are supported in the device but not
supported by the driver, e.g, for virtio-net, mq & cq implementation is
not ready in the driver.
Thanks!
+
/* Only one queue pair for now. */
#define IFCVF_MAX_QUEUE_PAIRS 1
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index 99b0a6b4c227..9b6a38b798fa 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -171,7 +171,11 @@ static u64 ifcvf_vdpa_get_features(struct
vdpa_device *vdpa_dev)
struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
u64 features;
- features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
+ if (vf->dev_type == VIRTIO_ID_NET)
+ features = ifcvf_get_features(vf) &
IFCVF_NET_SUPPORTED_FEATURES;
+
+ if (vf->dev_type == VIRTIO_ID_BLOCK)
+ features = ifcvf_get_features(vf) &
IFCVF_BLK_SUPPORTED_FEATURES;
return features;
}
@@ -509,6 +513,10 @@ static struct pci_device_id ifcvf_pci_ids[] = {
C5000X_PL_DEVICE_ID,
C5000X_PL_SUBSYS_VENDOR_ID,
C5000X_PL_SUBSYS_DEVICE_ID) },
+ { PCI_DEVICE_SUB(C5000X_PL_BLK_VENDOR_ID,
+ C5000X_PL_BLK_DEVICE_ID,
+ C5000X_PL_BLK_SUBSYS_VENDOR_ID,
+ C5000X_PL_BLK_SUBSYS_DEVICE_ID) },
{ 0 },
};
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] vDPA/ifcvf: deduce VIRTIO device ID when probe
On 4/15/2021 11:30 AM, Jason Wang wrote:
在 2021/4/14 下午5:18, Zhu Lingshan 写道:
This commit deduces VIRTIO device ID as device type when probe,
then ifcvf_vdpa_get_device_id() can simply return the ID.
ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
can work properly based on the device ID.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
b/drivers/vdpa/ifcvf/ifcvf_base.h
index b2eeb16b9c2c..1c04cd256fa7 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -84,6 +84,7 @@ struct ifcvf_hw {
u32 notify_off_multiplier;
u64 req_features;
u64 hw_features;
+ u32 dev_type;
struct virtio_pci_common_cfg __iomem *common_cfg;
void __iomem *net_cfg;
struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index 44d7586019da..99b0a6b4c227 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct
vdpa_device *vdpa_dev)
static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
{
- struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
- struct pci_dev *pdev = adapter->pdev;
- u32 ret = -ENODEV;
-
- if (pdev->device < 0x1000 || pdev->device > 0x107f)
- return ret;
-
- if (pdev->device < 0x1040)
- ret = pdev->subsystem_device;
- else
- ret = pdev->device - 0x1040;
+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
- return ret;
+ return vf->dev_type;
}
static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
@@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
pci_set_drvdata(pdev, adapter);
vf = &adapter->vf;
+ if (pdev->device < 0x1000 || pdev->device > 0x107f)
+ return -EOPNOTSUPP;
+
+ if (pdev->device < 0x1040)
+ vf->dev_type = pdev->subsystem_device;
+ else
+ vf->dev_type = pdev->device - 0x1040;
So a question here, is the device a transtional device or modern one?
If it's a transitonal one, can it swtich endianess automatically or not?
Thanks
Hi Jason,
This driver should drive both modern and transitional devices as we
discussed before.
If it's a transitional one, it will act as a modern device by default,
legacy mode is a fail-over path.
For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it must in
modern mode.
I think we don't need to worry about endianess for legacy mode.
Thanks
Zhu Lingshan
+
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] sound: virtio: correct the function name in kernel-doc comment
Fix kernel-doc warning that the wrong function name is used in a kernel-doc comment: ../sound/virtio/virtio_ctl_msg.c:70: warning: expecting prototype for virtsnd_ctl_msg_request(). Prototype was for virtsnd_ctl_msg_response() instead Signed-off-by: Randy Dunlap Cc: Anton Yakovlev Cc: "Michael S. Tsirkin" Cc: virtualization@lists.linux-foundation.org Cc: alsa-de...@alsa-project.org --- sound/virtio/virtio_ctl_msg.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- linux-next-20210414.orig/sound/virtio/virtio_ctl_msg.c +++ linux-next-20210414/sound/virtio/virtio_ctl_msg.c @@ -61,7 +61,7 @@ void *virtsnd_ctl_msg_request(struct vir } /** - * virtsnd_ctl_msg_request() - Get a pointer to the response header. + * virtsnd_ctl_msg_response() - Get a pointer to the response header. * @msg: Control message. * * Context: Any context. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
在 2021/3/23 下午10:19, Jie Deng 写道:
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 in v10:
- Fix some typo errors.
- Refined the virtio_i2c_complete_reqs to use less code lines.
Changes in v9:
- Remove the virtio_adapter and update its members in probe.
- Refined the virtio_i2c_complete_reqs for buf free.
Changes in v8:
- Make virtio_i2c.adap a pointer.
- Mark members in virtio_i2c_req with cacheline_aligned.
Changes in v7:
- Remove unused headers.
- Update Makefile and Kconfig.
- Add the cleanup after completing reqs.
- Avoid memcpy for data marked with I2C_M_DMA_SAFE.
- Fix something reported by kernel test robot.
Changes in v6:
- Move struct virtio_i2c_req into the driver.
- Use only one buf in struct virtio_i2c_req.
Changes in v5:
- The first version based on the acked specification.
drivers/i2c/busses/Kconfig | 11 ++
drivers/i2c/busses/Makefile | 3 +
drivers/i2c/busses/i2c-virtio.c | 276
include/uapi/linux/virtio_i2c.h | 40 ++
include/uapi/linux/virtio_ids.h | 1 +
5 files changed, 331 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 05ebf75..cb8d0d8 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 615f35e..efdd3f3 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -145,4 +145,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..99a1e30
--- /dev/null
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -0,0 +1,276 @@
+// 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
+ * @lock: lock for virtqueue processing
+ * @vq: the virtio virtqueue for communication
+ */
+struct virtio_i2c {
+ struct virtio_device *vdev;
+ struct completion completion;
+ struct i2c_adapter adap;
+ struct mutex lock;
+ 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;
+
+ complete(&vi->completion);
+}
+
+static int virtio_i2c_send_reqs(struct virtqueue *vq,
+ struct virtio_i2c_req *reqs,
+ struct i2c_msg *msgs, int nr)
+{
+ struct scatterlist *sgs[3], ou
Re: [PATCH 3/3] vDPA/ifcvf: get_config_size should return dev specific config size
在 2021/4/14 下午5:18, Zhu Lingshan 写道:
get_config_size() should return the size based on the decected
device type.
Signed-off-by: Zhu Lingshan
Acked-by: Jason Wang
---
drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 9b6a38b798fa..b48b9789b69e 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device
*vdpa_dev)
static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
{
- return sizeof(struct virtio_net_config);
+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
+ size_t size;
+
+ if (vf->dev_type == VIRTIO_ID_NET)
+ size = sizeof(struct virtio_net_config);
+
+ if (vf->dev_type == VIRTIO_ID_BLOCK)
+ size = sizeof(struct virtio_blk_config);
+
+ return size;
}
static void ifcvf_vdpa_get_config(struct vdpa_device *vdpa_dev,
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/3] vDPA/ifcvf: enable Intel C5000X-PL virtio-block for vDPA
在 2021/4/14 下午5:18, Zhu Lingshan 写道:
This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-block
for vDPA.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_base.h | 17 -
drivers/vdpa/ifcvf/ifcvf_main.c | 10 +-
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index 1c04cd256fa7..8b403522bf06 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
@@ -28,7 +29,12 @@
#define C5000X_PL_SUBSYS_VENDOR_ID0x8086
#define C5000X_PL_SUBSYS_DEVICE_ID0x0001
-#define IFCVF_SUPPORTED_FEATURES \
+#define C5000X_PL_BLK_VENDOR_ID0x1AF4
+#define C5000X_PL_BLK_DEVICE_ID0x1001
+#define C5000X_PL_BLK_SUBSYS_VENDOR_ID 0x8086
+#define C5000X_PL_BLK_SUBSYS_DEVICE_ID 0x0002
+
+#define IFCVF_NET_SUPPORTED_FEATURES \
((1ULL << VIRTIO_NET_F_MAC) | \
(1ULL << VIRTIO_F_ANY_LAYOUT)| \
(1ULL << VIRTIO_F_VERSION_1) | \
@@ -37,6 +43,15 @@
(1ULL << VIRTIO_F_ACCESS_PLATFORM) | \
(1ULL << VIRTIO_NET_F_MRG_RXBUF))
+#define IFCVF_BLK_SUPPORTED_FEATURES \
+ ((1ULL << VIRTIO_BLK_F_SIZE_MAX) | \
+(1ULL << VIRTIO_BLK_F_SEG_MAX) | \
+(1ULL << VIRTIO_BLK_F_BLK_SIZE) | \
+(1ULL << VIRTIO_BLK_F_TOPOLOGY) | \
+(1ULL << VIRTIO_BLK_F_MQ)| \
+(1ULL << VIRTIO_F_VERSION_1) | \
+(1ULL << VIRTIO_F_ACCESS_PLATFORM))
I think we've discussed this sometime in the past but what's the reason
for such whitelist consider there's already a get_features() implemention?
E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or VIRTIO_F_RING_PACKED?
Thanks
+
/* Only one queue pair for now. */
#define IFCVF_MAX_QUEUE_PAIRS 1
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 99b0a6b4c227..9b6a38b798fa 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -171,7 +171,11 @@ static u64 ifcvf_vdpa_get_features(struct vdpa_device
*vdpa_dev)
struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
u64 features;
- features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
+ if (vf->dev_type == VIRTIO_ID_NET)
+ features = ifcvf_get_features(vf) &
IFCVF_NET_SUPPORTED_FEATURES;
+
+ if (vf->dev_type == VIRTIO_ID_BLOCK)
+ features = ifcvf_get_features(vf) &
IFCVF_BLK_SUPPORTED_FEATURES;
return features;
}
@@ -509,6 +513,10 @@ static struct pci_device_id ifcvf_pci_ids[] = {
C5000X_PL_DEVICE_ID,
C5000X_PL_SUBSYS_VENDOR_ID,
C5000X_PL_SUBSYS_DEVICE_ID) },
+ { PCI_DEVICE_SUB(C5000X_PL_BLK_VENDOR_ID,
+C5000X_PL_BLK_DEVICE_ID,
+C5000X_PL_BLK_SUBSYS_VENDOR_ID,
+C5000X_PL_BLK_SUBSYS_DEVICE_ID) },
{ 0 },
};
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] vDPA/ifcvf: deduce VIRTIO device ID when probe
在 2021/4/14 下午5:18, Zhu Lingshan 写道:
This commit deduces VIRTIO device ID as device type when probe,
then ifcvf_vdpa_get_device_id() can simply return the ID.
ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
can work properly based on the device ID.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index b2eeb16b9c2c..1c04cd256fa7 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -84,6 +84,7 @@ struct ifcvf_hw {
u32 notify_off_multiplier;
u64 req_features;
u64 hw_features;
+ u32 dev_type;
struct virtio_pci_common_cfg __iomem *common_cfg;
void __iomem *net_cfg;
struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 44d7586019da..99b0a6b4c227 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct vdpa_device
*vdpa_dev)
static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
{
- struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
- struct pci_dev *pdev = adapter->pdev;
- u32 ret = -ENODEV;
-
- if (pdev->device < 0x1000 || pdev->device > 0x107f)
- return ret;
-
- if (pdev->device < 0x1040)
- ret = pdev->subsystem_device;
- else
- ret = pdev->device - 0x1040;
+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
- return ret;
+ return vf->dev_type;
}
static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
@@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
pci_set_drvdata(pdev, adapter);
vf = &adapter->vf;
+ if (pdev->device < 0x1000 || pdev->device > 0x107f)
+ return -EOPNOTSUPP;
+
+ if (pdev->device < 0x1040)
+ vf->dev_type = pdev->subsystem_device;
+ else
+ vf->dev_type = pdev->device - 0x1040;
So a question here, is the device a transtional device or modern one?
If it's a transitonal one, can it swtich endianess automatically or not?
Thanks
+
vf->base = pcim_iomap_table(pdev);
adapter->pdev = pdev;
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Re: [RFC v2] virtio-vsock: add description for datagram type
On Wed, Apr 14, 2021 at 2:38 AM Stefano Garzarella wrote: > > On Wed, Apr 14, 2021 at 03:20:07AM -0400, Michael S. Tsirkin wrote: > >On Wed, Apr 14, 2021 at 08:57:06AM +0200, Stefano Garzarella wrote: > >> On Tue, Apr 13, 2021 at 03:58:34PM -0400, Michael S. Tsirkin wrote: > >> > On Tue, Apr 13, 2021 at 04:03:51PM +0200, Stefano Garzarella wrote: > >> > > On Tue, Apr 13, 2021 at 09:50:45AM -0400, Michael S. Tsirkin wrote: > >> > > > On Tue, Apr 13, 2021 at 03:38:52PM +0200, Stefano Garzarella wrote: > >> > > > > On Tue, Apr 13, 2021 at 09:16:50AM -0400, Michael S. Tsirkin wrote: > >> > > > > > On Tue, Apr 13, 2021 at 02:58:53PM +0200, Stefano Garzarella > >> > > > > > wrote: > >> > > > > > > On Mon, Apr 12, 2021 at 03:42:23PM -0700, Jiang Wang . wrote: > >> > > > > > > > On Mon, Apr 12, 2021 at 7:21 AM Stefano Garzarella > >> > > > > > > > wrote: > >> > > > > > > > > > >> > > > > > > > > On Mon, Apr 12, 2021 at 02:50:17PM +0100, Stefan Hajnoczi > >> > > > > > > > > wrote: > >> > > > > > > > > >On Thu, Apr 01, 2021 at 04:36:02AM +, jiang.wang > >> > > > > > > > > >wrote: > > [...] > > >> > > > > > > > > >> > >> > > > > > > > > >> +Datagram sockets provide connectionless unreliable > >> > > > > > > > > >> messages of > >> > > > > > > > > >> +a fixed maximum length. > >> > > > > > > > > > > >> > > > > > > > > >Plus unordered (?) and with message boundaries. In other > >> > > > > > > > > >words: > >> > > > > > > > > > > >> > > > > > > > > > Datagram sockets provide unordered, unreliable, > >> > > > > > > > > > connectionless message > >> > > > > > > > > > with message boundaries and a fixed maximum length. > >> > > > > > > > > > > >> > > > > > > > > >I didn't think of the fixed maximum length aspect before. > >> > > > > > > > > >I guess the > >> > > > > > > > > >intention is that the rx buffer size is the message size > >> > > > > > > > > >limit? That's > >> > > > > > > > > >different from UDP messages, which can be fragmented into > >> > > > > > > > > >multiple IP > >> > > > > > > > > >packets and can be larger than 64KiB: > >> > > > > > > > > >https://en.wikipedia.org/wiki/User_Datagram_Protocol#UDP_datagram_structure > >> > > > > > > > > > > >> > > > > > > > > >Is it possible to support large datagram messages in > >> > > > > > > > > >vsock? I'm a little > >> > > > > > > > > >concerned that applications that run successfully over > >> > > > > > > > > >UDP will not be > >> > > > > > > > > >portable if vsock has this limitation because it would > >> > > > > > > > > >impose extra > >> > > > > > > > > >message boundaries that the application protocol might > >> > > > > > > > > >not tolerate. > >> > > > > > > > > > >> > > > > > > > > Maybe we can reuse the same approach Arseny is using for > >> > > > > > > > > SEQPACKET. > >> > > > > > > > > Fragment the packets according to the buffers in the > >> > > > > > > > > virtqueue and set > >> > > > > > > > > the EOR flag to indicate the last packet in the message. > >> > > > > > > > > > >> > > > > > > > Agree. Another option is to use the ones for skb since we > >> > > > > > > > may need to > >> > > > > > > > use skbs for multiple transport support anyway. > >> > > > > > > > > >> > > > > > > > >> > > > > > > The important thing I think is to have a single flag in > >> > > > > > > virtio-vsock that > >> > > > > > > identifies pretty much the same thing: this is the last > >> > > > > > > fragment of a series > >> > > > > > > to rebuild a packet. > >> > > > > > > > >> > > > > > > We should reuse the same flag for DGRAM and SEQPACKET. > >> > > > > > > > >> > > > > > > Thanks, > >> > > > > > > Stefano > >> > > > > > > >> > > > > > Well DGRAM can drop data so I wonder whether it can work ... > >> > > > > > > >> > > > > > >> > > > > Yep, this is true, but the channel should not be losing packets, > >> > > > > so if the > >> > > > > receiver discards packets, it knows that it must then discard all > >> > > > > of them > >> > > > > until the EOR. > >> > > > > >> > > > That is not so easy - they can come mixed up from multiple sources. > >> > > > >> > > I think we can prevent mixing because virtuqueue is point to point and > >> > > its > >> > > use is not thread safe, so the access (in the same peer) is already > >> > > serialized. > >> > > In the end the packet would be fragmented only before copying it to the > >> > > virtuqueue. > >> > > > >> > > But maybe I missed something... > >> > > >> > Well I ask what's the point of fragmenting then. I assume it's so we > >> > can pass huge messages around so you can't keep locks ... > >> > > >> > >> Maybe I'm wrong, but isn't this similar to what we do in virtio-net with > >> mergeable buffers? > > > >The point of mergeable buffers is to use less memory: both for each > >packet and for a full receive vq. > > > >> Also in this case I think the fragmentation will happen only in the device, > >> since the driver can enqueue the entire buffer. > >> > >> Maybe we can reuse mergeable buffers for virtio-vsock if
Re: [PATCH v6 10/10] Documentation: Add documentation for VDUSE
On Wed, Mar 31, 2021 at 04:05:19PM +0800, Xie Yongji wrote:
> VDUSE (vDPA Device in Userspace) is a framework to support
> implementing software-emulated vDPA devices in userspace. This
> document is intended to clarify the VDUSE design and usage.
>
> Signed-off-by: Xie Yongji
> ---
> Documentation/userspace-api/index.rst | 1 +
> Documentation/userspace-api/vduse.rst | 212
> ++
> 2 files changed, 213 insertions(+)
> create mode 100644 Documentation/userspace-api/vduse.rst
Just looking over the documentation briefly (I haven't studied the code
yet)...
> +How VDUSE works
> +
> +Each userspace vDPA device is created by the VDUSE_CREATE_DEV ioctl on
> +the character device (/dev/vduse/control). Then a device file with the
> +specified name (/dev/vduse/$NAME) will appear, which can be used to
> +implement the userspace vDPA device's control path and data path.
These steps are taken after sending the VDPA_CMD_DEV_NEW netlink
message? (Please consider reordering the documentation to make it clear
what the sequence of steps are.)
> + static int netlink_add_vduse(const char *name, int device_id)
> + {
> + struct nl_sock *nlsock;
> + struct nl_msg *msg;
> + int famid;
> +
> + nlsock = nl_socket_alloc();
> + if (!nlsock)
> + return -ENOMEM;
> +
> + if (genl_connect(nlsock))
> + goto free_sock;
> +
> + famid = genl_ctrl_resolve(nlsock, VDPA_GENL_NAME);
> + if (famid < 0)
> + goto close_sock;
> +
> + msg = nlmsg_alloc();
> + if (!msg)
> + goto close_sock;
> +
> + if (!genlmsg_put(msg, NL_AUTO_PORT, NL_AUTO_SEQ, famid, 0, 0,
> + VDPA_CMD_DEV_NEW, 0))
> + goto nla_put_failure;
> +
> + NLA_PUT_STRING(msg, VDPA_ATTR_DEV_NAME, name);
> + NLA_PUT_STRING(msg, VDPA_ATTR_MGMTDEV_DEV_NAME, "vduse");
> + NLA_PUT_U32(msg, VDPA_ATTR_DEV_ID, device_id);
What are the permission/capability requirements for VDUSE?
How does VDUSE interact with namespaces?
What is the meaning of VDPA_ATTR_DEV_ID? I don't see it in Linux
v5.12-rc6 drivers/vdpa/vdpa.c:vdpa_nl_cmd_dev_add_set_doit().
> +MMU-based IOMMU Driver
> +--
> +VDUSE framework implements an MMU-based on-chip IOMMU driver to support
> +mapping the kernel DMA buffer into the userspace iova region dynamically.
> +This is mainly designed for virtio-vdpa case (kernel virtio drivers).
> +
> +The basic idea behind this driver is treating MMU (VA->PA) as IOMMU
> (IOVA->PA).
> +The driver will set up MMU mapping instead of IOMMU mapping for the DMA
> transfer
> +so that the userspace process is able to use its virtual address to access
> +the DMA buffer in kernel.
> +
> +And to avoid security issue, a bounce-buffering mechanism is introduced to
> +prevent userspace accessing the original buffer directly which may contain
> other
> +kernel data. During the mapping, unmapping, the driver will copy the data
> from
> +the original buffer to the bounce buffer and back, depending on the
> direction of
> +the transfer. And the bounce-buffer addresses will be mapped into the user
> address
> +space instead of the original one.
Is mmap(2) the right interface if memory is not actually shared, why not
just use pread(2)/pwrite(2) to make the copy explicit? That way the copy
semantics are clear. For example, don't expect to be able to busy wait
on the memory because changes will not be visible to the other side.
(I guess I'm missing something here and that mmap(2) is the right
approach, but maybe this documentation section can be clarified.)
signature.asc
Description: PGP signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC v2] virtio-vsock: add description for datagram type
On Wed, Apr 14, 2021 at 03:20:07AM -0400, Michael S. Tsirkin wrote: On Wed, Apr 14, 2021 at 08:57:06AM +0200, Stefano Garzarella wrote: On Tue, Apr 13, 2021 at 03:58:34PM -0400, Michael S. Tsirkin wrote: > On Tue, Apr 13, 2021 at 04:03:51PM +0200, Stefano Garzarella wrote: > > On Tue, Apr 13, 2021 at 09:50:45AM -0400, Michael S. Tsirkin wrote: > > > On Tue, Apr 13, 2021 at 03:38:52PM +0200, Stefano Garzarella wrote: > > > > On Tue, Apr 13, 2021 at 09:16:50AM -0400, Michael S. Tsirkin wrote: > > > > > On Tue, Apr 13, 2021 at 02:58:53PM +0200, Stefano Garzarella wrote: > > > > > > On Mon, Apr 12, 2021 at 03:42:23PM -0700, Jiang Wang . wrote: > > > > > > > On Mon, Apr 12, 2021 at 7:21 AM Stefano Garzarella wrote: > > > > > > > > > > > > > > > > On Mon, Apr 12, 2021 at 02:50:17PM +0100, Stefan Hajnoczi wrote: > > > > > > > > >On Thu, Apr 01, 2021 at 04:36:02AM +, jiang.wang > > > > > > > > >wrote: [...] > > > > > > > > >> > > > > > > > > >> +Datagram sockets provide connectionless unreliable messages of > > > > > > > > >> +a fixed maximum length. > > > > > > > > > > > > > > > > > >Plus unordered (?) and with message boundaries. In other words: > > > > > > > > > > > > > > > > > > Datagram sockets provide unordered, unreliable, connectionless message > > > > > > > > > with message boundaries and a fixed maximum length. > > > > > > > > > > > > > > > > > >I didn't think of the fixed maximum length aspect before. I guess the > > > > > > > > >intention is that the rx buffer size is the message size limit? That's > > > > > > > > >different from UDP messages, which can be fragmented into multiple IP > > > > > > > > >packets and can be larger than 64KiB: > > > > > > > > >https://en.wikipedia.org/wiki/User_Datagram_Protocol#UDP_datagram_structure > > > > > > > > > > > > > > > > > >Is it possible to support large datagram messages in vsock? I'm a little > > > > > > > > >concerned that applications that run successfully over UDP will not be > > > > > > > > >portable if vsock has this limitation because it would impose extra > > > > > > > > >message boundaries that the application protocol might not tolerate. > > > > > > > > > > > > > > > > Maybe we can reuse the same approach Arseny is using for SEQPACKET. > > > > > > > > Fragment the packets according to the buffers in the virtqueue and set > > > > > > > > the EOR flag to indicate the last packet in the message. > > > > > > > > > > > > > > > Agree. Another option is to use the ones for skb since we may need to > > > > > > > use skbs for multiple transport support anyway. > > > > > > > > > > > > > > > > > > > The important thing I think is to have a single flag in virtio-vsock that > > > > > > identifies pretty much the same thing: this is the last fragment of a series > > > > > > to rebuild a packet. > > > > > > > > > > > > We should reuse the same flag for DGRAM and SEQPACKET. > > > > > > > > > > > > Thanks, > > > > > > Stefano > > > > > > > > > > Well DGRAM can drop data so I wonder whether it can work ... > > > > > > > > > > > > > Yep, this is true, but the channel should not be losing packets, so if the > > > > receiver discards packets, it knows that it must then discard all of them > > > > until the EOR. > > > > > > That is not so easy - they can come mixed up from multiple sources. > > > > I think we can prevent mixing because virtuqueue is point to point and its > > use is not thread safe, so the access (in the same peer) is already > > serialized. > > In the end the packet would be fragmented only before copying it to the > > virtuqueue. > > > > But maybe I missed something... > > Well I ask what's the point of fragmenting then. I assume it's so we > can pass huge messages around so you can't keep locks ... > Maybe I'm wrong, but isn't this similar to what we do in virtio-net with mergeable buffers? The point of mergeable buffers is to use less memory: both for each packet and for a full receive vq. Also in this case I think the fragmentation will happen only in the device, since the driver can enqueue the entire buffer. Maybe we can reuse mergeable buffers for virtio-vsock if the EOR flag is not suitable. That sounds very reasonable. It should also allow us to save the header for each fragment. @Jiang Do you want to explore this? I'm talking about VIRTIO_NET_F_MRG_RXBUF feature. IIUC in the vsock device the fragmentation for DGRAM will happen just before to queue it in the virtqueue, and the device can check how many buffers are available in the queue and it can decide whether to queue them all up or throw them away. > > > > Sure linux net core does this but with fragmentation added in, > > > I start wondering whether you are beginning to reinvent the net stack > > > ... > > > > No, I hope not :-), in the end our advantage is that we have a channel that > > doesn't lose packets, so I guess we can make assumptions that the network > > stack can't. > > > > Thanks, > > Stefano > > I still don't know how will credit
Re: [PATCH net-next v2] virtio-net: page_to_skb() use build_skb when there's sufficient tailroom
在 2021/4/14 上午9:52, Xuan Zhuo 写道:
In page_to_skb(), if we have enough tailroom to save skb_shared_info, we
can use build_skb to create skb directly. No need to alloc for
additional space. And it can save a 'frags slot', which is very friendly
to GRO.
Here, if the payload of the received package is too small (less than
GOOD_COPY_LEN), we still choose to copy it directly to the space got by
napi_alloc_skb. So we can reuse these pages.
Testing Machine:
The four queues of the network card are bound to the cpu1.
Test command:
for ((i=0;i<5;++i)); do sockperf tp --ip 192.168.122.64 -m 1000 -t 150&
done
The size of the udp package is 1000, so in the case of this patch, there
will always be enough tailroom to use build_skb. The sent udp packet
will be discarded because there is no port to receive it. The irqsoftd
of the machine is 100%, we observe the received quantity displayed by
sar -n DEV 1:
no build_skb: 956864.00 rxpck/s
build_skb:1158465.00 rxpck/s
Signed-off-by: Xuan Zhuo
Suggested-by: Jason Wang
---
v2: conflict resolution
drivers/net/virtio_net.c | 51 ++--
1 file changed, 33 insertions(+), 18 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 101659cd4b87..d7142b508bd0 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -383,17 +383,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info
*vi,
{
struct sk_buff *skb;
struct virtio_net_hdr_mrg_rxbuf *hdr;
- unsigned int copy, hdr_len, hdr_padded_len;
- char *p;
+ unsigned int copy, hdr_len, hdr_padded_len, tailroom, shinfo_size;
+ char *p, *hdr_p;
p = page_address(page) + offset;
-
- /* copy small packet so we can reuse these pages for small data */
- skb = napi_alloc_skb(&rq->napi, GOOD_COPY_LEN);
- if (unlikely(!skb))
- return NULL;
-
- hdr = skb_vnet_hdr(skb);
+ hdr_p = p;
hdr_len = vi->hdr_len;
if (vi->mergeable_rx_bufs)
@@ -401,14 +395,28 @@ static struct sk_buff *page_to_skb(struct virtnet_info
*vi,
else
hdr_padded_len = sizeof(struct padded_vnet_hdr);
- /* hdr_valid means no XDP, so we can copy the vnet header */
- if (hdr_valid)
- memcpy(hdr, p, hdr_len);
+ tailroom = truesize - len;
The math looks not correct in the case of XDP. Since the eBPF pgoram can
choose to adjust the header and insert meta which will cause the
truesize is less than len.
Note that in the case of XDP, we always reserve sufficient tailroom for
shinfo, see add_recvbuf_mergeable():
unsigned int tailroom = headroom ? sizeof(struct
skb_shared_info) : 0;
Thanks
len -= hdr_len;
offset += hdr_padded_len;
p += hdr_padded_len;
+ shinfo_size = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+
+ if (len > GOOD_COPY_LEN && tailroom >= shinfo_size) {
+ skb = build_skb(p, truesize);
+ if (unlikely(!skb))
+ return NULL;
+
+ skb_put(skb, len);
+ goto ok;
+ }
+
+ /* copy small packet so we can reuse these pages for small data */
+ skb = napi_alloc_skb(&rq->napi, GOOD_COPY_LEN);
+ if (unlikely(!skb))
+ return NULL;
+
/* Copy all frame if it fits skb->head, otherwise
* we let virtio_net_hdr_to_skb() and GRO pull headers as needed.
*/
@@ -418,11 +426,6 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
copy = ETH_HLEN + metasize;
skb_put_data(skb, p, copy);
- if (metasize) {
- __skb_pull(skb, metasize);
- skb_metadata_set(skb, metasize);
- }
-
len -= copy;
offset += copy;
@@ -431,7 +434,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
skb_add_rx_frag(skb, 0, page, offset, len, truesize);
else
put_page(page);
- return skb;
+ goto ok;
}
/*
@@ -458,6 +461,18 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
if (page)
give_pages(rq, page);
+ok:
+ /* hdr_valid means no XDP, so we can copy the vnet header */
+ if (hdr_valid) {
+ hdr = skb_vnet_hdr(skb);
+ memcpy(hdr, hdr_p, hdr_len);
+ }
+
+ if (metasize) {
+ __skb_pull(skb, metasize);
+ skb_metadata_set(skb, metasize);
+ }
+
return skb;
}
--
2.31.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_blk: Add support for lifetime feature
On Mon, Apr 12, 2021 at 10:42:17AM +0100, Christoph Hellwig wrote: > A note to the virtio committee: eMMC is the worst of all the currently > active storage standards by a large margin. It defines very strange > ad-hoc interfaces that expose very specific internals and often provides > very poor abstractions. It would be great it you could reach out to the > wider storage community before taking bad ideas from the eMMC standard > and putting it into virtio. As Michael mentioned, there is still time to change the virtio-blk spec since this feature hasn't been released yet. Why exactly is exposing eMMC-style lifetime information problematic? Can you and Enrico discuss the use case to figure out an alternative interface? Thanks, Stefan signature.asc Description: PGP signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtiofs: remove useless function
On Tue, Apr 13, 2021 at 05:22:23PM +0800, Jiapeng Chong wrote: > Fix the following clang warning: > > fs/fuse/virtio_fs.c:130:35: warning: unused function 'vq_to_fpq' > [-Wunused-function]. > > Reported-by: Abaci Robot > Signed-off-by: Jiapeng Chong > --- > fs/fuse/virtio_fs.c | 5 - > 1 file changed, 5 deletions(-) The function was never used... Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 09/10] vduse: Introduce VDUSE - vDPA Device in Userspace
在 2021/4/13 下午12:28, Yongji Xie 写道:
On Tue, Apr 13, 2021 at 11:35 AM Jason Wang wrote:
在 2021/4/12 下午5:59, Yongji Xie 写道:
On Mon, Apr 12, 2021 at 5:37 PM Jason Wang wrote:
在 2021/4/12 下午4:02, Yongji Xie 写道:
On Mon, Apr 12, 2021 at 3:16 PM Jason Wang wrote:
在 2021/4/9 下午4:02, Yongji Xie 写道:
+};
+
+struct vduse_dev_config_data {
+ __u32 offset; /* offset from the beginning of config space */
+ __u32 len; /* the length to read/write */
+ __u8 data[VDUSE_CONFIG_DATA_LEN]; /* data buffer used to read/write */
Note that since VDUSE_CONFIG_DATA_LEN is part of uAPI it means we can
not change it in the future.
So this might suffcient for future features or all type of virtio devices.
Do you mean 256 is no enough here？
Yes.
But this request will be submitted multiple times if config lengh is
larger than 256. So do you think whether we need to extent the size to
512 or larger?
So I think you'd better either:
1) document the limitation (256) in somewhere, (better both uapi and doc)
But the VDUSE_CONFIG_DATA_LEN doesn't mean the limitation of
configuration space. It only means the maximum size of one data
transfer for configuration space. Do you mean document this?
Yes, and another thing is that since you're using
data[VDUSE_CONFIG_DATA_LEN] in the uapi, it implies the length is always
256 which seems not good and not what the code is wrote.
How about renaming VDUSE_CONFIG_DATA_LEN to VDUSE_MAX_TRANSFER_LEN?
Thanks,
Yongji
So a question is the reason to have a limitation of this in the uAPI?
Note that in vhost-vdpa we don't have such:
struct vhost_vdpa_config {
__u32 off;
__u32 len;
__u8 buf[0];
};
If so, we need to call read()/write() multiple times each time
receiving/sending one request or response in userspace and kernel. For
example,
1. read and check request/response type
2. read and check config length if type is VDUSE_SET_CONFIG or VDUSE_GET_CONFIG
3. read the payload
Not sure if it's worth it.
Thanks,
Yongji
Right, I see.
So I'm fine with current approach.
Thanks
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 08/10] virtio-net: xsk zero copy xmit setup
在 2021/4/14 下午3:36, Magnus Karlsson 写道:
On Tue, Apr 13, 2021 at 9:58 AM Xuan Zhuo wrote:
xsk is a high-performance packet receiving and sending technology.
This patch implements the binding and unbinding operations of xsk and
the virtio-net queue for xsk zero copy xmit.
The xsk zero copy xmit depends on tx napi. So if tx napi is not true,
an error will be reported. And the entire operation is under the
protection of rtnl_lock.
If xsk is active, it will prevent ethtool from modifying tx napi.
Signed-off-by: Xuan Zhuo
Reviewed-by: Dust Li
---
drivers/net/virtio_net.c | 78 +++-
1 file changed, 77 insertions(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f52a25091322..8242a9e9f17d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
static int napi_weight = NAPI_POLL_WEIGHT;
module_param(napi_weight, int, 0444);
@@ -133,6 +134,11 @@ struct send_queue {
struct virtnet_sq_stats stats;
struct napi_struct napi;
+
+ struct {
+ /* xsk pool */
+ struct xsk_buff_pool __rcu *pool;
+ } xsk;
};
/* Internal representation of a receive virtqueue */
@@ -2249,8 +2255,19 @@ static int virtnet_set_coalesce(struct net_device *dev,
if (napi_weight ^ vi->sq[0].napi.weight) {
if (dev->flags & IFF_UP)
return -EBUSY;
- for (i = 0; i < vi->max_queue_pairs; i++)
+ for (i = 0; i < vi->max_queue_pairs; i++) {
+ /* xsk xmit depend on the tx napi. So if xsk is active,
+* prevent modifications to tx napi.
+*/
+ rcu_read_lock();
+ if (rcu_dereference(vi->sq[i].xsk.pool)) {
+ rcu_read_unlock();
+ continue;
+ }
+ rcu_read_unlock();
+
vi->sq[i].napi.weight = napi_weight;
+ }
}
return 0;
@@ -2518,11 +2535,70 @@ static int virtnet_xdp_set(struct net_device *dev,
struct bpf_prog *prog,
return err;
}
+static int virtnet_xsk_pool_enable(struct net_device *dev,
+ struct xsk_buff_pool *pool,
+ u16 qid)
+{
+ struct virtnet_info *vi = netdev_priv(dev);
+ struct send_queue *sq;
+
+ if (qid >= vi->curr_queue_pairs)
+ return -EINVAL;
Your implementation is the first implementation that only supports
zerocopy for one out of Rx and Tx, and this will currently confuse the
control plane in some situations since it assumes that both Rx and Tx
are enabled by a call to this NDO. For example: user space creates an
xsk socket with both an Rx and a Tx ring, then calls bind with the
XDP_ZEROCOPY flag. In this case, the call should fail if the device is
virtio-net since it only supports zerocopy for Tx. But it should
succeed if the user only created a Tx ring since that makes it a
Tx-only socket which can be supported.
So you need to introduce a new interface in xdp_sock_drv.h that can be
used to ask if this socket has Rx enabled and if so fail the call (at
least one of them has to be enabled, otherwise the bind call would
fail before this ndo is called). Then the logic above will act on that
and try to fall back to copy mode (if allowed). Such an interface
(with an added "is_tx_enabled") might in the future be useful for
physical NIC drivers too if they would like to save on resources for
Tx-only and Rx-only sockets. Currently, they all just assume every
socket is Rx and Tx.
So if there's no blocker for implementing the zerocopy RX, I think we'd
better to implement it in this series without introducing new APIs for
the upper layer.
Thanks
Thanks: Magnus
+
+ sq = &vi->sq[qid];
+
+ /* xsk zerocopy depend on the tx napi.
+*
+* xsk zerocopy xmit is driven by the tx interrupt. When the device is
+* not busy, napi will be called continuously to send data. When the
+* device is busy, wait for the notification interrupt after the
+* hardware has finished processing the data, and continue to send data
+* in napi.
+*/
+ if (!sq->napi.weight)
+ return -EPERM;
+
+ rcu_read_lock();
+ /* Here is already protected by rtnl_lock, so rcu_assign_pointer is
+* safe.
+*/
+ rcu_assign_pointer(sq->xsk.pool, pool);
+ rcu_read_unlock();
+
+ return 0;
+}
+
+static int virtnet_xsk_pool_disable(struct net_device *dev, u16 qid)
+{
+ struct virtnet_info *vi = netdev_priv(dev);
+ struct send_queue *sq;
+
+ if (qid >= vi->curr_queue_pairs)
+ return -EINVAL;
+
+ sq = &vi->sq[qid];
+
+ /* Here is already protected
Re: [PATCH v6 00/10] Introduce VDUSE - vDPA Device in Userspace
在 2021/4/14 下午3:34, Michael S. Tsirkin 写道: On Wed, Mar 31, 2021 at 04:05:09PM +0800, Xie Yongji wrote: This series introduces a framework, which can be used to implement vDPA Devices in a userspace program. The work consist of two parts: control path forwarding and data path offloading. In the control path, the VDUSE driver will make use of message mechnism to forward the config operation from vdpa bus driver to userspace. Userspace can use read()/write() to receive/reply those control messages. In the data path, the core is mapping dma buffer into VDUSE daemon's address space, which can be implemented in different ways depending on the vdpa bus to which the vDPA device is attached. In virtio-vdpa case, we implements a MMU-based on-chip IOMMU driver with bounce-buffering mechanism to achieve that. And in vhost-vdpa case, the dma buffer is reside in a userspace memory region which can be shared to the VDUSE userspace processs via transferring the shmfd. The details and our user case is shown below: - -- |Container || QEMU(VM) | | VDUSE daemon | | - || --- | | - | | |dev/vdx| || |/dev/vhost-vdpa-x| | | | vDPA device emulation | | block driver | | +--- ---+ -+--+- | || | | || | +---++--+- || block device | | vhost device || vduse driver | | TCP/IP || |---+ +---+ -+| | | | | || | --+-- --+--- ---+--- || | | virtio-blk driver | | vhost-vdpa driver | | vdpa device | || | --+-- --+--- ---+--- || | | virtio bus | | || | ++--- | | || || | | || | --+--| | || | | virtio-blk device || | || | --+--| | || || | | || | ---+--- | | || | | virtio-vdpa driver | | | || | ---+--- | | || || | |vdpa bus || | ---+--+---+ || | ---+--- | -| NIC |-- ---+--- | -+- | Remote Storages | --- This all looks quite similar to vhost-user-block except that one does not need any kernel support at all. So I am still scratching my head about its advantages over vhost-user-block. We make use of it to implement a block device connecting to our distributed storage, which can be used both in containers and VMs. Thus, we can have an unified technology stack in this two cases. Maybe the container part is the answer. How does that stack look? Yong Ji may add more and I think this has been demonstrated in the above figure: the userspace vDPA device can provi
Re: [PATCH v6 00/10] Introduce VDUSE - vDPA Device in Userspace
On Wed, Mar 31, 2021 at 04:05:09PM +0800, Xie Yongji wrote: > This series introduces a framework, which can be used to implement > vDPA Devices in a userspace program. The work consist of two parts: > control path forwarding and data path offloading. > > In the control path, the VDUSE driver will make use of message > mechnism to forward the config operation from vdpa bus driver > to userspace. Userspace can use read()/write() to receive/reply > those control messages. > > In the data path, the core is mapping dma buffer into VDUSE > daemon's address space, which can be implemented in different ways > depending on the vdpa bus to which the vDPA device is attached. > > In virtio-vdpa case, we implements a MMU-based on-chip IOMMU driver with > bounce-buffering mechanism to achieve that. And in vhost-vdpa case, the dma > buffer is reside in a userspace memory region which can be shared to the > VDUSE userspace processs via transferring the shmfd. > > The details and our user case is shown below: > > - > -- > |Container || QEMU(VM) | | > VDUSE daemon | > | - || --- | | > - | > | |dev/vdx| || |/dev/vhost-vdpa-x| | | | vDPA device > emulation | | block driver | | > +--- ---+ > -+--+- > | || > | > | || > | > +---++--+- > || block device | | vhost device || vduse driver | > | TCP/IP || > |---+ +---+ > -+| > | | | | > || > | --+-- --+--- ---+--- > || > | | virtio-blk driver | | vhost-vdpa driver | | vdpa device | > || > | --+-- --+--- ---+--- > || > | | virtio bus | | > || > | ++--- | | > || > || | | > || > | --+--| | > || > | | virtio-blk device || | > || > | --+--| | > || > || | | > || > | ---+--- | | > || > | | virtio-vdpa driver | | | > || > | ---+--- | | > || > || | |vdpa > bus || > | > ---+--+---+ > || > | >---+--- | > -| > NIC |-- > >---+--- > > | > > -+- > > | Remote Storages | > > --- This all looks quite similar to vhost-user-block except that one does not need any kernel support at all. So I am still scratching my head about its advantages over vhost-user-block. > We make use of it to implement a block device connecting to > our distributed storage, which can be used both in containers and > VMs. Thus, we can have an unified technology stack in this two cases. Maybe the container part is the answer. How does that stack loo
Re: [RFC v2] virtio-vsock: add description for datagram type
On Wed, Apr 14, 2021 at 08:57:06AM +0200, Stefano Garzarella wrote:
> On Tue, Apr 13, 2021 at 03:58:34PM -0400, Michael S. Tsirkin wrote:
> > On Tue, Apr 13, 2021 at 04:03:51PM +0200, Stefano Garzarella wrote:
> > > On Tue, Apr 13, 2021 at 09:50:45AM -0400, Michael S. Tsirkin wrote:
> > > > On Tue, Apr 13, 2021 at 03:38:52PM +0200, Stefano Garzarella wrote:
> > > > > On Tue, Apr 13, 2021 at 09:16:50AM -0400, Michael S. Tsirkin wrote:
> > > > > > On Tue, Apr 13, 2021 at 02:58:53PM +0200, Stefano Garzarella wrote:
> > > > > > > On Mon, Apr 12, 2021 at 03:42:23PM -0700, Jiang Wang . wrote:
> > > > > > > > On Mon, Apr 12, 2021 at 7:21 AM Stefano Garzarella
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > On Mon, Apr 12, 2021 at 02:50:17PM +0100, Stefan Hajnoczi
> > > > > > > > > wrote:
> > > > > > > > > >On Thu, Apr 01, 2021 at 04:36:02AM +, jiang.wang wrote:
> > > > > > > > > >> Add supports for datagram type for virtio-vsock. Datagram
> > > > > > > > > >> sockets are connectionless and unreliable. To avoid
> > > > > > > > > >> contention
> > > > > > > > > >> with stream and other sockets, add two more virtqueues and
> > > > > > > > > >> a new feature bit to identify if those two new queues
> > > > > > > > > >> exist or not.
> > > > > > > > > >>
> > > > > > > > > >> Also add descriptions for resource management of datagram,
> > > > > > > > > >> which
> > > > > > > > > >> does not use the existing credit update mechanism
> > > > > > > > > >> associated with
> > > > > > > > > >> stream sockets.
> > > > > > > > > >>
> > > > > > > > > >> Signed-off-by: Jiang Wang
> > > > > > > > > >> ---
> > > > > > > > > >> V2 addressed the comments for the previous version.
> > > > > > > > > >>
> > > > > > > > > >> virtio-vsock.tex | 62
> > > > > > > > > >> +++-
> > > > > > > > > >> 1 file changed, 52 insertions(+), 10 deletions(-)
> > > > > > > > > >>
> > > > > > > > > >> diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> > > > > > > > > >> index da7e641..62c12e0 100644
> > > > > > > > > >> --- a/virtio-vsock.tex
> > > > > > > > > >> +++ b/virtio-vsock.tex
> > > > > > > > > >> @@ -11,12 +11,25 @@
> > > > > > > > > >> \subsection{Virtqueues}\label{sec:Device Types / Socket
> > > > > > > > > >> Device / Virtqueues}
> > > > > > > > > >> \begin{description}
> > > > > > > > > >> \item[0] rx
> > > > > > > > > >> \item[1] tx
> > > > > > > > > >> +\item[2] datagram rx
> > > > > > > > > >> +\item[3] datagram tx
> > > > > > > > > >> +\item[4] event
> > > > > > > > > >> +\end{description}
> > > > > > > > > >> +The virtio socket device uses 5 queues if feature bit
> > > > > > > > > >> VIRTIO_VSOCK_F_DRGAM is set. Otherwise, it
> > > > > > > > > >> +only uses 3 queues, as the following. Rx and tx queues
> > > > > > > > > >> are always used for stream sockets.
> > > > > > > > > >> +
> > > > > > > > > >> +\begin{description}
> > > > > > > > > >> +\item[0] rx
> > > > > > > > > >> +\item[1] tx
> > > > > > > > > >> \item[2] event
> > > > > > > > > >> \end{description}
> > > > > > > > > >>
> > > > > > > > > >
> > > > > > > > > >I suggest renaming "rx" and "tx" to "stream rx" and "stream
> > > > > > > > > >tx"
> > > > > > > > > >virtqueues and also adding this:
> > > > > > > > > >
> > > > > > > > > > When behavior differs between stream and datagram rx/tx
> > > > > > > > > > virtqueues
> > > > > > > > > > their full names are used. Common behavior is simply
> > > > > > > > > > described in
> > > > > > > > > > terms of rx/tx virtqueues and applies to both stream and
> > > > > > > > > > datagram
> > > > > > > > > > virtqueues.
> > > > > > > > > >
> > > > > > > > > >This way you won't need to duplicate portions of the spec
> > > > > > > > > >that deal with
> > > > > > > > > >populating the virtqueues, for example.
> > > > > > > > > >
> > > > > > > > > >It's also clearer to use a full name for stream rx/tx
> > > > > > > > > >virtqueues instead
> > > > > > > > > >of calling them rx/tx virtqueues now that we have datagram
> > > > > > > > > >rx/tx
> > > > > > > > > >virtqueues.
> > > > > > > > > >
> > > > > > > > > >> +
> > > > > > > > > >> \subsection{Feature bits}\label{sec:Device Types / Socket
> > > > > > > > > >> Device / Feature bits}
> > > > > > > > > >>
> > > > > > > > > >> -There are currently no feature bits defined for this
> > > > > > > > > >> device.
> > > > > > > > > >> +\begin{description}
> > > > > > > > > >> +\item[VIRTIO_VSOCK_F_DGRAM (0)] Device has support for
> > > > > > > > > >> datagram socket type.
> > > > > > > > > >> +\end{description}
> > > > > > > > > >>
> > > > > > > > > >> \subsection{Device configuration layout}\label{sec:Device
> > > > > > > > > >> Types / Socket Device / Device configuration layout}
> > > > > > > > > >>
> > > > > > > > > >> @@ -107,6 +120,9 @@ \subsection{Device
> > > > > > > > > >> Operation}\label{sec:Device Types / Socket Device / Device
> > > > > > > > > >> Op
> > > > > > > > > >>
> > > > > > > >
Re: [RFC v2] virtio-vsock: add description for datagram type
On Tue, Apr 13, 2021 at 03:00:50PM -0700, Jiang Wang . wrote:
On Tue, Apr 13, 2021 at 12:58 PM Michael S. Tsirkin wrote:
On Tue, Apr 13, 2021 at 04:03:51PM +0200, Stefano Garzarella wrote:
> On Tue, Apr 13, 2021 at 09:50:45AM -0400, Michael S. Tsirkin wrote:
> > On Tue, Apr 13, 2021 at 03:38:52PM +0200, Stefano Garzarella wrote:
> > > On Tue, Apr 13, 2021 at 09:16:50AM -0400, Michael S. Tsirkin wrote:
> > > > On Tue, Apr 13, 2021 at 02:58:53PM +0200, Stefano Garzarella wrote:
> > > > > On Mon, Apr 12, 2021 at 03:42:23PM -0700, Jiang Wang . wrote:
> > > > > > On Mon, Apr 12, 2021 at 7:21 AM Stefano Garzarella
wrote:
> > > > > > >
> > > > > > > On Mon, Apr 12, 2021 at 02:50:17PM +0100, Stefan Hajnoczi wrote:
> > > > > > > >On Thu, Apr 01, 2021 at 04:36:02AM +, jiang.wang wrote:
> > > > > > > >> Add supports for datagram type for virtio-vsock. Datagram
> > > > > > > >> sockets are connectionless and unreliable. To avoid contention
> > > > > > > >> with stream and other sockets, add two more virtqueues and
> > > > > > > >> a new feature bit to identify if those two new queues exist or
not.
> > > > > > > >>
> > > > > > > >> Also add descriptions for resource management of datagram,
which
> > > > > > > >> does not use the existing credit update mechanism associated
with
> > > > > > > >> stream sockets.
> > > > > > > >>
> > > > > > > >> Signed-off-by: Jiang Wang
> > > > > > > >> ---
> > > > > > > >> V2 addressed the comments for the previous version.
> > > > > > > >>
> > > > > > > >> virtio-vsock.tex | 62
+++-
> > > > > > > >> 1 file changed, 52 insertions(+), 10 deletions(-)
> > > > > > > >>
> > > > > > > >> diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> > > > > > > >> index da7e641..62c12e0 100644
> > > > > > > >> --- a/virtio-vsock.tex
> > > > > > > >> +++ b/virtio-vsock.tex
> > > > > > > >> @@ -11,12 +11,25 @@ \subsection{Virtqueues}\label{sec:Device
Types / Socket Device / Virtqueues}
> > > > > > > >> \begin{description}
> > > > > > > >> \item[0] rx
> > > > > > > >> \item[1] tx
> > > > > > > >> +\item[2] datagram rx
> > > > > > > >> +\item[3] datagram tx
> > > > > > > >> +\item[4] event
> > > > > > > >> +\end{description}
> > > > > > > >> +The virtio socket device uses 5 queues if feature bit
VIRTIO_VSOCK_F_DRGAM is set. Otherwise, it
> > > > > > > >> +only uses 3 queues, as the following. Rx and tx queues are
always used for stream sockets.
> > > > > > > >> +
> > > > > > > >> +\begin{description}
> > > > > > > >> +\item[0] rx
> > > > > > > >> +\item[1] tx
> > > > > > > >> \item[2] event
> > > > > > > >> \end{description}
> > > > > > > >>
> > > > > > > >
> > > > > > > >I suggest renaming "rx" and "tx" to "stream rx" and "stream tx"
> > > > > > > >virtqueues and also adding this:
> > > > > > > >
> > > > > > > > When behavior differs between stream and datagram rx/tx
virtqueues
> > > > > > > > their full names are used. Common behavior is simply described
in
> > > > > > > > terms of rx/tx virtqueues and applies to both stream and
datagram
> > > > > > > > virtqueues.
> > > > > > > >
> > > > > > > >This way you won't need to duplicate portions of the spec that
deal with
> > > > > > > >populating the virtqueues, for example.
> > > > > > > >
> > > > > > > >It's also clearer to use a full name for stream rx/tx virtqueues
instead
> > > > > > > >of calling them rx/tx virtqueues now that we have datagram rx/tx
> > > > > > > >virtqueues.
> > > > > > > >
> > > > > > > >> +
> > > > > > > >> \subsection{Feature bits}\label{sec:Device Types / Socket
Device / Feature bits}
> > > > > > > >>
> > > > > > > >> -There are currently no feature bits defined for this device.
> > > > > > > >> +\begin{description}
> > > > > > > >> +\item[VIRTIO_VSOCK_F_DGRAM (0)] Device has support for
datagram socket type.
> > > > > > > >> +\end{description}
> > > > > > > >>
> > > > > > > >> \subsection{Device configuration layout}\label{sec:Device
Types / Socket Device / Device configuration layout}
> > > > > > > >>
> > > > > > > >> @@ -107,6 +120,9 @@ \subsection{Device
Operation}\label{sec:Device Types / Socket Device / Device Op
> > > > > > > >>
> > > > > > > >> \subsubsection{Virtqueue Flow Control}\label{sec:Device Types
/ Socket Device / Device Operation / Virtqueue Flow Control}
> > > > > > > >>
> > > > > > > >> +Flow control applies to stream sockets; datagram sockets do
not have
> > > > > > > >> +flow control.
> > > > > > > >> +
> > > > > > > >> The tx virtqueue carries packets initiated by applications
and replies to
> > > > > > > >> received packets. The rx virtqueue carries packets initiated
by the device and
> > > > > > > >> replies to previously transmitted packets.
> > > > > > > >> @@ -140,12 +156,15 @@
\subsubsection{Addressing}\label{sec:Device Types / Socket Device / Device Opera
> > > > > > > >> consists of a (cid, port number) tuple. The header fields
used for this are
> > > > > > > >> \field{src_cid}, \field{src_port},
