Re: [PATCH v4 09/17] KVM: arm64: Add selftests for the pKVM heap allocator

[Vincent Donnefort <vdonnefort@google.com>]

[...]

> > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > index 346365e76ef2..d99c9b1b0c82 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > @@ -633,6 +633,28 @@ static void handle___pkvm_finalize_teardown_vm(struct kvm_cpu_context *host_ctxt
> >         cpu_reg(host_ctxt, 1) = __pkvm_finalize_teardown_vm(handle);
> >  }
> >
> > +#ifdef CONFIG_NVHE_EL2_DEBUG
> > +static void handle___pkvm_hyp_alloc_selftest(struct kvm_cpu_context *host_ctxt)
> > +{
> > +       struct pkvm_hyp_req req = { .type = PKVM_HYP_NO_REQ };
> > +       int ret;
> > +
> > +       ret = hyp_allocator_selftest();
> > +       if (ret == -ENOMEM) {
> > +               req.type = PKVM_HYP_REQ_HYP_ALLOC_SELFTEST;
> > +               req.mem.nr_pages = hyp_alloc_selftest_topup_needed();
> > +       }
> > +
> > +       cpu_reg(host_ctxt, 1) = ret;
> > +       pkvm_hyp_req_to_smccc(host_ctxt, &req);
> > +}
> > +#else
> > +static void handle___pkvm_hyp_alloc_selftest(struct kvm_cpu_context *host_ctxt)
> > +{
> > +       cpu_reg(host_ctxt, 1) = -EPERM;
> > +}
> > +#endif
> 
> My tag stands, but thought about this while going through Sashiko's reviews.
> 
> The other arm writes x2 through pkvm_hyp_req_to_smccc(), this one does
> not, and -EPERM is an error so pkvm_call_hyp_req() goes and decodes
> x2. Nothing initialises it on a call with no arguments.
> 
> It is inert because pkvm_selftests() sits behind the same #ifdef, but
> the handler is registered either way and nothing states the rule.
> errno_to_smccc(-EPERM, host_ctxt) here would write the zero, and it
> may be worth saying on pkvm_call_hyp_req() that any handler reached
> through it has to set x2.
> 
> Cheers,
> /fuad

Clearly not something that would happen but I can put the #ifdef inside
handle___pkvm_hyp_alloc_selftest() so it looks cleaner:

  static void handle___pkvm_hyp_alloc_selftest(struct kvm_cpu_context *host_ctxt)
  {
          struct pkvm_hyp_req req = { .type = PKVM_HYP_NO_REQ };
          int ret = -EPERM;

  #ifdef CONFIG_NVHE_EL2_DEBUG
          ret = hyp_allocator_selftest();
          if (ret == -ENOMEM) {
                  req.type = PKVM_HYP_REQ_HYP_ALLOC_SELFTEST;
                  req.mem.nr_pages = hyp_alloc_selftest_topup_needed();
          }
  #endif
          cpu_reg(host_ctxt, 1) = ret;
          pkvm_hyp_req_to_smccc(host_ctxt, &req);
  }

Regarding pkvm_call_hyp_req(), how about a proper kerneldoc?

 /**
  * pkvm_call_hyp_req() - Issue an HVC that can return hypervisor requests
  * @f: Hypervisor function symbol to call.
  * @...: Arguments to pass to the hypercall.
  *
  * Re-issue an HVC and process any pending hypervisor request until completion
  * or error.
  *
  * Only use this helper for HVCs whose hypervisor handlers format their return
  * registers with pkvm_hyp_req_to_smccc().
  *
  * Return: Result of the hypercall or a negative error if the hyp request
  * handling failed.
  */

-- 
Vincent