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

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

On 2014/11/3 18:02, Jan Beulich wrote:
>>>> On 03.11.14 at 10:55, <tiejun.chen@intel.com> wrote:
>> On 2014/11/3 17:45, Jan Beulich wrote:
>>>>>> On 03.11.14 at 10:32, <tiejun.chen@intel.com> wrote:
>>>> On 2014/11/3 16:53, Jan Beulich wrote:
>>>>>>>> On 03.11.14 at 03:22, <tiejun.chen@intel.com> wrote:
>>>>>> On 2014/10/31 16:14, Jan Beulich wrote:
>>>>>>>>>> On 31.10.14 at 03:20, <tiejun.chen@intel.com> wrote:
>>>>>>>> On 2014/10/30 17:13, Jan Beulich wrote:
>>>>>>>>>>>> On 30.10.14 at 06:55, <tiejun.chen@intel.com> wrote:
>>>>>>>>>> On 2014/10/29 17:05, Jan Beulich wrote:
>>>>>>>>>>>>>> On 29.10.14 at 07:54, <tiejun.chen@intel.com> 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?
>>
>> Currently there are two cases in tools/hvmloader, setup pci and build
>> e820 table. Each time, as you know we don't know how may entries we
>> should require, so we always allocate one instance then according to the
>> return value to allocate the proper instances to get that.
>
> Hmm, two uses isn't really that bad, i.e. I'd then still be in favor of
> a more "normal" interface.
>

Just go back here since I realize we always use mem_alloc(), which is 
pick from RESERVED_MEMORY, to allocate all buffer inside this hypercall 
caller in hvmloader, but unfortunately we have no any associated free 
function implementation in hvmloader, so if we call this multiple times 
this means it really waster more memory in RESERVED_MEMORY. So I still 
think one global variable should be fine.

Thanks
Tiejun