> -----Original Message----- > From: Anup Patel [mailto:anup@xxxxxxxxxxxxxx] > Sent: Monday, November 30, 2020 6:22 PM > To: Jiangyifei <jiangyifei@xxxxxxxxxx> > Cc: Anup Patel <anup.patel@xxxxxxx>; Palmer Dabbelt > <palmer@xxxxxxxxxxx>; Palmer Dabbelt <palmerdabbelt@xxxxxxxxxx>; Paul > Walmsley <paul.walmsley@xxxxxxxxxx>; Albert Ou <aou@xxxxxxxxxxxxxxxxx>; > Paolo Bonzini <pbonzini@xxxxxxxxxx>; Alexander Graf <graf@xxxxxxxxxx>; > Atish Patra <atish.patra@xxxxxxx>; Alistair Francis > <Alistair.Francis@xxxxxxx>; Damien Le Moal <damien.lemoal@xxxxxxx>; > kvm@xxxxxxxxxxxxxxx; kvm-riscv@xxxxxxxxxxxxxxxxxxx; > linux-riscv@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Zhangxiaofeng (F) > <victor.zhangxiaofeng@xxxxxxxxxx>; Wubin (H) <wu.wubin@xxxxxxxxxx>; > dengkai (A) <dengkai1@xxxxxxxxxx>; yinyipeng <yinyipeng1@xxxxxxxxxx> > Subject: Re: [PATCH v15 10/17] RISC-V: KVM: Implement stage2 page table > programming > > On Tue, Nov 24, 2020 at 2:56 PM Anup Patel <anup@xxxxxxxxxxxxxx> wrote: > > > > On Mon, Nov 16, 2020 at 2:59 PM Jiangyifei <jiangyifei@xxxxxxxxxx> wrote: > > > > > > > > > > -----Original Message----- > > > > From: Anup Patel [mailto:anup.patel@xxxxxxx] > > > > Sent: Monday, November 9, 2020 7:33 PM > > > > To: Palmer Dabbelt <palmer@xxxxxxxxxxx>; Palmer Dabbelt > > > > <palmerdabbelt@xxxxxxxxxx>; Paul Walmsley > > > > <paul.walmsley@xxxxxxxxxx>; Albert Ou <aou@xxxxxxxxxxxxxxxxx>; > > > > Paolo Bonzini <pbonzini@xxxxxxxxxx> > > > > Cc: Alexander Graf <graf@xxxxxxxxxx>; Atish Patra > > > > <atish.patra@xxxxxxx>; Alistair Francis > > > > <Alistair.Francis@xxxxxxx>; Damien Le Moal > > > > <damien.lemoal@xxxxxxx>; Anup Patel <anup@xxxxxxxxxxxxxx>; > > > > kvm@xxxxxxxxxxxxxxx; kvm-riscv@xxxxxxxxxxxxxxxxxxx; > > > > linux-riscv@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > > > > Anup Patel <anup.patel@xxxxxxx>; Jiangyifei > > > > <jiangyifei@xxxxxxxxxx> > > > > Subject: [PATCH v15 10/17] RISC-V: KVM: Implement stage2 page > > > > table programming > > > > > > > > This patch implements all required functions for programming the > > > > stage2 page table for each Guest/VM. > > > > > > > > At high-level, the flow of stage2 related functions is similar > > > > from KVM > > > > ARM/ARM64 implementation but the stage2 page table format is quite > > > > different for KVM RISC-V. > > > > > > > > [jiangyifei: stage2 dirty log support] > > > > Signed-off-by: Yifei Jiang <jiangyifei@xxxxxxxxxx> > > > > Signed-off-by: Anup Patel <anup.patel@xxxxxxx> > > > > Acked-by: Paolo Bonzini <pbonzini@xxxxxxxxxx> > > > > Reviewed-by: Paolo Bonzini <pbonzini@xxxxxxxxxx> > > > > --- > > > > arch/riscv/include/asm/kvm_host.h | 12 + > > > > arch/riscv/include/asm/pgtable-bits.h | 1 + > > > > arch/riscv/kvm/Kconfig | 1 + > > > > arch/riscv/kvm/main.c | 19 + > > > > arch/riscv/kvm/mmu.c | 649 > > > > +++++++++++++++++++++++++- > > > > arch/riscv/kvm/vm.c | 6 - > > > > 6 files changed, 672 insertions(+), 16 deletions(-) > > > > > > > > > > ...... > > > > > > > > > > > int kvm_riscv_stage2_map(struct kvm_vcpu *vcpu, @@ -69,27 > > > > +562,163 @@ int kvm_riscv_stage2_map(struct kvm_vcpu *vcpu, > > > > gpa_t gpa, unsigned long hva, > > > > bool writeable, bool is_write) { > > > > - /* TODO: */ > > > > - return 0; > > > > + int ret; > > > > + kvm_pfn_t hfn; > > > > + short vma_pageshift; > > > > + gfn_t gfn = gpa >> PAGE_SHIFT; > > > > + struct vm_area_struct *vma; > > > > + struct kvm *kvm = vcpu->kvm; > > > > + struct kvm_mmu_page_cache *pcache = > &vcpu->arch.mmu_page_cache; > > > > + bool logging = (memslot->dirty_bitmap && > > > > + !(memslot->flags & KVM_MEM_READONLY)) ? > true : false; > > > > + unsigned long vma_pagesize; > > > > + > > > > + mmap_read_lock(current->mm); > > > > + > > > > + vma = find_vma_intersection(current->mm, hva, hva + 1); > > > > + if (unlikely(!vma)) { > > > > + kvm_err("Failed to find VMA for hva 0x%lx\n", hva); > > > > + mmap_read_unlock(current->mm); > > > > + return -EFAULT; > > > > + } > > > > + > > > > + if (is_vm_hugetlb_page(vma)) > > > > + vma_pageshift = huge_page_shift(hstate_vma(vma)); > > > > + else > > > > + vma_pageshift = PAGE_SHIFT; > > > > + vma_pagesize = 1ULL << vma_pageshift; > > > > + if (logging || (vma->vm_flags & VM_PFNMAP)) > > > > + vma_pagesize = PAGE_SIZE; > > > > + > > > > + if (vma_pagesize == PMD_SIZE || vma_pagesize == PGDIR_SIZE) > > > > + gfn = (gpa & huge_page_mask(hstate_vma(vma))) >> > > > > + PAGE_SHIFT; > > > > + > > > > + mmap_read_unlock(current->mm); > > > > + > > > > + if (vma_pagesize != PGDIR_SIZE && > > > > + vma_pagesize != PMD_SIZE && > > > > + vma_pagesize != PAGE_SIZE) { > > > > + kvm_err("Invalid VMA page size 0x%lx\n", > vma_pagesize); > > > > + return -EFAULT; > > > > + } > > > > + > > > > + /* We need minimum second+third level pages */ > > > > + ret = stage2_cache_topup(pcache, stage2_pgd_levels, > > > > + > KVM_MMU_PAGE_CACHE_NR_OBJS); > > > > + if (ret) { > > > > + kvm_err("Failed to topup stage2 cache\n"); > > > > + return ret; > > > > + } > > > > + > > > > + hfn = gfn_to_pfn_prot(kvm, gfn, is_write, NULL); > > > > + if (hfn == KVM_PFN_ERR_HWPOISON) { > > > > + send_sig_mceerr(BUS_MCEERR_AR, (void __user *)hva, > > > > + vma_pageshift, current); > > > > + return 0; > > > > + } > > > > + if (is_error_noslot_pfn(hfn)) > > > > + return -EFAULT; > > > > + > > > > + /* > > > > + * If logging is active then we allow writable pages only > > > > + * for write faults. > > > > + */ > > > > + if (logging && !is_write) > > > > + writeable = false; > > > > + > > > > + spin_lock(&kvm->mmu_lock); > > > > + > > > > + if (writeable) { > > > > > > Hi Anup, > > > > > > What is the purpose of "writable = !memslot_is_readonly(slot)" in this > series? > > > > Where ? I don't see this line in any of the patches. > > > > > > > > When mapping the HVA to HPA above, it doesn't know that the PTE > writeable of stage2 is "!memslot_is_readonly(slot)". > > > This may causes the difference between the writability of HVA->HPA and > GPA->HPA. > > > For example, GPA->HPA is writeable, but HVA->HPA is not writeable. > > > > Yes, this is possible particularly when Host kernel is updating > > writability of HVA->HPA mappings for swapping in/out pages. > > > > > > > > Is it better that the writability of HVA->HPA is also determined by whether > the memslot is readonly in this change? > > > Like this: > > > - hfn = gfn_to_pfn_prot(kvm, gfn, is_write, NULL); > > > + hfn = gfn_to_pfn_prot(kvm, gfn, writeable, NULL); > > > > The gfn_to_pfn_prot() needs to know what type of fault we got (i.e > > read/write fault). Rest of the information (such as whether slot is > > writable or not) is already available to gfn_to_pfn_prot(). > > > > The question here is should we pass "&writeable" or NULL as last > > parameter to gfn_to_pfn_prot(). The recent JUMP label support in Linux > > RISC-V causes problem on HW where PTE 'A' and 'D' bits are not updated > > by HW so I have to change last parameter of gfn_to_pfn_prot() from > > "&writeable" to NULL. > > > > I am still investigating this. > > This turned-out to be a bug in Spike which is not fixed. > > I will include following change in v16 patch series: > > > diff --git a/arch/riscv/include/asm/kvm_host.h > b/arch/riscv/include/asm/kvm_host.h > index 241030956d47..dc2666b4180b 100644 > --- a/arch/riscv/include/asm/kvm_host.h > +++ b/arch/riscv/include/asm/kvm_host.h > @@ -232,8 +232,7 @@ void __kvm_riscv_hfence_gvma_all(void); > > int kvm_riscv_stage2_map(struct kvm_vcpu *vcpu, > struct kvm_memory_slot *memslot, > - gpa_t gpa, unsigned long hva, > - bool writeable, bool is_write); > + gpa_t gpa, unsigned long hva, bool is_write); > void kvm_riscv_stage2_flush_cache(struct kvm_vcpu *vcpu); int > kvm_riscv_stage2_alloc_pgd(struct kvm *kvm); void > kvm_riscv_stage2_free_pgd(struct kvm *kvm); diff --git > a/arch/riscv/kvm/mmu.c b/arch/riscv/kvm/mmu.c index > fcaeadc9b34d..56fda9ef70fd 100644 > --- a/arch/riscv/kvm/mmu.c > +++ b/arch/riscv/kvm/mmu.c > @@ -689,11 +689,11 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned > long hva) > > int kvm_riscv_stage2_map(struct kvm_vcpu *vcpu, > struct kvm_memory_slot *memslot, > - gpa_t gpa, unsigned long hva, > - bool writeable, bool is_write) > + gpa_t gpa, unsigned long hva, bool is_write) > { > int ret; > kvm_pfn_t hfn; > + bool writeable; > short vma_pageshift; > gfn_t gfn = gpa >> PAGE_SHIFT; > struct vm_area_struct *vma; > @@ -742,7 +742,7 @@ int kvm_riscv_stage2_map(struct kvm_vcpu *vcpu, > > mmu_seq = kvm->mmu_notifier_seq; > > - hfn = gfn_to_pfn_prot(kvm, gfn, is_write, NULL); > + hfn = gfn_to_pfn_prot(kvm, gfn, is_write, &writeable); > if (hfn == KVM_PFN_ERR_HWPOISON) { > send_sig_mceerr(BUS_MCEERR_AR, (void __user *)hva, > vma_pageshift, current); diff --git > a/arch/riscv/kvm/vcpu_exit.c b/arch/riscv/kvm/vcpu_exit.c index > f054406792a6..058cfa168abe 100644 > --- a/arch/riscv/kvm/vcpu_exit.c > +++ b/arch/riscv/kvm/vcpu_exit.c > @@ -445,7 +445,7 @@ static int stage2_page_fault(struct kvm_vcpu *vcpu, > struct kvm_run *run, > }; > } > > - ret = kvm_riscv_stage2_map(vcpu, memslot, fault_addr, hva, writeable, > + ret = kvm_riscv_stage2_map(vcpu, memslot, fault_addr, hva, > (trap->scause == EXC_STORE_GUEST_PAGE_FAULT) ? true : false); > if (ret < 0) > return ret; > > Regards, > Anup This change looks good. Yifei
