[This series is RFC because I don't have MIPS to compile and test]
kvm_flush_remote_tlbs() can be arch-specific, by either:
- Completely replace kvm_flush_remote_tlbs(), like ARM, who is the
only user of CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL so far
- Doing something extra before kvm_flush_remote_tlbs(), like MIPS VZ
support, however still wants to have the common tlb flush to be part
of the process. Could refer to kvm_vz_flush_shadow_all(). Then in
MIPS it's awkward to flush remote TLBs: we'll need to call the mips
hooks.
It's awkward to have different ways to specialize this procedure,
especially MIPS cannot use the genenal interface which is quite a
pity. It's good to make it a common interface.
This patch series removes the 2nd MIPS usage above, and let it also
use the common kvm_flush_remote_tlbs() interface. It should be
suggested that we always keep kvm_flush_remote_tlbs() be a common
entrance for tlb flushing on all archs.
This idea comes from the reading of Sean's patchset on dynamic memslot
allocation, where a new dirty log specific hook is added for flushing
TLBs only for the MIPS code [1]. With this patchset, logically the
new hook in that patch can be dropped so we can directly use
kvm_flush_remote_tlbs().
TODO: We can even extend another common interface for ranged TLB, but
let's see how we think about this series first.
Any comment is welcomed, thanks.
Peter Xu (4):
KVM: Provide kvm_flush_remote_tlbs_common()
KVM: MIPS: Drop flush_shadow_memslot() callback
KVM: MIPS: Replace all the kvm_flush_remote_tlbs() references
KVM: MIPS: Define arch-specific kvm_flush_remote_tlbs()
arch/mips/include/asm/kvm_host.h | 7 -------
arch/mips/kvm/Kconfig | 1 +
arch/mips/kvm/mips.c | 22 ++++++++++------------
arch/mips/kvm/trap_emul.c | 15 +--------------
arch/mips/kvm/vz.c | 14 ++------------
include/linux/kvm_host.h | 1 +
virt/kvm/kvm_main.c | 10 ++++++++--
7 files changed, 23 insertions(+), 47 deletions(-)
--
2.24.1
Replace kvm_flush_remote_tlbs() calls in MIPS code into
kvm_flush_remote_tlbs_common(). This is to prepare that MIPS will
define its own kvm_flush_remote_tlbs() soon.
The only three references are all in the flush_shadow_all() hooks.
One of them can be directly dropped because it's exactly the
kvm_flush_remote_tlbs_common(). Since at it, refactors the other one
a bit.
No functional change expected.
Signed-off-by: Peter Xu <[email protected]>
---
arch/mips/kvm/trap_emul.c | 8 +-------
arch/mips/kvm/vz.c | 7 ++-----
2 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/arch/mips/kvm/trap_emul.c b/arch/mips/kvm/trap_emul.c
index 2ecb430ea0f1..ced481c963be 100644
--- a/arch/mips/kvm/trap_emul.c
+++ b/arch/mips/kvm/trap_emul.c
@@ -697,12 +697,6 @@ static int kvm_trap_emul_vcpu_setup(struct kvm_vcpu *vcpu)
return 0;
}
-static void kvm_trap_emul_flush_shadow_all(struct kvm *kvm)
-{
- /* Flush GVA page tables and invalidate GVA ASIDs on all VCPUs */
- kvm_flush_remote_tlbs(kvm);
-}
-
static u64 kvm_trap_emul_get_one_regs[] = {
KVM_REG_MIPS_CP0_INDEX,
KVM_REG_MIPS_CP0_ENTRYLO0,
@@ -1285,7 +1279,7 @@ static struct kvm_mips_callbacks kvm_trap_emul_callbacks = {
.vcpu_init = kvm_trap_emul_vcpu_init,
.vcpu_uninit = kvm_trap_emul_vcpu_uninit,
.vcpu_setup = kvm_trap_emul_vcpu_setup,
- .flush_shadow_all = kvm_trap_emul_flush_shadow_all,
+ .flush_shadow_all = kvm_flush_remote_tlbs_common,
.gva_to_gpa = kvm_trap_emul_gva_to_gpa_cb,
.queue_timer_int = kvm_mips_queue_timer_int_cb,
.dequeue_timer_int = kvm_mips_dequeue_timer_int_cb,
diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c
index 814bd1564a79..91fbf6710da4 100644
--- a/arch/mips/kvm/vz.c
+++ b/arch/mips/kvm/vz.c
@@ -3105,10 +3105,7 @@ static int kvm_vz_vcpu_setup(struct kvm_vcpu *vcpu)
static void kvm_vz_flush_shadow_all(struct kvm *kvm)
{
- if (cpu_has_guestid) {
- /* Flush GuestID for each VCPU individually */
- kvm_flush_remote_tlbs(kvm);
- } else {
+ if (!cpu_has_guestid) {
/*
* For each CPU there is a single GPA ASID used by all VCPUs in
* the VM, so it doesn't make sense for the VCPUs to handle
@@ -3119,8 +3116,8 @@ static void kvm_vz_flush_shadow_all(struct kvm *kvm)
* kick any running VCPUs so they check asid_flush_mask.
*/
cpumask_setall(&kvm->arch.asid_flush_mask);
- kvm_flush_remote_tlbs(kvm);
}
+ kvm_flush_remote_tlbs_common(kvm);
}
static void kvm_vz_vcpu_reenter(struct kvm_run *run, struct kvm_vcpu *vcpu)
--
2.24.1
Select HAVE_KVM_ARCH_TLB_FLUSH_ALL for MIPS to define its own
kvm_flush_remote_tlbs(). It's as simple as calling the
flush_shadow_all(kvm) hook. Then replace all the flush_shadow_all()
calls to use this KVM generic API to do TLB flush.
Signed-off-by: Peter Xu <[email protected]>
---
arch/mips/kvm/Kconfig | 1 +
arch/mips/kvm/mips.c | 22 ++++++++++------------
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/arch/mips/kvm/Kconfig b/arch/mips/kvm/Kconfig
index eac25aef21e0..8a06f660d39e 100644
--- a/arch/mips/kvm/Kconfig
+++ b/arch/mips/kvm/Kconfig
@@ -26,6 +26,7 @@ config KVM
select KVM_MMIO
select MMU_NOTIFIER
select SRCU
+ select HAVE_KVM_ARCH_TLB_FLUSH_ALL
---help---
Support for hosting Guest kernels.
diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
index 1d5e7ffda746..518020b466bf 100644
--- a/arch/mips/kvm/mips.c
+++ b/arch/mips/kvm/mips.c
@@ -194,13 +194,16 @@ int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot,
return 0;
}
+void kvm_flush_remote_tlbs(struct kvm *kvm)
+{
+ kvm_mips_callbacks->flush_shadow_all(kvm);
+}
+
void kvm_arch_flush_shadow_all(struct kvm *kvm)
{
/* Flush whole GPA */
kvm_mips_flush_gpa_pt(kvm, 0, ~0);
-
- /* Let implementation do the rest */
- kvm_mips_callbacks->flush_shadow_all(kvm);
+ kvm_flush_remote_tlbs(kvm);
}
void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
@@ -215,8 +218,7 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
/* Flush slot from GPA */
kvm_mips_flush_gpa_pt(kvm, slot->base_gfn,
slot->base_gfn + slot->npages - 1);
- /* Let implementation do the rest */
- kvm_mips_callbacks->flush_shadow_all(kvm);
+ kvm_flush_remote_tlbs(kvm);
spin_unlock(&kvm->mmu_lock);
}
@@ -258,7 +260,7 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
new->base_gfn + new->npages - 1);
/* Let implementation do the rest */
if (needs_flush)
- kvm_mips_callbacks->flush_shadow_all(kvm);
+ kvm_flush_remote_tlbs(kvm);
spin_unlock(&kvm->mmu_lock);
}
}
@@ -1001,9 +1003,7 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
if (flush) {
slots = kvm_memslots(kvm);
memslot = id_to_memslot(slots, log->slot);
-
- /* Let implementation handle TLB/GVA invalidation */
- kvm_mips_callbacks->flush_shadow_all(kvm);
+ kvm_flush_remote_tlbs(kvm);
}
mutex_unlock(&kvm->slots_lock);
@@ -1024,9 +1024,7 @@ int kvm_vm_ioctl_clear_dirty_log(struct kvm *kvm, struct kvm_clear_dirty_log *lo
if (flush) {
slots = kvm_memslots(kvm);
memslot = id_to_memslot(slots, log->slot);
-
- /* Let implementation handle TLB/GVA invalidation */
- kvm_mips_callbacks->flush_shadow_all(kvm);
+ kvm_flush_remote_tlbs(kvm);
}
mutex_unlock(&kvm->slots_lock);
--
2.24.1
It's exactly kvm_flush_remote_tlbs() now but a internal wrapper of the
common code path. With this, an arch can then optionally select
CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL=y and will be able to use the
common flushing code.
Signed-off-by: Peter Xu <[email protected]>
---
include/linux/kvm_host.h | 1 +
virt/kvm/kvm_main.c | 10 ++++++++--
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 6d5331b0d937..915df64125f9 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -798,6 +798,7 @@ int kvm_vcpu_yield_to(struct kvm_vcpu *target);
void kvm_vcpu_on_spin(struct kvm_vcpu *vcpu, bool usermode_vcpu_not_eligible);
void kvm_flush_remote_tlbs(struct kvm *kvm);
+void kvm_flush_remote_tlbs_common(struct kvm *kvm);
void kvm_reload_remote_mmus(struct kvm *kvm);
bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req,
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index eb3709d55139..9c7b39b7bb21 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -302,8 +302,7 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
return called;
}
-#ifndef CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL
-void kvm_flush_remote_tlbs(struct kvm *kvm)
+void kvm_flush_remote_tlbs_common(struct kvm *kvm)
{
/*
* Read tlbs_dirty before setting KVM_REQ_TLB_FLUSH in
@@ -327,6 +326,13 @@ void kvm_flush_remote_tlbs(struct kvm *kvm)
++kvm->stat.remote_tlb_flush;
cmpxchg(&kvm->tlbs_dirty, dirty_count, 0);
}
+EXPORT_SYMBOL_GPL(kvm_flush_remote_tlbs_common);
+
+#ifndef CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL
+void kvm_flush_remote_tlbs(struct kvm *kvm)
+{
+ kvm_flush_remote_tlbs_common(kvm);
+}
EXPORT_SYMBOL_GPL(kvm_flush_remote_tlbs);
#endif
--
2.24.1
The MIPS flush_shadow_memslot() callback is always calling the
flush_shadow_all() implementation no matter for trap-emul or VZ.
Delete it and call flush_shadow_all() instead.
This patch prepares for a further replacement of letting MIPS to use
the common kvm_flush_remote_tlbs() call in all places.
No functional change expected.
Signed-off-by: Peter Xu <[email protected]>
---
arch/mips/include/asm/kvm_host.h | 7 -------
arch/mips/kvm/mips.c | 8 ++++----
arch/mips/kvm/trap_emul.c | 7 -------
arch/mips/kvm/vz.c | 7 -------
4 files changed, 4 insertions(+), 25 deletions(-)
diff --git a/arch/mips/include/asm/kvm_host.h b/arch/mips/include/asm/kvm_host.h
index 41204a49cf95..e95faffb23d8 100644
--- a/arch/mips/include/asm/kvm_host.h
+++ b/arch/mips/include/asm/kvm_host.h
@@ -786,13 +786,6 @@ struct kvm_mips_callbacks {
void (*vcpu_uninit)(struct kvm_vcpu *vcpu);
int (*vcpu_setup)(struct kvm_vcpu *vcpu);
void (*flush_shadow_all)(struct kvm *kvm);
- /*
- * Must take care of flushing any cached GPA PTEs (e.g. guest entries in
- * VZ root TLB, or T&E GVA page tables and corresponding root TLB
- * mappings).
- */
- void (*flush_shadow_memslot)(struct kvm *kvm,
- const struct kvm_memory_slot *slot);
gpa_t (*gva_to_gpa)(gva_t gva);
void (*queue_timer_int)(struct kvm_vcpu *vcpu);
void (*dequeue_timer_int)(struct kvm_vcpu *vcpu);
diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
index 2606f3f02b54..1d5e7ffda746 100644
--- a/arch/mips/kvm/mips.c
+++ b/arch/mips/kvm/mips.c
@@ -216,7 +216,7 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
kvm_mips_flush_gpa_pt(kvm, slot->base_gfn,
slot->base_gfn + slot->npages - 1);
/* Let implementation do the rest */
- kvm_mips_callbacks->flush_shadow_memslot(kvm, slot);
+ kvm_mips_callbacks->flush_shadow_all(kvm);
spin_unlock(&kvm->mmu_lock);
}
@@ -258,7 +258,7 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
new->base_gfn + new->npages - 1);
/* Let implementation do the rest */
if (needs_flush)
- kvm_mips_callbacks->flush_shadow_memslot(kvm, new);
+ kvm_mips_callbacks->flush_shadow_all(kvm);
spin_unlock(&kvm->mmu_lock);
}
}
@@ -1003,7 +1003,7 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
memslot = id_to_memslot(slots, log->slot);
/* Let implementation handle TLB/GVA invalidation */
- kvm_mips_callbacks->flush_shadow_memslot(kvm, memslot);
+ kvm_mips_callbacks->flush_shadow_all(kvm);
}
mutex_unlock(&kvm->slots_lock);
@@ -1026,7 +1026,7 @@ int kvm_vm_ioctl_clear_dirty_log(struct kvm *kvm, struct kvm_clear_dirty_log *lo
memslot = id_to_memslot(slots, log->slot);
/* Let implementation handle TLB/GVA invalidation */
- kvm_mips_callbacks->flush_shadow_memslot(kvm, memslot);
+ kvm_mips_callbacks->flush_shadow_all(kvm);
}
mutex_unlock(&kvm->slots_lock);
diff --git a/arch/mips/kvm/trap_emul.c b/arch/mips/kvm/trap_emul.c
index 5a11e83dffe6..2ecb430ea0f1 100644
--- a/arch/mips/kvm/trap_emul.c
+++ b/arch/mips/kvm/trap_emul.c
@@ -703,12 +703,6 @@ static void kvm_trap_emul_flush_shadow_all(struct kvm *kvm)
kvm_flush_remote_tlbs(kvm);
}
-static void kvm_trap_emul_flush_shadow_memslot(struct kvm *kvm,
- const struct kvm_memory_slot *slot)
-{
- kvm_trap_emul_flush_shadow_all(kvm);
-}
-
static u64 kvm_trap_emul_get_one_regs[] = {
KVM_REG_MIPS_CP0_INDEX,
KVM_REG_MIPS_CP0_ENTRYLO0,
@@ -1292,7 +1286,6 @@ static struct kvm_mips_callbacks kvm_trap_emul_callbacks = {
.vcpu_uninit = kvm_trap_emul_vcpu_uninit,
.vcpu_setup = kvm_trap_emul_vcpu_setup,
.flush_shadow_all = kvm_trap_emul_flush_shadow_all,
- .flush_shadow_memslot = kvm_trap_emul_flush_shadow_memslot,
.gva_to_gpa = kvm_trap_emul_gva_to_gpa_cb,
.queue_timer_int = kvm_mips_queue_timer_int_cb,
.dequeue_timer_int = kvm_mips_dequeue_timer_int_cb,
diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c
index dde20887a70d..814bd1564a79 100644
--- a/arch/mips/kvm/vz.c
+++ b/arch/mips/kvm/vz.c
@@ -3123,12 +3123,6 @@ static void kvm_vz_flush_shadow_all(struct kvm *kvm)
}
}
-static void kvm_vz_flush_shadow_memslot(struct kvm *kvm,
- const struct kvm_memory_slot *slot)
-{
- kvm_vz_flush_shadow_all(kvm);
-}
-
static void kvm_vz_vcpu_reenter(struct kvm_run *run, struct kvm_vcpu *vcpu)
{
int cpu = smp_processor_id();
@@ -3185,7 +3179,6 @@ static struct kvm_mips_callbacks kvm_vz_callbacks = {
.vcpu_uninit = kvm_vz_vcpu_uninit,
.vcpu_setup = kvm_vz_vcpu_setup,
.flush_shadow_all = kvm_vz_flush_shadow_all,
- .flush_shadow_memslot = kvm_vz_flush_shadow_memslot,
.gva_to_gpa = kvm_vz_gva_to_gpa_cb,
.queue_timer_int = kvm_vz_queue_timer_int_cb,
.dequeue_timer_int = kvm_vz_dequeue_timer_int_cb,
--
2.24.1
On Fri, Feb 07, 2020 at 05:35:16PM -0500, Peter Xu wrote:
> [This series is RFC because I don't have MIPS to compile and test]
>
> kvm_flush_remote_tlbs() can be arch-specific, by either:
>
> - Completely replace kvm_flush_remote_tlbs(), like ARM, who is the
> only user of CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL so far
>
> - Doing something extra before kvm_flush_remote_tlbs(), like MIPS VZ
> support, however still wants to have the common tlb flush to be part
> of the process. Could refer to kvm_vz_flush_shadow_all(). Then in
> MIPS it's awkward to flush remote TLBs: we'll need to call the mips
> hooks.
>
> It's awkward to have different ways to specialize this procedure,
> especially MIPS cannot use the genenal interface which is quite a
> pity. It's good to make it a common interface.
>
> This patch series removes the 2nd MIPS usage above, and let it also
> use the common kvm_flush_remote_tlbs() interface. It should be
> suggested that we always keep kvm_flush_remote_tlbs() be a common
> entrance for tlb flushing on all archs.
>
> This idea comes from the reading of Sean's patchset on dynamic memslot
> allocation, where a new dirty log specific hook is added for flushing
> TLBs only for the MIPS code [1]. With this patchset, logically the
[1] https://lore.kernel.org/kvm/[email protected]/T/#m2da733d75dab5e54e2ae68de94fe8411166d6274
> new hook in that patch can be dropped so we can directly use
> kvm_flush_remote_tlbs().
>
> TODO: We can even extend another common interface for ranged TLB, but
> let's see how we think about this series first.
>
> Any comment is welcomed, thanks.
>
> Peter Xu (4):
> KVM: Provide kvm_flush_remote_tlbs_common()
> KVM: MIPS: Drop flush_shadow_memslot() callback
> KVM: MIPS: Replace all the kvm_flush_remote_tlbs() references
> KVM: MIPS: Define arch-specific kvm_flush_remote_tlbs()
>
> arch/mips/include/asm/kvm_host.h | 7 -------
> arch/mips/kvm/Kconfig | 1 +
> arch/mips/kvm/mips.c | 22 ++++++++++------------
> arch/mips/kvm/trap_emul.c | 15 +--------------
> arch/mips/kvm/vz.c | 14 ++------------
> include/linux/kvm_host.h | 1 +
> virt/kvm/kvm_main.c | 10 ++++++++--
> 7 files changed, 23 insertions(+), 47 deletions(-)
>
> --
> 2.24.1
>
--
Peter Xu
On 07/02/20 23:35, Peter Xu wrote:
> [This series is RFC because I don't have MIPS to compile and test]
>
> kvm_flush_remote_tlbs() can be arch-specific, by either:
>
> - Completely replace kvm_flush_remote_tlbs(), like ARM, who is the
> only user of CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL so far
>
> - Doing something extra before kvm_flush_remote_tlbs(), like MIPS VZ
> support, however still wants to have the common tlb flush to be part
> of the process. Could refer to kvm_vz_flush_shadow_all(). Then in
> MIPS it's awkward to flush remote TLBs: we'll need to call the mips
> hooks.
>
> It's awkward to have different ways to specialize this procedure,
> especially MIPS cannot use the genenal interface which is quite a
> pity. It's good to make it a common interface.
>
> This patch series removes the 2nd MIPS usage above, and let it also
> use the common kvm_flush_remote_tlbs() interface. It should be
> suggested that we always keep kvm_flush_remote_tlbs() be a common
> entrance for tlb flushing on all archs.
>
> This idea comes from the reading of Sean's patchset on dynamic memslot
> allocation, where a new dirty log specific hook is added for flushing
> TLBs only for the MIPS code [1]. With this patchset, logically the
> new hook in that patch can be dropped so we can directly use
> kvm_flush_remote_tlbs().
>
> TODO: We can even extend another common interface for ranged TLB, but
> let's see how we think about this series first.
>
> Any comment is welcomed, thanks.
>
> Peter Xu (4):
> KVM: Provide kvm_flush_remote_tlbs_common()
> KVM: MIPS: Drop flush_shadow_memslot() callback
> KVM: MIPS: Replace all the kvm_flush_remote_tlbs() references
> KVM: MIPS: Define arch-specific kvm_flush_remote_tlbs()
>
> arch/mips/include/asm/kvm_host.h | 7 -------
> arch/mips/kvm/Kconfig | 1 +
> arch/mips/kvm/mips.c | 22 ++++++++++------------
> arch/mips/kvm/trap_emul.c | 15 +--------------
> arch/mips/kvm/vz.c | 14 ++------------
> include/linux/kvm_host.h | 1 +
> virt/kvm/kvm_main.c | 10 ++++++++--
> 7 files changed, 23 insertions(+), 47 deletions(-)
>
Compile-tested and queued.
MIPS folks, I see that arch/mips/kvm/mmu.c uses pud_index, so it's not
clear to me if it's meant to only work if CONFIG_PGTABLE_LEVELS=4 or
it's just bit rot. Should I add a "depends on PGTABLE_LEVEL=4" to
arch/mips/Kconfig?
Paolo
On 07/02/20 23:35, Peter Xu wrote:
> It's exactly kvm_flush_remote_tlbs() now but a internal wrapper of the
> common code path. With this, an arch can then optionally select
> CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL=y and will be able to use the
> common flushing code.
>
> Signed-off-by: Peter Xu <[email protected]>
Slightly more efficient, making it an inline function:
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index e89eb67356cb..f92180eeffc6 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -802,9 +802,18 @@ int kvm_vcpu_write_guest(struct kvm_vcpu *vcpu, gpa_t gpa, const void *data,
int kvm_vcpu_yield_to(struct kvm_vcpu *target);
void kvm_vcpu_on_spin(struct kvm_vcpu *vcpu, bool usermode_vcpu_not_eligible);
-void kvm_flush_remote_tlbs(struct kvm *kvm);
+void kvm_flush_remote_tlbs_common(struct kvm *kvm);
void kvm_reload_remote_mmus(struct kvm *kvm);
+#ifdef CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL
+void kvm_flush_remote_tlbs(struct kvm *kvm);
+#else
+static inline void kvm_flush_remote_tlbs(struct kvm *kvm)
+{
+ kvm_flush_remote_tlbs_common(kvm);
+}
+#endif
+
bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req,
unsigned long *vcpu_bitmap, cpumask_var_t tmp);
bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 70f03ce0e5c1..027259af883e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -303,8 +303,7 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
return called;
}
-#ifndef CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL
-void kvm_flush_remote_tlbs(struct kvm *kvm)
+void kvm_flush_remote_tlbs_common(struct kvm *kvm)
{
/*
* Read tlbs_dirty before setting KVM_REQ_TLB_FLUSH in
@@ -328,8 +327,7 @@ void kvm_flush_remote_tlbs(struct kvm *kvm)
++kvm->stat.remote_tlb_flush;
cmpxchg(&kvm->tlbs_dirty, dirty_count, 0);
}
-EXPORT_SYMBOL_GPL(kvm_flush_remote_tlbs);
-#endif
+EXPORT_SYMBOL_GPL(kvm_flush_remote_tlbs_common);
void kvm_reload_remote_mmus(struct kvm *kvm)
{
Hi Paolo,
On Wed, Feb 12, 2020 at 01:25:30PM +0100, Paolo Bonzini wrote:
> MIPS folks, I see that arch/mips/kvm/mmu.c uses pud_index, so it's not
> clear to me if it's meant to only work if CONFIG_PGTABLE_LEVELS=4 or
> it's just bit rot. Should I add a "depends on PGTABLE_LEVEL=4" to
> arch/mips/Kconfig?
I'm no expert on this bit of code, but I'm pretty sure the systems
KVM/VZ has been used on the most internally had PGTABLE_LEVEL=3.
I suspect this is actually a regression from commit 31168f033e37 ("mips:
drop __pXd_offset() macros that duplicate pXd_index() ones"). Whilst
that commit is correct that pud_index() & __pud_offset() are the same
when pud_index() is actually provided, it doesn't take into account the
__PAGETABLE_PUD_FOLDED case. There __pud_offset() was available but
would always evaluate to zero, whereas pud_index() isn't defined...
Thanks,
Paul
On 12/02/20 17:30, Paul Burton wrote:
> Hi Paolo,
>
> On Wed, Feb 12, 2020 at 01:25:30PM +0100, Paolo Bonzini wrote:
>> MIPS folks, I see that arch/mips/kvm/mmu.c uses pud_index, so it's not
>> clear to me if it's meant to only work if CONFIG_PGTABLE_LEVELS=4 or
>> it's just bit rot. Should I add a "depends on PGTABLE_LEVEL=4" to
>> arch/mips/Kconfig?
>
> I'm no expert on this bit of code, but I'm pretty sure the systems
> KVM/VZ has been used on the most internally had PGTABLE_LEVEL=3.
>
> I suspect this is actually a regression from commit 31168f033e37 ("mips:
> drop __pXd_offset() macros that duplicate pXd_index() ones"). Whilst
> that commit is correct that pud_index() & __pud_offset() are the same
> when pud_index() is actually provided, it doesn't take into account the
> __PAGETABLE_PUD_FOLDED case. There __pud_offset() was available but
> would always evaluate to zero, whereas pud_index() isn't defined...
Ok, I'll try to whip out a patch that handles __PAGETABLE_PUD_FOLDED.
On the other hand this makes me worry about how much KVM is being tested
by people that care about MIPS (even just compile-tested).
Paolo
On 2/12/20 5:40 PM, Paolo Bonzini wrote:
> On 12/02/20 17:30, Paul Burton wrote:
>> Hi Paolo,
>>
>> On Wed, Feb 12, 2020 at 01:25:30PM +0100, Paolo Bonzini wrote:
>>> MIPS folks, I see that arch/mips/kvm/mmu.c uses pud_index, so it's not
>>> clear to me if it's meant to only work if CONFIG_PGTABLE_LEVELS=4 or
>>> it's just bit rot. Should I add a "depends on PGTABLE_LEVEL=4" to
>>> arch/mips/Kconfig?
>>
>> I'm no expert on this bit of code, but I'm pretty sure the systems
>> KVM/VZ has been used on the most internally had PGTABLE_LEVEL=3.
>>
>> I suspect this is actually a regression from commit 31168f033e37 ("mips:
>> drop __pXd_offset() macros that duplicate pXd_index() ones"). Whilst
>> that commit is correct that pud_index() & __pud_offset() are the same
>> when pud_index() is actually provided, it doesn't take into account the
>> __PAGETABLE_PUD_FOLDED case. There __pud_offset() was available but
>> would always evaluate to zero, whereas pud_index() isn't defined...
>
> Ok, I'll try to whip out a patch that handles __PAGETABLE_PUD_FOLDED.
> On the other hand this makes me worry about how much KVM is being tested
> by people that care about MIPS (even just compile-tested).
FYI last time James confirmed he tested QEMU was in 2017:
https://www.mail-archive.com/[email protected]/msg477133.html
At the end of 2019 he orphaned the QEMU part:
https://www.mail-archive.com/[email protected]/msg667240.html
and dropped the kernel maintainance:
https://git.kernel.org/pub/scm/linux/kernel/git/mips/linux.git/commit/?id=9c48c48cd499
On Wed, Feb 12, 2020 at 01:25:30PM +0100, Paolo Bonzini wrote:
> On 07/02/20 23:35, Peter Xu wrote:
> > [This series is RFC because I don't have MIPS to compile and test]
> >
> > kvm_flush_remote_tlbs() can be arch-specific, by either:
> >
> > - Completely replace kvm_flush_remote_tlbs(), like ARM, who is the
> > only user of CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL so far
> >
> > - Doing something extra before kvm_flush_remote_tlbs(), like MIPS VZ
> > support, however still wants to have the common tlb flush to be part
> > of the process. Could refer to kvm_vz_flush_shadow_all(). Then in
> > MIPS it's awkward to flush remote TLBs: we'll need to call the mips
> > hooks.
> >
> > It's awkward to have different ways to specialize this procedure,
> > especially MIPS cannot use the genenal interface which is quite a
> > pity. It's good to make it a common interface.
> >
> > This patch series removes the 2nd MIPS usage above, and let it also
> > use the common kvm_flush_remote_tlbs() interface. It should be
> > suggested that we always keep kvm_flush_remote_tlbs() be a common
> > entrance for tlb flushing on all archs.
> >
> > This idea comes from the reading of Sean's patchset on dynamic memslot
> > allocation, where a new dirty log specific hook is added for flushing
> > TLBs only for the MIPS code [1]. With this patchset, logically the
> > new hook in that patch can be dropped so we can directly use
> > kvm_flush_remote_tlbs().
> >
> > TODO: We can even extend another common interface for ranged TLB, but
> > let's see how we think about this series first.
> >
> > Any comment is welcomed, thanks.
> >
> > Peter Xu (4):
> > KVM: Provide kvm_flush_remote_tlbs_common()
> > KVM: MIPS: Drop flush_shadow_memslot() callback
> > KVM: MIPS: Replace all the kvm_flush_remote_tlbs() references
> > KVM: MIPS: Define arch-specific kvm_flush_remote_tlbs()
> >
> > arch/mips/include/asm/kvm_host.h | 7 -------
> > arch/mips/kvm/Kconfig | 1 +
> > arch/mips/kvm/mips.c | 22 ++++++++++------------
> > arch/mips/kvm/trap_emul.c | 15 +--------------
> > arch/mips/kvm/vz.c | 14 ++------------
> > include/linux/kvm_host.h | 1 +
> > virt/kvm/kvm_main.c | 10 ++++++++--
> > 7 files changed, 23 insertions(+), 47 deletions(-)
> >
>
> Compile-tested and queued.
Just in case it fells through the crach - Paolo, do you still have
plan to queue this again?
Thanks,
--
Peter Xu
On 11/03/20 19:32, Peter Xu wrote:
> On Wed, Feb 12, 2020 at 01:25:30PM +0100, Paolo Bonzini wrote:
>> On 07/02/20 23:35, Peter Xu wrote:
>>> [This series is RFC because I don't have MIPS to compile and test]
>>>
>>> kvm_flush_remote_tlbs() can be arch-specific, by either:
>>>
>>> - Completely replace kvm_flush_remote_tlbs(), like ARM, who is the
>>> only user of CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL so far
>>>
>>> - Doing something extra before kvm_flush_remote_tlbs(), like MIPS VZ
>>> support, however still wants to have the common tlb flush to be part
>>> of the process. Could refer to kvm_vz_flush_shadow_all(). Then in
>>> MIPS it's awkward to flush remote TLBs: we'll need to call the mips
>>> hooks.
>>>
>>> It's awkward to have different ways to specialize this procedure,
>>> especially MIPS cannot use the genenal interface which is quite a
>>> pity. It's good to make it a common interface.
>>>
>>> This patch series removes the 2nd MIPS usage above, and let it also
>>> use the common kvm_flush_remote_tlbs() interface. It should be
>>> suggested that we always keep kvm_flush_remote_tlbs() be a common
>>> entrance for tlb flushing on all archs.
>>>
>>> This idea comes from the reading of Sean's patchset on dynamic memslot
>>> allocation, where a new dirty log specific hook is added for flushing
>>> TLBs only for the MIPS code [1]. With this patchset, logically the
>>> new hook in that patch can be dropped so we can directly use
>>> kvm_flush_remote_tlbs().
>>>
>>> TODO: We can even extend another common interface for ranged TLB, but
>>> let's see how we think about this series first.
>>>
>>> Any comment is welcomed, thanks.
>>>
>>> Peter Xu (4):
>>> KVM: Provide kvm_flush_remote_tlbs_common()
>>> KVM: MIPS: Drop flush_shadow_memslot() callback
>>> KVM: MIPS: Replace all the kvm_flush_remote_tlbs() references
>>> KVM: MIPS: Define arch-specific kvm_flush_remote_tlbs()
>>>
>>> arch/mips/include/asm/kvm_host.h | 7 -------
>>> arch/mips/kvm/Kconfig | 1 +
>>> arch/mips/kvm/mips.c | 22 ++++++++++------------
>>> arch/mips/kvm/trap_emul.c | 15 +--------------
>>> arch/mips/kvm/vz.c | 14 ++------------
>>> include/linux/kvm_host.h | 1 +
>>> virt/kvm/kvm_main.c | 10 ++++++++--
>>> 7 files changed, 23 insertions(+), 47 deletions(-)
>>>
>>
>> Compile-tested and queued.
>
> Just in case it fells through the crach - Paolo, do you still have
> plan to queue this again?
Yes, I wanted to make it compile first though. I'm undecided between
queuing your series and killing KVM MIPS honestly.
Paolo
On Tue, Mar 17, 2020 at 02:33:00PM +0100, Paolo Bonzini wrote:
> On 11/03/20 19:32, Peter Xu wrote:
> > On Wed, Feb 12, 2020 at 01:25:30PM +0100, Paolo Bonzini wrote:
> >> On 07/02/20 23:35, Peter Xu wrote:
> >>> [This series is RFC because I don't have MIPS to compile and test]
> >>>
> >>> kvm_flush_remote_tlbs() can be arch-specific, by either:
> >>>
> >>> - Completely replace kvm_flush_remote_tlbs(), like ARM, who is the
> >>> only user of CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL so far
> >>>
> >>> - Doing something extra before kvm_flush_remote_tlbs(), like MIPS VZ
> >>> support, however still wants to have the common tlb flush to be part
> >>> of the process. Could refer to kvm_vz_flush_shadow_all(). Then in
> >>> MIPS it's awkward to flush remote TLBs: we'll need to call the mips
> >>> hooks.
> >>>
> >>> It's awkward to have different ways to specialize this procedure,
> >>> especially MIPS cannot use the genenal interface which is quite a
> >>> pity. It's good to make it a common interface.
> >>>
> >>> This patch series removes the 2nd MIPS usage above, and let it also
> >>> use the common kvm_flush_remote_tlbs() interface. It should be
> >>> suggested that we always keep kvm_flush_remote_tlbs() be a common
> >>> entrance for tlb flushing on all archs.
> >>>
> >>> This idea comes from the reading of Sean's patchset on dynamic memslot
> >>> allocation, where a new dirty log specific hook is added for flushing
> >>> TLBs only for the MIPS code [1]. With this patchset, logically the
> >>> new hook in that patch can be dropped so we can directly use
> >>> kvm_flush_remote_tlbs().
> >>>
> >>> TODO: We can even extend another common interface for ranged TLB, but
> >>> let's see how we think about this series first.
> >>>
> >>> Any comment is welcomed, thanks.
> >>>
> >>> Peter Xu (4):
> >>> KVM: Provide kvm_flush_remote_tlbs_common()
> >>> KVM: MIPS: Drop flush_shadow_memslot() callback
> >>> KVM: MIPS: Replace all the kvm_flush_remote_tlbs() references
> >>> KVM: MIPS: Define arch-specific kvm_flush_remote_tlbs()
> >>>
> >>> arch/mips/include/asm/kvm_host.h | 7 -------
> >>> arch/mips/kvm/Kconfig | 1 +
> >>> arch/mips/kvm/mips.c | 22 ++++++++++------------
> >>> arch/mips/kvm/trap_emul.c | 15 +--------------
> >>> arch/mips/kvm/vz.c | 14 ++------------
> >>> include/linux/kvm_host.h | 1 +
> >>> virt/kvm/kvm_main.c | 10 ++++++++--
> >>> 7 files changed, 23 insertions(+), 47 deletions(-)
> >>>
> >>
> >> Compile-tested and queued.
> >
> > Just in case it fells through the crach - Paolo, do you still have
> > plan to queue this again?
>
> Yes, I wanted to make it compile first though. I'm undecided between
> queuing your series and killing KVM MIPS honestly.
Understood. Yep killing that will provide the same thing too as what
the series wanted to do anyways, as we'll remove the only outlier of
kvm_flush_remote_tlbs(). Thanks,
--
Peter Xu
On 02/08/2020 06:35 AM, Peter Xu wrote:
> Select HAVE_KVM_ARCH_TLB_FLUSH_ALL for MIPS to define its own
> kvm_flush_remote_tlbs(). It's as simple as calling the
> flush_shadow_all(kvm) hook. Then replace all the flush_shadow_all()
> calls to use this KVM generic API to do TLB flush.
>
> Signed-off-by: Peter Xu <[email protected]>
> ---
> arch/mips/kvm/Kconfig | 1 +
> arch/mips/kvm/mips.c | 22 ++++++++++------------
> 2 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/arch/mips/kvm/Kconfig b/arch/mips/kvm/Kconfig
> index eac25aef21e0..8a06f660d39e 100644
> --- a/arch/mips/kvm/Kconfig
> +++ b/arch/mips/kvm/Kconfig
> @@ -26,6 +26,7 @@ config KVM
> select KVM_MMIO
> select MMU_NOTIFIER
> select SRCU
> + select HAVE_KVM_ARCH_TLB_FLUSH_ALL
> ---help---
> Support for hosting Guest kernels.
>
> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
> index 1d5e7ffda746..518020b466bf 100644
> --- a/arch/mips/kvm/mips.c
> +++ b/arch/mips/kvm/mips.c
> @@ -194,13 +194,16 @@ int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot,
> return 0;
> }
>
> +void kvm_flush_remote_tlbs(struct kvm *kvm)
> +{
> + kvm_mips_callbacks->flush_shadow_all(kvm);
> +}
> +
Hi Peter,
Although I do not understand mip VZ fully, however it changes behavior
of MIPS VZ, since kvm_flush_remote_tlbs is also called in function
kvm_mmu_notifier_change_pte/kvm_mmu_notifier_invalidate_range_start
regards
bibo, mao
> void kvm_arch_flush_shadow_all(struct kvm *kvm)
> {
> /* Flush whole GPA */
> kvm_mips_flush_gpa_pt(kvm, 0, ~0);
> -
> - /* Let implementation do the rest */
> - kvm_mips_callbacks->flush_shadow_all(kvm);
> + kvm_flush_remote_tlbs(kvm);
> }
>
> void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
> @@ -215,8 +218,7 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
> /* Flush slot from GPA */
> kvm_mips_flush_gpa_pt(kvm, slot->base_gfn,
> slot->base_gfn + slot->npages - 1);
> - /* Let implementation do the rest */
> - kvm_mips_callbacks->flush_shadow_all(kvm);
> + kvm_flush_remote_tlbs(kvm);
> spin_unlock(&kvm->mmu_lock);
> }
>
> @@ -258,7 +260,7 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
> new->base_gfn + new->npages - 1);
> /* Let implementation do the rest */
> if (needs_flush)
> - kvm_mips_callbacks->flush_shadow_all(kvm);
> + kvm_flush_remote_tlbs(kvm);
> spin_unlock(&kvm->mmu_lock);
> }
> }
> @@ -1001,9 +1003,7 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
> if (flush) {
> slots = kvm_memslots(kvm);
> memslot = id_to_memslot(slots, log->slot);
> -
> - /* Let implementation handle TLB/GVA invalidation */
> - kvm_mips_callbacks->flush_shadow_all(kvm);
> + kvm_flush_remote_tlbs(kvm);
> }
>
> mutex_unlock(&kvm->slots_lock);
> @@ -1024,9 +1024,7 @@ int kvm_vm_ioctl_clear_dirty_log(struct kvm *kvm, struct kvm_clear_dirty_log *lo
> if (flush) {
> slots = kvm_memslots(kvm);
> memslot = id_to_memslot(slots, log->slot);
> -
> - /* Let implementation handle TLB/GVA invalidation */
> - kvm_mips_callbacks->flush_shadow_all(kvm);
> + kvm_flush_remote_tlbs(kvm);
> }
>
> mutex_unlock(&kvm->slots_lock);
>
On Wed, Mar 18, 2020 at 11:03:13AM +0800, maobibo wrote:
>
>
> On 02/08/2020 06:35 AM, Peter Xu wrote:
> > Select HAVE_KVM_ARCH_TLB_FLUSH_ALL for MIPS to define its own
> > kvm_flush_remote_tlbs(). It's as simple as calling the
> > flush_shadow_all(kvm) hook. Then replace all the flush_shadow_all()
> > calls to use this KVM generic API to do TLB flush.
> >
> > Signed-off-by: Peter Xu <[email protected]>
> > ---
> > arch/mips/kvm/Kconfig | 1 +
> > arch/mips/kvm/mips.c | 22 ++++++++++------------
> > 2 files changed, 11 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/mips/kvm/Kconfig b/arch/mips/kvm/Kconfig
> > index eac25aef21e0..8a06f660d39e 100644
> > --- a/arch/mips/kvm/Kconfig
> > +++ b/arch/mips/kvm/Kconfig
> > @@ -26,6 +26,7 @@ config KVM
> > select KVM_MMIO
> > select MMU_NOTIFIER
> > select SRCU
> > + select HAVE_KVM_ARCH_TLB_FLUSH_ALL
> > ---help---
> > Support for hosting Guest kernels.
> >
> > diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
> > index 1d5e7ffda746..518020b466bf 100644
> > --- a/arch/mips/kvm/mips.c
> > +++ b/arch/mips/kvm/mips.c
> > @@ -194,13 +194,16 @@ int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot,
> > return 0;
> > }
> >
> > +void kvm_flush_remote_tlbs(struct kvm *kvm)
> > +{
> > + kvm_mips_callbacks->flush_shadow_all(kvm);
> > +}
> > +
> Hi Peter,
Hi, Bibo,
>
> Although I do not understand mip VZ fully, however it changes behavior of
> MIPS VZ, since kvm_flush_remote_tlbs is also called in function
> kvm_mmu_notifier_change_pte/kvm_mmu_notifier_invalidate_range_start
I'm not familiar with MIPS either, however... I would start to suspect
MIPS could be problematic with MMU notifiers when cpu_has_guestid is
not set. If that's the case, then this series might instead fix it.
Thanks,
--
Peter Xu
On 03/18/2020 11:28 PM, Peter Xu wrote:
> On Wed, Mar 18, 2020 at 11:03:13AM +0800, maobibo wrote:
>>
>>
>> On 02/08/2020 06:35 AM, Peter Xu wrote:
>>> Select HAVE_KVM_ARCH_TLB_FLUSH_ALL for MIPS to define its own
>>> kvm_flush_remote_tlbs(). It's as simple as calling the
>>> flush_shadow_all(kvm) hook. Then replace all the flush_shadow_all()
>>> calls to use this KVM generic API to do TLB flush.
>>>
>>> Signed-off-by: Peter Xu <[email protected]>
>>> ---
>>> arch/mips/kvm/Kconfig | 1 +
>>> arch/mips/kvm/mips.c | 22 ++++++++++------------
>>> 2 files changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch/mips/kvm/Kconfig b/arch/mips/kvm/Kconfig
>>> index eac25aef21e0..8a06f660d39e 100644
>>> --- a/arch/mips/kvm/Kconfig
>>> +++ b/arch/mips/kvm/Kconfig
>>> @@ -26,6 +26,7 @@ config KVM
>>> select KVM_MMIO
>>> select MMU_NOTIFIER
>>> select SRCU
>>> + select HAVE_KVM_ARCH_TLB_FLUSH_ALL
>>> ---help---
>>> Support for hosting Guest kernels.
>>>
>>> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
>>> index 1d5e7ffda746..518020b466bf 100644
>>> --- a/arch/mips/kvm/mips.c
>>> +++ b/arch/mips/kvm/mips.c
>>> @@ -194,13 +194,16 @@ int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot,
>>> return 0;
>>> }
>>>
>>> +void kvm_flush_remote_tlbs(struct kvm *kvm)
>>> +{
>>> + kvm_mips_callbacks->flush_shadow_all(kvm);
>>> +}
>>> +
>> Hi Peter,
>
> Hi, Bibo,
>
>>
>> Although I do not understand mip VZ fully, however it changes behavior of
>> MIPS VZ, since kvm_flush_remote_tlbs is also called in function
>> kvm_mmu_notifier_change_pte/kvm_mmu_notifier_invalidate_range_start
>
> I'm not familiar with MIPS either, however... I would start to suspect
> MIPS could be problematic with MMU notifiers when cpu_has_guestid is
> not set. If that's the case, then this series might instead fix it.
yeap, from my viewpoint this series actually fix it when cpu_has_guestid
is not set, previous kvm_flush_remote_tlbs function do nothing actually
for MIPS VZ machine without cpu_has_guestid flag.
>
> Thanks,
>