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: Mon, 03 Nov 2014 17:32:48 +0800 Message-ID: <54574BC0.7000709@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <545750AA02000078000443AD@mail.emea.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 Cc: kevin.tian@intel.com, 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@intel.com List-Id: xen-devel@lists.xenproject.org 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. Thanks Tiejun