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

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

[Tiejun Chen]

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

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 .gitignore                        | 1 +
 tools/firmware/hvmloader/Makefile | 8 ++++++--
 tools/firmware/hvmloader/util.h   | 1 +
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index b24e905..52c3038 100644
--- a/.gitignore
+++ b/.gitignore
@@ -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
diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index 46a79c5..2e0f062 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -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)
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index a70e4aa..1352025 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -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__)
-- 
1.9.1

Re: [PATCH] tools/hvmloader: link errno.h from xen internal

[Jan Beulich]

>>> On 31.10.14 at 03:18, <tiejun.chen@intel.com> wrote:

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

> 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).

> --- a/tools/firmware/hvmloader/Makefile
> +++ b/tools/firmware/hvmloader/Makefile
> @@ -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?

> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -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

Re: [PATCH] tools/hvmloader: link errno.h from xen internal

[Andrew Cooper]

On 31/10/14 02:18, Tiejun Chen wrote:
> We will use some error numbers in hvmloader so here just link
> this head file from xen.
>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>

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

> ---
>  .gitignore                        | 1 +
>  tools/firmware/hvmloader/Makefile | 8 ++++++--
>  tools/firmware/hvmloader/util.h   | 1 +
>  3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/.gitignore b/.gitignore
> index b24e905..52c3038 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -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
> diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
> index 46a79c5..2e0f062 100644
> --- a/tools/firmware/hvmloader/Makefile
> +++ b/tools/firmware/hvmloader/Makefile
> @@ -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)
> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> index a70e4aa..1352025 100644
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -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__)

Re: [PATCH] tools/hvmloader: link errno.h from xen internal

[Jan Beulich]

>>> On 31.10.14 at 11:04, <andrew.cooper3@citrix.com> wrote:
> On 31/10/14 02:18, Tiejun Chen wrote:
>> We will use some error numbers in hvmloader so here just link
>> this head file from xen.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> 
> 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

Re: [PATCH] tools/hvmloader: link errno.h from xen internal

[Andrew Cooper]

On 31/10/14 10:51, Jan Beulich wrote:
>>>> On 31.10.14 at 11:04, <andrew.cooper3@citrix.com> wrote:
>> On 31/10/14 02:18, Tiejun Chen wrote:
>>> We will use some error numbers in hvmloader so here just link
>>> this head file from xen.
>>>
>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> 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).

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

Re: [PATCH] tools/hvmloader: link errno.h from xen internal

[Jan Beulich]

>>> On 31.10.14 at 11:54, <andrew.cooper3@citrix.com> wrote:
> On 31/10/14 10:51, Jan Beulich wrote:
>>>>> On 31.10.14 at 11:04, <andrew.cooper3@citrix.com> wrote:
>>> On 31/10/14 02:18, Tiejun Chen wrote:
>>>> We will use some error numbers in hvmloader so here just link
>>>> this head file from xen.
>>>>
>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>> 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).
> 
> 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

Re: [PATCH] tools/hvmloader: link errno.h from xen internal

[Chen, Tiejun]

On 2014/10/31 17:15, Jan Beulich wrote:
>>>> On 31.10.14 at 03:18, <tiejun.chen@intel.com> 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 <stefano.stabellini@eu.citrix.com>
Wei Liu <wei.liu2@citrix.com>
xen-devel@lists.xen.org

So please tell me whom should be included specifically.

>
>> 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.

>> --- a/tools/firmware/hvmloader/Makefile
>> +++ b/tools/firmware/hvmloader/Makefile
>> @@ -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?

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

>
>> --- a/tools/firmware/hvmloader/util.h
>> +++ b/tools/firmware/hvmloader/util.h
>> @@ -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?

> 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

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

Re: [PATCH] tools/hvmloader: link errno.h from xen internal

[Chen, Tiejun]

On 2014/10/31 18:04, Andrew Cooper wrote:
> On 31/10/14 02:18, Tiejun Chen wrote:
>> We will use some error numbers in hvmloader so here just link
>> this head file from xen.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>
> 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

>
> ~Andrew
>
>> ---
>>   .gitignore                        | 1 +
>>   tools/firmware/hvmloader/Makefile | 8 ++++++--
>>   tools/firmware/hvmloader/util.h   | 1 +
>>   3 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/.gitignore b/.gitignore
>> index b24e905..52c3038 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -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
>> diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
>> index 46a79c5..2e0f062 100644
>> --- a/tools/firmware/hvmloader/Makefile
>> +++ b/tools/firmware/hvmloader/Makefile
>> @@ -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)
>> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
>> index a70e4aa..1352025 100644
>> --- a/tools/firmware/hvmloader/util.h
>> +++ b/tools/firmware/hvmloader/util.h
>> @@ -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__)
>
>
>

Re: [PATCH] tools/hvmloader: link errno.h from xen internal

[Jan Beulich]

>>> On 03.11.14 at 03:03, <tiejun.chen@intel.com> wrote:
> On 2014/10/31 17:15, Jan Beulich wrote:
>>>>> On 31.10.14 at 03:18, <tiejun.chen@intel.com> 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 <stefano.stabellini@eu.citrix.com>
> Wei Liu <wei.liu2@citrix.com>
> xen-devel@lists.xen.org 
> 
> 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.

>>> --- a/tools/firmware/hvmloader/Makefile
>>> +++ b/tools/firmware/hvmloader/Makefile
>>> @@ -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?
> 
> 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?

>>> --- a/tools/firmware/hvmloader/util.h
>>> +++ b/tools/firmware/hvmloader/util.h
>>> @@ -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?

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