Patchwork build: introduce target CONFIG_ variables and use them for kvm

login
register
mail settings
Submitter Anthony Liguori
Date June 20, 2012, 2:44 p.m.
Message ID <1340203444-20394-1-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/166100/
State New
Headers show

Comments

Anthony Liguori - June 20, 2012, 2:44 p.m.
This avoids the problem associated with having multiple target specific files
in a single directory with the current build system.

We can eventually get rid of the hw/$BASE_ARCH/Makefiles.obj files too

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
I tried to add a nice comment to the config-target.mak that described how to
use these macros but that upset the header generation script.

So I left this out of this patch.
---
 configure             |   30 ++++++++++++++++++++----------
 hw/Makefile.objs      |    2 ++
 hw/i386/Makefile.objs |    1 -
 hw/kvm/Makefile.objs  |    2 +-
 4 files changed, 23 insertions(+), 12 deletions(-)
Peter Maydell - June 20, 2012, 3:01 p.m.
On 20 June 2012 15:51, Andreas Färber <afaerber@suse.de> wrote:
> Am 20.06.2012 16:44, schrieb Anthony Liguori:
>> --- a/hw/kvm/Makefile.objs
>> +++ b/hw/kvm/Makefile.objs
>> @@ -1 +1 @@
>> -obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o
>> +obj-$(CONFIG_I386) += clock.o apic.o i8259.o ioapic.o i8254.o
>
> NACK. As long as the CPUState conversion is not completed (which after
> part 4 will take some time still) any user of CPUArchState must be
> compiled per target, not per base target.

I'm confused -- as far as I can tell these files are compiled per
target, eg we have both
  CC    i386-softmmu/hw/kvm/clock.o
and
  CC    x86_64-softmmu/hw/kvm/clock.o

-- PMM
Andreas Färber - June 20, 2012, 3:04 p.m.
Am 20.06.2012 17:01, schrieb Peter Maydell:
> On 20 June 2012 15:51, Andreas Färber <afaerber@suse.de> wrote:
>> Am 20.06.2012 16:44, schrieb Anthony Liguori:
>>> --- a/hw/kvm/Makefile.objs
>>> +++ b/hw/kvm/Makefile.objs
>>> @@ -1 +1 @@
>>> -obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o
>>> +obj-$(CONFIG_I386) += clock.o apic.o i8259.o ioapic.o i8254.o
>>
>> NACK. As long as the CPUState conversion is not completed (which after
>> part 4 will take some time still) any user of CPUArchState must be
>> compiled per target, not per base target.
> 
> I'm confused -- as far as I can tell these files are compiled per
> target, eg we have both
>   CC    i386-softmmu/hw/kvm/clock.o
> and
>   CC    x86_64-softmmu/hw/kvm/clock.o

Currently yes, but with this patch Anthony seems to be changing this to
build in libhwX, no? Cf. hw/Makefile.objs.

Andreas
Peter Maydell - June 20, 2012, 3:07 p.m.
On 20 June 2012 16:04, Andreas Färber <afaerber@suse.de> wrote:
> Am 20.06.2012 17:01, schrieb Peter Maydell:
>> I'm confused -- as far as I can tell these files are compiled per
>> target, eg we have both
>>   CC    i386-softmmu/hw/kvm/clock.o
>> and
>>   CC    x86_64-softmmu/hw/kvm/clock.o
>
> Currently yes, but with this patch Anthony seems to be changing this to
> build in libhwX, no? Cf. hw/Makefile.objs.

Those quoted CC lines are the output of a build with this patch applied.
The patch adds kvm/ to obj-y, not hw-obj-y.

-- PMM
Peter Maydell - June 20, 2012, 3:09 p.m.
On 20 June 2012 15:44, Anthony Liguori <aliguori@us.ibm.com> wrote:
> This avoids the problem associated with having multiple target specific files
> in a single directory with the current build system.
>
> We can eventually get rid of the hw/$BASE_ARCH/Makefiles.obj files too
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
> I tried to add a nice comment to the config-target.mak that described how to
> use these macros but that upset the header generation script.
>
> So I left this out of this patch.
> ---
>  configure             |   30 ++++++++++++++++++++----------
>  hw/Makefile.objs      |    2 ++
>  hw/i386/Makefile.objs |    1 -
>  hw/kvm/Makefile.objs  |    2 +-
>  4 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/configure b/configure
> index b68c0ca..f07c464 100755
> --- a/configure
> +++ b/configure
> @@ -3684,19 +3684,29 @@ case "$target_arch2" in
>   ;;
>  esac
>
> -echo "TARGET_SHORT_ALIGNMENT=$target_short_alignment" >> $config_target_mak
> -echo "TARGET_INT_ALIGNMENT=$target_int_alignment" >> $config_target_mak
> -echo "TARGET_LONG_ALIGNMENT=$target_long_alignment" >> $config_target_mak
> -echo "TARGET_LLONG_ALIGNMENT=$target_llong_alignment" >> $config_target_mak
> -echo "TARGET_ARCH=$TARGET_ARCH" >> $config_target_mak
> -target_arch_name="`echo $TARGET_ARCH | LC_ALL=C tr '[a-z]' '[A-Z]'`"
> -echo "TARGET_$target_arch_name=y" >> $config_target_mak
> -echo "TARGET_ARCH2=$target_arch2" >> $config_target_mak
> -echo "TARGET_BASE_ARCH=$TARGET_BASE_ARCH" >> $config_target_mak
> +upper() {
> +    echo "$@" | LC_ALL=C tr '[a-z]' '[A-Z]'
> +}
> +
> +target_arch_name="`upper $TARGET_ARCH`"
>  if [ "$TARGET_ABI_DIR" = "" ]; then
>   TARGET_ABI_DIR=$TARGET_ARCH
>  fi
> -echo "TARGET_ABI_DIR=$TARGET_ABI_DIR" >> $config_target_mak
> +
> +cat <<EOF >> $config_target_mak
> +TARGET_SHORT_ALIGNMENT=$target_short_alignment
> +TARGET_INT_ALIGNMENT=$target_int_alignment
> +TARGET_LONG_ALIGNMENT=$target_long_alignment
> +TARGET_LLONG_ALIGNMENT=$target_llong_alignment
> +TARGET_ARCH=$TARGET_ARCH
> +TARGET_$target_arch_name=y
> +TARGET_ARCH2=$target_arch2
> +TARGET_BASE_ARCH=$TARGET_BASE_ARCH
> +TARGET_ABI_DIR=$TARGET_ABI_DIR
> +CONFIG_`upper $TARGET_BASE_ARCH`=y
> +CONFIG_`upper $TARGET_ARCH`=y
> +EOF
> +

I'd prefer the rearrangement of the existing code to be in
a separate patch -- otherwise it's a bit hard to tell what's been
added here. A brief comment describing the difference between
TARGET_I386 and CONFIG_I386 would also be good.

>  case "$target_arch2" in
>   i386|x86_64)
>     if test "$xen" = "yes" -a "$target_softmmu" = "yes" ; then
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 3d77259..cee0e06 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -166,6 +166,8 @@ obj-$(CONFIG_VGA) += vga.o
>  obj-$(CONFIG_SOFTMMU) += device-hotplug.o
>  obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o
>

I think this blank line is superfluous.

> +obj-$(CONFIG_KVM) += kvm/
> +
>  # Inter-VM PCI shared memory
>  ifeq ($(CONFIG_PCI), y)
>  obj-$(CONFIG_KVM) += ivshmem.o

Otherwise looks good, and I've given it a quick smoke test and
it seems to DTRT.

-- PMM
Anthony Liguori - June 20, 2012, 3:20 p.m.
On 06/20/2012 09:51 AM, Andreas Färber wrote:
> Am 20.06.2012 16:44, schrieb Anthony Liguori:
>> diff --git a/hw/kvm/Makefile.objs b/hw/kvm/Makefile.objs
>> index 226497a..cf734ba 100644
>> --- a/hw/kvm/Makefile.objs
>> +++ b/hw/kvm/Makefile.objs
>> @@ -1 +1 @@
>> -obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o
>> +obj-$(CONFIG_I386) += clock.o apic.o i8259.o ioapic.o i8254.o
>
> NACK. As long as the CPUState conversion is not completed (which after
> part 4 will take some time still) any user of CPUArchState must be
> compiled per target, not per base target. It may or may not work
> depending on fields accessed, but it is architecturally wrong.

It's admittedly confusing.

obj-y => this is built per-target (there is no concept of per-base target)

hw-obj-y => this is built once for 32-bit, once for 64-bit

common-obj-y => this is built once for all targets

CONFIG_I386=y if target arch == i386 or base arch == i386.

Regards.

Anthony Liguori

>
> Andreas
>
Andreas Färber - June 20, 2012, 3:23 p.m.
Am 20.06.2012 17:07, schrieb Peter Maydell:
> On 20 June 2012 16:04, Andreas Färber <afaerber@suse.de> wrote:
>> Am 20.06.2012 17:01, schrieb Peter Maydell:
>>> I'm confused -- as far as I can tell these files are compiled per
>>> target, eg we have both
>>>   CC    i386-softmmu/hw/kvm/clock.o
>>> and
>>>   CC    x86_64-softmmu/hw/kvm/clock.o
>>
>> Currently yes, but with this patch Anthony seems to be changing this to
>> build in libhwX, no? Cf. hw/Makefile.objs.
> 
> Those quoted CC lines are the output of a build with this patch applied.
> The patch adds kvm/ to obj-y, not hw-obj-y.

Ah, the Makefile system still is confusing... if it's compiled twice I'm
okay with it.

/-F
Peter Maydell - June 28, 2012, 5:51 p.m.
On 20 June 2012 15:44, Anthony Liguori <aliguori@us.ibm.com> wrote:
> This avoids the problem associated with having multiple target specific files
> in a single directory with the current build system.
>
> We can eventually get rid of the hw/$BASE_ARCH/Makefiles.obj files too

Ping? I can't figure out if this discussion got wedged (in which
case, how should it be unwedged?) or if people were eventually happy
with this patch...

thanks
-- PMM
Andreas Färber - June 29, 2012, 12:31 p.m.
Am 28.06.2012 19:51, schrieb Peter Maydell:
> On 20 June 2012 15:44, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> This avoids the problem associated with having multiple target specific files
>> in a single directory with the current build system.
>>
>> We can eventually get rid of the hw/$BASE_ARCH/Makefiles.obj files too
> 
> Ping? I can't figure out if this discussion got wedged (in which
> case, how should it be unwedged?) or if people were eventually happy
> with this patch...

My guess is we're waiting for Paolo to return from vacation and to
comment. Basically the patch is "correct" but the open issue is how we
want to structure the directories - do we want hw/kvm/ to contain
multiple architectures' files, or do we want to separate devices by
architecture. There's reasons for both once it works technically.

If Anthony split off the kvm/ change it might be less controversial.

Andreas
Anthony Liguori - June 29, 2012, 2:04 p.m.
On 06/28/2012 12:51 PM, Peter Maydell wrote:
> On 20 June 2012 15:44, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>> This avoids the problem associated with having multiple target specific files
>> in a single directory with the current build system.
>>
>> We can eventually get rid of the hw/$BASE_ARCH/Makefiles.obj files too
>
> Ping? I can't figure out if this discussion got wedged (in which
> case, how should it be unwedged?) or if people were eventually happy
> with this patch...

I have a v2 that I will send out this afternoon.  I got a bit side tracked on 
the virtio-rng stuff.

I'm happy with this patch.  I don't think there are any major objections.

Regards,

Anthony Liguori

>
> thanks
> -- PMM
>
>
Paolo Bonzini - July 1, 2012, 2:10 p.m.
Il 29/06/2012 14:31, Andreas Färber ha scritto:
>> > Ping? I can't figure out if this discussion got wedged (in which
>> > case, how should it be unwedged?) or if people were eventually happy
>> > with this patch...
> My guess is we're waiting for Paolo to return from vacation and to
> comment. Basically the patch is "correct" but the open issue is how we
> want to structure the directories - do we want hw/kvm/ to contain
> multiple architectures' files, or do we want to separate devices by
> architecture. There's reasons for both once it works technically.
> 
> If Anthony split off the kvm/ change it might be less controversial.

I don't really care much about that, I hope it's temporary anyway. :)

However, one change is necessary: the config-all-devices.mak should also
include the arch defines (or if you don't like the naming, we could have
another config-all-arches.mak file).  It should not be hard with grep
(perhaps after renaming the variable should probably be named
CONFIG_ARCH_$ARCH).  With this change, the patch is perfectly fine!

Paolo
Peter Maydell - July 23, 2012, 2:21 p.m.
On 29 June 2012 15:04, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 06/28/2012 12:51 PM, Peter Maydell wrote:
>> On 20 June 2012 15:44, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>>> This avoids the problem associated with having multiple target specific
>>> files in a single directory with the current build system.
>>>
>>> We can eventually get rid of the hw/$BASE_ARCH/Makefiles.obj files too
>>
>> Ping? I can't figure out if this discussion got wedged (in which
>> case, how should it be unwedged?) or if people were eventually happy
>> with this patch...
>
> I have a v2 that I will send out this afternoon.

Ping? If you sent out a v2 I think I must have missed it...

thanks
-- PMM
Peter Maydell - July 31, 2012, 5:28 p.m.
On 23 July 2012 15:21, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 29 June 2012 15:04, Anthony Liguori <anthony@codemonkey.ws> wrote:
>> On 06/28/2012 12:51 PM, Peter Maydell wrote:
>>> On 20 June 2012 15:44, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>>>> This avoids the problem associated with having multiple target specific
>>>> files in a single directory with the current build system.
>>>>
>>>> We can eventually get rid of the hw/$BASE_ARCH/Makefiles.obj files too
>>>
>>> Ping? I can't figure out if this discussion got wedged (in which
>>> case, how should it be unwedged?) or if people were eventually happy
>>> with this patch...
>>
>> I have a v2 that I will send out this afternoon.
>
> Ping? If you sent out a v2 I think I must have missed it...

...ping^2?

-- PMM

Patch

diff --git a/configure b/configure
index b68c0ca..f07c464 100755
--- a/configure
+++ b/configure
@@ -3684,19 +3684,29 @@  case "$target_arch2" in
   ;;
 esac
 
-echo "TARGET_SHORT_ALIGNMENT=$target_short_alignment" >> $config_target_mak
-echo "TARGET_INT_ALIGNMENT=$target_int_alignment" >> $config_target_mak
-echo "TARGET_LONG_ALIGNMENT=$target_long_alignment" >> $config_target_mak
-echo "TARGET_LLONG_ALIGNMENT=$target_llong_alignment" >> $config_target_mak
-echo "TARGET_ARCH=$TARGET_ARCH" >> $config_target_mak
-target_arch_name="`echo $TARGET_ARCH | LC_ALL=C tr '[a-z]' '[A-Z]'`"
-echo "TARGET_$target_arch_name=y" >> $config_target_mak
-echo "TARGET_ARCH2=$target_arch2" >> $config_target_mak
-echo "TARGET_BASE_ARCH=$TARGET_BASE_ARCH" >> $config_target_mak
+upper() {
+    echo "$@" | LC_ALL=C tr '[a-z]' '[A-Z]'
+}
+
+target_arch_name="`upper $TARGET_ARCH`"
 if [ "$TARGET_ABI_DIR" = "" ]; then
   TARGET_ABI_DIR=$TARGET_ARCH
 fi
-echo "TARGET_ABI_DIR=$TARGET_ABI_DIR" >> $config_target_mak
+
+cat <<EOF >> $config_target_mak
+TARGET_SHORT_ALIGNMENT=$target_short_alignment
+TARGET_INT_ALIGNMENT=$target_int_alignment
+TARGET_LONG_ALIGNMENT=$target_long_alignment
+TARGET_LLONG_ALIGNMENT=$target_llong_alignment
+TARGET_ARCH=$TARGET_ARCH
+TARGET_$target_arch_name=y
+TARGET_ARCH2=$target_arch2
+TARGET_BASE_ARCH=$TARGET_BASE_ARCH
+TARGET_ABI_DIR=$TARGET_ABI_DIR
+CONFIG_`upper $TARGET_BASE_ARCH`=y
+CONFIG_`upper $TARGET_ARCH`=y
+EOF
+
 case "$target_arch2" in
   i386|x86_64)
     if test "$xen" = "yes" -a "$target_softmmu" = "yes" ; then
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index 3d77259..cee0e06 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -166,6 +166,8 @@  obj-$(CONFIG_VGA) += vga.o
 obj-$(CONFIG_SOFTMMU) += device-hotplug.o
 obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o
 
+obj-$(CONFIG_KVM) += kvm/
+
 # Inter-VM PCI shared memory
 ifeq ($(CONFIG_PCI), y)
 obj-$(CONFIG_KVM) += ivshmem.o
diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
index eb171b7..14738e5 100644
--- a/hw/i386/Makefile.objs
+++ b/hw/i386/Makefile.objs
@@ -7,7 +7,6 @@  obj-y += debugcon.o multiboot.o
 obj-y += pc_piix.o
 obj-y += pc_sysfw.o
 obj-$(CONFIG_XEN) += xen_platform.o xen_apic.o
-obj-y += kvm/
 obj-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
 
 obj-y := $(addprefix ../,$(obj-y))
diff --git a/hw/kvm/Makefile.objs b/hw/kvm/Makefile.objs
index 226497a..cf734ba 100644
--- a/hw/kvm/Makefile.objs
+++ b/hw/kvm/Makefile.objs
@@ -1 +1 @@ 
-obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o
+obj-$(CONFIG_I386) += clock.o apic.o i8259.o ioapic.o i8254.o