On 2023/7/4 22:53, Eugenio Perez Martin wrote:
> On Thu, Jun 29, 2023 at 5:26 PM Hawkins Jiawei <yin31...@gmail.com> wrote:
>>
>> This patch refactors vhost_vdpa_net_load_mac() to
>> restore the MAC address filtering state at device's startup.
>>
>> Signed-off-by: Hawkins Jiawei <yin31...@gmail.com>
>> ---
>> v2:
>> - use iovec suggested by Eugenio
>> - avoid sending CVQ command in default state
>>
>> v1:
>> https://lore.kernel.org/all/00f72fe154a882fd6dc15bc39e3a1ac63f9dadce.1687402580.git.yin31...@gmail.com/
>>
>> net/vhost-vdpa.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 51 insertions(+)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index 0bd1c7817c..cb45c84c88 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -665,6 +665,57 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
>> const VirtIONet *n)
>> }
>> }
>>
>> + if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
>> + if (n->mac_table.in_use != 0) {
>
> This may be just style nitpicking, but I find it more clear to return
> early if conditions are not met and then send the CVQ command.
Yes, this makes code more clear to read.
But it appears that we may meet a problem if the function
vhost_vdpa_net_load_x() sends multiple CVQ commands. It is possible that
we might not meet the condition for one of the CVQ commands, but we
could still meet the conditions for other CVQ commands.
Therefore, in the case of vhost_vdpa_net_load_x() sending multiple CVQ
commands, if we still hope to use this style, should we split the
function into multiple functions, with each function responsible for
sending only one CVQ command? Or should we jump to the next CVQ command
instead of returning from the function if the conditions are not met?
Thanks!
> Something like:
> /*
> * According to ...
> */
> if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) ||
> (n->mac_table.in_use == 0)) {
> return 0
> }
>
> uni_entries = n->mac_table.first_multi,
> ...
> ---
>
> Now I just realized vhost_vdpa_net_load_mac does not follow this for
> checking VIRTIO_NET_F_CTRL_MAC_ADDR.
>
> I'm ok if you leave it this way though.
>
> Thanks!
>
>> + /*
>> + * According to virtio_net_reset(), device uses an empty MAC
>> filter
>> + * table as its default state.
>> + *
>> + * Therefore, there is no need to send this CVQ command if the
>> + * driver also sets an empty MAC filter table, which aligns with
>> + * the device's defaults.
>> + *
>> + * Note that the device's defaults can mismatch the driver's
>> + * configuration only at live migration.
>> + */
>> + uint32_t uni_entries = n->mac_table.first_multi,
>> + uni_macs_size = uni_entries * ETH_ALEN,
>> + mul_entries = n->mac_table.in_use - uni_entries,
>> + mul_macs_size = mul_entries * ETH_ALEN;
>> + struct virtio_net_ctrl_mac uni = {
>> + .entries = cpu_to_le32(uni_entries),
>> + };
>> + struct virtio_net_ctrl_mac mul = {
>> + .entries = cpu_to_le32(mul_entries),
>> + };
>> + const struct iovec data[] = {
>> + {
>> + .iov_base = &uni,
>> + .iov_len = sizeof(uni),
>> + }, {
>> + .iov_base = n->mac_table.macs,
>> + .iov_len = uni_macs_size,
>> + }, {
>> + .iov_base = &mul,
>> + .iov_len = sizeof(mul),
>> + }, {
>> + .iov_base = &n->mac_table.macs[uni_macs_size],
>> + .iov_len = mul_macs_size,
>> + },
>> + };
>> + ssize_t dev_written = vhost_vdpa_net_load_cmd(s,
>> + VIRTIO_NET_CTRL_MAC,
>> + VIRTIO_NET_CTRL_MAC_TABLE_SET,
>> + data, ARRAY_SIZE(data));
>> + if (unlikely(dev_written < 0)) {
>> + return dev_written;
>> + }
>> + if (*s->status != VIRTIO_NET_OK) {
>> + return -EINVAL;
>> + }
>> + }
>> + }
>> +
>> return 0;
>> }
>>
>> --
>> 2.25.1
>>
>
