On Thu, Aug 17, 2023 at 2:47 PM Hawkins Jiawei <yin31...@gmail.com> wrote:
>
> On 2023/8/17 18:18, Eugenio Perez Martin wrote:
> > On Fri, Jul 7, 2023 at 5:27 PM Hawkins Jiawei <yin31...@gmail.com> 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 <yin31...@gmail.com>
> >> ---
> >> v3:
> >> - return early if mismatch the condition suggested by Eugenio
> >>
> >> v2:
> >> https://lore.kernel.org/all/2f2560f749186c0eb1055f9926f464587e419eeb.1688051252.git.yin31...@gmail.com/
> >> - use iovec suggested by Eugenio
> >> - avoid sending CVQ command in default state
> >>
> >> v1:
> >> https://lore.kernel.org/all/00f72fe154a882fd6dc15bc39e3a1ac63f9dadce.1687402580.git.yin31...@gmail.com/
> >>
> >> 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 <epere...@redhat.com>
> >
> >> + 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
> >>
> >
>
