From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v7][RFC][PATCH 04/13] hvmloader/util: get reserved device memory maps Date: Fri, 21 Nov 2014 16:54:18 +0800 Message-ID: <546EFDBA.6020008@intel.com> References: <1414136077-18599-1-git-send-email-tiejun.chen@intel.com> <1414136077-18599-5-git-send-email-tiejun.chen@intel.com> <544A7CCB0200007800041FBA@mail.emea.novell.com> <544DB809.9020108@intel.com> <544E22410200007800042568@mail.emea.novell.com> <544F27BD.7060003@intel.com> <544F749A0200007800042B74@mail.emea.novell.com> <54508F1B.2030903@intel.com> <5450BBF50200007800043032@mail.emea.novell.com> <5451D2B5.50609@intel.com> <54520F2C0200007800043625@mail.emea.novell.com> <5452F207.1070105@intel.com> <545352F40200007800043D82@mail.emea.novell.com> <5456E6E9.1080104@intel.com> <545750AA02000078000443AD@mail.emea.novell.com> <54574BC0.7000709@intel.com> <54575CD60200007800044426@mail.emea.novell.com> <545750FA.9090001@intel.com> <545760B60200007800044471@mail.emea.novell.com> <546EDB0D.8030605@intel.com> <546EFDB902000078000499E6@smtp.nue.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <546EFDB902000078000499E6@smtp.nue.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Kevin Tian Cc: "wei.liu2@citrix.com" , "ian.campbell@citrix.com" , "stefano.stabellini@eu.citrix.com" , "tim@xen.org" , "ian.jackson@eu.citrix.com" , "xen-devel@lists.xen.org" , Yang Z Zhang List-Id: xen-devel@lists.xenproject.org On 2014/11/21 15:54, Jan Beulich wrote: >>>> On 21.11.14 at 08:43, 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, wrote: >>>>> On 2014/11/3 17:45, Jan Beulich wrote: >>>>>>>>> On 03.11.14 at 10:32, wrote: >>>>>>> On 2014/11/3 16:53, Jan Beulich wrote: >>>>>>>>>>> On 03.11.14 at 03:22, wrote: >>>>>>>>> On 2014/10/31 16:14, Jan Beulich wrote: >>>>>>>>>>>>> On 31.10.14 at 03:20, wrote: >>>>>>>>>>> On 2014/10/30 17:13, Jan Beulich wrote: >>>>>>>>>>>>>>> On 30.10.14 at 06:55, wrote: >>>>>>>>>>>>> On 2014/10/29 17:05, Jan Beulich wrote: >>>>>>>>>>>>>>>>> On 29.10.14 at 07:54, 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 >>>>>>>>>>>>> ', >>>>>>>>>>>>> >>>>>>>>>>>>> #include "util.h" >>>>>>>>>>>>> ... >>>>>>>>>>>>> #include >>>>>>>>>>>>> >>>>>>>>>>>>> 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 > >