[RFC PATCH 0/6] Use Clang for compiling in the 64-bit MSYS2 job

[RFC PATCH 0/6] Use Clang for compiling in the 64-bit MSYS2 job

[Thomas Huth]

The 64-bit MSYS2 job often times out in our CI, though we already have
limited it to a very minimum by using --without-default-devices etc.
GCC is incredibly slow here. By using Clang instead of GCC, the job
is ca. 15 minutes faster - that's enough buffer to avoid the timeouts
here.

The caveat is that Clang does not support __attribute__((gcc_struct))
on Windows. So we have to make sure that our structs are still padded
right. By checking the build with "pahole", Daniel discovered that
there is still an issue in VTD_IR_TableEntry, so that's why this
struct gets changed in this patch series, too. All other structs did
not show a difference with "pahole", so building with Clang should
hopefully be fine after these modifications here.

Daniel P. Berrangé (1):
  gitlab: remove duplication between msys jobs

Marc-André Lureau (1):
  ui/dbus: fix clang compilation issue

Thomas Huth (4):
  util/oslib-win32: Fix compiling with Clang from MSYS2
  hw/i386/intel_iommu: Fix VTD_IR_TableEntry for ms_struct layout
  include/qemu/compiler: Fix problem with gcc_struct and Clang
  gitlab-ci.d/windows: Use Clang for compiling in the 64-bit MSYS2 job

 include/hw/i386/intel_iommu.h |  14 ++--
 include/qemu/compiler.h       |   2 +-
 hw/i386/intel_iommu.c         |   2 +-
 ui/dbus-listener.c            |   3 +-
 util/oslib-win32.c            |   4 +-
 .gitlab-ci.d/windows.yml      | 134 +++++++++++++---------------------
 6 files changed, 65 insertions(+), 94 deletions(-)

-- 
2.39.3

[RFC PATCH 1/6] gitlab: remove duplication between msys jobs

[Thomas Huth]

From: Daniel P. Berrangé <berrange@redhat.com>

Although they share a common parent, the two msys jobs still have
massive duplication in their script definitions that can easily be
collapsed.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20230726161942.229093-1-berrange@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 .gitlab-ci.d/windows.yml | 132 +++++++++++++++------------------------
 1 file changed, 49 insertions(+), 83 deletions(-)

diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
index f889a468b5..f086540e40 100644
--- a/.gitlab-ci.d/windows.yml
+++ b/.gitlab-ci.d/windows.yml
@@ -35,97 +35,63 @@
   - .\msys64\usr\bin\bash -lc 'pacman --noconfirm -Syuu'  # Core update
   - .\msys64\usr\bin\bash -lc 'pacman --noconfirm -Syuu'  # Normal update
   - taskkill /F /FI "MODULES eq msys-2.0.dll"
-
-msys2-64bit:
-  extends: .shared_msys2_builder
   script:
   - .\msys64\usr\bin\bash -lc "pacman -Sy --noconfirm --needed
       bison diffutils flex
       git grep make sed
-      mingw-w64-x86_64-capstone
-      mingw-w64-x86_64-curl
-      mingw-w64-x86_64-cyrus-sasl
-      mingw-w64-x86_64-dtc
-      mingw-w64-x86_64-gcc
-      mingw-w64-x86_64-glib2
-      mingw-w64-x86_64-gnutls
-      mingw-w64-x86_64-gtk3
-      mingw-w64-x86_64-libgcrypt
-      mingw-w64-x86_64-libjpeg-turbo
-      mingw-w64-x86_64-libnfs
-      mingw-w64-x86_64-libpng
-      mingw-w64-x86_64-libssh
-      mingw-w64-x86_64-libtasn1
-      mingw-w64-x86_64-libusb
-      mingw-w64-x86_64-lzo2
-      mingw-w64-x86_64-nettle
-      mingw-w64-x86_64-ninja
-      mingw-w64-x86_64-pixman
-      mingw-w64-x86_64-pkgconf
-      mingw-w64-x86_64-python
-      mingw-w64-x86_64-SDL2
-      mingw-w64-x86_64-SDL2_image
-      mingw-w64-x86_64-snappy
-      mingw-w64-x86_64-spice
-      mingw-w64-x86_64-usbredir
-      mingw-w64-x86_64-zstd "
+      $MINGW_TARGET-capstone
+      $MINGW_TARGET-curl
+      $MINGW_TARGET-cyrus-sasl
+      $MINGW_TARGET-dtc
+      $MINGW_TARGET-gcc
+      $MINGW_TARGET-glib2
+      $MINGW_TARGET-gnutls
+      $MINGW_TARGET-gtk3
+      $MINGW_TARGET-libgcrypt
+      $MINGW_TARGET-libjpeg-turbo
+      $MINGW_TARGET-libnfs
+      $MINGW_TARGET-libpng
+      $MINGW_TARGET-libssh
+      $MINGW_TARGET-libtasn1
+      $MINGW_TARGET-libusb
+      $MINGW_TARGET-lzo2
+      $MINGW_TARGET-nettle
+      $MINGW_TARGET-ninja
+      $MINGW_TARGET-pixman
+      $MINGW_TARGET-pkgconf
+      $MINGW_TARGET-python
+      $MINGW_TARGET-SDL2
+      $MINGW_TARGET-SDL2_image
+      $MINGW_TARGET-snappy
+      $MINGW_TARGET-spice
+      $MINGW_TARGET-usbredir
+      $MINGW_TARGET-zstd "
   - $env:CHERE_INVOKING = 'yes'  # Preserve the current working directory
-  - $env:MSYSTEM = 'MINGW64'     # Start a 64-bit MinGW environment
   - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
   - mkdir build
   - cd build
-  # Note: do not remove "--without-default-devices"!
-  # commit 9f8e6cad65a6 ("gitlab-ci: Speed up the msys2-64bit job by using --without-default-devices"
-  # changed to compile QEMU with the --without-default-devices switch
-  # for the msys2 64-bit job, due to the build could not complete within
-  # the project timeout.
-  - ..\msys64\usr\bin\bash -lc '../configure --target-list=x86_64-softmmu
-      --without-default-devices --enable-fdt=system'
-  - ..\msys64\usr\bin\bash -lc 'make'
-  # qTests don't run successfully with "--without-default-devices",
-  # so let's exclude the qtests from CI for now.
-  - ..\msys64\usr\bin\bash -lc 'make check MTESTARGS=\"--no-suite qtest\" || { cat meson-logs/testlog.txt; exit 1; } ;'
+  - ..\msys64\usr\bin\bash -lc "../configure --enable-fdt=system $CONFIGURE_ARGS"
+  - ..\msys64\usr\bin\bash -lc "make"
+  - ..\msys64\usr\bin\bash -lc "make check MTESTARGS='$TEST_ARGS' || { cat meson-logs/testlog.txt; exit 1; } ;"
+
+msys2-64bit:
+  extends: .shared_msys2_builder
+  variables:
+    MINGW_TARGET: mingw-w64-x86_64
+    MSYSTEM: MINGW64
+    # do not remove "--without-default-devices"!
+    # commit 9f8e6cad65a6 ("gitlab-ci: Speed up the msys2-64bit job by using --without-default-devices"
+    # changed to compile QEMU with the --without-default-devices switch
+    # for the msys2 64-bit job, due to the build could not complete within
+    CONFIGURE_ARGS:  --target-list=x86_64-softmmu --without-default-devices
+    # qTests don't run successfully with "--without-default-devices",
+    # so let's exclude the qtests from CI for now.
+    TEST_ARGS: --no-suite qtest
 
 msys2-32bit:
   extends: .shared_msys2_builder
-  script:
-  - .\msys64\usr\bin\bash -lc "pacman -Sy --noconfirm --needed
-      bison diffutils flex
-      git grep make sed
-      mingw-w64-i686-capstone
-      mingw-w64-i686-curl
-      mingw-w64-i686-cyrus-sasl
-      mingw-w64-i686-dtc
-      mingw-w64-i686-gcc
-      mingw-w64-i686-glib2
-      mingw-w64-i686-gnutls
-      mingw-w64-i686-gtk3
-      mingw-w64-i686-libgcrypt
-      mingw-w64-i686-libjpeg-turbo
-      mingw-w64-i686-libnfs
-      mingw-w64-i686-libpng
-      mingw-w64-i686-libssh
-      mingw-w64-i686-libtasn1
-      mingw-w64-i686-libusb
-      mingw-w64-i686-lzo2
-      mingw-w64-i686-nettle
-      mingw-w64-i686-ninja
-      mingw-w64-i686-pixman
-      mingw-w64-i686-pkgconf
-      mingw-w64-i686-python
-      mingw-w64-i686-SDL2
-      mingw-w64-i686-SDL2_image
-      mingw-w64-i686-snappy
-      mingw-w64-i686-spice
-      mingw-w64-i686-usbredir
-      mingw-w64-i686-zstd "
-  - $env:CHERE_INVOKING = 'yes'  # Preserve the current working directory
-  - $env:MSYSTEM = 'MINGW32'     # Start a 32-bit MinGW environment
-  - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
-  - mkdir build
-  - cd build
-  - ..\msys64\usr\bin\bash -lc '../configure --target-list=ppc64-softmmu
-                                --enable-fdt=system'
-  - ..\msys64\usr\bin\bash -lc 'make'
-  - ..\msys64\usr\bin\bash -lc 'make check MTESTARGS=\"--no-suite qtest\" ||
-                                { cat meson-logs/testlog.txt; exit 1; }'
+  variables:
+    MINGW_TARGET: mingw-w64-i686
+    MSYSTEM: MINGW32
+    CONFIGURE_ARGS:  --target-list=ppc64-softmmu
+    TEST_ARGS: --no-suite qtest
-- 
2.39.3

[RFC PATCH 2/6] ui/dbus: fix clang compilation issue

[Thomas Huth]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../ui/dbus-listener.c:236:9: error: expected expression
        Error *err = NULL;

See:
https://gitlab.com/qemu-project/qemu/-/issues/1782#note_1488517427

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/dbus-listener.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index 68ff343799..2657d9f8bb 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -232,7 +232,7 @@ static void dbus_call_update_gl(DisplayChangeListener *dcl,
         egl_fb_read_rect(ddl->ds, &ddl->fb, x, y, w, h);
         dbus_gfx_update(dcl, x, y, w, h);
         break;
-    case SHARE_KIND_D3DTEX:
+    case SHARE_KIND_D3DTEX: {
         Error *err = NULL;
         assert(ddl->d3d_texture);
 
@@ -249,6 +249,7 @@ static void dbus_call_update_gl(DisplayChangeListener *dcl,
             dbus_update_gl_cb,
             g_object_ref(ddl));
         break;
+    }
     default:
         g_warn_if_reached();
     }
-- 
2.39.3

[RFC PATCH 3/6] util/oslib-win32: Fix compiling with Clang from MSYS2

[Thomas Huth]

Clang complains:

../util/oslib-win32.c:483:56: error: omitting the parameter name in a
 function definition is a C2x extension [-Werror,-Wc2x-extensions]
win32_close_exception_handler(struct _EXCEPTION_RECORD*,
                                                       ^

Fix it by adding parameter names.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 util/oslib-win32.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 429542face..070bb455d3 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -480,8 +480,8 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr *addr,
 }
 
 EXCEPTION_DISPOSITION
-win32_close_exception_handler(struct _EXCEPTION_RECORD*,
-                              void*, struct _CONTEXT*, void*)
+win32_close_exception_handler(struct _EXCEPTION_RECORD *exrec,
+                              void *ptr1, struct _CONTEXT *cntx, void *ptr2)
 {
     return EXCEPTION_EXECUTE_HANDLER;
 }
-- 
2.39.3

[RFC PATCH 4/6] hw/i386/intel_iommu: Fix VTD_IR_TableEntry for ms_struct layout

[Thomas Huth]

We might want to compile QEMU with Clang on Windows - but it
does not support the __attribute__((gcc_struct)) yet. So we
have to make sure that the structs will stay the same when
the compiler uses the "ms_struct" layout. The VTD_IR_TableEntry
struct is affected - rewrite it a little bit so that it works
fine with both struct layouts.

Reported-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/i386/intel_iommu.h | 14 ++++++++------
 hw/i386/intel_iommu.c         |  2 +-
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 89dcbc5e1e..08bf220393 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -204,18 +204,20 @@ union VTD_IR_TableEntry {
 #endif
         uint32_t dest_id;            /* Destination ID */
         uint16_t source_id;          /* Source-ID */
+        uint16_t __reserved_2;       /* Reserved 2 */
 #if HOST_BIG_ENDIAN
-        uint64_t __reserved_2:44;    /* Reserved 2 */
-        uint64_t sid_vtype:2;        /* Source-ID Validation Type */
-        uint64_t sid_q:2;            /* Source-ID Qualifier */
+        uint32_t __reserved_3:28;    /* Reserved 3 */
+        uint32_t sid_vtype:2;        /* Source-ID Validation Type */
+        uint32_t sid_q:2;            /* Source-ID Qualifier */
 #else
-        uint64_t sid_q:2;            /* Source-ID Qualifier */
-        uint64_t sid_vtype:2;        /* Source-ID Validation Type */
-        uint64_t __reserved_2:44;    /* Reserved 2 */
+        uint32_t sid_q:2;            /* Source-ID Qualifier */
+        uint32_t sid_vtype:2;        /* Source-ID Validation Type */
+        uint32_t __reserved_3:28;    /* Reserved 3 */
 #endif
     } QEMU_PACKED irte;
     uint64_t data[2];
 };
+QEMU_BUILD_BUG_ON(sizeof(union VTD_IR_TableEntry) != 16);
 
 #define VTD_IR_INT_FORMAT_COMPAT     (0) /* Compatible Interrupt */
 #define VTD_IR_INT_FORMAT_REMAP      (1) /* Remappable Interrupt */
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index dcc334060c..d5c5ee0751 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3335,7 +3335,7 @@ static int vtd_irte_get(IntelIOMMUState *iommu, uint16_t index,
     }
 
     if (entry->irte.__reserved_0 || entry->irte.__reserved_1 ||
-        entry->irte.__reserved_2) {
+        entry->irte.__reserved_2 || entry->irte.__reserved_3) {
         error_report_once("%s: detected non-zero reserved IRTE "
                           "(index=%u, high=0x%" PRIx64 ", low=0x%" PRIx64 ")",
                           __func__, index, le64_to_cpu(entry->data[1]),
-- 
2.39.3

[RFC PATCH 5/6] include/qemu/compiler: Fix problem with gcc_struct and Clang

[Thomas Huth]

Clang on Windows does not seem to know the "gcc_struct" attribute
and emits a warning when we try to use it. Add an additional check
here with __has_attribute() to avoid this problem.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/qemu/compiler.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index a309f90c76..5065b4447c 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -22,7 +22,7 @@
 #define QEMU_EXTERN_C extern
 #endif
 
-#if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__))
+#if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__)) && !defined(__clang__)
 # define QEMU_PACKED __attribute__((gcc_struct, packed))
 #else
 # define QEMU_PACKED __attribute__((packed))
-- 
2.39.3

[RFC PATCH 6/6] gitlab-ci.d/windows: Use Clang for compiling in the 64-bit MSYS2 job

[Thomas Huth]

We are struggeling with timeouts in the 64-bit MSYS2 job. Clang seems
to be a little bit faster, so let's use this compiler now instead.

There is a problem with compiling the spice headers with Clang, though,
so we can only test this in the 32-bit builds with GCC now. And we have
to disable dbus-display - otherwise the compilation aborts in the CI.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 .gitlab-ci.d/windows.yml | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
index f086540e40..ff9e9af4bb 100644
--- a/.gitlab-ci.d/windows.yml
+++ b/.gitlab-ci.d/windows.yml
@@ -43,7 +43,6 @@
       $MINGW_TARGET-curl
       $MINGW_TARGET-cyrus-sasl
       $MINGW_TARGET-dtc
-      $MINGW_TARGET-gcc
       $MINGW_TARGET-glib2
       $MINGW_TARGET-gnutls
       $MINGW_TARGET-gtk3
@@ -63,9 +62,9 @@
       $MINGW_TARGET-SDL2
       $MINGW_TARGET-SDL2_image
       $MINGW_TARGET-snappy
-      $MINGW_TARGET-spice
       $MINGW_TARGET-usbredir
-      $MINGW_TARGET-zstd "
+      $MINGW_TARGET-zstd
+      $EXTRA_PACKAGES "
   - $env:CHERE_INVOKING = 'yes'  # Preserve the current working directory
   - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
   - mkdir build
@@ -77,13 +76,15 @@
 msys2-64bit:
   extends: .shared_msys2_builder
   variables:
-    MINGW_TARGET: mingw-w64-x86_64
-    MSYSTEM: MINGW64
+    MINGW_TARGET: mingw-w64-clang-x86_64
+    MSYSTEM: CLANG64
+    EXTRA_PACKAGES: binutils mingw-w64-clang-x86_64-clang
     # do not remove "--without-default-devices"!
     # commit 9f8e6cad65a6 ("gitlab-ci: Speed up the msys2-64bit job by using --without-default-devices"
     # changed to compile QEMU with the --without-default-devices switch
     # for the msys2 64-bit job, due to the build could not complete within
-    CONFIGURE_ARGS:  --target-list=x86_64-softmmu --without-default-devices
+    CONFIGURE_ARGS: --target-list=x86_64-softmmu --without-default-devices
+                    --cc=clang --disable-dbus-display
     # qTests don't run successfully with "--without-default-devices",
     # so let's exclude the qtests from CI for now.
     TEST_ARGS: --no-suite qtest
@@ -93,5 +94,6 @@ msys2-32bit:
   variables:
     MINGW_TARGET: mingw-w64-i686
     MSYSTEM: MINGW32
+    EXTRA_PACKAGES: mingw-w64-i686-gcc mingw-w64-i686-spice
     CONFIGURE_ARGS:  --target-list=ppc64-softmmu
     TEST_ARGS: --no-suite qtest
-- 
2.39.3

Re: [RFC PATCH 4/6] hw/i386/intel_iommu: Fix VTD_IR_TableEntry for ms_struct layout

[Daniel P. Berrangé]

On Fri, Jul 28, 2023 at 04:27:46PM +0200, Thomas Huth wrote:
> We might want to compile QEMU with Clang on Windows - but it
> does not support the __attribute__((gcc_struct)) yet. So we
> have to make sure that the structs will stay the same when
> the compiler uses the "ms_struct" layout. The VTD_IR_TableEntry
> struct is affected - rewrite it a little bit so that it works
> fine with both struct layouts.
> 
> Reported-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/hw/i386/intel_iommu.h | 14 ++++++++------
>  hw/i386/intel_iommu.c         |  2 +-
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index 89dcbc5e1e..08bf220393 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -204,18 +204,20 @@ union VTD_IR_TableEntry {
>  #endif
>          uint32_t dest_id;            /* Destination ID */
>          uint16_t source_id;          /* Source-ID */
> +        uint16_t __reserved_2;       /* Reserved 2 */
>  #if HOST_BIG_ENDIAN
> -        uint64_t __reserved_2:44;    /* Reserved 2 */
> -        uint64_t sid_vtype:2;        /* Source-ID Validation Type */
> -        uint64_t sid_q:2;            /* Source-ID Qualifier */
> +        uint32_t __reserved_3:28;    /* Reserved 3 */
> +        uint32_t sid_vtype:2;        /* Source-ID Validation Type */
> +        uint32_t sid_q:2;            /* Source-ID Qualifier */
>  #else
> -        uint64_t sid_q:2;            /* Source-ID Qualifier */
> -        uint64_t sid_vtype:2;        /* Source-ID Validation Type */
> -        uint64_t __reserved_2:44;    /* Reserved 2 */
> +        uint32_t sid_q:2;            /* Source-ID Qualifier */
> +        uint32_t sid_vtype:2;        /* Source-ID Validation Type */
> +        uint32_t __reserved_3:28;    /* Reserved 3 */

Hasn't this has changed the struct layout in the else clause

 Old layout:

   source_id : 16
   sid_q : 2 
   sid_vtype : 2
   reserved_2 : 44

 New layout

   source_id : 16
   reserved_2 : 16
   sid_q : 2 
   sid_vtype : 2
   reserved_3 : 28

Was there something wrong with the change I suggested to
just make source_id be a bitfield too:

       uint64_t source_id: 16;          /* Source-ID */

which could make ms_struct layout avoid padding to the following
bitfields.

>  #endif
>      } QEMU_PACKED irte;
>      uint64_t data[2];
>  };

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH 5/6] include/qemu/compiler: Fix problem with gcc_struct and Clang

[Peter Maydell]

On Fri, 28 Jul 2023 at 15:28, Thomas Huth <thuth@redhat.com> wrote:
>
> Clang on Windows does not seem to know the "gcc_struct" attribute
> and emits a warning when we try to use it. Add an additional check
> here with __has_attribute() to avoid this problem.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/qemu/compiler.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> index a309f90c76..5065b4447c 100644
> --- a/include/qemu/compiler.h
> +++ b/include/qemu/compiler.h
> @@ -22,7 +22,7 @@
>  #define QEMU_EXTERN_C extern
>  #endif
>
> -#if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__))
> +#if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__)) && !defined(__clang__)
>  # define QEMU_PACKED __attribute__((gcc_struct, packed))
>  #else
>  # define QEMU_PACKED __attribute__((packed))

I'm not sure about this. The idea of QEMU_PACKED is that
it's supposed to give you the same struct layout
regardless of compiler. With this change it no longer
does that, and there's no compile-time guard against
using something in a packed struct that has a different
layout on Windows clang vs everything else.

If it was OK to use plain attribute(packed) we wouldn't
need the ifdef at all because we could use it on GCC too.

thanks
-- PMM

Re: [RFC PATCH 4/6] hw/i386/intel_iommu: Fix VTD_IR_TableEntry for ms_struct layout

[Richard Henderson]

On 7/28/23 07:37, Daniel P. Berrangé wrote:
> On Fri, Jul 28, 2023 at 04:27:46PM +0200, Thomas Huth wrote:
>> We might want to compile QEMU with Clang on Windows - but it
>> does not support the __attribute__((gcc_struct)) yet. So we
>> have to make sure that the structs will stay the same when
>> the compiler uses the "ms_struct" layout. The VTD_IR_TableEntry
>> struct is affected - rewrite it a little bit so that it works
>> fine with both struct layouts.
>>
>> Reported-by: Daniel P. Berrangé <berrange@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   include/hw/i386/intel_iommu.h | 14 ++++++++------
>>   hw/i386/intel_iommu.c         |  2 +-
>>   2 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
>> index 89dcbc5e1e..08bf220393 100644
>> --- a/include/hw/i386/intel_iommu.h
>> +++ b/include/hw/i386/intel_iommu.h
>> @@ -204,18 +204,20 @@ union VTD_IR_TableEntry {
>>   #endif
>>           uint32_t dest_id;            /* Destination ID */
>>           uint16_t source_id;          /* Source-ID */
>> +        uint16_t __reserved_2;       /* Reserved 2 */
>>   #if HOST_BIG_ENDIAN
>> -        uint64_t __reserved_2:44;    /* Reserved 2 */
>> -        uint64_t sid_vtype:2;        /* Source-ID Validation Type */
>> -        uint64_t sid_q:2;            /* Source-ID Qualifier */
>> +        uint32_t __reserved_3:28;    /* Reserved 3 */
>> +        uint32_t sid_vtype:2;        /* Source-ID Validation Type */
>> +        uint32_t sid_q:2;            /* Source-ID Qualifier */
>>   #else
>> -        uint64_t sid_q:2;            /* Source-ID Qualifier */
>> -        uint64_t sid_vtype:2;        /* Source-ID Validation Type */
>> -        uint64_t __reserved_2:44;    /* Reserved 2 */
>> +        uint32_t sid_q:2;            /* Source-ID Qualifier */
>> +        uint32_t sid_vtype:2;        /* Source-ID Validation Type */
>> +        uint32_t __reserved_3:28;    /* Reserved 3 */
> 
> Hasn't this has changed the struct layout in the else clause
> 
>   Old layout:
> 
>     source_id : 16
>     sid_q : 2
>     sid_vtype : 2
>     reserved_2 : 44
> 
>   New layout
> 
>     source_id : 16
>     reserved_2 : 16
>     sid_q : 2
>     sid_vtype : 2
>     reserved_3 : 28
> 
> Was there something wrong with the change I suggested to
> just make source_id be a bitfield too:
> 
>         uint64_t source_id: 16;          /* Source-ID */
> 
> which could make ms_struct layout avoid padding to the following
> bitfields.
> 
>>   #endif
>>       } QEMU_PACKED irte;
>>       uint64_t data[2];
>>   };

Making the point that we should never use bitfields to match hardware.  This should be 
converted to <hw/registerfields.h>, ARRAY_FIELD_EX64.


r~

Re: [RFC PATCH 4/6] hw/i386/intel_iommu: Fix VTD_IR_TableEntry for ms_struct layout

[Thomas Huth]

On 28/07/2023 16.37, Daniel P. Berrangé wrote:
> On Fri, Jul 28, 2023 at 04:27:46PM +0200, Thomas Huth wrote:
>> We might want to compile QEMU with Clang on Windows - but it
>> does not support the __attribute__((gcc_struct)) yet. So we
>> have to make sure that the structs will stay the same when
>> the compiler uses the "ms_struct" layout. The VTD_IR_TableEntry
>> struct is affected - rewrite it a little bit so that it works
>> fine with both struct layouts.
>>
>> Reported-by: Daniel P. Berrangé <berrange@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   include/hw/i386/intel_iommu.h | 14 ++++++++------
>>   hw/i386/intel_iommu.c         |  2 +-
>>   2 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
>> index 89dcbc5e1e..08bf220393 100644
>> --- a/include/hw/i386/intel_iommu.h
>> +++ b/include/hw/i386/intel_iommu.h
>> @@ -204,18 +204,20 @@ union VTD_IR_TableEntry {
>>   #endif
>>           uint32_t dest_id;            /* Destination ID */
>>           uint16_t source_id;          /* Source-ID */
>> +        uint16_t __reserved_2;       /* Reserved 2 */
>>   #if HOST_BIG_ENDIAN
>> -        uint64_t __reserved_2:44;    /* Reserved 2 */
>> -        uint64_t sid_vtype:2;        /* Source-ID Validation Type */
>> -        uint64_t sid_q:2;            /* Source-ID Qualifier */
>> +        uint32_t __reserved_3:28;    /* Reserved 3 */
>> +        uint32_t sid_vtype:2;        /* Source-ID Validation Type */
>> +        uint32_t sid_q:2;            /* Source-ID Qualifier */
>>   #else
>> -        uint64_t sid_q:2;            /* Source-ID Qualifier */
>> -        uint64_t sid_vtype:2;        /* Source-ID Validation Type */
>> -        uint64_t __reserved_2:44;    /* Reserved 2 */
>> +        uint32_t sid_q:2;            /* Source-ID Qualifier */
>> +        uint32_t sid_vtype:2;        /* Source-ID Validation Type */
>> +        uint32_t __reserved_3:28;    /* Reserved 3 */
> 
> Hasn't this has changed the struct layout in the else clause
> 
>   Old layout:
> 
>     source_id : 16
>     sid_q : 2
>     sid_vtype : 2
>     reserved_2 : 44
> 
>   New layout
> 
>     source_id : 16
>     reserved_2 : 16
>     sid_q : 2
>     sid_vtype : 2
>     reserved_3 : 28

Drat, you're right, I missed the fact that the whole stuff is read and 
written via the uint64_t data[2] part from the union in the code ... :-(

> Was there something wrong with the change I suggested to
> just make source_id be a bitfield too:
> 
>         uint64_t source_id: 16;          /* Source-ID */
> 
> which could make ms_struct layout avoid padding to the following
> bitfields.

That likely works, but I think we then need to add it then twice, one time 
in the HOST_BIG_ENDIAN at the end, and one time in the #else part?

Anyway, that whole code looks like it's completely wrong on big endian 
machines. The struct is read via dma_memory_read() from guest memory, but 
then the values are never byte-swapped, except for the error_report and 
trace functions, e.g. entry->irte.present is used without calling 
le64_to_cpu() first.
entry->irte.source_id is swapped with le32_to_cpu() which looks also wrong 
since this is a 16 bit field.

Sigh. This is another good example why we shouldn't use bitfields at all in 
structures that exchange data. As Richard suggested in his reply, this 
really should be rewritten, e.g. with the stuff from hw/registerfields.h.

  Thomas

Re: [RFC PATCH 5/6] include/qemu/compiler: Fix problem with gcc_struct and Clang

[Thomas Huth]

On 28/07/2023 17.13, Peter Maydell wrote:
> On Fri, 28 Jul 2023 at 15:28, Thomas Huth <thuth@redhat.com> wrote:
>>
>> Clang on Windows does not seem to know the "gcc_struct" attribute
>> and emits a warning when we try to use it. Add an additional check
>> here with __has_attribute() to avoid this problem.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   include/qemu/compiler.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
>> index a309f90c76..5065b4447c 100644
>> --- a/include/qemu/compiler.h
>> +++ b/include/qemu/compiler.h
>> @@ -22,7 +22,7 @@
>>   #define QEMU_EXTERN_C extern
>>   #endif
>>
>> -#if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__))
>> +#if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__)) && !defined(__clang__)
>>   # define QEMU_PACKED __attribute__((gcc_struct, packed))
>>   #else
>>   # define QEMU_PACKED __attribute__((packed))
> 
> I'm not sure about this. The idea of QEMU_PACKED is that
> it's supposed to give you the same struct layout
> regardless of compiler. With this change it no longer
> does that, and there's no compile-time guard against
> using something in a packed struct that has a different
> layout on Windows clang vs everything else.
> 
> If it was OK to use plain attribute(packed) we wouldn't
> need the ifdef at all because we could use it on GCC too.

I still haven't quite grasped whether this attribute just affects structures 
with bitfields in it, or whether it could also affect other structures 
without bitfields.

Looking at https://gcc.gnu.org/onlinedocs/gcc/x86-Type-Attributes.html and 
https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html#index-mms-bitfields , it 
sounds like it only affects structs with bitfields, unless you specify an 
"aligned(x)" attribute, too?

Anyway, using bitfields in structs for exchanging data with the guest is 
just way too error-prone, as you can see in the discussion about that 
VTD_IR_TableEntry in my other patch. We should maybe advise against 
bitfields in our coding style and point people to registerfields.h instead 
for new code? ... so that we use QEMU_PACKED mainly for legacy code. Would 
it then be OK for you, Peter, to go on with this approach?

Or do you see another possibility how we could fix that timeout problem in 
the 64-bit MSYS2 job? Still switching to clang, but compiling with 
--extra-cflags="-Wno-unknown-attributes" maybe?

  Thomas

Re: [RFC PATCH 5/6] include/qemu/compiler: Fix problem with gcc_struct and Clang

[Daniel P. Berrangé]

On Mon, Jul 31, 2023 at 11:10:58AM +0200, Thomas Huth wrote:
> On 28/07/2023 17.13, Peter Maydell wrote:
> > On Fri, 28 Jul 2023 at 15:28, Thomas Huth <thuth@redhat.com> wrote:
> > > 
> > > Clang on Windows does not seem to know the "gcc_struct" attribute
> > > and emits a warning when we try to use it. Add an additional check
> > > here with __has_attribute() to avoid this problem.
> > > 
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > ---
> > >   include/qemu/compiler.h | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> > > index a309f90c76..5065b4447c 100644
> > > --- a/include/qemu/compiler.h
> > > +++ b/include/qemu/compiler.h
> > > @@ -22,7 +22,7 @@
> > >   #define QEMU_EXTERN_C extern
> > >   #endif
> > > 
> > > -#if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__))
> > > +#if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__)) && !defined(__clang__)
> > >   # define QEMU_PACKED __attribute__((gcc_struct, packed))
> > >   #else
> > >   # define QEMU_PACKED __attribute__((packed))
> > 
> > I'm not sure about this. The idea of QEMU_PACKED is that
> > it's supposed to give you the same struct layout
> > regardless of compiler. With this change it no longer
> > does that, and there's no compile-time guard against
> > using something in a packed struct that has a different
> > layout on Windows clang vs everything else.
> > 
> > If it was OK to use plain attribute(packed) we wouldn't
> > need the ifdef at all because we could use it on GCC too.
> 
> I still haven't quite grasped whether this attribute just affects structures
> with bitfields in it, or whether it could also affect other structures
> without bitfields.
> 
> Looking at https://gcc.gnu.org/onlinedocs/gcc/x86-Type-Attributes.html and
> https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html#index-mms-bitfields , it
> sounds like it only affects structs with bitfields, unless you specify an
> "aligned(x)" attribute, too?
> 
> Anyway, using bitfields in structs for exchanging data with the guest is
> just way too error-prone, as you can see in the discussion about that
> VTD_IR_TableEntry in my other patch. We should maybe advise against
> bitfields in our coding style and point people to registerfields.h instead
> for new code? ... so that we use QEMU_PACKED mainly for legacy code. Would
> it then be OK for you, Peter, to go on with this approach?
> 
> Or do you see another possibility how we could fix that timeout problem in
> the 64-bit MSYS2 job? Still switching to clang, but compiling with
> --extra-cflags="-Wno-unknown-attributes" maybe?

I was surprised to see that we're not using ccache in gitlab CI. It wouldn't
help the from-clean compile time, but thereafter it ought to help. I'm doing
some tests with that to see the impact.

Another option might be to try precompiled headers, which meson supports
quite nicely / transparently. Might especially help on Windows where the
entire world is declared in windows.h


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH 5/6] include/qemu/compiler: Fix problem with gcc_struct and Clang

[Peter Maydell]

On Mon, 31 Jul 2023 at 10:11, Thomas Huth <thuth@redhat.com> wrote:
> Or do you see another possibility how we could fix that timeout problem in
> the 64-bit MSYS2 job? Still switching to clang, but compiling with
> --extra-cflags="-Wno-unknown-attributes" maybe?

Is there any way we can separate out the "take 25 minutes to
install a pile of packages" part from the "actually compile and
test QEMU" part ?

thanks
-- PMM

Re: [RFC PATCH 5/6] include/qemu/compiler: Fix problem with gcc_struct and Clang

[Thomas Huth]

On 31/07/2023 11.32, Daniel P. Berrangé wrote:
> On Mon, Jul 31, 2023 at 11:10:58AM +0200, Thomas Huth wrote:
...
>> Or do you see another possibility how we could fix that timeout problem in
>> the 64-bit MSYS2 job? Still switching to clang, but compiling with
>> --extra-cflags="-Wno-unknown-attributes" maybe?
> 
> I was surprised to see that we're not using ccache in gitlab CI. It wouldn't
> help the from-clean compile time, but thereafter it ought to help. I'm doing
> some tests with that to see the impact.

I tried that two years ago, but the results were rather disappointing:

  https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg02189.html

... but that was only with the Linux runners. Maybe it helps with the MSYS 
runners?

  Thomas

Re: [RFC PATCH 5/6] include/qemu/compiler: Fix problem with gcc_struct and Clang

[Thomas Huth]

On 31/07/2023 12.05, Peter Maydell wrote:
> On Mon, 31 Jul 2023 at 10:11, Thomas Huth <thuth@redhat.com> wrote:
>> Or do you see another possibility how we could fix that timeout problem in
>> the 64-bit MSYS2 job? Still switching to clang, but compiling with
>> --extra-cflags="-Wno-unknown-attributes" maybe?
> 
> Is there any way we can separate out the "take 25 minutes to
> install a pile of packages" part from the "actually compile and
> test QEMU" part ?

At least I don't have a clue ... we'd need someone who knows how to deal 
with that Windows stuff...

  Thomas

Re: [RFC PATCH 5/6] include/qemu/compiler: Fix problem with gcc_struct and Clang

[Daniel P. Berrangé]

On Mon, Jul 31, 2023 at 11:05:42AM +0100, Peter Maydell wrote:
> On Mon, 31 Jul 2023 at 10:11, Thomas Huth <thuth@redhat.com> wrote:
> > Or do you see another possibility how we could fix that timeout problem in
> > the 64-bit MSYS2 job? Still switching to clang, but compiling with
> > --extra-cflags="-Wno-unknown-attributes" maybe?
> 
> Is there any way we can separate out the "take 25 minutes to
> install a pile of packages" part from the "actually compile and
> test QEMU" part ?

Possibly, depends if msys installs all the packages to a specific
sub-directory location, or splatters all over the hierarchy. If the
former we might be able to put the whole installed sub-tree into the
gitlab cache, which is likely faster to deploy. Testing this out in
gitlab itself is kinda tedious, so needs a local windows VM to debug.
If msys relies on the registry for anything during package install,
then all bets are off, as I don't know how we'd cache & restore that.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH 5/6] include/qemu/compiler: Fix problem with gcc_struct and Clang

[Daniel P. Berrangé]

On Mon, Jul 31, 2023 at 11:10:58AM +0200, Thomas Huth wrote:
> Anyway, using bitfields in structs for exchanging data with the guest is
> just way too error-prone, as you can see in the discussion about that
> VTD_IR_TableEntry in my other patch. We should maybe advise against
> bitfields in our coding style and point people to registerfields.h instead
> for new code? ... so that we use QEMU_PACKED mainly for legacy code. Would
> it then be OK for you, Peter, to go on with this approach?

The registerfields.h usage doesn't seem to be documented at all from the
quick look I had. IOW, in addition to a mention in coding style, it would
be nice if someone can document the general usage pattern for it.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH 5/6] include/qemu/compiler: Fix problem with gcc_struct and Clang

[Philippe Mathieu-Daudé]

On 31/7/23 14:57, Daniel P. Berrangé wrote:
> On Mon, Jul 31, 2023 at 11:10:58AM +0200, Thomas Huth wrote:
>> Anyway, using bitfields in structs for exchanging data with the guest is
>> just way too error-prone, as you can see in the discussion about that
>> VTD_IR_TableEntry in my other patch. We should maybe advise against
>> bitfields in our coding style and point people to registerfields.h instead
>> for new code? ... so that we use QEMU_PACKED mainly for legacy code. Would
>> it then be OK for you, Peter, to go on with this approach?
> 
> The registerfields.h usage doesn't seem to be documented at all from the
> quick look I had. IOW, in addition to a mention in coding style, it would
> be nice if someone can document the general usage pattern for it.

Cc'ing ex-Xilinx folks.

> With regards,
> Daniel

Re: [RFC PATCH 5/6] include/qemu/compiler: Fix problem with gcc_struct and Clang

[Philippe Mathieu-Daudé]

On 31/7/23 11:32, Daniel P. Berrangé wrote:

> I was surprised to see that we're not using ccache in gitlab CI. It wouldn't
> help the from-clean compile time, but thereafter it ought to help. I'm doing
> some tests with that to see the impact.

I tried that few years ago and this had very negative impact on custom
runners (maybe I wasn't doing it correctly). Hopefully that changed.

See some previous comments:
https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg02220.html

> Another option might be to try precompiled headers, which meson supports
> quite nicely / transparently. Might especially help on Windows where the
> entire world is declared in windows.h
> 
> 
> With regards,
> Daniel

Re: [RFC PATCH 3/6] util/oslib-win32: Fix compiling with Clang from MSYS2

[Philippe Mathieu-Daudé]

On 28/7/23 16:27, Thomas Huth wrote:
> Clang complains:
> 
> ../util/oslib-win32.c:483:56: error: omitting the parameter name in a
>   function definition is a C2x extension [-Werror,-Wc2x-extensions]
> win32_close_exception_handler(struct _EXCEPTION_RECORD*,
>                                                         ^
> 
> Fix it by adding parameter names.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   util/oslib-win32.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 429542face..070bb455d3 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -480,8 +480,8 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr *addr,
>   }
>   
>   EXCEPTION_DISPOSITION
> -win32_close_exception_handler(struct _EXCEPTION_RECORD*,
> -                              void*, struct _CONTEXT*, void*)
> +win32_close_exception_handler(struct _EXCEPTION_RECORD *exrec,
> +                              void *ptr1, struct _CONTEXT *cntx, void *ptr2)

Per https://learn.microsoft.com/en-us/cpp/c-runtime-library/except-handler3:

-- >8 --
diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index 91aa0d7ec0..cb42508745 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -260,8 +260,9 @@ ssize_t qemu_recvfrom_wrap(int sockfd, void *buf, 
size_t len, int flags,
                             struct sockaddr *addr, socklen_t *addrlen);

  EXCEPTION_DISPOSITION
-win32_close_exception_handler(struct _EXCEPTION_RECORD*, void*,
-                              struct _CONTEXT*, void*);
+win32_close_exception_handler(struct _EXCEPTION_RECORD *exception_record,
+                              void *registration, struct _CONTEXT *context,
+                              void *dispatcher);

  void *qemu_win32_map_alloc(size_t size, HANDLE *h, Error **errp);
  void qemu_win32_map_free(void *ptr, HANDLE h, Error **errp);
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 429542face..19a0ea7fbe 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -480,8 +480,9 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr 
*addr,
  }

  EXCEPTION_DISPOSITION
-win32_close_exception_handler(struct _EXCEPTION_RECORD*,
-                              void*, struct _CONTEXT*, void*)
+win32_close_exception_handler(struct _EXCEPTION_RECORD *exception_record,
+                              void *registration, struct _CONTEXT *context,
+                              void *dispatcher)
  {
      return EXCEPTION_EXECUTE_HANDLER;
  }
---

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [RFC PATCH 6/6] gitlab-ci.d/windows: Use Clang for compiling in the 64-bit MSYS2 job

[Philippe Mathieu-Daudé]

On 28/7/23 16:27, Thomas Huth wrote:
> We are struggeling with timeouts in the 64-bit MSYS2 job. Clang seems
> to be a little bit faster, so let's use this compiler now instead.
> 
> There is a problem with compiling the spice headers with Clang, though,
> so we can only test this in the 32-bit builds with GCC now. And we have
> to disable dbus-display - otherwise the compilation aborts in the CI.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   .gitlab-ci.d/windows.yml | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
> index f086540e40..ff9e9af4bb 100644
> --- a/.gitlab-ci.d/windows.yml
> +++ b/.gitlab-ci.d/windows.yml
> @@ -43,7 +43,6 @@
>         $MINGW_TARGET-curl
>         $MINGW_TARGET-cyrus-sasl
>         $MINGW_TARGET-dtc
> -      $MINGW_TARGET-gcc
>         $MINGW_TARGET-glib2
>         $MINGW_TARGET-gnutls
>         $MINGW_TARGET-gtk3
> @@ -63,9 +62,9 @@
>         $MINGW_TARGET-SDL2
>         $MINGW_TARGET-SDL2_image
>         $MINGW_TARGET-snappy
> -      $MINGW_TARGET-spice
>         $MINGW_TARGET-usbredir
> -      $MINGW_TARGET-zstd "
> +      $MINGW_TARGET-zstd
> +      $EXTRA_PACKAGES "
>     - $env:CHERE_INVOKING = 'yes'  # Preserve the current working directory
>     - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
>     - mkdir build
> @@ -77,13 +76,15 @@
>   msys2-64bit:
>     extends: .shared_msys2_builder
>     variables:
> -    MINGW_TARGET: mingw-w64-x86_64
> -    MSYSTEM: MINGW64
> +    MINGW_TARGET: mingw-w64-clang-x86_64
> +    MSYSTEM: CLANG64

OK to use Clang, but I'm tempted to keep the GCC job in manual mode...

Re: [RFC PATCH 6/6] gitlab-ci.d/windows: Use Clang for compiling in the 64-bit MSYS2 job

[Thomas Huth]

On 31/07/2023 16.23, Philippe Mathieu-Daudé wrote:
> On 28/7/23 16:27, Thomas Huth wrote:
>> We are struggeling with timeouts in the 64-bit MSYS2 job. Clang seems
>> to be a little bit faster, so let's use this compiler now instead.
>>
>> There is a problem with compiling the spice headers with Clang, though,
>> so we can only test this in the 32-bit builds with GCC now. And we have
>> to disable dbus-display - otherwise the compilation aborts in the CI.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   .gitlab-ci.d/windows.yml | 14 ++++++++------
>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
>> index f086540e40..ff9e9af4bb 100644
>> --- a/.gitlab-ci.d/windows.yml
>> +++ b/.gitlab-ci.d/windows.yml
>> @@ -43,7 +43,6 @@
>>         $MINGW_TARGET-curl
>>         $MINGW_TARGET-cyrus-sasl
>>         $MINGW_TARGET-dtc
>> -      $MINGW_TARGET-gcc
>>         $MINGW_TARGET-glib2
>>         $MINGW_TARGET-gnutls
>>         $MINGW_TARGET-gtk3
>> @@ -63,9 +62,9 @@
>>         $MINGW_TARGET-SDL2
>>         $MINGW_TARGET-SDL2_image
>>         $MINGW_TARGET-snappy
>> -      $MINGW_TARGET-spice
>>         $MINGW_TARGET-usbredir
>> -      $MINGW_TARGET-zstd "
>> +      $MINGW_TARGET-zstd
>> +      $EXTRA_PACKAGES "
>>     - $env:CHERE_INVOKING = 'yes'  # Preserve the current working directory
>>     - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
>>     - mkdir build
>> @@ -77,13 +76,15 @@
>>   msys2-64bit:
>>     extends: .shared_msys2_builder
>>     variables:
>> -    MINGW_TARGET: mingw-w64-x86_64
>> -    MSYSTEM: MINGW64
>> +    MINGW_TARGET: mingw-w64-clang-x86_64
>> +    MSYSTEM: CLANG64
> 
> OK to use Clang, but I'm tempted to keep the GCC job in manual mode...

Why? We still have the 32-bit job with GCC, and the MinGW cross-compiler job 
with GCC, so that's already quite a bit of coverage, isn't it?

  Thomas

Re: [RFC PATCH 5/6] include/qemu/compiler: Fix problem with gcc_struct and Clang

[Daniel P. Berrangé]

On Mon, Jul 31, 2023 at 04:10:36PM +0200, Philippe Mathieu-Daudé wrote:
> On 31/7/23 11:32, Daniel P. Berrangé wrote:
> 
> > I was surprised to see that we're not using ccache in gitlab CI. It wouldn't
> > help the from-clean compile time, but thereafter it ought to help. I'm doing
> > some tests with that to see the impact.
> 
> I tried that few years ago and this had very negative impact on custom
> runners (maybe I wasn't doing it correctly). Hopefully that changed.

Our runner usage model has changed since then quite alot. What was previously
mostly on shared runners, is now on Azure private runners. I can imagine it
will vary tremendously on what you're using as a private runner.

In the specific case of the windows jobs though, we're using the shared
runners.

Either way, if our jobs are all wired up for ccache correctly, it is then
trivial to selectively turn off usage of ccache by just tweaking a few
env vars.

> See some previous comments:
> https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg02220.html
> 
> > Another option might be to try precompiled headers, which meson supports
> > quite nicely / transparently. Might especially help on Windows where the
> > entire world is declared in windows.h
> > 
> > 
> > With regards,
> > Daniel
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH 4/6] hw/i386/intel_iommu: Fix VTD_IR_TableEntry for ms_struct layout

[Peter Xu]

On Mon, Jul 31, 2023 at 10:20:40AM +0200, Thomas Huth wrote:
> On 28/07/2023 16.37, Daniel P. Berrangé wrote:
> > On Fri, Jul 28, 2023 at 04:27:46PM +0200, Thomas Huth wrote:
> > > We might want to compile QEMU with Clang on Windows - but it
> > > does not support the __attribute__((gcc_struct)) yet. So we
> > > have to make sure that the structs will stay the same when
> > > the compiler uses the "ms_struct" layout. The VTD_IR_TableEntry
> > > struct is affected - rewrite it a little bit so that it works
> > > fine with both struct layouts.
> > > 
> > > Reported-by: Daniel P. Berrangé <berrange@redhat.com>
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > ---
> > >   include/hw/i386/intel_iommu.h | 14 ++++++++------
> > >   hw/i386/intel_iommu.c         |  2 +-
> > >   2 files changed, 9 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> > > index 89dcbc5e1e..08bf220393 100644
> > > --- a/include/hw/i386/intel_iommu.h
> > > +++ b/include/hw/i386/intel_iommu.h
> > > @@ -204,18 +204,20 @@ union VTD_IR_TableEntry {
> > >   #endif
> > >           uint32_t dest_id;            /* Destination ID */
> > >           uint16_t source_id;          /* Source-ID */
> > > +        uint16_t __reserved_2;       /* Reserved 2 */
> > >   #if HOST_BIG_ENDIAN
> > > -        uint64_t __reserved_2:44;    /* Reserved 2 */
> > > -        uint64_t sid_vtype:2;        /* Source-ID Validation Type */
> > > -        uint64_t sid_q:2;            /* Source-ID Qualifier */
> > > +        uint32_t __reserved_3:28;    /* Reserved 3 */
> > > +        uint32_t sid_vtype:2;        /* Source-ID Validation Type */
> > > +        uint32_t sid_q:2;            /* Source-ID Qualifier */
> > >   #else
> > > -        uint64_t sid_q:2;            /* Source-ID Qualifier */
> > > -        uint64_t sid_vtype:2;        /* Source-ID Validation Type */
> > > -        uint64_t __reserved_2:44;    /* Reserved 2 */
> > > +        uint32_t sid_q:2;            /* Source-ID Qualifier */
> > > +        uint32_t sid_vtype:2;        /* Source-ID Validation Type */
> > > +        uint32_t __reserved_3:28;    /* Reserved 3 */
> > 
> > Hasn't this has changed the struct layout in the else clause
> > 
> >   Old layout:
> > 
> >     source_id : 16
> >     sid_q : 2
> >     sid_vtype : 2
> >     reserved_2 : 44
> > 
> >   New layout
> > 
> >     source_id : 16
> >     reserved_2 : 16
> >     sid_q : 2
> >     sid_vtype : 2
> >     reserved_3 : 28
> 
> Drat, you're right, I missed the fact that the whole stuff is read and
> written via the uint64_t data[2] part from the union in the code ... :-(

Yes, that's actually part of the VT-d spec.

> 
> > Was there something wrong with the change I suggested to
> > just make source_id be a bitfield too:
> > 
> >         uint64_t source_id: 16;          /* Source-ID */
> > 
> > which could make ms_struct layout avoid padding to the following
> > bitfields.
> 
> That likely works, but I think we then need to add it then twice, one time
> in the HOST_BIG_ENDIAN at the end, and one time in the #else part?
> 
> Anyway, that whole code looks like it's completely wrong on big endian
> machines. The struct is read via dma_memory_read() from guest memory, but
> then the values are never byte-swapped, except for the error_report and
> trace functions, e.g. entry->irte.present is used without calling
> le64_to_cpu() first.
> entry->irte.source_id is swapped with le32_to_cpu() which looks also wrong
> since this is a 16 bit field.
> 
> Sigh. This is another good example why we shouldn't use bitfields at all in
> structures that exchange data. As Richard suggested in his reply, this
> really should be rewritten, e.g. with the stuff from hw/registerfields.h.

I can definitely review the iommu-side changes if someone would like to
finally enable that for either clang or whatever purpose.  Sorry if it
never worked..

But then if it's broken for 7 years since the start, it probably also means
no one ever used it on big endian hosts, either, as a functionality.. so
another approach is we can opt-out VT-d as a whole for big endian, if
that's easier.

Thanks,

-- 
Peter Xu

Re: [RFC PATCH 4/6] hw/i386/intel_iommu: Fix VTD_IR_TableEntry for ms_struct layout

[Michael S. Tsirkin]

On Mon, Jul 31, 2023 at 04:26:44PM -0400, Peter Xu wrote:
> On Mon, Jul 31, 2023 at 10:20:40AM +0200, Thomas Huth wrote:
> > On 28/07/2023 16.37, Daniel P. Berrangé wrote:
> > > On Fri, Jul 28, 2023 at 04:27:46PM +0200, Thomas Huth wrote:
> > > > We might want to compile QEMU with Clang on Windows - but it
> > > > does not support the __attribute__((gcc_struct)) yet. So we
> > > > have to make sure that the structs will stay the same when
> > > > the compiler uses the "ms_struct" layout. The VTD_IR_TableEntry
> > > > struct is affected - rewrite it a little bit so that it works
> > > > fine with both struct layouts.
> > > > 
> > > > Reported-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > > ---
> > > >   include/hw/i386/intel_iommu.h | 14 ++++++++------
> > > >   hw/i386/intel_iommu.c         |  2 +-
> > > >   2 files changed, 9 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> > > > index 89dcbc5e1e..08bf220393 100644
> > > > --- a/include/hw/i386/intel_iommu.h
> > > > +++ b/include/hw/i386/intel_iommu.h
> > > > @@ -204,18 +204,20 @@ union VTD_IR_TableEntry {
> > > >   #endif
> > > >           uint32_t dest_id;            /* Destination ID */
> > > >           uint16_t source_id;          /* Source-ID */
> > > > +        uint16_t __reserved_2;       /* Reserved 2 */
> > > >   #if HOST_BIG_ENDIAN
> > > > -        uint64_t __reserved_2:44;    /* Reserved 2 */
> > > > -        uint64_t sid_vtype:2;        /* Source-ID Validation Type */
> > > > -        uint64_t sid_q:2;            /* Source-ID Qualifier */
> > > > +        uint32_t __reserved_3:28;    /* Reserved 3 */
> > > > +        uint32_t sid_vtype:2;        /* Source-ID Validation Type */
> > > > +        uint32_t sid_q:2;            /* Source-ID Qualifier */
> > > >   #else
> > > > -        uint64_t sid_q:2;            /* Source-ID Qualifier */
> > > > -        uint64_t sid_vtype:2;        /* Source-ID Validation Type */
> > > > -        uint64_t __reserved_2:44;    /* Reserved 2 */
> > > > +        uint32_t sid_q:2;            /* Source-ID Qualifier */
> > > > +        uint32_t sid_vtype:2;        /* Source-ID Validation Type */
> > > > +        uint32_t __reserved_3:28;    /* Reserved 3 */
> > > 
> > > Hasn't this has changed the struct layout in the else clause
> > > 
> > >   Old layout:
> > > 
> > >     source_id : 16
> > >     sid_q : 2
> > >     sid_vtype : 2
> > >     reserved_2 : 44
> > > 
> > >   New layout
> > > 
> > >     source_id : 16
> > >     reserved_2 : 16
> > >     sid_q : 2
> > >     sid_vtype : 2
> > >     reserved_3 : 28
> > 
> > Drat, you're right, I missed the fact that the whole stuff is read and
> > written via the uint64_t data[2] part from the union in the code ... :-(
> 
> Yes, that's actually part of the VT-d spec.
> 
> > 
> > > Was there something wrong with the change I suggested to
> > > just make source_id be a bitfield too:
> > > 
> > >         uint64_t source_id: 16;          /* Source-ID */
> > > 
> > > which could make ms_struct layout avoid padding to the following
> > > bitfields.
> > 
> > That likely works, but I think we then need to add it then twice, one time
> > in the HOST_BIG_ENDIAN at the end, and one time in the #else part?
> > 
> > Anyway, that whole code looks like it's completely wrong on big endian
> > machines. The struct is read via dma_memory_read() from guest memory, but
> > then the values are never byte-swapped, except for the error_report and
> > trace functions, e.g. entry->irte.present is used without calling
> > le64_to_cpu() first.
> > entry->irte.source_id is swapped with le32_to_cpu() which looks also wrong
> > since this is a 16 bit field.
> > 
> > Sigh. This is another good example why we shouldn't use bitfields at all in
> > structures that exchange data. As Richard suggested in his reply, this
> > really should be rewritten, e.g. with the stuff from hw/registerfields.h.
> 
> I can definitely review the iommu-side changes if someone would like to
> finally enable that for either clang or whatever purpose.  Sorry if it
> never worked..
> 
> But then if it's broken for 7 years since the start, it probably also means
> no one ever used it on big endian hosts, either, as a functionality.. so
> another approach is we can opt-out VT-d as a whole for big endian, if
> that's easier.
> 
> Thanks,

Let's just fix it properly please. Bad code proliferates.


-- 
MST

Re: [RFC PATCH 5/6] include/qemu/compiler: Fix problem with gcc_struct and Clang

[Philippe Mathieu-Daudé]

On 31/7/23 19:25, Daniel P. Berrangé wrote:
> On Mon, Jul 31, 2023 at 04:10:36PM +0200, Philippe Mathieu-Daudé wrote:
>> On 31/7/23 11:32, Daniel P. Berrangé wrote:
>>
>>> I was surprised to see that we're not using ccache in gitlab CI. It wouldn't
>>> help the from-clean compile time, but thereafter it ought to help. I'm doing
>>> some tests with that to see the impact.
>>
>> I tried that few years ago and this had very negative impact on custom
>> runners (maybe I wasn't doing it correctly). Hopefully that changed.
> 
> Our runner usage model has changed since then quite alot. What was previously
> mostly on shared runners, is now on Azure private runners. I can imagine it
> will vary tremendously on what you're using as a private runner.
> 
> In the specific case of the windows jobs though, we're using the shared
> runners.
> 
> Either way, if our jobs are all wired up for ccache correctly, it is then
> trivial to selectively turn off usage of ccache by just tweaking a few
> env vars.

IIRC we weren't using runner tag to filter, so jobs expected to run on
shared runner were ending on private runner, and using env vars was not
working. But you are right, the comments I pointed are obsolete, I
clearly haven't followed all CI changes. Thanks for the tips,

Phil.

Re: [RFC PATCH 6/6] gitlab-ci.d/windows: Use Clang for compiling in the 64-bit MSYS2 job

[Philippe Mathieu-Daudé]

On 31/7/23 16:43, Thomas Huth wrote:
> On 31/07/2023 16.23, Philippe Mathieu-Daudé wrote:
>> On 28/7/23 16:27, Thomas Huth wrote:
>>> We are struggeling with timeouts in the 64-bit MSYS2 job. Clang seems
>>> to be a little bit faster, so let's use this compiler now instead.
>>>
>>> There is a problem with compiling the spice headers with Clang, though,
>>> so we can only test this in the 32-bit builds with GCC now. And we have
>>> to disable dbus-display - otherwise the compilation aborts in the CI.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   .gitlab-ci.d/windows.yml | 14 ++++++++------
>>>   1 file changed, 8 insertions(+), 6 deletions(-)


>>> @@ -77,13 +76,15 @@
>>>   msys2-64bit:
>>>     extends: .shared_msys2_builder
>>>     variables:
>>> -    MINGW_TARGET: mingw-w64-x86_64
>>> -    MSYSTEM: MINGW64
>>> +    MINGW_TARGET: mingw-w64-clang-x86_64
>>> +    MSYSTEM: CLANG64
>>
>> OK to use Clang, but I'm tempted to keep the GCC job in manual mode...
> 
> Why? We still have the 32-bit job with GCC, and the MinGW cross-compiler 
> job with GCC, so that's already quite a bit of coverage, isn't it?

OK we are good then :)

Thanks,

Phil.

Re: [RFC PATCH 0/6] Use Clang for compiling in the 64-bit MSYS2 job

[Daniel P. Berrangé]

On Fri, Jul 28, 2023 at 04:27:42PM +0200, Thomas Huth wrote:
> The 64-bit MSYS2 job often times out in our CI, though we already have
> limited it to a very minimum by using --without-default-devices etc.
> GCC is incredibly slow here. By using Clang instead of GCC, the job
> is ca. 15 minutes faster - that's enough buffer to avoid the timeouts
> here.

FYI, I have a complementary series here that achieves a 25 minute
speedup cummulative speedup with GCC - the ideas would apply to
clang too.

https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg00055.html


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH 5/6] include/qemu/compiler: Fix problem with gcc_struct and Clang

[Daniel P. Berrangé]

On Mon, Jul 31, 2023 at 11:05:42AM +0100, Peter Maydell wrote:
> On Mon, 31 Jul 2023 at 10:11, Thomas Huth <thuth@redhat.com> wrote:
> > Or do you see another possibility how we could fix that timeout problem in
> > the 64-bit MSYS2 job? Still switching to clang, but compiling with
> > --extra-cflags="-Wno-unknown-attributes" maybe?
> 
> Is there any way we can separate out the "take 25 minutes to
> install a pile of packages" part from the "actually compile and
> test QEMU" part ?

I was thinking a little about what we actually aim to achieve with the
Windows MSys2 jobs.

We have long had the mingw cross compilation jobs for testing the Windows
platform compile phase. Please correct me if I'm wrong, but IIUC, the
msys2 jobs haven't identified any compilation problems that weren't already
found from the linux mingw cross compile jobs.

The definitely unique thing that the msys2 jobs *can* do though, is to
run the test suite. We can't run the test suite from the mingw cross jobs
unless we do something slightly crazy like adding Wine to the containers.
In libvirt we considered the latter until we realized that the smallest
possible wine install would still be enourmous.

In the Linux world we have the jobs split into three stages

  * Create the container images  (eg amd64-fedora-container)
  * Do the compilation work      (eg build-system-fedora)
  * Run the test suite           (eg check-system-fedora/avocado-system-fedora)

If the value of the msys2 jobs is that they let us run the test suite,
can we limit the usage of msys2 to just that part, and chain it upto
the fedora mingw cross compile jobs ie.

  win64-fedora-cross-container
    |
    +-> cross-win64-system
          |
	  +-> check-win64-msys

In the "cross-win64-system" job we would have to publish the entire QEMU
'build' directory as an artifact, to pass it over to the msys job.  If
we also published /usr/x86_64-w64-mingw32/ as an artifact, then we would
not need to install any mingw packages under msys. The basic msys installer
can be run (which takes a couple of minutes), and then then we just dump
the Fedora artifacts of /usr/x86_64-w64-mingw32/ into the right place
and run the test suite.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH 5/6] include/qemu/compiler: Fix problem with gcc_struct and Clang

[Thomas Huth]

On 01/08/2023 15.49, Daniel P. Berrangé wrote:
...
> If the value of the msys2 jobs is that they let us run the test suite,
> can we limit the usage of msys2 to just that part, and chain it upto
> the fedora mingw cross compile jobs ie.
> 
>    win64-fedora-cross-container
>      |
>      +-> cross-win64-system
>            |
> 	  +-> check-win64-msys
> 
> In the "cross-win64-system" job we would have to publish the entire QEMU
> 'build' directory as an artifact, to pass it over to the msys job.  If
> we also published /usr/x86_64-w64-mingw32/ as an artifact, then we would
> not need to install any mingw packages under msys. The basic msys installer
> can be run (which takes a couple of minutes), and then then we just dump
> the Fedora artifacts of /usr/x86_64-w64-mingw32/ into the right place
> and run the test suite.

It's a nice idea at a first glance - but at a second glance I doubt that 
this is easy to realize. You need the configured meson environment for 
running the tests, and you cannot easily pass that from the Fedora container 
to MSYS2. So you'd either need to re-run meson setup in the MSYS2 job and 
then trick it into believing that the binaries have already been built, or 
you have to run the test binaries "manually" without meson... both might be 
possible, but it sounds rather fragile to me.

  Thomas

Re: [RFC PATCH 5/6] include/qemu/compiler: Fix problem with gcc_struct and Clang

[Daniel P. Berrangé]

On Mon, Jul 31, 2023 at 02:04:42PM +0200, Thomas Huth wrote:
> On 31/07/2023 11.32, Daniel P. Berrangé wrote:
> > On Mon, Jul 31, 2023 at 11:10:58AM +0200, Thomas Huth wrote:
> ...
> > > Or do you see another possibility how we could fix that timeout problem in
> > > the 64-bit MSYS2 job? Still switching to clang, but compiling with
> > > --extra-cflags="-Wno-unknown-attributes" maybe?
> > 
> > I was surprised to see that we're not using ccache in gitlab CI. It wouldn't
> > help the from-clean compile time, but thereafter it ought to help. I'm doing
> > some tests with that to see the impact.
> 
> I tried that two years ago, but the results were rather disappointing:
> 
>  https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg02189.html
> 
> ... but that was only with the Linux runners. Maybe it helps with the MSYS
> runners?

I've done some tests over the last few days and my results are
rather different.  The speed up is *spectacular* if the hit rate
is high.

eg, consider you are retriggering broadly the same pipeline over &
over with iterations of your patches. eg 99% of the QEMU code
probably doesn't change as you're just re-trying to fix some edge
case in 1 file, and thus you'll get near 100% hit rate.

With such a case I see:

The msys64 job can complete in 24 minutes, instead of 50-60 minutes

The build-system-fedora job can complete in 6 minutes instead of 20
minutes

The build-user-hexagon job can cmplete in 3 minutes instead of 6

Basically a 50% cut in time on all compile jobs I've looked at
so far.

NB, these are both shared runners, since the pipeline is in my
fork. I've no idea what the impact will be on the Azure private
runners for upstream.

Given the CI limitations these days, such a speedup is massively
beneficial for our contributors though.

The downside is storage for the cache. In theory gitlab has defined
a quota for storage per account. In practice they are still yet to
enforce that. If they do ever enforce it, we're likely doomed as
our container images alone will exceed it several times over, let
alone leave room for build logs, job caches, artifacts, etc. While
storage quota isn't enforced though, we might as well use ccache.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|