From: Si-Wei Liu <si-wei.liu@oracle.com> To: Eli Cohen <elic@nvidia.com> Cc: mst@redhat.com, jasowang@redhat.com, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, netdev@vger.kernel.org Subject: Re: [PATCH 2/3] mlx5_vdpa: fix feature negotiation across device reset Date: Mon, 8 Feb 2021 17:20:11 -0800 [thread overview] Message-ID: <061486d5-6235-731b-d036-f5d5e9fac22e@oracle.com> (raw) In-Reply-To: <20210208053500.GA137517@mtl-vdi-166.wap.labs.mlnx> 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 >>
WARNING: multiple messages have this Message-ID (diff)
From: Si-Wei Liu <si-wei.liu@oracle.com> To: Eli Cohen <elic@nvidia.com> Cc: netdev@vger.kernel.org, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, mst@redhat.com Subject: Re: [PATCH 2/3] mlx5_vdpa: fix feature negotiation across device reset Date: Mon, 8 Feb 2021 17:20:11 -0800 [thread overview] Message-ID: <061486d5-6235-731b-d036-f5d5e9fac22e@oracle.com> (raw) In-Reply-To: <20210208053500.GA137517@mtl-vdi-166.wap.labs.mlnx> 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 >> _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-02-09 1:22 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-02-06 12:29 [PATCH 1/3] mlx5_vdpa: should exclude header length and fcs from mtu Si-Wei Liu 2021-02-06 12:29 ` Si-Wei Liu 2021-02-06 12:29 ` [PATCH 2/3] mlx5_vdpa: fix feature negotiation across device reset Si-Wei Liu 2021-02-06 12:29 ` Si-Wei Liu 2021-02-08 4:37 ` Jason Wang 2021-02-08 4:37 ` Jason Wang 2021-02-08 5:35 ` Eli Cohen 2021-02-09 1:20 ` Si-Wei Liu [this message] 2021-02-09 1:20 ` Si-Wei Liu 2021-02-10 12:28 ` Michael S. Tsirkin 2021-02-10 12:28 ` Michael S. Tsirkin 2021-02-06 12:29 ` [PATCH 3/3] mlx5_vdpa: defer clear_virtqueues to until DRIVER_OK Si-Wei Liu 2021-02-06 12:29 ` Si-Wei Liu 2021-02-08 4:38 ` Jason Wang 2021-02-08 4:38 ` Jason Wang 2021-02-08 5:48 ` Eli Cohen 2021-02-09 1:40 ` Si-Wei Liu 2021-02-09 1:40 ` Si-Wei Liu 2021-02-09 3:37 ` Jason Wang 2021-02-09 3:37 ` Jason Wang 2021-02-10 0:26 ` Si-Wei Liu 2021-02-10 0:26 ` Si-Wei Liu 2021-02-10 4:00 ` Jason Wang 2021-02-10 4:00 ` Jason Wang 2021-02-08 4:38 ` [PATCH 1/3] mlx5_vdpa: should exclude header length and fcs from mtu Jason Wang 2021-02-08 4:38 ` Jason Wang 2021-02-08 5:35 ` Eli Cohen
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=061486d5-6235-731b-d036-f5d5e9fac22e@oracle.com \ --to=si-wei.liu@oracle.com \ --cc=elic@nvidia.com \ --cc=jasowang@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=mst@redhat.com \ --cc=netdev@vger.kernel.org \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.