From: Sean Christopherson <seanjc@google.com>
To: Ricardo Koller <ricarkol@google.com>
Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
andrew.jones@linux.dev, pbonzini@redhat.com, maz@kernel.org,
alexandru.elisei@arm.com, eric.auger@redhat.com,
oupton@google.com, reijiw@google.com, rananta@google.com,
bgardon@google.com, dmatlack@google.com,
axelrasmussen@google.com, Oliver Upton <oliver.upton@linux.dev>
Subject: Re: [PATCH v7 08/13] KVM: selftests: Use the right memslot for code, page-tables, and data allocations
Date: Tue, 20 Sep 2022 18:07:13 +0000 [thread overview]
Message-ID: <YyoBUcSD6ZyxKxza@google.com> (raw)
In-Reply-To: <20220920042551.3154283-9-ricarkol@google.com>
On Tue, Sep 20, 2022, Ricardo Koller wrote:
> The previous commit added support for callers of ____vm_create() to specify
Changelog is stale, ____vm_create() no longer takes the struct.
Side topic, it's usually a good idea to use "strong" terminology when referencing
past/future changes, e.g. if patches get shuffled around for whatever reason,
then "previous commit" may become stale/misleading.
It's fairly easy to convey the same info ("something happened recently" or
"something is going to happen soon") without being so explicit, e.g.
Wire up common code to use the appropriate code, page table, and data
memmslots that were recently added instead of hardcoding '0' for the
memslot.
or
Now that kvm_vm allows specifying different memslots for code, page
tables, and data, use the appropriate memslot when making allocations
in common/libraty code.
> what memslots to use for code, page-tables, and data allocations. Change
> them accordingly:
>
> - stacks, code, and exception tables use the code memslot
Huh? Stacks and exceptions are very much data, not code.
> - page tables and the pgd use the pt memslot
> - data (anything allocated with vm_vaddr_alloc()) uses the data memslot
>
> No functional change intended. All allocators keep using memslot #0.
...
> -vm_vaddr_t vm_vaddr_alloc(struct kvm_vm *vm, size_t sz, vm_vaddr_t vaddr_min)
> +vm_vaddr_t __vm_vaddr_alloc(struct kvm_vm *vm, size_t sz,
> + vm_vaddr_t vaddr_min, enum kvm_mem_region_type mrt)
Similar to other feedback, I'd prefer "type" over "mrt".
> {
> uint64_t pages = (sz >> vm->page_shift) + ((sz % vm->page_size) != 0);
>
> virt_pgd_alloc(vm);
> vm_paddr_t paddr = vm_phy_pages_alloc(vm, pages,
> - KVM_UTIL_MIN_PFN * vm->page_size, 0);
> + KVM_UTIL_MIN_PFN * vm->page_size,
> + vm->memslots[mrt]);
Please align parameters.
>
> /*
> * Find an unused range of virtual page addresses of at least
> @@ -1230,6 +1213,30 @@ vm_vaddr_t vm_vaddr_alloc(struct kvm_vm *vm, size_t sz, vm_vaddr_t vaddr_min)
> return vaddr_start;
> }
...
> +vm_vaddr_t vm_vaddr_alloc(struct kvm_vm *vm, size_t sz, vm_vaddr_t vaddr_min)
> +{
> + return __vm_vaddr_alloc(vm, sz, vaddr_min, MEM_REGION_DATA);
I like the vm_alloc_page_table() wrapper, what about adding vm_alloc_code() and
vm_alloc_data() too? Feel free to ignore if it's not much of a benefit.
> +}
> +
> /*
> * VM Virtual Address Allocate Pages
> *
> @@ -1249,6 +1256,11 @@ vm_vaddr_t vm_vaddr_alloc_pages(struct kvm_vm *vm, int nr_pages)
> return vm_vaddr_alloc(vm, nr_pages * getpagesize(), KVM_UTIL_MIN_VADDR);
> }
>
> +vm_vaddr_t __vm_vaddr_alloc_page(struct kvm_vm *vm, enum kvm_mem_region_type mrt)
> +{
> + return __vm_vaddr_alloc(vm, getpagesize(), KVM_UTIL_MIN_VADDR, mrt);
> +}
> +
> /*
> * VM Virtual Address Allocate Page
> *
> @@ -1814,7 +1826,8 @@ vm_paddr_t vm_phy_page_alloc(struct kvm_vm *vm, vm_paddr_t paddr_min,
>
> vm_paddr_t vm_alloc_page_table(struct kvm_vm *vm)
> {
> - return vm_phy_page_alloc(vm, KVM_GUEST_PAGE_TABLE_MIN_PADDR, 0);
> + return vm_phy_page_alloc(vm, KVM_GUEST_PAGE_TABLE_MIN_PADDR,
> + vm->memslots[MEM_REGION_PT]);
> }
>
> /*
> diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c
> index 604478151212..26c8d3dffb9a 100644
> --- a/tools/testing/selftests/kvm/lib/riscv/processor.c
> +++ b/tools/testing/selftests/kvm/lib/riscv/processor.c
> @@ -58,7 +58,7 @@ void virt_arch_pgd_alloc(struct kvm_vm *vm)
> if (!vm->pgd_created) {
> vm_paddr_t paddr = vm_phy_pages_alloc(vm,
> page_align(vm, ptrs_per_pte(vm) * 8) / vm->page_size,
> - KVM_GUEST_PAGE_TABLE_MIN_PADDR, 0);
> + KVM_GUEST_PAGE_TABLE_MIN_PADDR, vm->memslots[MEM_REGION_PT]);
Ah, more copy-paste from aarch64. Not your code (though the s390x change below is
new badness), but align params please. Similar to newlines before function names,
running over the 80 char soft limit is preferrable to weird alignment. And for
the soft limit, it's often easy to stay under the soft limit by refactoring,
e.g. in a separate prep patch for RISC-V and aarch64, do:
size_t nr_pages = page_align(vm, ptrs_per_pgd(vm) * 8) / vm->page_size;
if (vm->pgd_created)
return;
vm->pgd = vm_phy_pages_alloc(vm, nr_pages,
KVM_GUEST_PAGE_TABLE_MIN_PADDR,
vm->memslots[MEM_REGION_PT]);
vm->pgd_created = true;
> vm->pgd = paddr;
> vm->pgd_created = true;
> }
> @@ -282,8 +282,9 @@ struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
> size_t stack_size = vm->page_size == 4096 ?
> DEFAULT_STACK_PGS * vm->page_size :
> vm->page_size;
> - unsigned long stack_vaddr = vm_vaddr_alloc(vm, stack_size,
> - DEFAULT_RISCV_GUEST_STACK_VADDR_MIN);
> + unsigned long stack_vaddr = __vm_vaddr_alloc(vm, stack_size,
> + DEFAULT_RISCV_GUEST_STACK_VADDR_MIN,
> + MEM_REGION_CODE);
Stack is data, not code.
> unsigned long current_gp = 0;
> struct kvm_mp_state mps;
> struct kvm_vcpu *vcpu;
> diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
> index 89d7340d9cbd..410ae2b59847 100644
> --- a/tools/testing/selftests/kvm/lib/s390x/processor.c
> +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
> @@ -21,7 +21,7 @@ void virt_arch_pgd_alloc(struct kvm_vm *vm)
> return;
>
> paddr = vm_phy_pages_alloc(vm, PAGES_PER_REGION,
> - KVM_GUEST_PAGE_TABLE_MIN_PADDR, 0);
> + KVM_GUEST_PAGE_TABLE_MIN_PADDR, vm->memslots[MEM_REGION_PT]);
Please align.
paddr = vm_phy_pages_alloc(vm, PAGES_PER_REGION,
KVM_GUEST_PAGE_TABLE_MIN_PADDR,
vm->memslots[MEM_REGION_PT]);
> memset(addr_gpa2hva(vm, paddr), 0xff, PAGES_PER_REGION * vm->page_size);
>
> vm->pgd = paddr;
> @@ -167,8 +167,9 @@ struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
> TEST_ASSERT(vm->page_size == 4096, "Unsupported page size: 0x%x",
> vm->page_size);
>
> - stack_vaddr = vm_vaddr_alloc(vm, stack_size,
> - DEFAULT_GUEST_STACK_VADDR_MIN);
> + stack_vaddr = __vm_vaddr_alloc(vm, stack_size,
> + DEFAULT_GUEST_STACK_VADDR_MIN,
> + MEM_REGION_CODE);
Same bug here.
>
> vcpu = __vm_vcpu_add(vm, vcpu_id);
>
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 2e6e61bbe81b..f7b90a6c7d19 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -525,7 +525,7 @@ vm_paddr_t addr_arch_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva)
> static void kvm_setup_gdt(struct kvm_vm *vm, struct kvm_dtable *dt)
> {
> if (!vm->gdt)
> - vm->gdt = vm_vaddr_alloc_page(vm);
> + vm->gdt = __vm_vaddr_alloc_page(vm, MEM_REGION_CODE);
GDT is data, not code.
> dt->base = vm->gdt;
> dt->limit = getpagesize();
> @@ -535,7 +535,7 @@ static void kvm_setup_tss_64bit(struct kvm_vm *vm, struct kvm_segment *segp,
> int selector)
> {
> if (!vm->tss)
> - vm->tss = vm_vaddr_alloc_page(vm);
> + vm->tss = __vm_vaddr_alloc_page(vm, MEM_REGION_CODE);
TSS is data, not code.
>
> memset(segp, 0, sizeof(*segp));
> segp->base = vm->tss;
> @@ -620,8 +620,9 @@ struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
> vm_vaddr_t stack_vaddr;
> struct kvm_vcpu *vcpu;
>
> - stack_vaddr = vm_vaddr_alloc(vm, DEFAULT_STACK_PGS * getpagesize(),
> - DEFAULT_GUEST_STACK_VADDR_MIN);
> + stack_vaddr = __vm_vaddr_alloc(vm, DEFAULT_STACK_PGS * getpagesize(),
> + DEFAULT_GUEST_STACK_VADDR_MIN,
> + MEM_REGION_CODE);
Stack is data, not code.
> vcpu = __vm_vcpu_add(vm, vcpu_id);
> vcpu_init_cpuid(vcpu, kvm_get_supported_cpuid());
> @@ -1118,8 +1119,8 @@ void vm_init_descriptor_tables(struct kvm_vm *vm)
> extern void *idt_handlers;
> int i;
>
> - vm->idt = vm_vaddr_alloc_page(vm);
> - vm->handlers = vm_vaddr_alloc_page(vm);
> + vm->idt = __vm_vaddr_alloc_page(vm, MEM_REGION_CODE);
IDT is data, not code.
> + vm->handlers = __vm_vaddr_alloc_page(vm, MEM_REGION_CODE);
> /* Handlers have the same address in both address spaces.*/
> for (i = 0; i < NUM_INTERRUPTS; i++)
> set_idt_entry(vm, i, (unsigned long)(&idt_handlers)[i], 0,
> --
> 2.37.3.968.ga6b4b080e4-goog
>
next prev parent reply other threads:[~2022-09-20 18:07 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-20 4:25 [PATCH v7 00/13] KVM: selftests: Add aarch64/page_fault_test Ricardo Koller
2022-09-20 4:25 ` [PATCH v7 01/13] KVM: selftests: Add a userfaultfd library Ricardo Koller
2022-09-20 4:25 ` [PATCH v7 02/13] KVM: selftests: aarch64: Add virt_get_pte_hva() library function Ricardo Koller
2022-09-20 4:25 ` [PATCH v7 03/13] KVM: selftests: Add missing close and munmap in __vm_mem_region_delete() Ricardo Koller
2022-09-20 4:25 ` [PATCH v7 04/13] KVM: selftests: aarch64: Construct DEFAULT_MAIR_EL1 using sysreg.h macros Ricardo Koller
2022-09-20 4:25 ` [PATCH v7 05/13] tools: Copy bitfield.h from the kernel sources Ricardo Koller
2022-09-20 4:25 ` [PATCH v7 06/13] KVM: selftests: Stash backing_src_type in struct userspace_mem_region Ricardo Koller
2022-09-20 4:25 ` [PATCH v7 07/13] KVM: selftests: Add vm->memslots[] and enum kvm_mem_region_type Ricardo Koller
2022-09-20 7:50 ` Andrew Jones
2022-09-20 17:39 ` Sean Christopherson
2022-09-20 17:49 ` Ricardo Koller
2022-09-20 4:25 ` [PATCH v7 08/13] KVM: selftests: Use the right memslot for code, page-tables, and data allocations Ricardo Koller
2022-09-20 7:51 ` Andrew Jones
2022-09-20 18:07 ` Sean Christopherson [this message]
2022-09-20 18:23 ` Ricardo Koller
2022-09-20 18:40 ` Sean Christopherson
2022-09-20 18:57 ` Ricardo Koller
2022-09-20 4:25 ` [PATCH v7 09/13] KVM: selftests: aarch64: Add aarch64/page_fault_test Ricardo Koller
2022-09-20 8:05 ` Andrew Jones
2022-09-20 17:13 ` Ricardo Koller
2022-09-20 4:25 ` [PATCH v7 10/13] KVM: selftests: aarch64: Add userfaultfd tests into page_fault_test Ricardo Koller
2022-09-20 4:25 ` [PATCH v7 11/13] KVM: selftests: aarch64: Add dirty logging " Ricardo Koller
2022-09-20 4:25 ` [PATCH v7 12/13] KVM: selftests: aarch64: Add readonly memslot " Ricardo Koller
2022-09-20 4:25 ` [PATCH v7 13/13] KVM: selftests: aarch64: Add mix of " Ricardo Koller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YyoBUcSD6ZyxKxza@google.com \
--to=seanjc@google.com \
--cc=alexandru.elisei@arm.com \
--cc=andrew.jones@linux.dev \
--cc=axelrasmussen@google.com \
--cc=bgardon@google.com \
--cc=dmatlack@google.com \
--cc=eric.auger@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=maz@kernel.org \
--cc=oliver.upton@linux.dev \
--cc=oupton@google.com \
--cc=pbonzini@redhat.com \
--cc=rananta@google.com \
--cc=reijiw@google.com \
--cc=ricarkol@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox