[PATCH v3 0/4] KVM: arm64: Add VLPI migration support on GICv4.1

[PATCH v3 0/4] KVM: arm64: Add VLPI migration support on GICv4.1

[Shenming Lu]

Hi Marc, sorry for the late commit.

In GICv4.1, migration has been supported except for (directly-injected)
VLPI. And GICv4.1 Spec explicitly gives a way to get the VLPI's pending
state (which was crucially missing in GICv4.0). So we make VLPI migration
capable on GICv4.1 in this patch set.

In order to support VLPI migration, we need to save and restore all
required configuration information and pending states of VLPIs. But
in fact, the configuration information of VLPIs has already been saved
(or will be reallocated on the dst host...) in vgic(kvm) migration.
So we only have to migrate the pending states of VLPIs specially.

Below is the related workflow in migration.

On the save path:
	In migration completion:
		pause all vCPUs
				|
		call each VM state change handler:
			pause other devices (just keep from sending interrupts, and
			such as VFIO migration protocol has already realized it [1])
					|
			flush ITS tables into guest RAM
					|
			flush RDIST pending tables (also flush VLPI state here)
				|
		...
On the resume path:
	load each device's state:
		restore ITS tables (include pending tables) from guest RAM
				|
		for other (PCI) devices (paused), if configured to have VLPIs,
		establish the forwarding paths of their VLPIs (and transfer
		the pending states from kvm's vgic to VPT here)

We have tested this series in VFIO migration, and found some related
issues in QEMU [2].

Links:
[1] vfio: UAPI for migration interface for device state:
    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a8a24f3f6e38103b77cf399c38eb54e1219d00d6
[2] vfio: Some fixes and optimizations for VFIO migration:
    https://patchwork.ozlabs.org/cover/1413263/

History:

v2 -> v3
 - Add the vgic initialized check to ensure that the allocation and enabling
   of the doorbells have already been done before unmapping the vPEs.
 - Check all get_vlpi_state related conditions in save_pending_tables in one place.
 - Nit fixes.

v1 -> v2:
 - Get the VLPI state from the KVM side.
 - Nit fixes.

Thanks,
Shenming


Shenming Lu (3):
  KVM: arm64: GICv4.1: Add function to get VLPI state
  KVM: arm64: GICv4.1: Try to save hw pending state in
    save_pending_tables
  KVM: arm64: GICv4.1: Give a chance to save VLPI's pending state

Zenghui Yu (1):
  KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

 .../virt/kvm/devices/arm-vgic-its.rst         |  2 +-
 arch/arm64/kvm/vgic/vgic-its.c                |  6 +-
 arch/arm64/kvm/vgic/vgic-v3.c                 | 61 +++++++++++++++++--
 arch/arm64/kvm/vgic/vgic-v4.c                 | 33 ++++++++++
 arch/arm64/kvm/vgic/vgic.h                    |  1 +
 5 files changed, 93 insertions(+), 10 deletions(-)

-- 
2.19.1

[PATCH v3 1/4] KVM: arm64: GICv4.1: Add function to get VLPI state

[Shenming Lu]

With GICv4.1 and the vPE unmapped, which indicates the invalidation
of any VPT caches associated with the vPE, we can get the VLPI state
by peeking at the VPT. So we add a function for this.

Signed-off-by: Shenming Lu <lushenming@huawei.com>
---
 arch/arm64/kvm/vgic/vgic-v4.c | 19 +++++++++++++++++++
 arch/arm64/kvm/vgic/vgic.h    |  1 +
 2 files changed, 20 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
index 66508b03094f..ac029ba3d337 100644
--- a/arch/arm64/kvm/vgic/vgic-v4.c
+++ b/arch/arm64/kvm/vgic/vgic-v4.c
@@ -203,6 +203,25 @@ void vgic_v4_configure_vsgis(struct kvm *kvm)
 	kvm_arm_resume_guest(kvm);
 }
 
+/*
+ * Must be called with GICv4.1 and the vPE unmapped, which
+ * indicates the invalidation of any VPT caches associated
+ * with the vPE, thus we can get the VLPI state by peeking
+ * at the VPT.
+ */
+void vgic_v4_get_vlpi_state(struct vgic_irq *irq, bool *val)
+{
+	struct its_vpe *vpe = &irq->target_vcpu->arch.vgic_cpu.vgic_v3.its_vpe;
+	int mask = BIT(irq->intid % BITS_PER_BYTE);
+	void *va;
+	u8 *ptr;
+
+	va = page_address(vpe->vpt_page);
+	ptr = va + irq->intid / BITS_PER_BYTE;
+
+	*val = !!(*ptr & mask);
+}
+
 /**
  * vgic_v4_init - Initialize the GICv4 data structures
  * @kvm:	Pointer to the VM being initialized
diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
index 64fcd7511110..d8cfd360838c 100644
--- a/arch/arm64/kvm/vgic/vgic.h
+++ b/arch/arm64/kvm/vgic/vgic.h
@@ -317,5 +317,6 @@ bool vgic_supports_direct_msis(struct kvm *kvm);
 int vgic_v4_init(struct kvm *kvm);
 void vgic_v4_teardown(struct kvm *kvm);
 void vgic_v4_configure_vsgis(struct kvm *kvm);
+void vgic_v4_get_vlpi_state(struct vgic_irq *irq, bool *val);
 
 #endif
-- 
2.19.1

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

[Shenming Lu]

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.

As for the saving of VSGIs, which needs the vPEs to be mapped and might
conflict with the saving of VLPIs, but since we will map the vPEs back
at the end of save_pending_tables and both savings require the kvm->lock
to be held (only happen serially), it will work fine.

Signed-off-by: Shenming Lu <lushenming@huawei.com>
---
 arch/arm64/kvm/vgic/vgic-v3.c | 61 +++++++++++++++++++++++++++++++----
 1 file changed, 55 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 52915b342351..06b1162b7a0a 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,32 @@ 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;
+
+	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;
+
+	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 +393,26 @@ 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;
+	bool vlpi_avail = false;
+	int ret = 0;
 	u8 val;
 
+	/*
+	 * As a preparation for getting any VLPI states.
+	 * The vgic initialized check ensures that the allocation and
+	 * enabling of the doorbells have already been done.
+	 */
+	if (kvm_vgic_global_state.has_gicv4_1 && !WARN_ON(!vgic_initialized(kvm))) {
+		unmap_all_vpes(dist);
+		vlpi_avail = true;
+	}
+
 	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 +427,33 @@ 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 && vlpi_avail)
+			vgic_v4_get_vlpi_state(irq, &is_pending);
+
+		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:
+	if (vlpi_avail)
+		map_all_vpes(dist);
+
+	return ret;
 }
 
 /**
-- 
2.19.1

[PATCH v3 3/4] KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

[Shenming Lu]

From: Zenghui Yu <yuzenghui@huawei.com>

When setting the forwarding path of a VLPI (switch to the HW mode),
we could also transfer the pending state from irq->pending_latch to
VPT (especially in migration, the pending states of VLPIs are restored
into kvm’s vgic first). And we currently send "INT+VSYNC" to trigger
a VLPI to pending.

Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
Signed-off-by: Shenming Lu <lushenming@huawei.com>
---
 arch/arm64/kvm/vgic/vgic-v4.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
index ac029ba3d337..a3542af6f04a 100644
--- a/arch/arm64/kvm/vgic/vgic-v4.c
+++ b/arch/arm64/kvm/vgic/vgic-v4.c
@@ -449,6 +449,20 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
 	irq->host_irq	= virq;
 	atomic_inc(&map.vpe->vlpi_count);
 
+	/* Transfer pending state */
+	if (irq->pending_latch) {
+		ret = irq_set_irqchip_state(irq->host_irq,
+					    IRQCHIP_STATE_PENDING,
+					    irq->pending_latch);
+		WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
+
+		/*
+		 * Let it be pruned from ap_list later and don't bother
+		 * the List Register.
+		 */
+		irq->pending_latch = false;
+	}
+
 out:
 	mutex_unlock(&its->its_lock);
 	return ret;
-- 
2.19.1

[PATCH v3 4/4] KVM: arm64: GICv4.1: Give a chance to save VLPI's pending state

[Shenming Lu]

Before GICv4.1, we don't have direct access to the VLPI's pending
state. So we simply let it fail early when encountering any VLPI.

But now we don't have to return -EACCES directly if on GICv4.1. So
let’s change the hard code and give a chance to save the VLPI's pending
state (and preserve the UAPI).

Signed-off-by: Shenming Lu <lushenming@huawei.com>
---
 Documentation/virt/kvm/devices/arm-vgic-its.rst | 2 +-
 arch/arm64/kvm/vgic/vgic-its.c                  | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/virt/kvm/devices/arm-vgic-its.rst b/Documentation/virt/kvm/devices/arm-vgic-its.rst
index 6c304fd2b1b4..d257eddbae29 100644
--- a/Documentation/virt/kvm/devices/arm-vgic-its.rst
+++ b/Documentation/virt/kvm/devices/arm-vgic-its.rst
@@ -80,7 +80,7 @@ KVM_DEV_ARM_VGIC_GRP_CTRL
     -EFAULT  Invalid guest ram access
     -EBUSY   One or more VCPUS are running
     -EACCES  The virtual ITS is backed by a physical GICv4 ITS, and the
-	     state is not available
+	     state is not available without GICv4.1
     =======  ==========================================================
 
 KVM_DEV_ARM_VGIC_GRP_ITS_REGS
diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 40cbaca81333..ec7543a9617c 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -2218,10 +2218,10 @@ static int vgic_its_save_itt(struct vgic_its *its, struct its_device *device)
 		/*
 		 * If an LPI carries the HW bit, this means that this
 		 * interrupt is controlled by GICv4, and we do not
-		 * have direct access to that state. Let's simply fail
-		 * the save operation...
+		 * have direct access to that state without GICv4.1.
+		 * Let's simply fail the save operation...
 		 */
-		if (ite->irq->hw)
+		if (ite->irq->hw && !kvm_vgic_global_state.has_gicv4_1)
 			return -EACCES;
 
 		ret = vgic_its_save_ite(its, device, ite, gpa, ite_esz);
-- 
2.19.1

Re: [PATCH v3 0/4] KVM: arm64: Add VLPI migration support on GICv4.1

[Shenming Lu]

Hi Marc,

Gentle ping. Does this series need any further modification? Wish you can pick it up. :-)

Thanks,
Shenming

On 2021/1/27 20:13, Shenming Lu wrote:
> Hi Marc, sorry for the late commit.
> 
> In GICv4.1, migration has been supported except for (directly-injected)
> VLPI. And GICv4.1 Spec explicitly gives a way to get the VLPI's pending
> state (which was crucially missing in GICv4.0). So we make VLPI migration
> capable on GICv4.1 in this patch set.
> 
> In order to support VLPI migration, we need to save and restore all
> required configuration information and pending states of VLPIs. But
> in fact, the configuration information of VLPIs has already been saved
> (or will be reallocated on the dst host...) in vgic(kvm) migration.
> So we only have to migrate the pending states of VLPIs specially.
> 
> Below is the related workflow in migration.
> 
> On the save path:
> 	In migration completion:
> 		pause all vCPUs
> 				|
> 		call each VM state change handler:
> 			pause other devices (just keep from sending interrupts, and
> 			such as VFIO migration protocol has already realized it [1])
> 					|
> 			flush ITS tables into guest RAM
> 					|
> 			flush RDIST pending tables (also flush VLPI state here)
> 				|
> 		...
> On the resume path:
> 	load each device's state:
> 		restore ITS tables (include pending tables) from guest RAM
> 				|
> 		for other (PCI) devices (paused), if configured to have VLPIs,
> 		establish the forwarding paths of their VLPIs (and transfer
> 		the pending states from kvm's vgic to VPT here)
> 
> We have tested this series in VFIO migration, and found some related
> issues in QEMU [2].
> 
> Links:
> [1] vfio: UAPI for migration interface for device state:
>     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a8a24f3f6e38103b77cf399c38eb54e1219d00d6
> [2] vfio: Some fixes and optimizations for VFIO migration:
>     https://patchwork.ozlabs.org/cover/1413263/
> 
> History:
> 
> v2 -> v3
>  - Add the vgic initialized check to ensure that the allocation and enabling
>    of the doorbells have already been done before unmapping the vPEs.
>  - Check all get_vlpi_state related conditions in save_pending_tables in one place.
>  - Nit fixes.
> 
> v1 -> v2:
>  - Get the VLPI state from the KVM side.
>  - Nit fixes.
> 
> Thanks,
> Shenming
> 
> 
> Shenming Lu (3):
>   KVM: arm64: GICv4.1: Add function to get VLPI state
>   KVM: arm64: GICv4.1: Try to save hw pending state in
>     save_pending_tables
>   KVM: arm64: GICv4.1: Give a chance to save VLPI's pending state
> 
> Zenghui Yu (1):
>   KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side
> 
>  .../virt/kvm/devices/arm-vgic-its.rst         |  2 +-
>  arch/arm64/kvm/vgic/vgic-its.c                |  6 +-
>  arch/arm64/kvm/vgic/vgic-v3.c                 | 61 +++++++++++++++++--
>  arch/arm64/kvm/vgic/vgic-v4.c                 | 33 ++++++++++
>  arch/arm64/kvm/vgic/vgic.h                    |  1 +
>  5 files changed, 93 insertions(+), 10 deletions(-)
> 

Re: [PATCH v3 0/4] KVM: arm64: Add VLPI migration support on GICv4.1

[Shenming Lu]

Hi,

Sorry to bother again, I am really hoping a response for this series. :-)

Thanks,
Shenming

On 2021/2/26 16:58, Shenming Lu wrote:
> Hi Marc,
> 
> Gentle ping. Does this series need any further modification? Wish you can pick it up. :-)
> 
> Thanks,
> Shenming
> 
> On 2021/1/27 20:13, Shenming Lu wrote:
>> Hi Marc, sorry for the late commit.
>>
>> In GICv4.1, migration has been supported except for (directly-injected)
>> VLPI. And GICv4.1 Spec explicitly gives a way to get the VLPI's pending
>> state (which was crucially missing in GICv4.0). So we make VLPI migration
>> capable on GICv4.1 in this patch set.
>>
>> In order to support VLPI migration, we need to save and restore all
>> required configuration information and pending states of VLPIs. But
>> in fact, the configuration information of VLPIs has already been saved
>> (or will be reallocated on the dst host...) in vgic(kvm) migration.
>> So we only have to migrate the pending states of VLPIs specially.
>>
>> Below is the related workflow in migration.
>>
>> On the save path:
>> 	In migration completion:
>> 		pause all vCPUs
>> 				|
>> 		call each VM state change handler:
>> 			pause other devices (just keep from sending interrupts, and
>> 			such as VFIO migration protocol has already realized it [1])
>> 					|
>> 			flush ITS tables into guest RAM
>> 					|
>> 			flush RDIST pending tables (also flush VLPI state here)
>> 				|
>> 		...
>> On the resume path:
>> 	load each device's state:
>> 		restore ITS tables (include pending tables) from guest RAM
>> 				|
>> 		for other (PCI) devices (paused), if configured to have VLPIs,
>> 		establish the forwarding paths of their VLPIs (and transfer
>> 		the pending states from kvm's vgic to VPT here)
>>
>> We have tested this series in VFIO migration, and found some related
>> issues in QEMU [2].
>>
>> Links:
>> [1] vfio: UAPI for migration interface for device state:
>>     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a8a24f3f6e38103b77cf399c38eb54e1219d00d6
>> [2] vfio: Some fixes and optimizations for VFIO migration:
>>     https://patchwork.ozlabs.org/cover/1413263/
>>
>> History:
>>
>> v2 -> v3
>>  - Add the vgic initialized check to ensure that the allocation and enabling
>>    of the doorbells have already been done before unmapping the vPEs.
>>  - Check all get_vlpi_state related conditions in save_pending_tables in one place.
>>  - Nit fixes.
>>
>> v1 -> v2:
>>  - Get the VLPI state from the KVM side.
>>  - Nit fixes.
>>
>> Thanks,
>> Shenming
>>
>>
>> Shenming Lu (3):
>>   KVM: arm64: GICv4.1: Add function to get VLPI state
>>   KVM: arm64: GICv4.1: Try to save hw pending state in
>>     save_pending_tables
>>   KVM: arm64: GICv4.1: Give a chance to save VLPI's pending state
>>
>> Zenghui Yu (1):
>>   KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side
>>
>>  .../virt/kvm/devices/arm-vgic-its.rst         |  2 +-
>>  arch/arm64/kvm/vgic/vgic-its.c                |  6 +-
>>  arch/arm64/kvm/vgic/vgic-v3.c                 | 61 +++++++++++++++++--
>>  arch/arm64/kvm/vgic/vgic-v4.c                 | 33 ++++++++++
>>  arch/arm64/kvm/vgic/vgic.h                    |  1 +
>>  5 files changed, 93 insertions(+), 10 deletions(-)
>>

Re: [PATCH v3 1/4] KVM: arm64: GICv4.1: Add function to get VLPI state

[Shenming Lu]

On 2021/3/11 16:57, Marc Zyngier wrote:
> On Wed, 27 Jan 2021 12:13:34 +0000,
> Shenming Lu <lushenming@huawei.com> wrote:
>>
>> With GICv4.1 and the vPE unmapped, which indicates the invalidation
>> of any VPT caches associated with the vPE, we can get the VLPI state
>> by peeking at the VPT. So we add a function for this.
>>
>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>> ---
>>  arch/arm64/kvm/vgic/vgic-v4.c | 19 +++++++++++++++++++
>>  arch/arm64/kvm/vgic/vgic.h    |  1 +
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
>> index 66508b03094f..ac029ba3d337 100644
>> --- a/arch/arm64/kvm/vgic/vgic-v4.c
>> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
>> @@ -203,6 +203,25 @@ void vgic_v4_configure_vsgis(struct kvm *kvm)
>>  	kvm_arm_resume_guest(kvm);
>>  }
>>  
>> +/*
>> + * Must be called with GICv4.1 and the vPE unmapped, which
>> + * indicates the invalidation of any VPT caches associated
>> + * with the vPE, thus we can get the VLPI state by peeking
>> + * at the VPT.
>> + */
>> +void vgic_v4_get_vlpi_state(struct vgic_irq *irq, bool *val)
>> +{
>> +	struct its_vpe *vpe = &irq->target_vcpu->arch.vgic_cpu.vgic_v3.its_vpe;
>> +	int mask = BIT(irq->intid % BITS_PER_BYTE);
>> +	void *va;
>> +	u8 *ptr;
>> +
>> +	va = page_address(vpe->vpt_page);
>> +	ptr = va + irq->intid / BITS_PER_BYTE;
>> +
>> +	*val = !!(*ptr & mask);
> 
> What guarantees that you can actually read anything valid? Yes, the
> VPT caches are clean. But that doesn't make them coherent with CPU
> caches.
> 
> You need some level of cache maintenance here.

Yeah, and you have come up with some codes for this in v2:

diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index 7db602434ac5..2dbef127ca15 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -4552,6 +4552,10 @@ static void its_vpe_irq_domain_deactivate(struct
irq_domain *domain,

  		its_send_vmapp(its, vpe, false);
  	}
+
+	if (find_4_1_its() && !atomic_read(vpe->vmapp_count))
+		gic_flush_dcache_to_poc(page_address(vpe->vpt_page),
+					LPI_PENDBASE_SZ);
  }

  static const struct irq_domain_ops its_vpe_domain_ops = {

Could I add it to this series? :-)

Thanks,
Shenming

> 
> Thanks,
> 
> 	M.
> 

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

[Shenming Lu]

On 2021/3/11 17:09, Marc Zyngier wrote:
> On Wed, 27 Jan 2021 12:13:35 +0000,
> Shenming Lu <lushenming@huawei.com> 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.
>>
>> As for the saving of VSGIs, which needs the vPEs to be mapped and might
>> conflict with the saving of VLPIs, but since we will map the vPEs back
>> at the end of save_pending_tables and both savings require the kvm->lock
>> to be held (only happen serially), it will work fine.
>>
>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>> ---
>>  arch/arm64/kvm/vgic/vgic-v3.c | 61 +++++++++++++++++++++++++++++++----
>>  1 file changed, 55 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
>> index 52915b342351..06b1162b7a0a 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,32 @@ 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;
>> +
>> +	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;
>> +
>> +	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 +393,26 @@ 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;
>> +	bool vlpi_avail = false;
>> +	int ret = 0;
>>  	u8 val;
>>  
>> +	/*
>> +	 * As a preparation for getting any VLPI states.
>> +	 * The vgic initialized check ensures that the allocation and
>> +	 * enabling of the doorbells have already been done.
>> +	 */
>> +	if (kvm_vgic_global_state.has_gicv4_1 && !WARN_ON(!vgic_initialized(kvm))) {
> 
> Should we bail out if we ever spot !vgic_initialized()? In general, I
> find the double negation horrible to read).

Ok, I will change it.

> 
>> +		unmap_all_vpes(dist);
>> +		vlpi_avail = true;
>> +	}
>> +
>>  	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 +427,33 @@ 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 && vlpi_avail)
>> +			vgic_v4_get_vlpi_state(irq, &is_pending);
> 
> Keep the 'is_pending = irq->pending_latch;' statement close to the VPT
> read, since they represent the same state.

Ok, make sense.

> 
>> +
>> +		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:
>> +	if (vlpi_avail)
>> +		map_all_vpes(dist);
> 
> I have asked that question in the past: is it actually safe to remap
> the vPEs and expect them to be runnable

In my opinion, logically it can work, but there might be problems like the
one below that I didn't notice...

> 
> Also, the current code assumes that VMAPP.PTZ can be advertised if a
> VPT is mapped for the first time. Clearly, it is unlikely that the VPT
> will be only populated with 0s, so you'll end up with state corruption
> on the first remap.

Oh, thanks for pointing it out.
And if we always signal PTZ when alloc = 1, does it mean that we can't remap the
vPE when the VPT is not empty, thus there is no chance to get the VLPI state?
Could we just assume that the VPT is not empty when first mapping the vPE?

Thanks,
Shenming

> 
> Thanks,
> 
> 	M.
> 

Re: [PATCH v3 3/4] KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

[Shenming Lu]

On 2021/3/11 17:14, Marc Zyngier wrote:
> On Wed, 27 Jan 2021 12:13:36 +0000,
> Shenming Lu <lushenming@huawei.com> wrote:
>>
>> From: Zenghui Yu <yuzenghui@huawei.com>
>>
>> When setting the forwarding path of a VLPI (switch to the HW mode),
>> we could also transfer the pending state from irq->pending_latch to
>> VPT (especially in migration, the pending states of VLPIs are restored
>> into kvm’s vgic first). And we currently send "INT+VSYNC" to trigger
>> a VLPI to pending.
>>
>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>> ---
>>  arch/arm64/kvm/vgic/vgic-v4.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
>> index ac029ba3d337..a3542af6f04a 100644
>> --- a/arch/arm64/kvm/vgic/vgic-v4.c
>> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
>> @@ -449,6 +449,20 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
>>  	irq->host_irq	= virq;
>>  	atomic_inc(&map.vpe->vlpi_count);
>>  
>> +	/* Transfer pending state */
>> +	if (irq->pending_latch) {
>> +		ret = irq_set_irqchip_state(irq->host_irq,
>> +					    IRQCHIP_STATE_PENDING,
>> +					    irq->pending_latch);
>> +		WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
>> +
>> +		/*
>> +		 * Let it be pruned from ap_list later and don't bother
>> +		 * the List Register.
>> +		 */
>> +		irq->pending_latch = false;
> 
> NAK. If the interrupt is on the AP list, it must be pruned from it
> *immediately*. The only case where it can be !pending and still on the
> AP list is in interval between sync and prune. If we start messing
> with this, we can't reason about the state of this list anymore.
> 
> Consider calling vgic_queue_irq_unlock() here.

Thanks for giving a hint, but it seems that vgic_queue_irq_unlock() only
queues an IRQ after checking, did you mean vgic_prune_ap_list() instead?

Thanks a lot for the comments! :-)
Shenming

> 
> Thanks,
> 
> 	M.
> 

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

[Shenming Lu]

On 2021/3/12 17:02, Marc Zyngier wrote:
> On Thu, 11 Mar 2021 12:31:48 +0000,
> Shenming Lu <lushenming@huawei.com> wrote:
>>
>> On 2021/3/11 17:09, Marc Zyngier wrote:
> 
>>> I have asked that question in the past: is it actually safe to remap
>>> the vPEs and expect them to be runnable
>>
>> In my opinion, logically it can work, but there might be problems like the
>> one below that I didn't notice...
> 
> One thing is that you will have lost interrupts in the meantime
> (assuming your devices are still alive). How will you make up for
> that?

I think that devices should be paused for (not only) saving interrupt states,
and in fact, that's exactly what such as VFIO devices do...

> 
>>
>>>
>>> Also, the current code assumes that VMAPP.PTZ can be advertised if a
>>> VPT is mapped for the first time. Clearly, it is unlikely that the VPT
>>> will be only populated with 0s, so you'll end up with state corruption
>>> on the first remap.
>>
>> Oh, thanks for pointing it out.
>> And if we always signal PTZ when alloc = 1, does it mean that we
>> can't remap the vPE when the VPT is not empty, thus there is no
>> chance to get the VLPI state?  Could we just assume that the VPT is
>> not empty when first mapping the vPE?
> 
> I think we should drop the setting of PTZ altogether. It is a silly
> micro-optimisation, and if the HW can't parse the VPT efficiently when
> it is empty, then the HW is pretty bad, PTZ or not.

agree :-)

Thanks,
Shenming

> 
> Thanks,
> 
> 	M.
> 

Re: [PATCH v3 3/4] KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

[Shenming Lu]

On 2021/3/12 17:05, Marc Zyngier wrote:
> On Thu, 11 Mar 2021 12:32:07 +0000,
> Shenming Lu <lushenming@huawei.com> wrote:
>>
>> On 2021/3/11 17:14, Marc Zyngier wrote:
>>> On Wed, 27 Jan 2021 12:13:36 +0000,
>>> Shenming Lu <lushenming@huawei.com> wrote:
>>>>
>>>> From: Zenghui Yu <yuzenghui@huawei.com>
>>>>
>>>> When setting the forwarding path of a VLPI (switch to the HW mode),
>>>> we could also transfer the pending state from irq->pending_latch to
>>>> VPT (especially in migration, the pending states of VLPIs are restored
>>>> into kvm’s vgic first). And we currently send "INT+VSYNC" to trigger
>>>> a VLPI to pending.
>>>>
>>>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>>>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>>>> ---
>>>>  arch/arm64/kvm/vgic/vgic-v4.c | 14 ++++++++++++++
>>>>  1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
>>>> index ac029ba3d337..a3542af6f04a 100644
>>>> --- a/arch/arm64/kvm/vgic/vgic-v4.c
>>>> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
>>>> @@ -449,6 +449,20 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
>>>>  	irq->host_irq	= virq;
>>>>  	atomic_inc(&map.vpe->vlpi_count);
>>>>  
>>>> +	/* Transfer pending state */
>>>> +	if (irq->pending_latch) {
>>>> +		ret = irq_set_irqchip_state(irq->host_irq,
>>>> +					    IRQCHIP_STATE_PENDING,
>>>> +					    irq->pending_latch);
>>>> +		WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
>>>> +
>>>> +		/*
>>>> +		 * Let it be pruned from ap_list later and don't bother
>>>> +		 * the List Register.
>>>> +		 */
>>>> +		irq->pending_latch = false;
>>>
>>> NAK. If the interrupt is on the AP list, it must be pruned from it
>>> *immediately*. The only case where it can be !pending and still on the
>>> AP list is in interval between sync and prune. If we start messing
>>> with this, we can't reason about the state of this list anymore.
>>>
>>> Consider calling vgic_queue_irq_unlock() here.
>>
>> Thanks for giving a hint, but it seems that vgic_queue_irq_unlock() only
>> queues an IRQ after checking, did you mean vgic_prune_ap_list() instead?
> 
> No, I really mean vgic_queue_irq_unlock(). It can be used to remove
> the pending state from an interrupt, and drop it from the AP
> list. This is exactly what happens when clearing the pending state of
> a level interrupt, for example.

Hi, I have gone through vgic_queue_irq_unlock more than once, but still can't
find the place in it to drop an IRQ from the AP list... Did I miss something ?...
Or could you help to point it out? Thanks very much for this!

Shenming

> 
> 	M.
> 

Re: [PATCH v3 3/4] KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

[Shenming Lu]

On 2021/3/12 19:10, Marc Zyngier wrote:
> On Fri, 12 Mar 2021 10:48:29 +0000,
> Shenming Lu <lushenming@huawei.com> wrote:
>>
>> On 2021/3/12 17:05, Marc Zyngier wrote:
>>> On Thu, 11 Mar 2021 12:32:07 +0000,
>>> Shenming Lu <lushenming@huawei.com> wrote:
>>>>
>>>> On 2021/3/11 17:14, Marc Zyngier wrote:
>>>>> On Wed, 27 Jan 2021 12:13:36 +0000,
>>>>> Shenming Lu <lushenming@huawei.com> wrote:
>>>>>>
>>>>>> From: Zenghui Yu <yuzenghui@huawei.com>
>>>>>>
>>>>>> When setting the forwarding path of a VLPI (switch to the HW mode),
>>>>>> we could also transfer the pending state from irq->pending_latch to
>>>>>> VPT (especially in migration, the pending states of VLPIs are restored
>>>>>> into kvm’s vgic first). And we currently send "INT+VSYNC" to trigger
>>>>>> a VLPI to pending.
>>>>>>
>>>>>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>>>>>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>>>>>> ---
>>>>>>  arch/arm64/kvm/vgic/vgic-v4.c | 14 ++++++++++++++
>>>>>>  1 file changed, 14 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
>>>>>> index ac029ba3d337..a3542af6f04a 100644
>>>>>> --- a/arch/arm64/kvm/vgic/vgic-v4.c
>>>>>> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
>>>>>> @@ -449,6 +449,20 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
>>>>>>  	irq->host_irq	= virq;
>>>>>>  	atomic_inc(&map.vpe->vlpi_count);
>>>>>>  
>>>>>> +	/* Transfer pending state */
>>>>>> +	if (irq->pending_latch) {
>>>>>> +		ret = irq_set_irqchip_state(irq->host_irq,
>>>>>> +					    IRQCHIP_STATE_PENDING,
>>>>>> +					    irq->pending_latch);
>>>>>> +		WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
>>>>>> +
>>>>>> +		/*
>>>>>> +		 * Let it be pruned from ap_list later and don't bother
>>>>>> +		 * the List Register.
>>>>>> +		 */
>>>>>> +		irq->pending_latch = false;
>>>>>
>>>>> NAK. If the interrupt is on the AP list, it must be pruned from it
>>>>> *immediately*. The only case where it can be !pending and still on the
>>>>> AP list is in interval between sync and prune. If we start messing
>>>>> with this, we can't reason about the state of this list anymore.
>>>>>
>>>>> Consider calling vgic_queue_irq_unlock() here.
>>>>
>>>> Thanks for giving a hint, but it seems that vgic_queue_irq_unlock() only
>>>> queues an IRQ after checking, did you mean vgic_prune_ap_list() instead?
>>>
>>> No, I really mean vgic_queue_irq_unlock(). It can be used to remove
>>> the pending state from an interrupt, and drop it from the AP
>>> list. This is exactly what happens when clearing the pending state of
>>> a level interrupt, for example.
>>
>> Hi, I have gone through vgic_queue_irq_unlock more than once, but
>> still can't find the place in it to drop an IRQ from the AP
>> list... Did I miss something ?...  Or could you help to point it
>> out? Thanks very much for this!
> 
> NO, you are right. I think this is a missing optimisation. Please call
> the function anyway, as that's what is required to communicate a
> change of state in general.>
> I'll have a think about it.

Maybe we could call vgic_prune_ap_list() if (irq->vcpu && !vgic_target_oracle(irq)) in vgic_queue_irq_unlock()...

OK, I will retest this series and send a v4 soon. :-)

Thanks,
Shenming

> 
> Thanks,
> 
> 	M.
> 

Re: [PATCH v3 3/4] KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

[Shenming Lu]

On 2021/3/12 20:02, Marc Zyngier wrote:
> On Fri, 12 Mar 2021 11:34:07 +0000,
> Shenming Lu <lushenming@huawei.com> wrote:
>>
>> On 2021/3/12 19:10, Marc Zyngier wrote:
>>> On Fri, 12 Mar 2021 10:48:29 +0000,
>>> Shenming Lu <lushenming@huawei.com> wrote:
>>>>
>>>> On 2021/3/12 17:05, Marc Zyngier wrote:
>>>>> On Thu, 11 Mar 2021 12:32:07 +0000,
>>>>> Shenming Lu <lushenming@huawei.com> wrote:
>>>>>>
>>>>>> On 2021/3/11 17:14, Marc Zyngier wrote:
>>>>>>> On Wed, 27 Jan 2021 12:13:36 +0000,
>>>>>>> Shenming Lu <lushenming@huawei.com> wrote:
>>>>>>>>
>>>>>>>> From: Zenghui Yu <yuzenghui@huawei.com>
>>>>>>>>
>>>>>>>> When setting the forwarding path of a VLPI (switch to the HW mode),
>>>>>>>> we could also transfer the pending state from irq->pending_latch to
>>>>>>>> VPT (especially in migration, the pending states of VLPIs are restored
>>>>>>>> into kvm’s vgic first). And we currently send "INT+VSYNC" to trigger
>>>>>>>> a VLPI to pending.
>>>>>>>>
>>>>>>>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>>>>>>>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>>>>>>>> ---
>>>>>>>>  arch/arm64/kvm/vgic/vgic-v4.c | 14 ++++++++++++++
>>>>>>>>  1 file changed, 14 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
>>>>>>>> index ac029ba3d337..a3542af6f04a 100644
>>>>>>>> --- a/arch/arm64/kvm/vgic/vgic-v4.c
>>>>>>>> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
>>>>>>>> @@ -449,6 +449,20 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
>>>>>>>>  	irq->host_irq	= virq;
>>>>>>>>  	atomic_inc(&map.vpe->vlpi_count);
>>>>>>>>  
>>>>>>>> +	/* Transfer pending state */
>>>>>>>> +	if (irq->pending_latch) {
>>>>>>>> +		ret = irq_set_irqchip_state(irq->host_irq,
>>>>>>>> +					    IRQCHIP_STATE_PENDING,
>>>>>>>> +					    irq->pending_latch);
>>>>>>>> +		WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
>>>>>>>> +
>>>>>>>> +		/*
>>>>>>>> +		 * Let it be pruned from ap_list later and don't bother
>>>>>>>> +		 * the List Register.
>>>>>>>> +		 */
>>>>>>>> +		irq->pending_latch = false;
>>>>>>>
>>>>>>> NAK. If the interrupt is on the AP list, it must be pruned from it
>>>>>>> *immediately*. The only case where it can be !pending and still on the
>>>>>>> AP list is in interval between sync and prune. If we start messing
>>>>>>> with this, we can't reason about the state of this list anymore.
>>>>>>>
>>>>>>> Consider calling vgic_queue_irq_unlock() here.
>>>>>>
>>>>>> Thanks for giving a hint, but it seems that vgic_queue_irq_unlock() only
>>>>>> queues an IRQ after checking, did you mean vgic_prune_ap_list() instead?
>>>>>
>>>>> No, I really mean vgic_queue_irq_unlock(). It can be used to remove
>>>>> the pending state from an interrupt, and drop it from the AP
>>>>> list. This is exactly what happens when clearing the pending state of
>>>>> a level interrupt, for example.
>>>>
>>>> Hi, I have gone through vgic_queue_irq_unlock more than once, but
>>>> still can't find the place in it to drop an IRQ from the AP
>>>> list... Did I miss something ?...  Or could you help to point it
>>>> out? Thanks very much for this!
>>>
>>> NO, you are right. I think this is a missing optimisation. Please call
>>> the function anyway, as that's what is required to communicate a
>>> change of state in general.>
>>> I'll have a think about it.
>>
>> Maybe we could call vgic_prune_ap_list() if (irq->vcpu &&
>> !vgic_target_oracle(irq)) in vgic_queue_irq_unlock()...
> 
> The locking is pretty ugly in this case, and I don't want to reparse
> the whole AP list. It is basically doing the same work as the
> insertion, but with a list_del() instead of a list_add()...

make sense..

Thanks,
Shenming

> 
> We can live without it for now.
> 
>> OK, I will retest this series and send a v4 soon. :-)
> 
> Thanks,
> 
> 	M.
>