Re: [PATCH v3 5/5] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

Re: [PATCH v3 5/5] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Michael S. Tsirkin]

On Wed, Nov 03, 2010 at 09:51:10AM +0100, Jan Kiszka wrote:
> Am 03.11.2010 09:43, Michael S. Tsirkin wrote:
> > On Wed, Nov 03, 2010 at 09:11:16AM +0100, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> PCI 2.3 allows to generically disable IRQ sources at device level. This
> >> enables us to share IRQs of such devices between on the host side when
> >> passing them to a guest. This feature is optional, user space has to
> >> request it explicitly.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > 
> > I just realized something.
> > With this patch, if guest ever looks at
> > interrupt disable bit, it will go crazy as that bit goes on/off by
> > itself. I guess we could have an ioctl to set/clear the bit on
> > device, and have qemu call that on config write into command/status
> > register.
> 
> I understand the problem, but I don't get why the kernel should bother.
> User space has to filter the config space access, returning precisely
> the value of the INTx disabled bit that the guest wrote.

Yes but if guest disables INTx it should not get interrupts :)

> > 
> > There's also something I don't completely unerstand with current code:
> > how does interrupt sharing work? E.g. can assigned and emulated
> > devices share an interrupt?
> 
> You mean on the guest IRQ line (host-side sharing is obviously fine)?
> Don't know, but that wouldn't be a new issue. Need to study the sources
> /wrt IRQ line arbitration and concurrent use.
> 
> Jan
> 

Re: [PATCH v3 5/5] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Michael S. Tsirkin]

On Wed, Nov 03, 2010 at 09:56:24AM +0100, Jan Kiszka wrote:
> > Hmm, this does an extra config read on each interrupt (another one is in
> > pci_2_3_irq_unmask).  These reads are pretty expensive... I do realize
> > locking becomes ugly, though. Maybe my idea to avoid set level to 0
> > was silly? Thoughts?
> 
> Well, reading twice is the price to pay here, putting kvm_set_irq under
> spin_lock_irq again is a no-go. From that POV, the previous version was
> probably the cheapest: no extra efforts in the common case, but still
> avoiding reassertion via the host IRQ handler whenever possible.
> 
> Jan
> 

Sigh. I guess so. Any chance of a benchmark to let us figure this out?

-- 
MST

Re: [PATCH v3 5/5] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Jan Kiszka]

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

Am 03.11.2010 10:05, Michael S. Tsirkin wrote:
> On Wed, Nov 03, 2010 at 09:51:10AM +0100, Jan Kiszka wrote:
>> Am 03.11.2010 09:43, Michael S. Tsirkin wrote:
>>> On Wed, Nov 03, 2010 at 09:11:16AM +0100, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> PCI 2.3 allows to generically disable IRQ sources at device level. This
>>>> enables us to share IRQs of such devices between on the host side when
>>>> passing them to a guest. This feature is optional, user space has to
>>>> request it explicitly.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>>
>>> I just realized something.
>>> With this patch, if guest ever looks at
>>> interrupt disable bit, it will go crazy as that bit goes on/off by
>>> itself. I guess we could have an ioctl to set/clear the bit on
>>> device, and have qemu call that on config write into command/status
>>> register.
>>
>> I understand the problem, but I don't get why the kernel should bother.
>> User space has to filter the config space access, returning precisely
>> the value of the INTx disabled bit that the guest wrote.
> 
> Yes but if guest disables INTx it should not get interrupts :)

Right, got this meanwhile. KVM-in-KVM with nested device assignment
would break otherwise - intolerable. :)

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH v3 5/5] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Jan Kiszka]

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

Am 03.11.2010 10:10, Michael S. Tsirkin wrote:
> On Wed, Nov 03, 2010 at 09:56:24AM +0100, Jan Kiszka wrote:
>>> Hmm, this does an extra config read on each interrupt (another one is in
>>> pci_2_3_irq_unmask).  These reads are pretty expensive... I do realize
>>> locking becomes ugly, though. Maybe my idea to avoid set level to 0
>>> was silly? Thoughts?
>>
>> Well, reading twice is the price to pay here, putting kvm_set_irq under
>> spin_lock_irq again is a no-go. From that POV, the previous version was
>> probably the cheapest: no extra efforts in the common case, but still
>> avoiding reassertion via the host IRQ handler whenever possible.
>>
>> Jan
>>
> 
> Sigh. I guess so. Any chance of a benchmark to let us figure this out?
> 

Will see what I can do. 2.6.37 claims to have improved IRQ load
accounting, maybe that helps to measure something useful.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH v3 5/5] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Michael S. Tsirkin]

On Wed, Nov 03, 2010 at 10:15:59AM +0100, Jan Kiszka wrote:
> Am 03.11.2010 10:05, Michael S. Tsirkin wrote:
> > On Wed, Nov 03, 2010 at 09:51:10AM +0100, Jan Kiszka wrote:
> >> Am 03.11.2010 09:43, Michael S. Tsirkin wrote:
> >>> On Wed, Nov 03, 2010 at 09:11:16AM +0100, Jan Kiszka wrote:
> >>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>
> >>>> PCI 2.3 allows to generically disable IRQ sources at device level. This
> >>>> enables us to share IRQs of such devices between on the host side when
> >>>> passing them to a guest. This feature is optional, user space has to
> >>>> request it explicitly.
> >>>>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>
> >>>
> >>> I just realized something.
> >>> With this patch, if guest ever looks at
> >>> interrupt disable bit, it will go crazy as that bit goes on/off by
> >>> itself. I guess we could have an ioctl to set/clear the bit on
> >>> device, and have qemu call that on config write into command/status
> >>> register.
> >>
> >> I understand the problem, but I don't get why the kernel should bother.
> >> User space has to filter the config space access, returning precisely
> >> the value of the INTx disabled bit that the guest wrote.
> > 
> > Yes but if guest disables INTx it should not get interrupts :)
> 
> Right, got this meanwhile. KVM-in-KVM with nested device assignment
> would break otherwise - intolerable. :)
> 
> Jan
> 

Or something simpler like WHQL :)

-- 
MST

Re: [PATCH v3 5/5] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Alex Williamson]

On Wed, 2010-11-03 at 10:43 +0200, Michael S. Tsirkin wrote:
> On Wed, Nov 03, 2010 at 09:11:16AM +0100, Jan Kiszka wrote:
> > From: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > PCI 2.3 allows to generically disable IRQ sources at device level. This
> > enables us to share IRQs of such devices between on the host side when
> > passing them to a guest. This feature is optional, user space has to
> > request it explicitly.
> > 
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> 
> I just realized something.
> With this patch, if guest ever looks at
> interrupt disable bit, it will go crazy as that bit goes on/off by
> itself. I guess we could have an ioctl to set/clear the bit on
> device, and have qemu call that on config write into command/status
> register.
> 
> There's also something I don't completely unerstand with current code:
> how does interrupt sharing work? E.g. can assigned and emulated
> devices share an interrupt?

I've been pondering this with VFIO too.  There it seems to work, even
when I enable irqfd.  The VFIO kernel/qemu driver needs to filter EOIs
based on whether the interrupt was actually asserted by the device, but
I think we're likely relying somewhat on interrupts being reasserted to
help us keep everything serviced.

Alex

Re: [PATCH v3 5/5] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Jan Kiszka]

Am 03.11.2010 16:57, Alex Williamson wrote:
> On Wed, 2010-11-03 at 10:43 +0200, Michael S. Tsirkin wrote:
>> On Wed, Nov 03, 2010 at 09:11:16AM +0100, Jan Kiszka wrote:
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> PCI 2.3 allows to generically disable IRQ sources at device level. This
>>> enables us to share IRQs of such devices between on the host side when
>>> passing them to a guest. This feature is optional, user space has to
>>> request it explicitly.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>
>>
>> I just realized something.
>> With this patch, if guest ever looks at
>> interrupt disable bit, it will go crazy as that bit goes on/off by
>> itself. I guess we could have an ioctl to set/clear the bit on
>> device, and have qemu call that on config write into command/status
>> register.
>>
>> There's also something I don't completely unerstand with current code:
>> how does interrupt sharing work? E.g. can assigned and emulated
>> devices share an interrupt?
> 
> I've been pondering this with VFIO too.  There it seems to work, even
> when I enable irqfd.  The VFIO kernel/qemu driver needs to filter EOIs
> based on whether the interrupt was actually asserted by the device, but
> I think we're likely relying somewhat on interrupts being reasserted to
> help us keep everything serviced.

I don't think this filtering exists. The ack notifier that is fired on
EOI matches the GSI, hitting anyone who is registered.

I think the problem is that, while user space properly or's the input of
all PCI devices on a IRQ line (e.g. in piix3_set_irq), kernel-side users
apparently prefer to mess directly with the irqchip. Unless I'm missing
something, that is long broken.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH v3 5/5] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Jan Kiszka]

Am 03.11.2010 18:27, Jan Kiszka wrote:
> Am 03.11.2010 16:57, Alex Williamson wrote:
>> On Wed, 2010-11-03 at 10:43 +0200, Michael S. Tsirkin wrote:
>>> On Wed, Nov 03, 2010 at 09:11:16AM +0100, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> PCI 2.3 allows to generically disable IRQ sources at device level. This
>>>> enables us to share IRQs of such devices between on the host side when
>>>> passing them to a guest. This feature is optional, user space has to
>>>> request it explicitly.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>>
>>> I just realized something.
>>> With this patch, if guest ever looks at
>>> interrupt disable bit, it will go crazy as that bit goes on/off by
>>> itself. I guess we could have an ioctl to set/clear the bit on
>>> device, and have qemu call that on config write into command/status
>>> register.
>>>
>>> There's also something I don't completely unerstand with current code:
>>> how does interrupt sharing work? E.g. can assigned and emulated
>>> devices share an interrupt?
>>
>> I've been pondering this with VFIO too.  There it seems to work, even
>> when I enable irqfd.  The VFIO kernel/qemu driver needs to filter EOIs
>> based on whether the interrupt was actually asserted by the device, but
>> I think we're likely relying somewhat on interrupts being reasserted to
>> help us keep everything serviced.
> 
> I don't think this filtering exists. The ack notifier that is fired on
> EOI matches the GSI, hitting anyone who is registered.
> 
> I think the problem is that, while user space properly or's the input of
> all PCI devices on a IRQ line (e.g. in piix3_set_irq), kernel-side users
> apparently prefer to mess directly with the irqchip. Unless I'm missing
> something, that is long broken.

No, it's actually or'ed in the kernel as well: kvm_irq_line_state. So,
any issue remaining?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH v3 5/5] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Michael S. Tsirkin]

On Wed, Nov 03, 2010 at 06:27:50PM +0100, Jan Kiszka wrote:
> Am 03.11.2010 16:57, Alex Williamson wrote:
> > On Wed, 2010-11-03 at 10:43 +0200, Michael S. Tsirkin wrote:
> >> On Wed, Nov 03, 2010 at 09:11:16AM +0100, Jan Kiszka wrote:
> >>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>
> >>> PCI 2.3 allows to generically disable IRQ sources at device level. This
> >>> enables us to share IRQs of such devices between on the host side when
> >>> passing them to a guest. This feature is optional, user space has to
> >>> request it explicitly.
> >>>
> >>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >>
> >> I just realized something.
> >> With this patch, if guest ever looks at
> >> interrupt disable bit, it will go crazy as that bit goes on/off by
> >> itself. I guess we could have an ioctl to set/clear the bit on
> >> device, and have qemu call that on config write into command/status
> >> register.
> >>
> >> There's also something I don't completely unerstand with current code:
> >> how does interrupt sharing work? E.g. can assigned and emulated
> >> devices share an interrupt?
> > 
> > I've been pondering this with VFIO too.  There it seems to work, even
> > when I enable irqfd.  The VFIO kernel/qemu driver needs to filter EOIs
> > based on whether the interrupt was actually asserted by the device, but
> > I think we're likely relying somewhat on interrupts being reasserted to
> > help us keep everything serviced.
> 
> I don't think this filtering exists. The ack notifier that is fired on
> EOI matches the GSI, hitting anyone who is registered.
> I think the problem is that, while user space properly or's the input of
> all PCI devices on a IRQ line (e.g. in piix3_set_irq), kernel-side users
> apparently prefer to mess directly with the irqchip. Unless I'm missing
> something, that is long broken.
> 
> Jan

Well we do check host_irq_disabled. I think this is why it's there.

> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

Re: [PATCH v3 5/5] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Michael S. Tsirkin]

On Wed, Nov 03, 2010 at 06:35:48PM +0100, Jan Kiszka wrote:
> Am 03.11.2010 18:27, Jan Kiszka wrote:
> > Am 03.11.2010 16:57, Alex Williamson wrote:
> >> On Wed, 2010-11-03 at 10:43 +0200, Michael S. Tsirkin wrote:
> >>> On Wed, Nov 03, 2010 at 09:11:16AM +0100, Jan Kiszka wrote:
> >>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>
> >>>> PCI 2.3 allows to generically disable IRQ sources at device level. This
> >>>> enables us to share IRQs of such devices between on the host side when
> >>>> passing them to a guest. This feature is optional, user space has to
> >>>> request it explicitly.
> >>>>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>
> >>>
> >>> I just realized something.
> >>> With this patch, if guest ever looks at
> >>> interrupt disable bit, it will go crazy as that bit goes on/off by
> >>> itself. I guess we could have an ioctl to set/clear the bit on
> >>> device, and have qemu call that on config write into command/status
> >>> register.
> >>>
> >>> There's also something I don't completely unerstand with current code:
> >>> how does interrupt sharing work? E.g. can assigned and emulated
> >>> devices share an interrupt?
> >>
> >> I've been pondering this with VFIO too.  There it seems to work, even
> >> when I enable irqfd.  The VFIO kernel/qemu driver needs to filter EOIs
> >> based on whether the interrupt was actually asserted by the device, but
> >> I think we're likely relying somewhat on interrupts being reasserted to
> >> help us keep everything serviced.
> > 
> > I don't think this filtering exists. The ack notifier that is fired on
> > EOI matches the GSI, hitting anyone who is registered.
> > 
> > I think the problem is that, while user space properly or's the input of
> > all PCI devices on a IRQ line (e.g. in piix3_set_irq), kernel-side users
> > apparently prefer to mess directly with the irqchip. Unless I'm missing
> > something, that is long broken.
> 
> No, it's actually or'ed in the kernel as well: kvm_irq_line_state. So,
> any issue remaining?

No issue. Assigned devices have a different source id, this is the trick
I forgot about.

Current VFIO patches go through userspace for level IRQs so
it's simple, to make VFIO go directly through kernel we'll need
to get a source id for each VFIO device, and associate it with
eventfds somehow.


> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

Re: [PATCH v3 5/5] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Jan Kiszka]

Am 03.11.2010 18:37, Michael S. Tsirkin wrote:
> On Wed, Nov 03, 2010 at 06:27:50PM +0100, Jan Kiszka wrote:
>> Am 03.11.2010 16:57, Alex Williamson wrote:
>>> On Wed, 2010-11-03 at 10:43 +0200, Michael S. Tsirkin wrote:
>>>> On Wed, Nov 03, 2010 at 09:11:16AM +0100, Jan Kiszka wrote:
>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>> PCI 2.3 allows to generically disable IRQ sources at device level. This
>>>>> enables us to share IRQs of such devices between on the host side when
>>>>> passing them to a guest. This feature is optional, user space has to
>>>>> request it explicitly.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>>
>>>> I just realized something.
>>>> With this patch, if guest ever looks at
>>>> interrupt disable bit, it will go crazy as that bit goes on/off by
>>>> itself. I guess we could have an ioctl to set/clear the bit on
>>>> device, and have qemu call that on config write into command/status
>>>> register.
>>>>
>>>> There's also something I don't completely unerstand with current code:
>>>> how does interrupt sharing work? E.g. can assigned and emulated
>>>> devices share an interrupt?
>>>
>>> I've been pondering this with VFIO too.  There it seems to work, even
>>> when I enable irqfd.  The VFIO kernel/qemu driver needs to filter EOIs
>>> based on whether the interrupt was actually asserted by the device, but
>>> I think we're likely relying somewhat on interrupts being reasserted to
>>> help us keep everything serviced.
>>
>> I don't think this filtering exists. The ack notifier that is fired on
>> EOI matches the GSI, hitting anyone who is registered.
>> I think the problem is that, while user space properly or's the input of
>> all PCI devices on a IRQ line (e.g. in piix3_set_irq), kernel-side users
>> apparently prefer to mess directly with the irqchip. Unless I'm missing
>> something, that is long broken.
>>
>> Jan
> 
> Well we do check host_irq_disabled. I think this is why it's there.

Probably. As an optimization. Once we force user space to inform the
kernel about INTx masking, we can restore this also for PCI 2.3.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH v3 2/5] KVM: Switch assigned device IRQ forwarding to threaded handler

[Marcelo Tosatti]

On Wed, Nov 03, 2010 at 09:11:13AM +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> This improves the IRQ forwarding for assigned devices: By using the
> kernel's threaded IRQ scheme, we can get rid of the latency-prone work
> queue and simplify the code in the same run.
> 
> Moreover, we no longer have to hold assigned_dev_lock while raising the
> guest IRQ, which can be a lenghty operation as we may have to iterate
> over all VCPUs. The lock is now only used for synchronizing masking vs.
> unmasking of INTx-type IRQs, thus is renames to intx_lock.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  include/linux/kvm_host.h |   12 +----
>  virt/kvm/assigned-dev.c  |  106 ++++++++++++++-------------------------------
>  2 files changed, 35 insertions(+), 83 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index bcf71c7..eaacb5d 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -457,16 +457,8 @@ struct kvm_irq_ack_notifier {
>  	void (*irq_acked)(struct kvm_irq_ack_notifier *kian);
>  };
>  
> -#define KVM_ASSIGNED_MSIX_PENDING		0x1
> -struct kvm_guest_msix_entry {
> -	u32 vector;
> -	u16 entry;
> -	u16 flags;
> -};
> -
>  struct kvm_assigned_dev_kernel {
>  	struct kvm_irq_ack_notifier ack_notifier;
> -	struct work_struct interrupt_work;
>  	struct list_head list;
>  	int assigned_dev_id;
>  	int host_segnr;
> @@ -477,13 +469,13 @@ struct kvm_assigned_dev_kernel {
>  	bool host_irq_disabled;
>  	struct msix_entry *host_msix_entries;
>  	int guest_irq;
> -	struct kvm_guest_msix_entry *guest_msix_entries;
> +	struct msix_entry *guest_msix_entries;
>  	unsigned long irq_requested_type;
>  	int irq_source_id;
>  	int flags;
>  	struct pci_dev *dev;
>  	struct kvm *kvm;
> -	spinlock_t assigned_dev_lock;
> +	spinlock_t intx_lock;
>  };
>  
>  struct kvm_irq_mask_notifier {
> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> index ecc4419..d0b07ea 100644
> --- a/virt/kvm/assigned-dev.c
> +++ b/virt/kvm/assigned-dev.c
> @@ -55,58 +55,31 @@ static int find_index_from_host_irq(struct kvm_assigned_dev_kernel
>  	return index;
>  }
>  
> -static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
> +static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id)
>  {
> -	struct kvm_assigned_dev_kernel *assigned_dev;
> -	int i;
> +	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
> +	u32 vector;
> +	int index;
>  
> -	assigned_dev = container_of(work, struct kvm_assigned_dev_kernel,
> -				    interrupt_work);
> +	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_INTX) {
> +		spin_lock(&assigned_dev->intx_lock);
> +		disable_irq_nosync(irq);
> +		assigned_dev->host_irq_disabled = true;
> +		spin_unlock(&assigned_dev->intx_lock);
> +	}

Shouldnt this happen in the hardirq handler? Otherwise host will receive
interrupts between ->eoi and this point.

Re: [PATCH v3 2/5] KVM: Switch assigned device IRQ forwarding to threaded handler

[Jan Kiszka]

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

Am 03.11.2010 23:13, Marcelo Tosatti wrote:
> On Wed, Nov 03, 2010 at 09:11:13AM +0100, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> This improves the IRQ forwarding for assigned devices: By using the
>> kernel's threaded IRQ scheme, we can get rid of the latency-prone work
>> queue and simplify the code in the same run.
>>
>> Moreover, we no longer have to hold assigned_dev_lock while raising the
>> guest IRQ, which can be a lenghty operation as we may have to iterate
>> over all VCPUs. The lock is now only used for synchronizing masking vs.
>> unmasking of INTx-type IRQs, thus is renames to intx_lock.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  include/linux/kvm_host.h |   12 +----
>>  virt/kvm/assigned-dev.c  |  106 ++++++++++++++-------------------------------
>>  2 files changed, 35 insertions(+), 83 deletions(-)
>>
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index bcf71c7..eaacb5d 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -457,16 +457,8 @@ struct kvm_irq_ack_notifier {
>>  	void (*irq_acked)(struct kvm_irq_ack_notifier *kian);
>>  };
>>  
>> -#define KVM_ASSIGNED_MSIX_PENDING		0x1
>> -struct kvm_guest_msix_entry {
>> -	u32 vector;
>> -	u16 entry;
>> -	u16 flags;
>> -};
>> -
>>  struct kvm_assigned_dev_kernel {
>>  	struct kvm_irq_ack_notifier ack_notifier;
>> -	struct work_struct interrupt_work;
>>  	struct list_head list;
>>  	int assigned_dev_id;
>>  	int host_segnr;
>> @@ -477,13 +469,13 @@ struct kvm_assigned_dev_kernel {
>>  	bool host_irq_disabled;
>>  	struct msix_entry *host_msix_entries;
>>  	int guest_irq;
>> -	struct kvm_guest_msix_entry *guest_msix_entries;
>> +	struct msix_entry *guest_msix_entries;
>>  	unsigned long irq_requested_type;
>>  	int irq_source_id;
>>  	int flags;
>>  	struct pci_dev *dev;
>>  	struct kvm *kvm;
>> -	spinlock_t assigned_dev_lock;
>> +	spinlock_t intx_lock;
>>  };
>>  
>>  struct kvm_irq_mask_notifier {
>> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
>> index ecc4419..d0b07ea 100644
>> --- a/virt/kvm/assigned-dev.c
>> +++ b/virt/kvm/assigned-dev.c
>> @@ -55,58 +55,31 @@ static int find_index_from_host_irq(struct kvm_assigned_dev_kernel
>>  	return index;
>>  }
>>  
>> -static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
>> +static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id)
>>  {
>> -	struct kvm_assigned_dev_kernel *assigned_dev;
>> -	int i;
>> +	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
>> +	u32 vector;
>> +	int index;
>>  
>> -	assigned_dev = container_of(work, struct kvm_assigned_dev_kernel,
>> -				    interrupt_work);
>> +	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_INTX) {
>> +		spin_lock(&assigned_dev->intx_lock);
>> +		disable_irq_nosync(irq);
>> +		assigned_dev->host_irq_disabled = true;
>> +		spin_unlock(&assigned_dev->intx_lock);
>> +	}
> 
> Shouldnt this happen in the hardirq handler? Otherwise host will receive
> interrupts between ->eoi and this point.
> 

Yeah, there is indeed still a bug. I thought the kernel would
automatically keep the line masked until the threaded handler returned,
but that's only the case if we set IRQF_ONESHOT. Will fix in v4.

Thanks,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]