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, 31 Oct 2014 10:20:55 +0800 Message-ID: <5452F207.1070105@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54520F2C0200007800043625@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/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: >>>> On 2014/10/28 17:48, Jan Beulich wrote: >>>>>>>> On 28.10.14 at 06:21, wrote: >>>>>> On 2014/10/27 17:45, Jan Beulich wrote: >>>>>>>>>> On 27.10.14 at 04:12, wrote: >>>>>>>> On 2014/10/24 22:22, Jan Beulich wrote: >>>>>>>>>>>> On 24.10.14 at 09:34, wrote: >>>>>>>>>> --- a/tools/firmware/hvmloader/util.h >>>>>>>>>> +++ b/tools/firmware/hvmloader/util.h >>>>>>>>>> @@ -241,6 +241,12 @@ int build_e820_table(struct e820entry *e820, >>>>>>>>>> unsigned int bios_image_base); >>>>>>>>>> void dump_e820_table(struct e820entry *e820, unsigned int nr); >>>>>>>>>> >>>>>>>>>> +#include >>>>>>>>>> +#define ENOBUFS 105 /* No buffer space available */ >>>>>>>>> >>>>>>>>> This is a joke I hope? The #include belongs at the top (albeit afaict >>>>>>>>> you don't really need it here), and the #define is completely >>>>>>>> >>>>>>>> If without this line, #include , >>>>>>>> >>>>>>>> In file included from build.c:25:0: >>>>>>>> ../util.h:246:70: error: array type has incomplete element type >>>>>>>> int get_reserved_device_memory_map(struct xen_reserved_device_memory >>>>>>>> entries[], >>>>>>>> ^ >>>>>>>> make[8]: *** [build.o] Error 1 >>>>>>> >>>>>>> So just forward declare the structure ahead of the function >>>>>>> declaration. >>>>>> >>>>>> tools/firmware/hvmloader/pci.c:28:#include >>>>>> tools/firmware/hvmloader/ovmf.c:36:#include >>>>>> >>>>>> So any reason I can't do such a same thing? >>>>> >>>>> You can, but it's undesirable. You're wanting this in a header, i.e. >>>>> you'll make everyone consuming that header also implicitly depend >>>>> on the new header you would include. We shouldn't pointlessly >>>>> add build dependencies (and we should really try to reduce them >>>>> where possible). >>>> >>>> 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); since this is enough to pci_setup() and construct_rdm_e820_maps(). tools/firmware/hvmloader/util.c: struct xen_reserved_device_memory *rdm_map; static int get_reserved_device_memory_map(struct xen_reserved_device_memory entries[], uint32_t *max_entries) { ... } unsigned int hvm_get_reserved_device_memory_map(void) { ... } > >> So all tools maintainers, >> >> Could you give us such a comment? >> >> 'ln -sf $(XEN_ROOT)/xen/include/xen/errno.h xen' versus >> 'ln -sf $(XEN_ROOT)/xen/include/xen/errno.h >> $(XEN_ROOT)/tools/firmware/hvmloader/, > > I think if it is (as suggested) to be limited to hvmloader, the > symlinking also should be done in its Makefile rather than along > with other tools stuff. You're right. I just send out a separate patch to ask a review from tools side, and CCed you. Thanks Tiejun