[Qemu-devel] [PATCH] build: introduce target CONFIG_ variables and use them for kvm

[Qemu-devel] [PATCH] build: introduce target CONFIG_ variables and use them for kvm

[Anthony Liguori]

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
+
 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
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH] build: introduce target CONFIG_ variables and use them for kvm

[Andreas Färber]

Am 20.06.2012 16:44, schrieb Anthony Liguori:
> 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
> +
>  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

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.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] build: introduce target CONFIG_ variables and use them for kvm

[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

-- PMM

Re: [Qemu-devel] [PATCH] build: introduce target CONFIG_ variables and use them for kvm

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] build: introduce target CONFIG_ variables and use them for kvm

[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.

-- PMM

Re: [Qemu-devel] [PATCH] build: introduce target CONFIG_ variables and use them for kvm

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] build: introduce target CONFIG_ variables and use them for kvm

[Anthony Liguori]

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
>

Re: [Qemu-devel] [PATCH] build: introduce target CONFIG_ variables and use them for kvm

[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
>
> 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

Re: [Qemu-devel] [PATCH] build: introduce target CONFIG_ variables and use them for kvm

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] build: introduce target CONFIG_ variables and use them for kvm

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] build: introduce target CONFIG_ variables and use them for kvm

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] build: introduce target CONFIG_ variables and use them for kvm

[Paolo Bonzini]

(Sorry for breaking the thread).

> This avoids the problem associated with having multiple target specific files
> in a single directory with the current build system.

What is exactly the problem?

I saw something about dependencies, I think that should be solved with
something like

$(foreach var, $(nested-vars), $(eval -include $(patsubst %.o, %.d, $($(var)))))

at the very end of unnest-vars.

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

The goal should be to limit hw/$BASE_ARCH/Makefile.objs to hardware
that is CPU-dependent and to board descriptions.

I _think_ (but I don't have a checkout at hand) that hardware like
virtio can use obj-$(CONFIG_VIRTIO) while staying in hw/Makefile.objs,
but it should really be the only case of target-dependent file in hw/.
 Everything else in hw/$BASE_ARCH should move to target-$BASE_ARCH/hw.
 The steps should be as follows:

1) Identify more groups of hardware that can be moved from
hw/$BASE_ARCH to libhw. Move them.

2) At this point, hw/$BASE_ARCH/Makefile.objs should only refer to a)
boards b) hardware that is CPU dependent c) KVM device models with
host dependencies. Move the sources to hw/$BASE_ARCH, possibly
hw/$BASE_ARCH/kvm, and remove the addprefix invocations from
hw/$BASE_ARCH/Makefile.objs.

3) Move hw/$BASE_ARCH to target-$BASE_ARCH/hw.

I think CONFIG_$BASE_ARCH is a bad idea because it violates the
modularity that Juan introduced together with the config-devices.mak
files.

Paolo

Re: [Qemu-devel] [PATCH] build: introduce target CONFIG_ variables and use them for kvm

[Anthony Liguori]

On 06/21/2012 07:31 AM, Paolo Bonzini wrote:
> (Sorry for breaking the thread).
>
>> This avoids the problem associated with having multiple target specific files
>> in a single directory with the current build system.
>
> What is exactly the problem?

Peter's got an ARM specific KVM device he wants to stick in hw/kvm.

Right now, kvm/ is all x86 specific and wants CONFIG_KVM && CONFIG_I386.  We 
gets this by doing:

hw/${TARGET}/Makefile.objs  <=  CONFIG_I386

And then within Makefile.objs:

obj-$(CONFIG_KVM) += kvm/

But this applies for the whole directory.  Previously, you did:

obj-$(CONFIG_KVM) += kvm/apic.o kvm/clock.o ...

The way you did it made it possible for hw/arm/Makefile.obj to have a different 
set of objects but also didn't use sub directory makefiles.

So this patch allows us to achieve CONFIG_KVM && CONFIG_I386 by doing:

hw/Makefiles.objs:

obj-$(CONFIG_KVM) += kvm/

hw/kvm/Makefiles.obj:

obj-$(CONFIG_I386) += apic.o clock.o

Which I think is actually more straight forward.  This gives us the logic we 
need and let's use us subdirectory makefiles too.

>
> I saw something about dependencies, I think that should be solved with
> something like
>
> $(foreach var, $(nested-vars), $(eval -include $(patsubst %.o, %.d, $($(var)))))
>
> at the very end of unnest-vars.

Already added that BTW :-)  That's a different thread.

>> We can eventually get rid of the hw/$BASE_ARCH/Makefiles.obj files too
>
> The goal should be to limit hw/$BASE_ARCH/Makefile.objs to hardware
> that is CPU-dependent and to board descriptions.
>
> I _think_ (but I don't have a checkout at hand) that hardware like
> virtio can use obj-$(CONFIG_VIRTIO) while staying in hw/Makefile.objs,
> but it should really be the only case of target-dependent file in hw/.
>   Everything else in hw/$BASE_ARCH should move to target-$BASE_ARCH/hw.
>   The steps should be as follows:

Yup, I'm trying to refactor some of this.

Most of what's in hw/$BASE_ARCH today is really just hardware that doesn't apply 
to any other targets but is not truly target specific.

We could introduce per-device CONFIG variables and update all of the 
default-configs/ but that's a big pain for marginal benefit.  Instead, I think 
what we want in the long term is to have machine-specific CONFIG variables. 
Something like CONFIG_PC or CONFIG_VERSITALE.

But in the very short term, CONFIG_I386 makes a good stepping stone to CONFIG_PC 
and let's use refactor the Makefiles such that we can introduce more granular 
CONFIG_* down the road without changing object locations.

I think that's really the way to think of it.  We start with big guards 
(CONFIG_I386) and over time break them down into smaller guards (CONFIG_PC).

> 1) Identify more groups of hardware that can be moved from
> hw/$BASE_ARCH to libhw. Move them.
>
> 2) At this point, hw/$BASE_ARCH/Makefile.objs should only refer to a)
> boards b) hardware that is CPU dependent c) KVM device models with
> host dependencies. Move the sources to hw/$BASE_ARCH, possibly
> hw/$BASE_ARCH/kvm, and remove the addprefix invocations from
> hw/$BASE_ARCH/Makefile.objs.
>
> 3) Move hw/$BASE_ARCH to target-$BASE_ARCH/hw.
>
> I think CONFIG_$BASE_ARCH is a bad idea because it violates the
> modularity that Juan introduced together with the config-devices.mak
> files.

On the contrary, I think it's the easiest way to improve our modularity.  See above.

Regards,

Anthony Liguori

>
> Paolo

Re: [Qemu-devel] [PATCH] build: introduce target CONFIG_ variables and use them for kvm

[Paolo Bonzini]

>     What is exactly the problem?
>
> Peter's got an ARM specific KVM device he wants to stick in hw/kvm.

Can't it go in hw/arm/kvm (mimicking the final desired place, which
will be target-arm/hw/kvm)? And hw/kvm can be moved to hw/i386/kvm, or
we can leave it there for now until we're ready to move it to
target-i386/hw/kvm.

Anyway, not a big deal since you pointed out another (much better :))
reason to have CONFIG_$ARCH/CONFIG_$BASE_ARCH.

> The way you did it made it possible for hw/arm/Makefile.obj to have a
> different set of objects but also didn't use sub directory makefiles.

Usage of subdirectory makefiles should be treated like a convenience, it isn't
mandatory.  You added hw/kvm/Makefile.objs in the patch that fixed
dependency inclusion, hence my confusion.  Please try to keep patches
separate for ease of review (especially because you then have separate
commit messages and separate justification for the various bits).

> But in the very short term, CONFIG_I386 makes a good stepping stone to CONFIG_PC > and let's use refactor the Makefiles such that we can introduce more granular CONFIG_* > down the road without changing object locations.

Indeed, that's a good idea.  However, your patch still won't add the
architecture
configuration variables to config-all-devices.mak (or it could be a new file
config-all-archs.mak).  This is necessary to move files into hw/Makefile.objs
that were in Makefile.target.

Paolo

Re: [Qemu-devel] [PATCH] build: introduce target CONFIG_ variables and use them for kvm

[Peter Maydell]

On 23 June 2012 08:53, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Peter's got an ARM specific KVM device he wants to stick in hw/kvm.
>
> Can't it go in hw/arm/kvm (mimicking the final desired place, which
> will be target-arm/hw/kvm)? And hw/kvm can be moved to hw/i386/kvm, or
> we can leave it there for now until we're ready to move it to
> target-i386/hw/kvm.

Why's the final desired place target-arm/hw/kvm ? That doesn't
make much sense to me...

-- PMM

Re: [Qemu-devel] [PATCH] build: introduce target CONFIG_ variables and use them for kvm

[Paolo Bonzini]

Il 23/06/2012 12:30, Peter Maydell ha scritto:
>> > Can't it go in hw/arm/kvm (mimicking the final desired place, which
>> > will be target-arm/hw/kvm)? And hw/kvm can be moved to hw/i386/kvm, or
>> > we can leave it there for now until we're ready to move it to
>> > target-i386/hw/kvm.
> Why's the final desired place target-arm/hw/kvm ? That doesn't
> make much sense to me...

Doesn't it have some dependency on target-arm/kvm.c?

Paolo

Re: [Qemu-devel] [PATCH] build: introduce target CONFIG_ variables and use them for kvm

[Peter Maydell]

On 1 July 2012 14:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 23/06/2012 12:30, Peter Maydell ha scritto:
>>> > Can't it go in hw/arm/kvm (mimicking the final desired place, which
>>> > will be target-arm/hw/kvm)? And hw/kvm can be moved to hw/i386/kvm, or
>>> > we can leave it there for now until we're ready to move it to
>>> > target-i386/hw/kvm.
>> Why's the final desired place target-arm/hw/kvm ? That doesn't
>> make much sense to me...
>
> Doesn't it have some dependency on target-arm/kvm.c?

Well, it does at the moment, but I'm not entirely sure it
should (this is on my list of things to look at). I would
expect that "insert interrupt into the KVM kernel irqchip"
should be a generic interface the same way that KVM hooks
into cpu_interrupt(), only it doesn't seem to be handled
that way for eg PPC. Having device models in hw/ make
direct calls to per-target KVM functions in target-*/kvm.c
seems like a bit of a layering violation to me.

-- PMM