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

[Houcheng Lin]

Hi Paolo,

(Please ignore the previous mail that did not include "qemu-devel")

Thanks for your review and suggestions. I'll fix this patch
accordingly and please see my replies below.

best regards,
Houcheng Lin

2015-09-15 17:41 GMT+08:00 Paolo Bonzini <address@hidden>:

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

Will fix it.
>
>
>>     - 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.

I'll modify the bionic C library to support these functions and feedback
to google's AOSP project. But the android kernel does not support shmem,
so I will prevent compile the ivshmem.c when in android config. The config
for ivshmem in pci.mak will be:

CONFIG_IVSHMEM=$(call land, $(call lnot,$(CONFIG_ANDROID)),$(CONFIG_KVM))

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

It's better to put headers on kernel user headers. Thanks.

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

Okay.

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

Okay.

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

I'll use wrapper in next release and specify with environment variable.
Later, I may generate pkg-config data file while building library and install
it into 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.

Okay, I will fix it.

>> +#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.

Okay.

>> +#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
>
> ?

Yes, we can use this approach. In android, PAGE_SIZE is defined
to 4096 and this code can detect definition mismatch.

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

I'll write a document to describe steps to build QEMU for Android.
But some library need minor modification to build for Android.
Maybe I shall also put my modified library onto github.

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

Okay, I'll use the strcpy approach to avoid 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!

Many thanks for your comments and I'll do the next version.

>
> Paolo
>


--
Best regards,
Houcheng Lin