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