Re: [PATCH] KVM: arm64: Allow in-atomic injection of SPIs

[Auger Eric <eric.auger@redhat.com>]

Hi Marc,

On 6/8/20 7:19 PM, Marc Zyngier wrote:
> Hi Eric,
> 
> On 2020-06-08 17:58, Auger Eric wrote:
>> Hi Marc,
>>
>> On 5/26/20 6:11 PM, Marc Zyngier wrote:
>>> On a system that uses SPIs to implement MSIs (as it would be
>>> the case on a GICv2 system exposing a GICv2m to its guests),
>>> we deny the possibility of injecting SPIs on the in-atomic
>>> fast-path.
>>>
>>> This results in a very large amount of context-switches
>>> (roughly equivalent to twice the interrupt rate) on the host,
>>> and suboptimal performance for the guest (as measured with
>>> a test workload involving a virtio interface backed by vhost-net).
>>> Given that GICv2 systems are usually on the low-end of the spectrum
>>> performance wise, they could do without the aggravation.
>>>
>>> We solved this for GICv3+ITS by having a translation cache. But
>>> SPIs do not need any extra infrastructure, and can be immediately
>>> injected in the virtual distributor as the locking is already
>>> heavy enough that we don't need to worry about anything.
>>>
>>> This halves the number of context switches for the same workload.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>  arch/arm64/kvm/vgic/vgic-irqfd.c | 20 ++++++++++++++++----
>>>  arch/arm64/kvm/vgic/vgic-its.c   |  3 +--
>>>  2 files changed, 17 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/vgic/vgic-irqfd.c
>>> b/arch/arm64/kvm/vgic/vgic-irqfd.c
>>> index d8cdfea5cc96..11a9f81115ab 100644
>>> --- a/arch/arm64/kvm/vgic/vgic-irqfd.c
>>> +++ b/arch/arm64/kvm/vgic/vgic-irqfd.c
>> There is still a comment above saying
>>  * Currently only direct MSI injection is supported.
> 
> I believe this comment to be correct. There is no path other
> than MSI injection that leads us here. Case in point, we only
> ever inject a rising edge through this API, never a falling one.

Isn't this path entered whenever you have either of the combo being used?
KVM_SET_MSI_ROUTING + KVM_IRQFD
KVM_SET_GSI_ROUTING + KVM_IRQFD

I had in mind direct MSI injection was KVM_SIGNAL_MSI/
kvm_send_userspace_msi/kvm_set_msi
> 
>>> @@ -107,15 +107,27 @@ int kvm_arch_set_irq_inatomic(struct
>>> kvm_kernel_irq_routing_entry *e,
>>>                    struct kvm *kvm, int irq_source_id, int level,
>>>                    bool line_status)
>>>  {
>>> -    if (e->type == KVM_IRQ_ROUTING_MSI && vgic_has_its(kvm) && level) {
>>> +    if (!level)
>>> +        return -EWOULDBLOCK;
>>> +
>>> +    switch (e->type) {
>>> +    case KVM_IRQ_ROUTING_MSI: {
>>>          struct kvm_msi msi;
>>>
>>> +        if (!vgic_has_its(kvm))
>>> +            return -EINVAL;
>> Shouldn't we return -EWOULDBLOCK by default?
>> QEMU does not use that path with GICv2m but in kvm_set_routing_entry() I
>> don't see any check related to the ITS.
> 
> Fair enough. I really don't anticipate anyone to be using
> KVM_IRQ_ROUTING_MSI with anything but the ITS, but who knows,
> people are crazy! ;-)
> 
>>> +
>>>          kvm_populate_msi(e, &msi);
>>> -        if (!vgic_its_inject_cached_translation(kvm, &msi))
>>> -            return 0;
>>> +        return vgic_its_inject_cached_translation(kvm, &msi);
>>
>>>      }
>>>
>>> -    return -EWOULDBLOCK;
>>> +    case KVM_IRQ_ROUTING_IRQCHIP:
>>> +        /* Injecting SPIs is always possible in atomic context */
>>> +        return vgic_irqfd_set_irq(e, kvm, irq_source_id, 1,
>>> line_status);
>> what about the     mutex_lock(&kvm->lock) called from within
>> vgic_irqfd_set_irq/kvm_vgic_inject_irq/vgic_lazy_init
> 
> Holy crap. The lazy GIC init strikes again :-(.
> How about this on top of the existing patch:
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-irqfd.c
> b/arch/arm64/kvm/vgic/vgic-irqfd.c
> index 11a9f81115ab..6e5ca04d5589 100644
> --- a/arch/arm64/kvm/vgic/vgic-irqfd.c
> +++ b/arch/arm64/kvm/vgic/vgic-irqfd.c
> @@ -115,19 +115,23 @@ int kvm_arch_set_irq_inatomic(struct
> kvm_kernel_irq_routing_entry *e,
>          struct kvm_msi msi;
> 
>          if (!vgic_has_its(kvm))
> -            return -EINVAL;
> +            break;
> 
>          kvm_populate_msi(e, &msi);
>          return vgic_its_inject_cached_translation(kvm, &msi);
>      }
> 
>      case KVM_IRQ_ROUTING_IRQCHIP:
> -        /* Injecting SPIs is always possible in atomic context */
> +        /*
> +         * Injecting SPIs is always possible in atomic context
> +         * as long as the damn vgic is initialized.
> +         */
> +        if (unlikely(!vgic_initialized(kvm)))
> +            break;
Yes this should prevent the wait situation. But may be worth to deep
into the call stack. Won't analyzers complain at some point?

Thanks

Eric
>          return vgic_irqfd_set_irq(e, kvm, irq_source_id, 1, line_status);
> -
> -    default:
> -        return -EWOULDBLOCK;
>      }
> +
> +    return -EWOULDBLOCK;
>  }
> 
>  int kvm_vgic_setup_default_irq_routing(struct kvm *kvm)
> 
> 
> Thanks,
> 
>         M.