Skip to contentSkip to conversation

tools/hvmloader: link errno.h from xen internal

PATCH Superseded Tiejun Chen 3 participants 9 messages Oct 31, 2014 – Nov 3, 2014

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Tiejun Chen

author
#0 lore raw

PATCH 1/1 tools/hvmloader: link errno.h from xen internal

We will use some error numbers in hvmloader so here just link this head file from xen.

Signed-off-by Tiejun Chen <[email protected]>
---
 .gitignore                        | 1 +
 tools/firmware/hvmloader/Makefile | 8 ++++++--
 tools/firmware/hvmloader/util.h   | 1 +
 3 files changed, 8 insertions(+), 2 deletions(-)
.gitignore +1 -0
index b24e905..52c3038 100644
@@ -127,6 +127,7 @@ tools/firmware/hvmloader/acpi/ssdt_*.h
 tools/firmware/hvmloader/hvmloader
 tools/firmware/hvmloader/roms.h
 tools/firmware/hvmloader/roms.inc
+tools/firmware/hvmloader/errno.h
 tools/firmware/rombios/BIOS-bochs-[^/]*
 tools/firmware/rombios/_rombios[^/]*_.c
 tools/firmware/rombios/rombios[^/]*.s
tools/firmware/hvmloader/Makefile +6 -2
index 46a79c5..2e0f062 100644
@@ -84,9 +84,13 @@ ROMS += $(SEABIOS_ROM)
 endif
 
 .PHONY: all
-all: subdirs-all
+all: subdirs-all .dir
 	$(MAKE) hvmloader
 
+.dir:
+	@rm -rf errno.h
+	ln -sf $(XEN_ROOT)/xen/include/xen/errno.h .
+
 ovmf.o rombios.o seabios.o hvmloader.o: roms.inc
 smbios.o: CFLAGS += -D__SMBIOS_DATE__="\"$(shell date +%m/%d/%Y)\""
 

@@ -136,7 +140,7 @@ endif
 
 .PHONY: clean
 clean: subdirs-clean
-	rm -f roms.inc roms.inc.new acpi.h
+	rm -f roms.inc roms.inc.new acpi.h errno.h
 	rm -f hvmloader hvmloader.tmp *.o $(DEPS)
 
 -include $(DEPS)
tools/firmware/hvmloader/util.h +1 -0
index a70e4aa..1352025 100644
@@ -6,6 +6,7 @@
 #include <stddef.h>
 #include <xen/xen.h>
 #include <xen/hvm/hvm_info_table.h>
+#include "errno.h"
 
 #define __STR(...) #__VA_ARGS__
 #define STR(...) __STR(__VA_ARGS__)
Signature · 1 line
1.9.1
#1 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line · On 31.10.14 at 03:18, <[email protected]> wrote:

(You omitted half of the tools maintainers; Cc-ing them now.)

Quoted 2 lines

We will use some error numbers in hvmloader so here just link this head file from xen.

This is not a proper reasoning for using the Xen internal header here. You should make clear that we want to act on specific hypercall error codes, and hence require the hypervisors view on the errno.h values rather than the build environment's (as was sufficient for the use in xenbus.c).

tools/firmware/hvmloader/Makefile ROMS += $(SEABIOS_ROM) · line 91

Quoted 12 lines
tools/firmware/hvmloader/Makefile +3 -1
@@ -84,9 +84,13 @@ ROMS += $(SEABIOS_ROM)
 endif
 
 .PHONY: all
-all: subdirs-all
+all: subdirs-all .dir
 	$(MAKE) hvmloader
 
+.dir:
+	@rm -rf errno.h

Why?

tools/firmware/hvmloader/util.h line 9

Quoted 7 lines
tools/firmware/hvmloader/util.h +1 -0
@@ -6,6 +6,7 @@
 #include <stddef.h>
 #include <xen/xen.h>
 #include <xen/hvm/hvm_info_table.h>
+#include "errno.h"

Does this allow xenbus.c to still build? I think this either should go into the .c file wanting to use the values (preferable - remember my earlier comment about introducing unnecessary dependencies?), or the respective #include <errno.h> in xenbus.c should be dropped.

Jan

author
#2 lore raw
Quoted 3 lines · Jan Beulich
Quoted 1 line
Quoted 1 line
Quoted 1 line · On 31.10.14 at 03:18, <[email protected]> wrote:

(You omitted half of the tools maintainers; Cc-ing them now.)

I think I already pick all relevant maintainers to review this patch,

$ ./scripts/get_maintainer.pl 0004-tools-hvmloader-link-errno.h-from-xen-internal.patch
Stefano Stabellini <[email protected]>
Wei Liu <[email protected]>
[email protected]

So please tell me whom should be included specifically.

Quoted 10 lines
Quoted 3 lines

We will use some error numbers in hvmloader so here just link this head file from xen.

This is not a proper reasoning for using the Xen internal header here. You should make clear that we want to act on specific hypercall error codes, and hence require the hypervisors view on the errno.h values rather than the build environment's (as was sufficient for the use in xenbus.c).

So rephrase,

tools/hvmloader: link errno.h from xen internal

We need to act on some specific hypercall error numbers, so require the hypervisor view on the errno.h value rather than just the build environment's number. So here link this headfile from xen.

Quoted 10 lines
Quoted 10 lines
tools/firmware/hvmloader/Makefile +1 -1
@@ -84,9 +84,13 @@ ROMS += $(SEABIOS_ROM)
  endif
 
  .PHONY: all
-all: subdirs-all
+all: subdirs-all .dir
  	$(MAKE) hvmloader
Quoted 4 lines

tools/firmware/hvmloader/Makefile ROMS += $(SEABIOS_ROM) · line 91

Quoted 2 lines
tools/firmware/hvmloader/Makefile +2 -0
+.dir:
+	@rm -rf errno.h

Why?

We should make sure we are linking to create a non-existing file, otherwise you may see this,

ln: failed to create symbolic link '...': File exists

Quoted 10 lines

tools/firmware/hvmloader/util.h line 9

Quoted 8 lines
tools/firmware/hvmloader/util.h +1 -0
@@ -6,6 +6,7 @@
  #include <stddef.h>
  #include <xen/xen.h>
  #include <xen/hvm/hvm_info_table.h>
+#include "errno.h"

Does this allow xenbus.c to still build? I think this either should go into

I did a test as follows:
1: make clean
2: make xen
3: make tools
4: make clean
5: make tools

Is this covering xenbus?

Quoted 2 lines

the .c file wanting to use the values (preferable - remember my earlier comment about introducing unnecessary dependencies?), or the

Okay, I can remove this part, then just add this head file into any necessary .c files. This will be addressed in other RMRR patches.

Thanks
Tiejun

Quoted 5 lines

respective #include <errno.h> in xenbus.c should be dropped.

Jan

#3 lore raw
Quoted 8 lines
Quoted 1 line
Quoted 1 line · On 03.11.14 at 03:03, <[email protected]> wrote:
Quoted 3 lines · Jan Beulich
Quoted 1 line
Quoted 1 line
Quoted 1 line · On 31.10.14 at 03:18, <[email protected]> wrote:

(You omitted half of the tools maintainers; Cc-ing them now.)

I think I already pick all relevant maintainers to review this patch,

Quoted 7 lines
$ ./scripts/get_maintainer.pl 
0004-tools-hvmloader-link-errno.h-from-xen-internal.patch
Stefano Stabellini <[email protected]>
Wei Liu <[email protected]>
[email protected]

So please tell me whom should be included specifically.

It's not clear to me why the script doesn't name the others. Just go the ./MAINTAINERS and look at the relevant section.

Quoted 15 lines
Quoted 10 lines
Quoted 10 lines
tools/firmware/hvmloader/Makefile +1 -1
@@ -84,9 +84,13 @@ ROMS += $(SEABIOS_ROM)
  endif
 
  .PHONY: all
-all: subdirs-all
+all: subdirs-all .dir
  	$(MAKE) hvmloader
Quoted 4 lines

tools/firmware/hvmloader/Makefile ROMS += $(SEABIOS_ROM) · line 91

Quoted 2 lines
tools/firmware/hvmloader/Makefile +2 -0
+.dir:
+	@rm -rf errno.h

Why?

Quoted 4 lines

We should make sure we are linking to create a non-existing file, otherwise you may see this,

ln: failed to create symbolic link '...': File exists

But you know of ln's -f option, don't you?

Quoted 10 lines
Quoted 9 lines

tools/firmware/hvmloader/util.h line 9

Quoted 7 lines
tools/firmware/hvmloader/util.h +1 -0
@@ -6,6 +6,7 @@
  #include <stddef.h>
  #include <xen/xen.h>
  #include <xen/hvm/hvm_info_table.h>
+#include "errno.h"

Does this allow xenbus.c to still build? I think this either should go into

Quoted 8 lines

I did a test as follows:
1: make clean
2: make xen
3: make tools
4: make clean
5: make tools

Is this covering xenbus?

Yes, but it's still surprising that including both works. Presumably this is because the definitions are in fact identical when you build on Linux. Building elsewhere might then be a problem, so this should be avoid in any event.

Jan

#4 lore raw
In reply to Tiejun Chen
Quoted 4 lines · Tiejun Chen

We will use some error numbers in hvmloader so here just link this head file from xen.

Signed-off-by Tiejun Chen <[email protected]>

We already use stdint, stdarg, stdarg and error (in xenbus.c) from the build environment.

All of these are safe from an embedded point of view, which is essentially what we are doing with hvmloader.

I highly suggest we just go with the environments error.h and be done with it. C99 is very nice to us in this way.

~Andrew

Quoted 58 lines
---
 .gitignore                        | 1 +
 tools/firmware/hvmloader/Makefile | 8 ++++++--
 tools/firmware/hvmloader/util.h   | 1 +
 3 files changed, 8 insertions(+), 2 deletions(-)
.gitignore +1 -0
index b24e905..52c3038 100644
@@ -127,6 +127,7 @@ tools/firmware/hvmloader/acpi/ssdt_*.h
 tools/firmware/hvmloader/hvmloader
 tools/firmware/hvmloader/roms.h
 tools/firmware/hvmloader/roms.inc
+tools/firmware/hvmloader/errno.h
 tools/firmware/rombios/BIOS-bochs-[^/]*
 tools/firmware/rombios/_rombios[^/]*_.c
 tools/firmware/rombios/rombios[^/]*.s
tools/firmware/hvmloader/Makefile +6 -2
index 46a79c5..2e0f062 100644
@@ -84,9 +84,13 @@ ROMS += $(SEABIOS_ROM)
 endif
 
 .PHONY: all
-all: subdirs-all
+all: subdirs-all .dir
 	$(MAKE) hvmloader
 
+.dir:
+	@rm -rf errno.h
+	ln -sf $(XEN_ROOT)/xen/include/xen/errno.h .
+
 ovmf.o rombios.o seabios.o hvmloader.o: roms.inc
 smbios.o: CFLAGS += -D__SMBIOS_DATE__="\"$(shell date +%m/%d/%Y)\""
 

@@ -136,7 +140,7 @@ endif
 
 .PHONY: clean
 clean: subdirs-clean
-	rm -f roms.inc roms.inc.new acpi.h
+	rm -f roms.inc roms.inc.new acpi.h errno.h
 	rm -f hvmloader hvmloader.tmp *.o $(DEPS)
 
 -include $(DEPS)
tools/firmware/hvmloader/util.h +1 -0
index a70e4aa..1352025 100644
@@ -6,6 +6,7 @@
 #include <stddef.h>
 #include <xen/xen.h>
 #include <xen/hvm/hvm_info_table.h>
+#include "errno.h"
 
 #define __STR(...) #__VA_ARGS__
 #define STR(...) __STR(__VA_ARGS__)
#5 lore raw
Quoted 7 lines
Quoted 1 line
Quoted 1 line · On 31.10.14 at 11:04, <[email protected]> wrote:
Quoted 4 lines · Tiejun Chen

We will use some error numbers in hvmloader so here just link this head file from xen.

Signed-off-by Tiejun Chen <[email protected]>
Quoted 8 lines

We already use stdint, stdarg, stdarg and error (in xenbus.c) from the build environment.

All of these are safe from an embedded point of view, which is essentially what we are doing with hvmloader.

I highly suggest we just go with the environments error.h and be done with it. C99 is very nice to us in this way.

No, specifically not, as the values defined there may not correspond with the ones Xen defines (POSIX doesn't mandate specific numbers).

Jan

#6 lore raw
Quoted 9 lines · Jan Beulich
Quoted 9 lines
Quoted 1 line
Quoted 1 line · On 31.10.14 at 11:04, <[email protected]> wrote:
Quoted 4 lines · Tiejun Chen

We will use some error numbers in hvmloader so here just link this head file from xen.

Signed-off-by Tiejun Chen <[email protected]>

We already use stdint, stdarg, stdarg and error (in xenbus.c) from the build environment.

Quoted 7 lines
Quoted 5 lines

All of these are safe from an embedded point of view, which is essentially what we are doing with hvmloader.

I highly suggest we just go with the environments error.h and be done with it. C99 is very nice to us in this way.

No, specifically not, as the values defined there may not correspond with the ones Xen defines (POSIX doesn't mandate specific numbers).

Good point, which reminds me.

The Xen error numbers must form part of the public API, as they are already part of the public ABI.

~Andrew

#7 lore raw
Quoted 16 lines
Quoted 1 line
Quoted 1 line · On 31.10.14 at 11:54, <[email protected]> wrote:
Quoted 14 lines · Jan Beulich
Quoted 14 lines
Quoted 1 line
Quoted 1 line · On 31.10.14 at 11:04, <[email protected]> wrote:
Quoted 4 lines · Tiejun Chen

We will use some error numbers in hvmloader so here just link this head file from xen.

Signed-off-by Tiejun Chen <[email protected]>

We already use stdint, stdarg, stdarg and error (in xenbus.c) from the build environment.

All of these are safe from an embedded point of view, which is essentially what we are doing with hvmloader.

I highly suggest we just go with the environments error.h and be done with it. C99 is very nice to us in this way.

Quoted 7 lines
Quoted 2 lines

No, specifically not, as the values defined there may not correspond with the ones Xen defines (POSIX doesn't mandate specific numbers).

Good point, which reminds me.

The Xen error numbers must form part of the public API, as they are already part of the public ABI.

Right. But probably something to be dealt with post-4.5.

Jan

author
#8 lore raw
In reply to Andrew Cooper
Quoted 6 lines · Andrew Cooper
Quoted 4 lines · Tiejun Chen

We will use some error numbers in hvmloader so here just link this head file from xen.

Signed-off-by Tiejun Chen <[email protected]>
Quoted 8 lines

We already use stdint, stdarg, stdarg and error (in xenbus.c) from the build environment.

All of these are safe from an embedded point of view, which is essentially what we are doing with hvmloader.

I highly suggest we just go with the environments error.h and be done with it. C99 is very nice to us in this way.

Thanks for your comments. But looks we should follow-up Jan's further explanation.

Thanks
Tiejun

Quoted 64 lines

~Andrew

Quoted 58 lines
---
  .gitignore                        | 1 +
  tools/firmware/hvmloader/Makefile | 8 ++++++--
  tools/firmware/hvmloader/util.h   | 1 +
  3 files changed, 8 insertions(+), 2 deletions(-)
.gitignore +1 -0
index b24e905..52c3038 100644
@@ -127,6 +127,7 @@ tools/firmware/hvmloader/acpi/ssdt_*.h
  tools/firmware/hvmloader/hvmloader
  tools/firmware/hvmloader/roms.h
  tools/firmware/hvmloader/roms.inc
+tools/firmware/hvmloader/errno.h
  tools/firmware/rombios/BIOS-bochs-[^/]*
  tools/firmware/rombios/_rombios[^/]*_.c
  tools/firmware/rombios/rombios[^/]*.s
tools/firmware/hvmloader/Makefile +6 -2
index 46a79c5..2e0f062 100644
@@ -84,9 +84,13 @@ ROMS += $(SEABIOS_ROM)
  endif
 
  .PHONY: all
-all: subdirs-all
+all: subdirs-all .dir
  	$(MAKE) hvmloader
 
+.dir:
+	@rm -rf errno.h
+	ln -sf $(XEN_ROOT)/xen/include/xen/errno.h .
+
  ovmf.o rombios.o seabios.o hvmloader.o: roms.inc
  smbios.o: CFLAGS += -D__SMBIOS_DATE__="\"$(shell date +%m/%d/%Y)\""
 

@@ -136,7 +140,7 @@ endif
 
  .PHONY: clean
  clean: subdirs-clean
-	rm -f roms.inc roms.inc.new acpi.h
+	rm -f roms.inc roms.inc.new acpi.h errno.h
  	rm -f hvmloader hvmloader.tmp *.o $(DEPS)
 
  -include $(DEPS)
tools/firmware/hvmloader/util.h +1 -0
index a70e4aa..1352025 100644
@@ -6,6 +6,7 @@
  #include <stddef.h>
  #include <xen/xen.h>
  #include <xen/hvm/hvm_info_table.h>
+#include "errno.h"
 
  #define __STR(...) #__VA_ARGS__
  #define STR(...) __STR(__VA_ARGS__)