On 2021/1/5 17:13, Marc Zyngier wrote:
> On 2021-01-04 08:16, Shenming Lu wrote:
>> After pausing all vCPUs and devices capable of interrupting, in order
>> 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@xxxxxxxxxx>
>> ---
>> arch/arm64/kvm/vgic/vgic-v3.c | 58 +++++++++++++++++++++++++++++++----
>> 1 file changed, 52 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
>> index 9cdf39a94a63..a58c94127cb0 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,38 @@ int vgic_v3_lpi_sync_pending_status(struct kvm
>> *kvm, struct vgic_irq *irq)
>> return 0;
>> }
>>
>> +/*
>> + * The deactivation of the doorbell interrupt will trigger the
>> + * unmapping of the associated vPE.
>> + */
>> +static void unmap_all_vpes(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 map_all_vpes(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_activate_irq(irq_desc_get_irq_data(desc), false);
>> + }
>> +}
>> +
>> /**
>> * vgic_v3_save_pending_tables - Save the pending tables into guest RAM
>> * kvm lock and all vcpu lock must be held
>> @@ -365,14 +399,18 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>> struct vgic_dist *dist = &kvm->arch.vgic;
>> struct vgic_irq *irq;
>> gpa_t last_ptr = ~(gpa_t)0;
>> - int ret;
>> + int ret = 0;
>> u8 val;
>>
>> + /* As a preparation for getting any VLPI states. */
>> + unmap_all_vpes(dist);
>
> What if the VPEs are not mapped yet? Is it possible to snapshot a VM
> that has not run at all?
What I see in QEMU is that the saving of the pending tables would only be
called when stopping the VM and it needs the current VM state to be RUNNING.
>
>> +
>> list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
>> int byte_offset, bit_nr;
>> struct kvm_vcpu *vcpu;
>> gpa_t pendbase, ptr;
>> bool stored;
>> + bool is_pending = irq->pending_latch;
>>
>> vcpu = irq->target_vcpu;
>> if (!vcpu)
>> @@ -387,24 +425,32 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>> if (ptr != last_ptr) {
>> ret = kvm_read_guest_lock(kvm, ptr, &val, 1);
>> if (ret)
>> - return ret;
>> + goto out;
>> last_ptr = ptr;
>> }
>>
>> stored = val & (1U << bit_nr);
>> - if (stored == irq->pending_latch)
>> +
>> + if (irq->hw)
>> + vgic_v4_get_vlpi_state(irq, &is_pending);
>
> You don't check the return value here, so I wonder why the checks
> in vgic_v4_get_vlpi_state().
Since I have already checked the condition and reported in save_its_tables
(patch 4), I just check in get_vlpi_state and don't report again here.
>
> Another thing that worries me is that vgic_v4_get_vlpi_state() doesn't
> have any cache invalidation, and can end-up hitting in the CPU cache
> (there is no guarantee of coherency between the GIC and the CPU, only
> that the GIC will have flushed its caches).
>
> I'd expect this to happen at unmap time, though, in order to avoid
> repeated single byte invalidations.
Ok, I will add a cache invalidation at unmap time.
>
>> +
>> + 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:
>> + map_all_vpes(dist);
>> +
>> + return ret;
>> }
>>
>> /**
>
> Thanks,
>
> M.
