Re: [RFC PATCH 7/9] KVM: arm: vgic: allow dynamic mapping of physical/virtual interrupts

[Eric Auger <eric.auger@linaro.org>]

On 08/11/2014 10:01 AM, Christoffer Dall wrote:
> On Thu, Aug 07, 2014 at 05:47:53PM +0200, Eric Auger wrote:
>> On 08/04/2014 03:13 PM, Marc Zyngier wrote:
>>> On Sun, Aug 03 2014 at 10:48:52 am BST, Eric Auger <eric.auger@linaro.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 - .
>> Hi Marc,
>>
>> Christoffer suggested me to put state bitmap reset in
>> __kvm_vgic_sync_hwstate where we check whether the LR were consumed. It
>> seems to work fine and we do no assumption about user action.
>>
> 
> What I said specifically was that we currently don't have to worry about
> clearing the active bit or resample the level when we look through the
> ELRSR bitmap, because level-triggered interrupts that have been
> processed currently always raise a maintenance interrupt.
> 
> So my suggestion would be to factor out the "this is some common
> housekeeping we have to do for all level-triggered interrupts which have
> not been completed on a VCPU interface" from vgic_process_maintenance()
> and call this functionality from both vgic_process_maintenance() and
> from __kvm_vgic_sync_hwstate().
> 
> [...]
> 
>>>
>>> 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?
> 
> I'm afraid I'm not quite sure what you mean here?
Hi Christoffer,

you can forget that comment now we decorrelate status bitmap clearing
from injection.
> 
>>>
>>> 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?
>> So in practice this will directly write into the GICC_DIR right? I will
>> try this.
>>
> Not sure what you mean with "this will directly write into the
> GICC_DIR"?  What is 'this' ?
this = clear the active bit. Was looking for the translation into HW
register.

> 
> I haven't experimented a lot with this, but I think you need to catch
> all forwarded IRQs that may have been in flight when the VM was killed
> and use the API to the irqchip to tell it that it's no longer forwarded
> and either the cleanup then sits within the specific irqchip logic or
> you do some more manual cleanup inside KVM.  I would lean towards the
> first option (because the irqchip would always be in charge of
> guaranteeting some queiesced state), but we should look at the specific
> details of what needs to be done.
OK
> 
> -Christoffer
>