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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <878rriicez.wl-maz@kernel.org>
Date:   Tue, 03 May 2022 21:33:40 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Raghavendra Rao Ananta <rananta@...gle.com>
Cc:     Andrew Jones <drjones@...hat.com>,
        James Morse <james.morse@....com>,
        Alexandru Elisei <alexandru.elisei@....com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, Peter Shier <pshier@...gle.com>,
        Ricardo Koller <ricarkol@...gle.com>,
        Oliver Upton <oupton@...gle.com>,
        Reiji Watanabe <reijiw@...gle.com>,
        Jing Zhang <jingzhangos@...gle.com>,
        linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.cs.columbia.edu,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH v7 0/9] KVM: arm64: Add support for hypercall services selection

On Tue, 03 May 2022 19:49:13 +0100,
Raghavendra Rao Ananta <rananta@...gle.com> wrote:
> 
> Hi Marc,
> 
> On Tue, May 3, 2022 at 10:24 AM Marc Zyngier <maz@...nel.org> wrote:
> >
> > On Tue, 03 May 2022 00:38:44 +0100,
> > Raghavendra Rao Ananta <rananta@...gle.com> 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).

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

	M.

-- 
Without deviation from the norm, progress is not possible.