On 9/14/2022 2:57 PM, Eugenio Perez Martin wrote:
On Wed, Sep 14, 2022 at 1:33 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
On 9/14/2022 3:20 AM, Jason Wang wrote:
On Fri, Sep 9, 2022 at 4:02 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
On Fri, Sep 9, 2022 at 8:40 AM Jason Wang <jasowang@redhat.com> wrote:
On Fri, Sep 9, 2022 at 2:38 PM Jason Wang <jasowang@redhat.com> wrote:
On Wed, Sep 7, 2022 at 12:36 AM Eugenio Pérez <eperezma@redhat.com> wrote:
To have enabled vlans at device startup may happen in the destination of
a live migration, so this configuration must be restored.
At this moment the code is not accessible, since SVQ refuses to start if
vlan feature is exposed by the device.
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
net/vhost-vdpa.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 44 insertions(+), 2 deletions(-)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 4bc3fd01a8..ecbfd08eb9 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -100,6 +100,8 @@ static const uint64_t vdpa_svq_device_features =
BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
BIT_ULL(VIRTIO_NET_F_STANDBY);
+#define MAX_VLAN (1 << 12) /* Per 802.1Q definition */
+
VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
{
VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
@@ -423,6 +425,47 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
return *s->status != VIRTIO_NET_OK;
}
+static int vhost_vdpa_net_load_single_vlan(VhostVDPAState *s,
+ const VirtIONet *n,
+ uint16_t vid)
+{
+ ssize_t dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_VLAN,
+ VIRTIO_NET_CTRL_VLAN_ADD,
+ &vid, sizeof(vid));
+ if (unlikely(dev_written < 0)) {
+ return dev_written;
+ }
+
+ if (unlikely(*s->status != VIRTIO_NET_OK)) {
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int vhost_vdpa_net_load_vlan(VhostVDPAState *s,
+ const VirtIONet *n)
+{
+ uint64_t features = n->parent_obj.guest_features;
+
+ if (!(features & BIT_ULL(VIRTIO_NET_F_CTRL_VLAN))) {
+ return 0;
+ }
+
+ for (int i = 0; i < MAX_VLAN >> 5; i++) {
+ for (int j = 0; n->vlans[i] && j <= 0x1f; j++) {
+ if (n->vlans[i] & (1U << j)) {
+ int r = vhost_vdpa_net_load_single_vlan(s, n, (i << 5) + j);
This seems to cause a lot of latency if the driver has a lot of vlans.
I wonder if it's simply to let all vlan traffic go by disabling
CTRL_VLAN feature at vDPA layer.
The guest will not be able to recover that vlan configuration.
Spec said it's best effort and we actually don't do this for
vhost-net/user for years and manage to survive.
I thought there's a border line between best effort and do nothing. ;-)
I also think it is worth at least trying to migrate it for sure. For
MAC there may be too many entries, but VLAN should be totally
manageable (and the information is already there, the bitmap is
actually being migrated).
The vlan bitmap is migrated, though you'd still need to add vlan filter
one by one through ctrl vq commands, correct? AFAIK you can add one
filter for one single vlan ID at a time via VIRTIO_NET_CTRL_VLAN_ADD.
I think that the reason this could survive is really software
implementation specific - say if the backend doesn't start with VLAN
filtering disabled (meaning allow all VLAN traffic to pass) then it
would become a problem. This won't be a problem for almost PF NICs but
may be problematic for vDPA e.g. VF backed VDPAs.
I'd say the driver would expect all vlan filters to be cleared after a
reset, isn't it?
If I understand the intended behavior (from QEMU implementation)
correctly, when VIRTIO_NET_F_CTRL_VLAN is negotiated it would start with
all VLANs filtered (meaning only untagged traffic can be received and
traffic with VLAN tag would get dropped). However, if
VIRTIO_NET_F_CTRL_VLAN is not negotiated, all traffic including untagged
and tagged can be received.
The spec doesn't explicitly say anything about that
as far as I see.
Here the spec is totally ruled by the (software artifact of)
implementation rather than what a real device is expected to work with
VLAN rx filters. Are we sure we'd stick to this flawed device
implementation? The guest driver seems to be agnostic with this broken
spec behavior so far, and I am afraid it's an overkill to add another
feature bit or ctrl command to VLAN filter in clean way.