Re: [PATCH v6 6/8] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs

[Marc Zyngier <marc.zyngier@arm.com>]

On 05/12/17 15:03, Yury Norov wrote:
> On Mon, Dec 04, 2017 at 09:05:04PM +0100, Christoffer Dall wrote:
>> From: Christoffer Dall <christoffer.dall@linaro.org>
>>
>> For mapped IRQs (with the HW bit set in the LR) we have to follow some
>> rules of the architecture.  One of these rules is that VM must not be
>> allowed to deactivate a virtual interrupt with the HW bit set unless the
>> physical interrupt is also active.
>>
>> This works fine when injecting mapped interrupts, because we leave it up
>> to the injector to either set EOImode==1 or manually set the active
>> state of the physical interrupt.
>>
>> However, the guest can set virtual interrupt to be pending or active by
>> writing to the virtual distributor, which could lead to deactivating a
>> virtual interrupt with the HW bit set without the physical interrupt
>> being active.
>>
>> We could set the physical interrupt to active whenever we are about to
>> enter the VM with a HW interrupt either pending or active, but that
>> would be really slow, especially on GICv2.  So we take the long way
>> around and do the hard work when needed, which is expected to be
>> extremely rare.
>>
>> When the VM sets the pending state for a HW interrupt on the virtual
>> distributor we set the active state on the physical distributor, because
>> the virtual interrupt can become active and then the guest can
>> deactivate it.
>>
>> When the VM clears the pending state we also clear it on the physical
>> side, because the injector might otherwise raise the interrupt.  We also
>> clear the physical active state when the virtual interrupt is not
>> active, since otherwise a SPEND/CPEND sequence from the guest would
>> prevent signaling of future interrupts.
>>
>> Changing the state of mapped interrupts from userspace is not supported,
>> and it's expected that userspace unmaps devices from VFIO before
>> attempting to set the interrupt state, because the interrupt state is
>> driven by hardware.
>>
>> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>> ---
>>  virt/kvm/arm/vgic/vgic-mmio.c | 71 +++++++++++++++++++++++++++++++++++++++----
>>  virt/kvm/arm/vgic/vgic.c      |  7 +++++
>>  virt/kvm/arm/vgic/vgic.h      |  1 +
>>  3 files changed, 73 insertions(+), 6 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>> index 747b0a3b4784..8d173d99a7a4 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>> @@ -16,6 +16,7 @@
>>  #include <linux/kvm.h>
>>  #include <linux/kvm_host.h>
>>  #include <kvm/iodev.h>
>> +#include <kvm/arm_arch_timer.h>
>>  #include <kvm/arm_vgic.h>
>>  
>>  #include "vgic.h"
>> @@ -143,10 +144,22 @@ static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
>>  	return vcpu;
>>  }
>>  
>> +/* Must be called with irq->irq_lock held */
> 
> Instead of enforcing this rule in comment, you can enforce it in code:
> 
> BUG_ON(!spin_is_locked(irq->irq_lock))

Are we going to litter the kernel with such assertions? I don't think
that's a valuable thing to do.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...