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.
Attachment:
signature.asc
Description: PGP signature
