| Message ID | 20231005180325.525236-1-oliver.upton@linux.dev (mailing list archive) |
|---|---|
| Headers | show |
| Series | tools: arm64: Sync sysreg.h with the kernel | expand |
On Thu, 05 Oct 2023 19:03:22 +0100, Oliver Upton <oliver.upton@linux.dev> wrote: > > KVM selftests needs to use the full set of sysreg definitions for an > upcoming change [1]. We took a stab at copying the entire sysreg > generation infrastructure into the tools directory, but that exploded > and broke the build for perf, oops [2]. > > Short of better build infrastructure in tools for handling common > prerequisite tasks, this series takes the lazy route and copies the > generated output of the sysreg infra from the kernel. Hopefully this will spark some interest. In the meantime, this gets us going, so: Acked-by: Marc Zyngier <maz@kernel.org> M.
On Thu, Oct 05, 2023 at 06:03:23PM +0000, Oliver Upton wrote: > Wiring up the build infrastructure necessary to generate the sysreg > definitions for dependent targets (e.g. perf, KVM selftests) is a bit of > an undertaking with near zero benefit. Just take what the kernel > generated instead. > > Cc: Mark Brown <broonie@kernel.org> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > --- > tools/arch/arm64/include/asm/sysreg-defs.h | 6806 ++++++++++++++++++++ > 1 file changed, 6806 insertions(+) > create mode 100644 tools/arch/arm64/include/asm/sysreg-defs.h If we're going to go with this approach we should probably script the syncing of the generated file and ideally have something that detects if there is a generated copy in the main kernel build with differences to what's here. There are regular syncs which I'm guessing are automated somehow, and I see that perf has some tooling to notice differences in the checked in files alraedy. That said I'm not 100% clear why this isn't being added to "make headers" and/or the perf build stuff? Surely if perf is happy to peer into the main kernel source it could just as well do the generation as part of the build? There's no great obstacle to having a target which runs the generation script that I can see.
On Fri, Oct 06, 2023 at 01:23:08AM +0100, Mark Brown wrote: > On Thu, Oct 05, 2023 at 06:03:23PM +0000, Oliver Upton wrote: > > > Wiring up the build infrastructure necessary to generate the sysreg > > definitions for dependent targets (e.g. perf, KVM selftests) is a bit of > > an undertaking with near zero benefit. Just take what the kernel > > generated instead. > > > > Cc: Mark Brown <broonie@kernel.org> > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > > --- > > tools/arch/arm64/include/asm/sysreg-defs.h | 6806 ++++++++++++++++++++ > > 1 file changed, 6806 insertions(+) > > create mode 100644 tools/arch/arm64/include/asm/sysreg-defs.h > > If we're going to go with this approach we should probably script the > syncing of the generated file and ideally have something that detects if > there is a generated copy in the main kernel build with differences to > what's here. There are regular syncs which I'm guessing are automated > somehow, and I see that perf has some tooling to notice differences in > the checked in files alraedy. Yeah, I see no reason why this can't plug in to the check-headers script to look at the output from the kernel side of the show. > That said I'm not 100% clear why this isn't being added to "make > headers" and/or the perf build stuff? Isn't 'make headers' just for UAPI headers though? Also, perf is just an incidental user of this via cputype.h, KVM selftests is what's taking the direct dependency. > Surely if perf is happy to peer into the main kernel source it could > just as well do the generation as part of the build? There's no great > obstacle to having a target which runs the generation script that I > can see. That'd be less than ideal, IMO. tools maintaining a separate set of kernel headers from the authoritative source avoids the need to coordinate changes across kernel and tools. To keep things that way we either need to copy the script+encoding or the header output. The latter isn't that bothersome if/when we need an update.
On Fri, Oct 06, 2023 at 09:23:59AM +0000, Oliver Upton wrote: > On Fri, Oct 06, 2023 at 01:23:08AM +0100, Mark Brown wrote: > > That said I'm not 100% clear why this isn't being added to "make > > headers" and/or the perf build stuff? > Isn't 'make headers' just for UAPI headers though? Also, perf is just an Possibly, I didn't look closely. It'd certainly be more tasteful. > incidental user of this via cputype.h, KVM selftests is what's taking > the direct dependency. If perf doesn't care perhaps just restructure things so it doesn't pull it in and do whatever you were doing originally then? > > Surely if perf is happy to peer into the main kernel source it could > > just as well do the generation as part of the build? There's no great > > obstacle to having a target which runs the generation script that I > > can see. > That'd be less than ideal, IMO. tools maintaining a separate set of kernel > headers from the authoritative source avoids the need to coordinate > changes across kernel and tools. To keep things that way we either need > to copy the script+encoding or the header output. The latter isn't that > bothersome if/when we need an update. The error Stephen found was: | In file included from util/../../arch/arm64/include/asm/cputype.h:201, | from util/arm-spe.c:37: | tools/arch/arm64/include/asm/sysreg.h:132:10: fatal error: asm/sysreg-defs.h: No such file or directory so that's already happening - see perf's arm-spe.c. You could also fix perf by avoiding spelunking in the main kernel source like it's currently doing.
On Fri, Oct 06, 2023 at 12:33:48PM +0100, Mark Brown wrote: > On Fri, Oct 06, 2023 at 09:23:59AM +0000, Oliver Upton wrote: > > On Fri, Oct 06, 2023 at 01:23:08AM +0100, Mark Brown wrote: [...] > > incidental user of this via cputype.h, KVM selftests is what's taking > > the direct dependency. > > If perf doesn't care perhaps just restructure things so it doesn't pull > it in and do whatever you were doing originally then? The only option would be to use yet another set of headers that are specific to KVM selftests, which I feel would only complicate things further. > > > Surely if perf is happy to peer into the main kernel source it could > > > just as well do the generation as part of the build? There's no great > > > obstacle to having a target which runs the generation script that I > > > can see. > > > That'd be less than ideal, IMO. tools maintaining a separate set of kernel > > headers from the authoritative source avoids the need to coordinate > > changes across kernel and tools. To keep things that way we either need > > to copy the script+encoding or the header output. The latter isn't that > > bothersome if/when we need an update. > > The error Stephen found was: > > | In file included from util/../../arch/arm64/include/asm/cputype.h:201, > | from util/arm-spe.c:37: > | tools/arch/arm64/include/asm/sysreg.h:132:10: fatal error: asm/sysreg-defs.h: No such file or directory > > so that's already happening - see perf's arm-spe.c. You could also fix > perf by avoiding spelunking in the main kernel source like it's > currently doing. My interpretation of that relative path is tools/arch/arm64/include/asm/cputype.h The include path in tools/perf/Makefile.config is using tools/ headers as well: INC_FLAGS += -I$(src-perf)/util/include INC_FLAGS += -I$(src-perf)/arch/$(SRCARCH)/include INC_FLAGS += -I$(srctree)/tools/include/ INC_FLAGS += -I$(srctree)/tools/arch/$(SRCARCH)/include/uapi INC_FLAGS += -I$(srctree)/tools/include/uapi INC_FLAGS += -I$(srctree)/tools/arch/$(SRCARCH)/include/ INC_FLAGS += -I$(srctree)/tools/arch/$(SRCARCH)/ perf + KVM selftests aren't directly using kernel headers, and I'd rather not revisit that just for handling the sysreg definitions. So then if we must periodically copy things out of the kernel tree anyway, what value do we derive from copying the script as opposed to just lifting the generated output? We must do _something_ about this, as updates to sysreg.h are blocked until the handling of generated headers gets worked out.
On Fri, Oct 06, 2023 at 07:41:53PM +0000, Oliver Upton wrote: > On Fri, Oct 06, 2023 at 12:33:48PM +0100, Mark Brown wrote: > > On Fri, Oct 06, 2023 at 09:23:59AM +0000, Oliver Upton wrote: > > > On Fri, Oct 06, 2023 at 01:23:08AM +0100, Mark Brown wrote: > > > incidental user of this via cputype.h, KVM selftests is what's taking > > > the direct dependency. > > If perf doesn't care perhaps just restructure things so it doesn't pull > > it in and do whatever you were doing originally then? > The only option would be to use yet another set of headers that are > specific to KVM selftests, which I feel would only complicate things > further. I don't see that it needs to be a different copy? It'd just be perf skipping the generation part of things which AIUI it doesn't actually need. > > | In file included from util/../../arch/arm64/include/asm/cputype.h:201, > > | from util/arm-spe.c:37: > > | tools/arch/arm64/include/asm/sysreg.h:132:10: fatal error: asm/sysreg-defs.h: No such file or directory > > so that's already happening - see perf's arm-spe.c. You could also fix > > perf by avoiding spelunking in the main kernel source like it's > > currently doing. > My interpretation of that relative path is tools/arch/arm64/include/asm/cputype.h Ugh, right - that directory of fun :( It's not exactly copies of the kernel internal headers, it's separate thing that happens to look at lot like them but isn't always the same - most of the files are straight copies but there's also some independent implementations in there. > perf + KVM selftests aren't directly using kernel headers, and I'd rather > not revisit that just for handling the sysreg definitions. So then if we > must periodically copy things out of the kernel tree anyway, what value > do we derive from copying the script as opposed to just lifting the > generated output? The original code was generating things during the build so I'm really confused as to why that's now completely off the table? It does seem like the obvious approach, it feels like I'm missing some of the analysis here. Doing generation makes keeping things in sync marginally easier (you don't have to do do an arch specific build) and the source smaller, and makes it clear that this is actually duplication. > We must do _something_ about this, as updates to sysreg.h are blocked > until the handling of generated headers gets worked out. So then the original code seems like the obvious thing surely, it just put the Makefile fragment doing the generation in the wrong place (or perhaps should have duplicated it in the perf Makefile given that there's no obviously sensible shared place already, it's just a couple of lines)? Or like I said previously tweaked things so that perf isn't pulling in the generated file in the first place and doesn't need to worry about it. TBH when doing generation I'm not sure I see the value in doing a copy in the first place, I'm not sure people ever actually pull the tools directory out of the kernel source and build it independently so we can just skip the effort of keeping a copy in sync entirely. With copying the headers we gain control of exactly which headers tools are looking at, plus there's the cases where the headers diverge. If we need explicit rules to generate and don't expect to ever diverge then we get both properties as part of the generation process with just the duplication of the rules. There might be use cases where tools/ needs to be actually free standing though. In any case I don't think we should just check a raw copy of the generated file in with nothing to automate the drift detection and sync parts of things which is what the current patch does.