[PATCH 0/4] VT-d/qinval: miscellaneous adjustments

[PATCH 0/4] VT-d/qinval: miscellaneous adjustments

[Jan Beulich]

1: make local variable used for communication with IOMMU "volatile"
2: drop redundant calls to invalidate_sync()
3: clean up error handling
4: queue index is always unsigned

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

[PATCH 1/4] VT-d/qinval: make local variable used for communication with IOMMU "volatile"

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

Without that there is - afaict - nothing preventing the compiler from
putting the variable into a register for the duration of the wait loop.

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

--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -196,7 +196,7 @@ static int queue_invalidate_wait(struct 
     u8 iflag, u8 sw, u8 fn)
 {
     s_time_t start_time;
-    u32 poll_slot = QINVAL_STAT_INIT;
+    volatile u32 poll_slot = QINVAL_STAT_INIT;
     int index = -1;
     int ret = -1;
     unsigned long flags;




[-- Attachment #2: VT-d-qi-poll_slot-volatile.patch --]
[-- Type: text/plain, Size: 646 bytes --]

VT-d/qinval: make local variable used for communication with IOMMU "volatile"

Without that there is - afaict - nothing preventing the compiler from
putting the variable into a register for the duration of the wait loop.

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

--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -196,7 +196,7 @@ static int queue_invalidate_wait(struct 
     u8 iflag, u8 sw, u8 fn)
 {
     s_time_t start_time;
-    u32 poll_slot = QINVAL_STAT_INIT;
+    volatile u32 poll_slot = QINVAL_STAT_INIT;
     int index = -1;
     int ret = -1;
     unsigned long flags;

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 2/4] VT-d: drop redundant calls to invalidate_sync()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1946 bytes --]

The call tree iommu_flush_iec_index() -> __iommu_flush_iec() already
invokes invalidate_sync(). Removing the superfluous instances at once
allows the function to become static.

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

--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -48,7 +48,6 @@ int queue_invalidate_iotlb(struct iommu 
     u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr);
 int queue_invalidate_iec(struct iommu *iommu,
     u8 granu, u8 im, u16 iidx);
-int invalidate_sync(struct iommu *iommu);
 int iommu_flush_iec_global(struct iommu *iommu);
 int iommu_flush_iec_index(struct iommu *iommu, u8 im, u16 iidx);
 void clear_fault_bits(struct iommu *iommu);
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -358,7 +358,6 @@ static int ioapic_rte_to_remap_entry(str
     memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
     iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
     iommu_flush_iec_index(iommu, 0, index);
-    invalidate_sync(iommu);
 
     unmap_vtd_domain_page(iremap_entries);
     spin_unlock_irqrestore(&ir_ctrl->iremap_lock, flags);
@@ -643,7 +642,6 @@ static int msi_msg_to_remap_entry(
     memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
     iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
     iommu_flush_iec_index(iommu, 0, index);
-    invalidate_sync(iommu);
 
     unmap_vtd_domain_page(iremap_entries);
     spin_unlock_irqrestore(&ir_ctrl->iremap_lock, flags);
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -228,7 +228,7 @@ static int queue_invalidate_wait(struct 
     return ret;
 }
 
-int invalidate_sync(struct iommu *iommu)
+static int invalidate_sync(struct iommu *iommu)
 {
     int ret = -1;
     struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);




[-- Attachment #2: VT-d-duplicate-invalidate.patch --]
[-- Type: text/plain, Size: 1991 bytes --]

VT-d: drop redundant calls to invalidate_sync()

The call tree iommu_flush_iec_index() -> __iommu_flush_iec() already
invokes invalidate_sync(). Removing the superfluous instances at once
allows the function to become static.

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

--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -48,7 +48,6 @@ int queue_invalidate_iotlb(struct iommu 
     u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr);
 int queue_invalidate_iec(struct iommu *iommu,
     u8 granu, u8 im, u16 iidx);
-int invalidate_sync(struct iommu *iommu);
 int iommu_flush_iec_global(struct iommu *iommu);
 int iommu_flush_iec_index(struct iommu *iommu, u8 im, u16 iidx);
 void clear_fault_bits(struct iommu *iommu);
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -358,7 +358,6 @@ static int ioapic_rte_to_remap_entry(str
     memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
     iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
     iommu_flush_iec_index(iommu, 0, index);
-    invalidate_sync(iommu);
 
     unmap_vtd_domain_page(iremap_entries);
     spin_unlock_irqrestore(&ir_ctrl->iremap_lock, flags);
@@ -643,7 +642,6 @@ static int msi_msg_to_remap_entry(
     memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
     iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
     iommu_flush_iec_index(iommu, 0, index);
-    invalidate_sync(iommu);
 
     unmap_vtd_domain_page(iremap_entries);
     spin_unlock_irqrestore(&ir_ctrl->iremap_lock, flags);
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -228,7 +228,7 @@ static int queue_invalidate_wait(struct 
     return ret;
 }
 
-int invalidate_sync(struct iommu *iommu)
+static int invalidate_sync(struct iommu *iommu)
 {
     int ret = -1;
     struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 3/4] VT-d/qinval: clean up error handling

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 6475 bytes --]

- neither qinval_update_qtail() nor qinval_next_index() can fail: make
  the former return "void", and drop caller error checks for the latter
  (all of which would otherwise return with a spin lock still held)
- or-ing together error codes is a bad idea

At once drop bogus initializers.

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

--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -58,7 +58,7 @@ static int qinval_next_index(struct iomm
     return tail;
 }
 
-static int qinval_update_qtail(struct iommu *iommu, int index)
+static void qinval_update_qtail(struct iommu *iommu, int index)
 {
     u64 val;
 
@@ -66,7 +66,6 @@ static int qinval_update_qtail(struct io
     ASSERT( spin_is_locked(&iommu->register_lock) );
     val = (index + 1) % QINVAL_ENTRY_NR;
     dmar_writeq(iommu->reg, DMAR_IQT_REG, (val << QINVAL_INDEX_SHIFT));
-    return 0;
 }
 
 static int gen_cc_inv_dsc(struct iommu *iommu, int index,
@@ -100,17 +99,15 @@ static int gen_cc_inv_dsc(struct iommu *
 int queue_invalidate_context(struct iommu *iommu,
     u16 did, u16 source_id, u8 function_mask, u8 granu)
 {
-    int ret = -1;
+    int ret;
     unsigned long flags;
     int index = -1;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);
-    if ( index == -1 )
-        return -EBUSY;
     ret = gen_cc_inv_dsc(iommu, index, did, source_id,
                          function_mask, granu);
-    ret |= qinval_update_qtail(iommu, index);
+    qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
     return ret;
 }
@@ -150,18 +147,16 @@ static int gen_iotlb_inv_dsc(struct iomm
 int queue_invalidate_iotlb(struct iommu *iommu,
     u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr)
 {
-    int ret = -1;
+    int ret;
     unsigned long flags;
     int index = -1;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
 
     index = qinval_next_index(iommu);
-    if ( index == -1 )
-        return -EBUSY;
     ret = gen_iotlb_inv_dsc(iommu, index, granu, dr, dw, did,
                             am, ih, addr);
-    ret |= qinval_update_qtail(iommu, index);
+    qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
     return ret;
 }
@@ -198,16 +193,13 @@ static int queue_invalidate_wait(struct 
     s_time_t start_time;
     volatile u32 poll_slot = QINVAL_STAT_INIT;
     int index = -1;
-    int ret = -1;
+    int ret;
     unsigned long flags;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);
-    if ( index == -1 )
-        return -EBUSY;
-
     ret = gen_wait_dsc(iommu, index, iflag, sw, fn, QINVAL_STAT_DONE, &poll_slot);
-    ret |= qinval_update_qtail(iommu, index);
+    qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 
     /* Now we don't support interrupt method */
@@ -225,19 +217,17 @@ static int queue_invalidate_wait(struct 
             cpu_relax();
         }
     }
+    else if ( !ret )
+        ret = -EOPNOTSUPP;
     return ret;
 }
 
 static int invalidate_sync(struct iommu *iommu)
 {
-    int ret = -1;
     struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
 
-    if ( qi_ctrl->qinval_maddr != 0 )
-    {
-        ret = queue_invalidate_wait(iommu, 0, 1, 1);
-        return ret;
-    }
+    if ( qi_ctrl->qinval_maddr )
+        return queue_invalidate_wait(iommu, 0, 1, 1);
     return 0;
 }
 
@@ -274,17 +264,15 @@ static int gen_dev_iotlb_inv_dsc(struct 
 int qinval_device_iotlb(struct iommu *iommu,
     u32 max_invs_pend, u16 sid, u16 size, u64 addr)
 {
-    int ret = -1;
+    int ret;
     unsigned long flags;
     int index = -1;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);
-    if ( index == -1 )
-        return -EBUSY;
     ret = gen_dev_iotlb_inv_dsc(iommu, index, max_invs_pend,
                                 sid, size, addr);
-    ret |= qinval_update_qtail(iommu, index);
+    qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
     return ret;
 }
@@ -324,26 +312,23 @@ int queue_invalidate_iec(struct iommu *i
 
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);
-    if ( index == -1 )
-        return -EBUSY;
     ret = gen_iec_inv_dsc(iommu, index, granu, im, iidx);
-    ret |= qinval_update_qtail(iommu, index);
+    qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
     return ret;
 }
 
 static int __iommu_flush_iec(struct iommu *iommu, u8 granu, u8 im, u16 iidx)
 {
-    int ret;
-    ret = queue_invalidate_iec(iommu, granu, im, iidx);
-    ret |= invalidate_sync(iommu);
+    int ret = queue_invalidate_iec(iommu, granu, im, iidx);
+    int rc = invalidate_sync(iommu);
 
     /*
      * reading vt-d architecture register will ensure
      * draining happens in implementation independent way.
      */
     (void)dmar_readq(iommu->reg, DMAR_CAP_REG);
-    return ret;
+    return ret ?: rc;
 }
 
 int iommu_flush_iec_global(struct iommu *iommu)
@@ -380,9 +365,13 @@ static int flush_context_qi(
 
     if ( qi_ctrl->qinval_maddr != 0 )
     {
+        int rc;
+
         ret = queue_invalidate_context(iommu, did, sid, fm,
                                        type >> DMA_CCMD_INVL_GRANU_OFFSET);
-        ret |= invalidate_sync(iommu);
+        rc = invalidate_sync(iommu);
+        if ( !ret )
+            ret = rc;
     }
     return ret;
 }
@@ -413,6 +402,8 @@ static int flush_iotlb_qi(
 
     if ( qi_ctrl->qinval_maddr != 0 )
     {
+        int rc;
+
         /* use queued invalidation */
         if (cap_write_drain(iommu->cap))
             dw = 1;
@@ -423,8 +414,10 @@ static int flush_iotlb_qi(
                   (type >> DMA_TLB_FLUSH_GRANU_OFFSET), dr,
                   dw, did, (u8)size_order, 0, addr);
         if ( flush_dev_iotlb )
-            ret |= dev_invalidate_iotlb(iommu, did, addr, size_order, type);
-        ret |= invalidate_sync(iommu);
+            ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
+        rc = invalidate_sync(iommu);
+        if ( !ret )
+            ret = rc;
     }
     return ret;
 }



[-- Attachment #2: VT-d-qi-error-handling.patch --]
[-- Type: text/plain, Size: 6511 bytes --]

VT-d/qinval: clean up error handling

- neither qinval_update_qtail() nor qinval_next_index() can fail: make
  the former return "void", and drop caller error checks for the latter
  (all of which would otherwise return with a spin lock still held)
- or-ing together error codes is a bad idea

At once drop bogus initializers.

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

--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -58,7 +58,7 @@ static int qinval_next_index(struct iomm
     return tail;
 }
 
-static int qinval_update_qtail(struct iommu *iommu, int index)
+static void qinval_update_qtail(struct iommu *iommu, int index)
 {
     u64 val;
 
@@ -66,7 +66,6 @@ static int qinval_update_qtail(struct io
     ASSERT( spin_is_locked(&iommu->register_lock) );
     val = (index + 1) % QINVAL_ENTRY_NR;
     dmar_writeq(iommu->reg, DMAR_IQT_REG, (val << QINVAL_INDEX_SHIFT));
-    return 0;
 }
 
 static int gen_cc_inv_dsc(struct iommu *iommu, int index,
@@ -100,17 +99,15 @@ static int gen_cc_inv_dsc(struct iommu *
 int queue_invalidate_context(struct iommu *iommu,
     u16 did, u16 source_id, u8 function_mask, u8 granu)
 {
-    int ret = -1;
+    int ret;
     unsigned long flags;
     int index = -1;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);
-    if ( index == -1 )
-        return -EBUSY;
     ret = gen_cc_inv_dsc(iommu, index, did, source_id,
                          function_mask, granu);
-    ret |= qinval_update_qtail(iommu, index);
+    qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
     return ret;
 }
@@ -150,18 +147,16 @@ static int gen_iotlb_inv_dsc(struct iomm
 int queue_invalidate_iotlb(struct iommu *iommu,
     u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr)
 {
-    int ret = -1;
+    int ret;
     unsigned long flags;
     int index = -1;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
 
     index = qinval_next_index(iommu);
-    if ( index == -1 )
-        return -EBUSY;
     ret = gen_iotlb_inv_dsc(iommu, index, granu, dr, dw, did,
                             am, ih, addr);
-    ret |= qinval_update_qtail(iommu, index);
+    qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
     return ret;
 }
@@ -198,16 +193,13 @@ static int queue_invalidate_wait(struct 
     s_time_t start_time;
     volatile u32 poll_slot = QINVAL_STAT_INIT;
     int index = -1;
-    int ret = -1;
+    int ret;
     unsigned long flags;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);
-    if ( index == -1 )
-        return -EBUSY;
-
     ret = gen_wait_dsc(iommu, index, iflag, sw, fn, QINVAL_STAT_DONE, &poll_slot);
-    ret |= qinval_update_qtail(iommu, index);
+    qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 
     /* Now we don't support interrupt method */
@@ -225,19 +217,17 @@ static int queue_invalidate_wait(struct 
             cpu_relax();
         }
     }
+    else if ( !ret )
+        ret = -EOPNOTSUPP;
     return ret;
 }
 
 static int invalidate_sync(struct iommu *iommu)
 {
-    int ret = -1;
     struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
 
-    if ( qi_ctrl->qinval_maddr != 0 )
-    {
-        ret = queue_invalidate_wait(iommu, 0, 1, 1);
-        return ret;
-    }
+    if ( qi_ctrl->qinval_maddr )
+        return queue_invalidate_wait(iommu, 0, 1, 1);
     return 0;
 }
 
@@ -274,17 +264,15 @@ static int gen_dev_iotlb_inv_dsc(struct 
 int qinval_device_iotlb(struct iommu *iommu,
     u32 max_invs_pend, u16 sid, u16 size, u64 addr)
 {
-    int ret = -1;
+    int ret;
     unsigned long flags;
     int index = -1;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);
-    if ( index == -1 )
-        return -EBUSY;
     ret = gen_dev_iotlb_inv_dsc(iommu, index, max_invs_pend,
                                 sid, size, addr);
-    ret |= qinval_update_qtail(iommu, index);
+    qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
     return ret;
 }
@@ -324,26 +312,23 @@ int queue_invalidate_iec(struct iommu *i
 
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);
-    if ( index == -1 )
-        return -EBUSY;
     ret = gen_iec_inv_dsc(iommu, index, granu, im, iidx);
-    ret |= qinval_update_qtail(iommu, index);
+    qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
     return ret;
 }
 
 static int __iommu_flush_iec(struct iommu *iommu, u8 granu, u8 im, u16 iidx)
 {
-    int ret;
-    ret = queue_invalidate_iec(iommu, granu, im, iidx);
-    ret |= invalidate_sync(iommu);
+    int ret = queue_invalidate_iec(iommu, granu, im, iidx);
+    int rc = invalidate_sync(iommu);
 
     /*
      * reading vt-d architecture register will ensure
      * draining happens in implementation independent way.
      */
     (void)dmar_readq(iommu->reg, DMAR_CAP_REG);
-    return ret;
+    return ret ?: rc;
 }
 
 int iommu_flush_iec_global(struct iommu *iommu)
@@ -380,9 +365,13 @@ static int flush_context_qi(
 
     if ( qi_ctrl->qinval_maddr != 0 )
     {
+        int rc;
+
         ret = queue_invalidate_context(iommu, did, sid, fm,
                                        type >> DMA_CCMD_INVL_GRANU_OFFSET);
-        ret |= invalidate_sync(iommu);
+        rc = invalidate_sync(iommu);
+        if ( !ret )
+            ret = rc;
     }
     return ret;
 }
@@ -413,6 +402,8 @@ static int flush_iotlb_qi(
 
     if ( qi_ctrl->qinval_maddr != 0 )
     {
+        int rc;
+
         /* use queued invalidation */
         if (cap_write_drain(iommu->cap))
             dw = 1;
@@ -423,8 +414,10 @@ static int flush_iotlb_qi(
                   (type >> DMA_TLB_FLUSH_GRANU_OFFSET), dr,
                   dw, did, (u8)size_order, 0, addr);
         if ( flush_dev_iotlb )
-            ret |= dev_invalidate_iotlb(iommu, did, addr, size_order, type);
-        ret |= invalidate_sync(iommu);
+            ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
+        rc = invalidate_sync(iommu);
+        if ( !ret )
+            ret = rc;
     }
     return ret;
 }

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 4/4] VT-d/qinval: queue index is always unsigned

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3470 bytes --]

At once drop bogus initializers.

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

--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -43,7 +43,7 @@ static void print_qi_regs(struct iommu *
     printk("DMAR_IQT_REG = %"PRIx64"\n", val);
 }
 
-static int qinval_next_index(struct iommu *iommu)
+static unsigned int qinval_next_index(struct iommu *iommu)
 {
     u64 tail;
 
@@ -58,7 +58,7 @@ static int qinval_next_index(struct iomm
     return tail;
 }
 
-static void qinval_update_qtail(struct iommu *iommu, int index)
+static void qinval_update_qtail(struct iommu *iommu, unsigned int index)
 {
     u64 val;
 
@@ -68,7 +68,7 @@ static void qinval_update_qtail(struct i
     dmar_writeq(iommu->reg, DMAR_IQT_REG, (val << QINVAL_INDEX_SHIFT));
 }
 
-static int gen_cc_inv_dsc(struct iommu *iommu, int index,
+static int gen_cc_inv_dsc(struct iommu *iommu, unsigned int index,
     u16 did, u16 source_id, u8 function_mask, u8 granu)
 {
     unsigned long flags;
@@ -101,7 +101,7 @@ int queue_invalidate_context(struct iomm
 {
     int ret;
     unsigned long flags;
-    int index = -1;
+    unsigned int index;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);
@@ -112,7 +112,7 @@ int queue_invalidate_context(struct iomm
     return ret;
 }
 
-static int gen_iotlb_inv_dsc(struct iommu *iommu, int index,
+static int gen_iotlb_inv_dsc(struct iommu *iommu, unsigned int index,
     u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr)
 {
     unsigned long flags;
@@ -149,7 +149,7 @@ int queue_invalidate_iotlb(struct iommu 
 {
     int ret;
     unsigned long flags;
-    int index = -1;
+    unsigned int index;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
 
@@ -161,7 +161,7 @@ int queue_invalidate_iotlb(struct iommu 
     return ret;
 }
 
-static int gen_wait_dsc(struct iommu *iommu, int index,
+static int gen_wait_dsc(struct iommu *iommu, unsigned int index,
     u8 iflag, u8 sw, u8 fn, u32 sdata, volatile u32 *saddr)
 {
     unsigned long flags;
@@ -192,7 +192,7 @@ static int queue_invalidate_wait(struct 
 {
     s_time_t start_time;
     volatile u32 poll_slot = QINVAL_STAT_INIT;
-    int index = -1;
+    unsigned int index;
     int ret;
     unsigned long flags;
 
@@ -231,7 +231,7 @@ static int invalidate_sync(struct iommu 
     return 0;
 }
 
-static int gen_dev_iotlb_inv_dsc(struct iommu *iommu, int index,
+static int gen_dev_iotlb_inv_dsc(struct iommu *iommu, unsigned int index,
     u32 max_invs_pend, u16 sid, u16 size, u64 addr)
 {
     unsigned long flags;
@@ -266,7 +266,7 @@ int qinval_device_iotlb(struct iommu *io
 {
     int ret;
     unsigned long flags;
-    int index = -1;
+    unsigned int index;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);
@@ -277,7 +277,7 @@ int qinval_device_iotlb(struct iommu *io
     return ret;
 }
 
-static int gen_iec_inv_dsc(struct iommu *iommu, int index,
+static int gen_iec_inv_dsc(struct iommu *iommu, unsigned int index,
     u8 granu, u8 im, u16 iidx)
 {
     unsigned long flags;
@@ -308,7 +308,7 @@ int queue_invalidate_iec(struct iommu *i
 {
     int ret;
     unsigned long flags;
-    int index = -1;
+    unsigned int index;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);




[-- Attachment #2: VT-d-qi-unsigned-index.patch --]
[-- Type: text/plain, Size: 3511 bytes --]

VT-d/qinval: queue index is always unsigned

At once drop bogus initializers.

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

--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -43,7 +43,7 @@ static void print_qi_regs(struct iommu *
     printk("DMAR_IQT_REG = %"PRIx64"\n", val);
 }
 
-static int qinval_next_index(struct iommu *iommu)
+static unsigned int qinval_next_index(struct iommu *iommu)
 {
     u64 tail;
 
@@ -58,7 +58,7 @@ static int qinval_next_index(struct iomm
     return tail;
 }
 
-static void qinval_update_qtail(struct iommu *iommu, int index)
+static void qinval_update_qtail(struct iommu *iommu, unsigned int index)
 {
     u64 val;
 
@@ -68,7 +68,7 @@ static void qinval_update_qtail(struct i
     dmar_writeq(iommu->reg, DMAR_IQT_REG, (val << QINVAL_INDEX_SHIFT));
 }
 
-static int gen_cc_inv_dsc(struct iommu *iommu, int index,
+static int gen_cc_inv_dsc(struct iommu *iommu, unsigned int index,
     u16 did, u16 source_id, u8 function_mask, u8 granu)
 {
     unsigned long flags;
@@ -101,7 +101,7 @@ int queue_invalidate_context(struct iomm
 {
     int ret;
     unsigned long flags;
-    int index = -1;
+    unsigned int index;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);
@@ -112,7 +112,7 @@ int queue_invalidate_context(struct iomm
     return ret;
 }
 
-static int gen_iotlb_inv_dsc(struct iommu *iommu, int index,
+static int gen_iotlb_inv_dsc(struct iommu *iommu, unsigned int index,
     u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr)
 {
     unsigned long flags;
@@ -149,7 +149,7 @@ int queue_invalidate_iotlb(struct iommu 
 {
     int ret;
     unsigned long flags;
-    int index = -1;
+    unsigned int index;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
 
@@ -161,7 +161,7 @@ int queue_invalidate_iotlb(struct iommu 
     return ret;
 }
 
-static int gen_wait_dsc(struct iommu *iommu, int index,
+static int gen_wait_dsc(struct iommu *iommu, unsigned int index,
     u8 iflag, u8 sw, u8 fn, u32 sdata, volatile u32 *saddr)
 {
     unsigned long flags;
@@ -192,7 +192,7 @@ static int queue_invalidate_wait(struct 
 {
     s_time_t start_time;
     volatile u32 poll_slot = QINVAL_STAT_INIT;
-    int index = -1;
+    unsigned int index;
     int ret;
     unsigned long flags;
 
@@ -231,7 +231,7 @@ static int invalidate_sync(struct iommu 
     return 0;
 }
 
-static int gen_dev_iotlb_inv_dsc(struct iommu *iommu, int index,
+static int gen_dev_iotlb_inv_dsc(struct iommu *iommu, unsigned int index,
     u32 max_invs_pend, u16 sid, u16 size, u64 addr)
 {
     unsigned long flags;
@@ -266,7 +266,7 @@ int qinval_device_iotlb(struct iommu *io
 {
     int ret;
     unsigned long flags;
-    int index = -1;
+    unsigned int index;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);
@@ -277,7 +277,7 @@ int qinval_device_iotlb(struct iommu *io
     return ret;
 }
 
-static int gen_iec_inv_dsc(struct iommu *iommu, int index,
+static int gen_iec_inv_dsc(struct iommu *iommu, unsigned int index,
     u8 granu, u8 im, u16 iidx)
 {
     unsigned long flags;
@@ -308,7 +308,7 @@ int queue_invalidate_iec(struct iommu *i
 {
     int ret;
     unsigned long flags;
-    int index = -1;
+    unsigned int index;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/4] VT-d/qinval: make local variable used for communication with IOMMU "volatile"

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 980 bytes --]

On 16/06/14 13:47, Jan Beulich wrote:
> Without that there is - afaict - nothing preventing the compiler from
> putting the variable into a register for the duration of the wait loop.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I seem to remember Tim saying that behaviour like this is covered by
-fno-strict-aliasing

Either way, it is certainly correct for it to be marked as volatile.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -196,7 +196,7 @@ static int queue_invalidate_wait(struct 
>      u8 iflag, u8 sw, u8 fn)
>  {
>      s_time_t start_time;
> -    u32 poll_slot = QINVAL_STAT_INIT;
> +    volatile u32 poll_slot = QINVAL_STAT_INIT;
>      int index = -1;
>      int ret = -1;
>      unsigned long flags;
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 1908 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/4] VT-d/qinval: make local variable used for communication with IOMMU "volatile"

[Jan Beulich]

>>> On 16.06.14 at 14:55, <andrew.cooper3@citrix.com> wrote:
> On 16/06/14 13:47, Jan Beulich wrote:
>> Without that there is - afaict - nothing preventing the compiler from
>> putting the variable into a register for the duration of the wait loop.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I seem to remember Tim saying that behaviour like this is covered by
> -fno-strict-aliasing

Afaik it would be if the variable was not an automatic one.

Jan

Re: [PATCH 2/4] VT-d: drop redundant calls to invalidate_sync()

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2223 bytes --]

On 16/06/14 13:48, Jan Beulich wrote:
> The call tree iommu_flush_iec_index() -> __iommu_flush_iec() already
> invokes invalidate_sync(). Removing the superfluous instances at once
> allows the function to become static.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -48,7 +48,6 @@ int queue_invalidate_iotlb(struct iommu 
>      u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr);
>  int queue_invalidate_iec(struct iommu *iommu,
>      u8 granu, u8 im, u16 iidx);
> -int invalidate_sync(struct iommu *iommu);
>  int iommu_flush_iec_global(struct iommu *iommu);
>  int iommu_flush_iec_index(struct iommu *iommu, u8 im, u16 iidx);
>  void clear_fault_bits(struct iommu *iommu);
> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -358,7 +358,6 @@ static int ioapic_rte_to_remap_entry(str
>      memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
>      iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
>      iommu_flush_iec_index(iommu, 0, index);
> -    invalidate_sync(iommu);
>  
>      unmap_vtd_domain_page(iremap_entries);
>      spin_unlock_irqrestore(&ir_ctrl->iremap_lock, flags);
> @@ -643,7 +642,6 @@ static int msi_msg_to_remap_entry(
>      memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
>      iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
>      iommu_flush_iec_index(iommu, 0, index);
> -    invalidate_sync(iommu);
>  
>      unmap_vtd_domain_page(iremap_entries);
>      spin_unlock_irqrestore(&ir_ctrl->iremap_lock, flags);
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -228,7 +228,7 @@ static int queue_invalidate_wait(struct 
>      return ret;
>  }
>  
> -int invalidate_sync(struct iommu *iommu)
> +static int invalidate_sync(struct iommu *iommu)
>  {
>      int ret = -1;
>      struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 3082 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/4] VT-d/qinval: clean up error handling

[Andrew Cooper]

On 16/06/14 13:48, Jan Beulich wrote:
> - neither qinval_update_qtail() nor qinval_next_index() can fail: make
>   the former return "void", and drop caller error checks for the latter
>   (all of which would otherwise return with a spin lock still held)
> - or-ing together error codes is a bad idea
>
> At once drop bogus initializers.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -58,7 +58,7 @@ static int qinval_next_index(struct iomm
>      return tail;
>  }
>  
> -static int qinval_update_qtail(struct iommu *iommu, int index)
> +static void qinval_update_qtail(struct iommu *iommu, int index)
>  {
>      u64 val;
>  
> @@ -66,7 +66,6 @@ static int qinval_update_qtail(struct io
>      ASSERT( spin_is_locked(&iommu->register_lock) );
>      val = (index + 1) % QINVAL_ENTRY_NR;
>      dmar_writeq(iommu->reg, DMAR_IQT_REG, (val << QINVAL_INDEX_SHIFT));
> -    return 0;
>  }
>  
>  static int gen_cc_inv_dsc(struct iommu *iommu, int index,
> @@ -100,17 +99,15 @@ static int gen_cc_inv_dsc(struct iommu *
>  int queue_invalidate_context(struct iommu *iommu,
>      u16 did, u16 source_id, u8 function_mask, u8 granu)
>  {
> -    int ret = -1;
> +    int ret;
>      unsigned long flags;
>      int index = -1;

This index initialiser is also bogus.  It is assigned straight from
qinval_next_index().

But peeking ahead to your next patch, you have addressed it there.

I would suggest that the dropping of -EBUSY clauses would more logically
live with the following patch, but the end result of patches 3 + 4
appear fine.

Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 4/4] VT-d/qinval: queue index is always unsigned

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 3816 bytes --]

On 16/06/14 13:49, Jan Beulich wrote:
> At once drop bogus initializers.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -43,7 +43,7 @@ static void print_qi_regs(struct iommu *
>      printk("DMAR_IQT_REG = %"PRIx64"\n", val);
>  }
>  
> -static int qinval_next_index(struct iommu *iommu)
> +static unsigned int qinval_next_index(struct iommu *iommu)
>  {
>      u64 tail;
>  
> @@ -58,7 +58,7 @@ static int qinval_next_index(struct iomm
>      return tail;
>  }
>  
> -static void qinval_update_qtail(struct iommu *iommu, int index)
> +static void qinval_update_qtail(struct iommu *iommu, unsigned int index)
>  {
>      u64 val;
>  
> @@ -68,7 +68,7 @@ static void qinval_update_qtail(struct i
>      dmar_writeq(iommu->reg, DMAR_IQT_REG, (val << QINVAL_INDEX_SHIFT));
>  }
>  
> -static int gen_cc_inv_dsc(struct iommu *iommu, int index,
> +static int gen_cc_inv_dsc(struct iommu *iommu, unsigned int index,
>      u16 did, u16 source_id, u8 function_mask, u8 granu)
>  {
>      unsigned long flags;
> @@ -101,7 +101,7 @@ int queue_invalidate_context(struct iomm
>  {
>      int ret;
>      unsigned long flags;
> -    int index = -1;
> +    unsigned int index;
>  
>      spin_lock_irqsave(&iommu->register_lock, flags);
>      index = qinval_next_index(iommu);
> @@ -112,7 +112,7 @@ int queue_invalidate_context(struct iomm
>      return ret;
>  }
>  
> -static int gen_iotlb_inv_dsc(struct iommu *iommu, int index,
> +static int gen_iotlb_inv_dsc(struct iommu *iommu, unsigned int index,
>      u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr)
>  {
>      unsigned long flags;
> @@ -149,7 +149,7 @@ int queue_invalidate_iotlb(struct iommu 
>  {
>      int ret;
>      unsigned long flags;
> -    int index = -1;
> +    unsigned int index;
>  
>      spin_lock_irqsave(&iommu->register_lock, flags);
>  
> @@ -161,7 +161,7 @@ int queue_invalidate_iotlb(struct iommu 
>      return ret;
>  }
>  
> -static int gen_wait_dsc(struct iommu *iommu, int index,
> +static int gen_wait_dsc(struct iommu *iommu, unsigned int index,
>      u8 iflag, u8 sw, u8 fn, u32 sdata, volatile u32 *saddr)
>  {
>      unsigned long flags;
> @@ -192,7 +192,7 @@ static int queue_invalidate_wait(struct 
>  {
>      s_time_t start_time;
>      volatile u32 poll_slot = QINVAL_STAT_INIT;
> -    int index = -1;
> +    unsigned int index;
>      int ret;
>      unsigned long flags;
>  
> @@ -231,7 +231,7 @@ static int invalidate_sync(struct iommu 
>      return 0;
>  }
>  
> -static int gen_dev_iotlb_inv_dsc(struct iommu *iommu, int index,
> +static int gen_dev_iotlb_inv_dsc(struct iommu *iommu, unsigned int index,
>      u32 max_invs_pend, u16 sid, u16 size, u64 addr)
>  {
>      unsigned long flags;
> @@ -266,7 +266,7 @@ int qinval_device_iotlb(struct iommu *io
>  {
>      int ret;
>      unsigned long flags;
> -    int index = -1;
> +    unsigned int index;
>  
>      spin_lock_irqsave(&iommu->register_lock, flags);
>      index = qinval_next_index(iommu);
> @@ -277,7 +277,7 @@ int qinval_device_iotlb(struct iommu *io
>      return ret;
>  }
>  
> -static int gen_iec_inv_dsc(struct iommu *iommu, int index,
> +static int gen_iec_inv_dsc(struct iommu *iommu, unsigned int index,
>      u8 granu, u8 im, u16 iidx)
>  {
>      unsigned long flags;
> @@ -308,7 +308,7 @@ int queue_invalidate_iec(struct iommu *i
>  {
>      int ret;
>      unsigned long flags;
> -    int index = -1;
> +    unsigned int index;
>  
>      spin_lock_irqsave(&iommu->register_lock, flags);
>      index = qinval_next_index(iommu);
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 4549 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/4] VT-d/qinval: make local variable used for communication with IOMMU "volatile"

[Zhang, Yang Z]

Jan Beulich wrote on 2014-06-16:
> Without that there is - afaict - nothing preventing the compiler from 
> putting the variable into a register for the duration of the wait loop.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Yang Zhang <yang.z.zhang@intel.com>

> 
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -196,7 +196,7 @@ static int queue_invalidate_wait(struct
>      u8 iflag, u8 sw, u8 fn) { s_time_t start_time;
> -    u32 poll_slot = QINVAL_STAT_INIT;
> +    volatile u32 poll_slot = QINVAL_STAT_INIT;
>      int index = -1;
>      int ret = -1;
>      unsigned long flags;
>


Best regards,
Yang

Re: [PATCH 2/4] VT-d: drop redundant calls to invalidate_sync()

[Zhang, Yang Z]

Jan Beulich wrote on 2014-06-16:
> The call tree iommu_flush_iec_index() -> __iommu_flush_iec() already 
> invokes invalidate_sync(). Removing the superfluous instances at once 
> allows the function to become static.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Yang Zhang <yang.z.zhang@intel.com>

> 
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -48,7 +48,6 @@ int queue_invalidate_iotlb(struct iommu
>      u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr);  int
>      queue_invalidate_iec(struct iommu *iommu, u8 granu, u8 im, u16
>      iidx);
> -int invalidate_sync(struct iommu *iommu);  int 
> iommu_flush_iec_global(struct iommu *iommu);  int 
> iommu_flush_iec_index(struct iommu *iommu, u8 im, u16 iidx);  void 
> clear_fault_bits(struct iommu *iommu);
> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -358,7 +358,6 @@ static int ioapic_rte_to_remap_entry(str
>      memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
>      iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
>      iommu_flush_iec_index(iommu, 0, index);
> -    invalidate_sync(iommu);
> 
>      unmap_vtd_domain_page(iremap_entries);
>      spin_unlock_irqrestore(&ir_ctrl->iremap_lock, flags); @@ -643,7
>      +642,6 @@ static int msi_msg_to_remap_entry( memcpy(iremap_entry,
>      &new_ire, sizeof(struct iremap_entry));
>      iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
>      iommu_flush_iec_index(iommu, 0, index);
> -    invalidate_sync(iommu);
> 
>      unmap_vtd_domain_page(iremap_entries);
>      spin_unlock_irqrestore(&ir_ctrl->iremap_lock, flags);
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -228,7 +228,7 @@ static int queue_invalidate_wait(struct
>      return ret;
>  }
> -int invalidate_sync(struct iommu *iommu)
> +static int invalidate_sync(struct iommu *iommu)
>  {
>      int ret = -1;
>      struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
>


Best regards,
Yang

Re: [PATCH 4/4] VT-d/qinval: queue index is always unsigned

[Zhang, Yang Z]

Jan Beulich wrote on 2014-06-16:
> At once drop bogus initializers.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Yang Zhang <yang.z.zhang@intel.com>

> 
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -43,7 +43,7 @@ static void print_qi_regs(struct iommu *
>      printk("DMAR_IQT_REG = %"PRIx64"\n", val);  } -static int 
> qinval_next_index(struct iommu *iommu)
> +static unsigned int qinval_next_index(struct iommu *iommu)
>  {
>      u64 tail;
> @@ -58,7 +58,7 @@ static int qinval_next_index(struct iomm
>      return tail;
>  }
> -static void qinval_update_qtail(struct iommu *iommu, int index)
> +static void qinval_update_qtail(struct iommu *iommu, unsigned int
> +index)
>  {
>      u64 val;
> @@ -68,7 +68,7 @@ static void qinval_update_qtail(struct i
>      dmar_writeq(iommu->reg, DMAR_IQT_REG, (val << 
> QINVAL_INDEX_SHIFT));  }
> 
> -static int gen_cc_inv_dsc(struct iommu *iommu, int index,
> +static int gen_cc_inv_dsc(struct iommu *iommu, unsigned int index,
>      u16 did, u16 source_id, u8 function_mask, u8 granu)  { unsigned
>      long flags; @@ -101,7 +101,7 @@ int queue_invalidate_context(struct
>      iomm  { int ret; unsigned long flags;
> -    int index = -1;
> +    unsigned int index;
> 
>      spin_lock_irqsave(&iommu->register_lock, flags); index =
>      qinval_next_index(iommu); @@ -112,7 +112,7 @@ int
>      queue_invalidate_context(struct iomm return ret;  } -static int 
> gen_iotlb_inv_dsc(struct iommu *iommu, int index,
> +static int gen_iotlb_inv_dsc(struct iommu *iommu, unsigned int index,
>      u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr)  {
>      unsigned long flags; @@ -149,7 +149,7 @@ int
>      queue_invalidate_iotlb(struct iommu  { int ret; unsigned long flags;
> -    int index = -1;
> +    unsigned int index;
> 
>      spin_lock_irqsave(&iommu->register_lock, flags); @@ -161,7 +161,7 
> @@ int queue_invalidate_iotlb(struct iommu
>      return ret;
>  }
> -static int gen_wait_dsc(struct iommu *iommu, int index,
> +static int gen_wait_dsc(struct iommu *iommu, unsigned int index,
>      u8 iflag, u8 sw, u8 fn, u32 sdata, volatile u32 *saddr)  { unsigned
>      long flags; @@ -192,7 +192,7 @@ static int
>      queue_invalidate_wait(struct  { s_time_t start_time; volatile u32
>      poll_slot = QINVAL_STAT_INIT;
> -    int index = -1;
> +    unsigned int index;
>      int ret;
>      unsigned long flags;
> @@ -231,7 +231,7 @@ static int invalidate_sync(struct iommu
>      return 0;
>  }
> -static int gen_dev_iotlb_inv_dsc(struct iommu *iommu, int index,
> +static int gen_dev_iotlb_inv_dsc(struct iommu *iommu, unsigned int 
> +index,
>      u32 max_invs_pend, u16 sid, u16 size, u64 addr)  { unsigned long
>      flags; @@ -266,7 +266,7 @@ int qinval_device_iotlb(struct iommu *io
>       { int ret; unsigned long flags;
> -    int index = -1;
> +    unsigned int index;
> 
>      spin_lock_irqsave(&iommu->register_lock, flags); index =
>      qinval_next_index(iommu); @@ -277,7 +277,7 @@ int
>      qinval_device_iotlb(struct iommu *io return ret;  } -static int 
> gen_iec_inv_dsc(struct iommu *iommu, int index,
> +static int gen_iec_inv_dsc(struct iommu *iommu, unsigned int index,
>      u8 granu, u8 im, u16 iidx) { unsigned long flags; @@ -308,7 +308,7
>      @@ int queue_invalidate_iec(struct iommu *i  { int ret; unsigned
>      long flags;
> -    int index = -1;
> +    unsigned int index;
> 
>      spin_lock_irqsave(&iommu->register_lock, flags);
>      index = qinval_next_index(iommu);
>


Best regards,
Yang

Re: [PATCH 3/4] VT-d/qinval: clean up error handling

[Zhang, Yang Z]

Jan Beulich wrote on 2014-06-16:
> - neither qinval_update_qtail() nor qinval_next_index() can fail: make
>   the former return "void", and drop caller error checks for the latter
>   (all of which would otherwise return with a spin lock still held)

I saw lots of other functions are never fail too, e.g., gen_cc_inv_dsc(), gen_iotlb_inv_dsc(), gen_wait_dsc() and others. Any reason only changes the above two and keeps others?

Best regards,
Yang

Re: [PATCH 3/4] VT-d/qinval: clean up error handling

[Jan Beulich]

>>> On 20.06.14 at 04:12, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-06-16:
>> - neither qinval_update_qtail() nor qinval_next_index() can fail: make
>>   the former return "void", and drop caller error checks for the latter
>>   (all of which would otherwise return with a spin lock still held)
> 
> I saw lots of other functions are never fail too, e.g., gen_cc_inv_dsc(), 
> gen_iotlb_inv_dsc(), gen_wait_dsc() and others. Any reason only changes the 
> above two and keeps others?

I wanted to leave the gen_* function family aside for the moment, as
they're having more problems than just their return types: Their use
of qinval_lock is completely bogus, as in all cases the callers already
hold iommu->register_lock. The former lock therefore could go away
altogether. And then it becomes rather questionable whether these
single-use functions really need to be separate ones or wouldn't
better be integrated into their callers.

Jan

Re: [PATCH 3/4] VT-d/qinval: clean up error handling

[Zhang, Yang Z]

Jan Beulich wrote on 2014-06-20:
>>>> On 20.06.14 at 04:12, <yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2014-06-16:
>>> - neither qinval_update_qtail() nor qinval_next_index() can fail: make
>>>   the former return "void", and drop caller error checks for the latter
>>>   (all of which would otherwise return with a spin lock still held)
>> 
>> I saw lots of other functions are never fail too, e.g.,
>> gen_cc_inv_dsc(), gen_iotlb_inv_dsc(), gen_wait_dsc() and others.
>> Any reason only changes the above two and keeps others?
> 
> I wanted to leave the gen_* function family aside for the moment, as
> they're having more problems than just their return types: Their use
> of qinval_lock is completely bogus, as in all cases the callers already hold iommu->register_lock.
> The former lock therefore could go away altogether. And then it
> becomes rather questionable whether these single-use functions really
> need to be separate ones or wouldn't better be integrated into their callers.

I see. 

For this patch:

Acked-by: Yang Zhang <yang.z.zhang@intel.com>


> 
> Jan


Best regards,
Yang