Message-ID: <87bns0gzy8.fsf@approximate.cambridge.arm.com>
Date:	Mon, 04 Aug 2014 14:13:35 +0100
From:	Marc Zyngier <marc.zyngier@....com>
To:	Eric Auger <eric.auger@...aro.org>
Cc:	"kvmarm\@lists.cs.columbia.edu" <kvmarm@...ts.cs.columbia.edu>,
	"linux-arm-kernel\@lists.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
	Will Deacon <Will.Deacon@....com>,
	Catalin Marinas <Catalin.Marinas@....com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Christoffer Dall <christoffer.dall@...aro.org>
Subject: Re: [RFC PATCH 7/9] KVM: arm: vgic: allow dynamic mapping of physical/virtual interrupts

On Sun, Aug 03 2014 at 10:48:52 am BST, Eric Auger <eric.auger@...aro.org> wrote:
> On 06/25/2014 11:28 AM, Marc Zyngier wrote:
>> In order to be able to feed physical interrupts to a guest, we need
>> to be able to establish the virtual-physical mapping between the two
>> worlds.
>> 
>> As we try to keep the injection interface simple, find out what the
>> physical interrupt is (if any) when we actually build the LR.
>> 
>> The mapping is kept in a rbtree, indexed by virtual interrupts.
>
> Hi Marc,
>
> I suspect there is a piece missing here related to bitmap state
> management. When using maintenance IRQ, in process_maintenance we cleared
> - dist->irq_pending (and new dist->irq_level)
> - vcpu->irq_queued
>
> Now this does not exist anymore for forwarded irqs, when a subsequent
> IRQ will be injected, vgic_update_irq_pending will fail in injecting the
> IRQ because the states are reflecting the IRQ is still in progress.
>
> Since I have a modified version of your code, using Christoffer patches
> I may have missed some modifications you did but at least on my side I
> was forced to add bitmap clearing.
>
> It is not clear to me where to put that code however. Since user-side
> can inject an IRQ while the previous one is not completed at guest and
> host level, it cannot be in update_irq_pending - or we shall prevent the
> user from injecting fwd IRQs - .

Interesting. Indeed, userspace shouldn't be allowed to inject a
forwarded interrupt (or actually the virtual interrupt that matches the
physical one). This interrupt is now under complete control of the
kernel, and shouldn't triggered by userspace.

Now, it is completely possible that we're missing something here (or
actually doing too much).

> In my case (VFIO/IRQFD), by construction I only inject a new forwarded
> IRQ when the previous one was completed so I could put it in the irqfd
> injection function. But even irqfd is injected through eventfd trigger.
> We shall forbid the user-side to trigger that eventfd in place of the
> VFIO driver. What do you think?

Yup. userspace can't interfere with a forwarded interrupt, that's way
too dangerous.

> A question related to guest kill. Cannot it happen the guest sometimes
> does not complete the vIRQ before exiting? Currently I observe cases
> where when I launch qemu-system after a kill, forwarded irqs do not work
> properly. I am not yet sure this is the cause of my problem but just in
> case, can the host write into GICV_EOIR in place of guest?

It is quite possible that the interrupt is left active when the guest is
killed, which would tend to indicate that we need a way to cleanup
behind us. It should be enough to clear the active bit, shouldn't it?

> Besides those problems, the patch works in my test environment

Thanks for testing!

	M.
-- 
Jazz is not dead. It just smells funny.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists