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
>
