[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/30 17:13, Jan Beulich wrote:
On 30.10.14 at 06:55, <tiejun.chen@xxxxxxxxx> wrote:
On 2014/10/29 17:05, Jan Beulich wrote:
On 29.10.14 at 07:54, <tiejun.chen@xxxxxxxxx> wrote:
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.

Please stop thinking this way. Declarations for things defined in .c
files are to be present in headers, and the defining .c file has to
include that header (making sure declaration and definition are and
remain in sync). I hate having to again repeat my remark that you
shouldn't forget it's not application code that you're modifying.
Robust and maintainable code are a requirement in the hypervisor
(and, as said it being an extension of it, hvmloader). Which - just
to avoid any misunderstanding - isn't to say that this shouldn't also
apply to application code. It's just that in the hypervisor and kernel
(and certain other code system components) the consequences of
being lax are much more severe.

Okay. But currently, the pci.c file already include 'util.h' and

#include "util.h"
#include <xen/memory.h>

We can't redefine struct xen_reserved_device_memory in util.h.

Redefine? I said forward declare.

Seems we just need to declare hvm_get_reserved_device_memory_map() in the head file, tools/firmware/hvmloader/util.h,

unsigned int hvm_get_reserved_device_memory_map(void);

since this is enough to pci_setup() and construct_rdm_e820_maps().


struct xen_reserved_device_memory *rdm_map;
static int
get_reserved_device_memory_map(struct xen_reserved_device_memory entries[], uint32_t *max_entries)
unsigned int hvm_get_reserved_device_memory_map(void)

So all tools maintainers,

Could you give us such a comment?

'ln -sf $(XEN_ROOT)/xen/include/xen/errno.h xen' versus
'ln -sf $(XEN_ROOT)/xen/include/xen/errno.h

I think if it is (as suggested) to be limited to hvmloader, the
symlinking also should be done in its Makefile rather than along
with other tools stuff.

You're right.

I just send out a separate patch to ask a review from tools side, and CCed you.


Xen-devel mailing list



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