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 <yin31149@gmail.com>
---
v2:
- avoid sending CVQ command in default state suggested by Eugenio
v1: https://lore.kernel.org/all/86eeddcd6f6b04e5c1e44e901ddea3b1b8b6c183.1687402580.git.yin31149@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)) {
+ /* 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;
+ }
+ 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.1On Thu, Jun 29, 2023 at 5:26 PM Hawkins Jiawei <yin31149@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 <yin31149@gmail.com>
> ---
> v2:
> - avoid sending CVQ command in default state suggested by Eugenio
>
> v1: https://lore.kernel.org/all/86eeddcd6f6b04e5c1e44e901ddea3b1b8b6c183.1687402580.git.yin31149@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
>
On 2023/7/4 23:39, Eugenio Perez Martin wrote:
> On Thu, Jun 29, 2023 at 5:26 PM Hawkins Jiawei <yin31149@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 <yin31149@gmail.com>
>> ---
>> v2:
>> - avoid sending CVQ command in default state suggested by Eugenio
>>
>> v1: https://lore.kernel.org/all/86eeddcd6f6b04e5c1e44e901ddea3b1b8b6c183.1687402580.git.yin31149@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?
>
>> + /* 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
>>
>
On Wed, Jul 5, 2023 at 4:09 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> On 2023/7/4 23:39, Eugenio Perez Martin wrote:
> > On Thu, Jun 29, 2023 at 5:26 PM Hawkins Jiawei <yin31149@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 <yin31149@gmail.com>
> >> ---
> >> v2:
> >> - avoid sending CVQ command in default state suggested by Eugenio
> >>
> >> v1: https://lore.kernel.org/all/86eeddcd6f6b04e5c1e44e901ddea3b1b8b6c183.1687402580.git.yin31149@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
> >>
> >
>
On 2023/7/5 14:40, Eugenio Perez Martin wrote:
> On Wed, Jul 5, 2023 at 4:09 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
>>
>> On 2023/7/4 23:39, Eugenio Perez Martin wrote:
>>> On Thu, Jun 29, 2023 at 5:26 PM Hawkins Jiawei <yin31149@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 <yin31149@gmail.com>
>>>> ---
>>>> v2:
>>>> - avoid sending CVQ command in default state suggested by Eugenio
>>>>
>>>> v1: https://lore.kernel.org/all/86eeddcd6f6b04e5c1e44e901ddea3b1b8b6c183.1687402580.git.yin31149@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
Thanks for your explanation, it makes sense.
I will send the v3 patch based on your suggestion.
Thanks!
>
>>>
>>>> + /* 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
>>>>
>>>
>>
>
© 2016 - 2026 Red Hat, Inc.