Re: [PATCH 01/50] x86/boot/e820: Introduce arch/x86/include/asm/e820/types.h

[Ingo Molnar]

* Sam Ravnborg <sam@xxxxxxxxxxxx> wrote:

> On Sat, Jan 28, 2017 at 11:11:22PM +0100, Ingo Molnar wrote:
> > 
> > The plan is to keep the old UAPI header in place but the kernel won't
> > use it anymore - and after some time we'll try to remove it. (User-space
> > tools better have local copies of headers anyway, instead of relying
> > on kernel headers.)
> 
> The idea with uapi is the the kernel provides a sane set of headers
> to be used by user space.
> So we avoid random copies that is maintained by random people in random
> ways resulting in random bugs.

Your argument is simplistic which presents a false dichotomy: maintaining a copy 
or fully sharing the header are not the only two options available to share the 
information in the headers between the kernel and tooling: for example perf uses a 
half-automated method where headers are copied from the kernel, but also checked 
automatically against the upstream kernel, and a (non-fatal) warning is emitted 
during the build if the upstream header has changed.

For example today the perf build shows these UAPI header warnings:

 Warning: arch/powerpc/include/uapi/asm/kvm.h differs from kernel
 Warning: arch/arm/include/uapi/asm/kvm.h differs from kernel

... because new bits were added to those two UAPI headers. For example the new ARM 
bits were:

triton:~/tip/tools/perf> diff -up ../arch/arm/include/uapi/asm/kvm.h ../../arch/arm/include/uapi/asm/kvm.h
--- ../arch/arm/include/uapi/asm/kvm.h  2017-01-23 10:10:18.846003002 +0100
+++ ../../arch/arm/include/uapi/asm/kvm.h       2017-01-28 09:35:12.383587930 +0100
@@ -84,6 +84,15 @@ struct kvm_regs {
 #define KVM_VGIC_V2_DIST_SIZE          0x1000
 #define KVM_VGIC_V2_CPU_SIZE           0x2000
 
+/* Supported VGICv3 address types  */
+#define KVM_VGIC_V3_ADDR_TYPE_DIST     2
+#define KVM_VGIC_V3_ADDR_TYPE_REDIST   3
+#define KVM_VGIC_ITS_ADDR_TYPE         4
+
+#define KVM_VGIC_V3_DIST_SIZE          SZ_64K
+#define KVM_VGIC_V3_REDIST_SIZE                (2 * SZ_64K)
+#define KVM_VGIC_V3_ITS_SIZE           (2 * SZ_64K)
+
 #define KVM_ARM_VCPU_POWER_OFF         0 /* CPU is started in OFF state */
 #define KVM_ARM_VCPU_PSCI_0_2          1 /* CPU uses PSCI v0.2 */
 
... so for these changes the perf side copy can be updated safely. Had the changes 
been more intricate, the changes can be copied too - while adopting the tooling 
source code as well.

See the tools/perf/check-headers.sh script.

This IMHO is a far more intelligent and far more robust approach than blind 
sharing or detached copies, because it:

 - forces new changes from upstream to be considered and adapted by tooling

 - header (and thus ABI) synchronization is guaranteed (eventually)

 - it does not actually couple the two source code bases in a rigid fashion:

 - the upstream kernel is free to change those headers (at least from perf's POV)
   in any sane way, those changes can be adapted.

 - every step is conscious and there's no way to accidentally break tooling via
   header changes - nor does tooling hinder the kernel from progressing its source 
   code base.

It's basically a script based COW filesystem with guaranteed propagation and 
guaranteed synchronization.

> The step(s) outlined here can only result in inconsistency and
> cannot benefit neither user space nor the kernel in the long run.

That's simply not true, see above.

> The uapi shall be lean and clean headers, and shall include
> no info whatsoever that is not relevant for user space.

I agree with that characterization, and that will be even more so with my changes: 
my series makes uapi/asm/bootparam.h more self-contained, more lean - while still 
defining the full ABI.

> But requiring all user space programs (diverse libc variants,
> other programs) to maintain their own copy can only result in
> inconsistencies that is the benefit for no one.

That's simply not true, see above.

Note that my changes try to keep the 'UAPI promise' (in that the old e820.h header 
is still around), while still modifying the kernel side.

What _IS_ insane is to somehow construe the UAPI headers as a rigid construct that 
forces the kernel source to keep using poorly chosen names like 'struct e820entry' 
forever...

Thanks,

	Ingo