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>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index fa78e8288ebb..1c26139d02fe 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -3824,6 +3824,9 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
 		err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
 		if (err)
 			goto err_alloc;
+
+		if (is_zero_ether_addr(config->mac))
+			eth_random_addr(config->mac);
 	}
 
 	if (!is_zero_ether_addr(config->mac)) {
-- 
2.45.1

On 8/27/2024 9:02 AM, Dragos Tatulea 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>
> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index fa78e8288ebb..1c26139d02fe 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -3824,6 +3824,9 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>   		err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
>   		if (err)
>   			goto err_alloc;
> +
> +		if (is_zero_ether_addr(config->mac))
> +			eth_random_addr(config->mac);
I wonder with this change we no longer honor the historical behaviour to 
retain the zero mac address and clear the _F_MAC bit, should we head to 
remove the below logic? It looks to me below would become dead code 
effectively.

         } else if ((add_config->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) 
== 0) {
                 /*
                  * We used to clear _F_MAC feature bit if seeing
                  * zero mac address when device features are not
                  * specifically provisioned. Keep the behaviour
                  * so old scripts do not break.
                  */
                 device_features &= ~BIT_ULL(VIRTIO_NET_F_MAC);

If we are not going to honor old behaviour any more, looks to me we 
should also block users from creating vdpa device with zero mac address, 
if the mac attribute is specified. There's more sorrow than help the 
zero mac address could buy for users.

         if (add_config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
                 memcpy(ndev->config.mac, add_config->net.mac, ETH_ALEN);

Regards,
-Siwei

>   	}
>   
>   	if (!is_zero_ether_addr(config->mac)) {

On 28.08.24 07:54, Si-Wei Liu wrote:
> 
> 
> On 8/27/2024 9:02 AM, Dragos Tatulea 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>
>> ---
>>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> index fa78e8288ebb..1c26139d02fe 100644
>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> @@ -3824,6 +3824,9 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>>           err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
>>           if (err)
>>               goto err_alloc;
>> +
>> +        if (is_zero_ether_addr(config->mac))
>> +            eth_random_addr(config->mac);
> I wonder with this change we no longer honor the historical behaviour to retain the zero mac address and clear the _F_MAC bit, should we head to remove the below logic? It looks to me below would become dead code effectively.
> 
It is still possible to create a vdpa device with a zero mac address
explicitly, right? 

>         } else if ((add_config->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) == 0) {
>                 /*
>                  * We used to clear _F_MAC feature bit if seeing
>                  * zero mac address when device features are not
>                  * specifically provisioned. Keep the behaviour
>                  * so old scripts do not break.
>                  */
>                 device_features &= ~BIT_ULL(VIRTIO_NET_F_MAC);
> 
> If we are not going to honor old behaviour any more, looks to me we should also block users from creating vdpa device with zero mac address, if the mac attribute is specified. There's more sorrow than help the zero mac address could buy for users.
That makes sense. There is a small risk of breaking user's scripts that
do this by accident...

Thanks,
Dragos

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

> ---
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index fa78e8288ebb..1c26139d02fe 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -3824,6 +3824,9 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>                 err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
>                 if (err)
>                         goto err_alloc;
> +
> +               if (is_zero_ether_addr(config->mac))
> +                       eth_random_addr(config->mac);
>         }
>
>         if (!is_zero_ether_addr(config->mac)) {
> --
> 2.45.1
>

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.

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.

Thanks
Cindy


> > ---
> >  drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index fa78e8288ebb..1c26139d02fe 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -3824,6 +3824,9 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> >                 err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
> >                 if (err)
> >                         goto err_alloc;
> > +
> > +               if (is_zero_ether_addr(config->mac))
> > +                       eth_random_addr(config->mac);
> >         }
> >
> >         if (!is_zero_ether_addr(config->mac)) {
> > --
> > 2.45.1
> >
>

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.

> 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.

Thanks,
Dragos

> Thanks
> Cindy
> 
> 
>>> ---
>>>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> index fa78e8288ebb..1c26139d02fe 100644
>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> @@ -3824,6 +3824,9 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>>>                 err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
>>>                 if (err)
>>>                         goto err_alloc;
>>> +
>>> +               if (is_zero_ether_addr(config->mac))
>>> +                       eth_random_addr(config->mac);
>>>         }
>>>
>>>         if (!is_zero_ether_addr(config->mac)) {
>>> --
>>> 2.45.1
>>>
>>
> 

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.

> > 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.
Thanks
cindy

> Thanks,
> Dragos
>
> > Thanks
> > Cindy
> >
> >
> >>> ---
> >>>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>> index fa78e8288ebb..1c26139d02fe 100644
> >>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>> @@ -3824,6 +3824,9 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> >>>                 err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
> >>>                 if (err)
> >>>                         goto err_alloc;
> >>> +
> >>> +               if (is_zero_ether_addr(config->mac))
> >>> +                       eth_random_addr(config->mac);
> >>>         }
> >>>
> >>>         if (!is_zero_ether_addr(config->mac)) {
> >>> --
> >>> 2.45.1
> >>>
> >>
> >
>

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

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.
As I mentioned before, this is a bug in QEMU, and I'm working on fixing it.

Thanks
Cindy

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.

Thanks,
Dragos

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

On Fri, 30 Aug 2024 at 22:46, Dragos Tatulea <dtatulea@nvidia.com> wrote:
>
> 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
>
Hi Dragos

Thanks for your test and help, I think I can add a check for
VIRTIO_NET_F_MAC in the qemu code. if the device's Mac is 0 and the
VIRTIO_NET_F_MAC is not set. The guest VM will fail to load. I will
double-check this
Thanks

Cindy

On 02.09.24 10:40, Cindy Lu wrote:
> On Fri, 30 Aug 2024 at 22:46, Dragos Tatulea <dtatulea@nvidia.com> wrote:
>>
>> 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
>>
> Hi Dragos
> 
> Thanks for your test and help, I think I can add a check for
> VIRTIO_NET_F_MAC in the qemu code. if the device's Mac is 0 and the
> VIRTIO_NET_F_MAC is not set. The guest VM will fail to load. I will
> double-check this
My request was to use the random MAC from qemu in this case. qemu is
able to configure the device via CVQ. At least this device...

Thanks,
Dragos

On 02.09.24 10:53, Dragos Tatulea wrote:
> 
> 
> On 02.09.24 10:40, Cindy Lu wrote:
>> On Fri, 30 Aug 2024 at 22:46, Dragos Tatulea <dtatulea@nvidia.com> wrote:
>>>
>>> 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
>>>
>> Hi Dragos
>>
>> Thanks for your test and help, I think I can add a check for
>> VIRTIO_NET_F_MAC in the qemu code. if the device's Mac is 0 and the
>> VIRTIO_NET_F_MAC is not set. The guest VM will fail to load. I will
>> double-check this
> My request was to use the random MAC from qemu in this case. qemu is
> able to configure the device via CVQ. At least this device...
Also, it makes sense that if a device doesn't have a MAC set and
VIRTIO_NET_F_CTRL_MAC_ADDR is set then qemu can set a random mac on
that device. 

Thanks,
Dragos

On Mon, 2 Sept 2024 at 16:54, Dragos Tatulea <dtatulea@nvidia.com> wrote:
>
>
>
> On 02.09.24 10:40, Cindy Lu wrote:
> > On Fri, 30 Aug 2024 at 22:46, Dragos Tatulea <dtatulea@nvidia.com> wrote:
> >>
> >> 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
> >>
> > Hi Dragos
> >
> > Thanks for your test and help, I think I can add a check for
> > VIRTIO_NET_F_MAC in the qemu code. if the device's Mac is 0 and the
> > VIRTIO_NET_F_MAC is not set. The guest VM will fail to load. I will
> > double-check this
> My request was to use the random MAC from qemu in this case. qemu is
> able to configure the device via CVQ. At least this device...
>
Ok, I got it. Sorry for my misunderstanding. I understand what you
mean. Then the device needs to set the bit VIRTIO_NET_F_CTRL_MAC_ADDR
to use CVQ. I will verify this and change my patch.
Thanks
Cindy
> Thanks,
> Dragos
>

On 29.08.24 12:00, Dragos Tatulea 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.
(Side note)
While looking at another issue I discovered that it's possible to
configure a random MAC on the mlx5_vdpa device at VM boot time if
device MAC configuration is implemented during during .set_config(). So
I was able to boot up a VM with a random MAC address coming from qemu
and the traffic worked with this new MAC.

So now I'm not sure if this is just by luck or if the .set_config()
op should be implemented for the MAC part in our device.

Thanks,
Dragos

On Fri, 30 Aug 2024 at 03:03, Dragos Tatulea <dtatulea@nvidia.com> wrote:
>
>
>
> On 29.08.24 12:00, Dragos Tatulea 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.
> (Side note)
> While looking at another issue I discovered that it's possible to
> configure a random MAC on the mlx5_vdpa device at VM boot time if
> device MAC configuration is implemented during during .set_config(). So
> I was able to boot up a VM with a random MAC address coming from qemu
> and the traffic worked with this new MAC.
>
> So now I'm not sure if this is just by luck or if the .set_config()
> op should be implemented for the MAC part in our device.
>
> Thanks,
> Dragos
>
Hi Dragos，
For qemu part, I think this is not set from set_config()?  it should
be from the CVQ?
Usually, we don't recommend using the set_config() function because
the configuration space should be read-only for modern devices.

Now there is a bug in this part of qemu, and we plan to remove the
code to set_config() in virtio_net_device_realize(), here is the patch
https://lore.kernel.org/all/CACGkMEvCSKfahpBQLAMmSzdFN-QPhg5Zx+UQVrFX0HsWybZZNA@mail.gmail.com/T/
and this is still under review

thanks
cindy

On Fri, Aug 30, 2024 at 05:29:14PM +0800, Cindy Lu wrote:
> On Fri, 30 Aug 2024 at 03:03, Dragos Tatulea <dtatulea@nvidia.com> wrote:
> >
> >
> >
> > On 29.08.24 12:00, Dragos Tatulea 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.
> > (Side note)
> > While looking at another issue I discovered that it's possible to
> > configure a random MAC on the mlx5_vdpa device at VM boot time if
> > device MAC configuration is implemented during during .set_config(). So
> > I was able to boot up a VM with a random MAC address coming from qemu
> > and the traffic worked with this new MAC.
> >
> > So now I'm not sure if this is just by luck or if the .set_config()
> > op should be implemented for the MAC part in our device.
> >
> > Thanks,
> > Dragos
> >
> Hi Dragos，
> For qemu part, I think this is not set from set_config()?  it should
> be from the CVQ?


You are confusing two things.
Provisioning is not through CVQ.


> Usually, we don't recommend using the set_config() function because
> the configuration space should be read-only for modern devices.
> 
> Now there is a bug in this part of qemu, and we plan to remove the
> code to set_config() in virtio_net_device_realize(), here is the patch
> https://lore.kernel.org/all/CACGkMEvCSKfahpBQLAMmSzdFN-QPhg5Zx+UQVrFX0HsWybZZNA@mail.gmail.com/T/
> and this is still under review
> 
> thanks
> cindy

On Mon, 2 Sept 2024 at 02:49, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Aug 30, 2024 at 05:29:14PM +0800, Cindy Lu wrote:
> > On Fri, 30 Aug 2024 at 03:03, Dragos Tatulea <dtatulea@nvidia.com> wrote:
> > >
> > >
> > >
> > > On 29.08.24 12:00, Dragos Tatulea 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.
> > > (Side note)
> > > While looking at another issue I discovered that it's possible to
> > > configure a random MAC on the mlx5_vdpa device at VM boot time if
> > > device MAC configuration is implemented during during .set_config(). So
> > > I was able to boot up a VM with a random MAC address coming from qemu
> > > and the traffic worked with this new MAC.
> > >
> > > So now I'm not sure if this is just by luck or if the .set_config()
> > > op should be implemented for the MAC part in our device.
> > >
> > > Thanks,
> > > Dragos
> > >
> > Hi Dragos，
> > For qemu part, I think this is not set from set_config()?  it should
> > be from the CVQ?
>
>
> You are confusing two things.
> Provisioning is not through CVQ.
>
Thanks, Michael. I'm sorry for the confusion. I just meant that maybe
the MAC address is set through CVQ. I will make it clearer next time.
Thanks
Cindy
>
> > Usually, we don't recommend using the set_config() function because
> > the configuration space should be read-only for modern devices.
> >
> > Now there is a bug in this part of qemu, and we plan to remove the
> > code to set_config() in virtio_net_device_realize(), here is the patch
> > https://lore.kernel.org/all/CACGkMEvCSKfahpBQLAMmSzdFN-QPhg5Zx+UQVrFX0HsWybZZNA@mail.gmail.com/T/
> > and this is still under review
> >
> > thanks
> > cindy
>

Hi Cindy,

On 30.08.24 11:29, Cindy Lu wrote:
> On Fri, 30 Aug 2024 at 03:03, Dragos Tatulea <dtatulea@nvidia.com> wrote:
>>
>>
>>
>> On 29.08.24 12:00, Dragos Tatulea 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.
>> (Side note)
>> While looking at another issue I discovered that it's possible to
>> configure a random MAC on the mlx5_vdpa device at VM boot time if
>> device MAC configuration is implemented during during .set_config(). So
>> I was able to boot up a VM with a random MAC address coming from qemu
>> and the traffic worked with this new MAC.
>>
>> So now I'm not sure if this is just by luck or if the .set_config()
>> op should be implemented for the MAC part in our device.
>>
>> Thanks,
>> Dragos
>>
> Hi Dragos，
> For qemu part, I think this is not set from set_config()?  it should
> be from the CVQ?
I see that .set_config() is called during boot time. CVQ commands are
happening only when the MAC is configured from within the VM.

> Usually, we don't recommend using the set_config() function because
> the configuration space should be read-only for modern devices.
> 
Ack

> Now there is a bug in this part of qemu, and we plan to remove the
> code to set_config() in virtio_net_device_realize(), here is the patch
> https://lore.kernel.org/all/CACGkMEvCSKfahpBQLAMmSzdFN-QPhg5Zx+UQVrFX0HsWybZZNA@mail.gmail.com/T/
> and this is still under review
> 
Thanks for the clarification. So if I understand correctly,
there will be no way for qemu to set a random MAC address for vdpa
devices going forward. Unless it is done through the CVQ from within
the VM.


Thanks,
Dragos

On 8/27/2024 9:02 AM, Dragos Tatulea 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>
> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index fa78e8288ebb..1c26139d02fe 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -3824,6 +3824,9 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>                  err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
>                  if (err)
>                          goto err_alloc;
> +
> +               if (is_zero_ether_addr(config->mac))
> +                       eth_random_addr(config->mac);
>          }
> 
>          if (!is_zero_ether_addr(config->mac)) {
> --
> 2.45.1
> 
> 

Reviewed-by: Shannon Nelson <shannon.nelson@amd.com>