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

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

On 2014/11/21 15:54, Jan Beulich wrote:
> > > > 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
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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel