[PATCH 1/2] ARM: hyp-stub: improve ABI

On Sat, Dec 17, 2016 at 12:07:11PM +0000, Catalin Marinas wrote:

On Thu, Dec 15, 2016 at 06:57:18PM +0000, Russell King - ARM Linux wrote:

quoted

On Thu, Dec 15, 2016 at 03:37:15PM +0000, Marc Zyngier wrote:

quoted

And this interface exists for the sole purpose of enabling KVM. Call it
a hypervisor if you wish, but its usefulness is doubtful on its own.

IMO, the interface between EL1 and EL2 _is_ ABI. However, it should not
be confused with a *stable* ABI like the one we expose to user. Since
all users of the EL2 ABI live inside the kernel (either on the EL1 or
EL2 side), we are free to change it as long as everything is updated
simultaneously. I don't see this any different from other in-kernel ABI
like that exposed to loadable modules (for the latter, we have the
advantage of a partly self-documenting ABI as part of the C language).

I would welcome some documentation for the EL2 ABI as well, especially
since head.S and the KVM counterpart is maintained by different people.

Well, it seems that different people within ARM have different opinions
on this subject - that makes it extremely hard to make progress on this.

As you're all local to each other, please can you hash it out amongst
yourselves and come to some sort of concensus!

quoted

What's also coming clear is that there's very few people who understand
all the interactions here, and the whole thing seems to be an undocumented
mess.  Something needs to change there - people need to stop shovelling
half baked crap into the kernel.  KVM have the privilege of being able to
push ARM stuff directly, I now see that was a very big mistake.

So, I want KVM further changes to come through my tree once this merge
window is over -

I'm afraid I strongly disagree. There are a few reasons neither you nor
me gatekeep the KVM port: (a) the ARM KVM maintainers know a lot more
about KVM than either of us, (b) the KVM code under arch/arm is reused
by arm64 and (c) you or I would certainly become a bottleneck. There is
a lot more to KVM support on ARM than the hyp stub and realistically you
won't have the time to review all the stuff that comes your way.

I don't think so - in general the statistics for arch/arm/kvm are very
quiet - there's a bigger update once in a while:

v4.1:  8 files changed, 281 insertions(+), 150 deletions(-)
v4.2:  7 files changed, 56 insertions(+), 38 deletions(-)
v4.3:  8 files changed, 59 insertions(+), 41 deletions(-)
v4.4:  6 files changed, 95 insertions(+), 50 deletions(-)
v4.5:  6 files changed, 97 insertions(+), 50 deletions(-)
v4.6:  22 files changed, 1203 insertions(+), 1307 deletions(-)
v4.7:  5 files changed, 351 insertions(+), 262 deletions(-)
v4.8:  9 files changed, 134 insertions(+), 163 deletions(-)
v4.9:  12 files changed, 209 insertions(+), 159 deletions(-)

In terms of number of commits:

v4.1: 17
v4.2: 14
v4.3: 15
v4.4: 13
v4.5: 9
v4.6: 48
v4.7: 18
v4.8: 22
v4.9: 17

So there isn't much going on there.

quoted

Let's start with some documentation on the KVM hypervisor interfaces on
32-bit ARM.  Documentation/virtual/kvm/hypercalls.txt already contains
some for x86, s390 and PPC, so that would be a good place to document
the ARM side.  Arguably, that should have already been done.

I'm all for documenting the interface.

(even though Will and I co-maintain arch/arm64, I deliberately kept his
name out of the above as I haven't had the chance to ask for his
opinion on this matter, which may as well differ)

Well, I started discussing this with Will before I produced the patch,
and Will thought that the ABI was entirely localised within hyp-stub.S.

This is exactly my point: very few people seem to have a proper
understanding of this (as illustrated by the number of different
opinions people appear to hold over various parts of the code), which
makes it very difficult for problems to get fixed.

Either we need more people to have an understanding (so if one of them
gets run over by a bus, we're not left floundering around) or we need
it to be documented - even if it's just a simple comment "the ABI in
this file is shared with XYZ, if you change the ABI here, also update
XYZ too."

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.