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

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

[Oliver Upton]

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);
 
 	pthread_create(&migration_thread, NULL, migration_worker, 0);
 
-- 
2.33.0.685.g46640cef36-goog

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

[Sean Christopherson]

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.

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

[Oliver Upton]

On Thu, Sep 23, 2021 at 5:09 PM Sean Christopherson <seanjc@google.com> 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.

I think that is best, I was planning on sending out a fix that does
this later on. Just wanted to stop the bleeding with a minimal patch
first.

--
Thanks,
Oliver

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

[Paolo Bonzini]

On 24/09/21 02:10, Oliver Upton wrote:
>>>         */
>>>        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.
> I think that is best, I was planning on sending out a fix that does
> this later on. Just wanted to stop the bleeding with a minimal patch
> first.

For now I've queued it, for 5.16 we can do it in vm_create().

Paolo

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

[Andrew Jones]

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.
>

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.

Thanks,
drew

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

[Sean Christopherson]

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.

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

[Andrew Jones]

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@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.
>

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

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

[Sean Christopherson]

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.

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

[Paolo Bonzini]

On 24/09/21 00:00, 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);
>   
>   	pthread_create(&migration_thread, NULL, migration_worker, 0);
>   
> 

Queued, thanks.

Paolo