Re: [Xen-devel] [PATCH v6 6/8] AMD/IOMMU: tidy struct ivrs_mappings

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Jan 
> Beulich
> Sent: 19 September 2019 14:24
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Suravee Suthikulpanit 
> <suravee.suthikulpanit@xxxxxxx>
> Subject: [Xen-devel] [PATCH v6 6/8] AMD/IOMMU: tidy struct ivrs_mappings
> 
> Move the device flags field up into an unused hole, thus shrinking
> overall structure size by 8 bytes. Use bool and uint<N>_t as
> appropriate. Drop pointless (redundant) initializations.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

...although I wonder...

> ---
> v6: New.
> 
> ---
>  xen/drivers/passthrough/amd/iommu_acpi.c |    6 +++---
>  xen/drivers/passthrough/amd/iommu_init.c |    6 ------
>  xen/include/asm-x86/amd-iommu.h          |   17 +++++++++--------
>  3 files changed, 12 insertions(+), 17 deletions(-)
> 
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -165,7 +165,7 @@ static void __init reserve_unity_map_for
>      /* extend r/w permissioms and keep aggregate */
>      ivrs_mappings[bdf].write_permission = iw;
>      ivrs_mappings[bdf].read_permission = ir;
> -    ivrs_mappings[bdf].unity_map_enable = IOMMU_CONTROL_ENABLED;
> +    ivrs_mappings[bdf].unity_map_enable = true;
>      ivrs_mappings[bdf].addr_range_start = base;
>      ivrs_mappings[bdf].addr_range_length = length;
>  }
> @@ -242,8 +242,8 @@ static int __init register_exclusion_ran
>      if ( limit >= iommu_top  )
>      {
>          reserve_iommu_exclusion_range(iommu, base, limit);
> -        ivrs_mappings[bdf].dte_allow_exclusion = IOMMU_CONTROL_ENABLED;
> -        ivrs_mappings[req].dte_allow_exclusion = IOMMU_CONTROL_ENABLED;
> +        ivrs_mappings[bdf].dte_allow_exclusion = true;
> +        ivrs_mappings[req].dte_allow_exclusion = true;
>      }
> 
>      return 0;
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -1222,12 +1222,6 @@ static int __init alloc_ivrs_mappings(u1
>      for ( bdf = 0; bdf < ivrs_bdf_entries; bdf++ )
>      {
>          ivrs_mappings[bdf].dte_requestor_id = bdf;
> -        ivrs_mappings[bdf].dte_allow_exclusion = IOMMU_CONTROL_DISABLED;
> -        ivrs_mappings[bdf].unity_map_enable = IOMMU_CONTROL_DISABLED;
> -        ivrs_mappings[bdf].iommu = NULL;
> -
> -        ivrs_mappings[bdf].intremap_table = NULL;
> -        ivrs_mappings[bdf].device_flags = 0;
> 
>          if ( amd_iommu_perdev_intremap )
>              spin_lock_init(&ivrs_mappings[bdf].intremap_lock);
> --- a/xen/include/asm-x86/amd-iommu.h
> +++ b/xen/include/asm-x86/amd-iommu.h
> @@ -106,12 +106,16 @@ struct amd_iommu {
>  };
> 
>  struct ivrs_mappings {
> -    u16 dte_requestor_id;
> -    u8 dte_allow_exclusion;
> -    u8 unity_map_enable;
> -    u8 write_permission;
> -    u8 read_permission;
> +    uint16_t dte_requestor_id;
>      bool valid;
> +    bool dte_allow_exclusion;
> +    bool unity_map_enable;
> +    bool write_permission;
> +    bool read_permission;

Could you shrink this even more by using a bit-field instead of this sequence 
of bools?

> +
> +    /* ivhd device data settings */
> +    uint8_t device_flags;
> +
>      unsigned long addr_range_start;
>      unsigned long addr_range_length;
>      struct amd_iommu *iommu;
> @@ -120,9 +124,6 @@ struct ivrs_mappings {
>      void *intremap_table;
>      unsigned long *intremap_inuse;
>      spinlock_t intremap_lock;
> -
> -    /* ivhd device data settings */
> -    u8 device_flags;
>  };
> 
>  extern unsigned int ivrs_bdf_entries;
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel