[PATCH v2 0/6] LoongArch: KVM: Fix some issues relative with mmu

[PATCH v2 0/6] LoongArch: KVM: Fix some issues relative with mmu

[Bibo Mao]

This patchset is mmu relative, it fixes potential issue about tlb flush 
of secondary mmu and huge page selection etc. Also it hardens LoongArch
kvm mmu module.

With this patchset, VM migration is stabler than before.

Bibo Mao (6):
  LoongArch: KVM: Delay secondary mmu tlb flush until guest entry
  LoongArch: KVM: Select huge page only if secondary mmu supports it
  LoongArch: KVM: Discard dirty page tracking on readonly memslot
  LoongArch: KVM: Add memory barrier before update pmd entry
  LoongArch: KVM: Add dirty bitmap initially all set support
  LoongArch: KVM: Mark page accessed and dirty with page ref added

 arch/loongarch/include/asm/kvm_host.h |  5 ++
 arch/loongarch/include/asm/kvm_mmu.h  |  2 +-
 arch/loongarch/kvm/main.c             |  1 +
 arch/loongarch/kvm/mmu.c              | 67 ++++++++++++++++++++-------
 arch/loongarch/kvm/tlb.c              |  5 +-
 arch/loongarch/kvm/vcpu.c             | 18 +++++++
 6 files changed, 75 insertions(+), 23 deletions(-)


base-commit: 92e5605a199efbaee59fb19e15d6cc2103a04ec2
-- 
2.39.3

[PATCH v2 1/6] LoongArch: KVM: Delay secondary mmu tlb flush until guest entry

[Bibo Mao]

If there is page fault for secondary mmu, there needs tlb flush
operation indexed with fault gpa address and VMID. VMID is stored
at register CSR_GSTAT and will be reload or recalculated during
guest entry.

Currently CSR_GSTAT is not saved and restored during vcpu context
switch, it is recalculated during guest entry. So CSR_GSTAT is in
effect only when vcpu runs in guest mode, however it may be not in
effected if vcpu exits to host mode, since register CSR_GSTAT may
be stale, it maybe records VMID of last scheduled vcpu, rather than
current vcpu.

Function kvm_flush_tlb_gpa() should be called with its real VMID,
here move it to guest entrance. Also arch specific request id
KVM_REQ_TLB_FLUSH_GPA is added to flush tlb, and it can be optimized
if VMID is updated, since all guest tlb entries will be invalid if
VMID is updated.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 arch/loongarch/include/asm/kvm_host.h |  2 ++
 arch/loongarch/kvm/main.c             |  1 +
 arch/loongarch/kvm/mmu.c              |  4 ++--
 arch/loongarch/kvm/tlb.c              |  5 +----
 arch/loongarch/kvm/vcpu.c             | 18 ++++++++++++++++++
 5 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/arch/loongarch/include/asm/kvm_host.h b/arch/loongarch/include/asm/kvm_host.h
index c87b6ea0ec47..32c4948f534f 100644
--- a/arch/loongarch/include/asm/kvm_host.h
+++ b/arch/loongarch/include/asm/kvm_host.h
@@ -30,6 +30,7 @@
 #define KVM_PRIVATE_MEM_SLOTS		0
 
 #define KVM_HALT_POLL_NS_DEFAULT	500000
+#define KVM_REQ_TLB_FLUSH_GPA		KVM_ARCH_REQ(0)
 
 #define KVM_GUESTDBG_SW_BP_MASK		\
 	(KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP)
@@ -190,6 +191,7 @@ struct kvm_vcpu_arch {
 
 	/* vcpu's vpid */
 	u64 vpid;
+	gpa_t flush_gpa;
 
 	/* Frequency of stable timer in Hz */
 	u64 timer_mhz;
diff --git a/arch/loongarch/kvm/main.c b/arch/loongarch/kvm/main.c
index 86a2f2d0cb27..844736b99d38 100644
--- a/arch/loongarch/kvm/main.c
+++ b/arch/loongarch/kvm/main.c
@@ -242,6 +242,7 @@ void kvm_check_vpid(struct kvm_vcpu *vcpu)
 		kvm_update_vpid(vcpu, cpu);
 		trace_kvm_vpid_change(vcpu, vcpu->arch.vpid);
 		vcpu->cpu = cpu;
+		kvm_clear_request(KVM_REQ_TLB_FLUSH_GPA, vcpu);
 	}
 
 	/* Restore GSTAT(0x50).vpid */
diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
index 98883aa23ab8..9e39d28fec35 100644
--- a/arch/loongarch/kvm/mmu.c
+++ b/arch/loongarch/kvm/mmu.c
@@ -908,8 +908,8 @@ int kvm_handle_mm_fault(struct kvm_vcpu *vcpu, unsigned long gpa, bool write)
 		return ret;
 
 	/* Invalidate this entry in the TLB */
-	kvm_flush_tlb_gpa(vcpu, gpa);
-
+	vcpu->arch.flush_gpa = gpa;
+	kvm_make_request(KVM_REQ_TLB_FLUSH_GPA, vcpu);
 	return 0;
 }
 
diff --git a/arch/loongarch/kvm/tlb.c b/arch/loongarch/kvm/tlb.c
index 02535df6b51f..ebdbe9264e9c 100644
--- a/arch/loongarch/kvm/tlb.c
+++ b/arch/loongarch/kvm/tlb.c
@@ -23,10 +23,7 @@ void kvm_flush_tlb_all(void)
 
 void kvm_flush_tlb_gpa(struct kvm_vcpu *vcpu, unsigned long gpa)
 {
-	unsigned long flags;
-
-	local_irq_save(flags);
+	lockdep_assert_irqs_disabled();
 	gpa &= (PAGE_MASK << 1);
 	invtlb(INVTLB_GID_ADDR, read_csr_gstat() & CSR_GSTAT_GID, gpa);
-	local_irq_restore(flags);
 }
diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
index 9e8030d45129..b747bd8bc037 100644
--- a/arch/loongarch/kvm/vcpu.c
+++ b/arch/loongarch/kvm/vcpu.c
@@ -51,6 +51,16 @@ static int kvm_check_requests(struct kvm_vcpu *vcpu)
 	return RESUME_GUEST;
 }
 
+static void kvm_late_check_requests(struct kvm_vcpu *vcpu)
+{
+	lockdep_assert_irqs_disabled();
+	if (kvm_check_request(KVM_REQ_TLB_FLUSH_GPA, vcpu))
+		if (vcpu->arch.flush_gpa != INVALID_GPA) {
+			kvm_flush_tlb_gpa(vcpu, vcpu->arch.flush_gpa);
+			vcpu->arch.flush_gpa = INVALID_GPA;
+		}
+}
+
 /*
  * Check and handle pending signal and vCPU requests etc
  * Run with irq enabled and preempt enabled
@@ -101,6 +111,13 @@ static int kvm_pre_enter_guest(struct kvm_vcpu *vcpu)
 		/* Make sure the vcpu mode has been written */
 		smp_store_mb(vcpu->mode, IN_GUEST_MODE);
 		kvm_check_vpid(vcpu);
+
+		/*
+		 * Called after function kvm_check_vpid()
+		 * Since it updates csr_gstat used by kvm_flush_tlb_gpa(),
+		 * also it may clear KVM_REQ_TLB_FLUSH_GPA pending bit
+		 */
+		kvm_late_check_requests(vcpu);
 		vcpu->arch.host_eentry = csr_read64(LOONGARCH_CSR_EENTRY);
 		/* Clear KVM_LARCH_SWCSR_LATEST as CSR will change when enter guest */
 		vcpu->arch.aux_inuse &= ~KVM_LARCH_SWCSR_LATEST;
@@ -994,6 +1011,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 	struct loongarch_csrs *csr;
 
 	vcpu->arch.vpid = 0;
+	vcpu->arch.flush_gpa = INVALID_GPA;
 
 	hrtimer_init(&vcpu->arch.swtimer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_PINNED);
 	vcpu->arch.swtimer.function = kvm_swtimer_wakeup;
-- 
2.39.3

[PATCH v2 2/6] LoongArch: KVM: Select huge page only if secondary mmu supports it

[Bibo Mao]

Currently page level selection about secondary mmu depends on memory
slot and page level about host mmu. There will be problem if page level
of secondary mmu is zero already. So page level selection should depend
on the following three conditions.
 1. Memslot is aligned for huge page and vm is not migrating.
 2. Page level of host mmu is huge page also.
 3. Page level of secondary mmu is suituable for huge page, it cannot
be normal page since it is not supported to merge normal pages into
huge page now.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 arch/loongarch/include/asm/kvm_mmu.h |  2 +-
 arch/loongarch/kvm/mmu.c             | 16 +++++++++++++---
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/loongarch/include/asm/kvm_mmu.h b/arch/loongarch/include/asm/kvm_mmu.h
index 099bafc6f797..d06ae0e0dde5 100644
--- a/arch/loongarch/include/asm/kvm_mmu.h
+++ b/arch/loongarch/include/asm/kvm_mmu.h
@@ -55,7 +55,7 @@ static inline void kvm_set_pte(kvm_pte_t *ptep, kvm_pte_t val)
 static inline int kvm_pte_write(kvm_pte_t pte) { return pte & _PAGE_WRITE; }
 static inline int kvm_pte_dirty(kvm_pte_t pte) { return pte & _PAGE_DIRTY; }
 static inline int kvm_pte_young(kvm_pte_t pte) { return pte & _PAGE_ACCESSED; }
-static inline int kvm_pte_huge(kvm_pte_t pte) { return pte & _PAGE_HUGE; }
+static inline int kvm_pte_huge(kvm_pte_t pte)  { return !!(pte & _PAGE_HUGE); }
 
 static inline kvm_pte_t kvm_pte_mkyoung(kvm_pte_t pte)
 {
diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
index 9e39d28fec35..c6351d13ca1b 100644
--- a/arch/loongarch/kvm/mmu.c
+++ b/arch/loongarch/kvm/mmu.c
@@ -858,10 +858,20 @@ static int kvm_map_page(struct kvm_vcpu *vcpu, unsigned long gpa, bool write)
 
 	/* Disable dirty logging on HugePages */
 	level = 0;
-	if (!fault_supports_huge_mapping(memslot, hva, write)) {
-		level = 0;
-	} else {
+	if (fault_supports_huge_mapping(memslot, hva, write)) {
+		/* Check page level about host mmu*/
 		level = host_pfn_mapping_level(kvm, gfn, memslot);
+		if (level == 1) {
+			/*
+			 * Check page level about secondary mmu
+			 * Disable hugepage if it is normal page on
+			 * secondary mmu already
+			 */
+			ptep = kvm_populate_gpa(kvm, NULL, gpa, 0);
+			if (ptep && !kvm_pte_huge(*ptep))
+				level = 0;
+		}
+
 		if (level == 1) {
 			gfn = gfn & ~(PTRS_PER_PTE - 1);
 			pfn = pfn & ~(PTRS_PER_PTE - 1);
-- 
2.39.3

[PATCH v2 3/6] LoongArch: KVM: Discard dirty page tracking on readonly memslot

[Bibo Mao]

For readonly memslot such as UEFI bios or UEFI var space, guest can
not write this memory space directly. So it is not necessary to track
dirty pages for readonly memslot. Here there is such optimization
in function kvm_arch_commit_memory_region().

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 arch/loongarch/kvm/mmu.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
index c6351d13ca1b..1690828bd44b 100644
--- a/arch/loongarch/kvm/mmu.c
+++ b/arch/loongarch/kvm/mmu.c
@@ -444,6 +444,17 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
 				   enum kvm_mr_change change)
 {
 	int needs_flush;
+	u32 old_flags = old ? old->flags : 0;
+	u32 new_flags = new ? new->flags : 0;
+	bool log_dirty_pages = new_flags & KVM_MEM_LOG_DIRTY_PAGES;
+
+	/* only track memslot flags changed */
+	if (change != KVM_MR_FLAGS_ONLY)
+		return;
+
+	/* Discard dirty page tracking on readonly memslot */
+	if ((old_flags & new_flags) & KVM_MEM_READONLY)
+		return;
 
 	/*
 	 * If dirty page logging is enabled, write protect all pages in the slot
@@ -454,9 +465,7 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
 	 * MOVE/DELETE:	The old mappings will already have been cleaned up by
 	 *		kvm_arch_flush_shadow_memslot()
 	 */
-	if (change == KVM_MR_FLAGS_ONLY &&
-	    (!(old->flags & KVM_MEM_LOG_DIRTY_PAGES) &&
-	     new->flags & KVM_MEM_LOG_DIRTY_PAGES)) {
+	if (!(old_flags & KVM_MEM_LOG_DIRTY_PAGES) && log_dirty_pages) {
 		spin_lock(&kvm->mmu_lock);
 		/* Write protect GPA page table entries */
 		needs_flush = kvm_mkclean_gpa_pt(kvm, new->base_gfn,
-- 
2.39.3

[PATCH v2 4/6] LoongArch: KVM: Add memory barrier before update pmd entry

[Bibo Mao]

When updating pmd entry such as allocating new pmd page or splitting
huge page into normal page, it is necessary to firstly update all pte
entries, and then update pmd entry.

It is weak order with LoongArch system, there will be problem if other
vcpus sees pmd update firstly however pte is not updated. Here smp_wmb()
is added to assure this.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 arch/loongarch/kvm/mmu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
index 1690828bd44b..7f04edfbe428 100644
--- a/arch/loongarch/kvm/mmu.c
+++ b/arch/loongarch/kvm/mmu.c
@@ -163,6 +163,7 @@ static kvm_pte_t *kvm_populate_gpa(struct kvm *kvm,
 
 			child = kvm_mmu_memory_cache_alloc(cache);
 			_kvm_pte_init(child, ctx.invalid_ptes[ctx.level - 1]);
+			smp_wmb(); /* make pte visible before pmd */
 			kvm_set_pte(entry, __pa(child));
 		} else if (kvm_pte_huge(*entry)) {
 			return entry;
@@ -746,6 +747,7 @@ static kvm_pte_t *kvm_split_huge(struct kvm_vcpu *vcpu, kvm_pte_t *ptep, gfn_t g
 		val += PAGE_SIZE;
 	}
 
+	smp_wmb();
 	/* The later kvm_flush_tlb_gpa() will flush hugepage tlb */
 	kvm_set_pte(ptep, __pa(child));
 
-- 
2.39.3

[PATCH v2 5/6] LoongArch: KVM: Add dirty bitmap initially all set support

[Bibo Mao]

Add KVM_DIRTY_LOG_INITIALLY_SET support on LoongArch system, this
feature comes from other architectures like x86 and arm64.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 arch/loongarch/include/asm/kvm_host.h | 3 +++
 arch/loongarch/kvm/mmu.c              | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/arch/loongarch/include/asm/kvm_host.h b/arch/loongarch/include/asm/kvm_host.h
index 32c4948f534f..309ce329b747 100644
--- a/arch/loongarch/include/asm/kvm_host.h
+++ b/arch/loongarch/include/asm/kvm_host.h
@@ -37,6 +37,9 @@
 #define KVM_GUESTDBG_VALID_MASK		\
 	(KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP | KVM_GUESTDBG_SINGLESTEP)
 
+#define KVM_DIRTY_LOG_MANUAL_CAPS	\
+	(KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | KVM_DIRTY_LOG_INITIALLY_SET)
+
 struct kvm_vm_stat {
 	struct kvm_vm_stat_generic generic;
 	u64 pages;
diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
index 7f04edfbe428..3b862f3a72cb 100644
--- a/arch/loongarch/kvm/mmu.c
+++ b/arch/loongarch/kvm/mmu.c
@@ -467,6 +467,13 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
 	 *		kvm_arch_flush_shadow_memslot()
 	 */
 	if (!(old_flags & KVM_MEM_LOG_DIRTY_PAGES) && log_dirty_pages) {
+		/*
+		 * Initially-all-set does not require write protecting any page
+		 * because they're all assumed to be dirty.
+		 */
+		if (kvm_dirty_log_manual_protect_and_init_set(kvm))
+			return;
+
 		spin_lock(&kvm->mmu_lock);
 		/* Write protect GPA page table entries */
 		needs_flush = kvm_mkclean_gpa_pt(kvm, new->base_gfn,
-- 
2.39.3

[PATCH v2 6/6] LoongArch: KVM: Mark page accessed and dirty with page ref added

[Bibo Mao]

Function kvm_map_page_fast() is fast path of secondary mmu page fault
flow, pfn is parsed from secondary mmu page table walker. However
the corresponding page reference is not added, it is dangerious to
access page out of mmu_lock.

Here page ref is added inside mmu_lock, function kvm_set_pfn_accessed()
and kvm_set_pfn_dirty() is called with page ref added, so that the
page will not be freed by others.

Also kvm_set_pfn_accessed() is removed here since it is called in
the following function kvm_release_pfn_clean().

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 arch/loongarch/kvm/mmu.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
index 3b862f3a72cb..5a820a81fd97 100644
--- a/arch/loongarch/kvm/mmu.c
+++ b/arch/loongarch/kvm/mmu.c
@@ -557,6 +557,7 @@ static int kvm_map_page_fast(struct kvm_vcpu *vcpu, unsigned long gpa, bool writ
 	gfn_t gfn = gpa >> PAGE_SHIFT;
 	struct kvm *kvm = vcpu->kvm;
 	struct kvm_memory_slot *slot;
+	struct page *page;
 
 	spin_lock(&kvm->mmu_lock);
 
@@ -599,19 +600,22 @@ static int kvm_map_page_fast(struct kvm_vcpu *vcpu, unsigned long gpa, bool writ
 	if (changed) {
 		kvm_set_pte(ptep, new);
 		pfn = kvm_pte_pfn(new);
+		page = kvm_pfn_to_refcounted_page(pfn);
+		if (page)
+			get_page(page);
 	}
 	spin_unlock(&kvm->mmu_lock);
 
-	/*
-	 * Fixme: pfn may be freed after mmu_lock
-	 * kvm_try_get_pfn(pfn)/kvm_release_pfn pair to prevent this?
-	 */
-	if (kvm_pte_young(changed))
-		kvm_set_pfn_accessed(pfn);
+	if (changed) {
+		if (kvm_pte_young(changed))
+			kvm_set_pfn_accessed(pfn);
 
-	if (kvm_pte_dirty(changed)) {
-		mark_page_dirty(kvm, gfn);
-		kvm_set_pfn_dirty(pfn);
+		if (kvm_pte_dirty(changed)) {
+			mark_page_dirty(kvm, gfn);
+			kvm_set_pfn_dirty(pfn);
+		}
+		if (page)
+			put_page(page);
 	}
 	return ret;
 out:
@@ -920,7 +924,6 @@ static int kvm_map_page(struct kvm_vcpu *vcpu, unsigned long gpa, bool write)
 		kvm_set_pfn_dirty(pfn);
 	}
 
-	kvm_set_pfn_accessed(pfn);
 	kvm_release_pfn_clean(pfn);
 out:
 	srcu_read_unlock(&kvm->srcu, srcu_idx);
-- 
2.39.3

Re: [PATCH v2 6/6] LoongArch: KVM: Mark page accessed and dirty with page ref added

[kernel test robot]

Hi Bibo,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 92e5605a199efbaee59fb19e15d6cc2103a04ec2]

url:    https://github.com/intel-lab-lkp/linux/commits/Bibo-Mao/LoongArch-KVM-Delay-secondary-mmu-tlb-flush-until-guest-entry/20240619-161831
base:   92e5605a199efbaee59fb19e15d6cc2103a04ec2
patch link:    https://lore.kernel.org/r/20240619080940.2690756-7-maobibo%40loongson.cn
patch subject: [PATCH v2 6/6] LoongArch: KVM: Mark page accessed and dirty with page ref added
config: loongarch-defconfig (https://download.01.org/0day-ci/archive/20240620/202406201000.BjivosoH-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240620/202406201000.BjivosoH-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406201000.BjivosoH-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> arch/loongarch/kvm/mmu.o: warning: objtool: __jump_table+0x0: special: can't find orig instruction


objdump-func vmlinux.o __jump_table:

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 6/6] LoongArch: KVM: Mark page accessed and dirty with page ref added

[Huacai Chen]

Hi, Bibo,

What is the relationship between this patch and the below one?
https://lore.kernel.org/loongarch/20240611034609.3442344-1-maobibo@loongson.cn/T/#u


Huacai

On Wed, Jun 19, 2024 at 4:09 PM Bibo Mao <maobibo@loongson.cn> wrote:
>
> Function kvm_map_page_fast() is fast path of secondary mmu page fault
> flow, pfn is parsed from secondary mmu page table walker. However
> the corresponding page reference is not added, it is dangerious to
> access page out of mmu_lock.
>
> Here page ref is added inside mmu_lock, function kvm_set_pfn_accessed()
> and kvm_set_pfn_dirty() is called with page ref added, so that the
> page will not be freed by others.
>
> Also kvm_set_pfn_accessed() is removed here since it is called in
> the following function kvm_release_pfn_clean().
>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>  arch/loongarch/kvm/mmu.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
> index 3b862f3a72cb..5a820a81fd97 100644
> --- a/arch/loongarch/kvm/mmu.c
> +++ b/arch/loongarch/kvm/mmu.c
> @@ -557,6 +557,7 @@ static int kvm_map_page_fast(struct kvm_vcpu *vcpu, unsigned long gpa, bool writ
>         gfn_t gfn = gpa >> PAGE_SHIFT;
>         struct kvm *kvm = vcpu->kvm;
>         struct kvm_memory_slot *slot;
> +       struct page *page;
>
>         spin_lock(&kvm->mmu_lock);
>
> @@ -599,19 +600,22 @@ static int kvm_map_page_fast(struct kvm_vcpu *vcpu, unsigned long gpa, bool writ
>         if (changed) {
>                 kvm_set_pte(ptep, new);
>                 pfn = kvm_pte_pfn(new);
> +               page = kvm_pfn_to_refcounted_page(pfn);
> +               if (page)
> +                       get_page(page);
>         }
>         spin_unlock(&kvm->mmu_lock);
>
> -       /*
> -        * Fixme: pfn may be freed after mmu_lock
> -        * kvm_try_get_pfn(pfn)/kvm_release_pfn pair to prevent this?
> -        */
> -       if (kvm_pte_young(changed))
> -               kvm_set_pfn_accessed(pfn);
> +       if (changed) {
> +               if (kvm_pte_young(changed))
> +                       kvm_set_pfn_accessed(pfn);
>
> -       if (kvm_pte_dirty(changed)) {
> -               mark_page_dirty(kvm, gfn);
> -               kvm_set_pfn_dirty(pfn);
> +               if (kvm_pte_dirty(changed)) {
> +                       mark_page_dirty(kvm, gfn);
> +                       kvm_set_pfn_dirty(pfn);
> +               }
> +               if (page)
> +                       put_page(page);
>         }
>         return ret;
>  out:
> @@ -920,7 +924,6 @@ static int kvm_map_page(struct kvm_vcpu *vcpu, unsigned long gpa, bool write)
>                 kvm_set_pfn_dirty(pfn);
>         }
>
> -       kvm_set_pfn_accessed(pfn);
>         kvm_release_pfn_clean(pfn);
>  out:
>         srcu_read_unlock(&kvm->srcu, srcu_idx);
> --
> 2.39.3
>

Re: [PATCH v2 1/6] LoongArch: KVM: Delay secondary mmu tlb flush until guest entry

[Huacai Chen]

Hi, Bibo,

On Wed, Jun 19, 2024 at 4:09 PM Bibo Mao <maobibo@loongson.cn> wrote:
>
> If there is page fault for secondary mmu, there needs tlb flush
What does "secondary mmu" in this context mean? Maybe "guest mmu"?

Huacai

> operation indexed with fault gpa address and VMID. VMID is stored
> at register CSR_GSTAT and will be reload or recalculated during
> guest entry.
>
> Currently CSR_GSTAT is not saved and restored during vcpu context
> switch, it is recalculated during guest entry. So CSR_GSTAT is in
> effect only when vcpu runs in guest mode, however it may be not in
> effected if vcpu exits to host mode, since register CSR_GSTAT may
> be stale, it maybe records VMID of last scheduled vcpu, rather than
> current vcpu.
>
> Function kvm_flush_tlb_gpa() should be called with its real VMID,
> here move it to guest entrance. Also arch specific request id
> KVM_REQ_TLB_FLUSH_GPA is added to flush tlb, and it can be optimized
> if VMID is updated, since all guest tlb entries will be invalid if
> VMID is updated.
>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>  arch/loongarch/include/asm/kvm_host.h |  2 ++
>  arch/loongarch/kvm/main.c             |  1 +
>  arch/loongarch/kvm/mmu.c              |  4 ++--
>  arch/loongarch/kvm/tlb.c              |  5 +----
>  arch/loongarch/kvm/vcpu.c             | 18 ++++++++++++++++++
>  5 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/arch/loongarch/include/asm/kvm_host.h b/arch/loongarch/include/asm/kvm_host.h
> index c87b6ea0ec47..32c4948f534f 100644
> --- a/arch/loongarch/include/asm/kvm_host.h
> +++ b/arch/loongarch/include/asm/kvm_host.h
> @@ -30,6 +30,7 @@
>  #define KVM_PRIVATE_MEM_SLOTS          0
>
>  #define KVM_HALT_POLL_NS_DEFAULT       500000
> +#define KVM_REQ_TLB_FLUSH_GPA          KVM_ARCH_REQ(0)
>
>  #define KVM_GUESTDBG_SW_BP_MASK                \
>         (KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP)
> @@ -190,6 +191,7 @@ struct kvm_vcpu_arch {
>
>         /* vcpu's vpid */
>         u64 vpid;
> +       gpa_t flush_gpa;
>
>         /* Frequency of stable timer in Hz */
>         u64 timer_mhz;
> diff --git a/arch/loongarch/kvm/main.c b/arch/loongarch/kvm/main.c
> index 86a2f2d0cb27..844736b99d38 100644
> --- a/arch/loongarch/kvm/main.c
> +++ b/arch/loongarch/kvm/main.c
> @@ -242,6 +242,7 @@ void kvm_check_vpid(struct kvm_vcpu *vcpu)
>                 kvm_update_vpid(vcpu, cpu);
>                 trace_kvm_vpid_change(vcpu, vcpu->arch.vpid);
>                 vcpu->cpu = cpu;
> +               kvm_clear_request(KVM_REQ_TLB_FLUSH_GPA, vcpu);
>         }
>
>         /* Restore GSTAT(0x50).vpid */
> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
> index 98883aa23ab8..9e39d28fec35 100644
> --- a/arch/loongarch/kvm/mmu.c
> +++ b/arch/loongarch/kvm/mmu.c
> @@ -908,8 +908,8 @@ int kvm_handle_mm_fault(struct kvm_vcpu *vcpu, unsigned long gpa, bool write)
>                 return ret;
>
>         /* Invalidate this entry in the TLB */
> -       kvm_flush_tlb_gpa(vcpu, gpa);
> -
> +       vcpu->arch.flush_gpa = gpa;
> +       kvm_make_request(KVM_REQ_TLB_FLUSH_GPA, vcpu);
>         return 0;
>  }
>
> diff --git a/arch/loongarch/kvm/tlb.c b/arch/loongarch/kvm/tlb.c
> index 02535df6b51f..ebdbe9264e9c 100644
> --- a/arch/loongarch/kvm/tlb.c
> +++ b/arch/loongarch/kvm/tlb.c
> @@ -23,10 +23,7 @@ void kvm_flush_tlb_all(void)
>
>  void kvm_flush_tlb_gpa(struct kvm_vcpu *vcpu, unsigned long gpa)
>  {
> -       unsigned long flags;
> -
> -       local_irq_save(flags);
> +       lockdep_assert_irqs_disabled();
>         gpa &= (PAGE_MASK << 1);
>         invtlb(INVTLB_GID_ADDR, read_csr_gstat() & CSR_GSTAT_GID, gpa);
> -       local_irq_restore(flags);
>  }
> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index 9e8030d45129..b747bd8bc037 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c
> @@ -51,6 +51,16 @@ static int kvm_check_requests(struct kvm_vcpu *vcpu)
>         return RESUME_GUEST;
>  }
>
> +static void kvm_late_check_requests(struct kvm_vcpu *vcpu)
> +{
> +       lockdep_assert_irqs_disabled();
> +       if (kvm_check_request(KVM_REQ_TLB_FLUSH_GPA, vcpu))
> +               if (vcpu->arch.flush_gpa != INVALID_GPA) {
> +                       kvm_flush_tlb_gpa(vcpu, vcpu->arch.flush_gpa);
> +                       vcpu->arch.flush_gpa = INVALID_GPA;
> +               }
> +}
> +
>  /*
>   * Check and handle pending signal and vCPU requests etc
>   * Run with irq enabled and preempt enabled
> @@ -101,6 +111,13 @@ static int kvm_pre_enter_guest(struct kvm_vcpu *vcpu)
>                 /* Make sure the vcpu mode has been written */
>                 smp_store_mb(vcpu->mode, IN_GUEST_MODE);
>                 kvm_check_vpid(vcpu);
> +
> +               /*
> +                * Called after function kvm_check_vpid()
> +                * Since it updates csr_gstat used by kvm_flush_tlb_gpa(),
> +                * also it may clear KVM_REQ_TLB_FLUSH_GPA pending bit
> +                */
> +               kvm_late_check_requests(vcpu);
>                 vcpu->arch.host_eentry = csr_read64(LOONGARCH_CSR_EENTRY);
>                 /* Clear KVM_LARCH_SWCSR_LATEST as CSR will change when enter guest */
>                 vcpu->arch.aux_inuse &= ~KVM_LARCH_SWCSR_LATEST;
> @@ -994,6 +1011,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>         struct loongarch_csrs *csr;
>
>         vcpu->arch.vpid = 0;
> +       vcpu->arch.flush_gpa = INVALID_GPA;
>
>         hrtimer_init(&vcpu->arch.swtimer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_PINNED);
>         vcpu->arch.swtimer.function = kvm_swtimer_wakeup;
> --
> 2.39.3
>

Re: [PATCH v2 2/6] LoongArch: KVM: Select huge page only if secondary mmu supports it

[Huacai Chen]

Hi, Bibo,

On Wed, Jun 19, 2024 at 4:09 PM Bibo Mao <maobibo@loongson.cn> wrote:
>
> Currently page level selection about secondary mmu depends on memory
> slot and page level about host mmu. There will be problem if page level
> of secondary mmu is zero already. So page level selection should depend
> on the following three conditions.
>  1. Memslot is aligned for huge page and vm is not migrating.
>  2. Page level of host mmu is huge page also.
>  3. Page level of secondary mmu is suituable for huge page, it cannot
> be normal page since it is not supported to merge normal pages into
> huge page now.
>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>  arch/loongarch/include/asm/kvm_mmu.h |  2 +-
>  arch/loongarch/kvm/mmu.c             | 16 +++++++++++++---
>  2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/arch/loongarch/include/asm/kvm_mmu.h b/arch/loongarch/include/asm/kvm_mmu.h
> index 099bafc6f797..d06ae0e0dde5 100644
> --- a/arch/loongarch/include/asm/kvm_mmu.h
> +++ b/arch/loongarch/include/asm/kvm_mmu.h
> @@ -55,7 +55,7 @@ static inline void kvm_set_pte(kvm_pte_t *ptep, kvm_pte_t val)
>  static inline int kvm_pte_write(kvm_pte_t pte) { return pte & _PAGE_WRITE; }
>  static inline int kvm_pte_dirty(kvm_pte_t pte) { return pte & _PAGE_DIRTY; }
>  static inline int kvm_pte_young(kvm_pte_t pte) { return pte & _PAGE_ACCESSED; }
> -static inline int kvm_pte_huge(kvm_pte_t pte) { return pte & _PAGE_HUGE; }
> +static inline int kvm_pte_huge(kvm_pte_t pte)  { return !!(pte & _PAGE_HUGE); }
Why do we need this change?

Huacai

>
>  static inline kvm_pte_t kvm_pte_mkyoung(kvm_pte_t pte)
>  {
> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
> index 9e39d28fec35..c6351d13ca1b 100644
> --- a/arch/loongarch/kvm/mmu.c
> +++ b/arch/loongarch/kvm/mmu.c
> @@ -858,10 +858,20 @@ static int kvm_map_page(struct kvm_vcpu *vcpu, unsigned long gpa, bool write)
>
>         /* Disable dirty logging on HugePages */
>         level = 0;
> -       if (!fault_supports_huge_mapping(memslot, hva, write)) {
> -               level = 0;
> -       } else {
> +       if (fault_supports_huge_mapping(memslot, hva, write)) {
> +               /* Check page level about host mmu*/
>                 level = host_pfn_mapping_level(kvm, gfn, memslot);
> +               if (level == 1) {
> +                       /*
> +                        * Check page level about secondary mmu
> +                        * Disable hugepage if it is normal page on
> +                        * secondary mmu already
> +                        */
> +                       ptep = kvm_populate_gpa(kvm, NULL, gpa, 0);
> +                       if (ptep && !kvm_pte_huge(*ptep))
> +                               level = 0;
> +               }
> +
>                 if (level == 1) {
>                         gfn = gfn & ~(PTRS_PER_PTE - 1);
>                         pfn = pfn & ~(PTRS_PER_PTE - 1);
> --
> 2.39.3
>

Re: [PATCH v2 4/6] LoongArch: KVM: Add memory barrier before update pmd entry

[Huacai Chen]

Hi, Bibo,

On Wed, Jun 19, 2024 at 4:09 PM Bibo Mao <maobibo@loongson.cn> wrote:
>
> When updating pmd entry such as allocating new pmd page or splitting
> huge page into normal page, it is necessary to firstly update all pte
> entries, and then update pmd entry.
>
> It is weak order with LoongArch system, there will be problem if other
> vcpus sees pmd update firstly however pte is not updated. Here smp_wmb()
> is added to assure this.
Memory barriers should be in pairs in most cases. That means you may
lose smp_rmb() in another place.

Huacai

>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>  arch/loongarch/kvm/mmu.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
> index 1690828bd44b..7f04edfbe428 100644
> --- a/arch/loongarch/kvm/mmu.c
> +++ b/arch/loongarch/kvm/mmu.c
> @@ -163,6 +163,7 @@ static kvm_pte_t *kvm_populate_gpa(struct kvm *kvm,
>
>                         child = kvm_mmu_memory_cache_alloc(cache);
>                         _kvm_pte_init(child, ctx.invalid_ptes[ctx.level - 1]);
> +                       smp_wmb(); /* make pte visible before pmd */
>                         kvm_set_pte(entry, __pa(child));
>                 } else if (kvm_pte_huge(*entry)) {
>                         return entry;
> @@ -746,6 +747,7 @@ static kvm_pte_t *kvm_split_huge(struct kvm_vcpu *vcpu, kvm_pte_t *ptep, gfn_t g
>                 val += PAGE_SIZE;
>         }
>
> +       smp_wmb();
>         /* The later kvm_flush_tlb_gpa() will flush hugepage tlb */
>         kvm_set_pte(ptep, __pa(child));
>
> --
> 2.39.3
>

Re: [PATCH v2 6/6] LoongArch: KVM: Mark page accessed and dirty with page ref added

[maobibo]

On 2024/6/22 下午1:21, Huacai Chen wrote:
> Hi, Bibo,
> 
> What is the relationship between this patch and the below one?
> https://lore.kernel.org/loongarch/20240611034609.3442344-1-maobibo@loongson.cn/T/#u

It is updated version about the patch listed at this website, I put all 
migration relative patches into one patch set, to prevent that it is 
lost in so many mail threads:)

Regards
Bibo Mao
> 
> 
> Huacai
> 
> On Wed, Jun 19, 2024 at 4:09 PM Bibo Mao <maobibo@loongson.cn> wrote:
>>
>> Function kvm_map_page_fast() is fast path of secondary mmu page fault
>> flow, pfn is parsed from secondary mmu page table walker. However
>> the corresponding page reference is not added, it is dangerious to
>> access page out of mmu_lock.
>>
>> Here page ref is added inside mmu_lock, function kvm_set_pfn_accessed()
>> and kvm_set_pfn_dirty() is called with page ref added, so that the
>> page will not be freed by others.
>>
>> Also kvm_set_pfn_accessed() is removed here since it is called in
>> the following function kvm_release_pfn_clean().
>>
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>>   arch/loongarch/kvm/mmu.c | 23 +++++++++++++----------
>>   1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
>> index 3b862f3a72cb..5a820a81fd97 100644
>> --- a/arch/loongarch/kvm/mmu.c
>> +++ b/arch/loongarch/kvm/mmu.c
>> @@ -557,6 +557,7 @@ static int kvm_map_page_fast(struct kvm_vcpu *vcpu, unsigned long gpa, bool writ
>>          gfn_t gfn = gpa >> PAGE_SHIFT;
>>          struct kvm *kvm = vcpu->kvm;
>>          struct kvm_memory_slot *slot;
>> +       struct page *page;
>>
>>          spin_lock(&kvm->mmu_lock);
>>
>> @@ -599,19 +600,22 @@ static int kvm_map_page_fast(struct kvm_vcpu *vcpu, unsigned long gpa, bool writ
>>          if (changed) {
>>                  kvm_set_pte(ptep, new);
>>                  pfn = kvm_pte_pfn(new);
>> +               page = kvm_pfn_to_refcounted_page(pfn);
>> +               if (page)
>> +                       get_page(page);
>>          }
>>          spin_unlock(&kvm->mmu_lock);
>>
>> -       /*
>> -        * Fixme: pfn may be freed after mmu_lock
>> -        * kvm_try_get_pfn(pfn)/kvm_release_pfn pair to prevent this?
>> -        */
>> -       if (kvm_pte_young(changed))
>> -               kvm_set_pfn_accessed(pfn);
>> +       if (changed) {
>> +               if (kvm_pte_young(changed))
>> +                       kvm_set_pfn_accessed(pfn);
>>
>> -       if (kvm_pte_dirty(changed)) {
>> -               mark_page_dirty(kvm, gfn);
>> -               kvm_set_pfn_dirty(pfn);
>> +               if (kvm_pte_dirty(changed)) {
>> +                       mark_page_dirty(kvm, gfn);
>> +                       kvm_set_pfn_dirty(pfn);
>> +               }
>> +               if (page)
>> +                       put_page(page);
>>          }
>>          return ret;
>>   out:
>> @@ -920,7 +924,6 @@ static int kvm_map_page(struct kvm_vcpu *vcpu, unsigned long gpa, bool write)
>>                  kvm_set_pfn_dirty(pfn);
>>          }
>>
>> -       kvm_set_pfn_accessed(pfn);
>>          kvm_release_pfn_clean(pfn);
>>   out:
>>          srcu_read_unlock(&kvm->srcu, srcu_idx);
>> --
>> 2.39.3
>>

Re: [PATCH v2 1/6] LoongArch: KVM: Delay secondary mmu tlb flush until guest entry

[maobibo]

On 2024/6/23 下午3:54, Huacai Chen wrote:
> Hi, Bibo,
> 
> On Wed, Jun 19, 2024 at 4:09 PM Bibo Mao <maobibo@loongson.cn> wrote:
>>
>> If there is page fault for secondary mmu, there needs tlb flush
> What does "secondary mmu" in this context mean? Maybe "guest mmu"?
"secondary mmu" following x86 concepts, the weblink is:
    https://lwn.net/Articles/977945/

It is called stage-2 mmu on ARM64 also. "guest mmu" cannot represent 
whether it is gva to gpa, or gpa to hpa, or gva to hpa directly.

Regards
Bibo Mao

> 
> Huacai
> 
>> operation indexed with fault gpa address and VMID. VMID is stored
>> at register CSR_GSTAT and will be reload or recalculated during
>> guest entry.
>>
>> Currently CSR_GSTAT is not saved and restored during vcpu context
>> switch, it is recalculated during guest entry. So CSR_GSTAT is in
>> effect only when vcpu runs in guest mode, however it may be not in
>> effected if vcpu exits to host mode, since register CSR_GSTAT may
>> be stale, it maybe records VMID of last scheduled vcpu, rather than
>> current vcpu.
>>
>> Function kvm_flush_tlb_gpa() should be called with its real VMID,
>> here move it to guest entrance. Also arch specific request id
>> KVM_REQ_TLB_FLUSH_GPA is added to flush tlb, and it can be optimized
>> if VMID is updated, since all guest tlb entries will be invalid if
>> VMID is updated.
>>
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>>   arch/loongarch/include/asm/kvm_host.h |  2 ++
>>   arch/loongarch/kvm/main.c             |  1 +
>>   arch/loongarch/kvm/mmu.c              |  4 ++--
>>   arch/loongarch/kvm/tlb.c              |  5 +----
>>   arch/loongarch/kvm/vcpu.c             | 18 ++++++++++++++++++
>>   5 files changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/loongarch/include/asm/kvm_host.h b/arch/loongarch/include/asm/kvm_host.h
>> index c87b6ea0ec47..32c4948f534f 100644
>> --- a/arch/loongarch/include/asm/kvm_host.h
>> +++ b/arch/loongarch/include/asm/kvm_host.h
>> @@ -30,6 +30,7 @@
>>   #define KVM_PRIVATE_MEM_SLOTS          0
>>
>>   #define KVM_HALT_POLL_NS_DEFAULT       500000
>> +#define KVM_REQ_TLB_FLUSH_GPA          KVM_ARCH_REQ(0)
>>
>>   #define KVM_GUESTDBG_SW_BP_MASK                \
>>          (KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP)
>> @@ -190,6 +191,7 @@ struct kvm_vcpu_arch {
>>
>>          /* vcpu's vpid */
>>          u64 vpid;
>> +       gpa_t flush_gpa;
>>
>>          /* Frequency of stable timer in Hz */
>>          u64 timer_mhz;
>> diff --git a/arch/loongarch/kvm/main.c b/arch/loongarch/kvm/main.c
>> index 86a2f2d0cb27..844736b99d38 100644
>> --- a/arch/loongarch/kvm/main.c
>> +++ b/arch/loongarch/kvm/main.c
>> @@ -242,6 +242,7 @@ void kvm_check_vpid(struct kvm_vcpu *vcpu)
>>                  kvm_update_vpid(vcpu, cpu);
>>                  trace_kvm_vpid_change(vcpu, vcpu->arch.vpid);
>>                  vcpu->cpu = cpu;
>> +               kvm_clear_request(KVM_REQ_TLB_FLUSH_GPA, vcpu);
>>          }
>>
>>          /* Restore GSTAT(0x50).vpid */
>> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
>> index 98883aa23ab8..9e39d28fec35 100644
>> --- a/arch/loongarch/kvm/mmu.c
>> +++ b/arch/loongarch/kvm/mmu.c
>> @@ -908,8 +908,8 @@ int kvm_handle_mm_fault(struct kvm_vcpu *vcpu, unsigned long gpa, bool write)
>>                  return ret;
>>
>>          /* Invalidate this entry in the TLB */
>> -       kvm_flush_tlb_gpa(vcpu, gpa);
>> -
>> +       vcpu->arch.flush_gpa = gpa;
>> +       kvm_make_request(KVM_REQ_TLB_FLUSH_GPA, vcpu);
>>          return 0;
>>   }
>>
>> diff --git a/arch/loongarch/kvm/tlb.c b/arch/loongarch/kvm/tlb.c
>> index 02535df6b51f..ebdbe9264e9c 100644
>> --- a/arch/loongarch/kvm/tlb.c
>> +++ b/arch/loongarch/kvm/tlb.c
>> @@ -23,10 +23,7 @@ void kvm_flush_tlb_all(void)
>>
>>   void kvm_flush_tlb_gpa(struct kvm_vcpu *vcpu, unsigned long gpa)
>>   {
>> -       unsigned long flags;
>> -
>> -       local_irq_save(flags);
>> +       lockdep_assert_irqs_disabled();
>>          gpa &= (PAGE_MASK << 1);
>>          invtlb(INVTLB_GID_ADDR, read_csr_gstat() & CSR_GSTAT_GID, gpa);
>> -       local_irq_restore(flags);
>>   }
>> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
>> index 9e8030d45129..b747bd8bc037 100644
>> --- a/arch/loongarch/kvm/vcpu.c
>> +++ b/arch/loongarch/kvm/vcpu.c
>> @@ -51,6 +51,16 @@ static int kvm_check_requests(struct kvm_vcpu *vcpu)
>>          return RESUME_GUEST;
>>   }
>>
>> +static void kvm_late_check_requests(struct kvm_vcpu *vcpu)
>> +{
>> +       lockdep_assert_irqs_disabled();
>> +       if (kvm_check_request(KVM_REQ_TLB_FLUSH_GPA, vcpu))
>> +               if (vcpu->arch.flush_gpa != INVALID_GPA) {
>> +                       kvm_flush_tlb_gpa(vcpu, vcpu->arch.flush_gpa);
>> +                       vcpu->arch.flush_gpa = INVALID_GPA;
>> +               }
>> +}
>> +
>>   /*
>>    * Check and handle pending signal and vCPU requests etc
>>    * Run with irq enabled and preempt enabled
>> @@ -101,6 +111,13 @@ static int kvm_pre_enter_guest(struct kvm_vcpu *vcpu)
>>                  /* Make sure the vcpu mode has been written */
>>                  smp_store_mb(vcpu->mode, IN_GUEST_MODE);
>>                  kvm_check_vpid(vcpu);
>> +
>> +               /*
>> +                * Called after function kvm_check_vpid()
>> +                * Since it updates csr_gstat used by kvm_flush_tlb_gpa(),
>> +                * also it may clear KVM_REQ_TLB_FLUSH_GPA pending bit
>> +                */
>> +               kvm_late_check_requests(vcpu);
>>                  vcpu->arch.host_eentry = csr_read64(LOONGARCH_CSR_EENTRY);
>>                  /* Clear KVM_LARCH_SWCSR_LATEST as CSR will change when enter guest */
>>                  vcpu->arch.aux_inuse &= ~KVM_LARCH_SWCSR_LATEST;
>> @@ -994,6 +1011,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>>          struct loongarch_csrs *csr;
>>
>>          vcpu->arch.vpid = 0;
>> +       vcpu->arch.flush_gpa = INVALID_GPA;
>>
>>          hrtimer_init(&vcpu->arch.swtimer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_PINNED);
>>          vcpu->arch.swtimer.function = kvm_swtimer_wakeup;
>> --
>> 2.39.3
>>

Re: [PATCH v2 2/6] LoongArch: KVM: Select huge page only if secondary mmu supports it

[maobibo]

On 2024/6/23 下午3:55, Huacai Chen wrote:
> Hi, Bibo,
> 
> On Wed, Jun 19, 2024 at 4:09 PM Bibo Mao <maobibo@loongson.cn> wrote:
>>
>> Currently page level selection about secondary mmu depends on memory
>> slot and page level about host mmu. There will be problem if page level
>> of secondary mmu is zero already. So page level selection should depend
>> on the following three conditions.
>>   1. Memslot is aligned for huge page and vm is not migrating.
>>   2. Page level of host mmu is huge page also.
>>   3. Page level of secondary mmu is suituable for huge page, it cannot
>> be normal page since it is not supported to merge normal pages into
>> huge page now.
>>
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>>   arch/loongarch/include/asm/kvm_mmu.h |  2 +-
>>   arch/loongarch/kvm/mmu.c             | 16 +++++++++++++---
>>   2 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/loongarch/include/asm/kvm_mmu.h b/arch/loongarch/include/asm/kvm_mmu.h
>> index 099bafc6f797..d06ae0e0dde5 100644
>> --- a/arch/loongarch/include/asm/kvm_mmu.h
>> +++ b/arch/loongarch/include/asm/kvm_mmu.h
>> @@ -55,7 +55,7 @@ static inline void kvm_set_pte(kvm_pte_t *ptep, kvm_pte_t val)
>>   static inline int kvm_pte_write(kvm_pte_t pte) { return pte & _PAGE_WRITE; }
>>   static inline int kvm_pte_dirty(kvm_pte_t pte) { return pte & _PAGE_DIRTY; }
>>   static inline int kvm_pte_young(kvm_pte_t pte) { return pte & _PAGE_ACCESSED; }
>> -static inline int kvm_pte_huge(kvm_pte_t pte) { return pte & _PAGE_HUGE; }
>> +static inline int kvm_pte_huge(kvm_pte_t pte)  { return !!(pte & _PAGE_HUGE); }
> Why do we need this change?
In later there is such usage like !kvm_pte_huge(*ptep)
       if (ptep && !kvm_pte_huge(*ptep))

I had thought it should be 0/1 if !kvm_pte_huge() is used. However the 
original is ok by test.

I will remove this modification.

Regards
Bibo Mao


> 
> Huacai
> 
>>
>>   static inline kvm_pte_t kvm_pte_mkyoung(kvm_pte_t pte)
>>   {
>> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
>> index 9e39d28fec35..c6351d13ca1b 100644
>> --- a/arch/loongarch/kvm/mmu.c
>> +++ b/arch/loongarch/kvm/mmu.c
>> @@ -858,10 +858,20 @@ static int kvm_map_page(struct kvm_vcpu *vcpu, unsigned long gpa, bool write)
>>
>>          /* Disable dirty logging on HugePages */
>>          level = 0;
>> -       if (!fault_supports_huge_mapping(memslot, hva, write)) {
>> -               level = 0;
>> -       } else {
>> +       if (fault_supports_huge_mapping(memslot, hva, write)) {
>> +               /* Check page level about host mmu*/
>>                  level = host_pfn_mapping_level(kvm, gfn, memslot);
>> +               if (level == 1) {
>> +                       /*
>> +                        * Check page level about secondary mmu
>> +                        * Disable hugepage if it is normal page on
>> +                        * secondary mmu already
>> +                        */
>> +                       ptep = kvm_populate_gpa(kvm, NULL, gpa, 0);
>> +                       if (ptep && !kvm_pte_huge(*ptep))
>> +                               level = 0;
>> +               }
>> +
>>                  if (level == 1) {
>>                          gfn = gfn & ~(PTRS_PER_PTE - 1);
>>                          pfn = pfn & ~(PTRS_PER_PTE - 1);
>> --
>> 2.39.3
>>

Re: [PATCH v2 4/6] LoongArch: KVM: Add memory barrier before update pmd entry

[maobibo]

On 2024/6/23 下午6:18, Huacai Chen wrote:
> Hi, Bibo,
> 
> On Wed, Jun 19, 2024 at 4:09 PM Bibo Mao <maobibo@loongson.cn> wrote:
>>
>> When updating pmd entry such as allocating new pmd page or splitting
>> huge page into normal page, it is necessary to firstly update all pte
>> entries, and then update pmd entry.
>>
>> It is weak order with LoongArch system, there will be problem if other
>> vcpus sees pmd update firstly however pte is not updated. Here smp_wmb()
>> is added to assure this.
> Memory barriers should be in pairs in most cases. That means you may
> lose smp_rmb() in another place.
The idea adding smp_wmb() comes from function __split_huge_pmd_locked()
in file mm/huge_memory.c, and the explanation is reasonable.

                 ...
                 set_ptes(mm, haddr, pte, entry, HPAGE_PMD_NR);
         }
         ...
         smp_wmb(); /* make pte visible before pmd */
         pmd_populate(mm, pmd, pgtable);

It is strange that why smp_rmb() should be in pairs with smp_wmb(),
I never hear this rule -:(

Regards
Bibo Mao
> 
> Huacai
> 
>>
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>>   arch/loongarch/kvm/mmu.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
>> index 1690828bd44b..7f04edfbe428 100644
>> --- a/arch/loongarch/kvm/mmu.c
>> +++ b/arch/loongarch/kvm/mmu.c
>> @@ -163,6 +163,7 @@ static kvm_pte_t *kvm_populate_gpa(struct kvm *kvm,
>>
>>                          child = kvm_mmu_memory_cache_alloc(cache);
>>                          _kvm_pte_init(child, ctx.invalid_ptes[ctx.level - 1]);
>> +                       smp_wmb(); /* make pte visible before pmd */
>>                          kvm_set_pte(entry, __pa(child));
>>                  } else if (kvm_pte_huge(*entry)) {
>>                          return entry;
>> @@ -746,6 +747,7 @@ static kvm_pte_t *kvm_split_huge(struct kvm_vcpu *vcpu, kvm_pte_t *ptep, gfn_t g
>>                  val += PAGE_SIZE;
>>          }
>>
>> +       smp_wmb();
>>          /* The later kvm_flush_tlb_gpa() will flush hugepage tlb */
>>          kvm_set_pte(ptep, __pa(child));
>>
>> --
>> 2.39.3
>>

Re: [PATCH v2 4/6] LoongArch: KVM: Add memory barrier before update pmd entry

[Huacai Chen]

On Mon, Jun 24, 2024 at 9:37 AM maobibo <maobibo@loongson.cn> wrote:
>
>
>
> On 2024/6/23 下午6:18, Huacai Chen wrote:
> > Hi, Bibo,
> >
> > On Wed, Jun 19, 2024 at 4:09 PM Bibo Mao <maobibo@loongson.cn> wrote:
> >>
> >> When updating pmd entry such as allocating new pmd page or splitting
> >> huge page into normal page, it is necessary to firstly update all pte
> >> entries, and then update pmd entry.
> >>
> >> It is weak order with LoongArch system, there will be problem if other
> >> vcpus sees pmd update firstly however pte is not updated. Here smp_wmb()
> >> is added to assure this.
> > Memory barriers should be in pairs in most cases. That means you may
> > lose smp_rmb() in another place.
> The idea adding smp_wmb() comes from function __split_huge_pmd_locked()
> in file mm/huge_memory.c, and the explanation is reasonable.
>
>                  ...
>                  set_ptes(mm, haddr, pte, entry, HPAGE_PMD_NR);
>          }
>          ...
>          smp_wmb(); /* make pte visible before pmd */
>          pmd_populate(mm, pmd, pgtable);
>
> It is strange that why smp_rmb() should be in pairs with smp_wmb(),
> I never hear this rule -:(
https://docs.kernel.org/core-api/wrappers/memory-barriers.html

SMP BARRIER PAIRING
-------------------

When dealing with CPU-CPU interactions, certain types of memory barrier should
always be paired.  A lack of appropriate pairing is almost certainly an error.


Huacai

>
> Regards
> Bibo Mao
> >
> > Huacai
> >
> >>
> >> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> >> ---
> >>   arch/loongarch/kvm/mmu.c | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
> >> index 1690828bd44b..7f04edfbe428 100644
> >> --- a/arch/loongarch/kvm/mmu.c
> >> +++ b/arch/loongarch/kvm/mmu.c
> >> @@ -163,6 +163,7 @@ static kvm_pte_t *kvm_populate_gpa(struct kvm *kvm,
> >>
> >>                          child = kvm_mmu_memory_cache_alloc(cache);
> >>                          _kvm_pte_init(child, ctx.invalid_ptes[ctx.level - 1]);
> >> +                       smp_wmb(); /* make pte visible before pmd */
> >>                          kvm_set_pte(entry, __pa(child));
> >>                  } else if (kvm_pte_huge(*entry)) {
> >>                          return entry;
> >> @@ -746,6 +747,7 @@ static kvm_pte_t *kvm_split_huge(struct kvm_vcpu *vcpu, kvm_pte_t *ptep, gfn_t g
> >>                  val += PAGE_SIZE;
> >>          }
> >>
> >> +       smp_wmb();
> >>          /* The later kvm_flush_tlb_gpa() will flush hugepage tlb */
> >>          kvm_set_pte(ptep, __pa(child));
> >>
> >> --
> >> 2.39.3
> >>
>
>

Re: [PATCH v2 1/6] LoongArch: KVM: Delay secondary mmu tlb flush until guest entry

[Huacai Chen]

On Mon, Jun 24, 2024 at 9:23 AM maobibo <maobibo@loongson.cn> wrote:
>
>
>
> On 2024/6/23 下午3:54, Huacai Chen wrote:
> > Hi, Bibo,
> >
> > On Wed, Jun 19, 2024 at 4:09 PM Bibo Mao <maobibo@loongson.cn> wrote:
> >>
> >> If there is page fault for secondary mmu, there needs tlb flush
> > What does "secondary mmu" in this context mean? Maybe "guest mmu"?
> "secondary mmu" following x86 concepts, the weblink is:
>     https://lwn.net/Articles/977945/
>
> It is called stage-2 mmu on ARM64 also. "guest mmu" cannot represent
> whether it is gva to gpa, or gpa to hpa, or gva to hpa directly.
Then it is better to explain "what is secondary mmu" in the commit
message when it appears at the first time.

Huacai

>
> Regards
> Bibo Mao
>
> >
> > Huacai
> >
> >> operation indexed with fault gpa address and VMID. VMID is stored
> >> at register CSR_GSTAT and will be reload or recalculated during
> >> guest entry.
> >>
> >> Currently CSR_GSTAT is not saved and restored during vcpu context
> >> switch, it is recalculated during guest entry. So CSR_GSTAT is in
> >> effect only when vcpu runs in guest mode, however it may be not in
> >> effected if vcpu exits to host mode, since register CSR_GSTAT may
> >> be stale, it maybe records VMID of last scheduled vcpu, rather than
> >> current vcpu.
> >>
> >> Function kvm_flush_tlb_gpa() should be called with its real VMID,
> >> here move it to guest entrance. Also arch specific request id
> >> KVM_REQ_TLB_FLUSH_GPA is added to flush tlb, and it can be optimized
> >> if VMID is updated, since all guest tlb entries will be invalid if
> >> VMID is updated.
> >>
> >> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> >> ---
> >>   arch/loongarch/include/asm/kvm_host.h |  2 ++
> >>   arch/loongarch/kvm/main.c             |  1 +
> >>   arch/loongarch/kvm/mmu.c              |  4 ++--
> >>   arch/loongarch/kvm/tlb.c              |  5 +----
> >>   arch/loongarch/kvm/vcpu.c             | 18 ++++++++++++++++++
> >>   5 files changed, 24 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/arch/loongarch/include/asm/kvm_host.h b/arch/loongarch/include/asm/kvm_host.h
> >> index c87b6ea0ec47..32c4948f534f 100644
> >> --- a/arch/loongarch/include/asm/kvm_host.h
> >> +++ b/arch/loongarch/include/asm/kvm_host.h
> >> @@ -30,6 +30,7 @@
> >>   #define KVM_PRIVATE_MEM_SLOTS          0
> >>
> >>   #define KVM_HALT_POLL_NS_DEFAULT       500000
> >> +#define KVM_REQ_TLB_FLUSH_GPA          KVM_ARCH_REQ(0)
> >>
> >>   #define KVM_GUESTDBG_SW_BP_MASK                \
> >>          (KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP)
> >> @@ -190,6 +191,7 @@ struct kvm_vcpu_arch {
> >>
> >>          /* vcpu's vpid */
> >>          u64 vpid;
> >> +       gpa_t flush_gpa;
> >>
> >>          /* Frequency of stable timer in Hz */
> >>          u64 timer_mhz;
> >> diff --git a/arch/loongarch/kvm/main.c b/arch/loongarch/kvm/main.c
> >> index 86a2f2d0cb27..844736b99d38 100644
> >> --- a/arch/loongarch/kvm/main.c
> >> +++ b/arch/loongarch/kvm/main.c
> >> @@ -242,6 +242,7 @@ void kvm_check_vpid(struct kvm_vcpu *vcpu)
> >>                  kvm_update_vpid(vcpu, cpu);
> >>                  trace_kvm_vpid_change(vcpu, vcpu->arch.vpid);
> >>                  vcpu->cpu = cpu;
> >> +               kvm_clear_request(KVM_REQ_TLB_FLUSH_GPA, vcpu);
> >>          }
> >>
> >>          /* Restore GSTAT(0x50).vpid */
> >> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
> >> index 98883aa23ab8..9e39d28fec35 100644
> >> --- a/arch/loongarch/kvm/mmu.c
> >> +++ b/arch/loongarch/kvm/mmu.c
> >> @@ -908,8 +908,8 @@ int kvm_handle_mm_fault(struct kvm_vcpu *vcpu, unsigned long gpa, bool write)
> >>                  return ret;
> >>
> >>          /* Invalidate this entry in the TLB */
> >> -       kvm_flush_tlb_gpa(vcpu, gpa);
> >> -
> >> +       vcpu->arch.flush_gpa = gpa;
> >> +       kvm_make_request(KVM_REQ_TLB_FLUSH_GPA, vcpu);
> >>          return 0;
> >>   }
> >>
> >> diff --git a/arch/loongarch/kvm/tlb.c b/arch/loongarch/kvm/tlb.c
> >> index 02535df6b51f..ebdbe9264e9c 100644
> >> --- a/arch/loongarch/kvm/tlb.c
> >> +++ b/arch/loongarch/kvm/tlb.c
> >> @@ -23,10 +23,7 @@ void kvm_flush_tlb_all(void)
> >>
> >>   void kvm_flush_tlb_gpa(struct kvm_vcpu *vcpu, unsigned long gpa)
> >>   {
> >> -       unsigned long flags;
> >> -
> >> -       local_irq_save(flags);
> >> +       lockdep_assert_irqs_disabled();
> >>          gpa &= (PAGE_MASK << 1);
> >>          invtlb(INVTLB_GID_ADDR, read_csr_gstat() & CSR_GSTAT_GID, gpa);
> >> -       local_irq_restore(flags);
> >>   }
> >> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> >> index 9e8030d45129..b747bd8bc037 100644
> >> --- a/arch/loongarch/kvm/vcpu.c
> >> +++ b/arch/loongarch/kvm/vcpu.c
> >> @@ -51,6 +51,16 @@ static int kvm_check_requests(struct kvm_vcpu *vcpu)
> >>          return RESUME_GUEST;
> >>   }
> >>
> >> +static void kvm_late_check_requests(struct kvm_vcpu *vcpu)
> >> +{
> >> +       lockdep_assert_irqs_disabled();
> >> +       if (kvm_check_request(KVM_REQ_TLB_FLUSH_GPA, vcpu))
> >> +               if (vcpu->arch.flush_gpa != INVALID_GPA) {
> >> +                       kvm_flush_tlb_gpa(vcpu, vcpu->arch.flush_gpa);
> >> +                       vcpu->arch.flush_gpa = INVALID_GPA;
> >> +               }
> >> +}
> >> +
> >>   /*
> >>    * Check and handle pending signal and vCPU requests etc
> >>    * Run with irq enabled and preempt enabled
> >> @@ -101,6 +111,13 @@ static int kvm_pre_enter_guest(struct kvm_vcpu *vcpu)
> >>                  /* Make sure the vcpu mode has been written */
> >>                  smp_store_mb(vcpu->mode, IN_GUEST_MODE);
> >>                  kvm_check_vpid(vcpu);
> >> +
> >> +               /*
> >> +                * Called after function kvm_check_vpid()
> >> +                * Since it updates csr_gstat used by kvm_flush_tlb_gpa(),
> >> +                * also it may clear KVM_REQ_TLB_FLUSH_GPA pending bit
> >> +                */
> >> +               kvm_late_check_requests(vcpu);
> >>                  vcpu->arch.host_eentry = csr_read64(LOONGARCH_CSR_EENTRY);
> >>                  /* Clear KVM_LARCH_SWCSR_LATEST as CSR will change when enter guest */
> >>                  vcpu->arch.aux_inuse &= ~KVM_LARCH_SWCSR_LATEST;
> >> @@ -994,6 +1011,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
> >>          struct loongarch_csrs *csr;
> >>
> >>          vcpu->arch.vpid = 0;
> >> +       vcpu->arch.flush_gpa = INVALID_GPA;
> >>
> >>          hrtimer_init(&vcpu->arch.swtimer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_PINNED);
> >>          vcpu->arch.swtimer.function = kvm_swtimer_wakeup;
> >> --
> >> 2.39.3
> >>
>
>

Re: [PATCH v2 4/6] LoongArch: KVM: Add memory barrier before update pmd entry

[maobibo]

On 2024/6/24 上午9:56, Huacai Chen wrote:
> On Mon, Jun 24, 2024 at 9:37 AM maobibo <maobibo@loongson.cn> wrote:
>>
>>
>>
>> On 2024/6/23 下午6:18, Huacai Chen wrote:
>>> Hi, Bibo,
>>>
>>> On Wed, Jun 19, 2024 at 4:09 PM Bibo Mao <maobibo@loongson.cn> wrote:
>>>>
>>>> When updating pmd entry such as allocating new pmd page or splitting
>>>> huge page into normal page, it is necessary to firstly update all pte
>>>> entries, and then update pmd entry.
>>>>
>>>> It is weak order with LoongArch system, there will be problem if other
>>>> vcpus sees pmd update firstly however pte is not updated. Here smp_wmb()
>>>> is added to assure this.
>>> Memory barriers should be in pairs in most cases. That means you may
>>> lose smp_rmb() in another place.
>> The idea adding smp_wmb() comes from function __split_huge_pmd_locked()
>> in file mm/huge_memory.c, and the explanation is reasonable.
>>
>>                   ...
>>                   set_ptes(mm, haddr, pte, entry, HPAGE_PMD_NR);
>>           }
>>           ...
>>           smp_wmb(); /* make pte visible before pmd */
>>           pmd_populate(mm, pmd, pgtable);
>>
>> It is strange that why smp_rmb() should be in pairs with smp_wmb(),
>> I never hear this rule -:(
> https://docs.kernel.org/core-api/wrappers/memory-barriers.html
> 
> SMP BARRIER PAIRING
> -------------------
> 
> When dealing with CPU-CPU interactions, certain types of memory barrier should
> always be paired.  A lack of appropriate pairing is almost certainly an error.
    CPU 1                 CPU 2
         ===============       ===============
         WRITE_ONCE(a, 1);
         <write barrier>
         WRITE_ONCE(b, 2);     x = READ_ONCE(b);
                               <read barrier>
                               y = READ_ONCE(a);

With split_huge scenery to update pte/pmd entry, there is no strong 
relationship between address ptex and pmd.
CPU1
      WRITE_ONCE(pte0, 1);
      WRITE_ONCE(pte511, 1);
      <write barrier>
      WRITE_ONCE(pmd, 2);

However with page table walk scenery, address ptep depends on the 
contents of pmd, so it is not necessary to add smp_rmb().
         ptep = pte_offset_map_lock(mm, pmd, address, &ptl);
         if (!ptep)
                 return no_page_table(vma, flags, address);
         pte = ptep_get(ptep);
         if (!pte_present(pte))

It is just my option, or do you think where smp_rmb() barrier should be 
added in page table reader path?

Regards
Bibo Mao
> 
> 
> Huacai
> 
>>
>> Regards
>> Bibo Mao
>>>
>>> Huacai
>>>
>>>>
>>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>>>> ---
>>>>    arch/loongarch/kvm/mmu.c | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
>>>> index 1690828bd44b..7f04edfbe428 100644
>>>> --- a/arch/loongarch/kvm/mmu.c
>>>> +++ b/arch/loongarch/kvm/mmu.c
>>>> @@ -163,6 +163,7 @@ static kvm_pte_t *kvm_populate_gpa(struct kvm *kvm,
>>>>
>>>>                           child = kvm_mmu_memory_cache_alloc(cache);
>>>>                           _kvm_pte_init(child, ctx.invalid_ptes[ctx.level - 1]);
>>>> +                       smp_wmb(); /* make pte visible before pmd */
>>>>                           kvm_set_pte(entry, __pa(child));
>>>>                   } else if (kvm_pte_huge(*entry)) {
>>>>                           return entry;
>>>> @@ -746,6 +747,7 @@ static kvm_pte_t *kvm_split_huge(struct kvm_vcpu *vcpu, kvm_pte_t *ptep, gfn_t g
>>>>                   val += PAGE_SIZE;
>>>>           }
>>>>
>>>> +       smp_wmb();
>>>>           /* The later kvm_flush_tlb_gpa() will flush hugepage tlb */
>>>>           kvm_set_pte(ptep, __pa(child));
>>>>
>>>> --
>>>> 2.39.3
>>>>
>>
>>

Re: [PATCH v2 1/6] LoongArch: KVM: Delay secondary mmu tlb flush until guest entry

[maobibo]

On 2024/6/24 上午9:58, Huacai Chen wrote:
> On Mon, Jun 24, 2024 at 9:23 AM maobibo <maobibo@loongson.cn> wrote:
>>
>>
>>
>> On 2024/6/23 下午3:54, Huacai Chen wrote:
>>> Hi, Bibo,
>>>
>>> On Wed, Jun 19, 2024 at 4:09 PM Bibo Mao <maobibo@loongson.cn> wrote:
>>>>
>>>> If there is page fault for secondary mmu, there needs tlb flush
>>> What does "secondary mmu" in this context mean? Maybe "guest mmu"?
>> "secondary mmu" following x86 concepts, the weblink is:
>>      https://lwn.net/Articles/977945/
>>
>> It is called stage-2 mmu on ARM64 also. "guest mmu" cannot represent
>> whether it is gva to gpa, or gpa to hpa, or gva to hpa directly.
> Then it is better to explain "what is secondary mmu" in the commit
> message when it appears at the first time.
Sure, will do in next patch.

Regards
Bibo Mao
> 
> Huacai
> 
>>
>> Regards
>> Bibo Mao
>>
>>>
>>> Huacai
>>>
>>>> operation indexed with fault gpa address and VMID. VMID is stored
>>>> at register CSR_GSTAT and will be reload or recalculated during
>>>> guest entry.
>>>>
>>>> Currently CSR_GSTAT is not saved and restored during vcpu context
>>>> switch, it is recalculated during guest entry. So CSR_GSTAT is in
>>>> effect only when vcpu runs in guest mode, however it may be not in
>>>> effected if vcpu exits to host mode, since register CSR_GSTAT may
>>>> be stale, it maybe records VMID of last scheduled vcpu, rather than
>>>> current vcpu.
>>>>
>>>> Function kvm_flush_tlb_gpa() should be called with its real VMID,
>>>> here move it to guest entrance. Also arch specific request id
>>>> KVM_REQ_TLB_FLUSH_GPA is added to flush tlb, and it can be optimized
>>>> if VMID is updated, since all guest tlb entries will be invalid if
>>>> VMID is updated.
>>>>
>>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>>>> ---
>>>>    arch/loongarch/include/asm/kvm_host.h |  2 ++
>>>>    arch/loongarch/kvm/main.c             |  1 +
>>>>    arch/loongarch/kvm/mmu.c              |  4 ++--
>>>>    arch/loongarch/kvm/tlb.c              |  5 +----
>>>>    arch/loongarch/kvm/vcpu.c             | 18 ++++++++++++++++++
>>>>    5 files changed, 24 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/arch/loongarch/include/asm/kvm_host.h b/arch/loongarch/include/asm/kvm_host.h
>>>> index c87b6ea0ec47..32c4948f534f 100644
>>>> --- a/arch/loongarch/include/asm/kvm_host.h
>>>> +++ b/arch/loongarch/include/asm/kvm_host.h
>>>> @@ -30,6 +30,7 @@
>>>>    #define KVM_PRIVATE_MEM_SLOTS          0
>>>>
>>>>    #define KVM_HALT_POLL_NS_DEFAULT       500000
>>>> +#define KVM_REQ_TLB_FLUSH_GPA          KVM_ARCH_REQ(0)
>>>>
>>>>    #define KVM_GUESTDBG_SW_BP_MASK                \
>>>>           (KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP)
>>>> @@ -190,6 +191,7 @@ struct kvm_vcpu_arch {
>>>>
>>>>           /* vcpu's vpid */
>>>>           u64 vpid;
>>>> +       gpa_t flush_gpa;
>>>>
>>>>           /* Frequency of stable timer in Hz */
>>>>           u64 timer_mhz;
>>>> diff --git a/arch/loongarch/kvm/main.c b/arch/loongarch/kvm/main.c
>>>> index 86a2f2d0cb27..844736b99d38 100644
>>>> --- a/arch/loongarch/kvm/main.c
>>>> +++ b/arch/loongarch/kvm/main.c
>>>> @@ -242,6 +242,7 @@ void kvm_check_vpid(struct kvm_vcpu *vcpu)
>>>>                   kvm_update_vpid(vcpu, cpu);
>>>>                   trace_kvm_vpid_change(vcpu, vcpu->arch.vpid);
>>>>                   vcpu->cpu = cpu;
>>>> +               kvm_clear_request(KVM_REQ_TLB_FLUSH_GPA, vcpu);
>>>>           }
>>>>
>>>>           /* Restore GSTAT(0x50).vpid */
>>>> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
>>>> index 98883aa23ab8..9e39d28fec35 100644
>>>> --- a/arch/loongarch/kvm/mmu.c
>>>> +++ b/arch/loongarch/kvm/mmu.c
>>>> @@ -908,8 +908,8 @@ int kvm_handle_mm_fault(struct kvm_vcpu *vcpu, unsigned long gpa, bool write)
>>>>                   return ret;
>>>>
>>>>           /* Invalidate this entry in the TLB */
>>>> -       kvm_flush_tlb_gpa(vcpu, gpa);
>>>> -
>>>> +       vcpu->arch.flush_gpa = gpa;
>>>> +       kvm_make_request(KVM_REQ_TLB_FLUSH_GPA, vcpu);
>>>>           return 0;
>>>>    }
>>>>
>>>> diff --git a/arch/loongarch/kvm/tlb.c b/arch/loongarch/kvm/tlb.c
>>>> index 02535df6b51f..ebdbe9264e9c 100644
>>>> --- a/arch/loongarch/kvm/tlb.c
>>>> +++ b/arch/loongarch/kvm/tlb.c
>>>> @@ -23,10 +23,7 @@ void kvm_flush_tlb_all(void)
>>>>
>>>>    void kvm_flush_tlb_gpa(struct kvm_vcpu *vcpu, unsigned long gpa)
>>>>    {
>>>> -       unsigned long flags;
>>>> -
>>>> -       local_irq_save(flags);
>>>> +       lockdep_assert_irqs_disabled();
>>>>           gpa &= (PAGE_MASK << 1);
>>>>           invtlb(INVTLB_GID_ADDR, read_csr_gstat() & CSR_GSTAT_GID, gpa);
>>>> -       local_irq_restore(flags);
>>>>    }
>>>> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
>>>> index 9e8030d45129..b747bd8bc037 100644
>>>> --- a/arch/loongarch/kvm/vcpu.c
>>>> +++ b/arch/loongarch/kvm/vcpu.c
>>>> @@ -51,6 +51,16 @@ static int kvm_check_requests(struct kvm_vcpu *vcpu)
>>>>           return RESUME_GUEST;
>>>>    }
>>>>
>>>> +static void kvm_late_check_requests(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +       lockdep_assert_irqs_disabled();
>>>> +       if (kvm_check_request(KVM_REQ_TLB_FLUSH_GPA, vcpu))
>>>> +               if (vcpu->arch.flush_gpa != INVALID_GPA) {
>>>> +                       kvm_flush_tlb_gpa(vcpu, vcpu->arch.flush_gpa);
>>>> +                       vcpu->arch.flush_gpa = INVALID_GPA;
>>>> +               }
>>>> +}
>>>> +
>>>>    /*
>>>>     * Check and handle pending signal and vCPU requests etc
>>>>     * Run with irq enabled and preempt enabled
>>>> @@ -101,6 +111,13 @@ static int kvm_pre_enter_guest(struct kvm_vcpu *vcpu)
>>>>                   /* Make sure the vcpu mode has been written */
>>>>                   smp_store_mb(vcpu->mode, IN_GUEST_MODE);
>>>>                   kvm_check_vpid(vcpu);
>>>> +
>>>> +               /*
>>>> +                * Called after function kvm_check_vpid()
>>>> +                * Since it updates csr_gstat used by kvm_flush_tlb_gpa(),
>>>> +                * also it may clear KVM_REQ_TLB_FLUSH_GPA pending bit
>>>> +                */
>>>> +               kvm_late_check_requests(vcpu);
>>>>                   vcpu->arch.host_eentry = csr_read64(LOONGARCH_CSR_EENTRY);
>>>>                   /* Clear KVM_LARCH_SWCSR_LATEST as CSR will change when enter guest */
>>>>                   vcpu->arch.aux_inuse &= ~KVM_LARCH_SWCSR_LATEST;
>>>> @@ -994,6 +1011,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>>>>           struct loongarch_csrs *csr;
>>>>
>>>>           vcpu->arch.vpid = 0;
>>>> +       vcpu->arch.flush_gpa = INVALID_GPA;
>>>>
>>>>           hrtimer_init(&vcpu->arch.swtimer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_PINNED);
>>>>           vcpu->arch.swtimer.function = kvm_swtimer_wakeup;
>>>> --
>>>> 2.39.3
>>>>
>>
>>

Re: [PATCH v2 4/6] LoongArch: KVM: Add memory barrier before update pmd entry

[Huacai Chen]

On Mon, Jun 24, 2024 at 10:21 AM maobibo <maobibo@loongson.cn> wrote:
>
>
>
> On 2024/6/24 上午9:56, Huacai Chen wrote:
> > On Mon, Jun 24, 2024 at 9:37 AM maobibo <maobibo@loongson.cn> wrote:
> >>
> >>
> >>
> >> On 2024/6/23 下午6:18, Huacai Chen wrote:
> >>> Hi, Bibo,
> >>>
> >>> On Wed, Jun 19, 2024 at 4:09 PM Bibo Mao <maobibo@loongson.cn> wrote:
> >>>>
> >>>> When updating pmd entry such as allocating new pmd page or splitting
> >>>> huge page into normal page, it is necessary to firstly update all pte
> >>>> entries, and then update pmd entry.
> >>>>
> >>>> It is weak order with LoongArch system, there will be problem if other
> >>>> vcpus sees pmd update firstly however pte is not updated. Here smp_wmb()
> >>>> is added to assure this.
> >>> Memory barriers should be in pairs in most cases. That means you may
> >>> lose smp_rmb() in another place.
> >> The idea adding smp_wmb() comes from function __split_huge_pmd_locked()
> >> in file mm/huge_memory.c, and the explanation is reasonable.
> >>
> >>                   ...
> >>                   set_ptes(mm, haddr, pte, entry, HPAGE_PMD_NR);
> >>           }
> >>           ...
> >>           smp_wmb(); /* make pte visible before pmd */
> >>           pmd_populate(mm, pmd, pgtable);
> >>
> >> It is strange that why smp_rmb() should be in pairs with smp_wmb(),
> >> I never hear this rule -:(
> > https://docs.kernel.org/core-api/wrappers/memory-barriers.html
> >
> > SMP BARRIER PAIRING
> > -------------------
> >
> > When dealing with CPU-CPU interactions, certain types of memory barrier should
> > always be paired.  A lack of appropriate pairing is almost certainly an error.
>     CPU 1                 CPU 2
>          ===============       ===============
>          WRITE_ONCE(a, 1);
>          <write barrier>
>          WRITE_ONCE(b, 2);     x = READ_ONCE(b);
>                                <read barrier>
>                                y = READ_ONCE(a);
>
> With split_huge scenery to update pte/pmd entry, there is no strong
> relationship between address ptex and pmd.
> CPU1
>       WRITE_ONCE(pte0, 1);
>       WRITE_ONCE(pte511, 1);
>       <write barrier>
>       WRITE_ONCE(pmd, 2);
>
> However with page table walk scenery, address ptep depends on the
> contents of pmd, so it is not necessary to add smp_rmb().
>          ptep = pte_offset_map_lock(mm, pmd, address, &ptl);
>          if (!ptep)
>                  return no_page_table(vma, flags, address);
>          pte = ptep_get(ptep);
>          if (!pte_present(pte))
>
> It is just my option, or do you think where smp_rmb() barrier should be
> added in page table reader path?
There are some possibilities:
1. Read barrier is missing in some places;
2. Write barrier is also unnecessary here;
3. Read barrier is really unnecessary, but there is a better API to
replace the write barrier;
4. Read barrier is really unnecessary, and write barrier is really the
best API here.

Maybe Rui Wang knows better here.

Huacai

>
> Regards
> Bibo Mao
> >
> >
> > Huacai
> >
> >>
> >> Regards
> >> Bibo Mao
> >>>
> >>> Huacai
> >>>
> >>>>
> >>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> >>>> ---
> >>>>    arch/loongarch/kvm/mmu.c | 2 ++
> >>>>    1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
> >>>> index 1690828bd44b..7f04edfbe428 100644
> >>>> --- a/arch/loongarch/kvm/mmu.c
> >>>> +++ b/arch/loongarch/kvm/mmu.c
> >>>> @@ -163,6 +163,7 @@ static kvm_pte_t *kvm_populate_gpa(struct kvm *kvm,
> >>>>
> >>>>                           child = kvm_mmu_memory_cache_alloc(cache);
> >>>>                           _kvm_pte_init(child, ctx.invalid_ptes[ctx.level - 1]);
> >>>> +                       smp_wmb(); /* make pte visible before pmd */
> >>>>                           kvm_set_pte(entry, __pa(child));
> >>>>                   } else if (kvm_pte_huge(*entry)) {
> >>>>                           return entry;
> >>>> @@ -746,6 +747,7 @@ static kvm_pte_t *kvm_split_huge(struct kvm_vcpu *vcpu, kvm_pte_t *ptep, gfn_t g
> >>>>                   val += PAGE_SIZE;
> >>>>           }
> >>>>
> >>>> +       smp_wmb();
> >>>>           /* The later kvm_flush_tlb_gpa() will flush hugepage tlb */
> >>>>           kvm_set_pte(ptep, __pa(child));
> >>>>
> >>>> --
> >>>> 2.39.3
> >>>>
> >>
> >>
>
>

Re: [PATCH v2 4/6] LoongArch: KVM: Add memory barrier before update pmd entry

[WANG Rui]

Hi,

On Mon, Jun 24, 2024 at 12:18 PM Huacai Chen <chenhuacai@kernel.org> wrote:
>
> On Mon, Jun 24, 2024 at 10:21 AM maobibo <maobibo@loongson.cn> wrote:
> >
> >
> >
> > On 2024/6/24 上午9:56, Huacai Chen wrote:
> > > On Mon, Jun 24, 2024 at 9:37 AM maobibo <maobibo@loongson.cn> wrote:
> > >>
> > >>
> > >>
> > >> On 2024/6/23 下午6:18, Huacai Chen wrote:
> > >>> Hi, Bibo,
> > >>>
> > >>> On Wed, Jun 19, 2024 at 4:09 PM Bibo Mao <maobibo@loongson.cn> wrote:
> > >>>>
> > >>>> When updating pmd entry such as allocating new pmd page or splitting
> > >>>> huge page into normal page, it is necessary to firstly update all pte
> > >>>> entries, and then update pmd entry.
> > >>>>
> > >>>> It is weak order with LoongArch system, there will be problem if other
> > >>>> vcpus sees pmd update firstly however pte is not updated. Here smp_wmb()
> > >>>> is added to assure this.
> > >>> Memory barriers should be in pairs in most cases. That means you may
> > >>> lose smp_rmb() in another place.
> > >> The idea adding smp_wmb() comes from function __split_huge_pmd_locked()
> > >> in file mm/huge_memory.c, and the explanation is reasonable.
> > >>
> > >>                   ...
> > >>                   set_ptes(mm, haddr, pte, entry, HPAGE_PMD_NR);
> > >>           }
> > >>           ...
> > >>           smp_wmb(); /* make pte visible before pmd */
> > >>           pmd_populate(mm, pmd, pgtable);
> > >>
> > >> It is strange that why smp_rmb() should be in pairs with smp_wmb(),
> > >> I never hear this rule -:(
> > > https://docs.kernel.org/core-api/wrappers/memory-barriers.html
> > >
> > > SMP BARRIER PAIRING
> > > -------------------
> > >
> > > When dealing with CPU-CPU interactions, certain types of memory barrier should
> > > always be paired.  A lack of appropriate pairing is almost certainly an error.
> >     CPU 1                 CPU 2
> >          ===============       ===============
> >          WRITE_ONCE(a, 1);
> >          <write barrier>
> >          WRITE_ONCE(b, 2);     x = READ_ONCE(b);
> >                                <read barrier>
> >                                y = READ_ONCE(a);
> >
> > With split_huge scenery to update pte/pmd entry, there is no strong
> > relationship between address ptex and pmd.
> > CPU1
> >       WRITE_ONCE(pte0, 1);
> >       WRITE_ONCE(pte511, 1);
> >       <write barrier>
> >       WRITE_ONCE(pmd, 2);
> >
> > However with page table walk scenery, address ptep depends on the
> > contents of pmd, so it is not necessary to add smp_rmb().
> >          ptep = pte_offset_map_lock(mm, pmd, address, &ptl);
> >          if (!ptep)
> >                  return no_page_table(vma, flags, address);
> >          pte = ptep_get(ptep);
> >          if (!pte_present(pte))
> >
> > It is just my option, or do you think where smp_rmb() barrier should be
> > added in page table reader path?
> There are some possibilities:
> 1. Read barrier is missing in some places;
> 2. Write barrier is also unnecessary here;
> 3. Read barrier is really unnecessary, but there is a better API to
> replace the write barrier;
> 4. Read barrier is really unnecessary, and write barrier is really the
> best API here.
>
> Maybe Rui Wang knows better here.

It appears that reading the pte address is data-dependent on the pmd,
rather than control-dependent. This creates an opportunity to omit the
read-side memory barrier.

Cheers,
-Rui


>
> Huacai
>
> >
> > Regards
> > Bibo Mao
> > >
> > >
> > > Huacai
> > >
> > >>
> > >> Regards
> > >> Bibo Mao
> > >>>
> > >>> Huacai
> > >>>
> > >>>>
> > >>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> > >>>> ---
> > >>>>    arch/loongarch/kvm/mmu.c | 2 ++
> > >>>>    1 file changed, 2 insertions(+)
> > >>>>
> > >>>> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
> > >>>> index 1690828bd44b..7f04edfbe428 100644
> > >>>> --- a/arch/loongarch/kvm/mmu.c
> > >>>> +++ b/arch/loongarch/kvm/mmu.c
> > >>>> @@ -163,6 +163,7 @@ static kvm_pte_t *kvm_populate_gpa(struct kvm *kvm,
> > >>>>
> > >>>>                           child = kvm_mmu_memory_cache_alloc(cache);
> > >>>>                           _kvm_pte_init(child, ctx.invalid_ptes[ctx.level - 1]);
> > >>>> +                       smp_wmb(); /* make pte visible before pmd */
> > >>>>                           kvm_set_pte(entry, __pa(child));
> > >>>>                   } else if (kvm_pte_huge(*entry)) {
> > >>>>                           return entry;
> > >>>> @@ -746,6 +747,7 @@ static kvm_pte_t *kvm_split_huge(struct kvm_vcpu *vcpu, kvm_pte_t *ptep, gfn_t g
> > >>>>                   val += PAGE_SIZE;
> > >>>>           }
> > >>>>
> > >>>> +       smp_wmb();
> > >>>>           /* The later kvm_flush_tlb_gpa() will flush hugepage tlb */
> > >>>>           kvm_set_pte(ptep, __pa(child));
> > >>>>
> > >>>> --
> > >>>> 2.39.3
> > >>>>
> > >>
> > >>
> >
> >
>