On Wed, Jul 5, 2023 at 4:09 AM Hawkins Jiawei <yin31...@gmail.com> wrote:
>
> On 2023/7/4 23:39, Eugenio Perez Martin wrote:
> > 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.
>
> So, for CVQ commands related to VIRTIO_NET_F_CTRL_EXTRA_RX, would it be
> more appropriate to create a new function, maybe
> vhost_vdpa_net_load_rx_extra, to handle them instead of sending those
> CVQ commands within this function, if we choose to return early?
>
My understanding is that VIRTIO_NET_F_CTRL_RX_EXTRA depends on
VIRTIO_NET_F_CTRL_RX, so we can do:
if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
return;
}
// Process CTRL_RX commands
if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
return;
}
// process CTRL_RX_EXTRA commands
> >
> >> + /* 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?
>
> Yes, I will refactor these according to your suggestion.
>
> Thanks!
>
>
> >
> > 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
> >>
> >
>
