Re: [PATCH v3 1/2] vdpa/mlx5: Extend driver support for new features
On 3/20/2023 4:49 AM, Eli Cohen wrote: Extend the possible list for features that can be supported by firmware. Note that different versions of firmware may or may not support these features. The driver is made aware of them by querying the firmware. While doing this, improve the code so we use enum names instead of hard coded numerical values. The new features supported by the driver are the following: VIRTIO_NET_F_MRG_RXBUF VIRTIO_NET_F_HOST_ECN VIRTIO_NET_F_GUEST_ECN VIRTIO_NET_F_GUEST_TSO6 VIRTIO_NET_F_GUEST_TSO4 Signed-off-by: Eli Cohen --- drivers/vdpa/mlx5/net/mlx5_vnet.c | 56 ++- 1 file changed, 40 insertions(+), 16 deletions(-) diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c index 520646ae7fa0..5285dd76c793 100644 --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c @@ -778,12 +778,28 @@ static bool vq_is_tx(u16 idx) return idx % 2; } -static u16 get_features_12_3(u64 features) +enum { + MLX5_VIRTIO_NET_F_MRG_RXBUF = 2, + MLX5_VIRTIO_NET_F_HOST_ECN = 4, + MLX5_VIRTIO_NET_F_GUEST_ECN = 6, + MLX5_VIRTIO_NET_F_GUEST_TSO6 = 7, + MLX5_VIRTIO_NET_F_GUEST_TSO4 = 8, + MLX5_VIRTIO_NET_F_GUEST_CSUM = 9, + MLX5_VIRTIO_NET_F_CSUM = 10, + MLX5_VIRTIO_NET_F_HOST_TSO6 = 11, + MLX5_VIRTIO_NET_F_HOST_TSO4 = 12, +}; + +static u16 get_features(u64 features) { - return (!!(features & BIT_ULL(VIRTIO_NET_F_HOST_TSO4)) << 9) | - (!!(features & BIT_ULL(VIRTIO_NET_F_HOST_TSO6)) << 8) | - (!!(features & BIT_ULL(VIRTIO_NET_F_CSUM)) << 7) | - (!!(features & BIT_ULL(VIRTIO_NET_F_GUEST_CSUM)) << 6); + return (!!(features & BIT_ULL(VIRTIO_NET_F_MRG_RXBUF)) << MLX5_VIRTIO_NET_F_MRG_RXBUF) | + (!!(features & BIT_ULL(VIRTIO_NET_F_HOST_ECN)) << MLX5_VIRTIO_NET_F_HOST_ECN) | + (!!(features & BIT_ULL(VIRTIO_NET_F_GUEST_ECN)) << MLX5_VIRTIO_NET_F_GUEST_ECN) | + (!!(features & BIT_ULL(VIRTIO_NET_F_GUEST_TSO6)) << MLX5_VIRTIO_NET_F_GUEST_TSO6) | + (!!(features & BIT_ULL(VIRTIO_NET_F_GUEST_TSO4)) << MLX5_VIRTIO_NET_F_GUEST_TSO4) | + (!!(features & BIT_ULL(VIRTIO_NET_F_CSUM)) << MLX5_VIRTIO_NET_F_CSUM) | + (!!(features & BIT_ULL(VIRTIO_NET_F_HOST_TSO6)) << MLX5_VIRTIO_NET_F_HOST_TSO6) | + (!!(features & BIT_ULL(VIRTIO_NET_F_HOST_TSO4)) << MLX5_VIRTIO_NET_F_HOST_TSO4); } static bool counters_supported(const struct mlx5_vdpa_dev *mvdev) @@ -797,6 +813,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque int inlen = MLX5_ST_SZ_BYTES(create_virtio_net_q_in); u32 out[MLX5_ST_SZ_DW(create_virtio_net_q_out)] = {}; void *obj_context; + u16 mlx_features; void *cmd_hdr; void *vq_ctx; void *in; @@ -812,6 +829,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque goto err_alloc; } + mlx_features = get_features(ndev->mvdev.actual_features); cmd_hdr = MLX5_ADDR_OF(create_virtio_net_q_in, in, general_obj_in_cmd_hdr); MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, opcode, MLX5_CMD_OP_CREATE_GENERAL_OBJECT); @@ -822,7 +840,9 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque 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)); +mlx_features >> 3); + MLX5_SET(virtio_net_q_object, obj_context, queue_feature_bit_mask_2_0, +mlx_features & 7); vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context); MLX5_SET(virtio_q, vq_ctx, virtio_q_type, get_queue_type(ndev)); @@ -2171,23 +2191,27 @@ static u32 mlx5_vdpa_get_vq_group(struct vdpa_device *vdev, u16 idx) return MLX5_VDPA_DATAVQ_GROUP; } -enum { MLX5_VIRTIO_NET_F_GUEST_CSUM = 1 << 9, - MLX5_VIRTIO_NET_F_CSUM = 1 << 10, - MLX5_VIRTIO_NET_F_HOST_TSO6 = 1 << 11, - MLX5_VIRTIO_NET_F_HOST_TSO4 = 1 << 12, -}; - static u64 mlx_to_vritio_features(u16 dev_features) { u64 result = 0; - if (dev_features & MLX5_VIRTIO_NET_F_GUEST_CSUM) + if (dev_features & BIT_ULL(MLX5_VIRTIO_NET_F_MRG_RXBUF)) + result += BIT_ULL(VIRTIO_NET_F_MRG_RXBUF); + if (dev_features & BIT_ULL(MLX5_VIRTIO_NET_F_HOST_ECN)) + result += BIT_ULL(VIRTIO_NET_F_HOST_ECN); + if (dev_features & BIT_ULL(MLX5_VIRTIO_NET_F_GUEST_ECN)) + result += BIT_ULL(VIRTIO_NET_F_GUEST_ECN); + if (dev_features & BIT_ULL(MLX5_VIRTIO_NET_F_GUEST_TSO6)) + result += BIT_ULL(VIRTIO_NET_F_GUEST_TSO6);
Re: [PATCH v2 1/2] vdpa/mlx5: Extend driver support for new features
On 3/19/2023 11:44 PM, Jason Wang wrote: On Fri, Mar 17, 2023 at 9:58 PM Parav Pandit wrote: From: Eli Cohen Sent: Wednesday, March 15, 2023 3:28 AM Extend the possible list for features that can be supported by firmware. Note that different versions of firmware may or may not support these features. The driver is made aware of them by querying the firmware. While doing this, improve the code so we use enum names instead of hard coded numerical values. The new features supported by the driver are the following: VIRTIO_NET_F_MRG_RXBUF VIRTIO_NET_F_HOST_UFO UFO is deprecated in Linux kernel, there are no known user either and we do not plan to support it. Note that there's an emulation code for preserving migration compatibility in the kernel. I wonder if there's a command line option to prohibit QEMU from saving this host capability to the migration stream? If not I think it's a nightmare every vendor has to support already-deprecated UFO in their h/w device. Thanks, -Siwei Please remove this entry along with below GUEST_UFO. If there's no plan for supporting migration from existing software backends, we can remove this. Thanks VIRTIO_NET_F_HOST_ECN VIRTIO_NET_F_GUEST_UFO VIRTIO_NET_F_GUEST_ECN VIRTIO_NET_F_GUEST_TSO6 VIRTIO_NET_F_GUEST_TSO4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vdpa/mlx5: Remove debugfs file after device unregister
On Mon, Mar 20, 2023 at 5:35 PM Eli Cohen wrote: > > > On 20/03/2023 11:28, Jason Wang wrote: > > On Mon, Mar 20, 2023 at 5:07 PM Eli Cohen wrote: > >> On 17/03/2023 5:32, Jason Wang wrote: > >>> On Sun, Mar 12, 2023 at 4:41 PM Eli Cohen wrote: > When deleting the vdpa device, the debugfs files need to be removed so > need to remove debugfs after the device has been unregistered. > > This fixes null pointer dereference when someone deletes the device > after debugfs has been populated. > > Fixes: 294221004322 ("vdpa/mlx5: Add debugfs subtree") > 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 3858ba1e8975..3f6149f2ffd4 100644 > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > @@ -3322,8 +3322,6 @@ static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev > *v_mdev, struct vdpa_device * > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev); > struct workqueue_struct *wq; > > - mlx5_vdpa_remove_debugfs(ndev->debugfs); > - ndev->debugfs = NULL; > if (ndev->nb_registered) { > ndev->nb_registered = false; > mlx5_notifier_unregister(mvdev->mdev, &ndev->nb); > @@ -3332,6 +3330,8 @@ static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev > *v_mdev, struct vdpa_device * > mvdev->wq = NULL; > destroy_workqueue(wq); > _vdpa_unregister_device(dev); > >>> What if the user tries to access debugfs after _vdpa_unregister_device()? > >> I don't see an issue with that. During the process of deleting a device, > >> the resources are destroyed and their corresponding debugfs files are > >> also destroyed just prior to destroying the resource. > > For example, rx_flow_table_show() requires the structure mlx5_vdpa_net > > alive, but it seems the structure has been freed after > > _vdpa_unregister_device(). > But in this case, rx_flow_table_show() gets called first, so the file > will be removed from the debugfs before > > mlx5_vdpa_net gets released. Just to make sure we are at the same page. I meant: [CPU 0] _vdpa_unregister_device(dev); [CPU 1] rx_flow_table_show() [CPU 0] mlx5_vdpa_remove_debugfs() So when CPU 1 tries to access the flow table, the ndev has been released by CPU0 in _vdpa_unregister_device()? Thanks > > > > > If a user tries to access that file after _vdpa_unregister_device() > > but before mlx5_vdpa_remove_debugfs(), will we end up with > > use-after-free? > > > > Thanks > > > > > >>> Thanks > >>> > + mlx5_vdpa_remove_debugfs(ndev->debugfs); > + ndev->debugfs = NULL; > mgtdev->ndev = NULL; > } > > -- > 2.38.1 > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 0/7] vhost-scsi: Fix crashes and management op hangs
On 3/20/23 9:06 PM, Mike Christie wrote: > The following patches were made over Linus tree. Hi Michael, I see you merged my first version of the patchset in your vhost branch. Do you want me to just send a followup patchset? The major diff between the 2 versions: 1. I added the first 2 patches which fix some bugs in the existing code I found while doing some code review and testing another LIO patchset plus v1. Note: The other day I posted that I thought the 3rd patch in v1 caused the bugs but they were already in the code. 2. In v2 I made one of the patches not need the vhost device lock when unmapping/mapping LUNs, so you can add new LUNs even if one LUN on the same vhost_scsi device was hung. Since it's not regressions with the existing patches, I can just send those as a followup patchset if that's preferred. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support
On Tue, Mar 21, 2023 at 7:21 AM Viktor Prutyanov wrote: > > According to VirtIO spec v1.2, VIRTIO_F_NOTIFICATION_DATA feature > indicates that the driver passes extra data along with the queue > notifications. > > In a split queue case, the extra data is 16-bit available index. In a > packed queue case, the extra data is 1-bit wrap counter and 15-bit > available index. > > Add support for this feature for MMIO and PCI transports. Channel I/O > transport will not accept this feature. > > Signed-off-by: Viktor Prutyanov > --- > > v2: reject the feature in virtio_ccw, replace __le32 with u32 > > drivers/s390/virtio/virtio_ccw.c | 4 +--- > drivers/virtio/virtio_mmio.c | 15 ++- > drivers/virtio/virtio_pci_common.c | 10 ++ > drivers/virtio/virtio_pci_common.h | 4 > drivers/virtio/virtio_pci_legacy.c | 2 +- > drivers/virtio/virtio_pci_modern.c | 2 +- > drivers/virtio/virtio_ring.c | 17 + > include/linux/virtio_ring.h| 2 ++ > include/uapi/linux/virtio_config.h | 6 ++ > 9 files changed, 56 insertions(+), 6 deletions(-) > > diff --git a/drivers/s390/virtio/virtio_ccw.c > b/drivers/s390/virtio/virtio_ccw.c > index a10dbe632ef9..d72a59415527 100644 > --- a/drivers/s390/virtio/virtio_ccw.c > +++ b/drivers/s390/virtio/virtio_ccw.c > @@ -789,9 +789,7 @@ static u64 virtio_ccw_get_features(struct virtio_device > *vdev) > > static void ccw_transport_features(struct virtio_device *vdev) > { > - /* > -* Currently nothing to do here. > -*/ > + __virtio_clear_bit(vdev, VIRTIO_F_NOTIFICATION_DATA); Is there any restriction that prevents us from implementing VIRTIO_F_NOTIFICATION_DATA? (Spec seems doesn't limit us from this) > } > > static int virtio_ccw_finalize_features(struct virtio_device *vdev) > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c > index 3ff746e3f24a..0e13da17fe0a 100644 > --- a/drivers/virtio/virtio_mmio.c > +++ b/drivers/virtio/virtio_mmio.c > @@ -285,6 +285,19 @@ static bool vm_notify(struct virtqueue *vq) > return true; > } > > +static bool vm_notify_with_data(struct virtqueue *vq) > +{ > + struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev); > + u32 data = vring_fill_notification_data(vq); Can we move this to the initialization? Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support
On Mon, 20 Mar 2023 14:54:51 +0300, Viktor Prutyanov wrote: > According to VirtIO spec v1.2, VIRTIO_F_NOTIFICATION_DATA feature > indicates that the driver passes extra data along with the queue > notifications. > > In a split queue case, the extra data is 16-bit available index. In a > packed queue case, the extra data is 1-bit wrap counter and 15-bit > available index. > > Add support for this feature for both MMIO and PCI. > > Signed-off-by: Viktor Prutyanov > --- > drivers/virtio/virtio_mmio.c | 15 ++- > drivers/virtio/virtio_pci_common.c | 10 ++ > drivers/virtio/virtio_pci_common.h | 4 > drivers/virtio/virtio_pci_legacy.c | 2 +- > drivers/virtio/virtio_pci_modern.c | 2 +- > drivers/virtio/virtio_ring.c | 17 + > include/linux/virtio_ring.h| 2 ++ > include/uapi/linux/virtio_config.h | 6 ++ > 8 files changed, 55 insertions(+), 3 deletions(-) > > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c > index 3ff746e3f24a..05da5ad7fc93 100644 > --- a/drivers/virtio/virtio_mmio.c > +++ b/drivers/virtio/virtio_mmio.c > @@ -285,6 +285,19 @@ static bool vm_notify(struct virtqueue *vq) > return true; > } > > +static bool vm_notify_with_data(struct virtqueue *vq) > +{ > + struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev); > + __le32 data = vring_fill_notification_data(vq); > + > + writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY); > + > + return true; > +} > + > +#define VM_NOTIFY(vdev) (__virtio_test_bit((vdev), > VIRTIO_F_NOTIFICATION_DATA) \ > + ? vm_notify_with_data : vm_notify) Is this macro necessary, it is only used once. And I don't recognize that this logic is necessary to use macro. > + > /* Notify all virtqueues on an interrupt. */ > static irqreturn_t vm_interrupt(int irq, void *opaque) > { > @@ -397,7 +410,7 @@ static struct virtqueue *vm_setup_vq(struct virtio_device > *vdev, unsigned int in > > /* Create the vring */ > vq = vring_create_virtqueue(index, num, VIRTIO_MMIO_VRING_ALIGN, vdev, > - true, true, ctx, vm_notify, callback, name); > + true, true, ctx, VM_NOTIFY(vdev), callback, name); > if (!vq) { > err = -ENOMEM; > goto error_new_virtqueue; > diff --git a/drivers/virtio/virtio_pci_common.c > b/drivers/virtio/virtio_pci_common.c > index a6c86f916dbd..bf7daad9ce65 100644 > --- a/drivers/virtio/virtio_pci_common.c > +++ b/drivers/virtio/virtio_pci_common.c > @@ -43,6 +43,16 @@ bool vp_notify(struct virtqueue *vq) > /* we write the queue's selector into the notification register to >* signal the other end */ > iowrite16(vq->index, (void __iomem *)vq->priv); > + > + return true; > +} > + > +bool vp_notify_with_data(struct virtqueue *vq) > +{ > + __le32 data = vring_fill_notification_data(vq); > + > + iowrite32(data, (void __iomem *)vq->priv); > + > return true; > } > > diff --git a/drivers/virtio/virtio_pci_common.h > b/drivers/virtio/virtio_pci_common.h > index 23112d84218f..9a7212dcbb32 100644 > --- a/drivers/virtio/virtio_pci_common.h > +++ b/drivers/virtio/virtio_pci_common.h > @@ -105,6 +105,7 @@ static struct virtio_pci_device *to_vp_device(struct > virtio_device *vdev) > void vp_synchronize_vectors(struct virtio_device *vdev); > /* the notify function used when creating a virt queue */ > bool vp_notify(struct virtqueue *vq); > +bool vp_notify_with_data(struct virtqueue *vq); > /* the config->del_vqs() implementation */ > void vp_del_vqs(struct virtio_device *vdev); > /* the config->find_vqs() implementation */ > @@ -114,6 +115,9 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned int > nvqs, > struct irq_affinity *desc); > const char *vp_bus_name(struct virtio_device *vdev); > > +#define VP_NOTIFY(vdev) (__virtio_test_bit((vdev), > VIRTIO_F_NOTIFICATION_DATA) \ > + ? vp_notify : vp_notify_with_data) > + I also think that this is not necessary, although it has been used twice. > /* Setup the affinity for a virtqueue: > * - force the affinity for per vq vector > * - OR over all affinities for shared MSI > diff --git a/drivers/virtio/virtio_pci_legacy.c > b/drivers/virtio/virtio_pci_legacy.c > index 2257f1b3d8ae..b98e994cae48 100644 > --- a/drivers/virtio/virtio_pci_legacy.c > +++ b/drivers/virtio/virtio_pci_legacy.c > @@ -131,7 +131,7 @@ static struct virtqueue *setup_vq(struct > virtio_pci_device *vp_dev, > vq = vring_create_virtqueue(index, num, > VIRTIO_PCI_VRING_ALIGN, &vp_dev->vdev, > true, false, ctx, > - vp_notify, callback, name); > + VP_NOTIFY(&vp_dev->vdev), callback, name); > if (!vq) > return ERR_PTR(-ENOMEM); > > diff --git a/drivers/virtio/virtio_pci_modern.c > b/drivers/virtio/virtio_pci_mode
[PATCH v2 6/7] vhost-scsi: Drop vhost_scsi_mutex use in port callouts
We are using the vhost_scsi_mutex to make sure vhost_scsi_port_link and vhost_scsi_port_unlink see if vhost_scsi_clear_endpoint has cleared tpg->vhost_scsi and it can't be freed while they are using. However, we currently set the tpg->vhost_scsi pointer while holding tv_tpg_mutex. So, we can just hold that while calling vhost_scsi_hotplug/hotunplug. We then don't need to hold the vhost_scsi_mutex while vhost_scsi_clear_endpoint is holding it and doing a flush which could cause the LUN map/unmap to have to wait on another device's flush. Signed-off-by: Mike Christie --- drivers/vhost/scsi.c | 14 ++ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c index ba8097fcea43..d4372a4aff49 100644 --- a/drivers/vhost/scsi.c +++ b/drivers/vhost/scsi.c @@ -2040,15 +2040,10 @@ static int vhost_scsi_port_link(struct se_portal_group *se_tpg, struct vhost_scsi_tpg *tpg = container_of(se_tpg, struct vhost_scsi_tpg, se_tpg); - mutex_lock(&vhost_scsi_mutex); - mutex_lock(&tpg->tv_tpg_mutex); tpg->tv_tpg_port_count++; - mutex_unlock(&tpg->tv_tpg_mutex); - vhost_scsi_hotplug(tpg, lun); - - mutex_unlock(&vhost_scsi_mutex); + mutex_unlock(&tpg->tv_tpg_mutex); return 0; } @@ -2059,15 +2054,10 @@ static void vhost_scsi_port_unlink(struct se_portal_group *se_tpg, struct vhost_scsi_tpg *tpg = container_of(se_tpg, struct vhost_scsi_tpg, se_tpg); - mutex_lock(&vhost_scsi_mutex); - mutex_lock(&tpg->tv_tpg_mutex); tpg->tv_tpg_port_count--; - mutex_unlock(&tpg->tv_tpg_mutex); - vhost_scsi_hotunplug(tpg, lun); - - mutex_unlock(&vhost_scsi_mutex); + mutex_unlock(&tpg->tv_tpg_mutex); } static ssize_t vhost_scsi_tpg_attrib_fabric_prot_type_store( -- 2.25.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 7/7] vhost-scsi: Reduce vhost_scsi_mutex use
We on longer need to hold the vhost_scsi_mutex the entire time we set/clear the endpoint. The tv_tpg_mutex handles tpg accesses not related to the tpg list, the port link/unlink functions use the tv_tpg_mutex while accessing the tpg->vhost_scsi pointer, vhost_scsi_do_plug will no longer queue events after the virtqueue's backend has been cleared and flushed, and we don't drop our refcount to the tpg until after we have stopped cmds and wait for outstanding cmds to complete. This moves the vhost_scsi_mutex use to it's documented use of being used to access the tpg list. We then don't need to hold it while a flush is being performed causing other device's vhost_scsi_set_endpoint and vhost_scsi_make_tpg/vhost_scsi_drop_tpg calls to have to wait on a flakey device. Signed-off-by: Mike Christie --- drivers/vhost/scsi.c | 16 +--- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c index d4372a4aff49..3b0b556c57ef 100644 --- a/drivers/vhost/scsi.c +++ b/drivers/vhost/scsi.c @@ -229,7 +229,10 @@ struct vhost_scsi_ctx { struct iov_iter out_iter; }; -/* Global spinlock to protect vhost_scsi TPG list for vhost IOCTL access */ +/* + * Global mutex to protect vhost_scsi TPG list for vhost IOCTLs and LIO + * configfs management operations. + */ static DEFINE_MUTEX(vhost_scsi_mutex); static LIST_HEAD(vhost_scsi_list); @@ -1526,7 +1529,7 @@ static int vhost_scsi_setup_vq_cmds(struct vhost_virtqueue *vq, int max_cmds) * vhost_scsi_tpg with an active struct vhost_scsi_nexus * * The lock nesting rule is: - *vhost_scsi_mutex -> vs->dev.mutex -> tpg->tv_tpg_mutex -> vq->mutex + *vs->dev.mutex -> vhost_scsi_mutex -> tpg->tv_tpg_mutex -> vq->mutex */ static int vhost_scsi_set_endpoint(struct vhost_scsi *vs, @@ -1540,7 +1543,6 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs, int index, ret, i, len; bool match = false; - mutex_lock(&vhost_scsi_mutex); mutex_lock(&vs->dev.mutex); /* Verify that ring has been setup correctly. */ @@ -1561,6 +1563,7 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs, if (vs->vs_tpg) memcpy(vs_tpg, vs->vs_tpg, len); + mutex_lock(&vhost_scsi_mutex); list_for_each_entry(tpg, &vhost_scsi_list, tv_tpg_list) { mutex_lock(&tpg->tv_tpg_mutex); if (!tpg->tpg_nexus) { @@ -1576,6 +1579,7 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs, if (!strcmp(tv_tport->tport_name, t->vhost_wwpn)) { if (vs->vs_tpg && vs->vs_tpg[tpg->tport_tpgt]) { mutex_unlock(&tpg->tv_tpg_mutex); + mutex_unlock(&vhost_scsi_mutex); ret = -EEXIST; goto undepend; } @@ -1590,6 +1594,7 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs, if (ret) { pr_warn("target_depend_item() failed: %d\n", ret); mutex_unlock(&tpg->tv_tpg_mutex); + mutex_unlock(&vhost_scsi_mutex); goto undepend; } tpg->tv_tpg_vhost_count++; @@ -1599,6 +1604,7 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs, } mutex_unlock(&tpg->tv_tpg_mutex); } + mutex_unlock(&vhost_scsi_mutex); if (match) { memcpy(vs->vs_vhost_wwpn, t->vhost_wwpn, @@ -1654,7 +1660,6 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs, kfree(vs_tpg); out: mutex_unlock(&vs->dev.mutex); - mutex_unlock(&vhost_scsi_mutex); return ret; } @@ -1670,7 +1675,6 @@ vhost_scsi_clear_endpoint(struct vhost_scsi *vs, int index, ret, i; u8 target; - mutex_lock(&vhost_scsi_mutex); mutex_lock(&vs->dev.mutex); /* Verify that ring has been setup correctly. */ for (index = 0; index < vs->dev.nvqs; ++index) { @@ -1757,12 +1761,10 @@ vhost_scsi_clear_endpoint(struct vhost_scsi *vs, vs->vs_tpg = NULL; WARN_ON(vs->vs_events_nr); mutex_unlock(&vs->dev.mutex); - mutex_unlock(&vhost_scsi_mutex); return 0; err_dev: mutex_unlock(&vs->dev.mutex); - mutex_unlock(&vhost_scsi_mutex); return ret; } -- 2.25.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 5/7] vhost-scsi: Check for a cleared backend before queueing an event
We currenly hold the vhost_scsi_mutex while clearing the endpoint and while performing vhost_scsi_do_plug, so tpg->vhost_scsi can't be freed from uder us, and to make sure anything queued is handled by the full call in vhost_scsi_clear_endpoint. This patch removes the need for the vhost_scsi_mutex for the latter case. In the next patches, we won't hold the vhost_scsi_mutex while flushing so this patch adds a check for the clearing of the virtqueue from vhost_scsi_clear_endpoint. We then know that once vhost_scsi_clear_endpoint has cleared the backend that no new events will be queued, and the flush after the vhost_vq_set_backend(vq, NULL) call will see everything that's been queued to that point. So the flush will then handle all events without the need for the vhost_scsi_mutex. Signed-off-by: Mike Christie --- drivers/vhost/scsi.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c index c945136ecf18..ba8097fcea43 100644 --- a/drivers/vhost/scsi.c +++ b/drivers/vhost/scsi.c @@ -2010,9 +2010,17 @@ vhost_scsi_do_plug(struct vhost_scsi_tpg *tpg, vq = &vs->vqs[VHOST_SCSI_VQ_EVT].vq; mutex_lock(&vq->mutex); + /* +* We can't queue events if the backend has been cleared, because +* we could end up queueing an event after the flush. +*/ + if (!vhost_vq_get_backend(vq)) + goto unlock; + if (vhost_has_feature(vq, VIRTIO_SCSI_F_HOTPLUG)) vhost_scsi_send_evt(vs, tpg, lun, VIRTIO_SCSI_T_TRANSPORT_RESET, reason); +unlock: mutex_unlock(&vq->mutex); } -- 2.25.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 4/7] vhost-scsi: Drop device mutex use in vhost_scsi_do_plug
We don't need the device mutex in vhost_scsi_do_plug because: 1. we have the vhost_scsi_mutex so the tpg->vhost_scsi pointer will not change on us and the vhost_scsi can't be freed from under us if it was set. 2. vhost_scsi_clear_endpoint will stop the virtqueues and flush them while holding the vhost_scsi_mutex so we know once vhost_scsi_clear_endpoint has completed that vhost_scsi_do_plug can't send new events and any queued ones have completed. So this patch drops the device mutex use in vhost_scsi_do_plug. Signed-off-by: Mike Christie --- drivers/vhost/scsi.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c index 502d6803df0b..c945136ecf18 100644 --- a/drivers/vhost/scsi.c +++ b/drivers/vhost/scsi.c @@ -2003,8 +2003,6 @@ vhost_scsi_do_plug(struct vhost_scsi_tpg *tpg, if (!vs) return; - mutex_lock(&vs->dev.mutex); - if (plug) reason = VIRTIO_SCSI_EVT_RESET_RESCAN; else @@ -2016,7 +2014,6 @@ vhost_scsi_do_plug(struct vhost_scsi_tpg *tpg, vhost_scsi_send_evt(vs, tpg, lun, VIRTIO_SCSI_T_TRANSPORT_RESET, reason); mutex_unlock(&vq->mutex); - mutex_unlock(&vs->dev.mutex); } static void vhost_scsi_hotplug(struct vhost_scsi_tpg *tpg, struct se_lun *lun) -- 2.25.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 2/7] vhost-scsi: Fix crash during LUN unmapping
We normally clear the endpoint then unmap LUNs so the devices are fully shutdown when the LUN is unmapped, but it's legal to unmap before clearing. If the user does that while TMFs are running then we can end up crashing. vhost_scsi_port_unlink assumes that the LUN's tmf struct will always be on the tmf_queue list. However, if a TMF is running then it will have been removed while it's executing. If we do a LUN unmap at this time, then we assume the entry is on the list and just start accessing it and free it. This fixes the bug by just allocating the vhost_scsi_tmf struct when it's needed like is done with the se_tmr struct that's needed when we submit the TMF. In this path perf is not an issue and we can use GFP_KERNEL since it won't swing directly back on us, so we don't need to preallocate the struct. Signed-off-by: Mike Christie --- drivers/vhost/scsi.c | 36 1 file changed, 4 insertions(+), 32 deletions(-) diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c index 5875241e1654..32d0be968103 100644 --- a/drivers/vhost/scsi.c +++ b/drivers/vhost/scsi.c @@ -125,7 +125,6 @@ struct vhost_scsi_tpg { struct se_portal_group se_tpg; /* Pointer back to vhost_scsi, protected by tv_tpg_mutex */ struct vhost_scsi *vhost_scsi; - struct list_head tmf_queue; }; struct vhost_scsi_tport { @@ -206,10 +205,8 @@ struct vhost_scsi { struct vhost_scsi_tmf { struct vhost_work vwork; - struct vhost_scsi_tpg *tpg; struct vhost_scsi *vhost; struct vhost_scsi_virtqueue *svq; - struct list_head queue_entry; struct se_cmd se_cmd; u8 scsi_resp; @@ -352,12 +349,9 @@ static void vhost_scsi_release_cmd_res(struct se_cmd *se_cmd) static void vhost_scsi_release_tmf_res(struct vhost_scsi_tmf *tmf) { - struct vhost_scsi_tpg *tpg = tmf->tpg; struct vhost_scsi_inflight *inflight = tmf->inflight; - mutex_lock(&tpg->tv_tpg_mutex); - list_add_tail(&tpg->tmf_queue, &tmf->queue_entry); - mutex_unlock(&tpg->tv_tpg_mutex); + kfree(tmf); vhost_scsi_put_inflight(inflight); } @@ -1194,19 +1188,11 @@ vhost_scsi_handle_tmf(struct vhost_scsi *vs, struct vhost_scsi_tpg *tpg, goto send_reject; } - mutex_lock(&tpg->tv_tpg_mutex); - if (list_empty(&tpg->tmf_queue)) { - pr_err("Missing reserve TMF. Could not handle LUN RESET.\n"); - mutex_unlock(&tpg->tv_tpg_mutex); + tmf = kzalloc(sizeof(*tmf), GFP_KERNEL); + if (!tmf) goto send_reject; - } - - tmf = list_first_entry(&tpg->tmf_queue, struct vhost_scsi_tmf, - queue_entry); - list_del_init(&tmf->queue_entry); - mutex_unlock(&tpg->tv_tpg_mutex); - tmf->tpg = tpg; + vhost_work_init(&tmf->vwork, vhost_scsi_tmf_resp_work); tmf->vhost = vs; tmf->svq = svq; tmf->resp_iov = vq->iov[vc->out]; @@ -2035,19 +2021,11 @@ static int vhost_scsi_port_link(struct se_portal_group *se_tpg, { struct vhost_scsi_tpg *tpg = container_of(se_tpg, struct vhost_scsi_tpg, se_tpg); - struct vhost_scsi_tmf *tmf; - - tmf = kzalloc(sizeof(*tmf), GFP_KERNEL); - if (!tmf) - return -ENOMEM; - INIT_LIST_HEAD(&tmf->queue_entry); - vhost_work_init(&tmf->vwork, vhost_scsi_tmf_resp_work); mutex_lock(&vhost_scsi_mutex); mutex_lock(&tpg->tv_tpg_mutex); tpg->tv_tpg_port_count++; - list_add_tail(&tmf->queue_entry, &tpg->tmf_queue); mutex_unlock(&tpg->tv_tpg_mutex); vhost_scsi_hotplug(tpg, lun); @@ -2062,16 +2040,11 @@ static void vhost_scsi_port_unlink(struct se_portal_group *se_tpg, { struct vhost_scsi_tpg *tpg = container_of(se_tpg, struct vhost_scsi_tpg, se_tpg); - struct vhost_scsi_tmf *tmf; mutex_lock(&vhost_scsi_mutex); mutex_lock(&tpg->tv_tpg_mutex); tpg->tv_tpg_port_count--; - tmf = list_first_entry(&tpg->tmf_queue, struct vhost_scsi_tmf, - queue_entry); - list_del(&tmf->queue_entry); - kfree(tmf); mutex_unlock(&tpg->tv_tpg_mutex); vhost_scsi_hotunplug(tpg, lun); @@ -2332,7 +2305,6 @@ vhost_scsi_make_tpg(struct se_wwn *wwn, const char *name) } mutex_init(&tpg->tv_tpg_mutex); INIT_LIST_HEAD(&tpg->tv_tpg_list); - INIT_LIST_HEAD(&tpg->tmf_queue); tpg->tport = tport; tpg->tport_tpgt = tpgt; -- 2.25.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 0/7] vhost-scsi: Fix crashes and management op hangs
The following patches were made over Linus tree. The patches fix 3 issues: 1. If a user performs LIO LUN unmapping before the endpoint has been cleared then we can end up trying to free a bogus tmf struct if the TMF is still exucuting when we do the unmap. 2. If vhost_scsi_setup_vq_cmds fails we can leave the tpg->vhost_scsi pointer set and we can end up trying to access a freed struct. 3. Management operations like LUN mapping/unmapping and device addition hang for 30 seconds or up to N minutes depending on the device. The problem is that we use a global mutex to protect the list of tpgs and for accessing the tpg, and to make sure they are flushed. We then hold that mutex during a lot of management operations. So if you are just trying to add another device, it will have to wait on another device if we are in the middle of clearing it's endpoint and it's waiting on hung IO. This patchset fixes up the ordering of how we flush IO and release refcounts and how often the global mutex is used so we don't need to always hold it v2: 1. Added fix for possible use after free and merge with a locking cleanup patch. 2. Added fix for LIO LUN unmap during TMF execution bug. 3. Fixed bug where we needed to hold the tpg mutex instead of the vhost_scsi_mutex when calling vhost_scsi_do_plug. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 1/7] vhost-scsi: Fix vhost_scsi struct use after free
If vhost_scsi_setup_vq_cmds fails we leave the tpg->vhost_scsi pointer set. If the device is freed and then the user unmaps the LUN, the call to vhost_scsi_port_unlink -> vhost_scsi_hotunplug will see the that tpg->vhost_scsi is still set and try to use it. This has us clear the vhost_scsi pointer in the failure path. It also has us take tv_tpg_mutex in this failure path, because tv_tpg_vhost_count is accessed under this mutex in vhost_scsi_drop_nexus and in the future we will want to serialize access to tpg->vhost_scsi with that mutex instead of the vhost_scsi_mutex. Signed-off-by: Mike Christie --- drivers/vhost/scsi.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c index b244e7c0f514..5875241e1654 100644 --- a/drivers/vhost/scsi.c +++ b/drivers/vhost/scsi.c @@ -1658,7 +1658,10 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs, for (i = 0; i < VHOST_SCSI_MAX_TARGET; i++) { tpg = vs_tpg[i]; if (tpg) { + mutex_lock(&tpg->tv_tpg_mutex); + tpg->vhost_scsi = NULL; tpg->tv_tpg_vhost_count--; + mutex_unlock(&tpg->tv_tpg_mutex); target_undepend_item(&tpg->se_tpg.tpg_group.cg_item); } } -- 2.25.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 3/7] vhost-scsi: Delay releasing our refcount on the tpg
We currently hold the vhost_scsi_mutex the entire time we are running vhost_scsi_clear_endpoint. One of the reasons for this is that it prevents userspace from being able to free the se_tpg from under us after we have called target_undepend_item. However, it forces management operations for for other devices to have to wait on a flakey device's: vhost_scsi_clear_endpoint -> vhost_scsi_flush() call which can which can take a long time. This moves the target_undepend_item call and the tpg unsetup code to after we have stopped new IO from starting up and after we have waited on running IO. We can then release our refcount on the tpg and session knowing our device is no longer accessing them. We can then drop the vhost_scsi_mutex use during thee flush call in later patches in this set, when we have removed other reasons for holding it. Signed-off-by: Mike Christie --- drivers/vhost/scsi.c | 61 +++- 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c index 32d0be968103..502d6803df0b 100644 --- a/drivers/vhost/scsi.c +++ b/drivers/vhost/scsi.c @@ -1691,11 +1691,10 @@ vhost_scsi_clear_endpoint(struct vhost_scsi *vs, if (!tpg) continue; - mutex_lock(&tpg->tv_tpg_mutex); tv_tport = tpg->tport; if (!tv_tport) { ret = -ENODEV; - goto err_tpg; + goto err_dev; } if (strcmp(tv_tport->tport_name, t->vhost_wwpn)) { @@ -1704,35 +1703,51 @@ vhost_scsi_clear_endpoint(struct vhost_scsi *vs, tv_tport->tport_name, tpg->tport_tpgt, t->vhost_wwpn, t->vhost_tpgt); ret = -EINVAL; - goto err_tpg; + goto err_dev; } + match = true; + } + if (!match) + goto free_vs_tpg; + + /* Prevent new cmds from starting and accessing the tpgs/sessions */ + for (i = 0; i < vs->dev.nvqs; i++) { + vq = &vs->vqs[i].vq; + mutex_lock(&vq->mutex); + vhost_vq_set_backend(vq, NULL); + mutex_unlock(&vq->mutex); + } + /* Make sure cmds are not running before tearing them down. */ + vhost_scsi_flush(vs); + + for (i = 0; i < vs->dev.nvqs; i++) { + vq = &vs->vqs[i].vq; + vhost_scsi_destroy_vq_cmds(vq); + } + + /* +* We can now release our hold on the tpg and sessions and userspace +* can free them after this point. +*/ + for (i = 0; i < VHOST_SCSI_MAX_TARGET; i++) { + target = i; + tpg = vs->vs_tpg[target]; + if (!tpg) + continue; + + mutex_lock(&tpg->tv_tpg_mutex); + tpg->tv_tpg_vhost_count--; tpg->vhost_scsi = NULL; vs->vs_tpg[target] = NULL; - match = true; + mutex_unlock(&tpg->tv_tpg_mutex); - /* -* Release se_tpg->tpg_group.cg_item configfs dependency now -* to allow vhost-scsi WWPN se_tpg->tpg_group shutdown to occur. -*/ + se_tpg = &tpg->se_tpg; target_undepend_item(&se_tpg->tpg_group.cg_item); } - if (match) { - for (i = 0; i < vs->dev.nvqs; i++) { - vq = &vs->vqs[i].vq; - mutex_lock(&vq->mutex); - vhost_vq_set_backend(vq, NULL); - mutex_unlock(&vq->mutex); - } - /* Make sure cmds are not running before tearing them down. */ - vhost_scsi_flush(vs); - for (i = 0; i < vs->dev.nvqs; i++) { - vq = &vs->vqs[i].vq; - vhost_scsi_destroy_vq_cmds(vq); - } - } +free_vs_tpg: /* * Act as synchronize_rcu to make sure access to * old vs->vs_tpg is finished. @@ -1745,8 +1760,6 @@ vhost_scsi_clear_endpoint(struct vhost_scsi *vs, mutex_unlock(&vhost_scsi_mutex); return 0; -err_tpg: - mutex_unlock(&tpg->tv_tpg_mutex); err_dev: mutex_unlock(&vs->dev.mutex); mutex_unlock(&vhost_scsi_mutex); -- 2.25.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support
Hi Viktor, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on mst-vhost/linux-next] [also build test WARNING on linus/master v6.3-rc3 next-20230320] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Viktor-Prutyanov/virtio-add-VIRTIO_F_NOTIFICATION_DATA-feature-support/20230320-195725 base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next patch link: https://lore.kernel.org/r/20230320115451.1232171-1-viktor%40daynix.com patch subject: [PATCH] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support config: sparc64-randconfig-s031-20230319 (https://download.01.org/0day-ci/archive/20230321/202303210740.w7riuizb-...@intel.com/config) compiler: sparc64-linux-gcc (GCC) 12.1.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.4-39-gce1a6720-dirty # https://github.com/intel-lab-lkp/linux/commit/b6212a12ca1691dc346e5de046ec46bd3ce11247 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Viktor-Prutyanov/virtio-add-VIRTIO_F_NOTIFICATION_DATA-feature-support/20230320-195725 git checkout b6212a12ca1691dc346e5de046ec46bd3ce11247 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc64 olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc64 SHELL=/bin/bash drivers/virtio/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot | Link: https://lore.kernel.org/oe-kbuild-all/202303210740.w7riuizb-...@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/virtio/virtio_pci_common.c:54:19: sparse: sparse: incorrect type in >> argument 1 (different base types) @@ expected unsigned int [usertype] l >> @@ got restricted __le32 [usertype] data @@ drivers/virtio/virtio_pci_common.c:54:19: sparse: expected unsigned int [usertype] l drivers/virtio/virtio_pci_common.c:54:19: sparse: got restricted __le32 [usertype] data vim +54 drivers/virtio/virtio_pci_common.c 49 50 bool vp_notify_with_data(struct virtqueue *vq) 51 { 52 __le32 data = vring_fill_notification_data(vq); 53 > 54 iowrite32(data, (void __iomem *)vq->priv); 55 56 return true; 57 } 58 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support
Hi Viktor, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on mst-vhost/linux-next] [also build test WARNING on linus/master v6.3-rc3 next-20230320] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Viktor-Prutyanov/virtio-add-VIRTIO_F_NOTIFICATION_DATA-feature-support/20230320-195725 base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next patch link: https://lore.kernel.org/r/20230320115451.1232171-1-viktor%40daynix.com patch subject: [PATCH] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support config: alpha-randconfig-s043-20230319 (https://download.01.org/0day-ci/archive/20230321/202303210759.krnnnzb4-...@intel.com/config) compiler: alpha-linux-gcc (GCC) 12.1.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.4-39-gce1a6720-dirty # https://github.com/intel-lab-lkp/linux/commit/b6212a12ca1691dc346e5de046ec46bd3ce11247 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Viktor-Prutyanov/virtio-add-VIRTIO_F_NOTIFICATION_DATA-feature-support/20230320-195725 git checkout b6212a12ca1691dc346e5de046ec46bd3ce11247 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=alpha olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=alpha SHELL=/bin/bash drivers/virtio/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot | Link: https://lore.kernel.org/oe-kbuild-all/202303210759.krnnnzb4-...@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/virtio/virtio_mmio.c:293:16: sparse: sparse: incorrect type in >> argument 1 (different base types) @@ expected unsigned int [usertype] b >> @@ got restricted __le32 [usertype] data @@ drivers/virtio/virtio_mmio.c:293:16: sparse: expected unsigned int [usertype] b drivers/virtio/virtio_mmio.c:293:16: sparse: got restricted __le32 [usertype] data vim +293 drivers/virtio/virtio_mmio.c 287 288 static bool vm_notify_with_data(struct virtqueue *vq) 289 { 290 struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev); 291 __le32 data = vring_fill_notification_data(vq); 292 > 293 writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY); 294 295 return true; 296 } 297 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support
Hi Viktor, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on mst-vhost/linux-next] [also build test WARNING on linus/master v6.3-rc3 next-20230320] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Viktor-Prutyanov/virtio-add-VIRTIO_F_NOTIFICATION_DATA-feature-support/20230320-195725 base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next patch link: https://lore.kernel.org/r/20230320115451.1232171-1-viktor%40daynix.com patch subject: [PATCH] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support config: i386-randconfig-s002 (https://download.01.org/0day-ci/archive/20230321/202303210515.zhhe1nmc-...@intel.com/config) compiler: gcc-11 (Debian 11.3.0-8) 11.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.4-39-gce1a6720-dirty # https://github.com/intel-lab-lkp/linux/commit/b6212a12ca1691dc346e5de046ec46bd3ce11247 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Viktor-Prutyanov/virtio-add-VIRTIO_F_NOTIFICATION_DATA-feature-support/20230320-195725 git checkout b6212a12ca1691dc346e5de046ec46bd3ce11247 # save the config file mkdir build_dir && cp config build_dir/.config make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 olddefconfig make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/virtio/ net/bluetooth/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot | Link: https://lore.kernel.org/oe-kbuild-all/202303210515.zhhe1nmc-...@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/virtio/virtio_pci_common.c:54:19: sparse: sparse: incorrect type in >> argument 1 (different base types) @@ expected unsigned int [usertype] @@ >> got restricted __le32 [usertype] data @@ drivers/virtio/virtio_pci_common.c:54:19: sparse: expected unsigned int [usertype] drivers/virtio/virtio_pci_common.c:54:19: sparse: got restricted __le32 [usertype] data vim +54 drivers/virtio/virtio_pci_common.c 49 50 bool vp_notify_with_data(struct virtqueue *vq) 51 { 52 __le32 data = vring_fill_notification_data(vq); 53 > 54 iowrite32(data, (void __iomem *)vq->priv); 55 56 return true; 57 } 58 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support
Hi Viktor, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on mst-vhost/linux-next] [also build test WARNING on linus/master v6.3-rc3 next-20230320] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Viktor-Prutyanov/virtio-add-VIRTIO_F_NOTIFICATION_DATA-feature-support/20230320-195725 base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next patch link: https://lore.kernel.org/r/20230320115451.1232171-1-viktor%40daynix.com patch subject: [PATCH] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support config: loongarch-randconfig-s032-20230319 (https://download.01.org/0day-ci/archive/20230321/202303210441.xoa05pbs-...@intel.com/config) compiler: loongarch64-linux-gcc (GCC) 12.1.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.4-39-gce1a6720-dirty # https://github.com/intel-lab-lkp/linux/commit/b6212a12ca1691dc346e5de046ec46bd3ce11247 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Viktor-Prutyanov/virtio-add-VIRTIO_F_NOTIFICATION_DATA-feature-support/20230320-195725 git checkout b6212a12ca1691dc346e5de046ec46bd3ce11247 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=loongarch olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=loongarch SHELL=/bin/bash drivers/virtio/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot | Link: https://lore.kernel.org/oe-kbuild-all/202303210441.xoa05pbs-...@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/virtio/virtio_pci_common.c:54:19: sparse: sparse: incorrect type in >> argument 1 (different base types) @@ expected unsigned int [usertype] >> value @@ got restricted __le32 [usertype] data @@ drivers/virtio/virtio_pci_common.c:54:19: sparse: expected unsigned int [usertype] value drivers/virtio/virtio_pci_common.c:54:19: sparse: got restricted __le32 [usertype] data vim +54 drivers/virtio/virtio_pci_common.c 49 50 bool vp_notify_with_data(struct virtqueue *vq) 51 { 52 __le32 data = vring_fill_notification_data(vq); 53 > 54 iowrite32(data, (void __iomem *)vq->priv); 55 56 return true; 57 } 58 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support
Hi Viktor, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on mst-vhost/linux-next] [also build test WARNING on linus/master v6.3-rc3 next-20230320] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Viktor-Prutyanov/virtio-add-VIRTIO_F_NOTIFICATION_DATA-feature-support/20230320-195725 base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next patch link: https://lore.kernel.org/r/20230320115451.1232171-1-viktor%40daynix.com patch subject: [PATCH] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support config: sparc64-randconfig-s052-20230319 (https://download.01.org/0day-ci/archive/20230321/202303210405.8gkuvbfx-...@intel.com/config) compiler: sparc64-linux-gcc (GCC) 12.1.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.4-39-gce1a6720-dirty # https://github.com/intel-lab-lkp/linux/commit/b6212a12ca1691dc346e5de046ec46bd3ce11247 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Viktor-Prutyanov/virtio-add-VIRTIO_F_NOTIFICATION_DATA-feature-support/20230320-195725 git checkout b6212a12ca1691dc346e5de046ec46bd3ce11247 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc64 olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc64 SHELL=/bin/bash drivers/virtio/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot | Link: https://lore.kernel.org/oe-kbuild-all/202303210405.8gkuvbfx-...@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/virtio/virtio_mmio.c:293:16: sparse: sparse: incorrect type in >> argument 1 (different base types) @@ expected unsigned int [usertype] l >> @@ got restricted __le32 [usertype] data @@ drivers/virtio/virtio_mmio.c:293:16: sparse: expected unsigned int [usertype] l drivers/virtio/virtio_mmio.c:293:16: sparse: got restricted __le32 [usertype] data vim +293 drivers/virtio/virtio_mmio.c 287 288 static bool vm_notify_with_data(struct virtqueue *vq) 289 { 290 struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev); 291 __le32 data = vring_fill_notification_data(vq); 292 > 293 writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY); 294 295 return true; 296 } 297 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support
Hi Viktor, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on mst-vhost/linux-next] [also build test WARNING on linus/master v6.3-rc3 next-20230320] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Viktor-Prutyanov/virtio-add-VIRTIO_F_NOTIFICATION_DATA-feature-support/20230320-195725 base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next patch link: https://lore.kernel.org/r/20230320115451.1232171-1-viktor%40daynix.com patch subject: [PATCH] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support config: i386-randconfig-s003 (https://download.01.org/0day-ci/archive/20230321/202303210403.lsrg8goq-...@intel.com/config) compiler: gcc-11 (Debian 11.3.0-8) 11.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.4-39-gce1a6720-dirty # https://github.com/intel-lab-lkp/linux/commit/b6212a12ca1691dc346e5de046ec46bd3ce11247 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Viktor-Prutyanov/virtio-add-VIRTIO_F_NOTIFICATION_DATA-feature-support/20230320-195725 git checkout b6212a12ca1691dc346e5de046ec46bd3ce11247 # save the config file mkdir build_dir && cp config build_dir/.config make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 olddefconfig make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/virtio/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot | Link: https://lore.kernel.org/oe-kbuild-all/202303210403.lsrg8goq-...@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/virtio/virtio_mmio.c:293:16: sparse: sparse: incorrect type in >> argument 1 (different base types) @@ expected unsigned int val @@ >> got restricted __le32 [usertype] data @@ drivers/virtio/virtio_mmio.c:293:16: sparse: expected unsigned int val drivers/virtio/virtio_mmio.c:293:16: sparse: got restricted __le32 [usertype] data vim +293 drivers/virtio/virtio_mmio.c 287 288 static bool vm_notify_with_data(struct virtqueue *vq) 289 { 290 struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev); 291 __le32 data = vring_fill_notification_data(vq); 292 > 293 writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY); 294 295 return true; 296 } 297 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support
Hi Viktor, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on mst-vhost/linux-next] [also build test WARNING on linus/master v6.3-rc3 next-20230320] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Viktor-Prutyanov/virtio-add-VIRTIO_F_NOTIFICATION_DATA-feature-support/20230320-195725 base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next patch link: https://lore.kernel.org/r/20230320115451.1232171-1-viktor%40daynix.com patch subject: [PATCH] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support config: nios2-randconfig-s051-20230319 (https://download.01.org/0day-ci/archive/20230321/202303210449.m2erqjxb-...@intel.com/config) compiler: nios2-linux-gcc (GCC) 12.1.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.4-39-gce1a6720-dirty # https://github.com/intel-lab-lkp/linux/commit/b6212a12ca1691dc346e5de046ec46bd3ce11247 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Viktor-Prutyanov/virtio-add-VIRTIO_F_NOTIFICATION_DATA-feature-support/20230320-195725 git checkout b6212a12ca1691dc346e5de046ec46bd3ce11247 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=nios2 olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=nios2 SHELL=/bin/bash drivers/virtio/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot | Link: https://lore.kernel.org/oe-kbuild-all/202303210449.m2erqjxb-...@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/virtio/virtio_mmio.c:293:16: sparse: sparse: incorrect type in >> argument 1 (different base types) @@ expected unsigned int [usertype] >> value @@ got restricted __le32 [usertype] data @@ drivers/virtio/virtio_mmio.c:293:16: sparse: expected unsigned int [usertype] value drivers/virtio/virtio_mmio.c:293:16: sparse: got restricted __le32 [usertype] data vim +293 drivers/virtio/virtio_mmio.c 287 288 static bool vm_notify_with_data(struct virtqueue *vq) 289 { 290 struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev); 291 __le32 data = vring_fill_notification_data(vq); 292 > 293 writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY); 294 295 return true; 296 } 297 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 2/2] vdpa/mlx5: Make VIRTIO_NET_F_MRG_RXBUF off by default
On Mon, Mar 20, 2023 at 01:49:30PM +0200, Eli Cohen wrote: > One can still enable it when creating the vdpa device using vdpa tool by > providing features that include it. > > For example: > $ vdpa dev add name vdpa0 mgmtdev pci/:86:00.2 device_features 0x300cb982b > > Please be aware that this feature was not supported before the previous > patch in this list was introduced so we don't change user experience. so I would say the patches have to be reordered to avoid a regression? > Current firmware versions show degradation in packet rate when using > MRG_RXBUF. Users who favor memory saving over packet rate could enable > this feature but we want to keep it off by default. > > Signed-off-by: Eli Cohen OK and when future firmware will (maybe) fix this up how will you know it's ok to enable by default? Some version check I guess? It would be better if firmware specified flags to enable by default ... > --- > drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c > b/drivers/vdpa/mlx5/net/mlx5_vnet.c > index 5285dd76c793..24397a71d6f3 100644 > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > @@ -3146,6 +3146,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev > *v_mdev, const char *name, > return -EINVAL; > } > device_features &= add_config->device_features; > + } else { > + device_features &= ~BIT_ULL(VIRTIO_NET_F_MRG_RXBUF); > } > if (!(device_features & BIT_ULL(VIRTIO_F_VERSION_1) && > device_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM))) { > -- > 2.38.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support
On Mon, Mar 20, 2023 at 02:54:51PM +0300, Viktor Prutyanov wrote: > According to VirtIO spec v1.2, VIRTIO_F_NOTIFICATION_DATA feature > indicates that the driver passes extra data along with the queue > notifications. > > In a split queue case, the extra data is 16-bit available index. In a > packed queue case, the extra data is 1-bit wrap counter and 15-bit > available index. > > Add support for this feature for both MMIO and PCI. > > Signed-off-by: Viktor Prutyanov > --- > drivers/virtio/virtio_mmio.c | 15 ++- > drivers/virtio/virtio_pci_common.c | 10 ++ > drivers/virtio/virtio_pci_common.h | 4 > drivers/virtio/virtio_pci_legacy.c | 2 +- > drivers/virtio/virtio_pci_modern.c | 2 +- > drivers/virtio/virtio_ring.c | 17 + > include/linux/virtio_ring.h| 2 ++ > include/uapi/linux/virtio_config.h | 6 ++ > 8 files changed, 55 insertions(+), 3 deletions(-) > > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c > index 3ff746e3f24a..05da5ad7fc93 100644 > --- a/drivers/virtio/virtio_mmio.c > +++ b/drivers/virtio/virtio_mmio.c > @@ -285,6 +285,19 @@ static bool vm_notify(struct virtqueue *vq) > return true; > } > > +static bool vm_notify_with_data(struct virtqueue *vq) > +{ > + struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev); > + __le32 data = vring_fill_notification_data(vq); > + > + writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY); > + > + return true; > +} > + > +#define VM_NOTIFY(vdev) (__virtio_test_bit((vdev), > VIRTIO_F_NOTIFICATION_DATA) \ > + ? vm_notify_with_data : vm_notify) > + > /* Notify all virtqueues on an interrupt. */ > static irqreturn_t vm_interrupt(int irq, void *opaque) > { > @@ -397,7 +410,7 @@ static struct virtqueue *vm_setup_vq(struct virtio_device > *vdev, unsigned int in > > /* Create the vring */ > vq = vring_create_virtqueue(index, num, VIRTIO_MMIO_VRING_ALIGN, vdev, > - true, true, ctx, vm_notify, callback, name); > + true, true, ctx, VM_NOTIFY(vdev), callback, name); > if (!vq) { > err = -ENOMEM; > goto error_new_virtqueue; > diff --git a/drivers/virtio/virtio_pci_common.c > b/drivers/virtio/virtio_pci_common.c > index a6c86f916dbd..bf7daad9ce65 100644 > --- a/drivers/virtio/virtio_pci_common.c > +++ b/drivers/virtio/virtio_pci_common.c > @@ -43,6 +43,16 @@ bool vp_notify(struct virtqueue *vq) > /* we write the queue's selector into the notification register to >* signal the other end */ > iowrite16(vq->index, (void __iomem *)vq->priv); > + > + return true; > +} > + > +bool vp_notify_with_data(struct virtqueue *vq) > +{ > + __le32 data = vring_fill_notification_data(vq); > + > + iowrite32(data, (void __iomem *)vq->priv); > + > return true; > } > > diff --git a/drivers/virtio/virtio_pci_common.h > b/drivers/virtio/virtio_pci_common.h > index 23112d84218f..9a7212dcbb32 100644 > --- a/drivers/virtio/virtio_pci_common.h > +++ b/drivers/virtio/virtio_pci_common.h > @@ -105,6 +105,7 @@ static struct virtio_pci_device *to_vp_device(struct > virtio_device *vdev) > void vp_synchronize_vectors(struct virtio_device *vdev); > /* the notify function used when creating a virt queue */ > bool vp_notify(struct virtqueue *vq); > +bool vp_notify_with_data(struct virtqueue *vq); > /* the config->del_vqs() implementation */ > void vp_del_vqs(struct virtio_device *vdev); > /* the config->find_vqs() implementation */ > @@ -114,6 +115,9 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned int > nvqs, > struct irq_affinity *desc); > const char *vp_bus_name(struct virtio_device *vdev); > > +#define VP_NOTIFY(vdev) (__virtio_test_bit((vdev), > VIRTIO_F_NOTIFICATION_DATA) \ > + ? vp_notify : vp_notify_with_data) > + > /* Setup the affinity for a virtqueue: > * - force the affinity for per vq vector > * - OR over all affinities for shared MSI > diff --git a/drivers/virtio/virtio_pci_legacy.c > b/drivers/virtio/virtio_pci_legacy.c > index 2257f1b3d8ae..b98e994cae48 100644 > --- a/drivers/virtio/virtio_pci_legacy.c > +++ b/drivers/virtio/virtio_pci_legacy.c > @@ -131,7 +131,7 @@ static struct virtqueue *setup_vq(struct > virtio_pci_device *vp_dev, > vq = vring_create_virtqueue(index, num, > VIRTIO_PCI_VRING_ALIGN, &vp_dev->vdev, > true, false, ctx, > - vp_notify, callback, name); > + VP_NOTIFY(&vp_dev->vdev), callback, name); > if (!vq) > return ERR_PTR(-ENOMEM); > > diff --git a/drivers/virtio/virtio_pci_modern.c > b/drivers/virtio/virtio_pci_modern.c > index 9e496e288cfa..7fcd8af5af7e 100644 > --- a/drivers/virtio/virtio_pci_modern.c > +++ b/drivers/virtio/virtio_pci_modern.c > @@ -321,7 +321,7 @@ static struct virt
Re: [RFC PATCH v1 3/3] test/vsock: skbuff merging test
On Sun, Mar 19, 2023 at 09:53:54PM +0300, Arseniy Krasnov wrote: This adds test which checks case when data of newly received skbuff is appended to the last skbuff in the socket's queue. This test is actual only for virtio transport. Signed-off-by: Arseniy Krasnov --- tools/testing/vsock/vsock_test.c | 81 1 file changed, 81 insertions(+) diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c index 3de10dbb50f5..00216c52d8b6 100644 --- a/tools/testing/vsock/vsock_test.c +++ b/tools/testing/vsock/vsock_test.c @@ -968,6 +968,82 @@ static void test_seqpacket_inv_buf_server(const struct test_opts *opts) test_inv_buf_server(opts, false); } +static void test_stream_virtio_skb_merge_client(const struct test_opts *opts) +{ + ssize_t res; + int fd; + + fd = vsock_stream_connect(opts->peer_cid, 1234); + if (fd < 0) { + perror("connect"); + exit(EXIT_FAILURE); + } + Please use a macro for "HELLO" or a variabile, e.g. char *buf; ... buf = "HELLO"; res = send(fd, buf, strlen(buf), 0); ... + res = send(fd, "HELLO", strlen("HELLO"), 0); + if (res != strlen("HELLO")) { + fprintf(stderr, "unexpected send(2) result %zi\n", res); + exit(EXIT_FAILURE); + } + + control_writeln("SEND0"); + /* Peer reads part of first packet. */ + control_expectln("REPLY0"); + + /* Send second skbuff, it will be merged. */ + res = send(fd, "WORLD", strlen("WORLD"), 0); Ditto. + if (res != strlen("WORLD")) { + fprintf(stderr, "unexpected send(2) result %zi\n", res); + exit(EXIT_FAILURE); + } + + control_writeln("SEND1"); + /* Peer reads merged skbuff packet. */ + control_expectln("REPLY1"); + + close(fd); +} + +static void test_stream_virtio_skb_merge_server(const struct test_opts *opts) +{ + unsigned char buf[64]; + ssize_t res; + int fd; + + fd = vsock_stream_accept(VMADDR_CID_ANY, 1234, NULL); + if (fd < 0) { + perror("accept"); + exit(EXIT_FAILURE); + } + + control_expectln("SEND0"); + + /* Read skbuff partially. */ + res = recv(fd, buf, 2, 0); + if (res != 2) { + fprintf(stderr, "expected recv(2) failure, got %zi\n", res); We don't expect a failure, so please update the error message and make it easy to figure out which recv() is failing. For example by saying how many bytes you expected and how many you received. + exit(EXIT_FAILURE); + } + + control_writeln("REPLY0"); + control_expectln("SEND1"); + + + res = recv(fd, buf, sizeof(buf), 0); Perhaps a comment here to explain why we expect only 8 bytes. + if (res != 8) { + fprintf(stderr, "expected recv(2) failure, got %zi\n", res); Ditto. + exit(EXIT_FAILURE); + } + + res = recv(fd, buf, sizeof(buf), MSG_DONTWAIT); + if (res != -1) { + fprintf(stderr, "expected recv(2) success, got %zi\n", res); It's the other way around, isn't it? Here you expect it to fail instead it is not failing. + exit(EXIT_FAILURE); + } Moving the pointer correctly, I would also check that there is HELLOWORLD in the buffer. Thanks for adding tests in this suite! Stefano + + control_writeln("REPLY1"); + + close(fd); +} + static struct test_case test_cases[] = { { .name = "SOCK_STREAM connection reset", @@ -1038,6 +1114,11 @@ static struct test_case test_cases[] = { .run_client = test_seqpacket_inv_buf_client, .run_server = test_seqpacket_inv_buf_server, }, + { + .name = "SOCK_STREAM virtio skb merge", + .run_client = test_stream_virtio_skb_merge_client, + .run_server = test_stream_virtio_skb_merge_server, + }, {}, }; -- 2.25.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH v1 2/3] virtio/vsock: add WARN() for invalid state of socket
On Sun, Mar 19, 2023 at 09:52:19PM +0300, Arseniy Krasnov wrote: This prints WARN() and returns from stream dequeue callback when socket's queue is empty, but 'rx_bytes' still non-zero. Signed-off-by: Arseniy Krasnov --- net/vmw_vsock/virtio_transport_common.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index 3c75986e16c2..c35b03adad8d 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -388,6 +388,13 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, u32 free_space; spin_lock_bh(&vvs->rx_lock); + + if (skb_queue_empty(&vvs->rx_queue) && vvs->rx_bytes) { + WARN(1, "No skbuffs with non-zero 'rx_bytes'\n"); I would use WARN_ONCE, since we can't recover so we will flood the log. And you can put the condition in the first argument, I mean something like this: if (WARN_ONCE(skb_queue_empty(&vvs->rx_queue) && vvs->rx_bytes, "rx_queue is empty, but rx_bytes is non-zero\n")) { Thanks, Stefano + spin_unlock_bh(&vvs->rx_lock); + return err; + } + while (total < len && !skb_queue_empty(&vvs->rx_queue)) { skb = skb_peek(&vvs->rx_queue); -- 2.25.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 3/8] drm/fb-helper: Export drm_fb_helper_release_info()
Export the fb_info release code as drm_fb_helper_release_info(). Will help with cleaning up failed fbdev probing. Signed-off-by: Thomas Zimmermann Reviewed-by: Javier Martinez Canillas Acked-by: Zack Rusin --- drivers/gpu/drm/drm_fb_helper.c | 33 - include/drm/drm_fb_helper.h | 5 + 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index a39998047f8a..7e96ed9efdb5 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -538,6 +538,29 @@ struct fb_info *drm_fb_helper_alloc_info(struct drm_fb_helper *fb_helper) } EXPORT_SYMBOL(drm_fb_helper_alloc_info); +/** + * drm_fb_helper_release_info - release fb_info and its members + * @fb_helper: driver-allocated fbdev helper + * + * A helper to release fb_info and the member cmap. Drivers do not + * need to release the allocated fb_info structure themselves, this is + * automatically done when calling drm_fb_helper_fini(). + */ +void drm_fb_helper_release_info(struct drm_fb_helper *fb_helper) +{ + struct fb_info *info = fb_helper->info; + + if (!info) + return; + + fb_helper->info = NULL; + + if (info->cmap.len) + fb_dealloc_cmap(&info->cmap); + framebuffer_release(info); +} +EXPORT_SYMBOL(drm_fb_helper_release_info); + /** * drm_fb_helper_unregister_info - unregister fb_info framebuffer device * @fb_helper: driver-allocated fbdev helper, can be NULL @@ -561,8 +584,6 @@ EXPORT_SYMBOL(drm_fb_helper_unregister_info); */ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper) { - struct fb_info *info; - if (!fb_helper) return; @@ -574,13 +595,7 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper) cancel_work_sync(&fb_helper->resume_work); cancel_work_sync(&fb_helper->damage_work); - info = fb_helper->info; - if (info) { - if (info->cmap.len) - fb_dealloc_cmap(&info->cmap); - framebuffer_release(info); - } - fb_helper->info = NULL; + drm_fb_helper_release_info(fb_helper); mutex_lock(&kernel_fb_helper_lock); if (!list_empty(&fb_helper->kernel_fb_list)) { diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 013654de3fc5..c5822ec2fdd1 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -256,6 +256,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper); struct fb_info *drm_fb_helper_alloc_info(struct drm_fb_helper *fb_helper); +void drm_fb_helper_release_info(struct drm_fb_helper *fb_helper); void drm_fb_helper_unregister_info(struct drm_fb_helper *fb_helper); void drm_fb_helper_fill_info(struct fb_info *info, struct drm_fb_helper *fb_helper, @@ -365,6 +366,10 @@ drm_fb_helper_alloc_info(struct drm_fb_helper *fb_helper) return NULL; } +static inline void drm_fb_helper_release_info(struct drm_fb_helper *fb_helper) +{ +} + static inline void drm_fb_helper_unregister_info(struct drm_fb_helper *fb_helper) { } -- 2.40.0 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 6/8] drm/fbdev-generic: Clean up after failed probing
Clean up fbdev and client state if the probe function fails. It used to leak allocated resources. Also reorder the individual steps to simplify cleanup. v2: * move screen_size update into separate patches Signed-off-by: Thomas Zimmermann Reviewed-by: Javier Martinez Canillas Acked-by: Zack Rusin --- drivers/gpu/drm/drm_fbdev_generic.c | 40 - 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c index 73834a3cc6b0..e7eeba0c44b4 100644 --- a/drivers/gpu/drm/drm_fbdev_generic.c +++ b/drivers/gpu/drm/drm_fbdev_generic.c @@ -77,6 +77,7 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper, struct drm_client_buffer *buffer; struct fb_info *info; size_t screen_size; + void *screen_buffer; u32 format; int ret; @@ -92,36 +93,51 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper, fb_helper->buffer = buffer; fb_helper->fb = buffer->fb; + screen_size = buffer->gem->size; + screen_buffer = vzalloc(screen_size); + if (!screen_buffer) { + ret = -ENOMEM; + goto err_drm_client_framebuffer_delete; + } info = drm_fb_helper_alloc_info(fb_helper); - if (IS_ERR(info)) - return PTR_ERR(info); + if (IS_ERR(info)) { + ret = PTR_ERR(info); + goto err_vfree; + } + + drm_fb_helper_fill_info(info, fb_helper, sizes); info->fbops = &drm_fbdev_fb_ops; - info->screen_size = screen_size; - info->fix.smem_len = screen_size; info->flags = FBINFO_DEFAULT; - drm_fb_helper_fill_info(info, fb_helper, sizes); - - info->screen_buffer = vzalloc(screen_size); - if (!info->screen_buffer) - return -ENOMEM; + /* screen */ info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST; - + info->screen_buffer = screen_buffer; info->fix.smem_start = page_to_phys(vmalloc_to_page(info->screen_buffer)); + info->fix.smem_len = screen_size; - /* Set a default deferred I/O handler */ + /* deferred I/O */ fb_helper->fbdefio.delay = HZ / 20; fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io; info->fbdefio = &fb_helper->fbdefio; ret = fb_deferred_io_init(info); if (ret) - return ret; + goto err_drm_fb_helper_release_info; return 0; + +err_drm_fb_helper_release_info: + drm_fb_helper_release_info(fb_helper); +err_vfree: + vfree(screen_buffer); +err_drm_client_framebuffer_delete: + fb_helper->fb = NULL; + fb_helper->buffer = NULL; + drm_client_framebuffer_delete(buffer); + return ret; } static void drm_fbdev_damage_blit_real(struct drm_fb_helper *fb_helper, -- 2.40.0 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 1/8] drm/fbdev-generic: Always use shadow buffering
Remove all codepaths that implement fbdev output directly on GEM buffers. Always allocate a shadow buffer in system memory and set up deferred I/O for mmap. The fbdev code that operated directly on GEM buffers was used by drivers based on GEM DMA helpers. Those drivers have been migrated to use fbdev-dma, a dedicated fbdev emulation for DMA memory. All remaining users of fbdev-generic require shadow buffering. Memory management of the remaining callers uses TTM, GEM SHMEM helpers or a variant of GEM DMA helpers that is incompatible with fbdev-dma. Therefore remove the unused codepaths from fbdev-generic and simplify the code. Using a shadow buffer with deferred I/O is probably the best case for most remaining callers. Some of the TTM-based drivers might benefit from a dedicated fbdev emulation that operates directly on the driver's video memory. Signed-off-by: Thomas Zimmermann Reviewed-by: Javier Martinez Canillas Acked-by: Zack Rusin --- drivers/gpu/drm/drm_fbdev_generic.c | 184 +--- 1 file changed, 30 insertions(+), 154 deletions(-) diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c index 4d6325e91565..e48a8e82378d 100644 --- a/drivers/gpu/drm/drm_fbdev_generic.c +++ b/drivers/gpu/drm/drm_fbdev_generic.c @@ -11,16 +11,6 @@ #include -static bool drm_fbdev_use_shadow_fb(struct drm_fb_helper *fb_helper) -{ - struct drm_device *dev = fb_helper->dev; - struct drm_framebuffer *fb = fb_helper->fb; - - return dev->mode_config.prefer_shadow_fbdev || - dev->mode_config.prefer_shadow || - fb->funcs->dirty; -} - /* @user: 1=userspace, 0=fbcon */ static int drm_fbdev_fb_open(struct fb_info *info, int user) { @@ -46,115 +36,33 @@ static int drm_fbdev_fb_release(struct fb_info *info, int user) static void drm_fbdev_fb_destroy(struct fb_info *info) { struct drm_fb_helper *fb_helper = info->par; - void *shadow = NULL; + void *shadow = info->screen_buffer; if (!fb_helper->dev) return; - if (info->fbdefio) - fb_deferred_io_cleanup(info); - if (drm_fbdev_use_shadow_fb(fb_helper)) - shadow = info->screen_buffer; - + fb_deferred_io_cleanup(info); drm_fb_helper_fini(fb_helper); - - if (shadow) - vfree(shadow); - else if (fb_helper->buffer) - drm_client_buffer_vunmap(fb_helper->buffer); - + vfree(shadow); drm_client_framebuffer_delete(fb_helper->buffer); - drm_client_release(&fb_helper->client); + drm_client_release(&fb_helper->client); drm_fb_helper_unprepare(fb_helper); kfree(fb_helper); } -static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) -{ - struct drm_fb_helper *fb_helper = info->par; - - if (drm_fbdev_use_shadow_fb(fb_helper)) - return fb_deferred_io_mmap(info, vma); - else if (fb_helper->dev->driver->gem_prime_mmap) - return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma); - else - return -ENODEV; -} - -static bool drm_fbdev_use_iomem(struct fb_info *info) -{ - struct drm_fb_helper *fb_helper = info->par; - struct drm_client_buffer *buffer = fb_helper->buffer; - - return !drm_fbdev_use_shadow_fb(fb_helper) && buffer->map.is_iomem; -} - -static ssize_t drm_fbdev_fb_read(struct fb_info *info, char __user *buf, -size_t count, loff_t *ppos) -{ - ssize_t ret; - - if (drm_fbdev_use_iomem(info)) - ret = drm_fb_helper_cfb_read(info, buf, count, ppos); - else - ret = drm_fb_helper_sys_read(info, buf, count, ppos); - - return ret; -} - -static ssize_t drm_fbdev_fb_write(struct fb_info *info, const char __user *buf, - size_t count, loff_t *ppos) -{ - ssize_t ret; - - if (drm_fbdev_use_iomem(info)) - ret = drm_fb_helper_cfb_write(info, buf, count, ppos); - else - ret = drm_fb_helper_sys_write(info, buf, count, ppos); - - return ret; -} - -static void drm_fbdev_fb_fillrect(struct fb_info *info, - const struct fb_fillrect *rect) -{ - if (drm_fbdev_use_iomem(info)) - drm_fb_helper_cfb_fillrect(info, rect); - else - drm_fb_helper_sys_fillrect(info, rect); -} - -static void drm_fbdev_fb_copyarea(struct fb_info *info, - const struct fb_copyarea *area) -{ - if (drm_fbdev_use_iomem(info)) - drm_fb_helper_cfb_copyarea(info, area); - else - drm_fb_helper_sys_copyarea(info, area); -} - -static void drm_fbdev_fb_imageblit(struct fb_info *info, - const struct fb_image *image) -{ - if (drm_fbdev_use_iomem(info)) - drm_fb_helper_cfb_imageblit(inf
[PATCH v2 5/8] drm/fbdev-generic: Set screen size to size of GEM buffer
The size of the screen memory should be equivalent to the size of the screen's GEM buffer. Don't recalculate the value. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/drm_fbdev_generic.c | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c index e48a8e82378d..73834a3cc6b0 100644 --- a/drivers/gpu/drm/drm_fbdev_generic.c +++ b/drivers/gpu/drm/drm_fbdev_generic.c @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -74,8 +75,8 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper, struct drm_client_dev *client = &fb_helper->client; struct drm_device *dev = fb_helper->dev; struct drm_client_buffer *buffer; - struct drm_framebuffer *fb; struct fb_info *info; + size_t screen_size; u32 format; int ret; @@ -91,20 +92,20 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper, fb_helper->buffer = buffer; fb_helper->fb = buffer->fb; - fb = buffer->fb; + screen_size = buffer->gem->size; info = drm_fb_helper_alloc_info(fb_helper); if (IS_ERR(info)) return PTR_ERR(info); info->fbops = &drm_fbdev_fb_ops; - info->screen_size = sizes->surface_height * fb->pitches[0]; - info->fix.smem_len = info->screen_size; + info->screen_size = screen_size; + info->fix.smem_len = screen_size; info->flags = FBINFO_DEFAULT; drm_fb_helper_fill_info(info, fb_helper, sizes); - info->screen_buffer = vzalloc(info->screen_size); + info->screen_buffer = vzalloc(screen_size); if (!info->screen_buffer) return -ENOMEM; info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST; -- 2.40.0 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 2/8] drm/fbdev-generic: Remove unused prefer_shadow_fbdev flag
Remove the flag prefer_shadow_fbdev from struct drm_mode_config. Drivers set this flag to enable shadow buffering in the generic fbdev emulation. Such shadow buffering is now mandatory, so the flag is unused. Signed-off-by: Thomas Zimmermann Reviewed-by: Javier Martinez Canillas Reviewed-by: Zack Rusin --- drivers/gpu/drm/tiny/bochs.c| 1 - drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 1 - include/drm/drm_mode_config.h | 7 --- 3 files changed, 9 deletions(-) diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c index 024346054c70..d254679a136e 100644 --- a/drivers/gpu/drm/tiny/bochs.c +++ b/drivers/gpu/drm/tiny/bochs.c @@ -545,7 +545,6 @@ static int bochs_kms_init(struct bochs_device *bochs) bochs->dev->mode_config.preferred_depth = 24; bochs->dev->mode_config.prefer_shadow = 0; - bochs->dev->mode_config.prefer_shadow_fbdev = 1; bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true; bochs->dev->mode_config.funcs = &bochs_mode_funcs; diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 84d6380b9895..5162a7a12792 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -2112,7 +2112,6 @@ int vmw_kms_init(struct vmw_private *dev_priv) dev->mode_config.max_width = dev_priv->texture_max_width; dev->mode_config.max_height = dev_priv->texture_max_height; dev->mode_config.preferred_depth = dev_priv->assume_16bpp ? 16 : 32; - dev->mode_config.prefer_shadow_fbdev = !dev_priv->has_mob; drm_mode_create_suggested_offset_properties(dev); vmw_kms_create_hotplug_mode_update_property(dev_priv); diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index e5b053001d22..973119a9176b 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -890,13 +890,6 @@ struct drm_mode_config { /* dumb ioctl parameters */ uint32_t preferred_depth, prefer_shadow; - /** -* @prefer_shadow_fbdev: -* -* Hint to framebuffer emulation to prefer shadow-fb rendering. -*/ - bool prefer_shadow_fbdev; - /** * @quirk_addfb_prefer_xbgr_30bpp: * -- 2.40.0 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 8/8] drm/fbdev-generic: Rename symbols
Rename symbols to match the style of other fbdev-emulation source code. No functional changes. Signed-off-by: Thomas Zimmermann Reviewed-by: Javier Martinez Canillas Acked-by: Zack Rusin --- drivers/gpu/drm/drm_fbdev_generic.c | 66 ++--- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c index e7eeba0c44b4..8e5148bf40bb 100644 --- a/drivers/gpu/drm/drm_fbdev_generic.c +++ b/drivers/gpu/drm/drm_fbdev_generic.c @@ -13,7 +13,7 @@ #include /* @user: 1=userspace, 0=fbcon */ -static int drm_fbdev_fb_open(struct fb_info *info, int user) +static int drm_fbdev_generic_fb_open(struct fb_info *info, int user) { struct drm_fb_helper *fb_helper = info->par; @@ -24,7 +24,7 @@ static int drm_fbdev_fb_open(struct fb_info *info, int user) return 0; } -static int drm_fbdev_fb_release(struct fb_info *info, int user) +static int drm_fbdev_generic_fb_release(struct fb_info *info, int user) { struct drm_fb_helper *fb_helper = info->par; @@ -34,7 +34,7 @@ static int drm_fbdev_fb_release(struct fb_info *info, int user) return 0; } -static void drm_fbdev_fb_destroy(struct fb_info *info) +static void drm_fbdev_generic_fb_destroy(struct fb_info *info) { struct drm_fb_helper *fb_helper = info->par; void *shadow = info->screen_buffer; @@ -52,10 +52,10 @@ static void drm_fbdev_fb_destroy(struct fb_info *info) kfree(fb_helper); } -static const struct fb_ops drm_fbdev_fb_ops = { +static const struct fb_ops drm_fbdev_generic_fb_ops = { .owner = THIS_MODULE, - .fb_open= drm_fbdev_fb_open, - .fb_release = drm_fbdev_fb_release, + .fb_open= drm_fbdev_generic_fb_open, + .fb_release = drm_fbdev_generic_fb_release, .fb_read= drm_fb_helper_sys_read, .fb_write = drm_fb_helper_sys_write, DRM_FB_HELPER_DEFAULT_OPS, @@ -63,14 +63,14 @@ static const struct fb_ops drm_fbdev_fb_ops = { .fb_copyarea= drm_fb_helper_sys_copyarea, .fb_imageblit = drm_fb_helper_sys_imageblit, .fb_mmap= fb_deferred_io_mmap, - .fb_destroy = drm_fbdev_fb_destroy, + .fb_destroy = drm_fbdev_generic_fb_destroy, }; /* * This function uses the client API to create a framebuffer backed by a dumb buffer. */ -static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper, - struct drm_fb_helper_surface_size *sizes) +static int drm_fbdev_generic_helper_fb_probe(struct drm_fb_helper *fb_helper, +struct drm_fb_helper_surface_size *sizes) { struct drm_client_dev *client = &fb_helper->client; struct drm_device *dev = fb_helper->dev; @@ -109,7 +109,7 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper, drm_fb_helper_fill_info(info, fb_helper, sizes); - info->fbops = &drm_fbdev_fb_ops; + info->fbops = &drm_fbdev_generic_fb_ops; info->flags = FBINFO_DEFAULT; /* screen */ @@ -140,9 +140,9 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper, return ret; } -static void drm_fbdev_damage_blit_real(struct drm_fb_helper *fb_helper, - struct drm_clip_rect *clip, - struct iosys_map *dst) +static void drm_fbdev_generic_damage_blit_real(struct drm_fb_helper *fb_helper, + struct drm_clip_rect *clip, + struct iosys_map *dst) { struct drm_framebuffer *fb = fb_helper->fb; size_t offset = clip->y1 * fb->pitches[0]; @@ -179,8 +179,8 @@ static void drm_fbdev_damage_blit_real(struct drm_fb_helper *fb_helper, } } -static int drm_fbdev_damage_blit(struct drm_fb_helper *fb_helper, -struct drm_clip_rect *clip) +static int drm_fbdev_generic_damage_blit(struct drm_fb_helper *fb_helper, +struct drm_clip_rect *clip) { struct drm_client_buffer *buffer = fb_helper->buffer; struct iosys_map map, dst; @@ -204,7 +204,7 @@ static int drm_fbdev_damage_blit(struct drm_fb_helper *fb_helper, goto out; dst = map; - drm_fbdev_damage_blit_real(fb_helper, clip, &dst); + drm_fbdev_generic_damage_blit_real(fb_helper, clip, &dst); drm_client_buffer_vunmap(buffer); @@ -214,7 +214,8 @@ static int drm_fbdev_damage_blit(struct drm_fb_helper *fb_helper, return ret; } -static int drm_fbdev_fb_dirty(struct drm_fb_helper *helper, struct drm_clip_rect *clip) +static int drm_fbdev_generic_helper_fb_dirty(struct drm_fb_helper *helper, +struct drm_clip_rect *clip) { struct drm_device *dev = helper->dev; int ret; @@ -223,7 +
[PATCH v2 7/8] drm/fb-helper: Consolidate CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM
Consolidate all handling of CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM by making the module parameter optional in drm_fb_helper.c. Without the config option, modules can set smem_start in struct fb_info for internal usage, but not export if to userspace. The address can only be exported by enabling the option and setting the module parameter. Also update the comment. Signed-off-by: Thomas Zimmermann Reviewed-by: Javier Martinez Canillas Acked-by: Zack Rusin --- drivers/gpu/drm/drm_fb_helper.c | 22 -- drivers/gpu/drm/drm_fbdev_dma.c | 9 + include/drm/drm_fb_helper.h | 9 - 3 files changed, 9 insertions(+), 31 deletions(-) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index bb0b25209b3e..63ec95e86d0e 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -60,16 +60,17 @@ MODULE_PARM_DESC(drm_fbdev_overalloc, * In order to keep user-space compatibility, we want in certain use-cases * to keep leaking the fbdev physical address to the user-space program * handling the fbdev buffer. - * This is a bad habit essentially kept into closed source opengl driver - * that should really be moved into open-source upstream projects instead - * of using legacy physical addresses in user space to communicate with - * other out-of-tree kernel modules. + * + * This is a bad habit, essentially kept to support closed-source OpenGL + * drivers that should really be moved into open-source upstream projects + * instead of using legacy physical addresses in user space to communicate + * with other out-of-tree kernel modules. * * This module_param *should* be removed as soon as possible and be * considered as a broken and legacy behaviour from a modern fbdev device. */ -#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM) static bool drm_leak_fbdev_smem; +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM) module_param_unsafe(drm_leak_fbdev_smem, bool, 0600); MODULE_PARM_DESC(drm_leak_fbdev_smem, "Allow unsafe leaking fbdev physical smem address [default=false]"); @@ -1983,10 +1984,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper) return ret; } -#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM) - fb_helper->hint_leak_smem_start = drm_leak_fbdev_smem; -#endif - /* push down into drivers */ ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes); if (ret < 0) @@ -2185,11 +2182,8 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper) info = fb_helper->info; info->var.pixclock = 0; - /* Shamelessly allow physical address leaking to userspace */ -#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM) - if (!fb_helper->hint_leak_smem_start) -#endif - /* don't leak any physical addresses to userspace */ + + if (!drm_leak_fbdev_smem) info->flags |= FBINFO_HIDE_SMEM_START; /* Need to drop locks to avoid recursive deadlock in diff --git a/drivers/gpu/drm/drm_fbdev_dma.c b/drivers/gpu/drm/drm_fbdev_dma.c index cf553ac12a0f..728deffcc0d9 100644 --- a/drivers/gpu/drm/drm_fbdev_dma.c +++ b/drivers/gpu/drm/drm_fbdev_dma.c @@ -136,16 +136,9 @@ static int drm_fbdev_dma_helper_fb_probe(struct drm_fb_helper *fb_helper, info->flags |= FBINFO_READS_FAST; /* signal caching */ info->screen_size = sizes->surface_height * fb->pitches[0]; info->screen_buffer = map.vaddr; + info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer)); info->fix.smem_len = info->screen_size; -#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM) - /* -* Shamelessly leak the physical address to user-space. -*/ - if (fb_helper->hint_leak_smem_start && !info->fix.smem_start) - info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer)); -#endif - return 0; err_drm_client_buffer_vunmap: diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index c5822ec2fdd1..72032c354a30 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -195,15 +195,6 @@ struct drm_fb_helper { */ int preferred_bpp; - /** -* @hint_leak_smem_start: -* -* Hint to the fbdev emulation to store the framebuffer's physical -* address in struct &fb_info.fix.smem_start. If the hint is unset, -* the smem_start field should always be cleared to zero. -*/ - bool hint_leak_smem_start; - #ifdef CONFIG_FB_DEFERRED_IO /** * @fbdefio: -- 2.40.0 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 4/8] drm/fb-helper: Support smem_len in deferred I/O
The size of the framebuffer can either be stored in screen_info or smem_len. Take both into account in the deferred I/O code. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/drm_fb_helper.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 7e96ed9efdb5..bb0b25209b3e 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -672,7 +672,7 @@ static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off, void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagereflist) { struct drm_fb_helper *helper = info->par; - unsigned long start, end, min_off, max_off; + unsigned long start, end, min_off, max_off, total_size; struct fb_deferred_io_pageref *pageref; struct drm_rect damage_area; @@ -690,7 +690,11 @@ void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagerefli * of the screen and account for non-existing scanlines. Hence, * keep the covered memory area within the screen buffer. */ - max_off = min(max_off, info->screen_size); + if (info->screen_size) + total_size = info->screen_size; + else + total_size = info->fix.smem_len; + max_off = min(max_off, total_size); if (min_off < max_off) { drm_fb_helper_memory_range_to_clip(info, min_off, max_off - min_off, &damage_area); -- 2.40.0 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 0/8] drm/fbdev-generic: Mandatory shadow buffering
After adding fbdev-dma and converting drivers, all users of fbdev-generic require shadow buffering. Make it mandatory and remove all other codepaths. This change greatly simplifies the code for generic fbdev emulation. It will work with any driver that supports GEM's vmap and vunmap. The change further allows for a number of cleanups and fixes. The flag prefer_shadow_fbdev is unused and to be removed. Probing in fbdev-generic is now simple enough to roll back if it fails. Further simplify the code for exporting the framebuffer's physical address. Finally rename the symbols to follow other fbdev emulation. v2: * handle screen_size in separate patches (Javier) Thomas Zimmermann (8): drm/fbdev-generic: Always use shadow buffering drm/fbdev-generic: Remove unused prefer_shadow_fbdev flag drm/fb-helper: Export drm_fb_helper_release_info() drm/fb-helper: Support smem_len in deferred I/O drm/fbdev-generic: Set screen size to size of GEM buffer drm/fbdev-generic: Clean up after failed probing drm/fb-helper: Consolidate CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM drm/fbdev-generic: Rename symbols drivers/gpu/drm/drm_fb_helper.c | 63 --- drivers/gpu/drm/drm_fbdev_dma.c | 9 +- drivers/gpu/drm/drm_fbdev_generic.c | 279 +--- drivers/gpu/drm/tiny/bochs.c| 1 - drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 1 - include/drm/drm_fb_helper.h | 14 +- include/drm/drm_mode_config.h | 7 - 7 files changed, 130 insertions(+), 244 deletions(-) base-commit: 280906872bb8c2dff02666c7a8e46f085b24 -- 2.40.0 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH v1 1/3] virtio/vsock: fix header length on skb merging
On Sun, Mar 19, 2023 at 09:51:06PM +0300, Arseniy Krasnov wrote: This fixes header length calculation of skbuff during data appending to it. When such skbuff is processed in dequeue callbacks, e.g. 'skb_pull()' is called on it, 'skb->len' is dynamic value, so it is impossible to use it in header, because value from header must be permanent for valid credit calculation ('rx_bytes'/'fwd_cnt'). Fixes: 077706165717 ("virtio/vsock: don't use skbuff state to account credit") I don't understand how this commit introduced this problem, can you explain it better? Is it related more to the credit than to the size in the header itself? Anyway, the patch LGTM, but we should explain better the issue. Thanks, Stefano Signed-off-by: Arseniy Krasnov --- net/vmw_vsock/virtio_transport_common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index 6d15cd4d090a..3c75986e16c2 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -1091,7 +1091,7 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk, memcpy(skb_put(last_skb, skb->len), skb->data, skb->len); free_pkt = true; last_hdr->flags |= hdr->flags; - last_hdr->len = cpu_to_le32(last_skb->len); + le32_add_cpu(&last_hdr->len, len); goto out; } } -- 2.25.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH v2] virtio/vsock: allocate multiple skbuffs on tx
On Sun, Mar 19, 2023 at 09:46:10PM +0300, Arseniy Krasnov wrote: This adds small optimization for tx path: instead of allocating single skbuff on every call to transport, allocate multiple skbuff's until credit space allows, thus trying to send as much as possible data without return to af_vsock.c. Signed-off-by: Arseniy Krasnov --- Link to v1: https://lore.kernel.org/netdev/2c52aa26-8181-d37a-bccd-a86bd3cbc...@sberdevices.ru/ Changelog: v1 -> v2: - If sent something, return number of bytes sent (even in case of error). Return error only if failed to sent first skbuff. net/vmw_vsock/virtio_transport_common.c | 53 ++--- 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index 6564192e7f20..3fdf1433ec28 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -196,7 +196,8 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk, const struct virtio_transport *t_ops; struct virtio_vsock_sock *vvs; u32 pkt_len = info->pkt_len; - struct sk_buff *skb; + u32 rest_len; + int ret; info->type = virtio_transport_get_type(sk_vsock(vsk)); @@ -216,10 +217,6 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk, vvs = vsk->trans; - /* we can send less than pkt_len bytes */ - if (pkt_len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE) - pkt_len = VIRTIO_VSOCK_MAX_PKT_BUF_SIZE; - /* virtio_transport_get_credit might return less than pkt_len credit */ pkt_len = virtio_transport_get_credit(vvs, pkt_len); @@ -227,17 +224,45 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk, if (pkt_len == 0 && info->op == VIRTIO_VSOCK_OP_RW) return pkt_len; - skb = virtio_transport_alloc_skb(info, pkt_len, -src_cid, src_port, -dst_cid, dst_port); - if (!skb) { - virtio_transport_put_credit(vvs, pkt_len); - return -ENOMEM; - } + ret = 0; + rest_len = pkt_len; + + do { + struct sk_buff *skb; + size_t skb_len; + + skb_len = min_t(u32, VIRTIO_VSOCK_MAX_PKT_BUF_SIZE, rest_len); + + skb = virtio_transport_alloc_skb(info, skb_len, +src_cid, src_port, +dst_cid, dst_port); + if (!skb) { + ret = -ENOMEM; + break; + } + + virtio_transport_inc_tx_pkt(vvs, skb); + + ret = t_ops->send_pkt(skb); + + if (ret < 0) + break; - virtio_transport_inc_tx_pkt(vvs, skb); + rest_len -= skb_len; t_ops->send_pkt() is returning the number of bytes sent. Current implementations always return `skb_len`, so there should be no problem, but it would be better to put a comment here, or we should handle the case where ret != skb_len to avoid future issues. + } while (rest_len); - return t_ops->send_pkt(skb); + /* Don't call this function with zero as argument: +* it tries to acquire spinlock and such argument +* makes this call useless. Good point, can we do the same also for virtio_transport_get_credit()? (Maybe in a separate patch) I'm thinking if may be better to do it directly inside the functions, but I don't have a strong opinion on that since we only call them here. Thanks, Stefano +*/ + if (rest_len) + virtio_transport_put_credit(vvs, rest_len); + + /* Return number of bytes, if any data has been sent. */ + if (rest_len != pkt_len) + ret = pkt_len - rest_len; + + return ret; } static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs, -- 2.25.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH v1] virtio/vsock: check transport before skb allocation
On Fri, Mar 17, 2023 at 01:37:10PM +0300, Arseniy Krasnov wrote: Pointer to transport could be checked before allocation of skbuff, thus there is no need to free skbuff when this pointer is NULL. Signed-off-by: Arseniy Krasnov --- net/vmw_vsock/virtio_transport_common.c | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) LGTM, I think net-next is fine for this. Reviewed-by: Stefano Garzarella Thanks, Stefano diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index cda587196475..607149259e8b 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -867,6 +867,9 @@ static int virtio_transport_reset_no_sock(const struct virtio_transport *t, if (le16_to_cpu(hdr->op) == VIRTIO_VSOCK_OP_RST) return 0; + if (!t) + return -ENOTCONN; + reply = virtio_transport_alloc_skb(&info, 0, le64_to_cpu(hdr->dst_cid), le32_to_cpu(hdr->dst_port), @@ -875,11 +878,6 @@ static int virtio_transport_reset_no_sock(const struct virtio_transport *t, if (!reply) return -ENOMEM; - if (!t) { - kfree_skb(reply); - return -ENOTCONN; - } - return t->send_pkt(reply); } -- 2.25.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 4/6] drm/fbdev-generic: Clean up after failed probing
Hi Javier Am 17.03.23 um 13:24 schrieb Javier Martinez Canillas: Thomas Zimmermann writes: [...] @@ -91,36 +93,52 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper, fb_helper->buffer = buffer; fb_helper->fb = buffer->fb; - fb = buffer->fb; + + screen_size = buffer->gem->size; [...] - info->screen_size = sizes->surface_height * fb->pitches[0]; [...] I wonder if this change should be a separate patch? I know that it should be the same size but still feels like an unrelated change that deserves a different patch and description. This comment made me look up the exact meaning if screen_size, which is "Amount of ioremapped VRAM or 0". [1] Other drivers with shadow buffers (udlfb, metronomefb) apparently don't set this field. So the generic fbdev probably shouldn't either. The size of the video memory (physical or shadowed) in in fix.smem_len. [2] From grep'ing through the source code, it's not clear to me why screen_size exists in the first place. Best regards Thomas [1] https://elixir.bootlin.com/linux/latest/source/include/linux/fb.h#L494 [2] https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/fb.h#L161 [...] - /* Set a default deferred I/O handler */ + /* deferred I/O */ I would either have it as /* Generic fbdev deferred I/O handler */ or just remove the comment. I understand why you are removing the "default", since implies that drivers can change the handler and that's not the case here. But I think that just leaving the "deferred I/O" comment there doesn't say that much. Reviewed-by: Javier Martinez Canillas -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 0/2] [RFC] virtio-rng entropy leak reporting feature
On Mon, 2023-03-13 at 19:05 +0100, Amit Shah wrote: > Hey Herbert / Jason / crypto maintainers, [...] > Let's wait a couple more days for responses, otherwise I suggest you > resubmit to kickstart a new discussion, with the note that Jason had > something else in mind - so that it doesn't appear as though we're > trying to override that. I reached out to Jason on IRC, and he mentioned he will follow up with a patch that incorporates ideas from your patch plus hooking into random.c. Sounds promising! Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 2/2] vdpa/snet: support the suspend vDPA callback
When suspend is called, the driver sends a suspend command to the DPU through the control mechanism. Signed-off-by: Alvaro Karsz --- drivers/vdpa/solidrun/snet_ctrl.c | 6 ++ drivers/vdpa/solidrun/snet_main.c | 15 +++ drivers/vdpa/solidrun/snet_vdpa.h | 1 + 3 files changed, 22 insertions(+) diff --git a/drivers/vdpa/solidrun/snet_ctrl.c b/drivers/vdpa/solidrun/snet_ctrl.c index 54909549a00..7d08e6f 100644 --- a/drivers/vdpa/solidrun/snet_ctrl.c +++ b/drivers/vdpa/solidrun/snet_ctrl.c @@ -15,6 +15,7 @@ enum snet_ctrl_opcodes { SNET_CTRL_OP_DESTROY = 1, SNET_CTRL_OP_READ_VQ_STATE, + SNET_CTRL_OP_SUSPEND, }; #define SNET_CTRL_TIMEOUT 200 @@ -316,3 +317,8 @@ int snet_read_vq_state(struct snet *snet, u16 idx, struct vdpa_vq_state *state) return snet_ctrl_read_from_dpu(snet, SNET_CTRL_OP_READ_VQ_STATE, idx, state, sizeof(*state)); } + +int snet_suspend_dev(struct snet *snet) +{ + return snet_send_ctrl_msg(snet, SNET_CTRL_OP_SUSPEND, 0); +} diff --git a/drivers/vdpa/solidrun/snet_main.c b/drivers/vdpa/solidrun/snet_main.c index daeb69d00ed..96830e58211 100644 --- a/drivers/vdpa/solidrun/snet_main.c +++ b/drivers/vdpa/solidrun/snet_main.c @@ -483,6 +483,20 @@ static void snet_set_config(struct vdpa_device *vdev, unsigned int offset, iowrite8(*buf_ptr++, cfg_ptr + i); } +static int snet_suspend(struct vdpa_device *vdev) +{ + struct snet *snet = vdpa_to_snet(vdev); + int ret; + + ret = snet_suspend_dev(snet); + if (ret) + SNET_ERR(snet->pdev, "SNET[%u] suspend failed, err: %d\n", snet->sid, ret); + else + SNET_DBG(snet->pdev, "Suspend SNET[%u] device\n", snet->sid); + + return ret; +} + static const struct vdpa_config_ops snet_config_ops = { .set_vq_address = snet_set_vq_address, .set_vq_num = snet_set_vq_num, @@ -508,6 +522,7 @@ static const struct vdpa_config_ops snet_config_ops = { .set_status = snet_set_status, .get_config = snet_get_config, .set_config = snet_set_config, + .suspend= snet_suspend, }; static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet) diff --git a/drivers/vdpa/solidrun/snet_vdpa.h b/drivers/vdpa/solidrun/snet_vdpa.h index a4213e97cfc..6dbb95fe2b7 100644 --- a/drivers/vdpa/solidrun/snet_vdpa.h +++ b/drivers/vdpa/solidrun/snet_vdpa.h @@ -201,5 +201,6 @@ void psnet_create_hwmon(struct pci_dev *pdev); void snet_ctrl_clear(struct snet *snet); int snet_destroy_dev(struct snet *snet); int snet_read_vq_state(struct snet *snet, u16 idx, struct vdpa_vq_state *state); +int snet_suspend_dev(struct snet *snet); #endif //_SNET_VDPA_H_ -- 2.34.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 1/2] vdpa/snet: support getting and setting VQ state
This patch adds the get_vq_state and set_vq_state vDPA callbacks. In order to get the VQ state, the state needs to be read from the DPU. In order to allow that, the old messaging mechanism is replaced with a new, flexible control mechanism. This mechanism allows to read data from the DPU. The mechanism can be used if the negotiated config version is 2 or higher. If the new mechanism is used when the config version is 1, it will call snet_send_ctrl_msg_old, which is config 1 compatible. Signed-off-by: Alvaro Karsz --- drivers/vdpa/solidrun/Makefile | 1 + drivers/vdpa/solidrun/snet_ctrl.c | 318 + drivers/vdpa/solidrun/snet_hwmon.c | 2 +- drivers/vdpa/solidrun/snet_main.c | 111 -- drivers/vdpa/solidrun/snet_vdpa.h | 17 +- 5 files changed, 378 insertions(+), 71 deletions(-) create mode 100644 drivers/vdpa/solidrun/snet_ctrl.c diff --git a/drivers/vdpa/solidrun/Makefile b/drivers/vdpa/solidrun/Makefile index c0aa3415bf7..9116252cd5f 100644 --- a/drivers/vdpa/solidrun/Makefile +++ b/drivers/vdpa/solidrun/Makefile @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_SNET_VDPA) += snet_vdpa.o snet_vdpa-$(CONFIG_SNET_VDPA) += snet_main.o +snet_vdpa-$(CONFIG_SNET_VDPA) += snet_ctrl.o ifdef CONFIG_HWMON snet_vdpa-$(CONFIG_SNET_VDPA) += snet_hwmon.o endif diff --git a/drivers/vdpa/solidrun/snet_ctrl.c b/drivers/vdpa/solidrun/snet_ctrl.c new file mode 100644 index 000..54909549a00 --- /dev/null +++ b/drivers/vdpa/solidrun/snet_ctrl.c @@ -0,0 +1,318 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * SolidRun DPU driver for control plane + * + * Copyright (C) 2022-2023 SolidRun + * + * Author: Alvaro Karsz + * + */ + +#include + +#include "snet_vdpa.h" + +enum snet_ctrl_opcodes { + SNET_CTRL_OP_DESTROY = 1, + SNET_CTRL_OP_READ_VQ_STATE, +}; + +#define SNET_CTRL_TIMEOUT 200 + +#define SNET_CTRL_DATA_SIZE_MASK 0x +#define SNET_CTRL_IN_PROCESS_MASK 0x0001 +#define SNET_CTRL_CHUNK_RDY_MASK 0x0002 +#define SNET_CTRL_ERROR_MASK 0x0FFC + +#define SNET_VAL_TO_ERR(val) (-(((val) & SNET_CTRL_ERROR_MASK) >> 18)) +#define SNET_EMPTY_CTRL(val) (((val) & SNET_CTRL_ERROR_MASK) || \ + !((val) & SNET_CTRL_IN_PROCESS_MASK)) +#define SNET_DATA_READY(val) ((val) & (SNET_CTRL_ERROR_MASK | SNET_CTRL_CHUNK_RDY_MASK)) + +/* Control register used to read data from the DPU */ +struct snet_ctrl_reg_ctrl { + /* Chunk size in 4B words */ + u16 data_size; + /* We are in the middle of a command */ + u16 in_process:1; + /* A data chunk is ready and can be consumed */ + u16 chunk_ready:1; + /* Error code */ + u16 error:10; + /* Saved for future usage */ + u16 rsvd:4; +}; + +/* Opcode register */ +struct snet_ctrl_reg_op { + u16 opcode; + /* Only if VQ index is relevant for the command */ + u16 vq_idx; +}; + +struct snet_ctrl_regs { + struct snet_ctrl_reg_op op; + struct snet_ctrl_reg_ctrl ctrl; + u32 rsvd; + u32 data[]; +}; + +static struct snet_ctrl_regs __iomem *snet_get_ctrl(struct snet *snet) +{ + return snet->bar + snet->psnet->cfg.ctrl_off; +} + +static int snet_wait_for_empty_ctrl(struct snet_ctrl_regs __iomem *regs) +{ + u32 val; + + return readx_poll_timeout(ioread32, ®s->ctrl, val, SNET_EMPTY_CTRL(val), 10, + SNET_CTRL_TIMEOUT); +} + +static int snet_wait_for_empty_op(struct snet_ctrl_regs __iomem *regs) +{ + u32 val; + + return readx_poll_timeout(ioread32, ®s->op, val, !val, 10, SNET_CTRL_TIMEOUT); +} + +static int snet_wait_for_data(struct snet_ctrl_regs __iomem *regs) +{ + u32 val; + + return readx_poll_timeout(ioread32, ®s->ctrl, val, SNET_DATA_READY(val), 10, + SNET_CTRL_TIMEOUT); +} + +static u32 snet_read32_word(struct snet_ctrl_regs __iomem *ctrl_regs, u16 word_idx) +{ + return ioread32(&ctrl_regs->data[word_idx]); +} + +static u32 snet_read_ctrl(struct snet_ctrl_regs __iomem *ctrl_regs) +{ + return ioread32(&ctrl_regs->ctrl); +} + +static void snet_write_ctrl(struct snet_ctrl_regs __iomem *ctrl_regs, u32 val) +{ + iowrite32(val, &ctrl_regs->ctrl); +} + +static void snet_write_op(struct snet_ctrl_regs __iomem *ctrl_regs, u32 val) +{ + iowrite32(val, &ctrl_regs->op); +} + +static int snet_wait_for_dpu_completion(struct snet_ctrl_regs __iomem *ctrl_regs) +{ + /* Wait until the DPU finishes completely. +* It will clear the opcode register. +*/ + return snet_wait_for_empty_op(ctrl_regs); +} + +/* Reading ctrl from the DPU: + * buf_size must be 4B aligned + * + * Steps: + * + * (1) Verify that the DPU is not in the middle of another operation by + * reading the in_process and error bits in the control register. + * (2) Write the reques
[PATCH 0/2] vdpa/snet: support [s/g]et_vq_state and suspend
Add more vDPA callbacks. [s/g]et_vq_state is added in patch 1, including a new control mechanism to read data from the DPU. suspend is added in patch 2. Alvaro Karsz (2): vdpa/snet: support getting and setting VQ state vdpa/snet: support the suspend vDPA callback drivers/vdpa/solidrun/Makefile | 1 + drivers/vdpa/solidrun/snet_ctrl.c | 324 + drivers/vdpa/solidrun/snet_hwmon.c | 2 +- drivers/vdpa/solidrun/snet_main.c | 126 ++- drivers/vdpa/solidrun/snet_vdpa.h | 18 +- 5 files changed, 400 insertions(+), 71 deletions(-) create mode 100644 drivers/vdpa/solidrun/snet_ctrl.c -- 2.34.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 03/11] vdpa: Add set_irq_affinity callback in vdpa_config_ops
On Fri, Mar 17, 2023 at 3:45 PM Yongji Xie wrote: > > On Thu, Mar 16, 2023 at 12:03 PM Jason Wang wrote: > > > > On Tue, Feb 28, 2023 at 5:42 PM Xie Yongji wrote: > > > > > > This introduces set_irq_affinity callback in > > > vdpa_config_ops so that vdpa device driver can > > > get the interrupt affinity hint from the virtio > > > device driver. The interrupt affinity hint would > > > be needed by the interrupt affinity spreading > > > mechanism. > > > > > > Signed-off-by: Xie Yongji > > > --- > > > drivers/virtio/virtio_vdpa.c | 4 > > > include/linux/vdpa.h | 9 + > > > 2 files changed, 13 insertions(+) > > > > > > diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c > > > index f72696b4c1c2..9eee8afabda8 100644 > > > --- a/drivers/virtio/virtio_vdpa.c > > > +++ b/drivers/virtio/virtio_vdpa.c > > > @@ -282,9 +282,13 @@ static int virtio_vdpa_find_vqs(struct virtio_device > > > *vdev, unsigned int nvqs, > > > struct virtio_vdpa_device *vd_dev = to_virtio_vdpa_device(vdev); > > > struct vdpa_device *vdpa = vd_get_vdpa(vdev); > > > const struct vdpa_config_ops *ops = vdpa->config; > > > + struct irq_affinity default_affd = { 0 }; > > > struct vdpa_callback cb; > > > int i, err, queue_idx = 0; > > > > > > + if (ops->set_irq_affinity) > > > + ops->set_irq_affinity(vdpa, desc ? desc : &default_affd); > > > + > > > for (i = 0; i < nvqs; ++i) { > > > if (!names[i]) { > > > vqs[i] = NULL; > > > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h > > > index d61f369f9cd6..10bd22387276 100644 > > > --- a/include/linux/vdpa.h > > > +++ b/include/linux/vdpa.h > > > @@ -259,6 +259,13 @@ struct vdpa_map_file { > > > * @vdev: vdpa device > > > * @idx: virtqueue index > > > * Returns the irq affinity mask > > > + * @set_irq_affinity: Pass the irq affinity hint (best effort) > > > > Note that this could easily confuse the users. I wonder if we can > > unify it with set_irq_affinity. Looking at vduse's implementation, it > > should be possible. > > > > Do you mean unify set_irq_affinity() with set_vq_affinity()? Actually > I didn't get how to achieve that. The set_vq_affinity() callback is > called by virtio_config_ops.set_vq_affinity() but the set_irq_affinity > is called by virtio_config_ops.find_vqs(), I don't know where to call > the unified callback. I meant, can we stick a single per vq affinity config ops then use that in virtio-vpda's find_vqs() by something like: masks = create_affinity_masks(dev->vq_num, desc); for (i = 0; i < dev->vq_num; i++) config->set_vq_affinity() ... ? Thanks > > Thanks, > Yongji > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vdpa/mlx5: Remove debugfs file after device unregister
On Mon, Mar 20, 2023 at 5:07 PM Eli Cohen wrote: > > On 17/03/2023 5:32, Jason Wang wrote: > > On Sun, Mar 12, 2023 at 4:41 PM Eli Cohen wrote: > >> When deleting the vdpa device, the debugfs files need to be removed so > >> need to remove debugfs after the device has been unregistered. > >> > >> This fixes null pointer dereference when someone deletes the device > >> after debugfs has been populated. > >> > >> Fixes: 294221004322 ("vdpa/mlx5: Add debugfs subtree") > >> 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 3858ba1e8975..3f6149f2ffd4 100644 > >> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > >> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > >> @@ -3322,8 +3322,6 @@ static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev > >> *v_mdev, struct vdpa_device * > >> struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev); > >> struct workqueue_struct *wq; > >> > >> - mlx5_vdpa_remove_debugfs(ndev->debugfs); > >> - ndev->debugfs = NULL; > >> if (ndev->nb_registered) { > >> ndev->nb_registered = false; > >> mlx5_notifier_unregister(mvdev->mdev, &ndev->nb); > >> @@ -3332,6 +3330,8 @@ static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev > >> *v_mdev, struct vdpa_device * > >> mvdev->wq = NULL; > >> destroy_workqueue(wq); > >> _vdpa_unregister_device(dev); > > What if the user tries to access debugfs after _vdpa_unregister_device()? > I don't see an issue with that. During the process of deleting a device, > the resources are destroyed and their corresponding debugfs files are > also destroyed just prior to destroying the resource. For example, rx_flow_table_show() requires the structure mlx5_vdpa_net alive, but it seems the structure has been freed after _vdpa_unregister_device(). If a user tries to access that file after _vdpa_unregister_device() but before mlx5_vdpa_remove_debugfs(), will we end up with use-after-free? Thanks > > > > Thanks > > > >> + mlx5_vdpa_remove_debugfs(ndev->debugfs); > >> + ndev->debugfs = NULL; > >> mgtdev->ndev = NULL; > >> } > >> > >> -- > >> 2.38.1 > >> > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC net-next 0/8] virtio_net: refactor xdp codes
On Wed, 15 Mar 2023 12:10:34 +0800, Xuan Zhuo wrote: Ping Thanks. > Due to historical reasons, the implementation of XDP in virtio-net is > relatively > chaotic. For example, the processing of XDP actions has two copies of similar > code. Such as page, xdp_page processing, etc. > > The purpose of this patch set is to refactor these code. Reduce the difficulty > of subsequent maintenance. Subsequent developers will not introduce new bugs > because of some complex logical relationships. > > In addition, the supporting to AF_XDP that I want to submit later will also > need > to reuse the logic of XDP, such as the processing of actions, I don't want to > introduce a new similar code. In this way, I can reuse these codes in the > future. > > This patches are developed on the top of another patch set[1]. I may have to > wait to merge this. So this patch set is a RFC. > > Please review. > > Thanks. > > [1]. > https://lore.kernel.org/netdev/20230315015223.89137-1-xuanz...@linux.alibaba.com/ > > > Xuan Zhuo (8): > virtio_net: mergeable xdp: put old page immediately > virtio_net: mergeable xdp: introduce mergeable_xdp_prepare > virtio_net: introduce virtnet_xdp_handler() to seprate the logic of > run xdp > virtio_net: separate the logic of freeing xdp shinfo > virtio_net: separate the logic of freeing the rest mergeable buf > virtio_net: auto release xdp shinfo > virtio_net: introduce receive_mergeable_xdp() > virtio_net: introduce receive_small_xdp() > > drivers/net/virtio_net.c | 615 +++ > 1 file changed, 357 insertions(+), 258 deletions(-) > > -- > 2.32.0.3.g01195cf9f > > ___ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization