1. Patchew
  2. Xen
  3. [XEN PATCH v3 00/25] Toolstack build system improvement, toward non-recursive makefiles
  4. View patch

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

Anthony PERARD posted 25 patches 3 years, 7 months ago
There is a newer version of this series
[XEN PATCH v3 04/25] tools/firmware/hvmloader: rework Makefile
Posted by Anthony PERARD 3 years, 7 months ago 
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 $@
 
 .PHONY: clean
 clean:
-- 
Anthony PERARD
Re: [XEN PATCH v3 04/25] tools/firmware/hvmloader: rework Makefile
Posted by Jan Beulich 3 years, 7 months ago 
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:
Re: [XEN PATCH v3 04/25] tools/firmware/hvmloader: rework Makefile
Posted by Anthony PERARD 3 years, 7 months ago 
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,

-- 
Anthony PERARD
Re: [XEN PATCH v3 04/25] tools/firmware/hvmloader: rework Makefile
Posted by Luca Fancellu 3 years, 7 months ago 

> 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
> 
>
Re: [XEN PATCH v3 04/25] tools/firmware/hvmloader: rework Makefile
Posted by Jan Beulich 3 years, 7 months ago 
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
Re: [XEN PATCH v3 04/25] tools/firmware/hvmloader: rework Makefile
Posted by Anthony PERARD 3 years, 7 months ago 
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!

-- 
Anthony PERARD
Re: [XEN PATCH v3 04/25] tools/firmware/hvmloader: rework Makefile
Posted by Luca Fancellu 3 years, 7 months ago 

> 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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.