[PATCH linux-next v2] vdpa: Fix memory leak in error unwinding path
When device get command fails to find the device or mdev, it skips to free the skb during error unwinding path. Fix it by freeing in error unwind path. While at it, make error unwind path more clear to avoid such errors. Fixes: a12a2f694ce8 ("vdpa: Enable user to query vdpa device info") Signed-off-by: Parav Pandit --- changelog: v1->v2: - Addressed Stefano's comment to make error unwind code more clear --- drivers/vdpa/vdpa.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index 3d997b389345..da67f07e24fd 100644 --- a/drivers/vdpa/vdpa.c +++ b/drivers/vdpa/vdpa.c @@ -538,22 +538,22 @@ static int vdpa_nl_cmd_dev_get_doit(struct sk_buff *skb, struct genl_info *info) mutex_lock(&vdpa_dev_mutex); dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match); if (!dev) { - mutex_unlock(&vdpa_dev_mutex); NL_SET_ERR_MSG_MOD(info->extack, "device not found"); - return -ENODEV; + err = -ENODEV; + goto err; } vdev = container_of(dev, struct vdpa_device, dev); if (!vdev->mdev) { - mutex_unlock(&vdpa_dev_mutex); - put_device(dev); - return -EINVAL; + err = -EINVAL; + goto mdev_err; } err = vdpa_dev_fill(vdev, msg, info->snd_portid, info->snd_seq, 0, info->extack); if (!err) err = genlmsg_reply(msg, info); +mdev_err: put_device(dev); +err: mutex_unlock(&vdpa_dev_mutex); - if (err) nlmsg_free(msg); return err; -- 2.26.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1] vdpa/mlx5: Restore the hardware used index after change map
On 2/10/2021 7:45 AM, Eli Cohen wrote: On Wed, Feb 10, 2021 at 12:59:03AM -0800, Si-Wei Liu wrote: On 2/9/2021 7:53 PM, Jason Wang wrote: On 2021/2/10 上午10:30, Si-Wei Liu wrote: On 2/8/2021 10:37 PM, Jason Wang wrote: On 2021/2/9 下午2:12, Eli Cohen wrote: On Tue, Feb 09, 2021 at 11:20:14AM +0800, Jason Wang wrote: On 2021/2/8 下午6:04, Eli Cohen wrote: On Mon, Feb 08, 2021 at 05:04:27PM +0800, Jason Wang wrote: On 2021/2/8 下午2:37, Eli Cohen wrote: On Mon, Feb 08, 2021 at 12:27:18PM +0800, Jason Wang wrote: On 2021/2/6 上午7:07, Si-Wei Liu wrote: On 2/3/2021 11:36 PM, Eli Cohen wrote: When a change of memory map occurs, the hardware resources are destroyed and then re-created again with the new memory map. In such case, we need to restore the hardware available and used indices. The driver failed to restore the used index which is added here. Also, since the driver also fails to reset the available and used indices upon device reset, fix this here to avoid regression caused by the fact that used index may not be zero upon device reset. Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices") Signed-off-by: Eli Cohen --- v0 -> v1: Clear indices upon device reset drivers/vdpa/mlx5/net/mlx5_vnet.c | 18 ++ 1 file changed, 18 insertions(+) diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c index 88dde3455bfd..b5fe6d2ad22f 100644 --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c @@ -87,6 +87,7 @@ struct mlx5_vq_restore_info { u64 device_addr; u64 driver_addr; u16 avail_index; + u16 used_index; bool ready; struct vdpa_callback cb; bool restore; @@ -121,6 +122,7 @@ struct mlx5_vdpa_virtqueue { u32 virtq_id; struct mlx5_vdpa_net *ndev; u16 avail_idx; + u16 used_idx; int fw_state; /* keep last in the struct */ @@ -804,6 +806,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque obj_context = MLX5_ADDR_OF(create_virtio_net_q_in, in, obj_context); MLX5_SET(virtio_net_q_object, obj_context, hw_available_index, mvq->avail_idx); + MLX5_SET(virtio_net_q_object, obj_context, hw_used_index, mvq->used_idx); MLX5_SET(virtio_net_q_object, obj_context, queue_feature_bit_mask_12_3, get_features_12_3(ndev->mvdev.actual_features)); vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context); @@ -1022,6 +1025,7 @@ static int connect_qps(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m struct mlx5_virtq_attr { u8 state; u16 available_index; + u16 used_index; }; static int query_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq, @@ -1052,6 +1056,7 @@ static int query_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueu memset(attr, 0, sizeof(*attr)); attr->state = MLX5_GET(virtio_net_q_object, obj_context, state); attr->available_index = MLX5_GET(virtio_net_q_object, obj_context, hw_available_index); + attr->used_index = MLX5_GET(virtio_net_q_object, obj_context, hw_used_index); kfree(out); return 0; @@ -1535,6 +1540,16 @@ static void teardown_virtqueues(struct mlx5_vdpa_net *ndev) } } +static void clear_virtqueues(struct mlx5_vdpa_net *ndev) +{ + int i; + + for (i = ndev->mvdev.max_vqs - 1; i >= 0; i--) { + ndev->vqs[i].avail_idx = 0; + ndev->vqs[i].used_idx = 0; + } +} + /* TODO: cross-endian support */ static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev) { @@ -1610,6 +1625,7 @@ static int save_channel_info(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqu return err; ri->avail_index = attr.available_index; + ri->used_index = attr.used_index; ri->ready = mvq->ready; ri->num_ent = mvq->num_ent; ri->desc_addr = mvq->desc_addr; @@ -1654,6 +1670,7 @@ static void restore_channels_info(struct mlx5_vdpa_net *ndev) continue; mvq->avail_idx = ri->avail_index; + mvq->used_idx = ri->used_index; mvq->ready = ri->ready; mvq->num_ent = ri->num_ent; mvq->desc_addr = ri->desc_addr; @@ -1768,6 +1785,7 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) if (!status) { mlx5_vdpa_info(mvdev, "performing device reset\n"); teardown_driver(ndev); + clear_virtqueues(ndev); The clearing looks fine at the first glance, as it aligns with the other state cleanups floating around at the same place. However, the thing is get_vq_state() is supposed to be called right after to get sync'ed with the latest internal avail_index from device while vq is stopped. The index was saved in the driver software at
Re: [PATCH v5 4/8] x86/mm/tlb: Flush remote and local TLBs concurrently
> On Feb 16, 2021, at 4:10 AM, Peter Zijlstra wrote: > > On Tue, Feb 09, 2021 at 02:16:49PM -0800, Nadav Amit wrote: >> @@ -816,8 +821,8 @@ STATIC_NOPV void native_flush_tlb_others(const struct >> cpumask *cpumask, >> * doing a speculative memory access. >> */ >> if (info->freed_tables) { >> -smp_call_function_many(cpumask, flush_tlb_func, >> - (void *)info, 1); >> +on_each_cpu_cond_mask(NULL, flush_tlb_func, (void *)info, true, >> + cpumask); >> } else { >> /* >> * Although we could have used on_each_cpu_cond_mask(), >> @@ -844,14 +849,15 @@ STATIC_NOPV void native_flush_tlb_others(const struct >> cpumask *cpumask, >> if (tlb_is_not_lazy(cpu)) >> __cpumask_set_cpu(cpu, cond_cpumask); >> } >> -smp_call_function_many(cond_cpumask, flush_tlb_func, (void >> *)info, 1); >> +on_each_cpu_cond_mask(NULL, flush_tlb_func, (void *)info, true, >> + cpumask); >> } >> } > > Surely on_each_cpu_mask() is more appropriate? There the compiler can do > the NULL propagation because it's on the same TU. > > --- a/arch/x86/mm/tlb.c > +++ b/arch/x86/mm/tlb.c > @@ -821,8 +821,7 @@ STATIC_NOPV void native_flush_tlb_multi( >* doing a speculative memory access. >*/ > if (info->freed_tables) { > - on_each_cpu_cond_mask(NULL, flush_tlb_func, (void *)info, true, > - cpumask); > + on_each_cpu_mask(cpumask, flush_tlb_func, (void *)info, true); > } else { > /* >* Although we could have used on_each_cpu_cond_mask(), > @@ -849,8 +848,7 @@ STATIC_NOPV void native_flush_tlb_multi( > if (tlb_is_not_lazy(cpu)) > __cpumask_set_cpu(cpu, cond_cpumask); > } > - on_each_cpu_cond_mask(NULL, flush_tlb_func, (void *)info, true, > - cpumask); > + on_each_cpu_mask(cpumask, flush_tlb_func, (void *)info, true); > } > } Indeed, and there is actually an additional bug - I used cpumask in the second on_each_cpu_cond_mask() instead of cond_cpumask. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [External] Re: vsock virtio: questions about supporting DGRAM type
On Tue, Feb 16, 2021 at 12:50 AM Stefano Garzarella wrote: > > On Tue, Feb 16, 2021 at 12:23:19AM -0800, Jiang Wang . wrote: > >On Tue, Feb 16, 2021 at 12:09 AM Stefano Garzarella > >wrote: > >> > >> On Mon, Feb 15, 2021 at 08:50:36PM -0800, Jiang Wang . wrote: > >> >On Mon, Feb 15, 2021 at 12:31 AM Stefano Garzarella > >> >wrote: > >> >> > >> >> On Sat, Feb 13, 2021 at 03:26:18PM -0800, Jiang Wang . wrote: > >> >> >On Fri, Feb 12, 2021 at 1:02 AM Stefano Garzarella > >> >> > wrote: > >> >> >> > >> >> >> Hi Jiang, > >> >> >> > >> >> >> CCing Arseny who is working on SOCK_SEQPACKET support for > >> >> >> virtio-vsock > >> >> >> [1]. > >> >> >> > >> >> >> On Thu, Feb 11, 2021 at 10:04:34PM -0800, Jiang Wang . wrote: > >> >> >> >Hi guys, > >> >> >> > > >> >> >> >I am working on supporting DGRAM type for virtio/vhost vsock. I > >> >> >> >already did some work and a draft code is here (which passed my > >> >> >> >tests, > >> >> >> >but still need some cleanup and only works from host to guest as of > >> >> >> >now, will add host to guest soon): > >> >> >> >https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d > >> >> >> >qemu changes are here: > >> >> >> >https://github.com/Jiang1155/qemu/commit/7ab778801e3e8969ab98e44539943810a2fb03eb > >> >> >> > > >> >> >> >Today, I just noticed that the Asias had an old version of virtio > >> >> >> >which had both dgram and stream support, see this link: > >> >> >> >https://kvm.vger.kernel.narkive.com/BMvH9eEr/rfc-v2-0-7-introduce-vm-sockets-virtio-transport#post1 > >> >> >> > > >> >> >> >But somehow, the dgram part seems never merged to upstream linux > >> >> >> >(the > >> >> >> >stream part is merged). If so, does anyone know what is the reason > >> >> >> >for > >> >> >> >this? Did we drop dgram support for some specific reason or the code > >> >> >> >needs some improvement? > >> >> >> > >> >> >> I wasn't involved yet in virtio-vsock development when Asias posted > >> >> >> that > >> >> >> patches, so I don't know the exact reason. > >> >> >> > >> >> >> Maybe could be related on how to handle the credit mechanism for a > >> >> >> connection-less sockets and how to manage the packet queue, if for > >> >> >> example no one is listening. > >> >> >> > >> >> > > >> >> >I see. thanks. > >> >> > > >> >> >> > > >> >> >> >My current code differs from Asias' code in some ways. It does not > >> >> >> >use > >> >> >> >credit and does not support fragmentation. It basically adds two > >> >> >> >virt > >> >> >> > >> >> >> If you don't use credit, do you have some threshold when you start to > >> >> >> drop packets on the RX side? > >> >> >> > >> >> > > >> >> >As of now, I don't have any threshold to drop packets on RX side. In my > >> >> >view, DGRAM is like UDP and is a best effort service. If the virtual > >> >> >queue > >> >> >is full on TX (or the available buffer size is less than the > >> >> >packet size), > >> >> >I drop the packets on the TX side. > >> >> > >> >> I have to check the code, my concern is we should have a limit for the > >> >> allocation, for example if the user space doesn't consume RX packets. > >> >> > >> > > >> >I think we already have a limit for the allocation. If a DGRAM client > >> >sends packets while no socket is bound on the server side , > >> >the packet will be dropped when looking for the socket. If the socket is > >> >bound on the server side, but the server program somehow does not > >> >call recv or similar functions, the packets will be dropped when the > >> >buffer is full as in your previous patch at here: > >> >https://lore.kernel.org/patchwork/patch/1141083/ > >> >If there are still other cases that we don't have protection, let me know > >> >and > >> >I can add some threshold. > >> > >> Maybe I misunderstood, but I thought you didn't use credit for DGRAM > >> messages. > >> > >I don't use credit for DGRAM. But the dropping code I mentioned does not > >rely on credit too. Just to make it clear, following is one part of code that > >I referred to: > > > >static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs, > >struct virtio_vsock_pkt *pkt) > >{ > >if (vvs->rx_bytes + pkt->len > vvs->buf_alloc) > >return false; > > > >vvs->rx_bytes += pkt->len; > >return true; > >} > > Okay, now it's clear. The transmitter doesn't care about the receiver's > credit, but the receiver uses that part of the credit mechanism to > discard packets. > > I think that's fine, but when you send the patches, explain it well in > the cover letter/commit message. > Sure, thanks. > > > > > >> If you use it to limit buffer occupancy, then it should be okay, but > >> again, I still have to look at the code :-) > > > >Sure. I think you mean you need to look at my final patches. I will > >send it later. Just a friendly reminder, if you just want to get some > >idea of my patches, you could check this link : > >https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d > > It's my fault because
Re: [PATCH] virtio_console: remove pointless check for debugfs_create_dir()
On Tue, 2021-02-16 at 16:04 +0100, Greg Kroah-Hartman wrote: > It is impossible for debugfs_create_dir() to return NULL, so checking > for it gives people a false sense that they actually are doing something > if an error occurs. As there is no need to ever change kernel logic if > debugfs is working "properly" or not, there is no need to check the > return value of debugfs calls, so remove the checks here as they will > never be triggered and are wrong. > > Cc: Amit Shah > Cc: Arnd Bergmann > Cc: virtualization@lists.linux-foundation.org > Cc: linux-ker...@vger.kernel.org > Signed-off-by: Greg Kroah-Hartman > --- > drivers/char/virtio_console.c | 23 +-- > 1 file changed, 9 insertions(+), 14 deletions(-) > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > index 1836cc56e357..59dfd9c421a1 100644 > --- a/drivers/char/virtio_console.c > +++ b/drivers/char/virtio_console.c > @@ -1456,18 +1456,15 @@ static int add_port(struct ports_device *portdev, u32 > id) >*/ > send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1); > > - if (pdrvdata.debugfs_dir) { > - /* > - * Finally, create the debugfs file that we can use to > - * inspect a port's state at any time > - */ > - snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u", > - port->portdev->vdev->index, id); > - port->debugfs_file = debugfs_create_file(debugfs_name, 0444, > - pdrvdata.debugfs_dir, > - port, > - &port_debugfs_fops); > - } > + /* > + * Finally, create the debugfs file that we can use to > + * inspect a port's state at any time > + */ > + snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u", > + port->portdev->vdev->index, id); > + port->debugfs_file = debugfs_create_file(debugfs_name, 0444, > + pdrvdata.debugfs_dir, > + port, &port_debugfs_fops); > return 0; > > free_inbufs: > @@ -2244,8 +2241,6 @@ static int __init init(void) > } > > pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL); > - if (!pdrvdata.debugfs_dir) > - pr_warn("Error creating debugfs dir for virtio-ports\n"); > INIT_LIST_HEAD(&pdrvdata.consoles); > INIT_LIST_HEAD(&pdrvdata.portdevs); > Reviewed-by: Amit Shah ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio_console: remove pointless check for debugfs_create_dir()
It is impossible for debugfs_create_dir() to return NULL, so checking for it gives people a false sense that they actually are doing something if an error occurs. As there is no need to ever change kernel logic if debugfs is working "properly" or not, there is no need to check the return value of debugfs calls, so remove the checks here as they will never be triggered and are wrong. Cc: Amit Shah Cc: Arnd Bergmann Cc: virtualization@lists.linux-foundation.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Greg Kroah-Hartman --- drivers/char/virtio_console.c | 23 +-- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index 1836cc56e357..59dfd9c421a1 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -1456,18 +1456,15 @@ static int add_port(struct ports_device *portdev, u32 id) */ send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1); - if (pdrvdata.debugfs_dir) { - /* -* Finally, create the debugfs file that we can use to -* inspect a port's state at any time -*/ - snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u", -port->portdev->vdev->index, id); - port->debugfs_file = debugfs_create_file(debugfs_name, 0444, -pdrvdata.debugfs_dir, -port, -&port_debugfs_fops); - } + /* +* Finally, create the debugfs file that we can use to +* inspect a port's state at any time +*/ + snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u", +port->portdev->vdev->index, id); + port->debugfs_file = debugfs_create_file(debugfs_name, 0444, +pdrvdata.debugfs_dir, +port, &port_debugfs_fops); return 0; free_inbufs: @@ -2244,8 +2241,6 @@ static int __init init(void) } pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL); - if (!pdrvdata.debugfs_dir) - pr_warn("Error creating debugfs dir for virtio-ports\n"); INIT_LIST_HEAD(&pdrvdata.consoles); INIT_LIST_HEAD(&pdrvdata.portdevs); -- 2.30.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH for 5.10 v2 4/5] vdpa_sim: make 'config' generic and usable for any device type
commit f37cbbc65178e0a45823d281d290c4c02da9631c upstream. Add new 'config_size' attribute in 'vdpasim_dev_attr' and allocates 'config' dynamically to support any device types. Acked-by: Jason Wang Signed-off-by: Stefano Garzarella Link: https://lore.kernel.org/r/20201215144256.155342-12-sgarz...@redhat.com Signed-off-by: Michael S. Tsirkin Cc: # 5.10.x Signed-off-by: Stefano Garzarella --- drivers/vdpa/vdpa_sim/vdpa_sim.c | 17 + 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c index bc27fa485c3e..d9c494455156 100644 --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c @@ -70,6 +70,7 @@ static u64 vdpasim_features = (1ULL << VIRTIO_F_ANY_LAYOUT) | (1ULL << VIRTIO_NET_F_MAC); struct vdpasim_dev_attr { + size_t config_size; int nvqs; }; @@ -81,7 +82,8 @@ struct vdpasim { struct vdpasim_dev_attr dev_attr; /* spinlock to synchronize virtqueue state */ spinlock_t lock; - struct virtio_net_config config; + /* virtio config according to device type */ + void *config; struct vhost_iotlb *iommu; void *buffer; u32 status; @@ -380,6 +382,10 @@ static struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *dev_attr) goto err_iommu; set_dma_ops(dev, &vdpasim_dma_ops); + vdpasim->config = kzalloc(dev_attr->config_size, GFP_KERNEL); + if (!vdpasim->config) + goto err_iommu; + vdpasim->vqs = kcalloc(dev_attr->nvqs, sizeof(struct vdpasim_virtqueue), GFP_KERNEL); if (!vdpasim->vqs) @@ -518,7 +524,8 @@ static u64 vdpasim_get_features(struct vdpa_device *vdpa) static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features) { struct vdpasim *vdpasim = vdpa_to_sim(vdpa); - struct virtio_net_config *config = &vdpasim->config; + struct virtio_net_config *config = + (struct virtio_net_config *)vdpasim->config; /* DMA mapping must be done by driver */ if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM))) @@ -588,8 +595,8 @@ static void vdpasim_get_config(struct vdpa_device *vdpa, unsigned int offset, { struct vdpasim *vdpasim = vdpa_to_sim(vdpa); - if (offset + len < sizeof(struct virtio_net_config)) - memcpy(buf, (u8 *)&vdpasim->config + offset, len); + if (offset + len < vdpasim->dev_attr.config_size) + memcpy(buf, vdpasim->config + offset, len); } static void vdpasim_set_config(struct vdpa_device *vdpa, unsigned int offset, @@ -676,6 +683,7 @@ static void vdpasim_free(struct vdpa_device *vdpa) if (vdpasim->iommu) vhost_iotlb_free(vdpasim->iommu); kfree(vdpasim->vqs); + kfree(vdpasim->config); } static const struct vdpa_config_ops vdpasim_net_config_ops = { @@ -736,6 +744,7 @@ static int __init vdpasim_dev_init(void) struct vdpasim_dev_attr dev_attr = {}; dev_attr.nvqs = VDPASIM_VQ_NUM; + dev_attr.config_size = sizeof(struct virtio_net_config); vdpasim_dev = vdpasim_create(&dev_attr); -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH for 5.10 v2 5/5] vdpa_sim: add get_config callback in vdpasim_dev_attr
commit 65b709586e222fa6ffd4166ac7fdb5d5dad113ee upstream. The get_config callback can be used by the device to fill the config structure. The callback will be invoked in vdpasim_get_config() before copying bytes into caller buffer. Move vDPA-net config updates from vdpasim_set_features() in the new vdpasim_net_get_config() callback. This is safe since in vdpa_get_config() we already check that .set_features() callback is called before .get_config(). Signed-off-by: Stefano Garzarella Link: https://lore.kernel.org/r/20201215144256.155342-13-sgarz...@redhat.com Signed-off-by: Michael S. Tsirkin Cc: # 5.10.x Signed-off-by: Stefano Garzarella --- drivers/vdpa/vdpa_sim/vdpa_sim.c | 35 +++- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c index d9c494455156..f2ad450db547 100644 --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c @@ -69,9 +69,12 @@ static u64 vdpasim_features = (1ULL << VIRTIO_F_ANY_LAYOUT) | (1ULL << VIRTIO_F_ACCESS_PLATFORM) | (1ULL << VIRTIO_NET_F_MAC); +struct vdpasim; + struct vdpasim_dev_attr { size_t config_size; int nvqs; + void (*get_config)(struct vdpasim *vdpasim, void *config); }; /* State of each vdpasim device */ @@ -524,8 +527,6 @@ static u64 vdpasim_get_features(struct vdpa_device *vdpa) static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features) { struct vdpasim *vdpasim = vdpa_to_sim(vdpa); - struct virtio_net_config *config = - (struct virtio_net_config *)vdpasim->config; /* DMA mapping must be done by driver */ if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM))) @@ -533,16 +534,6 @@ static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features) vdpasim->features = features & vdpasim_features; - /* We generally only know whether guest is using the legacy interface -* here, so generally that's the earliest we can set config fields. -* Note: We actually require VIRTIO_F_ACCESS_PLATFORM above which -* implies VIRTIO_F_VERSION_1, but let's not try to be clever here. -*/ - - config->mtu = cpu_to_vdpasim16(vdpasim, 1500); - config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP); - memcpy(config->mac, macaddr_buf, ETH_ALEN); - return 0; } @@ -595,8 +586,13 @@ static void vdpasim_get_config(struct vdpa_device *vdpa, unsigned int offset, { struct vdpasim *vdpasim = vdpa_to_sim(vdpa); - if (offset + len < vdpasim->dev_attr.config_size) - memcpy(buf, vdpasim->config + offset, len); + if (offset + len > vdpasim->dev_attr.config_size) + return; + + if (vdpasim->dev_attr.get_config) + vdpasim->dev_attr.get_config(vdpasim, vdpasim->config); + + memcpy(buf, vdpasim->config + offset, len); } static void vdpasim_set_config(struct vdpa_device *vdpa, unsigned int offset, @@ -739,12 +735,23 @@ static const struct vdpa_config_ops vdpasim_net_batch_config_ops = { .free = vdpasim_free, }; +static void vdpasim_net_get_config(struct vdpasim *vdpasim, void *config) +{ + struct virtio_net_config *net_config = + (struct virtio_net_config *)config; + + net_config->mtu = cpu_to_vdpasim16(vdpasim, 1500); + net_config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP); + memcpy(net_config->mac, macaddr_buf, ETH_ALEN); +} + static int __init vdpasim_dev_init(void) { struct vdpasim_dev_attr dev_attr = {}; dev_attr.nvqs = VDPASIM_VQ_NUM; dev_attr.config_size = sizeof(struct virtio_net_config); + dev_attr.get_config = vdpasim_net_get_config; vdpasim_dev = vdpasim_create(&dev_attr); -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH for 5.10 v2 1/5] vdpa_sim: remove hard-coded virtq count
From: Max Gurtovoy commit 423248d60d2b655321fc49eca1545f95a1bc9d6c upstream. Add a new attribute that will define the number of virt queues to be created for the vdpasim device. Signed-off-by: Max Gurtovoy [sgarzare: replace kmalloc_array() with kcalloc()] Acked-by: Jason Wang Signed-off-by: Stefano Garzarella Link: https://lore.kernel.org/r/20201215144256.155342-4-sgarz...@redhat.com Signed-off-by: Michael S. Tsirkin Cc: # 5.10.x Signed-off-by: Stefano Garzarella --- drivers/vdpa/vdpa_sim/vdpa_sim.c | 18 +- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c index 6a90fdb9cbfc..ee8f24a4643b 100644 --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c @@ -70,7 +70,7 @@ static u64 vdpasim_features = (1ULL << VIRTIO_F_ANY_LAYOUT) | /* State of each vdpasim device */ struct vdpasim { struct vdpa_device vdpa; - struct vdpasim_virtqueue vqs[VDPASIM_VQ_NUM]; + struct vdpasim_virtqueue *vqs; struct work_struct work; /* spinlock to synchronize virtqueue state */ spinlock_t lock; @@ -80,6 +80,7 @@ struct vdpasim { u32 status; u32 generation; u64 features; + int nvqs; /* spinlock to synchronize iommu table */ spinlock_t iommu_lock; }; @@ -144,7 +145,7 @@ static void vdpasim_reset(struct vdpasim *vdpasim) { int i; - for (i = 0; i < VDPASIM_VQ_NUM; i++) + for (i = 0; i < vdpasim->nvqs; i++) vdpasim_vq_reset(&vdpasim->vqs[i]); spin_lock(&vdpasim->iommu_lock); @@ -350,7 +351,7 @@ static struct vdpasim *vdpasim_create(void) const struct vdpa_config_ops *ops; struct vdpasim *vdpasim; struct device *dev; - int ret = -ENOMEM; + int i, ret = -ENOMEM; if (batch_mapping) ops = &vdpasim_net_batch_config_ops; @@ -361,6 +362,7 @@ static struct vdpasim *vdpasim_create(void) if (!vdpasim) goto err_alloc; + vdpasim->nvqs = VDPASIM_VQ_NUM; INIT_WORK(&vdpasim->work, vdpasim_work); spin_lock_init(&vdpasim->lock); spin_lock_init(&vdpasim->iommu_lock); @@ -371,6 +373,11 @@ static struct vdpasim *vdpasim_create(void) goto err_iommu; set_dma_ops(dev, &vdpasim_dma_ops); + vdpasim->vqs = kcalloc(vdpasim->nvqs, sizeof(struct vdpasim_virtqueue), + GFP_KERNEL); + if (!vdpasim->vqs) + goto err_iommu; + vdpasim->iommu = vhost_iotlb_alloc(2048, 0); if (!vdpasim->iommu) goto err_iommu; @@ -389,8 +396,8 @@ static struct vdpasim *vdpasim_create(void) eth_random_addr(vdpasim->config.mac); } - vringh_set_iotlb(&vdpasim->vqs[0].vring, vdpasim->iommu); - vringh_set_iotlb(&vdpasim->vqs[1].vring, vdpasim->iommu); + for (i = 0; i < vdpasim->nvqs; i++) + vringh_set_iotlb(&vdpasim->vqs[i].vring, vdpasim->iommu); vdpasim->vdpa.dma_dev = dev; ret = vdpa_register_device(&vdpasim->vdpa); @@ -659,6 +666,7 @@ static void vdpasim_free(struct vdpa_device *vdpa) kfree(vdpasim->buffer); if (vdpasim->iommu) vhost_iotlb_free(vdpasim->iommu); + kfree(vdpasim->vqs); } static const struct vdpa_config_ops vdpasim_net_config_ops = { -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH for 5.10 v2 3/5] vdpa_sim: store parsed MAC address in a buffer
commit cf1a3b35382c10ce315c32bd2b3d7789897fbe13 upstream. As preparation for the next patches, we store the MAC address, parsed during the vdpasim_create(), in a buffer that will be used to fill 'config' together with other configurations. Acked-by: Jason Wang Signed-off-by: Stefano Garzarella Link: https://lore.kernel.org/r/20201215144256.155342-11-sgarz...@redhat.com Signed-off-by: Michael S. Tsirkin Cc: # 5.10.x Signed-off-by: Stefano Garzarella --- drivers/vdpa/vdpa_sim/vdpa_sim.c | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c index 6a5603f9d24c..bc27fa485c3e 100644 --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c @@ -42,6 +42,8 @@ static char *macaddr; module_param(macaddr, charp, 0); MODULE_PARM_DESC(macaddr, "Ethernet MAC address"); +u8 macaddr_buf[ETH_ALEN]; + struct vdpasim_virtqueue { struct vringh vring; struct vringh_kiov iov; @@ -392,13 +394,13 @@ static struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *dev_attr) goto err_iommu; if (macaddr) { - mac_pton(macaddr, vdpasim->config.mac); - if (!is_valid_ether_addr(vdpasim->config.mac)) { + mac_pton(macaddr, macaddr_buf); + if (!is_valid_ether_addr(macaddr_buf)) { ret = -EADDRNOTAVAIL; goto err_iommu; } } else { - eth_random_addr(vdpasim->config.mac); + eth_random_addr(macaddr_buf); } for (i = 0; i < dev_attr->nvqs; i++) @@ -532,6 +534,8 @@ static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features) config->mtu = cpu_to_vdpasim16(vdpasim, 1500); config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP); + memcpy(config->mac, macaddr_buf, ETH_ALEN); + return 0; } -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH for 5.10 v2 2/5] vdpa_sim: add struct vdpasim_dev_attr for device attributes
commit 6c6e28fe45794054410ad8cd2770af69fbe0338d upstream. vdpasim_dev_attr will contain device specific attributes. We starting moving the number of virtqueues (i.e. nvqs) to vdpasim_dev_attr. vdpasim_create() creates a new vDPA simulator following the device attributes defined in the vdpasim_dev_attr parameter. Co-developed-by: Max Gurtovoy Signed-off-by: Max Gurtovoy Acked-by: Jason Wang Signed-off-by: Stefano Garzarella Link: https://lore.kernel.org/r/20201215144256.155342-7-sgarz...@redhat.com Signed-off-by: Michael S. Tsirkin Cc: # 5.10.x Signed-off-by: Stefano Garzarella --- drivers/vdpa/vdpa_sim/vdpa_sim.c | 25 + 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c index ee8f24a4643b..6a5603f9d24c 100644 --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c @@ -67,11 +67,16 @@ static u64 vdpasim_features = (1ULL << VIRTIO_F_ANY_LAYOUT) | (1ULL << VIRTIO_F_ACCESS_PLATFORM) | (1ULL << VIRTIO_NET_F_MAC); +struct vdpasim_dev_attr { + int nvqs; +}; + /* State of each vdpasim device */ struct vdpasim { struct vdpa_device vdpa; struct vdpasim_virtqueue *vqs; struct work_struct work; + struct vdpasim_dev_attr dev_attr; /* spinlock to synchronize virtqueue state */ spinlock_t lock; struct virtio_net_config config; @@ -80,7 +85,6 @@ struct vdpasim { u32 status; u32 generation; u64 features; - int nvqs; /* spinlock to synchronize iommu table */ spinlock_t iommu_lock; }; @@ -145,7 +149,7 @@ static void vdpasim_reset(struct vdpasim *vdpasim) { int i; - for (i = 0; i < vdpasim->nvqs; i++) + for (i = 0; i < vdpasim->dev_attr.nvqs; i++) vdpasim_vq_reset(&vdpasim->vqs[i]); spin_lock(&vdpasim->iommu_lock); @@ -346,7 +350,7 @@ static const struct dma_map_ops vdpasim_dma_ops = { static const struct vdpa_config_ops vdpasim_net_config_ops; static const struct vdpa_config_ops vdpasim_net_batch_config_ops; -static struct vdpasim *vdpasim_create(void) +static struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *dev_attr) { const struct vdpa_config_ops *ops; struct vdpasim *vdpasim; @@ -358,11 +362,12 @@ static struct vdpasim *vdpasim_create(void) else ops = &vdpasim_net_config_ops; - vdpasim = vdpa_alloc_device(struct vdpasim, vdpa, NULL, ops, VDPASIM_VQ_NUM); + vdpasim = vdpa_alloc_device(struct vdpasim, vdpa, NULL, ops, + dev_attr->nvqs); if (!vdpasim) goto err_alloc; - vdpasim->nvqs = VDPASIM_VQ_NUM; + vdpasim->dev_attr = *dev_attr; INIT_WORK(&vdpasim->work, vdpasim_work); spin_lock_init(&vdpasim->lock); spin_lock_init(&vdpasim->iommu_lock); @@ -373,7 +378,7 @@ static struct vdpasim *vdpasim_create(void) goto err_iommu; set_dma_ops(dev, &vdpasim_dma_ops); - vdpasim->vqs = kcalloc(vdpasim->nvqs, sizeof(struct vdpasim_virtqueue), + vdpasim->vqs = kcalloc(dev_attr->nvqs, sizeof(struct vdpasim_virtqueue), GFP_KERNEL); if (!vdpasim->vqs) goto err_iommu; @@ -396,7 +401,7 @@ static struct vdpasim *vdpasim_create(void) eth_random_addr(vdpasim->config.mac); } - for (i = 0; i < vdpasim->nvqs; i++) + for (i = 0; i < dev_attr->nvqs; i++) vringh_set_iotlb(&vdpasim->vqs[i].vring, vdpasim->iommu); vdpasim->vdpa.dma_dev = dev; @@ -724,7 +729,11 @@ static const struct vdpa_config_ops vdpasim_net_batch_config_ops = { static int __init vdpasim_dev_init(void) { - vdpasim_dev = vdpasim_create(); + struct vdpasim_dev_attr dev_attr = {}; + + dev_attr.nvqs = VDPASIM_VQ_NUM; + + vdpasim_dev = vdpasim_create(&dev_attr); if (!IS_ERR(vdpasim_dev)) return 0; -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH for 5.10 v2 0/5] vdpa_sim: fix param validation in vdpasim_get_config()
v1: https://lore.kernel.org/stable/20210211162519.215418-1-sgarz...@redhat.com/ v2: - backport the upstream patch and related patches needed Commit 65b709586e22 ("vdpa_sim: add get_config callback in vdpasim_dev_attr") unintentionally solved an issue in vdpasim_get_config() upstream while refactoring vdpa_sim.c to support multiple devices. Before that patch, if 'offset + len' was equal to sizeof(struct virtio_net_config), the entire buffer wasn't filled, returning incorrect values to the caller. Since 'vdpasim->config' type is 'struct virtio_net_config', we can safely copy its content under this condition. The minimum set of patches to backport the patch that fixes the issue, is the following: 423248d60d2b vdpa_sim: remove hard-coded virtq count 6c6e28fe4579 vdpa_sim: add struct vdpasim_dev_attr for device attributes cf1a3b35382c vdpa_sim: store parsed MAC address in a buffer f37cbbc65178 vdpa_sim: make 'config' generic and usable for any device type 65b709586e22 vdpa_sim: add get_config callback in vdpasim_dev_attr The patches apply fairly cleanly. There are a few contextual differences due to the lack of the other patches: $ git backport-diff -u master -r linux-5.10.y..HEAD Key: [] : patches are identical [] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively 001/5:[] [--] 'vdpa_sim: remove hard-coded virtq count' 002/5:[] [-C] 'vdpa_sim: add struct vdpasim_dev_attr for device attributes' 003/5:[] [--] 'vdpa_sim: store parsed MAC address in a buffer' 004/5:[] [-C] 'vdpa_sim: make 'config' generic and usable for any device type' 005/5:[] [-C] 'vdpa_sim: add get_config callback in vdpasim_dev_attr' Thanks, Stefano Max Gurtovoy (1): vdpa_sim: remove hard-coded virtq count Stefano Garzarella (4): vdpa_sim: add struct vdpasim_dev_attr for device attributes vdpa_sim: store parsed MAC address in a buffer vdpa_sim: make 'config' generic and usable for any device type vdpa_sim: add get_config callback in vdpasim_dev_attr drivers/vdpa/vdpa_sim/vdpa_sim.c | 83 +++- 1 file changed, 60 insertions(+), 23 deletions(-) -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH for 5.10] vdpa_sim: fix param validation in vdpasim_get_config()
On Mon, Feb 15, 2021 at 04:23:54PM +0100, Greg KH wrote: On Mon, Feb 15, 2021 at 04:03:21PM +0100, Stefano Garzarella wrote: On Mon, Feb 15, 2021 at 03:32:19PM +0100, Greg KH wrote: > On Thu, Feb 11, 2021 at 05:25:19PM +0100, Stefano Garzarella wrote: > > Commit 65b709586e222fa6ffd4166ac7fdb5d5dad113ee upstream. > > No, this really is not that commit, so please do not say it is. Oops, sorry. > > > Before this patch, if 'offset + len' was equal to > > sizeof(struct virtio_net_config), the entire buffer wasn't filled, > > returning incorrect values to the caller. > > > > Since 'vdpasim->config' type is 'struct virtio_net_config', we can > > safely copy its content under this condition. > > > > Commit 65b709586e22 ("vdpa_sim: add get_config callback in > > vdpasim_dev_attr") unintentionally solved it upstream while > > refactoring vdpa_sim.c to support multiple devices. But we don't want > > to backport it to stable branches as it contains many changes. > > > > Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator") > > Cc: # 5.10.x > > Signed-off-by: Stefano Garzarella > > --- > > drivers/vdpa/vdpa_sim/vdpa_sim.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c > > index 6a90fdb9cbfc..8ca178d7b02f 100644 > > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c > > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c > > @@ -572,7 +572,7 @@ static void vdpasim_get_config(struct vdpa_device *vdpa, unsigned int offset, > > { > > struct vdpasim *vdpasim = vdpa_to_sim(vdpa); > > > > - if (offset + len < sizeof(struct virtio_net_config)) > > + if (offset + len <= sizeof(struct virtio_net_config)) > > memcpy(buf, (u8 *)&vdpasim->config + offset, len); > > } > > I'll be glad to take a one-off patch, but why can't we take the real > upstream patch? That is always the better long-term solution, right? Because that patch depends on the following patches merged in v5.11-rc1 while refactoring vdpa_sim: f37cbbc65178 vdpa_sim: make 'config' generic and usable for any device type cf1a3b35382c vdpa_sim: store parsed MAC address in a buffer a13b5918fdd0 vdpa_sim: add work_fn in vdpasim_dev_attr 011c35bac5ef vdpa_sim: add supported_features field in vdpasim_dev_attr 2f8f46188805 vdpa_sim: add device id field in vdpasim_dev_attr 6c6e28fe4579 vdpa_sim: add struct vdpasim_dev_attr for device attributes 36a9c3063025 vdpa_sim: rename vdpasim_config_ops variables 423248d60d2b vdpa_sim: remove hard-coded virtq count Maybe we can skip some of them, but IMHO should be less risky to apply only this change. If you want I can try to figure out the minimum sub-set of patches needed for 65b709586e22 ("vdpa_sim: add get_config callback in vdpasim_dev_attr"). The minimum is always nice :) The minimum set, including the patch that fixes the issue, is the following: 65b709586e22 vdpa_sim: add get_config callback in vdpasim_dev_attr f37cbbc65178 vdpa_sim: make 'config' generic and usable for any device type cf1a3b35382c vdpa_sim: store parsed MAC address in a buffer 6c6e28fe4579 vdpa_sim: add struct vdpasim_dev_attr for device attributes 423248d60d2b vdpa_sim: remove hard-coded virtq count The patches apply fairly cleanly. There are a few contextual differences due to the lack of the other patches: $ git backport-diff -u master -r linux-5.10.y..HEAD Key: [] : patches are identical [] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively 001/5:[] [--] 'vdpa_sim: remove hard-coded virtq count' 002/5:[] [-C] 'vdpa_sim: add struct vdpasim_dev_attr for device attributes' 003/5:[] [--] 'vdpa_sim: store parsed MAC address in a buffer' 004/5:[] [-C] 'vdpa_sim: make 'config' generic and usable for any device type' 005/5:[] [-C] 'vdpa_sim: add get_config callback in vdpasim_dev_attr' If it's just too much churn for no good reason, then yes, the one-line change above will be ok, but you need to document the heck out of why this is not upstream and that it is a one-off thing. Shortly I'll send the series to sta...@vger.kernel.org so you can judge if it's okay or better to resend this patch with a better description. Thanks Stefano ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
Am 16.02.21 um 14:27 schrieb Thomas Zimmermann: Hi this is a shadow-buffered plane. Did you consider using the new helpers for shadow-buffered planes? They will map the user BO for you and provide the mapping in the plane state. From there, you should implement your own plane state on top of struct drm_shadow_plane_state, and also move all the other allocations and vmaps into prepare_fb and cleanup_fb. Most of this is not actually allowed in commit tails. All we'd have to do is to export the reset, duplicate and destroy code; similar to what __drm_atomic_helper_plane_reset() does. AFAICT the cursor_bo is used to implement double buffering for the cursor image. Ideally, you can do what ast does: pre-allocate/vmap 2 BOs at the end of the vram. Then pageflip between them in atomic_update(). Resolves all the allocation and mapping headaches. Best regards Thomas Best regards Thomas Am 16.02.21 um 12:37 schrieb Gerd Hoffmann: We don't have to map in atomic_update callback then, making locking a bit less complicated. Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_display.c | 14 +- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 7500560db8e4..39b8c5116d34 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -584,7 +584,6 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, struct drm_gem_object *obj; struct qxl_bo *cursor_bo = NULL, *user_bo = NULL, *old_cursor_bo = NULL; int ret; - struct dma_buf_map user_map; struct dma_buf_map cursor_map; void *user_ptr; int size = 64*64*4; @@ -599,11 +598,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, obj = fb->obj[0]; user_bo = gem_to_qxl_bo(obj); - /* pinning is done in the prepare/cleanup framevbuffer */ - ret = qxl_bo_kmap_locked(user_bo, &user_map); - if (ret) - goto out_free_release; - user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction properly */ + /* mapping is done in the prepare/cleanup framevbuffer */ + user_ptr = user_bo->map.vaddr; /* TODO: Use mapping abstraction properly */ ret = qxl_alloc_bo_reserved(qdev, release, sizeof(struct qxl_cursor) + size, @@ -639,7 +635,6 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, cursor->chunk.data_size = size; memcpy(cursor->chunk.data, user_ptr, size); qxl_bo_kunmap_locked(cursor_bo); - qxl_bo_kunmap_locked(user_bo); cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release); cmd->u.set.visible = 1; @@ -778,6 +773,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane, struct drm_gem_object *obj; struct qxl_bo *user_bo; struct qxl_surface surf; + struct dma_buf_map unused; if (!new_state->fb) return 0; @@ -815,7 +811,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane, } } - return qxl_bo_pin(user_bo); + return qxl_bo_kmap(user_bo, &unused); } static void qxl_plane_cleanup_fb(struct drm_plane *plane, @@ -834,7 +830,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane, obj = old_state->fb->obj[0]; user_bo = gem_to_qxl_bo(obj); - qxl_bo_unpin(user_bo); + qxl_bo_kunmap(user_bo); if (old_state->fb != plane->state->fb && user_bo->shadow) { qxl_bo_unpin(user_bo->shadow); ___ dri-devel mailing list dri-de...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer OpenPGP_signature Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 10/10] drm/qxl: add lock asserts to qxl_bo_kmap_locked + qxl_bo_kunmap_locked
Hi Am 16.02.21 um 12:37 schrieb Gerd Hoffmann: Try avoid re-introducing locking bugs. Signed-off-by: Gerd Hoffmann Acked-by: Thomas Zimmermann IMHO this should eventually be done in the rsp TTM functions. But so far, I'd expect this to break some drivers. Best regards Thomas --- drivers/gpu/drm/qxl/qxl_object.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c index 22748b9566af..90d5e5b7f927 100644 --- a/drivers/gpu/drm/qxl/qxl_object.c +++ b/drivers/gpu/drm/qxl/qxl_object.c @@ -162,6 +162,8 @@ int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map) { int r; + dma_resv_assert_held(bo->tbo.base.resv); + if (bo->kptr) { bo->map_count++; goto out; @@ -236,6 +238,8 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, void qxl_bo_kunmap_locked(struct qxl_bo *bo) { + dma_resv_assert_held(bo->tbo.base.resv); + if (bo->kptr == NULL) return; bo->map_count--; -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer OpenPGP_signature Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
Hi this is a shadow-buffered plane. Did you consider using the new helpers for shadow-buffered planes? They will map the user BO for you and provide the mapping in the plane state. From there, you should implement your own plane state on top of struct drm_shadow_plane_state, and also move all the other allocations and vmaps into prepare_fb and cleanup_fb. Most of this is not actually allowed in commit tails. All we'd have to do is to export the reset, duplicate and destroy code; similar to what __drm_atomic_helper_plane_reset() does. Best regards Thomas Am 16.02.21 um 12:37 schrieb Gerd Hoffmann: We don't have to map in atomic_update callback then, making locking a bit less complicated. Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_display.c | 14 +- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 7500560db8e4..39b8c5116d34 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -584,7 +584,6 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, struct drm_gem_object *obj; struct qxl_bo *cursor_bo = NULL, *user_bo = NULL, *old_cursor_bo = NULL; int ret; - struct dma_buf_map user_map; struct dma_buf_map cursor_map; void *user_ptr; int size = 64*64*4; @@ -599,11 +598,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, obj = fb->obj[0]; user_bo = gem_to_qxl_bo(obj); - /* pinning is done in the prepare/cleanup framevbuffer */ - ret = qxl_bo_kmap_locked(user_bo, &user_map); - if (ret) - goto out_free_release; - user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction properly */ + /* mapping is done in the prepare/cleanup framevbuffer */ + user_ptr = user_bo->map.vaddr; /* TODO: Use mapping abstraction properly */ ret = qxl_alloc_bo_reserved(qdev, release, sizeof(struct qxl_cursor) + size, @@ -639,7 +635,6 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, cursor->chunk.data_size = size; memcpy(cursor->chunk.data, user_ptr, size); qxl_bo_kunmap_locked(cursor_bo); - qxl_bo_kunmap_locked(user_bo); cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release); cmd->u.set.visible = 1; @@ -778,6 +773,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane, struct drm_gem_object *obj; struct qxl_bo *user_bo; struct qxl_surface surf; + struct dma_buf_map unused; if (!new_state->fb) return 0; @@ -815,7 +811,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane, } } - return qxl_bo_pin(user_bo); + return qxl_bo_kmap(user_bo, &unused); } static void qxl_plane_cleanup_fb(struct drm_plane *plane, @@ -834,7 +830,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane, obj = old_state->fb->obj[0]; user_bo = gem_to_qxl_bo(obj); - qxl_bo_unpin(user_bo); + qxl_bo_kunmap(user_bo); if (old_state->fb != plane->state->fb && user_bo->shadow) { qxl_bo_unpin(user_bo->shadow); -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer OpenPGP_signature Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 08/10] drm/qxl: fix monitors object kmap
Am 16.02.21 um 12:37 schrieb Gerd Hoffmann: Use the correct kmap variant. We don't hold a reservation here, so we can't use the _locked variant. We can drop the pin because qxl_bo_kmap will do that for us. Signed-off-by: Gerd Hoffmann Acked-by: Thomas Zimmermann --- drivers/gpu/drm/qxl/qxl_display.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 8672385a1caf..7500560db8e4 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -1159,12 +1159,10 @@ int qxl_create_monitors_object(struct qxl_device *qdev) } qdev->monitors_config_bo = gem_to_qxl_bo(gobj); - ret = qxl_bo_pin(qdev->monitors_config_bo); + ret = qxl_bo_kmap(qdev->monitors_config_bo, &map); if (ret) return ret; - qxl_bo_kmap_locked(qdev->monitors_config_bo, &map); - qdev->monitors_config = qdev->monitors_config_bo->kptr; qdev->ram_header->monitors_config = qxl_bo_physical_address(qdev, qdev->monitors_config_bo, 0); @@ -1189,8 +1187,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev) qdev->monitors_config = NULL; qdev->ram_header->monitors_config = 0; - qxl_bo_kunmap_locked(qdev->monitors_config_bo); - ret = qxl_bo_unpin(qdev->monitors_config_bo); + ret = qxl_bo_kunmap(qdev->monitors_config_bo); if (ret) return ret; -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer OpenPGP_signature Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vdpa/mlx5: Extract correct pointer from driver data
On Tue, Feb 16, 2021 at 02:45:40PM +0200, Eli Cohen wrote: > On Tue, Feb 16, 2021 at 09:37:34AM +0200, Leon Romanovsky wrote: > > On Tue, Feb 16, 2021 at 08:42:26AM +0200, Eli Cohen wrote: > > > On Tue, Feb 16, 2021 at 08:35:51AM +0200, Leon Romanovsky wrote: > > > > On Tue, Feb 16, 2021 at 07:50:22AM +0200, Eli Cohen wrote: > > > > > struct mlx5_vdpa_net pointer was stored in drvdata. Extract it as well > > > > > in mlx5v_remove(). > > > > > > > > > > Fixes: 74c9729dd892 ("vdpa/mlx5: Connect mlx5_vdpa to auxiliary bus") > > > > > Signed-off-by: Eli Cohen > > > > > --- > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > index 6b0a42183622..4103d3b64a2a 100644 > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > @@ -2036,9 +2036,9 @@ static int mlx5v_probe(struct auxiliary_device > > > > > *adev, > > > > > > > > > > static void mlx5v_remove(struct auxiliary_device *adev) > > > > > { > > > > > - struct mlx5_vdpa_dev *mvdev = dev_get_drvdata(&adev->dev); > > > > > + struct mlx5_vdpa_net *ndev = dev_get_drvdata(&adev->dev); > > > > > > > > > > - vdpa_unregister_device(&mvdev->vdev); > > > > > + vdpa_unregister_device(&ndev->mvdev.vdev); > > > > > } > > > > > > > > IMHO, The more correct solution is to fix dev_set_drvdata() call, > > > > because we are regustering/unregistering/allocating "struct > > > > mlx5_vdpa_dev". > > > > > > > > > > We're allocating "struct mlx5_vdpa_net". "struct mlx5_vdpa_dev" is just > > > a member field of "struct mlx5_vdpa_net". > > > > I referred to these lines in the mlx5v_probe(): > > 1986 err = mlx5_vdpa_alloc_resources(&ndev->mvdev); > > 1987 if (err) > > 1988 goto err_mtu; > > 1989 > > 1990 err = alloc_resources(ndev); > > 1991 if (err) > > 1992 goto err_res; > > 1993 > > 1994 err = vdpa_register_device(&mvdev->vdev); > > > > So mlx5v_remove() is better to be symmetrical. > > > > It's "struct mlx5_vdpa_net" that is being allocated here so it makes > sense to set this pointer as the the driver data. Anyway, it doesn't important. Thanks, for the original patch. Reviewed-by: Leon Romanovsky ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 06/10] drm/qxl: add qxl_bo_kmap/qxl_bo_kunmap
Hi Am 16.02.21 um 12:37 schrieb Gerd Hoffmann: Add kmap/kunmap variants which reserve (and pin) the bo. They can be used in case the caller doesn't hold a reservation for the bo. Again, these functions should rather be called vmap/vunamp. Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_object.h | 2 ++ drivers/gpu/drm/qxl/qxl_object.c | 36 2 files changed, 38 insertions(+) diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h index 5bd33650183f..360972ae4869 100644 --- a/drivers/gpu/drm/qxl/qxl_object.h +++ b/drivers/gpu/drm/qxl/qxl_object.h @@ -64,7 +64,9 @@ extern int qxl_bo_create(struct qxl_device *qdev, u32 priority, struct qxl_surface *surf, struct qxl_bo **bo_ptr); +extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map); extern int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map); +extern int qxl_bo_kunmap(struct qxl_bo *bo); extern void qxl_bo_kunmap_locked(struct qxl_bo *bo); void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset); void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *map); diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c index b56d4dfc28cb..22748b9566af 100644 --- a/drivers/gpu/drm/qxl/qxl_object.c +++ b/drivers/gpu/drm/qxl/qxl_object.c @@ -29,6 +29,9 @@ #include "qxl_drv.h" #include "qxl_object.h" +static int __qxl_bo_pin(struct qxl_bo *bo); +static void __qxl_bo_unpin(struct qxl_bo *bo); + static void qxl_ttm_bo_destroy(struct ttm_buffer_object *tbo) { struct qxl_bo *bo; @@ -179,6 +182,25 @@ int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map) return 0; } +int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map) +{ + int r; + + r = qxl_bo_reserve(bo); + if (r) + return r; + + r = __qxl_bo_pin(bo); + if (r) { + qxl_bo_unreserve(bo); + return r; + } + + r = qxl_bo_kmap_locked(bo, map); + qxl_bo_unreserve(bo); + return r; +} + void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset) { @@ -223,6 +245,20 @@ void qxl_bo_kunmap_locked(struct qxl_bo *bo) ttm_bo_vunmap(&bo->tbo, &bo->map); } +int qxl_bo_kunmap(struct qxl_bo *bo) +{ + int r; + + r = qxl_bo_reserve(bo); + if (r) + return r; + + qxl_bo_kunmap_locked(bo); + __qxl_bo_unpin(bo); + qxl_bo_unreserve(bo); + return 0; +} + void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *pmap) { -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer OpenPGP_signature Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 07/10] drm/qxl: fix prime kmap
Hi Am 16.02.21 um 12:37 schrieb Gerd Hoffmann: Use the correct kmap variant. We don't have a reservation here, so we can't use the _locked version. I'd merge this change into patch 6. So the new functions come with a caller. Best regards Thomas Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_prime.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c index dc295b2e2b52..4aa949799446 100644 --- a/drivers/gpu/drm/qxl/qxl_prime.c +++ b/drivers/gpu/drm/qxl/qxl_prime.c @@ -59,7 +59,7 @@ int qxl_gem_prime_vmap(struct drm_gem_object *obj, struct dma_buf_map *map) struct qxl_bo *bo = gem_to_qxl_bo(obj); int ret; - ret = qxl_bo_kmap_locked(bo, map); + ret = qxl_bo_kmap(bo, map); if (ret < 0) return ret; @@ -71,7 +71,7 @@ void qxl_gem_prime_vunmap(struct drm_gem_object *obj, { struct qxl_bo *bo = gem_to_qxl_bo(obj); - qxl_bo_kunmap_locked(bo); + qxl_bo_kunmap(bo); } int qxl_gem_prime_mmap(struct drm_gem_object *obj, -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer OpenPGP_signature Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 05/10] drm/qxl: rename qxl_bo_kmap -> qxl_bo_kmap_locked
Hi Am 16.02.21 um 12:37 schrieb Gerd Hoffmann: Make clear that these functions should be called with reserved bo's only. No functional change. Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_object.h | 4 ++-- drivers/gpu/drm/qxl/qxl_display.c | 14 +++--- drivers/gpu/drm/qxl/qxl_draw.c| 8 drivers/gpu/drm/qxl/qxl_image.c | 2 +- drivers/gpu/drm/qxl/qxl_object.c | 8 drivers/gpu/drm/qxl/qxl_prime.c | 4 ++-- 6 files changed, 20 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h index dc1659e717f1..5bd33650183f 100644 --- a/drivers/gpu/drm/qxl/qxl_object.h +++ b/drivers/gpu/drm/qxl/qxl_object.h @@ -64,8 +64,8 @@ extern int qxl_bo_create(struct qxl_device *qdev, u32 priority, struct qxl_surface *surf, struct qxl_bo **bo_ptr); -extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map); -extern void qxl_bo_kunmap(struct qxl_bo *bo); +extern int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map); +extern void qxl_bo_kunmap_locked(struct qxl_bo *bo); Nowadaways kmap/kunmap is a misnomer. Since you're renaming; maybe rename them to vmap/vunmap. Best regards Thomas void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset); void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *map); extern struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo); diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index a1b5cc5918bc..8672385a1caf 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -600,7 +600,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, user_bo = gem_to_qxl_bo(obj); /* pinning is done in the prepare/cleanup framevbuffer */ - ret = qxl_bo_kmap(user_bo, &user_map); + ret = qxl_bo_kmap_locked(user_bo, &user_map); if (ret) goto out_free_release; user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction properly */ @@ -619,7 +619,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, if (ret) goto out_unpin; - ret = qxl_bo_kmap(cursor_bo, &cursor_map); + ret = qxl_bo_kmap_locked(cursor_bo, &cursor_map); if (ret) goto out_backoff; if (cursor_map.is_iomem) /* TODO: Use mapping abstraction properly */ @@ -638,8 +638,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, cursor->chunk.prev_chunk = 0; cursor->chunk.data_size = size; memcpy(cursor->chunk.data, user_ptr, size); - qxl_bo_kunmap(cursor_bo); - qxl_bo_kunmap(user_bo); + qxl_bo_kunmap_locked(cursor_bo); + qxl_bo_kunmap_locked(user_bo); cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release); cmd->u.set.visible = 1; @@ -681,7 +681,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, out_free_bo: qxl_bo_unref(&cursor_bo); out_kunmap: - qxl_bo_kunmap(user_bo); + qxl_bo_kunmap_locked(user_bo); out_free_release: qxl_release_free(qdev, release); return; @@ -1163,7 +1163,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev) if (ret) return ret; - qxl_bo_kmap(qdev->monitors_config_bo, &map); + qxl_bo_kmap_locked(qdev->monitors_config_bo, &map); qdev->monitors_config = qdev->monitors_config_bo->kptr; qdev->ram_header->monitors_config = @@ -1189,7 +1189,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev) qdev->monitors_config = NULL; qdev->ram_header->monitors_config = 0; - qxl_bo_kunmap(qdev->monitors_config_bo); + qxl_bo_kunmap_locked(qdev->monitors_config_bo); ret = qxl_bo_unpin(qdev->monitors_config_bo); if (ret) return ret; diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c index 7b7acb910780..294542d49015 100644 --- a/drivers/gpu/drm/qxl/qxl_draw.c +++ b/drivers/gpu/drm/qxl/qxl_draw.c @@ -48,7 +48,7 @@ static struct qxl_rect *drawable_set_clipping(struct qxl_device *qdev, struct qxl_clip_rects *dev_clips; int ret; - ret = qxl_bo_kmap(clips_bo, &map); + ret = qxl_bo_kmap_locked(clips_bo, &map); if (ret) return NULL; dev_clips = map.vaddr; /* TODO: Use mapping abstraction properly */ @@ -202,7 +202,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev, if (ret) goto out_release_backoff; - ret = qxl_bo_kmap(bo, &surface_map); + ret = qxl_bo_kmap_locked(bo, &surface_map); if (ret) goto out_release_backoff;
Re: [PATCH 01/10] drm/qxl: properly handle device init failures
Am 16.02.21 um 12:37 schrieb Gerd Hoffmann: Specifically do not try release resources which where not allocated in the first place. I still think this should eventually be resolved by using managed code. But for now Acked-by: Thomas Zimmermann Cc: Tong Zhang Tested-by: Tong Zhang Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_display.c | 3 +++ drivers/gpu/drm/qxl/qxl_kms.c | 4 2 files changed, 7 insertions(+) diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index c326412136c5..ec50d2cfd4e1 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -1183,6 +1183,9 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev) { int ret; + if (!qdev->monitors_config_bo) + return 0; + qdev->monitors_config = NULL; qdev->ram_header->monitors_config = 0; diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c index 66d74aaaee06..4dc5ad13f12c 100644 --- a/drivers/gpu/drm/qxl/qxl_kms.c +++ b/drivers/gpu/drm/qxl/qxl_kms.c @@ -288,6 +288,10 @@ void qxl_device_fini(struct qxl_device *qdev) { int cur_idx; + /* check if qxl_device_init() was successful (gc_work is initialized last) */ + if (!qdev->gc_work.func) + return; + for (cur_idx = 0; cur_idx < 3; cur_idx++) { if (!qdev->current_release_bo[cur_idx]) continue; -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer OpenPGP_signature Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5 4/8] x86/mm/tlb: Flush remote and local TLBs concurrently
On Tue, Feb 09, 2021 at 02:16:49PM -0800, Nadav Amit wrote: > @@ -816,8 +821,8 @@ STATIC_NOPV void native_flush_tlb_others(const struct > cpumask *cpumask, >* doing a speculative memory access. >*/ > if (info->freed_tables) { > - smp_call_function_many(cpumask, flush_tlb_func, > -(void *)info, 1); > + on_each_cpu_cond_mask(NULL, flush_tlb_func, (void *)info, true, > + cpumask); > } else { > /* >* Although we could have used on_each_cpu_cond_mask(), > @@ -844,14 +849,15 @@ STATIC_NOPV void native_flush_tlb_others(const struct > cpumask *cpumask, > if (tlb_is_not_lazy(cpu)) > __cpumask_set_cpu(cpu, cond_cpumask); > } > - smp_call_function_many(cond_cpumask, flush_tlb_func, (void > *)info, 1); > + on_each_cpu_cond_mask(NULL, flush_tlb_func, (void *)info, true, > + cpumask); > } > } Surely on_each_cpu_mask() is more appropriate? There the compiler can do the NULL propagation because it's on the same TU. --- a/arch/x86/mm/tlb.c +++ b/arch/x86/mm/tlb.c @@ -821,8 +821,7 @@ STATIC_NOPV void native_flush_tlb_multi( * doing a speculative memory access. */ if (info->freed_tables) { - on_each_cpu_cond_mask(NULL, flush_tlb_func, (void *)info, true, - cpumask); + on_each_cpu_mask(cpumask, flush_tlb_func, (void *)info, true); } else { /* * Although we could have used on_each_cpu_cond_mask(), @@ -849,8 +848,7 @@ STATIC_NOPV void native_flush_tlb_multi( if (tlb_is_not_lazy(cpu)) __cpumask_set_cpu(cpu, cond_cpumask); } - on_each_cpu_cond_mask(NULL, flush_tlb_func, (void *)info, true, - cpumask); + on_each_cpu_mask(cpumask, flush_tlb_func, (void *)info, true); } } ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
We don't have to map in atomic_update callback then, making locking a bit less complicated. Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_display.c | 14 +- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 7500560db8e4..39b8c5116d34 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -584,7 +584,6 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, struct drm_gem_object *obj; struct qxl_bo *cursor_bo = NULL, *user_bo = NULL, *old_cursor_bo = NULL; int ret; - struct dma_buf_map user_map; struct dma_buf_map cursor_map; void *user_ptr; int size = 64*64*4; @@ -599,11 +598,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, obj = fb->obj[0]; user_bo = gem_to_qxl_bo(obj); - /* pinning is done in the prepare/cleanup framevbuffer */ - ret = qxl_bo_kmap_locked(user_bo, &user_map); - if (ret) - goto out_free_release; - user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction properly */ + /* mapping is done in the prepare/cleanup framevbuffer */ + user_ptr = user_bo->map.vaddr; /* TODO: Use mapping abstraction properly */ ret = qxl_alloc_bo_reserved(qdev, release, sizeof(struct qxl_cursor) + size, @@ -639,7 +635,6 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, cursor->chunk.data_size = size; memcpy(cursor->chunk.data, user_ptr, size); qxl_bo_kunmap_locked(cursor_bo); - qxl_bo_kunmap_locked(user_bo); cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release); cmd->u.set.visible = 1; @@ -778,6 +773,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane, struct drm_gem_object *obj; struct qxl_bo *user_bo; struct qxl_surface surf; + struct dma_buf_map unused; if (!new_state->fb) return 0; @@ -815,7 +811,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane, } } - return qxl_bo_pin(user_bo); + return qxl_bo_kmap(user_bo, &unused); } static void qxl_plane_cleanup_fb(struct drm_plane *plane, @@ -834,7 +830,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane, obj = old_state->fb->obj[0]; user_bo = gem_to_qxl_bo(obj); - qxl_bo_unpin(user_bo); + qxl_bo_kunmap(user_bo); if (old_state->fb != plane->state->fb && user_bo->shadow) { qxl_bo_unpin(user_bo->shadow); -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 03/10] drm/qxl: use ttm bo priorities
Allow to set priorities for buffer objects. Use priority 1 for surface and cursor command releases. Use priority 0 for drawing command releases. That way the short-living drawing commands are first in line when it comes to eviction, making it *much* less likely that ttm_bo_mem_force_space() picks something which can't be evicted and throws an error after waiting a while without success. Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_object.h | 1 + drivers/gpu/drm/qxl/qxl_cmd.c | 2 +- drivers/gpu/drm/qxl/qxl_display.c | 4 ++-- drivers/gpu/drm/qxl/qxl_gem.c | 2 +- drivers/gpu/drm/qxl/qxl_object.c | 5 +++-- drivers/gpu/drm/qxl/qxl_release.c | 19 +-- 6 files changed, 21 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h index e60a8f88e226..dc1659e717f1 100644 --- a/drivers/gpu/drm/qxl/qxl_object.h +++ b/drivers/gpu/drm/qxl/qxl_object.h @@ -61,6 +61,7 @@ static inline u64 qxl_bo_mmap_offset(struct qxl_bo *bo) extern int qxl_bo_create(struct qxl_device *qdev, unsigned long size, bool kernel, bool pinned, u32 domain, +u32 priority, struct qxl_surface *surf, struct qxl_bo **bo_ptr); extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map); diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c index 7e22a81bfb36..7b00c955cd82 100644 --- a/drivers/gpu/drm/qxl/qxl_cmd.c +++ b/drivers/gpu/drm/qxl/qxl_cmd.c @@ -269,7 +269,7 @@ int qxl_alloc_bo_reserved(struct qxl_device *qdev, int ret; ret = qxl_bo_create(qdev, size, false /* not kernel - device */, - false, QXL_GEM_DOMAIN_VRAM, NULL, &bo); + false, QXL_GEM_DOMAIN_VRAM, 0, NULL, &bo); if (ret) { DRM_ERROR("failed to allocate VRAM BO\n"); return ret; diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index ec50d2cfd4e1..a1b5cc5918bc 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -799,8 +799,8 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane, qdev->dumb_shadow_bo = NULL; } qxl_bo_create(qdev, surf.height * surf.stride, - true, true, QXL_GEM_DOMAIN_SURFACE, &surf, - &qdev->dumb_shadow_bo); + true, true, QXL_GEM_DOMAIN_SURFACE, 0, + &surf, &qdev->dumb_shadow_bo); } if (user_bo->shadow != qdev->dumb_shadow_bo) { if (user_bo->shadow) { diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c index 48e096285b4c..a08da0bd9098 100644 --- a/drivers/gpu/drm/qxl/qxl_gem.c +++ b/drivers/gpu/drm/qxl/qxl_gem.c @@ -55,7 +55,7 @@ int qxl_gem_object_create(struct qxl_device *qdev, int size, /* At least align on page size */ if (alignment < PAGE_SIZE) alignment = PAGE_SIZE; - r = qxl_bo_create(qdev, size, kernel, false, initial_domain, surf, &qbo); + r = qxl_bo_create(qdev, size, kernel, false, initial_domain, 0, surf, &qbo); if (r) { if (r != -ERESTARTSYS) DRM_ERROR( diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c index 705b51535492..7eada4ad217b 100644 --- a/drivers/gpu/drm/qxl/qxl_object.c +++ b/drivers/gpu/drm/qxl/qxl_object.c @@ -103,8 +103,8 @@ static const struct drm_gem_object_funcs qxl_object_funcs = { .print_info = drm_gem_ttm_print_info, }; -int qxl_bo_create(struct qxl_device *qdev, - unsigned long size, bool kernel, bool pinned, u32 domain, +int qxl_bo_create(struct qxl_device *qdev, unsigned long size, + bool kernel, bool pinned, u32 domain, u32 priority, struct qxl_surface *surf, struct qxl_bo **bo_ptr) { @@ -137,6 +137,7 @@ int qxl_bo_create(struct qxl_device *qdev, qxl_ttm_placement_from_domain(bo, domain); + bo->tbo.priority = priority; r = ttm_bo_init_reserved(&qdev->mman.bdev, &bo->tbo, size, type, &bo->placement, 0, &ctx, NULL, NULL, &qxl_ttm_bo_destroy); diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index 63818b56218c..a831184e014a 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -161,11 +161,12 @@ qxl_release_free(struct qxl_device *qdev, } static int qxl_release_bo_alloc(struct qxl_device *qdev, - struct qxl_bo **bo) + struct qxl_bo **bo, + u32 prio
[PATCH 06/10] drm/qxl: add qxl_bo_kmap/qxl_bo_kunmap
Add kmap/kunmap variants which reserve (and pin) the bo. They can be used in case the caller doesn't hold a reservation for the bo. Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_object.h | 2 ++ drivers/gpu/drm/qxl/qxl_object.c | 36 2 files changed, 38 insertions(+) diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h index 5bd33650183f..360972ae4869 100644 --- a/drivers/gpu/drm/qxl/qxl_object.h +++ b/drivers/gpu/drm/qxl/qxl_object.h @@ -64,7 +64,9 @@ extern int qxl_bo_create(struct qxl_device *qdev, u32 priority, struct qxl_surface *surf, struct qxl_bo **bo_ptr); +extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map); extern int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map); +extern int qxl_bo_kunmap(struct qxl_bo *bo); extern void qxl_bo_kunmap_locked(struct qxl_bo *bo); void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset); void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *map); diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c index b56d4dfc28cb..22748b9566af 100644 --- a/drivers/gpu/drm/qxl/qxl_object.c +++ b/drivers/gpu/drm/qxl/qxl_object.c @@ -29,6 +29,9 @@ #include "qxl_drv.h" #include "qxl_object.h" +static int __qxl_bo_pin(struct qxl_bo *bo); +static void __qxl_bo_unpin(struct qxl_bo *bo); + static void qxl_ttm_bo_destroy(struct ttm_buffer_object *tbo) { struct qxl_bo *bo; @@ -179,6 +182,25 @@ int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map) return 0; } +int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map) +{ + int r; + + r = qxl_bo_reserve(bo); + if (r) + return r; + + r = __qxl_bo_pin(bo); + if (r) { + qxl_bo_unreserve(bo); + return r; + } + + r = qxl_bo_kmap_locked(bo, map); + qxl_bo_unreserve(bo); + return r; +} + void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset) { @@ -223,6 +245,20 @@ void qxl_bo_kunmap_locked(struct qxl_bo *bo) ttm_bo_vunmap(&bo->tbo, &bo->map); } +int qxl_bo_kunmap(struct qxl_bo *bo) +{ + int r; + + r = qxl_bo_reserve(bo); + if (r) + return r; + + qxl_bo_kunmap_locked(bo); + __qxl_bo_unpin(bo); + qxl_bo_unreserve(bo); + return 0; +} + void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *pmap) { -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 08/10] drm/qxl: fix monitors object kmap
Use the correct kmap variant. We don't hold a reservation here, so we can't use the _locked variant. We can drop the pin because qxl_bo_kmap will do that for us. Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_display.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 8672385a1caf..7500560db8e4 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -1159,12 +1159,10 @@ int qxl_create_monitors_object(struct qxl_device *qdev) } qdev->monitors_config_bo = gem_to_qxl_bo(gobj); - ret = qxl_bo_pin(qdev->monitors_config_bo); + ret = qxl_bo_kmap(qdev->monitors_config_bo, &map); if (ret) return ret; - qxl_bo_kmap_locked(qdev->monitors_config_bo, &map); - qdev->monitors_config = qdev->monitors_config_bo->kptr; qdev->ram_header->monitors_config = qxl_bo_physical_address(qdev, qdev->monitors_config_bo, 0); @@ -1189,8 +1187,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev) qdev->monitors_config = NULL; qdev->ram_header->monitors_config = 0; - qxl_bo_kunmap_locked(qdev->monitors_config_bo); - ret = qxl_bo_unpin(qdev->monitors_config_bo); + ret = qxl_bo_kunmap(qdev->monitors_config_bo); if (ret) return ret; -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 05/10] drm/qxl: rename qxl_bo_kmap -> qxl_bo_kmap_locked
Make clear that these functions should be called with reserved bo's only. No functional change. Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_object.h | 4 ++-- drivers/gpu/drm/qxl/qxl_display.c | 14 +++--- drivers/gpu/drm/qxl/qxl_draw.c| 8 drivers/gpu/drm/qxl/qxl_image.c | 2 +- drivers/gpu/drm/qxl/qxl_object.c | 8 drivers/gpu/drm/qxl/qxl_prime.c | 4 ++-- 6 files changed, 20 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h index dc1659e717f1..5bd33650183f 100644 --- a/drivers/gpu/drm/qxl/qxl_object.h +++ b/drivers/gpu/drm/qxl/qxl_object.h @@ -64,8 +64,8 @@ extern int qxl_bo_create(struct qxl_device *qdev, u32 priority, struct qxl_surface *surf, struct qxl_bo **bo_ptr); -extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map); -extern void qxl_bo_kunmap(struct qxl_bo *bo); +extern int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map); +extern void qxl_bo_kunmap_locked(struct qxl_bo *bo); void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset); void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *map); extern struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo); diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index a1b5cc5918bc..8672385a1caf 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -600,7 +600,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, user_bo = gem_to_qxl_bo(obj); /* pinning is done in the prepare/cleanup framevbuffer */ - ret = qxl_bo_kmap(user_bo, &user_map); + ret = qxl_bo_kmap_locked(user_bo, &user_map); if (ret) goto out_free_release; user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction properly */ @@ -619,7 +619,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, if (ret) goto out_unpin; - ret = qxl_bo_kmap(cursor_bo, &cursor_map); + ret = qxl_bo_kmap_locked(cursor_bo, &cursor_map); if (ret) goto out_backoff; if (cursor_map.is_iomem) /* TODO: Use mapping abstraction properly */ @@ -638,8 +638,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, cursor->chunk.prev_chunk = 0; cursor->chunk.data_size = size; memcpy(cursor->chunk.data, user_ptr, size); - qxl_bo_kunmap(cursor_bo); - qxl_bo_kunmap(user_bo); + qxl_bo_kunmap_locked(cursor_bo); + qxl_bo_kunmap_locked(user_bo); cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release); cmd->u.set.visible = 1; @@ -681,7 +681,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, out_free_bo: qxl_bo_unref(&cursor_bo); out_kunmap: - qxl_bo_kunmap(user_bo); + qxl_bo_kunmap_locked(user_bo); out_free_release: qxl_release_free(qdev, release); return; @@ -1163,7 +1163,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev) if (ret) return ret; - qxl_bo_kmap(qdev->monitors_config_bo, &map); + qxl_bo_kmap_locked(qdev->monitors_config_bo, &map); qdev->monitors_config = qdev->monitors_config_bo->kptr; qdev->ram_header->monitors_config = @@ -1189,7 +1189,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev) qdev->monitors_config = NULL; qdev->ram_header->monitors_config = 0; - qxl_bo_kunmap(qdev->monitors_config_bo); + qxl_bo_kunmap_locked(qdev->monitors_config_bo); ret = qxl_bo_unpin(qdev->monitors_config_bo); if (ret) return ret; diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c index 7b7acb910780..294542d49015 100644 --- a/drivers/gpu/drm/qxl/qxl_draw.c +++ b/drivers/gpu/drm/qxl/qxl_draw.c @@ -48,7 +48,7 @@ static struct qxl_rect *drawable_set_clipping(struct qxl_device *qdev, struct qxl_clip_rects *dev_clips; int ret; - ret = qxl_bo_kmap(clips_bo, &map); + ret = qxl_bo_kmap_locked(clips_bo, &map); if (ret) return NULL; dev_clips = map.vaddr; /* TODO: Use mapping abstraction properly */ @@ -202,7 +202,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev, if (ret) goto out_release_backoff; - ret = qxl_bo_kmap(bo, &surface_map); + ret = qxl_bo_kmap_locked(bo, &surface_map); if (ret) goto out_release_backoff; surface_base = surface_map.vaddr; /* TODO: Use mapping abstraction properly */ @@ -210,7 +210,7 @@ void qxl_draw_dirty_fb(str
[PATCH 10/10] drm/qxl: add lock asserts to qxl_bo_kmap_locked + qxl_bo_kunmap_locked
Try avoid re-introducing locking bugs. Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_object.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c index 22748b9566af..90d5e5b7f927 100644 --- a/drivers/gpu/drm/qxl/qxl_object.c +++ b/drivers/gpu/drm/qxl/qxl_object.c @@ -162,6 +162,8 @@ int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map) { int r; + dma_resv_assert_held(bo->tbo.base.resv); + if (bo->kptr) { bo->map_count++; goto out; @@ -236,6 +238,8 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, void qxl_bo_kunmap_locked(struct qxl_bo *bo) { + dma_resv_assert_held(bo->tbo.base.resv); + if (bo->kptr == NULL) return; bo->map_count--; -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 07/10] drm/qxl: fix prime kmap
Use the correct kmap variant. We don't have a reservation here, so we can't use the _locked version. Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_prime.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c index dc295b2e2b52..4aa949799446 100644 --- a/drivers/gpu/drm/qxl/qxl_prime.c +++ b/drivers/gpu/drm/qxl/qxl_prime.c @@ -59,7 +59,7 @@ int qxl_gem_prime_vmap(struct drm_gem_object *obj, struct dma_buf_map *map) struct qxl_bo *bo = gem_to_qxl_bo(obj); int ret; - ret = qxl_bo_kmap_locked(bo, map); + ret = qxl_bo_kmap(bo, map); if (ret < 0) return ret; @@ -71,7 +71,7 @@ void qxl_gem_prime_vunmap(struct drm_gem_object *obj, { struct qxl_bo *bo = gem_to_qxl_bo(obj); - qxl_bo_kunmap_locked(bo); + qxl_bo_kunmap(bo); } int qxl_gem_prime_mmap(struct drm_gem_object *obj, -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 01/10] drm/qxl: properly handle device init failures
Specifically do not try release resources which where not allocated in the first place. Cc: Tong Zhang Tested-by: Tong Zhang Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_display.c | 3 +++ drivers/gpu/drm/qxl/qxl_kms.c | 4 2 files changed, 7 insertions(+) diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index c326412136c5..ec50d2cfd4e1 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -1183,6 +1183,9 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev) { int ret; + if (!qdev->monitors_config_bo) + return 0; + qdev->monitors_config = NULL; qdev->ram_header->monitors_config = 0; diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c index 66d74aaaee06..4dc5ad13f12c 100644 --- a/drivers/gpu/drm/qxl/qxl_kms.c +++ b/drivers/gpu/drm/qxl/qxl_kms.c @@ -288,6 +288,10 @@ void qxl_device_fini(struct qxl_device *qdev) { int cur_idx; + /* check if qxl_device_init() was successful (gc_work is initialized last) */ + if (!qdev->gc_work.func) + return; + for (cur_idx = 0; cur_idx < 3; cur_idx++) { if (!qdev->current_release_bo[cur_idx]) continue; -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 04/10] drm/qxl: fix lockdep issue in qxl_alloc_release_reserved
Call qxl_bo_unpin (which does a reservation) without holding the release_mutex lock. Fixes lockdep (correctly) warning on a possible deadlock. Fixes: 65ffea3c6e73 ("drm/qxl: unpin release objects") Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_release.c | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index a831184e014a..352a11a8485b 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -284,7 +284,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size, int type, struct qxl_release **release, struct qxl_bo **rbo) { - struct qxl_bo *bo; + struct qxl_bo *bo, *free_bo = NULL; int idr_ret; int ret = 0; union qxl_release_info *info; @@ -317,8 +317,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size, mutex_lock(&qdev->release_mutex); if (qdev->current_release_bo_offset[cur_idx] + 1 >= releases_per_bo[cur_idx]) { - qxl_bo_unpin(qdev->current_release_bo[cur_idx]); - qxl_bo_unref(&qdev->current_release_bo[cur_idx]); + free_bo = qdev->current_release_bo[cur_idx]; qdev->current_release_bo_offset[cur_idx] = 0; qdev->current_release_bo[cur_idx] = NULL; } @@ -326,6 +325,10 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size, ret = qxl_release_bo_alloc(qdev, &qdev->current_release_bo[cur_idx], priority); if (ret) { mutex_unlock(&qdev->release_mutex); + if (free_bo) { + qxl_bo_unpin(free_bo); + qxl_bo_unref(&free_bo); + } qxl_release_free(qdev, *release); return ret; } @@ -341,6 +344,10 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size, *rbo = bo; mutex_unlock(&qdev->release_mutex); + if (free_bo) { + qxl_bo_unpin(free_bo); + qxl_bo_unref(&free_bo); + } ret = qxl_release_list_add(*release, bo); qxl_bo_unref(&bo); -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 02/10] drm/qxl: more fence wait rework
Move qxl_io_notify_oom() call into wait condition. That way the driver will call it again if one call wasn't enough. Also allows to remove the extra dma_fence_is_signaled() check and the goto. Fixes: 5a838e5d5825 ("drm/qxl: simplify qxl_fence_wait") Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_release.c | 9 +++-- 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index 6ed673d75f9f..63818b56218c 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -62,16 +62,13 @@ static long qxl_fence_wait(struct dma_fence *fence, bool intr, qdev = container_of(fence->lock, struct qxl_device, release_lock); - if (dma_fence_is_signaled(fence)) - goto signaled; - - qxl_io_notify_oom(qdev); if (!wait_event_timeout(qdev->release_event, - dma_fence_is_signaled(fence), + (dma_fence_is_signaled(fence) || ( + qxl_io_notify_oom(qdev), + 0)), timeout)) return 0; -signaled: cur = jiffies; if (time_after(cur, end)) return 0; -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2] virtio/s390: implement virtio-ccw revision 2 correctly
CCW_CMD_READ_STATUS was introduced with revision 2 of virtio-ccw, and drivers should only rely on it being implemented when they negotiated at least that revision with the device. However, virtio_ccw_get_status() issued READ_STATUS for any device operating at least at revision 1. If the device accepts READ_STATUS regardless of the negotiated revision (which some implementations like QEMU do, even though the spec currently does not allow it), everything works as intended. While a device rejecting the command should also be handled gracefully, we will not be able to see any changes the device makes to the status, such as setting NEEDS_RESET or setting the status to zero after a completed reset. We negotiated the revision to at most 1, as we never bumped the maximum revision; let's do that now and properly send READ_STATUS only if we are operating at least at revision 2. Cc: sta...@vger.kernel.org Fixes: 7d3ce5ab9430 ("virtio/s390: support READ_STATUS command for virtio-ccw") Reviewed-by: Halil Pasic Signed-off-by: Cornelia Huck --- v1->v2: tweak patch description and cc:stable --- drivers/s390/virtio/virtio_ccw.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c index 5730572b52cd..54e686dca6de 100644 --- a/drivers/s390/virtio/virtio_ccw.c +++ b/drivers/s390/virtio/virtio_ccw.c @@ -117,7 +117,7 @@ struct virtio_rev_info { }; /* the highest virtio-ccw revision we support */ -#define VIRTIO_CCW_REV_MAX 1 +#define VIRTIO_CCW_REV_MAX 2 struct virtio_ccw_vq_info { struct virtqueue *vq; @@ -952,7 +952,7 @@ static u8 virtio_ccw_get_status(struct virtio_device *vdev) u8 old_status = vcdev->dma_area->status; struct ccw1 *ccw; - if (vcdev->revision < 1) + if (vcdev->revision < 2) return vcdev->dma_area->status; ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw)); -- 2.26.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio/s390: implement virtio-ccw revision 2 correctly
On Tue, 16 Feb 2021 11:39:07 +0100 Cornelia Huck wrote: > > > > Reviewed-by: Halil Pasic > > Thanks! > > I'll do a v2 with a tweaked commit message and cc:stable. Sounds good! ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio/s390: implement virtio-ccw revision 2 correctly
On Mon, 15 Feb 2021 19:51:44 +0100 Halil Pasic wrote: > On Mon, 15 Feb 2021 12:47:02 +0100 > Cornelia Huck wrote: > > > On Fri, 12 Feb 2021 18:04:11 +0100 > > Cornelia Huck wrote: > > > > > CCW_CMD_READ_STATUS was introduced with revision 2 of virtio-ccw, > > > and drivers should only rely on it being implemented when they > > > negotiated at least that revision with the device. > > > > > > However, virtio_ccw_get_status() issued READ_STATUS for any > > > device operating at least at revision 1. If the device accepts > > > READ_STATUS regardless of the negotiated revision (which it is > > > free to do), > > > > So, looking at the standard again, the device is actually required to > > reject the READ_STATUS if only rev 1 had been negotiated... regardless > > of that, I don't think we should change QEMU's behaviour, as it would > > affect existing guests (they would lose access to the status bits as > > observed by the device, including DEVICE_NEEDS_RESET.) > > Not only that, without READ_STATUS, we can't do device reset which > is a prerequisite for a proper cleanup, as required by the spec. > > You certainly remember, the driver has may not assume the reset > was performed (and thus virtqueues are not live) until it reads > back status 0. But without READ_STATUS virtio_ccw_get_status() will > keep returning the status the driver last set via > virtio_ccw_set_status(). And CCW_CMD_VDEV_RESET is of course > revision 1 material. This looks ugly! Yes, that problem kind of cascades down. > > > > > > everything works as intended; a device rejecting the > > > command should also be handled gracefully. For correctness, we > > > should really limit the command to revision 2 or higher, though. > > > > > > We also negotiated the revision to at most 1, as we never bumped > > > the maximum revision; let's do that now. > > > > > > Fixes: 7d3ce5ab9430 ("virtio/s390: support READ_STATUS command for > > > virtio-ccw") > > > Signed-off-by: Cornelia Huck > > > --- > > > > > > QEMU does not fence off READ_STATUS for revisions < 2, which is probably > > > why we never noticed this. I'm not aware of other hypervisors that do > > > fence it off, nor any that cannot deal properly with an unknown command. > > > > > > Not sure whether this is stable worthy? > > > > Maybe it is, given the MUST reject clause in the standard? > > > > Yes, IMHO this must be backported. A device that ain't violating the > spec would currently reject READ_STATUS. Which would break RESET_VDEV > like I described above. > > Can we change that MUST to should? There are now good reasons for not > doing like the spec says in case of READ_STATUS. Yes. I'm not so sure forcing the device to reject the command was such a good idea anyway, and relaxing the requirement keeps existing implementations in compliance. I've opened https://github.com/oasis-tcs/virtio-spec/issues/96 and will send a patch for the spec later. > > > > > > > --- > > > drivers/s390/virtio/virtio_ccw.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c > > > b/drivers/s390/virtio/virtio_ccw.c > > > index 5730572b52cd..54e686dca6de 100644 > > > --- a/drivers/s390/virtio/virtio_ccw.c > > > +++ b/drivers/s390/virtio/virtio_ccw.c > > > @@ -117,7 +117,7 @@ struct virtio_rev_info { > > > }; > > > > > > /* the highest virtio-ccw revision we support */ > > > -#define VIRTIO_CCW_REV_MAX 1 > > > +#define VIRTIO_CCW_REV_MAX 2 > > > > > > struct virtio_ccw_vq_info { > > > struct virtqueue *vq; > > > @@ -952,7 +952,7 @@ static u8 virtio_ccw_get_status(struct virtio_device > > > *vdev) > > > u8 old_status = vcdev->dma_area->status; > > > struct ccw1 *ccw; > > > > > > - if (vcdev->revision < 1) > > > + if (vcdev->revision < 2) > > > return vcdev->dma_area->status; > > I don't think our faking of the status read (i.e. returning the old one) > is contributing to spec compliance. Especially not if the inability to > READ is not transient. > > Also return old_status; would tell the story better, but on the > other hand, that would be an unrelated cosmetic change. Maybe > a separate patch? We would also need to actively check for success or failure of the channel program in that case. I'm currently looking at the virtio-ccw code anyway, so I can put that on my list as well. > > Reviewed-by: Halil Pasic Thanks! I'll do a v2 with a tweaked commit message and cc:stable. > > Regards, > Halil > > > > > > > ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw)); > > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH 10/10] vhost/vdpa: return configuration bytes read and written to user space
vdpa_get_config() and vdpa_set_config() now return the amount of bytes read and written, so let's return them to the user space. We also modify vhost_vdpa_config_validate() to return 0 (bytes read or written) instead of an error, when the buffer length is 0. Signed-off-by: Stefano Garzarella --- drivers/vhost/vdpa.c | 26 +++--- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index 21eea2be5afa..b754c53171a7 100644 --- a/drivers/vhost/vdpa.c +++ b/drivers/vhost/vdpa.c @@ -191,9 +191,6 @@ static ssize_t vhost_vdpa_config_validate(struct vhost_vdpa *v, struct vdpa_device *vdpa = v->vdpa; u32 size = vdpa->config->get_config_size(vdpa); - if (c->len == 0) - return -EINVAL; - return min(c->len, size); } @@ -204,6 +201,7 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v, struct vhost_vdpa_config config; unsigned long size = offsetof(struct vhost_vdpa_config, buf); ssize_t config_size; + long ret; u8 *buf; if (copy_from_user(&config, c, size)) @@ -217,15 +215,18 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v, if (!buf) return -ENOMEM; - vdpa_get_config(vdpa, config.off, buf, config_size); - - if (copy_to_user(c->buf, buf, config_size)) { - kvfree(buf); - return -EFAULT; + ret = vdpa_get_config(vdpa, config.off, buf, config_size); + if (ret < 0) { + ret = -EFAULT; + goto out; } + if (copy_to_user(c->buf, buf, config_size)) + ret = -EFAULT; + +out: kvfree(buf); - return 0; + return ret; } static long vhost_vdpa_set_config(struct vhost_vdpa *v, @@ -235,6 +236,7 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v, struct vhost_vdpa_config config; unsigned long size = offsetof(struct vhost_vdpa_config, buf); ssize_t config_size; + long ret; u8 *buf; if (copy_from_user(&config, c, size)) @@ -248,10 +250,12 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v, if (IS_ERR(buf)) return PTR_ERR(buf); - vdpa_set_config(vdpa, config.off, buf, config_size); + ret = vdpa_set_config(vdpa, config.off, buf, config_size); + if (ret < 0) + ret = -EFAULT; kvfree(buf); - return 0; + return ret; } static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep) -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH 09/10] vhost/vdpa: use get_config_size callback in vhost_vdpa_config_validate()
Let's use the new 'get_config_size()' callback available instead of using the 'virtio_id' to get the size of the device config space. Signed-off-by: Stefano Garzarella --- drivers/vhost/vdpa.c | 9 ++--- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index 544f8582a42b..21eea2be5afa 100644 --- a/drivers/vhost/vdpa.c +++ b/drivers/vhost/vdpa.c @@ -188,13 +188,8 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp) static ssize_t vhost_vdpa_config_validate(struct vhost_vdpa *v, struct vhost_vdpa_config *c) { - u32 size = 0; - - switch (v->virtio_id) { - case VIRTIO_ID_NET: - size = sizeof(struct virtio_net_config); - break; - } + struct vdpa_device *vdpa = v->vdpa; + u32 size = vdpa->config->get_config_size(vdpa); if (c->len == 0) return -EINVAL; -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH 08/10] vhost/vdpa: allow user space to pass buffers bigger than config space
vdpa_get_config() and vdpa_set_config() now are able to read/write only the bytes available in the device configuration space, also if the buffer provided is bigger than that. Let's use this feature to allow the user space application to pass any buffer. We limit the size of the internal bounce buffer allocated with the device config size. Signed-off-by: Stefano Garzarella --- drivers/vhost/vdpa.c | 36 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index cdd8f24168b2..544f8582a42b 100644 --- a/drivers/vhost/vdpa.c +++ b/drivers/vhost/vdpa.c @@ -185,10 +185,10 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp) return 0; } -static int vhost_vdpa_config_validate(struct vhost_vdpa *v, - struct vhost_vdpa_config *c) +static ssize_t vhost_vdpa_config_validate(struct vhost_vdpa *v, + struct vhost_vdpa_config *c) { - long size = 0; + u32 size = 0; switch (v->virtio_id) { case VIRTIO_ID_NET: @@ -199,10 +199,7 @@ static int vhost_vdpa_config_validate(struct vhost_vdpa *v, if (c->len == 0) return -EINVAL; - if (c->len > size - c->off) - return -E2BIG; - - return 0; + return min(c->len, size); } static long vhost_vdpa_get_config(struct vhost_vdpa *v, @@ -211,19 +208,23 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v, struct vdpa_device *vdpa = v->vdpa; struct vhost_vdpa_config config; unsigned long size = offsetof(struct vhost_vdpa_config, buf); + ssize_t config_size; u8 *buf; if (copy_from_user(&config, c, size)) return -EFAULT; - if (vhost_vdpa_config_validate(v, &config)) - return -EINVAL; - buf = kvzalloc(config.len, GFP_KERNEL); + + config_size = vhost_vdpa_config_validate(v, &config); + if (config_size <= 0) + return config_size; + + buf = kvzalloc(config_size, GFP_KERNEL); if (!buf) return -ENOMEM; - vdpa_get_config(vdpa, config.off, buf, config.len); + vdpa_get_config(vdpa, config.off, buf, config_size); - if (copy_to_user(c->buf, buf, config.len)) { + if (copy_to_user(c->buf, buf, config_size)) { kvfree(buf); return -EFAULT; } @@ -238,18 +239,21 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v, struct vdpa_device *vdpa = v->vdpa; struct vhost_vdpa_config config; unsigned long size = offsetof(struct vhost_vdpa_config, buf); + ssize_t config_size; u8 *buf; if (copy_from_user(&config, c, size)) return -EFAULT; - if (vhost_vdpa_config_validate(v, &config)) - return -EINVAL; - buf = vmemdup_user(c->buf, config.len); + config_size = vhost_vdpa_config_validate(v, &config); + if (config_size <= 0) + return config_size; + + buf = vmemdup_user(c->buf, config_size); if (IS_ERR(buf)) return PTR_ERR(buf); - vdpa_set_config(vdpa, config.off, buf, config.len); + vdpa_set_config(vdpa, config.off, buf, config_size); kvfree(buf); return 0; -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH 06/10] virtio_vdpa: use vdpa_set_config()
Instead of calling the 'set_config' callback directly, we call the new vdpa_set_config() helper which also checks the parameters. Signed-off-by: Stefano Garzarella --- drivers/virtio/virtio_vdpa.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c index e28acf482e0c..2f1c4a2dd241 100644 --- a/drivers/virtio/virtio_vdpa.c +++ b/drivers/virtio/virtio_vdpa.c @@ -65,9 +65,8 @@ static void virtio_vdpa_set(struct virtio_device *vdev, unsigned offset, const void *buf, unsigned len) { struct vdpa_device *vdpa = vd_get_vdpa(vdev); - const struct vdpa_config_ops *ops = vdpa->config; - ops->set_config(vdpa, offset, buf, len); + vdpa_set_config(vdpa, offset, buf, len); } static u32 virtio_vdpa_generation(struct virtio_device *vdev) -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH 07/10] vhost/vdpa: use vdpa_set_config()
Instead of calling the 'set_config' callback directly, we call the new vdpa_set_config() helper which also checks the parameters. Signed-off-by: Stefano Garzarella --- drivers/vhost/vdpa.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index ef688c8c0e0e..cdd8f24168b2 100644 --- a/drivers/vhost/vdpa.c +++ b/drivers/vhost/vdpa.c @@ -236,7 +236,6 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v, struct vhost_vdpa_config __user *c) { struct vdpa_device *vdpa = v->vdpa; - const struct vdpa_config_ops *ops = vdpa->config; struct vhost_vdpa_config config; unsigned long size = offsetof(struct vhost_vdpa_config, buf); u8 *buf; @@ -250,7 +249,7 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v, if (IS_ERR(buf)) return PTR_ERR(buf); - ops->set_config(vdpa, config.off, buf, config.len); + vdpa_set_config(vdpa, config.off, buf, config.len); kvfree(buf); return 0; -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH 04/10] vdpa: remove param checks in the get/set_config callbacks
vdpa_get_config() and vdpa_set_config() now check parameters before calling callbacks, so we can remove these redundant checks. Signed-off-by: Stefano Garzarella --- drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 +-- drivers/vdpa/vdpa_sim/vdpa_sim.c | 6 -- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c index 78043ee567b6..ab63dc9b8432 100644 --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c @@ -1825,8 +1825,7 @@ static void mlx5_vdpa_get_config(struct vdpa_device *vdev, unsigned int offset, struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev); struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev); - if (offset + len <= sizeof(struct virtio_net_config)) - memcpy(buf, (u8 *)&ndev->config + offset, len); + memcpy(buf, (u8 *)&ndev->config + offset, len); } static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, const void *buf, diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c index 779ae6c144d7..392180c6f2cf 100644 --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c @@ -451,9 +451,6 @@ static void vdpasim_get_config(struct vdpa_device *vdpa, unsigned int offset, { struct vdpasim *vdpasim = vdpa_to_sim(vdpa); - if (offset + len > vdpasim->dev_attr.config_size) - return; - if (vdpasim->dev_attr.get_config) vdpasim->dev_attr.get_config(vdpasim, vdpasim->config); @@ -465,9 +462,6 @@ static void vdpasim_set_config(struct vdpa_device *vdpa, unsigned int offset, { struct vdpasim *vdpasim = vdpa_to_sim(vdpa); - if (offset + len > vdpasim->dev_attr.config_size) - return; - memcpy(vdpasim->config + offset, buf, len); if (vdpasim->dev_attr.set_config) -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH 05/10] vdpa: remove WARN_ON() in the get/set_config callbacks
vdpa_get_config() and vdpa_set_config() now check parameters before calling callbacks, so we can remove these warnings. Signed-off-by: Stefano Garzarella --- Maybe we can skip this patch and leave the WARN_ONs in place. What do you recommend? --- drivers/vdpa/ifcvf/ifcvf_base.c | 3 +-- drivers/vdpa/ifcvf/ifcvf_main.c | 2 -- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c index f2a128e56de5..5941ecf934d0 100644 --- a/drivers/vdpa/ifcvf/ifcvf_base.c +++ b/drivers/vdpa/ifcvf/ifcvf_base.c @@ -222,7 +222,6 @@ void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset, u8 old_gen, new_gen, *p; int i; - WARN_ON(offset + length > sizeof(struct virtio_net_config)); do { old_gen = ifc_ioread8(&hw->common_cfg->config_generation); p = dst; @@ -240,7 +239,7 @@ void ifcvf_write_net_config(struct ifcvf_hw *hw, u64 offset, int i; p = src; - WARN_ON(offset + length > sizeof(struct virtio_net_config)); + for (i = 0; i < length; i++) ifc_iowrite8(*p++, hw->net_cfg + offset + i); } diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c index 2443271e17d2..e55f88c57461 100644 --- a/drivers/vdpa/ifcvf/ifcvf_main.c +++ b/drivers/vdpa/ifcvf/ifcvf_main.c @@ -343,7 +343,6 @@ static void ifcvf_vdpa_get_config(struct vdpa_device *vdpa_dev, { struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); - WARN_ON(offset + len > sizeof(struct virtio_net_config)); ifcvf_read_net_config(vf, offset, buf, len); } @@ -353,7 +352,6 @@ static void ifcvf_vdpa_set_config(struct vdpa_device *vdpa_dev, { struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); - WARN_ON(offset + len > sizeof(struct virtio_net_config)); ifcvf_write_net_config(vf, offset, buf, len); } -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH 03/10] vdpa: add vdpa_set_config() helper
Let's add a function similar to vpda_get_config() to check the 'offset' and 'len' parameters, call the set_config() device callback, and return the amount of bytes written. Signed-off-by: Stefano Garzarella --- include/linux/vdpa.h | 2 ++ drivers/vdpa/vdpa.c | 16 2 files changed, 18 insertions(+) diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h index 8a679c98f8b1..562fcd14f4b5 100644 --- a/include/linux/vdpa.h +++ b/include/linux/vdpa.h @@ -334,6 +334,8 @@ static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features) int vdpa_get_config(struct vdpa_device *vdev, unsigned int offset, void *buf, unsigned int len); +int vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, + const void *buf, unsigned int len); /** * vdpa_mgmtdev_ops - vdpa device ops diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index 9ed6c779c63c..825afc690a7e 100644 --- a/drivers/vdpa/vdpa.c +++ b/drivers/vdpa/vdpa.c @@ -86,6 +86,22 @@ int vdpa_get_config(struct vdpa_device *vdev, unsigned int offset, } EXPORT_SYMBOL_GPL(vdpa_get_config); +int vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, + const void *buf, unsigned int len) +{ + const struct vdpa_config_ops *ops = vdev->config; + int bytes_set; + + bytes_set = vdpa_config_size_wrap(vdev, offset, len); + if (bytes_set <= 0) + return bytes_set; + + ops->set_config(vdev, offset, buf, bytes_set); + + return bytes_set; +} +EXPORT_SYMBOL_GPL(vdpa_set_config); + static void vdpa_release_dev(struct device *d) { struct vdpa_device *vdev = dev_to_vdpa(d); -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH 01/10] vdpa: add get_config_size callback in vdpa_config_ops
This new callback is used to get the size of the configuration space of vDPA devices. Signed-off-by: Stefano Garzarella --- include/linux/vdpa.h | 4 drivers/vdpa/ifcvf/ifcvf_main.c | 6 ++ drivers/vdpa/mlx5/net/mlx5_vnet.c | 6 ++ drivers/vdpa/vdpa_sim/vdpa_sim.c | 9 + 4 files changed, 25 insertions(+) diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h index 4ab5494503a8..fddf42b17573 100644 --- a/include/linux/vdpa.h +++ b/include/linux/vdpa.h @@ -150,6 +150,9 @@ struct vdpa_iova_range { * @set_status:Set the device status * @vdev: vdpa device * @status: virtio device status + * @get_config_size: Get the size of the configuration space + * @vdev: vdpa device + * Returns size_t: configuration size * @get_config:Read from device specific configuration space * @vdev: vdpa device * @offset: offset from the beginning of @@ -231,6 +234,7 @@ struct vdpa_config_ops { u32 (*get_vendor_id)(struct vdpa_device *vdev); u8 (*get_status)(struct vdpa_device *vdev); void (*set_status)(struct vdpa_device *vdev, u8 status); + size_t (*get_config_size)(struct vdpa_device *vdev); void (*get_config)(struct vdpa_device *vdev, unsigned int offset, void *buf, unsigned int len); void (*set_config)(struct vdpa_device *vdev, unsigned int offset, diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c index 7c8bbfcf6c3e..2443271e17d2 100644 --- a/drivers/vdpa/ifcvf/ifcvf_main.c +++ b/drivers/vdpa/ifcvf/ifcvf_main.c @@ -332,6 +332,11 @@ static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev) return IFCVF_QUEUE_ALIGNMENT; } +static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev) +{ + return sizeof(struct virtio_net_config); +} + static void ifcvf_vdpa_get_config(struct vdpa_device *vdpa_dev, unsigned int offset, void *buf, unsigned int len) @@ -392,6 +397,7 @@ static const struct vdpa_config_ops ifc_vdpa_ops = { .get_device_id = ifcvf_vdpa_get_device_id, .get_vendor_id = ifcvf_vdpa_get_vendor_id, .get_vq_align = ifcvf_vdpa_get_vq_align, + .get_config_size= ifcvf_vdpa_get_config_size, .get_config = ifcvf_vdpa_get_config, .set_config = ifcvf_vdpa_set_config, .set_config_cb = ifcvf_vdpa_set_config_cb, diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c index 10e9b09932eb..78043ee567b6 100644 --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c @@ -1814,6 +1814,11 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED; } +static size_t mlx5_vdpa_get_config_size(struct vdpa_device *vdev) +{ + return sizeof(struct virtio_net_config); +} + static void mlx5_vdpa_get_config(struct vdpa_device *vdev, unsigned int offset, void *buf, unsigned int len) { @@ -1900,6 +1905,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = { .get_vendor_id = mlx5_vdpa_get_vendor_id, .get_status = mlx5_vdpa_get_status, .set_status = mlx5_vdpa_set_status, + .get_config_size = mlx5_vdpa_get_config_size, .get_config = mlx5_vdpa_get_config, .set_config = mlx5_vdpa_set_config, .get_generation = mlx5_vdpa_get_generation, diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c index d5942842432d..779ae6c144d7 100644 --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c @@ -439,6 +439,13 @@ static void vdpasim_set_status(struct vdpa_device *vdpa, u8 status) spin_unlock(&vdpasim->lock); } +static size_t vdpasim_get_config_size(struct vdpa_device *vdpa) +{ + struct vdpasim *vdpasim = vdpa_to_sim(vdpa); + + return vdpasim->dev_attr.config_size; +} + static void vdpasim_get_config(struct vdpa_device *vdpa, unsigned int offset, void *buf, unsigned int len) { @@ -566,6 +573,7 @@ static const struct vdpa_config_ops vdpasim_config_ops = { .get_vendor_id = vdpasim_get_vendor_id, .get_status = vdpasim_get_status, .set_status = vdpasim_set_status, + .get_config_size= vdpasim_get_config_size, .get_config = vdpasim_get_config, .set_config = vdpasim_set_config, .get_generation = vdpasim_get_generation, @@ -593,6 +601,7 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = { .get_vendor_id = vdpasim_get_vendor_id, .get_status
[RFC PATCH 02/10] vdpa: check vdpa_get_config() parameters and return bytes read
Now we have the 'get_config_size()' callback available, so we can check that 'offset' and 'len' parameters are valid. When these exceed boundaries, we limit the reading to the available configuration space in the device, and we return the amount of bytes read. We also move vdpa_get_config() implementation in drivers/vdpa/vdpa.c, since the function are growing. Signed-off-by: Stefano Garzarella --- include/linux/vdpa.h | 16 ++-- drivers/vdpa/vdpa.c | 35 +++ 2 files changed, 37 insertions(+), 14 deletions(-) diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h index fddf42b17573..8a679c98f8b1 100644 --- a/include/linux/vdpa.h +++ b/include/linux/vdpa.h @@ -332,20 +332,8 @@ static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features) return ops->set_features(vdev, features); } - -static inline void vdpa_get_config(struct vdpa_device *vdev, unsigned offset, - void *buf, unsigned int len) -{ -const struct vdpa_config_ops *ops = vdev->config; - - /* -* Config accesses aren't supposed to trigger before features are set. -* If it does happen we assume a legacy guest. -*/ - if (!vdev->features_valid) - vdpa_set_features(vdev, 0); - ops->get_config(vdev, offset, buf, len); -} +int vdpa_get_config(struct vdpa_device *vdev, unsigned int offset, + void *buf, unsigned int len); /** * vdpa_mgmtdev_ops - vdpa device ops diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index 3d997b389345..9ed6c779c63c 100644 --- a/drivers/vdpa/vdpa.c +++ b/drivers/vdpa/vdpa.c @@ -51,6 +51,41 @@ static struct bus_type vdpa_bus = { .remove = vdpa_dev_remove, }; +static int vdpa_config_size_wrap(struct vdpa_device *vdev, unsigned int offset, +unsigned int len) +{ + const struct vdpa_config_ops *ops = vdev->config; + unsigned int config_size = ops->get_config_size(vdev); + + if (offset > config_size || len > config_size) + return -1; + + return min(len, config_size - offset); +} + +int vdpa_get_config(struct vdpa_device *vdev, unsigned int offset, + void *buf, unsigned int len) +{ + const struct vdpa_config_ops *ops = vdev->config; + int bytes_get; + + bytes_get = vdpa_config_size_wrap(vdev, offset, len); + if (bytes_get <= 0) + return bytes_get; + + /* +* Config accesses aren't supposed to trigger before features are set. +* If it does happen we assume a legacy guest. +*/ + if (!vdev->features_valid) + vdpa_set_features(vdev, 0); + + ops->get_config(vdev, offset, buf, bytes_get); + + return bytes_get; +} +EXPORT_SYMBOL_GPL(vdpa_get_config); + static void vdpa_release_dev(struct device *d) { struct vdpa_device *vdev = dev_to_vdpa(d); -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH 00/10] vdpa: get/set_config() rework
Following the discussion with Michael and Jason [1], I reworked a bit get/set_config() in vdpa. I changed vdpa_get_config() to check the boundaries and added vdpa_set_config(). When 'offset' or 'len' parameters exceed boundaries, we limit the reading to the available configuration space in the device, and we return the amount of bytes read/written. In this way the user space can pass buffers bigger than config space. I also returned the amount of bytes read and written to user space. Patches also available here: https://github.com/stefano-garzarella/linux/tree/vdpa-get-set-config-refactoring Thanks for your comments, Stefano [1] https://lkml.org/lkml/2021/2/10/350 Stefano Garzarella (10): vdpa: add get_config_size callback in vdpa_config_ops vdpa: check vdpa_get_config() parameters and return bytes read vdpa: add vdpa_set_config() helper vdpa: remove param checks in the get/set_config callbacks vdpa: remove WARN_ON() in the get/set_config callbacks virtio_vdpa: use vdpa_set_config() vhost/vdpa: use vdpa_set_config() vhost/vdpa: allow user space to pass buffers bigger than config space vhost/vdpa: use get_config_size callback in vhost_vdpa_config_validate() vhost/vdpa: return configuration bytes read and written to user space include/linux/vdpa.h | 22 --- drivers/vdpa/ifcvf/ifcvf_base.c | 3 +- drivers/vdpa/ifcvf/ifcvf_main.c | 8 +++- drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 - drivers/vdpa/vdpa.c | 51 drivers/vdpa/vdpa_sim/vdpa_sim.c | 15 +--- drivers/vhost/vdpa.c | 64 --- drivers/virtio/virtio_vdpa.c | 3 +- 8 files changed, 116 insertions(+), 59 deletions(-) -- 2.29.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [External] Re: vsock virtio: questions about supporting DGRAM type
On Tue, Feb 16, 2021 at 12:23:19AM -0800, Jiang Wang . wrote: On Tue, Feb 16, 2021 at 12:09 AM Stefano Garzarella wrote: On Mon, Feb 15, 2021 at 08:50:36PM -0800, Jiang Wang . wrote: >On Mon, Feb 15, 2021 at 12:31 AM Stefano Garzarella wrote: >> >> On Sat, Feb 13, 2021 at 03:26:18PM -0800, Jiang Wang . wrote: >> >On Fri, Feb 12, 2021 at 1:02 AM Stefano Garzarella wrote: >> >> >> >> Hi Jiang, >> >> >> >> CCing Arseny who is working on SOCK_SEQPACKET support for virtio-vsock >> >> [1]. >> >> >> >> On Thu, Feb 11, 2021 at 10:04:34PM -0800, Jiang Wang . wrote: >> >> >Hi guys, >> >> > >> >> >I am working on supporting DGRAM type for virtio/vhost vsock. I >> >> >already did some work and a draft code is here (which passed my tests, >> >> >but still need some cleanup and only works from host to guest as of >> >> >now, will add host to guest soon): >> >> >https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d >> >> >qemu changes are here: >> >> >https://github.com/Jiang1155/qemu/commit/7ab778801e3e8969ab98e44539943810a2fb03eb >> >> > >> >> >Today, I just noticed that the Asias had an old version of virtio >> >> >which had both dgram and stream support, see this link: >> >> >https://kvm.vger.kernel.narkive.com/BMvH9eEr/rfc-v2-0-7-introduce-vm-sockets-virtio-transport#post1 >> >> > >> >> >But somehow, the dgram part seems never merged to upstream linux (the >> >> >stream part is merged). If so, does anyone know what is the reason for >> >> >this? Did we drop dgram support for some specific reason or the code >> >> >needs some improvement? >> >> >> >> I wasn't involved yet in virtio-vsock development when Asias posted that >> >> patches, so I don't know the exact reason. >> >> >> >> Maybe could be related on how to handle the credit mechanism for a >> >> connection-less sockets and how to manage the packet queue, if for >> >> example no one is listening. >> >> >> > >> >I see. thanks. >> > >> >> > >> >> >My current code differs from Asias' code in some ways. It does not use >> >> >credit and does not support fragmentation. It basically adds two virt >> >> >> >> If you don't use credit, do you have some threshold when you start to >> >> drop packets on the RX side? >> >> >> > >> >As of now, I don't have any threshold to drop packets on RX side. In my >> >view, DGRAM is like UDP and is a best effort service. If the virtual >> >queue >> >is full on TX (or the available buffer size is less than the >> >packet size), >> >I drop the packets on the TX side. >> >> I have to check the code, my concern is we should have a limit for the >> allocation, for example if the user space doesn't consume RX packets. >> > >I think we already have a limit for the allocation. If a DGRAM client >sends packets while no socket is bound on the server side , >the packet will be dropped when looking for the socket. If the socket is >bound on the server side, but the server program somehow does not >call recv or similar functions, the packets will be dropped when the >buffer is full as in your previous patch at here: >https://lore.kernel.org/patchwork/patch/1141083/ >If there are still other cases that we don't have protection, let me know and >I can add some threshold. Maybe I misunderstood, but I thought you didn't use credit for DGRAM messages. I don't use credit for DGRAM. But the dropping code I mentioned does not rely on credit too. Just to make it clear, following is one part of code that I referred to: static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs, struct virtio_vsock_pkt *pkt) { if (vvs->rx_bytes + pkt->len > vvs->buf_alloc) return false; vvs->rx_bytes += pkt->len; return true; } Okay, now it's clear. The transmitter doesn't care about the receiver's credit, but the receiver uses that part of the credit mechanism to discard packets. I think that's fine, but when you send the patches, explain it well in the cover letter/commit message. If you use it to limit buffer occupancy, then it should be okay, but again, I still have to look at the code :-) Sure. I think you mean you need to look at my final patches. I will send it later. Just a friendly reminder, if you just want to get some idea of my patches, you could check this link : https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d It's my fault because I didn't have time, I saw the link. :-) Remember that we should also plan changes to the VIRTIO spec for the DGRAM support. Thanks, Stefano ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [External] Re: vsock virtio: questions about supporting DGRAM type
On Tue, Feb 16, 2021 at 12:09 AM Stefano Garzarella wrote: > > On Mon, Feb 15, 2021 at 08:50:36PM -0800, Jiang Wang . wrote: > >On Mon, Feb 15, 2021 at 12:31 AM Stefano Garzarella > >wrote: > >> > >> On Sat, Feb 13, 2021 at 03:26:18PM -0800, Jiang Wang . wrote: > >> >On Fri, Feb 12, 2021 at 1:02 AM Stefano Garzarella > >> >wrote: > >> >> > >> >> Hi Jiang, > >> >> > >> >> CCing Arseny who is working on SOCK_SEQPACKET support for virtio-vsock > >> >> [1]. > >> >> > >> >> On Thu, Feb 11, 2021 at 10:04:34PM -0800, Jiang Wang . wrote: > >> >> >Hi guys, > >> >> > > >> >> >I am working on supporting DGRAM type for virtio/vhost vsock. I > >> >> >already did some work and a draft code is here (which passed my tests, > >> >> >but still need some cleanup and only works from host to guest as of > >> >> >now, will add host to guest soon): > >> >> >https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d > >> >> >qemu changes are here: > >> >> >https://github.com/Jiang1155/qemu/commit/7ab778801e3e8969ab98e44539943810a2fb03eb > >> >> > > >> >> >Today, I just noticed that the Asias had an old version of virtio > >> >> >which had both dgram and stream support, see this link: > >> >> >https://kvm.vger.kernel.narkive.com/BMvH9eEr/rfc-v2-0-7-introduce-vm-sockets-virtio-transport#post1 > >> >> > > >> >> >But somehow, the dgram part seems never merged to upstream linux (the > >> >> >stream part is merged). If so, does anyone know what is the reason for > >> >> >this? Did we drop dgram support for some specific reason or the code > >> >> >needs some improvement? > >> >> > >> >> I wasn't involved yet in virtio-vsock development when Asias posted that > >> >> patches, so I don't know the exact reason. > >> >> > >> >> Maybe could be related on how to handle the credit mechanism for a > >> >> connection-less sockets and how to manage the packet queue, if for > >> >> example no one is listening. > >> >> > >> > > >> >I see. thanks. > >> > > >> >> > > >> >> >My current code differs from Asias' code in some ways. It does not use > >> >> >credit and does not support fragmentation. It basically adds two virt > >> >> > >> >> If you don't use credit, do you have some threshold when you start to > >> >> drop packets on the RX side? > >> >> > >> > > >> >As of now, I don't have any threshold to drop packets on RX side. In my > >> >view, DGRAM is like UDP and is a best effort service. If the virtual > >> >queue > >> >is full on TX (or the available buffer size is less than the packet size), > >> >I drop the packets on the TX side. > >> > >> I have to check the code, my concern is we should have a limit for the > >> allocation, for example if the user space doesn't consume RX packets. > >> > > > >I think we already have a limit for the allocation. If a DGRAM client > >sends packets while no socket is bound on the server side , > >the packet will be dropped when looking for the socket. If the socket is > >bound on the server side, but the server program somehow does not > >call recv or similar functions, the packets will be dropped when the > >buffer is full as in your previous patch at here: > >https://lore.kernel.org/patchwork/patch/1141083/ > >If there are still other cases that we don't have protection, let me know and > >I can add some threshold. > > Maybe I misunderstood, but I thought you didn't use credit for DGRAM > messages. > I don't use credit for DGRAM. But the dropping code I mentioned does not rely on credit too. Just to make it clear, following is one part of code that I referred to: static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs, struct virtio_vsock_pkt *pkt) { if (vvs->rx_bytes + pkt->len > vvs->buf_alloc) return false; vvs->rx_bytes += pkt->len; return true; } > If you use it to limit buffer occupancy, then it should be okay, but > again, I still have to look at the code :-) Sure. I think you mean you need to look at my final patches. I will send it later. Just a friendly reminder, if you just want to get some idea of my patches, you could check this link : https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d thanks. :) > Thanks, > Stefano > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [External] Re: vsock virtio: questions about supporting DGRAM type
On Mon, Feb 15, 2021 at 08:50:36PM -0800, Jiang Wang . wrote: On Mon, Feb 15, 2021 at 12:31 AM Stefano Garzarella wrote: On Sat, Feb 13, 2021 at 03:26:18PM -0800, Jiang Wang . wrote: >On Fri, Feb 12, 2021 at 1:02 AM Stefano Garzarella wrote: >> >> Hi Jiang, >> >> CCing Arseny who is working on SOCK_SEQPACKET support for virtio-vsock >> [1]. >> >> On Thu, Feb 11, 2021 at 10:04:34PM -0800, Jiang Wang . wrote: >> >Hi guys, >> > >> >I am working on supporting DGRAM type for virtio/vhost vsock. I >> >already did some work and a draft code is here (which passed my tests, >> >but still need some cleanup and only works from host to guest as of >> >now, will add host to guest soon): >> >https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d >> >qemu changes are here: >> >https://github.com/Jiang1155/qemu/commit/7ab778801e3e8969ab98e44539943810a2fb03eb >> > >> >Today, I just noticed that the Asias had an old version of virtio >> >which had both dgram and stream support, see this link: >> >https://kvm.vger.kernel.narkive.com/BMvH9eEr/rfc-v2-0-7-introduce-vm-sockets-virtio-transport#post1 >> > >> >But somehow, the dgram part seems never merged to upstream linux (the >> >stream part is merged). If so, does anyone know what is the reason for >> >this? Did we drop dgram support for some specific reason or the code >> >needs some improvement? >> >> I wasn't involved yet in virtio-vsock development when Asias posted that >> patches, so I don't know the exact reason. >> >> Maybe could be related on how to handle the credit mechanism for a >> connection-less sockets and how to manage the packet queue, if for >> example no one is listening. >> > >I see. thanks. > >> > >> >My current code differs from Asias' code in some ways. It does not use >> >credit and does not support fragmentation. It basically adds two virt >> >> If you don't use credit, do you have some threshold when you start to >> drop packets on the RX side? >> > >As of now, I don't have any threshold to drop packets on RX side. In my >view, DGRAM is like UDP and is a best effort service. If the virtual >queue >is full on TX (or the available buffer size is less than the packet size), >I drop the packets on the TX side. I have to check the code, my concern is we should have a limit for the allocation, for example if the user space doesn't consume RX packets. I think we already have a limit for the allocation. If a DGRAM client sends packets while no socket is bound on the server side , the packet will be dropped when looking for the socket. If the socket is bound on the server side, but the server program somehow does not call recv or similar functions, the packets will be dropped when the buffer is full as in your previous patch at here: https://lore.kernel.org/patchwork/patch/1141083/ If there are still other cases that we don't have protection, let me know and I can add some threshold. Maybe I misunderstood, but I thought you didn't use credit for DGRAM messages. If you use it to limit buffer occupancy, then it should be okay, but again, I still have to look at the code :-) Thanks, Stefano ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization