Re: [Xen-devel] [PATCHv6] 02/28] build: build Kconfig and config rules

[Doug Goldstein <cardoe@xxxxxxxxxx>]

On 12/3/15 2:57 AM, Jan Beulich wrote:
>>>> On 03.12.15 at 01:34, <cardoe@xxxxxxxxxx> wrote:
>> On 12/1/15 5:22 AM, Jan Beulich wrote:
>>>>>> On 30.11.15 at 18:53, <cardoe@xxxxxxxxxx> wrote:
>>>> On 11/30/15 8:36 AM, Jan Beulich wrote:
>>>>>>>> On 24.11.15 at 18:51, <cardoe@xxxxxxxxxx> wrote:
>>>>>> +mainmenu "Xen/$SRCARCH $XEN_FULLVERSION Configuration"
>>>>>> +
>>>>>> +config SRCARCH
>>>>>> +        string
>>>>>> +        option env="SRCARCH"
>>>>>> +
>>>>>> +config ARCH
>>>>>> +        string
>>>>>> +        option env="ARCH"
>>>>>> +
>>>>>> +source "arch/$SRCARCH/Kconfig"
>>>>>> +
>>>>>> +config XEN_FULLVERSION
>>>>>> +        string
>>>>>> +        option env="XEN_FULLVERSION"
>>>>>> +
>>>>>> +config DEFCONFIG_LIST
>>>>>> +        string
>>>>>> +        option defconfig_list
>>>>>> +        default "$ARCH_DEFCONFIG"
>>>>>> +        default "arch/$SRCARCH/defconfig"
>>>>>
>>>>> Linux'es variant has just SRCARCH and the source directive. Why
>>>>> do we need so much more here? In any even I again think you
>>>>> should at least briefly explain any extensions you do in the commit
>>>>> message.
>>>>
>>>> I'm not understanding the question here. Linux has 5 items in their
>>>> list. The last two are identical to the 2 that I retained. Given other
>>>> evolutions of the patchset I can drop the last one and keep it as just
>>>> $ARCH_DEFCONFIG since I've always got that defined for arm and x86.
>>>
>>> Oh, the context was ambiguous: The comment related to everything
>>> quoted, not just the last option. I.e. I'm being irritated by there being
>>> quite a few more config options here compared to Linux.
>>
>> So all these items are in Linux. And they seem to actually be necessary.
>> You'll just find them in different files (e.g. init/Kconfig) that I
>> didn't import because we really didn't need them for anything.
> 
> Okay, this makes sense. Remains the question on the
> XEN_FULLVERSION name: Doesn't this collide with the identically
> named make variable? Or is it intentionally named the same? In
> which case - what's the purpose? There's no
> CONFIG_KERNELVERSION resulting from this in Linux (which seems
> in line with documentation).

This is intentional. Its for when you run "make {menu,n,g,x}config" and
it'll show the version info at the top. It also puts the version info as
a comment inside the .config. Just cosmetic stuff.

> 
>>>>>> --- a/xen/Makefile
>>>>>> +++ b/xen/Makefile
>>>>>> @@ -17,6 +17,9 @@ export XEN_ROOT := $(BASEDIR)/..
>>>>>>  
>>>>>>  EFI_MOUNTPOINT ?= $(BOOT_DIR)/efi
>>>>>>  
>>>>>> +# Don't break if the build process wasn't called from the top level
>>>>>> +XEN_TARGET_ARCH ?= $(shell uname -m)
>>>>>
>>>>> I don't see the need for this. We already require setting this on the
>>>>> command line when invoking a build process bypassing the top level
>>>>> directory. Later in the series, when actually using the produced
>>>>> xen/.config, I could see this becoming a nice enhancement (by
>>>>> reading the value from the file).
>>>>
>>>> One of the early on reviews told me it was not required (and I can
>>>> confirm that I can run "git clean -xdf && cd xen && make" and it will
>>>> build). This change makes that behavior still work.
>>>
>>> Yes, non-cross builds of course have been working without setting
>>> that variable. I was (not clearly enough) referring to cross builds,
>>> including hypervisor builds on ix86 distros (which I consider at best
>>> kind of cross, but would really deem to be native with there not
>>> being any 32-bit hypervisor anymore).
>>>
>>> So can you clarify what exactly breaks in which way without this
>>> change?
>>
>> The ability to "git clone git://xen && cd xen/xen && make" because I
>> couldn't pick the correct defconfig to use. See the rules just below
>> this reply in xen/Makefile where they are used.
> 
> Okay. But - does "uname -m" really produce e.g. "arm32" and "arm64"
> respectively, and not, as I would expect, "armv..." and "aarch64"?
> See the setup of XEN_COMPILE_ARCH in ./Config.mk.

I've switched to pulling in Config.mk

> 
>>>>>> @@ -216,3 +220,16 @@ FORCE:
>>>>>>  
>>>>>>  %/: FORCE
>>>>>>          $(MAKE) -f $(BASEDIR)/Rules.mk -C $* built_in.o built_in_bin.o
>>>>>> +
>>>>>> +kconfig := silentoldconfig oldconfig config menuconfig defconfig \
>>>>>> +        nconfig xconfig gconfig savedefconfig listnewconfig olddefconfig
>>>>>> +.PHONY: $(kconfig)
>>>>>> +$(kconfig):
>>>>>> +        $(MAKE) -f $(BASEDIR)/scripts/kconfig/Makefile 
>>>>>> ARCH=$(XEN_TARGET_ARCH) $@
>>>>>> +
>>>>>> +$(BASEDIR)/include/config/%.conf: 
>>>>>> $(BASEDIR)/include/config/auto.conf.cmd
>>>>>> +        $(Q)$(MAKE) -f $(BASEDIR)/scripts/kconfig/Makefile 
>>>>>> ARCH=$(XEN_TARGET_ARCH) silentoldconfig
>>>>>
>>>>> Okay, I have found the Linux original to this and it's the same there,
>>>>> but I'd like to ask anyway: How can this work without $* getting
>>>>> passed on?
>>>>
>>>> silentoldconfig is the step that actually takes the .config and
>>>> generates out all the files like auto.conf and generated/config.h It
>>>> should be in xen/scripts/kconfig/Makefile.linux I believe.
>>>
>>> Not sure what you're trying to tell me, but in any event I don't see
>>> this to answer the question.
>>
>> So maybe I'm not understanding your question. I don't understand why $*
>> would need to be passed on. The target "silentoldconfig" in the kconfig
>> Makefile takes care of building all of the files that the rule could
>> match. That's what I tried to say in my original response. I believe
>> your point is that I would have to pass the target file on like the
>> below implicit case?
> 
> Yes, that's what one would expect from a pattern rule. And yes, now
> I understand your answer to the original question.
> 
>>>>>> +config ARCH_DEFCONFIG
>>>>>> +        string
>>>>>> +        default "arch/x86/configs/x86_64_defconfig"
>>>>>
>>>>> x86_defconfig perhaps?
>>>>
>>>> No. I was told to drop support for x86 entirely in an earlier review.
>>>> Its not possible to configure for 32-bit x86 in v6.
>>>
>>> x86 != 32-bit. I think you're mixing this up with ix86 or x86-32.
>>> Here I consider x86 as to basic architecture without any
>>> particular bit width in mind.
>>
>> ok. Well the syntax is still "arch/SUBARCH/configs/ARCH_defconfig" so
>> the original is correct. There is no defconfig for the ambiguous x86
>> family. You're either building for x86_64 or x86_32 (which I referred to
>> as x86 in my original response).
>>
>> This defconfig is for the 64-bit architecture of x86 (x86_64) and there
>> for its named correctly.
> 
> But there is no x86_32 architecture form the hypervisor build's
> point of view, and hence x86 isn't ambiguous. In fact the mid-term
> plan is to remove leftovers of references to x86_64 (like the
> arch/x86/x86_64/ or include/asm-x86/x86_64/ directories) where
> possible. The only place they need to be kept are in the public
> interface.

That's fine but you don't build things for "x86". You build them for
"x86_64". XEN_TARGET_ARCH takes in "x86_64".

> 
>>>>>> +# Set our default defconfig file
>>>>>> +KBUILD_DEFCONFIG := $(ARCH)_defconfig
>>>>>> +
>>>>>> +# provide our shell
>>>>>> +CONFIG_SHELL := $(SHELL)
>>>>>> +
>>>>>> +# provide the host compiler
>>>>>> +HOSTCC := gcc
>>>>>> +HOSTCXX := g++
>>>>>
>>>>> Why is the C setting from ./Config.mk not good enough (the C++
>>>>> one should then be added there if indeed needed, which I doubt)?
>>>>
>>>> From an earlier review I dropped the usage of Config.mk because I was
>>>> told the hypervisor (xen/) should build without ./configure being run.
>>>> The C++ is needed for the kconfig frontends.
>>>
>>> But Config.mk has nothing to do with top level configure, and is e.g.
>>> being included from xen/Rules.mk.
>>
>> I'm not including Config.mk in those build files because it includes a
>> lot and many of them are redefinitions of what kconfig does. All I would
>> get out of it would be HOSTCC.
>>
>> $ wc -l config/x86_64.mk config/Linux.mk config/StdGNU.mk Config.mk
>>    32 config/x86_64.mk
>>     3 config/Linux.mk
>>    50 config/StdGNU.mk
>>   287 Config.mk
>>   372 total
>>
>> If there was a bit more granularity (config/host.mk) it wouldn't be so bad.
> 
> Indeed introducing that would seem like a useful intermediate step.
> No multiple points of defining the same thing please (because of
> someone actually needing to fiddle with them will quite likely touch
> only one instance, resulting in time wasted figuring out why things
> no longer work uniformly).
> 
> Jan
> 

I can submit that outside of this series before the next repost.

-- 
Doug Goldstein

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel