Re: [RFC PATCH] os-android: Add support to android platform, built by ndk-r10

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 15/09/2015 04:11, Houcheng Lin wrote:
> The OS dependent code for android that implement functions missing in bionic C, including:
>     - getdtablesize(): call getrlimit() instead.

This is okay and can be done unconditionally (introduce a new
qemu_getdtablesize function that is defined in util/oslib-posix.c).

>     - a_ptsname(): call pstname_r() instead.

This is not necessary, see below.

>     - sigtimewait(): call __rt_sigtimewait() instead.
>     - lockf(): not see this feature in android, directly return -1.
>     - shm_open(): not see this feature in android, directly return -1.

This is not okay.  Please fix your libc instead.

> The kernel headers for android include two kernel header missing in android: scsi/sg.h and
> sys/io.h.

For sys/io.h, we can just remove the inclusion.  It is not necessary.

scsi/sg.h should be exported by the Linux kernel, so that we can use
scripts/update-linux-headers.sh to copy it from QEMU.  I've sent a Linux
kernel patch and CCed you.

More comments follow:

> diff --git a/configure b/configure
> index 5c06663..3ff6ffa 100755
> --- a/configure
> +++ b/configure
> @@ -566,7 +566,6 @@ fi
>  
>  # host *BSD for user mode
>  HOST_VARIANT_DIR=""
> -
>  case $targetos in
>  CYGWIN*)
>    mingw32="yes"
> @@ -692,9 +691,23 @@ Haiku)
>    vhost_net="yes"
>    vhost_scsi="yes"
>    QEMU_INCLUDES="-I\$(SRC_PATH)/linux-headers -I$(pwd)/linux-headers $QEMU_INCLUDES"
> +  case $cross_prefix in
> +    *android*)
> +      android="yes"
> +      guest_agent="no"

You should instead disable the guest agent on your configure command line.

> +      QEMU_INCLUDES="-I\$(SRC_PATH)/include/android $QEMU_INCLUDES"
> +    ;;
> +    *)
> +    ;;
> +  esac
>  ;;
>  esac
>  
> +if [ "$android" = "yes" ] ; then
> +  QEMU_CFLAGS="-DANDROID $QEMU_CFLAGS"

If you have CONFIG_ANDROID, you do not need -DANDROID.

> +  LIBS="-lglib-2.0 -lgthread-2.0 -lz -lpixman-1 -lintl -liconv -lc $LIBS"
> +  libs_qga="-lglib-2.0 -lgthread-2.0 -lz -lpixman-1 -lintl -liconv -lc"

This should not be necessary, QEMU uses pkg-config.

> +fi
>  if [ "$bsd" = "yes" ] ; then
>    if [ "$darwin" != "yes" ] ; then
>      bsd_user="yes"
> @@ -1736,7 +1749,14 @@ fi
>  # pkg-config probe
>  
>  if ! has "$pkg_config_exe"; then
> -  error_exit "pkg-config binary '$pkg_config_exe' not found"
> +  case $cross_prefix in
> +    *android*)
> +	pkg_config_exe=scripts/android-pkg-config

Neither should this.  Your cross-compilation environment is not
correctly set up if you do not have a pkg-config executable.  If you
want to use a wrapper, you can specify it with the PKG_CONFIG
environment variable.  But it need not be maintained in the QEMU
repository, because QEMU assumes a complete cross-compilation environment.

> +	;;
> +    *)
> +	error_exit "pkg-config binary '$pkg_config_exe' not found"
> +	;;
> +  esac
>  fi
>  
>  ##########################################
> @@ -3764,7 +3784,7 @@ elif compile_prog "" "$pthread_lib -lrt" ; then
>  fi
>  
>  if test "$darwin" != "yes" -a "$mingw32" != "yes" -a "$solaris" != yes -a \
> -        "$aix" != "yes" -a "$haiku" != "yes" ; then
> +        "$aix" != "yes" -a "$haiku" != "yes" -a "$android" != "yes" ; then
>      libs_softmmu="-lutil $libs_softmmu"
>  fi
>  
> @@ -4709,6 +4729,10 @@ if test "$linux" = "yes" ; then
>    echo "CONFIG_LINUX=y" >> $config_host_mak
>  fi
>  
> +if test "$android" = "yes" ; then
> +  echo "CONFIG_ANDROID=y" >> $config_host_mak
> +fi
> +
>  if test "$darwin" = "yes" ; then
>    echo "CONFIG_DARWIN=y" >> $config_host_mak
>  fi
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index ab3c876..1ba22be 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -59,6 +59,10 @@
>  #define WEXITSTATUS(x) (x)
>  #endif
>  
> +#ifdef ANDROID
> + #include "sysemu/os-android.h"
> +#endif
> +
>  #ifdef _WIN32
>  #include "sysemu/os-win32.h"
>  #endif
> diff --git a/include/sysemu/os-android.h b/include/sysemu/os-android.h
> new file mode 100644
> index 0000000..7f73777
> --- /dev/null
> +++ b/include/sysemu/os-android.h
> @@ -0,0 +1,35 @@
> +#ifndef QEMU_OS_ANDROID_H
> +#define QEMU_OS_ANDROID_H
> +
> +#include <sys/statvfs.h>
> +
> +/*
> + * For include the basename prototyping in android.
> + */
> +#include <libgen.h>

These two includes can be added to sysemu/os-posix.h.

> +extern int __rt_sigtimedwait(const sigset_t *uthese, siginfo_t *uinfo, \
> +                             const struct timespec *uts, size_t sigsetsize);
> +
> +int getdtablesize(void);
> +int lockf(int fd, int cmd, off_t len);
> +int sigtimedwait(const sigset_t *set, siginfo_t *info, \
> +                 const struct timespec *ts);
> +int shm_open(const char *name, int oflag, mode_t mode);
> +char *a_ptsname(int fd);
> +
> +#define F_TLOCK         0
> +#define IOV_MAX         256

libc should really be fixed for all of these (except getdtablesize and
a_ptsname).  The way you're working around it is introducing subtle
bugs, for example the pidfile is _not_ locked.

> +#define __SID           ('S' << 8)
> +
> +#define I_NREAD     (__SID | 1) /* Counts the number of data bytes in the data
> +                                   block in the first message.  */
> +#define I_PUSH      (__SID | 2) /* Push STREAMS module onto top of the current
> +                                   STREAM, just below the STREAM head.  */
> +#define I_POP       (__SID | 3) /* Remove STREAMS module from just below the
> +                                   STREAM head.  */
> +#define I_LOOK      (__SID | 4) /* Retrieve the name of the module just below
> +                                   the STREAM head and place it in a character
> +                                   string.  */

These are not necessary.

> +#endif
> diff --git a/kvm-all.c b/kvm-all.c
> index c6f5128..3e0d726 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -45,6 +45,9 @@
>  #endif
>  
>  /* KVM uses PAGE_SIZE in its definition of COALESCED_MMIO_MAX */
> +#ifdef ANDROID
> +#undef PAGE_SIZE
> +#endif

What is the definition of PAGE_SIZE on Android?  Can we do

#ifdef PAGE_SIZE
QEMU_BUILD_BUG_ON(PAGE_SIZE != TARGET_PAGE_SIZE);
#else
#define PAGE_SIZE TARGET_PAGE_SIZE
#endif

?

>  #define PAGE_SIZE TARGET_PAGE_SIZE
>  
>  //#define DEBUG_KVM
> diff --git a/scripts/android-pkg-config b/scripts/android-pkg-config
> new file mode 100755
> index 0000000..52a0aa8
> --- /dev/null
> +++ b/scripts/android-pkg-config

See above, this should not be part of QEMU.  The reason is that QEMU
developers do not test Android builds, and the script would rot.
Instead, you should document how to prepare a complete environment to
cross-compile QEMU for Android.

> diff --git a/tests/Makefile b/tests/Makefile
> index 34c6136..99faf1f 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -418,8 +418,10 @@ tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o libqemuutil.a libqemustub.
>  tests/test-write-threshold$(EXESUF): tests/test-write-threshold.o $(block-obj-y) libqemuutil.a libqemustub.a
>  
>  ifeq ($(CONFIG_POSIX),y)
> +ifneq ($(CONFIG_ANDROID),y)
>  LIBS += -lutil
>  endif
> +endif

This is okay.

>  
>  # QTest rules
>  
> diff --git a/util/osdep.c b/util/osdep.c
> index 0092bb6..6f572fd 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> +/*
> + * Prevent wanring of android gcc, but may still concurrency access.
> + */
> +char *a_ptsname(int fd)
> +{
> +    static char namebuf[PATH_MAX];
> +    int ret;
> +    ret = ptsname_r(fd, namebuf, sizeof(namebuf));
> +    if (ret == 0) {
> +        return namebuf;
> +    }
> +    return NULL;
> +}
> +#endif
> diff --git a/util/qemu-openpty.c b/util/qemu-openpty.c
> index 4c53211..75a1e99 100644
> --- a/util/qemu-openpty.c
> +++ b/util/qemu-openpty.c
> @@ -51,7 +51,12 @@
>  # include <termios.h>
>  #endif
>  
> -#ifdef __sun__
> +#if defined(__sun__) || defined(ANDROID)
> +
> +#ifdef ANDROID
> +#define ptsname(a) a_ptsname(a)
> +#endif
> +

The ioctl(sfd, I_PUSH, "ptem") is not needed on Android.

In addition, you can add a

        if (name)
                strcpy(name, slave);

and inside qemu_openpty_row add Android to the "#if defined(__OpenBSD__)
|| defined(__DragonFly__)" case.  This avoids the need to introduce
a_openpty.

The biggest problem is the workarounds you have added for pkg-config.
Everything else is easy to fix.  I look forward to reviewing the next
version!

Paolo

>  /* Once Solaris has openpty(), this is going to be removed. */
>  static int openpty(int *amaster, int *aslave, char *name,
>                     struct termios *termp, struct winsize *winp)
> @@ -93,7 +98,8 @@ err:
>          close(mfd);
>          return -1;
>  }
> -
> +#endif
> +#ifdef __sun__
>  static void cfmakeraw (struct termios *termios_p)
>  {
>          termios_p->c_iflag &=
> 
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux