diff mbox series

[XEN,v3,04/25] tools/firmware/hvmloader: rework Makefile

Message ID 20220624160422.53457-5-anthony.perard@citrix.com (mailing list archive)
State Superseded
Headers show
Series Toolstack build system improvement, toward non-recursive makefiles | expand

Commit Message

Anthony PERARD June 24, 2022, 4:04 p.m. UTC 
Setup proper dependencies with libacpi so we don't need to run "make
hvmloader" in the "all" target. ("build.o" new prerequisite isn't
exactly proper but a side effect of building the $(DSDT_FILES) is to
generate the "ssdt_*.h" needed by "build.o".)

Make use if "-iquote" instead of a plain "-I".

For "roms.inc" target, use "$(SHELL)" instead of plain "sh". And use
full path to "mkhex" instead of a relative one. Lastly, add "-f" flag
to "mv", in case the target already exist.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/firmware/hvmloader/Makefile | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Comments

Luca Fancellu July 8, 2022, 3:39 p.m. UTC | #1 
> On 24 Jun 2022, at 17:04, Anthony PERARD <anthony.perard@citrix.com> wrote:
> 
> Setup proper dependencies with libacpi so we don't need to run "make
> hvmloader" in the "all" target. ("build.o" new prerequisite isn't
> exactly proper but a side effect of building the $(DSDT_FILES) is to
> generate the "ssdt_*.h" needed by "build.o".)
> 
> Make use if "-iquote" instead of a plain "-I".
> 
> For "roms.inc" target, use "$(SHELL)" instead of plain "sh". And use
> full path to "mkhex" instead of a relative one. Lastly, add "-f" flag
> to "mv", in case the target already exist.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> tools/firmware/hvmloader/Makefile | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
> index b754220839..fc20932110 100644
> --- a/tools/firmware/hvmloader/Makefile
> +++ b/tools/firmware/hvmloader/Makefile
> @@ -60,8 +60,7 @@ ROMS += $(ROMBIOS_ROM) $(STDVGA_ROM) $(CIRRUSVGA_ROM)
> endif
> 
> .PHONY: all
> -all: acpi
> -	$(MAKE) hvmloader
> +all: hvmloader
> 
> .PHONY: acpi
> acpi:
> @@ -73,12 +72,15 @@ smbios.o: CFLAGS += -D__SMBIOS_DATE__="\"$(SMBIOS_REL_DATE)\""
> ACPI_PATH = ../../libacpi
> DSDT_FILES = dsdt_anycpu.c dsdt_15cpu.c dsdt_anycpu_qemu_xen.c
> ACPI_OBJS = $(patsubst %.c,%.o,$(DSDT_FILES)) build.o static_tables.o
> -$(ACPI_OBJS): CFLAGS += -I. -DLIBACPI_STDUTILS=\"$(CURDIR)/util.h\"
> +$(ACPI_OBJS): CFLAGS += -iquote . -DLIBACPI_STDUTILS=\"$(CURDIR)/util.h\"
> CFLAGS += -I$(ACPI_PATH)
> vpath build.c $(ACPI_PATH)
> vpath static_tables.c $(ACPI_PATH)
> OBJS += $(ACPI_OBJS)
> 
> +$(DSDT_FILES): acpi
> +build.o: $(DSDT_FILES)
> +
> hvmloader: $(OBJS) hvmloader.lds
> 	$(LD) $(LDFLAGS_DIRECT) -N -T hvmloader.lds -o $@ $(OBJS)
> 
> @@ -87,21 +89,21 @@ roms.inc: $(ROMS)
> 
> ifneq ($(ROMBIOS_ROM),)
> 	echo "#ifdef ROM_INCLUDE_ROMBIOS" >> $@.new
> -	sh ../../misc/mkhex rombios $(ROMBIOS_ROM) >> $@.new
> +	$(SHELL) $(XEN_ROOT)/tools/misc/mkhex rombios $(ROMBIOS_ROM) >> $@.new
> 	echo "#endif" >> $@.new
> endif
> 
> ifneq ($(STDVGA_ROM),)
> 	echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
> -	sh ../../misc/mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
> +	$(SHELL) $(XEN_ROOT)/tools/misc/mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
> 	echo "#endif" >> $@.new
> endif
> ifneq ($(CIRRUSVGA_ROM),)
> 	echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
> -	sh ../../misc/mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
> +	$(SHELL) $(XEN_ROOT)/tools/misc/mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
> 	echo "#endif" >> $@.new
> endif
> -	mv $@.new $@
> +	mv -f $@.new $@

Here, instead of -f, is it safer -u? What’s your opinion on that? The patch looks good to me.

> 
> .PHONY: clean
> clean:
> -- 
> Anthony PERARD
> 
>
Anthony PERARD July 11, 2022, 1:38 p.m. UTC | #2 
On Fri, Jul 08, 2022 at 03:39:00PM +0000, Luca Fancellu wrote:
> > On 24 Jun 2022, at 17:04, Anthony PERARD <anthony.perard@citrix.com> wrote:
[...]
> > For "roms.inc" target, use "$(SHELL)" instead of plain "sh". And use
> > full path to "mkhex" instead of a relative one. Lastly, add "-f" flag
> > to "mv", in case the target already exist.
> > 
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> > diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
> > index b754220839..fc20932110 100644
> > --- a/tools/firmware/hvmloader/Makefile
> > +++ b/tools/firmware/hvmloader/Makefile
> > @@ -87,21 +89,21 @@ roms.inc: $(ROMS)
> > 
> > ifneq ($(ROMBIOS_ROM),)
> > 	echo "#ifdef ROM_INCLUDE_ROMBIOS" >> $@.new
> > -	sh ../../misc/mkhex rombios $(ROMBIOS_ROM) >> $@.new
> > +	$(SHELL) $(XEN_ROOT)/tools/misc/mkhex rombios $(ROMBIOS_ROM) >> $@.new
> > 	echo "#endif" >> $@.new
> > endif
> > 
> > ifneq ($(STDVGA_ROM),)
> > 	echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
> > -	sh ../../misc/mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
> > +	$(SHELL) $(XEN_ROOT)/tools/misc/mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
> > 	echo "#endif" >> $@.new
> > endif
> > ifneq ($(CIRRUSVGA_ROM),)
> > 	echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
> > -	sh ../../misc/mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
> > +	$(SHELL) $(XEN_ROOT)/tools/misc/mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
> > 	echo "#endif" >> $@.new
> > endif
> > -	mv $@.new $@
> > +	mv -f $@.new $@
> 
> Here, instead of -f, is it safer -u? What’s your opinion on that? The patch looks good to me.

make want to rebuild the target, so there is no reason to keep the
existing target. We do need to overwrite the existing target if it
exist.

Thanks for the reviews!
Luca Fancellu July 11, 2022, 1:40 p.m. UTC | #3 
> On 11 Jul 2022, at 14:38, Anthony PERARD <anthony.perard@citrix.com> wrote:
> 
> On Fri, Jul 08, 2022 at 03:39:00PM +0000, Luca Fancellu wrote:
>>> On 24 Jun 2022, at 17:04, Anthony PERARD <anthony.perard@citrix.com> wrote:
> [...]
>>> For "roms.inc" target, use "$(SHELL)" instead of plain "sh". And use
>>> full path to "mkhex" instead of a relative one. Lastly, add "-f" flag
>>> to "mv", in case the target already exist.
>>> 
>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>> ---
>>> diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
>>> index b754220839..fc20932110 100644
>>> --- a/tools/firmware/hvmloader/Makefile
>>> +++ b/tools/firmware/hvmloader/Makefile
>>> @@ -87,21 +89,21 @@ roms.inc: $(ROMS)
>>> 
>>> ifneq ($(ROMBIOS_ROM),)
>>> 	echo "#ifdef ROM_INCLUDE_ROMBIOS" >> $@.new
>>> -	sh ../../misc/mkhex rombios $(ROMBIOS_ROM) >> $@.new
>>> +	$(SHELL) $(XEN_ROOT)/tools/misc/mkhex rombios $(ROMBIOS_ROM) >> $@.new
>>> 	echo "#endif" >> $@.new
>>> endif
>>> 
>>> ifneq ($(STDVGA_ROM),)
>>> 	echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
>>> -	sh ../../misc/mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
>>> +	$(SHELL) $(XEN_ROOT)/tools/misc/mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
>>> 	echo "#endif" >> $@.new
>>> endif
>>> ifneq ($(CIRRUSVGA_ROM),)
>>> 	echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
>>> -	sh ../../misc/mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
>>> +	$(SHELL) $(XEN_ROOT)/tools/misc/mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
>>> 	echo "#endif" >> $@.new
>>> endif
>>> -	mv $@.new $@
>>> +	mv -f $@.new $@
>> 
>> Here, instead of -f, is it safer -u? What’s your opinion on that? The patch looks good to me.
> 
> make want to rebuild the target, so there is no reason to keep the
> existing target. We do need to overwrite the existing target if it
> exist.
> 
> Thanks for the reviews!

Ok thanks for the clarification, as I said the changes looks good to me:

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>


> 
> -- 
> Anthony PERARD
Jan Beulich July 11, 2022, 1:41 p.m. UTC | #4 
On 08.07.2022 17:39, Luca Fancellu wrote:
>> On 24 Jun 2022, at 17:04, Anthony PERARD <anthony.perard@citrix.com> wrote:
>> @@ -87,21 +89,21 @@ roms.inc: $(ROMS)
>>
>> ifneq ($(ROMBIOS_ROM),)
>> 	echo "#ifdef ROM_INCLUDE_ROMBIOS" >> $@.new
>> -	sh ../../misc/mkhex rombios $(ROMBIOS_ROM) >> $@.new
>> +	$(SHELL) $(XEN_ROOT)/tools/misc/mkhex rombios $(ROMBIOS_ROM) >> $@.new
>> 	echo "#endif" >> $@.new
>> endif
>>
>> ifneq ($(STDVGA_ROM),)
>> 	echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
>> -	sh ../../misc/mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
>> +	$(SHELL) $(XEN_ROOT)/tools/misc/mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
>> 	echo "#endif" >> $@.new
>> endif
>> ifneq ($(CIRRUSVGA_ROM),)
>> 	echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
>> -	sh ../../misc/mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
>> +	$(SHELL) $(XEN_ROOT)/tools/misc/mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
>> 	echo "#endif" >> $@.new
>> endif
>> -	mv $@.new $@
>> +	mv -f $@.new $@
> 
> Here, instead of -f, is it safer -u? What’s your opinion on that? The patch looks good to me.

Would -u be an option to use in the first place? It's not a standard
option to mv, afaict.

Jan
Jan Beulich July 11, 2022, 1:52 p.m. UTC | #5 
On 24.06.2022 18:04, Anthony PERARD wrote:
> Setup proper dependencies with libacpi so we don't need to run "make
> hvmloader" in the "all" target. ("build.o" new prerequisite isn't
> exactly proper but a side effect of building the $(DSDT_FILES) is to
> generate the "ssdt_*.h" needed by "build.o".)

Maybe leave a brief comment there?

> Make use if "-iquote" instead of a plain "-I".
> 
> For "roms.inc" target, use "$(SHELL)" instead of plain "sh". And use
> full path to "mkhex" instead of a relative one. Lastly, add "-f" flag
> to "mv", in case the target already exist.

Hmm - according to my understanding -f isn't needed just because the
file may already exist. It would be needed if a pre-existing file
isn't writable. (I don't mind the addition of the flag, but I think
what you say can end up misleading.)

Jan

> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/firmware/hvmloader/Makefile | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
> index b754220839..fc20932110 100644
> --- a/tools/firmware/hvmloader/Makefile
> +++ b/tools/firmware/hvmloader/Makefile
> @@ -60,8 +60,7 @@ ROMS += $(ROMBIOS_ROM) $(STDVGA_ROM) $(CIRRUSVGA_ROM)
>  endif
>  
>  .PHONY: all
> -all: acpi
> -	$(MAKE) hvmloader
> +all: hvmloader
>  
>  .PHONY: acpi
>  acpi:
> @@ -73,12 +72,15 @@ smbios.o: CFLAGS += -D__SMBIOS_DATE__="\"$(SMBIOS_REL_DATE)\""
>  ACPI_PATH = ../../libacpi
>  DSDT_FILES = dsdt_anycpu.c dsdt_15cpu.c dsdt_anycpu_qemu_xen.c
>  ACPI_OBJS = $(patsubst %.c,%.o,$(DSDT_FILES)) build.o static_tables.o
> -$(ACPI_OBJS): CFLAGS += -I. -DLIBACPI_STDUTILS=\"$(CURDIR)/util.h\"
> +$(ACPI_OBJS): CFLAGS += -iquote . -DLIBACPI_STDUTILS=\"$(CURDIR)/util.h\"
>  CFLAGS += -I$(ACPI_PATH)
>  vpath build.c $(ACPI_PATH)
>  vpath static_tables.c $(ACPI_PATH)
>  OBJS += $(ACPI_OBJS)
>  
> +$(DSDT_FILES): acpi
> +build.o: $(DSDT_FILES)
> +
>  hvmloader: $(OBJS) hvmloader.lds
>  	$(LD) $(LDFLAGS_DIRECT) -N -T hvmloader.lds -o $@ $(OBJS)
>  
> @@ -87,21 +89,21 @@ roms.inc: $(ROMS)
>  
>  ifneq ($(ROMBIOS_ROM),)
>  	echo "#ifdef ROM_INCLUDE_ROMBIOS" >> $@.new
> -	sh ../../misc/mkhex rombios $(ROMBIOS_ROM) >> $@.new
> +	$(SHELL) $(XEN_ROOT)/tools/misc/mkhex rombios $(ROMBIOS_ROM) >> $@.new
>  	echo "#endif" >> $@.new
>  endif
>  
>  ifneq ($(STDVGA_ROM),)
>  	echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
> -	sh ../../misc/mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
> +	$(SHELL) $(XEN_ROOT)/tools/misc/mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
>  	echo "#endif" >> $@.new
>  endif
>  ifneq ($(CIRRUSVGA_ROM),)
>  	echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
> -	sh ../../misc/mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
> +	$(SHELL) $(XEN_ROOT)/tools/misc/mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
>  	echo "#endif" >> $@.new
>  endif
> -	mv $@.new $@
> +	mv -f $@.new $@
>  
>  .PHONY: clean
>  clean:
Anthony PERARD July 11, 2022, 5:01 p.m. UTC | #6 
On Mon, Jul 11, 2022 at 03:52:52PM +0200, Jan Beulich wrote:
> On 24.06.2022 18:04, Anthony PERARD wrote:
> > Setup proper dependencies with libacpi so we don't need to run "make
> > hvmloader" in the "all" target. ("build.o" new prerequisite isn't
> > exactly proper but a side effect of building the $(DSDT_FILES) is to
> > generate the "ssdt_*.h" needed by "build.o".)
> 
> Maybe leave a brief comment there?

Sounds good.

> > Make use if "-iquote" instead of a plain "-I".
> > 
> > For "roms.inc" target, use "$(SHELL)" instead of plain "sh". And use
> > full path to "mkhex" instead of a relative one. Lastly, add "-f" flag
> > to "mv", in case the target already exist.
> 
> Hmm - according to my understanding -f isn't needed just because the
> file may already exist. It would be needed if a pre-existing file
> isn't writable. (I don't mind the addition of the flag, but I think
> what you say can end up misleading.)

Yes. After reading the posix doc about `mv`, the following would be
better:

    Lastly, add "-f" flag to "mv" to avoid a prompt in case the target
    already exist and we don't have write permission.

Thanks,
diff mbox series

Patch

diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index b754220839..fc20932110 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -60,8 +60,7 @@  ROMS += $(ROMBIOS_ROM) $(STDVGA_ROM) $(CIRRUSVGA_ROM)
 endif
 
 .PHONY: all
-all: acpi
-	$(MAKE) hvmloader
+all: hvmloader
 
 .PHONY: acpi
 acpi:
@@ -73,12 +72,15 @@  smbios.o: CFLAGS += -D__SMBIOS_DATE__="\"$(SMBIOS_REL_DATE)\""
 ACPI_PATH = ../../libacpi
 DSDT_FILES = dsdt_anycpu.c dsdt_15cpu.c dsdt_anycpu_qemu_xen.c
 ACPI_OBJS = $(patsubst %.c,%.o,$(DSDT_FILES)) build.o static_tables.o
-$(ACPI_OBJS): CFLAGS += -I. -DLIBACPI_STDUTILS=\"$(CURDIR)/util.h\"
+$(ACPI_OBJS): CFLAGS += -iquote . -DLIBACPI_STDUTILS=\"$(CURDIR)/util.h\"
 CFLAGS += -I$(ACPI_PATH)
 vpath build.c $(ACPI_PATH)
 vpath static_tables.c $(ACPI_PATH)
 OBJS += $(ACPI_OBJS)
 
+$(DSDT_FILES): acpi
+build.o: $(DSDT_FILES)
+
 hvmloader: $(OBJS) hvmloader.lds
 	$(LD) $(LDFLAGS_DIRECT) -N -T hvmloader.lds -o $@ $(OBJS)
 
@@ -87,21 +89,21 @@  roms.inc: $(ROMS)
 
 ifneq ($(ROMBIOS_ROM),)
 	echo "#ifdef ROM_INCLUDE_ROMBIOS" >> $@.new
-	sh ../../misc/mkhex rombios $(ROMBIOS_ROM) >> $@.new
+	$(SHELL) $(XEN_ROOT)/tools/misc/mkhex rombios $(ROMBIOS_ROM) >> $@.new
 	echo "#endif" >> $@.new
 endif
 
 ifneq ($(STDVGA_ROM),)
 	echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
-	sh ../../misc/mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
+	$(SHELL) $(XEN_ROOT)/tools/misc/mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
 	echo "#endif" >> $@.new
 endif
 ifneq ($(CIRRUSVGA_ROM),)
 	echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
-	sh ../../misc/mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
+	$(SHELL) $(XEN_ROOT)/tools/misc/mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
 	echo "#endif" >> $@.new
 endif
-	mv $@.new $@
+	mv -f $@.new $@
 
 .PHONY: clean
 clean: