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: Wed, 29 Oct 2014 14:54:19 +0800 Message-ID: <54508F1B.2030903@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <544F749A0200007800042B74@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: yang.z.zhang@intel.com, kevin.tian@intel.com, tim@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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. > >>>>> misplaced here. While I generally wouldn't recommend doing this, I >>>>> think in the case here including the hypervisor header that defines >>>>> them would be okay. Perhaps not via relative path, but via having So is the following is a way "via having the Makefile symlink the hypervisor header here."? --- a/tools/include/Makefile +++ b/tools/include/Makefile @@ -17,6 +17,7 @@ xen/.dir: ln -sf ../xen-sys/$(XEN_OS) xen/sys ln -sf $(addprefix $(XEN_ROOT)/xen/include/xen/,libelf.h elfstructs.h) xen/libelf/ ln -s ../xen-foreign xen/foreign + ln -sf $(XEN_ROOT)/xen/include/xen/errno.h xen touch $@ .PHONY: install Then we just need include this in util.c: --- a/tools/firmware/hvmloader/util.c +++ b/tools/firmware/hvmloader/util.c @@ -26,6 +26,7 @@ #include #include #include +#include void wrmsr(uint32_t idx, uint64_t v) { Thanks Tiejun >>>> >>>> Seems we just need to include this, >>>> >>>> #include >>> >>> You shouldn't include system headers here - what if the build system's >>> -E... values differ from Xen's? Please remember that what your making >> >> tools/firmware/hvmloader/xenbus.c:30:#include > > This is a completely different case: For one, no-one really looks at > the error codes generated here. And even if someone would, these > would be error value purely internal to hvmloader. Whereas in your > case you want to interpret a value you get back from the hypervisor. > >> And why will Xen define this different? > > Why would Linux, *BSD, Solaris, and whatever else OS usable as a > build host for building Xen all be required to use exactly the same > -E... definitions when already Linux has variations for some of them > depending on host architecture (i.e. when doing cross builds you'd > risk running into problems even on Linux)? > > Once again - please always keep in mind that you're modifying > hypervisor code, not some simple user mode application. > > Jan > > >