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

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

> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Friday, November 21, 2014 3:54 PM
> 
> >>> 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.
> 

that's what I meant.

Thanks
Kevin

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