[v3,2/7] vdpa: Restore MAC address filtering state

Commit Message

Hawkins Jiawei July 7, 2023, 3:27 p.m. UTC

This patch refactors vhost_vdpa_net_load_mac() to
restore the MAC address filtering state at device's startup.

Signed-off-by: Hawkins Jiawei <[e-mail]>
---
v3:
  - return early if mismatch the condition suggested by Eugenio

v2: https://lore.kernel.org/all/[e-mail]/
  - use iovec suggested by Eugenio
  - avoid sending CVQ command in default state

v1: https://lore.kernel.org/all/[e-mail]/

 net/vhost-vdpa.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

Comments

Eugenio Perez Martin Aug. 17, 2023, 10:18 a.m. UTC | #1

On Fri, Jul 7, 2023 at 5:27 PM Hawkins Jiawei <[e-mail]> wrote:
>
> This patch refactors vhost_vdpa_net_load_mac() to
> restore the MAC address filtering state at device's startup.
>
> Signed-off-by: Hawkins Jiawei <[e-mail]>
> ---
> v3:
>   - return early if mismatch the condition suggested by Eugenio
>
> v2: https://lore.kernel.org/all/[e-mail]/
>   - use iovec suggested by Eugenio
>   - avoid sending CVQ command in default state
>
> v1: https://lore.kernel.org/all/[e-mail]/
>
>  net/vhost-vdpa.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 31ef6ad6ec..7189ccafaf 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -660,6 +660,58 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
>          }
>      }
>
> +    /*
> +     * According to VirtIO standard, "The device MUST have an
> +     * empty MAC filtering table on reset.".
> +     *
> +     * Therefore, there is no need to send this CVQ command if the
> +     * driver also sets an empty MAC filter table, which aligns with
> +     * the device's defaults.
> +     *
> +     * Note that the device's defaults can mismatch the driver's
> +     * configuration only at live migration.
> +     */
> +    if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX) ||
> +        n->mac_table.in_use == 0) {
> +        return 0;
> +    }
> +
> +    uint32_t uni_entries = n->mac_table.first_multi,

QEMU coding style prefers declarations at the beginning of the code
block. Previous uses of these variable names would need to be
refactored to met this rule.

Apart from that,

Acked-by: Eugenio Pérez <[e-mail]>

> +             uni_macs_size = uni_entries * ETH_ALEN,
> +             mul_entries = n->mac_table.in_use - uni_entries,
> +             mul_macs_size = mul_entries * ETH_ALEN;
> +    struct virtio_net_ctrl_mac uni = {
> +        .entries = cpu_to_le32(uni_entries),
> +    };
> +    struct virtio_net_ctrl_mac mul = {
> +        .entries = cpu_to_le32(mul_entries),
> +    };
> +    const struct iovec data[] = {
> +        {
> +            .iov_base = &uni,
> +            .iov_len = sizeof(uni),
> +        }, {
> +            .iov_base = n->mac_table.macs,
> +            .iov_len = uni_macs_size,
> +        }, {
> +            .iov_base = &mul,
> +            .iov_len = sizeof(mul),
> +        }, {
> +            .iov_base = &n->mac_table.macs[uni_macs_size],
> +            .iov_len = mul_macs_size,
> +        },
> +    };
> +    ssize_t dev_written = vhost_vdpa_net_load_cmd(s,
> +                                VIRTIO_NET_CTRL_MAC,
> +                                VIRTIO_NET_CTRL_MAC_TABLE_SET,
> +                                data, ARRAY_SIZE(data));
> +    if (unlikely(dev_written < 0)) {
> +        return dev_written;
> +    }
> +    if (*s->status != VIRTIO_NET_OK) {
> +        return -EIO;
> +    }
> +
>      return 0;
>  }
>
> --
> 2.25.1
>

Hawkins Jiawei Aug. 17, 2023, 12:46 p.m. UTC | #2

On 2023/8/17 18:18, Eugenio Perez Martin wrote:
> On Fri, Jul 7, 2023 at 5:27 PM Hawkins Jiawei <[e-mail]> wrote:
>>
>> This patch refactors vhost_vdpa_net_load_mac() to
>> restore the MAC address filtering state at device's startup.
>>
>> Signed-off-by: Hawkins Jiawei <[e-mail]>
>> ---
>> v3:
>>    - return early if mismatch the condition suggested by Eugenio
>>
>> v2: https://lore.kernel.org/all/[e-mail]/
>>    - use iovec suggested by Eugenio
>>    - avoid sending CVQ command in default state
>>
>> v1: https://lore.kernel.org/all/[e-mail]/
>>
>>   net/vhost-vdpa.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 52 insertions(+)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index 31ef6ad6ec..7189ccafaf 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -660,6 +660,58 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
>>           }
>>       }
>>
>> +    /*
>> +     * According to VirtIO standard, "The device MUST have an
>> +     * empty MAC filtering table on reset.".
>> +     *
>> +     * Therefore, there is no need to send this CVQ command if the
>> +     * driver also sets an empty MAC filter table, which aligns with
>> +     * the device's defaults.
>> +     *
>> +     * Note that the device's defaults can mismatch the driver's
>> +     * configuration only at live migration.
>> +     */
>> +    if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX) ||
>> +        n->mac_table.in_use == 0) {
>> +        return 0;
>> +    }
>> +
>> +    uint32_t uni_entries = n->mac_table.first_multi,
>
> QEMU coding style prefers declarations at the beginning of the code
> block. Previous uses of these variable names would need to be
> refactored to met this rule.

Hi Eugenio,

Thanks for the detailed explanation.

Since this patch series has already been merged into master, I will
submit a separate patch to correct this problem.

I will take care of this problem in the future.

Thanks!


>
> Apart from that,
>
> Acked-by: Eugenio Pérez <[e-mail]>
>
>> +             uni_macs_size = uni_entries * ETH_ALEN,
>> +             mul_entries = n->mac_table.in_use - uni_entries,
>> +             mul_macs_size = mul_entries * ETH_ALEN;
>> +    struct virtio_net_ctrl_mac uni = {
>> +        .entries = cpu_to_le32(uni_entries),
>> +    };
>> +    struct virtio_net_ctrl_mac mul = {
>> +        .entries = cpu_to_le32(mul_entries),
>> +    };
>> +    const struct iovec data[] = {
>> +        {
>> +            .iov_base = &uni,
>> +            .iov_len = sizeof(uni),
>> +        }, {
>> +            .iov_base = n->mac_table.macs,
>> +            .iov_len = uni_macs_size,
>> +        }, {
>> +            .iov_base = &mul,
>> +            .iov_len = sizeof(mul),
>> +        }, {
>> +            .iov_base = &n->mac_table.macs[uni_macs_size],
>> +            .iov_len = mul_macs_size,
>> +        },
>> +    };
>> +    ssize_t dev_written = vhost_vdpa_net_load_cmd(s,
>> +                                VIRTIO_NET_CTRL_MAC,
>> +                                VIRTIO_NET_CTRL_MAC_TABLE_SET,
>> +                                data, ARRAY_SIZE(data));
>> +    if (unlikely(dev_written < 0)) {
>> +        return dev_written;
>> +    }
>> +    if (*s->status != VIRTIO_NET_OK) {
>> +        return -EIO;
>> +    }
>> +
>>       return 0;
>>   }
>>
>> --
>> 2.25.1
>>
>

Eugenio Perez Martin Aug. 17, 2023, 2:24 p.m. UTC | #3

On Thu, Aug 17, 2023 at 2:47 PM Hawkins Jiawei <[e-mail]> wrote:
>
> On 2023/8/17 18:18, Eugenio Perez Martin wrote:
> > On Fri, Jul 7, 2023 at 5:27 PM Hawkins Jiawei <[e-mail]> wrote:
> >>
> >> This patch refactors vhost_vdpa_net_load_mac() to
> >> restore the MAC address filtering state at device's startup.
> >>
> >> Signed-off-by: Hawkins Jiawei <[e-mail]>
> >> ---
> >> v3:
> >>    - return early if mismatch the condition suggested by Eugenio
> >>
> >> v2: https://lore.kernel.org/all/[e-mail]/
> >>    - use iovec suggested by Eugenio
> >>    - avoid sending CVQ command in default state
> >>
> >> v1: https://lore.kernel.org/all/[e-mail]/
> >>
> >>   net/vhost-vdpa.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 52 insertions(+)
> >>
> >> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> >> index 31ef6ad6ec..7189ccafaf 100644
> >> --- a/net/vhost-vdpa.c
> >> +++ b/net/vhost-vdpa.c
> >> @@ -660,6 +660,58 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
> >>           }
> >>       }
> >>
> >> +    /*
> >> +     * According to VirtIO standard, "The device MUST have an
> >> +     * empty MAC filtering table on reset.".
> >> +     *
> >> +     * Therefore, there is no need to send this CVQ command if the
> >> +     * driver also sets an empty MAC filter table, which aligns with
> >> +     * the device's defaults.
> >> +     *
> >> +     * Note that the device's defaults can mismatch the driver's
> >> +     * configuration only at live migration.
> >> +     */
> >> +    if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX) ||
> >> +        n->mac_table.in_use == 0) {
> >> +        return 0;
> >> +    }
> >> +
> >> +    uint32_t uni_entries = n->mac_table.first_multi,
> >
> > QEMU coding style prefers declarations at the beginning of the code
> > block. Previous uses of these variable names would need to be
> > refactored to met this rule.
>
> Hi Eugenio,
>
> Thanks for the detailed explanation.
>
> Since this patch series has already been merged into master, I will
> submit a separate patch to correct this problem.
>
> I will take care of this problem in the future.
>

If the maintainer is ok with this, I'm totally ok with leaving the
code as it is right now.

Thanks!

> Thanks!
>
>
> >
> > Apart from that,
> >
> > Acked-by: Eugenio Pérez <[e-mail]>
> >
> >> +             uni_macs_size = uni_entries * ETH_ALEN,
> >> +             mul_entries = n->mac_table.in_use - uni_entries,
> >> +             mul_macs_size = mul_entries * ETH_ALEN;
> >> +    struct virtio_net_ctrl_mac uni = {
> >> +        .entries = cpu_to_le32(uni_entries),
> >> +    };
> >> +    struct virtio_net_ctrl_mac mul = {
> >> +        .entries = cpu_to_le32(mul_entries),
> >> +    };
> >> +    const struct iovec data[] = {
> >> +        {
> >> +            .iov_base = &uni,
> >> +            .iov_len = sizeof(uni),
> >> +        }, {
> >> +            .iov_base = n->mac_table.macs,
> >> +            .iov_len = uni_macs_size,
> >> +        }, {
> >> +            .iov_base = &mul,
> >> +            .iov_len = sizeof(mul),
> >> +        }, {
> >> +            .iov_base = &n->mac_table.macs[uni_macs_size],
> >> +            .iov_len = mul_macs_size,
> >> +        },
> >> +    };
> >> +    ssize_t dev_written = vhost_vdpa_net_load_cmd(s,
> >> +                                VIRTIO_NET_CTRL_MAC,
> >> +                                VIRTIO_NET_CTRL_MAC_TABLE_SET,
> >> +                                data, ARRAY_SIZE(data));
> >> +    if (unlikely(dev_written < 0)) {
> >> +        return dev_written;
> >> +    }
> >> +    if (*s->status != VIRTIO_NET_OK) {
> >> +        return -EIO;
> >> +    }
> >> +
> >>       return 0;
> >>   }
> >>
> >> --
> >> 2.25.1
> >>
> >
>

Patch

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 31ef6ad6ec..7189ccafaf 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -660,6 +660,58 @@  static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
         }
     }
 
+    /*
+     * According to VirtIO standard, "The device MUST have an
+     * empty MAC filtering table on reset.".
+     *
+     * Therefore, there is no need to send this CVQ command if the
+     * driver also sets an empty MAC filter table, which aligns with
+     * the device's defaults.
+     *
+     * Note that the device's defaults can mismatch the driver's
+     * configuration only at live migration.
+     */
+    if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX) ||
+        n->mac_table.in_use == 0) {
+        return 0;
+    }
+
+    uint32_t uni_entries = n->mac_table.first_multi,
+             uni_macs_size = uni_entries * ETH_ALEN,
+             mul_entries = n->mac_table.in_use - uni_entries,
+             mul_macs_size = mul_entries * ETH_ALEN;
+    struct virtio_net_ctrl_mac uni = {
+        .entries = cpu_to_le32(uni_entries),
+    };
+    struct virtio_net_ctrl_mac mul = {
+        .entries = cpu_to_le32(mul_entries),
+    };
+    const struct iovec data[] = {
+        {
+            .iov_base = &uni,
+            .iov_len = sizeof(uni),
+        }, {
+            .iov_base = n->mac_table.macs,
+            .iov_len = uni_macs_size,
+        }, {
+            .iov_base = &mul,
+            .iov_len = sizeof(mul),
+        }, {
+            .iov_base = &n->mac_table.macs[uni_macs_size],
+            .iov_len = mul_macs_size,
+        },
+    };
+    ssize_t dev_written = vhost_vdpa_net_load_cmd(s,
+                                VIRTIO_NET_CTRL_MAC,
+                                VIRTIO_NET_CTRL_MAC_TABLE_SET,
+                                data, ARRAY_SIZE(data));
+    if (unlikely(dev_written < 0)) {
+        return dev_written;
+    }
+    if (*s->status != VIRTIO_NET_OK) {
+        return -EIO;
+    }
+
     return 0;
 }