On 2023/7/4 23:39, Eugenio Perez Martin wrote:
> On Thu, Jun 29, 2023 at 5:26 PM Hawkins Jiawei <[email protected]> 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 <[email protected]>
>> ---
>> 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?
>
>> + /* 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
>>
>
