Re: [PATCH v7 0/9] KVM: arm64: Add support for hypercall services selection

[Raghavendra Rao Ananta]

On Tue, May 3, 2022 at 1:33 PM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Tue, 03 May 2022 19:49:13 +0100,
> Raghavendra Rao Ananta <rananta@xxxxxxxxxx> wrote:
> >
> > Hi Marc,
> >
> > On Tue, May 3, 2022 at 10:24 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > >
> > > On Tue, 03 May 2022 00:38:44 +0100,
> > > Raghavendra Rao Ananta <rananta@xxxxxxxxxx> wrote:
> > > >
> > > > Hello,
> > > >
> > > > Continuing the discussion from [1], the series tries to add support
> > > > for the userspace to elect the hypercall services that it wishes
> > > > to expose to the guest, rather than the guest discovering them
> > > > unconditionally. The idea employed by the series was taken from
> > > > [1] as suggested by Marc Z.
> > >
> > > As it took some time to get there, and that there was still a bunch of
> > > things to address, I've taken the liberty to apply my own fixes to the
> > > series.
> > >
> > > Please have a look at [1], and let me know if you're OK with the
> > > result. If you are, I'll merge the series for 5.19.
> > >
> > > Thanks,
> > >
> > >         M.
> > >
> > Thank you for speeding up the process; appreciate it. However, the
> > series's selftest patches have a dependency on Oliver's
> > PSCI_SYSTEM_SUSPEND's selftest patches [1][2]. Can we pull them in
> > too?
>
> Urgh... I guess this is the time to set some ground rules:
>
> - Please don't introduce dependencies between series, that's
>   unmanageable. I really need to see each series independently, and if
>   there is a merge conflict, that's my job to fix (and I don't really
>   mind).
>
> - If there is a dependency between series, please post a version of
>   the required patches as a prefix to your series, assuming this
>   prefix is itself standalone. If it isn't, then something really is
>   wrong, and the series should be resplit.
>
> - You also should be basing your series on an *official* tag from
>   Linus' tree (preferably -rc1, -rc2 or -rc3), and not something
>   random like any odd commit from the KVM tree (I had conflicts while
>   applying this on -rc3, probably due to the non-advertised dependency
>   on Oliver's series).
>
Thanks for picking the dependency patches. I'll keep these mind the
next time I push changes.

> >
> > aarch64/hypercalls.c: In function ‘guest_test_hvc’:
> > aarch64/hypercalls.c:95:30: error: storage size of ‘res’ isn’t known
> >    95 |         struct arm_smccc_res res;
> >       |                              ^~~
> > aarch64/hypercalls.c:103:17: warning: implicit declaration of function
> > ‘smccc_hvc’ [-Wimplicit-function-declaration]
> >   103 |                 smccc_hvc(hc_info->func_id, hc_info->arg1, 0,
> > 0, 0, 0, 0, 0, &res);
> >       |                 ^~~~~~~~~
> >
>
> I've picked the two patches, which means they will most likely appear
> twice in the history. In the future, please reach out so that we can
> organise this better.
>
> > Also, just a couple of readability nits in the fixed version:
> >
> > 1. Patch-2/9, hypercall.c:kvm_hvc_call_default_allowed(), in the
> > 'default' case, do you think we should probably add a small comment
> > that mentions we are checking for func_id in the PSCI range?
>
> Dumped a one-liner there.
>
> > 2. Patch-2/9, arm_hypercall.h, clear all the macros in this patch
> > itself instead of doing it in increments (unless there's some reason
> > that I'm missing)?
>
> Ah, rebasing leftovers, now gone.
>
> I've pushed an updated branch again, please have a look.
>
Thanks for addressing these. The series looks good now.

Regards,
Raghavendra
>         M.
>
> --
> Without deviation from the norm, progress is not possible.