On Thu, Jun 29, 2023 at 5:26 PM Hawkins Jiawei <yin31...@gmail.com> wrote:
>
> This patch introduces vhost_vdpa_net_load_rx_mode()
> and vhost_vdpa_net_load_rx() to restore the packet
> receive filtering state in relation to
> VIRTIO_NET_F_CTRL_RX feature at device's startup.
>
> Signed-off-by: Hawkins Jiawei <yin31...@gmail.com>
> ---
> v2:
> - avoid sending CVQ command in default state suggested by Eugenio
>
> v1:
> https://lore.kernel.org/all/86eeddcd6f6b04e5c1e44e901ddea3b1b8b6c183.1687402580.git.yin31...@gmail.com/
>
> net/vhost-vdpa.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 104 insertions(+)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index cb45c84c88..9d5d88756c 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -792,6 +792,106 @@ static int vhost_vdpa_net_load_offloads(VhostVDPAState
> *s,
> return 0;
> }
>
> +static int vhost_vdpa_net_load_rx_mode(VhostVDPAState *s,
> + uint8_t cmd,
> + uint8_t on)
> +{
> + ssize_t dev_written;
> + const struct iovec data = {
> + .iov_base = &on,
> + .iov_len = sizeof(on),
> + };
> + dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_RX,
> + cmd, &data, 1);
> + if (unlikely(dev_written < 0)) {
> + return dev_written;
> + }
> + if (*s->status != VIRTIO_NET_OK) {
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
> + const VirtIONet *n)
> +{
> + uint8_t on;
> + int r;
> +
> + if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
Also suggesting early returns here.
> + /* Load the promiscous mode */
> + if (n->mac_table.uni_overflow) {
> + /*
> + * According to VirtIO standard, "Since there are no guarantees,
> + * it can use a hash filter or silently switch to
> + * allmulti or promiscuous mode if it is given too many
> addresses."
> + *
> + * QEMU ignores non-multicast(unicast) MAC addresses and
> + * marks `uni_overflow` for the device internal state
> + * if guest sets too many non-multicast(unicast) MAC addresses.
> + * Therefore, we should turn promiscous mode on in this case.
> + */
> + on = 1;
> + } else {
> + on = n->promisc;
> + }
I think we can remove the "on" variable and just do:
/*
* According to ...
*/
if (n->mac_table.uni_overflow || n->promisc) {
r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC, on);
if (r < 0) {
return r;
}
---
And the equivalent for multicast.
Would that make sense?
Thanks!
> + if (on != 1) {
> + /*
> + * According to virtio_net_reset(), device turns promiscuous
> mode on
> + * by default.
> + *
> + * Therefore, there is no need to send this CVQ command if the
> + * driver also sets promiscuous mode on, which aligns with
> + * the device's defaults.
> + *
> + * Note that the device's defaults can mismatch the driver's
> + * configuration only at live migration.
> + */
> + r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC,
> on);
> + if (r < 0) {
> + return r;
> + }
> + }
> +
> + /* Load the all-multicast mode */
> + if (n->mac_table.multi_overflow) {
> + /*
> + * According to VirtIO standard, "Since there are no guarantees,
> + * it can use a hash filter or silently switch to
> + * allmulti or promiscuous mode if it is given too many
> addresses."
> + *
> + * QEMU ignores multicast MAC addresses and
> + * marks `multi_overflow` for the device internal state
> + * if guest sets too many multicast MAC addresses.
> + * Therefore, we should turn all-multicast mode on in this case.
> + */
> + on = 1;
> + } else {
> + on = n->allmulti;
> + }
> + if (on != 0) {
> + /*
> + * According to virtio_net_reset(), device turns all-multicast
> mode
> + * off by default.
> + *
> + * Therefore, there is no need to send this CVQ command if the
> + * driver also sets all-multicast mode off, which aligns with
> + * the device's defaults.
> + *
> + * Note that the device's defaults can mismatch the driver's
> + * configuration only at live migration.
> + */
> + r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_ALLMULTI,
> on);
> + if (r < 0) {
> + return r;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> static int vhost_vdpa_net_load(NetClientState *nc)
> {
> VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> @@ -818,6 +918,10 @@ static int vhost_vdpa_net_load(NetClientState *nc)
> if (unlikely(r)) {
> return r;
> }
> + r = vhost_vdpa_net_load_rx(s, n);
> + if (unlikely(r)) {
> + return r;
> + }
>
> return 0;
> }
> --
> 2.25.1
>
