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

["Daniel P. Berrangé" <berrange@redhat.com>]

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