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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.11.14 at 10:32, <tiejun.chen@xxxxxxxxx> wrote:
> On 2014/11/3 16:53, Jan Beulich wrote:
>>>>> On 03.11.14 at 03:22, <tiejun.chen@xxxxxxxxx> wrote:
>>> On 2014/10/31 16:14, Jan Beulich wrote:
>>>>>>> On 31.10.14 at 03:20, <tiejun.chen@xxxxxxxxx> wrote:
>>>>> 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:
>>>>>>>>> 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);
>>>>
>>>> To me this looks very much like poor programming style, even if in
>>>> the context of hvmloader communicating information via global
>>>> variables rather than function arguments and return values is
>>>
>>> Do you mean you don't like a global variable? But it can be use to get
>>> RDM without more hypercall or function call in the context of hvmloader.
>>
>> This argument which you brought up before, and which we commented
>> on before, is pretty pointless. We don't really care much about doing
>> one or two more hypercalls from hvmloader, unless these would be
>> long-running ones.
>>
> 
> Another benefit to use a global variable is that we wouldn't allocate 
> xen_reserved_device_memory * N each time, and reduce some duplicated 
> codes, unless you mean I should define that as static inside in local.

Now this reason is indeed worth a consideration. How many times is
the data being needed/retrieved?

Jan


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