diff mbox series

[v7,08/13] KVM: selftests: Use the right memslot for code, page-tables, and data allocations

Message ID 20220920042551.3154283-9-ricarkol@google.com (mailing list archive)
State New, archived
Headers show
Series KVM: selftests: Add aarch64/page_fault_test | expand

Commit Message

Ricardo Koller Sept. 20, 2022, 4:25 a.m. UTC 
The previous commit added support for callers of ____vm_create() to specify
what memslots to use for code, page-tables, and data allocations. Change
them accordingly:

- stacks, code, and exception tables use the code memslot
- 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.

Reviewed-by: Oliver Upton <oliver.upton@linux.dev>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 .../selftests/kvm/include/kvm_util_base.h     |  3 +
 .../selftests/kvm/lib/aarch64/processor.c     | 11 ++--
 tools/testing/selftests/kvm/lib/elf.c         |  3 +-
 tools/testing/selftests/kvm/lib/kvm_util.c    | 57 ++++++++++++-------
 .../selftests/kvm/lib/riscv/processor.c       |  7 ++-
 .../selftests/kvm/lib/s390x/processor.c       |  7 ++-
 .../selftests/kvm/lib/x86_64/processor.c      | 13 +++--
 7 files changed, 61 insertions(+), 40 deletions(-)

Comments

Andrew Jones Sept. 20, 2022, 7:51 a.m. UTC | #1 
On Tue, Sep 20, 2022 at 04:25:46AM +0000, Ricardo Koller wrote:
> The previous commit added support for callers of ____vm_create() to specify
> what memslots to use for code, page-tables, and data allocations. Change
> them accordingly:
> 
> - stacks, code, and exception tables use the code memslot
> - 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.
> 
> Reviewed-by: Oliver Upton <oliver.upton@linux.dev>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Andrew Jones <andrew.jones@linux.dev>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  .../selftests/kvm/include/kvm_util_base.h     |  3 +
>  .../selftests/kvm/lib/aarch64/processor.c     | 11 ++--
>  tools/testing/selftests/kvm/lib/elf.c         |  3 +-
>  tools/testing/selftests/kvm/lib/kvm_util.c    | 57 ++++++++++++-------
>  .../selftests/kvm/lib/riscv/processor.c       |  7 ++-
>  .../selftests/kvm/lib/s390x/processor.c       |  7 ++-
>  .../selftests/kvm/lib/x86_64/processor.c      | 13 +++--
>  7 files changed, 61 insertions(+), 40 deletions(-)
>

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
Sean Christopherson Sept. 20, 2022, 6:07 p.m. UTC | #2 
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
>
Ricardo Koller Sept. 20, 2022, 6:23 p.m. UTC | #3 
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
> >
Sean Christopherson Sept. 20, 2022, 6:40 p.m. UTC | #4 
On Tue, Sep 20, 2022, Ricardo Koller wrote:
> 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.

That's fine, but allocating stack as "code" is super confusing.

> 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.

Why?  Code is very different than all those other things.  E.g. the main reason
KVM doesn't provide "not-executable" or "execute-only" memslots is because there's
never been a compelling use case, not because it's difficult to implement.  If KVM
were to ever add such functionality, then we'd want/need selftests to have a
dedicated code memslot.

> 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.

I like (B), though I'd just call 'em "DATA" and "TEST_DATA".  IIUC, TEST_DATA is
the one you want to be special, i.e. it's ok if something that's not "core" allocates
in DATA, but it's not ok if "core" allocates in TEST_DATA.  That yields an easy
to understand "never use TEST_DATA" rule for library/common/core functionality,
with the code vs. page tables vs. data decision (hopefully) being fairly obvious.

Defining CORE_DATA will force developers to make judgement calls and probably
lead to bikeshedding over whether something is considered "core" code.
Ricardo Koller Sept. 20, 2022, 6:57 p.m. UTC | #5 
On Tue, Sep 20, 2022 at 06:40:03PM +0000, Sean Christopherson wrote:
> On Tue, Sep 20, 2022, Ricardo Koller wrote:
> > 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.
> 
> That's fine, but allocating stack as "code" is super confusing.
> 
> > 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.
> 
> Why?  Code is very different than all those other things.  E.g. the main reason
> KVM doesn't provide "not-executable" or "execute-only" memslots is because there's
> never been a compelling use case, not because it's difficult to implement.  If KVM
> were to ever add such functionality, then we'd want/need selftests to have a
> dedicated code memslot.
> 
> > 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.
> 
> I like (B), though I'd just call 'em "DATA" and "TEST_DATA".  IIUC, TEST_DATA is
> the one you want to be special, i.e. it's ok if something that's not "core" allocates
> in DATA, but it's not ok if "core" allocates in TEST_DATA.  That yields an easy
> to understand "never use TEST_DATA" rule for library/common/core functionality,
> with the code vs. page tables vs. data decision (hopefully) being fairly obvious.
> 
> Defining CORE_DATA will force developers to make judgement calls and probably
> lead to bikeshedding over whether something is considered "core" code.

Sounds good, Option B then (with code, pt, data, test-data).

Thanks,
Ricardo
diff mbox series

Patch

diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 26187b8a66c6..59a33000903e 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -402,7 +402,10 @@  void vm_mem_region_move(struct kvm_vm *vm, uint32_t slot, uint64_t new_gpa);
 void vm_mem_region_delete(struct kvm_vm *vm, uint32_t slot);
 struct kvm_vcpu *__vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id);
 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 mr);
 vm_vaddr_t vm_vaddr_alloc_pages(struct kvm_vm *vm, int nr_pages);
+vm_vaddr_t __vm_vaddr_alloc_page(struct kvm_vm *vm, enum kvm_mem_region_type mr);
 vm_vaddr_t vm_vaddr_alloc_page(struct kvm_vm *vm);
 
 void virt_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
index 26f0eccff6fe..3d4bb11bbd85 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
@@ -79,7 +79,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_pgd(vm) * 8) / vm->page_size,
-			KVM_GUEST_PAGE_TABLE_MIN_PADDR, 0);
+			KVM_GUEST_PAGE_TABLE_MIN_PADDR, vm->memslots[MEM_REGION_PT]);
 		vm->pgd = paddr;
 		vm->pgd_created = true;
 	}
@@ -328,8 +328,9 @@  struct kvm_vcpu *aarch64_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;
-	uint64_t stack_vaddr = vm_vaddr_alloc(vm, stack_size,
-					      DEFAULT_ARM64_GUEST_STACK_VADDR_MIN);
+	uint64_t stack_vaddr = __vm_vaddr_alloc(vm, stack_size,
+						DEFAULT_ARM64_GUEST_STACK_VADDR_MIN,
+						MEM_REGION_CODE);
 	struct kvm_vcpu *vcpu = __vm_vcpu_add(vm, vcpu_id);
 
 	aarch64_vcpu_setup(vcpu, init);
@@ -435,8 +436,8 @@  void route_exception(struct ex_regs *regs, int vector)
 
 void vm_init_descriptor_tables(struct kvm_vm *vm)
 {
-	vm->handlers = vm_vaddr_alloc(vm, sizeof(struct handlers),
-			vm->page_size);
+	vm->handlers = __vm_vaddr_alloc(vm, sizeof(struct handlers),
+					vm->page_size, MEM_REGION_CODE);
 
 	*(vm_vaddr_t *)addr_gva2hva(vm, (vm_vaddr_t)(&exception_handlers)) = vm->handlers;
 }
diff --git a/tools/testing/selftests/kvm/lib/elf.c b/tools/testing/selftests/kvm/lib/elf.c
index 9f54c098d9d0..51f280c412ba 100644
--- a/tools/testing/selftests/kvm/lib/elf.c
+++ b/tools/testing/selftests/kvm/lib/elf.c
@@ -161,7 +161,8 @@  void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename)
 		seg_vend |= vm->page_size - 1;
 		size_t seg_size = seg_vend - seg_vstart + 1;
 
-		vm_vaddr_t vaddr = vm_vaddr_alloc(vm, seg_size, seg_vstart);
+		vm_vaddr_t vaddr = __vm_vaddr_alloc(vm, seg_size, seg_vstart,
+						    MEM_REGION_CODE);
 		TEST_ASSERT(vaddr == seg_vstart, "Unable to allocate "
 			"virtual memory for segment at requested min addr,\n"
 			"  segment idx: %u\n"
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 2e36d6323518..7e5d565f9b8c 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -1184,32 +1184,15 @@  static vm_vaddr_t vm_vaddr_unused_gap(struct kvm_vm *vm, size_t sz,
 	return pgidx_start * vm->page_size;
 }
 
-/*
- * VM Virtual Address Allocate
- *
- * Input Args:
- *   vm - Virtual Machine
- *   sz - Size in bytes
- *   vaddr_min - Minimum starting virtual address
- *
- * Output Args: None
- *
- * Return:
- *   Starting guest virtual address
- *
- * Allocates at least sz bytes within the virtual address space of the vm
- * given by vm.  The allocated bytes are mapped to a virtual address >=
- * the address given by vaddr_min.  Note that each allocation uses a
- * a unique set of pages, with the minimum real allocation being at least
- * a page.
- */
-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)
 {
 	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]);
 
 	/*
 	 * 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 Virtual Address Allocate
+ *
+ * Input Args:
+ *   vm - Virtual Machine
+ *   sz - Size in bytes
+ *   vaddr_min - Minimum starting virtual address
+ *
+ * Output Args: None
+ *
+ * Return:
+ *   Starting guest virtual address
+ *
+ * Allocates at least sz bytes within the virtual address space of the vm
+ * given by vm.  The allocated bytes are mapped to a virtual address >=
+ * the address given by vaddr_min.  Note that each allocation uses a
+ * a unique set of pages, with the minimum real allocation being at least
+ * a page. The allocated physical space comes from the data memory region.
+ */
+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);
+}
+
 /*
  * 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]);
 		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);
 	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]);
 	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);
 
 	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);
 
 	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);
 
 	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);
 
 	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);
+	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,