Mailing List Archive

[PATCHv2 2 of 2] 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:

--
<<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)
Re: [PATCHv2 2 of 2] Move IOMMU faults handling into softirq for AMD-Vi. [ In reply to ]
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:



_______________________________________________
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. [ In reply to ]
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)
Re: [PATCHv2 2 of 2] Move IOMMU faults handling into softirq for AMD-Vi. [ In reply to ]
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
>



_______________________________________________
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. [ In reply to ]
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)