* [PATCH v2 0/3] mlx5_vdpa bug fixes @ 2021-02-10 21:47 ` Si-Wei Liu 0 siblings, 0 replies; 17+ messages in thread From: Si-Wei Liu @ 2021-02-10 21:47 UTC (permalink / raw) To: mst, jasowang, elic; +Cc: linux-kernel, virtualization, netdev, si-wei.liu This set attempts to fix a few independent issues recently found in mlx5_vdpa driver. Please find details for each in the commit message. Patch 1 and patch 3 are already Ack'ed by Jason Wang. Patch 2 is reworked to move virtio feature capability query to mlx5v_probe() as suggested by Eli. -- v1->v2: move feature capability query to probing (Eli) Si-Wei Liu (3): vdpa/mlx5: should exclude header length and fcs from mtu vdpa/mlx5: fix feature negotiation across device reset vdpa/mlx5: defer clear_virtqueues to until DRIVER_OK drivers/vdpa/mlx5/core/mlx5_vdpa.h | 4 ++++ drivers/vdpa/mlx5/net/mlx5_vnet.c | 42 +++++++++++++++++++++++++++----------- 2 files changed, 34 insertions(+), 12 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 0/3] mlx5_vdpa bug fixes @ 2021-02-10 21:47 ` Si-Wei Liu 0 siblings, 0 replies; 17+ messages in thread From: Si-Wei Liu @ 2021-02-10 21:47 UTC (permalink / raw) To: mst, jasowang, elic; +Cc: si-wei.liu, netdev, linux-kernel, virtualization This set attempts to fix a few independent issues recently found in mlx5_vdpa driver. Please find details for each in the commit message. Patch 1 and patch 3 are already Ack'ed by Jason Wang. Patch 2 is reworked to move virtio feature capability query to mlx5v_probe() as suggested by Eli. -- v1->v2: move feature capability query to probing (Eli) Si-Wei Liu (3): vdpa/mlx5: should exclude header length and fcs from mtu vdpa/mlx5: fix feature negotiation across device reset vdpa/mlx5: defer clear_virtqueues to until DRIVER_OK drivers/vdpa/mlx5/core/mlx5_vdpa.h | 4 ++++ drivers/vdpa/mlx5/net/mlx5_vnet.c | 42 +++++++++++++++++++++++++++----------- 2 files changed, 34 insertions(+), 12 deletions(-) -- 1.8.3.1 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/3] vdpa/mlx5: should exclude header length and fcs from mtu 2021-02-10 21:47 ` Si-Wei Liu @ 2021-02-10 21:47 ` Si-Wei Liu -1 siblings, 0 replies; 17+ messages in thread From: Si-Wei Liu @ 2021-02-10 21:47 UTC (permalink / raw) To: mst, jasowang, elic; +Cc: linux-kernel, virtualization, netdev, si-wei.liu When feature VIRTIO_NET_F_MTU is negotiated on mlx5_vdpa, 22 extra bytes worth of MTU length is shown in guest. This is because the mlx5_query_port_max_mtu API returns the "hardware" MTU value, which does not just contain the Ethernet payload, but includes extra lengths starting from the Ethernet header up to the FCS altogether. Fix the MTU so packets won't get dropped silently. Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices") Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> Acked-by: Jason Wang <jasowang@redhat.com> Acked-by: Eli Cohen <elic@nvidia.com> --- drivers/vdpa/mlx5/core/mlx5_vdpa.h | 4 ++++ drivers/vdpa/mlx5/net/mlx5_vnet.c | 15 ++++++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h b/drivers/vdpa/mlx5/core/mlx5_vdpa.h index 08f742f..b6cc53b 100644 --- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h +++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h @@ -4,9 +4,13 @@ #ifndef __MLX5_VDPA_H__ #define __MLX5_VDPA_H__ +#include <linux/etherdevice.h> +#include <linux/if_vlan.h> #include <linux/vdpa.h> #include <linux/mlx5/driver.h> +#define MLX5V_ETH_HARD_MTU (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN) + struct mlx5_vdpa_direct_mr { u64 start; u64 end; diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c index dc88559..b8416c4 100644 --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c @@ -1907,6 +1907,19 @@ static int mlx5_get_vq_irq(struct vdpa_device *vdv, u16 idx) .free = mlx5_vdpa_free, }; +static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu) +{ + u16 hw_mtu; + int err; + + err = mlx5_query_nic_vport_mtu(mdev, &hw_mtu); + if (err) + return err; + + *mtu = hw_mtu - MLX5V_ETH_HARD_MTU; + return 0; +} + static int alloc_resources(struct mlx5_vdpa_net *ndev) { struct mlx5_vdpa_net_resources *res = &ndev->res; @@ -1992,7 +2005,7 @@ static int mlx5v_probe(struct auxiliary_device *adev, init_mvqs(ndev); mutex_init(&ndev->reslock); config = &ndev->config; - err = mlx5_query_nic_vport_mtu(mdev, &ndev->mtu); + err = query_mtu(mdev, &ndev->mtu); if (err) goto err_mtu; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 1/3] vdpa/mlx5: should exclude header length and fcs from mtu @ 2021-02-10 21:47 ` Si-Wei Liu 0 siblings, 0 replies; 17+ messages in thread From: Si-Wei Liu @ 2021-02-10 21:47 UTC (permalink / raw) To: mst, jasowang, elic; +Cc: si-wei.liu, netdev, linux-kernel, virtualization When feature VIRTIO_NET_F_MTU is negotiated on mlx5_vdpa, 22 extra bytes worth of MTU length is shown in guest. This is because the mlx5_query_port_max_mtu API returns the "hardware" MTU value, which does not just contain the Ethernet payload, but includes extra lengths starting from the Ethernet header up to the FCS altogether. Fix the MTU so packets won't get dropped silently. Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices") Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> Acked-by: Jason Wang <jasowang@redhat.com> Acked-by: Eli Cohen <elic@nvidia.com> --- drivers/vdpa/mlx5/core/mlx5_vdpa.h | 4 ++++ drivers/vdpa/mlx5/net/mlx5_vnet.c | 15 ++++++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h b/drivers/vdpa/mlx5/core/mlx5_vdpa.h index 08f742f..b6cc53b 100644 --- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h +++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h @@ -4,9 +4,13 @@ #ifndef __MLX5_VDPA_H__ #define __MLX5_VDPA_H__ +#include <linux/etherdevice.h> +#include <linux/if_vlan.h> #include <linux/vdpa.h> #include <linux/mlx5/driver.h> +#define MLX5V_ETH_HARD_MTU (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN) + struct mlx5_vdpa_direct_mr { u64 start; u64 end; diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c index dc88559..b8416c4 100644 --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c @@ -1907,6 +1907,19 @@ static int mlx5_get_vq_irq(struct vdpa_device *vdv, u16 idx) .free = mlx5_vdpa_free, }; +static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu) +{ + u16 hw_mtu; + int err; + + err = mlx5_query_nic_vport_mtu(mdev, &hw_mtu); + if (err) + return err; + + *mtu = hw_mtu - MLX5V_ETH_HARD_MTU; + return 0; +} + static int alloc_resources(struct mlx5_vdpa_net *ndev) { struct mlx5_vdpa_net_resources *res = &ndev->res; @@ -1992,7 +2005,7 @@ static int mlx5v_probe(struct auxiliary_device *adev, init_mvqs(ndev); mutex_init(&ndev->reslock); config = &ndev->config; - err = mlx5_query_nic_vport_mtu(mdev, &ndev->mtu); + err = query_mtu(mdev, &ndev->mtu); if (err) goto err_mtu; -- 1.8.3.1 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/3] vdpa/mlx5: fix feature negotiation across device reset 2021-02-10 21:47 ` Si-Wei Liu @ 2021-02-10 21:47 ` Si-Wei Liu -1 siblings, 0 replies; 17+ messages in thread From: Si-Wei Liu @ 2021-02-10 21:47 UTC (permalink / raw) To: mst, jasowang, elic; +Cc: linux-kernel, virtualization, netdev, si-wei.liu The mlx_features denotes the capability for which set of virtio features is supported by device. In principle, this field needs not be cleared during virtio device reset, as this capability is static and does not change across reset. In fact, the current code may have the assumption that mlx_features can be reloaded from firmware via the .get_features ops after device is reset (via the .set_status ops), which is unfortunately not true. The userspace VMM might save a copy of backend capable features and won't call into kernel again to get it on reset. This causes all virtio features getting disabled on newly created virtqs after device reset, while guest would hold mismatched view of available features. For e.g., the guest may still assume tx checksum offload is available after reset and feature negotiation, causing frames with bogus (incomplete) checksum transmitted on the wire. Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices") Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> --- drivers/vdpa/mlx5/net/mlx5_vnet.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c index b8416c4..7c1f789 100644 --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c @@ -1486,16 +1486,8 @@ static u64 mlx_to_vritio_features(u16 dev_features) static u64 mlx5_vdpa_get_features(struct vdpa_device *vdev) { struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev); - struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev); - u16 dev_features; - dev_features = MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, device_features_bits_mask); - ndev->mvdev.mlx_features = mlx_to_vritio_features(dev_features); - if (MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, virtio_version_1_0)) - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_VERSION_1); - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM); - print_features(mvdev, ndev->mvdev.mlx_features, false); - return ndev->mvdev.mlx_features; + return mvdev->mlx_features; } static int verify_min_features(struct mlx5_vdpa_dev *mvdev, u64 features) @@ -1788,7 +1780,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) clear_virtqueues(ndev); mlx5_vdpa_destroy_mr(&ndev->mvdev); ndev->mvdev.status = 0; - ndev->mvdev.mlx_features = 0; ++mvdev->generation; return; } @@ -1907,6 +1898,19 @@ static int mlx5_get_vq_irq(struct vdpa_device *vdv, u16 idx) .free = mlx5_vdpa_free, }; +static void query_virtio_features(struct mlx5_vdpa_net *ndev) +{ + struct mlx5_vdpa_dev *mvdev = &ndev->mvdev; + u16 dev_features; + + dev_features = MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, device_features_bits_mask); + mvdev->mlx_features = mlx_to_vritio_features(dev_features); + if (MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, virtio_version_1_0)) + mvdev->mlx_features |= BIT_ULL(VIRTIO_F_VERSION_1); + mvdev->mlx_features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM); + print_features(mvdev, mvdev->mlx_features, false); +} + static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu) { u16 hw_mtu; @@ -2005,6 +2009,7 @@ static int mlx5v_probe(struct auxiliary_device *adev, init_mvqs(ndev); mutex_init(&ndev->reslock); config = &ndev->config; + query_virtio_features(ndev); err = query_mtu(mdev, &ndev->mtu); if (err) goto err_mtu; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/3] vdpa/mlx5: fix feature negotiation across device reset @ 2021-02-10 21:47 ` Si-Wei Liu 0 siblings, 0 replies; 17+ messages in thread From: Si-Wei Liu @ 2021-02-10 21:47 UTC (permalink / raw) To: mst, jasowang, elic; +Cc: si-wei.liu, netdev, linux-kernel, virtualization The mlx_features denotes the capability for which set of virtio features is supported by device. In principle, this field needs not be cleared during virtio device reset, as this capability is static and does not change across reset. In fact, the current code may have the assumption that mlx_features can be reloaded from firmware via the .get_features ops after device is reset (via the .set_status ops), which is unfortunately not true. The userspace VMM might save a copy of backend capable features and won't call into kernel again to get it on reset. This causes all virtio features getting disabled on newly created virtqs after device reset, while guest would hold mismatched view of available features. For e.g., the guest may still assume tx checksum offload is available after reset and feature negotiation, causing frames with bogus (incomplete) checksum transmitted on the wire. Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices") Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> --- drivers/vdpa/mlx5/net/mlx5_vnet.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c index b8416c4..7c1f789 100644 --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c @@ -1486,16 +1486,8 @@ static u64 mlx_to_vritio_features(u16 dev_features) static u64 mlx5_vdpa_get_features(struct vdpa_device *vdev) { struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev); - struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev); - u16 dev_features; - dev_features = MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, device_features_bits_mask); - ndev->mvdev.mlx_features = mlx_to_vritio_features(dev_features); - if (MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, virtio_version_1_0)) - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_VERSION_1); - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM); - print_features(mvdev, ndev->mvdev.mlx_features, false); - return ndev->mvdev.mlx_features; + return mvdev->mlx_features; } static int verify_min_features(struct mlx5_vdpa_dev *mvdev, u64 features) @@ -1788,7 +1780,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) clear_virtqueues(ndev); mlx5_vdpa_destroy_mr(&ndev->mvdev); ndev->mvdev.status = 0; - ndev->mvdev.mlx_features = 0; ++mvdev->generation; return; } @@ -1907,6 +1898,19 @@ static int mlx5_get_vq_irq(struct vdpa_device *vdv, u16 idx) .free = mlx5_vdpa_free, }; +static void query_virtio_features(struct mlx5_vdpa_net *ndev) +{ + struct mlx5_vdpa_dev *mvdev = &ndev->mvdev; + u16 dev_features; + + dev_features = MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, device_features_bits_mask); + mvdev->mlx_features = mlx_to_vritio_features(dev_features); + if (MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, virtio_version_1_0)) + mvdev->mlx_features |= BIT_ULL(VIRTIO_F_VERSION_1); + mvdev->mlx_features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM); + print_features(mvdev, mvdev->mlx_features, false); +} + static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu) { u16 hw_mtu; @@ -2005,6 +2009,7 @@ static int mlx5v_probe(struct auxiliary_device *adev, init_mvqs(ndev); mutex_init(&ndev->reslock); config = &ndev->config; + query_virtio_features(ndev); err = query_mtu(mdev, &ndev->mtu); if (err) goto err_mtu; -- 1.8.3.1 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] vdpa/mlx5: fix feature negotiation across device reset 2021-02-10 21:47 ` Si-Wei Liu (?) @ 2021-02-11 7:33 ` Eli Cohen -1 siblings, 0 replies; 17+ messages in thread From: Eli Cohen @ 2021-02-11 7:33 UTC (permalink / raw) To: Si-Wei Liu; +Cc: mst, jasowang, linux-kernel, virtualization, netdev On Wed, Feb 10, 2021 at 01:47:59PM -0800, Si-Wei Liu wrote: > The mlx_features denotes the capability for which > set of virtio features is supported by device. In > principle, this field needs not be cleared during > virtio device reset, as this capability is static > and does not change across reset. > > In fact, the current code may have the assumption > that mlx_features can be reloaded from firmware > via the .get_features ops after device is reset > (via the .set_status ops), which is unfortunately > not true. The userspace VMM might save a copy > of backend capable features and won't call into > kernel again to get it on reset. This causes all > virtio features getting disabled on newly created > virtqs after device reset, while guest would hold > mismatched view of available features. For e.g., > the guest may still assume tx checksum offload > is available after reset and feature negotiation, > causing frames with bogus (incomplete) checksum > transmitted on the wire. > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices") > Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> Acked-by: Eli Cohen <elic@nvidia.com> > --- > drivers/vdpa/mlx5/net/mlx5_vnet.c | 25 +++++++++++++++---------- > 1 file changed, 15 insertions(+), 10 deletions(-) > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > index b8416c4..7c1f789 100644 > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > @@ -1486,16 +1486,8 @@ static u64 mlx_to_vritio_features(u16 dev_features) > static u64 mlx5_vdpa_get_features(struct vdpa_device *vdev) > { > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev); > - struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev); > - u16 dev_features; > > - dev_features = MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, device_features_bits_mask); > - ndev->mvdev.mlx_features = mlx_to_vritio_features(dev_features); > - if (MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, virtio_version_1_0)) > - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_VERSION_1); > - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM); > - print_features(mvdev, ndev->mvdev.mlx_features, false); > - return ndev->mvdev.mlx_features; > + return mvdev->mlx_features; > } > > static int verify_min_features(struct mlx5_vdpa_dev *mvdev, u64 features) > @@ -1788,7 +1780,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) > clear_virtqueues(ndev); > mlx5_vdpa_destroy_mr(&ndev->mvdev); > ndev->mvdev.status = 0; > - ndev->mvdev.mlx_features = 0; > ++mvdev->generation; > return; > } > @@ -1907,6 +1898,19 @@ static int mlx5_get_vq_irq(struct vdpa_device *vdv, u16 idx) > .free = mlx5_vdpa_free, > }; > > +static void query_virtio_features(struct mlx5_vdpa_net *ndev) > +{ > + struct mlx5_vdpa_dev *mvdev = &ndev->mvdev; > + u16 dev_features; > + > + dev_features = MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, device_features_bits_mask); > + mvdev->mlx_features = mlx_to_vritio_features(dev_features); > + if (MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, virtio_version_1_0)) > + mvdev->mlx_features |= BIT_ULL(VIRTIO_F_VERSION_1); > + mvdev->mlx_features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM); > + print_features(mvdev, mvdev->mlx_features, false); > +} > + > static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu) > { > u16 hw_mtu; > @@ -2005,6 +2009,7 @@ static int mlx5v_probe(struct auxiliary_device *adev, > init_mvqs(ndev); > mutex_init(&ndev->reslock); > config = &ndev->config; > + query_virtio_features(ndev); > err = query_mtu(mdev, &ndev->mtu); > if (err) > goto err_mtu; > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] vdpa/mlx5: fix feature negotiation across device reset 2021-02-10 21:47 ` Si-Wei Liu @ 2021-02-18 6:36 ` Jason Wang -1 siblings, 0 replies; 17+ messages in thread From: Jason Wang @ 2021-02-18 6:36 UTC (permalink / raw) To: Si-Wei Liu, mst, elic; +Cc: linux-kernel, virtualization, netdev On 2021/2/11 上午5:47, Si-Wei Liu wrote: > The mlx_features denotes the capability for which > set of virtio features is supported by device. In > principle, this field needs not be cleared during > virtio device reset, as this capability is static > and does not change across reset. > > In fact, the current code may have the assumption > that mlx_features can be reloaded from firmware > via the .get_features ops after device is reset > (via the .set_status ops), which is unfortunately > not true. The userspace VMM might save a copy > of backend capable features and won't call into > kernel again to get it on reset. This causes all > virtio features getting disabled on newly created > virtqs after device reset, while guest would hold > mismatched view of available features. For e.g., > the guest may still assume tx checksum offload > is available after reset and feature negotiation, > causing frames with bogus (incomplete) checksum > transmitted on the wire. > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices") > Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> Acked-by: Jason Wang <jasowang@redhat.com> > --- > drivers/vdpa/mlx5/net/mlx5_vnet.c | 25 +++++++++++++++---------- > 1 file changed, 15 insertions(+), 10 deletions(-) > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > index b8416c4..7c1f789 100644 > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > @@ -1486,16 +1486,8 @@ static u64 mlx_to_vritio_features(u16 dev_features) > static u64 mlx5_vdpa_get_features(struct vdpa_device *vdev) > { > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev); > - struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev); > - u16 dev_features; > > - dev_features = MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, device_features_bits_mask); > - ndev->mvdev.mlx_features = mlx_to_vritio_features(dev_features); > - if (MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, virtio_version_1_0)) > - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_VERSION_1); > - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM); > - print_features(mvdev, ndev->mvdev.mlx_features, false); > - return ndev->mvdev.mlx_features; > + return mvdev->mlx_features; > } > > static int verify_min_features(struct mlx5_vdpa_dev *mvdev, u64 features) > @@ -1788,7 +1780,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) > clear_virtqueues(ndev); > mlx5_vdpa_destroy_mr(&ndev->mvdev); > ndev->mvdev.status = 0; > - ndev->mvdev.mlx_features = 0; > ++mvdev->generation; > return; > } > @@ -1907,6 +1898,19 @@ static int mlx5_get_vq_irq(struct vdpa_device *vdv, u16 idx) > .free = mlx5_vdpa_free, > }; > > +static void query_virtio_features(struct mlx5_vdpa_net *ndev) > +{ > + struct mlx5_vdpa_dev *mvdev = &ndev->mvdev; > + u16 dev_features; > + > + dev_features = MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, device_features_bits_mask); > + mvdev->mlx_features = mlx_to_vritio_features(dev_features); > + if (MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, virtio_version_1_0)) > + mvdev->mlx_features |= BIT_ULL(VIRTIO_F_VERSION_1); > + mvdev->mlx_features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM); > + print_features(mvdev, mvdev->mlx_features, false); > +} > + > static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu) > { > u16 hw_mtu; > @@ -2005,6 +2009,7 @@ static int mlx5v_probe(struct auxiliary_device *adev, > init_mvqs(ndev); > mutex_init(&ndev->reslock); > config = &ndev->config; > + query_virtio_features(ndev); > err = query_mtu(mdev, &ndev->mtu); > if (err) > goto err_mtu; ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] vdpa/mlx5: fix feature negotiation across device reset @ 2021-02-18 6:36 ` Jason Wang 0 siblings, 0 replies; 17+ messages in thread From: Jason Wang @ 2021-02-18 6:36 UTC (permalink / raw) To: Si-Wei Liu, mst, elic; +Cc: netdev, linux-kernel, virtualization On 2021/2/11 上午5:47, Si-Wei Liu wrote: > The mlx_features denotes the capability for which > set of virtio features is supported by device. In > principle, this field needs not be cleared during > virtio device reset, as this capability is static > and does not change across reset. > > In fact, the current code may have the assumption > that mlx_features can be reloaded from firmware > via the .get_features ops after device is reset > (via the .set_status ops), which is unfortunately > not true. The userspace VMM might save a copy > of backend capable features and won't call into > kernel again to get it on reset. This causes all > virtio features getting disabled on newly created > virtqs after device reset, while guest would hold > mismatched view of available features. For e.g., > the guest may still assume tx checksum offload > is available after reset and feature negotiation, > causing frames with bogus (incomplete) checksum > transmitted on the wire. > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices") > Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> Acked-by: Jason Wang <jasowang@redhat.com> > --- > drivers/vdpa/mlx5/net/mlx5_vnet.c | 25 +++++++++++++++---------- > 1 file changed, 15 insertions(+), 10 deletions(-) > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > index b8416c4..7c1f789 100644 > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > @@ -1486,16 +1486,8 @@ static u64 mlx_to_vritio_features(u16 dev_features) > static u64 mlx5_vdpa_get_features(struct vdpa_device *vdev) > { > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev); > - struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev); > - u16 dev_features; > > - dev_features = MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, device_features_bits_mask); > - ndev->mvdev.mlx_features = mlx_to_vritio_features(dev_features); > - if (MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, virtio_version_1_0)) > - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_VERSION_1); > - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM); > - print_features(mvdev, ndev->mvdev.mlx_features, false); > - return ndev->mvdev.mlx_features; > + return mvdev->mlx_features; > } > > static int verify_min_features(struct mlx5_vdpa_dev *mvdev, u64 features) > @@ -1788,7 +1780,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) > clear_virtqueues(ndev); > mlx5_vdpa_destroy_mr(&ndev->mvdev); > ndev->mvdev.status = 0; > - ndev->mvdev.mlx_features = 0; > ++mvdev->generation; > return; > } > @@ -1907,6 +1898,19 @@ static int mlx5_get_vq_irq(struct vdpa_device *vdv, u16 idx) > .free = mlx5_vdpa_free, > }; > > +static void query_virtio_features(struct mlx5_vdpa_net *ndev) > +{ > + struct mlx5_vdpa_dev *mvdev = &ndev->mvdev; > + u16 dev_features; > + > + dev_features = MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, device_features_bits_mask); > + mvdev->mlx_features = mlx_to_vritio_features(dev_features); > + if (MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, virtio_version_1_0)) > + mvdev->mlx_features |= BIT_ULL(VIRTIO_F_VERSION_1); > + mvdev->mlx_features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM); > + print_features(mvdev, mvdev->mlx_features, false); > +} > + > static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu) > { > u16 hw_mtu; > @@ -2005,6 +2009,7 @@ static int mlx5v_probe(struct auxiliary_device *adev, > init_mvqs(ndev); > mutex_init(&ndev->reslock); > config = &ndev->config; > + query_virtio_features(ndev); > err = query_mtu(mdev, &ndev->mtu); > if (err) > goto err_mtu; _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 3/3] vdpa/mlx5: defer clear_virtqueues to until DRIVER_OK 2021-02-10 21:47 ` Si-Wei Liu @ 2021-02-10 21:48 ` Si-Wei Liu -1 siblings, 0 replies; 17+ messages in thread From: Si-Wei Liu @ 2021-02-10 21:48 UTC (permalink / raw) To: mst, jasowang, elic; +Cc: linux-kernel, virtualization, netdev, si-wei.liu While virtq is stopped, get_vq_state() is supposed to be called to get sync'ed with the latest internal avail_index from device. The saved avail_index is used to restate the virtq once device is started. Commit b35ccebe3ef7 introduced the clear_virtqueues() routine to reset the saved avail_index, however, the index gets cleared a bit earlier before get_vq_state() tries to read it. This would cause consistency problems when virtq is restarted, e.g. through a series of link down and link up events. We could defer the clearing of avail_index to until the device is to be started, i.e. until VIRTIO_CONFIG_S_DRIVER_OK is set again in set_status(). Fixes: b35ccebe3ef7 ("vdpa/mlx5: Restore the hardware used index after change map") Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> Acked-by: Jason Wang <jasowang@redhat.com> --- drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c index 7c1f789..ce6aae8 100644 --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c @@ -1777,7 +1777,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) if (!status) { mlx5_vdpa_info(mvdev, "performing device reset\n"); teardown_driver(ndev); - clear_virtqueues(ndev); mlx5_vdpa_destroy_mr(&ndev->mvdev); ndev->mvdev.status = 0; ++mvdev->generation; @@ -1786,6 +1785,7 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) if ((status ^ ndev->mvdev.status) & VIRTIO_CONFIG_S_DRIVER_OK) { if (status & VIRTIO_CONFIG_S_DRIVER_OK) { + clear_virtqueues(ndev); err = setup_driver(ndev); if (err) { mlx5_vdpa_warn(mvdev, "failed to setup driver\n"); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 3/3] vdpa/mlx5: defer clear_virtqueues to until DRIVER_OK @ 2021-02-10 21:48 ` Si-Wei Liu 0 siblings, 0 replies; 17+ messages in thread From: Si-Wei Liu @ 2021-02-10 21:48 UTC (permalink / raw) To: mst, jasowang, elic; +Cc: si-wei.liu, netdev, linux-kernel, virtualization While virtq is stopped, get_vq_state() is supposed to be called to get sync'ed with the latest internal avail_index from device. The saved avail_index is used to restate the virtq once device is started. Commit b35ccebe3ef7 introduced the clear_virtqueues() routine to reset the saved avail_index, however, the index gets cleared a bit earlier before get_vq_state() tries to read it. This would cause consistency problems when virtq is restarted, e.g. through a series of link down and link up events. We could defer the clearing of avail_index to until the device is to be started, i.e. until VIRTIO_CONFIG_S_DRIVER_OK is set again in set_status(). Fixes: b35ccebe3ef7 ("vdpa/mlx5: Restore the hardware used index after change map") Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> Acked-by: Jason Wang <jasowang@redhat.com> --- drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c index 7c1f789..ce6aae8 100644 --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c @@ -1777,7 +1777,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) if (!status) { mlx5_vdpa_info(mvdev, "performing device reset\n"); teardown_driver(ndev); - clear_virtqueues(ndev); mlx5_vdpa_destroy_mr(&ndev->mvdev); ndev->mvdev.status = 0; ++mvdev->generation; @@ -1786,6 +1785,7 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) if ((status ^ ndev->mvdev.status) & VIRTIO_CONFIG_S_DRIVER_OK) { if (status & VIRTIO_CONFIG_S_DRIVER_OK) { + clear_virtqueues(ndev); err = setup_driver(ndev); if (err) { mlx5_vdpa_warn(mvdev, "failed to setup driver\n"); -- 1.8.3.1 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] vdpa/mlx5: defer clear_virtqueues to until DRIVER_OK 2021-02-10 21:48 ` Si-Wei Liu (?) @ 2021-02-11 7:33 ` Eli Cohen 2021-02-16 15:21 ` Eli Cohen -1 siblings, 1 reply; 17+ messages in thread From: Eli Cohen @ 2021-02-11 7:33 UTC (permalink / raw) To: Si-Wei Liu; +Cc: mst, jasowang, linux-kernel, virtualization, netdev On Wed, Feb 10, 2021 at 01:48:00PM -0800, Si-Wei Liu wrote: > While virtq is stopped, get_vq_state() is supposed to > be called to get sync'ed with the latest internal > avail_index from device. The saved avail_index is used > to restate the virtq once device is started. Commit > b35ccebe3ef7 introduced the clear_virtqueues() routine > to reset the saved avail_index, however, the index > gets cleared a bit earlier before get_vq_state() tries > to read it. This would cause consistency problems when > virtq is restarted, e.g. through a series of link down > and link up events. We could defer the clearing of > avail_index to until the device is to be started, > i.e. until VIRTIO_CONFIG_S_DRIVER_OK is set again in > set_status(). > > Fixes: b35ccebe3ef7 ("vdpa/mlx5: Restore the hardware used index after change map") > Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> > Acked-by: Jason Wang <jasowang@redhat.com> Acked-by: Eli Cohen <elic@nvidia.com> > --- > drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > index 7c1f789..ce6aae8 100644 > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > @@ -1777,7 +1777,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) > if (!status) { > mlx5_vdpa_info(mvdev, "performing device reset\n"); > teardown_driver(ndev); > - clear_virtqueues(ndev); > mlx5_vdpa_destroy_mr(&ndev->mvdev); > ndev->mvdev.status = 0; > ++mvdev->generation; > @@ -1786,6 +1785,7 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) > > if ((status ^ ndev->mvdev.status) & VIRTIO_CONFIG_S_DRIVER_OK) { > if (status & VIRTIO_CONFIG_S_DRIVER_OK) { > + clear_virtqueues(ndev); > err = setup_driver(ndev); > if (err) { > mlx5_vdpa_warn(mvdev, "failed to setup driver\n"); > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] vdpa/mlx5: defer clear_virtqueues to until DRIVER_OK 2021-02-11 7:33 ` Eli Cohen @ 2021-02-16 15:21 ` Eli Cohen 2021-02-17 21:55 ` Si-Wei Liu 0 siblings, 1 reply; 17+ messages in thread From: Eli Cohen @ 2021-02-16 15:21 UTC (permalink / raw) To: Si-Wei Liu; +Cc: mst, jasowang, linux-kernel, virtualization, netdev On Thu, Feb 11, 2021 at 09:33:14AM +0200, Eli Cohen wrote: > On Wed, Feb 10, 2021 at 01:48:00PM -0800, Si-Wei Liu wrote: > > While virtq is stopped, get_vq_state() is supposed to > > be called to get sync'ed with the latest internal > > avail_index from device. The saved avail_index is used > > to restate the virtq once device is started. Commit > > b35ccebe3ef7 introduced the clear_virtqueues() routine > > to reset the saved avail_index, however, the index > > gets cleared a bit earlier before get_vq_state() tries > > to read it. This would cause consistency problems when > > virtq is restarted, e.g. through a series of link down > > and link up events. We could defer the clearing of > > avail_index to until the device is to be started, > > i.e. until VIRTIO_CONFIG_S_DRIVER_OK is set again in > > set_status(). > > > > Fixes: b35ccebe3ef7 ("vdpa/mlx5: Restore the hardware used index after change map") > > Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> > > Acked-by: Jason Wang <jasowang@redhat.com> > > Acked-by: Eli Cohen <elic@nvidia.com> > I take it back. I think we don't need to clear the indexes at all. In case we need to restore indexes we'll get the right values through set_vq_state(). If we suspend the virtqueue due to VM being suspended, qemu will query first and will provide the the queried value. In case of VM reboot, it will provide 0 in set_vq_state(). I am sending a patch that addresses both reboot and suspend. > > --- > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > index 7c1f789..ce6aae8 100644 > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > @@ -1777,7 +1777,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) > > if (!status) { > > mlx5_vdpa_info(mvdev, "performing device reset\n"); > > teardown_driver(ndev); > > - clear_virtqueues(ndev); > > mlx5_vdpa_destroy_mr(&ndev->mvdev); > > ndev->mvdev.status = 0; > > ++mvdev->generation; > > @@ -1786,6 +1785,7 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) > > > > if ((status ^ ndev->mvdev.status) & VIRTIO_CONFIG_S_DRIVER_OK) { > > if (status & VIRTIO_CONFIG_S_DRIVER_OK) { > > + clear_virtqueues(ndev); > > err = setup_driver(ndev); > > if (err) { > > mlx5_vdpa_warn(mvdev, "failed to setup driver\n"); > > -- > > 1.8.3.1 > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] vdpa/mlx5: defer clear_virtqueues to until DRIVER_OK 2021-02-16 15:21 ` Eli Cohen @ 2021-02-17 21:55 ` Si-Wei Liu 0 siblings, 0 replies; 17+ messages in thread From: Si-Wei Liu @ 2021-02-17 21:55 UTC (permalink / raw) To: Eli Cohen, Michael S. Tsirkin Cc: jasowang, linux-kernel, virtualization, netdev On 2/16/2021 7:21 AM, Eli Cohen wrote: > On Thu, Feb 11, 2021 at 09:33:14AM +0200, Eli Cohen wrote: >> On Wed, Feb 10, 2021 at 01:48:00PM -0800, Si-Wei Liu wrote: >>> While virtq is stopped, get_vq_state() is supposed to >>> be called to get sync'ed with the latest internal >>> avail_index from device. The saved avail_index is used >>> to restate the virtq once device is started. Commit >>> b35ccebe3ef7 introduced the clear_virtqueues() routine >>> to reset the saved avail_index, however, the index >>> gets cleared a bit earlier before get_vq_state() tries >>> to read it. This would cause consistency problems when >>> virtq is restarted, e.g. through a series of link down >>> and link up events. We could defer the clearing of >>> avail_index to until the device is to be started, >>> i.e. until VIRTIO_CONFIG_S_DRIVER_OK is set again in >>> set_status(). >>> >>> Fixes: b35ccebe3ef7 ("vdpa/mlx5: Restore the hardware used index after change map") >>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> >>> Acked-by: Jason Wang <jasowang@redhat.com> >> Acked-by: Eli Cohen <elic@nvidia.com> >> > I take it back. I think we don't need to clear the indexes at all. In > case we need to restore indexes we'll get the right values through > set_vq_state(). If we suspend the virtqueue due to VM being suspended, > qemu will query first and will provide the the queried value. In case of > VM reboot, it will provide 0 in set_vq_state(). > > I am sending a patch that addresses both reboot and suspend. With set_vq_state() repurposed to restoring used_index I'm fine with this approach. Do I have to repost a v3 of this series while dropping the 3rd patch? -Siwei > >>> --- >>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c >>> index 7c1f789..ce6aae8 100644 >>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c >>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c >>> @@ -1777,7 +1777,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) >>> if (!status) { >>> mlx5_vdpa_info(mvdev, "performing device reset\n"); >>> teardown_driver(ndev); >>> - clear_virtqueues(ndev); >>> mlx5_vdpa_destroy_mr(&ndev->mvdev); >>> ndev->mvdev.status = 0; >>> ++mvdev->generation; >>> @@ -1786,6 +1785,7 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) >>> >>> if ((status ^ ndev->mvdev.status) & VIRTIO_CONFIG_S_DRIVER_OK) { >>> if (status & VIRTIO_CONFIG_S_DRIVER_OK) { >>> + clear_virtqueues(ndev); >>> err = setup_driver(ndev); >>> if (err) { >>> mlx5_vdpa_warn(mvdev, "failed to setup driver\n"); >>> -- >>> 1.8.3.1 >>> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] vdpa/mlx5: defer clear_virtqueues to until DRIVER_OK @ 2021-02-17 21:55 ` Si-Wei Liu 0 siblings, 0 replies; 17+ messages in thread From: Si-Wei Liu @ 2021-02-17 21:55 UTC (permalink / raw) To: Eli Cohen, Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization On 2/16/2021 7:21 AM, Eli Cohen wrote: > On Thu, Feb 11, 2021 at 09:33:14AM +0200, Eli Cohen wrote: >> On Wed, Feb 10, 2021 at 01:48:00PM -0800, Si-Wei Liu wrote: >>> While virtq is stopped, get_vq_state() is supposed to >>> be called to get sync'ed with the latest internal >>> avail_index from device. The saved avail_index is used >>> to restate the virtq once device is started. Commit >>> b35ccebe3ef7 introduced the clear_virtqueues() routine >>> to reset the saved avail_index, however, the index >>> gets cleared a bit earlier before get_vq_state() tries >>> to read it. This would cause consistency problems when >>> virtq is restarted, e.g. through a series of link down >>> and link up events. We could defer the clearing of >>> avail_index to until the device is to be started, >>> i.e. until VIRTIO_CONFIG_S_DRIVER_OK is set again in >>> set_status(). >>> >>> Fixes: b35ccebe3ef7 ("vdpa/mlx5: Restore the hardware used index after change map") >>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> >>> Acked-by: Jason Wang <jasowang@redhat.com> >> Acked-by: Eli Cohen <elic@nvidia.com> >> > I take it back. I think we don't need to clear the indexes at all. In > case we need to restore indexes we'll get the right values through > set_vq_state(). If we suspend the virtqueue due to VM being suspended, > qemu will query first and will provide the the queried value. In case of > VM reboot, it will provide 0 in set_vq_state(). > > I am sending a patch that addresses both reboot and suspend. With set_vq_state() repurposed to restoring used_index I'm fine with this approach. Do I have to repost a v3 of this series while dropping the 3rd patch? -Siwei > >>> --- >>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c >>> index 7c1f789..ce6aae8 100644 >>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c >>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c >>> @@ -1777,7 +1777,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) >>> if (!status) { >>> mlx5_vdpa_info(mvdev, "performing device reset\n"); >>> teardown_driver(ndev); >>> - clear_virtqueues(ndev); >>> mlx5_vdpa_destroy_mr(&ndev->mvdev); >>> ndev->mvdev.status = 0; >>> ++mvdev->generation; >>> @@ -1786,6 +1785,7 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status) >>> >>> if ((status ^ ndev->mvdev.status) & VIRTIO_CONFIG_S_DRIVER_OK) { >>> if (status & VIRTIO_CONFIG_S_DRIVER_OK) { >>> + clear_virtqueues(ndev); >>> err = setup_driver(ndev); >>> if (err) { >>> mlx5_vdpa_warn(mvdev, "failed to setup driver\n"); >>> -- >>> 1.8.3.1 >>> _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] vdpa/mlx5: defer clear_virtqueues to until DRIVER_OK 2021-02-17 21:55 ` Si-Wei Liu @ 2021-02-18 6:37 ` Jason Wang -1 siblings, 0 replies; 17+ messages in thread From: Jason Wang @ 2021-02-18 6:37 UTC (permalink / raw) To: Si-Wei Liu, Eli Cohen, Michael S. Tsirkin Cc: linux-kernel, virtualization, netdev On 2021/2/18 上午5:55, Si-Wei Liu wrote: > > > On 2/16/2021 7:21 AM, Eli Cohen wrote: >> On Thu, Feb 11, 2021 at 09:33:14AM +0200, Eli Cohen wrote: >>> On Wed, Feb 10, 2021 at 01:48:00PM -0800, Si-Wei Liu wrote: >>>> While virtq is stopped, get_vq_state() is supposed to >>>> be called to get sync'ed with the latest internal >>>> avail_index from device. The saved avail_index is used >>>> to restate the virtq once device is started. Commit >>>> b35ccebe3ef7 introduced the clear_virtqueues() routine >>>> to reset the saved avail_index, however, the index >>>> gets cleared a bit earlier before get_vq_state() tries >>>> to read it. This would cause consistency problems when >>>> virtq is restarted, e.g. through a series of link down >>>> and link up events. We could defer the clearing of >>>> avail_index to until the device is to be started, >>>> i.e. until VIRTIO_CONFIG_S_DRIVER_OK is set again in >>>> set_status(). >>>> >>>> Fixes: b35ccebe3ef7 ("vdpa/mlx5: Restore the hardware used index >>>> after change map") >>>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> >>>> Acked-by: Jason Wang <jasowang@redhat.com> >>> Acked-by: Eli Cohen <elic@nvidia.com> >>> >> I take it back. I think we don't need to clear the indexes at all. In >> case we need to restore indexes we'll get the right values through >> set_vq_state(). If we suspend the virtqueue due to VM being suspended, >> qemu will query first and will provide the the queried value. In case of >> VM reboot, it will provide 0 in set_vq_state(). >> >> I am sending a patch that addresses both reboot and suspend. > With set_vq_state() repurposed to restoring used_index I'm fine with > this approach. > > Do I have to repost a v3 of this series while dropping the 3rd patch? > > -Siwei Yes, please. Thanks ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] vdpa/mlx5: defer clear_virtqueues to until DRIVER_OK @ 2021-02-18 6:37 ` Jason Wang 0 siblings, 0 replies; 17+ messages in thread From: Jason Wang @ 2021-02-18 6:37 UTC (permalink / raw) To: Si-Wei Liu, Eli Cohen, Michael S. Tsirkin Cc: netdev, linux-kernel, virtualization On 2021/2/18 上午5:55, Si-Wei Liu wrote: > > > On 2/16/2021 7:21 AM, Eli Cohen wrote: >> On Thu, Feb 11, 2021 at 09:33:14AM +0200, Eli Cohen wrote: >>> On Wed, Feb 10, 2021 at 01:48:00PM -0800, Si-Wei Liu wrote: >>>> While virtq is stopped, get_vq_state() is supposed to >>>> be called to get sync'ed with the latest internal >>>> avail_index from device. The saved avail_index is used >>>> to restate the virtq once device is started. Commit >>>> b35ccebe3ef7 introduced the clear_virtqueues() routine >>>> to reset the saved avail_index, however, the index >>>> gets cleared a bit earlier before get_vq_state() tries >>>> to read it. This would cause consistency problems when >>>> virtq is restarted, e.g. through a series of link down >>>> and link up events. We could defer the clearing of >>>> avail_index to until the device is to be started, >>>> i.e. until VIRTIO_CONFIG_S_DRIVER_OK is set again in >>>> set_status(). >>>> >>>> Fixes: b35ccebe3ef7 ("vdpa/mlx5: Restore the hardware used index >>>> after change map") >>>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> >>>> Acked-by: Jason Wang <jasowang@redhat.com> >>> Acked-by: Eli Cohen <elic@nvidia.com> >>> >> I take it back. I think we don't need to clear the indexes at all. In >> case we need to restore indexes we'll get the right values through >> set_vq_state(). If we suspend the virtqueue due to VM being suspended, >> qemu will query first and will provide the the queried value. In case of >> VM reboot, it will provide 0 in set_vq_state(). >> >> I am sending a patch that addresses both reboot and suspend. > With set_vq_state() repurposed to restoring used_index I'm fine with > this approach. > > Do I have to repost a v3 of this series while dropping the 3rd patch? > > -Siwei Yes, please. Thanks _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-02-18 7:27 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-10 21:47 [PATCH v2 0/3] mlx5_vdpa bug fixes Si-Wei Liu 2021-02-10 21:47 ` Si-Wei Liu 2021-02-10 21:47 ` [PATCH v2 1/3] vdpa/mlx5: should exclude header length and fcs from mtu Si-Wei Liu 2021-02-10 21:47 ` Si-Wei Liu 2021-02-10 21:47 ` [PATCH v2 2/3] vdpa/mlx5: fix feature negotiation across device reset Si-Wei Liu 2021-02-10 21:47 ` Si-Wei Liu 2021-02-11 7:33 ` Eli Cohen 2021-02-18 6:36 ` Jason Wang 2021-02-18 6:36 ` Jason Wang 2021-02-10 21:48 ` [PATCH v2 3/3] vdpa/mlx5: defer clear_virtqueues to until DRIVER_OK Si-Wei Liu 2021-02-10 21:48 ` Si-Wei Liu 2021-02-11 7:33 ` Eli Cohen 2021-02-16 15:21 ` Eli Cohen 2021-02-17 21:55 ` Si-Wei Liu 2021-02-17 21:55 ` Si-Wei Liu 2021-02-18 6:37 ` Jason Wang 2021-02-18 6:37 ` Jason Wang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.