Re: [PATCH RFC v2 3/4] vdpa: Restore packet receive filtering state relative with _F_CTRL_RX feature

[Hawkins Jiawei]

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
>>>>
>>>
>>
>