On 2023/7/5 14:29, Eugenio Perez Martin wrote:
> On Wed, Jul 5, 2023 at 3:43 AM Hawkins Jiawei <[email protected]> wrote:
>>
>> On 2023/7/4 22:53, Eugenio Perez Martin wrote:
>>> On Thu, Jun 29, 2023 at 5:26 PM Hawkins Jiawei <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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?
>>
>
> In that case I'd suggest using multiples if() {}, as each ctrl command
> processing code is very small.
>
> But for VIRTIO_NET_F_CTRL_RX particular case your patch propose:
> if (x) {
> if (y) {
> ...
> }
> }
>
> So in my opinion it makes sense to convert to:
> if (!x || !y) {
> return;
> }
> ...
Thanks for your explanation.
I will refactor the patch according to your suggestion.
Thanks!
>
> We can always change later if needed.
>
> Thanks!
>
>> 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
>>>>
>>>
>>
>
