[PATCHv2 0 of 2] Deal with IOMMU faults in softirq context.

[PATCHv2 0 of 2] Deal with IOMMU faults in softirq context.

[Dario Faggioli]

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

Hello everyone,

Reposting with after having applied the (minor) fixes suggested by Wei
and Jan.

Allen, if you can tell us what you think about this, or suggest someone
else to ask some feedback to, if you're no longer involved with VT-d,
that would be great! :-)

--

As already discussed here [1], dealing with IOMMU faults in interrupt
context may cause nasty things to happen, up to being used as a form of
DoS attack, e.g., by generating a "storm" of IOMMU faults that will
livelock a pCPU.

To avoid this, IOMMU faults handling is being moved from interrupt to
softirq context. Basically, the inerrupt handler of the IRQ originated
by an IOMMU (page) fault will raise a softirq-tasklet which will then
deal with the actual fault records by clearing the logs and re-enabling
interrupts from the offending IOMMU(s). A single tasklet is being used
even if there are more than just one IOMMU in the system, as the event
should be rare enough.

The series introduces the described mechanism for both Intel VT-d and
AMD-Vi, and has been tested on both platforms with an hacked DomU bnx2
network driver which was generating I/O page faults upon request.

Thanks and Regards,
Dario

[1] http://old-list-archives.xen.org/archives/html/xen-devel/2011-08/msg00638.html

--
 iommu-fault-tasklet_vtd.patch
 iommu-fault-tasklet_amd.patch

 xen/drivers/passthrough/amd/iommu_init.c |  47 ++++++++++++++++++++++++++++++++++++++++++++---
 xen/drivers/passthrough/vtd/iommu.c      |  39 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 80 insertions(+), 6 deletions(-)

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

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

[PATCHv2 1 of 2] Move IOMMU faults handling into softirq for VT-d.

[Dario Faggioli]

[-- Attachment #1.1.1: Type: text/plain, Size: 3067 bytes --]

Dealing with interrupts from VT-d IOMMU(s) is deferred to a softirq-tasklet,
raised by the actual IRQ handler. Since a new interrupt is not generated,
even if further faults occur, until we cleared all the pending ones,
there's no need of disabling IRQs, as the hardware does it by its own.
Notice that this may cause the log to overflow, but none of the existing
entry will be overwritten.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

diff -r efaa28639a71 xen/drivers/passthrough/vtd/iommu.c
--- a/xen/drivers/passthrough/vtd/iommu.c	Wed Jan 04 16:12:44 2012 +0000
+++ b/xen/drivers/passthrough/vtd/iommu.c	Thu Jan 05 15:17:47 2012 +0100
@@ -53,6 +53,8 @@ bool_t __read_mostly untrusted_msi;
 
 int nr_iommus;
 
+static struct tasklet vtd_fault_tasklet;
+
 static void setup_dom0_device(struct pci_dev *);
 static void setup_dom0_rmrr(struct domain *d);
 
@@ -918,10 +920,8 @@ static void iommu_fault_status(u32 fault
 }
 
 #define PRIMARY_FAULT_REG_LEN (16)
-static void iommu_page_fault(int irq, void *dev_id,
-                             struct cpu_user_regs *regs)
+static void __do_iommu_page_fault(struct iommu *iommu)
 {
-    struct iommu *iommu = dev_id;
     int reg, fault_index;
     u32 fault_status;
     unsigned long flags;
@@ -996,6 +996,37 @@ clear_overflow:
     }
 }
 
+static void do_iommu_page_fault(unsigned long data)
+{
+    struct acpi_drhd_unit *drhd;
+
+    if ( list_empty(&acpi_drhd_units) )
+    {
+       INTEL_IOMMU_DEBUG("no device found, something must be very wrong!\n");
+       return;
+    }
+
+    /*
+     * No matter from whom the interrupt came from, check all the
+     * IOMMUs present in the system. This allows for having just one
+     * tasklet (instead of one per each IOMMUs) and should be more than
+     * fine, considering how rare the event of a fault should be.
+     */
+    for_each_drhd_unit ( drhd )
+        __do_iommu_page_fault(drhd->iommu);
+}
+
+static void iommu_page_fault(int irq, void *dev_id,
+                             struct cpu_user_regs *regs)
+{
+    /*
+     * Just flag the tasklet as runnable. This is fine, according to VT-d
+     * specs since a new interrupt won't be generated until we clear all
+     * the faults that caused this one to happen.
+     */
+    tasklet_schedule(&vtd_fault_tasklet);
+}
+
 static void dma_msi_unmask(struct irq_desc *desc)
 {
     struct iommu *iommu = desc->action->dev_id;
@@ -2144,6 +2175,8 @@ int __init intel_vtd_setup(void)
         iommu->irq = ret;
     }
 
+    softirq_tasklet_init(&vtd_fault_tasklet, do_iommu_page_fault, 0);
+
     if ( !iommu_qinval && iommu_intremap )
     {
         iommu_intremap = 0;

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)


[-- Attachment #1.1.2: iommu-fault-tasklet_vtd.patch --]
[-- Type: text/x-patch, Size: 2858 bytes --]

# HG changeset patch
# Parent efaa28639a71524a693ba500624f8512e5795e18
Move IOMMU faults handling into softirq for VT-d.

Dealing with interrupts from VT-d IOMMU(s) is deferred to a softirq-tasklet,
raised by the actual IRQ handler. Since a new interrupt is not generated,
even if further faults occur, until we cleared all the pending ones,
there's no need of disabling IRQs, as the hardware does it by its own.
Notice that this may cause the log to overflow, but none of the existing
entry will be overwritten.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

diff -r efaa28639a71 xen/drivers/passthrough/vtd/iommu.c
--- a/xen/drivers/passthrough/vtd/iommu.c	Wed Jan 04 16:12:44 2012 +0000
+++ b/xen/drivers/passthrough/vtd/iommu.c	Thu Jan 05 15:17:47 2012 +0100
@@ -53,6 +53,8 @@ bool_t __read_mostly untrusted_msi;
 
 int nr_iommus;
 
+static struct tasklet vtd_fault_tasklet;
+
 static void setup_dom0_device(struct pci_dev *);
 static void setup_dom0_rmrr(struct domain *d);
 
@@ -918,10 +920,8 @@ static void iommu_fault_status(u32 fault
 }
 
 #define PRIMARY_FAULT_REG_LEN (16)
-static void iommu_page_fault(int irq, void *dev_id,
-                             struct cpu_user_regs *regs)
+static void __do_iommu_page_fault(struct iommu *iommu)
 {
-    struct iommu *iommu = dev_id;
     int reg, fault_index;
     u32 fault_status;
     unsigned long flags;
@@ -996,6 +996,37 @@ clear_overflow:
     }
 }
 
+static void do_iommu_page_fault(unsigned long data)
+{
+    struct acpi_drhd_unit *drhd;
+
+    if ( list_empty(&acpi_drhd_units) )
+    {
+       INTEL_IOMMU_DEBUG("no device found, something must be very wrong!\n");
+       return;
+    }
+
+    /*
+     * No matter from whom the interrupt came from, check all the
+     * IOMMUs present in the system. This allows for having just one
+     * tasklet (instead of one per each IOMMUs) and should be more than
+     * fine, considering how rare the event of a fault should be.
+     */
+    for_each_drhd_unit ( drhd )
+        __do_iommu_page_fault(drhd->iommu);
+}
+
+static void iommu_page_fault(int irq, void *dev_id,
+                             struct cpu_user_regs *regs)
+{
+    /*
+     * Just flag the tasklet as runnable. This is fine, according to VT-d
+     * specs since a new interrupt won't be generated until we clear all
+     * the faults that caused this one to happen.
+     */
+    tasklet_schedule(&vtd_fault_tasklet);
+}
+
 static void dma_msi_unmask(struct irq_desc *desc)
 {
     struct iommu *iommu = desc->action->dev_id;
@@ -2144,6 +2175,8 @@ int __init intel_vtd_setup(void)
         iommu->irq = ret;
     }
 
+    softirq_tasklet_init(&vtd_fault_tasklet, do_iommu_page_fault, 0);
+
     if ( !iommu_qinval && iommu_intremap )
     {
         iommu_intremap = 0;

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

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

Re: [PATCHv2 1 of 2] Move IOMMU faults handling into softirq for VT-d.

[Keir Fraser]

On 05/01/2012 15:25, "Dario Faggioli" <raistlin@linux.it> wrote:

> Dealing with interrupts from VT-d IOMMU(s) is deferred to a softirq-tasklet,
> raised by the actual IRQ handler. Since a new interrupt is not generated,
> even if further faults occur, until we cleared all the pending ones,
> there's no need of disabling IRQs, as the hardware does it by its own.
> Notice that this may cause the log to overflow, but none of the existing
> entry will be overwritten.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

Applied, thanks.

 -- Keir

> diff -r efaa28639a71 xen/drivers/passthrough/vtd/iommu.c
> --- a/xen/drivers/passthrough/vtd/iommu.c Wed Jan 04 16:12:44 2012 +0000
> +++ b/xen/drivers/passthrough/vtd/iommu.c Thu Jan 05 15:17:47 2012 +0100
> @@ -53,6 +53,8 @@ bool_t __read_mostly untrusted_msi;
>  
>  int nr_iommus;
>  
> +static struct tasklet vtd_fault_tasklet;
> +
>  static void setup_dom0_device(struct pci_dev *);
>  static void setup_dom0_rmrr(struct domain *d);
>  
> @@ -918,10 +920,8 @@ static void iommu_fault_status(u32 fault
>  }
>  
>  #define PRIMARY_FAULT_REG_LEN (16)
> -static void iommu_page_fault(int irq, void *dev_id,
> -                             struct cpu_user_regs *regs)
> +static void __do_iommu_page_fault(struct iommu *iommu)
>  {
> -    struct iommu *iommu = dev_id;
>      int reg, fault_index;
>      u32 fault_status;
>      unsigned long flags;
> @@ -996,6 +996,37 @@ clear_overflow:
>      }
>  }
>  
> +static void do_iommu_page_fault(unsigned long data)
> +{
> +    struct acpi_drhd_unit *drhd;
> +
> +    if ( list_empty(&acpi_drhd_units) )
> +    {
> +       INTEL_IOMMU_DEBUG("no device found, something must be very wrong!\n");
> +       return;
> +    }
> +
> +    /*
> +     * No matter from whom the interrupt came from, check all the
> +     * IOMMUs present in the system. This allows for having just one
> +     * tasklet (instead of one per each IOMMUs) and should be more than
> +     * fine, considering how rare the event of a fault should be.
> +     */
> +    for_each_drhd_unit ( drhd )
> +        __do_iommu_page_fault(drhd->iommu);
> +}
> +
> +static void iommu_page_fault(int irq, void *dev_id,
> +                             struct cpu_user_regs *regs)
> +{
> +    /*
> +     * Just flag the tasklet as runnable. This is fine, according to VT-d
> +     * specs since a new interrupt won't be generated until we clear all
> +     * the faults that caused this one to happen.
> +     */
> +    tasklet_schedule(&vtd_fault_tasklet);
> +}
> +
>  static void dma_msi_unmask(struct irq_desc *desc)
>  {
>      struct iommu *iommu = desc->action->dev_id;
> @@ -2144,6 +2175,8 @@ int __init intel_vtd_setup(void)
>          iommu->irq = ret;
>      }
>  
> +    softirq_tasklet_init(&vtd_fault_tasklet, do_iommu_page_fault, 0);
> +
>      if ( !iommu_qinval && iommu_intremap )
>      {
>          iommu_intremap = 0;

[PATCHv2 2 of 2] Move IOMMU faults handling into softirq for AMD-Vi.

[Dario Faggioli]

[-- Attachment #1.1.1: Type: text/plain, Size: 3512 bytes --]

Dealing with interrupts from AMD-Vi IOMMU(s) is deferred to a softirq-tasklet,
raised by the actual IRQ handler. To avoid more interrupts being generated
(because of further faults), they must be masked in the IOMMU within the low
level IRQ handler and enabled back in the tasklet body. Notice that this may
cause the log to overflow, but none of the existing entry will be overwritten.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

diff -r 3cb587bb34d0 xen/drivers/passthrough/amd/iommu_init.c
--- a/xen/drivers/passthrough/amd/iommu_init.c	Thu Jan 05 15:12:35 2012 +0100
+++ b/xen/drivers/passthrough/amd/iommu_init.c	Thu Jan 05 15:14:03 2012 +0100
@@ -32,6 +32,8 @@
 
 static int __initdata nr_amd_iommus;
 
+static struct tasklet amd_iommu_fault_tasklet;
+
 unsigned short ivrs_bdf_entries;
 static struct radix_tree_root ivrs_maps;
 struct list_head amd_iommu_head;
@@ -522,12 +524,10 @@ static void parse_event_log_entry(struct
     }
 }
 
-static void amd_iommu_page_fault(int irq, void *dev_id,
-                             struct cpu_user_regs *regs)
+static void __do_amd_iommu_page_fault(struct amd_iommu *iommu)
 {
     u32 entry;
     unsigned long flags;
-    struct amd_iommu *iommu = dev_id;
 
     spin_lock_irqsave(&iommu->lock, flags);
     amd_iommu_read_event_log(iommu);
@@ -546,6 +546,45 @@ static void amd_iommu_page_fault(int irq
     spin_unlock_irqrestore(&iommu->lock, flags);
 }
 
+static void do_amd_iommu_page_fault(unsigned long data)
+{
+    struct amd_iommu *iommu;
+
+    if ( !iommu_found() )
+    {
+        AMD_IOMMU_DEBUG("no device found, something must be very wrong!\n");
+        return;
+    }
+
+    /*
+     * No matter from whom the interrupt came from, check all the
+     * IOMMUs present in the system. This allows for having just one
+     * tasklet (instead of one per each IOMMUs) and should be more than
+     * fine, considering how rare the event of a fault should be.
+     */
+    for_each_amd_iommu ( iommu )
+        __do_amd_iommu_page_fault(iommu);
+}
+
+static void amd_iommu_page_fault(int irq, void *dev_id,
+                             struct cpu_user_regs *regs)
+{
+    u32 entry;
+    unsigned long flags;
+    struct amd_iommu *iommu = dev_id;
+
+    /* silence interrupts. The tasklet will enable them back */
+    spin_lock_irqsave(&iommu->lock, flags);
+    entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
+    iommu_clear_bit(&entry, IOMMU_STATUS_EVENT_LOG_INT_SHIFT);
+    writel(entry, iommu->mmio_base+IOMMU_STATUS_MMIO_OFFSET);
+    spin_unlock_irqrestore(&iommu->lock, flags);
+
+    /* Flag the tasklet as runnable so that it can execute, clear
+     * the log and re-enable interrupts. */
+    tasklet_schedule(&amd_iommu_fault_tasklet);
+}
+
 static int __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
 {
     int irq, ret;
@@ -884,6 +923,8 @@ int __init amd_iommu_init(void)
         if ( amd_iommu_init_one(iommu) != 0 )
             goto error_out;
 
+    softirq_tasklet_init(&amd_iommu_fault_tasklet, do_amd_iommu_page_fault, 0);
+
     return 0;
 
 error_out:

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)


[-- Attachment #1.1.2: iommu-fault-tasklet_amd.patch --]
[-- Type: text/x-patch, Size: 3305 bytes --]

# HG changeset patch
# Parent 3cb587bb34d0b1a586b0f789d70e423655fa8c27
Move IOMMU faults handling into softirq for AMD-Vi.

Dealing with interrupts from AMD-Vi IOMMU(s) is deferred to a softirq-tasklet,
raised by the actual IRQ handler. To avoid more interrupts being generated
(because of further faults), they must be masked in the IOMMU within the low
level IRQ handler and enabled back in the tasklet body. Notice that this may
cause the log to overflow, but none of the existing entry will be overwritten.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

diff -r 3cb587bb34d0 xen/drivers/passthrough/amd/iommu_init.c
--- a/xen/drivers/passthrough/amd/iommu_init.c	Thu Jan 05 15:12:35 2012 +0100
+++ b/xen/drivers/passthrough/amd/iommu_init.c	Thu Jan 05 15:14:03 2012 +0100
@@ -32,6 +32,8 @@
 
 static int __initdata nr_amd_iommus;
 
+static struct tasklet amd_iommu_fault_tasklet;
+
 unsigned short ivrs_bdf_entries;
 static struct radix_tree_root ivrs_maps;
 struct list_head amd_iommu_head;
@@ -522,12 +524,10 @@ static void parse_event_log_entry(struct
     }
 }
 
-static void amd_iommu_page_fault(int irq, void *dev_id,
-                             struct cpu_user_regs *regs)
+static void __do_amd_iommu_page_fault(struct amd_iommu *iommu)
 {
     u32 entry;
     unsigned long flags;
-    struct amd_iommu *iommu = dev_id;
 
     spin_lock_irqsave(&iommu->lock, flags);
     amd_iommu_read_event_log(iommu);
@@ -546,6 +546,45 @@ static void amd_iommu_page_fault(int irq
     spin_unlock_irqrestore(&iommu->lock, flags);
 }
 
+static void do_amd_iommu_page_fault(unsigned long data)
+{
+    struct amd_iommu *iommu;
+
+    if ( !iommu_found() )
+    {
+        AMD_IOMMU_DEBUG("no device found, something must be very wrong!\n");
+        return;
+    }
+
+    /*
+     * No matter from whom the interrupt came from, check all the
+     * IOMMUs present in the system. This allows for having just one
+     * tasklet (instead of one per each IOMMUs) and should be more than
+     * fine, considering how rare the event of a fault should be.
+     */
+    for_each_amd_iommu ( iommu )
+        __do_amd_iommu_page_fault(iommu);
+}
+
+static void amd_iommu_page_fault(int irq, void *dev_id,
+                             struct cpu_user_regs *regs)
+{
+    u32 entry;
+    unsigned long flags;
+    struct amd_iommu *iommu = dev_id;
+
+    /* silence interrupts. The tasklet will enable them back */
+    spin_lock_irqsave(&iommu->lock, flags);
+    entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
+    iommu_clear_bit(&entry, IOMMU_STATUS_EVENT_LOG_INT_SHIFT);
+    writel(entry, iommu->mmio_base+IOMMU_STATUS_MMIO_OFFSET);
+    spin_unlock_irqrestore(&iommu->lock, flags);
+
+    /* Flag the tasklet as runnable so that it can execute, clear
+     * the log and re-enable interrupts. */
+    tasklet_schedule(&amd_iommu_fault_tasklet);
+}
+
 static int __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
 {
     int irq, ret;
@@ -884,6 +923,8 @@ int __init amd_iommu_init(void)
         if ( amd_iommu_init_one(iommu) != 0 )
             goto error_out;
 
+    softirq_tasklet_init(&amd_iommu_fault_tasklet, do_amd_iommu_page_fault, 0);
+
     return 0;
 
 error_out:

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

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

Re: [PATCHv2 2 of 2] Move IOMMU faults handling into softirq for AMD-Vi.

[Keir Fraser]

On 05/01/2012 15:27, "Dario Faggioli" <raistlin@linux.it> wrote:

> Dealing with interrupts from AMD-Vi IOMMU(s) is deferred to a softirq-tasklet,
> raised by the actual IRQ handler. To avoid more interrupts being generated
> (because of further faults), they must be masked in the IOMMU within the low
> level IRQ handler and enabled back in the tasklet body. Notice that this may
> cause the log to overflow, but none of the existing entry will be overwritten.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

This patch needs fixing to apply to xen-unstable tip. Please do that and
resubmit.

 -- Keir

> diff -r 3cb587bb34d0 xen/drivers/passthrough/amd/iommu_init.c
> --- a/xen/drivers/passthrough/amd/iommu_init.c Thu Jan 05 15:12:35 2012 +0100
> +++ b/xen/drivers/passthrough/amd/iommu_init.c Thu Jan 05 15:14:03 2012 +0100
> @@ -32,6 +32,8 @@
>  
>  static int __initdata nr_amd_iommus;
>  
> +static struct tasklet amd_iommu_fault_tasklet;
> +
>  unsigned short ivrs_bdf_entries;
>  static struct radix_tree_root ivrs_maps;
>  struct list_head amd_iommu_head;
> @@ -522,12 +524,10 @@ static void parse_event_log_entry(struct
>      }
>  }
>  
> -static void amd_iommu_page_fault(int irq, void *dev_id,
> -                             struct cpu_user_regs *regs)
> +static void __do_amd_iommu_page_fault(struct amd_iommu *iommu)
>  {
>      u32 entry;
>      unsigned long flags;
> -    struct amd_iommu *iommu = dev_id;
>  
>      spin_lock_irqsave(&iommu->lock, flags);
>      amd_iommu_read_event_log(iommu);
> @@ -546,6 +546,45 @@ static void amd_iommu_page_fault(int irq
>      spin_unlock_irqrestore(&iommu->lock, flags);
>  }
>  
> +static void do_amd_iommu_page_fault(unsigned long data)
> +{
> +    struct amd_iommu *iommu;
> +
> +    if ( !iommu_found() )
> +    {
> +        AMD_IOMMU_DEBUG("no device found, something must be very wrong!\n");
> +        return;
> +    }
> +
> +    /*
> +     * No matter from whom the interrupt came from, check all the
> +     * IOMMUs present in the system. This allows for having just one
> +     * tasklet (instead of one per each IOMMUs) and should be more than
> +     * fine, considering how rare the event of a fault should be.
> +     */
> +    for_each_amd_iommu ( iommu )
> +        __do_amd_iommu_page_fault(iommu);
> +}
> +
> +static void amd_iommu_page_fault(int irq, void *dev_id,
> +                             struct cpu_user_regs *regs)
> +{
> +    u32 entry;
> +    unsigned long flags;
> +    struct amd_iommu *iommu = dev_id;
> +
> +    /* silence interrupts. The tasklet will enable them back */
> +    spin_lock_irqsave(&iommu->lock, flags);
> +    entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
> +    iommu_clear_bit(&entry, IOMMU_STATUS_EVENT_LOG_INT_SHIFT);
> +    writel(entry, iommu->mmio_base+IOMMU_STATUS_MMIO_OFFSET);
> +    spin_unlock_irqrestore(&iommu->lock, flags);
> +
> +    /* Flag the tasklet as runnable so that it can execute, clear
> +     * the log and re-enable interrupts. */
> +    tasklet_schedule(&amd_iommu_fault_tasklet);
> +}
> +
>  static int __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
>  {
>      int irq, ret;
> @@ -884,6 +923,8 @@ int __init amd_iommu_init(void)
>          if ( amd_iommu_init_one(iommu) != 0 )
>              goto error_out;
>  
> +    softirq_tasklet_init(&amd_iommu_fault_tasklet, do_amd_iommu_page_fault,
> 0);
> +
>      return 0;
>  
>  error_out:

Re: [PATCHv2 2 of 2] Move IOMMU faults handling into softirq for AMD-Vi.

[Dario Faggioli]

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

On Tue, 2012-01-17 at 11:17 +0000, Keir Fraser wrote: 
> > Dealing with interrupts from AMD-Vi IOMMU(s) is deferred to a softirq-tasklet,
> > raised by the actual IRQ handler. To avoid more interrupts being generated
> > (because of further faults), they must be masked in the IOMMU within the low
> > level IRQ handler and enabled back in the tasklet body. Notice that this may
> > cause the log to overflow, but none of the existing entry will be overwritten.
> > 
> > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> 
> This patch needs fixing to apply to xen-unstable tip. Please do that and
> resubmit.
> 
I see. I can easily rebase the patch but there are functional changes
involved, so I'd like to know what you think it's best to do first.

In particular, the clash is against Wei's patches introducing PPR. So
now the IOMMU interrupt handler checks both event log and ppr log.

Question is, should I move _BOTH_ these checks into softirq or just
defer event log processing, and leave ppr log handling in hard-irq
context? Quickly looking at the new specs, it seems to me that deferring
both should be fine, but I'd really appreciate your thoughts...

Wei, Jan, Tim?

Thanks and regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)



[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

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

Re: [PATCHv2 2 of 2] Move IOMMU faults handling into softirq for AMD-Vi.

[Wei Wang]

On 01/18/2012 09:53 AM, Dario Faggioli wrote:
> On Tue, 2012-01-17 at 11:17 +0000, Keir Fraser wrote:
>>> Dealing with interrupts from AMD-Vi IOMMU(s) is deferred to a softirq-tasklet,
>>> raised by the actual IRQ handler. To avoid more interrupts being generated
>>> (because of further faults), they must be masked in the IOMMU within the low
>>> level IRQ handler and enabled back in the tasklet body. Notice that this may
>>> cause the log to overflow, but none of the existing entry will be overwritten.
>>>
>>> Signed-off-by: Dario Faggioli<dario.faggioli@citrix.com>
>>
>> This patch needs fixing to apply to xen-unstable tip. Please do that and
>> resubmit.
>>
> I see. I can easily rebase the patch but there are functional changes
> involved, so I'd like to know what you think it's best to do first.
>
> In particular, the clash is against Wei's patches introducing PPR. So
> now the IOMMU interrupt handler checks both event log and ppr log.
>
> Question is, should I move _BOTH_ these checks into softirq or just
> defer event log processing, and leave ppr log handling in hard-irq
> context? Quickly looking at the new specs, it seems to me that deferring
> both should be fine, but I'd really appreciate your thoughts...

I think put both event log and ppr log into softirq is fine. If you 
could have a patch like this, I can do a quick test on my machine.
Thanks,
Wei

> Wei, Jan, Tim?
>
> Thanks and regards,
> Dario
>

Re: [PATCHv2 2 of 2] Move IOMMU faults handling into softirq for AMD-Vi.

[Dario Faggioli]

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

On Wed, 2012-01-18 at 11:40 +0100, Wei Wang wrote:
> I think put both event log and ppr log into softirq is fine. 
>
Ok then.

> If you 
> could have a patch like this, I can do a quick test on my machine.
>
Oh, that would be great. I'll put it together and send ASAP.

Thanks,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

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

[PATCHv3] Move IOMMU faults handling into softirq for AMD-Vi.

[Dario Faggioli]

[-- Attachment #1.1.1: Type: text/plain, Size: 3553 bytes --]

Hello Wei,

Here it is, and it seems to work for me, but of course I'm not testing
PPR. If you could give this a go and let me know... I'll repost in a
separate thread if it happens to be fine.

Thanks again,
Dario

--

Move IOMMU faults handling into softirq for AMD-Vi.
                  
Dealing with interrupts from AMD-Vi IOMMU(s) is deferred to a softirq-tasklet,
raised by the actual IRQ handler. To avoid more interrupts being generated
(because of further faults), they must be masked in the IOMMU within the low
level IRQ handler and enabled back in the tasklet body. Notice that this may
cause the log to overflow, but none of the existing entry will be overwritten.
                                 
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

diff -r 15ab61865ecb xen/drivers/passthrough/amd/iommu_init.c
--- a/xen/drivers/passthrough/amd/iommu_init.c	Tue Jan 17 12:40:52 2012 +0000
+++ b/xen/drivers/passthrough/amd/iommu_init.c	Wed Jan 18 13:01:23 2012 +0100
@@ -32,6 +32,8 @@
 
 static int __initdata nr_amd_iommus;
 
+static struct tasklet amd_iommu_irq_tasklet;
+
 unsigned short ivrs_bdf_entries;
 static struct radix_tree_root ivrs_maps;
 struct list_head amd_iommu_head;
@@ -689,14 +691,48 @@ static void iommu_check_ppr_log(struct a
     spin_unlock_irqrestore(&iommu->lock, flags);
 }
 
+static void do_amd_iommu_irq(unsigned long data)
+{
+    struct amd_iommu *iommu;
+
+    if ( !iommu_found() )
+    {
+        AMD_IOMMU_DEBUG("no device found, something must be very wrong!\n");
+        return;
+    }
+
+    /*
+     * No matter from where the interrupt came from, check all the
+     * IOMMUs present in the system. This allows for having just one
+     * tasklet (instead of one per each IOMMUs).
+     */
+    for_each_amd_iommu ( iommu ) {
+        iommu_check_event_log(iommu);
+
+        if ( iommu->ppr_log.buffer != NULL )
+            iommu_check_ppr_log(iommu);
+    }
+}
+
 static void iommu_interrupt_handler(int irq, void *dev_id,
                                     struct cpu_user_regs *regs)
 {
+    u32 entry;
+    unsigned long flags;
     struct amd_iommu *iommu = dev_id;
-    iommu_check_event_log(iommu);
 
-    if ( iommu->ppr_log.buffer != NULL )
-        iommu_check_ppr_log(iommu);
+    spin_lock_irqsave(&iommu->lock, flags);
+
+    /* Silence interrupts from both event and PPR logging */
+    entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
+    iommu_clear_bit(&entry, IOMMU_STATUS_EVENT_LOG_INT_SHIFT);
+    iommu_clear_bit(&entry, IOMMU_STATUS_PPR_LOG_INT_SHIFT);
+    writel(entry, iommu->mmio_base+IOMMU_STATUS_MMIO_OFFSET);
+
+    spin_unlock_irqrestore(&iommu->lock, flags);
+
+    /* It is the tasklet that will clear the logs and re-enable interrupts */
+    tasklet_schedule(&amd_iommu_irq_tasklet);
 }
 
 static int __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
@@ -876,6 +912,8 @@ static int __init amd_iommu_init_one(str
     printk("AMD-Vi: IOMMU %d Enabled.\n", nr_amd_iommus );
     nr_amd_iommus++;
 
+    softirq_tasklet_init(&amd_iommu_irq_tasklet, do_amd_iommu_irq, 0);
+
     return 0;
 
 error_out:


-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)


[-- Attachment #1.1.2: iommu-fault-tasklet_amd.patch --]
[-- Type: text/x-patch, Size: 3058 bytes --]

# HG changeset patch
# Parent 15ab61865ecbd146f6ce65fbea5bf49bfd9c6cb1
Move IOMMU faults handling into softirq for AMD-Vi.
                  
Dealing with interrupts from AMD-Vi IOMMU(s) is deferred to a softirq-tasklet,
raised by the actual IRQ handler. To avoid more interrupts being generated
(because of further faults), they must be masked in the IOMMU within the low
level IRQ handler and enabled back in the tasklet body. Notice that this may
cause the log to overflow, but none of the existing entry will be overwritten.
                                 
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

diff -r 15ab61865ecb xen/drivers/passthrough/amd/iommu_init.c
--- a/xen/drivers/passthrough/amd/iommu_init.c	Tue Jan 17 12:40:52 2012 +0000
+++ b/xen/drivers/passthrough/amd/iommu_init.c	Wed Jan 18 13:01:23 2012 +0100
@@ -32,6 +32,8 @@
 
 static int __initdata nr_amd_iommus;
 
+static struct tasklet amd_iommu_irq_tasklet;
+
 unsigned short ivrs_bdf_entries;
 static struct radix_tree_root ivrs_maps;
 struct list_head amd_iommu_head;
@@ -689,14 +691,48 @@ static void iommu_check_ppr_log(struct a
     spin_unlock_irqrestore(&iommu->lock, flags);
 }
 
+static void do_amd_iommu_irq(unsigned long data)
+{
+    struct amd_iommu *iommu;
+
+    if ( !iommu_found() )
+    {
+        AMD_IOMMU_DEBUG("no device found, something must be very wrong!\n");
+        return;
+    }
+
+    /*
+     * No matter from where the interrupt came from, check all the
+     * IOMMUs present in the system. This allows for having just one
+     * tasklet (instead of one per each IOMMUs).
+     */
+    for_each_amd_iommu ( iommu ) {
+        iommu_check_event_log(iommu);
+
+        if ( iommu->ppr_log.buffer != NULL )
+            iommu_check_ppr_log(iommu);
+    }
+}
+
 static void iommu_interrupt_handler(int irq, void *dev_id,
                                     struct cpu_user_regs *regs)
 {
+    u32 entry;
+    unsigned long flags;
     struct amd_iommu *iommu = dev_id;
-    iommu_check_event_log(iommu);
 
-    if ( iommu->ppr_log.buffer != NULL )
-        iommu_check_ppr_log(iommu);
+    spin_lock_irqsave(&iommu->lock, flags);
+
+    /* Silence interrupts from both event and PPR logging */
+    entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
+    iommu_clear_bit(&entry, IOMMU_STATUS_EVENT_LOG_INT_SHIFT);
+    iommu_clear_bit(&entry, IOMMU_STATUS_PPR_LOG_INT_SHIFT);
+    writel(entry, iommu->mmio_base+IOMMU_STATUS_MMIO_OFFSET);
+
+    spin_unlock_irqrestore(&iommu->lock, flags);
+
+    /* It is the tasklet that will clear the logs and re-enable interrupts */
+    tasklet_schedule(&amd_iommu_irq_tasklet);
 }
 
 static int __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
@@ -876,6 +912,8 @@ static int __init amd_iommu_init_one(str
     printk("AMD-Vi: IOMMU %d Enabled.\n", nr_amd_iommus );
     nr_amd_iommus++;
 
+    softirq_tasklet_init(&amd_iommu_irq_tasklet, do_amd_iommu_irq, 0);
+
     return 0;
 
 error_out:

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

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

Re: [PATCHv3] Move IOMMU faults handling into softirq for AMD-Vi.

[Wei Wang]

On 01/18/2012 02:51 PM, Dario Faggioli wrote:
> Hello Wei,
>
> Here it is, and it seems to work for me, but of course I'm not testing
> PPR. If you could give this a go and let me know... I'll repost in a
> separate thread if it happens to be fine.

Hi,
It works on my machine with PPR log enabled.
Thanks,
Wei

Tested-by: Wei Wang <wei.wang2@amd.com>

> Thanks again,
> Dario
>
> --
>
> Move IOMMU faults handling into softirq for AMD-Vi.
>
> Dealing with interrupts from AMD-Vi IOMMU(s) is deferred to a softirq-tasklet,
> raised by the actual IRQ handler. To avoid more interrupts being generated
> (because of further faults), they must be masked in the IOMMU within the low
> level IRQ handler and enabled back in the tasklet body. Notice that this may
> cause the log to overflow, but none of the existing entry will be overwritten.
>
> Signed-off-by: Dario Faggioli<dario.faggioli@citrix.com>
>
> diff -r 15ab61865ecb xen/drivers/passthrough/amd/iommu_init.c
> --- a/xen/drivers/passthrough/amd/iommu_init.c	Tue Jan 17 12:40:52 2012 +0000
> +++ b/xen/drivers/passthrough/amd/iommu_init.c	Wed Jan 18 13:01:23 2012 +0100
> @@ -32,6 +32,8 @@
>
>   static int __initdata nr_amd_iommus;
>
> +static struct tasklet amd_iommu_irq_tasklet;
> +
>   unsigned short ivrs_bdf_entries;
>   static struct radix_tree_root ivrs_maps;
>   struct list_head amd_iommu_head;
> @@ -689,14 +691,48 @@ static void iommu_check_ppr_log(struct a
>       spin_unlock_irqrestore(&iommu->lock, flags);
>   }
>
> +static void do_amd_iommu_irq(unsigned long data)
> +{
> +    struct amd_iommu *iommu;
> +
> +    if ( !iommu_found() )
> +    {
> +        AMD_IOMMU_DEBUG("no device found, something must be very wrong!\n");
> +        return;
> +    }
> +
> +    /*
> +     * No matter from where the interrupt came from, check all the
> +     * IOMMUs present in the system. This allows for having just one
> +     * tasklet (instead of one per each IOMMUs).
> +     */
> +    for_each_amd_iommu ( iommu ) {
> +        iommu_check_event_log(iommu);
> +
> +        if ( iommu->ppr_log.buffer != NULL )
> +            iommu_check_ppr_log(iommu);
> +    }
> +}
> +
>   static void iommu_interrupt_handler(int irq, void *dev_id,
>                                       struct cpu_user_regs *regs)
>   {
> +    u32 entry;
> +    unsigned long flags;
>       struct amd_iommu *iommu = dev_id;
> -    iommu_check_event_log(iommu);
>
> -    if ( iommu->ppr_log.buffer != NULL )
> -        iommu_check_ppr_log(iommu);
> +    spin_lock_irqsave(&iommu->lock, flags);
> +
> +    /* Silence interrupts from both event and PPR logging */
> +    entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
> +    iommu_clear_bit(&entry, IOMMU_STATUS_EVENT_LOG_INT_SHIFT);
> +    iommu_clear_bit(&entry, IOMMU_STATUS_PPR_LOG_INT_SHIFT);
> +    writel(entry, iommu->mmio_base+IOMMU_STATUS_MMIO_OFFSET);
> +
> +    spin_unlock_irqrestore(&iommu->lock, flags);
> +
> +    /* It is the tasklet that will clear the logs and re-enable interrupts */
> +    tasklet_schedule(&amd_iommu_irq_tasklet);
>   }
>
>   static int __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
> @@ -876,6 +912,8 @@ static int __init amd_iommu_init_one(str
>       printk("AMD-Vi: IOMMU %d Enabled.\n", nr_amd_iommus );
>       nr_amd_iommus++;
>
> +    softirq_tasklet_init(&amd_iommu_irq_tasklet, do_amd_iommu_irq, 0);
> +
>       return 0;
>
>   error_out:
>
>

Re: [PATCHv3] Move IOMMU faults handling into softirq for AMD-Vi.

[Dario Faggioli]

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

On Wed, 2012-01-18 at 16:53 +0100, Wei Wang wrote:
> Hi,
> It works on my machine with PPR log enabled.
>
Ok then, patch resent in a separate thread right now.

Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

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