Re: [PATCH] selftests: KVM: Call ucall_init when setting up in rseq_test

[Sean Christopherson <seanjc@google.com>]

On Tue, Sep 28, 2021, Andrew Jones wrote:
> On Mon, Sep 27, 2021 at 08:05:14PM +0000, Sean Christopherson wrote:
> > My vote would be to rework arm64's ucall_init() as a prep patch and drop the param
> > in the process.  There are zero tests that provide a non-NULL value, but that's
> > likely because tests that care deliberately defer ucall_init() until after memory
> > regions and page tables have been configured.
> > 
> > IMO, arm64's approach is unnecessarily complex (that's a common theme for KVM's
> > selftests...).  The code attempts to avoid magic numbers by not hardcoding the MMIO
> > range, but in doing so makes the end result even more magical, e.g. starting at
> > 5/8ths of min(MAX_PA, MAX_VA).
> > 
> > E.g. why not put the ucall MMIO range immediately after the so called "default"
> > memory region added at the end of vm_create()?  That way the location of the ucall
> > range is completely predictable, and while still arbitrary, less magical.
> >
> 
> While we do hardcode zero as the guest physical base address, we don't
> require tests to use DEFAULT_GUEST_PHY_PAGES for slot0. They only get
> that if they use vm_create_default* to create the vm. While trying to
> keep the framework flexible for the unit tests does lead to complexity,
> I think the ucall mmio address really needs to be something that can move.

Rats, I had contradicting information in my reply.  Ignore the part about dropping
the param.  My intended suggestion was to dynamically place the ucall range after
the default region, i.e. it would float around, but the relative location is fixed.

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 10a8ed691c66..0ec2de586bf7 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -315,6 +315,8 @@ struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm)
                vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS,
                                            0, 0, phy_pages, 0);

+       ucall_init(0 + <size of default region>);
+
        return vm;
 }

> It's not part of the test setup, i.e. whatever the unit test wants to
> test, it's just part of the framework. It needs to stay out of the way.

Sort of.  In this specific case, I think it's the tests' responsibility to not
stomp over the ucall region as much as it's the framework's responsibility to not
select a conflicting range.  Arch-agnostic tests _can't_ care about absolute
addresses, and we done messed up if we pick a ucall range that is at all
interesting/unique on arm64.

The problem with the current approach is that it's unnecessarily difficult for
either side to do the right thing.  The framework should not have to search
memory regions, and test writers should be given simple (and documented!) rules
for what memory regions are reserved by the framework.