Messages in this thread | | | Subject | Re: [PATCH v1 2/4] selftests: kvm: Add processor tests for LoongArch KVM | From | zhaotianrui <> | Date | Thu, 3 Aug 2023 14:32:03 +0800 |
| |
在 2023/8/3 上午2:07, Sean Christopherson 写道: > On Tue, Aug 01, 2023, Tianrui Zhao wrote: >> Add processor tests for LoongArch KVM, including vcpu initialize > Nit, AFAICT these aren't tests, this is simply the core KVM selftests support > for LoongArch. Thanks, I will fix this comment. > >> and tlb refill exception handler. >> >> Based-on: <20230720062813.4126751-1-zhaotianrui@loongson.cn> >> Signed-off-by: Tianrui Zhao <zhaotianrui@loongson.cn> >> --- >> .../selftests/kvm/lib/loongarch/exception.S | 27 ++ >> .../selftests/kvm/lib/loongarch/processor.c | 367 ++++++++++++++++++ >> 2 files changed, 394 insertions(+) >> create mode 100644 tools/testing/selftests/kvm/lib/loongarch/exception.S >> create mode 100644 tools/testing/selftests/kvm/lib/loongarch/processor.c >> >> diff --git a/tools/testing/selftests/kvm/lib/loongarch/exception.S b/tools/testing/selftests/kvm/lib/loongarch/exception.S >> new file mode 100644 >> index 000000000000..19dc50993da4 >> --- /dev/null >> +++ b/tools/testing/selftests/kvm/lib/loongarch/exception.S >> @@ -0,0 +1,27 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> + >> +#include "sysreg.h" >> + >> +/* address of refill exception should be 4K aligned */ >> +.align 12 > .align works on bytes, not on shifts. I.e. this will make handle_tlb_refill > 12-byte aligned, not 4096-byte aligned. Thanks, I will fix it to .balign 4096. > >> +.global handle_tlb_refill >> +handle_tlb_refill: >> + csrwr t0, LOONGARCH_CSR_TLBRSAVE >> + csrrd t0, LOONGARCH_CSR_PGD >> + lddir t0, t0, 3 >> + lddir t0, t0, 1 >> + ldpte t0, 0 >> + ldpte t0, 1 >> + tlbfill >> + csrrd t0, LOONGARCH_CSR_TLBRSAVE >> + ertn >> + >> +/* address of general exception should be 4K aligned */ >> +.align 12 > Same thing here. I will fix it too. > >> +.global handle_exception >> +handle_exception: >> +1: >> + nop >> + b 1b >> + nop >> + ertn >> diff --git a/tools/testing/selftests/kvm/lib/loongarch/processor.c b/tools/testing/selftests/kvm/lib/loongarch/processor.c >> new file mode 100644 >> index 000000000000..2e50b6e2c556 >> --- /dev/null >> +++ b/tools/testing/selftests/kvm/lib/loongarch/processor.c >> @@ -0,0 +1,367 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * KVM selftest LoongArch library code, including CPU-related functions. >> + * > Again, unnecessary IMO. If you do keep the comment, the extra line with a bare > asterisk should be dropped. Thanks, I will remove this comment. > >> + */ >> + >> +#include <assert.h> >> +#include <linux/bitfield.h> >> +#include <linux/compiler.h> >> + >> +#include "kvm_util.h" >> +#include "processor.h" >> +#include "sysreg.h" >> + >> +#define DEFAULT_LOONGARCH_GUEST_STACK_VADDR_MIN 0xac0000 > Why diverge from the common? > > #define DEFAULT_GUEST_STACK_VADDR_MIN 0xab6000 > > AFAIK, the common value is also mostly arbitrary, but that just makes it even > more confusing as to why LoongArch needs to bump the min by 0x4000. This is reference from ARM, and I will fix it to use the the common value. > >> +uint64_t *virt_get_pte_hva(struct kvm_vm *vm, vm_vaddr_t gva) >> +{ >> + uint64_t *ptep; >> + >> + if (!vm->pgd_created) >> + goto unmapped_gva; >> + >> + ptep = addr_gpa2hva(vm, vm->pgd) + pgd_index(vm, gva) * 8; >> + if (!ptep) >> + goto unmapped_gva; >> + >> + switch (vm->pgtable_levels) { >> + case 4: >> + ptep = addr_gpa2hva(vm, pte_addr(vm, *ptep)) + pud_index(vm, gva) * 8; >> + if (!ptep) >> + goto unmapped_gva; > This wants a "fallthrough" annotation. Thanks, I will add the "fallthrough" annotation. > >> + case 3: >> + ptep = addr_gpa2hva(vm, pte_addr(vm, *ptep)) + pmd_index(vm, gva) * 8; >> + if (!ptep) >> + goto unmapped_gva; >> + case 2: >> + ptep = addr_gpa2hva(vm, pte_addr(vm, *ptep)) + pte_index(vm, gva) * 8; >> + if (!ptep) >> + goto unmapped_gva; >> + break; >> + default: >> + TEST_FAIL("Page table levels must be 2, 3, or 4"); > Obviously it shouldn't come up, but print the actual pgtable_levels to make debug > a wee bit easier e.g. > TEST_FAIL("Got %u page table levels, expected 2, 3, or 4", > vm->pgtable_levels); Thanks, I will also print the actual pgtable_levels in this debug function. > > Mostly out of curiosity, but also because it looks like this was heavily copy+pasted > from ARM: does LoongArch actually support 2-level page tables? Yes, this codes are mostly copy pasted from ARM, but LoongArch does not support 2-levels page tables, it only support 3-level and 4-level page tables, and I will fix it. >> +static void loongarch_set_csr(struct kvm_vcpu *vcpu, uint64_t id, uint64_t val) >> +{ >> + uint64_t csrid; >> + >> + csrid = KVM_REG_LOONGARCH_CSR | KVM_REG_SIZE_U64 | 8 * id; >> + vcpu_set_reg(vcpu, csrid, val); >> +} >> + >> +static void loongarch_vcpu_setup(struct kvm_vcpu *vcpu) >> +{ >> + unsigned long val; >> + int width; >> + struct kvm_vm *vm = vcpu->vm; >> + >> + switch (vm->mode) { >> + case VM_MODE_P48V48_16K: >> + case VM_MODE_P40V48_16K: >> + case VM_MODE_P36V48_16K: >> + case VM_MODE_P36V47_16K: >> + break; >> + >> + default: >> + TEST_FAIL("Unknown guest mode, mode: 0x%x", vm->mode); >> + } >> + >> + /* user mode and page enable mode */ >> + val = PLV_USER | CSR_CRMD_PG; >> + loongarch_set_csr(vcpu, LOONGARCH_CSR_CRMD, val); >> + loongarch_set_csr(vcpu, LOONGARCH_CSR_PRMD, val); >> + loongarch_set_csr(vcpu, LOONGARCH_CSR_EUEN, 1); >> + loongarch_set_csr(vcpu, LOONGARCH_CSR_ECFG, 0); >> + loongarch_set_csr(vcpu, LOONGARCH_CSR_TCFG, 0); >> + loongarch_set_csr(vcpu, LOONGARCH_CSR_ASID, 1); >> + >> + width = vm->page_shift - 3; >> + val = 0; >> + switch (vm->pgtable_levels) { >> + case 4: >> + /* pud page shift and width */ >> + val = (vm->page_shift + width * 2) << 20 | (width << 25); >> + case 3: >> + /* pmd page shift and width */ >> + val |= (vm->page_shift + width) << 10 | (width << 15); >> + case 2: >> + /* pte page shift and width */ >> + val |= vm->page_shift | width << 5; >> + break; >> + default: >> + TEST_FAIL("Page table levels must be 2, 3, or 4"); >> + } >> + loongarch_set_csr(vcpu, LOONGARCH_CSR_PWCTL0, val); >> + >> + /* pgd page shift and width */ >> + val = (vm->page_shift + width * (vm->pgtable_levels - 1)) | width << 6; >> + loongarch_set_csr(vcpu, LOONGARCH_CSR_PWCTL1, val); >> + >> + loongarch_set_csr(vcpu, LOONGARCH_CSR_PGDL, vm->pgd); >> + >> + extern void handle_tlb_refill(void); >> + extern void handle_exception(void); > Eww. I get that it's probably undesirable to expose these via processor.h, but > at least declare them outside of the function. Thanks, I will declare them outside of the function. > >> +struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id, >> + void *guest_code) >> +{ >> + return loongarch_vcpu_add(vm, vcpu_id, guest_code); > Please drop the single-line passthrough, i.e. drop loongarch_vcpu_add(). I'm > guessing you copy+pasted much of this from ARM. ARM's passthrough isn't a pure > passthrough, which is directly related to why the "passthrough" is ok: there are > other callers to aarch64_vcpu_add() that pass a non-NULL @source. Yes, this is also copy pasted from ARM, and I will drop the loongarch_vcpu_add() function and move the content of it to here.
| |