On Mon, Sep 27, 2021 at 08:05:14PM +0000, Sean Christopherson wrote:
> 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@xxxxxxxxxx>
> > > > ---
> > > > 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.
>
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.
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.
You're right that we can't improve things by adding ucall_init to
vm_create though, even if we added my suggestion of changing ucall_init
to be an unmap-old, map-new type of thing, since we'd still always need
the deferred ucall_init call to be sure we got it right.
We could replace the mmio address search with a hardcoded default address
though, probably the start address of the search (most likely that's the
one that's always used). Then, if there's a problem with that address,
an explicit ucall_init with its optional argument would need to be
provided in the unit test, along with a comment explaining why it's
there to ensure nobody removes it.
Thanks,
drew
