Re: [Qemu-devel] [RFC PATCH 01/10] configure: factor out list of supported Xen/KVM targets

[Stefano Stabellini]

On Mon, 17 Sep 2012, Peter Maydell wrote:
> On 17 September 2012 17:00, Paolo Bonzini <address@hidden> wrote:
> > Signed-off-by: Paolo Bonzini <address@hidden>
> > ---
> >  configure | 63 
> > +++++++++++++++++++++++++++++++++++++--------------------------
> >  1 file modificato, 37 inserzioni(+), 26 rimozioni(-)
> >
> > diff --git a/configure b/configure
> > index 7e23309..cea8db0 100755
> > --- a/configure
> > +++ b/configure
> > @@ -3664,6 +3664,31 @@ if test "$linux" = "yes" ; then
> >      fi
> >  fi
> >
> > +supported_kvm_target() {
> > +    test "$kvm" = "yes" || return 1
> > +    test "$target_softmmu" = "yes" || return 1
> > +    case "$target_arch2:$cpu" in
> > +        i386:i386 | i386:x86_64 | x86_64:i386 | x86_64:x86_64 | \
> > +        ppc:ppc | ppcemb:ppc | ppc64:ppc | \
> > +        ppc:ppc64 | ppcemb:ppc64 | ppc64:ppc64 | \
> > +        s390x:s390x)
> > +            return 0
> > +        ;;
> > +    esac
> > +    return 1
> > +}
> > +
> > +supported_xen_target() {
> > +    test "$xen" = "yes" || return 1
> > +    test "$target_softmmu" = "yes" || return 1
> > +    case "$target_arch2:$cpu" in
> > +        i386:i386 | i386:x86_64 | x86_64:i386 | x86_64:x86_64)
> > +            return 0
> > +        ;;
> > +    esac
> > +    return 1
> > +}
> 
> This is a change in behaviour, isn't it? The old configure code
> enables CONFIG_XEN based only on $target_arch2, it doesn't try
> to ensure that the host and target CPU are the same architecture.
> Granted, that looks like a bug, but it should probably be fixed
> in a separate patch to this kind of refactoring patch.

That's because the target cpu is irrelevant, QEMU never sees it (it is
only a device emulator on Xen).
I have no problems with the introduction of supported_xen_target(), but
I would prefer if you could avoid $cpu tests.


> >  for target in $target_list; do
> >  target_dir="$target"
> >  config_target_mak=$target_dir/config-target.mak
> > @@ -3886,38 +3911,24 @@ if [ "$TARGET_ABI_DIR" = "" ]; then
> >    TARGET_ABI_DIR=$TARGET_ARCH
> >  fi
> >  echo "TARGET_ABI_DIR=$TARGET_ABI_DIR" >> $config_target_mak
> > -case "$target_arch2" in
> > -  i386|x86_64)
> > -    if test "$xen" = "yes" -a "$target_softmmu" = "yes" ; then
> > -      target_phys_bits=64
> > -      echo "CONFIG_XEN=y" >> $config_target_mak
> > -      if test "$xen_pci_passthrough" = yes; then
> > +
> > +if supported_xen_target; then
> > +    target_phys_bits=64
> 
> ...and while I'm looking at this code, this looks kind of bogus.
> If Xen requires 64 bit physaddrs it should probably just be asserting
> this here, not randomly changing target_phys_bits. In fact all the
> supported Xen target archs already have 64 bit physaddrs, so it's
> harmless. But if there ever were a target Xen arch which didn't support
> 64 bit phys addrs then the right approach would be to convert it to
> do so regardless of whether we were using Xen or not.

I have just noticed that nowadays even i386 sets target_phys_bits to 64.
In that case we can remove these two lines.