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

[Sean Christopherson <seanjc@google.com>]

On Fri, Sep 24, 2021, Andrew Jones wrote:
> On Fri, Sep 24, 2021 at 12:09:06AM +0000, Sean Christopherson wrote:
> > On Thu, Sep 23, 2021, Oliver Upton wrote:
> > > While x86 does not require any additional setup to use the ucall
> > > infrastructure, arm64 needs to set up the MMIO address used to signal a
> > > ucall to userspace. rseq_test does not initialize the MMIO address,
> > > resulting in the test spinning indefinitely.
> > > 
> > > Fix the issue by calling ucall_init() during setup.
> > > 
> > > Fixes: 61e52f1630f5 ("KVM: selftests: Add a test for KVM_RUN+rseq to detect task migration bugs")
> > > Signed-off-by: Oliver Upton <oupton@google.com>
> > > ---
> > >  tools/testing/selftests/kvm/rseq_test.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c
> > > index 060538bd405a..c5e0dd664a7b 100644
> > > --- a/tools/testing/selftests/kvm/rseq_test.c
> > > +++ b/tools/testing/selftests/kvm/rseq_test.c
> > > @@ -180,6 +180,7 @@ int main(int argc, char *argv[])
> > >  	 * CPU affinity.
> > >  	 */
> > >  	vm = vm_create_default(VCPU_ID, 0, guest_code);
> > > +	ucall_init(vm, NULL);
> > 
> > Any reason not to do this automatically in vm_create()?  There is 0% chance I'm
> > going to remember to add this next time I write a common selftest, arm64 is the
> > oddball here.

Ugh, reading through arm64's ucall_init(), moving this to vm_create() is a bad
idea.  If a test creates memory regions at hardcoded address, the test could
randomly fail if ucall_init() selects a conflicting address.  More below.

> Yes, please. But, it'll take more than just adding a ucall_init(vm, NULL)
> call to vm_create. We should also modify aarch64's ucall_init to allow
> a *new* explicit mapping to be made. It already allows an explicit mapping
> when arg != NULL, but we'll need to unmap the default mapping first, now.
> The reason is that a unit test may not be happy with the automatically
> selected address (that hasn't happened yet, but...) and want to set its
> own.

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.