Re: [PATCH v2 00/20] vfio: Add migration pre-copy support and device dirty tracking

[Avihai Horon]

On 27/02/2023 15:50, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 2/26/23 18:00, Avihai Horon wrote:
> > On 24/02/2023 21:26, Joao Martins wrote:
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > On 23/02/2023 14:56, Avihai Horon wrote:
> > > > On 22/02/2023 22:55, Alex Williamson wrote:
> > > > > There are various errors running this through the CI on gitlab.
> > > > >
> > > > > This one seems bogus but needs to be resolved regardless:
> > > > >
> > > > > https://gitlab.com/alex.williamson/qemu/-/jobs/3817940731
> > > > > FAILED: libqemu-aarch64-softmmu.fa.p/hw_vfio_common.c.o
> > > > > 2786s390x-linux-gnu-gcc -m64 -Ilibqemu-aarch64-softmmu.fa.p -I. -I..
> > > > > -Itarget/arm -I../target/arm -Iqapi -Itrace -Iui -Iui/shader
> > > > > -I/usr/include/pixman-1 -I/usr/include/capstone 
> > > > > -I/usr/include/glib-2.0
> > > > > -I/usr/lib/s390x-linux-gnu/glib-2.0/include 
> > > > > -fdiagnostics-color=auto -Wall
> > > > > -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem
> > > > > /builds/alex.williamson/qemu/linux-headers -isystem linux-headers 
> > > > > -iquote .
> > > > > -iquote /builds/alex.williamson/qemu -iquote
> > > > > /builds/alex.williamson/qemu/include -iquote
> > > > > /builds/alex.williamson/qemu/tcg/s390x -pthread -U_FORTIFY_SOURCE
> > > > > -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 
> > > > > -D_LARGEFILE_SOURCE
> > > > > -fno-strict-aliasing -fno-common -fwrapv -Wundef -Wwrite-strings
> > > > > -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls
> > > > > -Wold-style-declaration -Wold-style-definition -Wtype-limits 
> > > > > -Wformat-security
> > > > > -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body 
> > > > > -Wnested-externs
> > > > > -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2
> > > > > -Wmissing-format-attribute -Wno-missing-include-dirs 
> > > > > -Wno-shift-negative-value
> > > > > -Wno-psabi -fstack-protector-strong -fPIE -isystem../linux-headers
> > > > > -isystemlinux-headers -DNEED_CPU_H
> > > > > '-DCONFIG_TARGET="aarch64-softmmu-config-target.h"'
> > > > > '-DCONFIG_DEVICES="aarch64-softmmu-config-devices.h"' -MD -MQ
> > > > > libqemu-aarch64-softmmu.fa.p/hw_vfio_common.c.o -MF
> > > > > libqemu-aarch64-softmmu.fa.p/hw_vfio_common.c.o.d -o
> > > > > libqemu-aarch64-softmmu.fa.p/hw_vfio_common.c.o -c 
> > > > > ../hw/vfio/common.c
> > > > > 2787../hw/vfio/common.c: In function 
> > > > > ‘vfio_listener_log_global_start’:
> > > > > 2788../hw/vfio/common.c:1772:8: error: ‘ret’ may be used 
> > > > > uninitialized in this
> > > > > function [-Werror=maybe-uninitialized]
> > > > > 2789 1772 |     if (ret) {
> > > > > 2790      |        ^
> > > > >
> > > > > 32-bit builds have some actual errors though:
> > > > >
> > > > > https://gitlab.com/alex.williamson/qemu/-/jobs/3817940719
> > > > > FAILED: libqemu-aarch64-softmmu.fa.p/hw_vfio_common.c.o
> > > > > 2601cc -m32 -Ilibqemu-aarch64-softmmu.fa.p -I. -I.. -Itarget/arm
> > > > > -I../target/arm -Iqapi -Itrace -Iui -Iui/shader 
> > > > > -I/usr/include/pixman-1
> > > > > -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include 
> > > > > -I/usr/include/sysprof-4
> > > > > -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 
> > > > > -O2 -g
> > > > > -isystem /builds/alex.williamson/qemu/linux-headers -isystem 
> > > > > linux-headers
> > > > > -iquote . -iquote /builds/alex.williamson/qemu -iquote
> > > > > /builds/alex.williamson/qemu/include -iquote
> > > > > /builds/alex.williamson/qemu/tcg/i386 -pthread -U_FORTIFY_SOURCE
> > > > > -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 
> > > > > -D_LARGEFILE_SOURCE
> > > > > -fno-strict-aliasing -fno-common -fwrapv -Wundef -Wwrite-strings
> > > > > -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls
> > > > > -Wold-style-declaration -Wold-style-definition -Wtype-limits 
> > > > > -Wformat-security
> > > > > -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body 
> > > > > -Wnested-externs
> > > > > -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2
> > > > > -Wmissing-format-attribute -Wno-missing-include-dirs 
> > > > > -Wno-shift-negative-value
> > > > > -Wno-psabi -fstack-protector-strong -fPIE -isystem../linux-headers
> > > > > -isystemlinux-headers -DNEED_CPU_H
> > > > > '-DCONFIG_TARGET="aarch64-softmmu-config-target.h"'
> > > > > '-DCONFIG_DEVICES="aarch64-softmmu-config-devices.h"' -MD -MQ
> > > > > libqemu-aarch64-softmmu.fa.p/hw_vfio_common.c.o -MF
> > > > > libqemu-aarch64-softmmu.fa.p/hw_vfio_common.c.o.d -o
> > > > > libqemu-aarch64-softmmu.fa.p/hw_vfio_common.c.o -c 
> > > > > ../hw/vfio/common.c
> > > > > 2602../hw/vfio/common.c: In function
> > > > > 'vfio_device_feature_dma_logging_start_create':
> > > > > 2603../hw/vfio/common.c:1572:27: error: cast from pointer to 
> > > > > integer of
> > > > > different size [-Werror=pointer-to-int-cast]
> > > > > 2604 1572 |         control->ranges = (uint64_t)ranges;
> > > > > 2605      |                           ^
> > > > > 2606../hw/vfio/common.c:1596:23: error: cast from pointer to 
> > > > > integer of
> > > > > different size [-Werror=pointer-to-int-cast]
> > > > > 2607 1596 |     control->ranges = (uint64_t)ranges;
> > > > > 2608      |                       ^
> > > > > 2609../hw/vfio/common.c: In function
> > > > > 'vfio_device_feature_dma_logging_start_destroy':
> > > > > 2610../hw/vfio/common.c:1620:9: error: cast to pointer from 
> > > > > integer of
> > > > > different size [-Werror=int-to-pointer-cast]
> > > > > 2611 1620 |         (struct vfio_device_feature_dma_logging_range
> > > > > *)control->ranges;
> > > > > 2612      |         ^
> > > > > 2613../hw/vfio/common.c: In function 
> > > > > 'vfio_device_dma_logging_report':
> > > > > 2614../hw/vfio/common.c:1810:22: error: cast from pointer to 
> > > > > integer of
> > > > > different size [-Werror=pointer-to-int-cast]
> > > > > 2615 1810 |     report->bitmap = (uint64_t)bitmap;
> > > > > 2616      |                      ^
> > > > Sure, I will fix these errors.
> > > Just a thought: should the pre-copy series be moved towards the end 
> > > of this
> > > series, given that it's more of an improvement of downtime than a 
> > > must-have like
> > > dirty tracking?
> >
> > Given recent discussion, maybe it would be better to split this 
> > series and go one step at a time:
> > Start with basic support for device dirty tracking (without vIOMMU 
> > support), then add pre-copy and then add vIOMMU support to device 
> > dirty tracking.
>
> and add the fixes first in the series. They could be merged quickly.

Yes, of course. I will add them.

Thanks.