[RFC/PATCH,1/1] USB code fenced for s390

From: Eugene Dvurechenski <jno@linux.vnet.ibm.com>

on s390 there is no usb support and the -usb option doesnt
do anything.
This patch fences all usb code for s390.

Signed-off-by: Eugene Dvurechenski <jno@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 configure        |   66 +++++++++++++++++++++++++++++++++++++++++------------
 hmp-commands.hx  |    6 +++++
 hw/Makefile.objs |    5 ++-
 monitor.c        |    4 +++
 qemu-options.hx  |    4 +++
 sysemu.h         |    2 +
 vl.c             |    8 +++++-
 7 files changed, 76 insertions(+), 19 deletions(-)

On 7 August 2012 13:19, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> +#if defined(TARGET_HAS_USB) && (TARGET_HAS_USB == 1)
>      /* init USB devices */
>      if (usb_enabled) {
>          if (foreach_device_config(DEV_USB, usb_parse) < 0)
>              exit(1);
>      }
> +#endif

Whether there is USB or not is a property of the machine model,
not the target CPU architecture, so a TARGET_HAS_USB define
is definitely the wrong approach.

-- PMM

Am 07.08.2012 14:19, schrieb Christian Borntraeger:
> From: Eugene Dvurechenski <jno@linux.vnet.ibm.com>
> 
> on s390 there is no usb support and the -usb option doesnt
> do anything.
> This patch fences all usb code for s390.
> 
> Signed-off-by: Eugene Dvurechenski <jno@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  configure        |   66 +++++++++++++++++++++++++++++++++++++++++------------
>  hmp-commands.hx  |    6 +++++
>  hw/Makefile.objs |    5 ++-
>  monitor.c        |    4 +++
>  qemu-options.hx  |    4 +++
>  sysemu.h         |    2 +
>  vl.c             |    8 +++++-
>  7 files changed, 76 insertions(+), 19 deletions(-)

This patch seems to be mixing two things:

1) Disabling USB host support. If System z mainframes don't have USB
support, it doesn't make much sense to enable USB host passthrough.

2) Disabling USB support for guest machines. Wasn't there a recent
virtio-usb RFC? In that case it would very well be possible to connect
virtual USB devices (including smartcard) to the virtio s390 machine.

Regards,
Andreas

On 2012年08月07日 20:19, Christian Borntraeger wrote:
> From: Eugene Dvurechenski <jno@linux.vnet.ibm.com>
>
> on s390 there is no usb support and the -usb option doesnt
> do anything.
> This patch fences all usb code for s390.
>
> Signed-off-by: Eugene Dvurechenski <jno@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>   configure        |   66 +++++++++++++++++++++++++++++++++++++++++------------
>   hmp-commands.hx  |    6 +++++
>   hw/Makefile.objs |    5 ++-
>   monitor.c        |    4 +++
>   qemu-options.hx  |    4 +++
>   sysemu.h         |    2 +
>   vl.c             |    8 +++++-
>   7 files changed, 76 insertions(+), 19 deletions(-)
>
> diff --git a/configure b/configure
> index 280726c..405b201 100755
> --- a/configure
> +++ b/configure
> @@ -486,7 +486,10 @@ Haiku)
>     audio_possible_drivers="oss alsa sdl esd pa"
>     linux="yes"
>     linux_user="yes"
> -  usb="linux"
> +  case "$cpu" in
> +    s390*) usb='none';;
> +    *)     usb="linux";;
> +  esac
>     kvm="yes"
>     vhost_net="yes"
>     if [ "$cpu" = "i386" -o "$cpu" = "x86_64" ] ; then
> @@ -2647,6 +2650,11 @@ EOF
>   fi
>
>   # check for libcacard for smartcard support
> +if test "$usb" = 'none'; then
> +    smartcard='no' # USB support required
> +    smartcard_nss='no'
> +    smartcard_cflags=""
> +fi
>   if test "$smartcard" != "no" ; then
>       smartcard="yes"
>       smartcard_cflags=""
> @@ -2686,7 +2694,12 @@ if test "$smartcard" = "no" ; then
>   fi
>
>   # check for usbredirparser for usb network redirection support
> -if test "$usb_redir" != "no" ; then
> +if test "$usb" = 'no'; then
> +        if test "$usb_redir" = "yes"; then
> +            feature_not_found "usb-redir"
> +        fi
> +	usb_redir='no'
> +elif test "$usb_redir" != "no" ; then
>       if $pkg_config --atleast-version=0.3.4 libusbredirparser >/dev/null 2>&1 ; then
>           usb_redir="yes"
>           usb_redir_cflags=$($pkg_config --cflags libusbredirparser 2>/dev/null)
> @@ -3102,6 +3115,7 @@ echo "OpenGL support    $opengl"
>   echo "libiscsi support  $libiscsi"
>   echo "build guest agent $guest_agent"
>   echo "coroutine backend $coroutine_backend"
> +echo "USB support       $usb"
>
>   if test "$sdl_too_old" = "yes"; then
>   echo "-> Your SDL version is too old - please upgrade to have SDL support"
> @@ -3377,18 +3391,20 @@ if test "$spice" = "yes" ; then
>     echo "CONFIG_SPICE=y" >> $config_host_mak
>   fi
>
> -if test "$smartcard" = "yes" ; then
> -  echo "CONFIG_SMARTCARD=y" >> $config_host_mak
> -fi
> +if test "$usb" != 'none'; then
> +  if test "$smartcard" = "yes" ; then
> +    echo "CONFIG_SMARTCARD=y" >> $config_host_mak
> +  fi
>
> -if test "$smartcard_nss" = "yes" ; then
> -  echo "CONFIG_SMARTCARD_NSS=y" >> $config_host_mak
> -  echo "libcacard_libs=$libcacard_libs" >> $config_host_mak
> -  echo "libcacard_cflags=$libcacard_cflags" >> $config_host_mak
> -fi
> +  if test "$smartcard_nss" = "yes" ; then
> +    echo "CONFIG_SMARTCARD_NSS=y" >> $config_host_mak
> +    echo "libcacard_libs=$libcacard_libs" >> $config_host_mak
> +    echo "libcacard_cflags=$libcacard_cflags" >> $config_host_mak
> +  fi
>
> -if test "$usb_redir" = "yes" ; then
> -  echo "CONFIG_USB_REDIR=y" >> $config_host_mak
> +  if test "$usb_redir" = "yes" ; then
> +    echo "CONFIG_USB_REDIR=y" >> $config_host_mak
> +  fi
>   fi
>
>   if test "$opengl" = "yes" ; then
> @@ -3602,6 +3618,7 @@ target_int_alignment=4
>   target_long_alignment=4
>   target_llong_alignment=8
>   target_libs_softmmu=
> +target_has_usb="yes"
>
>   TARGET_ARCH="$target_arch2"
>   TARGET_BASE_ARCH=""
> @@ -3733,6 +3750,7 @@ case "$target_arch2" in
>       target_nptl="yes"
>       target_phys_bits=64
>       target_long_alignment=8
> +    target_has_usb="no"
>     ;;
>     unicore32)
>       target_phys_bits=32
> @@ -3858,9 +3876,19 @@ if test "$target_bsd_user" = "yes" ; then
>     echo "CONFIG_BSD_USER=y" >> $config_target_mak
>   fi
>
> +cflags=''
> +if test "$usb" = 'none' ; then
> +	echo "TARGET_HAS_USB=0"
> +	echo "CONFIG_HAS_USB=n"
> +	cflags="-UTARGET_HAS_USB"
> +else
> +	echo "TARGET_HAS_USB=1"
> +	echo "CONFIG_HAS_USB=y"
> +	cflags="-DTARGET_HAS_USB=1"
> +fi >> $config_target_mak
> +
>   # generate QEMU_CFLAGS/LDFLAGS for targets
>
> -cflags=""
>   includes=""
>   ldflags=""
>
> @@ -3961,6 +3989,9 @@ if test "$tcg_interpreter" = "yes" ; then
>   fi
>
>   case "$ARCH" in
> +s390*)	# it has no USB at all
> +  cflags="$cflags -UTARGET_HAS_USB"
> +;;
>   alpha)
>     # Ensure there's only a single GP
>     cflags="-msmall-data $cflags"
> @@ -4025,10 +4056,15 @@ DIRS="tests tests/tcg tests/tcg/cris tests/tcg/lm32"
>   DIRS="$DIRS pc-bios/optionrom pc-bios/spapr-rtas"
>   DIRS="$DIRS roms/seabios roms/vgabios"
>   DIRS="$DIRS qapi-generated"
> -DIRS="$DIRS libcacard libcacard/libcacard libcacard/trace"
> +if test "$usb" != 'none'; then
> +  DIRS="$DIRS libcacard libcacard/libcacard libcacard/trace"
> +fi
>   FILES="Makefile tests/tcg/Makefile qdict-test-data.txt"
>   FILES="$FILES tests/tcg/cris/Makefile tests/tcg/cris/.gdbinit"
> -FILES="$FILES tests/tcg/lm32/Makefile libcacard/Makefile"
> +FILES="$FILES tests/tcg/lm32/Makefile"
> +if test "$usb" != 'none'; then
> +  FILES="$FILES libcacard/Makefile"
> +fi
>   FILES="$FILES pc-bios/optionrom/Makefile pc-bios/keymaps"
>   FILES="$FILES pc-bios/spapr-rtas/Makefile"
>   FILES="$FILES roms/seabios/Makefile roms/vgabios/Makefile"
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index eea8b32..c989123 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -568,6 +568,7 @@ STEXI
>   Compute the checksum of a memory region.
>   ETEXI
>
> +#if defined(TARGET_HAS_USB) && (TARGET_HAS_USB == 1)
>       {
>           .name       = "usb_add",
>           .args_type  = "devname:s",
> @@ -575,6 +576,7 @@ ETEXI
>           .help       = "add USB device (e.g. 'host:bus.addr' or 'host:vendor_id:product_id')",
>           .mhandler.cmd = do_usb_add,
>       },
> +#endif
>
>   STEXI
>   @item usb_add @var{devname}
> @@ -584,6 +586,7 @@ Add the USB device @var{devname}.  For details of available devices see
>   @ref{usb_devices}
>   ETEXI
>
> +#if defined(TARGET_HAS_USB) && (TARGET_HAS_USB == 1)
>       {
>           .name       = "usb_del",
>           .args_type  = "devname:s",
> @@ -591,6 +594,7 @@ ETEXI
>           .help       = "remove USB device 'bus.addr'",
>           .mhandler.cmd = do_usb_del,
>       },
> +#endif
>
>   STEXI
>   @item usb_del @var{devname}
> @@ -601,6 +605,7 @@ hub. @var{devname} has the syntax @code{bus.addr}. Use the monitor
>   command @code{info usb} to see the devices you can remove.
>   ETEXI
>
> +#if defined(TARGET_HAS_USB) && (TARGET_HAS_USB == 1)
>       {
>           .name       = "device_add",
>           .args_type  = "device:O",
> @@ -609,6 +614,7 @@ ETEXI
>           .user_print = monitor_user_noop,
>           .mhandler.cmd_new = do_device_add,
>       },
> +#endif
>
>   STEXI
>   @item device_add @var{config}
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 8327e55..e6811f2 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -1,4 +1,5 @@
> -hw-obj-y = usb/ ide/
> +hw-obj-y = ide/
> +hw-obj-$(CONFIG_HAS_USB) += usb/
>   hw-obj-y += loader.o
>   hw-obj-$(CONFIG_VIRTIO) += virtio-console.o
>   hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
> @@ -121,7 +122,7 @@ hw-obj-$(CONFIG_SOUND) += $(sound-obj-y)
>
>   hw-obj-$(CONFIG_REALLY_VIRTFS) += 9pfs/
>
> -common-obj-y += usb/
> +common-obj-$(CONFIG_HAS_USB) += usb/
>   common-obj-y += irq.o
>   common-obj-$(CONFIG_PTIMER) += ptimer.o
>   common-obj-$(CONFIG_MAX7310) += max7310.o
> diff --git a/monitor.c b/monitor.c
> index 49dccfe..554cc48 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -24,7 +24,9 @@
>   #include <dirent.h>
>   #include "hw/hw.h"
>   #include "hw/qdev.h"
> +#if defined(TARGET_HAS_USB) && (TARGET_HAS_USB == 1)
>   #include "hw/usb.h"
> +#endif
>   #include "hw/pcmcia.h"
>   #include "hw/pc.h"
>   #include "hw/pci.h"
> @@ -2543,6 +2545,7 @@ static mon_cmd_t info_cmds[] = {
>           .help       = "show NUMA information",
>           .mhandler.info = do_info_numa,
>       },
> +#if defined(TARGET_HAS_USB) && (TARGET_HAS_USB == 1)
>       {
>           .name       = "usb",
>           .args_type  = "",
> @@ -2557,6 +2560,7 @@ static mon_cmd_t info_cmds[] = {
>           .help       = "show host USB devices",
>           .mhandler.info = usb_host_info,
>       },
> +#endif
>       {
>           .name       = "profile",
>           .args_type  = "",
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 5e7d0dc..9b2b4a3 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -491,9 +491,11 @@ STEXI
>   @end table
>   ETEXI
>
> +#if defined(TARGET_HAS_USB) && (TARGET_HAS_USB == 1)
>   DEF("usb", 0, QEMU_OPTION_usb,
>       "-usb            enable the USB driver (will be the default soon)\n",
>       QEMU_ARCH_ALL)
> +#endif
>   STEXI
>   USB options:
>   @table @option
> @@ -503,9 +505,11 @@ USB options:
>   Enable the USB driver (will be the default soon)
>   ETEXI
>
> +#if defined(TARGET_HAS_USB) && (TARGET_HAS_USB == 1)
>   DEF("usbdevice", HAS_ARG, QEMU_OPTION_usbdevice,
>       "-usbdevice name add the host or guest USB device 'name'\n",
>       QEMU_ARCH_ALL)
> +#endif
>   STEXI
>
>   @item -usbdevice @var{devname}
> diff --git a/sysemu.h b/sysemu.h
> index 4669348..c1d73c7 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -177,9 +177,11 @@ extern CharDriverState *serial_hds[MAX_SERIAL_PORTS];
>
>   extern CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
>
> +#if defined(TARGET_HAS_USB) && (TARGET_HAS_USB == 1)
>   void do_usb_add(Monitor *mon, const QDict *qdict);
>   void do_usb_del(Monitor *mon, const QDict *qdict);
>   void usb_info(Monitor *mon);
> +#endif
>
>   void rtc_change_mon_event(struct tm *tm);
>
> diff --git a/vl.c b/vl.c
> index e71cb30..389ce95 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1042,7 +1042,7 @@ static void smp_parse(const char *optarg)
>
>   /***********************************************************/
>   /* USB devices */
> -
> +#if defined(TARGET_HAS_USB) && (TARGET_HAS_USB == 1)
>   static int usb_device_add(const char *devname)
>   {
>       const char *p;
> @@ -1122,7 +1122,7 @@ void do_usb_del(Monitor *mon, const QDict *qdict)
>           error_report("could not delete USB device '%s'", devname);
>       }
>   }
> -
> +#endif
>   /***********************************************************/
>   /* PCMCIA/Cardbus */
>
> @@ -2976,6 +2976,7 @@ int main(int argc, char **argv, char **envp)
>                       machine = machine_parse(optarg);
>                   }
>                   break;
> +#if defined(TARGET_HAS_USB) && (TARGET_HAS_USB == 1)
>               case QEMU_OPTION_usb:
>                   usb_enabled = 1;
This line will be replaced by the following in my patch:
machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
if (machine_opts) {
qemu_opt_set_bool(machine_opts, "usb", true);
}

BTW, does s390 use the same vl.c as other platforms?
It seems that the code is not as the code I have seen in qemu. :)
>                   break;
> @@ -2983,6 +2984,7 @@ int main(int argc, char **argv, char **envp)
>                   usb_enabled = 1;
This is the same as I mention above.
>                   add_device_config(DEV_USB, optarg);
>                   break;
> +#endif
>               case QEMU_OPTION_device:
>                   if (!qemu_opts_parse(qemu_find_opts("device"), optarg, 1)) {
>                       exit(1);
> @@ -3525,11 +3527,13 @@ int main(int argc, char **argv, char **envp)
>
>       current_machine = machine;
>
> +#if defined(TARGET_HAS_USB) && (TARGET_HAS_USB == 1)
>       /* init USB devices */
>       if (usb_enabled) {
usb_enabled is removed and it is replaced with usb_enabled() function.
You can see my patch.
>           if (foreach_device_config(DEV_USB, usb_parse) < 0)
>               exit(1);
>       }
> +#endif
>
>       /* init generic devices */
>       if (qemu_opts_foreach(qemu_find_opts("device"), device_init_func, NULL, 1) != 0)

On Tue, Aug 7, 2012 at 12:26 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 7 August 2012 13:19, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>> +#if defined(TARGET_HAS_USB) && (TARGET_HAS_USB == 1)
>>      /* init USB devices */
>>      if (usb_enabled) {
>>          if (foreach_device_config(DEV_USB, usb_parse) < 0)
>>              exit(1);
>>      }
>> +#endif
>
> Whether there is USB or not is a property of the machine model,
> not the target CPU architecture, so a TARGET_HAS_USB define
> is definitely the wrong approach.

Yes. I'd add CONFIG_USB=y to files in default-configs/ except for
s390*, Sparc32 and user emulators. Unless it's enough for USB to have
a dependency on PCI?

>
> -- PMM
>

On 07/08/12 22:22, Blue Swirl wrote:
> On Tue, Aug 7, 2012 at 12:26 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 7 August 2012 13:19, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>> +#if defined(TARGET_HAS_USB) && (TARGET_HAS_USB == 1)
>>>      /* init USB devices */
>>>      if (usb_enabled) {
>>>          if (foreach_device_config(DEV_USB, usb_parse) < 0)
>>>              exit(1);
>>>      }
>>> +#endif
>>
>> Whether there is USB or not is a property of the machine model,
>> not the target CPU architecture, so a TARGET_HAS_USB define
>> is definitely the wrong approach.
> 
> Yes. I'd add CONFIG_USB=y to files in default-configs/ except for
> s390*, Sparc32 and user emulators. Unless it's enough for USB to have
> a dependency on PCI?

Yes, that seems to be the right place - thanks.  A dependency on pci would also
do for us. 

On the other hand, if there are really plans to add a virtio-usb then we should
not disable usb support at all - especially since disabling it does cause some
trouble with libvirt and the current state does not cause problems (we assumed that
libvirt will detect usb capability like it does for other parameters, but it doesnt).
In other words this patch was mostly cosmetics. So lets just drop this patch, go ahead
with Li's patch set and later on we can see if we should do anything regarding s390.

Christian