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

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

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
> > > > entries[],
> > > >                                                                          ^
> > > > make[8]: *** [build.o] Error 1
> > > So just forward declare the structure ahead of the function
> > > declaration.
> > 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)
 {

Thanks
Tiejun

> > > > 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.
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel