[PATCH 0/6] VT-d: assorted cleanup

[PATCH 0/6] VT-d: assorted cleanup

[Jan Beulich]

This is effectively the other "half" of what was submitted as 4.15
candidates in "[PATCH 0/4][4.15?] VT-d: mostly S3 related corrections".

1: improve save/restore of registers across S3
2: don't open-code dmar_readl()
3: bring print_qi_regs() in line with print_iommu_regs()
4: qinval indexes are only up to 19 bits wide
5: avoid pointless use of 64-bit constants
6: drop unused #define-s

Jan

[PATCH 1/6] VT-d: improve save/restore of registers across S3

[Jan Beulich]

The static allocation of the save space is not only very inefficient
(most of the array slots won't ever get used), but is also the sole
reason for a build-time upper bound on the number of IOMMUs. Introduce
a structure containing just the one needed field we can't (easily)
restore from other in-memory state, and allocate the respective
array dynamically.

Take the opportunity and make the FEUADDR write dependent upon
x2apic_enabled, like is already the case in dma_msi_set_affinity().

Also alter properties of nr_iommus: static, unsigned, and __initdata.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -57,7 +57,7 @@ bool __read_mostly iommu_qinval = true;
 bool __read_mostly iommu_snoop = true;
 #endif
 
-int nr_iommus;
+static unsigned int __initdata nr_iommus;
 
 static struct iommu_ops vtd_ops;
 static struct tasklet vtd_fault_tasklet;
@@ -1165,13 +1165,6 @@ int __init iommu_alloc(struct acpi_drhd_
     unsigned long sagaw, nr_dom;
     int agaw;
 
-    if ( nr_iommus >= MAX_IOMMUS )
-    {
-        dprintk(XENLOG_ERR VTDPREFIX,
-                "IOMMU: nr_iommus %d > MAX_IOMMUS\n", nr_iommus + 1);
-        return -ENOMEM;
-    }
-
     iommu = xzalloc(struct vtd_iommu);
     if ( iommu == NULL )
         return -ENOMEM;
@@ -2226,6 +2219,10 @@ static void __hwdom_init setup_hwdom_rmr
     pcidevs_unlock();
 }
 
+static struct iommu_state {
+    uint32_t fectl;
+} *__read_mostly iommu_state;
+
 static int __init vtd_setup(void)
 {
     struct acpi_drhd_unit *drhd;
@@ -2251,6 +2248,13 @@ static int __init vtd_setup(void)
         goto error;
     }
 
+    iommu_state = xmalloc_array(struct iommu_state, nr_iommus);
+    if ( !iommu_state )
+    {
+        ret = -ENOMEM;
+        goto error;
+    }
+
     /* We enable the following features only if they are supported by all VT-d
      * engines: Snoop Control, DMA passthrough, Queued Invalidation, Interrupt
      * Remapping, and Posted Interrupt
@@ -2508,8 +2512,6 @@ static int intel_iommu_group_id(u16 seg,
         return PCI_BDF2(bus, devfn);
 }
 
-static u32 iommu_state[MAX_IOMMUS][MAX_IOMMU_REGS];
-
 static int __must_check vtd_suspend(void)
 {
     struct acpi_drhd_unit *drhd;
@@ -2534,14 +2536,7 @@ static int __must_check vtd_suspend(void
         iommu = drhd->iommu;
         i = iommu->index;
 
-        iommu_state[i][DMAR_FECTL_REG] =
-            (u32) dmar_readl(iommu->reg, DMAR_FECTL_REG);
-        iommu_state[i][DMAR_FEDATA_REG] =
-            (u32) dmar_readl(iommu->reg, DMAR_FEDATA_REG);
-        iommu_state[i][DMAR_FEADDR_REG] =
-            (u32) dmar_readl(iommu->reg, DMAR_FEADDR_REG);
-        iommu_state[i][DMAR_FEUADDR_REG] =
-            (u32) dmar_readl(iommu->reg, DMAR_FEUADDR_REG);
+        iommu_state[i].fectl = dmar_readl(iommu->reg, DMAR_FECTL_REG);
 
         /* don't disable VT-d engine when force_iommu is set. */
         if ( force_iommu )
@@ -2594,15 +2589,13 @@ static void vtd_resume(void)
     for_each_drhd_unit ( drhd )
     {
         iommu = drhd->iommu;
-        i = iommu->index;
 
         spin_lock_irqsave(&iommu->register_lock, flags);
-        dmar_writel(iommu->reg, DMAR_FEDATA_REG,
-                    iommu_state[i][DMAR_FEDATA_REG]);
-        dmar_writel(iommu->reg, DMAR_FEADDR_REG,
-                    iommu_state[i][DMAR_FEADDR_REG]);
-        dmar_writel(iommu->reg, DMAR_FEUADDR_REG,
-                    iommu_state[i][DMAR_FEUADDR_REG]);
+        dmar_writel(iommu->reg, DMAR_FEDATA_REG, iommu->msi.msg.data);
+        dmar_writel(iommu->reg, DMAR_FEADDR_REG, iommu->msi.msg.address_lo);
+        if ( x2apic_enabled )
+            dmar_writel(iommu->reg, DMAR_FEUADDR_REG,
+                        iommu->msi.msg.address_hi);
         spin_unlock_irqrestore(&iommu->register_lock, flags);
     }
 
@@ -2615,8 +2608,7 @@ static void vtd_resume(void)
         i = iommu->index;
 
         spin_lock_irqsave(&iommu->register_lock, flags);
-        dmar_writel(iommu->reg, DMAR_FECTL_REG,
-                    (u32) iommu_state[i][DMAR_FECTL_REG]);
+        dmar_writel(iommu->reg, DMAR_FECTL_REG, iommu_state[i].fectl);
         spin_unlock_irqrestore(&iommu->register_lock, flags);
 
         iommu_enable_translation(drhd);
--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -499,8 +499,6 @@ struct qinval_entry {
 #define VTD_PAGE_TABLE_LEVEL_3  3
 #define VTD_PAGE_TABLE_LEVEL_4  4
 
-#define MAX_IOMMU_REGS 0xc0
-
 extern struct list_head acpi_drhd_units;
 extern struct list_head acpi_rmrr_units;
 extern struct list_head acpi_ioapic_units;
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -22,7 +22,6 @@
 #include <asm/hvm/vmx/vmcs.h>
 
 #define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
-#define MAX_IOMMUS 32
 
 struct g2m_ioport {
     struct list_head list;

[PATCH 2/6] VT-d: don't open-code dmar_readl()

[Jan Beulich]

While at it also drop the unnecessary use of a local variable there.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -990,8 +990,7 @@ static void __do_iommu_page_fault(struct
     }
 clear_overflow:
     /* clear primary fault overflow */
-    fault_status = readl(iommu->reg + DMAR_FSTS_REG);
-    if ( fault_status & DMA_FSTS_PFO )
+    if ( dmar_readl(iommu->reg, DMAR_FSTS_REG) & DMA_FSTS_PFO )
     {
         spin_lock_irqsave(&iommu->register_lock, flags);
         dmar_writel(iommu->reg, DMAR_FSTS_REG, DMA_FSTS_PFO);

[PATCH 3/6] VT-d: bring print_qi_regs() in line with print_iommu_regs()

[Jan Beulich]

Shorten the names printed. There's also no need to go through a local
variable.

While at it also constify the function's parameter.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -33,18 +33,11 @@
 
 static int __must_check invalidate_sync(struct vtd_iommu *iommu);
 
-static void print_qi_regs(struct vtd_iommu *iommu)
+static void print_qi_regs(const struct vtd_iommu *iommu)
 {
-    u64 val;
-
-    val = dmar_readq(iommu->reg, DMAR_IQA_REG);
-    printk("DMAR_IQA_REG = %"PRIx64"\n", val);
-
-    val = dmar_readq(iommu->reg, DMAR_IQH_REG);
-    printk("DMAR_IQH_REG = %"PRIx64"\n", val);
-
-    val = dmar_readq(iommu->reg, DMAR_IQT_REG);
-    printk("DMAR_IQT_REG = %"PRIx64"\n", val);
+    printk(" IQA = %"PRIx64"\n", dmar_readq(iommu->reg, DMAR_IQA_REG));
+    printk(" IQH = %"PRIx64"\n", dmar_readq(iommu->reg, DMAR_IQH_REG));
+    printk(" IQT = %"PRIx64"\n", dmar_readq(iommu->reg, DMAR_IQT_REG));
 }
 
 static unsigned int qinval_next_index(struct vtd_iommu *iommu)

[PATCH 4/6] VT-d: qinval indexes are only up to 19 bits wide

[Jan Beulich]

There's no need for 64-bit accesses to these registers (outside of
initial setup and dumping).

Also remove some stray blanks.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -42,14 +42,13 @@ static void print_qi_regs(const struct v
 
 static unsigned int qinval_next_index(struct vtd_iommu *iommu)
 {
-    u64 tail;
+    unsigned int tail = dmar_readl(iommu->reg, DMAR_IQT_REG);
 
-    tail = dmar_readq(iommu->reg, DMAR_IQT_REG);
     tail >>= QINVAL_INDEX_SHIFT;
 
     /* (tail+1 == head) indicates a full queue, wait for HW */
-    while ( ( tail + 1 ) % QINVAL_ENTRY_NR ==
-            ( dmar_readq(iommu->reg, DMAR_IQH_REG) >> QINVAL_INDEX_SHIFT ) )
+    while ( (tail + 1) % QINVAL_ENTRY_NR ==
+            (dmar_readl(iommu->reg, DMAR_IQH_REG) >> QINVAL_INDEX_SHIFT) )
         cpu_relax();
 
     return tail;
@@ -57,12 +56,12 @@ static unsigned int qinval_next_index(st
 
 static void qinval_update_qtail(struct vtd_iommu *iommu, unsigned int index)
 {
-    u64 val;
+    unsigned int val;
 
     /* Need hold register lock when update tail */
     ASSERT( spin_is_locked(&iommu->register_lock) );
     val = (index + 1) % QINVAL_ENTRY_NR;
-    dmar_writeq(iommu->reg, DMAR_IQT_REG, (val << QINVAL_INDEX_SHIFT));
+    dmar_writel(iommu->reg, DMAR_IQT_REG, val << QINVAL_INDEX_SHIFT);
 }
 
 static int __must_check queue_invalidate_context_sync(struct vtd_iommu *iommu,

[PATCH 5/6] VT-d: avoid pointless use of 64-bit constants

[Jan Beulich]

When the respective registers are just 32 bits wide there's no point in
making corresponding constants 64-bit ones.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -121,30 +121,30 @@
 #define DMA_TLB_IVA_HINT(x) ((((u64)x) & 1) << 6)
 
 /* GCMD_REG */
-#define DMA_GCMD_TE     (((u64)1) << 31)
-#define DMA_GCMD_SRTP   (((u64)1) << 30)
-#define DMA_GCMD_SFL    (((u64)1) << 29)
-#define DMA_GCMD_EAFL   (((u64)1) << 28)
-#define DMA_GCMD_WBF    (((u64)1) << 27)
-#define DMA_GCMD_QIE    (((u64)1) << 26)
-#define DMA_GCMD_IRE    (((u64)1) << 25)
-#define DMA_GCMD_SIRTP  (((u64)1) << 24)
-#define DMA_GCMD_CFI    (((u64)1) << 23)
+#define DMA_GCMD_TE     (1u << 31)
+#define DMA_GCMD_SRTP   (1u << 30)
+#define DMA_GCMD_SFL    (1u << 29)
+#define DMA_GCMD_EAFL   (1u << 28)
+#define DMA_GCMD_WBF    (1u << 27)
+#define DMA_GCMD_QIE    (1u << 26)
+#define DMA_GCMD_IRE    (1u << 25)
+#define DMA_GCMD_SIRTP  (1u << 24)
+#define DMA_GCMD_CFI    (1u << 23)
 
 /* GSTS_REG */
-#define DMA_GSTS_TES    (((u64)1) << 31)
-#define DMA_GSTS_RTPS   (((u64)1) << 30)
-#define DMA_GSTS_FLS    (((u64)1) << 29)
-#define DMA_GSTS_AFLS   (((u64)1) << 28)
-#define DMA_GSTS_WBFS   (((u64)1) << 27)
-#define DMA_GSTS_QIES   (((u64)1) <<26)
-#define DMA_GSTS_IRES   (((u64)1) <<25)
-#define DMA_GSTS_SIRTPS (((u64)1) << 24)
-#define DMA_GSTS_CFIS   (((u64)1) <<23)
+#define DMA_GSTS_TES    (1u << 31)
+#define DMA_GSTS_RTPS   (1u << 30)
+#define DMA_GSTS_FLS    (1u << 29)
+#define DMA_GSTS_AFLS   (1u << 28)
+#define DMA_GSTS_WBFS   (1u << 27)
+#define DMA_GSTS_QIES   (1u << 26)
+#define DMA_GSTS_IRES   (1u << 25)
+#define DMA_GSTS_SIRTPS (1u << 24)
+#define DMA_GSTS_CFIS   (1u << 23)
 
 /* PMEN_REG */
-#define DMA_PMEN_EPM    (((u32)1) << 31)
-#define DMA_PMEN_PRS    (((u32)1) << 0)
+#define DMA_PMEN_EPM    (1u << 31)
+#define DMA_PMEN_PRS    (1u <<  0)
 
 /* CCMD_REG */
 #define DMA_CCMD_INVL_GRANU_OFFSET  61
@@ -164,21 +164,23 @@
 #define DMA_CCMD_CAIG_MASK(x) (((u64)x) & ((u64) 0x3 << 59))
 
 /* FECTL_REG */
-#define DMA_FECTL_IM (((u64)1) << 31)
+#define DMA_FECTL_IM (1u << 31)
 
 /* FSTS_REG */
-#define DMA_FSTS_PFO ((u64)1 << 0)
-#define DMA_FSTS_PPF ((u64)1 << 1)
-#define DMA_FSTS_AFO ((u64)1 << 2)
-#define DMA_FSTS_APF ((u64)1 << 3)
-#define DMA_FSTS_IQE ((u64)1 << 4)
-#define DMA_FSTS_ICE ((u64)1 << 5)
-#define DMA_FSTS_ITE ((u64)1 << 6)
-#define DMA_FSTS_FAULTS    DMA_FSTS_PFO | DMA_FSTS_PPF | DMA_FSTS_AFO | DMA_FSTS_APF | DMA_FSTS_IQE | DMA_FSTS_ICE | DMA_FSTS_ITE
+#define DMA_FSTS_PFO (1u << 0)
+#define DMA_FSTS_PPF (1u << 1)
+#define DMA_FSTS_AFO (1u << 2)
+#define DMA_FSTS_APF (1u << 3)
+#define DMA_FSTS_IQE (1u << 4)
+#define DMA_FSTS_ICE (1u << 5)
+#define DMA_FSTS_ITE (1u << 6)
+#define DMA_FSTS_FAULTS (DMA_FSTS_PFO | DMA_FSTS_PPF | DMA_FSTS_AFO | \
+                         DMA_FSTS_APF | DMA_FSTS_IQE | DMA_FSTS_ICE | \
+                         DMA_FSTS_ITE)
 #define dma_fsts_fault_record_index(s) (((s) >> 8) & 0xff)
 
 /* FRCD_REG, 32 bits access */
-#define DMA_FRCD_F (((u64)1) << 31)
+#define DMA_FRCD_F (1u << 31)
 #define dma_frcd_type(d) ((d >> 30) & 1)
 #define dma_frcd_fault_reason(c) (c & 0xff)
 #define dma_frcd_source_id(c) (c & 0xffff)

[PATCH 6/6] VT-d: drop unused #define-s

[Jan Beulich]

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -471,26 +471,12 @@ struct qinval_entry {
 /* Queue invalidation head/tail shift */
 #define QINVAL_INDEX_SHIFT 4
 
-#define qinval_present(v) ((v).lo & 1)
-#define qinval_fault_disable(v) (((v).lo >> 1) & 1)
-
-#define qinval_set_present(v) do {(v).lo |= 1;} while(0)
-#define qinval_clear_present(v) do {(v).lo &= ~1;} while(0)
-
-#define RESERVED_VAL        0
-
 #define TYPE_INVAL_CONTEXT      0x1
 #define TYPE_INVAL_IOTLB        0x2
 #define TYPE_INVAL_DEVICE_IOTLB 0x3
 #define TYPE_INVAL_IEC          0x4
 #define TYPE_INVAL_WAIT         0x5
 
-#define NOTIFY_TYPE_POLL        1
-#define NOTIFY_TYPE_INTR        1
-#define INTERRUTP_FLAG          1
-#define STATUS_WRITE            1
-#define FENCE_FLAG              1
-
 #define IEC_GLOBAL_INVL         0
 #define IEC_INDEX_INVL          1
 #define IRTA_EIME               (((u64)1) << 11)

RE: [PATCH 0/6] VT-d: assorted cleanup

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, April 1, 2021 4:43 PM
> 
> This is effectively the other "half" of what was submitted as 4.15
> candidates in "[PATCH 0/4][4.15?] VT-d: mostly S3 related corrections".
> 
> 1: improve save/restore of registers across S3
> 2: don't open-code dmar_readl()
> 3: bring print_qi_regs() in line with print_iommu_regs()
> 4: qinval indexes are only up to 19 bits wide
> 5: avoid pointless use of 64-bit constants
> 6: drop unused #define-s
> 
> Jan

For the whole series:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>