| Message ID | 1612614564-4220-2-git-send-email-si-wei.liu@oracle.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/3] mlx5_vdpa: should exclude header length and fcs from mtu | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 2021/2/6 下午8:29, 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 is not the behavior of Qemu but it's valid. > 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. > > 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 | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > index b8416c4..aa6f8cd 100644 > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > @@ -1788,7 +1788,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; > }
On Sat, Feb 06, 2021 at 04:29:23AM -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. > > Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> > --- > drivers/vdpa/mlx5/net/mlx5_vnet.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > index b8416c4..aa6f8cd 100644 > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > @@ -1788,7 +1788,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; > } Since we assume that device capabilities don't change, I think I would get the features through a call done in mlx5v_probe after the netdev object is created and change mlx5_vdpa_get_features() to just return ndev->mvdev.mlx_features. Did you actually see this issue in action? If you did, can you share with us how you trigerred this? > -- > 1.8.3.1 >
On 2/7/2021 9:35 PM, Eli Cohen wrote: > On Sat, Feb 06, 2021 at 04:29:23AM -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. >> >> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> >> --- >> drivers/vdpa/mlx5/net/mlx5_vnet.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c >> index b8416c4..aa6f8cd 100644 >> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c >> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c >> @@ -1788,7 +1788,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; >> } > Since we assume that device capabilities don't change, I think I would > get the features through a call done in mlx5v_probe after the netdev > object is created and change mlx5_vdpa_get_features() to just return > ndev->mvdev.mlx_features. Yep, it makes sense. Will post a revised patch. If vdpa tool allows reconfiguration post probing, the code has to be reconciled then. > > Did you actually see this issue in action? If you did, can you share > with us how you trigerred this? Issue is indeed seen in action. The mismatched tx-checksum offload as described in the commit message was one of such examples. You would need a guest reboot though (triggering device reset via the .set_status ops and zero'ed mlx_features was loaded to deduce new actual_features for creating the h/w virtq object) for repro. -Siwei > >> -- >> 1.8.3.1 >>
On Mon, Feb 08, 2021 at 05:20:11PM -0800, Si-Wei Liu wrote: > > > On 2/7/2021 9:35 PM, Eli Cohen wrote: > > On Sat, Feb 06, 2021 at 04:29:23AM -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. > > > > > > Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> > > > --- > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 1 - > > > 1 file changed, 1 deletion(-) > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > index b8416c4..aa6f8cd 100644 > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > @@ -1788,7 +1788,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; > > > } > > Since we assume that device capabilities don't change, I think I would > > get the features through a call done in mlx5v_probe after the netdev > > object is created and change mlx5_vdpa_get_features() to just return > > ndev->mvdev.mlx_features. > Yep, it makes sense. Will post a revised patch. So I'm waiting for v2 of this patchset. Please make sure to post a cover letter with an overall description. > If vdpa tool allows > reconfiguration post probing, the code has to be reconciled then. > > > > > Did you actually see this issue in action? If you did, can you share > > with us how you trigerred this? > Issue is indeed seen in action. The mismatched tx-checksum offload as > described in the commit message was one of such examples. You would need a > guest reboot though (triggering device reset via the .set_status ops and > zero'ed mlx_features was loaded to deduce new actual_features for creating > the h/w virtq object) for repro. > > -Siwei > > > > > -- > > > 1.8.3.1 > > >
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c index b8416c4..aa6f8cd 100644 --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c @@ -1788,7 +1788,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; }
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. Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> --- drivers/vdpa/mlx5/net/mlx5_vnet.c | 1 - 1 file changed, 1 deletion(-)