Re: [PATCH v4 06/13] KVM: selftests: Add vm_mem_region_get_src_fd library function
On Fri, Jun 24, 2022 at 02:32:50PM -0700, Ricardo Koller wrote:
> Add a library function to get the backing source FD of a memslot.
>
> Reviewed-by: Oliver Upton
> Signed-off-by: Ricardo Koller
> ---
> .../selftests/kvm/include/kvm_util_base.h | 1 +
> tools/testing/selftests/kvm/lib/kvm_util.c| 23 +++
> 2 files changed, 24 insertions(+)
>
> diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h
> b/tools/testing/selftests/kvm/include/kvm_util_base.h
> index 54ede9fc923c..72c8881fe8fb 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> @@ -322,6 +322,7 @@ void vm_userspace_mem_region_add(struct kvm_vm *vm,
> void vm_mem_region_set_flags(struct kvm_vm *vm, uint32_t slot, uint32_t
> flags);
> 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);
> +int vm_mem_region_get_src_fd(struct kvm_vm *vm, uint32_t memslot);
> 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_pages(struct kvm_vm *vm, int nr_pages);
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c
> b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 3e45e3776bdf..7c81028f23d8 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -466,6 +466,29 @@ kvm_userspace_memory_region_find(struct kvm_vm *vm,
> uint64_t start,
> return >region;
> }
>
> +/*
> + * KVM Userspace Memory Get Backing Source FD
> + *
> + * Input Args:
> + * vm - Virtual Machine
> + * memslot - KVM memory slot ID
> + *
> + * Output Args: None
> + *
> + * Return:
> + * Backing source file descriptor, -1 if the memslot is an anonymous
> region.
> + *
> + * Returns the backing source fd of a memslot, so tests can use it to punch
> + * holes, or to setup permissions.
> + */
nit: We're starting to slowly change these verbose function headers into
smaller headers, so this could be reduced.
> +int vm_mem_region_get_src_fd(struct kvm_vm *vm, uint32_t memslot)
> +{
> + struct userspace_mem_region *region;
> +
> + region = memslot2region(vm, memslot);
> + return region->fd;
> +}
> +
> /*
> * VM VCPU Remove
> *
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>
Reviewed-by: Andrew Jones
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 05/13] KVM: selftests: Add missing close and munmap in __vm_mem_region_delete
On Fri, Jun 24, 2022 at 02:32:49PM -0700, Ricardo Koller wrote:
> Deleting a memslot (when freeing a VM) is not closing the backing fd,
> nor it's unmapping the alias mapping. Fix by adding the missing close
> and munmap.
>
> Reviewed-by: Oliver Upton
> Reviewed-by: Ben Gardon
> Signed-off-by: Ricardo Koller
> ---
> tools/testing/selftests/kvm/lib/kvm_util.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c
> b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 5ee20d4da222..3e45e3776bdf 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -531,6 +531,12 @@ static void __vm_mem_region_delete(struct kvm_vm *vm,
> sparsebit_free(>unused_phy_pages);
> ret = munmap(region->mmap_start, region->mmap_size);
> TEST_ASSERT(!ret, __KVM_SYSCALL_ERROR("munmap()", ret));
> + if (region->fd >= 0) {
> + /* There's an extra map when using shared memory. */
> + ret = munmap(region->mmap_alias, region->mmap_size);
> + TEST_ASSERT(!ret, __KVM_SYSCALL_ERROR("munmap()", ret));
> + close(region->fd);
> + }
>
> free(region);
> }
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>
Reviewed-by: Andrew Jones
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 07/13] KVM: selftests: aarch64: Construct DEFAULT_MAIR_EL1 using sysreg.h macros
On Fri, Jun 24, 2022 at 02:32:51PM -0700, Ricardo Koller wrote:
> Define macros for memory type indexes and construct DEFAULT_MAIR_EL1
> with macros from asm/sysreg.h. The index macros can then be used when
> constructing PTEs (instead of using raw numbers).
>
> Reviewed-by: Oliver Upton
> Signed-off-by: Ricardo Koller
> ---
> .../selftests/kvm/include/aarch64/processor.h | 25 ++-
> .../selftests/kvm/lib/aarch64/processor.c | 2 +-
> 2 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h
> b/tools/testing/selftests/kvm/include/aarch64/processor.h
> index 6649671fa7c1..74f10d006e15 100644
> --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> @@ -38,12 +38,25 @@
> * NORMAL 4 :
> * NORMAL_WT 5 1011:1011
> */
> -#define DEFAULT_MAIR_EL1 ((0x00ul << (0 * 8)) | \
> - (0x04ul << (1 * 8)) | \
> - (0x0cul << (2 * 8)) | \
> - (0x44ul << (3 * 8)) | \
> - (0xfful << (4 * 8)) | \
> - (0xbbul << (5 * 8)))
> +
> +/* Linux doesn't use these memory types, so let's define them. */
> +#define MAIR_ATTR_DEVICE_GRE UL(0x0c)
> +#define MAIR_ATTR_NORMAL_WT UL(0xbb)
> +
> +#define MT_DEVICE_nGnRnE 0
> +#define MT_DEVICE_nGnRE 1
> +#define MT_DEVICE_GRE2
> +#define MT_NORMAL_NC 3
> +#define MT_NORMAL4
> +#define MT_NORMAL_WT 5
> +
> +#define DEFAULT_MAIR_EL1
> \
> + (MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) |
> \
> + MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRE, MT_DEVICE_nGnRE) |
> \
> + MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) |
> \
> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) |
> \
> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) |
> \
> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT))
>
> #define MPIDR_HWID_BITMASK (0xff00fful)
>
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> index 8dd511aa79c2..733a2b713580 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> @@ -133,7 +133,7 @@ void _virt_pg_map(struct kvm_vm *vm, uint64_t vaddr,
> uint64_t paddr,
>
> void virt_arch_pg_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr)
> {
> - uint64_t attr_idx = 4; /* NORMAL (See DEFAULT_MAIR_EL1) */
> + uint64_t attr_idx = MT_NORMAL;
>
> _virt_pg_map(vm, vaddr, paddr, attr_idx, 0);
> }
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>
Reviewed-by: Andrew Jones
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 04/13] KVM: selftests: aarch64: Export _virt_pg_map with a pt_memslot arg
On Fri, Jun 24, 2022 at 02:32:48PM -0700, Ricardo Koller wrote: > Add an argument, pt_memslot, into _virt_pg_map in order to use a > specific memslot for the page-table allocations performed when creating > a new map. This will be used in a future commit to test having PTEs > stored on memslots with different setups (e.g., hugetlb with a hole). > > Reviewed-by: Oliver Upton > Signed-off-by: Ricardo Koller > --- > .../selftests/kvm/include/aarch64/processor.h| 3 +++ > tools/testing/selftests/kvm/lib/aarch64/processor.c | 12 ++-- > 2 files changed, 9 insertions(+), 6 deletions(-) > Reviewed-by: Andrew Jones ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 03/13] KVM: selftests: Add vm_alloc_page_table_in_memslot library function
On Fri, Jun 24, 2022 at 02:32:47PM -0700, Ricardo Koller wrote:
> Add a library function to allocate a page-table physical page in a
> particular memslot. The default behavior is to create new page-table
> pages in memslot 0.
>
> Reviewed-by: Oliver Upton
> Reviewed-by: Ben Gardon
> Signed-off-by: Ricardo Koller
> ---
> tools/testing/selftests/kvm/include/kvm_util_base.h | 1 +
> tools/testing/selftests/kvm/lib/kvm_util.c | 8 +++-
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h
> b/tools/testing/selftests/kvm/include/kvm_util_base.h
> index 7ebfc8c7de17..54ede9fc923c 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> @@ -579,6 +579,7 @@ vm_paddr_t vm_phy_page_alloc(struct kvm_vm *vm,
> vm_paddr_t paddr_min,
> vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
> vm_paddr_t paddr_min, uint32_t memslot);
> vm_paddr_t vm_alloc_page_table(struct kvm_vm *vm);
> +vm_paddr_t vm_alloc_page_table_in_memslot(struct kvm_vm *vm, uint32_t
> pt_memslot);
>
> /*
> * vm_create() does KVM_CREATE_VM and little else. __vm_create() also
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c
> b/tools/testing/selftests/kvm/lib/kvm_util.c
> index f8c104dba258..5ee20d4da222 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -1784,9 +1784,15 @@ vm_paddr_t vm_phy_page_alloc(struct kvm_vm *vm,
> vm_paddr_t paddr_min,
> /* Arbitrary minimum physical address used for virtual translation tables. */
> #define KVM_GUEST_PAGE_TABLE_MIN_PADDR 0x18
>
> +vm_paddr_t vm_alloc_page_table_in_memslot(struct kvm_vm *vm, uint32_t
> pt_memslot)
> +{
> + return vm_phy_page_alloc(vm, KVM_GUEST_PAGE_TABLE_MIN_PADDR,
> + pt_memslot);
> +}
> +
> 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_alloc_page_table_in_memslot(vm, 0);
> }
>
> /*
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>
Reviewed-by: Andrew Jones
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 02/13] KVM: selftests: aarch64: Add virt_get_pte_hva library function
On Fri, Jun 24, 2022 at 02:32:46PM -0700, Ricardo Koller wrote:
> Add a library function to get the PTE (a host virtual address) of a
> given GVA. This will be used in a future commit by a test to clear and
> check the access flag of a particular page.
>
> Signed-off-by: Ricardo Koller
> ---
> .../selftests/kvm/include/aarch64/processor.h | 2 ++
> tools/testing/selftests/kvm/lib/aarch64/processor.c | 13 ++---
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h
> b/tools/testing/selftests/kvm/include/aarch64/processor.h
> index a8124f9dd68a..df4bfac69551 100644
> --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> @@ -109,6 +109,8 @@ void vm_install_exception_handler(struct kvm_vm *vm,
> void vm_install_sync_handler(struct kvm_vm *vm,
> int vector, int ec, handler_fn handler);
>
> +uint64_t *virt_get_pte_hva(struct kvm_vm *vm, vm_vaddr_t gva);
> +
> static inline void cpu_relax(void)
> {
> asm volatile("yield" ::: "memory");
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> index 6f5551368944..63ef3c78e55e 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> @@ -138,7 +138,7 @@ void virt_arch_pg_map(struct kvm_vm *vm, uint64_t vaddr,
> uint64_t paddr)
> _virt_pg_map(vm, vaddr, paddr, attr_idx);
> }
>
> -vm_paddr_t addr_arch_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva)
> +uint64_t *virt_get_pte_hva(struct kvm_vm *vm, vm_vaddr_t gva)
> {
> uint64_t *ptep;
>
> @@ -169,11 +169,18 @@ vm_paddr_t addr_arch_gva2gpa(struct kvm_vm *vm,
> vm_vaddr_t gva)
> TEST_FAIL("Page table levels must be 2, 3, or 4");
> }
>
> - return pte_addr(vm, *ptep) + (gva & (vm->page_size - 1));
> + return ptep;
>
> unmapped_gva:
> TEST_FAIL("No mapping for vm virtual address, gva: 0x%lx", gva);
> - exit(1);
> + exit(EXIT_FAILURE);
> +}
> +
> +vm_paddr_t addr_arch_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva)
> +{
> + uint64_t *ptep = virt_get_pte_hva(vm, gva);
> +
> + return pte_addr(vm, *ptep) + (gva & (vm->page_size - 1));
> }
>
> static void pte_dump(FILE *stream, struct kvm_vm *vm, uint8_t indent,
> uint64_t page, int level)
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>
Reviewed-by: Andrew Jones
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 05/19] KVM: arm64: Consolidate sysreg userspace accesses
Hi Reiji,
On Sat, 09 Jul 2022 07:55:08 +0100,
Reiji Watanabe wrote:
>
> Hi Marc,
[...]
> > @@ -2854,17 +2794,28 @@ static int demux_c15_set(u64 id, void __user *uaddr)
> > int kvm_sys_reg_get_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg
> > *reg,
> > const struct sys_reg_desc table[], unsigned int
> > num)
> > {
> > - void __user *uaddr = (void __user *)(unsigned long)reg->addr;
> > + u64 __user *uaddr = (u64 __user *)(unsigned long)reg->addr;
> > const struct sys_reg_desc *r;
> > + u64 val;
> > + int ret;
> >
> > r = id_to_sys_reg_desc(vcpu, reg->id, table, num);
> > if (!r)
> > return -ENOENT;
> >
> > - if (r->get_user)
> > - return (r->get_user)(vcpu, r, reg, uaddr);
> > + if (r->get_user) {
> > + ret = (r->get_user)(vcpu, r, );
> > + } else {
> > + val = __vcpu_sys_reg(vcpu, r->reg);
> > + ret = 0;
> > + }
> > +
> > + if (!ret) {
> > + if (put_user(val, uaddr))
> > + ret = -EFAULT;
>
> Nit: Since put_user() returns -EFAULT when it fails,
> we can simply set 'ret' to the return value from put_user().
Indeed. I've now reworked all the instances of this that survive the
incremental rework. Thanks for the suggestion.
> (same for get_user())
This one is a harder sell, as get_user() usually occurs at the
beginning of the function, without a preexisting error context.
>
> Reviewed-by: Reiji Watanabe
>
> I like this fix!
Thanks!
M.
--
Without deviation from the norm, progress is not possible.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 08/19] KVM: arm64: vgic-v3: Push user access into vgic_v3_cpu_sysregs_uaccess()
Hi Reiji,
On Tue, 12 Jul 2022 07:11:39 +0100,
Reiji Watanabe wrote:
>
> Hi Marc,
>
> On Wed, Jul 6, 2022 at 9:43 AM Marc Zyngier wrote:
> >
> > In order to start making the vgic sysreg access from userspace
> > similar to all the other sysregs, push the userspace memory
> > access one level down into vgic_v3_cpu_sysregs_uaccess().
> >
> > The next step will be to rely on the sysreg infrastructure
> > to perform this task.
> >
> > Signed-off-by: Marc Zyngier
> > ---
> > arch/arm64/kvm/vgic-sys-reg-v3.c | 22 +--
> > arch/arm64/kvm/vgic/vgic-kvm-device.c | 31 ++-
> > arch/arm64/kvm/vgic/vgic.h| 4 ++--
> > 3 files changed, 23 insertions(+), 34 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c
> > b/arch/arm64/kvm/vgic-sys-reg-v3.c
> > index 85a5e1d15e9f..8c56e285fde9 100644
> > --- a/arch/arm64/kvm/vgic-sys-reg-v3.c
> > +++ b/arch/arm64/kvm/vgic-sys-reg-v3.c
> > @@ -278,15 +278,21 @@ int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu
> > *vcpu, struct kvm_device_attr *
> > return -ENXIO;
> > }
> >
> > -int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64
> > id,
> > - u64 *reg)
> > +int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu,
> > + struct kvm_device_attr *attr,
> > + bool is_write)
> > {
> > + u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> > struct sys_reg_params params;
> > const struct sys_reg_desc *r;
> > - u64 sysreg = (id & KVM_DEV_ARM_VGIC_SYSREG_MASK) | KVM_REG_SIZE_U64;
> > + u64 sysreg;
> >
> > - if (is_write)
> > - params.regval = *reg;
> > + sysreg = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_MASK) |
> > KVM_REG_SIZE_U64;
>
> Why don't you use attr_to_id() here ?
This actually happens in the following patch. Happy to move the change
here though.
>
>
> > +
> > + if (is_write) {
> > + if (get_user(params.regval, uaddr))
> > + return -EFAULT;
> > + }
> > params.is_write = is_write;
> >
> > r = find_reg_by_id(sysreg, , gic_v3_icc_reg_descs,
> > @@ -297,8 +303,10 @@ int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu,
> > bool is_write, u64 id,
> > if (!r->access(vcpu, , r))
> > return -EINVAL;
> >
> > - if (!is_write)
> > - *reg = params.regval;
> > + if (!is_write) {
> > + if (put_user(params.regval, uaddr))
> > + return -EFAULT;
> > + }
> >
> > return 0;
> > }
> > diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > index c6d52a1fd9c8..d8269300632d 100644
> > --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > @@ -561,14 +561,9 @@ static int vgic_v3_attr_regs_access(struct kvm_device
> > *dev,
> > if (!is_write)
> > *reg = tmp32;
> > break;
> > - case KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS: {
> > - u64 regid;
> > -
> > - regid = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK);
> > - ret = vgic_v3_cpu_sysregs_uaccess(vcpu, is_write,
> > - regid, reg);
> > + case KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS:
> > + ret = vgic_v3_cpu_sysregs_uaccess(vcpu, attr, is_write);
>
> Nit: Since @reg that is passed to vgic_v3_attr_regs_access() will be NULL
> for KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS, I think it would be more clear
> if you could update the comment for vgic_v3_attr_regs_access accordingly.
>
>
> /*
> * vgic_v3_attr_regs_access - allows user space to access VGIC v3 state
> *
> * @dev: kvm device handle
> * @attr: kvm device attribute
> * @reg: address the value is read or written
>~~~
> * @is_write: true if userspace is writing a register
> */
> static int vgic_v3_attr_regs_access(struct kvm_device *dev,
> struct kvm_device_attr *attr,
> u64 *reg, bool is_write)
@reg disappears completely in patch #12. Do you see value in rewriting
this comment even if I end-up removing it 4 patches down the line?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
