[PATCH v9 00/11] iOS and Apple Silicon host support

[PATCH v9 00/11] iOS and Apple Silicon host support

[Joelle van Dyne]

These set of changes brings QEMU TCG to iOS devices and future Apple Silicon
devices. They were originally developed last year and have been working in the
UTM app. Recently, we ported the changes to master, re-wrote a lot of the build
script changes for meson, and broke up the patches into more distinct units.

The bulk of the changes allow for cross-compiling for both iOS and macOS running
Apple Silicon and adds feature detection for parts of QEMU that are not
compatible with iOS.

Since v9:

* system() feature check moved to meson.build

Since v8:

* Moved some feature checks to meson.build
* system() stub return error instead of assertion

Since v7:

* Removed libucontext (will be submitted in another patchset)
* Removed slirp build flags update (superseded by subproject patchset)
* Reworked all patches to use feature detection instead of #ifdef CONFIG_IOS
* Added feature detection for CoreAudio
* Fix various cross compiling issues on macOS

Since v6:

* Dropped the Apple Silicon JIT support patch (superseded by another patchset)
* Changed libucontext to be a Meson subproject
* Cache availablity check for preadv/pwritev on macOS 11 and iOS 14

Since v5:

* Fixed some more instances of QAPI define of CONFIG_HOST_BLOCK_DEVICE
* Fixed libucontext build on newer version of GCC

Since v4:

* Updated QAPI schema for CONFIG_HOST_BLOCK_DEVICE
* Updated maintainers file for iOS host support
* Moved system() changes to osdep.h
* Fixed typo in libucontext meson.build change

Since v3:

* Moved mirror JIT support to a different patch set.
* Removed dependency on `pthread_jit_write_protect_np` because it was redundent
  and also crashes if called on a non-jailbroken iOS device.
* Removed `--enable-cross-compile` option
* Fixed checkpatch errors
* Fixed iOS build on master due to new test recently added which calls system()

Since v2:

* Changed getting mirror pointer from a macro to inline functions
* Split constification of TCG code pointers to separate patch
* Removed slirp updates (will send future patch once slirp changes are in)
* Removed shared library patch (will send future patch)

-j

Joelle van Dyne (11):
  block: feature detection for host block support
  configure: cross-compiling with empty cross_prefix
  configure: check for sys/disk.h
  slirp: feature detection for smbd
  osdep: build with non-working system() function
  darwin: remove redundant dependency declaration
  darwin: fix cross-compiling for Darwin
  configure: cross compile should use x86_64 cpu_family
  block: check availablity for preadv/pwritev on mac
  darwin: detect CoreAudio for build
  darwin: remove 64-bit build detection on 32-bit OS

 configure            | 84 ++++++++++++++++++++++++++++++++++----------
 meson.build          | 10 ++++--
 qapi/block-core.json | 10 ++++--
 include/qemu/osdep.h | 12 +++++++
 block.c              |  2 +-
 block/file-posix.c   | 68 ++++++++++++++++++++++++++++-------
 net/slirp.c          | 16 ++++-----
 7 files changed, 158 insertions(+), 44 deletions(-)

-- 
2.28.0

[PATCH v9 01/11] block: feature detection for host block support

[Joelle van Dyne]

On Darwin (iOS), there are no system level APIs for directly accessing
host block devices. We detect this at configure time.

Signed-off-by: Joelle van Dyne <j@getutm.app>
---
 meson.build          |  6 +++++-
 qapi/block-core.json | 10 +++++++---
 block/file-posix.c   | 33 ++++++++++++++++++++++-----------
 3 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/meson.build b/meson.build
index af2bc89741..27110075df 100644
--- a/meson.build
+++ b/meson.build
@@ -180,7 +180,7 @@ if targetos == 'windows'
                                       include_directories: include_directories('.'))
 elif targetos == 'darwin'
   coref = dependency('appleframeworks', modules: 'CoreFoundation')
-  iokit = dependency('appleframeworks', modules: 'IOKit')
+  iokit = dependency('appleframeworks', modules: 'IOKit', required: false)
 elif targetos == 'sunos'
   socket = [cc.find_library('socket'),
             cc.find_library('nsl'),
@@ -1023,6 +1023,9 @@ if get_option('cfi')
   add_project_link_arguments(cfi_flags, native: false, language: ['c', 'cpp', 'objc'])
 endif
 
+have_host_block_device = (targetos != 'darwin' or
+    cc.has_header('IOKit/storage/IOMedia.h'))
+
 #################
 # config-host.h #
 #################
@@ -1113,6 +1116,7 @@ config_host_data.set('HAVE_DRM_H', cc.has_header('libdrm/drm.h'))
 config_host_data.set('HAVE_PTY_H', cc.has_header('pty.h'))
 config_host_data.set('HAVE_SYS_IOCCOM_H', cc.has_header('sys/ioccom.h'))
 config_host_data.set('HAVE_SYS_KCOV_H', cc.has_header('sys/kcov.h'))
+config_host_data.set('HAVE_HOST_BLOCK_DEVICE', have_host_block_device)
 
 ignored = ['CONFIG_QEMU_INTERP_PREFIX'] # actually per-target
 arrays = ['CONFIG_AUDIO_DRIVERS', 'CONFIG_BDRV_RW_WHITELIST', 'CONFIG_BDRV_RO_WHITELIST']
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 3484986d1c..1a9576de8d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -959,7 +959,8 @@
   'discriminator': 'driver',
   'data': {
       'file': 'BlockStatsSpecificFile',
-      'host_device': 'BlockStatsSpecificFile',
+      'host_device': { 'type': 'BlockStatsSpecificFile',
+                       'if': 'defined(HAVE_HOST_BLOCK_DEVICE)' },
       'nvme': 'BlockStatsSpecificNvme' } }
 
 ##
@@ -2827,7 +2828,9 @@
 { 'enum': 'BlockdevDriver',
   'data': [ 'blkdebug', 'blklogwrites', 'blkreplay', 'blkverify', 'bochs',
             'cloop', 'compress', 'copy-on-read', 'dmg', 'file', 'ftp', 'ftps',
-            'gluster', 'host_cdrom', 'host_device', 'http', 'https', 'iscsi',
+            'gluster', 'host_cdrom',
+            {'name': 'host_device', 'if': 'defined(HAVE_HOST_BLOCK_DEVICE)' },
+            'http', 'https', 'iscsi',
             'luks', 'nbd', 'nfs', 'null-aio', 'null-co', 'nvme', 'parallels',
             'preallocate', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'rbd',
             { 'name': 'replication', 'if': 'defined(CONFIG_REPLICATION)' },
@@ -4012,7 +4015,8 @@
       'ftps':       'BlockdevOptionsCurlFtps',
       'gluster':    'BlockdevOptionsGluster',
       'host_cdrom': 'BlockdevOptionsFile',
-      'host_device':'BlockdevOptionsFile',
+      'host_device': { 'type': 'BlockdevOptionsFile',
+                       'if': 'defined(HAVE_HOST_BLOCK_DEVICE)' },
       'http':       'BlockdevOptionsCurlHttp',
       'https':      'BlockdevOptionsCurlHttps',
       'iscsi':      'BlockdevOptionsIscsi',
diff --git a/block/file-posix.c b/block/file-posix.c
index 00cdaaa2d4..11d2021346 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -42,6 +42,8 @@
 #include "scsi/constants.h"
 
 #if defined(__APPLE__) && (__MACH__)
+#include <sys/ioctl.h>
+#if defined(HAVE_HOST_BLOCK_DEVICE)
 #include <paths.h>
 #include <sys/param.h>
 #include <IOKit/IOKitLib.h>
@@ -52,6 +54,7 @@
 //#include <IOKit/storage/IOCDTypes.h>
 #include <IOKit/storage/IODVDMedia.h>
 #include <CoreFoundation/CoreFoundation.h>
+#endif /* defined(HAVE_HOST_BLOCK_DEVICE) */
 #endif
 
 #ifdef __sun__
@@ -181,7 +184,17 @@ typedef struct BDRVRawReopenState {
     bool check_cache_dropped;
 } BDRVRawReopenState;
 
-static int fd_open(BlockDriverState *bs);
+static int fd_open(BlockDriverState *bs)
+{
+    BDRVRawState *s = bs->opaque;
+
+    /* this is just to ensure s->fd is sane (its called by io ops) */
+    if (s->fd >= 0) {
+        return 0;
+    }
+    return -EIO;
+}
+
 static int64_t raw_getlength(BlockDriverState *bs);
 
 typedef struct RawPosixAIOData {
@@ -3014,6 +3027,7 @@ static BlockStatsSpecific *raw_get_specific_stats(BlockDriverState *bs)
     return stats;
 }
 
+#if defined(HAVE_HOST_BLOCK_DEVICE)
 static BlockStatsSpecific *hdev_get_specific_stats(BlockDriverState *bs)
 {
     BlockStatsSpecific *stats = g_new(BlockStatsSpecific, 1);
@@ -3023,6 +3037,7 @@ static BlockStatsSpecific *hdev_get_specific_stats(BlockDriverState *bs)
 
     return stats;
 }
+#endif /* HAVE_HOST_BLOCK_DEVICE */
 
 static QemuOptsList raw_create_opts = {
     .name = "raw-create-opts",
@@ -3247,6 +3262,8 @@ BlockDriver bdrv_file = {
 /***********************************************/
 /* host device */
 
+#if defined(HAVE_HOST_BLOCK_DEVICE)
+
 #if defined(__APPLE__) && defined(__MACH__)
 static kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
                                 CFIndex maxPathSize, int flags);
@@ -3539,16 +3556,6 @@ hdev_co_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
 }
 #endif /* linux */
 
-static int fd_open(BlockDriverState *bs)
-{
-    BDRVRawState *s = bs->opaque;
-
-    /* this is just to ensure s->fd is sane (its called by io ops) */
-    if (s->fd >= 0)
-        return 0;
-    return -EIO;
-}
-
 static coroutine_fn int
 hdev_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes)
 {
@@ -3872,6 +3879,8 @@ static BlockDriver bdrv_host_cdrom = {
 };
 #endif /* __FreeBSD__ */
 
+#endif /* HAVE_HOST_BLOCK_DEVICE */
+
 static void bdrv_file_init(void)
 {
     /*
@@ -3879,6 +3888,7 @@ static void bdrv_file_init(void)
      * registered last will get probed first.
      */
     bdrv_register(&bdrv_file);
+#if defined(HAVE_HOST_BLOCK_DEVICE)
     bdrv_register(&bdrv_host_device);
 #ifdef __linux__
     bdrv_register(&bdrv_host_cdrom);
@@ -3886,6 +3896,7 @@ static void bdrv_file_init(void)
 #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
     bdrv_register(&bdrv_host_cdrom);
 #endif
+#endif /* HAVE_HOST_BLOCK_DEVICE */
 }
 
 block_init(bdrv_file_init);
-- 
2.28.0

[PATCH v9 02/11] configure: cross-compiling with empty cross_prefix

[Joelle van Dyne]

The iOS toolchain does not use the host prefix naming convention. So we
need to enable cross-compile options while allowing the PREFIX to be
blank.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Joelle van Dyne <j@getutm.app>
---
 configure | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 6f6a319c2f..8d8a4733d7 100755
--- a/configure
+++ b/configure
@@ -238,6 +238,7 @@ cpu=""
 iasl="iasl"
 interp_prefix="/usr/gnemul/qemu-%M"
 static="no"
+cross_compile="no"
 cross_prefix=""
 audio_drv_list=""
 block_drv_rw_whitelist=""
@@ -469,6 +470,7 @@ for opt do
   optarg=$(expr "x$opt" : 'x[^=]*=\(.*\)')
   case "$opt" in
   --cross-prefix=*) cross_prefix="$optarg"
+                    cross_compile="yes"
   ;;
   --cc=*) CC="$optarg"
   ;;
@@ -1696,7 +1698,7 @@ $(echo Deprecated targets: $deprecated_targets_list | \
   --target-list-exclude=LIST exclude a set of targets from the default target-list
 
 Advanced options (experts only):
-  --cross-prefix=PREFIX    use PREFIX for compile tools [$cross_prefix]
+  --cross-prefix=PREFIX    use PREFIX for compile tools, PREFIX can be blank [$cross_prefix]
   --cc=CC                  use C compiler CC [$cc]
   --iasl=IASL              use ACPI compiler IASL [$iasl]
   --host-cc=CC             use C compiler CC [$host_cc] for code run at
@@ -6390,7 +6392,7 @@ if has $sdl2_config; then
 fi
 echo "strip = [$(meson_quote $strip)]" >> $cross
 echo "windres = [$(meson_quote $windres)]" >> $cross
-if test -n "$cross_prefix"; then
+if test "$cross_compile" = "yes"; then
     cross_arg="--cross-file config-meson.cross"
     echo "[host_machine]" >> $cross
     if test "$mingw32" = "yes" ; then
-- 
2.28.0

[PATCH v9 03/11] configure: check for sys/disk.h

[Joelle van Dyne]

Some BSD platforms do not have this header.

Signed-off-by: Joelle van Dyne <j@getutm.app>
---
 meson.build        | 1 +
 block.c            | 2 +-
 block/file-posix.c | 2 +-
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build
index 27110075df..6818d97df5 100644
--- a/meson.build
+++ b/meson.build
@@ -1117,6 +1117,7 @@ config_host_data.set('HAVE_PTY_H', cc.has_header('pty.h'))
 config_host_data.set('HAVE_SYS_IOCCOM_H', cc.has_header('sys/ioccom.h'))
 config_host_data.set('HAVE_SYS_KCOV_H', cc.has_header('sys/kcov.h'))
 config_host_data.set('HAVE_HOST_BLOCK_DEVICE', have_host_block_device)
+config_host_data.set('HAVE_SYS_DISK_H', cc.has_header('sys/disk.h'))
 
 ignored = ['CONFIG_QEMU_INTERP_PREFIX'] # actually per-target
 arrays = ['CONFIG_AUDIO_DRIVERS', 'CONFIG_BDRV_RW_WHITELIST', 'CONFIG_BDRV_RO_WHITELIST']
diff --git a/block.c b/block.c
index 8b9d457546..c4cf391dea 100644
--- a/block.c
+++ b/block.c
@@ -54,7 +54,7 @@
 #ifdef CONFIG_BSD
 #include <sys/ioctl.h>
 #include <sys/queue.h>
-#ifndef __DragonFly__
+#if defined(HAVE_SYS_DISK_H)
 #include <sys/disk.h>
 #endif
 #endif
diff --git a/block/file-posix.c b/block/file-posix.c
index 11d2021346..666d3e7504 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2320,7 +2320,7 @@ again:
         }
         if (size == 0)
 #endif
-#if defined(__APPLE__) && defined(__MACH__)
+#if defined(HAVE_SYS_DISK_H) && defined(__APPLE__) && defined(__MACH__)
         {
             uint64_t sectors = 0;
             uint32_t sector_size = 0;
-- 
2.28.0

Re: [PATCH v9 03/11] configure: check for sys/disk.h

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 1822 bytes --]

On Mon, Jan 25, 2021 at 6:33 PM Joelle van Dyne <j@getutm.app> wrote:

> Some BSD platforms do not have this header.
>
> Signed-off-by: Joelle van Dyne <j@getutm.app>
> ---
>  meson.build        | 1 +
>  block.c            | 2 +-
>  block/file-posix.c | 2 +-
>  3 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index 27110075df..6818d97df5 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1117,6 +1117,7 @@ config_host_data.set('HAVE_PTY_H',
> cc.has_header('pty.h'))
>  config_host_data.set('HAVE_SYS_IOCCOM_H', cc.has_header('sys/ioccom.h'))
>  config_host_data.set('HAVE_SYS_KCOV_H', cc.has_header('sys/kcov.h'))
>  config_host_data.set('HAVE_HOST_BLOCK_DEVICE', have_host_block_device)
> +config_host_data.set('HAVE_SYS_DISK_H', cc.has_header('sys/disk.h'))
>
>  ignored = ['CONFIG_QEMU_INTERP_PREFIX'] # actually per-target
>  arrays = ['CONFIG_AUDIO_DRIVERS', 'CONFIG_BDRV_RW_WHITELIST',
> 'CONFIG_BDRV_RO_WHITELIST']
> diff --git a/block.c b/block.c
> index 8b9d457546..c4cf391dea 100644
> --- a/block.c
> +++ b/block.c
> @@ -54,7 +54,7 @@
>  #ifdef CONFIG_BSD
>  #include <sys/ioctl.h>
>  #include <sys/queue.h>
> -#ifndef __DragonFly__
> +#if defined(HAVE_SYS_DISK_H)
>  #include <sys/disk.h>
>  #endif
>  #endif
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 11d2021346..666d3e7504 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2320,7 +2320,7 @@ again:
>          }
>          if (size == 0)
>  #endif
> -#if defined(__APPLE__) && defined(__MACH__)
> +#if defined(HAVE_SYS_DISK_H) && defined(__APPLE__) && defined(__MACH__)
>

Why is this needed? __DragonFly__ doesn't define either __APPLE__ or
__MACH__

Warner


>          {
>              uint64_t sectors = 0;
>              uint32_t sector_size = 0;
> --
> 2.28.0
>
>
>

[-- Attachment #2: Type: text/html, Size: 2642 bytes --]

Re: [PATCH v9 03/11] configure: check for sys/disk.h

[Joelle van Dyne]

Previously, the only case where sys/disk.h does not exist is on
platforms that define __DragonFly__. However, iOS also does not have
this header. Previously, I had it as

#if defined(__DragonFly__) || defined(CONFIG_IOS)

But there was a code review comment that we should use feature flags
instead of platform defines. So I added the HAS_SYS_DISK_H flag.

-j

On Mon, Jan 25, 2021 at 8:35 PM Warner Losh <imp@bsdimp.com> wrote:
>
>
>
> On Mon, Jan 25, 2021 at 6:33 PM Joelle van Dyne <j@getutm.app> wrote:
>>
>> Some BSD platforms do not have this header.
>>
>> Signed-off-by: Joelle van Dyne <j@getutm.app>
>> ---
>>  meson.build        | 1 +
>>  block.c            | 2 +-
>>  block/file-posix.c | 2 +-
>>  3 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/meson.build b/meson.build
>> index 27110075df..6818d97df5 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -1117,6 +1117,7 @@ config_host_data.set('HAVE_PTY_H', cc.has_header('pty.h'))
>>  config_host_data.set('HAVE_SYS_IOCCOM_H', cc.has_header('sys/ioccom.h'))
>>  config_host_data.set('HAVE_SYS_KCOV_H', cc.has_header('sys/kcov.h'))
>>  config_host_data.set('HAVE_HOST_BLOCK_DEVICE', have_host_block_device)
>> +config_host_data.set('HAVE_SYS_DISK_H', cc.has_header('sys/disk.h'))
>>
>>  ignored = ['CONFIG_QEMU_INTERP_PREFIX'] # actually per-target
>>  arrays = ['CONFIG_AUDIO_DRIVERS', 'CONFIG_BDRV_RW_WHITELIST', 'CONFIG_BDRV_RO_WHITELIST']
>> diff --git a/block.c b/block.c
>> index 8b9d457546..c4cf391dea 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -54,7 +54,7 @@
>>  #ifdef CONFIG_BSD
>>  #include <sys/ioctl.h>
>>  #include <sys/queue.h>
>> -#ifndef __DragonFly__
>> +#if defined(HAVE_SYS_DISK_H)
>>  #include <sys/disk.h>
>>  #endif
>>  #endif
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 11d2021346..666d3e7504 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -2320,7 +2320,7 @@ again:
>>          }
>>          if (size == 0)
>>  #endif
>> -#if defined(__APPLE__) && defined(__MACH__)
>> +#if defined(HAVE_SYS_DISK_H) && defined(__APPLE__) && defined(__MACH__)
>
>
> Why is this needed? __DragonFly__ doesn't define either __APPLE__ or __MACH__
>
> Warner
>
>>
>>          {
>>              uint64_t sectors = 0;
>>              uint32_t sector_size = 0;
>> --
>> 2.28.0
>>
>>

Re: [PATCH v9 03/11] configure: check for sys/disk.h

[Philippe Mathieu-Daudé]

On 1/26/21 6:55 AM, Joelle van Dyne wrote:
> Previously, the only case where sys/disk.h does not exist is on
> platforms that define __DragonFly__. However, iOS also does not have
> this header. Previously, I had it as
> 
> #if defined(__DragonFly__) || defined(CONFIG_IOS)
> 
> But there was a code review comment that we should use feature flags
> instead of platform defines. So I added the HAS_SYS_DISK_H flag.

On technical lists, it's best to avoid top-posting, and to
instead reply inline to make the conversation easier to follow.

> 
> -j
> 
> On Mon, Jan 25, 2021 at 8:35 PM Warner Losh <imp@bsdimp.com> wrote:
>>
>>
>>
>> On Mon, Jan 25, 2021 at 6:33 PM Joelle van Dyne <j@getutm.app> wrote:
>>>
>>> Some BSD platforms do not have this header.
>>>
>>> Signed-off-by: Joelle van Dyne <j@getutm.app>
>>> ---
>>>  meson.build        | 1 +
>>>  block.c            | 2 +-
>>>  block/file-posix.c | 2 +-
>>>  3 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index 27110075df..6818d97df5 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -1117,6 +1117,7 @@ config_host_data.set('HAVE_PTY_H', cc.has_header('pty.h'))
>>>  config_host_data.set('HAVE_SYS_IOCCOM_H', cc.has_header('sys/ioccom.h'))
>>>  config_host_data.set('HAVE_SYS_KCOV_H', cc.has_header('sys/kcov.h'))
>>>  config_host_data.set('HAVE_HOST_BLOCK_DEVICE', have_host_block_device)
>>> +config_host_data.set('HAVE_SYS_DISK_H', cc.has_header('sys/disk.h'))
>>>
>>>  ignored = ['CONFIG_QEMU_INTERP_PREFIX'] # actually per-target
>>>  arrays = ['CONFIG_AUDIO_DRIVERS', 'CONFIG_BDRV_RW_WHITELIST', 'CONFIG_BDRV_RO_WHITELIST']
>>> diff --git a/block.c b/block.c
>>> index 8b9d457546..c4cf391dea 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -54,7 +54,7 @@
>>>  #ifdef CONFIG_BSD
>>>  #include <sys/ioctl.h>
>>>  #include <sys/queue.h>
>>> -#ifndef __DragonFly__
>>> +#if defined(HAVE_SYS_DISK_H)
>>>  #include <sys/disk.h>
>>>  #endif
>>>  #endif
>>> diff --git a/block/file-posix.c b/block/file-posix.c
>>> index 11d2021346..666d3e7504 100644
>>> --- a/block/file-posix.c
>>> +++ b/block/file-posix.c
>>> @@ -2320,7 +2320,7 @@ again:
>>>          }
>>>          if (size == 0)
>>>  #endif
>>> -#if defined(__APPLE__) && defined(__MACH__)
>>> +#if defined(HAVE_SYS_DISK_H) && defined(__APPLE__) && defined(__MACH__)
>>
>>
>> Why is this needed? __DragonFly__ doesn't define either __APPLE__ or __MACH__

Hmm we could also add:

  config_host_data.set('HAVE_DKIOCGETBLOCKCOUNT', cc.compiles(...))

Then this block would be easier to read:

  #if defined(HAVE_DKIOCGETBLOCKCOUNT)
  ...

(Maybe this is what Warner meant?)

>>
>> Warner
>>
>>>
>>>          {
>>>              uint64_t sectors = 0;
>>>              uint32_t sector_size = 0;
>>> --
>>> 2.28.0
>>>
>>>
> 

Re: [PATCH v9 03/11] configure: check for sys/disk.h

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 3529 bytes --]

On Tue, Jan 26, 2021 at 12:08 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> On 1/26/21 6:55 AM, Joelle van Dyne wrote:
> > Previously, the only case where sys/disk.h does not exist is on
> > platforms that define __DragonFly__. However, iOS also does not have
> > this header. Previously, I had it as
> >
> > #if defined(__DragonFly__) || defined(CONFIG_IOS)
> >
> > But there was a code review comment that we should use feature flags
> > instead of platform defines. So I added the HAS_SYS_DISK_H flag.
>
> On technical lists, it's best to avoid top-posting, and to
> instead reply inline to make the conversation easier to follow.
>
> >
> > -j
> >
> > On Mon, Jan 25, 2021 at 8:35 PM Warner Losh <imp@bsdimp.com> wrote:
> >>
> >>
> >>
> >> On Mon, Jan 25, 2021 at 6:33 PM Joelle van Dyne <j@getutm.app> wrote:
> >>>
> >>> Some BSD platforms do not have this header.
> >>>
> >>> Signed-off-by: Joelle van Dyne <j@getutm.app>
> >>> ---
> >>>  meson.build        | 1 +
> >>>  block.c            | 2 +-
> >>>  block/file-posix.c | 2 +-
> >>>  3 files changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/meson.build b/meson.build
> >>> index 27110075df..6818d97df5 100644
> >>> --- a/meson.build
> >>> +++ b/meson.build
> >>> @@ -1117,6 +1117,7 @@ config_host_data.set('HAVE_PTY_H',
> cc.has_header('pty.h'))
> >>>  config_host_data.set('HAVE_SYS_IOCCOM_H',
> cc.has_header('sys/ioccom.h'))
> >>>  config_host_data.set('HAVE_SYS_KCOV_H', cc.has_header('sys/kcov.h'))
> >>>  config_host_data.set('HAVE_HOST_BLOCK_DEVICE', have_host_block_device)
> >>> +config_host_data.set('HAVE_SYS_DISK_H', cc.has_header('sys/disk.h'))
> >>>
> >>>  ignored = ['CONFIG_QEMU_INTERP_PREFIX'] # actually per-target
> >>>  arrays = ['CONFIG_AUDIO_DRIVERS', 'CONFIG_BDRV_RW_WHITELIST',
> 'CONFIG_BDRV_RO_WHITELIST']
> >>> diff --git a/block.c b/block.c
> >>> index 8b9d457546..c4cf391dea 100644
> >>> --- a/block.c
> >>> +++ b/block.c
> >>> @@ -54,7 +54,7 @@
> >>>  #ifdef CONFIG_BSD
> >>>  #include <sys/ioctl.h>
> >>>  #include <sys/queue.h>
> >>> -#ifndef __DragonFly__
> >>> +#if defined(HAVE_SYS_DISK_H)
> >>>  #include <sys/disk.h>
> >>>  #endif
> >>>  #endif
> >>> diff --git a/block/file-posix.c b/block/file-posix.c
> >>> index 11d2021346..666d3e7504 100644
> >>> --- a/block/file-posix.c
> >>> +++ b/block/file-posix.c
> >>> @@ -2320,7 +2320,7 @@ again:
> >>>          }
> >>>          if (size == 0)
> >>>  #endif
> >>> -#if defined(__APPLE__) && defined(__MACH__)
> >>> +#if defined(HAVE_SYS_DISK_H) && defined(__APPLE__) &&
> defined(__MACH__)
> >>
> >>
> >> Why is this needed? __DragonFly__ doesn't define either __APPLE__ or
> __MACH__
>
> Hmm we could also add:
>
>   config_host_data.set('HAVE_DKIOCGETBLOCKCOUNT', cc.compiles(...))
>
> Then this block would be easier to read:
>
>   #if defined(HAVE_DKIOCGETBLOCKCOUNT)
>   ...
>
> (Maybe this is what Warner meant?)
>

Close. I'd test it more directly since DKIOCGETBLOCKCOUNT is already a
#define, and is unlikely to change...

When I saw Joelle's response, I realized I'd been needlessly cryptic in my
comments, so posted what I had in mind for cleanup. I'm not sure if the
norms of qemu code reviews would say my suggestion was too big to be in
scope, or not.

Warner

>>
> >> Warner
> >>
> >>>
> >>>          {
> >>>              uint64_t sectors = 0;
> >>>              uint32_t sector_size = 0;
> >>> --
> >>> 2.28.0
> >>>
> >>>
> >
>
>

[-- Attachment #2: Type: text/html, Size: 5104 bytes --]

Re: [PATCH v9 03/11] configure: check for sys/disk.h

[Philippe Mathieu-Daudé]

On 1/26/21 8:08 AM, Philippe Mathieu-Daudé wrote:
> On 1/26/21 6:55 AM, Joelle van Dyne wrote:
>> Previously, the only case where sys/disk.h does not exist is on
>> platforms that define __DragonFly__. However, iOS also does not have
>> this header. Previously, I had it as
>>
>> #if defined(__DragonFly__) || defined(CONFIG_IOS)
>>
>> But there was a code review comment that we should use feature flags
>> instead of platform defines. So I added the HAS_SYS_DISK_H flag.
> 
> On technical lists, it's best to avoid top-posting, and to
> instead reply inline to make the conversation easier to follow.
> 
>>
>> -j
>>
>> On Mon, Jan 25, 2021 at 8:35 PM Warner Losh <imp@bsdimp.com> wrote:
>>>
>>>
>>>
>>> On Mon, Jan 25, 2021 at 6:33 PM Joelle van Dyne <j@getutm.app> wrote:
>>>>
>>>> Some BSD platforms do not have this header.
>>>>
>>>> Signed-off-by: Joelle van Dyne <j@getutm.app>
>>>> ---
>>>>  meson.build        | 1 +
>>>>  block.c            | 2 +-
>>>>  block/file-posix.c | 2 +-
>>>>  3 files changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/meson.build b/meson.build
>>>> index 27110075df..6818d97df5 100644
>>>> --- a/meson.build
>>>> +++ b/meson.build
>>>> @@ -1117,6 +1117,7 @@ config_host_data.set('HAVE_PTY_H', cc.has_header('pty.h'))
>>>>  config_host_data.set('HAVE_SYS_IOCCOM_H', cc.has_header('sys/ioccom.h'))
>>>>  config_host_data.set('HAVE_SYS_KCOV_H', cc.has_header('sys/kcov.h'))
>>>>  config_host_data.set('HAVE_HOST_BLOCK_DEVICE', have_host_block_device)
>>>> +config_host_data.set('HAVE_SYS_DISK_H', cc.has_header('sys/disk.h'))
>>>>
>>>>  ignored = ['CONFIG_QEMU_INTERP_PREFIX'] # actually per-target
>>>>  arrays = ['CONFIG_AUDIO_DRIVERS', 'CONFIG_BDRV_RW_WHITELIST', 'CONFIG_BDRV_RO_WHITELIST']
>>>> diff --git a/block.c b/block.c
>>>> index 8b9d457546..c4cf391dea 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -54,7 +54,7 @@
>>>>  #ifdef CONFIG_BSD
>>>>  #include <sys/ioctl.h>
>>>>  #include <sys/queue.h>
>>>> -#ifndef __DragonFly__
>>>> +#if defined(HAVE_SYS_DISK_H)
>>>>  #include <sys/disk.h>
>>>>  #endif
>>>>  #endif
>>>> diff --git a/block/file-posix.c b/block/file-posix.c
>>>> index 11d2021346..666d3e7504 100644
>>>> --- a/block/file-posix.c
>>>> +++ b/block/file-posix.c
>>>> @@ -2320,7 +2320,7 @@ again:
>>>>          }
>>>>          if (size == 0)
>>>>  #endif
>>>> -#if defined(__APPLE__) && defined(__MACH__)
>>>> +#if defined(HAVE_SYS_DISK_H) && defined(__APPLE__) && defined(__MACH__)
>>>
>>>
>>> Why is this needed? __DragonFly__ doesn't define either __APPLE__ or __MACH__
> 
> Hmm we could also add:
> 
>   config_host_data.set('HAVE_DKIOCGETBLOCKCOUNT', cc.compiles(...))

If DKIOCGETBLOCKCOUNT were in a know header, we could use Meson's

  cc.has_header_symbol('header.h', 'DKIOCGETBLOCKCOUNT')

But as the previous hunk shows, sys/disk.h isn't on DragonFlyBSD.

If there were only 2 known headers, you could do:

  config_host_data.set('HAVE_DKIOCGETBLOCKCOUNT',
      cc.has_header_symbol('header.h', 'DKIOCGETBLOCKCOUNT') or
      cc.has_header_symbol('sys/disk.h', 'DKIOCGETBLOCKCOUNT'))

> 
> Then this block would be easier to read:
> 
>   #if defined(HAVE_DKIOCGETBLOCKCOUNT)
>   ...
> 
> (Maybe this is what Warner meant?)
> 
>>>
>>> Warner
>>>
>>>>
>>>>          {
>>>>              uint64_t sectors = 0;
>>>>              uint32_t sector_size = 0;
>>>> --
>>>> 2.28.0
>>>>
>>>>
>>
> 

Re: [PATCH v9 03/11] configure: check for sys/disk.h

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 4866 bytes --]

On Mon, Jan 25, 2021 at 10:55 PM Joelle van Dyne <j@getutm.app> wrote:

> Previously, the only case where sys/disk.h does not exist is on
> platforms that define __DragonFly__. However, iOS also does not have
> this header. Previously, I had it as
>
> #if defined(__DragonFly__) || defined(CONFIG_IOS)
>
> But there was a code review comment that we should use feature flags
> instead of platform defines. So I added the HAS_SYS_DISK_H flag.
>

Right. I like that the #include is now protected like that. However,
sys/disk.h never was standardized and varies considerably among the systems
that it exists on.


> -j
>
> On Mon, Jan 25, 2021 at 8:35 PM Warner Losh <imp@bsdimp.com> wrote:
> >
> >
> >
> > On Mon, Jan 25, 2021 at 6:33 PM Joelle van Dyne <j@getutm.app> wrote:
> >>
> >> Some BSD platforms do not have this header.
> >>
> >> Signed-off-by: Joelle van Dyne <j@getutm.app>
> >> ---
> >>  meson.build        | 1 +
> >>  block.c            | 2 +-
> >>  block/file-posix.c | 2 +-
> >>  3 files changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/meson.build b/meson.build
> >> index 27110075df..6818d97df5 100644
> >> --- a/meson.build
> >> +++ b/meson.build
> >> @@ -1117,6 +1117,7 @@ config_host_data.set('HAVE_PTY_H',
> cc.has_header('pty.h'))
> >>  config_host_data.set('HAVE_SYS_IOCCOM_H',
> cc.has_header('sys/ioccom.h'))
> >>  config_host_data.set('HAVE_SYS_KCOV_H', cc.has_header('sys/kcov.h'))
> >>  config_host_data.set('HAVE_HOST_BLOCK_DEVICE', have_host_block_device)
> >> +config_host_data.set('HAVE_SYS_DISK_H', cc.has_header('sys/disk.h'))
> >>
> >>  ignored = ['CONFIG_QEMU_INTERP_PREFIX'] # actually per-target
> >>  arrays = ['CONFIG_AUDIO_DRIVERS', 'CONFIG_BDRV_RW_WHITELIST',
> 'CONFIG_BDRV_RO_WHITELIST']
> >> diff --git a/block.c b/block.c
> >> index 8b9d457546..c4cf391dea 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -54,7 +54,7 @@
> >>  #ifdef CONFIG_BSD
> >>  #include <sys/ioctl.h>
> >>  #include <sys/queue.h>
> >> -#ifndef __DragonFly__
> >> +#if defined(HAVE_SYS_DISK_H)
> >>  #include <sys/disk.h>
> >>  #endif
> >>  #endif
> >> diff --git a/block/file-posix.c b/block/file-posix.c
> >> index 11d2021346..666d3e7504 100644
> >> --- a/block/file-posix.c
> >> +++ b/block/file-posix.c
> >> @@ -2320,7 +2320,7 @@ again:
> >>          }
> >>          if (size == 0)
> >>  #endif
> >> -#if defined(__APPLE__) && defined(__MACH__)
> >> +#if defined(HAVE_SYS_DISK_H) && defined(__APPLE__) && defined(__MACH__)
> >
> >
> > Why is this needed? __DragonFly__ doesn't define either __APPLE__ or
> __MACH_
>

Which is why I asked this question...

Let me ask it another way. Why not base this on the
ioctl  DKIOCGETBLOCKCOUNT like the rest of this function? It's simple and
on platforms that don't have that ioctl, it won't be used.  I don't even
know how to read the proposed change logically. If IOS doesn't have this
interface, then you'll need another #else <something-else> to work reliably
anyway, since the seek trick that's used there may or may not work. However
that starts to get kinda nested and twisty.  So maybe something more like
the following would make it clearer... though that might be beyond the
scope of what you're trying to do.

diff --git a/block/file-posix.c b/block/file-posix.c
index 00cdaaa2d4..704ded68b0 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2295,8 +2295,10 @@ static int64_t raw_getlength(BlockDriverState *bs)
 again:
 #endif
     if (!fstat(fd, &sb) && (S_IFCHR & sb.st_mode)) {
+        size = 0;
 #ifdef DIOCGMEDIASIZE
         if (ioctl(fd, DIOCGMEDIASIZE, (off_t *)&size))
+            size = 0;
 #elif defined(DIOCGPART)
         {
                 struct partinfo pi;
@@ -2305,9 +2307,7 @@ again:
                 else
                         size = 0;
         }
-        if (size == 0)
-#endif
-#if defined(__APPLE__) && defined(__MACH__)
+#elif defined(DKIOCGETBLOCKCOUNT) && defined(DKIOCGETBLOCKSIZE)
         {
             uint64_t sectors = 0;
             uint32_t sector_size = 0;
@@ -2315,19 +2315,15 @@ again:
             if (ioctl(fd, DKIOCGETBLOCKCOUNT, &sectors) == 0
                && ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) == 0) {
                 size = sectors * sector_size;
-            } else {
-                size = lseek(fd, 0LL, SEEK_END);
-                if (size < 0) {
-                    return -errno;
-                }
             }
         }
-#else
-        size = lseek(fd, 0LL, SEEK_END);
+#endif
+        if (size == 0) {
+            size = lseek(fd, 0LL, SEEK_END);
+        }
         if (size < 0) {
             return -errno;
         }
-#endif
 #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
         switch(s->type) {
         case FTYPE_CD:

Warner

>
> >>
> >>          {
> >>              uint64_t sectors = 0;
> >>              uint32_t sector_size = 0;
> >> --
> >> 2.28.0
> >>
> >>
>

[-- Attachment #2: Type: text/html, Size: 6831 bytes --]

[PATCH v9 04/11] slirp: feature detection for smbd

[Joelle van Dyne]

Replace Windows specific macro with a more generic feature detection
macro. Allows slirp smb feature to be disabled manually as well.

Signed-off-by: Joelle van Dyne <j@getutm.app>
---
 configure   | 22 +++++++++++++++++++++-
 meson.build |  2 +-
 net/slirp.c | 16 ++++++++--------
 3 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/configure b/configure
index 8d8a4733d7..d72ab22da5 100755
--- a/configure
+++ b/configure
@@ -464,6 +464,7 @@ fuse="auto"
 fuse_lseek="auto"
 
 malloc_trim="auto"
+slirp_smbd="auto"
 
 # parse CC options second
 for opt do
@@ -845,7 +846,18 @@ do
     fi
 done
 
+# Check for smbd dupport
 : ${smbd=${SMBD-/usr/sbin/smbd}}
+if test "$slirp_smbd" != "no" ; then
+  if test "$mingw32" = "yes" ; then
+    if test "$slirp_smbd" = "yes" ; then
+      error_exit "Host smbd not supported on this platform."
+    fi
+    slirp_smbd=no
+  else
+    slirp_smbd=yes
+  fi
+fi
 
 # Default objcc to clang if available, otherwise use CC
 if has clang; then
@@ -1560,6 +1572,10 @@ for opt do
   ;;
   --disable-fuse-lseek) fuse_lseek="disabled"
   ;;
+  --enable-slirp-smbd) slirp_smbd=yes
+  ;;
+  --disable-slirp-smbd) slirp_smbd=no
+  ;;
   *)
       echo "ERROR: unknown option $opt"
       echo "Try '$0 --help' for more information"
@@ -1899,6 +1915,7 @@ disabled with --disable-FEATURE, default is enabled if available
   libdaxctl       libdaxctl support
   fuse            FUSE block device export
   fuse-lseek      SEEK_HOLE/SEEK_DATA support for FUSE exports
+  slirp-smbd      use smbd (at path --smbd=*) in slirp networking
 
 NOTE: The object files are built at the place where configure is launched
 EOF
@@ -5573,7 +5590,10 @@ fi
 if test "$guest_agent" = "yes" ; then
   echo "CONFIG_GUEST_AGENT=y" >> $config_host_mak
 fi
-echo "CONFIG_SMBD_COMMAND=\"$smbd\"" >> $config_host_mak
+if test "$slirp_smbd" = "yes" ; then
+  echo "CONFIG_SLIRP_SMBD=y" >> $config_host_mak
+  echo "CONFIG_SMBD_COMMAND=\"$smbd\"" >> $config_host_mak
+fi
 if test "$vde" = "yes" ; then
   echo "CONFIG_VDE=y" >> $config_host_mak
   echo "VDE_LIBS=$vde_libs" >> $config_host_mak
diff --git a/meson.build b/meson.build
index 6818d97df5..f1e67b8cd1 100644
--- a/meson.build
+++ b/meson.build
@@ -2336,7 +2336,7 @@ summary_info += {'sphinx-build':      sphinx_build.found()}
 summary_info += {'genisoimage':       config_host['GENISOIMAGE']}
 # TODO: add back version
 summary_info += {'slirp support':     slirp_opt == 'disabled' ? false : slirp_opt}
-if slirp_opt != 'disabled'
+if slirp_opt != 'disabled' and 'CONFIG_SLIRP_SMBD' in config_host
   summary_info += {'smbd':            config_host['CONFIG_SMBD_COMMAND']}
 endif
 summary_info += {'module support':    config_host.has_key('CONFIG_MODULES')}
diff --git a/net/slirp.c b/net/slirp.c
index 8350c6d45f..4348e74805 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -27,7 +27,7 @@
 #include "net/slirp.h"
 
 
-#ifndef _WIN32
+#if defined(CONFIG_SLIRP_SMBD)
 #include <pwd.h>
 #include <sys/wait.h>
 #endif
@@ -90,7 +90,7 @@ typedef struct SlirpState {
     Slirp *slirp;
     Notifier poll_notifier;
     Notifier exit_notifier;
-#ifndef _WIN32
+#if defined(CONFIG_SLIRP_SMBD)
     gchar *smb_dir;
 #endif
     GSList *fwd;
@@ -103,7 +103,7 @@ static QTAILQ_HEAD(, SlirpState) slirp_stacks =
 static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp);
 static int slirp_guestfwd(SlirpState *s, const char *config_str, Error **errp);
 
-#ifndef _WIN32
+#if defined(CONFIG_SLIRP_SMBD)
 static int slirp_smb(SlirpState *s, const char *exported_dir,
                      struct in_addr vserver_addr, Error **errp);
 static void slirp_smb_cleanup(SlirpState *s);
@@ -367,7 +367,7 @@ static int net_slirp_init(NetClientState *peer, const char *model,
     struct in6_addr ip6_prefix;
     struct in6_addr ip6_host;
     struct in6_addr ip6_dns;
-#ifndef _WIN32
+#if defined(CONFIG_SLIRP_SMBD)
     struct in_addr smbsrv = { .s_addr = 0 };
 #endif
     NetClientState *nc;
@@ -477,7 +477,7 @@ static int net_slirp_init(NetClientState *peer, const char *model,
         return -1;
     }
 
-#ifndef _WIN32
+#if defined(CONFIG_SLIRP_SMBD)
     if (vsmbserver && !inet_aton(vsmbserver, &smbsrv)) {
         error_setg(errp, "Failed to parse SMB address");
         return -1;
@@ -592,7 +592,7 @@ static int net_slirp_init(NetClientState *peer, const char *model,
             }
         }
     }
-#ifndef _WIN32
+#if defined(CONFIG_SLIRP_SMBD)
     if (smb_export) {
         if (slirp_smb(s, smb_export, smbsrv, errp) < 0) {
             goto error;
@@ -784,7 +784,7 @@ void hmp_hostfwd_add(Monitor *mon, const QDict *qdict)
 
 }
 
-#ifndef _WIN32
+#if defined(CONFIG_SLIRP_SMBD)
 
 /* automatic user mode samba server configuration */
 static void slirp_smb_cleanup(SlirpState *s)
@@ -899,7 +899,7 @@ static int slirp_smb(SlirpState* s, const char *exported_dir,
     return 0;
 }
 
-#endif /* !defined(_WIN32) */
+#endif /* defined(CONFIG_SLIRP_SMBD) */
 
 static int guestfwd_can_read(void *opaque)
 {
-- 
2.28.0

Re: [PATCH v9 04/11] slirp: feature detection for smbd

[Philippe Mathieu-Daudé]

On 1/26/21 2:24 AM, Joelle van Dyne wrote:
> Replace Windows specific macro with a more generic feature detection
> macro. Allows slirp smb feature to be disabled manually as well.
> 
> Signed-off-by: Joelle van Dyne <j@getutm.app>
> ---
>  configure   | 22 +++++++++++++++++++++-
>  meson.build |  2 +-
>  net/slirp.c | 16 ++++++++--------
>  3 files changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/configure b/configure
> index 8d8a4733d7..d72ab22da5 100755
> --- a/configure
> +++ b/configure
> @@ -464,6 +464,7 @@ fuse="auto"
>  fuse_lseek="auto"
>  
>  malloc_trim="auto"
> +slirp_smbd="auto"
>  
>  # parse CC options second
>  for opt do
> @@ -845,7 +846,18 @@ do
>      fi
>  done
>  
> +# Check for smbd dupport
>  : ${smbd=${SMBD-/usr/sbin/smbd}}
> +if test "$slirp_smbd" != "no" ; then

Here slirp_smbd is always "auto".

> +  if test "$mingw32" = "yes" ; then
> +    if test "$slirp_smbd" = "yes" ; then
> +      error_exit "Host smbd not supported on this platform."
> +    fi
> +    slirp_smbd=no
> +  else
> +    slirp_smbd=yes
> +  fi
> +fi

So this check ...

>  
>  # Default objcc to clang if available, otherwise use CC
>  if has clang; then
> @@ -1560,6 +1572,10 @@ for opt do
>    ;;
>    --disable-fuse-lseek) fuse_lseek="disabled"
>    ;;
> +  --enable-slirp-smbd) slirp_smbd=yes
> +  ;;
> +  --disable-slirp-smbd) slirp_smbd=no
> +  ;;
>    *)

... should be placed after the cmdline options processing,
isn't it?

Re: [PATCH v9 04/11] slirp: feature detection for smbd

[Joelle van Dyne]

On Mon, Jan 25, 2021 at 11:30 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> On 1/26/21 2:24 AM, Joelle van Dyne wrote:
> > Replace Windows specific macro with a more generic feature detection
> > macro. Allows slirp smb feature to be disabled manually as well.
> >
> > Signed-off-by: Joelle van Dyne <j@getutm.app>
> > ---
> >  configure   | 22 +++++++++++++++++++++-
> >  meson.build |  2 +-
> >  net/slirp.c | 16 ++++++++--------
> >  3 files changed, 30 insertions(+), 10 deletions(-)
> >
> > diff --git a/configure b/configure
> > index 8d8a4733d7..d72ab22da5 100755
> > --- a/configure
> > +++ b/configure
> > @@ -464,6 +464,7 @@ fuse="auto"
> >  fuse_lseek="auto"
> >
> >  malloc_trim="auto"
> > +slirp_smbd="auto"
> >
> >  # parse CC options second
> >  for opt do
> > @@ -845,7 +846,18 @@ do
> >      fi
> >  done
> >
> > +# Check for smbd dupport
> >  : ${smbd=${SMBD-/usr/sbin/smbd}}
> > +if test "$slirp_smbd" != "no" ; then
>
> Here slirp_smbd is always "auto".
>
> > +  if test "$mingw32" = "yes" ; then
> > +    if test "$slirp_smbd" = "yes" ; then
> > +      error_exit "Host smbd not supported on this platform."
> > +    fi
> > +    slirp_smbd=no
> > +  else
> > +    slirp_smbd=yes
> > +  fi
> > +fi
>
> So this check ...
>
> >
> >  # Default objcc to clang if available, otherwise use CC
> >  if has clang; then
> > @@ -1560,6 +1572,10 @@ for opt do
> >    ;;
> >    --disable-fuse-lseek) fuse_lseek="disabled"
> >    ;;
> > +  --enable-slirp-smbd) slirp_smbd=yes
> > +  ;;
> > +  --disable-slirp-smbd) slirp_smbd=no
> > +  ;;
> >    *)
>
> ... should be placed after the cmdline options processing,
> isn't it?

That's right, good catch.
>

[PATCH v9 05/11] osdep: build with non-working system() function

[Joelle van Dyne]

Build without error on hosts without a working system(). If system()
is called, return -1 with ENOSYS.

Signed-off-by: Joelle van Dyne <j@getutm.app>
---
 meson.build          |  1 +
 include/qemu/osdep.h | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/meson.build b/meson.build
index f1e67b8cd1..a7650956d3 100644
--- a/meson.build
+++ b/meson.build
@@ -1118,6 +1118,7 @@ config_host_data.set('HAVE_SYS_IOCCOM_H', cc.has_header('sys/ioccom.h'))
 config_host_data.set('HAVE_SYS_KCOV_H', cc.has_header('sys/kcov.h'))
 config_host_data.set('HAVE_HOST_BLOCK_DEVICE', have_host_block_device)
 config_host_data.set('HAVE_SYS_DISK_H', cc.has_header('sys/disk.h'))
+config_host_data.set('HAVE_SYSTEM_FUNCTION', cc.has_function('system', prefix: '#include <stdlib.h>'))
 
 ignored = ['CONFIG_QEMU_INTERP_PREFIX'] # actually per-target
 arrays = ['CONFIG_AUDIO_DRIVERS', 'CONFIG_BDRV_RW_WHITELIST', 'CONFIG_BDRV_RO_WHITELIST']
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index a434382c58..5bd1a67769 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -682,4 +682,16 @@ char *qemu_get_host_name(Error **errp);
  */
 size_t qemu_get_host_physmem(void);
 
+/**
+ * Platforms which do not support system() return ENOSYS
+ */
+#ifndef HAVE_SYSTEM_FUNCTION
+#define system platform_does_not_support_system
+static inline int platform_does_not_support_system(const char *command)
+{
+    errno = ENOSYS;
+    return -1;
+}
+#endif /* !HAVE_SYSTEM_FUNCTION */
+
 #endif
-- 
2.28.0

[PATCH v9 06/11] darwin: remove redundant dependency declaration

[Joelle van Dyne]

Meson will find CoreFoundation, IOKit, and Cocoa as needed.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Joelle van Dyne <j@getutm.app>
---
 configure | 1 -
 1 file changed, 1 deletion(-)

diff --git a/configure b/configure
index d72ab22da5..1b2fc502ea 100755
--- a/configure
+++ b/configure
@@ -781,7 +781,6 @@ Darwin)
   fi
   audio_drv_list="coreaudio try-sdl"
   audio_possible_drivers="coreaudio sdl"
-  QEMU_LDFLAGS="-framework CoreFoundation -framework IOKit $QEMU_LDFLAGS"
   # Disable attempts to use ObjectiveC features in os/object.h since they
   # won't work when we're compiling with gcc as a C compiler.
   QEMU_CFLAGS="-DOS_OBJECT_USE_OBJC=0 $QEMU_CFLAGS"
-- 
2.28.0

[PATCH v9 07/11] darwin: fix cross-compiling for Darwin

[Joelle van Dyne]

Add objc to the Meson cross file as well as detection of Darwin.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Joelle van Dyne <j@getutm.app>
---
 configure | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/configure b/configure
index 1b2fc502ea..96738a19bc 100755
--- a/configure
+++ b/configure
@@ -6402,6 +6402,7 @@ echo "cpp_link_args = [${LDFLAGS:+$(meson_quote $LDFLAGS)}]" >> $cross
 echo "[binaries]" >> $cross
 echo "c = [$(meson_quote $cc)]" >> $cross
 test -n "$cxx" && echo "cpp = [$(meson_quote $cxx)]" >> $cross
+test -n "$objcc" && echo "objc = [$(meson_quote $objcc)]" >> $cross
 echo "ar = [$(meson_quote $ar)]" >> $cross
 echo "nm = [$(meson_quote $nm)]" >> $cross
 echo "pkgconfig = [$(meson_quote $pkg_config_exe)]" >> $cross
@@ -6420,6 +6421,9 @@ if test "$cross_compile" = "yes"; then
     if test "$linux" = "yes" ; then
         echo "system = 'linux'" >> $cross
     fi
+    if test "$darwin" = "yes" ; then
+        echo "system = 'darwin'" >> $cross
+    fi
     case "$ARCH" in
         i386|x86_64)
             echo "cpu_family = 'x86'" >> $cross
-- 
2.28.0

Re: [PATCH v9 07/11] darwin: fix cross-compiling for Darwin

[Philippe Mathieu-Daudé]

On 1/26/21 2:24 AM, Joelle van Dyne wrote:
> Add objc to the Meson cross file as well as detection of Darwin.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Joelle van Dyne <j@getutm.app>
> ---
>  configure | 4 ++++
>  1 file changed, 4 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v9 08/11] configure: cross compile should use x86_64 cpu_family

[Joelle van Dyne]

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Joelle van Dyne <j@getutm.app>
---
 configure | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 96738a19bc..fddd11fed9 100755
--- a/configure
+++ b/configure
@@ -6425,9 +6425,12 @@ if test "$cross_compile" = "yes"; then
         echo "system = 'darwin'" >> $cross
     fi
     case "$ARCH" in
-        i386|x86_64)
+        i386)
             echo "cpu_family = 'x86'" >> $cross
             ;;
+        x86_64)
+            echo "cpu_family = 'x86_64'" >> $cross
+            ;;
         ppc64le)
             echo "cpu_family = 'ppc64'" >> $cross
             ;;
-- 
2.28.0

[PATCH v9 09/11] block: check availablity for preadv/pwritev on mac

[Joelle van Dyne]

macOS 11/iOS 14 added preadv/pwritev APIs. Due to weak linking, configure
will succeed with CONFIG_PREADV even when targeting a lower OS version.
We therefore need to check at run time if we can actually use these APIs.

Signed-off-by: Joelle van Dyne <j@getutm.app>
---
 block/file-posix.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 666d3e7504..6473f84db8 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1386,17 +1386,50 @@ static int handle_aiocb_flush(void *opaque)
 #ifdef CONFIG_PREADV
 
 static bool preadv_present = true;
+static bool preadv_checked;
 
 static ssize_t
 qemu_preadv(int fd, const struct iovec *iov, int nr_iov, off_t offset)
 {
+#ifdef CONFIG_DARWIN /* preadv introduced in macOS 11 */
+    if (unlikely(!preadv_checked)) {
+        if (__builtin_available(macOS 11, iOS 14, watchOS 7, tvOS 14, *)) {
+            preadv_checked = true;
+        } else {
+            preadv_present = false;
+            return -ENOSYS;
+        }
+    }
+    /* Now we suppress the availability warning since we use the cached check */
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wunguarded-availability-new"
+    return preadv(fd, iov, nr_iov, offset);
+#pragma clang diagnostic pop
+#else /* CONFIG_DARWIN */
     return preadv(fd, iov, nr_iov, offset);
+#endif
 }
 
 static ssize_t
 qemu_pwritev(int fd, const struct iovec *iov, int nr_iov, off_t offset)
 {
+#ifdef CONFIG_DARWIN /* preadv introduced in macOS 11 */
+    if (unlikely(!preadv_checked)) {
+        if (__builtin_available(macOS 11, iOS 14, watchOS 7, tvOS 14, *)) {
+            preadv_checked = true;
+        } else {
+            preadv_present = false;
+            return -ENOSYS;
+        }
+    }
+    /* Now we suppress the availability warning since we use the cached check */
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wunguarded-availability-new"
+    return pwritev(fd, iov, nr_iov, offset);
+#pragma clang diagnostic pop
+#else /* CONFIG_DARWIN */
     return pwritev(fd, iov, nr_iov, offset);
+#endif
 }
 
 #else
-- 
2.28.0

Re: [PATCH v9 09/11] block: check availablity for preadv/pwritev on mac

[Peter Maydell]

On Tue, 26 Jan 2021 at 01:38, Joelle van Dyne <j@getutm.app> wrote:
>
> macOS 11/iOS 14 added preadv/pwritev APIs. Due to weak linking, configure
> will succeed with CONFIG_PREADV even when targeting a lower OS version.

I just ran into this this afternoon. It turns out that all our OSX
CI configs pass --enable-werror or equivalent to configure, which
means that when the configure test provokes the warning that
"'preadv' is only available on macOS 11.0 or newer", that is a
compile error due to -Werror, and configure decides preadv is
not available. But if you do a configure for the default setup that
doesn't add -Werror then the test passes and then the binaries
fail at runtime... and this is the first time I'd happened to do
a build with the newer XCode SDK and without -Werror enabled.

So I think that leaves two points for this patch:

(1) we need to fix the configure test so that it either
succeeds without warnings or fails, so that --enable-werror
and --disable-werror configures make the same decision about
preadv support.

(2) we need to decide whether we want to support the OSX idea
that you can compile in support for a function like preadv()
and then find that it's not present at runtime, or if we just
want to make the choice at configure time. I'm on the fence about
this.

I'm going to send out a patch which converts the configure
test to a meson.build one-liner -- this fixes (1) and
(by default) leaves the answer to (2) at "no" (you get preadv()
only if you built on macOS 11 for macOS 11; if you build with
10.x support then you dont' get it).

I'm agnostic about the final answer to (2) -- we do have the
support for the runtime preadv_present flag in this file already,
after all -- so I guess I'll leave that to the block maintainers.
In the meantime we can fix the non-controversial part.

thanks
-- PMM

[PATCH v9 10/11] darwin: detect CoreAudio for build

[Joelle van Dyne]

On iOS there is no CoreAudio, so we should not assume Darwin always
has it.

Signed-off-by: Joelle van Dyne <j@getutm.app>
---
 configure | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index fddd11fed9..89836f8692 100755
--- a/configure
+++ b/configure
@@ -319,6 +319,7 @@ fdt="auto"
 netmap="no"
 sdl="auto"
 sdl_image="auto"
+coreaudio="auto"
 virtiofsd="auto"
 virtfs="auto"
 libudev="auto"
@@ -779,7 +780,7 @@ Darwin)
     QEMU_CFLAGS="-arch x86_64 $QEMU_CFLAGS"
     QEMU_LDFLAGS="-arch x86_64 $QEMU_LDFLAGS"
   fi
-  audio_drv_list="coreaudio try-sdl"
+  audio_drv_list="try-coreaudio try-sdl"
   audio_possible_drivers="coreaudio sdl"
   # Disable attempts to use ObjectiveC features in os/object.h since they
   # won't work when we're compiling with gcc as a C compiler.
@@ -3162,6 +3163,24 @@ EOF
   fi
 fi
 
+##########################################
+# detect CoreAudio
+if test "$coreaudio" != "no" ; then
+  coreaudio_libs="-framework CoreAudio"
+  cat > $TMPC << EOF
+#include <CoreAudio/CoreAudio.h>
+int main(void)
+{
+  return (int)AudioGetCurrentHostTime();
+}
+EOF
+  if compile_prog "" "$coreaudio_libs" ; then
+    coreaudio=yes
+  else
+    coreaudio=no
+  fi
+fi
+
 ##########################################
 # Sound support libraries probe
 
@@ -3218,8 +3237,20 @@ for drv in $audio_drv_list; do
     fi
     ;;
 
-    coreaudio)
+    coreaudio | try-coreaudio)
+    if test "$coreaudio" = "no"; then
+      if test "$drv" = "try-coreaudio"; then
+        audio_drv_list=$(echo "$audio_drv_list" | sed -e 's/try-coreaudio//')
+      else
+        error_exit "$drv check failed" \
+                "Make sure to have the $drv is available."
+      fi
+    else
       coreaudio_libs="-framework CoreAudio"
+      if test "$drv" = "try-coreaudio"; then
+        audio_drv_list=$(echo "$audio_drv_list" | sed -e 's/try-coreaudio/coreaudio/')
+      fi
+    fi
     ;;
 
     dsound)
-- 
2.28.0

Re: [PATCH v9 10/11] darwin: detect CoreAudio for build

[Philippe Mathieu-Daudé]

On 1/26/21 2:24 AM, Joelle van Dyne wrote:
> On iOS there is no CoreAudio, so we should not assume Darwin always
> has it.
> 
> Signed-off-by: Joelle van Dyne <j@getutm.app>
> ---
>  configure | 35 +++++++++++++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v9 11/11] darwin: remove 64-bit build detection on 32-bit OS

[Joelle van Dyne]

A workaround added in early days of 64-bit OSX forced x86_64 if the
host machine had 64-bit support. This creates issues when cross-
compiling for ARM64. Additionally, the user can always use --cpu=* to
manually set the host CPU and therefore this workaround should be
removed.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Joelle van Dyne <j@getutm.app>
---
 configure | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/configure b/configure
index 89836f8692..537ed111d8 100755
--- a/configure
+++ b/configure
@@ -626,13 +626,6 @@ fi
 # the correct CPU with the --cpu option.
 case $targetos in
 Darwin)
-  # on Leopard most of the system is 32-bit, so we have to ask the kernel if we can
-  # run 64-bit userspace code.
-  # If the user didn't specify a CPU explicitly and the kernel says this is
-  # 64 bit hw, then assume x86_64. Otherwise fall through to the usual detection code.
-  if test -z "$cpu" && test "$(sysctl -n hw.optional.x86_64)" = "1"; then
-    cpu="x86_64"
-  fi
   HOST_DSOSUF=".dylib"
   ;;
 SunOS)
@@ -776,10 +769,6 @@ OpenBSD)
 Darwin)
   bsd="yes"
   darwin="yes"
-  if [ "$cpu" = "x86_64" ] ; then
-    QEMU_CFLAGS="-arch x86_64 $QEMU_CFLAGS"
-    QEMU_LDFLAGS="-arch x86_64 $QEMU_LDFLAGS"
-  fi
   audio_drv_list="try-coreaudio try-sdl"
   audio_possible_drivers="coreaudio sdl"
   # Disable attempts to use ObjectiveC features in os/object.h since they
-- 
2.28.0

Re: [PATCH v9 00/11] iOS and Apple Silicon host support

[Peter Maydell]

On Tue, 26 Jan 2021 at 01:29, Joelle van Dyne <j@getutm.app> wrote:
>
> These set of changes brings QEMU TCG to iOS devices and future Apple Silicon
> devices. They were originally developed last year and have been working in the
> UTM app. Recently, we ported the changes to master, re-wrote a lot of the build
> script changes for meson, and broke up the patches into more distinct units.
>
> The bulk of the changes allow for cross-compiling for both iOS and macOS running
> Apple Silicon and adds feature detection for parts of QEMU that are not
> compatible with iOS.

Hi; more than half of this series has been reviewed, so given that
the patches are all more-or-less independent fixes I'm going to
take the reviewed patches into target-arm.next (which I'm planning to
do a pullreq for on Friday), so you don't have to keep carrying
those parts around as you revise the series.

Specifically, I'm taking:

>   configure: cross-compiling with empty cross_prefix
>   osdep: build with non-working system() function
>   darwin: remove redundant dependency declaration
>   darwin: fix cross-compiling for Darwin
>   configure: cross compile should use x86_64 cpu_family
>   darwin: detect CoreAudio for build
>   darwin: remove 64-bit build detection on 32-bit OS

thanks
-- PMM

Re: [PATCH v9 04/11] slirp: feature detection for smbd

[Programmingkid]

I've noticed that my guests like Windows XP and Mac OS 10.4 cannot access the internet using the recent versions of QEMU on Mac OS 11. I'm on an M1 Mac. Does your patchset enable guests to be able to access the internet?