[PATCH 0/2] tools/x86: adjust populating of tools/include/xen/

[PATCH 0/2] tools/x86: adjust populating of tools/include/xen/

[Jan Beulich]

While the first change is a possible 4.15 candidate, the second is pure
cleanup (but could, should patch 1 end up being controversial, also be
re-based ahead).

1: don't rebuild cpuid-autogen.h every time
2: move arch-specific include/xen/ population into arch-specific rule

Jan

[PATCH 1/2][4.15?] tools/x86: don't rebuild cpuid-autogen.h every time

[Jan Beulich]

The first thing the "xen-dir" rule does is delete the entire xen/
subtree. Obviously this includes deleting xen/lib/x86/*autogen.h. As a
result there's no original version for $(move-if-changed ...) to compare
against, and hence the file and all its consumers would get rebuilt
every time. Introduce a "prep-y" rule to move xen/lib/x86/ on the side,
to then recover any *autogen.h from there prior to invoking the
respective recursive $(MAKE) invocation.

Fixes: eddf9559c977 ("libx86: generate cpuid-autogen.h in the libx86 include dir")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/.gitignore
+++ b/.gitignore
@@ -234,6 +234,7 @@
 tools/hotplug/NetBSD/rc.d/xendriverdomain
 tools/include/acpi
 tools/include/_libxl*.h
+tools/include/.xen*/*
 tools/include/_xentoolcore_list.h
 tools/include/xen/*
 tools/include/xen-xsm/*
--- a/tools/include/Makefile
+++ b/tools/include/Makefile
@@ -10,15 +10,15 @@ include $(XEN_ROOT)/tools/Rules.mk
 # Relative to $(XEN_ROOT)/xen/xsm/flask
 FLASK_H_DEPEND := policy/initial_sids
 
-.PHONY: all all-y build xen-dir
+.PHONY: all all-y build prep-y xen-dir
 all build: all-y xen-foreign xen-dir xen-xsm/.dir
-all-y:
+all-y prep-y:
 
 .PHONY: xen-foreign
 xen-foreign:
 	$(MAKE) -C xen-foreign
 
-xen-dir:
+xen-dir: prep-y
 	@rm -rf xen acpi
 	mkdir -p xen/libelf acpi
 	ln -s $(XEN_ROOT)/xen/include/public/COPYING xen/
@@ -36,7 +36,15 @@ ifeq ($(CONFIG_X86),y)
 	ln -s $(XEN_ROOT)/xen/include/xen/lib/x86/Makefile xen/lib/x86/
 endif
 
+# Arrange for preserving of auto-generated headers (to avoid them getting
+# rebuilt every time): Move the entire xen/lib/x86/ to a temporary place.
+prep-$(CONFIG_X86):
+	rm -rf .xen-lib-x86
+	test ! -d xen/lib/x86 || mv xen/lib/x86 .xen-lib-x86
+
 all-$(CONFIG_X86): xen-dir
+	$(if $(wildcard .xen-lib-x86/*autogen.h),mv .xen-lib-x86/*autogen.h xen/lib/x86/)
+	rm -rf .xen-lib-x86
 	$(MAKE) -C xen/lib/x86 all XEN_ROOT=$(XEN_ROOT) PYTHON=$(PYTHON)
 
 # Not xen/xsm as that clashes with link to
@@ -78,7 +86,7 @@ uninstall:
 
 .PHONY: clean
 clean:
-	rm -rf xen xen-xsm acpi
+	rm -rf xen xen-xsm .xen* acpi
 	$(MAKE) -C xen-foreign clean
 
 .PHONY: dist

[PATCH 2/2] tools/x86: move arch-specific include/xen/ population into arch-specific rule

[Jan Beulich]

There's no need for the common "xen-dir" rule to have an arch-specific
part when there already is a arch-specific rule where this can be taken
care of.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I was tempted to also uniformly change the pattern from *autogen.h to
*-autogen.h right here - thoughts?

I was also tempted to move the "xen-dir" dependency from
all-$(CONFIG_X86) to the general all-y, but perhaps that's less related
and hence should really be a separate change (and there may also be
arguments against that).

--- a/tools/include/Makefile
+++ b/tools/include/Makefile
@@ -29,12 +29,6 @@ xen-dir: prep-y
 	ln -s ../xen-foreign xen/foreign
 	ln -s $(XEN_ROOT)/xen/include/acpi/platform acpi/
 	ln -s $(XEN_ROOT)/xen/include/acpi/ac*.h acpi/
-ifeq ($(CONFIG_X86),y)
-	ln -s $(XEN_ROOT)/xen/include/asm-x86 xen/asm
-	mkdir -p xen/lib/x86
-	ln -s $(filter-out %autogen.h,$(wildcard $(XEN_ROOT)/xen/include/xen/lib/x86/*.h)) xen/lib/x86/
-	ln -s $(XEN_ROOT)/xen/include/xen/lib/x86/Makefile xen/lib/x86/
-endif
 
 # Arrange for preserving of auto-generated headers (to avoid them getting
 # rebuilt every time): Move the entire xen/lib/x86/ to a temporary place.
@@ -43,6 +37,10 @@ prep-$(CONFIG_X86):
 	test ! -d xen/lib/x86 || mv xen/lib/x86 .xen-lib-x86
 
 all-$(CONFIG_X86): xen-dir
+	ln -s $(XEN_ROOT)/xen/include/asm-x86 xen/asm
+	mkdir -p xen/lib/x86
+	ln -s $(filter-out %autogen.h,$(wildcard $(XEN_ROOT)/xen/include/xen/lib/x86/*.h)) xen/lib/x86/
+	ln -s $(XEN_ROOT)/xen/include/xen/lib/x86/Makefile xen/lib/x86/
 	$(if $(wildcard .xen-lib-x86/*autogen.h),mv .xen-lib-x86/*autogen.h xen/lib/x86/)
 	rm -rf .xen-lib-x86
 	$(MAKE) -C xen/lib/x86 all XEN_ROOT=$(XEN_ROOT) PYTHON=$(PYTHON)

Re: [PATCH 1/2][4.15?] tools/x86: don't rebuild cpuid-autogen.h every time

[Ian Jackson]

Jan Beulich writes ("[PATCH 1/2][4.15?] tools/x86: don't rebuild cpuid-autogen.h every time"):
> The first thing the "xen-dir" rule does is delete the entire xen/
> subtree. Obviously this includes deleting xen/lib/x86/*autogen.h. As a
> result there's no original version for $(move-if-changed ...) to compare
> against, and hence the file and all its consumers would get rebuilt
> every time. Introduce a "prep-y" rule to move xen/lib/x86/ on the side,
> to then recover any *autogen.h from there prior to invoking the
> respective recursive $(MAKE) invocation.

Urgh.  Thanks for working on this swamp.

However,

> +# Arrange for preserving of auto-generated headers (to avoid them getting
> +# rebuilt every time): Move the entire xen/lib/x86/ to a temporary place.
> +prep-$(CONFIG_X86):
> +	rm -rf .xen-lib-x86
> +	test ! -d xen/lib/x86 || mv xen/lib/x86 .xen-lib-x86
> +
>  all-$(CONFIG_X86): xen-dir
> +	$(if $(wildcard .xen-lib-x86/*autogen.h),mv .xen-lib-x86/*autogen.h xen/lib/x86/)
> +	rm -rf .xen-lib-x86
>  	$(MAKE) -C xen/lib/x86 all XEN_ROOT=$(XEN_ROOT) PYTHON=$(PYTHON)

Isn't there some better way of doing this ?  I am very wary of adding
additional on-disk Makefile-managed state to a Makefile which is
already going wrong.  I haven't thought about this in enough detail to
identify a specific bug but I think convincing myself that it is
definitely correct is nontrivial.

Perhaps we could do the removal with a find rune instead, so we can
just skip the files we wanted to keep ?

Ian.

Re: [PATCH 2/2] tools/x86: move arch-specific include/xen/ population into arch-specific rule

[Ian Jackson]

Jan Beulich writes ("[PATCH 2/2] tools/x86: move arch-specific include/xen/ population into arch-specific rule"):
> There's no need for the common "xen-dir" rule to have an arch-specific
> part when there already is a arch-specific rule where this can be taken
> care of.

I think the symlinks being made in this arch-specific code are more
logically part of the xen-dir target than part of the all target.

> I was tempted to also uniformly change the pattern from *autogen.h to
> *-autogen.h right here - thoughts?

I haven't read this in enough detail to know whether that's right, but
if it is right it is IMO preferable.

Thanks,
Ian.

Re: [PATCH 2/2] tools/x86: move arch-specific include/xen/ population into arch-specific rule

[Jan Beulich]

On 08.03.2021 11:00, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH 2/2] tools/x86: move arch-specific include/xen/ population into arch-specific rule"):
>> There's no need for the common "xen-dir" rule to have an arch-specific
>> part when there already is a arch-specific rule where this can be taken
>> care of.
> 
> I think the symlinks being made in this arch-specific code are more
> logically part of the xen-dir target than part of the all target.

But that's true of what has been in all-$(CONFIG_X86) as well, isn't it?
It's all about populating xen/.

Jan

Re: [PATCH 1/2][4.15?] tools/x86: don't rebuild cpuid-autogen.h every time

[Jan Beulich]

On 08.03.2021 10:59, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH 1/2][4.15?] tools/x86: don't rebuild cpuid-autogen.h every time"):
>> The first thing the "xen-dir" rule does is delete the entire xen/
>> subtree. Obviously this includes deleting xen/lib/x86/*autogen.h. As a
>> result there's no original version for $(move-if-changed ...) to compare
>> against, and hence the file and all its consumers would get rebuilt
>> every time. Introduce a "prep-y" rule to move xen/lib/x86/ on the side,
>> to then recover any *autogen.h from there prior to invoking the
>> respective recursive $(MAKE) invocation.
> 
> Urgh.  Thanks for working on this swamp.
> 
> However,
> 
>> +# Arrange for preserving of auto-generated headers (to avoid them getting
>> +# rebuilt every time): Move the entire xen/lib/x86/ to a temporary place.
>> +prep-$(CONFIG_X86):
>> +	rm -rf .xen-lib-x86
>> +	test ! -d xen/lib/x86 || mv xen/lib/x86 .xen-lib-x86
>> +
>>  all-$(CONFIG_X86): xen-dir
>> +	$(if $(wildcard .xen-lib-x86/*autogen.h),mv .xen-lib-x86/*autogen.h xen/lib/x86/)
>> +	rm -rf .xen-lib-x86
>>  	$(MAKE) -C xen/lib/x86 all XEN_ROOT=$(XEN_ROOT) PYTHON=$(PYTHON)
> 
> Isn't there some better way of doing this ?  I am very wary of adding
> additional on-disk Makefile-managed state to a Makefile which is
> already going wrong.  I haven't thought about this in enough detail to
> identify a specific bug but I think convincing myself that it is
> definitely correct is nontrivial.
> 
> Perhaps we could do the removal with a find rune instead, so we can
> just skip the files we wanted to keep ?

Maybe, and I did consider the option, but it would have felt more
fragile to me than this dedicated keep-just-the-few-files approach.
The problems we've had with this symlinking don't make me confident
in leaving around parts of this subtree; populating from scratch
seems like the most robust model (short of the suggested but never
carried out removal of the symlinking) to me.

Jan

Re: [PATCH 1/2][4.15?] tools/x86: don't rebuild cpuid-autogen.h every time

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 1/2][4.15?] tools/x86: don't rebuild cpuid-autogen.h every time"):
> On 08.03.2021 10:59, Ian Jackson wrote:
> > Jan Beulich writes ("[PATCH 1/2][4.15?] tools/x86: don't rebuild cpuid-autogen.h every time"):
> >> +# Arrange for preserving of auto-generated headers (to avoid them getting
> >> +# rebuilt every time): Move the entire xen/lib/x86/ to a temporary place.
> >> +prep-$(CONFIG_X86):
> >> +	rm -rf .xen-lib-x86
> >> +	test ! -d xen/lib/x86 || mv xen/lib/x86 .xen-lib-x86
> >> +
> >>  all-$(CONFIG_X86): xen-dir
> >> +	$(if $(wildcard .xen-lib-x86/*autogen.h),mv .xen-lib-x86/*autogen.h xen/lib/x86/)
> >> +	rm -rf .xen-lib-x86
> >>  	$(MAKE) -C xen/lib/x86 all XEN_ROOT=$(XEN_ROOT) PYTHON=$(PYTHON)
> > 
> > Isn't there some better way of doing this ?  I am very wary of adding
> > additional on-disk Makefile-managed state to a Makefile which is
> > already going wrong.  I haven't thought about this in enough detail to
> > identify a specific bug but I think convincing myself that it is
> > definitely correct is nontrivial.
> > 
> > Perhaps we could do the removal with a find rune instead, so we can
> > just skip the files we wanted to keep ?
> 
> Maybe, and I did consider the option, but it would have felt more
> fragile to me than this dedicated keep-just-the-few-files approach.
> The problems we've had with this symlinking don't make me confident
> in leaving around parts of this subtree; populating from scratch
> seems like the most robust model (short of the suggested but never
> carried out removal of the symlinking) to me.

I'm confused by your reply.

You aren't confident "leaving around parts of this subtree" but you
are happy to move it aside and put it back, which seems equivalent.  I
don't understand why you think the latter would be more reliable.

It seems to me that a find rune which deletes individual files can be
at least as specific as your current wildcard and mv approach.

Thanks,
Ian.

Re: [PATCH 1/2][4.15?] tools/x86: don't rebuild cpuid-autogen.h every time

[Jan Beulich]

On 08.03.2021 12:08, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH 1/2][4.15?] tools/x86: don't rebuild cpuid-autogen.h every time"):
>> On 08.03.2021 10:59, Ian Jackson wrote:
>>> Jan Beulich writes ("[PATCH 1/2][4.15?] tools/x86: don't rebuild cpuid-autogen.h every time"):
>>>> +# Arrange for preserving of auto-generated headers (to avoid them getting
>>>> +# rebuilt every time): Move the entire xen/lib/x86/ to a temporary place.
>>>> +prep-$(CONFIG_X86):
>>>> +	rm -rf .xen-lib-x86
>>>> +	test ! -d xen/lib/x86 || mv xen/lib/x86 .xen-lib-x86
>>>> +
>>>>  all-$(CONFIG_X86): xen-dir
>>>> +	$(if $(wildcard .xen-lib-x86/*autogen.h),mv .xen-lib-x86/*autogen.h xen/lib/x86/)
>>>> +	rm -rf .xen-lib-x86
>>>>  	$(MAKE) -C xen/lib/x86 all XEN_ROOT=$(XEN_ROOT) PYTHON=$(PYTHON)
>>>
>>> Isn't there some better way of doing this ?  I am very wary of adding
>>> additional on-disk Makefile-managed state to a Makefile which is
>>> already going wrong.  I haven't thought about this in enough detail to
>>> identify a specific bug but I think convincing myself that it is
>>> definitely correct is nontrivial.
>>>
>>> Perhaps we could do the removal with a find rune instead, so we can
>>> just skip the files we wanted to keep ?
>>
>> Maybe, and I did consider the option, but it would have felt more
>> fragile to me than this dedicated keep-just-the-few-files approach.
>> The problems we've had with this symlinking don't make me confident
>> in leaving around parts of this subtree; populating from scratch
>> seems like the most robust model (short of the suggested but never
>> carried out removal of the symlinking) to me.
> 
> I'm confused by your reply.
> 
> You aren't confident "leaving around parts of this subtree" but you
> are happy to move it aside and put it back, which seems equivalent.  I
> don't understand why you think the latter would be more reliable.

I move a subdir aside and then move certain files out of it back.
This doesn't leave much uncertainty as to whether other (symlinked)
files / dirs may mistakenly also left around.

> It seems to me that a find rune which deletes individual files can be
> at least as specific as your current wildcard and mv approach.

Possibly, but it may end up being more complex: We want to only
retain files of specific names from a single dir. I don't think
this is as straightforward to express in a find rune. Of course
I'll be fine whichever way the bug gets fixed, but I'm afraid I
don't feel convinced I want to put time into trying the alternative
you suggest. If otoh you wanted to try out yours and it turned out
equivalent or better, I wouldn't mind at all.

Jan

Re: [PATCH 1/2][4.15?] tools/x86: don't rebuild cpuid-autogen.h every time

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 1/2][4.15?] tools/x86: don't rebuild cpuid-autogen.h every time"):
> Possibly, but it may end up being more complex: We want to only
> retain files of specific names from a single dir. I don't think
> this is as straightforward to express in a find rune. Of course
> I'll be fine whichever way the bug gets fixed, but I'm afraid I
> don't feel convinced I want to put time into trying the alternative
> you suggest. If otoh you wanted to try out yours and it turned out
> equivalent or better, I wouldn't mind at all.

Untested, but I think something like this should DTRT

 find xen ! -type d ! -path 'xen/lib/x86/*-autogen.h' -print0 | xargs -0r rm --

Since my background is GNU utilities, I checked the FreeBSD manpages
for find, xargs and rm.  They support these options.

This will leave the entire directory structure but I think that is
fine.  The xen-dir target uses mkdir -p and should there be any stale
directories (eg due to switching branches or whatever) they wouldn't
be a problem.

Ian.

Re: [PATCH 1/2][4.15?] tools/x86: don't rebuild cpuid-autogen.h every time

[Jan Beulich]

On 08.03.2021 13:12, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH 1/2][4.15?] tools/x86: don't rebuild cpuid-autogen.h every time"):
>> Possibly, but it may end up being more complex: We want to only
>> retain files of specific names from a single dir. I don't think
>> this is as straightforward to express in a find rune. Of course
>> I'll be fine whichever way the bug gets fixed, but I'm afraid I
>> don't feel convinced I want to put time into trying the alternative
>> you suggest. If otoh you wanted to try out yours and it turned out
>> equivalent or better, I wouldn't mind at all.
> 
> Untested, but I think something like this should DTRT
> 
>  find xen ! -type d ! -path 'xen/lib/x86/*-autogen.h' -print0 | xargs -0r rm --
> 
> Since my background is GNU utilities, I checked the FreeBSD manpages
> for find, xargs and rm.  They support these options.
> 
> This will leave the entire directory structure but I think that is
> fine.  The xen-dir target uses mkdir -p and should there be any stale
> directories (eg due to switching branches or whatever) they wouldn't
> be a problem.

Right. Thinking of it though - all we do is setting up symlinks
plus produce this generated header. Couldn't we therefore have
find simply arrange for all symlinks to be found and deleted?

Jan

Re: [PATCH 1/2][4.15?] tools/x86: don't rebuild cpuid-autogen.h every time

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 1/2][4.15?] tools/x86: don't rebuild cpuid-autogen.h every time"):
> On 08.03.2021 13:12, Ian Jackson wrote:
> > This will leave the entire directory structure but I think that is
> > fine.  The xen-dir target uses mkdir -p and should there be any stale
> > directories (eg due to switching branches or whatever) they wouldn't
> > be a problem.
> 
> Right. Thinking of it though - all we do is setting up symlinks
> plus produce this generated header. Couldn't we therefore have
> find simply arrange for all symlinks to be found and deleted?

I think that would be another alternative, if there is nothing else in
there but symlinks.

Ian.