On Wed, Jul 5, 2023 at 3:43 AM Hawkins Jiawei <yin31...@gmail.com> wrote:
>
> On 2023/7/4 22:53, Eugenio Perez Martin wrote:
> > On Thu, Jun 29, 2023 at 5:26 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>
> >> ---
> >> v2:
> >> - 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 | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 51 insertions(+)
> >>
> >> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> >> index 0bd1c7817c..cb45c84c88 100644
> >> --- a/net/vhost-vdpa.c
> >> +++ b/net/vhost-vdpa.c
> >> @@ -665,6 +665,57 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
> >> const VirtIONet *n)
> >> }
> >> }
> >>
> >> + if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
> >> + if (n->mac_table.in_use != 0) {
> >
> > This may be just style nitpicking, but I find it more clear to return
> > early if conditions are not met and then send the CVQ command.
>
> Yes, this makes code more clear to read.
>
> But it appears that we may meet a problem if the function
> vhost_vdpa_net_load_x() sends multiple CVQ commands. It is possible that
> we might not meet the condition for one of the CVQ commands, but we
> could still meet the conditions for other CVQ commands.
>
> Therefore, in the case of vhost_vdpa_net_load_x() sending multiple CVQ
> commands, if we still hope to use this style, should we split the
> function into multiple functions, with each function responsible for
> sending only one CVQ command? Or should we jump to the next CVQ command
> instead of returning from the function if the conditions are not met?
>
In that case I'd suggest using multiples if() {}, as each ctrl command
processing code is very small.
But for VIRTIO_NET_F_CTRL_RX particular case your patch propose:
if (x) {
if (y) {
...
}
}
So in my opinion it makes sense to convert to:
if (!x || !y) {
return;
}
...
We can always change later if needed.
Thanks!
> Thanks!
>
>
> > Something like:
> > /*
> > * According to ...
> > */
> > if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) ||
> > (n->mac_table.in_use == 0)) {
> > return 0
> > }
> >
> > uni_entries = n->mac_table.first_multi,
> > ...
> > ---
> >
> > Now I just realized vhost_vdpa_net_load_mac does not follow this for
> > checking VIRTIO_NET_F_CTRL_MAC_ADDR.
> >
> > I'm ok if you leave it this way though.
> >
> > Thanks!
> >
> >> + /*
> >> + * According to virtio_net_reset(), device uses an empty MAC
> >> filter
> >> + * table as its default state.
> >> + *
> >> + * 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.
> >> + */
> >> + 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 -EINVAL;
> >> + }
> >> + }
> >> + }
> >> +
> >> return 0;
> >> }
> >>
> >> --
> >> 2.25.1
> >>
> >
>
