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

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

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
> > > > > > > > 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.
> > > 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
> > '<xen/memory.h>,
> >
> > #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().

tools/firmware/hvmloader/util.c:

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
> > $(XEN_ROOT)/tools/firmware/hvmloader/,
> 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.
Thanks
Tiejun

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel