[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [v7][RFC][PATCH 04/13] hvmloader/util: get reserved device memory maps

On 2014/10/28 17:48, Jan Beulich wrote:
On 28.10.14 at 06:21, <tiejun.chen@xxxxxxxxx> wrote:
On 2014/10/27 17:45, Jan Beulich wrote:
On 27.10.14 at 04:12, <tiejun.chen@xxxxxxxxx> wrote:
On 2014/10/24 22:22, Jan Beulich wrote:
On 24.10.14 at 09:34, <tiejun.chen@xxxxxxxxx> wrote:
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -241,6 +241,12 @@ int build_e820_table(struct e820entry *e820,
                         unsigned int bios_image_base);
    void dump_e820_table(struct e820entry *e820, unsigned int nr);

+#include <xen/memory.h>
+#define ENOBUFS     105 /* No buffer space available */

This is a joke I hope? The #include belongs at the top (albeit afaict
you don't really need it here), and the #define is completely

If without this line, #include <xen/memory.h>,

In file included from build.c:25:0:
../util.h:246:70: error: array type has incomplete element type
    int get_reserved_device_memory_map(struct xen_reserved_device_memory
make[8]: *** [build.o] Error 1

So just forward declare the structure ahead of the function

tools/firmware/hvmloader/pci.c:28:#include <xen/memory.h>
tools/firmware/hvmloader/ovmf.c:36:#include <xen/memory.h>

So any reason I can't do such a same thing?

You can, but it's undesirable. You're wanting this in a header, i.e.
you'll make everyone consuming that header also implicitly depend
on the new header you would include. We shouldn't pointlessly
add build dependencies (and we should really try to reduce them
where possible).

Looks I can remove those stuff from util.h and just add 'extern' to them when we really need them.

misplaced here. While I generally wouldn't recommend doing this, I
think in the case here including the hypervisor header that defines
them would be okay. Perhaps not via relative path, but via having

So is the following is a way "via having the Makefile symlink the hypervisor header here."?

--- a/tools/include/Makefile
+++ b/tools/include/Makefile
@@ -17,6 +17,7 @@ xen/.dir:
        ln -sf ../xen-sys/$(XEN_OS) xen/sys
ln -sf $(addprefix $(XEN_ROOT)/xen/include/xen/,libelf.h elfstructs.h) xen/libelf/
        ln -s ../xen-foreign xen/foreign
+       ln -sf $(XEN_ROOT)/xen/include/xen/errno.h xen
        touch $@

 .PHONY: install

Then we just need include this in util.c:

--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -26,6 +26,7 @@
 #include <xen/xen.h>
 #include <xen/memory.h>
 #include <xen/sched.h>
+#include <xen/errno.h>

 void wrmsr(uint32_t idx, uint64_t v)


Seems we just need to include this,

#include <errno.h>

You shouldn't include system headers here - what if the build system's
-E... values differ from Xen's? Please remember that what your making

tools/firmware/hvmloader/xenbus.c:30:#include <errno.h>

This is a completely different case: For one, no-one really looks at
the error codes generated here. And even if someone would, these
would be error value purely internal to hvmloader. Whereas in your
case you want to interpret a value you get back from the hypervisor.

And why will Xen define this different?

Why would Linux, *BSD, Solaris, and whatever else OS usable as a
build host for building Xen all be required to use exactly the same
-E... definitions when already Linux has variations for some of them
depending on host architecture (i.e. when doing cross builds you'd
risk running into problems even on Linux)?

Once again - please always keep in mind that you're modifying
hypervisor code, not some simple user mode application.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.