| Message ID | 1340203444-20394-1-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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 >
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
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
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
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 > >
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
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
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
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
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(-)