Re: [Xen-devel] [PATCH v2 02/10] AMD/IOMMU: use bit field for extended feature register

[Jan Beulich <JBeulich@xxxxxxxx>]

On 02.07.2019 14:09, Andrew Cooper wrote:
> On 27/06/2019 16:19, Jan Beulich wrote:
>>       printk("AMD-Vi: IOMMU Extended Features:\n");
>>   
>> -    while ( feature_str[i] )
>> +#define MASK(fld) ((union amd_iommu_ext_features){ .flds.fld = ~0 }).raw
>> +#define FEAT(fld, str) do { \
>> +    if ( MASK(fld) & (MASK(fld) - 1) ) \
>> +        printk( "- " str ": %#x\n", iommu->features.flds.fld); \
>> +    else if ( iommu->features.raw & MASK(fld) ) \
>> +        printk( "- " str "\n"); \
>> +} while ( false )
> 
> Sadly, Clang dislikes this construct.
> 
> https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/243795095
> (Click on the "Complete Raw" button)
> 
> iommu_detect.c:90:5: error: implicit truncation from 'int' to bitfield 
> changes value from -1 to 1 [-Werror,-Wbitfield-constant-conversion]
>      FEAT(pref_sup,           "Prefetch Pages Command");
>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> iommu_detect.c:84:10: note: expanded from macro 'FEAT'
>      if ( MASK(fld) & (MASK(fld) - 1) ) \
>           ^~~~~~~~~
> iommu_detect.c:82:64: note: expanded from macro 'MASK'
> #define MASK(fld) ((union amd_iommu_ext_features){ .flds.fld = ~0 }).raw
>                                                                 ^~
> 
> 
> which is a shame.  Furthermore, switching to ~(0u) won't work either,
> because that will then get a truncation warning.
> 
> Clever as this trick is, this is write-once code and isn't going to
> change moving forward.  I'd do away with the compile-time cleverness and
> have simple FEAT() and MASK() macros, and use the correct one below.

I don't immediately see what you would mean by "simple FEAT() and MASK()
macros", but perhaps I'll figure when I actually make this change. What
I'm concerned about when changing away from the chosen model is that
there'll likely be a need to explicitly know whether a field is just a
boolean or holds an actual (wider) value. I.e. that's what is not "write
once" about this code, since future additions equally become more
fragile.

I was actually hoping to use this "mask from bitfield" approach
elsewhere, so this is yet another case where I wonder whether us wanting
to be able to build with clang is actually becoming an increasing
hindrance.

I'll see if I can come up with something else, still matching the
original idea. Clearly clang can't be consistent with its value
truncation warnings, or else Xen wouldn't build with it at all.

>> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
>> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
>> @@ -346,26 +346,57 @@ struct amd_iommu_dte {
>> +union amd_iommu_ext_features {
>> +    uint64_t raw;
>> +    struct {
>> +        unsigned int pref_sup:1;
>> +        unsigned int ppr_sup:1;
>> +        unsigned int xt_sup:1;
>> +        unsigned int nx_sup:1;
>> +        unsigned int gt_sup:1;
>> +        unsigned int gappi_sup:1;
>> +        unsigned int ia_sup:1;
>> +        unsigned int ga_sup:1;
>> +        unsigned int he_sup:1;
>> +        unsigned int pc_sup:1;
>> +        unsigned int hats:2;
>> +        unsigned int gats:2;
>> +        unsigned int glx_sup:2;
>> +        unsigned int smif_sup:2;
>> +        unsigned int smif_rc:3;
>> +        unsigned int gam_sup:3;
>> +        unsigned int dual_ppr_log_sup:2;
>> +        unsigned int :2;
>> +        unsigned int dual_event_log_sup:2;
>> +        unsigned int :1;
>> +        unsigned int sats_sup:1;
>> +        unsigned int pas_max:5;
>> +        unsigned int us_sup:1;
>> +        unsigned int dev_tbl_seg_sup:2;
>> +        unsigned int ppr_early_of_sup:1;
>> +        unsigned int ppr_auto_rsp_sup:1;
>> +        unsigned int marc_sup:2;
>> +        unsigned int blk_stop_mrk_sup:1;
>> +        unsigned int perf_opt_sup:1;
>> +        unsigned int msi_cap_mmio_sup:1;
>> +        unsigned int :1;
>> +        unsigned int gio_sup:1;
>> +        unsigned int ha_sup:1;
>> +        unsigned int eph_sup:1;
>> +        unsigned int attr_fw_sup:1;
>> +        unsigned int hd_sup:1;
>> +        unsigned int :1;
>> +        unsigned int inv_iotlb_type_sup:1;
>> +        unsigned int viommu_sup:1;
>> +        unsigned int vm_guard_io_sup:1;
>> +        unsigned int vm_table_size:4;
>> +        unsigned int ga_update_dis_sup:1;
>> +        unsigned int :2;
>> +    } flds;
> 
> Why the .flds name?  What is wrong with this becoming anonymous?

The initializer in guest_iommu_reg_init() (with old gcc).

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel