Message-ID: <CACLfguVibPU_WzvRKJ7oVN+SaFy1YYysPbmBWEfdYgFjYc-h0Q@mail.gmail.com>
Date: Mon, 2 Sep 2024 17:05:01 +0800
From: Cindy Lu <lulu@...hat.com>
To: Dragos Tatulea <dtatulea@...dia.com>
Cc: Jason Wang <jasowang@...hat.com>, "Michael S. Tsirkin" <mst@...hat.com>, 
	Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Eugenio Pérez <eperezma@...hat.com>, 
	si-wei.liu@...cle.com, Jiri Pirko <jiri@...dia.com>, virtualization@...ts.linux.dev, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vdpa/mlx5: Use random MAC address when no nic vport MAC set

On Mon, 2 Sept 2024 at 16:54, Dragos Tatulea <dtatulea@...dia.com> wrote:
>
>
>
> On 02.09.24 10:40, Cindy Lu wrote:
> > On Fri, 30 Aug 2024 at 22:46, Dragos Tatulea <dtatulea@...dia.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@...dia.com> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 29.08.24 11:05, Cindy Lu wrote:
> >>>>>> On Wed, 28 Aug 2024 at 17:37, Dragos Tatulea <dtatulea@...dia.com> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> On 28.08.24 11:00, Cindy Lu wrote:
> >>>>>>>> On Wed, 28 Aug 2024 at 09:51, Jason Wang <jasowang@...hat.com> wrote:
> >>>>>>>>>
> >>>>>>>>> On Wed, Aug 28, 2024 at 12:03 AM Dragos Tatulea <dtatulea@...dia.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@...dia.com>
> >>>>>>>>>> Reviewed-by: Jiri Pirko <jiri@...dia.com>
> >>>>>>>>>
> >>>>>>>>> Acked-by: Jason Wang <jasowang@...hat.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
>

Powered by blists - more mailing lists