Re: [PATCH] vdpa/mlx5: Use random MAC address when no nic vport MAC set

[Dragos Tatulea <dtatulea@nvidia.com>]

Hi Cindy,

On 30.08.24 15:52, Dragos Tatulea wrote:
> 
> 
> On 30.08.24 11:12, Cindy Lu wrote:
>> On Thu, 29 Aug 2024 at 18:00, Dragos Tatulea <dtatulea@nvidia.com> wrote:
>>>
>>>
>>>
>>> On 29.08.24 11:05, Cindy Lu wrote:
>>>> On Wed, 28 Aug 2024 at 17:37, Dragos Tatulea <dtatulea@nvidia.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 28.08.24 11:00, Cindy Lu wrote:
>>>>>> On Wed, 28 Aug 2024 at 09:51, Jason Wang <jasowang@redhat.com> wrote:
>>>>>>>
>>>>>>> On Wed, Aug 28, 2024 at 12:03 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>>>>>>>>
>>>>>>>> When the vdpa device is configured without a specific MAC
>>>>>>>> address, the vport MAC address is used. However, this
>>>>>>>> address can be 0 which prevents the driver from properly
>>>>>>>> configuring the MPFS and breaks steering.
>>>>>>>>
>>>>>>>> The solution is to simply generate a random MAC address
>>>>>>>> when no MAC is set on the nic vport.
>>>>>>>>
>>>>>>>> Now it's possible to create a vdpa device without a
>>>>>>>> MAC address and run qemu with this device without needing
>>>>>>>> to configure an explicit MAC address.
>>>>>>>>
>>>>>>>> Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
>>>>>>>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>>>>>>>
>>>>>>> Acked-by: Jason Wang <jasowang@redhat.com>
>>>>>>>
>>>>>>> (Adding Cindy for double checking if it has any side effect on Qemu side)
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>> But Now there is a bug in QEMU: if the hardware MAC address does not
>>>>>> match the one in the QEMU command line, it will cause traffic loss.
>>>>>>
>>>>> Why is this a new issue in qemu? qemu in it's current state won't work
>>>>> with a different mac address that the one that is set in HW anyway.
>>>>>
>>>> this is not a new bug. We are trying to fix it because it will cause
>>>> traffic lose without any warning.
>>>> in my fix , this setting (different mac in device and Qemu) will fail
>>>> to load the VM.
>>> Which is a good thing, right? Some feedback to the user that there is
>>> a misconfig. I got bitten by this so many times... Thank you for adding it.
>>>
>>>>
>>>>>> So, Just an FYI here: if your patch merged, it may cause traffic loss.
>>>>>> and now I'm working in the fix it in qemu, the link is
>>>>>> https://patchew.org/QEMU/20240716011349.821777-1-lulu@redhat.com/
>>>>>> The idea of this fix is
>>>>>> There are will only two acceptable situations for qemu:
>>>>>> 1. The hardware MAC address is the same as the MAC address specified
>>>>>> in the QEMU command line, and both MAC addresses are not 0.
>>>>>> 2. The hardware MAC address is not 0, and the MAC address in the QEMU
>>>>>> command line is 0. In this situation, the hardware MAC address will
>>>>>> overwrite the QEMU command line address.
>>>>>>
>>>>> Why would this not work with this patch? This patch simply sets a MAC
>>>>> if the vport doesn't have one set. Which allows for more scenarios to
>>>>> work.
>>>>>
>>>> I do not mean your patch will not work, I just want to make some
>>>> clarify here.Your patch + my fix may cause the VM to fail to load in
>>>> some situations, and this is as expected.
>>>> Your patch is good to merge.
>>> Ack. Thank you for the clarification.
>>>
>>> Thanks,
>>> Dragos
>>>
>> Hi Dragos，
>>  I think we need to hold this patch. Because it may not be working
>> with upstream qemu.
>>
>> MLX will create a random MAC address for your patch. Additionally, if
>> there is no specific MAC in the QEMU command line, QEMU will also
>> generate a random MAC.
>> these two MAC are not the same. and this will cause traffic loss.
> Ahaa, it turns out that qemu 8.x and 9.x have different behaviour.
> 
> Initially I was testing this scenario (vdpa device created with no mac
> and no mac set in qemu cli) with qemu 8.x. There, qemu was not being
> able to set the qemu generated random mac addres because .set_config()
> is a nop in mlx5_vdpa.
> 
> Then I moved to qemu 9.x and saw that this scenario was working because
> now the CVQ was used instead to configure the mac on the device.
> 
> So this patch should definitely not be applied.
> 
> I was thinking if there are ways to fix this for 8.x. The only feasible
> way is to implement .set_config() in mlx5_vdpa for the mac
> configuration. But as you previousy said, this is discouraged.
> 
I just tested your referenced qemu fix from patchwork and I found that
for the case when a vdpa device doesn't have a mac address (mac address
0 and VIRTIO_NET_F_MAC not set) qemu will return an error. So with this
fix we'd be back to square one where the user always has to set a mac
somewhere.

Would it be possible to take this case into consideration with your
fix?

Thanks,
Dragos