Re: [PATCH v2 2/2] rust: virtio: add virtio support
Hi Wedson, Martin,
First of all, thanks for the review.
> > + /// VirtIO driver remove.
> > + ///
> > + /// Called when a virtio device is removed.
> > + /// Implementers should prepare the device for complete
> > removal here.
> > + ///
> > + /// In particular, implementers must ensure no buffers are
> > left over in the
> > + /// virtqueues in use by calling [`virtqueue::get_buf()`]
> > until `None` is
> > + /// returned.
>
> What happens if implementers don't do this?
>
> If this is a safety requirement, we need to find a different way to
> enforce it.
>
> >
This is the worst part of this patch by far, unfortunately. If one
doesn't do this, then s/he will leak the `data` field passed in through
into_foreign() here:
> +// SAFETY: `self.ptr` is valid as per the type invariant.
> +let res = unsafe {
> +bindings::virtqueue_add_sgs(
> +self.ptr,
> +sgs.as_mut_ptr(),
> +num_out as u32,
> +num_in as u32,
> +data.into_foreign() as _,
> +gfp,
> +)
> +};
> +
Note the comment here:
> +// SAFETY: if there is a buffer token, it came from
> +// `into_foreign()` as called in `add_sgs()`.
> +::from_foreign(buf)
To be honest, I tried coming up with something clever to solve this,
but couldn't. Ideally this should happen when this function is called:
> +extern "C" fn remove_callback(virtio_device: *mut
bindings::virtio_device) {
But I did not find a way to iterate over the the `vqs` member from the
Rust side, i.e.:
```
struct virtio_device {
int index;
bool failed;
bool config_enabled;
bool config_change_pending;
spinlock_t config_lock;
spinlock_t vqs_list_lock; /* Protects VQs list access */
struct device dev;
struct virtio_device_id id;
const struct virtio_config_ops *config;
const struct vringh_config_ops *vringh_config;
struct list_head vqs; <--
```
Is there any wrappers over list_for_each_entry(), etc, to be used from
Rust? If so, I could not find them.
Doing this cleanup from Virtqueue::Drop() is not an option either:
since we wrap over a pointer owned by the kernel, there's no guarantee
that the actual virtqueue is going away when drop is called on the
wrapper. In fact, this is never the case, as virtqueues are deleted
through this call:
> +void rust_helper_virtio_del_vqs(struct virtio_device *vdev)
> +{
> + vdev->config->del_vqs(vdev);
> +}
> +
Suggestions welcome,
-- Daniel
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vdpa: solidrun: constify pointers to hwmon_channel_info
On 4/7/23 08:01, Krzysztof Kozlowski wrote:
Statically allocated array of pointed to hwmon_channel_info can be made
const for safety.
Signed-off-by: Krzysztof Kozlowski
---
This depends on hwmon core patch:
https://lore.kernel.org/all/20230406203103.3011503-2-krzysztof.kozlow...@linaro.org/
Therefore I propose this should also go via hwmon tree.
I am not going to apply patches for 10+ subsystems through the hwmon tree.
This can only result in chaos. The dependent patch is available at
git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
hwmon-const
or wait until after the next commit window to apply this patch.
Thanks,
Guenter
Cc: Jean Delvare
Cc: Guenter Roeck
Cc: linux-hw...@vger.kernel.org
---
drivers/vdpa/solidrun/snet_hwmon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/vdpa/solidrun/snet_hwmon.c
b/drivers/vdpa/solidrun/snet_hwmon.c
index e695e36ff753..65304354b34a 100644
--- a/drivers/vdpa/solidrun/snet_hwmon.c
+++ b/drivers/vdpa/solidrun/snet_hwmon.c
@@ -159,7 +159,7 @@ static const struct hwmon_ops snet_hwmon_ops = {
.read_string = snet_hwmon_read_string
};
-static const struct hwmon_channel_info *snet_hwmon_info[] = {
+static const struct hwmon_channel_info * const snet_hwmon_info[] = {
HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
HWMON_T_LABEL,
HWMON_T_INPUT | HWMON_T_CRIT | HWMON_T_LABEL),
HWMON_CHANNEL_INFO(power, HWMON_P_INPUT | HWMON_P_LABEL),
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] drm/qxl: remove unused num_relocs variable
On Fri, Mar 31, 2023 at 10:24 AM Tom Rix wrote:
>
> clang with W=1 reports
> drivers/gpu/drm/qxl/qxl_ioctl.c:149:14: error: variable
> 'num_relocs' set but not used [-Werror,-Wunused-but-set-variable]
> int i, ret, num_relocs;
> ^
> This variable is not used so remove it.
>
> Signed-off-by: Tom Rix
Thanks for the patch!
Fixes: 74d9a6335dce ("drm/qxl: Simplify cleaning qxl processing command")
Reviewed-by: Nick Desaulniers
That commit should also have removed the label out_free_bos IMO since
having two labels for the same statement is a code smell. Tom, do you
mind sending a v2 with that folded in?
> ---
> drivers/gpu/drm/qxl/qxl_ioctl.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
> index 30f58b21372a..3422206d59d4 100644
> --- a/drivers/gpu/drm/qxl/qxl_ioctl.c
> +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
> @@ -146,7 +146,7 @@ static int qxl_process_single_command(struct qxl_device
> *qdev,
> struct qxl_release *release;
> struct qxl_bo *cmd_bo;
> void *fb_cmd;
> - int i, ret, num_relocs;
> + int i, ret;
> int unwritten;
>
> switch (cmd->type) {
> @@ -201,7 +201,6 @@ static int qxl_process_single_command(struct qxl_device
> *qdev,
> }
>
> /* fill out reloc info structs */
> - num_relocs = 0;
> for (i = 0; i < cmd->relocs_num; ++i) {
> struct drm_qxl_reloc reloc;
> struct drm_qxl_reloc __user *u = u64_to_user_ptr(cmd->relocs);
> @@ -231,7 +230,6 @@ static int qxl_process_single_command(struct qxl_device
> *qdev,
> reloc_info[i].dst_bo = cmd_bo;
> reloc_info[i].dst_offset = reloc.dst_offset +
> release->release_offset;
> }
> - num_relocs++;
>
> /* reserve and validate the reloc dst bo */
> if (reloc.reloc_type == QXL_RELOC_TYPE_BO ||
> reloc.src_handle) {
> --
> 2.27.0
>
--
Thanks,
~Nick Desaulniers
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support
> > This function is used by virtio_vdpa as well > > (drivers/virtio/virtio_vdpa.c:virtio_vdpa_finalize_features). > > A vDPA device can offer this feature and it will be accepted, even though > > VIRTIO_F_NOTIFICATION_DATA is not a thing for the vDPA transport at the > > moment. > > > > I don't know if this is bad, since offering VIRTIO_F_NOTIFICATION_DATA is > > meaningless for a vDPA device at the moment. > > > > I submitted a patch adding support for vDPA transport. > > https://lore.kernel.org/virtualization/20230402081034.1021886-1-alvaro.ka...@solid-run.com/T/#u > > Hmm. So it seems we need to first apply yours then this patch, > is that right? Or the other way around? What is the right way to make it not > break bisect? My patch should be applied on top of Viktor's, it uses one of the functions created by Viktor: vring_notification_data. Viktor's patch without mine won't break bisect, it will just accept the feature for a virtio_vdpa without any meaning. > Do you mind including this patch with yours in a patchset > in the correct order? Viktor, since you did most of the work here, maybe you want to create the patchset? I can do it myself if you prefer so. Thanks, Alvaro ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vdpa: solidrun: constify pointers to hwmon_channel_info
> Statically allocated array of pointed to hwmon_channel_info can be made > const for safety. > > Signed-off-by: Krzysztof Kozlowski > Thanks! Reviewed-by: Alvaro Karsz ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vdpa: solidrun: constify pointers to hwmon_channel_info
On Fri, Apr 07, 2023 at 05:01:30PM +0200, Krzysztof Kozlowski wrote:
> Statically allocated array of pointed to hwmon_channel_info can be made
> const for safety.
>
> Signed-off-by: Krzysztof Kozlowski
sure, merge it as appropriate
Acked-by: Michael S. Tsirkin
> ---
>
> This depends on hwmon core patch:
> https://lore.kernel.org/all/20230406203103.3011503-2-krzysztof.kozlow...@linaro.org/
>
> Therefore I propose this should also go via hwmon tree.
>
> Cc: Jean Delvare
> Cc: Guenter Roeck
> Cc: linux-hw...@vger.kernel.org
> ---
> drivers/vdpa/solidrun/snet_hwmon.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/solidrun/snet_hwmon.c
> b/drivers/vdpa/solidrun/snet_hwmon.c
> index e695e36ff753..65304354b34a 100644
> --- a/drivers/vdpa/solidrun/snet_hwmon.c
> +++ b/drivers/vdpa/solidrun/snet_hwmon.c
> @@ -159,7 +159,7 @@ static const struct hwmon_ops snet_hwmon_ops = {
> .read_string = snet_hwmon_read_string
> };
>
> -static const struct hwmon_channel_info *snet_hwmon_info[] = {
> +static const struct hwmon_channel_info * const snet_hwmon_info[] = {
> HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> HWMON_T_LABEL,
> HWMON_T_INPUT | HWMON_T_CRIT | HWMON_T_LABEL),
> HWMON_CHANNEL_INFO(power, HWMON_P_INPUT | HWMON_P_LABEL),
> --
> 2.34.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 2/2] vdpa_sim_blk: support shared backend
The vdpa_sim_blk simulator uses a ramdisk as the backend. To test live
migration, we need two devices that share the backend to have the data
synchronized with each other.
Add a new module parameter to make the buffer shared between all devices.
The shared_buffer_mutex is used just to ensure that each operation is
atomic, but it is up to the user to use the devices knowing that the
underlying ramdisk is shared.
For example, when we do a migration, the VMM (e.g., QEMU) will guarantee
to write to the destination device, only after completing operations with
the source device.
Signed-off-by: Stefano Garzarella
---
drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 57
1 file changed, 50 insertions(+), 7 deletions(-)
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
index c996e750dc02..00d7d72713be 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -46,6 +46,7 @@
struct vdpasim_blk {
struct vdpasim vdpasim;
void *buffer;
+ bool shared_backend;
};
static struct vdpasim_blk *sim_to_blk(struct vdpasim *vdpasim)
@@ -55,6 +56,26 @@ static struct vdpasim_blk *sim_to_blk(struct vdpasim
*vdpasim)
static char vdpasim_blk_id[VIRTIO_BLK_ID_BYTES] = "vdpa_blk_sim";
+static bool shared_backend;
+module_param(shared_backend, bool, 0444);
+MODULE_PARM_DESC(shared_backend, "Enable the shared backend between virtio-blk
devices");
+
+static void *shared_buffer;
+/* mutex to synchronize shared_buffer access */
+static DEFINE_MUTEX(shared_buffer_mutex);
+
+static void vdpasim_blk_buffer_lock(struct vdpasim_blk *blk)
+{
+ if (blk->shared_backend)
+ mutex_lock(_buffer_mutex);
+}
+
+static void vdpasim_blk_buffer_unlock(struct vdpasim_blk *blk)
+{
+ if (blk->shared_backend)
+ mutex_unlock(_buffer_mutex);
+}
+
static bool vdpasim_blk_check_range(struct vdpasim *vdpasim, u64 start_sector,
u64 num_sectors, u64 max_sectors)
{
@@ -154,8 +175,10 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
break;
}
+ vdpasim_blk_buffer_lock(blk);
bytes = vringh_iov_push_iotlb(>vring, >in_iov,
blk->buffer + offset, to_push);
+ vdpasim_blk_buffer_unlock(blk);
if (bytes < 0) {
dev_dbg(>vdpa.dev,
"vringh_iov_push_iotlb() error: %zd offset:
0x%llx len: 0x%zx\n",
@@ -175,8 +198,10 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
break;
}
+ vdpasim_blk_buffer_lock(blk);
bytes = vringh_iov_pull_iotlb(>vring, >out_iov,
blk->buffer + offset, to_pull);
+ vdpasim_blk_buffer_unlock(blk);
if (bytes < 0) {
dev_dbg(>vdpa.dev,
"vringh_iov_pull_iotlb() error: %zd offset:
0x%llx len: 0x%zx\n",
@@ -256,8 +281,10 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
}
if (type == VIRTIO_BLK_T_WRITE_ZEROES) {
+ vdpasim_blk_buffer_lock(blk);
memset(blk->buffer + offset, 0,
num_sectors << SECTOR_SHIFT);
+ vdpasim_blk_buffer_unlock(blk);
}
break;
@@ -366,7 +393,8 @@ static void vdpasim_blk_free(struct vdpasim *vdpasim)
{
struct vdpasim_blk *blk = sim_to_blk(vdpasim);
- kvfree(blk->buffer);
+ if (!blk->shared_backend)
+ kvfree(blk->buffer);
}
static void vdpasim_blk_mgmtdev_release(struct device *dev)
@@ -404,12 +432,17 @@ static int vdpasim_blk_dev_add(struct vdpa_mgmt_dev
*mdev, const char *name,
return PTR_ERR(simdev);
blk = sim_to_blk(simdev);
-
- blk->buffer = kvmalloc(VDPASIM_BLK_CAPACITY << SECTOR_SHIFT,
- GFP_KERNEL);
- if (!blk->buffer) {
- ret = -ENOMEM;
- goto put_dev;
+ blk->shared_backend = shared_backend;
+
+ if (blk->shared_backend) {
+ blk->buffer = shared_buffer;
+ } else {
+ blk->buffer = kvmalloc(VDPASIM_BLK_CAPACITY << SECTOR_SHIFT,
+ GFP_KERNEL);
+ if (!blk->buffer) {
+ ret = -ENOMEM;
+ goto put_dev;
+ }
}
ret = _vdpa_register_device(>vdpa, VDPASIM_BLK_VQ_NUM);
@@ -461,6 +494,15 @@ static int __init vdpasim_blk_init(void)
if (ret)
goto parent_err;
+ if (shared_backend) {
+ shared_buffer = kvmalloc(VDPASIM_BLK_CAPACITY << SECTOR_SHIFT,
+GFP_KERNEL);
[PATCH 0/2] vdpa_sim_blk: support shared backend
This series is mainly for testing live migration between 2 vdpa_sim_blk devices. The first patch is preparation and moves the buffer allocation into devices, the second patch adds the `shared_buffer_mutex` parameter to vdpa_sim_blk to use the same ramdisk for all devices. Tested with QEMU v8.0.0-rc2 in this way: modprobe vhost_vdpa modprobe vdpa_sim_blk shared_backend=true vdpa dev add mgmtdev vdpasim_blk name blk0 vdpa dev add mgmtdev vdpasim_blk name blk1 qemu-system-x86_64 -m 512M -smp 2 -M q35,accel=kvm,memory-backend=mem \ -object memory-backend-file,share=on,id=mem,size="512M",mem-path="/dev/shm" ... -blockdev node-name=drive_src1,driver=virtio-blk-vhost-vdpa,path=/dev/vhost-vdpa-1,cache.direct=on \ -device virtio-blk-pci,id=src1,bootindex=2,drive=drive_src1 \ -incoming tcp:0: qemu-system-x86_64 -m 512M -smp 2 -M q35,accel=kvm,memory-backend=mem \ -object memory-backend-file,share=on,id=mem,size="512M",mem-path="/dev/shm" ... -blockdev node-name=drive_src1,driver=virtio-blk-vhost-vdpa,path=/dev/vhost-vdpa-0,cache.direct=on \ -device virtio-blk-pci,id=src1,bootindex=2,drive=drive_src1 (qemu) migrate -d tcp:0: Stefano Garzarella (2): vdpa_sim: move buffer allocation in the devices vdpa_sim_blk: support shared backend drivers/vdpa/vdpa_sim/vdpa_sim.h | 3 +- drivers/vdpa/vdpa_sim/vdpa_sim.c | 7 +-- drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 83 +--- drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 28 +++--- 4 files changed, 100 insertions(+), 21 deletions(-) -- 2.39.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 1/2] vdpa_sim: move buffer allocation in the devices
Currently, the vdpa_sim core does not use the buffer, but only
allocates it.
The buffer is used by devices differently, and some future devices
may not use it. So let's move all its management inside the devices.
Add a new `free` device callback called to clean up the resources
allocated by the device.
Signed-off-by: Stefano Garzarella
---
drivers/vdpa/vdpa_sim/vdpa_sim.h | 3 +--
drivers/vdpa/vdpa_sim/vdpa_sim.c | 7 ++---
drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 40 +++-
drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 28 ++-
4 files changed, 57 insertions(+), 21 deletions(-)
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
index 3a42887d05d9..bb137e479763 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
@@ -39,7 +39,6 @@ struct vdpasim_dev_attr {
u64 supported_features;
size_t alloc_size;
size_t config_size;
- size_t buffer_size;
int nvqs;
u32 id;
u32 ngroups;
@@ -51,6 +50,7 @@ struct vdpasim_dev_attr {
int (*get_stats)(struct vdpasim *vdpasim, u16 idx,
struct sk_buff *msg,
struct netlink_ext_ack *extack);
+ void (*free)(struct vdpasim *vdpasim);
};
/* State of each vdpasim device */
@@ -67,7 +67,6 @@ struct vdpasim {
void *config;
struct vhost_iotlb *iommu;
bool *iommu_pt;
- void *buffer;
u32 status;
u32 generation;
u64 features;
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 2c706bb18897..d343af4fa60e 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -261,10 +261,6 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr
*dev_attr,
for (i = 0; i < vdpasim->dev_attr.nas; i++)
vhost_iotlb_init(>iommu[i], max_iotlb_entries, 0);
- vdpasim->buffer = kvmalloc(dev_attr->buffer_size, GFP_KERNEL);
- if (!vdpasim->buffer)
- goto err_iommu;
-
for (i = 0; i < dev_attr->nvqs; i++)
vringh_set_iotlb(>vqs[i].vring, >iommu[0],
>iommu_lock);
@@ -714,7 +710,8 @@ static void vdpasim_free(struct vdpa_device *vdpa)
vringh_kiov_cleanup(>vqs[i].in_iov);
}
- kvfree(vdpasim->buffer);
+ vdpasim->dev_attr.free(vdpasim);
+
for (i = 0; i < vdpasim->dev_attr.nas; i++)
vhost_iotlb_reset(>iommu[i]);
kfree(vdpasim->iommu);
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
index 568119e1553f..c996e750dc02 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -43,6 +43,16 @@
#define VDPASIM_BLK_AS_NUM 1
#define VDPASIM_BLK_GROUP_NUM 1
+struct vdpasim_blk {
+ struct vdpasim vdpasim;
+ void *buffer;
+};
+
+static struct vdpasim_blk *sim_to_blk(struct vdpasim *vdpasim)
+{
+ return container_of(vdpasim, struct vdpasim_blk, vdpasim);
+}
+
static char vdpasim_blk_id[VIRTIO_BLK_ID_BYTES] = "vdpa_blk_sim";
static bool vdpasim_blk_check_range(struct vdpasim *vdpasim, u64 start_sector,
@@ -78,6 +88,7 @@ static bool vdpasim_blk_check_range(struct vdpasim *vdpasim,
u64 start_sector,
static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
struct vdpasim_virtqueue *vq)
{
+ struct vdpasim_blk *blk = sim_to_blk(vdpasim);
size_t pushed = 0, to_pull, to_push;
struct virtio_blk_outhdr hdr;
bool handled = false;
@@ -144,8 +155,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
}
bytes = vringh_iov_push_iotlb(>vring, >in_iov,
- vdpasim->buffer + offset,
- to_push);
+ blk->buffer + offset, to_push);
if (bytes < 0) {
dev_dbg(>vdpa.dev,
"vringh_iov_push_iotlb() error: %zd offset:
0x%llx len: 0x%zx\n",
@@ -166,8 +176,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
}
bytes = vringh_iov_pull_iotlb(>vring, >out_iov,
- vdpasim->buffer + offset,
- to_pull);
+ blk->buffer + offset, to_pull);
if (bytes < 0) {
dev_dbg(>vdpa.dev,
"vringh_iov_pull_iotlb() error: %zd offset:
0x%llx len: 0x%zx\n",
@@ -247,7 +256,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
}
if (type == VIRTIO_BLK_T_WRITE_ZEROES) {
- memset(vdpasim->buffer + offset, 0,
+ memset(blk->buffer +
Re: [PATCH vhost v6 08/11] virtio_ring: introduce virtqueue_dma_dev()
On 4/6/23 20:17, Xuan Zhuo wrote:
On Thu, 6 Apr 2023 10:20:09 -0700, Guenter Roeck wrote:
Hi,
On Mon, Mar 27, 2023 at 12:05:33PM +0800, Xuan Zhuo wrote:
Added virtqueue_dma_dev() to get DMA device for virtio. Then the
caller can do dma operation in advance. The purpose is to keep memory
mapped across multiple add/get buf operations.
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
On sparc64, this patch results in
[4.364643] Unable to handle kernel NULL pointer dereference
[4.365157] tsk->{mm,active_mm}->context =
[4.365394] tsk->{mm,active_mm}->pgd = f8402000
[4.365611] \|/ \|/
[4.365611] "@'/ .. \`@"
[4.365611] /_| \__/ |_\
[4.365611] \__U_/
[4.366165] swapper/0(1): Oops [#1]
[4.366630] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G N
6.3.0-rc5-next-20230405 #1
[4.367121] TSTATE: 004411001606 TPC: 004375c0 TNPC:
004375c4 Y: 0002Tainted: G N
[4.367548] TPC:
[4.368111] g0: g1: 01a93a50 g2:
g3: 01a96170
[4.368439] g4: f8000416e9a0 g5: f8001dc98000 g6: f8000417
g7: 0005
[4.368769] o0: o1: o2: 0001
o3: f800040b78d8
[4.369095] o4: o5: sp: f80004172d51
ret_pc: 004375ac
[4.369934] RPC:
[4.370160] l0: 0002 l1: 0002 l2:
l3:
[4.370493] l4: 0001 l5: 0119d2b0 l6: f8000416e9a0
l7: 018df000
[4.370820] i0: 0001 i1: i2: f80004657758
i3: 0125ac00
[4.371145] i4: 02362400 i5: i6: f80004172e01
i7: 0050e288
[4.371473] I7:
[4.371683] Call Trace:
[4.371864] [<0050e288>] dma_set_mask+0x28/0x80
[4.372123] [<00a83234>] virtio_dev_probe+0x14/0x400
[4.372348] [<00ac7a18>] really_probe+0xb8/0x340
[4.372555] [<00ac7d64>] driver_probe_device+0x24/0x120
[4.372794] [<00ac8010>] __driver_attach+0x90/0x1a0
[4.373012] [<00ac5b4c>] bus_for_each_dev+0x4c/0xa0
[4.373226] [<00ac6d80>] bus_add_driver+0x140/0x1e0
[4.373440] [<00ac8d94>] driver_register+0x74/0x120
[4.373653] [<01b2a690>] virtio_net_driver_init+0x74/0xa8
[4.373886] [<00427ee0>] do_one_initcall+0x60/0x340
[4.374099] [<01b02f50>] kernel_init_freeable+0x1bc/0x228
[4.374330] [<00f37264>] kernel_init+0x18/0x134
[4.374534] [<004060e8>] ret_from_fork+0x1c/0x2c
[4.374738] [<>] 0x0
[4.374981] Disabling lock debugging due to kernel taint
[4.375237] Caller[0050e288]: dma_set_mask+0x28/0x80
[4.375477] Caller[00a83234]: virtio_dev_probe+0x14/0x400
[4.375704] Caller[00ac7a18]: really_probe+0xb8/0x340
[4.375916] Caller[00ac7d64]: driver_probe_device+0x24/0x120
[4.376146] Caller[00ac8010]: __driver_attach+0x90/0x1a0
[4.376365] Caller[00ac5b4c]: bus_for_each_dev+0x4c/0xa0
[4.376583] Caller[00ac6d80]: bus_add_driver+0x140/0x1e0
[4.376805] Caller[00ac8d94]: driver_register+0x74/0x120
[4.377025] Caller[01b2a690]: virtio_net_driver_init+0x74/0xa8
[4.377262] Caller[00427ee0]: do_one_initcall+0x60/0x340
[4.377480] Caller[01b02f50]: kernel_init_freeable+0x1bc/0x228
[4.377721] Caller[00f37264]: kernel_init+0x18/0x134
[4.377930] Caller[004060e8]: ret_from_fork+0x1c/0x2c
[4.378140] Caller[]: 0x0
[4.378309] Instruction DUMP:
[4.378339] 80a22000
[4.378556] 1246
[4.378654] b0102001
[4.378746]
[4.378838] b0102000
[4.378930] 80a04019
[4.379022] b1653001
[4.379115] 81cfe008
[4.379507] 913a2000
[4.379617]
[4.380504] Kernel panic - not syncing: Attempted to kill init!
exitcode=0x0009
Reverting it fixes the problem. Bisect log attached.
The reason is that dma_supported() calls dma_4u_supported(), which
expects that dev->archdata.iommu has been initialized. However,
this is not the case for the virtio device. Result is a null pointer
dereference in dma_4u_supported().
static int dma_4u_supported(struct device *dev, u64 device_mask)
{
struct iommu *iommu = dev->archdata.iommu;
if (ali_sound_dma_hack(dev, device_mask))
return 1;
if (device_mask < iommu->dma_addr_mask)
Crash location
return 0;
return 1;
}
sparc64 does not support direct dma?
The virtio is a virtul device based on pci.
Do you know how we should adapt to sparc64?
It isn't exactly common or typical to set dev->dma_mask in driver
Re: [PATCH vhost v6 08/11] virtio_ring: introduce virtqueue_dma_dev()
On Thu, Apr 06, 2023 at 10:20:09AM -0700, Guenter Roeck wrote:
> Hi,
>
> On Mon, Mar 27, 2023 at 12:05:33PM +0800, Xuan Zhuo wrote:
> > Added virtqueue_dma_dev() to get DMA device for virtio. Then the
> > caller can do dma operation in advance. The purpose is to keep memory
> > mapped across multiple add/get buf operations.
> >
> > Signed-off-by: Xuan Zhuo
> > Acked-by: Jason Wang
>
> On sparc64, this patch results in
>
> [4.364643] Unable to handle kernel NULL pointer dereference
> [4.365157] tsk->{mm,active_mm}->context =
> [4.365394] tsk->{mm,active_mm}->pgd = f8402000
> [4.365611] \|/ \|/
> [4.365611] "@'/ .. \`@"
> [4.365611] /_| \__/ |_\
> [4.365611] \__U_/
> [4.366165] swapper/0(1): Oops [#1]
> [4.366630] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G N
> 6.3.0-rc5-next-20230405 #1
> [4.367121] TSTATE: 004411001606 TPC: 004375c0 TNPC:
> 004375c4 Y: 0002Tainted: G N
> [4.367548] TPC:
> [4.368111] g0: g1: 01a93a50 g2:
> g3: 01a96170
> [4.368439] g4: f8000416e9a0 g5: f8001dc98000 g6: f8000417
> g7: 0005
> [4.368769] o0: o1: o2: 0001
> o3: f800040b78d8
> [4.369095] o4: o5: sp: f80004172d51
> ret_pc: 004375ac
> [4.369934] RPC:
> [4.370160] l0: 0002 l1: 0002 l2:
> l3:
> [4.370493] l4: 0001 l5: 0119d2b0 l6: f8000416e9a0
> l7: 018df000
> [4.370820] i0: 0001 i1: i2: f80004657758
> i3: 0125ac00
> [4.371145] i4: 02362400 i5: i6: f80004172e01
> i7: 0050e288
> [4.371473] I7:
> [4.371683] Call Trace:
> [4.371864] [<0050e288>] dma_set_mask+0x28/0x80
> [4.372123] [<00a83234>] virtio_dev_probe+0x14/0x400
> [4.372348] [<00ac7a18>] really_probe+0xb8/0x340
> [4.372555] [<00ac7d64>] driver_probe_device+0x24/0x120
> [4.372794] [<00ac8010>] __driver_attach+0x90/0x1a0
> [4.373012] [<00ac5b4c>] bus_for_each_dev+0x4c/0xa0
> [4.373226] [<00ac6d80>] bus_add_driver+0x140/0x1e0
> [4.373440] [<00ac8d94>] driver_register+0x74/0x120
> [4.373653] [<01b2a690>] virtio_net_driver_init+0x74/0xa8
> [4.373886] [<00427ee0>] do_one_initcall+0x60/0x340
> [4.374099] [<01b02f50>] kernel_init_freeable+0x1bc/0x228
> [4.374330] [<00f37264>] kernel_init+0x18/0x134
> [4.374534] [<004060e8>] ret_from_fork+0x1c/0x2c
> [4.374738] [<>] 0x0
> [4.374981] Disabling lock debugging due to kernel taint
> [4.375237] Caller[0050e288]: dma_set_mask+0x28/0x80
> [4.375477] Caller[00a83234]: virtio_dev_probe+0x14/0x400
> [4.375704] Caller[00ac7a18]: really_probe+0xb8/0x340
> [4.375916] Caller[00ac7d64]: driver_probe_device+0x24/0x120
> [4.376146] Caller[00ac8010]: __driver_attach+0x90/0x1a0
> [4.376365] Caller[00ac5b4c]: bus_for_each_dev+0x4c/0xa0
> [4.376583] Caller[00ac6d80]: bus_add_driver+0x140/0x1e0
> [4.376805] Caller[00ac8d94]: driver_register+0x74/0x120
> [4.377025] Caller[01b2a690]: virtio_net_driver_init+0x74/0xa8
> [4.377262] Caller[00427ee0]: do_one_initcall+0x60/0x340
> [4.377480] Caller[01b02f50]: kernel_init_freeable+0x1bc/0x228
> [4.377721] Caller[00f37264]: kernel_init+0x18/0x134
> [4.377930] Caller[004060e8]: ret_from_fork+0x1c/0x2c
> [4.378140] Caller[]: 0x0
> [4.378309] Instruction DUMP:
> [4.378339] 80a22000
> [4.378556] 1246
> [4.378654] b0102001
> [4.378746]
> [4.378838] b0102000
> [4.378930] 80a04019
> [4.379022] b1653001
> [4.379115] 81cfe008
> [4.379507] 913a2000
> [4.379617]
> [4.380504] Kernel panic - not syncing: Attempted to kill init!
> exitcode=0x0009
>
> Reverting it fixes the problem. Bisect log attached.
>
> The reason is that dma_supported() calls dma_4u_supported(), which
> expects that dev->archdata.iommu has been initialized. However,
> this is not the case for the virtio device. Result is a null pointer
> dereference in dma_4u_supported().
>
> static int dma_4u_supported(struct device *dev, u64 device_mask)
> {
> struct iommu *iommu = dev->archdata.iommu;
>
> if (ali_sound_dma_hack(dev, device_mask))
> return 1;
>
> if (device_mask < iommu->dma_addr_mask)
> Crash location
> return 0;
> return 1;
> }
>
> Guenter
This
