It has been known case where no running VCPU context exists when the
vgic/its tables are saved. There are other two unknown cases where we
don't have the running VCPU context: (a) restore vgic3 LPI pending
status. (b) restoring vgic3 pending tables.
PATCH[1] includes 'kvm_mmu.h' to 'vgic.h'
PATCH[2] adds unified helper vgic_write_guest_lock()
PATCH[3 - 4] allows no-running-vcpu context for (a) and (b)
v2: https://lore.kernel.org/kvmarm/[email protected]/T/#t
v1: https://lore.kernel.org/kvmarm/[email protected]/T/#t
Changelog
=========
v3:
* Pick Oliver's r-bs
* Include 'kvm_mmu.h' to 'vgic.h' (Oliver)
v2:
* Add unified helper vgic_write_guest_lock() (Oliver)
* Dropped two patches to refactor mark_page_dirty_in_slot() (Sean)
Gavin Shan (4):
KVM: arm64: Include kvm_mmu.h from vgic.h
KVM: arm64: Add helper vgic_write_guest_lock()
KVM: arm64: Allow no running vcpu on restoring vgic3 LPI pending
status
KVM: arm64: Allow no running vcpu on saving vgic3 pending table
Documentation/virt/kvm/api.rst | 10 +++++++---
arch/arm64/kvm/vgic/vgic-debug.c | 1 -
arch/arm64/kvm/vgic/vgic-init.c | 1 -
arch/arm64/kvm/vgic/vgic-its.c | 14 +++++---------
arch/arm64/kvm/vgic/vgic-kvm-device.c | 1 -
arch/arm64/kvm/vgic/vgic-mmio-v3.c | 1 -
arch/arm64/kvm/vgic/vgic-v2.c | 1 -
arch/arm64/kvm/vgic/vgic-v3.c | 5 ++---
arch/arm64/kvm/vgic/vgic.h | 14 ++++++++++++++
include/kvm/arm_vgic.h | 2 +-
10 files changed, 29 insertions(+), 21 deletions(-)
--
2.23.0
We need a unified helper in 'kvm/vgic/vgic.h' to write guest memory. In
the helper, the check of no-running-vcpu context for dirty ring will be
applied. kvm_write_guest_lock(), defined in 'include/asm/kvm_mmu.h', is
going to be dereferenced by the unified helper.
Include 'include/asm/kvm_mmu.h' to 'kvm/vgic/vgic.h' to avoid including
the former header file when the later one is needed. With the change,
the duplicate inclusions of 'include/asm/kvm_mmu.h' are removed.
No functional change intended.
Suggested-by: Oliver Upton <[email protected]>
Signed-off-by: Gavin Shan <[email protected]>
---
arch/arm64/kvm/vgic/vgic-debug.c | 1 -
arch/arm64/kvm/vgic/vgic-init.c | 1 -
arch/arm64/kvm/vgic/vgic-its.c | 1 -
arch/arm64/kvm/vgic/vgic-kvm-device.c | 1 -
arch/arm64/kvm/vgic/vgic-mmio-v3.c | 1 -
arch/arm64/kvm/vgic/vgic-v2.c | 1 -
arch/arm64/kvm/vgic/vgic-v3.c | 1 -
arch/arm64/kvm/vgic/vgic.h | 1 +
8 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/arch/arm64/kvm/vgic/vgic-debug.c b/arch/arm64/kvm/vgic/vgic-debug.c
index 78cde687383c..69201c2dfc6c 100644
--- a/arch/arm64/kvm/vgic/vgic-debug.c
+++ b/arch/arm64/kvm/vgic/vgic-debug.c
@@ -10,7 +10,6 @@
#include <linux/kvm_host.h>
#include <linux/seq_file.h>
#include <kvm/arm_vgic.h>
-#include <asm/kvm_mmu.h>
#include "vgic.h"
/*
diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index f6d4f4052555..de389a5bec45 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -9,7 +9,6 @@
#include <linux/kvm_host.h>
#include <kvm/arm_vgic.h>
#include <asm/kvm_emulate.h>
-#include <asm/kvm_mmu.h>
#include "vgic.h"
/*
diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 94a666dd1443..ad4bb69ab83e 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -18,7 +18,6 @@
#include <asm/kvm_emulate.h>
#include <asm/kvm_arm.h>
-#include <asm/kvm_mmu.h>
#include "vgic.h"
#include "vgic-mmio.h"
diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
index edeac2380591..552668a91bd9 100644
--- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
+++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
@@ -8,7 +8,6 @@
#include <linux/kvm_host.h>
#include <kvm/arm_vgic.h>
#include <linux/uaccess.h>
-#include <asm/kvm_mmu.h>
#include <asm/cputype.h>
#include "vgic.h"
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 91201f743033..8ba04f4fa63d 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -13,7 +13,6 @@
#include <asm/kvm_emulate.h>
#include <asm/kvm_arm.h>
-#include <asm/kvm_mmu.h>
#include "vgic.h"
#include "vgic-mmio.h"
diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c
index 645648349c99..d8604fdfdfcd 100644
--- a/arch/arm64/kvm/vgic/vgic-v2.c
+++ b/arch/arm64/kvm/vgic/vgic-v2.c
@@ -7,7 +7,6 @@
#include <linux/kvm.h>
#include <linux/kvm_host.h>
#include <kvm/arm_vgic.h>
-#include <asm/kvm_mmu.h>
#include "vgic.h"
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 2074521d4a8c..5dfbd03e5e1a 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -7,7 +7,6 @@
#include <linux/kvm_host.h>
#include <kvm/arm_vgic.h>
#include <asm/kvm_hyp.h>
-#include <asm/kvm_mmu.h>
#include <asm/kvm_asm.h>
#include "vgic.h"
diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
index 0c8da72953f0..056425e3a490 100644
--- a/arch/arm64/kvm/vgic/vgic.h
+++ b/arch/arm64/kvm/vgic/vgic.h
@@ -6,6 +6,7 @@
#define __KVM_ARM_VGIC_NEW_H__
#include <linux/irqchip/arm-gic-common.h>
+#include <asm/kvm_mmu.h>
#define PRODUCT_ID_KVM 0x4b /* ASCII code K */
#define IMPLEMENTER_ARM 0x43b
--
2.23.0
Currently, the unknown no-running-vcpu sites are reported when a
dirty page is tracked by mark_page_dirty_in_slot(). Until now, the
only known no-running-vcpu site is saving vgic/its tables through
KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_SAVE_TABLES} command on KVM device
"kvm-arm-vgic-its". Unfortunately, there are more unknown sites to
be handled and no-running-vcpu context will be allowed in these
sites: (1) KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_RESTORE_TABLES} command
on KVM device "kvm-arm-vgic-its" to restore vgic/its tables. The
vgic3 LPI pending status could be restored. (2) Save vgic3 pending
table through KVM_DEV_ARM_{VGIC_GRP_CTRL, VGIC_SAVE_PENDING_TABLES}
command on KVM device "kvm-arm-vgic-v3".
In order to handle those unknown cases, we need a unified helper
vgic_write_guest_lock(). struct vgic_dist::save_its_tables_in_progress
is also renamed to struct vgic_dist::save_tables_in_progress.
No functional change intended.
Suggested-by: Oliver Upton <[email protected]>
Signed-off-by: Gavin Shan <[email protected]>
Reviewed-by: Oliver Upton <[email protected]>
---
arch/arm64/kvm/vgic/vgic-its.c | 13 +++++--------
arch/arm64/kvm/vgic/vgic.h | 13 +++++++++++++
include/kvm/arm_vgic.h | 2 +-
3 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index ad4bb69ab83e..887dfa6cc79d 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -2186,7 +2186,7 @@ static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
((u64)ite->irq->intid << KVM_ITS_ITE_PINTID_SHIFT) |
ite->collection->collection_id;
val = cpu_to_le64(val);
- return kvm_write_guest_lock(kvm, gpa, &val, ite_esz);
+ return vgic_write_guest_lock(kvm, gpa, &val, ite_esz);
}
/**
@@ -2338,7 +2338,7 @@ static int vgic_its_save_dte(struct vgic_its *its, struct its_device *dev,
(itt_addr_field << KVM_ITS_DTE_ITTADDR_SHIFT) |
(dev->num_eventid_bits - 1));
val = cpu_to_le64(val);
- return kvm_write_guest_lock(kvm, ptr, &val, dte_esz);
+ return vgic_write_guest_lock(kvm, ptr, &val, dte_esz);
}
/**
@@ -2525,7 +2525,7 @@ static int vgic_its_save_cte(struct vgic_its *its,
((u64)collection->target_addr << KVM_ITS_CTE_RDBASE_SHIFT) |
collection->collection_id);
val = cpu_to_le64(val);
- return kvm_write_guest_lock(its->dev->kvm, gpa, &val, esz);
+ return vgic_write_guest_lock(its->dev->kvm, gpa, &val, esz);
}
/*
@@ -2606,7 +2606,7 @@ static int vgic_its_save_collection_table(struct vgic_its *its)
*/
val = 0;
BUG_ON(cte_esz > sizeof(val));
- ret = kvm_write_guest_lock(its->dev->kvm, gpa, &val, cte_esz);
+ ret = vgic_write_guest_lock(its->dev->kvm, gpa, &val, cte_esz);
return ret;
}
@@ -2742,7 +2742,6 @@ static int vgic_its_has_attr(struct kvm_device *dev,
static int vgic_its_ctrl(struct kvm *kvm, struct vgic_its *its, u64 attr)
{
const struct vgic_its_abi *abi = vgic_its_get_abi(its);
- struct vgic_dist *dist = &kvm->arch.vgic;
int ret = 0;
if (attr == KVM_DEV_ARM_VGIC_CTRL_INIT) /* Nothing to do */
@@ -2762,9 +2761,7 @@ static int vgic_its_ctrl(struct kvm *kvm, struct vgic_its *its, u64 attr)
vgic_its_reset(kvm, its);
break;
case KVM_DEV_ARM_ITS_SAVE_TABLES:
- dist->save_its_tables_in_progress = true;
ret = abi->save_tables(its);
- dist->save_its_tables_in_progress = false;
break;
case KVM_DEV_ARM_ITS_RESTORE_TABLES:
ret = abi->restore_tables(its);
@@ -2791,7 +2788,7 @@ bool kvm_arch_allow_write_without_running_vcpu(struct kvm *kvm)
{
struct vgic_dist *dist = &kvm->arch.vgic;
- return dist->save_its_tables_in_progress;
+ return dist->save_tables_in_progress;
}
static int vgic_its_set_attr(struct kvm_device *dev,
diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
index 056425e3a490..3804b3e946fd 100644
--- a/arch/arm64/kvm/vgic/vgic.h
+++ b/arch/arm64/kvm/vgic/vgic.h
@@ -132,6 +132,19 @@ static inline bool vgic_irq_is_multi_sgi(struct vgic_irq *irq)
return vgic_irq_get_lr_count(irq) > 1;
}
+static inline int vgic_write_guest_lock(struct kvm *kvm, gpa_t gpa,
+ const void *data, unsigned long len)
+{
+ struct vgic_dist *dist = &kvm->arch.vgic;
+ int ret;
+
+ dist->save_tables_in_progress = true;
+ ret = kvm_write_guest_lock(kvm, gpa, data, len);
+ dist->save_tables_in_progress = false;
+
+ return ret;
+}
+
/*
* This struct provides an intermediate representation of the fields contained
* in the GICH_VMCR and ICH_VMCR registers, such that code exporting the GIC
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 9270cd87da3f..a0be53bc5703 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -263,7 +263,7 @@ struct vgic_dist {
struct vgic_io_device dist_iodev;
bool has_its;
- bool save_its_tables_in_progress;
+ bool save_tables_in_progress;
/*
* Contains the attributes and gpa of the LPI configuration table.
--
2.23.0
We don't have a running VCPU context to restore vgic3 LPI pending status
due to command KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_RESTORE_TABLES} on KVM
device "kvm-arm-vgic-its".
Use vgic_write_guest_lock() to restore vgic3 LPI pending status.
Signed-off-by: Gavin Shan <[email protected]>
Reviewed-by: Oliver Upton <[email protected]>
---
Documentation/virt/kvm/api.rst | 8 +++++---
arch/arm64/kvm/vgic/vgic-v3.c | 2 +-
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 9807b05a1b57..40ada313faa3 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -8070,9 +8070,11 @@ considering the state as complete. VMM needs to ensure that the dirty
state is final and avoid missing dirty pages from another ioctl ordered
after the bitmap collection.
-NOTE: One example of using the backup bitmap is saving arm64 vgic/its
-tables through KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_SAVE_TABLES} command on
-KVM device "kvm-arm-vgic-its" when dirty ring is enabled.
+NOTE: Multiple examples of using the backup bitmap: (1) save vgic/its
+tables through command KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_SAVE_TABLES} on
+KVM device "kvm-arm-vgic-its". (2) restore vgic/its tables through
+command KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_RESTORE_TABLES} on KVM device
+"kvm-arm-vgic-its". vgic3 LPI pending status is restored.
8.30 KVM_CAP_XEN_HVM
--------------------
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 5dfbd03e5e1a..c94e4d7520fc 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -338,7 +338,7 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq)
if (status) {
/* clear consumed data */
val &= ~(1 << bit_nr);
- ret = kvm_write_guest_lock(kvm, ptr, &val, 1);
+ ret = vgic_write_guest_lock(kvm, ptr, &val, 1);
if (ret)
return ret;
}
--
2.23.0
We don't have a running VCPU context to save vgic3 pending table due
to KVM_DEV_ARM_VGIC_{GRP_CTRL, SAVE_PENDING_TABLES} command on KVM
device "kvm-arm-vgic-v3". The unknown case is caught by kvm-unit-tests.
# ./kvm-unit-tests/tests/its-pending-migration
WARNING: CPU: 120 PID: 7973 at arch/arm64/kvm/../../../virt/kvm/kvm_main.c:3325 \
mark_page_dirty_in_slot+0x60/0xe0
:
mark_page_dirty_in_slot+0x60/0xe0
__kvm_write_guest_page+0xcc/0x100
kvm_write_guest+0x7c/0xb0
vgic_v3_save_pending_tables+0x148/0x2a0
vgic_set_common_attr+0x158/0x240
vgic_v3_set_attr+0x4c/0x5c
kvm_device_ioctl+0x100/0x160
__arm64_sys_ioctl+0xa8/0xf0
invoke_syscall.constprop.0+0x7c/0xd0
el0_svc_common.constprop.0+0x144/0x160
do_el0_svc+0x34/0x60
el0_svc+0x3c/0x1a0
el0t_64_sync_handler+0xb4/0x130
el0t_64_sync+0x178/0x17c
Use vgic_write_guest_lock() to save vgic3 pending table.
Reported-by: Zenghui Yu <[email protected]>
Signed-off-by: Gavin Shan <[email protected]>
Reviewed-by: Oliver Upton <[email protected]>
---
Documentation/virt/kvm/api.rst | 4 +++-
arch/arm64/kvm/vgic/vgic-v3.c | 2 +-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 40ada313faa3..07f07668995e 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -8074,7 +8074,9 @@ NOTE: Multiple examples of using the backup bitmap: (1) save vgic/its
tables through command KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_SAVE_TABLES} on
KVM device "kvm-arm-vgic-its". (2) restore vgic/its tables through
command KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_RESTORE_TABLES} on KVM device
-"kvm-arm-vgic-its". vgic3 LPI pending status is restored.
+"kvm-arm-vgic-its". vgic3 LPI pending status is restored. (3) save
+vgic3 pending table through KVM_DEV_ARM_VGIC_{GRP_CTRL, SAVE_PENDING_TABLES}
+command on KVM device "kvm-arm-vgic-v3".
8.30 KVM_CAP_XEN_HVM
--------------------
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index c94e4d7520fc..558ccc805fff 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -436,7 +436,7 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
else
val &= ~(1 << bit_nr);
- ret = kvm_write_guest_lock(kvm, ptr, &val, 1);
+ ret = vgic_write_guest_lock(kvm, ptr, &val, 1);
if (ret)
goto out;
}
--
2.23.0
On 2023/1/27 07:54, Gavin Shan wrote:
> We don't have a running VCPU context to save vgic3 pending table due
> to KVM_DEV_ARM_VGIC_{GRP_CTRL, SAVE_PENDING_TABLES} command on KVM
> device "kvm-arm-vgic-v3". The unknown case is caught by kvm-unit-tests.
>
> # ./kvm-unit-tests/tests/its-pending-migration
> WARNING: CPU: 120 PID: 7973 at arch/arm64/kvm/../../../virt/kvm/kvm_main.c:3325 \
> mark_page_dirty_in_slot+0x60/0xe0
> :
> mark_page_dirty_in_slot+0x60/0xe0
> __kvm_write_guest_page+0xcc/0x100
> kvm_write_guest+0x7c/0xb0
> vgic_v3_save_pending_tables+0x148/0x2a0
> vgic_set_common_attr+0x158/0x240
> vgic_v3_set_attr+0x4c/0x5c
> kvm_device_ioctl+0x100/0x160
> __arm64_sys_ioctl+0xa8/0xf0
> invoke_syscall.constprop.0+0x7c/0xd0
> el0_svc_common.constprop.0+0x144/0x160
> do_el0_svc+0x34/0x60
> el0_svc+0x3c/0x1a0
> el0t_64_sync_handler+0xb4/0x130
> el0t_64_sync+0x178/0x17c
>
> Use vgic_write_guest_lock() to save vgic3 pending table.
>
> Reported-by: Zenghui Yu <[email protected]>
> Signed-off-by: Gavin Shan <[email protected]>
> Reviewed-by: Oliver Upton <[email protected]>
> ---
> Documentation/virt/kvm/api.rst | 4 +++-
> arch/arm64/kvm/vgic/vgic-v3.c | 2 +-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 40ada313faa3..07f07668995e 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -8074,7 +8074,9 @@ NOTE: Multiple examples of using the backup bitmap: (1) save vgic/its
> tables through command KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_SAVE_TABLES} on
> KVM device "kvm-arm-vgic-its". (2) restore vgic/its tables through
> command KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_RESTORE_TABLES} on KVM device
> -"kvm-arm-vgic-its". vgic3 LPI pending status is restored.
> +"kvm-arm-vgic-its". vgic3 LPI pending status is restored. (3) save
> +vgic3 pending table through KVM_DEV_ARM_VGIC_{GRP_CTRL, SAVE_PENDING_TABLES}
> +command on KVM device "kvm-arm-vgic-v3".
Can we summarize these 3 examples with something like: "when the guest
memory (pending tables, ITS tables, etc) is dirtied by the virtual GIC
or ITS, which is typically triggered by a userspace request (e.g.,
KVM_DEV_ARM_ITS_SAVE_TABLES) and doesn't require a running VCPU
context"? In case there will be more no-running-vcpu
kvm_write_guest_lock() cases in the VGIC emulation code in future and we
have to extend the documentation..
But I don't have objection to your writing and the whole series looks
good.
Thanks,
Zenghui
[ just coming back from holiday, sorry for the late reply ]
On 2023/1/27 07:54, Gavin Shan wrote:
> Currently, the unknown no-running-vcpu sites are reported when a
> dirty page is tracked by mark_page_dirty_in_slot(). Until now, the
> only known no-running-vcpu site is saving vgic/its tables through
> KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_SAVE_TABLES} command on KVM device
> "kvm-arm-vgic-its". Unfortunately, there are more unknown sites to
> be handled and no-running-vcpu context will be allowed in these
> sites: (1) KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_RESTORE_TABLES} command
> on KVM device "kvm-arm-vgic-its" to restore vgic/its tables. The
> vgic3 LPI pending status could be restored. (2) Save vgic3 pending
We typically write it as "VGICv3".
> table through KVM_DEV_ARM_{VGIC_GRP_CTRL, VGIC_SAVE_PENDING_TABLES}
> command on KVM device "kvm-arm-vgic-v3".
>
> In order to handle those unknown cases, we need a unified helper
> vgic_write_guest_lock(). struct vgic_dist::save_its_tables_in_progress
> is also renamed to struct vgic_dist::save_tables_in_progress.
How about renaming it to 'write_tables_in_progress' which would look a
bit more generic? The rest looks good to me.
Thanks,
Zenghui
Hi Zenghui,
On 1/28/23 2:57 AM, Zenghui Yu wrote:
> [ just coming back from holiday, sorry for the late reply ]
>
Hope you have a nice refresh. Thanks for your review.
> On 2023/1/27 07:54, Gavin Shan wrote:
>> Currently, the unknown no-running-vcpu sites are reported when a
>> dirty page is tracked by mark_page_dirty_in_slot(). Until now, the
>> only known no-running-vcpu site is saving vgic/its tables through
>> KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_SAVE_TABLES} command on KVM device
>> "kvm-arm-vgic-its". Unfortunately, there are more unknown sites to
>> be handled and no-running-vcpu context will be allowed in these
>> sites: (1) KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_RESTORE_TABLES} command
>> on KVM device "kvm-arm-vgic-its" to restore vgic/its tables. The
>> vgic3 LPI pending status could be restored. (2) Save vgic3 pending
>
> We typically write it as "VGICv3".
>
Ok. I will fix by replacing 'vgic3' with 'VGICv3' in v4. However, the
term 'vgic/its' will be kept.
>> table through KVM_DEV_ARM_{VGIC_GRP_CTRL, VGIC_SAVE_PENDING_TABLES}
>> command on KVM device "kvm-arm-vgic-v3".
>>
>> In order to handle those unknown cases, we need a unified helper
>> vgic_write_guest_lock(). struct vgic_dist::save_its_tables_in_progress
>> is also renamed to struct vgic_dist::save_tables_in_progress.
>
> How about renaming it to 'write_tables_in_progress' which would look a
> bit more generic? The rest looks good to me.
>
'write_tables_in_progress' works for me. I will have it in v4, which
will be posted shortly.
Thanks,
Gavin
Hi Zenghui,
On 1/28/23 2:57 AM, Zenghui Yu wrote:
> On 2023/1/27 07:54, Gavin Shan wrote:
>> We don't have a running VCPU context to save vgic3 pending table due
>> to KVM_DEV_ARM_VGIC_{GRP_CTRL, SAVE_PENDING_TABLES} command on KVM
>> device "kvm-arm-vgic-v3". The unknown case is caught by kvm-unit-tests.
>>
>> # ./kvm-unit-tests/tests/its-pending-migration
>> WARNING: CPU: 120 PID: 7973 at arch/arm64/kvm/../../../virt/kvm/kvm_main.c:3325 \
>> mark_page_dirty_in_slot+0x60/0xe0
>> :
>> mark_page_dirty_in_slot+0x60/0xe0
>> __kvm_write_guest_page+0xcc/0x100
>> kvm_write_guest+0x7c/0xb0
>> vgic_v3_save_pending_tables+0x148/0x2a0
>> vgic_set_common_attr+0x158/0x240
>> vgic_v3_set_attr+0x4c/0x5c
>> kvm_device_ioctl+0x100/0x160
>> __arm64_sys_ioctl+0xa8/0xf0
>> invoke_syscall.constprop.0+0x7c/0xd0
>> el0_svc_common.constprop.0+0x144/0x160
>> do_el0_svc+0x34/0x60
>> el0_svc+0x3c/0x1a0
>> el0t_64_sync_handler+0xb4/0x130
>> el0t_64_sync+0x178/0x17c
>>
>> Use vgic_write_guest_lock() to save vgic3 pending table.
>>
>> Reported-by: Zenghui Yu <[email protected]>
>> Signed-off-by: Gavin Shan <[email protected]>
>> Reviewed-by: Oliver Upton <[email protected]>
>> ---
>> Documentation/virt/kvm/api.rst | 4 +++-
>> arch/arm64/kvm/vgic/vgic-v3.c | 2 +-
>> 2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
>> index 40ada313faa3..07f07668995e 100644
>> --- a/Documentation/virt/kvm/api.rst
>> +++ b/Documentation/virt/kvm/api.rst
>> @@ -8074,7 +8074,9 @@ NOTE: Multiple examples of using the backup bitmap: (1) save vgic/its
>> tables through command KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_SAVE_TABLES} on
>> KVM device "kvm-arm-vgic-its". (2) restore vgic/its tables through
>> command KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_RESTORE_TABLES} on KVM device
>> -"kvm-arm-vgic-its". vgic3 LPI pending status is restored.
>> +"kvm-arm-vgic-its". vgic3 LPI pending status is restored. (3) save
>> +vgic3 pending table through KVM_DEV_ARM_VGIC_{GRP_CTRL, SAVE_PENDING_TABLES}
>> +command on KVM device "kvm-arm-vgic-v3".
>
> Can we summarize these 3 examples with something like: "when the guest
> memory (pending tables, ITS tables, etc) is dirtied by the virtual GIC
> or ITS, which is typically triggered by a userspace request (e.g.,
> KVM_DEV_ARM_ITS_SAVE_TABLES) and doesn't require a running VCPU
> context"? In case there will be more no-running-vcpu
> kvm_write_guest_lock() cases in the VGIC emulation code in future and we
> have to extend the documentation..
>
> But I don't have objection to your writing and the whole series looks
> good.
>
There are discussions about the documentation when dirty ring is enabled
on ARM64. We prefer to keep the layout where the KVM devices and commands
are explicitly documented. The application developer can identify them
easily and to enable the backup bitmap when those KVM devices have been
used.
By the way, 'vgic3' will be replaced with 'VGICv3' as you suggested in
another reply.
Thanks,
Gavin
On Fri, 27 Jan 2023 07:54:47 +0800, Gavin Shan wrote:
> It has been known case where no running VCPU context exists when the
> vgic/its tables are saved. There are other two unknown cases where we
> don't have the running VCPU context: (a) restore vgic3 LPI pending
> status. (b) restoring vgic3 pending tables.
>
> PATCH[1] includes 'kvm_mmu.h' to 'vgic.h'
> PATCH[2] adds unified helper vgic_write_guest_lock()
> PATCH[3 - 4] allows no-running-vcpu context for (a) and (b)
>
> [...]
Applied to fixes, thanks!
[2/4] KVM: arm64: Add helper vgic_write_guest_lock()
commit: a23eaf9368aafa4defcc8904b20391b6ea07bb1e
[3/4] KVM: arm64: Allow no running vcpu on restoring vgic3 LPI pending status
commit: 2f8b1ad2228a7f1f1e2458864f4bfc1cbdf511ed
[4/4] KVM: arm64: Allow no running vcpu on saving vgic3 pending table
commit: 6028acbe3a5f2119a2a6ddd3e06453c87c09cae0
Cheers,
M.
--
Without deviation from the norm, progress is not possible.