On 2023/7/5 14:40, Eugenio Perez Martin wrote:
> 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
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
>>>>
>>>
>>
>
