From: Ricardo Koller <ricarkol@google.com>
To: Sean Christopherson <seanjc@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 11:23:00 -0700 [thread overview]
Message-ID: <YyoFBBn9uevAkIHT@google.com> (raw)
In-Reply-To: <YyoBUcSD6ZyxKxza@google.com>
On Tue, Sep 20, 2022 at 06:07:13PM +0000, Sean Christopherson wrote:
> 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.
>
I would *really* like to have the data region only store test data. It
makes things easier for the test implementation, like owning the whole
region. At the same I wanted to have a single region for all the "core
pages" like code, descriptors, exception tables, stacks, etc. Not sure
what to call it though. So, what about one of these 2 options:
Option A: 3 regions, where we call the "code" region something else, like
"core".
Option B: 4 regions: code, page-tables, core-data (stacks, exception tables, etc),
test-data.
> > - 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:23 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
2022-09-20 18:23 ` Ricardo Koller [this message]
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=YyoFBBn9uevAkIHT@google.com \
--to=ricarkol@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=seanjc@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