Re: [RFC PATCH v1 2/4] KVM: arm64: GICv4.1: Try to save hw pending state in save_pending_tables

[Marc Zyngier <maz@kernel.org>]

On 2020-11-24 07:40, Shenming Lu wrote:
> On 2020/11/23 17:18, Marc Zyngier wrote:
>> On 2020-11-23 06:54, Shenming Lu wrote:
>>> After pausing all vCPUs and devices capable of interrupting, in order
>>         ^^^^^^^^^^^^^^^^^
>> See my comment below about this.
>> 
>>> to save the information of all interrupts, besides flushing the 
>>> pending
>>> states in kvm’s vgic, we also try to flush the states of VLPIs in the
>>> virtual pending tables into guest RAM, but we need to have GICv4.1 
>>> and
>>> safely unmap the vPEs first.
>>> 
>>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>>> ---
>>>  arch/arm64/kvm/vgic/vgic-v3.c | 62 
>>> +++++++++++++++++++++++++++++++----
>>>  1 file changed, 56 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c 
>>> b/arch/arm64/kvm/vgic/vgic-v3.c
>>> index 9cdf39a94a63..e1b3aa4b2b12 100644
>>> --- a/arch/arm64/kvm/vgic/vgic-v3.c
>>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
>>> @@ -1,6 +1,8 @@
>>>  // SPDX-License-Identifier: GPL-2.0-only
>>> 
>>>  #include <linux/irqchip/arm-gic-v3.h>
>>> +#include <linux/irq.h>
>>> +#include <linux/irqdomain.h>
>>>  #include <linux/kvm.h>
>>>  #include <linux/kvm_host.h>
>>>  #include <kvm/arm_vgic.h>
>>> @@ -356,6 +358,39 @@ int vgic_v3_lpi_sync_pending_status(struct kvm
>>> *kvm, struct vgic_irq *irq)
>>>      return 0;
>>>  }
>>> 
>>> +/*
>>> + * With GICv4.1, we can get the VLPI's pending state after unmapping
>>> + * the vPE. The deactivation of the doorbell interrupt will trigger
>>> + * the unmapping of the associated vPE.
>>> + */
>>> +static void get_vlpi_state_pre(struct vgic_dist *dist)
>>> +{
>>> +    struct irq_desc *desc;
>>> +    int i;
>>> +
>>> +    if (!kvm_vgic_global_state.has_gicv4_1)
>>> +        return;
>>> +
>>> +    for (i = 0; i < dist->its_vm.nr_vpes; i++) {
>>> +        desc = irq_to_desc(dist->its_vm.vpes[i]->irq);
>>> +        irq_domain_deactivate_irq(irq_desc_get_irq_data(desc));
>>> +    }
>>> +}
>>> +
>>> +static void get_vlpi_state_post(struct vgic_dist *dist)
>> 
>> nit: the naming feels a bit... odd. Pre/post what?
> 
> My understanding is that the unmapping is a preparation for 
> get_vlpi_state...
> Maybe just call it unmap/map_all_vpes?

Yes, much better.

[...]

>>> +        if (irq->hw) {
>>> +            WARN_RATELIMIT(irq_get_irqchip_state(irq->host_irq,
>>> +                        IRQCHIP_STATE_PENDING, &is_pending),
>>> +                       "IRQ %d", irq->host_irq);
>> 
>> Isn't this going to warn like mad on a GICv4.0 system where this, by 
>> definition,
>> will generate an error?
> 
> As we have returned an error in save_its_tables if hw && !has_gicv4_1, 
> we don't
> have to warn this here?

Are you referring to the check in vgic_its_save_itt() that occurs in 
patch 4?
Fair enough, though I think the use of irq_get_irqchip_state() isn't 
quite
what we want, as per my comments on patch #1.

>> 
>>> +        }
>>> +
>>> +        if (stored == is_pending)
>>>              continue;
>>> 
>>> -        if (irq->pending_latch)
>>> +        if (is_pending)
>>>              val |= 1 << bit_nr;
>>>          else
>>>              val &= ~(1 << bit_nr);
>>> 
>>>          ret = kvm_write_guest_lock(kvm, ptr, &val, 1);
>>>          if (ret)
>>> -            return ret;
>>> +            goto out;
>>>      }
>>> -    return 0;
>>> +
>>> +out:
>>> +    get_vlpi_state_post(dist);
>> 
>> This bit worries me: you have unmapped the VPEs, so any interrupt that 
>> has been
>> generated during that phase is now forever lost (the GIC doesn't have 
>> ownership
>> of the pending tables).
> 
> In my opinion, during this phase, the devices capable of interrupting
> should have  already been paused (prevent from sending interrupts),
> such as VFIO migration protocol has already realized it.

Is that a hard guarantee? Pausing devices *may* be possible for a 
limited
set of endpoints, but I'm not sure that is universally possible to 
restart
them and expect a consistent state (you have just dropped a bunch of 
network
packets on the floor...).

>> Do you really expect the VM to be restartable from that point? I don't 
>> see how
>> this is possible.
>> 
> 
> If the migration has encountered an error, the src VM might be
> restarted, so we have to map the vPEs back.

As I said above, I doubt it is universally possible to do so, but
after all, this probably isn't worse that restarting on the target...

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