kernel selftest max_guest_memory_test fails when using more that 256 vCPUs

kernel selftest max_guest_memory_test fails when using more that 256 vCPUs

[mlevitsk]

Hi,

Recently I debugged a failure of this selftest and this is what is happening:

For each vCPU this test runs the guest till it does the ucall, then it resets
all the vCPU registers to their initial values (including RIP) and runs the guest again.
I don't know if this is needed.

What happens however is that ucall code allocates the ucall struct prior to calling the host,
and then expects the host to resume the guest, at which point the guest frees the struct.

However since the host manually resets the guest registers, the code that frees the ucall struct
is never reached and thus the ucall struct is leaked.

Currently ucall code has a pool of KVM_MAX_VCPUS (512) objects, thus if the test is run with more
than 256 vCPUs, the pool is exhausted and the test fails.

So either we need to:
  - add a way to manually free the ucall struct for such tests from the host side.
  - remove the manual reset of the vCPUs register state from this test and instead put the guest code
    in while(1) {} loop.

  - refactor the ucall code to not rely on a fixed pool of structs, making it possible to tolerate
    small memory leaks like that (I don't like this to be honest).


What do you suggest to do? (I will send a patch after I hear your opinion).

Best regards,
	Maxim Levitsky

Re: kernel selftest max_guest_memory_test fails when using more that 256 vCPUs

[Sean Christopherson]

On Mon, Mar 11, 2024, mlevitsk@redhat.com wrote:
> Hi,
> 
> Recently I debugged a failure of this selftest and this is what is happening:
> 
> For each vCPU this test runs the guest till it does the ucall, then it resets
> all the vCPU registers to their initial values (including RIP) and runs the guest again.
> I don't know if this is needed.
> 
> What happens however is that ucall code allocates the ucall struct prior to calling the host,
> and then expects the host to resume the guest, at which point the guest frees the struct.
> 
> However since the host manually resets the guest registers, the code that frees the ucall struct
> is never reached and thus the ucall struct is leaked.
> 
> Currently ucall code has a pool of KVM_MAX_VCPUS (512) objects, thus if the test is run with more
> than 256 vCPUs, the pool is exhausted and the test fails.
> 
> So either we need to:
>   - add a way to manually free the ucall struct for such tests from the host side.

Part of me wants to do something along these lines, as every GUEST_DONE() and
failed GUEST_ASSERT() is "leaking" a ucall structure.  But practically speaking,
freeing a ucall structure from anywhere except the vCPU context is bound to cause
more problems than it solves.

>   - remove the manual reset of the vCPUs register state from this test and
>   instead put the guest code in while(1) {} loop.

Definitely this one.  IIRC, the only reason I stuffed registers in the test was
because I was trying to force MMU reloads.  I can't think of any reason why a
simple infinite loop in the guest wouldn't work.  I'm pretty sure this is all
that's needed?

diff --git a/tools/testing/selftests/kvm/max_guest_memory_test.c b/tools/testing/selftests/kvm/max_guest_memory_test.c
index 6628dc4dda89..5f9950f41313 100644
--- a/tools/testing/selftests/kvm/max_guest_memory_test.c
+++ b/tools/testing/selftests/kvm/max_guest_memory_test.c
@@ -22,10 +22,12 @@ static void guest_code(uint64_t start_gpa, uint64_t end_gpa, uint64_t stride)
 {
        uint64_t gpa;
 
-       for (gpa = start_gpa; gpa < end_gpa; gpa += stride)
-               *((volatile uint64_t *)gpa) = gpa;
+       for (;;) {
+               for (gpa = start_gpa; gpa < end_gpa; gpa += stride)
+                       *((volatile uint64_t *)gpa) = gpa;
 
-       GUEST_DONE();
+               GUEST_DONE();
+       }
 }
 
 struct vcpu_info {
@@ -64,17 +66,12 @@ static void *vcpu_worker(void *data)
        struct kvm_vcpu *vcpu = info->vcpu;
        struct kvm_vm *vm = vcpu->vm;
        struct kvm_sregs sregs;
-       struct kvm_regs regs;
 
        vcpu_args_set(vcpu, 3, info->start_gpa, info->end_gpa, vm->page_size);
-
-       /* Snapshot regs before the first run. */
-       vcpu_regs_get(vcpu, &regs);
        rendezvous_with_boss();
 
        run_vcpu(vcpu);
        rendezvous_with_boss();
-       vcpu_regs_set(vcpu, &regs);
        vcpu_sregs_get(vcpu, &sregs);
 #ifdef __x86_64__
        /* Toggle CR0.WP to trigger a MMU context reset. */


>   - refactor the ucall code to not rely on a fixed pool of structs, making it
>   possible to tolerate small memory leaks like that (I don't like this to be
>   honest).

Heh, me neither.

Re: kernel selftest max_guest_memory_test fails when using more that 256 vCPUs

[Andrew Jones]

On Mon, Mar 11, 2024 at 05:05:18PM -0700, Sean Christopherson wrote:
> On Mon, Mar 11, 2024, mlevitsk@redhat.com wrote:
> > Hi,
> > 
> > Recently I debugged a failure of this selftest and this is what is happening:
> > 
> > For each vCPU this test runs the guest till it does the ucall, then it resets
> > all the vCPU registers to their initial values (including RIP) and runs the guest again.
> > I don't know if this is needed.
> > 
> > What happens however is that ucall code allocates the ucall struct prior to calling the host,
> > and then expects the host to resume the guest, at which point the guest frees the struct.
> > 
> > However since the host manually resets the guest registers, the code that frees the ucall struct
> > is never reached and thus the ucall struct is leaked.
> > 
> > Currently ucall code has a pool of KVM_MAX_VCPUS (512) objects, thus if the test is run with more
> > than 256 vCPUs, the pool is exhausted and the test fails.
> > 
> > So either we need to:
> >   - add a way to manually free the ucall struct for such tests from the host side.
> 
> Part of me wants to do something along these lines, as every GUEST_DONE() and
> failed GUEST_ASSERT() is "leaking" a ucall structure.  But practically speaking,
> freeing a ucall structure from anywhere except the vCPU context is bound to cause
> more problems than it solves.

Yes, ideally the host could clobber guest registers, or do whatever it
likes, without having to consider how it impacts the guest's ability
to manage the test. I.e. the guest code should be more the "software
under test" than the "test software", but kvm selftests blurs the line
between test code and tested code all the time, so freeing ucall objects
is just one more case of that.

> 
> >   - remove the manual reset of the vCPUs register state from this test and
> >   instead put the guest code in while(1) {} loop.
> 
> Definitely this one.

I agree.

> IIRC, the only reason I stuffed registers in the test was
> because I was trying to force MMU reloads.  I can't think of any reason why a
> simple infinite loop in the guest wouldn't work.  I'm pretty sure this is all
> that's needed?
> 
> diff --git a/tools/testing/selftests/kvm/max_guest_memory_test.c b/tools/testing/selftests/kvm/max_guest_memory_test.c
> index 6628dc4dda89..5f9950f41313 100644
> --- a/tools/testing/selftests/kvm/max_guest_memory_test.c
> +++ b/tools/testing/selftests/kvm/max_guest_memory_test.c
> @@ -22,10 +22,12 @@ static void guest_code(uint64_t start_gpa, uint64_t end_gpa, uint64_t stride)
>  {
>         uint64_t gpa;
>  
> -       for (gpa = start_gpa; gpa < end_gpa; gpa += stride)
> -               *((volatile uint64_t *)gpa) = gpa;
> +       for (;;) {
> +               for (gpa = start_gpa; gpa < end_gpa; gpa += stride)
> +                       *((volatile uint64_t *)gpa) = gpa;
>  
> -       GUEST_DONE();
> +               GUEST_DONE();

I'd change this to a GUEST_SYNC(0), since the infinite loop otherwise
contradicts the "done-ness".

Thanks,
drew

Re: kernel selftest max_guest_memory_test fails when using more that 256 vCPUs

[Sean Christopherson]

On Tue, Mar 12, 2024, Andrew Jones wrote:
> On Mon, Mar 11, 2024 at 05:05:18PM -0700, Sean Christopherson wrote:
> > On Mon, Mar 11, 2024, mlevitsk@redhat.com wrote:
> > diff --git a/tools/testing/selftests/kvm/max_guest_memory_test.c b/tools/testing/selftests/kvm/max_guest_memory_test.c
> > index 6628dc4dda89..5f9950f41313 100644
> > --- a/tools/testing/selftests/kvm/max_guest_memory_test.c
> > +++ b/tools/testing/selftests/kvm/max_guest_memory_test.c
> > @@ -22,10 +22,12 @@ static void guest_code(uint64_t start_gpa, uint64_t end_gpa, uint64_t stride)
> >  {
> >         uint64_t gpa;
> >  
> > -       for (gpa = start_gpa; gpa < end_gpa; gpa += stride)
> > -               *((volatile uint64_t *)gpa) = gpa;
> > +       for (;;) {
> > +               for (gpa = start_gpa; gpa < end_gpa; gpa += stride)
> > +                       *((volatile uint64_t *)gpa) = gpa;
> >  
> > -       GUEST_DONE();
> > +               GUEST_DONE();
> 
> I'd change this to a GUEST_SYNC(0), since the infinite loop otherwise
> contradicts the "done-ness".

Eh, the guest is "done" with an iteration/run.  :-)

I don't have a strong preference, I'm just biased against GUEST_SYNC() in general,
as tests that heavily use GUEST_SYNC() tend to be ridiculously hard to follow/debug.