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

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

On 2014/11/21 15:54, Jan Beulich wrote:
>>>> On 21.11.14 at 08:43, <kevin.tian@intel.com> wrote:
>>>   From: Chen, Tiejun
>>> Sent: Friday, November 21, 2014 2:26 PM
>>>
>>> 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.
>>
>> it's natural to get reserved information once, and then saved for either
>> one-time or multiple-time references.
>
> Not really natural, but possible. Yet if done this way, then the
> interface shouldn't give the appearance of retrieving it every time,
> i.e. the global should be set up separately and the users of the

Shouldn't we exactly implemented this previously?

+struct xen_mem_reserved_device_memory *rdm_map;

As a global variable, any caller should check if this is !NULL before 
they call that function.

Thanks
Tiejun

> data should access the data rather than calling a (fake) function.
>
> Jan
>
>