Re: [kvm-unit-tests PATCH] arm: Remove MAX_SMP probe loop
On Thu, Jan 05, 2023 at 11:09:58PM +, Colton Lewis wrote: > Andrew Jones writes: > > On Tue, Dec 20, 2022 at 04:32:00PM +, Colton Lewis wrote: > > > Alexandru Elisei writes: > > Ah, I think I understand now. Were you running 32-bit arm tests? If so, > > it'd be good to point that out explicitly in the commit message (the > > 'arm:' prefix in the summary is ambiguous). > > No, this was happening on arm64. Since it had been a while since I noted > this issue, I reviewed it and realized the issue was only happening > using -accel tcg. That was automatically being used on my problem test > machine without me noticing. That's where the limit of 8 seems to be > coming from and why the loop is triggered. > > qemu-system-aarch64: Number of SMP CPUs requested (152) exceeds max CPUs > supported by machine 'mach-virt' (8) > > Since this case doesn't directly involve KVM, I doubt anyone cares about > a fix. > > > Assuming the loop body was running because it needed to reduce MAX_SMP to > > 8 or lower for 32-bit arm tests, then we should be replacing the loop with > > something that caps MAX_SMP at 8 for 32-bit arm tests instead. > > We could cap at 8 for ACCEL=tcg. Even if no one cares, I'm tempted to do > it so no one hits the same little landmine as me in the future. TCG supports up to 255 CPUs. The only reason it'd have a max of 8 is if you were configuring a GICv2 instead of a GICv3. Using gic-version=3 or gic-version=max should allow the 152 CPUs to work. Actually, I should have asked about your gic version instead of whether or not the VM was AArch32 in the first place. I was incorrectly associating the gicv2 limits with arm32 since my memories of these things have started to blur together... Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH] arm: Remove MAX_SMP probe loop
On Tue, Dec 20, 2022 at 04:32:00PM +, Colton Lewis wrote: > Alexandru Elisei writes: > > > Though I'm not sure how you managed to get MAX_SMP to go down to 6 cores > > on > > a 12 core machine. MAX_SMP is initialized to $(getconf _NPROCESSORS_ONLN), > > so the body of the loop should never execute. I also tried it on a 6 core > > machine, and MAX_SMP was 6, not 3. > > > Am I missing something? > > To be clear, 12 cores was a simplified example I did not directly > verify. What happened to me was 152 cores being cut down to 4. I was > confused why one machine was running a test with 4 cores when my other > machines were running with 8 and traced it to that loop. In effect the > loop was doing MAX_SMP=floor(MAX_SMP / 2) until MAX_SMP <= 8. I printed > the iterations and MAX_SMP followed the sequence 152->76->38->19->9->4. Ah, I think I understand now. Were you running 32-bit arm tests? If so, it'd be good to point that out explicitly in the commit message (the 'arm:' prefix in the summary is ambiguous). Assuming the loop body was running because it needed to reduce MAX_SMP to 8 or lower for 32-bit arm tests, then we should be replacing the loop with something that caps MAX_SMP at 8 for 32-bit arm tests instead. Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH] arm: Remove MAX_SMP probe loop
On Mon, Dec 19, 2022 at 06:52:50PM +, Colton Lewis wrote: > This loop logic is broken for machines with a number of CPUs that > isn't a power of two. A machine with 8 CPUs will test with MAX_SMP=8 > but a machine with 12 CPUs will test with MAX_SMP=6 because 12 >> 2 == ^ 1 > 6. This can, in rare circumstances, lead to different test results > depending only on the number of CPUs the machine has. > > The loop is safe to remove with no side effects. It has an explanitory > comment explaining that it only applies to kernels <=v4.3 on arm and > suggestion deletion when it becomes tiresome to maintain. Removing this loop is safe if the body of the loop is not expected to ever run, i.e. we're through testing kernels older than v4.3. But, if MAX_SMP is getting reduced, as stated above, then that implies $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP is resulting in a "exceeds max CPUs" error. If that's the case, then the tests which configure $MAX_SMP cpus should be failing as well. IOW, the loop should never do anything except be a pointless extra invocation of QEMU. If it does do something, then we should understand why. Thanks, drew > > Signed-off-by: Colton Lewis > --- > scripts/runtime.bash | 14 -- > 1 file changed, 14 deletions(-) > > diff --git a/scripts/runtime.bash b/scripts/runtime.bash > index f8794e9..18a8dd7 100644 > --- a/scripts/runtime.bash > +++ b/scripts/runtime.bash > @@ -183,17 +183,3 @@ function run() > > return $ret > } > - > -# > -# Probe for MAX_SMP, in case it's less than the number of host cpus. > -# > -# This probing currently only works for ARM, as x86 bails on another > -# error first. Also, this probing isn't necessary for any ARM hosts > -# running kernels later than v4.3, i.e. those including ef748917b52 > -# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some > -# point when maintaining the while loop gets too tiresome, we can > -# just remove it... > -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \ > - |& grep -qi 'exceeds max CPUs'; do > - MAX_SMP=$((MAX_SMP >> 1)) > -done > -- > 2.39.0.314.g84b9a713c41-goog > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 0/7] KVM: selftests: Fixes for ucall pool + page_fault_test
On Fri, Dec 09, 2022 at 01:52:59AM +, Oliver Upton wrote: > The combination of the pool-based ucall implementation + page_fault_test > resulted in some 'fun' bugs. As has always been the case, KVM selftests > is a house of cards. > > Small series to fix up the issues on kvm/queue. Patches 1-2 can probably > be squashed into Paolo's merge resolution, if desired. > > Tested on Ampere Altra and a Skylake box, since there was a decent > amount of munging in architecture-generic code. > > v1 -> v2: > - Collect R-b from Sean (thanks!) > - Use a common routine for split and contiguous VA spaces, with >commentary on why arm64 is different since we all get to look at it >now. (Sean) > - Don't identity map the ucall MMIO hole > - Fix an off-by-one issue in the accounting of virtual memory, >discovered in fighting with #2 > - Fix an infinite loop in ucall_alloc(), discovered fighting with the >ucall_init() v. kvm_vm_elf_load() ordering issue > > Mark Brown (1): > KVM: selftests: Fix build due to ucall_uninit() removal > > Oliver Upton (6): > KVM: selftests: Setup ucall after loading program into guest memory > KVM: selftests: Mark correct page as mapped in virt_map() > KVM: selftests: Correctly initialize the VA space for TTBR0_EL1 > KVM: arm64: selftests: Don't identity map the ucall MMIO hole > KVM: selftests: Allocate ucall pool from MEM_REGION_DATA > KVM: selftests: Avoid infinite loop if ucall_alloc() fails > > .../selftests/kvm/aarch64/page_fault_test.c | 9 +++- > .../selftests/kvm/include/kvm_util_base.h | 1 + > .../testing/selftests/kvm/lib/aarch64/ucall.c | 6 ++- > tools/testing/selftests/kvm/lib/kvm_util.c| 53 --- > .../testing/selftests/kvm/lib/ucall_common.c | 14 +++-- > 5 files changed, 68 insertions(+), 15 deletions(-) > > > base-commit: 89b2395859651113375101bb07cd6340b1ba3637 This commit doesn't seem to exist linux-next or kvm/queue, but the patch context seems to match up with linux-next pretty well. Anyway, For the series Reviewed-by: Andrew Jones Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 3/4] KVM: arm64: selftests: Align VA space allocator with TTBR0
On Thu, Dec 08, 2022 at 01:09:38AM +, Sean Christopherson wrote: ... > Actually, before we do anything, we should get confirmation from the s390 and > RISC-V folks on whether they have a canonical hole like x86, i.e. maybe x86 is > the oddball. riscv splits like x86. Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH v4 0/4] arm: pmu: Fixes for bare metal
On Fri, Oct 28, 2022 at 04:01:50PM +0100, Marc Zyngier wrote: > Hi Drew, > > On Fri, 28 Oct 2022 12:40:41 +0100, > Andrew Jones wrote: > > > > On Thu, Aug 11, 2022 at 11:52:06AM -0700, Ricardo Koller wrote: > > > There are some tests that fail when running on bare metal (including a > > > passthrough prototype). There are three issues with the tests. The > > > first one is that there are some missing isb()'s between enabling event > > > counting and the actual counting. This wasn't an issue on KVM as > > > trapping on registers served as context synchronization events. The > > > second issue is that some tests assume that registers reset to 0. And > > > finally, the third issue is that overflowing the low counter of a > > > chained event sets the overflow flag in PMVOS and some tests fail by > > > checking for it not being set. > > > > > > Addressed all comments from the previous version: > > > https://lore.kernel.org/kvmarm/yvpsbkgbhhqp+...@google.com/T/#mb077998e2eb9fb3e15930b3412fd7ba2fb4103ca > > > - add pmu_reset() for 32-bit arm [Andrew] > > > - collect r-b from Alexandru > > > > > > Thanks! > > > Ricardo > > > > > > Ricardo Koller (4): > > > arm: pmu: Add missing isb()'s after sys register writing > > > arm: pmu: Add reset_pmu() for 32-bit arm > > > arm: pmu: Reset the pmu registers before starting some tests > > > arm: pmu: Check for overflow in the low counter in chained counters > > > tests > > > > > > arm/pmu.c | 72 ++- > > > 1 file changed, 55 insertions(+), 17 deletions(-) > > > > > > > Hi all, > > > > Please refresh my memory. Does this series work on current platforms? Or > > was it introducing new test failures which may be in the test, as opposed > > to KVM? If they work on most platforms, but not on every platform, then > > have we identified what triggers them to fail and whether that should be > > fixed or just worked-around? I'm sorry I still can't help out with the > > testing as I haven't yet had time to setup the Rpi that Mark Rutland gave > > me in Dublin. > > This series does show that KVM is buggy, and I have patches out to fix > it [1]. The patches should work on anything, really. > > > I know this series has been rotting on arm/queue for months, so I'll be > > happy to merge it if the consensus is to do so. I can also drop it, or > > some of the patches, if that's the consensus. > > I'd be very happy to see these patches being merged. Thanks for the information, Marc. I've gone ahead and merged the tests. What's the worst than can happen :-) Anyway, I agree that when the tests start failing in CIs, then they're doing their job. If your pending series can't be applied right away, then the CIs can likely be taught to temporarily ignore the known failures. Thanks, drew > > Thanks, > > M. > > [1] https://lore.kernel.org/r/20221028105402.2030192-1-...@kernel.org > > -- > 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: [kvm-unit-tests PATCH] MAINTAINERS: new kvmarm mailing list
On Tue, Oct 25, 2022 at 06:07:30PM +0200, Cornelia Huck wrote: > KVM/arm64 development is moving to a new mailing list (see > https://lore.kernel.org/all/20221001091245.3900668-1-...@kernel.org/); > kvm-unit-tests should advertise the new list as well. > > Signed-off-by: Cornelia Huck > --- > MAINTAINERS | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 90ead214a75d..649de509a511 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -67,7 +67,8 @@ ARM > M: Andrew Jones > S: Supported > L: k...@vger.kernel.org > -L: kvmarm@lists.cs.columbia.edu > +L: kvm...@lists.linux.dev > +L: kvmarm@lists.cs.columbia.edu (deprecated) > F: arm/ > F: lib/arm/ > F: lib/arm64/ > -- > 2.37.3 > Pushed, thanks! drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH] MAINTAINERS: new kvmarm mailing list
On Fri, Oct 28, 2022 at 01:37:34PM +0200, Cornelia Huck wrote: > On Tue, Oct 25 2022, Cornelia Huck wrote: > > > KVM/arm64 development is moving to a new mailing list (see > > https://lore.kernel.org/all/20221001091245.3900668-1-...@kernel.org/); > > kvm-unit-tests should advertise the new list as well. > > > > Signed-off-by: Cornelia Huck > > --- > > MAINTAINERS | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 90ead214a75d..649de509a511 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -67,7 +67,8 @@ ARM > > M: Andrew Jones > > S: Supported > > L: k...@vger.kernel.org > > -L: kvmarm@lists.cs.columbia.edu > > +L: kvm...@lists.linux.dev > > +L: kvmarm@lists.cs.columbia.edu (deprecated) > > As the days of the Columbia list really seem to be numbered (see > https://lore.kernel.org/all/364100e884023234e4ab9e525774d...@kernel.org/), > should we maybe drop it completely from MAINTAINERS, depending on when > this gets merged? I'll merge your patch now with the old (deprecated) list still there. When mail starts bouncing it may help people better understand why. When the kernel drops it from its MAINTAINERS file, then we can drop it here too. Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH v4 0/4] arm: pmu: Fixes for bare metal
On Thu, Aug 11, 2022 at 11:52:06AM -0700, Ricardo Koller wrote: > There are some tests that fail when running on bare metal (including a > passthrough prototype). There are three issues with the tests. The > first one is that there are some missing isb()'s between enabling event > counting and the actual counting. This wasn't an issue on KVM as > trapping on registers served as context synchronization events. The > second issue is that some tests assume that registers reset to 0. And > finally, the third issue is that overflowing the low counter of a > chained event sets the overflow flag in PMVOS and some tests fail by > checking for it not being set. > > Addressed all comments from the previous version: > https://lore.kernel.org/kvmarm/yvpsbkgbhhqp+...@google.com/T/#mb077998e2eb9fb3e15930b3412fd7ba2fb4103ca > - add pmu_reset() for 32-bit arm [Andrew] > - collect r-b from Alexandru > > Thanks! > Ricardo > > Ricardo Koller (4): > arm: pmu: Add missing isb()'s after sys register writing > arm: pmu: Add reset_pmu() for 32-bit arm > arm: pmu: Reset the pmu registers before starting some tests > arm: pmu: Check for overflow in the low counter in chained counters > tests > > arm/pmu.c | 72 ++- > 1 file changed, 55 insertions(+), 17 deletions(-) > Hi all, Please refresh my memory. Does this series work on current platforms? Or was it introducing new test failures which may be in the test, as opposed to KVM? If they work on most platforms, but not on every platform, then have we identified what triggers them to fail and whether that should be fixed or just worked-around? I'm sorry I still can't help out with the testing as I haven't yet had time to setup the Rpi that Mark Rutland gave me in Dublin. I know this series has been rotting on arm/queue for months, so I'll be happy to merge it if the consensus is to do so. I can also drop it, or some of the patches, if that's the consensus. Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v9 09/14] KVM: selftests: Use the right memslot for code, page-tables, and data allocations
On Tue, Oct 11, 2022 at 01:06:23AM +, Ricardo Koller wrote: > 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. Change them accordingly: > > - code (allocated by lib/elf) use the CODE memslot > - stacks, exception tables, and other core data pages (like the TSS in x86) > use the DATA memslot > - page tables and the PGD use the PT memslot > - test data (anything allocated with vm_vaddr_alloc()) uses the TEST_DATA > memslot > > No functional change intended. All allocators keep using memslot #0. > > Cc: Sean Christopherson > Cc: Andrew Jones > Signed-off-by: Ricardo Koller > Reviewed-by: Sean Christopherson > --- > .../selftests/kvm/include/kvm_util_base.h | 4 ++ > .../selftests/kvm/lib/aarch64/processor.c | 12 ++-- > tools/testing/selftests/kvm/lib/elf.c | 3 +- > tools/testing/selftests/kvm/lib/kvm_util.c| 57 --- > .../selftests/kvm/lib/riscv/processor.c | 8 ++- > .../selftests/kvm/lib/s390x/processor.c | 8 ++- > .../selftests/kvm/lib/x86_64/processor.c | 13 +++-- > 7 files changed, 65 insertions(+), 40 deletions(-) > Reviewed-by: Andrew Jones ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v9 08/14] KVM: selftests: Fix alignment in virt_arch_pgd_alloc() and vm_vaddr_alloc()
On Tue, Oct 11, 2022 at 01:06:22AM +, Ricardo Koller wrote: > Refactor virt_arch_pgd_alloc() and vm_vaddr_alloc() in both RISC-V and > aarch64 to fix the alignment of parameters in a couple of calls. This will > make it easier to fix the alignment in a future commit that adds an extra > parameter (that happens to be very long). > > No functional change intended. > > Suggested-by: Sean Christopherson > Signed-off-by: Ricardo Koller > --- > .../selftests/kvm/lib/aarch64/processor.c | 27 ++- > .../selftests/kvm/lib/riscv/processor.c | 27 ++- > 2 files changed, 30 insertions(+), 24 deletions(-) > Reviewed-by: Andrew Jones ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests RFC PATCH 05/19] lib/alloc_phys: Remove locking
On Tue, Sep 20, 2022 at 02:20:48PM +0100, Alexandru Elisei wrote: > Hi, > > On Tue, Sep 20, 2022 at 10:45:53AM +0200, Andrew Jones wrote: > > On Tue, Aug 09, 2022 at 10:15:44AM +0100, Alexandru Elisei wrote: > > > With powerpc moving the page allocator, there are no architectures left > > > which use the physical allocator after the boot setup: arm, arm64, > > > s390x and powerpc drain the physical allocator to initialize the page > > > allocator; and x86 calls setup_vm() to drain the allocator for each of > > > the tests that allocate memory. > > > > Please put the motivation for this change in the commit message. I looked > > ahead at the next patch to find it, but I'm not sure I agree with it. We > > should be able to keep the locking even when used early, since we probably > > need our locking to be something we can use early elsewhere anyway. > > You are correct, the commit message doesn't explain why locking is removed, > which makes the commit confusing. I will try to do a better job for the > next iteration (if we decide to keep this patch). > > I removed locking because the physical allocator by the end of the series > will end up being used only by arm64 to create the idmap, which is done on If only arm, and no unit tests, needs the phys allocator, then it can be integrated with whatever arm is using it for and removed from the general lib. > the boot CPU and with the MMU off. After that, the translation table > allocator functions will use the page allocator, which can be used > concurrently. > > Looking at the spinlock implementation, spin_lock() doesn't protect from > the concurrent accesses when the MMU is disabled (lock->v is > unconditionally set to 1). Which means that spin_lock() does not work (in > the sense that it doesn't protect against concurrent accesses) on the boot > path, which doesn't need a spinlock anyway, because no secondaries are > online secondaries. It also means that spinlocks don't work when > AUXINFO_MMU_OFF is set. So for the purpose of simplicity I preferred to > drop it entirely. If other architectures or unit tests have / could have uses for the phys allocator then we should either document that it doesn't have locks or keep the locks, and arm will just know that they don't work, but also that they don't need to for its purposes. Finally, if we drop the locks and arm doesn't have any other places where we use locks without the MMU enabled, then we can change the lock implementation to not have the no-mmu fallback - maybe by switching to the generic implementation as the other architectures have done. Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests RFC PATCH 16/19] arm/arm64: Allocate secondaries' stack using the page allocator
On Tue, Aug 09, 2022 at 10:15:55AM +0100, Alexandru Elisei wrote:
> The vmalloc allocator returns non-id mapped addresses, where the virtual
> address is different than the physical address. This makes it impossible
> to access the stack of the secondary CPUs while the MMU is disabled.
>
> On arm, THREAD_SIZE is 16K and PAGE_SIZE is 4K, which makes THREAD_SIZE
> a power of two multiple of PAGE_SIZE. On arm64, THREAD_SIZE is 16 when
> PAGE_SIZE is 4K or 16K, and 64K when PAGE_SIZE is 64K. In all cases,
> THREAD_SIZE is a power of two multiple of PAGE_SIZE. As a result, using
> memalign_pages() for the stack won't lead to wasted memory.
>
> memalign_pages() allocates memory in chunks of power of two number of
> pages, aligned to the allocation size, which makes it a drop-in
> replacement for vm_memalign (which is the value for alloc_ops->memalign
> when the stack is allocated).
>
> Using memalign_pages() has two distinct benefits:
>
> 1. The secondary CPUs' stack can be used with the MMU off.
>
> 2. The secondary CPUs' stack is identify mapped similar to the stack for
> the primary CPU, which makes the configuration of the CPUs consistent.
>
> memalign_pages_flags() has been used instead of memalign_pages() to
> instruct the allocator not to zero the stack, as it's already zeroed in the
> entry code.
>
> Signed-off-by: Alexandru Elisei
> ---
> lib/arm/asm/thread_info.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/arm/asm/thread_info.h b/lib/arm/asm/thread_info.h
> index eaa72582af86..190e082cbba0 100644
> --- a/lib/arm/asm/thread_info.h
> +++ b/lib/arm/asm/thread_info.h
> @@ -25,6 +25,7 @@
> #ifndef __ASSEMBLY__
> #include
> #include
> +#include
>
> #ifdef __arm__
> #include
> @@ -40,7 +41,7 @@
>
> static inline void *thread_stack_alloc(void)
> {
> - void *sp = memalign(THREAD_ALIGNMENT, THREAD_SIZE);
> + void *sp = memalign_pages_flags(THREAD_ALIGNMENT, THREAD_SIZE,
> FLAG_DONTZERO);
> return sp + THREAD_START_SP;
> }
>
> --
> 2.37.1
>
Reviewed-by: Andrew Jones
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests RFC PATCH 13/19] arm: page.h: Add missing libcflat.h include
I guess this should be squashed into one of the early patches in this
series since we don't have this issue with the current code.
Thanks,
drew
On Tue, Aug 09, 2022 at 10:15:52AM +0100, Alexandru Elisei wrote:
> Include libcflat from page.h to avoid error like this one:
>
> /path/to/kvm-unit-tests/lib/asm/page.h:19:9: error: unknown type name ‘u64’
>19 | typedef u64 pteval_t;
> | ^~~
> [..]
> /path/to/kvm-unit-tests/lib/asm/page.h:47:8: error: unknown type name
> ‘phys_addr_t’
>47 | extern phys_addr_t __virt_to_phys(unsigned long addr);
> |^~~
> | ^~~
> [..]
> /path/to/kvm-unit-tests/lib/asm/page.h:50:47: error: unknown type name
> ‘size_t’
>50 | extern void *__ioremap(phys_addr_t phys_addr, size_t size);
>
> The arm64 version of the header already includes libcflat since commit
> a2d06852fe59 ("arm64: Add support for configuring the translation
> granule").
>
> Signed-off-by: Alexandru Elisei
> ---
> lib/arm/asm/page.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/lib/arm/asm/page.h b/lib/arm/asm/page.h
> index 8eb4a883808e..0a46bda018c7 100644
> --- a/lib/arm/asm/page.h
> +++ b/lib/arm/asm/page.h
> @@ -8,6 +8,8 @@
>
> #include
>
> +#include
> +
> #define PAGE_SHIFT 12
> #define PAGE_SIZE(_AC(1,UL) << PAGE_SHIFT)
> #define PAGE_MASK(~(PAGE_SIZE-1))
> --
> 2.37.1
>
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests RFC PATCH 12/19] arm/arm64: assembler.h: Replace size with end address for dcache_by_line_op
On Tue, Aug 09, 2022 at 10:15:51AM +0100, Alexandru Elisei wrote:
> Commit b5f659be4775 ("arm/arm64: Remove dcache_line_size global
> variable") moved the dcache_by_line_op macro to assembler.h and changed
> it to take the size of the regions instead of the end address as
> parameter. This was done to keep the file in sync with the upstream
> Linux kernel implementation at the time.
>
> But in both places where the macro is used, the code has the start and
> end address of the region, and it has to compute the size to pass it to
> dcache_by_line_op. Then the macro itsef computes the end by adding size
> to start.
>
> Get rid of this massaging of parameters and change the macro to the end
> address as parameter directly.
>
> Besides slightly simplyfing the code by remove two unneeded arithmetic
> operations, this makes the macro compatible with the current upstream
> version of Linux (which was similarly changed to take the end address in
> commit 163d3f80695e ("arm64: dcache_by_line_op to take end parameter
> instead of size")), which will allow us to reuse (part of) the Linux C
> wrappers over the assembly macro.
>
> The change has been tested with the same snippet of code used to test
> commit 410b3bf09e76 ("arm/arm64: Perform dcache clean + invalidate after
> turning MMU off").
>
> Signed-off-by: Alexandru Elisei
> ---
> arm/cstart.S | 1 -
> arm/cstart64.S| 1 -
> lib/arm/asm/assembler.h | 11 +------
> lib/arm64/asm/assembler.h | 11 +--
> 4 files changed, 10 insertions(+), 14 deletions(-)
>
Reviewed-by: Andrew Jones
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests RFC PATCH 09/19] arm/arm64: Zero secondary CPUs' stack
On Tue, Aug 09, 2022 at 10:15:48AM +0100, Alexandru Elisei wrote: > For the boot CPU, the entire stack is zeroed in the entry code. For the > secondaries, only struct thread_info, which lives at the bottom of the > stack, is zeroed in thread_info_init(). > > Be consistent and zero the entire stack for the secondaries. This should > also improve reproducibility of the testsuite, as all the stacks now start > with the same contents, which is zero. And now that all the stacks are > zeroed in the entry code, there is no need to explicitely zero struct > thread_info in thread_info_init(). > > Signed-off-by: Alexandru Elisei > --- > arm/cstart.S | 6 ++ > arm/cstart64.S| 3 +++ > lib/arm/processor.c | 1 - > lib/arm64/processor.c | 1 - > 4 files changed, 9 insertions(+), 2 deletions(-) > Reviewed-by: Andrew Jones ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests RFC PATCH 08/19] arm/arm64: Use pgd_alloc() to allocate mmu_idmap
On Tue, Aug 09, 2022 at 10:15:47AM +0100, Alexandru Elisei wrote:
> Until commit 031755dbfefb ("arm: enable vmalloc"), the idmap was allocated
> using pgd_alloc(). After that commit, all the page table allocator
> functions were switched to using the page allocator, but pgd_alloc() was
> left unchanged and became unused, with the idmap now being allocated with
> alloc_page().
>
> For arm64, the pgd table size varies based on the page size, which is
> configured by the user. For arm, it will always contain 4 entries (it
> translates bits 31:30 of the input address). To keep things simple and
> consistent with the other functions and across both architectures, modify
> pgd_alloc() to use alloc_page() instead of memalign like the rest of the
> page table allocator functions and use it to allocate the idmap.
>
> Note that when the idmap is created, alloc_ops->memalign is
> memalign_pages() which allocates memory with page granularity. Using
> memalign() as before would still have allocated a full page.
>
> Signed-off-by: Alexandru Elisei
> ---
> lib/arm/asm/pgtable.h | 4 ++--
> lib/arm/mmu.c | 4 ++--
> lib/arm64/asm/pgtable.h | 4 ++--
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
Reviewed-by: Andrew Jones
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests RFC PATCH 07/19] arm/arm64: Mark the phys_end parameter as unused in setup_mmu()
On Tue, Aug 09, 2022 at 10:15:46AM +0100, Alexandru Elisei wrote:
> phys_end was used to cap the linearly mapped memory to 3G to allow 1G of
> room for the vmalloc area to grown down. This was made useless in commit
> c1cd1a2bed69 ("arm/arm64: mmu: Remove memory layout assumptions"), when
> setup_mmu() was changed to map all the detected memory regions without
> changing their limits.
c1cd1a2bed69 was a start, but as that commit says, the 3G-4G region was
still necessary due to assumptions in the virtual memory allocator. This
patch needs to point out a vmalloc commit which removes that assumption
as well for its justification.
Thanks,
drew
>
> Signed-off-by: Alexandru Elisei
> ---
> lib/arm/mmu.c | 6 +-
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> index e1a72fe4941f..8f936acafe8b 100644
> --- a/lib/arm/mmu.c
> +++ b/lib/arm/mmu.c
> @@ -153,14 +153,10 @@ void mmu_set_range_sect(pgd_t *pgtable, uintptr_t
> virt_offset,
> }
> }
>
> -void *setup_mmu(phys_addr_t phys_end, void *unused)
> +void *setup_mmu(phys_addr_t unused0, void *unused1)
> {
> struct mem_region *r;
>
> - /* 3G-4G region is reserved for vmalloc, cap phys_end at 3G */
> - if (phys_end > (3ul << 30))
> - phys_end = 3ul << 30;
> -
> #ifdef __aarch64__
> init_alloc_vpage((void*)(4ul << 30));
>
> --
> 2.37.1
>
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests RFC PATCH 05/19] lib/alloc_phys: Remove locking
On Tue, Aug 09, 2022 at 10:15:44AM +0100, Alexandru Elisei wrote: > With powerpc moving the page allocator, there are no architectures left > which use the physical allocator after the boot setup: arm, arm64, > s390x and powerpc drain the physical allocator to initialize the page > allocator; and x86 calls setup_vm() to drain the allocator for each of > the tests that allocate memory. Please put the motivation for this change in the commit message. I looked ahead at the next patch to find it, but I'm not sure I agree with it. We should be able to keep the locking even when used early, since we probably need our locking to be something we can use early elsewhere anyway. Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests RFC PATCH 06/19] lib/alloc_phys: Remove allocation accounting
On Tue, Aug 09, 2022 at 10:15:45AM +0100, Alexandru Elisei wrote: > The page allocator has better allocation tracking and is used by all > architectures, while the physical allocator is now never used for > allocating memory. > > Simplify the physical allocator by removing allocation accounting. This > accomplishes two things: > > 1. It makes the allocator more useful, as the warning that was displayed > each allocation after the 256th is removed. > > 2. Together with the lock removal, the physical allocator becomes more > appealing as a very early allocator, when using the page allocator might > not be desirable or feasible. How does the locking cause problems when used in an early allocator? > > Also, phys_alloc_show() has received a slight change in the way it displays > the use and free regions: the end of the region is now non-inclusive, to > allow phys_alloc_show() to express that no memory has been used, or no > memory is free, in which case the start and the end adresses are equal. > > Signed-off-by: Alexandru Elisei > --- > lib/alloc_phys.c | 65 ++-- > lib/alloc_phys.h | 5 ++-- > 2 files changed, 21 insertions(+), 49 deletions(-) > Reviewed-by: Andrew Jones ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests RFC PATCH 03/19] lib/alloc_phys: Use phys_alloc_aligned_safe and rename it to memalign_early
On Tue, Aug 09, 2022 at 10:15:42AM +0100, Alexandru Elisei wrote: > phys_alloc_aligned_safe() is called only by early_memalign() and the safe > parameter is always true. In the spirit of simplifying the code, merge the > two functions together. Rename it to memalign_early(), to match the naming > scheme used by the page allocator. > > Change the type of top_safe to phys_addr_t, to match the type of the top > and base variables describing the available physical memory; this is a > cosmetic change only, since libcflat.h defines phys_addr_t as an alias > for u64. > > Signed-off-by: Alexandru Elisei > --- > lib/alloc_phys.c | 38 ++ > 1 file changed, 14 insertions(+), 24 deletions(-) > Reviewed-by: Andrew Jones ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests RFC PATCH 02/19] lib/alloc_phys: Initialize align_min
On Tue, Aug 09, 2022 at 10:15:41AM +0100, Alexandru Elisei wrote:
> Commit 11c4715fbf87 ("alloc: implement free") changed align_min from a
> static variable to a field for the alloc_ops struct and carried over the
> initializer value of DEFAULT_MINIMUM_ALIGNMENT.
>
> Commit 7e3e823b78c0 ("lib/alloc.h: remove align_min from struct
> alloc_ops") removed the align_min field and changed it back to a static
> variable, but missed initializing it.
>
> Initialize align_min to DEFAULT_MINIMUM_ALIGNMENT, as it was intended.
>
> Signed-off-by: Alexandru Elisei
> ---
> lib/alloc_phys.c | 7 +++
> lib/alloc_phys.h | 2 --
> 2 files changed, 3 insertions(+), 6 deletions(-)
Reviewed-by: Andrew Jones
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests RFC PATCH 01/19] Makefile: Define __ASSEMBLY__ for assembly files
On Tue, Aug 09, 2022 at 10:15:40AM +0100, Alexandru Elisei wrote: > There are 25 header files today (found with grep -r "#ifndef __ASSEMBLY__) > with functionality relies on the __ASSEMBLY__ prepocessor constant being > correctly defined to work correctly. So far, kvm-unit-tests has relied on > the assembly files to define the constant before including any header > files which depend on it. > > Let's make sure that nobody gets this wrong and define it as a compiler > constant when compiling assembly files. __ASSEMBLY__ is now defined for all > .S files, even those that didn't set it explicitely before. > > Signed-off-by: Alexandru Elisei > --- > Makefile | 5 - > arm/cstart.S | 1 - > arm/cstart64.S | 1 - > powerpc/cstart64.S | 1 - > 4 files changed, 4 insertions(+), 4 deletions(-) > Reviewed-by: Andrew Jones ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 09/13] KVM: selftests: aarch64: Add aarch64/page_fault_test
On Tue, Sep 20, 2022 at 04:25:47AM +, Ricardo Koller wrote:
> Add a new test for stage 2 faults when using different combinations of
> guest accesses (e.g., write, S1PTW), backing source type (e.g., anon)
> and types of faults (e.g., read on hugetlbfs with a hole). The next
> commits will add different handling methods and more faults (e.g., uffd
> and dirty logging). This first commit starts by adding two sanity checks
> for all types of accesses: AF setting by the hw, and accessing memslots
> with holes.
>
> Signed-off-by: Ricardo Koller
> ---
> tools/testing/selftests/kvm/.gitignore| 1 +
> tools/testing/selftests/kvm/Makefile | 1 +
> .../selftests/kvm/aarch64/page_fault_test.c | 601 ++
> .../selftests/kvm/include/aarch64/processor.h | 8 +
> 4 files changed, 611 insertions(+)
> create mode 100644 tools/testing/selftests/kvm/aarch64/page_fault_test.c
>
> diff --git a/tools/testing/selftests/kvm/.gitignore
> b/tools/testing/selftests/kvm/.gitignore
> index d625a3f83780..7a9022cfa033 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -3,6 +3,7 @@
> /aarch64/debug-exceptions
> /aarch64/get-reg-list
> /aarch64/hypercalls
> +/aarch64/page_fault_test
> /aarch64/psci_test
> /aarch64/vcpu_width_config
> /aarch64/vgic_init
> diff --git a/tools/testing/selftests/kvm/Makefile
> b/tools/testing/selftests/kvm/Makefile
> index 1bb471aeb103..850e317b9e82 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -149,6 +149,7 @@ TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
> TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
> TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
> TEST_GEN_PROGS_aarch64 += aarch64/hypercalls
> +TEST_GEN_PROGS_aarch64 += aarch64/page_fault_test
> TEST_GEN_PROGS_aarch64 += aarch64/psci_test
> TEST_GEN_PROGS_aarch64 += aarch64/vcpu_width_config
> TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
> diff --git a/tools/testing/selftests/kvm/aarch64/page_fault_test.c
> b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
> new file mode 100644
> index ..98f12e7af14b
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
> @@ -0,0 +1,601 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * page_fault_test.c - Test stage 2 faults.
> + *
> + * This test tries different combinations of guest accesses (e.g., write,
> + * S1PTW), backing source type (e.g., anon) and types of faults (e.g., read
> on
> + * hugetlbfs with a hole). It checks that the expected handling method is
> + * called (e.g., uffd faults with the right address and write/read flag).
> + */
> +
> +#define _GNU_SOURCE
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "guest_modes.h"
> +#include "userfaultfd_util.h"
> +
> +/* Guest virtual addresses that point to the test page and its PTE. */
> +#define TEST_GVA 0xc000
> +#define TEST_EXEC_GVA(TEST_GVA + 0x8)
> +#define TEST_PTE_GVA 0xb000
> +#define TEST_DATA0x0123456789ABCDEF
> +
> +static uint64_t *guest_test_memory = (uint64_t *)TEST_GVA;
> +
> +#define CMD_NONE (0)
> +#define CMD_SKIP_TEST(1ULL << 1)
> +#define CMD_HOLE_PT (1ULL << 2)
> +#define CMD_HOLE_DATA(1ULL << 3)
> +
> +#define PREPARE_FN_NR10
> +#define CHECK_FN_NR 10
> +
> +struct test_desc {
> + const char *name;
> + uint64_t mem_mark_cmd;
> + /* Skip the test if any prepare function returns false */
> + bool (*guest_prepare[PREPARE_FN_NR])(void);
> + void (*guest_test)(void);
> + void (*guest_test_check[CHECK_FN_NR])(void);
> + void (*dabt_handler)(struct ex_regs *regs);
> + void (*iabt_handler)(struct ex_regs *regs);
> + uint32_t pt_memslot_flags;
> + uint32_t data_memslot_flags;
> + bool skip;
> +};
> +
> +struct test_params {
> + enum vm_mem_backing_src_type src_type;
> + struct test_desc *test_desc;
> +};
> +
> +static inline void flush_tlb_page(uint64_t vaddr)
> +{
> + uint64_t page = vaddr >> 12;
> +
> + dsb(ishst);
> + asm volatile("tlbi vaae1is, %0" :: "r" (page));
> + dsb(ish);
> + isb();
> +}
> +
> +static void guest_write64(void)
> +{
> + uint64_t val;
> +
> + WRITE_ONCE(*guest_test_memory, TEST_DATA);
> + val = READ_ONCE(*guest_test_memory);
> + GUEST_ASSERT_EQ(val, TEST_DATA);
> +}
> +
> +/* Check the system for atomic instructions. */
> +static bool guest_check_lse(void)
> +{
> + uint64_t isar0 = read_sysreg(id_aa64isar0_el1);
> + uint64_t atomic;
> +
> + atomic = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64ISAR0_ATOMICS), isar0);
> + return atomic >= 2;
> +}
> +
> +static bool
Re: [PATCH v7 08/13] KVM: selftests: Use the right memslot for code, page-tables, and data allocations
On Tue, Sep 20, 2022 at 04:25:46AM +, 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 > Cc: Sean Christopherson > Cc: Andrew Jones > Signed-off-by: Ricardo Koller > --- > .../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 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 07/13] KVM: selftests: Add vm->memslots[] and enum kvm_mem_region_type
On Tue, Sep 20, 2022 at 04:25:45AM +, Ricardo Koller wrote: > The vm_create() helpers are hardcoded to place most page types (code, > page-tables, stacks, etc) in the same memslot #0, and always backed with > anonymous 4K. There are a couple of issues with that. First, tests willing > to > differ a bit, like placing page-tables in a different backing source type must > replicate much of what's already done by the vm_create() functions. Second, > the hardcoded assumption of memslot #0 holding most things is spreaded > everywhere; this makes it very hard to change. > > Fix the above issues by having selftests specify how they want memory to be > laid out. Start by changing vm_create() to not create memslot #0; a future > commit will add test that specifies all memslots used by the VM. Then, add > the > vm->memslots[] array to specify the right memslot for different memory > allocators, e.g.,: lib/elf should use the vm->[MEM_REGION_CODE] memslot. This > will be used as a way to specify the page-tables memslots (to be backed by > huge > pages for example). > > There is no functional change intended. The current commit lays out memory > exactly as before. The next commit will change the allocators to get the > region > they should be using, e.g.,: like the page table allocators using the pt > memslot. > > Cc: Sean Christopherson > Cc: Andrew Jones > Signed-off-by: Ricardo Koller > --- > .../selftests/kvm/include/kvm_util_base.h | 25 +-- > tools/testing/selftests/kvm/lib/kvm_util.c | 18 +++-- > 2 files changed, 33 insertions(+), 10 deletions(-) > Reviewed-by: Andrew Jones ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH v4 07/12] arm: pmu: Basic event counter Tests
On Mon, Sep 19, 2022 at 10:30:01PM +0800, Zenghui Yu wrote: > Hi Eric, > > A few comments when looking through the PMU test code (2 years after > the series was merged). Yes, these patches were merged long ago. Now you need to send patches, not comments. Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v6 08/13] KVM: selftests: Use the right memslot for code, page-tables, and data allocations
On Tue, Sep 06, 2022 at 06:09:25PM +, 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. > > Cc: Sean Christopherson > Cc: Andrew Jones > Signed-off-by: Ricardo Koller > --- > .../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 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v6 07/13] KVM: selftests: Change ____vm_create() to take struct kvm_vm_mem_params
On Tue, Sep 06, 2022 at 06:09:24PM +, Ricardo Koller wrote: > The vm_create() helpers are hardcoded to place most page types (code, > page-tables, stacks, etc) in the same memslot #0, and always backed with > anonymous 4K. There are a couple of issues with that. First, tests willing > to > differ a bit, like placing page-tables in a different backing source type must > replicate much of what's already done by the vm_create() functions. Second, > the hardcoded assumption of memslot #0 holding most things is spreaded > everywhere; this makes it very hard to change. > > Fix the above issues by having selftests specify how they want memory to be > laid out: define the memory regions to use for code, pt (page-tables), and > data. Introduce a new structure, struct kvm_vm_mem_params, that defines: guest > mode, a list of memory region descriptions, and some fields specifying what > regions to use for code, pt, and data. > > There is no functional change intended. The current commit adds a default > struct kvm_vm_mem_params that lays out memory exactly as before. The next > commit will change the allocators to get the region they should be using, > e.g.,: like the page table allocators using the pt memslot. > > Cc: Sean Christopherson > Cc: Andrew Jones > Signed-off-by: Ricardo Koller > --- > .../selftests/kvm/include/kvm_util_base.h | 51 +++- > .../selftests/kvm/lib/aarch64/processor.c | 3 +- > tools/testing/selftests/kvm/lib/kvm_util.c| 58 ++++--- > 3 files changed, 102 insertions(+), 10 deletions(-) > Reviewed-by: Andrew Jones ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 7/7] KVM: selftests: Add test for RAZ/WI AArch32 ID registers
On Fri, Sep 02, 2022 at 03:48:03PM +, Oliver Upton wrote:
> Add a test to assert that KVM handles the AArch64 views of the AArch32
> ID registers as RAZ/WI (writable only from userspace).
>
> Signed-off-by: Oliver Upton
> ---
> tools/testing/selftests/kvm/.gitignore| 1 +
> tools/testing/selftests/kvm/Makefile | 1 +
> .../kvm/aarch64/aarch64_only_id_regs.c| 135 ++
> 3 files changed, 137 insertions(+)
> create mode 100644 tools/testing/selftests/kvm/aarch64/aarch64_only_id_regs.c
>
> diff --git a/tools/testing/selftests/kvm/.gitignore
> b/tools/testing/selftests/kvm/.gitignore
> index d625a3f83780..4331af62a982 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -1,4 +1,5 @@
> # SPDX-License-Identifier: GPL-2.0-only
> +/aarch64/aarch64_only_id_regs
> /aarch64/arch_timer
> /aarch64/debug-exceptions
> /aarch64/get-reg-list
> diff --git a/tools/testing/selftests/kvm/Makefile
> b/tools/testing/selftests/kvm/Makefile
> index 4c122f1b1737..efe155259095 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -144,6 +144,7 @@ TEST_GEN_PROGS_x86_64 += system_counter_offset_test
> # Compiled outputs used by test targets
> TEST_GEN_PROGS_EXTENDED_x86_64 += x86_64/nx_huge_pages_test
>
> +TEST_GEN_PROGS_aarch64 += aarch64/aarch64_only_id_regs
> TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
> TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
> TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
> diff --git a/tools/testing/selftests/kvm/aarch64/aarch64_only_id_regs.c
> b/tools/testing/selftests/kvm/aarch64/aarch64_only_id_regs.c
> new file mode 100644
> index ..704a3e7524a8
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/aarch64/aarch64_only_id_regs.c
> @@ -0,0 +1,135 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * aarch64_only_id_regs - Test for ID register behavior on AArch64-only
> systems
> + *
> + * Copyright (c) 2022 Google LLC.
> + *
> + * Test that KVM handles the AArch64 views of the AArch32 ID registers as RAZ
> + * and WI from userspace.
> + */
> +
> +#include
> +
> +#include "kvm_util.h"
> +#include "processor.h"
> +#include "test_util.h"
> +
> +#define BAD_ID_REG_VAL 0x1badc0deul
> +
> +#define GUEST_ASSERT_REG_RAZ(reg)GUEST_ASSERT_EQ(read_sysreg_s(reg), 0)
> +
> +static void guest_main(void)
> +{
> + GUEST_ASSERT_REG_RAZ(SYS_ID_PFR0_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_ID_PFR1_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_ID_DFR0_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_ID_MMFR0_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_ID_MMFR1_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_ID_MMFR2_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_ID_MMFR3_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR0_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR1_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR2_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR3_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR4_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR5_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_ID_MMFR4_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR6_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_MVFR0_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_MVFR1_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_MVFR2_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_ID_PFR2_EL1);
> + GUEST_ASSERT_REG_RAZ(SYS_ID_MMFR5_EL1);
> +
> + GUEST_DONE();
> +}
> +
> +static void test_guest_raz(struct kvm_vcpu *vcpu)
> +{
> + struct ucall uc;
> +
> + vcpu_run(vcpu);
> +
> + switch (get_ucall(vcpu, )) {
> + case UCALL_ABORT:
> + REPORT_GUEST_ASSERT(uc);
> + break;
> + case UCALL_DONE:
> + break;
> + default:
> + TEST_FAIL("Unexpected ucall: %lu", uc.cmd);
> + }
> +}
> +
> +static uint64_t reg_ids[] = {
> + KVM_ARM64_SYS_REG(SYS_ID_PFR0_EL1),
> + KVM_ARM64_SYS_REG(SYS_ID_PFR1_EL1),
> + KVM_ARM64_SYS_REG(SYS_ID_DFR0_EL1),
> + KVM_ARM64_SYS_REG(SYS_ID_MMFR0_EL1),
> + KVM_ARM64_SYS_REG(SYS_ID_MMFR1_EL1),
> + KVM_ARM64_SYS_REG(SYS_ID_MMFR2_EL1),
> + KVM_ARM64_SYS_REG(SYS_ID_MMFR3_EL1),
> + KVM_ARM64_SYS_REG(SYS_ID_ISAR0_EL1),
> + KVM_ARM64_SYS_REG(SYS_ID_ISAR1_EL1),
> + KVM_ARM64_SYS_REG(SYS_ID_ISAR2_EL1),
> + KVM_ARM64_SYS_REG(SYS_ID_ISAR3_EL1),
> + KVM_ARM64_SYS_REG(SYS_ID_ISAR4_EL1),
> + KVM_ARM64_SYS_REG(SYS_ID_ISAR5_EL1),
> + KVM_ARM64_SYS_REG(SYS_ID_MMFR4_EL1),
> + KVM_ARM64_SYS_REG(SYS_ID_ISAR6_EL1),
> + KVM_ARM64_SYS_REG(SYS_MVFR0_EL1),
> + KVM_ARM64_SYS_REG(SYS_MVFR1_EL1),
> + KVM_ARM64_SYS_REG(SYS_MVFR2_EL1),
> + KVM_ARM64_SYS_REG(SYS_ID_PFR2_EL1),
> + KVM_ARM64_SYS_REG(SYS_ID_MMFR5_EL1),
Hi Oliver,
I see all the hidden and unallocated registers have been filtered out of
the test lists. They should also behave as RAZ, right? Maybe we should
keep them in the lists here for consistency and to test them as well.
Thanks,
drew
> +};
> +
> +static void
Re: [PATCH v5 08/13] KVM: selftests: Use the right memslot for code, page-tables, and data allocations
On Tue, Aug 23, 2022 at 11:47:22PM +, 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.
>
> Cc: Sean Christopherson
> Cc: Andrew Jones
> Signed-off-by: Ricardo Koller
> ---
> .../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(-)
>
> diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h
> b/tools/testing/selftests/kvm/include/kvm_util_base.h
> index abe6c4e390ff..696719b227b9 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> @@ -406,7 +406,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, uint32_t memslot);
With the enum suggestion these take 'enum memslot_type', allowing this...
> 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, uint32_t memslot);
> 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 5a31dc85d054..b6ebfdaf4957 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->memslot.pt);
...to be KVM_GUEST_PAGE_TABLE_MIN_PADDR, MEMSLOT_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,
> + vm->memslot.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, vm->memslot.code);
>
> *(vm_vaddr_t *)addr_gva2hva(vm, (vm_vaddr_t)(_handlers)) =
> vm->handlers;
> }
> diff --git a/tools/testing/selftests/kvm/lib/elf.c
> b/tools/testing/selftests/kvm/lib/elf.c
> index 9f54c098d9d0..62cfbe3b6171 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_al
Re: [PATCH v5 07/13] KVM: selftests: Change ____vm_create() to take struct kvm_vm_mem_params
On Tue, Aug 23, 2022 at 11:47:21PM +, Ricardo Koller wrote:
> The vm_create() helpers are hardcoded to place most page types (code,
> page-tables, stacks, etc) in the same memslot #0, and always backed with
> anonymous 4K. There are a couple of issues with that. First, tests willing
> to
> differ a bit, like placing page-tables in a different backing source type must
> replicate much of what's already done by the vm_create() functions. Second,
> the hardcoded assumption of memslot #0 holding most things is spreaded
> everywhere; this makes it very hard to change.
>
> Fix the above issues by having selftests specify how they want memory to be
> laid out: define the memory regions to use for code, pt (page-tables), and
> data. Introduce a new structure, struct kvm_vm_mem_params, that defines: guest
> mode, a list of memory region descriptions, and some fields specifying what
> regions to use for code, pt, and data.
>
> There is no functional change intended. The current commit adds a default
> struct kvm_vm_mem_params that lays out memory exactly as before. The next
> commit will change the allocators to get the region they should be using,
> e.g.,: like the page table allocators using the pt memslot.
>
> Cc: Sean Christopherson
> Cc: Andrew Jones
> Signed-off-by: Ricardo Koller
> ---
> .../selftests/kvm/include/kvm_util_base.h | 61 -
> .../selftests/kvm/lib/aarch64/processor.c | 3 +-
> tools/testing/selftests/kvm/lib/kvm_util.c| 65 +--
> 3 files changed, 119 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h
> b/tools/testing/selftests/kvm/include/kvm_util_base.h
> index b2dbe253d4d0..abe6c4e390ff 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> @@ -93,6 +93,16 @@ struct kvm_vm {
> int stats_fd;
> struct kvm_stats_header stats_header;
> struct kvm_stats_desc *stats_desc;
> +
> + /*
> + * KVM region slots. These are the default memslots used by page
> + * allocators, e.g., lib/elf uses the code memslot.
> + */
> + struct {
> + uint32_t code;
> + uint32_t pt;
> + uint32_t data;
> + } memslot;
> };
>
>
> @@ -105,6 +115,21 @@ struct kvm_vm {
> struct userspace_mem_region *
> memslot2region(struct kvm_vm *vm, uint32_t memslot);
>
> +inline struct userspace_mem_region *vm_get_code_region(struct kvm_vm *vm)
> +{
> + return memslot2region(vm, vm->memslot.code);
> +}
> +
> +inline struct userspace_mem_region *vm_get_pt_region(struct kvm_vm *vm)
> +{
> + return memslot2region(vm, vm->memslot.pt);
> +}
> +
> +inline struct userspace_mem_region *vm_get_data_region(struct kvm_vm *vm)
> +{
> + return memslot2region(vm, vm->memslot.data);
> +}
I feel we'll be revisiting this frequently when more and more region types
are desired. For example, Sean wants a read-only memory region for ucall
exits. How about putting a mem slot array in struct kvm_vm, defining an
enum to index it (which will expand), and then single helper function,
something like
inline struct userspace_mem_region *
vm_get_mem_region(struct kvm_vm *vm, enum memslot_type mst)
{
return memslot2region(vm, vm->memslots[mst]);
}
> +
> /* Minimum allocated guest virtual and physical addresses */
> #define KVM_UTIL_MIN_VADDR 0x2000
> #define KVM_GUEST_PAGE_TABLE_MIN_PADDR 0x18
> @@ -637,19 +662,51 @@ 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);
>
> +#define MEM_PARAMS_MAX_MEMSLOTS 3
And this becomes MEMSLOT_MAX of the enum proposed above
enum memslot_type {
MEMSLOT_CODE,
MEMSLOT_PT,
MEMSLOT_DATA,
MEMSLOT_MAX,
};
> +
> +struct kvm_vm_mem_params {
> + enum vm_guest_mode mode;
> +
> + struct {
> + enum vm_mem_backing_src_type src_type;
> + uint64_t guest_paddr;
> + /*
> + * KVM region slot (same meaning as in struct
> + * kvm_userspace_memory_region).
> + */
> + uint32_t slot;
> + uint64_t npages;
> + uint32_t flags;
> + bool enabled;
> + } region[MEM_PARAMS_MAX_MEMSLOTS];
> +
> + /* Indexes into the above array. */
> + struct {
> + uint16_t code;
> + uint16_t pt;
> + uint16_t data;
> + } region_idx;
And this changes to another array of memslots also indexed with
enum memslot_typ
Re: [PATCH v5 7/7] KVM: selftests: Add ucall pool based implementation
selftests/kvm/lib/s390x/ucall.c
> b/tools/testing/selftests/kvm/lib/s390x/ucall.c
> index cbee520a26f2..a7f02dc372cf 100644
> --- a/tools/testing/selftests/kvm/lib/s390x/ucall.c
> +++ b/tools/testing/selftests/kvm/lib/s390x/ucall.c
> @@ -26,7 +26,7 @@ void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
> (run->s390_sieic.ipb >> 16) == 0x501) {
> int reg = run->s390_sieic.ipa & 0xf;
>
> - return addr_gva2hva(vcpu->vm, run->s.regs.gprs[reg]);
> + return (void *)run->s.regs.gprs[reg];
> }
> return NULL;
> }
> diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c
> b/tools/testing/selftests/kvm/lib/ucall_common.c
> index ced480860746..cc79d497b6b4 100644
> --- a/tools/testing/selftests/kvm/lib/ucall_common.c
> +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
> @@ -1,22 +1,85 @@
> // SPDX-License-Identifier: GPL-2.0-only
> #include "kvm_util.h"
> +#include "linux/types.h"
> +#include "linux/bitmap.h"
> +#include "linux/atomic.h"
> +
> +struct ucall_header {
> + DECLARE_BITMAP(in_use, KVM_MAX_VCPUS);
> + struct ucall ucalls[KVM_MAX_VCPUS];
> +};
> +
> +/*
> + * ucall_pool holds per-VM values (global data is duplicated by each VM), it
> + * must not be accessed from host code.
> + */
> +static struct ucall_header *ucall_pool;
> +
> +void ucall_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa)
> +{
> + struct ucall_header *hdr;
> + struct ucall *uc;
> + vm_vaddr_t vaddr;
> + int i;
> +
> + vaddr = vm_vaddr_alloc(vm, sizeof(*hdr), KVM_UTIL_MIN_VADDR);
> + hdr = (struct ucall_header *)addr_gva2hva(vm, vaddr);
> + memset(hdr, 0, sizeof(*hdr));
> +
> + for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> + uc = >ucalls[i];
> + uc->hva = uc;
> + }
> +
> + write_guest_global(vm, ucall_pool, (struct ucall_header *)vaddr);
> +
> + ucall_arch_init(vm, mmio_gpa);
> +}
> +
> +static struct ucall *ucall_alloc(void)
> +{
> + struct ucall *uc;
> + int i;
> +
> + GUEST_ASSERT(ucall_pool && ucall_pool->in_use);
ucall_pool->in_use will never be null.
> +
> + for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> + if (!atomic_test_and_set_bit(i, ucall_pool->in_use)) {
> + uc = _pool->ucalls[i];
> + memset(uc->args, 0, sizeof(uc->args));
> + return uc;
> + }
> + }
nit: blank line
> + GUEST_ASSERT(0);
> + return NULL;
> +}
> +
> +static void ucall_free(struct ucall *uc)
> +{
> + /* Beware, here be pointer arithmetic. */
> + clear_bit(uc - ucall_pool->ucalls, ucall_pool->in_use);
> +}
>
> void ucall(uint64_t cmd, int nargs, ...)
> {
> - struct ucall uc = {};
> + struct ucall *uc;
> va_list va;
> int i;
>
> - WRITE_ONCE(uc.cmd, cmd);
> + uc = ucall_alloc();
> +
> + WRITE_ONCE(uc->cmd, cmd);
>
> nargs = min(nargs, UCALL_MAX_ARGS);
>
> va_start(va, nargs);
> for (i = 0; i < nargs; ++i)
> - WRITE_ONCE(uc.args[i], va_arg(va, uint64_t));
> + WRITE_ONCE(uc->args[i], va_arg(va, uint64_t));
> va_end(va);
>
> - ucall_arch_do_ucall((vm_vaddr_t));
> + ucall_arch_do_ucall((vm_vaddr_t)uc->hva);
> +
> + ucall_free(uc);
> }
>
> uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/ucall.c
> b/tools/testing/selftests/kvm/lib/x86_64/ucall.c
> index eb8bf55b359a..4d41dc63cc9e 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/ucall.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/ucall.c
> @@ -26,7 +26,7 @@ void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
> struct kvm_regs regs;
>
> vcpu_regs_get(vcpu, );
> - return addr_gva2hva(vcpu->vm, regs.rdi);
> + return (void *)regs.rdi;
> }
> return NULL;
> }
> --
> 2.37.2.672.g94769d06f0-goog
Otherwise,
Reviewed-by: Andrew Jones
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v5 6/7] KVM: selftest: Drop now-unnecessary ucall_uninit()
On Thu, Aug 25, 2022 at 11:25:21PM +, Sean Christopherson wrote: > Drop ucall_uninit() and ucall_arch_uninit() now that ARM doesn't modify > the host's copy of ucall_exit_mmio_addr, i.e. now that there's no need to > reset the pointer before potentially creating a new VM. The few calls to > ucall_uninit() are all immediately followed by kvm_vm_free(), and that is > likely always going to hold true, i.e. it's extremely unlikely a test > will want to effectively disable ucall in the middle of a test. > > Signed-off-by: Sean Christopherson > --- > tools/testing/selftests/kvm/dirty_log_test.c | 1 - > tools/testing/selftests/kvm/include/ucall_common.h | 6 -- > tools/testing/selftests/kvm/kvm_page_table_test.c | 1 - > tools/testing/selftests/kvm/lib/aarch64/ucall.c| 14 ++ > tools/testing/selftests/kvm/lib/perf_test_util.c | 1 - > tools/testing/selftests/kvm/lib/riscv/ucall.c | 4 > tools/testing/selftests/kvm/lib/s390x/ucall.c | 4 > tools/testing/selftests/kvm/lib/x86_64/ucall.c | 4 > 8 files changed, 2 insertions(+), 33 deletions(-) > Reviewed-by: Andrew Jones ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v5 5/7] KVM: selftests: Make arm64's MMIO ucall multi-VM friendly
On Thu, Aug 25, 2022 at 11:25:20PM +, Sean Christopherson wrote: > Fix a mostly-theoretical bug where ARM's ucall MMIO setup could result in > different VMs stomping on each other by cloberring the global pointer. > > Fix the most obvious issue by saving the MMIO gpa into the VM. > > A more subtle bug is that creating VMs in parallel (on multiple tasks) > could result in a VM using the wrong address. Synchronizing a global to > a guest effectively snapshots the value on a per-VM basis, i.e. the > "global" is already prepped to work with multiple VMs, but setting the > global in the host is not thread-safe. To fix that bug, add > write_guest_global() to allow stuffing a VM's copy of a "global" without > modifying the host value. > > Signed-off-by: Sean Christopherson > --- > .../selftests/kvm/include/kvm_util_base.h | 15 +++ > .../testing/selftests/kvm/lib/aarch64/ucall.c | 19 ++- > 2 files changed, 29 insertions(+), 5 deletions(-) > Reviewed-by: Andrew Jones ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v5 3/7] KVM: selftests: Automatically do init_ucall() for non-barebones VMs
On Thu, Aug 25, 2022 at 11:25:18PM +, Sean Christopherson wrote: > Do init_ucall() automatically during VM creation to kill two (three?) > birds with one stone. > > First, initializing ucall immediately after VM creations allows forcing > aarch64's MMIO ucall address to immediately follow memslot0. This is > still somewhat fragile as tests could clobber the MMIO address with a > new memslot, but it's safe-ish since tests have to be conversative when > accounting for memslot0. And this can be hardened in the future by > creating a read-only memslot for the MMIO page (KVM ARM exits with MMIO > if the guest writes to a read-only memslot). Add a TODO to document that > selftests can and should use a memslot for the ucall MMIO (doing so > requires yet more rework because tests assumes thay can use all memslots > except memslot0). > > Second, initializing ucall for all VMs prepares for making ucall > initialization meaningful on all architectures. aarch64 is currently the > only arch that needs to do any setup, but that will change in the future > by switching to a pool-based implementation (instead of the current > stack-based approach). > > Lastly, defining the ucall MMIO address from common code will simplify > switching all architectures (except s390) to a common MMIO-based ucall > implementation (if there's ever sufficient motivation to do so). > > Cc: Oliver Upton > Signed-off-by: Sean Christopherson > --- > .../selftests/kvm/aarch64/arch_timer.c| 1 - > .../selftests/kvm/aarch64/debug-exceptions.c | 1 - > .../selftests/kvm/aarch64/hypercalls.c| 1 - > .../testing/selftests/kvm/aarch64/psci_test.c | 1 - > .../testing/selftests/kvm/aarch64/vgic_init.c | 2 - > .../testing/selftests/kvm/aarch64/vgic_irq.c | 1 - > tools/testing/selftests/kvm/dirty_log_test.c | 2 - > .../selftests/kvm/include/ucall_common.h | 6 +-- > .../selftests/kvm/kvm_page_table_test.c | 1 - > .../testing/selftests/kvm/lib/aarch64/ucall.c | 54 ++- > tools/testing/selftests/kvm/lib/kvm_util.c| 11 > .../selftests/kvm/lib/perf_test_util.c| 2 - > tools/testing/selftests/kvm/lib/riscv/ucall.c | 2 +- > tools/testing/selftests/kvm/lib/s390x/ucall.c | 2 +- > .../testing/selftests/kvm/lib/x86_64/ucall.c | 2 +- > .../testing/selftests/kvm/memslot_perf_test.c | 1 - > tools/testing/selftests/kvm/rseq_test.c | 1 - > tools/testing/selftests/kvm/steal_time.c | 1 - > .../kvm/system_counter_offset_test.c | 1 - > 19 files changed, 20 insertions(+), 73 deletions(-) > Reviewed-by: Andrew Jones Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v1 3/5] KVM: selftests: Dirty host pages in dirty_log_test
On Fri, Aug 19, 2022 at 08:55:59AM +0800, Gavin Shan wrote: > It's assumed that 1024 host pages, instead of guest pages, are dirtied > in each iteration in guest_code(). The current implementation misses > the case of mismatched page sizes in host and guest. For example, > ARM64 could have 64KB page size in guest, but 4KB page size in host. > (TEST_PAGES_PER_LOOP / 16), instead of TEST_PAGES_PER_LOOP, host pages > are dirtied in every iteration. > > Fix the issue by touching all sub-pages when we have mismatched > page sizes in host and guest. I'll let the dirty-log test authors decide what's best to do for this test, but I'd think we should let the guest continue dirtying its pages without knowledge of the host pages. Then, adjust the host test code to assert all sub-pages, other than the ones it expects the guest to have written, remain untouched. Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH v4 0/4] arm: pmu: Fixes for bare metal
On Thu, Aug 11, 2022 at 11:52:06AM -0700, Ricardo Koller wrote: > There are some tests that fail when running on bare metal (including a > passthrough prototype). There are three issues with the tests. The > first one is that there are some missing isb()'s between enabling event > counting and the actual counting. This wasn't an issue on KVM as > trapping on registers served as context synchronization events. The > second issue is that some tests assume that registers reset to 0. And > finally, the third issue is that overflowing the low counter of a > chained event sets the overflow flag in PMVOS and some tests fail by > checking for it not being set. > > Addressed all comments from the previous version: > https://lore.kernel.org/kvmarm/yvpsbkgbhhqp+...@google.com/T/#mb077998e2eb9fb3e15930b3412fd7ba2fb4103ca > - add pmu_reset() for 32-bit arm [Andrew] > - collect r-b from Alexandru You forgot to pick up Oliver's r-b's and his Link suggestion. I can do that again, though. Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH v3 2/3] arm: pmu: Reset the pmu registers before starting some tests
On Wed, Aug 10, 2022 at 04:33:26PM -0700, Ricardo Koller wrote: > On Wed, Aug 10, 2022 at 09:02:16PM +0200, Andrew Jones wrote: > > On Thu, Aug 04, 2022 at 05:41:38PM -0700, Ricardo Koller wrote: > > > Some registers like the PMOVS reset to an architecturally UNKNOWN value. > > > Most tests expect them to be reset (mostly zeroed) using pmu_reset(). > > > Add a pmu_reset() on all the tests that need one. > > > > > > As a bonus, fix a couple of comments related to the register state > > > before a sub-test. > > > > > > Reviewed-by: Eric Auger > > > Signed-off-by: Ricardo Koller > > > --- > > > arm/pmu.c | 6 -- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/arm/pmu.c b/arm/pmu.c > > > index 4c601b05..12e7d84e 100644 > > > --- a/arm/pmu.c > > > +++ b/arm/pmu.c > > > @@ -826,7 +826,7 @@ static void test_overflow_interrupt(void) > > > write_regn_el0(pmevcntr, 1, PRE_OVERFLOW); > > > isb(); > > > > > > - /* interrupts are disabled */ > > > + /* interrupts are disabled (PMINTENSET_EL1 == 0) */ > > > > > > mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E); > > > report(expect_interrupts(0), "no overflow interrupt after preset"); > > > @@ -842,7 +842,7 @@ static void test_overflow_interrupt(void) > > > isb(); > > > report(expect_interrupts(0), "no overflow interrupt after counting"); > > > > > > - /* enable interrupts */ > > > + /* enable interrupts (PMINTENSET_EL1 <= ALL_SET) */ > > > > > > pmu_reset_stats(); > > > > > > @@ -890,6 +890,7 @@ static bool check_cycles_increase(void) > > > bool success = true; > > > > > > /* init before event access, this test only cares about cycle count */ > > > + pmu_reset(); > > > > This and the other pmu_reset() call below break compilation on 32-bit arm, > > because there's no pmu_reset() defined for it. > I completely missed the 32-bit arm case. Thanks! > > > It'd probably be best if > > we actually implemented some sort of reset for arm, considering it's being > > called in common tests. > > Mind if I start by creating a pmu_reset() for 32-bit arm, which can > later be used by a general arm_reset()? No need to worry about a general one. We just need a pmu_reset implemented for 32-bit arm up in its #ifdef __arm__ section. Thanks, drew > > > > > Thanks, > > drew > > > > > set_pmcntenset(1 << PMU_CYCLE_IDX); > > > set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */ > > > > > > @@ -944,6 +945,7 @@ static bool check_cpi(int cpi) > > > uint32_t pmcr = get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E; > > > > > > /* init before event access, this test only cares about cycle count */ > > > + pmu_reset(); > > > set_pmcntenset(1 << PMU_CYCLE_IDX); > > > set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */ > > > > > > -- > > > 2.37.1.559.g78731f0fdb-goog > > > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH v3 2/3] arm: pmu: Reset the pmu registers before starting some tests
On Thu, Aug 04, 2022 at 05:41:38PM -0700, Ricardo Koller wrote: > Some registers like the PMOVS reset to an architecturally UNKNOWN value. > Most tests expect them to be reset (mostly zeroed) using pmu_reset(). > Add a pmu_reset() on all the tests that need one. > > As a bonus, fix a couple of comments related to the register state > before a sub-test. > > Reviewed-by: Eric Auger > Signed-off-by: Ricardo Koller > --- > arm/pmu.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/arm/pmu.c b/arm/pmu.c > index 4c601b05..12e7d84e 100644 > --- a/arm/pmu.c > +++ b/arm/pmu.c > @@ -826,7 +826,7 @@ static void test_overflow_interrupt(void) > write_regn_el0(pmevcntr, 1, PRE_OVERFLOW); > isb(); > > - /* interrupts are disabled */ > + /* interrupts are disabled (PMINTENSET_EL1 == 0) */ > > mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E); > report(expect_interrupts(0), "no overflow interrupt after preset"); > @@ -842,7 +842,7 @@ static void test_overflow_interrupt(void) > isb(); > report(expect_interrupts(0), "no overflow interrupt after counting"); > > - /* enable interrupts */ > + /* enable interrupts (PMINTENSET_EL1 <= ALL_SET) */ > > pmu_reset_stats(); > > @@ -890,6 +890,7 @@ static bool check_cycles_increase(void) > bool success = true; > > /* init before event access, this test only cares about cycle count */ > + pmu_reset(); This and the other pmu_reset() call below break compilation on 32-bit arm, because there's no pmu_reset() defined for it. It'd probably be best if we actually implemented some sort of reset for arm, considering it's being called in common tests. Thanks, drew > set_pmcntenset(1 << PMU_CYCLE_IDX); > set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */ > > @@ -944,6 +945,7 @@ static bool check_cpi(int cpi) > uint32_t pmcr = get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E; > > /* init before event access, this test only cares about cycle count */ > + pmu_reset(); > set_pmcntenset(1 << PMU_CYCLE_IDX); > set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */ > > -- > 2.37.1.559.g78731f0fdb-goog > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH v3 3/3] arm: pmu: Check for overflow in the low counter in chained counters tests
On Wed, Aug 10, 2022 at 12:30:26PM -0500, Oliver Upton wrote:
> On Thu, Aug 04, 2022 at 05:41:39PM -0700, Ricardo Koller wrote:
> > A chained event overflowing on the low counter can set the overflow flag
> > in PMOVS. KVM does not set it, but real HW and the fast-model seem to.
> > Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> > (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> > overflow.
> >
> > The pmu chain tests fail on bare metal when checking the overflow flag
> > of the low counter _not_ being set on overflow. Fix by checking for
> > overflow. Note that this test fails in KVM without the respective fix.
> >
>
> It'd be good to link out to the respective KVM fix, either by commit or
> lore link if this patch lands before the kernel patches:
>
> Link: http://lore.kernel.org/r/20220805135813.2102034-1-...@kernel.org
I'll pick that up with the tags when preparing to push.
Thanks,
drew
>
> --
> Thanks,
> Oliver
>
> > Reviewed-by: Eric Auger
> > Signed-off-by: Ricardo Koller
> > ---
> > arm/pmu.c | 33 ++---
> > 1 file changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/arm/pmu.c b/arm/pmu.c
> > index 12e7d84e..0a7e12f8 100644
> > --- a/arm/pmu.c
> > +++ b/arm/pmu.c
> > @@ -583,7 +583,7 @@ static void test_chained_counters(void)
> > precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> >
> > report(read_regn_el0(pmevcntr, 1) == 1, "CHAIN counter #1 incremented");
> > - report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained
> > incr #1");
> > + report(read_sysreg(pmovsclr_el0) == 0x1, "overflow recorded for chained
> > incr #1");
> >
> > /* test 64b overflow */
> >
> > @@ -595,7 +595,7 @@ static void test_chained_counters(void)
> > precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> > report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> > report(read_regn_el0(pmevcntr, 1) == 2, "CHAIN counter #1 set to 2");
> > - report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained
> > incr #2");
> > + report(read_sysreg(pmovsclr_el0) == 0x1, "overflow recorded for chained
> > incr #2");
> >
> > write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > write_regn_el0(pmevcntr, 1, ALL_SET);
> > @@ -603,7 +603,7 @@ static void test_chained_counters(void)
> > precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> > report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> > report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
> > - report(read_sysreg(pmovsclr_el0) == 0x2, "overflow on chain counter");
> > + report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd
> > counters");
> > }
> >
> > static void test_chained_sw_incr(void)
> > @@ -629,8 +629,9 @@ static void test_chained_sw_incr(void)
> > write_sysreg(0x1, pmswinc_el0);
> >
> > isb();
> > - report(!read_sysreg(pmovsclr_el0) && (read_regn_el0(pmevcntr, 1) == 1),
> > - "no overflow and chain counter incremented after 100
> > SW_INCR/CHAIN");
> > + report((read_sysreg(pmovsclr_el0) == 0x1) &&
> > + (read_regn_el0(pmevcntr, 1) == 1),
> > + "overflow and chain counter incremented after 100
> > SW_INCR/CHAIN");
> > report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
> > read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> >
> > @@ -648,10 +649,10 @@ static void test_chained_sw_incr(void)
> > write_sysreg(0x1, pmswinc_el0);
> >
> > isb();
> > - report((read_sysreg(pmovsclr_el0) == 0x2) &&
> > + report((read_sysreg(pmovsclr_el0) == 0x3) &&
> > (read_regn_el0(pmevcntr, 1) == 0) &&
> > (read_regn_el0(pmevcntr, 0) == 84),
> > - "overflow on chain counter and expected values after 100
> > SW_INCR/CHAIN");
> > + "expected overflows and values after 100 SW_INCR/CHAIN");
> > report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
> > read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> > }
> > @@ -731,8 +732,9 @@ static void test_chain_promotion(void)
> > report_info("MEM_ACCESS counter #0 has value 0x%lx",
> > read_regn_el0(pmevcntr, 0));
> >
> > - report((read_regn_el0(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
> > - "CHAIN counter enabled: CHAIN counter was incremented and no
> > overflow");
> > + report((read_regn_el0(pmevcntr, 1) == 1) &&
> > + (read_sysreg(pmovsclr_el0) == 0x1),
> > + "CHAIN counter enabled: CHAIN counter was incremented and
> > overflow");
> >
> > report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
> > read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
> > @@ -759,8 +761,9 @@ static void test_chain_promotion(void)
> > report_info("MEM_ACCESS counter #0 has value 0x%lx",
> > read_regn_el0(pmevcntr, 0));
> >
>
Re: [kvm-unit-tests PATCH v2 3/3] arm: pmu: Check for overflow in the low counter in chained counters tests
On Wed, Aug 03, 2022 at 11:23:28AM -0700, Ricardo Koller wrote:
> A chained event overflowing on the low counter can set the overflow flag
> in PMOVS. KVM does not set it, but real HW and the fast-model seem to.
> Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> overflow.
>
> The pmu chain tests fail on bare metal when checking the overflow flag
> of the low counter _not_ being set on overflow. Fix by checking for
> overflow. Note that this test fails in KVM without the respective fix.
>
> Signed-off-by: Ricardo Koller
> ---
> arm/pmu.c | 33 ++---
> 1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 7c5bc259..258780f4 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -583,7 +583,7 @@ static void test_chained_counters(void)
> precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
>
> report(read_regn_el0(pmevcntr, 1) == 1, "CHAIN counter #1 incremented");
> - report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained
> incr #1");
> + report(read_sysreg(pmovsclr_el0) == 0x1, "overflow recorded for chained
> incr #1");
>
> /* test 64b overflow */
>
> @@ -595,7 +595,7 @@ static void test_chained_counters(void)
> precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> report(read_regn_el0(pmevcntr, 1) == 2, "CHAIN counter #1 set to 2");
> - report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained
> incr #2");
> + report(read_sysreg(pmovsclr_el0) == 0x1, "overflow recorded for chained
> incr #2");
>
> write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> write_regn_el0(pmevcntr, 1, ALL_SET);
> @@ -603,7 +603,7 @@ static void test_chained_counters(void)
> precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
> - report(read_sysreg(pmovsclr_el0) == 0x2, "overflow on chain counter");
> + report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd
> counters");
> }
>
> static void test_chained_sw_incr(void)
> @@ -629,8 +629,9 @@ static void test_chained_sw_incr(void)
> write_sysreg(0x1, pmswinc_el0);
>
> isb();
> - report(!read_sysreg(pmovsclr_el0) && (read_regn_el0(pmevcntr, 1) == 1),
> - "no overflow and chain counter incremented after 100
> SW_INCR/CHAIN");
> + report((read_sysreg(pmovsclr_el0) == 0x1) &&
> + (read_regn_el0(pmevcntr, 1) == 1),
> + "overflow and chain counter incremented after 100
> SW_INCR/CHAIN");
> report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
> read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
>
> @@ -648,10 +649,10 @@ static void test_chained_sw_incr(void)
> write_sysreg(0x1, pmswinc_el0);
>
> isb();
> - report((read_sysreg(pmovsclr_el0) == 0x2) &&
> + report((read_sysreg(pmovsclr_el0) == 0x3) &&
> (read_regn_el0(pmevcntr, 1) == 0) &&
> (read_regn_el0(pmevcntr, 0) == 84),
> - "overflow on chain counter and expected values after 100
> SW_INCR/CHAIN");
> + "overflow on even and odd counters, and expected values after
> 100 SW_INCR/CHAIN");
Besides the extra space, this doesn't read well (to me).
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 09/13] KVM: selftests: aarch64: Add aarch64/page_fault_test
On Fri, Jul 22, 2022 at 06:20:07PM +, Sean Christopherson wrote: > On Fri, Jul 22, 2022, Ricardo Koller wrote: > > On Thu, Jul 21, 2022 at 01:29:34AM +, Sean Christopherson wrote: > > > If we don't care, and maybe even if we do, then my preference would be to > > > enhance the __vm_create family of helpers to allow for specifying what > > > backing type should be used for page tables, i.e. associate the info the > > > VM > > > instead of passing it around the stack. > > > > > > One idea would be to do something like David Matlack suggested a while > > > back > > > and replace extra_mem_pages with a struct, e.g. struct kvm_vm_mem_params > > > That struct can then provide the necessary knobs to control how memory is > > > allocated. And then the lib can provide a global > > > > > > struct kvm_vm_mem_params kvm_default_vm_mem_params; > > > > > > > I like this idea, passing the info at vm creation. > > > > What about dividing the changes in two. > > > > 1. Will add the struct to "__vm_create()" as part of this > > series, and then use it in this commit. There's only one user > > > > dirty_log_test.c: vm = __vm_create(mode, 1, extra_mem_pages); > > > > so that would avoid having to touch every test as part of this patchset. > > > > 2. I can then send another series to add support for all the other > > vm_create() functions. > > > > Alternatively, I can send a new series that does 1 and 2 afterwards. > > WDYT? > > Don't do #2, ever. :-) The intent of having vm_create() versus is > __vm_create() > is so that tests that don't care about things like backing pages don't have to > pass in extra params. I very much want to keep that behavior, i.e. I don't > want > to extend vm_create() at all. IMO, adding _anything_ is a slippery slope, > e.g. > why are the backing types special enough to get a param, but thing XYZ isn't? > > Thinking more, the struct idea probably isn't going to work all that well. It > again puts the selftests into a state where it becomes difficult to control > one > setting and ignore the rest, e.g. the dirty_log_test and anything else with > extra > pages suddenly has to care about the backing type for page tables and code. > > Rather than adding a struct, what about extending the @mode param? We already > have vm_mem_backing_src_type, we just need a way to splice things together. > There > are a total of four things we can control: primary mode, and then code, data, > and > page tables backing types. > > So, turn @mode into a uint32_t and carve out 8 bits for each of those four > "modes". > The defaults Just Work because VM_MEM_SRC_ANONYMOUS==0. Hi Sean, How about merging both proposals and turn @mode into a struct and pass around a pointer to it? Then, when calling something that requires @mode, if @mode is NULL, the called function would use vm_arch_default_mode() to get a pointer to the arch-specific default mode struct. If a test needs to modify the parameters then it can construct a mode struct from scratch or start with a copy of the default. As long as all members of the struct representing parameters, such as backing type, have defaults mapped to zero for the struct members, then we shouldn't be adding any burden to users that don't care about other parameters (other than ensuring their @mode struct was zero initialized). Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
On Fri, Jul 22, 2022 at 02:53:20PM -0700, Ricardo Koller wrote: > > Which brings me to what to do with this test. Should it be fixed for > bare-metal by ignoring the PMOVSSET check? or should it actually check > for PMOVSSET=1 and fail on KVM until KVM gets fixed? > Hi Ricardo, Please write the test per the spec. Failures pointed out in kvm-unit-tests are great, when the tests are written correctly, since it means it's doing its job :-) If some CI somewhere starts blocking builds due to the failure, then there are ways to skip the test. Unfortunately the easiest way is usually the oversized hammer of skipping every unittests.cfg entry that fails. To do better, either the CI needs to be taught about all the subtest failures it can ignore or the test code needs some work to allow silencing known failures. For the test code, refactoring to isolate the test into it's own unittests.cfg entry and then skipping that entry is one way, but probably won't work in this case, since the overflow checks are scattered. Another way is to guard all the overflow checks with a variable which can be set with a command line parameter or environment variable. Eventually, when the KVM bug is fixed, the guard variable could be forced off for kernel versions >= the version the fix is merged. The kernel version can be detected in the unit test by looking at the KERNEL_* environment variables. Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4] KVM: selftests: Fix target thread to be migrated in rseq_test
On Tue, Jul 19, 2022 at 10:08:30AM +0800, Gavin Shan wrote:
> In rseq_test, there are two threads, which are vCPU thread and migration
> worker separately. Unfortunately, the test has the wrong PID passed to
> sched_setaffinity() in the migration worker. It forces migration on the
> migration worker because zeroed PID represents the calling thread, which
> is the migration worker itself. It means the vCPU thread is never enforced
> to migration and it can migrate at any time, which eventually leads to
> failure as the following logs show.
>
> host# uname -r
> 5.19.0-rc6-gavin+
> host# # cat /proc/cpuinfo | grep processor | tail -n 1
> processor: 223
> host# pwd
> /home/gavin/sandbox/linux.main/tools/testing/selftests/kvm
> host# for i in `seq 1 100`; do \
> echo "> $i"; ./rseq_test; done
> > 1
> > 2
> > 3
> > 4
> > 5
> > 6
> Test Assertion Failure
> rseq_test.c:265: rseq_cpu == cpu
> pid=3925 tid=3925 errno=4 - Interrupted system call
>1 0x00401963: main at rseq_test.c:265 (discriminator 2)
>2 0xb044affb: ?? ??:0
>3 0xb044b0c7: ?? ??:0
>4 0x00401a6f: _start at ??:?
> rseq CPU = 4, sched CPU = 27
>
> Fix the issue by passing correct parameter, TID of the vCPU thread, to
> sched_setaffinity() in the migration worker.
>
> Fixes: 61e52f1630f5 ("KVM: selftests: Add a test for KVM_RUN+rseq to detect
> task migration bugs")
> Suggested-by: Sean Christopherson
> Signed-off-by: Gavin Shan
> Reviewed-by: Oliver Upton
> ---
> v4: Pick the code change as Sean suggested.
> ---
> tools/testing/selftests/kvm/rseq_test.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/rseq_test.c
> b/tools/testing/selftests/kvm/rseq_test.c
> index 4158da0da2bb..2237d1aac801 100644
> --- a/tools/testing/selftests/kvm/rseq_test.c
> +++ b/tools/testing/selftests/kvm/rseq_test.c
> @@ -82,8 +82,9 @@ static int next_cpu(int cpu)
> return cpu;
> }
>
> -static void *migration_worker(void *ign)
> +static void *migration_worker(void *__rseq_tid)
> {
> + pid_t rseq_tid = (pid_t)(unsigned long)__rseq_tid;
> cpu_set_t allowed_mask;
> int r, i, cpu;
>
> @@ -106,7 +107,7 @@ static void *migration_worker(void *ign)
>* stable, i.e. while changing affinity is in-progress.
>*/
> smp_wmb();
> - r = sched_setaffinity(0, sizeof(allowed_mask), _mask);
> + r = sched_setaffinity(rseq_tid, sizeof(allowed_mask),
> _mask);
> TEST_ASSERT(!r, "sched_setaffinity failed, errno = %d (%s)",
> errno, strerror(errno));
> smp_wmb();
> @@ -231,7 +232,8 @@ int main(int argc, char *argv[])
> vm = vm_create_default(VCPU_ID, 0, guest_code);
> ucall_init(vm, NULL);
>
> - pthread_create(_thread, NULL, migration_worker, 0);
> + pthread_create(_thread, NULL, migration_worker,
> +(void *)(unsigned long)gettid());
>
> for (i = 0; !done; i++) {
> vcpu_run(vm, VCPU_ID);
> --
> 2.23.0
Thanks for figuring this out Gavin and Sean!
Reviewed-by: Andrew Jones
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
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 4/4] KVM: selftests: Fix filename reporting in guest asserts
On Wed, Jul 06, 2022 at 03:29:30PM +, Colton Lewis wrote:
> On Mon, Jun 20, 2022 at 02:04:43PM +0200, Paolo Bonzini wrote:
> > On 6/16/22 14:45, Andrew Jones wrote:
> > > > +#define __GUEST_ASSERT(_condition, _condstr, _nargs, _args...) do {
> > > > \
> > > > + if (!(_condition))
> > > > \
> > > > + ucall(UCALL_ABORT, GUEST_ASSERT_BUILTIN_NARGS +
> > > > _nargs, \
> > > > + "Failed guest assert: " _condstr,
> > > > \
> > > > + __FILE__,
> > > > \
> > > > + __LINE__,
> > > > \
> > > > + ##_args);
> > > > \
> > > We don't need another level of indentation nor the ## operator on _args.
> > >
> >
> > The ## is needed to drop the comma if there are no _args.
>
> I haven't heard anything more about part 4 of this patch in a while,
> so I'm checking in that I didn't miss something requiring action on my
> part.
Paolo set me straight on the ## usage, so besides the indentation nit and
the other comment I had about losing an assert string, then it looks fine
to me.
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH kvm-unit-tests] MAINTAINERS: Change drew's email address
As a side effect of leaving Red Hat I won't be able to use my Red Hat email address anymore. I'm also changing the name of my gitlab group. Signed-off-by: Andrew Jones Signed-off-by: Andrew Jones --- MAINTAINERS | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index bab08e740332..5e4c7bd70786 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -55,7 +55,7 @@ Maintainers --- M: Paolo Bonzini M: Thomas Huth -M: Andrew Jones +M: Andrew Jones S: Supported L: k...@vger.kernel.org T: https://gitlab.com/kvm-unit-tests/kvm-unit-tests.git @@ -64,14 +64,14 @@ Architecture Specific Code: --- ARM -M: Andrew Jones +M: Andrew Jones S: Supported L: k...@vger.kernel.org L: kvmarm@lists.cs.columbia.edu F: arm/ F: lib/arm/ F: lib/arm64/ -T: https://gitlab.com/rhdrjones/kvm-unit-tests.git +T: https://gitlab.com/drew-jones/kvm-unit-tests.git POWERPC M: Laurent Vivier -- 2.34.3 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 2/4] KVM: selftests: Increase UCALL_MAX_ARGS to 7
On Wed, Jun 15, 2022 at 07:31:14PM +, Colton Lewis wrote:
> Increase UCALL_MAX_ARGS to 7 to allow GUEST_ASSERT_4 to pass 3 builtin
> ucall arguments specified in guest_assert_builtin_args plus 4
> user-specified arguments.
>
> Signed-off-by: Colton Lewis
> ---
> tools/testing/selftests/kvm/include/ucall_common.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/kvm/include/ucall_common.h
> b/tools/testing/selftests/kvm/include/ucall_common.h
> index dbe872870b83..568c562f14cd 100644
> --- a/tools/testing/selftests/kvm/include/ucall_common.h
> +++ b/tools/testing/selftests/kvm/include/ucall_common.h
> @@ -16,7 +16,7 @@ enum {
> UCALL_UNHANDLED,
> };
>
> -#define UCALL_MAX_ARGS 6
> +#define UCALL_MAX_ARGS 7
>
> struct ucall {
> uint64_t cmd;
> --
> 2.36.1.476.g0c4daa206d-goog
>
Reviewed-by: Andrew Jones
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 2/4] KVM: selftests: Increase UCALL_MAX_ARGS to 7
On Mon, Jun 20, 2022 at 09:21:11AM +0200, Andrew Jones wrote: > On Sat, Jun 18, 2022 at 12:09:11AM +, Sean Christopherson wrote: > > On Fri, Jun 17, 2022, Colton Lewis wrote: > > > On Thu, Jun 16, 2022 at 02:10:06PM +0200, Andrew Jones wrote: > > > > We probably want to ensure all architectures are good with this. afaict, > > > > riscv only expects 6 args and uses UCALL_MAX_ARGS to cap the ucall > > > > inputs, > > > > for example. > > > > > > All architectures use UCALL_MAX_ARGS for that. Are you saying there > > > might be limitations beyond the value of the macro? If so, who should > > > verify whether this is ok? > > > > I thought there were architectural limitations too, but I believe I was > > thinking > > of vcpu_args_set(), where the number of params is limited by the function > > call > > ABI, e.g. the number of registers. > > > > Unless there's something really, really subtle going on, all architectures > > pass > > the actual ucall struct purely through memory. Actually, that code is ripe > > for > > deduplication, and amazingly it doesn't conflict with Colton's series. > > Patches > > incoming... > > > > RISC-V uses sbi_ecall() for their implementation of ucall(). CC'ing Anup > for confirmation, but if I understand the SBI spec correctly, then inputs > are limited to registers a0-a5. Ah, never mind... I see SBI is limited to 6 registers, but of course it only needs one register to pass the uc address... We can make UCALL_MAX_ARGS whatever we want. Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 0/3] KVM: selftests: Consolidate ucall code
On Sat, Jun 18, 2022 at 12:16:15AM +, Sean Christopherson wrote: > Consolidate the code for making and getting ucalls. All architectures pass > the ucall struct via memory, so filling and copying the struct is 100% > generic. The only per-arch code is sending and receiving the address of > said struct. > > Tested on x86 and arm, compile tested on s390 and RISC-V. For the series Reviewed-by: Andrew Jones Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 2/4] KVM: selftests: Increase UCALL_MAX_ARGS to 7
On Sat, Jun 18, 2022 at 12:09:11AM +, Sean Christopherson wrote: > On Fri, Jun 17, 2022, Colton Lewis wrote: > > On Thu, Jun 16, 2022 at 02:10:06PM +0200, Andrew Jones wrote: > > > We probably want to ensure all architectures are good with this. afaict, > > > riscv only expects 6 args and uses UCALL_MAX_ARGS to cap the ucall inputs, > > > for example. > > > > All architectures use UCALL_MAX_ARGS for that. Are you saying there > > might be limitations beyond the value of the macro? If so, who should > > verify whether this is ok? > > I thought there were architectural limitations too, but I believe I was > thinking > of vcpu_args_set(), where the number of params is limited by the function call > ABI, e.g. the number of registers. > > Unless there's something really, really subtle going on, all architectures > pass > the actual ucall struct purely through memory. Actually, that code is ripe > for > deduplication, and amazingly it doesn't conflict with Colton's series. > Patches > incoming... > RISC-V uses sbi_ecall() for their implementation of ucall(). CC'ing Anup for confirmation, but if I understand the SBI spec correctly, then inputs are limited to registers a0-a5. Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [Question] remote_tlb_flush statistic is missed from kvm_flush_remote_tlbs() ?
On Fri, Jun 17, 2022 at 11:19:28AM +0100, Marc Zyngier wrote:
> On Fri, 17 Jun 2022 11:02:44 +0100,
> Gavin Shan wrote:
> >
> > Hi Folks,
> >
> > We're reviewing upstream commits and found that it seems that
> > ++kvm->stat.generic.remote_tlb_flush has been missed from
> > kvm_flush_remote_tlbs(). If I'm correct, we still need to
> > increase the statistic in kvm_flush_remote_tlbs()?
> >
> > History about the changes:
> >
> > ce6a7007048b staging: r8188eu: remove {read,write}_macreg
> > The changes were NOT there any more.
> > 419025b3b419 Merge branch kvm-arm64/misc-5.15 into kvmarm-master/next
> > The changes were still there
> > 38f703663d4c KVM: arm64: Count VMID-wide TLB invalidations
> > The changes were initially introduced by this commit,
> > to increase 'kvm->stat.generic.remote_tlb_flush' in
> > kvm_flush_remote_tlbs().
>
> I'm not sure what you are asking. This change is definitely still
> present in the upstream kernel, and I don't get your point with the
> staging commit, which is totally unrelated.
>
> $ git describe --contains ce6a7007048b --match=v\*
> v5.15-rc1~154^2~11
> $ git describe --contains 419025b3b419 --match=v\*
> v5.15-rc1~65^2~4^2
> $ git describe --contains 38f703663d4c --match=v\*
> v5.15-rc1~65^2~4^2^2~13
>
> As you can see, the commit fixing the statistics was merged after
> staging one (it appears closer to -rc1, as there is 6 days between the
> two merge commits from Linus).
>
Hi Marc,
I don't see the change for commit 38f703663d4c as of an upstream pull
right now
$ git show 47700948a4ab:arch/arm64/kvm/mmu.c | grep -A4 'void
kvm_flush_remote_tlbs'
void kvm_flush_remote_tlbs(struct kvm *kvm)
{
++kvm->stat.generic.remote_tlb_flush_requests;
kvm_call_hyp(__kvm_tlb_flush_vmid, >arch.mmu);
}
and I do see it got dropped with merge commit e99314a340d2.
$ git diff 419025b3b419 0d0a19395baa -- arch/arm64/kvm/mmu.c | grep -A5 'void
kvm_flush_remote_tlbs'
void kvm_flush_remote_tlbs(struct kvm *kvm)
{
+ ++kvm->stat.generic.remote_tlb_flush_requests;
kvm_call_hyp(__kvm_tlb_flush_vmid, >arch.mmu);
- ++kvm->stat.generic.remote_tlb_flush;
}
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] selftests: KVM: Handle compiler optimizations in ucall
On Thu, Jun 16, 2022 at 09:54:16PM +, David Laight wrote: > From: oliver.up...@linux.dev > > Sent: 16 June 2022 19:45 > > > > > June 16, 2022 11:48 AM, "David Laight" wrote: > > > No wonder I was confused. > > > It's not surprising the compiler optimises it all away. > > > > > > It doesn't seem right to be 'abusing' WRITE_ONCE() here. > > > Just adding barrier() should be enough and much more descriptive. > > > > I had the same thought, although I do not believe barrier() is sufficient > > on its own. barrier_data() with a pointer to uc passed through > > is required to keep clang from eliminating the dead store. > > A barrier() (full memory clobber) ought to be stronger than > the partial one than barrier_data() generates. > > I can't quite decide whether you need a barrier() both sides > of the 'magic write'. > Plausibly the compiler could discard the on-stack data > after the barrier() and before the 'magic write'. > > Certainly putting the 'magic write' inside a asm block > that has a memory clobber is a more correct solution. Indeed, since the magic write is actually a guest MMIO write, then it should be using writeq(). Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] selftests: KVM: Handle compiler optimizations in ucall
On Thu, Jun 16, 2022 at 03:58:52PM +, David Laight wrote:
> From: Andrew Jones
> > Sent: 16 June 2022 13:03
> >
> > On Wed, Jun 15, 2022 at 06:57:06PM +, Raghavendra Rao Ananta wrote:
> > > The selftests, when built with newer versions of clang, is found
> > > to have over optimized guests' ucall() function, and eliminating
> > > the stores for uc.cmd (perhaps due to no immediate readers). This
> > > resulted in the userspace side always reading a value of '0', and
> > > causing multiple test failures.
> > >
> > > As a result, prevent the compiler from optimizing the stores in
> > > ucall() with WRITE_ONCE().
> > >
> > > Suggested-by: Ricardo Koller
> > > Suggested-by: Reiji Watanabe
> > > Signed-off-by: Raghavendra Rao Ananta
> > > ---
> > > tools/testing/selftests/kvm/lib/aarch64/ucall.c | 9 -
> > > 1 file changed, 4 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> > b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> > > index e0b0164e9af8..be1d9728c4ce 100644
> > > --- a/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> > > +++ b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> > > @@ -73,20 +73,19 @@ void ucall_uninit(struct kvm_vm *vm)
> > >
> > > void ucall(uint64_t cmd, int nargs, ...)
> > > {
> > > - struct ucall uc = {
> > > - .cmd = cmd,
> > > - };
> > > + struct ucall uc = {};
> > > va_list va;
> > > int i;
> > >
> > > + WRITE_ONCE(uc.cmd, cmd);
> > > nargs = nargs <= UCALL_MAX_ARGS ? nargs : UCALL_MAX_ARGS;
> > >
> > > va_start(va, nargs);
> > > for (i = 0; i < nargs; ++i)
> > > - uc.args[i] = va_arg(va, uint64_t);
> > > + WRITE_ONCE(uc.args[i], va_arg(va, uint64_t));
> > > va_end(va);
> > >
> > > - *ucall_exit_mmio_addr = (vm_vaddr_t)
> > > + WRITE_ONCE(*ucall_exit_mmio_addr, (vm_vaddr_t));
> > > }
>
> Am I misreading things again?
> That function looks like it writes the address of an on-stack
> item into global data.
The write to the address that the global points at causes a switch
from guest to host context. The guest's stack remains intact while
executing host code and the host can access the uc stack variable
directly by its address. Take a look at lib/aarch64/ucall.c to see
all the details.
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 1/2] KVM: selftests: kvm_vm_elf_load() and elfhdr_get() should close fd
Hi Paolo, Can you pick this old patch up? Thanks, drew On Wed, Feb 16, 2022 at 07:49:46PM -0800, Reiji Watanabe wrote: > kvm_vm_elf_load() and elfhdr_get() open one file each, but they > never close the opened file descriptor. If a test repeatedly > creates and destroys a VM with vm_create_with_vcpus(), which > (directly or indirectly) calls those two functions, the test > might end up getting a open failure with EMFILE. > Fix those two functions to close the file descriptor. > > Signed-off-by: Reiji Watanabe > --- > tools/testing/selftests/kvm/lib/elf.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tools/testing/selftests/kvm/lib/elf.c > b/tools/testing/selftests/kvm/lib/elf.c > index 13e8e3dcf984..9b23537a3caa 100644 > --- a/tools/testing/selftests/kvm/lib/elf.c > +++ b/tools/testing/selftests/kvm/lib/elf.c > @@ -91,6 +91,7 @@ static void elfhdr_get(const char *filename, Elf64_Ehdr > *hdrp) > " hdrp->e_shentsize: %x\n" > " expected: %zx", > hdrp->e_shentsize, sizeof(Elf64_Shdr)); > + close(fd); > } > > /* VM ELF Load > @@ -190,4 +191,5 @@ void kvm_vm_elf_load(struct kvm_vm *vm, const char > *filename) > phdr.p_filesz); > } > } > + close(fd); > } > -- > 2.35.1.473.g83b2b277ed-goog > Reviewed-by: Andrew Jones ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 1/4] KVM: selftests: enumerate GUEST_ASSERT arguments
On Wed, Jun 15, 2022 at 07:31:13PM +, Colton Lewis wrote:
> Enumerate GUEST_ASSERT arguments to avoid magic indices to ucall.args.
>
> Signed-off-by: Colton Lewis
> ---
> tools/testing/selftests/kvm/include/ucall_common.h | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/tools/testing/selftests/kvm/include/ucall_common.h
> b/tools/testing/selftests/kvm/include/ucall_common.h
> index 98562f685151..dbe872870b83 100644
> --- a/tools/testing/selftests/kvm/include/ucall_common.h
> +++ b/tools/testing/selftests/kvm/include/ucall_common.h
> @@ -32,6 +32,14 @@ uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall
> *uc);
> ucall(UCALL_SYNC, 6, "hello", stage, arg1,
> arg2, arg3, arg4)
> #define GUEST_SYNC(stage)ucall(UCALL_SYNC, 2, "hello", stage)
> #define GUEST_DONE() ucall(UCALL_DONE, 0)
> +
> +enum guest_assert_builtin_args {
> + GUEST_ERROR_STRING,
> + GUEST_FILE,
> + GUEST_LINE,
> + GUEST_ASSERT_BUILTIN_NARGS
> +};
> +
> #define __GUEST_ASSERT(_condition, _condstr, _nargs, _args...) do {\
> if (!(_condition)) \
> ucall(UCALL_ABORT, 2 + _nargs, \
> --
> 2.36.1.476.g0c4daa206d-goog
>
Reviewed-by: Andrew Jones
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 3/4] KVM: selftests: Write REPORT_GUEST_ASSERT macros to pair with GUEST_ASSERT
On Wed, Jun 15, 2022 at 07:31:15PM +, Colton Lewis wrote:
> Write REPORT_GUEST_ASSERT macros to pair with GUEST_ASSERT to abstract
> and make consistent all guest assertion reporting. Every report
> includes an explanatory string, a filename, and a line number.
>
> Signed-off-by: Colton Lewis
> ---
> .../selftests/kvm/include/ucall_common.h | 42 +++
> 1 file changed, 42 insertions(+)
>
> diff --git a/tools/testing/selftests/kvm/include/ucall_common.h
> b/tools/testing/selftests/kvm/include/ucall_common.h
> index 568c562f14cd..e8af3b4fef6d 100644
> --- a/tools/testing/selftests/kvm/include/ucall_common.h
> +++ b/tools/testing/selftests/kvm/include/ucall_common.h
> @@ -6,6 +6,7 @@
> */
> #ifndef SELFTEST_KVM_UCALL_COMMON_H
> #define SELFTEST_KVM_UCALL_COMMON_H
> +#include "test_util.h"
>
> /* Common ucalls */
> enum {
> @@ -64,4 +65,45 @@ enum guest_assert_builtin_args {
>
> #define GUEST_ASSERT_EQ(a, b) __GUEST_ASSERT((a) == (b), #a " == " #b, 2, a,
> b)
>
> +#define __REPORT_GUEST_ASSERT(_ucall, fmt, _args...) \
> + TEST_FAIL("%s at %s:%ld\n" fmt, \
> + (const char *)(_ucall).args[GUEST_ERROR_STRING], \
> + (const char *)(_ucall).args[GUEST_FILE], \
> + (_ucall).args[GUEST_LINE],\
> + ##_args)
> +
> +#define GUEST_ASSERT_ARG(ucall, i) ((ucall).args[GUEST_ASSERT_BUILTIN_NARGS
> + i])
> +
> +#define REPORT_GUEST_ASSERT(ucall) \
> + __REPORT_GUEST_ASSERT((ucall), "")
> +
> +#define REPORT_GUEST_ASSERT_1(ucall, fmt)\
> + __REPORT_GUEST_ASSERT((ucall), \
> + fmt, \
> + GUEST_ASSERT_ARG((ucall), 0))
> +
> +#define REPORT_GUEST_ASSERT_2(ucall, fmt)\
> + __REPORT_GUEST_ASSERT((ucall), \
> + fmt, \
> + GUEST_ASSERT_ARG((ucall), 0), \
> + GUEST_ASSERT_ARG((ucall), 1))
> +
> +#define REPORT_GUEST_ASSERT_3(ucall, fmt)\
> + __REPORT_GUEST_ASSERT((ucall), \
> + fmt, \
> + GUEST_ASSERT_ARG((ucall), 0), \
> + GUEST_ASSERT_ARG((ucall), 1), \
> + GUEST_ASSERT_ARG((ucall), 2))
> +
> +#define REPORT_GUEST_ASSERT_4(ucall, fmt)\
> + __REPORT_GUEST_ASSERT((ucall), \
> + fmt, \
> + GUEST_ASSERT_ARG((ucall), 0), \
> + GUEST_ASSERT_ARG((ucall), 1), \
> + GUEST_ASSERT_ARG((ucall), 2), \
> + GUEST_ASSERT_ARG((ucall), 3))
> +
> +#define REPORT_GUEST_ASSERT_N(ucall, fmt, args...) \
> + __REPORT_GUEST_ASSERT((ucall), fmt, ##args)
> +
> #endif /* SELFTEST_KVM_UCALL_COMMON_H */
> --
> 2.36.1.476.g0c4daa206d-goog
>
nit: All the ()'s around ucall when it's between ( and , are unnecessary.
Otherwise,
Reviewed-by: Andrew Jones
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 4/4] KVM: selftests: Fix filename reporting in guest asserts
On Wed, Jun 15, 2022 at 07:31:16PM +, Colton Lewis wrote:
> Fix filename reporting in guest asserts by ensuring the GUEST_ASSERT
> macro records __FILE__ and substituting REPORT_GUEST_ASSERT for many
> repetitive calls to TEST_FAIL.
>
> Previously filename was reported by using __FILE__ directly in the
> selftest, wrongly assuming it would always be the same as where the
> assertion failed.y
>
> Signed-off-by: Colton Lewis
> Reported-by: Ricardo Koller
> Fixes: 4e18bccc2e5544f0be28fc1c4e6be47a469d6c60
> ---
> tools/testing/selftests/kvm/aarch64/arch_timer.c | 12
> .../selftests/kvm/aarch64/debug-exceptions.c | 4 +---
> tools/testing/selftests/kvm/aarch64/vgic_irq.c | 4 +---
> tools/testing/selftests/kvm/include/ucall_common.h | 14 --
> tools/testing/selftests/kvm/memslot_perf_test.c| 4 +---
> tools/testing/selftests/kvm/steal_time.c | 3 +--
> .../selftests/kvm/system_counter_offset_test.c | 3 +--
> tools/testing/selftests/kvm/x86_64/amx_test.c | 3 +--
> tools/testing/selftests/kvm/x86_64/cpuid_test.c| 3 +--
> .../selftests/kvm/x86_64/cr4_cpuid_sync_test.c | 2 +-
> .../selftests/kvm/x86_64/emulator_error_test.c | 3 +--
> tools/testing/selftests/kvm/x86_64/evmcs_test.c| 3 +--
> tools/testing/selftests/kvm/x86_64/hyperv_clock.c | 3 +--
> .../testing/selftests/kvm/x86_64/hyperv_features.c | 6 ++
> .../testing/selftests/kvm/x86_64/hyperv_svm_test.c | 3 +--
> .../testing/selftests/kvm/x86_64/kvm_clock_test.c | 3 +--
> tools/testing/selftests/kvm/x86_64/kvm_pv_test.c | 3 +--
> .../testing/selftests/kvm/x86_64/set_boot_cpu_id.c | 4 +---
> tools/testing/selftests/kvm/x86_64/state_test.c| 3 +--
> .../selftests/kvm/x86_64/svm_int_ctl_test.c| 2 +-
> .../testing/selftests/kvm/x86_64/svm_vmcall_test.c | 2 +-
> tools/testing/selftests/kvm/x86_64/tsc_msrs_test.c | 4 +---
> .../selftests/kvm/x86_64/userspace_io_test.c | 4 +---
> .../selftests/kvm/x86_64/userspace_msr_exit_test.c | 5 ++---
> .../selftests/kvm/x86_64/vmx_apic_access_test.c| 3 +--
> .../kvm/x86_64/vmx_close_while_nested_test.c | 2 +-
> .../selftests/kvm/x86_64/vmx_dirty_log_test.c | 3 +--
> .../kvm/x86_64/vmx_invalid_nested_guest_state.c| 2 +-
> .../kvm/x86_64/vmx_nested_tsc_scaling_test.c | 2 +-
> .../kvm/x86_64/vmx_preemption_timer_test.c | 3 +--
> .../selftests/kvm/x86_64/vmx_tsc_adjust_test.c | 2 +-
> .../testing/selftests/kvm/x86_64/xen_shinfo_test.c | 2 +-
> .../testing/selftests/kvm/x86_64/xen_vmcall_test.c | 2 +-
> 33 files changed, 49 insertions(+), 72 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c
> b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> index f68019be67c0..9d7bda70c20a 100644
> --- a/tools/testing/selftests/kvm/aarch64/arch_timer.c
> +++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> @@ -231,10 +231,14 @@ static void *test_vcpu_run(void *arg)
> break;
> case UCALL_ABORT:
> sync_global_from_guest(vm, *shared_data);
> - TEST_FAIL("%s at %s:%ld\n\tvalues: %lu, %lu; %lu, vcpu: %u;
> stage: %u; iter: %u",
> - (const char *)uc.args[0], __FILE__, uc.args[1],
> - uc.args[2], uc.args[3], uc.args[4], vcpu_idx,
> - shared_data->guest_stage, shared_data->nr_iter);
> + REPORT_GUEST_ASSERT_N(uc,
> + "values: %lu, %lu; %lu, vcpu %u; stage;
> %u; iter: %u",
> + GUEST_ASSERT_ARG(uc, 0),
> + GUEST_ASSERT_ARG(uc, 1),
> + GUEST_ASSERT_ARG(uc, 2),
> + vcpu_idx,
> + shared_data->guest_stage,
> + shared_data->nr_iter);
> break;
> default:
> TEST_FAIL("Unexpected guest exit\n");
> diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> index b8072b40ccc8..2ee35cf9801e 100644
> --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> @@ -283,9 +283,7 @@ int main(int argc, char *argv[])
> stage, (ulong)uc.args[1]);
> break;
> case UCALL_ABORT:
> - TEST_FAIL("%s at %s:%ld\n\tvalues: %#lx, %#lx",
> - (const char *)uc.args[0],
> - __FILE__, uc.args[1], uc.args[2], uc.args[3]);
> + REPORT_GUEST_ASSERT_2(uc, "values: %#lx, %#lx");
> break;
> case UCALL_DONE:
> goto done;
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
>
Re: [PATCH 2/4] KVM: selftests: Increase UCALL_MAX_ARGS to 7
On Wed, Jun 15, 2022 at 07:31:14PM +, Colton Lewis wrote:
> Increase UCALL_MAX_ARGS to 7 to allow GUEST_ASSERT_4 to pass 3 builtin
> ucall arguments specified in guest_assert_builtin_args plus 4
> user-specified arguments.
>
> Signed-off-by: Colton Lewis
> ---
> tools/testing/selftests/kvm/include/ucall_common.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/kvm/include/ucall_common.h
> b/tools/testing/selftests/kvm/include/ucall_common.h
> index dbe872870b83..568c562f14cd 100644
> --- a/tools/testing/selftests/kvm/include/ucall_common.h
> +++ b/tools/testing/selftests/kvm/include/ucall_common.h
> @@ -16,7 +16,7 @@ enum {
> UCALL_UNHANDLED,
> };
>
> -#define UCALL_MAX_ARGS 6
> +#define UCALL_MAX_ARGS 7
>
> struct ucall {
> uint64_t cmd;
> --
> 2.36.1.476.g0c4daa206d-goog
>
We probably want to ensure all architectures are good with this. afaict,
riscv only expects 6 args and uses UCALL_MAX_ARGS to cap the ucall inputs,
for example.
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] selftests: KVM: Handle compiler optimizations in ucall
On Wed, Jun 15, 2022 at 06:57:06PM +, Raghavendra Rao Ananta wrote:
> The selftests, when built with newer versions of clang, is found
> to have over optimized guests' ucall() function, and eliminating
> the stores for uc.cmd (perhaps due to no immediate readers). This
> resulted in the userspace side always reading a value of '0', and
> causing multiple test failures.
>
> As a result, prevent the compiler from optimizing the stores in
> ucall() with WRITE_ONCE().
>
> Suggested-by: Ricardo Koller
> Suggested-by: Reiji Watanabe
> Signed-off-by: Raghavendra Rao Ananta
> ---
> tools/testing/selftests/kvm/lib/aarch64/ucall.c | 9 -
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> index e0b0164e9af8..be1d9728c4ce 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> @@ -73,20 +73,19 @@ void ucall_uninit(struct kvm_vm *vm)
>
> void ucall(uint64_t cmd, int nargs, ...)
> {
> - struct ucall uc = {
> - .cmd = cmd,
> - };
> + struct ucall uc = {};
> va_list va;
> int i;
>
> + WRITE_ONCE(uc.cmd, cmd);
> nargs = nargs <= UCALL_MAX_ARGS ? nargs : UCALL_MAX_ARGS;
>
> va_start(va, nargs);
> for (i = 0; i < nargs; ++i)
> - uc.args[i] = va_arg(va, uint64_t);
> + WRITE_ONCE(uc.args[i], va_arg(va, uint64_t));
> va_end(va);
>
> - *ucall_exit_mmio_addr = (vm_vaddr_t)
> + WRITE_ONCE(*ucall_exit_mmio_addr, (vm_vaddr_t));
> }
>
> uint64_t get_ucall(struct kvm_vm *vm, uint32_t vcpu_id, struct ucall *uc)
> --
> 2.36.1.476.g0c4daa206d-goog
>
Reviewed-by: Andrew Jones
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH] arm/run: Use TCG with qemu-system-arm on arm64 systems
On Thu, Mar 17, 2022 at 06:03:07PM +, Alexandru Elisei wrote:
> Hi,
>
> On Thu, Mar 17, 2022 at 06:45:07PM +0100, Andrew Jones wrote:
> > On Thu, Mar 17, 2022 at 04:56:01PM +, Alexandru Elisei wrote:
> > > From: Andrew Jones
> > >
> > > If the user sets QEMU=qemu-system-arm on arm64 systems, the tests can only
> > > be run by using the TCG accelerator. In this case use TCG instead of KVM.
> > >
> > > Signed-off-by: Andrew Jones
> > > [ Alex E: Added commit message ]
> > > Signed-off-by: Alexandru Elisei
> > > ---
> > > arm/run | 12 ++--
> > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arm/run b/arm/run
> > > index 28a0b4ad2729..128489125dcb 100755
> > > --- a/arm/run
> > > +++ b/arm/run
> > > @@ -10,16 +10,24 @@ if [ -z "$KUT_STANDALONE" ]; then
> > > fi
> > > processor="$PROCESSOR"
> > >
> > > -ACCEL=$(get_qemu_accelerator) ||
> > > +accel=$(get_qemu_accelerator) ||
> > > exit $?
> > >
> > > -if [ "$ACCEL" = "kvm" ]; then
> > > +if [ "$accel" = "kvm" ]; then
> > > QEMU_ARCH=$HOST
> > > fi
> > >
> > > qemu=$(search_qemu_binary) ||
> > > exit $?
> > >
> > > +if [ "$QEMU" ] && [ -z "$ACCEL" ] &&
> > > + [ "$HOST" = "aarch64" ] && [ "$ARCH" = "arm" ] &&
> > > + [ "$(basename $QEMU)" = "qemu-system-arm" ]; then
> > > + accel=tcg
> > > +fi
> > > +
> > > +ACCEL=$accel
> > > +
> > > if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null;
> > > then
> > > echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting."
> > > exit 2
> > > --
> > > 2.35.1
> > >
> >
> > Ha, OK, I guess you posting this is a strong vote in favor of this
> > behavior. I've queued it
>
> Ah, yes, maybe I should've been more clear about it. I think this is more
> intuitive for the new users who might not be very familiar with
> run_tests.sh internals, and like you've said it won't break existing users
> who had to set ACCEL=tcg to get the desired behaviour anyway.
>
> Thanks you for queueing it so fast! Should probably have also mentioned
> this as a comment in the commit, but I take full responsibility for
> breaking stuff.
>
> Alex
>
> >
> > https://gitlab.com/rhdrjones/kvm-unit-tests/-/commits/arm/queue
I finally merged this.
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH] arm64: Check for dynamic relocs with readelf
On Wed, May 04, 2022 at 04:04:46PM -0700, Bill Wendling wrote: > Clang's version of objdump doesn't recognize "setftest.elf" as a dynamic > object and produces an error stating such. > > $ llvm-objdump -R ./arm/selftest.elf > arm/selftest.elf: file format elf64-littleaarch64 > llvm-objdump: error: './arm/selftest.elf': not a dynamic object > > This causes the ARM64 "arch_elf_check" check to fail. Using "readelf > -rW" is a better option way to get the same information and produces the > same information in both binutils and LLVM. > > Signed-off-by: Bill Wendling > --- > arm/Makefile.arm64 | 6 +++--- > configure | 2 ++ > 2 files changed, 5 insertions(+), 3 deletions(-) Merged to https://gitlab.com/kvm-unit-tests/kvm-unit-tests.git master Thanks, drew > > diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64 > index 6feac76f895f..42e18e771b3b 100644 > --- a/arm/Makefile.arm64 > +++ b/arm/Makefile.arm64 > @@ -14,9 +14,9 @@ mno_outline_atomics := $(call cc-option, > -mno-outline-atomics, "") > CFLAGS += $(mno_outline_atomics) > > define arch_elf_check = > - $(if $(shell ! $(OBJDUMP) -R $(1) >&/dev/null && echo "nok"), > - $(error $(shell $(OBJDUMP) -R $(1) 2>&1))) > - $(if $(shell $(OBJDUMP) -R $(1) | grep R_ | grep -v R_AARCH64_RELATIVE), > + $(if $(shell ! $(READELF) -rW $(1) >&/dev/null && echo "nok"), > + $(error $(shell $(READELF) -rW $(1) 2>&1))) > + $(if $(shell $(READELF) -rW $(1) | grep R_ | grep -v > R_AARCH64_RELATIVE), > $(error $(1) has unsupported reloc types)) > endef > > diff --git a/configure b/configure > index 86c3095a245a..23085da7dcc5 100755 > --- a/configure > +++ b/configure > @@ -12,6 +12,7 @@ cflags= > ld=ld > objcopy=objcopy > objdump=objdump > +readelf=readelf > ar=ar > addr2line=addr2line > arch=$(uname -m | sed -e > 's/i.86/i386/;s/arm64/aarch64/;s/arm.*/arm/;s/ppc64.*/ppc64/') > @@ -372,6 +373,7 @@ CFLAGS=$cflags > LD=$cross_prefix$ld > OBJCOPY=$cross_prefix$objcopy > OBJDUMP=$cross_prefix$objdump > +READELF=$cross_prefix$readelf > AR=$cross_prefix$ar > ADDR2LINE=$cross_prefix$addr2line > TEST_DIR=$testdir > -- > 2.36.0.464.gb9c8b46e94-goog > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH] libfdt: use logical "or" instead of bitwise "or" with boolean operands
On Fri, Mar 18, 2022 at 10:36:01AM +0100, Andrew Jones wrote:
> On Tue, Mar 15, 2022 at 11:02:14PM -0700, Bill Wendling wrote:
> > Clang warns about using a bitwise '|' with boolean operands. This seems
> > to be due to a small typo.
> >
> > lib/libfdt/fdt_rw.c:438:6: warning: use of bitwise '|' with boolean
> > operands [-Werror,-Wbitwise-instead-of-logical]
> > if (can_assume(LIBFDT_ORDER) |
> >
> > Using '||' removes this warnings.
> >
> > Signed-off-by: Bill Wendling
> > ---
> > lib/libfdt/fdt_rw.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/libfdt/fdt_rw.c b/lib/libfdt/fdt_rw.c
> > index 13854253ff86..3320e5559cac 100644
> > --- a/lib/libfdt/fdt_rw.c
> > +++ b/lib/libfdt/fdt_rw.c
> > @@ -435,7 +435,7 @@ int fdt_open_into(const void *fdt, void *buf, int
> > bufsize)
> > return struct_size;
> > }
> >
> > - if (can_assume(LIBFDT_ORDER) |
> > + if (can_assume(LIBFDT_ORDER) ||
> > !fdt_blocks_misordered_(fdt, mem_rsv_size, struct_size)) {
> > /* no further work necessary */
> > err = fdt_move(fdt, buf, bufsize);
> > --
> > 2.35.1.723.g4982287a31-goog
> >
>
> We're not getting as much interest in the submodule discussion as I hoped.
> I see one vote against on this thread and one vote for on a different
> thread[1]. For now I'll just commit a big rebase patch for libfdt. We can
> revisit it again after we decide what to do for QCBOR.
>
Now merged through misc/queue.
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH] libfdt: use logical "or" instead of bitwise "or" with boolean operands
On Tue, Mar 15, 2022 at 11:02:14PM -0700, Bill Wendling wrote:
> Clang warns about using a bitwise '|' with boolean operands. This seems
> to be due to a small typo.
>
> lib/libfdt/fdt_rw.c:438:6: warning: use of bitwise '|' with boolean
> operands [-Werror,-Wbitwise-instead-of-logical]
> if (can_assume(LIBFDT_ORDER) |
>
> Using '||' removes this warnings.
>
> Signed-off-by: Bill Wendling
> ---
> lib/libfdt/fdt_rw.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/libfdt/fdt_rw.c b/lib/libfdt/fdt_rw.c
> index 13854253ff86..3320e5559cac 100644
> --- a/lib/libfdt/fdt_rw.c
> +++ b/lib/libfdt/fdt_rw.c
> @@ -435,7 +435,7 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
> return struct_size;
> }
>
> - if (can_assume(LIBFDT_ORDER) |
> + if (can_assume(LIBFDT_ORDER) ||
> !fdt_blocks_misordered_(fdt, mem_rsv_size, struct_size)) {
> /* no further work necessary */
> err = fdt_move(fdt, buf, bufsize);
> --
> 2.35.1.723.g4982287a31-goog
>
We're not getting as much interest in the submodule discussion as I hoped.
I see one vote against on this thread and one vote for on a different
thread[1]. For now I'll just commit a big rebase patch for libfdt. We can
revisit it again after we decide what to do for QCBOR.
Thanks,
drew
[1]
https://lore.kernel.org/kvm/20220316105109.oi5g532ylijzldte@gator/T/#m48c47c761f3b3a4da636482b6385c59d4a990137
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH] arm/run: Use TCG with qemu-system-arm on arm64 systems
On Thu, Mar 17, 2022 at 04:56:01PM +, Alexandru Elisei wrote:
> From: Andrew Jones
>
> If the user sets QEMU=qemu-system-arm on arm64 systems, the tests can only
> be run by using the TCG accelerator. In this case use TCG instead of KVM.
>
> Signed-off-by: Andrew Jones
> [ Alex E: Added commit message ]
> Signed-off-by: Alexandru Elisei
> ---
> arm/run | 12 ++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/arm/run b/arm/run
> index 28a0b4ad2729..128489125dcb 100755
> --- a/arm/run
> +++ b/arm/run
> @@ -10,16 +10,24 @@ if [ -z "$KUT_STANDALONE" ]; then
> fi
> processor="$PROCESSOR"
>
> -ACCEL=$(get_qemu_accelerator) ||
> +accel=$(get_qemu_accelerator) ||
> exit $?
>
> -if [ "$ACCEL" = "kvm" ]; then
> +if [ "$accel" = "kvm" ]; then
> QEMU_ARCH=$HOST
> fi
>
> qemu=$(search_qemu_binary) ||
> exit $?
>
> +if [ "$QEMU" ] && [ -z "$ACCEL" ] &&
> + [ "$HOST" = "aarch64" ] && [ "$ARCH" = "arm" ] &&
> + [ "$(basename $QEMU)" = "qemu-system-arm" ]; then
> + accel=tcg
> +fi
> +
> +ACCEL=$accel
> +
> if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then
> echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting."
> exit 2
> --
> 2.35.1
>
Ha, OK, I guess you posting this is a strong vote in favor of this
behavior. I've queued it
https://gitlab.com/rhdrjones/kvm-unit-tests/-/commits/arm/queue
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH] KVM: selftests: get-reg-list: Add KVM_REG_ARM_FW_REG(3)
When testing a kernel with commit a5905d6af492 ("KVM: arm64:
Allow SMCCC_ARCH_WORKAROUND_3 to be discovered and migrated")
get-reg-list output
vregs: Number blessed registers: 234
vregs: Number registers: 238
vregs: There are 1 new registers.
Consider adding them to the blessed reg list with the following lines:
KVM_REG_ARM_FW_REG(3),
vregs: PASS
...
That output inspired two changes: 1) add the new register to the
blessed list and 2) explain why "Number registers" is actually four
larger than "Number blessed registers" (on the system used for
testing), even though only one register is being stated as new.
The reason is that some registers are host dependent and they get
filtered out when comparing with the blessed list. The system
used for the test apparently had three filtered registers.
Signed-off-by: Andrew Jones
---
tools/testing/selftests/kvm/aarch64/get-reg-list.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
index f769fc6cd927..7efe918ded9e 100644
--- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
+++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
@@ -503,8 +503,13 @@ static void run_test(struct vcpu_config *c)
++missing_regs;
if (new_regs || missing_regs) {
+ n = 0;
+ for_each_reg_filtered(i)
+ ++n;
+
printf("%s: Number blessed registers: %5lld\n", config_name(c),
blessed_n);
- printf("%s: Number registers: %5lld\n", config_name(c),
reg_list->n);
+ printf("%s: Number registers: %5lld (includes %lld
filtered registers)\n",
+ config_name(c), reg_list->n, reg_list->n - n);
}
if (new_regs) {
@@ -683,9 +688,10 @@ static __u64 base_regs[] = {
KVM_REG_ARM64 | KVM_REG_SIZE_U64 | KVM_REG_ARM_CORE |
KVM_REG_ARM_CORE_REG(spsr[4]),
KVM_REG_ARM64 | KVM_REG_SIZE_U32 | KVM_REG_ARM_CORE |
KVM_REG_ARM_CORE_REG(fp_regs.fpsr),
KVM_REG_ARM64 | KVM_REG_SIZE_U32 | KVM_REG_ARM_CORE |
KVM_REG_ARM_CORE_REG(fp_regs.fpcr),
- KVM_REG_ARM_FW_REG(0),
- KVM_REG_ARM_FW_REG(1),
- KVM_REG_ARM_FW_REG(2),
+ KVM_REG_ARM_FW_REG(0), /* KVM_REG_ARM_PSCI_VERSION */
+ KVM_REG_ARM_FW_REG(1), /* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1
*/
+ KVM_REG_ARM_FW_REG(2), /* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2
*/
+ KVM_REG_ARM_FW_REG(3), /* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_3
*/
ARM64_SYS_REG(3, 3, 14, 3, 1), /* CNTV_CTL_EL0 */
ARM64_SYS_REG(3, 3, 14, 3, 2), /* CNTV_CVAL_EL0 */
ARM64_SYS_REG(3, 3, 14, 0, 2),
--
2.34.1
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests] Adding the QCBOR library to kvm-unit-tests
On Wed, Mar 16, 2022 at 10:42:31AM +, Alexandru Elisei wrote: > Hi, > > On Tue, Mar 15, 2022 at 04:25:28PM +0100, Andrew Jones wrote: > > On Tue, Mar 15, 2022 at 01:33:57PM +, Alexandru Elisei wrote: > > > Hi, > > > > > > Arm is planning to upstream tests that are being developed as part of the > > > Confidential Compute Architecture [1]. Some of the tests target the > > > attestation part of creating and managing a confidential compute VM, which > > > requires the manipulation of messages in the Concise Binary Object > > > Representation (CBOR) format [2]. > > > > > > I would like to ask if it would be acceptable from a license perspective > > > to > > > include the QCBOR library [3] into kvm-unit-tests, which will be used for > > > encoding and decoding of CBOR messages. > > > > > > The library is licensed under the 3-Clause BSD license, which is > > > compatible > > > with GPLv2 [4]. Some of the files that were created inside Qualcomm before > > > the library was open-sourced have a slightly modified 3-Clause BSD > > > license, > > > where a NON-INFRINGMENT clause is added to the disclaimer: > > > > > > "THIS SOFTWARE IS PROVIDED "AS IS" AND ANY EXPRESS OR IMPLIED > > > WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF > > > MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE **AND NON-INFRINGEMENT** > > > ARE DISCLAIMED" (emphasis by me on the added clause). > > > > > > The files in question include the core files that implement the > > > encode/decode functionality, and thus would have to be included in > > > kvm-unit-tests. I believe that the above modification does not affect the > > > compatibility with GPLv2. > > > > > > I would also like to mention that the QCBOR library is also used in > > > Trusted > > > Firmware-M [5], which is licensed under BSD 3-Clause. > > > > > > [1] > > > https://www.arm.com/architecture/security-features/arm-confidential-compute-architecture > > > [2] https://datatracker.ietf.org/doc/html/rfc8949 > > > [3] https://github.com/laurencelundblade/QCBOR > > > [4] https://www.gnu.org/licenses/license-list.html#GPLCompatibleLicenses > > > [5] > > > https://git.trustedfirmware.org/TF-M/trusted-firmware-m.git/tree/lib/ext/qcbor > > > > > > Thanks, > > > Alex > > > > > > > Assuming the license is OK (I'm not educated in that stuff enough to give > > an opinion), then the next question is how do we want to integrate it? > > Bring it all in, like we did libfdt? Or use a git submodule? > > This is still a work in progress and at this time I'm not sure how it will > end up looking. Do you have a preference for one or the other? > I think I prefer a submodule, but I'm all ears on this. By coincidence the same topic is now also being discussed here https://lore.kernel.org/kvm/334cc93e-9843-daa9-5846-394c199e2...@redhat.com/T/#mb5db3e9143e4f2ca47d24a32b30e7b2f014934e8 Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH] libfdt: use logical "or" instead of bitwise "or" with boolean operands
On Tue, Mar 15, 2022 at 11:02:14PM -0700, Bill Wendling wrote:
> Clang warns about using a bitwise '|' with boolean operands. This seems
> to be due to a small typo.
>
> lib/libfdt/fdt_rw.c:438:6: warning: use of bitwise '|' with boolean
> operands [-Werror,-Wbitwise-instead-of-logical]
> if (can_assume(LIBFDT_ORDER) |
>
> Using '||' removes this warnings.
>
> Signed-off-by: Bill Wendling
> ---
> lib/libfdt/fdt_rw.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/libfdt/fdt_rw.c b/lib/libfdt/fdt_rw.c
> index 13854253ff86..3320e5559cac 100644
> --- a/lib/libfdt/fdt_rw.c
> +++ b/lib/libfdt/fdt_rw.c
> @@ -435,7 +435,7 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
> return struct_size;
> }
>
> - if (can_assume(LIBFDT_ORDER) |
> + if (can_assume(LIBFDT_ORDER) ||
> !fdt_blocks_misordered_(fdt, mem_rsv_size, struct_size)) {
> /* no further work necessary */
> err = fdt_move(fdt, buf, bufsize);
> --
> 2.35.1.723.g4982287a31-goog
>
This is fixed in libfdt upstream with commit 7be250b4 ("libfdt:
Correct condition for reordering blocks"), which is in v1.6.1.
We can either take this patch as a backport of 7be250b4 or we
can rebase all of our libfdt to v1.6.1. Based on the number of
fixes in v1.6.1, which appear to be mostly for compiling with
later compilers, I'm in favor of rebasing.
Actually, we can also use this opportunity to [re]visit the
idea of changing libfdt to a git submodule. I'd like to hear
opinions on that.
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH kvm-unit-tests] arch-run: Introduce QEMU_ARCH
On Tue, Mar 15, 2022 at 04:31:34PM +, Alexandru Elisei wrote:
> Well, kvm-unit-tests selects KVM or TCG under the hood without the user
> being involved at all.
The under the hood aspect isn't great. It's best for testers to know what
they're testing. It's pretty obvious, though, that if you choose
ARCH != HOST that you'll end up on TCG. And, since KVM has historically
been the primary focus of kvm-unit-tests, then it's probably reasonable
to assume KVM is used when ARCH == HOST. However, we still silently fall
back to TCG, even when ARCH == HOST, if /dev/kvm isn't available! And,
the whole AArch32 guest support on AArch64 hosts with KVM requiring a
different qemu binary muddies things further...
Anyway, I hope serious test runners always specify ACCEL and QEMU to
whatever they plan to test.
> In my opinion, it's slightly better from an
> usability perspective for kvm-unit-tests to do its best to run the tests
> based on what the user specifically set (QEMU=qemu-system-arm) than fail to
> run the tests because of an internal heuristic of which the user might be
> entirely ignorant (if arm64 and /dev/kvm is available, pick ACCEL=kvm).
If you'd like to post a patch for it, then I'd prefer something like
below, which spells out the condition that the override is applied
and also allows $QEMU to be checked by search_qemu_binary() before
using it to make decisions.
Thanks,
drew
diff --git a/arm/run b/arm/run
index 28a0b4ad2729..128489125dcb 100755
--- a/arm/run
+++ b/arm/run
@@ -10,16 +10,24 @@ if [ -z "$KUT_STANDALONE" ]; then
fi
processor="$PROCESSOR"
-ACCEL=$(get_qemu_accelerator) ||
+accel=$(get_qemu_accelerator) ||
exit $?
-if [ "$ACCEL" = "kvm" ]; then
+if [ "$accel" = "kvm" ]; then
QEMU_ARCH=$HOST
fi
qemu=$(search_qemu_binary) ||
exit $?
+if [ "$QEMU" ] && [ -z "$ACCEL" ] &&
+ [ "$HOST" = "aarch64" ] && [ "$ARCH" = "arm" ] &&
+ [ "$(basename $QEMU)" = "qemu-system-arm" ]; then
+ accel=tcg
+fi
+
+ACCEL=$accel
+
if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then
echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting."
exit 2
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests] Adding the QCBOR library to kvm-unit-tests
On Tue, Mar 15, 2022 at 01:33:57PM +, Alexandru Elisei wrote: > Hi, > > Arm is planning to upstream tests that are being developed as part of the > Confidential Compute Architecture [1]. Some of the tests target the > attestation part of creating and managing a confidential compute VM, which > requires the manipulation of messages in the Concise Binary Object > Representation (CBOR) format [2]. > > I would like to ask if it would be acceptable from a license perspective to > include the QCBOR library [3] into kvm-unit-tests, which will be used for > encoding and decoding of CBOR messages. > > The library is licensed under the 3-Clause BSD license, which is compatible > with GPLv2 [4]. Some of the files that were created inside Qualcomm before > the library was open-sourced have a slightly modified 3-Clause BSD license, > where a NON-INFRINGMENT clause is added to the disclaimer: > > "THIS SOFTWARE IS PROVIDED "AS IS" AND ANY EXPRESS OR IMPLIED > WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF > MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE **AND NON-INFRINGEMENT** > ARE DISCLAIMED" (emphasis by me on the added clause). > > The files in question include the core files that implement the > encode/decode functionality, and thus would have to be included in > kvm-unit-tests. I believe that the above modification does not affect the > compatibility with GPLv2. > > I would also like to mention that the QCBOR library is also used in Trusted > Firmware-M [5], which is licensed under BSD 3-Clause. > > [1] > https://www.arm.com/architecture/security-features/arm-confidential-compute-architecture > [2] https://datatracker.ietf.org/doc/html/rfc8949 > [3] https://github.com/laurencelundblade/QCBOR > [4] https://www.gnu.org/licenses/license-list.html#GPLCompatibleLicenses > [5] > https://git.trustedfirmware.org/TF-M/trusted-firmware-m.git/tree/lib/ext/qcbor > > Thanks, > Alex > Assuming the license is OK (I'm not educated in that stuff enough to give an opinion), then the next question is how do we want to integrate it? Bring it all in, like we did libfdt? Or use a git submodule? Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH kvm-unit-tests] arch-run: Introduce QEMU_ARCH
On Tue, Mar 15, 2022 at 12:33:17PM +, Alexandru Elisei wrote:
> Hi,
>
> On Tue, Mar 15, 2022 at 09:01:52AM +0100, Andrew Jones wrote:
> > Add QEMU_ARCH, which allows run scripts to specify which architecture
> > of QEMU should be used. This is useful on AArch64 when running with
> > KVM and running AArch32 tests. For those tests, we *don't* want to
> > select the 'arm' QEMU, as would have been selected, but rather the
> > $HOST ('aarch64') QEMU.
> >
> > To use this new variable, simply ensure it's set prior to calling
> > search_qemu_binary().
>
> Looks good, tested on an arm64 machine, with ACCEL set to tcg -
> run_tests.sh selects qemu-system-arm; ACCEL unset - run_tests.sh selects
> ACCEL=kvm and qemu-system-aarch64; also tested on an x86 machine -
> run_tests.sh selects ACCEL=tcg and qemu-system-arm:
>
> Tested-by: Alexandru Elisei
> Reviewed-by: Alexandru Elisei
>
> One thing I noticed is that if the user sets QEMU=qemu-system-arm on an arm64
> machine, run_tests.sh still selects ACCEL=kvm which leads to the following
> failure:
>
> SKIP selftest-setup (qemu-system-arm: -accel kvm: invalid accelerator kvm)
>
> I'm not sure if this deserves a fix, if the user set the QEMU variable I
> believe it is probable that the user is also aware of the ACCEL variable
> and the error message does a good job explaining what is wrong.
Yes, we assume the user selected the wrong qemu, rather than assuming the
user didn't expect KVM to be enabled. If we're wrong, then the error
message should hopefully imply to the user that they need to do
QEMU=qemu-system-arm ACCEL=tcg ...
> Just in
> case, this is what I did to make kvm-unit-tests pick the right accelerator
> (copied-and-pasted the find_word function from scripts/runtime.bash):
>
> diff --git a/arm/run b/arm/run
> index 94adcddb7399..b0c9613b8d28 100755
> --- a/arm/run
> +++ b/arm/run
> @@ -10,6 +15,10 @@ if [ -z "$KUT_STANDALONE" ]; then
> fi
> processor="$PROCESSOR"
>
> +if [ -z $ACCEL ] && [ "$HOST" = "aarch64" ] && ! find_word "qemu-system-arm"
> "$QEMU"; then
Instead of find_word,
[ "$QEMU" ] && [ "$(basename $QEMU)" = "qemu-system-arm" ]
> + ACCEL=tcg
> +fi
> +
When ACCEL is unset, we currently set it to kvm when we have /dev/kvm and
$HOST == $ARCH_NAME or ($HOST == aarch64 && $ARCH == arm) and tcg
otherwise. Adding logic like the above would allow overriding the
"set to kvm" logic when $QEMU == qemu-system-arm. That makes sense to
me, but we trade one assumption for another. We would now assume that
$QEMU is correct and the user wants to run with TCG, rather than that
$QEMU is wrong and the user wanted to run with KVM.
I think I'd prefer not adding the special case override. I think it's
more likely the user expects to run with KVM when running on an AArch64
host and that they mistakenly selected the wrong qemu, than that they
wanted TCG with qemu-system-arm. We also avoid a few more lines of code
and a change in behavior by maintaining the old assumption.
I've pushed this to arm/queue -- and MR coming up.
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH kvm-unit-tests] arch-run: Introduce QEMU_ARCH
Add QEMU_ARCH, which allows run scripts to specify which architecture
of QEMU should be used. This is useful on AArch64 when running with
KVM and running AArch32 tests. For those tests, we *don't* want to
select the 'arm' QEMU, as would have been selected, but rather the
$HOST ('aarch64') QEMU.
To use this new variable, simply ensure it's set prior to calling
search_qemu_binary().
Cc: Alexandru Elisei
Signed-off-by: Andrew Jones
---
arm/run | 4
scripts/arch-run.bash | 4 +++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/arm/run b/arm/run
index 0629b69a117c..28a0b4ad2729 100755
--- a/arm/run
+++ b/arm/run
@@ -13,6 +13,10 @@ processor="$PROCESSOR"
ACCEL=$(get_qemu_accelerator) ||
exit $?
+if [ "$ACCEL" = "kvm" ]; then
+ QEMU_ARCH=$HOST
+fi
+
qemu=$(search_qemu_binary) ||
exit $?
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index aae552321f9b..0dfaf017db0a 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -176,8 +176,10 @@ search_qemu_binary ()
local save_path=$PATH
local qemucmd qemu
+ : "${QEMU_ARCH:=$ARCH_NAME}"
+
export PATH=$PATH:/usr/libexec
- for qemucmd in ${QEMU:-qemu-system-$ARCH_NAME qemu-kvm}; do
+ for qemucmd in ${QEMU:-qemu-system-$QEMU_ARCH qemu-kvm}; do
if $qemucmd --help 2>/dev/null | grep -q 'QEMU'; then
qemu="$qemucmd"
break
--
2.34.1
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH v2 0/3] configure changes and rename --target-efi
On Wed, Feb 23, 2022 at 12:55:34PM +, Alexandru Elisei wrote:
> The first two patches are fixes for stuff I found while working on patch
> #3.
>
> Patch #3 ("Rename --target-efi to --[enable|disable]-efi") is where the
> configure option --target-efi gets renamed.
>
> Changes in v2:
>
> * Dropped what was patch #3, which made arm/arm64 configure option
> --target available to all architectures.
>
> * Renamed --target-efi to --[enable|disable]-efi instead of --efi-payload.
>
> Alexandru Elisei (3):
> configure: Fix whitespaces for the --gen-se-header help text
> configure: Restrict --target-efi to x86_64
> Rename --target-efi to --[enable|disable]-efi
>
> Makefile | 10 +++---
> configure| 22 +++---
> lib/x86/acpi.c | 4 ++--
> lib/x86/amd_sev.h| 4 ++--
> lib/x86/asm/page.h | 8
> lib/x86/asm/setup.h | 4 ++--
> lib/x86/setup.c | 4 ++--
> lib/x86/vm.c | 12 ++--
> scripts/runtime.bash | 4 ++--
> x86/Makefile.common | 6 +++---
> x86/Makefile.x86_64 | 6 +++---
> x86/access_test.c| 2 +-
> x86/efi/README.md| 2 +-
> x86/efi/run | 2 +-
> x86/run | 4 ++--
> 15 files changed, 49 insertions(+), 45 deletions(-)
>
> --
> 2.35.1
>
Applied to misc/queue and MR posted:
https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/merge_requests/26
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH] arm: Fix typos
On Fri, Mar 11, 2022 at 06:08:50PM +0100, Thomas Huth wrote:
> Correct typos which were discovered with the "codespell" utility.
>
> Signed-off-by: Thomas Huth
> ---
> arm/cstart.S | 2 +-
> arm/gic.c | 2 +-
> arm/micro-bench.c | 2 +-
> lib/arm64/asm/assembler.h | 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arm/cstart.S b/arm/cstart.S
> index 2401d92..7036e67 100644
> --- a/arm/cstart.S
> +++ b/arm/cstart.S
> @@ -44,7 +44,7 @@ start:
>
> /*
>* set stack, making room at top of stack for cpu0's
> - * exception stacks. Must start wtih stackptr, not
> + * exception stacks. Must start with stackptr, not
>* stacktop, so the thread size masking (shifts) work.
>*/
> ldr sp, =stackptr
> diff --git a/arm/gic.c b/arm/gic.c
> index 1e3ea80..60457e2 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -909,7 +909,7 @@ static void test_its_pending_migration(void)
> gicv3_lpi_rdist_disable(pe0);
> gicv3_lpi_rdist_disable(pe1);
>
> - /* lpis are interleaved inbetween the 2 PEs */
> + /* lpis are interleaved between the 2 PEs */
> for (i = 0; i < 256; i++) {
> struct its_collection *col = i % 2 ? collection[0] :
>collection[1];
> diff --git a/arm/micro-bench.c b/arm/micro-bench.c
> index c731b1d..8436612 100644
> --- a/arm/micro-bench.c
> +++ b/arm/micro-bench.c
> @@ -265,7 +265,7 @@ static void timer_post(uint64_t ntimes, uint64_t
> *total_ticks)
> {
> /*
>* We use a 10msec timer to test the latency of PPI,
> - * so we substract the ticks of 10msec to get the
> + * so we subtract the ticks of 10msec to get the
>* actual latency
>*/
> *total_ticks -= ntimes * (cntfrq / 100);
> diff --git a/lib/arm64/asm/assembler.h b/lib/arm64/asm/assembler.h
> index a271e4c..aa8c65a 100644
> --- a/lib/arm64/asm/assembler.h
> +++ b/lib/arm64/asm/assembler.h
> @@ -32,7 +32,7 @@
> * kvm-unit-tests has no concept of scheduling.
> *
> * op: operation passed to dc instruction
> - * domain: domain used in dsb instruciton
> + * domain: domain used in dsb instruction
> * addr: starting virtual address of the region
> * size: size of the region
> * Corrupts: addr, size, tmp1, tmp2
> --
> 2.27.0
>
Applied to arm/queue.
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH 2/2] arm/run: Fix using qemu-system-aarch64 to run aarch32 tests on aarch64
On Wed, Mar 09, 2022 at 05:12:05PM +, Alexandru Elisei wrote: > Hi, > > On Wed, Mar 09, 2022 at 05:58:12PM +0100, Andrew Jones wrote: > > On Wed, Mar 09, 2022 at 04:21:17PM +, Alexandru Elisei wrote: > > > From: Andrew Jones > > > > > > KVM on arm64 can create 32 bit and 64 bit VMs. kvm-unit-tests tries to > > > take advantage of this by setting the aarch64=off -cpu option. However, > > > get_qemu_accelerator() isn't aware that KVM on arm64 can run both types > > > of VMs and it selects qemu-system-arm instead of qemu-system-aarch64. > > > This leads to an error in premature_failure() and the test is marked as > > > skipped: > > > > > > $ ./run_tests.sh selftest-setup > > > SKIP selftest-setup (qemu-system-arm: -accel kvm: invalid accelerator kvm) > > > > > > Fix this by setting QEMU to the correct qemu binary before calling > > > get_qemu_accelerator(). > > > > > > Signed-off-by: Andrew Jones > > > [ Alex E: Added commit message, changed the logic to make it clearer ] > > > Signed-off-by: Alexandru Elisei > > > --- > > > arm/run | 5 + > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/arm/run b/arm/run > > > index 2153bd320751..5fe0a45c4820 100755 > > > --- a/arm/run > > > +++ b/arm/run > > > @@ -13,6 +13,11 @@ processor="$PROCESSOR" > > > ACCEL=$(get_qemu_accelerator) || > > > exit $? > > > > > > +# KVM for arm64 can create a VM in either aarch32 or aarch64 modes. > > > +if [ "$ACCEL" = kvm ] && [ -z "$QEMU" ] && [ "$HOST" = "aarch64" ]; then > > > + QEMU=qemu-system-aarch64 > > > +fi > > > + > > > qemu=$(search_qemu_binary) || > > > exit $? > > > > > > -- > > > 2.35.1 > > > > > > > So there's a bug with this patch which was also present in the patch I > > proposed. By setting $QEMU before we call search_qemu_binary() we may > > force a "A QEMU binary was not found." failure even though a perfectly > > good 'qemu-kvm' binary is present. > > I noticed that search_qemu_binary() tries to search for both > qemu-system-ARCH_NAME and qemu-kvm, and I first thought that qemu-kvm is a > legacy name for qemu-system-ARCH_NAME. > > I just did some googling, and I think it's actually how certain distros (like > SLES) package qemu-system-ARCH_NAME, is that correct? Right > > If that is so, one idea I toyed with (for something else) is to move the error > messages from search_qemu_binary() to the call sites, that way arm/run can > first > try to find qemu-system-aarch64, then fallback to qemu-kvm, and only after > both > aren't found exit with an error. Just a suggestion, in case you find it > useful. We don't have to move the error messages, even if we want to use search_qemu_binary() as a silent check. We can just call it with a &>/dev/null and then check its return code. I still need to allocate some time to think more about this though. Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH 1/2] arm: Change text base address for 32 bit tests when running under kvmtool
On Wed, Mar 09, 2022 at 04:21:16PM +, Alexandru Elisei wrote: > The 32 bit tests do not have relocation support and rely on the build > system to set the text base address to 0x4001_, which is the memory > location where the test is placed by qemu. However, kvmtool loads a payload > at a different address, 0x8000_8000, when loading a test with --kernel. > When using --firmware, the default is 0x8000_, but that can be changed > with the --firmware-address comand line option. > > When 32 bit tests are configured to run under kvmtool, set the text base > address to 0x8000_8000. > > Signed-off-by: Alexandru Elisei > --- > arm/Makefile.arm | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/arm/Makefile.arm b/arm/Makefile.arm > index 3a4cc6b26234..01fd4c7bb6e2 100644 > --- a/arm/Makefile.arm > +++ b/arm/Makefile.arm > @@ -14,7 +14,13 @@ CFLAGS += $(machine) > CFLAGS += -mcpu=$(PROCESSOR) > CFLAGS += -mno-unaligned-access > > +ifeq ($(TARGET),qemu) > arch_LDFLAGS = -Ttext=4001 > +else ifeq ($(TARGET),kvmtool) > +arch_LDFLAGS = -Ttext=80008000 > +else > +$(error Unknown target $(TARGET)) > +endif > > define arch_elf_check = > endef > -- > 2.35.1 > Applied to arm/queue, https://gitlab.com/rhdrjones/kvm-unit-tests/-/commits/arm/queue Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH 2/2] arm/run: Fix using qemu-system-aarch64 to run aarch32 tests on aarch64
On Wed, Mar 09, 2022 at 04:21:17PM +, Alexandru Elisei wrote: > From: Andrew Jones > > KVM on arm64 can create 32 bit and 64 bit VMs. kvm-unit-tests tries to > take advantage of this by setting the aarch64=off -cpu option. However, > get_qemu_accelerator() isn't aware that KVM on arm64 can run both types > of VMs and it selects qemu-system-arm instead of qemu-system-aarch64. > This leads to an error in premature_failure() and the test is marked as > skipped: > > $ ./run_tests.sh selftest-setup > SKIP selftest-setup (qemu-system-arm: -accel kvm: invalid accelerator kvm) > > Fix this by setting QEMU to the correct qemu binary before calling > get_qemu_accelerator(). > > Signed-off-by: Andrew Jones > [ Alex E: Added commit message, changed the logic to make it clearer ] > Signed-off-by: Alexandru Elisei > --- > arm/run | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/arm/run b/arm/run > index 2153bd320751..5fe0a45c4820 100755 > --- a/arm/run > +++ b/arm/run > @@ -13,6 +13,11 @@ processor="$PROCESSOR" > ACCEL=$(get_qemu_accelerator) || > exit $? > > +# KVM for arm64 can create a VM in either aarch32 or aarch64 modes. > +if [ "$ACCEL" = kvm ] && [ -z "$QEMU" ] && [ "$HOST" = "aarch64" ]; then > + QEMU=qemu-system-aarch64 > +fi > + > qemu=$(search_qemu_binary) || > exit $? > > -- > 2.35.1 > So there's a bug with this patch which was also present in the patch I proposed. By setting $QEMU before we call search_qemu_binary() we may force a "A QEMU binary was not found." failure even though a perfectly good 'qemu-kvm' binary is present. I'll try to come up with something better. Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: Timer delays in VM
On Mon, Feb 28, 2022 at 09:02:47PM +, Marc Zyngier wrote: > > You also don't mention what host kernel version you are running. > In general, please try and reproduce the issue using the latest > kernel version (5.16 at the moment). Please also indicate what > HW you are using. > Yes, please reply with the kernel version these delays are seen on and also try to reproduce with a latest upstream kernel version. If the delays are not present with the upstream kernel version, then we can open a CentOS bug. (You may want to experiment with host and guest kernel configs as well.) Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH] configure: arm: Fixes to build and run tests on Apple Silicon
On Thu, Feb 17, 2022 at 10:28:06AM +, Nikos Nikoleris wrote: > On MacOS: > > $> uname -m > > returns: > > arm64 > > To unify how we handle the achitecture detection across different > systems, sed it to aarch64 which is what's typically reported on Was "sed" a typo or a new verb for "sedding" stuff :-) > Linux. > > In addition, when HVF is the acceleration method on aarch64, make sure > we select the right processor when invoking qemu. > > Signed-off-by: Nikos Nikoleris > --- > arm/run | 3 +++ > configure | 2 +- > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arm/run b/arm/run > index 2153bd3..0629b69 100755 > --- a/arm/run > +++ b/arm/run > @@ -27,6 +27,9 @@ if [ "$ACCEL" = "kvm" ]; then > if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then > M+=',gic-version=host' > fi > +fi > + > +if [ "$ACCEL" = "kvm" ] || [ "$ACCEL" = "hvf" ]; then > if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then > processor="host" > if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then > diff --git a/configure b/configure > index 2d9c3e0..ff840c1 100755 > --- a/configure > +++ b/configure > @@ -14,7 +14,7 @@ objcopy=objcopy > objdump=objdump > ar=ar > addr2line=addr2line > -arch=$(uname -m | sed -e 's/i.86/i386/;s/arm.*/arm/;s/ppc64.*/ppc64/') > +arch=$(uname -m | sed -e > 's/i.86/i386/;s/arm64/aarch64/;s/arm.*/arm/;s/ppc64.*/ppc64/') > host=$arch > cross_prefix= > endian="" > -- > 2.32.0 (Apple Git-132) > So, with this, we've got kvm-unit-tests running on HVF now? Applied to arm/queue https://gitlab.com/rhdrjones/kvm-unit-tests/-/commits/arm/queue Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH 0/4] configure changes and rename --target-efi
On Thu, Feb 10, 2022 at 11:48:03AM -0800, Zixuan Wang wrote: > On Thu, Feb 10, 2022 at 11:36 AM Sean Christopherson > wrote: > > > > On Thu, Feb 10, 2022, Zixuan Wang wrote: > > > On Thu, Feb 10, 2022 at 11:05 AM Alexandru Elisei > > > wrote: > > > > > > > > Hi, > > > > > > > > On Thu, Feb 10, 2022 at 05:25:46PM +, Sean Christopherson wrote: > > > > > On Thu, Feb 10, 2022, Alexandru Elisei wrote: > > > > > > I renamed --target-efi to --efi-payload in the last patch because I > > > > > > felt it > > > > > > looked rather confusing to do ./configure --target=qemu > > > > > > --target-efi when > > > > > > configuring the tests. If the rename is not acceptable, I can think > > > > > > of a > > > > > > few other options: > > > > > > > > > > I find --target-efi to be odd irrespective of this new conflict. A > > > > > simple --efi > > > > > seems like it would be sufficient. > > > > > > > > > > > 1. Rename --target to --vmm. That was actually the original name > > > > > > for the > > > > > > option, but I changed it because I thought --target was more > > > > > > generic and > > > > > > that --target=efi would be the way going forward to compile > > > > > > kvm-unit-tests > > > > > > to run as an EFI payload. I realize now that separating the VMM from > > > > > > compiling kvm-unit-tests to run as an EFI payload is better, as > > > > > > there can > > > > > > be multiple VMMs that can run UEFI in a VM. Not many people use > > > > > > kvmtool as > > > > > > a test runner, so I think the impact on users should be minimal. > > > > > > > > > > Again irrespective of --target-efi, I think --target for the VMM is a > > > > > potentially > > > > > confusing name. Target Triplet[*] and --target have specific meaning > > > > > for the > > > > > compiler, usurping that for something similar but slightly different > > > > > is odd. > > > > > > > > Wouldn't that mean that --target-efi is equally confusing? Do you have > > > > suggestions for other names? > > > > > > How about --config-efi for configure, and CONFIG_EFI for source code? > > > I thought about this name when I was developing the initial patch, and > > > Varad also proposed similar names in his initial patch series [1]: > > > --efi and CONFIG_EFI. > > > > I don't mind CONFIG_EFI for the source, that provides a nice hint that it's > > a > > configure option and is familiar for kernel developers. But for the > > actually > > option, why require more typing? I really don't see any benefit of > > --config-efi > > over --efi. > > I agree, --efi looks better than --target-efi or --config-efi. > Works for me. Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH v3 0/3] GIC ITS tests
On Tue, Feb 01, 2022 at 01:10:13PM +, Alex Bennée wrote: > > Gentle ping, I'm trying to clear this off my internal JIRA so let me > know if you want me to do anything to help. > Sorry Alex! I've been juggling too many balls lately and completely dropped this one. I'll rebase arm/queue now and run it through some sanity tests. If all it good, I'll do the MR right away. Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 5/5] kvm: selftests: aarch64: use a tighter assert in vgic_poke_irq()
On Wed, Jan 26, 2022 at 07:08:58PM -0800, Ricardo Koller wrote: > vgic_poke_irq() checks that the attr argument passed to the vgic device > ioctl is sane. Make this check tighter by moving it to after the last > attr update. > > Signed-off-by: Ricardo Koller > Reported-by: Reiji Watanabe > Cc: Andrew Jones > --- > tools/testing/selftests/kvm/lib/aarch64/vgic.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/kvm/lib/aarch64/vgic.c > b/tools/testing/selftests/kvm/lib/aarch64/vgic.c > index 79864b941617..f365c32a7296 100644 > --- a/tools/testing/selftests/kvm/lib/aarch64/vgic.c > +++ b/tools/testing/selftests/kvm/lib/aarch64/vgic.c > @@ -138,9 +138,6 @@ static void vgic_poke_irq(int gic_fd, uint32_t intid, > uint64_t val; > bool intid_is_private = INTID_IS_SGI(intid) || INTID_IS_PPI(intid); > > - /* Check that the addr part of the attr is within 32 bits. */ > - assert(attr <= KVM_DEV_ARM_VGIC_OFFSET_MASK); > - > uint32_t group = intid_is_private ? KVM_DEV_ARM_VGIC_GRP_REDIST_REGS > : KVM_DEV_ARM_VGIC_GRP_DIST_REGS; > > @@ -150,6 +147,9 @@ static void vgic_poke_irq(int gic_fd, uint32_t intid, > attr += SZ_64K; > } > > + /* Check that the addr part of the attr is within 32 bits. */ > + assert((attr & ~KVM_DEV_ARM_VGIC_OFFSET_MASK) == 0); > + > /* >* All calls will succeed, even with invalid intid's, as long as the >* addr part of the attr is within 32 bits (checked above). An invalid > -- > 2.35.0.rc0.227.g00780c9af4-goog > Reviewed-by: Andrew Jones ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 4/5] kvm: selftests: aarch64: fix some vgic related comments
On Wed, Jan 26, 2022 at 07:08:57PM -0800, Ricardo Koller wrote:
> Fix the formatting of some comments and the wording of one of them (in
> gicv3_access_reg).
>
> Signed-off-by: Ricardo Koller
> Reported-by: Reiji Watanabe
> Cc: Andrew Jones
> Reviewed-by: Andrew Jones
I didn't give my r-b to this patch before, but you can keep it, because
here's another one
Reviewed-by: Andrew Jones
> ---
> tools/testing/selftests/kvm/aarch64/vgic_irq.c | 12
> tools/testing/selftests/kvm/lib/aarch64/gic_v3.c | 10 ++
> tools/testing/selftests/kvm/lib/aarch64/vgic.c | 3 ++-
> 3 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> index 0106fc464afe..f0230711fbe9 100644
> --- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> @@ -306,7 +306,8 @@ static void guest_restore_active(struct test_args *args,
> uint32_t prio, intid, ap1r;
> int i;
>
> - /* Set the priorities of the first (KVM_NUM_PRIOS - 1) IRQs
> + /*
> + * Set the priorities of the first (KVM_NUM_PRIOS - 1) IRQs
>* in descending order, so intid+1 can preempt intid.
>*/
> for (i = 0, prio = (num - 1) * 8; i < num; i++, prio -= 8) {
> @@ -315,7 +316,8 @@ static void guest_restore_active(struct test_args *args,
> gic_set_priority(intid, prio);
> }
>
> - /* In a real migration, KVM would restore all GIC state before running
> + /*
> + * In a real migration, KVM would restore all GIC state before running
>* guest code.
>*/
> for (i = 0; i < num; i++) {
> @@ -503,7 +505,8 @@ static void guest_code(struct test_args *args)
> test_injection_failure(args, f);
> }
>
> - /* Restore the active state of IRQs. This would happen when live
> + /*
> + * Restore the active state of IRQs. This would happen when live
>* migrating IRQs in the middle of being handled.
>*/
> for_each_supported_activate_fn(args, set_active_fns, f)
> @@ -840,7 +843,8 @@ int main(int argc, char **argv)
> }
> }
>
> - /* If the user just specified nr_irqs and/or gic_version, then run all
> + /*
> + * If the user just specified nr_irqs and/or gic_version, then run all
>* combinations.
>*/
> if (default_args) {
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> index e4945fe66620..263bf3ed8fd5 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> @@ -19,7 +19,7 @@ struct gicv3_data {
> unsigned int nr_spis;
> };
>
> -#define sgi_base_from_redist(redist_base)(redist_base + SZ_64K)
> +#define sgi_base_from_redist(redist_base)(redist_base + SZ_64K)
> #define DIST_BIT (1U << 31)
>
> enum gicv3_intid_range {
> @@ -105,7 +105,8 @@ static void gicv3_set_eoi_split(bool split)
> {
> uint32_t val;
>
> - /* All other fields are read-only, so no need to read CTLR first. In
> + /*
> + * All other fields are read-only, so no need to read CTLR first. In
>* fact, the kernel does the same.
>*/
> val = split ? (1U << 1) : 0;
> @@ -160,8 +161,9 @@ static void gicv3_access_reg(uint32_t intid, uint64_t
> offset,
>
> GUEST_ASSERT(bits_per_field <= reg_bits);
> GUEST_ASSERT(!write || *val < (1U << bits_per_field));
> - /* Some registers like IROUTER are 64 bit long. Those are currently not
> - * supported by readl nor writel, so just asserting here until then.
> + /*
> + * This function does not support 64 bit accesses. Just asserting here
> + * until we implement readq/writeq.
>*/
> GUEST_ASSERT(reg_bits == 32);
>
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> index b3a0fca0d780..79864b941617 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> @@ -150,7 +150,8 @@ static void vgic_poke_irq(int gic_fd, uint32_t intid,
> attr += SZ_64K;
> }
>
> - /* All calls will succeed, even with invalid intid's, as long as the
> + /*
> + * All calls will succeed, even with invalid intid's, as long as the
>* addr part of the attr is within 32 bits (checked above). An invalid
>* intid will just make the read/writes point to above the intended
>* register space (i.e., ICPENDR after ISPENDR).
> --
> 2.35.0.rc0.227.g00780c9af4-goog
>
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 2/5] kvm: selftests: aarch64: pass vgic_irq guest args as a pointer
On Wed, Jan 26, 2022 at 07:08:55PM -0800, Ricardo Koller wrote:
> The guest in vgic_irq gets its arguments in a struct. This struct used
> to fit nicely in a single register so vcpu_args_set() was able to pass
> it by value by setting x0 with it.
Ouch.
> Unfortunately, this args struct grew
> after some commits and some guest args became random (specically
> kvm_supports_irqfd).
>
> Fix this by passing the guest args as a pointer (after allocating some
> guest memory for it).
>
> Signed-off-by: Ricardo Koller
> Reported-by: Reiji Watanabe
> Cc: Andrew Jones
> ---
> .../testing/selftests/kvm/aarch64/vgic_irq.c | 29 ++-
> 1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> index e6c7d7f8fbd1..b701eb80128d 100644
> --- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> @@ -472,10 +472,10 @@ static void test_restore_active(struct test_args *args,
> struct kvm_inject_desc *
> guest_restore_active(args, MIN_SPI, 4, f->cmd);
> }
>
> -static void guest_code(struct test_args args)
> +static void guest_code(struct test_args *args)
> {
> - uint32_t i, nr_irqs = args.nr_irqs;
> - bool level_sensitive = args.level_sensitive;
> + uint32_t i, nr_irqs = args->nr_irqs;
> + bool level_sensitive = args->level_sensitive;
> struct kvm_inject_desc *f, *inject_fns;
>
> gic_init(GIC_V3, 1, dist, redist);
> @@ -484,11 +484,11 @@ static void guest_code(struct test_args args)
> gic_irq_enable(i);
>
> for (i = MIN_SPI; i < nr_irqs; i++)
> - gic_irq_set_config(i, !args.level_sensitive);
> + gic_irq_set_config(i, !level_sensitive);
>
> - gic_set_eoi_split(args.eoi_split);
> + gic_set_eoi_split(args->eoi_split);
>
> - reset_priorities();
> + reset_priorities(args);
> gic_set_priority_mask(CPU_PRIO_MASK);
>
> inject_fns = level_sensitive ? inject_level_fns
> @@ -497,17 +497,17 @@ static void guest_code(struct test_args args)
> local_irq_enable();
>
> /* Start the tests. */
> - for_each_supported_inject_fn(, inject_fns, f) {
> - test_injection(, f);
> - test_preemption(, f);
> - test_injection_failure(, f);
> + for_each_supported_inject_fn(args, inject_fns, f) {
> + test_injection(args, f);
> + test_preemption(args, f);
> + test_injection_failure(args, f);
> }
>
> /* Restore the active state of IRQs. This would happen when live
>* migrating IRQs in the middle of being handled.
>*/
> - for_each_supported_activate_fn(, set_active_fns, f)
> - test_restore_active(, f);
> + for_each_supported_activate_fn(args, set_active_fns, f)
> + test_restore_active(args, f);
>
> GUEST_DONE();
> }
> @@ -739,6 +739,7 @@ static void test_vgic(uint32_t nr_irqs, bool
> level_sensitive, bool eoi_split)
> int gic_fd;
> struct kvm_vm *vm;
> struct kvm_inject_args inject_args;
> + vm_vaddr_t args_gva;
>
> struct test_args args = {
> .nr_irqs = nr_irqs,
> @@ -757,7 +758,9 @@ static void test_vgic(uint32_t nr_irqs, bool
> level_sensitive, bool eoi_split)
> vcpu_init_descriptor_tables(vm, VCPU_ID);
>
> /* Setup the guest args page (so it gets the args). */
> - vcpu_args_set(vm, 0, 1, args);
> + args_gva = vm_vaddr_alloc_page(vm);
> + memcpy(addr_gva2hva(vm, args_gva), , sizeof(args));
> + vcpu_args_set(vm, 0, 1, args_gva);
>
> gic_fd = vgic_v3_setup(vm, 1, nr_irqs,
> GICD_BASE_GPA, GICR_BASE_GPA);
> --
> 2.35.0.rc0.227.g00780c9af4-goog
>
Reviewed-by: Andrew Jones
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/5] kvm: selftests: aarch64: fix assert in gicv3_access_reg
On Wed, Jan 26, 2022 at 07:08:54PM -0800, Ricardo Koller wrote: > The val argument in gicv3_access_reg can have any value when used for a > read, not necessarily 0. Fix the assert by checking val only for > writes. > > Signed-off-by: Ricardo Koller > Reported-by: Reiji Watanabe > Cc: Andrew Jones Commit message said my r-b would be here, but it doesn't appear to be. Here it is again Reviewed-by: Andrew Jones > --- > tools/testing/selftests/kvm/lib/aarch64/gic_v3.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c > b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c > index 00f613c0583c..e4945fe66620 100644 > --- a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c > +++ b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c > @@ -159,7 +159,7 @@ static void gicv3_access_reg(uint32_t intid, uint64_t > offset, > uint32_t cpu_or_dist; > > GUEST_ASSERT(bits_per_field <= reg_bits); > - GUEST_ASSERT(*val < (1U << bits_per_field)); > + GUEST_ASSERT(!write || *val < (1U << bits_per_field)); > /* Some registers like IROUTER are 64 bit long. Those are currently not >* supported by readl nor writel, so just asserting here until then. >*/ > -- > 2.35.0.rc0.227.g00780c9af4-goog > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 2/2] kvm: selftests: aarch64: fix some vgic related comments
On Thu, Jan 20, 2022 at 09:39:05AM -0800, Ricardo Koller wrote:
> Fix the formatting of some comments and the wording of one of them (in
> gicv3_access_reg).
>
> Signed-off-by: Ricardo Koller
> Reported-by: Reiji Watanabe
> Cc: Andrew Jones
> ---
> tools/testing/selftests/kvm/aarch64/vgic_irq.c | 12
> tools/testing/selftests/kvm/lib/aarch64/gic_v3.c | 11 +++
> tools/testing/selftests/kvm/lib/aarch64/vgic.c | 3 ++-
> 3 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> index e6c7d7f8fbd1..258bb5150a07 100644
> --- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> @@ -306,7 +306,8 @@ static void guest_restore_active(struct test_args *args,
> uint32_t prio, intid, ap1r;
> int i;
>
> - /* Set the priorities of the first (KVM_NUM_PRIOS - 1) IRQs
> + /*
> + * Set the priorities of the first (KVM_NUM_PRIOS - 1) IRQs
>* in descending order, so intid+1 can preempt intid.
>*/
> for (i = 0, prio = (num - 1) * 8; i < num; i++, prio -= 8) {
> @@ -315,7 +316,8 @@ static void guest_restore_active(struct test_args *args,
> gic_set_priority(intid, prio);
> }
>
> - /* In a real migration, KVM would restore all GIC state before running
> + /*
> + * In a real migration, KVM would restore all GIC state before running
>* guest code.
>*/
> for (i = 0; i < num; i++) {
> @@ -503,7 +505,8 @@ static void guest_code(struct test_args args)
> test_injection_failure(, f);
> }
>
> - /* Restore the active state of IRQs. This would happen when live
> + /*
> + * Restore the active state of IRQs. This would happen when live
>* migrating IRQs in the middle of being handled.
>*/
> for_each_supported_activate_fn(, set_active_fns, f)
> @@ -837,7 +840,8 @@ int main(int argc, char **argv)
> }
> }
>
> - /* If the user just specified nr_irqs and/or gic_version, then run all
> + /*
> + * If the user just specified nr_irqs and/or gic_version, then run all
>* combinations.
>*/
> if (default_args) {
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> index e4945fe66620..93fc35b88410 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> @@ -19,7 +19,7 @@ struct gicv3_data {
> unsigned int nr_spis;
> };
>
> -#define sgi_base_from_redist(redist_base)(redist_base + SZ_64K)
> +#define sgi_base_from_redist(redist_base)(redist_base + SZ_64K)
> #define DIST_BIT (1U << 31)
>
> enum gicv3_intid_range {
> @@ -105,7 +105,8 @@ static void gicv3_set_eoi_split(bool split)
> {
> uint32_t val;
>
> - /* All other fields are read-only, so no need to read CTLR first. In
> + /*
> + * All other fields are read-only, so no need to read CTLR first. In
>* fact, the kernel does the same.
>*/
> val = split ? (1U << 1) : 0;
> @@ -160,8 +161,10 @@ static void gicv3_access_reg(uint32_t intid, uint64_t
> offset,
>
> GUEST_ASSERT(bits_per_field <= reg_bits);
> GUEST_ASSERT(!write || *val < (1U << bits_per_field));
> - /* Some registers like IROUTER are 64 bit long. Those are currently not
> - * supported by readl nor writel, so just asserting here until then.
> + /*
> + * This function does not support 64 bit accesses as those are
> + * currently not supported by readl nor writel, so just asserting here
> + * until then.
Well, readl and writel will never support 64 bit accesses. We'd need to
implement readq and writeq for that. If we're going to rewrite this
comment please state it that way instead.
>*/
> GUEST_ASSERT(reg_bits == 32);
>
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> index b3a0fca0d780..79864b941617 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> @@ -150,7 +150,8 @@ static void vgic_poke_irq(int gic_fd, uint32_t intid,
> attr += SZ_64K;
> }
>
> - /* All calls will succeed, even with invalid intid's, as long as the
> + /*
> + * All calls will succeed, even with invalid intid's, as long as the
>* addr part of the attr is within 32 bit
Re: [PATCH 1/2] kvm: selftests: aarch64: fix assert in gicv3_access_reg
On Thu, Jan 20, 2022 at 09:39:04AM -0800, Ricardo Koller wrote: > The val argument in gicv3_access_reg can have any value when used for a > read, not necessarily 0. Fix the assert by checking val only for > writes. > > Signed-off-by: Ricardo Koller > Reported-by: Reiji Watanabe > Cc: Andrew Jones > --- > tools/testing/selftests/kvm/lib/aarch64/gic_v3.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c > b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c > index 00f613c0583c..e4945fe66620 100644 > --- a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c > +++ b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c > @@ -159,7 +159,7 @@ static void gicv3_access_reg(uint32_t intid, uint64_t > offset, > uint32_t cpu_or_dist; > > GUEST_ASSERT(bits_per_field <= reg_bits); > - GUEST_ASSERT(*val < (1U << bits_per_field)); > + GUEST_ASSERT(!write || *val < (1U << bits_per_field)); > /* Some registers like IROUTER are 64 bit long. Those are currently not >* supported by readl nor writel, so just asserting here until then. >*/ > -- > 2.35.0.rc0.227.g00780c9af4-goog > Reviewed-by: Andrew Jones ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3] kselftest: kvm/arm64: Skip tests if we can't create a vgic-v3
On Wed, Jan 26, 2022 at 02:52:42PM +, Mark Brown wrote: > The arch_timer and vgic_irq kselftests assume that they can create a > vgic-v3, using the library function vgic_v3_setup() which aborts with a > test failure if it is not possible to do so. Since vgic-v3 can only be > instantiated on systems where the host has GICv3 this leads to false > positives on older systems where that is not the case. > > Fix this by changing vgic_v3_setup() to return an error if the vgic can't > be instantiated and have the callers skip if this happens. We could also > exit flagging a skip in vgic_v3_setup() but this would prevent future test > cases conditionally deciding which GIC to use or generally doing more > complex output. > > Signed-off-by: Mark Brown > --- > > v3: > - Use custom print_skip() helper. > - Use internal version of _kvm_create_device. > v2: > - The test for being able to create the GIC doesn't actually >instantiate it, add a call doing so in that case. > > tools/testing/selftests/kvm/aarch64/arch_timer.c | 7 ++- > tools/testing/selftests/kvm/aarch64/vgic_irq.c | 4 > tools/testing/selftests/kvm/lib/aarch64/vgic.c | 4 +++- > 3 files changed, 13 insertions(+), 2 deletions(-) > Reviewed-by: Andrew Jones ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2] kselftest: kvm/arm64: Skip tests if we can't create a vgic-v3
On Wed, Jan 26, 2022 at 02:29:14PM +, Mark Brown wrote:
> On Wed, Jan 26, 2022 at 03:17:41PM +0100, Andrew Jones wrote:
> > On Wed, Jan 26, 2022 at 01:53:19PM +, Mark Brown wrote:
>
> > > - vgic_v3_setup(vm, nr_vcpus, 64, GICD_BASE_GPA, GICR_BASE_GPA);
> > > + ret = vgic_v3_setup(vm, nr_vcpus, 64, GICD_BASE_GPA, GICR_BASE_GPA);
> > > + if (ret < 0) {
> > > + pr_info("Failed to create vgic-v3, skipping\n");
>
> > Please use 'print_skip', which appends ", skipping test" to keep the skip
> > messages consistent. Also, print_skip can't be disabled with -DQUIET like
> > pr_info.
>
> I see. It might be nice to convert these tests to use the ksft_
> stuff...
Indeed. I'll add that to my TODO.
>
> > > - /* Distributor setup */
> > > + /* Distributor setup - test if it's possible then actually do it */
> > > + gic_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, true);
> > > + if (gic_fd != 0)
> > > + return -1;
> > > gic_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
>
> > kvm selftests generally asserts on failure with the nonunderscore
> > prefixed KVM ioctl wrapper functions, which is why you appear to
> > be forced to do this nasty dance. However, kvm selftests usually
> > always also offers an underscore prefixed version of the KVM ioctl
> > wrapper function too for cases like these. So we can just do
>
> > if (_kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false, _fd) != 0)
> > return -1;
>
> And the _ version is OK to use in the vgic code? The _ makes it look
> like it's internal only.
It's extra OK. Anything in lib/* or lib/*/* is internal to the lib.
However, it's even OK for a unit test to use the _ prefixed functions.
The _ isn't so much about being internal as it is about being a raw
version of the ioctl wrapper, which returns error codes, vs. the
asserting version of the wrapper which only returns results on success.
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2] kselftest: kvm/arm64: Skip tests if we can't create a vgic-v3
On Wed, Jan 26, 2022 at 01:53:19PM +, Mark Brown wrote:
> The arch_timer and vgic_irq kselftests assume that they can create a
> vgic-v3, using the library function vgic_v3_setup() which aborts with a
> test failure if it is not possible to do so. Since vgic-v3 can only be
> instantiated on systems where the host has GICv3 this leads to false
> positives on older systems where that is not the case.
>
> Fix this by changing vgic_v3_setup() to return an error if the vgic can't
> be instantiated and have the callers skip if this happens. We could also
> exit flagging a skip in vgic_v3_setup() but this would prevent future test
> cases conditionally deciding which GIC to use or generally doing more
> complex output.
>
> Signed-off-by: Mark Brown
> ---
>
> v2:
> - The test for being able to create the GIC doesn't actually
>instantiate it, add a call doing so in that case.
>
> tools/testing/selftests/kvm/aarch64/arch_timer.c | 7 ++-
> tools/testing/selftests/kvm/aarch64/vgic_irq.c | 4
> tools/testing/selftests/kvm/lib/aarch64/vgic.c | 5 -
> 3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c
> b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> index 9ad38bd360a4..791d38404652 100644
> --- a/tools/testing/selftests/kvm/aarch64/arch_timer.c
> +++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> @@ -366,6 +366,7 @@ static struct kvm_vm *test_vm_create(void)
> {
> struct kvm_vm *vm;
> unsigned int i;
> + int ret;
> int nr_vcpus = test_args.nr_vcpus;
>
> vm = vm_create_default_with_vcpus(nr_vcpus, 0, 0, guest_code, NULL);
> @@ -382,7 +383,11 @@ static struct kvm_vm *test_vm_create(void)
>
> ucall_init(vm, NULL);
> test_init_timer_irq(vm);
> - vgic_v3_setup(vm, nr_vcpus, 64, GICD_BASE_GPA, GICR_BASE_GPA);
> + ret = vgic_v3_setup(vm, nr_vcpus, 64, GICD_BASE_GPA, GICR_BASE_GPA);
> + if (ret < 0) {
> + pr_info("Failed to create vgic-v3, skipping\n");
Please use 'print_skip', which appends ", skipping test" to keep the skip
messages consistent. Also, print_skip can't be disabled with -DQUIET like
pr_info.
> + exit(KSFT_SKIP);
> + }
>
> /* Make all the test's cmdline args visible to the guest */
> sync_global_to_guest(vm, test_args);
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> index e6c7d7f8fbd1..b127a261fd29 100644
> --- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> @@ -761,6 +761,10 @@ static void test_vgic(uint32_t nr_irqs, bool
> level_sensitive, bool eoi_split)
>
> gic_fd = vgic_v3_setup(vm, 1, nr_irqs,
> GICD_BASE_GPA, GICR_BASE_GPA);
> + if (gic_fd < 0) {
> + pr_info("Failed to create vgic-v3, skipping\n");
print_skip
> + exit(KSFT_SKIP);
> + }
>
> vm_install_exception_handler(vm, VECTOR_IRQ_CURRENT,
> guest_irq_handlers[args.eoi_split][args.level_sensitive]);
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> index b3a0fca0d780..4ea65e119bdd 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> @@ -51,7 +51,10 @@ int vgic_v3_setup(struct kvm_vm *vm, unsigned int
> nr_vcpus, uint32_t nr_irqs,
> "Number of vCPUs requested (%u) doesn't match with the
> ones created for the VM (%u)\n",
> nr_vcpus, nr_vcpus_created);
>
> - /* Distributor setup */
> + /* Distributor setup - test if it's possible then actually do it */
> + gic_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, true);
> + if (gic_fd != 0)
> + return -1;
> gic_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
kvm selftests generally asserts on failure with the nonunderscore
prefixed KVM ioctl wrapper functions, which is why you appear to
be forced to do this nasty dance. However, kvm selftests usually
always also offers an underscore prefixed version of the KVM ioctl
wrapper function too for cases like these. So we can just do
if (_kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false, _fd) != 0)
return -1;
Thanks,
drew
>
> kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
> --
> 2.30.2
>
> ___
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH v2] arm64: debug: mark test_[bp,wp,ss] as noinline
On Wed, Jan 12, 2022 at 07:21:55AM -0800, Ricardo Koller wrote:
> Clang inlines some functions (like test_ss) which define global labels
> in inline assembly (e.g., ss_start). This results in:
>
> arm/debug.c:382:15: error: invalid symbol redefinition
> asm volatile("ss_start:\n"
> ^
> :1:2: note: instantiated into assembly here
> ss_start:
> ^
> 1 error generated.
>
> Fix these functions by marking them as "noinline".
>
> Cc: Andrew Jones
> Signed-off-by: Ricardo Koller
> ---
> This applies on top of: "[kvm-unit-tests PATCH 0/3] arm64: debug: add
> migration tests for debug state"
> which is in https://gitlab.com/rhdrjones/kvm-unit-tests/-/commits/arm/queue.
>
> arm/debug.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arm/debug.c b/arm/debug.c
> index 54f059d..e9f8056 100644
> --- a/arm/debug.c
> +++ b/arm/debug.c
> @@ -264,7 +264,7 @@ static void do_migrate(void)
> report_info("Migration complete");
> }
>
> -static void test_hw_bp(bool migrate)
> +static noinline void test_hw_bp(bool migrate)
> {
> extern unsigned char hw_bp0;
> uint32_t bcr;
> @@ -310,7 +310,7 @@ static void test_hw_bp(bool migrate)
>
> static volatile char write_data[16];
>
> -static void test_wp(bool migrate)
> +static noinline void test_wp(bool migrate)
> {
> uint32_t wcr;
> uint32_t mdscr;
> @@ -353,7 +353,7 @@ static void test_wp(bool migrate)
> }
> }
>
> -static void test_ss(bool migrate)
> +static noinline void test_ss(bool migrate)
> {
> extern unsigned char ss_start;
> uint32_t mdscr;
> --
> 2.34.1.575.g55b058a8bb-goog
>
Applied to arm/queue.
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC PATCH 3/3] KVM: selftests: Add vgic initialization for dirty log perf test for ARM
On Mon, Jan 10, 2022 at 09:04:41PM +, Jing Zhang wrote: > For ARM64, if no vgic is setup before the dirty log perf test, the > userspace irqchip would be used, which would affect the dirty log perf > test result. > > Signed-off-by: Jing Zhang > --- > tools/testing/selftests/kvm/dirty_log_perf_test.c | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c > b/tools/testing/selftests/kvm/dirty_log_perf_test.c > index 1954b964d1cf..b501338d9430 100644 > --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c > +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c > @@ -18,6 +18,12 @@ > #include "test_util.h" > #include "perf_test_util.h" > #include "guest_modes.h" > +#ifdef __aarch64__ > +#include "aarch64/vgic.h" > + > +#define GICD_BASE_GPA0x800ULL > +#define GICR_BASE_GPA0x80AULL > +#endif > > /* How many host loops to run by default (one KVM_GET_DIRTY_LOG for each > loop)*/ > #define TEST_HOST_LOOP_N 2UL > @@ -200,6 +206,10 @@ static void run_test(enum vm_guest_mode mode, void *arg) > vm_enable_cap(vm, ); > } > > +#ifdef __aarch64__ > + vgic_v3_setup(vm, nr_vcpus, 64, GICD_BASE_GPA, GICR_BASE_GPA); ^^ extra parameter Thanks, drew > +#endif > + > /* Start the iterations */ > iteration = 0; > host_quit = false; > -- > 2.34.1.575.g55b058a8bb-goog > > ___ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 2/2] KVM: arm64: selftests: Introduce vcpu_width_config
On Sun, Jan 09, 2022 at 09:40:42PM -0800, Reiji Watanabe wrote: > Introduce a test for aarch64 that ensures non-mixed-width vCPUs > (all 64bit vCPUs or all 32bit vcPUs) can be configured, and > mixed-width vCPUs cannot be configured. > > Signed-off-by: Reiji Watanabe > --- > tools/testing/selftests/kvm/.gitignore| 1 + > tools/testing/selftests/kvm/Makefile | 1 + > .../selftests/kvm/aarch64/vcpu_width_config.c | 125 ++ > 3 files changed, 127 insertions(+) > create mode 100644 tools/testing/selftests/kvm/aarch64/vcpu_width_config.c > Reviewed-by: Andrew Jones ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] hw/arm/virt: KVM: Enable PAuth when supported by the host
On Mon, Jan 03, 2022 at 06:05:41PM +, Marc Zyngier wrote: > Andrew Jones wrote: > > > > Thanks for considering a documentation update. In this case, though, I > > think we should delete the "TCG VCPU Features" pauth paragraph, rather > > than add a new "KVM VCPU Features" pauth paragraph. We don't need to > > document each CPU feature. We just document complex ones, like sve*, > > KVM specific ones (kvm-*), and TCG specific ones (now only pauth-impdef). > > Sure, works for me. Do we need to keep a trace of the available > options? For arm we need to extend target/arm/helper.c:arm_cpu_list() to output the possible flags like x86 does. On x86 doing this qemu-system-x86_64 -cpu help not only gives us a list of cpu types, but also a list of flags we can provide to the cpus (although not all flags will work on all cpus...) On arm doing this qemu-system-aarch64 -cpu help only gives us a list of cpu types. > I'm not sure how a user is supposed to find out about those > (I always end-up grepping through the code base, and something tells > me I'm doing it wrong...). The QMP stuff flies way over my head. > Indeed, currently grepping is less awkward than probing with QMP. With an extension to target/arm/helper.c:arm_cpu_list() we can avoid grepping too. I've just added this to my TODO [again]. It was there once already, but fell off the bottom... Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
