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

[Ricardo Koller <ricarkol@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
> > 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm