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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> 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.

Thanks
Kevin

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