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.
> --- 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?
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
