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

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

On 2014/10/29 17:05, Jan Beulich wrote:
>>>> On 29.10.14 at 07:54, <tiejun.chen@intel.com> wrote:
>> On 2014/10/28 17:48, Jan Beulich wrote:
>>>>>> On 28.10.14 at 06:21, <tiejun.chen@intel.com> wrote:
>>>> On 2014/10/27 17:45, Jan Beulich wrote:
>>>>>>>> On 27.10.14 at 04:12, <tiejun.chen@intel.com> wrote:
>>>>>> On 2014/10/24 22:22, Jan Beulich wrote:
>>>>>>>>>> On 24.10.14 at 09:34, <tiejun.chen@intel.com> 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.

>
>>>>>>> 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
>
> Along those lines at least. Consult with the tools maintainers as to
> whether this would better go into a special subdirectory (and
> perhaps not even under tools/include, but somewhere beneath
> tools/firmware/hvmloader/, since it's not meant to be used by

Agree with you.

> anyone else - as one of the ones de facto looking after hvmloader
> irrespective of what ./MAINTAINERS says, I'd strongly recommend
> limiting the visibility scope of this header as much as possible).

So this time this email is Toed the following maintainers

Ian Jackson <ian.jackson@eu.citrix.com>
Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Ian Campbell <ian.campbell@citrix.com>
Wei Liu <wei.liu2@citrix.com>

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/,

Thanks
Tiejun

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