pci_get_dev_by_id() from interrupt handlers

pci_get_dev_by_id() from interrupt handlers

[Joerg Roedel]

Hi Jesse,

I have a question regarding the warning in the pci_get_dev_by_id()
[search.c] function which triggers when called from interrupt context.
As far as I have seen this function should be save to be called in
atomic mode. Is there any other reason it should not be called in
interrupt handlers? I need to (indirectly) call this function for my
IOMMU driver to handle events from the IOMMU.

Thanks,
	Joerg

Re: pci_get_dev_by_id() from interrupt handlers

[Greg KH]

On Tue, Apr 20, 2010 at 06:04:24PM +0200, Joerg Roedel wrote:
> Hi Jesse,
> 
> I have a question regarding the warning in the pci_get_dev_by_id()
> [search.c] function which triggers when called from interrupt context.
> As far as I have seen this function should be save to be called in
> atomic mode.

Are you sure?

> Is there any other reason it should not be called in interrupt
> handlers?

klist traversal is not safe to be done in interrupt context as the
spinlock is not told to be interrupt safe.  Now you could go and change
the klist core to be interrupt safe, but you should probably rethink
your need here first.

> I need to (indirectly) call this function for my IOMMU driver to
> handle events from the IOMMU.

>From interrupt?  Why not use an interrupt thread instead if you really
need to do this.

Actually, what are you trying to accomplish here?

thanks,

greg k-h

Re: pci_get_dev_by_id() from interrupt handlers

[Joerg Roedel]

On Tue, Apr 20, 2010 at 09:32:15AM -0700, Greg KH wrote:
> On Tue, Apr 20, 2010 at 06:04:24PM +0200, Joerg Roedel wrote:
> > Hi Jesse,
> > 
> > I have a question regarding the warning in the pci_get_dev_by_id()
> > [search.c] function which triggers when called from interrupt context.
> > As far as I have seen this function should be save to be called in
> > atomic mode.
> 
> Are you sure?

Not anymore. You proved me wrong ;-)

> > Is there any other reason it should not be called in interrupt
> > handlers?
> 
> klist traversal is not safe to be done in interrupt context as the
> spinlock is not told to be interrupt safe.  Now you could go and change
> the klist core to be interrupt safe, but you should probably rethink
> your need here first.

The idea was to capture IO page faults in the IOMMU and propagate them
to KVM instead of just printk into dmesg. KVM could do something better
then like killing the guest. Currently a malicious guest could flood
host dmesg by causing IO page faults.
For the AMD IOMMU the page faults are reported in an event log and the
cpu is informed by an interrupt about it. The event log entry contains
the bus/dev/function of the device. To get the necessary information to
propagate this to KVM I need to get the 'struct device' for it.
I could certainly do this in a tasklet instead but doing that only for
the task of converting bus/device/function into 'struct device' sounds
a bit complicated to me.

Regards,

	Joerg

Re: pci_get_dev_by_id() from interrupt handlers

[Greg KH]

On Tue, Apr 20, 2010 at 07:35:59PM +0200, Joerg Roedel wrote:
> On Tue, Apr 20, 2010 at 09:32:15AM -0700, Greg KH wrote:
> > On Tue, Apr 20, 2010 at 06:04:24PM +0200, Joerg Roedel wrote:
> > > Is there any other reason it should not be called in interrupt
> > > handlers?
> > 
> > klist traversal is not safe to be done in interrupt context as the
> > spinlock is not told to be interrupt safe.  Now you could go and change
> > the klist core to be interrupt safe, but you should probably rethink
> > your need here first.
> 
> The idea was to capture IO page faults in the IOMMU and propagate them
> to KVM instead of just printk into dmesg. KVM could do something better
> then like killing the guest. Currently a malicious guest could flood
> host dmesg by causing IO page faults.

That's not good.

> For the AMD IOMMU the page faults are reported in an event log and the
> cpu is informed by an interrupt about it. The event log entry contains
> the bus/dev/function of the device. To get the necessary information to
> propagate this to KVM I need to get the 'struct device' for it.

Ok, that's reasonable.

> I could certainly do this in a tasklet instead but doing that only for
> the task of converting bus/device/function into 'struct device' sounds
> a bit complicated to me.

Why not do the whole thing in an interrupt task as the whole thing
sounds like something that shouldn't be done in interrupt context,
right?  Now that we have this type of functionality, we should take
advantage of it :)

Eventually, notifying KVM isn't something that you want to do from
interrupt context anyway, right?

thanks,

greg k-h

Re: pci_get_dev_by_id() from interrupt handlers

[Joerg Roedel]

On Tue, Apr 20, 2010 at 10:52:02AM -0700, Greg KH wrote:
> On Tue, Apr 20, 2010 at 07:35:59PM +0200, Joerg Roedel wrote:

> Why not do the whole thing in an interrupt task as the whole thing
> sounds like something that shouldn't be done in interrupt context,
> right?  Now that we have this type of functionality, we should take
> advantage of it :)

Ok, I think I move the IOMMU interrupt handling to a tasklet.

> Eventually, notifying KVM isn't something that you want to do from
> interrupt context anyway, right?

On the KVM side it is probably nothing more than setting a request bit.
But lets see how this could be done :-)

Thanks,

	Joerg

Re: pci_get_dev_by_id() from interrupt handlers

[Peter Zijlstra]

On Wed, 2010-04-21 at 12:05 +0200, Joerg Roedel wrote:
> 
> > Why not do the whole thing in an interrupt task as the whole thing
> > sounds like something that shouldn't be done in interrupt context,
> > right?  Now that we have this type of functionality, we should take
> > advantage of it :)
> 
> Ok, I think I move the IOMMU interrupt handling to a tasklet.
> 
tasklet is softirq context, and is not what gregkh was talking about.

You'd still need to change the klist spinlock to be softirq-safe.

Re: pci_get_dev_by_id() from interrupt handlers

[Joerg Roedel]

On Wed, Apr 21, 2010 at 12:21:11PM +0200, Peter Zijlstra wrote:
> On Wed, 2010-04-21 at 12:05 +0200, Joerg Roedel wrote:
> > 
> > > Why not do the whole thing in an interrupt task as the whole thing
> > > sounds like something that shouldn't be done in interrupt context,
> > > right?  Now that we have this type of functionality, we should take
> > > advantage of it :)
> > 
> > Ok, I think I move the IOMMU interrupt handling to a tasklet.
> > 
> tasklet is softirq context, and is not what gregkh was talking about.
> 
> You'd still need to change the klist spinlock to be softirq-safe.

Ah right. Thanks for pointing this out. I couldn't find much about the
interrupt tasks. Is it about the request_threaded_irq interface? And
this would be run in the context of a kernel thread, right?

	Joerg

Re: pci_get_dev_by_id() from interrupt handlers

[Peter Zijlstra]

On Wed, 2010-04-21 at 12:41 +0200, Joerg Roedel wrote:
> On Wed, Apr 21, 2010 at 12:21:11PM +0200, Peter Zijlstra wrote:
> > On Wed, 2010-04-21 at 12:05 +0200, Joerg Roedel wrote:
> > > 
> > > > Why not do the whole thing in an interrupt task as the whole thing
> > > > sounds like something that shouldn't be done in interrupt context,
> > > > right?  Now that we have this type of functionality, we should take
> > > > advantage of it :)
> > > 
> > > Ok, I think I move the IOMMU interrupt handling to a tasklet.
> > > 
> > tasklet is softirq context, and is not what gregkh was talking about.
> > 
> > You'd still need to change the klist spinlock to be softirq-safe.
> 
> Ah right. Thanks for pointing this out. I couldn't find much about the
> interrupt tasks. Is it about the request_threaded_irq interface? And
> this would be run in the context of a kernel thread, right?

Right, request_threaded_irq() is what was meant, the comment there does
a good job of describing how to use it.

Re: pci_get_dev_by_id() from interrupt handlers

[Joerg Roedel]

On Wed, Apr 21, 2010 at 12:46:56PM +0200, Peter Zijlstra wrote:
> On Wed, 2010-04-21 at 12:41 +0200, Joerg Roedel wrote:
> > On Wed, Apr 21, 2010 at 12:21:11PM +0200, Peter Zijlstra wrote:
> > > On Wed, 2010-04-21 at 12:05 +0200, Joerg Roedel wrote:
> > > > 
> > > > > Why not do the whole thing in an interrupt task as the whole thing
> > > > > sounds like something that shouldn't be done in interrupt context,
> > > > > right?  Now that we have this type of functionality, we should take
> > > > > advantage of it :)
> > > > 
> > > > Ok, I think I move the IOMMU interrupt handling to a tasklet.
> > > > 
> > > tasklet is softirq context, and is not what gregkh was talking about.
> > > 
> > > You'd still need to change the klist spinlock to be softirq-safe.
> > 
> > Ah right. Thanks for pointing this out. I couldn't find much about the
> > interrupt tasks. Is it about the request_threaded_irq interface? And
> > this would be run in the context of a kernel thread, right?
> 
> Right, request_threaded_irq() is what was meant, the comment there does
> a good job of describing how to use it.

Ok, thanks. I am going to use this interface instead.

	Joerg