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:55, <tiejun.chen@xxxxxxxxx> wrote:
> On 2014/11/3 17:45, Jan Beulich wrote:
>>>>> 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?
> 
> 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.

Jan


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