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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.11.14 at 08:43, <kevin.tian@xxxxxxxxx> 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@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.
>> >
>> 
>> 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
data should access the data rather than calling a (fake) function.

Jan

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