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)

It it possible that this has expired in the meantime? I can't seem to
be able to access it. But then, with what you write below, I probably
also have enough information.

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

If only I knew what you mean with "simple FEAT() and MASK() macros".
I can't think of variants not requiring to also introduce literal
numbers to use as constants. I'll (not just) therefore try to modify
the original approach such that hopefully there won't be any overflow
detected anymore. I can't see any such issue with the clang I use
anyway.

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