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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel