Re: [Qemu-devel] [PATCH 16/16] hub: add the support for hub own flow control

[Paolo Bonzini]

Il 24/07/2012 00:27, Laszlo Ersek ha scritto:
> This seems to consolidate net_hub_receive_iov()'s retval with that of
> net_hub_receive(), but may I ask why it used to be calculated differently?

Because until this patch was introduced, net/hub.c had a TODO about the
return values of net_hub_receive_*().  Probably this patch could be
squashed earlier in the series, but it's not paramount.

Paolo

Re: [Qemu-devel] [PATCH 21/22] Add XBZRLE statistics

[Orit Wasserman]

On 07/23/2012 10:33 PM, Luiz Capitulino wrote:
> On Fri, 13 Jul 2012 09:23:43 +0200
> Juan Quintela  wrote:
> 
>> From: Orit Wasserman 
>>
>> Signed-off-by: Benoit Hudzia 
>> Signed-off-by: Petter Svard 
>> Signed-off-by: Aidan Shribman 
>> Signed-off-by: Orit Wasserman 
>> Signed-off-by: Juan Quintela 
>> ---
>>  arch_init.c  |   66 
>> ++
>>  hmp.c|   13 +++
>>  migration.c  |   49 
>>  migration.h  |9 
>>  qapi-schema.json |   37 +-
>>  qmp-commands.hx  |   35 -
>>  6 files changed, 203 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index d972d84..ab3fb2c 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -202,6 +202,64 @@ int64_t xbzrle_cache_resize(int64_t new_size)
>>  return pow2floor(new_size);
>>  }
>>
>> +/* accounting for migration statistics */
>> +typedef struct AccountingInfo {
>> +uint64_t dup_pages;
>> +uint64_t norm_pages;
>> +uint64_t xbzrle_bytes;
>> +uint64_t xbzrle_pages;
>> +uint64_t xbzrle_cache_miss;
>> +uint64_t iterations;
>> +uint64_t xbzrle_overflows;
>> +} AccountingInfo;
>> +
>> +static AccountingInfo acct_info;
>> +
>> +static void acct_clear(void)
>> +{
>> +memset(&acct_info, 0, sizeof(acct_info));
>> +}
>> +
>> +uint64_t dup_mig_bytes_transferred(void)
>> +{
>> +return acct_info.dup_pages * TARGET_PAGE_SIZE;
>> +}
>> +
>> +uint64_t dup_mig_pages_transferred(void)
>> +{
>> +return acct_info.dup_pages;
>> +}
>> +
>> +uint64_t norm_mig_bytes_transferred(void)
>> +{
>> +return acct_info.norm_pages * TARGET_PAGE_SIZE;
>> +}
>> +
>> +uint64_t norm_mig_pages_transferred(void)
>> +{
>> +return acct_info.norm_pages;
>> +}
>> +
>> +uint64_t xbzrle_mig_bytes_transferred(void)
>> +{
>> +return acct_info.xbzrle_bytes;
>> +}
>> +
>> +uint64_t xbzrle_mig_pages_transferred(void)
>> +{
>> +return acct_info.xbzrle_pages;
>> +}
>> +
>> +uint64_t xbzrle_mig_pages_cache_miss(void)
>> +{
>> +return acct_info.xbzrle_cache_miss;
>> +}
>> +
>> +uint64_t xbzrle_mig_pages_overflow(void)
>> +{
>> +return acct_info.xbzrle_overflows;
>> +}
>> +
>>  static void save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
>>  int cont, int flag)
>>  {
>> @@ -230,6 +288,7 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t 
>> *current_data,
>>  if (!cache_is_cached(XBZRLE.cache, current_addr)) {
>>  cache_insert(XBZRLE.cache, current_addr,
>>   g_memdup(current_data, TARGET_PAGE_SIZE));
>> +acct_info.xbzrle_cache_miss++;
>>  return -1;
>>  }
>>
>> @@ -244,6 +303,7 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t 
>> *current_data,
>>  return 0;
>>  } else if (encoded_len == -1) {
>>  DPRINTF("Overflow\n");
>> +acct_info.xbzrle_overflows++;
>>  /* update data in the cache */
>>  memcpy(prev_cached_page, current_data, TARGET_PAGE_SIZE);
>>  return -1;
>> @@ -263,7 +323,9 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t 
>> *current_data,
>>  qemu_put_byte(f, hdr.xh_flags);
>>  qemu_put_be16(f, hdr.xh_len);
>>  qemu_put_buffer(f, XBZRLE.encoded_buf, encoded_len);
>> +acct_info.xbzrle_pages++;
>>  bytes_sent = encoded_len + sizeof(hdr);
>> +acct_info.xbzrle_bytes += bytes_sent;
>>
>>  return bytes_sent;
>>  }
>> @@ -303,6 +365,7 @@ static int ram_save_block(QEMUFile *f)
>>  p = memory_region_get_ram_ptr(mr) + offset;
>>
>>  if (is_dup_page(p)) {
>> +acct_info.dup_pages++;
>>  save_block_hdr(f, block, offset, cont, 
>> RAM_SAVE_FLAG_COMPRESS);
>>  qemu_put_byte(f, *p);
>>  bytes_sent = 1;
>> @@ -318,6 +381,7 @@ static int ram_save_block(QEMUFile *f)
>>  save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_PAGE);
>>  qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
>>  bytes_sent = TARGET_PAGE_SIZE;
>> +acct_info.norm_pages++;
>>  }
>>
>>  /* if page is unmodified, continue to the next */
>> @@ -437,6 +501,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>>  return -1;
>>  }
>>  XBZRLE.encoded_buf = g_malloc0(TARGET_PAGE_SIZE);
>> +acct_clear();
>>  }
>>
>>  QLIST_FOREACH(block, &ram_list.blocks, next) {
>> @@ -484,6 +549,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>>  break;
>>  }
>>  bytes_transferred += bytes_sent;
>> +acct_info.iterations++;
>>  /* we want to check in the 1st loop, just in case it was the 1st 
>> time
>> and we had to sync the dirty bitmap.
>> qemu_get_clock_ns() is a bit expensive, so we only check each 
>> some
>> diff --git a/hmp.c b/hmp.c
>> index 99ad

Re: [Qemu-devel] [PATCH 13/22] Add migration capabilities

[Orit Wasserman]

On 07/23/2012 09:23 PM, Luiz Capitulino wrote:
> On Fri, 13 Jul 2012 09:23:35 +0200
> Juan Quintela  wrote:
> 
>> From: Orit Wasserman 
>>
>> Add migration capabilities that can be queried by the management.
>> The management can query the source QEMU and the destination QEMU in order to
>> verify both support some migration capability (currently only XBZRLE).
>> The management can enable a capability for the next migration by using
>> migrate_set_parameter command.
> 
> Please, split this into one command per-patch. Otherwise it's difficult to
> review.
> 
Sure.
> Have libvirt folks acked this approach btw? It looks fine to me, but we need
> their ack too.
> 
> More comments below.
> 
>>
>> Signed-off-by: Orit Wasserman 
>> Signed-off-by: Juan Quintela 
>> ---
>>  hmp-commands.hx  |   16 
>>  hmp.c|   64 
>>  hmp.h|2 ++
>>  migration.c  |   72 
>> --
>>  migration.h  |2 ++
>>  monitor.c|7 ++
>>  qapi-schema.json |   53 ++--
>>  qmp-commands.hx  |   71 
>> ++---
>>  8 files changed, 280 insertions(+), 7 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index f5d9d91..9245bef 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -861,6 +861,20 @@ Set maximum tolerated downtime (in seconds) for 
>> migration.
>>  ETEXI
>>
>>  {
>> +.name   = "migrate_set_parameter",
>> +.args_type  = "capability:s,state:b",
>> +.params = "",
> 
> Please, fill in params.
ok
> 
>> +.help   = "Enable/Disable the usage of a capability for 
>> migration",
>> +.mhandler.cmd = hmp_migrate_set_parameter,
>> +},
>> +
>> +STEXI
>> +@item migrate_set_parameter @var{capability} @var{state}
>> +@findex migrate_set_parameter
>> +Enable/Disable the usage of a capability @var{capability} for migration.
>> +ETEXI
>> +
>> +{
>>  .name   = "client_migrate_info",
>>  .args_type  = 
>> "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?",
>>  .params = "protocol hostname port tls-port cert-subject",
>> @@ -1419,6 +1433,8 @@ show CPU statistics
>>  show user network stack connection states
>>  @item info migrate
>>  show migration status
>> +@item info migration_capabilities
>> +show migration capabilities
>>  @item info balloon
>>  show balloon information
>>  @item info qtree
>> diff --git a/hmp.c b/hmp.c
>> index 4c6d4ae..b0440e6 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -131,9 +131,19 @@ void hmp_info_mice(Monitor *mon)
>>  void hmp_info_migrate(Monitor *mon)
>>  {
>>  MigrationInfo *info;
>> +MigrationCapabilityInfoList *cap;
>>
>>  info = qmp_query_migrate(NULL);
>>
>> +if (info->has_capabilities && info->capabilities) {
>> +monitor_printf(mon, "capabilities: ");
>> +for (cap = info->capabilities; cap; cap = cap->next) {
>> +monitor_printf(mon, "%s: %s ",
>> +   
>> MigrationCapability_lookup[cap->value->capability],
>> +   cap->value->state ? "on" : "off");
>> +}
>> +monitor_printf(mon, "\n");
> 
> Why is this is needed? Isn't info migration-capabilities good enough?
> Besides, info migrate should only contain information about current migration
> process...
The reason we introduced capabilities is that xbzrle needs for both source and 
destination QEMU
to be able to handle it. Even if both side support xbzrle the user may decide 
not to use it.
User that wants to use xbzrle needs to check that both sides have support for 
it (using info capabilities) , than 
enable it in both sides (using migrate-set-parameter/s commands). This is a 
parameter for the current migration.
So the user needs to know if xbzrle was enabled or disabled for the current 
migration, this code displays it.

some day when there will be better migration protocol, feature negotiations can 
be part of it ...
> 
>> +}
>>  if (info->has_status) {
>>  monitor_printf(mon, "Migration status: %s\n", info->status);
>>  }
>> @@ -161,6 +171,25 @@ void hmp_info_migrate(Monitor *mon)
>>  qapi_free_MigrationInfo(info);
>>  }
>>
>> +void hmp_info_migration_capabilities(Monitor *mon)
>> +{
>> +MigrationCapabilityInfoList *caps_list, *cap;
>> +
>> +caps_list = qmp_query_migration_capabilities(NULL);
>> +if (!caps_list) {
>> +monitor_printf(mon, "No migration capabilities found\n");
>> +return;
>> +}
>> +
>> +for (cap = caps_list; cap; cap = cap->next) {
>> +monitor_printf(mon, "%s: %s ",
>> +   MigrationCapability_lookup[cap->value->capability],
>> +   cap->value->state ? "on" : "off");
>> +}
>> +
>> +qapi_free_MigrationCapabilityInfoList(caps_list);
>> +}
>> +
>>  void hmp_info_cpus(Moni

[Qemu-devel] [Bug 1028260] Re: ARM: stellaris lm3s6965evb machine model broken

[Peter Crosthwaite]

** Attachment added: "Test vector to replicate bug"
   
https://bugs.launchpad.net/qemu/+bug/1028260/+attachment/3233691/+files/stellaris-test.tar

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1028260

Title:
  ARM: stellaris lm3s6965evb machine model broken

Status in QEMU:
  New

Bug description:
  The Stellaris lm3s6965evb Machine model is broken:

  qemu-system-arm -M lm3s6965evb -kernel qs_ek-lm3s6965.bin
  GNU gdb (GDB) 7.1-ubuntu
  Copyright (C) 2010 Free Software Foundation, Inc.
  License GPLv3+: GNU GPL version 3 or later 
  This is free software: you are free to change and redistribute it.
  There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
  and "show warranty" for details.
  This GDB was configured as "x86_64-linux-gnu".
  For bug reporting instructions, please see:
  ...
  Reading symbols from 
/home/peterc/Petalogix/Internal/plgx_install/qemu-upstream-regression/petalinux-vx.x/tools/linux-i386/petalogix/bin/qemu-system-arm...done.
  (gdb) r
  Starting program: 
/home/peterc/Petalogix/Internal/plgx_install/qemu-upstream-regression/petalinux-vx.x/tools/linux-i386/petalogix/bin/qemu-system-arm
 -M lm3s6965evb -kernel qs_ek-lm3s6965.bin
  [Thread debugging using libthread_db enabled]
  warning: no loadable sections found in added symbol-file 
  [New Thread 0x763fc700 (LWP 15156)]
  qemu-system-arm: /home/peterc/Petalogix/Internal/plgx_src/qemu/hw/qdev.c:309: 
qdev_get_gpio_in: Assertion `n >= 0 && n < dev->num_gpio_in' failed.

  Program received signal SIGABRT, Aborted.
  0x006f4a2b in raise (sig=) at 
../nptl/sysdeps/unix/sysv/linux/pt-raise.c:42
  42../nptl/sysdeps/unix/sysv/linux/pt-raise.c: No such file or directory.
in ../nptl/sysdeps/unix/sysv/linux/pt-raise.c
  (gdb) bt
  #0  0x006f4a2b in raise (sig=)
  at ../nptl/sysdeps/unix/sysv/linux/pt-raise.c:42
  #1  0x007089a0 in abort ()
  #2  0x00702c15 in __assert_fail ()
  #3  0x00463ffa in qdev_get_gpio_in ()
  #4  0x00588f94 in armv7m_init ()
  #5  0x0060c62a in stellaris_init ()
  #6  0x0060cc4e in lm3s6965evb_init ()
  #7  0x004fdb11 in main ()
  (gdb) 

  Bisection points at this commit:

  commit 1e8cae4dfea2bcc91d3820dcf4f9284e7b0abb28
  Author: Peter Maydell 
  Date:   Wed May 2 16:49:42 2012 +

  hw/armv7m_nvic: Make the NVIC a freestanding class
  
  Rearrange the GIC and NVIC so both are straightforward
  subclasses of a common class, rather than having the NVIC
  source file textually include arm_gic.c.
  
  Signed-off-by: Peter Maydell 

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1028260/+subscriptions

Re: [Qemu-devel] [Bug 1028260] [NEW] ARM: stellaris lm3s6965evb machine model broken

[Peter Crosthwaite]

cc relevant maintainers (PMM and PB).

On Tue, Jul 24, 2012 at 3:58 PM, Peter Crosthwaite
 wrote:
> Public bug reported:
>
> The Stellaris lm3s6965evb Machine model is broken:
>
> qemu-system-arm -M lm3s6965evb -kernel qs_ek-lm3s6965.bin
> GNU gdb (GDB) 7.1-ubuntu
> Copyright (C) 2010 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later 
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
> and "show warranty" for details.
> This GDB was configured as "x86_64-linux-gnu".
> For bug reporting instructions, please see:
> ...
> Reading symbols from 
> /home/peterc/Petalogix/Internal/plgx_install/qemu-upstream-regression/petalinux-vx.x/tools/linux-i386/petalogix/bin/qemu-system-arm...done.
> (gdb) r
> Starting program: 
> /home/peterc/Petalogix/Internal/plgx_install/qemu-upstream-regression/petalinux-vx.x/tools/linux-i386/petalogix/bin/qemu-system-arm
>  -M lm3s6965evb -kernel qs_ek-lm3s6965.bin
> [Thread debugging using libthread_db enabled]
> warning: no loadable sections found in added symbol-file 
> [New Thread 0x763fc700 (LWP 15156)]
> qemu-system-arm: /home/peterc/Petalogix/Internal/plgx_src/qemu/hw/qdev.c:309: 
> qdev_get_gpio_in: Assertion `n >= 0 && n < dev->num_gpio_in' failed.
>
> Program received signal SIGABRT, Aborted.
> 0x006f4a2b in raise (sig=) at 
> ../nptl/sysdeps/unix/sysv/linux/pt-raise.c:42
> 42  ../nptl/sysdeps/unix/sysv/linux/pt-raise.c: No such file or directory.
> in ../nptl/sysdeps/unix/sysv/linux/pt-raise.c
> (gdb) bt
> #0  0x006f4a2b in raise (sig=)
> at ../nptl/sysdeps/unix/sysv/linux/pt-raise.c:42
> #1  0x007089a0 in abort ()
> #2  0x00702c15 in __assert_fail ()
> #3  0x00463ffa in qdev_get_gpio_in ()
> #4  0x00588f94 in armv7m_init ()
> #5  0x0060c62a in stellaris_init ()
> #6  0x0060cc4e in lm3s6965evb_init ()
> #7  0x004fdb11 in main ()
> (gdb)
>
> Bisection points at this commit:
>
> commit 1e8cae4dfea2bcc91d3820dcf4f9284e7b0abb28
> Author: Peter Maydell 
> Date:   Wed May 2 16:49:42 2012 +
>
> hw/armv7m_nvic: Make the NVIC a freestanding class
>
> Rearrange the GIC and NVIC so both are straightforward
> subclasses of a common class, rather than having the NVIC
> source file textually include arm_gic.c.
>
> Signed-off-by: Peter Maydell 
>
> ** Affects: qemu
>  Importance: Undecided
>  Status: New
>
> --
> You received this bug notification because you are a member of qemu-
> devel-ml, which is subscribed to QEMU.
> https://bugs.launchpad.net/bugs/1028260
>
> Title:
>   ARM: stellaris lm3s6965evb machine model broken
>
> Status in QEMU:
>   New
>
> Bug description:
>   The Stellaris lm3s6965evb Machine model is broken:
>
>   qemu-system-arm -M lm3s6965evb -kernel qs_ek-lm3s6965.bin
>   GNU gdb (GDB) 7.1-ubuntu
>   Copyright (C) 2010 Free Software Foundation, Inc.
>   License GPLv3+: GNU GPL version 3 or later 
> 
>   This is free software: you are free to change and redistribute it.
>   There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
>   and "show warranty" for details.
>   This GDB was configured as "x86_64-linux-gnu".
>   For bug reporting instructions, please see:
>   ...
>   Reading symbols from 
> /home/peterc/Petalogix/Internal/plgx_install/qemu-upstream-regression/petalinux-vx.x/tools/linux-i386/petalogix/bin/qemu-system-arm...done.
>   (gdb) r
>   Starting program: 
> /home/peterc/Petalogix/Internal/plgx_install/qemu-upstream-regression/petalinux-vx.x/tools/linux-i386/petalogix/bin/qemu-system-arm
>  -M lm3s6965evb -kernel qs_ek-lm3s6965.bin
>   [Thread debugging using libthread_db enabled]
>   warning: no loadable sections found in added symbol-file 
>   [New Thread 0x763fc700 (LWP 15156)]
>   qemu-system-arm: 
> /home/peterc/Petalogix/Internal/plgx_src/qemu/hw/qdev.c:309: 
> qdev_get_gpio_in: Assertion `n >= 0 && n < dev->num_gpio_in' failed.
>
>   Program received signal SIGABRT, Aborted.
>   0x006f4a2b in raise (sig=) at 
> ../nptl/sysdeps/unix/sysv/linux/pt-raise.c:42
>   42../nptl/sysdeps/unix/sysv/linux/pt-raise.c: No such file or directory.
> in ../nptl/sysdeps/unix/sysv/linux/pt-raise.c
>   (gdb) bt
>   #0  0x006f4a2b in raise (sig=)
>   at ../nptl/sysdeps/unix/sysv/linux/pt-raise.c:42
>   #1  0x007089a0 in abort ()
>   #2  0x00702c15 in __assert_fail ()
>   #3  0x00463ffa in qdev_get_gpio_in ()
>   #4  0x00588f94 in armv7m_init ()
>   #5  0x0060c62a in stellaris_init ()
>   #6  0x0060cc4e in lm3s6965evb_init ()
>   #7  0x004fdb11 in main ()
>   (gdb)
>
>   Bisection points at this commit:
>
>   commit 1e8cae4dfea2bcc91d3820dcf4f9284e7b0abb28
>   Author: Peter 

[Qemu-devel] [Bug 1028260] [NEW] ARM: stellaris lm3s6965evb machine model broken

[Peter Crosthwaite]

Public bug reported:

The Stellaris lm3s6965evb Machine model is broken:

qemu-system-arm -M lm3s6965evb -kernel qs_ek-lm3s6965.bin
GNU gdb (GDB) 7.1-ubuntu
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
For bug reporting instructions, please see:
...
Reading symbols from 
/home/peterc/Petalogix/Internal/plgx_install/qemu-upstream-regression/petalinux-vx.x/tools/linux-i386/petalogix/bin/qemu-system-arm...done.
(gdb) r
Starting program: 
/home/peterc/Petalogix/Internal/plgx_install/qemu-upstream-regression/petalinux-vx.x/tools/linux-i386/petalogix/bin/qemu-system-arm
 -M lm3s6965evb -kernel qs_ek-lm3s6965.bin
[Thread debugging using libthread_db enabled]
warning: no loadable sections found in added symbol-file 
[New Thread 0x763fc700 (LWP 15156)]
qemu-system-arm: /home/peterc/Petalogix/Internal/plgx_src/qemu/hw/qdev.c:309: 
qdev_get_gpio_in: Assertion `n >= 0 && n < dev->num_gpio_in' failed.

Program received signal SIGABRT, Aborted.
0x006f4a2b in raise (sig=) at 
../nptl/sysdeps/unix/sysv/linux/pt-raise.c:42
42  ../nptl/sysdeps/unix/sysv/linux/pt-raise.c: No such file or directory.
in ../nptl/sysdeps/unix/sysv/linux/pt-raise.c
(gdb) bt
#0  0x006f4a2b in raise (sig=)
at ../nptl/sysdeps/unix/sysv/linux/pt-raise.c:42
#1  0x007089a0 in abort ()
#2  0x00702c15 in __assert_fail ()
#3  0x00463ffa in qdev_get_gpio_in ()
#4  0x00588f94 in armv7m_init ()
#5  0x0060c62a in stellaris_init ()
#6  0x0060cc4e in lm3s6965evb_init ()
#7  0x004fdb11 in main ()
(gdb) 

Bisection points at this commit:

commit 1e8cae4dfea2bcc91d3820dcf4f9284e7b0abb28
Author: Peter Maydell 
Date:   Wed May 2 16:49:42 2012 +

hw/armv7m_nvic: Make the NVIC a freestanding class

Rearrange the GIC and NVIC so both are straightforward
subclasses of a common class, rather than having the NVIC
source file textually include arm_gic.c.

Signed-off-by: Peter Maydell 

** Affects: qemu
 Importance: Undecided
 Status: New

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1028260

Title:
  ARM: stellaris lm3s6965evb machine model broken

Status in QEMU:
  New

Bug description:
  The Stellaris lm3s6965evb Machine model is broken:

  qemu-system-arm -M lm3s6965evb -kernel qs_ek-lm3s6965.bin
  GNU gdb (GDB) 7.1-ubuntu
  Copyright (C) 2010 Free Software Foundation, Inc.
  License GPLv3+: GNU GPL version 3 or later 
  This is free software: you are free to change and redistribute it.
  There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
  and "show warranty" for details.
  This GDB was configured as "x86_64-linux-gnu".
  For bug reporting instructions, please see:
  ...
  Reading symbols from 
/home/peterc/Petalogix/Internal/plgx_install/qemu-upstream-regression/petalinux-vx.x/tools/linux-i386/petalogix/bin/qemu-system-arm...done.
  (gdb) r
  Starting program: 
/home/peterc/Petalogix/Internal/plgx_install/qemu-upstream-regression/petalinux-vx.x/tools/linux-i386/petalogix/bin/qemu-system-arm
 -M lm3s6965evb -kernel qs_ek-lm3s6965.bin
  [Thread debugging using libthread_db enabled]
  warning: no loadable sections found in added symbol-file 
  [New Thread 0x763fc700 (LWP 15156)]
  qemu-system-arm: /home/peterc/Petalogix/Internal/plgx_src/qemu/hw/qdev.c:309: 
qdev_get_gpio_in: Assertion `n >= 0 && n < dev->num_gpio_in' failed.

  Program received signal SIGABRT, Aborted.
  0x006f4a2b in raise (sig=) at 
../nptl/sysdeps/unix/sysv/linux/pt-raise.c:42
  42../nptl/sysdeps/unix/sysv/linux/pt-raise.c: No such file or directory.
in ../nptl/sysdeps/unix/sysv/linux/pt-raise.c
  (gdb) bt
  #0  0x006f4a2b in raise (sig=)
  at ../nptl/sysdeps/unix/sysv/linux/pt-raise.c:42
  #1  0x007089a0 in abort ()
  #2  0x00702c15 in __assert_fail ()
  #3  0x00463ffa in qdev_get_gpio_in ()
  #4  0x00588f94 in armv7m_init ()
  #5  0x0060c62a in stellaris_init ()
  #6  0x0060cc4e in lm3s6965evb_init ()
  #7  0x004fdb11 in main ()
  (gdb) 

  Bisection points at this commit:

  commit 1e8cae4dfea2bcc91d3820dcf4f9284e7b0abb28
  Author: Peter Maydell 
  Date:   Wed May 2 16:49:42 2012 +

  hw/armv7m_nvic: Make the NVIC a freestanding class
  
  Rearrange the GIC and NVIC so both are straightforward
  subclasses of a common class, rather than having the NVIC
  source file textually include arm_gic.c.
  
  Signed-off-by

Re: [Qemu-devel] [RFC PATCH 0/2] GlusterFS support in QEMU - v2

[Bharata B Rao]

On Mon, Jul 23, 2012 at 08:34:30AM -0600, Eric Blake wrote:
> On 07/23/2012 03:20 AM, Stefan Hajnoczi wrote:
> >>
> >> So without the capability to pass custom options to block drivers, am I 
> >> forced
> >> to keep extending the file= with more and more options ?
> >>
> >> file=gluster:transport:server:port:volname:image ?
> >>
> >> Looks ugly and not easy to make any particular option optional. If needed 
> >> I can
> >> support this from GlusterFS backend.
> > 
> > Kevin, Markus: Any thoughts on passing options to block drivers?
> > Encoding GlusterFS options into a "filename" string is pretty
> > cumbersome.
> 
> On 07/23/2012 03:28 AM, ronnie sahlberg wrote:> Why not use
> >
> > -drive file=gluster://server[:port]/volname/image
> 
> At which point, options can fit into this URI scheme:
> 
> -drive file=gluster://server:port/volname/image?option1=foo&option2=bar
> 
> where anything after the ? of the URI can introduce whichever options
> you need.

The URI covered everything and left only transport as the option, which could
be made part of the URI itself ?

So looks like we have two options:

gluster://server[:port]/[transport]/volname/image

vs

gluster:server:[port]:[transport]:volname:image

Unless there is a strong preference on one over the other, I am inclined
to go with the latter (colon based) approach and expect user to provide
double colons (::) wherever any default value needs to be specified.

Eg 1. gluster:localhost:::test:/a.img
Eg 2. gluster:localhost:0:socket:test:/a.img

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH 0/6] virtio-trace: Support virtio-trace

[Masami Hiramatsu]

(2012/07/24 11:36), Yoshihiro YUNOMAE wrote:
> Therefore, we propose a new system "virtio-trace", which uses enhanced
> virtio-serial and existing ring-buffer of ftrace, for collecting guest kernel
> tracing data. In this system, there are 5 main components:
>  (1) Ring-buffer of ftrace in a guest
>  - When trace agent reads ring-buffer, a page is removed from ring-buffer.
>  (2) Trace agent in the guest
>  - Splice the page of ring-buffer to read_pipe using splice() without
>memory copying. Then, the page is spliced from write_pipe to virtio
>without memory copying.
>  (3) Virtio-console driver in the guest
>  - Pass the page to virtio-ring
>  (4) Virtio-serial bus in QEMU
>  - Copy the page to kernel pipe
>  (5) Reader in the host
>  - Read guest tracing data via FIFO(named pipe)

So, this is our answer for the argued points in previous thread.
This virtio-serial and ftrace enhancements doesn't introduce new
"ringbuffer" in the kernel, and just use virtio's ringbuffer.
Also, using splice gives us a great advantage in the performance
because of copy-less trace-data transfer.

Actually, one copy should occur in the host (to write it into the pipe),
because removing physical pages of the guest is hard to track and may
involve a TLB flush per page, even if it is done in background.

Thank you,

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu...@hitachi.com

[Qemu-devel] [RFC PATCH 5/6] virtio/console: Allocate scatterlist according to the current pipe size

[Yoshihiro YUNOMAE]

From: Masami Hiramatsu 

Allocate scatterlist according to the current pipe size.
This allows splicing bigger buffer if the pipe size has
been changed by fcntl.

Signed-off-by: Masami Hiramatsu 
Cc: Amit Shah 
Cc: Arnd Bergmann 
Cc: Greg Kroah-Hartman 
---

 drivers/char/virtio_console.c |   23 ---
 1 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index e49d435..f5063d5 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -229,7 +229,6 @@ struct port {
bool guest_connected;
 };
 
-#define MAX_SPLICE_PAGES   32
 /* This is the very early arch-specified put chars function. */
 static int (*early_put_chars)(u32, const char *, int);
 
@@ -482,15 +481,16 @@ struct buffer_token {
void *buf;
struct scatterlist *sg;
} u;
-   bool sgpages;
+   /* If sgpages == 0 then buf is used, else sg is used */
+   unsigned int sgpages;
 };
 
-static void reclaim_sg_pages(struct scatterlist *sg)
+static void reclaim_sg_pages(struct scatterlist *sg, unsigned int nrpages)
 {
int i;
struct page *page;
 
-   for (i = 0; i < MAX_SPLICE_PAGES; i++) {
+   for (i = 0; i < nrpages; i++) {
page = sg_page(&sg[i]);
if (!page)
break;
@@ -511,7 +511,7 @@ static void reclaim_consumed_buffers(struct port *port)
}
while ((tok = virtqueue_get_buf(port->out_vq, &len))) {
if (tok->sgpages)
-   reclaim_sg_pages(tok->u.sg);
+   reclaim_sg_pages(tok->u.sg, tok->sgpages);
else
kfree(tok->u.buf);
kfree(tok);
@@ -581,7 +581,7 @@ static ssize_t send_buf(struct port *port, void *in_buf, 
size_t in_count,
tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
if (!tok)
return -ENOMEM;
-   tok->sgpages = false;
+   tok->sgpages = 0;
tok->u.buf = in_buf;
 
sg_init_one(sg, in_buf, in_count);
@@ -597,7 +597,7 @@ static ssize_t send_pages(struct port *port, struct 
scatterlist *sg, int nents,
tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
if (!tok)
return -ENOMEM;
-   tok->sgpages = true;
+   tok->sgpages = nents;
tok->u.sg = sg;
 
return __send_to_port(port, sg, nents, in_count, tok, nonblock);
@@ -797,6 +797,7 @@ out:
 
 struct sg_list {
unsigned int n;
+   unsigned int size;
size_t len;
struct scatterlist *sg;
 };
@@ -807,7 +808,7 @@ static int pipe_to_sg(struct pipe_inode_info *pipe, struct 
pipe_buffer *buf,
struct sg_list *sgl = sd->u.data;
unsigned int offset, len;
 
-   if (sgl->n == MAX_SPLICE_PAGES)
+   if (sgl->n == sgl->size)
return 0;
 
/* Try lock this page */
@@ -868,12 +869,12 @@ static ssize_t port_fops_splice_write(struct 
pipe_inode_info *pipe,
 
sgl.n = 0;
sgl.len = 0;
-   sgl.sg = kmalloc(sizeof(struct scatterlist) * MAX_SPLICE_PAGES,
-GFP_ATOMIC);
+   sgl.size = pipe->nrbufs;
+   sgl.sg = kmalloc(sizeof(struct scatterlist) * sgl.size, GFP_ATOMIC);
if (unlikely(!sgl.sg))
return -ENOMEM;
 
-   sg_init_table(sgl.sg, MAX_SPLICE_PAGES);
+   sg_init_table(sgl.sg, sgl.size);
ret = __splice_from_pipe(pipe, &sd, pipe_to_sg);
if (likely(ret > 0))
ret = send_pages(port, sgl.sg, sgl.n, sgl.len, true);

[Qemu-devel] [RFC PATCH 4/6] ftrace: Allow stealing pages from pipe buffer

[Yoshihiro YUNOMAE]

From: Masami Hiramatsu 

Use generic steal operation on pipe buffer to allow stealing
ring buffer's read page from pipe buffer.

Note that this could reduce the performance of splice on the
splice_write side operation without affinity setting.
Since the ring buffer's read pages are allocated on the
tracing-node, but the splice user does not always execute
splice write side operation on the same node. In this case,
the page will be accessed from the another node.
Thus, it is strongly recommended to assign the splicing
thread to corresponding node.

Signed-off-by: Masami Hiramatsu 
Cc: Steven Rostedt 
Cc: Frederic Weisbecker 
Cc: Ingo Molnar 
---

 kernel/trace/trace.c |8 +---
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index a120f98..ae01930 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4194,12 +4194,6 @@ static void buffer_pipe_buf_release(struct 
pipe_inode_info *pipe,
buf->private = 0;
 }
 
-static int buffer_pipe_buf_steal(struct pipe_inode_info *pipe,
-struct pipe_buffer *buf)
-{
-   return 1;
-}
-
 static void buffer_pipe_buf_get(struct pipe_inode_info *pipe,
struct pipe_buffer *buf)
 {
@@ -4215,7 +4209,7 @@ static const struct pipe_buf_operations 
buffer_pipe_buf_ops = {
.unmap  = generic_pipe_buf_unmap,
.confirm= generic_pipe_buf_confirm,
.release= buffer_pipe_buf_release,
-   .steal  = buffer_pipe_buf_steal,
+   .steal  = generic_pipe_buf_steal,
.get= buffer_pipe_buf_get,
 };
 

[Qemu-devel] [RFC PATCH 6/6] tools: Add guest trace agent as a user tool

[Yoshihiro YUNOMAE]

This patch adds a user tool, "trace agent" for sending trace data of a guest to
a Host in low overhead. This agent has the following functions:
 - splice a page of ring-buffer to read_pipe without memory copying
 - splice the page from write_pipe to virtio-console without memory copying
 - write trace data to stdout by using -o option
 - controlled by start/stop orders from a Host

Signed-off-by: Yoshihiro YUNOMAE 
---

 tools/virtio/virtio-trace/Makefile  |   14 +
 tools/virtio/virtio-trace/README|  118 
 tools/virtio/virtio-trace/trace-agent-ctl.c |  137 ++
 tools/virtio/virtio-trace/trace-agent-rw.c  |  192 +++
 tools/virtio/virtio-trace/trace-agent.c |  270 +++
 tools/virtio/virtio-trace/trace-agent.h |   75 
 6 files changed, 806 insertions(+), 0 deletions(-)
 create mode 100644 tools/virtio/virtio-trace/Makefile
 create mode 100644 tools/virtio/virtio-trace/README
 create mode 100644 tools/virtio/virtio-trace/trace-agent-ctl.c
 create mode 100644 tools/virtio/virtio-trace/trace-agent-rw.c
 create mode 100644 tools/virtio/virtio-trace/trace-agent.c
 create mode 100644 tools/virtio/virtio-trace/trace-agent.h

diff --git a/tools/virtio/virtio-trace/Makefile 
b/tools/virtio/virtio-trace/Makefile
new file mode 100644
index 000..ef3adfc
--- /dev/null
+++ b/tools/virtio/virtio-trace/Makefile
@@ -0,0 +1,14 @@
+CC = gcc
+CFLAGS = -O2 -Wall
+LFLAG = -lpthread
+
+all: trace-agent
+
+.c.o:
+   $(CC) $(CFLAGS) $(LFLAG) -c $^ -o $@
+
+trace-agent: trace-agent.o trace-agent-ctl.o trace-agent-rw.o
+   $(CC) $(CFLAGS) $(LFLAG) -o $@ $^
+
+clean:
+   rm -f *.o trace-agent
diff --git a/tools/virtio/virtio-trace/README b/tools/virtio/virtio-trace/README
new file mode 100644
index 000..b64845b
--- /dev/null
+++ b/tools/virtio/virtio-trace/README
@@ -0,0 +1,118 @@
+Trace Agent for virtio-trace
+
+
+Trace agent is a user tool for sending trace data of a guest to a Host in low
+overhead. Trace agent has the following functions:
+ - splice a page of ring-buffer to read_pipe without memory copying
+ - splice the page from write_pipe to virtio-console without memory copying
+ - write trace data to stdout by using -o option
+ - controlled by start/stop orders from a Host
+
+The trace agent operates as follows:
+ 1) Initialize all structures.
+ 2) Create a read/write thread per CPU. Each thread is bound to a CPU.
+The read/write threads hold it.
+ 3) A controller thread does poll() for a start order of a host.
+ 4) After the controller of the trace agent receives a start order from a host,
+the controller wake read/write threads.
+ 5) The read/write threads start to read trace data from ring-buffers and
+write the data to virtio-serial.
+ 6) If the controller receives a stop order from a host, the read/write threads
+stop to read trace data.
+
+
+Files
+=
+
+README: this file
+Makefile: Makefile of trace agent for virtio-trace
+trace-agent.c: includes main function, sets up for operating trace agent
+trace-agent.h: includes all structures and some macros
+trace-agent-ctl.c: includes controller function for read/write threads
+trace-agent-rw.c: includes read/write threads function
+
+
+Setup
+=
+
+To use this trace agent for virtio-trace, we need to prepare some virtio-serial
+I/Fs.
+
+1) Make FIFO in a host
+ virtio-trace uses virtio-serial pipe as trace data paths as to the number
+of CPUs and a control path, so FIFO (named pipe) should be created as follows:
+   # mkdir /tmp/virtio-trace/
+   # mkfifo /tmp/virtio-trace/trace-path-cpu{0,1,2,...,X}.{in,out}
+   # mkfifo /tmp/virtio-trace/agent-ctl-path.{in,out}
+
+For example, if a guest use three CPUs, the names are
+   trace-path-cpu{0,1,2}.{in.out}
+and
+   agent-ctl-path.{in,out}.
+
+2) Set up of virtio-serial pipe in a host
+ Add qemu option to use virtio-serial pipe.
+
+ ##virtio-serial device##
+ -device virtio-serial-pci,id=virtio-serial0\
+ ##control path##
+ -chardev pipe,id=charchannel0,path=/tmp/virtio-trace/agent-ctl-path\
+ -device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,\
+  id=channel0,name=agent-ctl-path\
+ ##data path##
+ -chardev pipe,id=charchannel1,path=/tmp/virtio-trace/trace-path-cpu0\
+ -device virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel0,\
+  id=channel1,name=trace-path-cpu0\
+  ...
+
+If you manage guests with libvirt, add the following tags to domain XML files.
+Then, libvirt passes the same command option to qemu.
+
+   
+  
+  
+  
+   
+   
+  
+  
+  
+   
+   ...
+Here, chardev names are restricted to trace-path-cpuX and agent-ctl-path. For
+example, if a guest use three CPUs, chardev names should be trace-path-cpu0,
+trace-path-cpu1, trace-path-cpu2, and agent-ctl-path.
+
+3) Boot the guest
+ You can find some chardev in 

[Qemu-devel] [RFC PATCH 3/6] virtio/console: Wait until the port is ready on splice

[Yoshihiro YUNOMAE]

From: Masami Hiramatsu 

Wait if the port is not connected or full on splice
like as write is doing.

Signed-off-by: Masami Hiramatsu 
Cc: Amit Shah 
Cc: Arnd Bergmann 
Cc: Greg Kroah-Hartman 
---

 drivers/char/virtio_console.c |   39 +++
 1 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 911cb3e..e49d435 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -724,6 +724,26 @@ static ssize_t port_fops_read(struct file *filp, char 
__user *ubuf,
return fill_readbuf(port, ubuf, count, true);
 }
 
+static int wait_port_writable(struct port *port, bool nonblock)
+{
+   int ret;
+
+   if (will_write_block(port)) {
+   if (nonblock)
+   return -EAGAIN;
+
+   ret = wait_event_freezable(port->waitqueue,
+  !will_write_block(port));
+   if (ret < 0)
+   return ret;
+   }
+   /* Port got hot-unplugged. */
+   if (!port->guest_connected)
+   return -ENODEV;
+
+   return 0;
+}
+
 static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
   size_t count, loff_t *offp)
 {
@@ -740,18 +760,9 @@ static ssize_t port_fops_write(struct file *filp, const 
char __user *ubuf,
 
nonblock = filp->f_flags & O_NONBLOCK;
 
-   if (will_write_block(port)) {
-   if (nonblock)
-   return -EAGAIN;
-
-   ret = wait_event_freezable(port->waitqueue,
-  !will_write_block(port));
-   if (ret < 0)
-   return ret;
-   }
-   /* Port got hot-unplugged. */
-   if (!port->guest_connected)
-   return -ENODEV;
+   ret = wait_port_writable(port, nonblock);
+   if (ret < 0)
+   return ret;
 
count = min((size_t)(32 * 1024), count);
 
@@ -851,6 +862,10 @@ static ssize_t port_fops_splice_write(struct 
pipe_inode_info *pipe,
.u.data = &sgl,
};
 
+   ret = wait_port_writable(port, filp->f_flags & O_NONBLOCK);
+   if (ret < 0)
+   return ret;
+
sgl.n = 0;
sgl.len = 0;
sgl.sg = kmalloc(sizeof(struct scatterlist) * MAX_SPLICE_PAGES,

[Qemu-devel] [RFC PATCH 1/6] virtio/console: Add splice_write support

[Yoshihiro YUNOMAE]

From: Masami Hiramatsu 

Enable to use splice_write from pipe to virtio-console port.
This steals pages from pipe and directly send it to host.

Note that this may accelerate only the guest to host path.

Signed-off-by: Masami Hiramatsu 
Cc: Amit Shah 
Cc: Arnd Bergmann 
Cc: Greg Kroah-Hartman 
---

 drivers/char/virtio_console.c |  136 +++--
 1 files changed, 128 insertions(+), 8 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index cdf2f54..fe31b2f 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -24,6 +24,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -227,6 +229,7 @@ struct port {
bool guest_connected;
 };
 
+#define MAX_SPLICE_PAGES   32
 /* This is the very early arch-specified put chars function. */
 static int (*early_put_chars)(u32, const char *, int);
 
@@ -474,26 +477,52 @@ static ssize_t send_control_msg(struct port *port, 
unsigned int event,
return 0;
 }
 
+struct buffer_token {
+   union {
+   void *buf;
+   struct scatterlist *sg;
+   } u;
+   bool sgpages;
+};
+
+static void reclaim_sg_pages(struct scatterlist *sg)
+{
+   int i;
+   struct page *page;
+
+   for (i = 0; i < MAX_SPLICE_PAGES; i++) {
+   page = sg_page(&sg[i]);
+   if (!page)
+   break;
+   put_page(page);
+   }
+   kfree(sg);
+}
+
 /* Callers must take the port->outvq_lock */
 static void reclaim_consumed_buffers(struct port *port)
 {
-   void *buf;
+   struct buffer_token *tok;
unsigned int len;
 
if (!port->portdev) {
/* Device has been unplugged.  vqs are already gone. */
return;
}
-   while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
-   kfree(buf);
+   while ((tok = virtqueue_get_buf(port->out_vq, &len))) {
+   if (tok->sgpages)
+   reclaim_sg_pages(tok->u.sg);
+   else
+   kfree(tok->u.buf);
+   kfree(tok);
port->outvq_full = false;
}
 }
 
-static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
-   bool nonblock)
+static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
+ int nents, size_t in_count,
+ struct buffer_token *tok, bool nonblock)
 {
-   struct scatterlist sg[1];
struct virtqueue *out_vq;
ssize_t ret;
unsigned long flags;
@@ -505,8 +534,7 @@ static ssize_t send_buf(struct port *port, void *in_buf, 
size_t in_count,
 
reclaim_consumed_buffers(port);
 
-   sg_init_one(sg, in_buf, in_count);
-   ret = virtqueue_add_buf(out_vq, sg, 1, 0, in_buf, GFP_ATOMIC);
+   ret = virtqueue_add_buf(out_vq, sg, nents, 0, tok, GFP_ATOMIC);
 
/* Tell Host to go! */
virtqueue_kick(out_vq);
@@ -544,6 +572,37 @@ done:
return in_count;
 }
 
+static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
+   bool nonblock)
+{
+   struct scatterlist sg[1];
+   struct buffer_token *tok;
+
+   tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
+   if (!tok)
+   return -ENOMEM;
+   tok->sgpages = false;
+   tok->u.buf = in_buf;
+
+   sg_init_one(sg, in_buf, in_count);
+
+   return __send_to_port(port, sg, 1, in_count, tok, nonblock);
+}
+
+static ssize_t send_pages(struct port *port, struct scatterlist *sg, int nents,
+ size_t in_count, bool nonblock)
+{
+   struct buffer_token *tok;
+
+   tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
+   if (!tok)
+   return -ENOMEM;
+   tok->sgpages = true;
+   tok->u.sg = sg;
+
+   return __send_to_port(port, sg, nents, in_count, tok, nonblock);
+}
+
 /*
  * Give out the data that's requested from the buffer that we have
  * queued up.
@@ -725,6 +784,66 @@ out:
return ret;
 }
 
+struct sg_list {
+   unsigned int n;
+   size_t len;
+   struct scatterlist *sg;
+};
+
+static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
+   struct splice_desc *sd)
+{
+   struct sg_list *sgl = sd->u.data;
+   unsigned int len = 0;
+
+   if (sgl->n == MAX_SPLICE_PAGES)
+   return 0;
+
+   /* Try lock this page */
+   if (buf->ops->steal(pipe, buf) == 0) {
+   /* Get reference and unlock page for moving */
+   get_page(buf->page);
+   unlock_page(buf->page);
+
+   len = min(buf->len, sd->len);
+   sg_set_page(&(sgl->sg[sgl->n]), buf->page, len, buf->offset);
+   sgl->n++;
+   sgl->len += len;
+   }
+
+   return len;
+}
+
+/* Faster zero-copy write by splicing */
+static

[Qemu-devel] [RFC PATCH 2/6] virtio/console: Add a failback for unstealable pipe buffer

[Yoshihiro YUNOMAE]

From: Masami Hiramatsu 

Add a failback memcpy path for unstealable pipe buffer.
If buf->ops->steal() fails, virtio-serial tries to
copy the page contents to an allocated page, instead
of just failing splice().

Signed-off-by: Masami Hiramatsu 
Cc: Amit Shah 
Cc: Arnd Bergmann 
Cc: Greg Kroah-Hartman 
---

 drivers/char/virtio_console.c |   28 +---
 1 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index fe31b2f..911cb3e 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -794,7 +794,7 @@ static int pipe_to_sg(struct pipe_inode_info *pipe, struct 
pipe_buffer *buf,
struct splice_desc *sd)
 {
struct sg_list *sgl = sd->u.data;
-   unsigned int len = 0;
+   unsigned int offset, len;
 
if (sgl->n == MAX_SPLICE_PAGES)
return 0;
@@ -807,9 +807,31 @@ static int pipe_to_sg(struct pipe_inode_info *pipe, struct 
pipe_buffer *buf,
 
len = min(buf->len, sd->len);
sg_set_page(&(sgl->sg[sgl->n]), buf->page, len, buf->offset);
-   sgl->n++;
-   sgl->len += len;
+   } else {
+   /* Failback to copying a page */
+   struct page *page = alloc_page(GFP_KERNEL);
+   char *src = buf->ops->map(pipe, buf, 1);
+   char *dst;
+
+   if (!page)
+   return -ENOMEM;
+   dst = kmap(page);
+
+   offset = sd->pos & ~PAGE_MASK;
+
+   len = sd->len;
+   if (len + offset > PAGE_SIZE)
+   len = PAGE_SIZE - offset;
+
+   memcpy(dst + offset, src + buf->offset, len);
+
+   kunmap(page);
+   buf->ops->unmap(pipe, buf, src);
+
+   sg_set_page(&(sgl->sg[sgl->n]), page, len, offset);
}
+   sgl->n++;
+   sgl->len += len;
 
return len;
 }

[Qemu-devel] [RFC PATCH 0/6] virtio-trace: Support virtio-trace

[Yoshihiro YUNOMAE]

Hi All,

The following patch set provides a low-overhead system for collecting kernel
tracing data of guests by a host in a virtualization environment.

A guest OS generally shares some devices with other guests or a host, so
reasons of any problems occurring in a guest may be from other guests or a host.
Then, to collect some tracing data of a number of guests and a host is needed
when some problems occur in a virtualization environment. One of methods to
realize that is to collect tracing data of guests in a host. To do this, network
is generally used. However, high load will be taken to applications on guests
using network I/O because there are many network stack layers. Therefore,
a communication method for collecting the data without using network is needed.

We submitted a patch set of "IVRing", a ring-buffer driver constructed on
Inter-VM shared memory (IVShmem), to LKML http://lwn.net/Articles/500304/ in
this June. IVRing and the IVRing reader use POSIX shared memory each other
without using network, so a low-overhead system for collecting guest tracing
data is realized. However, this patch set has some problems as follows:
 - use IVShmem instead of virtio
 - create a new ring-buffer without using existing ring-buffer in kernel
 - scalability
   -- not support SMP environment
   -- buffer size limitation
   -- not support live migration (maybe difficult for realize this)

Therefore, we propose a new system "virtio-trace", which uses enhanced
virtio-serial and existing ring-buffer of ftrace, for collecting guest kernel
tracing data. In this system, there are 5 main components:
 (1) Ring-buffer of ftrace in a guest
 - When trace agent reads ring-buffer, a page is removed from ring-buffer.
 (2) Trace agent in the guest
 - Splice the page of ring-buffer to read_pipe using splice() without
   memory copying. Then, the page is spliced from write_pipe to virtio
   without memory copying.
 (3) Virtio-console driver in the guest
 - Pass the page to virtio-ring
 (4) Virtio-serial bus in QEMU
 - Copy the page to kernel pipe
 (5) Reader in the host
 - Read guest tracing data via FIFO(named pipe) 

***Evaluation***
When a host collects tracing data of a guest, the performance of using
virtio-trace is compared with that of using native(just running ftrace),
IVRing, and virtio-serial(normal method of read/write).


The overview of this evaluation is as follows:
 (a) A guest on a KVM is prepared.
 - The guest is dedicated one physical CPU as a virtual CPU(VCPU).

 (b) The guest starts to write tracing data to ring-buffer of ftrace.
 - The probe points are all trace points of sched, timer, and kmem.

 (c) Writing trace data, dhrystone 2 in UNIX bench is executed as a benchmark
 tool in the guest.
 - Dhrystone 2 intends system performance by repeating integer arithmetic
   as a score.
 - Since higher score equals to better system performance, if the score
   decrease based on bare environment, it indicates that any operation
   disturbs the integer arithmetic. Then, we define the overhead of
   transporting trace data is calculated as follows:
OVERHEAD = (1 - SCORE_OF_A_METHOD/NATIVE_SCORE) * 100.

The performance of each method is compared as follows:
 [1] Native
 - only recording trace data to ring-buffer on a guest
 [2] Virtio-trace
 - running a trace agent on a guest
 - a reader on a host opens FIFO using cat command
 [3] IVRing
 - A SystemTap script in a guest records trace data to IVRing.
   -- probe points are same as ftrace.
 [4] Virtio-serial(normal)
 - A reader(using cat) on a guest output trace data to a host using
   standard output via virtio-serial.

Other information is as follows:
 - host
   kernel: 3.3.7-1 (Fedora16)
   CPU: Intel Xeon x5660@2.80GHz(12core)
   Memory: 48GB

 - guest(only booting one guest)
   kernel: 3.5.0-rc4+ (Fedora16)
   CPU: 1VCPU(dedicated)
   Memory: 1GB


3 patterns based on the bare environment were indicated as follows:
   Scores  overhead against [0] Native
[0] Native:  28807569.5   -
[1] Virtio-trace:28685049.5 0.43%
[2] IVRing:  28418595.5 1.35%
[3] Virtio-serial:   13262258.753.96%


***Just enhancement ideas***
 - Support for trace-cmd
 - Support for 9pfs protocol
 - Support for non-blocking mode in QEMU
 - Make "vhost-serial"

Thank you,

---

Masami Hiramatsu (5):
  virtio/console: Allocate scatterlist according to the current pipe size
  ftrace: Allow stealing pages from pipe buffer
  virtio/console: Wait until the port is ready on splice
  virtio/console: Add a failback for unstealable pipe buffer
  virtio/console: Add splice_write support

Yoshihiro YUNOMAE (1):
  tools: Add guest trace agent as a user tool


 drivers/char/virtio_console.c   |  198 ++--
 kernel/trace/trace.c  

Re: [Qemu-devel] [PATCH v5 1/6] qemu-char: Add MSG_CMSG_CLOEXEC flag to recvmsg

[Corey Bryant]

On 07/23/2012 06:50 PM, Eric Blake wrote:

On 07/23/2012 07:08 AM, Corey Bryant wrote:

Set the close-on-exec flag for the file descriptor received
via SCM_RIGHTS.




+++ b/qemu-char.c
@@ -2263,9 +2263,17 @@ static ssize_t tcp_chr_recv(CharDriverState *chr, char 
*buf, size_t len)
  msg.msg_control = &msg_control;
  msg.msg_controllen = sizeof(msg_control);

+#ifdef MSG_CMSG_CLOEXEC
+ret = recvmsg(s->fd, &msg, MSG_CMSG_CLOEXEC);
+#else
  ret = recvmsg(s->fd, &msg, 0);
-if (ret > 0 && s->is_unix)
+if (ret > 0) {
+qemu_set_cloexec(s->fd);


Wrong fd.  You aren't changing cloexec on the socket (s->fd), but on the
fd that was received via msg (which you don't know at this point in time).



Ugh, that's bad.


+}
+#endif
+if (ret > 0 && s->is_unix) {
  unix_process_msgfd(chr, &msg);


Only here do you know what fd you received.

I would write it more like:

int flags = 0;
#ifdef MSG_CMSG_CLOEXEC
flags |= MSG_CMSG_CLOEXEC
#endif
ret = recvmsg(s->fd, &msg, flags);
if (ret > 0 && s->is_unix) {
 unix_process_msgfd(chr, &msg);
#ifndef MSG_CMSG_CLOEXEC
 qemu_set_cloexec(/* fd determined from msg */)
#endif
}

which almost implies that unix_process_msgfd() should be the function
that sets cloexec, but without wasting the time doing so if recvmsg
already did the job.



Thanks for the suggestion and catching this.  I'll take this into 
account in the next version.


--
Regards,
Corey

[Qemu-devel] [PATCH 2/2] vhost-scsi: Add support for VHOST_SCSI_GET_ABI_VERSION ioctl

[Nicholas A. Bellinger]

From: Nicholas Bellinger 

This patch adds support in vhost-scsi QEMU userspace code to check the
running tcm_vhost ABI version using VHOST_SCSI_GET_ABI_VERSION.

This series selects VHOST_SCSI_ABI_VERSION=1 for current code, and
requires the pre-requsite tcm_vhost patch to expose an ABI version number.

This patch patch will fail to initialize vhost_scsi_start() unless both
vhost-scsi <-> tcm_vhost agree on VHOST_SCSI_ABI_VERSION=1.

Reported-by: Anthony Liguori 
Cc: Stefan Hajnoczi 
Cc: Michael S. Tsirkin 
Cc: Paolo Bonzini 
Cc: Zhi Yong Wu 
Signed-off-by: Nicholas Bellinger 
---
 hw/vhost-scsi.c |   17 +
 hw/vhost-scsi.h |   11 +++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/hw/vhost-scsi.c b/hw/vhost-scsi.c
index 3e9fa0e..9ab314c 100644
--- a/hw/vhost-scsi.c
+++ b/hw/vhost-scsi.c
@@ -68,6 +68,23 @@ int vhost_scsi_start(VHostSCSI *vs, VirtIODevice *vdev)
 return ret;
 }
 
+memset(&backend, 0, sizeof(backend));
+ret = ioctl(vs->dev.control, VHOST_SCSI_GET_ABI_VERSION, &backend);
+if (ret < 0) {
+ret = -errno;
+vhost_dev_stop(&vs->dev, vdev);
+return ret;
+}
+if (backend.abi_version > VHOST_SCSI_ABI_VERSION) {
+fprintf(stderr, "The running tcm_vhost kernel abi_version: %d is 
greater"
+   " than vhost_scsi userspace supports: %d\n", 
backend.abi_version,
+   VHOST_SCSI_ABI_VERSION);
+ret = -ENOSYS;
+vhost_dev_stop(&vs->dev, vdev);
+return ret;
+}
+printf("Using TCM_Vhost ABI version: %d\n", backend.abi_version);
+
 pstrcpy((char *)backend.vhost_wwpn, sizeof(backend.vhost_wwpn), vs->wwpn);
 backend.vhost_tpgt = vs->tpgt;
 ret = ioctl(vs->dev.control, VHOST_SCSI_SET_ENDPOINT, &backend);
diff --git a/hw/vhost-scsi.h b/hw/vhost-scsi.h
index 8ce974a..393fe39 100644
--- a/hw/vhost-scsi.h
+++ b/hw/vhost-scsi.h
@@ -17,9 +17,19 @@
 #include "qemu-common.h"
 #include "qemu-option.h"
 
+/*
+ * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
+ *
+ * ABI Rev 0: All pre 2012 revisions used by prototype out-of-tree code
+ * ABI Rev 1: 2012 version for v3.6 kernel merge candiate
+ */
+
+#define VHOST_SCSI_ABI_VERSION 1
+
 /* TODO #include  properly */
 /* For VHOST_SCSI_SET_ENDPOINT/VHOST_SCSI_CLEAR_ENDPOINT ioctl */
 struct vhost_scsi_target {
+int abi_version;
 unsigned char vhost_wwpn[224];
 unsigned short vhost_tpgt;
 };
@@ -27,6 +37,7 @@ struct vhost_scsi_target {
 #define VHOST_VIRTIO 0xAF
 #define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct 
vhost_scsi_target)
 #define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct 
vhost_scsi_target)
+#define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, struct 
vhost_scsi_target)
 
 VHostSCSI *find_vhost_scsi(const char *id);
 const char *vhost_scsi_get_id(VHostSCSI *vs);
-- 
1.7.2.5

[Qemu-devel] [PATCH 1/2] vhost-scsi: Rename vhost_vring_target -> vhost_scsi_target

[Nicholas A. Bellinger]

From: Nicholas Bellinger 

Rename the main IOCTL structure to vhost_scsi_target so that
it makes a little more sense than vhost_vring_target.

As requested by MST.

Reported-by: Michael S. Tsirkin 
Cc: Stefan Hajnoczi 
Cc: Anthony Liguori 
Cc: Paolo Bonzini 
Cc: Zhi Yong Wu 
Signed-off-by: Nicholas Bellinger 
---
 hw/vhost-scsi.c |4 ++--
 hw/vhost-scsi.h |6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/vhost-scsi.c b/hw/vhost-scsi.c
index 3e3378a..3e9fa0e 100644
--- a/hw/vhost-scsi.c
+++ b/hw/vhost-scsi.c
@@ -49,7 +49,7 @@ const char *vhost_scsi_get_id(VHostSCSI *vs)
 int vhost_scsi_start(VHostSCSI *vs, VirtIODevice *vdev)
 {
 int ret;
-struct vhost_vring_target backend;
+struct vhost_scsi_target backend;
 
 if (!vhost_dev_query(&vs->dev, vdev)) {
 return -ENOTSUP;
@@ -86,7 +86,7 @@ void vhost_scsi_stop(VHostSCSI *vs, VirtIODevice *vdev)
 fprintf(stderr, "vhost_scsi_stop\n");
 
 int ret;
-struct vhost_vring_target backend;
+struct vhost_scsi_target backend;
 
 pstrcpy((char *)backend.vhost_wwpn, sizeof(backend.vhost_wwpn), vs->wwpn);
 backend.vhost_tpgt = vs->tpgt;
diff --git a/hw/vhost-scsi.h b/hw/vhost-scsi.h
index 84e9097..8ce974a 100644
--- a/hw/vhost-scsi.h
+++ b/hw/vhost-scsi.h
@@ -19,14 +19,14 @@
 
 /* TODO #include  properly */
 /* For VHOST_SCSI_SET_ENDPOINT/VHOST_SCSI_CLEAR_ENDPOINT ioctl */
-struct vhost_vring_target {
+struct vhost_scsi_target {
 unsigned char vhost_wwpn[224];
 unsigned short vhost_tpgt;
 };
 
 #define VHOST_VIRTIO 0xAF
-#define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct 
vhost_vring_target)
-#define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct 
vhost_vring_target)
+#define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct 
vhost_scsi_target)
+#define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct 
vhost_scsi_target)
 
 VHostSCSI *find_vhost_scsi(const char *id);
 const char *vhost_scsi_get_id(VHostSCSI *vs);
-- 
1.7.2.5

[Qemu-devel] [PATCH 0/2] vhost-scsi: Check for tcm_vhost ABI version

[Nicholas A. Bellinger]

From: Nicholas Bellinger 

Hi Anthony,

Here are the two patches against Zhi's vhost-scsi tree to check for
a supported version (VHOST_SCSI_ABI_VERSION=1) that's now exposed via
the tcm_vhost ioctl.

Please have a look and let me know if this is what you had in mind.

Thanks!

Nicholas Bellinger (2):
  vhost-scsi: Rename vhost_vring_target -> vhost_scsi_target
  vhost-scsi: Add support for VHOST_SCSI_GET_ABI_VERSION ioctl

 hw/vhost-scsi.c |   21 +++--
 hw/vhost-scsi.h |   17 ++---
 2 files changed, 33 insertions(+), 5 deletions(-)

-- 
1.7.2.5

[Qemu-devel] [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION

[Nicholas A. Bellinger]

From: Nicholas Bellinger 

As requested by Anthony, here is a patch against target-pending/for-next-merge
to expose an ABI version to userspace via a new VHOST_SCSI_GET_ABI_VERSION
ioctl operation.

As mentioned in the comment, ABI Rev 0 is for pre 2012 out-of-tree code, and
ABI Rev 1 (the current rev) is for current WIP v3.6 kernel merge candiate code.

I think this is what you had in mind, and hopefully it will make MST happy too.
The incremental vhost-scsi patches against Zhi's QEMU are going out shortly 
ahead
of cutting a new vhost-scsi RFC over the next days.

Please have a look and let me know if you have any concerns here.

Thanks!

Reported-by: Anthony Liguori 
Cc: Stefan Hajnoczi 
Cc: Michael S. Tsirkin 
Cc: Paolo Bonzini 
Cc: Zhi Yong Wu 
Signed-off-by: Nicholas Bellinger 
---
 drivers/vhost/tcm_vhost.c |9 +
 drivers/vhost/tcm_vhost.h |   11 +++
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index dc7e024..3f04169 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -977,6 +977,15 @@ static long vhost_scsi_ioctl(struct file *f, unsigned int 
ioctl,
return -EFAULT;
 
return vhost_scsi_clear_endpoint(vs, &backend);
+   case VHOST_SCSI_GET_ABI_VERSION:
+   if (copy_from_user(&backend, argp, sizeof backend))
+   return -EFAULT;
+
+   backend.abi_version = VHOST_SCSI_ABI_VERSION;
+
+   if (copy_to_user(argp, &backend, sizeof backend))
+   return -EFAULT;
+   return 0;
case VHOST_GET_FEATURES:
features = VHOST_FEATURES;
if (copy_to_user(featurep, &features, sizeof features))
diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
index e942df9..3d5378f 100644
--- a/drivers/vhost/tcm_vhost.h
+++ b/drivers/vhost/tcm_vhost.h
@@ -80,7 +80,17 @@ struct tcm_vhost_tport {
 
 #include 
 
+/*
+ * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
+ *
+ * ABI Rev 0: All pre 2012 revisions used by prototype out-of-tree code
+ * ABI Rev 1: 2012 version for v3.6 kernel merge candiate
+ */
+
+#define VHOST_SCSI_ABI_VERSION 1
+
 struct vhost_scsi_target {
+   int abi_version;
unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
unsigned short vhost_tpgt;
 };
@@ -88,3 +98,4 @@ struct vhost_scsi_target {
 /* VHOST_SCSI specific defines */
 #define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct 
vhost_scsi_target)
 #define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct 
vhost_scsi_target)
+#define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, struct 
vhost_scsi_target)
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH v5 1/6] qemu-char: Add MSG_CMSG_CLOEXEC flag to recvmsg

[Eric Blake]

On 07/23/2012 07:08 AM, Corey Bryant wrote:
> Set the close-on-exec flag for the file descriptor received
> via SCM_RIGHTS.
> 

> +++ b/qemu-char.c
> @@ -2263,9 +2263,17 @@ static ssize_t tcp_chr_recv(CharDriverState *chr, char 
> *buf, size_t len)
>  msg.msg_control = &msg_control;
>  msg.msg_controllen = sizeof(msg_control);
>  
> +#ifdef MSG_CMSG_CLOEXEC
> +ret = recvmsg(s->fd, &msg, MSG_CMSG_CLOEXEC);
> +#else
>  ret = recvmsg(s->fd, &msg, 0);
> -if (ret > 0 && s->is_unix)
> +if (ret > 0) {
> +qemu_set_cloexec(s->fd);

Wrong fd.  You aren't changing cloexec on the socket (s->fd), but on the
fd that was received via msg (which you don't know at this point in time).

> +}
> +#endif
> +if (ret > 0 && s->is_unix) {
>  unix_process_msgfd(chr, &msg);

Only here do you know what fd you received.

I would write it more like:

int flags = 0;
#ifdef MSG_CMSG_CLOEXEC
flags |= MSG_CMSG_CLOEXEC
#endif
ret = recvmsg(s->fd, &msg, flags);
if (ret > 0 && s->is_unix) {
unix_process_msgfd(chr, &msg);
#ifndef MSG_CMSG_CLOEXEC
qemu_set_cloexec(/* fd determined from msg */)
#endif
}

which almost implies that unix_process_msgfd() should be the function
that sets cloexec, but without wasting the time doing so if recvmsg
already did the job.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 16/16] hub: add the support for hub own flow control

[Laszlo Ersek]

On 07/20/12 14:01, Stefan Hajnoczi wrote:
> From: Zhi Yong Wu 
> 
> Only when all other hub port's *peer* .can_receive() all return 1,
> the source hub port .can_receive() return 1.
> 
> Reviewed-off-by: Paolo Bonzini 

Probably a typo.

> Signed-off-by: Zhi Yong Wu 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  net/hub.c |   27 ---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/net/hub.c b/net/hub.c
> index f697a78..2d67df5 100644
> --- a/net/hub.c
> +++ b/net/hub.c
> @@ -15,6 +15,7 @@
>  #include "monitor.h"
>  #include "net.h"
>  #include "hub.h"
> +#include "iov.h"
>  
>  /*
>   * A hub broadcasts incoming packets to all its ports except the source port.
> @@ -59,16 +60,16 @@ static ssize_t net_hub_receive_iov(NetHub *hub, 
> NetHubPort *source_port,
> const struct iovec *iov, int iovcnt)
>  {
>  NetHubPort *port;
> -ssize_t ret = 0;
> +ssize_t len = iov_size(iov, iovcnt);
>  
>  QLIST_FOREACH(port, &hub->ports, next) {
>  if (port == source_port) {
>  continue;
>  }
>  
> -ret = qemu_sendv_packet(&port->nc, iov, iovcnt);
> +qemu_sendv_packet(&port->nc, iov, iovcnt);
>  }
> -return ret;
> +return len;
>  }

This seems to consolidate net_hub_receive_iov()'s retval with that of
net_hub_receive(), but may I ask why it used to be calculated differently?


>  
>  static NetHub *net_hub_new(unsigned int id)
> @@ -85,6 +86,25 @@ static NetHub *net_hub_new(unsigned int id)
>  return hub;
>  }
>  
> +static int net_hub_port_can_receive(NetClientState *nc)
> +{
> +NetHubPort *port;
> +NetHubPort *src_port = DO_UPCAST(NetHubPort, nc, nc);
> +NetHub *hub = src_port->hub;
> +
> +QLIST_FOREACH(port, &hub->ports, next) {
> +if (port == src_port) {
> +continue;
> +}
> +
> +if (!qemu_can_send_packet(&port->nc)) {
> +return 0;
> +}
> +}
> +
> +return 1;
> +}
> +

(qemu_can_send_packet() handles a NULL sender->peer, OK.)

No further comments for the series.

Thanks!
Laszlo

Re: [Qemu-devel] [Qemu-discuss] qemu-kvm: -netdev user: Parameter 'id' is missing

[anatoly techtonik]

Forwarding per discussion in qemu-discuss.
Please CC.

-- Forwarded message --
From: Mike Lovell 
Date: Mon, Jul 23, 2012 at 10:58 PM
Subject: Re: [Qemu-discuss] qemu-kvm: -netdev user: Parameter 'id' is missing
To: qemu-disc...@nongnu.org


On 07/20/2012 04:10 PM, anatoly techtonik wrote:
>
> The documentation at http://wiki.qemu.org/Documentation/Networking
> makes people think that 'id' parameter for -netdev user is optional,
> which doesn't appear to be true:
>
>  $ qemu-kvm -hda image.img -netdev user
>  qemu-kvm: -netdev user: Parameter 'id' is missing
>
> --
> anatoly t.


it does look like there are a few places where it refers to -netdev
without id= but i just took a look through some of the code and it
does look to be required. i don't have a user account on the wiki so i
can't fix it or help get you an account to fix it. you might want to
check with qemu-devel on that.

mike

Re: [Qemu-devel] [PATCH 13/16] net: Make "info network" output more readable info

[Laszlo Ersek]

On 07/20/12 14:01, Stefan Hajnoczi wrote:

> diff --git a/net.h b/net.h
> index 7e629d3..c819de4 100644
> --- a/net.h
> +++ b/net.h
> @@ -100,6 +100,7 @@ void qemu_check_nic_model(NICInfo *nd, const char *model);
>  int qemu_find_nic_model(NICInfo *nd, const char * const *models,
>  const char *default_model);
>  
> +void print_net_client(Monitor *mon, NetClientState *vc);
>  void do_info_network(Monitor *mon);
>  
>  /* NIC info */

The prototype should say "nc", not "vc".


> @@ -867,20 +867,24 @@ void do_info_network(Monitor *mon)
>  NetClientState *nc, *peer;
>  NetClientOptionsKind type;
>  
> -monitor_printf(mon, "Devices not on any VLAN:\n");
> +net_hub_info(mon);
> +
>  QTAILQ_FOREACH(nc, &net_clients, next) {
>  peer = nc->peer;
>  type = nc->info->type;
> +
> +if (net_hub_port_peer_nc(nc)) {
> +continue;
> +}
> +

OK, hub-port-peers are dealt with before the loop, so skip them in the
loop.


> +bool net_hub_port_peer_nc(NetClientState *nc)
> +{
> +NetHub *hub;
> +NetHubPort *port;
> +
> +QLIST_FOREACH(hub, &hubs, next) {
> +QLIST_FOREACH(port, &hub->ports, next) {
> +if (nc == port->nc.peer) {
> +return true;
> +}
> +}
> +}
> +
> +return false;
> +}

I think this function should

(a) be dropped, and do_info_network() should call

net_hub_id_for_client(nc, NULL) == 0

instead, just like assign_name() does starting with patch 2 ("net: Use
hubs for the vlan feature"). (BTW the comment in patch 2 shouldn't say
"on a vlan" any more, it should say "on a hub".) Or, this function
should

(b) stay, but call the updated net_hub_id_for_client(), or

(c) pick up the same update as net_hub_id_for_client().


> @@ -224,8 +243,8 @@ void net_hub_info(Monitor *mon)
>  QLIST_FOREACH(hub, &hubs, next) {
>  monitor_printf(mon, "hub %u\n", hub->id);
>  QLIST_FOREACH(port, &hub->ports, next) {
> -monitor_printf(mon, "port %u peer %s\n", port->id,
> -   port->nc.peer ? port->nc.peer->name : "");
> +monitor_printf(mon, " \\ ");
> +print_net_client(mon, port->nc.peer);
>  }
>  }

Are we sure "port->nc.peer" can't be NULL any longer? print_net_client()
doesn't verify it.

Leastways net_hub_port_find() (called by set_vlan() from
"hw/qdev-properties.c") concerns itself with a NULL port-peer (although
set_vlan() might be restricted to a separate, prior "phase", for what I
know.)

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH] qdev: Fix Open Firmware comment

[Peter Maydell]

On 23 July 2012 19:49, Blue Swirl  wrote:
> On Fri, Jul 20, 2012 at 9:04 PM, Stefan Weil  wrote:
>> +/*
>> + * This callback is used to create Open Firmware device path in 
>> accordance
>> + * with OF spec http://forthworks.com/standards/of1275.pdf. Individual 
>> bus
>> + * bindings can be found at http://playground.sun.com/1275/bindings/.
>> + */
>
> The links are dead. Since you are improving the text, maybe you could
> find archived versions.

Hard to tell not knowing what the original links were, but maybe
for bus bindings:
  http://www.openfirmware.org/ofwg/home.html#OFDbussupps

I suspect that finding a publicly downloadable copy of IEEE1275 is
going to be tricky given it's a commercially published standards doc.

-- PMM

[Qemu-devel] 1. Re: [PATCH] kvm: Move kvm_allows_irq0_override() to target-i386 (Avi Kivity) 2. Re: [PATCH] pc: Fix max_cpus (Dunrong Huang) 3. Re: [PATCH] kvm: Move kvm_allows_irq0_override() to targ

[Paulo Arcinas]

1. Re: [PATCH] kvm: Move kvm_allows_irq0_override() to target-i386 (Avi
Kivity) 2. Re: [PATCH] pc: Fix max_cpus (Dunrong Huang) 3. Re: [PATCH] kvm:
Move kvm_allows_irq0_override() to target-i386 (Avi Kivity) 4. Re: [PATCH]
check for available room when formatting OpenFirmware device path (Peter
Maydell) 5. [PATCH v5 0/3] refactor PC machine, i440fx and piix3 to take
advantage of QOM (Wanpeng Li) 6. [PATCH v5 3/3] convert pci-host to QOM
(Wanpeng Li)

Re: [Qemu-devel] [PATCH 08/16] net: Remove VLANState

[Laszlo Ersek]

On 07/20/12 14:01, Stefan Hajnoczi wrote:

> diff --git a/hw/exynos4_boards.c b/hw/exynos4_boards.c
> index e5c2a5f..4bb0a60 100644
> --- a/hw/exynos4_boards.c
> +++ b/hw/exynos4_boards.c
> @@ -81,7 +81,7 @@ static void lan9215_init(uint32_t base, qemu_irq irq)
>  SysBusDevice *s;
>  
>  /* This should be a 9215 but the 9118 is close enough */
> -if (nd_table[0].vlan) {
> +if (nd_table[0].used) {
>  qemu_check_nic_model(&nd_table[0], "lan9118");
>  dev = qdev_create(NULL, "lan9118");
>  qdev_set_nic_properties(dev, &nd_table[0]);

Right, relevant commits: 0ae18cee & 7697079b.


> @@ -970,7 +895,7 @@ void qmp_set_link(const char *name, bool up, Error **errp)
>  done:
>  
>  if (!vc) {
> -error_set(errp, QERR_DEVICE_NOT_FOUND, name);
> +qerror_report(QERR_DEVICE_NOT_FOUND, name);
>  return;
>  }

Why? The only caller (at this point) seems to be hmp_set_link(), and it
handles the error (with monitor_printf()).

Thanks,
Laszlo

Re: [Qemu-devel] [RFC PATCH 1/4] qapi: Convert savevm

[Luiz Capitulino]

On Mon, 16 Jul 2012 10:12:08 +0200
Pavel Hrdina  wrote:

> On 07/12/2012 07:53 PM, Eric Blake wrote:
> > On 07/12/2012 10:55 AM, Pavel Hrdina wrote:
> >> Signed-off-by: Pavel Hrdina
> >> ---
> >> +++ b/qapi-schema.json
> >> @@ -1868,3 +1868,25 @@
> >>   # Since: 0.14.0
> >>   ##
> >>   { 'command': 'netdev_del', 'data': {'id': 'str'} }
> >> +
> >> +##
> >> +# @savevm:
> >> +#
> >> +# Create a snapshot of the whole virtual machine. If 'tag' is provided,
> >> +# it is used as human readable identifier. If there is already a snapshot
> >> +# with the same tag or ID, it is replaced.
> >> +#
> >> +# @name: tag or id of new or existing snapshot
> > Needs an #optional designation, given the syntax below.
> >
> >> +#
> >> +# Returns: Nothing on success
> >> +#  If there is a writable device not supporting snapshots,
> >> +#SnapshotNotSupported
> >> +#  If no block device can accept snapshots, SnapshotNotAccepted
> >> +#  If an error occures while creating a snapshot, 
> >> SnapshotCreateFailed
> > s/occures/occurs/
> >
> >> +#  If open a block device for vm state fail, SnapshotOpenFailed
> >> +#  If an error uccures while writing vm state, SnapshotWriteFailed
> > s/uccures/occurs/
> >
> >> +#  If delete snapshot with same 'name' fail, SnapshotDeleteFailed
> > The notion of blindly overwriting the existing snapshot of the same name
> > seems a bit dangerous; should we take this opportunity to enhance the
> > command, and add a force flag, where things fail if the flag is false
> > but the name already exists, and where the reuse only happens if the
> > flag is present?  (In fact, it would make my life in libvirt easier, as
> > I have an action item to make libvirt reject attempts to create a
> > snapshot with tag named '1' if an existing snapshot already has an id of
> > '1'.)
> This sounds reasonable and I think that this could be also good for HMP, 
> not only for QMP.
> If there isn't anyone who disagree, I'll implement it.

I agree with it.

> 
> Pavel
> >> +#
> >> +# Since: 1.2
> >> +##
> >> +{ 'command': 'savevm', 'data': {'*name': 'str'} }
> >> \ No newline at end of file
> > Fix that.
> >
> >> @@ -1061,6 +1061,32 @@ Example:
> >>   
> >>   EQMP
> >>   {
> >> +.name   = "savevm",
> >> +.args_type  = "name:s?",
> >> +.params = "name",
> >> +.help   = "save a VM snapshot. If no tag or id are provided, 
> >> a new snapshot is created",
> >> +.mhandler.cmd_new = qmp_marshal_input_savevm
> >> +},
> >> +
> >> +SQMP
> >> +savevm
> > I know the HMP command is short, for ease of typing; but since 'savevm'
> > is not an English word, should we name the QMP command 'save-vm'?
> >
> 
> 
> 

Re: [Qemu-devel] [RFC PATCH 1/4] qapi: Convert savevm

[Luiz Capitulino]

On Thu, 12 Jul 2012 18:55:15 +0200
Pavel Hrdina  wrote:

> Signed-off-by: Pavel Hrdina 
> ---
>  hmp-commands.hx  |2 +-
>  hmp.c|   10 ++
>  hmp.h|1 +
>  qapi-schema.json |   22 ++
>  qerror.c |   24 
>  qerror.h |   18 ++
>  qmp-commands.hx  |   26 ++
>  savevm.c |   29 ++---
>  sysemu.h |1 -
>  9 files changed, 116 insertions(+), 17 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index f5d9d91..e8c5325 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -267,7 +267,7 @@ ETEXI
>  .args_type  = "name:s?",
>  .params = "[tag|id]",
>  .help   = "save a VM snapshot. If no tag or id are provided, a 
> new snapshot is created",
> -.mhandler.cmd = do_savevm,
> +.mhandler.cmd = hmp_savevm,
>  },
>  
>  STEXI
> diff --git a/hmp.c b/hmp.c
> index 4c6d4ae..491599d 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1002,3 +1002,13 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
>  qmp_netdev_del(id, &err);
>  hmp_handle_error(mon, &err);
>  }
> +
> +void hmp_savevm(Monitor *mon, const QDict *qdict)
> +{
> +const char *name = qdict_get_try_str(qdict, "name");
> +Error *err = NULL;
> +
> +qmp_savevm(!!name, name, &err);
> +
> +hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 79d138d..dc6410b 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -64,5 +64,6 @@ void hmp_device_del(Monitor *mon, const QDict *qdict);
>  void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
>  void hmp_netdev_add(Monitor *mon, const QDict *qdict);
>  void hmp_netdev_del(Monitor *mon, const QDict *qdict);
> +void hmp_savevm(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 1ab5dbd..4db1447 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1868,3 +1868,25 @@
>  # Since: 0.14.0
>  ##
>  { 'command': 'netdev_del', 'data': {'id': 'str'} }
> +
> +##
> +# @savevm:

Let's call it save-vm.

> +#
> +# Create a snapshot of the whole virtual machine. If 'tag' is provided,
> +# it is used as human readable identifier. If there is already a snapshot
> +# with the same tag or ID, it is replaced.

Please, document that the vm is automatically stopped and resumed, and that
this can take a long time.

> +#
> +# @name: tag or id of new or existing snapshot

I don't like the idea of making 'name' optional in QMP. We could (and should)
have it as optional in HMP though. This means that we should move the code
that auto-generates it to HMP.

Also, I remember an old discussion about distinguishing between 'tag' and 'id'.
I remember it was difficult to do, so we could choose not to do it, but let's
re-evaluate this again.

> +#
> +# Returns: Nothing on success
> +#  If there is a writable device not supporting snapshots,
> +#SnapshotNotSupported
> +#  If no block device can accept snapshots, SnapshotNotAccepted
> +#  If an error occures while creating a snapshot, 
> SnapshotCreateFailed
> +#  If open a block device for vm state fail, SnapshotOpenFailed
> +#  If an error uccures while writing vm state, SnapshotWriteFailed
> +#  If delete snapshot with same 'name' fail, SnapshotDeleteFailed

I don't think we should re-create all these errors, more comments on that
below.

> +#
> +# Since: 1.2
> +##
> +{ 'command': 'savevm', 'data': {'*name': 'str'} }
> \ No newline at end of file
> diff --git a/qerror.c b/qerror.c
> index 92c4eff..4e6efa1 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -309,6 +309,30 @@ static const QErrorStringTable qerror_table[] = {
>  .desc  = "Could not start VNC server on %(target)",
>  },
>  {
> +.error_fmt = QERR_SNAPSHOT_CREATE_FAILED,
> +.desc  = "Error '%(errno)' while creating snapshot on 
> '%(device)'",
> +},
> +{
> +.error_fmt = QERR_SNAPSHOT_DELETE_FAILED,
> +.desc  = "Error '%(errno)' while deleting snapshot on 
> '%(device)'",
> +},
> +{
> +.error_fmt = QERR_SNAPSHOT_NOT_ACCEPTED,
> +.desc  = "No block device can accept snapshots",
> +},
> +{
> +.error_fmt = QERR_SNAPSHOT_NOT_SUPPORTED,
> +.desc  = "Device '%(device)' is writable but does not support 
> snapshots",
> +},
> +{
> +.error_fmt = QERR_SNAPSHOT_OPEN_FAILED,
> +.desc  = "Error while opening snapshot on '%(device)'",
> +},
> +{
> +.error_fmt = QERR_SNAPSHOT_WRITE_FAILED,
> +.desc  = "Error '%(errno)' while writing snapshot on 
> '%(device)'",
> +},
> +{
>  .error_fmt = QERR_SOCKET_CONNECT_IN_PROGRESS,
>  .desc  = "Connection can not be completed immediately",
>  },
> diff --git a/qerror.h b/qerror.h
> index b4c8758..bc47f4a 100644
> --- a/qer

Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions

[Eduardo Habkost]

On Mon, Jul 23, 2012 at 07:44:44PM +, Blue Swirl wrote:
> On Mon, Jul 23, 2012 at 7:28 PM, Eduardo Habkost  wrote:
> > On Mon, Jul 23, 2012 at 07:11:11PM +, Blue Swirl wrote:
> >> On Mon, Jul 23, 2012 at 6:59 PM, Eduardo Habkost  
> >> wrote:
> >> > On Mon, Jul 23, 2012 at 04:49:07PM +, Blue Swirl wrote:
> >> >> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost  
> >> >> wrote:
> >> >> > On Sat, Jul 14, 2012 at 09:14:30AM +, Blue Swirl wrote:
> >> >> > [...]
> >> >> >> >> > diff --git a/tests/Makefile b/tests/Makefile
> >> >> >> >> > index b605e14..89bd890 100644
> >> >> >> >> > --- a/tests/Makefile
> >> >> >> >> > +++ b/tests/Makefile
> >> >> >> >> > @@ -15,6 +15,7 @@ check-unit-y += 
> >> >> >> >> > tests/test-string-output-visitor$(EXESUF)
> >> >> >> >> >  check-unit-y += tests/test-coroutine$(EXESUF)
> >> >> >> >> >  check-unit-y += tests/test-visitor-serialization$(EXESUF)
> >> >> >> >> >  check-unit-y += tests/test-iov$(EXESUF)
> >> >> >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF)
> >> >> >> >>
> >> >> >> >> This probably tries to build the cpuid test also for non-x86 
> >> >> >> >> targets
> >> >> >> >> and break them all.
> >> >> >> >
> >> >> >> > I don't think there's any concept of "targets" for the check-unit 
> >> >> >> > tests.
> >> >> >>
> >> >> >> How about:
> >> >> >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF)
> >> >> >
> >> >> > test-x86-cpuid is not a qtest test case.
> >> >>
> >> >> Why not? I don't think it is a unit test either, judging from what the
> >> >> other unit tests do.
> >> >
> >> > It is absolutely a unit test. I don't know why you don't think so. It
> >> > simply checks the results of the APIC ID calculation functions.
> >>
> >> Yes, but the other 'unit tests' (the names used here are very
> >> confusing, btw) check supporting functions like coroutines, not
> >> hardware. Hardware tests (qtest) need to bind to an architecture, in
> >> this case x86.
> >
> > test-x86-cpuid doesn't check hardware, either. It just checks the simple
> > functions that calculate APIC IDs (to make sure the math is correct).
> > Those functions aren't even used by any hardware emulation code until
> > later in the patch series (when the CPU initialization code gets changed
> > to use the new function).
> 
> By that time the function is clearly x86 HW and the check is an x86
> hardware check. QEMU as whole consists of simple functions that
> calculate something.

It's useful onily for a specific architecture, yes, but it surely
doesn't make libqtest necessary.

That's the difference between the unit tests and qtest test cases: unit
tests simply test small parts of the code (that may or may not be
hardware-specific), and qtest tests hardware emulation after starting up
a whole qemu process. It's a different kind of testing, with different
sets of goals.

I suppose you are not arguing that no function inside target-* would be
ever allowed to have a unit test written for it. That would be a very
silly constraint for people writing tests.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 06/16] net: Convert qdev_prop_vlan to peer with hub

[Laszlo Ersek]

On 07/20/12 14:01, Stefan Hajnoczi wrote:

> @@ -638,11 +642,17 @@ static void get_vlan(Object *obj, Visitor *v, void 
> *opaque,
>  {
>  DeviceState *dev = DEVICE(obj);
>  Property *prop = opaque;
> -VLANState **ptr = qdev_get_prop_ptr(dev, prop);
> -int64_t id;
> +VLANClientState **ptr = qdev_get_prop_ptr(dev, prop);
> +int64_t id = -1;
>  
> -id = *ptr ? (*ptr)->id : -1;
> -visit_type_int64(v, &id, name, errp);
> +if (*ptr) {
> +unsigned int hub_id;
> +if (!net_hub_id_for_client(*ptr, &hub_id)) {
> +id = (int64_t)hub_id;
> +}
> +}
> +
> +visit_type_int(v, &id, name, errp);
>  }

Should we use uint32 here? (No particular reason, just for "cleanliness"
or whatever.)

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH 4/8] qemu-option: add alias support

[Luiz Capitulino]

On Mon, 23 Jul 2012 20:33:52 +0200
Markus Armbruster  wrote:

> Luiz Capitulino  writes:
> 
> > On Sat, 21 Jul 2012 10:11:39 +0200
> > Markus Armbruster  wrote:
> >
> >> Luiz Capitulino  writes:
> >> 
> >> > Allow for specifying an alias for each option name, see next commits
> >> > for examples.
> >> >
> >> > Signed-off-by: Luiz Capitulino 
> >> > ---
> >> >  qemu-option.c | 5 +++--
> >> >  qemu-option.h | 1 +
> >> >  2 files changed, 4 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/qemu-option.c b/qemu-option.c
> >> > index 65ba1cf..b2f9e21 100644
> >> > --- a/qemu-option.c
> >> > +++ b/qemu-option.c
> >> > @@ -623,7 +623,8 @@ static const QemuOptDesc *find_desc_by_name(const 
> >> > QemuOptDesc *desc,
> >> >  int i;
> >> >  
> >> >  for (i = 0; desc[i].name != NULL; i++) {
> >> > -if (strcmp(desc[i].name, name) == 0) {
> >> > +if (strcmp(desc[i].name, name) == 0 ||
> >> > +(desc[i].alias && strcmp(desc[i].alias, name) == 0)) {
> >> >  return &desc[i];
> >> >  }
> >> >  }
> >> > @@ -645,7 +646,7 @@ static void opt_set(QemuOpts *opts, const char 
> >> > *name, const char *value,
> 
>   static void opt_set(QemuOpts *opts, const char *name, const 
>   bool prepend, Error **errp)
>   {
>   QemuOpt *opt;
>   const QemuOptDesc *desc;
>   Error *local_err = NULL;
> 
>   desc = find_desc_by_name(opts->list->desc, name);
>   if (!desc && !opts_accepts_any(opts)) {
>   error_set(errp, QERR_INVALID_PARAMETER, name);
>   return;
> >> >  }
> >> >  
> >> >  opt = g_malloc0(sizeof(*opt));
> >> > -opt->name = g_strdup(name);
> >> > +opt->name = g_strdup(desc ? desc->name : name);
> >> >  opt->opts = opts;
> >> >  if (prepend) {
> >> >  QTAILQ_INSERT_HEAD(&opts->head, opt, next);
> >> 
> >> Are you sure this hunk belongs to this patch?  If yes, please explain
> >> why :)
> >
> > Yes, I think it's fine because the change that makes it necessary to choose
> > between desc->name and name is introduced by the hunk above.
> >
> 
> I think I now get why you have this hunk:
> 
> We reach it only if the parameter with this name exists (desc non-null),
> or opts accepts anthing (opts_accepts_any(opts).
> 
> If it exists, name equals desc->name before your patch.  But afterwards,
> it could be either desc->name or desc->alias.  You normalize to
> desc->name.
> 
> Else, all we can do is stick to name.
> 
> Correct?

Yes.

> If yes, then "normal" options and "accept any" options behave
> differently: the former set opts->name to the canonical name, even when
> the user uses an alias.  The latter set opts->name to whatever the user
> uses.  I got a bad feeling about that.

Why? Or, more importantly, how do you think we should do it?

For 'normal' options, the alias is just a different name to refer to the
option name. At least in theory, it shouldn't matter that the option was
set through the alias.

For 'accept any' options, it's up to the code handling the options know
what to do with it. It can also support aliases if it wants to, or just
refuse it.

Re: [Qemu-devel] [PATCH 1/8] qemu-option: qemu_opt_set_bool(): fix code duplication

[Luiz Capitulino]

On Mon, 23 Jul 2012 20:14:31 +0200
Markus Armbruster  wrote:

> Luiz Capitulino  writes:
> 
> > On Sat, 21 Jul 2012 10:09:09 +0200
> > Markus Armbruster  wrote:
> >
> >> Luiz Capitulino  writes:
> >> 
> >> > Call qemu_opt_set() instead of duplicating opt_set().
> >> >
> >> > Signed-off-by: Luiz Capitulino 
> >> > ---
> >> >  qemu-option.c | 28 +---
> >> >  1 file changed, 1 insertion(+), 27 deletions(-)
> >> >
> >> > diff --git a/qemu-option.c b/qemu-option.c
> >> > index bb3886c..2cb2835 100644
> >> > --- a/qemu-option.c
> >> > +++ b/qemu-option.c
> >> > @@ -677,33 +677,7 @@ void qemu_opt_set_err(QemuOpts *opts, const char
> >> > *name, const char *value,
> >> >  
> >> >  int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
> >> >  {
> >> > -QemuOpt *opt;
> >> > -const QemuOptDesc *desc = opts->list->desc;
> >> > -int i;
> >> > -
> >> > -for (i = 0; desc[i].name != NULL; i++) {
> >> > -if (strcmp(desc[i].name, name) == 0) {
> >> > -break;
> >> > -}
> >> > -}
> >> > -if (desc[i].name == NULL) {
> >> > -if (i == 0) {
> >> > -/* empty list -> allow any */;
> >> > -} else {
> >> > -qerror_report(QERR_INVALID_PARAMETER, name);
> >> > -return -1;
> >> > -}
> >> > -}
> >> > -
> >> > -opt = g_malloc0(sizeof(*opt));
> >> > -opt->name = g_strdup(name);
> >> > -opt->opts = opts;
> >> > -QTAILQ_INSERT_TAIL(&opts->head, opt, next);
> >> > -if (desc[i].name != NULL) {
> >> > -opt->desc = desc+i;
> >> > -}
> >> > -opt->value.boolean = !!val;
> >> > -return 0;
> >> > +return qemu_opt_set(opts, name, val ? "on" : "off");
> >> >  }
> >> >  
> >> >  int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void 
> >> > *opaque,
> >> 
> >> Does a bit more than just obvious code de-duplication.  Two things in
> >> particular:
> >> 
> >> * Error reporting
> >> 
> >>   Old: qerror_report(); return -1
> >> 
> >>   New: opt_set() and qemu_opt_set() cooperate, like this:
> >>opt_set(): error_set(); return;
> >>qemu_opt_set():
> >> if (error_is_set(&local_err)) {
> >> qerror_report_err(local_err);
> >> error_free(local_err);
> >> return -1;
> >> }
> >> 
> >>   I guess the net effect is the same.  Not sure it's worth mentioning in
> >>   the commit message.
> >
> > The end result of calling qemu_opt_set_bool() is the same. The difference
> > between qerror_report() and qerror_report_err() is that the former gets 
> > error
> > information from the error call, while the latter gets error information
> > from the Error object. But they do the same thing.
> >
> >> * New sets opt->str to either "on" or "off" depending on val, then lets
> >>   reconstructs the value with qemu_opt_parse().  Which can't fail then.
> >>   I figure the latter part has no further impact.  But what about
> >>   setting opt->str?  Is this a bug fix?
> >
> > I don't remember if opt->str is read after the QemuOpt object is built. If
> > it's, then yes, this is a bug fix. Otherwise it just make the final
> > QemuOpt object more 'conforming'.
> 
> Uses of opt->str, and what happens when it isn't set:
> 
> * qemu_opt_get(): returns NULL, which means "not set".  Bug can bite
>   when value isn't the default value.
> 
> * qemu_opt_parse(): passes NULL to parse_option_bool(), which treats it
>   like "on".  Wrong if the value is actually false.  Bug can bite when
>   qemu_opts_validate() runs after qemu_opt_set_bool().
> 
> * qemu_opt_del(): passes NULL to g_free(), which is just fine.
> 
> * qemu_opt_foreach(): passes NULL to the callback, which is unlikely to
>   be prepared for it.
> 
> * qemu_opts_print(): prints NULL, which crashes on some systems.
> 
> * qemu_opts_to_qdict(): passes NULL to qstring_from_str(), which
>   crashes.

Thanks for the clarification.

> 
> Right now, the only use of qemu_opt_set_bool() is the one in vl.c.
> Can't see how to break it, but I didn't look hard.
> 
> I recommend to document the bug fix in the commit message.

Ok, will do.

Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions

[Blue Swirl]

On Mon, Jul 23, 2012 at 7:28 PM, Eduardo Habkost  wrote:
> On Mon, Jul 23, 2012 at 07:11:11PM +, Blue Swirl wrote:
>> On Mon, Jul 23, 2012 at 6:59 PM, Eduardo Habkost  wrote:
>> > On Mon, Jul 23, 2012 at 04:49:07PM +, Blue Swirl wrote:
>> >> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost  
>> >> wrote:
>> >> > On Sat, Jul 14, 2012 at 09:14:30AM +, Blue Swirl wrote:
>> >> > [...]
>> >> >> >> > diff --git a/tests/Makefile b/tests/Makefile
>> >> >> >> > index b605e14..89bd890 100644
>> >> >> >> > --- a/tests/Makefile
>> >> >> >> > +++ b/tests/Makefile
>> >> >> >> > @@ -15,6 +15,7 @@ check-unit-y += 
>> >> >> >> > tests/test-string-output-visitor$(EXESUF)
>> >> >> >> >  check-unit-y += tests/test-coroutine$(EXESUF)
>> >> >> >> >  check-unit-y += tests/test-visitor-serialization$(EXESUF)
>> >> >> >> >  check-unit-y += tests/test-iov$(EXESUF)
>> >> >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF)
>> >> >> >>
>> >> >> >> This probably tries to build the cpuid test also for non-x86 targets
>> >> >> >> and break them all.
>> >> >> >
>> >> >> > I don't think there's any concept of "targets" for the check-unit 
>> >> >> > tests.
>> >> >>
>> >> >> How about:
>> >> >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF)
>> >> >
>> >> > test-x86-cpuid is not a qtest test case.
>> >>
>> >> Why not? I don't think it is a unit test either, judging from what the
>> >> other unit tests do.
>> >
>> > It is absolutely a unit test. I don't know why you don't think so. It
>> > simply checks the results of the APIC ID calculation functions.
>>
>> Yes, but the other 'unit tests' (the names used here are very
>> confusing, btw) check supporting functions like coroutines, not
>> hardware. Hardware tests (qtest) need to bind to an architecture, in
>> this case x86.
>
> test-x86-cpuid doesn't check hardware, either. It just checks the simple
> functions that calculate APIC IDs (to make sure the math is correct).
> Those functions aren't even used by any hardware emulation code until
> later in the patch series (when the CPU initialization code gets changed
> to use the new function).

By that time the function is clearly x86 HW and the check is an x86
hardware check. QEMU as whole consists of simple functions that
calculate something.


>
> --
> Eduardo

Re: [Qemu-devel] SeaBIOS, FW_CFG_NUMA, and FW_CFG_MAX_CPUS

[Eduardo Habkost]

On Mon, Jul 23, 2012 at 07:25:01PM +, Blue Swirl wrote:
> On Mon, Jul 23, 2012 at 7:09 PM, Eduardo Habkost  wrote:
> > On Mon, Jul 23, 2012 at 06:40:51PM +, Blue Swirl wrote:
> >> On Fri, Jul 20, 2012 at 8:00 PM, Eduardo Habkost  
> >> wrote:
> >> > Hi,
> >> >
> >> > While working at the CPU index vs APIC ID changes, I stumbled upon
> >> > another not-very-well-defined interface between SeaBIOS and QEMU, and I
> >> > would like to clarify the semantics and constraints of some FW_CFG
> >> > entries.
> >> >
> >> > First, the facts/assumptions:
> >> >
> >> > - There's no concept of "CPU index" or "CPU identifier" that SeaBIOS and
> >> >   QEMU agree upon, except for the APIC ID. All SeaBIOS can really see
> >> >   are the CPU APIC IDs, on boot or on CPU hotplug.
> >> > - The APIC ID is already a perfectly good CPU identifier, that is
> >> >   present on bare metal hardware too.
> >> >   - Adding a new kind of "CPU identifier" in addition to the APIC ID
> >> > would just make things more complex.
> >> >   - The only problem with APIC IDs is that they may not be contiguous.
> >> >
> >> > That said, I would like to clarify the meaning of:
> >> >
> >> > - FW_CFG_MAX_CPUS
> >> >
> >> > What are the basic semantics and expectations about FW_CFG_MAX_CPUS?
> >>
> >> FYI: This originates from Sparc and PPC, it says how many SMP CPUs
> >> there are in the system. There we don't have (at least now) any CPU
> >> IDs and of course no APIC.
> >
> > Aren't you describing FW_CFG_NB_CPUS? If not, what's the difference
> > between FS_CFG_NB_CPUS and FW_CFG_MAX_CPUS on those architectures?
> 
> Yes, sorry. There's no difference.
> 
> >
> > Until now, the only purpose I see for max_cpus/FW_CFG_MAX_CPUS is to
> > allow CPU hotplug. I don't know if there are other use cases where
> > max_cpus/FW_CFG_MAX_CPUS is useful.
> >
> >
> >>
> >> But I have no idea what x86 should use. As a general rule, what would
> >> happen on a real machine should be emulated, but QEMU can also assist
> >> BIOS (for example to skip some complex HW probes).
> >
> > Right now I am divided between two approaches:
> >
> > - In case FW_CFG_MAX_CPUS' only purpose is to allow CPU hotplug, make it
> >   really mean "upper limit to APIC ID values" in x86;
> > - Otherwise, I am inclined to add a FW_CFG_MAX_APIC_ID entry to x86, so
> >   the firmware can (optionally) choose appropriate sizes for its
> >   internal APIC-ID-based data structures.
> 
> One integer does not tell very much.

It tells a lot: it contains exactly 32 perfectly good bits of
information.  ;-)

Jokes aside: one integer like that would be very helpful to SeaBIOS: by
knowing what's the maximum APIC ID value it would ever see, it can build
ACPI tables and support CPU hotplug without having to maintain and
update Processor ID => APIC ID tables inside the ACPI ASL code (that's
very hard to debug).

> 
> >
> >>
> >> > Considering that the APIC IDs may not be contiguous, is it supposed to
> >> > be:
> >> >
> >> > a) the maximum number of CPUs that will be ever online, doesn't matter
> >> >their APIC IDs, or
> >> > b) a value so that every CPU has APIC ID < MAX_CPUS.
> >> >
> >> > A practical example: suppose we have a machine with 18 CPUs with the
> >> > following APIC IDs: 0x00, 0x01, 0x02, 0x04, 0x05, 0x06, 0x08, 0x09,
> >> > 0x0a, 0x10, 0x11, 0x12, 0x14, 0x15, 0x16, 0x18, 0x19, 0x1a.
> >> >
> >> > (That's the expected result for a machine with 2 sockets, 3 cores per
> >> > socket, 3 threads per core.)
> >> >
> >> > In that case, should FW_CFG_MAX_CPUS be: a) 18, or b) 27 (0x1b)?
> >> >
> >> > If it should be 18, it will require additional work on SeaBIOS to make:
> >> > - CPU hotplug work
> >> > - SRAT/MADT/SSDT tables be built with Processor ID != APIC ID
> >> > - SRAT/MADT/SSDT tables be kept stable if the system is hibernated and
> >> >   resumed after a CPU is hot-plugged.
> >> >
> >> > (Probably in that case I would suggest introducing a FW_CFG_MAX_APIC_ID
> >> > entry, so that SeaBIOS can still build the ACPI tables more easily).
> >> >
> >> >
> >> > - FW_CFG_NUMA
> >> >
> >> > The first problem with FW_CFG_NUMA is that it depends on FW_CFG_MAX_CPUS
> >> > (so it inherits the same questions above). The second is that
> >> > FW_CFG_NUMA is a CPU-based table, but there's nothing SeaBIOS can use to
> >> > know what CPUs FW_CFG_NUMA is refering to, except for the APIC IDs. So,
> >> > should FW_CFG_NUMA be indexed by APIC IDs?
> >> >
> >> >
> >> > - My proposal:
> >> >
> >> > My proposal is to try to keep things simple, and just use the following
> >> > rule:
> >> >
> >> >  - Never have a CPU with APIC ID > FW_CFG_MAX_CPUS.
> >> >
> >> > This way:
> >> > - The SeaBIOS ACPI code can be kept simple.
> >> > - The current CPU hotplug interface can work as-is (up to 256 CPUs),
> >> >   based on APIC IDs.
> >> > - The current FW_CFG_NUMA interface can work as-is, indexed by APIC IDs.
> >> > - The ACPI tables can be easily kept stable between hibernate and
> >> >   resume, after CPU hot

Re: [Qemu-devel] [PATCH 05/16] net: Drop vlan argument to qemu_new_net_client()

[Laszlo Ersek]

On 07/20/12 14:01, Stefan Hajnoczi wrote:
> Since hubs are now used to implement the 'vlan' feature and the vlan
> argument is always NULL, remove the argument entirely and update all net
> clients that use qemu_new_net_client().

> @@ -249,7 +242,7 @@ NICState *qemu_new_nic(NetClientInfo *info,
>  assert(info->type == NET_CLIENT_OPTIONS_KIND_NIC);
>  assert(info->size >= sizeof(NICState));
>  
> -nc = qemu_new_net_client(info, conf->vlan, conf->peer, model, name);
> +nc = qemu_new_net_client(info, conf->peer, model, name);

The vlan argument depends on the caller here. I found the following
three callers that (may) set a non-NULL conf->vlan (at this point in the
series):

dp83932_init()
mcf_fec_init()
net_init() [hw/xen_nic.c]

However patch 8 ("net: Remove VLANState") fixes up the first two, and
patch 7 ("net: Remove vlan code from net.c") the last one.

Reviewed-by: Laszlo Ersek 

Re: [Qemu-devel] [PATCH 21/22] Add XBZRLE statistics

[Luiz Capitulino]

On Fri, 13 Jul 2012 09:23:43 +0200
Juan Quintela  wrote:

> From: Orit Wasserman 
> 
> Signed-off-by: Benoit Hudzia 
> Signed-off-by: Petter Svard 
> Signed-off-by: Aidan Shribman 
> Signed-off-by: Orit Wasserman 
> Signed-off-by: Juan Quintela 
> ---
>  arch_init.c  |   66 
> ++
>  hmp.c|   13 +++
>  migration.c  |   49 
>  migration.h  |9 
>  qapi-schema.json |   37 +-
>  qmp-commands.hx  |   35 -
>  6 files changed, 203 insertions(+), 6 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index d972d84..ab3fb2c 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -202,6 +202,64 @@ int64_t xbzrle_cache_resize(int64_t new_size)
>  return pow2floor(new_size);
>  }
> 
> +/* accounting for migration statistics */
> +typedef struct AccountingInfo {
> +uint64_t dup_pages;
> +uint64_t norm_pages;
> +uint64_t xbzrle_bytes;
> +uint64_t xbzrle_pages;
> +uint64_t xbzrle_cache_miss;
> +uint64_t iterations;
> +uint64_t xbzrle_overflows;
> +} AccountingInfo;
> +
> +static AccountingInfo acct_info;
> +
> +static void acct_clear(void)
> +{
> +memset(&acct_info, 0, sizeof(acct_info));
> +}
> +
> +uint64_t dup_mig_bytes_transferred(void)
> +{
> +return acct_info.dup_pages * TARGET_PAGE_SIZE;
> +}
> +
> +uint64_t dup_mig_pages_transferred(void)
> +{
> +return acct_info.dup_pages;
> +}
> +
> +uint64_t norm_mig_bytes_transferred(void)
> +{
> +return acct_info.norm_pages * TARGET_PAGE_SIZE;
> +}
> +
> +uint64_t norm_mig_pages_transferred(void)
> +{
> +return acct_info.norm_pages;
> +}
> +
> +uint64_t xbzrle_mig_bytes_transferred(void)
> +{
> +return acct_info.xbzrle_bytes;
> +}
> +
> +uint64_t xbzrle_mig_pages_transferred(void)
> +{
> +return acct_info.xbzrle_pages;
> +}
> +
> +uint64_t xbzrle_mig_pages_cache_miss(void)
> +{
> +return acct_info.xbzrle_cache_miss;
> +}
> +
> +uint64_t xbzrle_mig_pages_overflow(void)
> +{
> +return acct_info.xbzrle_overflows;
> +}
> +
>  static void save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
>  int cont, int flag)
>  {
> @@ -230,6 +288,7 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t 
> *current_data,
>  if (!cache_is_cached(XBZRLE.cache, current_addr)) {
>  cache_insert(XBZRLE.cache, current_addr,
>   g_memdup(current_data, TARGET_PAGE_SIZE));
> +acct_info.xbzrle_cache_miss++;
>  return -1;
>  }
> 
> @@ -244,6 +303,7 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t 
> *current_data,
>  return 0;
>  } else if (encoded_len == -1) {
>  DPRINTF("Overflow\n");
> +acct_info.xbzrle_overflows++;
>  /* update data in the cache */
>  memcpy(prev_cached_page, current_data, TARGET_PAGE_SIZE);
>  return -1;
> @@ -263,7 +323,9 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t 
> *current_data,
>  qemu_put_byte(f, hdr.xh_flags);
>  qemu_put_be16(f, hdr.xh_len);
>  qemu_put_buffer(f, XBZRLE.encoded_buf, encoded_len);
> +acct_info.xbzrle_pages++;
>  bytes_sent = encoded_len + sizeof(hdr);
> +acct_info.xbzrle_bytes += bytes_sent;
> 
>  return bytes_sent;
>  }
> @@ -303,6 +365,7 @@ static int ram_save_block(QEMUFile *f)
>  p = memory_region_get_ram_ptr(mr) + offset;
> 
>  if (is_dup_page(p)) {
> +acct_info.dup_pages++;
>  save_block_hdr(f, block, offset, cont, 
> RAM_SAVE_FLAG_COMPRESS);
>  qemu_put_byte(f, *p);
>  bytes_sent = 1;
> @@ -318,6 +381,7 @@ static int ram_save_block(QEMUFile *f)
>  save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_PAGE);
>  qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
>  bytes_sent = TARGET_PAGE_SIZE;
> +acct_info.norm_pages++;
>  }
> 
>  /* if page is unmodified, continue to the next */
> @@ -437,6 +501,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>  return -1;
>  }
>  XBZRLE.encoded_buf = g_malloc0(TARGET_PAGE_SIZE);
> +acct_clear();
>  }
> 
>  QLIST_FOREACH(block, &ram_list.blocks, next) {
> @@ -484,6 +549,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>  break;
>  }
>  bytes_transferred += bytes_sent;
> +acct_info.iterations++;
>  /* we want to check in the 1st loop, just in case it was the 1st time
> and we had to sync the dirty bitmap.
> qemu_get_clock_ns() is a bit expensive, so we only check each some
> diff --git a/hmp.c b/hmp.c
> index 99ad00a..0d7333b 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -168,6 +168,19 @@ void hmp_info_migrate(Monitor *mon)
> info->disk->total >> 10);
>  }
> 
> +if (info->has_cache) {
> 

Re: [Qemu-devel] [PATCH] configure: fix ALSA configure test

[Peter Maydell]

On 23 July 2012 18:45, Peter Maydell  wrote:
> On 23 July 2012 18:40, Blue Swirl  wrote:
>> On Tue, Jul 17, 2012 at 7:28 PM, Peter Maydell  
>> wrote:
>>> The trouble is that the warnings and errors here don't cause the
>>> build to fail noisily; that's a big distinction IMHO.
>>> I suppose we could make compile_prog do something like:
>>>  * run the compile test
>>
>> Unfortunately that would break cross compiling.
>
> Sorry, that was slightly unclear phrasing. By "run the test"
> I meant "do the check that we can compile the test code"
> "run the binary produced".

Gah. Missing 'not', should read:
# By "run the test" I meant
# "do the check that we can compile the test code", not
# "run the binary produced".

-- PMM

Re: [Qemu-devel] [PATCH 13/22] Add migration capabilities

[Eric Blake]

On 07/23/2012 12:23 PM, Luiz Capitulino wrote:
> On Fri, 13 Jul 2012 09:23:35 +0200
> Juan Quintela  wrote:
> 
>> From: Orit Wasserman 
>>
>> Add migration capabilities that can be queried by the management.
>> The management can query the source QEMU and the destination QEMU in order to
>> verify both support some migration capability (currently only XBZRLE).
>> The management can enable a capability for the next migration by using
>> migrate_set_parameter command.
> 
> Please, split this into one command per-patch. Otherwise it's difficult to
> review.
> 
> Have libvirt folks acked this approach btw? It looks fine to me, but we need
> their ack too.

Yes, I've been reviewing versions of this series, and am okay with the
libvirt impact with the current proposed set of new QMP commands.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions

[Eduardo Habkost]

On Mon, Jul 23, 2012 at 07:11:11PM +, Blue Swirl wrote:
> On Mon, Jul 23, 2012 at 6:59 PM, Eduardo Habkost  wrote:
> > On Mon, Jul 23, 2012 at 04:49:07PM +, Blue Swirl wrote:
> >> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost  
> >> wrote:
> >> > On Sat, Jul 14, 2012 at 09:14:30AM +, Blue Swirl wrote:
> >> > [...]
> >> >> >> > diff --git a/tests/Makefile b/tests/Makefile
> >> >> >> > index b605e14..89bd890 100644
> >> >> >> > --- a/tests/Makefile
> >> >> >> > +++ b/tests/Makefile
> >> >> >> > @@ -15,6 +15,7 @@ check-unit-y += 
> >> >> >> > tests/test-string-output-visitor$(EXESUF)
> >> >> >> >  check-unit-y += tests/test-coroutine$(EXESUF)
> >> >> >> >  check-unit-y += tests/test-visitor-serialization$(EXESUF)
> >> >> >> >  check-unit-y += tests/test-iov$(EXESUF)
> >> >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF)
> >> >> >>
> >> >> >> This probably tries to build the cpuid test also for non-x86 targets
> >> >> >> and break them all.
> >> >> >
> >> >> > I don't think there's any concept of "targets" for the check-unit 
> >> >> > tests.
> >> >>
> >> >> How about:
> >> >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF)
> >> >
> >> > test-x86-cpuid is not a qtest test case.
> >>
> >> Why not? I don't think it is a unit test either, judging from what the
> >> other unit tests do.
> >
> > It is absolutely a unit test. I don't know why you don't think so. It
> > simply checks the results of the APIC ID calculation functions.
> 
> Yes, but the other 'unit tests' (the names used here are very
> confusing, btw) check supporting functions like coroutines, not
> hardware. Hardware tests (qtest) need to bind to an architecture, in
> this case x86.

test-x86-cpuid doesn't check hardware, either. It just checks the simple
functions that calculate APIC IDs (to make sure the math is correct).
Those functions aren't even used by any hardware emulation code until
later in the patch series (when the CPU initialization code gets changed
to use the new function).

-- 
Eduardo

Re: [Qemu-devel] [PATCH 20/22] Add migrate_set_cachesize command

[Luiz Capitulino]

On Fri, 13 Jul 2012 09:23:42 +0200
Juan Quintela  wrote:

> From: Orit Wasserman 
> 
> Change XBZRLE cache size in bytes (the size should be a power of 2, it will be
> rounded down to the nearest power of 2).
> If XBZRLE cache size is too small there will be many cache miss.

As far as QMP is concerned:

 Reviewed-by: Luiz Capitulino 

> 
> Signed-off-by: Benoit Hudzia 
> Signed-off-by: Petter Svard 
> Signed-off-by: Aidan Shribman 
> Signed-off-by: Orit Wasserman 
> Signed-off-by: Juan Quintela 
> ---
>  arch_init.c  |   10 ++
>  hmp-commands.hx  |   20 
>  hmp.c|   13 +
>  hmp.h|1 +
>  migration.c  |   14 ++
>  migration.h  |2 ++
>  qapi-schema.json |   16 
>  qmp-commands.hx  |   23 +++
>  8 files changed, 99 insertions(+)
> 
> diff --git a/arch_init.c b/arch_init.c
> index a3a4707..d972d84 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -192,6 +192,16 @@ static struct {
>  .cache = NULL,
>  };
> 
> +
> +int64_t xbzrle_cache_resize(int64_t new_size)
> +{
> +if (XBZRLE.cache != NULL) {
> +return cache_resize(XBZRLE.cache, new_size / TARGET_PAGE_SIZE) *
> +TARGET_PAGE_SIZE;
> +}
> +return pow2floor(new_size);
> +}
> +
>  static void save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
>  int cont, int flag)
>  {
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 9245bef..052a0a3 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -829,6 +829,26 @@ STEXI
>  @item migrate_cancel
>  @findex migrate_cancel
>  Cancel the current VM migration.
> +
> +ETEXI
> +
> +{
> +.name   = "migrate_set_cachesize",
> +.args_type  = "value:o",
> +.params = "value",
> +.help   = "set cache size (in bytes) for XBZRLE migrations,"
> +  "the cache size will be rounded down to the nearest "
> +  "power of 2.\n"
> +  "The cache size effects the number of cache misses."
> +  "In case of a high cache miss ratio you need to 
> increase"
> +  " the cache size",
> +.mhandler.cmd = hmp_migrate_set_cachesize,
> +},
> +
> +STEXI
> +@item migrate_set_cachesize @var{value}
> +@findex migrate_set_cachesize
> +Set cache size to @var{value} (in bytes) for xbzrle migrations.
>  ETEXI
> 
>  {
> diff --git a/hmp.c b/hmp.c
> index b0440e6..99ad00a 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -758,6 +758,19 @@ void hmp_migrate_set_downtime(Monitor *mon, const QDict 
> *qdict)
>  qmp_migrate_set_downtime(value, NULL);
>  }
> 
> +void hmp_migrate_set_cachesize(Monitor *mon, const QDict *qdict)
> +{
> +int64_t value = qdict_get_int(qdict, "value");
> +Error *err = NULL;
> +
> +qmp_migrate_set_cache_size(value, &err);
> +if (err) {
> +monitor_printf(mon, "%s\n", error_get_pretty(err));
> +error_free(err);
> +return;
> +}
> +}
> +
>  void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict)
>  {
>  int64_t value = qdict_get_int(qdict, "value");
> diff --git a/hmp.h b/hmp.h
> index 09ba198..7c5117d 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -53,6 +53,7 @@ void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
>  void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
>  void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict);
>  void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict);
> +void hmp_migrate_set_cachesize(Monitor *mon, const QDict *qdict);
>  void hmp_set_password(Monitor *mon, const QDict *qdict);
>  void hmp_expire_password(Monitor *mon, const QDict *qdict);
>  void hmp_eject(Monitor *mon, const QDict *qdict);
> diff --git a/migration.c b/migration.c
> index 1a264a9..d134bf6 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -533,6 +533,20 @@ void qmp_migrate_cancel(Error **errp)
>  migrate_fd_cancel(migrate_get_current());
>  }
> 
> +void qmp_migrate_set_cache_size(int64_t value, Error **errp)
> +{
> +MigrationState *s = migrate_get_current();
> +
> +/* Check for truncation */
> +if (value != (size_t)value) {
> +error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> +  "exceeding address space");
> +return;
> +}
> +
> +s->xbzrle_cache_size = xbzrle_cache_resize(value);
> +}
> +
>  void qmp_migrate_set_speed(int64_t value, Error **errp)
>  {
>  MigrationState *s;
> diff --git a/migration.h b/migration.h
> index cdf6787..337e225 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -108,4 +108,6 @@ int xbzrle_decode_buffer(uint8_t *src, int slen, uint8_t 
> *dst, int dlen);
>  int migrate_use_xbzrle(void);
>  int64_t migrate_xbzrle_cache_size(void);
> 
> +int64_t xbzrle_cache_resize(int64_t new_size);
> +
>  #endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a8408fd..a0f0f95 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.

Re: [Qemu-devel] SeaBIOS, FW_CFG_NUMA, and FW_CFG_MAX_CPUS

[Blue Swirl]

On Mon, Jul 23, 2012 at 7:09 PM, Eduardo Habkost  wrote:
> On Mon, Jul 23, 2012 at 06:40:51PM +, Blue Swirl wrote:
>> On Fri, Jul 20, 2012 at 8:00 PM, Eduardo Habkost  wrote:
>> > Hi,
>> >
>> > While working at the CPU index vs APIC ID changes, I stumbled upon
>> > another not-very-well-defined interface between SeaBIOS and QEMU, and I
>> > would like to clarify the semantics and constraints of some FW_CFG
>> > entries.
>> >
>> > First, the facts/assumptions:
>> >
>> > - There's no concept of "CPU index" or "CPU identifier" that SeaBIOS and
>> >   QEMU agree upon, except for the APIC ID. All SeaBIOS can really see
>> >   are the CPU APIC IDs, on boot or on CPU hotplug.
>> > - The APIC ID is already a perfectly good CPU identifier, that is
>> >   present on bare metal hardware too.
>> >   - Adding a new kind of "CPU identifier" in addition to the APIC ID
>> > would just make things more complex.
>> >   - The only problem with APIC IDs is that they may not be contiguous.
>> >
>> > That said, I would like to clarify the meaning of:
>> >
>> > - FW_CFG_MAX_CPUS
>> >
>> > What are the basic semantics and expectations about FW_CFG_MAX_CPUS?
>>
>> FYI: This originates from Sparc and PPC, it says how many SMP CPUs
>> there are in the system. There we don't have (at least now) any CPU
>> IDs and of course no APIC.
>
> Aren't you describing FW_CFG_NB_CPUS? If not, what's the difference
> between FS_CFG_NB_CPUS and FW_CFG_MAX_CPUS on those architectures?

Yes, sorry. There's no difference.

>
> Until now, the only purpose I see for max_cpus/FW_CFG_MAX_CPUS is to
> allow CPU hotplug. I don't know if there are other use cases where
> max_cpus/FW_CFG_MAX_CPUS is useful.
>
>
>>
>> But I have no idea what x86 should use. As a general rule, what would
>> happen on a real machine should be emulated, but QEMU can also assist
>> BIOS (for example to skip some complex HW probes).
>
> Right now I am divided between two approaches:
>
> - In case FW_CFG_MAX_CPUS' only purpose is to allow CPU hotplug, make it
>   really mean "upper limit to APIC ID values" in x86;
> - Otherwise, I am inclined to add a FW_CFG_MAX_APIC_ID entry to x86, so
>   the firmware can (optionally) choose appropriate sizes for its
>   internal APIC-ID-based data structures.

One integer does not tell very much.

>
>>
>> > Considering that the APIC IDs may not be contiguous, is it supposed to
>> > be:
>> >
>> > a) the maximum number of CPUs that will be ever online, doesn't matter
>> >their APIC IDs, or
>> > b) a value so that every CPU has APIC ID < MAX_CPUS.
>> >
>> > A practical example: suppose we have a machine with 18 CPUs with the
>> > following APIC IDs: 0x00, 0x01, 0x02, 0x04, 0x05, 0x06, 0x08, 0x09,
>> > 0x0a, 0x10, 0x11, 0x12, 0x14, 0x15, 0x16, 0x18, 0x19, 0x1a.
>> >
>> > (That's the expected result for a machine with 2 sockets, 3 cores per
>> > socket, 3 threads per core.)
>> >
>> > In that case, should FW_CFG_MAX_CPUS be: a) 18, or b) 27 (0x1b)?
>> >
>> > If it should be 18, it will require additional work on SeaBIOS to make:
>> > - CPU hotplug work
>> > - SRAT/MADT/SSDT tables be built with Processor ID != APIC ID
>> > - SRAT/MADT/SSDT tables be kept stable if the system is hibernated and
>> >   resumed after a CPU is hot-plugged.
>> >
>> > (Probably in that case I would suggest introducing a FW_CFG_MAX_APIC_ID
>> > entry, so that SeaBIOS can still build the ACPI tables more easily).
>> >
>> >
>> > - FW_CFG_NUMA
>> >
>> > The first problem with FW_CFG_NUMA is that it depends on FW_CFG_MAX_CPUS
>> > (so it inherits the same questions above). The second is that
>> > FW_CFG_NUMA is a CPU-based table, but there's nothing SeaBIOS can use to
>> > know what CPUs FW_CFG_NUMA is refering to, except for the APIC IDs. So,
>> > should FW_CFG_NUMA be indexed by APIC IDs?
>> >
>> >
>> > - My proposal:
>> >
>> > My proposal is to try to keep things simple, and just use the following
>> > rule:
>> >
>> >  - Never have a CPU with APIC ID > FW_CFG_MAX_CPUS.
>> >
>> > This way:
>> > - The SeaBIOS ACPI code can be kept simple.
>> > - The current CPU hotplug interface can work as-is (up to 256 CPUs),
>> >   based on APIC IDs.
>> > - The current FW_CFG_NUMA interface can work as-is, indexed by APIC IDs.
>> > - The ACPI tables can be easily kept stable between hibernate and
>> >   resume, after CPU hotplug.
>> >
>> > This is the direction I am trying to go, and I am sending this just to
>> > make sure nobody is against it, and to not surprise anybody when I send
>> > a QEMU patch to make FW_CFG_MAX_CPUS be based on APIC IDs.
>> >
>> >
>> > My second proposal would be to introduce a FW_CFG_MAX_APIC_ID entry, so
>> > the SeaBIOS ACPI code can be kept simple.
>> >
>> > My third proposal would be to introduce a FW_CFG CPU Index => APIC ID
>> > table, but I really wouldn't like to introduce a new type of CPU
>> > identifier to be used between QEMU and SeaBIOS, when the APIC ID is a
>> > perfectly good unique CPU identifier that already exists in bare metal
>> > h

Re: [Qemu-devel] [PATCH 04/16] hub: Check that hubs are configured correctly

[Laszlo Ersek]

On 07/20/12 14:01, Stefan Hajnoczi wrote:
> Checks can be performed to make sure that hubs have at least one NIC and
> one host device, warning the user if this is not the case.
> Configurations which do not meet this rule tend to be broken but just
> emit a warning.  This patch preserves compatibility with the checks
> performed by net core on vlans.
> 
> Signed-off-by: Stefan Hajnoczi 
> Signed-off-by: Zhi Yong Wu 

Reviewed-by: Laszlo Ersek 

Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions

[Blue Swirl]

On Mon, Jul 23, 2012 at 6:59 PM, Eduardo Habkost  wrote:
> On Mon, Jul 23, 2012 at 04:49:07PM +, Blue Swirl wrote:
>> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost  wrote:
>> > On Sat, Jul 14, 2012 at 09:14:30AM +, Blue Swirl wrote:
>> > [...]
>> >> >> > diff --git a/tests/Makefile b/tests/Makefile
>> >> >> > index b605e14..89bd890 100644
>> >> >> > --- a/tests/Makefile
>> >> >> > +++ b/tests/Makefile
>> >> >> > @@ -15,6 +15,7 @@ check-unit-y += 
>> >> >> > tests/test-string-output-visitor$(EXESUF)
>> >> >> >  check-unit-y += tests/test-coroutine$(EXESUF)
>> >> >> >  check-unit-y += tests/test-visitor-serialization$(EXESUF)
>> >> >> >  check-unit-y += tests/test-iov$(EXESUF)
>> >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF)
>> >> >>
>> >> >> This probably tries to build the cpuid test also for non-x86 targets
>> >> >> and break them all.
>> >> >
>> >> > I don't think there's any concept of "targets" for the check-unit tests.
>> >>
>> >> How about:
>> >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF)
>> >
>> > test-x86-cpuid is not a qtest test case.
>>
>> Why not? I don't think it is a unit test either, judging from what the
>> other unit tests do.
>
> It is absolutely a unit test. I don't know why you don't think so. It
> simply checks the results of the APIC ID calculation functions.

Yes, but the other 'unit tests' (the names used here are very
confusing, btw) check supporting functions like coroutines, not
hardware. Hardware tests (qtest) need to bind to an architecture, in
this case x86.

>
>
>>
>> >
>> >>
>> >> > I had to do the following, to be able to make a test that uses the
>> >> > target-i386 code:
>> >> >
>> >> >> > +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386
>> >> >
>> >> > Any suggestions to avoid this hack would be welcome.
>> >>
>> >> Maybe it would be simpler to adjust #include path in the file.
>> >
>> > Using the full path on the #include line would break in case
>> > target-i386/topology.h include other files from the target-i386
>> > directory.
>>
>> That's fragile. Maybe the target-xyz files should use the full path.
>
> Yes, it would be fragile. That's why I used -Itarget-i386 (not a perfect
> solution, but more likely to stay working and not break easily).
>
> I don't know if I understand what you propose. You mean to change the
> 256 target-specific #include lines inside qemu?
>
>  $ git grep -f <(find target-* -name '*.h' | sed -e 's@target-[^/]*/@#include 
> "@' | sort -u) target-* | wc -l
>  256

That does not look very attractive either. Scripting could help for
such mechanical changes. Maybe later.

>
> --
> Eduardo

Re: [Qemu-devel] SeaBIOS, FW_CFG_NUMA, and FW_CFG_MAX_CPUS

[Eduardo Habkost]

On Mon, Jul 23, 2012 at 06:40:51PM +, Blue Swirl wrote:
> On Fri, Jul 20, 2012 at 8:00 PM, Eduardo Habkost  wrote:
> > Hi,
> >
> > While working at the CPU index vs APIC ID changes, I stumbled upon
> > another not-very-well-defined interface between SeaBIOS and QEMU, and I
> > would like to clarify the semantics and constraints of some FW_CFG
> > entries.
> >
> > First, the facts/assumptions:
> >
> > - There's no concept of "CPU index" or "CPU identifier" that SeaBIOS and
> >   QEMU agree upon, except for the APIC ID. All SeaBIOS can really see
> >   are the CPU APIC IDs, on boot or on CPU hotplug.
> > - The APIC ID is already a perfectly good CPU identifier, that is
> >   present on bare metal hardware too.
> >   - Adding a new kind of "CPU identifier" in addition to the APIC ID
> > would just make things more complex.
> >   - The only problem with APIC IDs is that they may not be contiguous.
> >
> > That said, I would like to clarify the meaning of:
> >
> > - FW_CFG_MAX_CPUS
> >
> > What are the basic semantics and expectations about FW_CFG_MAX_CPUS?
> 
> FYI: This originates from Sparc and PPC, it says how many SMP CPUs
> there are in the system. There we don't have (at least now) any CPU
> IDs and of course no APIC.

Aren't you describing FW_CFG_NB_CPUS? If not, what's the difference
between FS_CFG_NB_CPUS and FW_CFG_MAX_CPUS on those architectures?

Until now, the only purpose I see for max_cpus/FW_CFG_MAX_CPUS is to
allow CPU hotplug. I don't know if there are other use cases where
max_cpus/FW_CFG_MAX_CPUS is useful.


> 
> But I have no idea what x86 should use. As a general rule, what would
> happen on a real machine should be emulated, but QEMU can also assist
> BIOS (for example to skip some complex HW probes).

Right now I am divided between two approaches:

- In case FW_CFG_MAX_CPUS' only purpose is to allow CPU hotplug, make it
  really mean "upper limit to APIC ID values" in x86;
- Otherwise, I am inclined to add a FW_CFG_MAX_APIC_ID entry to x86, so
  the firmware can (optionally) choose appropriate sizes for its
  internal APIC-ID-based data structures.

> 
> > Considering that the APIC IDs may not be contiguous, is it supposed to
> > be:
> >
> > a) the maximum number of CPUs that will be ever online, doesn't matter
> >their APIC IDs, or
> > b) a value so that every CPU has APIC ID < MAX_CPUS.
> >
> > A practical example: suppose we have a machine with 18 CPUs with the
> > following APIC IDs: 0x00, 0x01, 0x02, 0x04, 0x05, 0x06, 0x08, 0x09,
> > 0x0a, 0x10, 0x11, 0x12, 0x14, 0x15, 0x16, 0x18, 0x19, 0x1a.
> >
> > (That's the expected result for a machine with 2 sockets, 3 cores per
> > socket, 3 threads per core.)
> >
> > In that case, should FW_CFG_MAX_CPUS be: a) 18, or b) 27 (0x1b)?
> >
> > If it should be 18, it will require additional work on SeaBIOS to make:
> > - CPU hotplug work
> > - SRAT/MADT/SSDT tables be built with Processor ID != APIC ID
> > - SRAT/MADT/SSDT tables be kept stable if the system is hibernated and
> >   resumed after a CPU is hot-plugged.
> >
> > (Probably in that case I would suggest introducing a FW_CFG_MAX_APIC_ID
> > entry, so that SeaBIOS can still build the ACPI tables more easily).
> >
> >
> > - FW_CFG_NUMA
> >
> > The first problem with FW_CFG_NUMA is that it depends on FW_CFG_MAX_CPUS
> > (so it inherits the same questions above). The second is that
> > FW_CFG_NUMA is a CPU-based table, but there's nothing SeaBIOS can use to
> > know what CPUs FW_CFG_NUMA is refering to, except for the APIC IDs. So,
> > should FW_CFG_NUMA be indexed by APIC IDs?
> >
> >
> > - My proposal:
> >
> > My proposal is to try to keep things simple, and just use the following
> > rule:
> >
> >  - Never have a CPU with APIC ID > FW_CFG_MAX_CPUS.
> >
> > This way:
> > - The SeaBIOS ACPI code can be kept simple.
> > - The current CPU hotplug interface can work as-is (up to 256 CPUs),
> >   based on APIC IDs.
> > - The current FW_CFG_NUMA interface can work as-is, indexed by APIC IDs.
> > - The ACPI tables can be easily kept stable between hibernate and
> >   resume, after CPU hotplug.
> >
> > This is the direction I am trying to go, and I am sending this just to
> > make sure nobody is against it, and to not surprise anybody when I send
> > a QEMU patch to make FW_CFG_MAX_CPUS be based on APIC IDs.
> >
> >
> > My second proposal would be to introduce a FW_CFG_MAX_APIC_ID entry, so
> > the SeaBIOS ACPI code can be kept simple.
> >
> > My third proposal would be to introduce a FW_CFG CPU Index => APIC ID
> > table, but I really wouldn't like to introduce a new type of CPU
> > identifier to be used between QEMU and SeaBIOS, when the APIC ID is a
> > perfectly good unique CPU identifier that already exists in bare metal
> > hardware.
> >
> > --
> > Eduardo
> >
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 01/16] net: Add a hub net client

[Blue Swirl]

On Mon, Jul 23, 2012 at 4:33 PM, Andreas Färber  wrote:
> Am 23.07.2012 17:23, schrieb Stefan Hajnoczi:
>> On Mon, Jul 23, 2012 at 4:16 PM, Laszlo Ersek  wrote:
>>> On 07/23/12 16:52, Stefan Hajnoczi wrote:
 On Mon, Jul 23, 2012 at 3:05 PM, Laszlo Ersek  wrote:
>>>
> The idea is, rather than
>
> unsigned int id = hub->num_ports++;
>
> it should say
>
> unsigned int id;
> /* ... */
> id = hub->num_ports++;

 "Be careful to not obfuscate the code by initializing variables in the
 declarations.  Use this feature only thoughtfully.  DO NOT use
 function calls in initializers."

 This?

 Do you know the rationale?  It's not clear to me how this "obfuscates" the 
 code.
>>>
>>> I think the rationale is that (a) people tend to reorganize definitions,
>>> and the expressions in the initializer lists may depend on the original
>>> order, (b) even worse with removal of variables, (c) many people have a
>>> "conceptual divide" between the definition block and the logic below it,
>>> and the first should set constant defaults at most. (Naturally this
>>> prevents C99/C++ style mixing of definitions and code as well, at least
>>> without explicit braces.)
>>>
>>> I'm one of those people, but again I'm not sure if qemu has any
>>> guideline on this.
>>>
>>>
 Messing around with side-effects in a series of variable declarations
 with intializers could be bug-prone.  But here there is only one
 initializer so it's not a problem.

 Regarding QEMU, there's no coding style rule against initializers.
 Personally I think that's a good thing because it keeps the code
 concise - people reading it don't have to keep the declaration in
 their mind until they hit the initializer later on.
>>>
>>> Well I keep the definitions at the top of the block so I can very easily
>>> return to them visually (and be sure they do nothing else than define
>>> variables / declare externs), but it's getting philosophical :)
>>>
>>> I have nothing against this initializer as-is, I just wanted to voice
>>> *if* there's such a guideline in qemu *then* it should be followed :)
>>
>> Yes, I understand - it's a question of style or flamewar fodder :).  I
>> double-checked that there is no guideline in ./CODING_STYLE and
>> ./HACKING.
>
> Hm, I'm not so much into those documents [cc'ing Blue], but we used to
> be stricter about ANSI C some years back (which iirc forbids
> non-constant expressions in initializers?). FWIW we have since switched
> to C99 struct initializers and use QOM cast macros (that translate to a
> function call) in initializers. -ansi -pedantic is unlikely to get far.

For example in block/vvfat.c, various initializers and nonstandard
stuff like variable length arrays have been used since day one. It's
not the finest example of code in QEMU though.

>
> Cheers,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
>

Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions

[Eduardo Habkost]

On Mon, Jul 23, 2012 at 04:49:07PM +, Blue Swirl wrote:
> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost  wrote:
> > On Sat, Jul 14, 2012 at 09:14:30AM +, Blue Swirl wrote:
> > [...]
> >> >> > diff --git a/tests/Makefile b/tests/Makefile
> >> >> > index b605e14..89bd890 100644
> >> >> > --- a/tests/Makefile
> >> >> > +++ b/tests/Makefile
> >> >> > @@ -15,6 +15,7 @@ check-unit-y += 
> >> >> > tests/test-string-output-visitor$(EXESUF)
> >> >> >  check-unit-y += tests/test-coroutine$(EXESUF)
> >> >> >  check-unit-y += tests/test-visitor-serialization$(EXESUF)
> >> >> >  check-unit-y += tests/test-iov$(EXESUF)
> >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF)
> >> >>
> >> >> This probably tries to build the cpuid test also for non-x86 targets
> >> >> and break them all.
> >> >
> >> > I don't think there's any concept of "targets" for the check-unit tests.
> >>
> >> How about:
> >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF)
> >
> > test-x86-cpuid is not a qtest test case.
> 
> Why not? I don't think it is a unit test either, judging from what the
> other unit tests do.

It is absolutely a unit test. I don't know why you don't think so. It
simply checks the results of the APIC ID calculation functions.


> 
> >
> >>
> >> > I had to do the following, to be able to make a test that uses the
> >> > target-i386 code:
> >> >
> >> >> > +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386
> >> >
> >> > Any suggestions to avoid this hack would be welcome.
> >>
> >> Maybe it would be simpler to adjust #include path in the file.
> >
> > Using the full path on the #include line would break in case
> > target-i386/topology.h include other files from the target-i386
> > directory.
> 
> That's fragile. Maybe the target-xyz files should use the full path.

Yes, it would be fragile. That's why I used -Itarget-i386 (not a perfect
solution, but more likely to stay working and not break easily).

I don't know if I understand what you propose. You mean to change the
256 target-specific #include lines inside qemu?

 $ git grep -f <(find target-* -name '*.h' | sed -e 's@target-[^/]*/@#include 
"@' | sort -u) target-* | wc -l
 256

-- 
Eduardo

Re: [Qemu-devel] [PATCH] qdev: Fix Open Firmware comment

[Blue Swirl]

On Fri, Jul 20, 2012 at 9:04 PM, Stefan Weil  wrote:
> Commit 0d936928ef87ca1bb7b41b5b89c400c699a7691c removed code,
> but left the related comment at a location where it no longer
> belongs to.
>
> The patch moves the comment to the correct callback and improves the text.
>
> Signed-off-by: Stefan Weil 
> ---
>  hw/qdev.h |   11 +--
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 247dd1e..a2cbd9d 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -78,12 +78,6 @@ struct DeviceState {
>  int alias_required_for_version;
>  };
>
> -/*
> - * This callback is used to create Open Firmware device path in accordance 
> with
> - * OF spec http://forthworks.com/standards/of1275.pdf. Indicidual bus 
> bindings
> - * can be found here http://playground.sun.com/1275/bindings/.
> - */
> -
>  #define TYPE_BUS "bus"
>  #define BUS(obj) OBJECT_CHECK(BusState, (obj), TYPE_BUS)
>  #define BUS_CLASS(klass) OBJECT_CLASS_CHECK(BusClass, (klass), TYPE_BUS)
> @@ -95,6 +89,11 @@ struct BusClass {
>  /* FIXME first arg should be BusState */
>  void (*print_dev)(Monitor *mon, DeviceState *dev, int indent);
>  char *(*get_dev_path)(DeviceState *dev);
> +/*
> + * This callback is used to create Open Firmware device path in 
> accordance
> + * with OF spec http://forthworks.com/standards/of1275.pdf. Individual 
> bus
> + * bindings can be found at http://playground.sun.com/1275/bindings/.
> + */

The links are dead. Since you are improving the text, maybe you could
find archived versions.

>  char *(*get_fw_dev_path)(DeviceState *dev);
>  int (*reset)(BusState *bus);
>  };
> --
> 1.7.10
>
>

Re: [Qemu-devel] SeaBIOS, FW_CFG_NUMA, and FW_CFG_MAX_CPUS

[Blue Swirl]

On Fri, Jul 20, 2012 at 8:00 PM, Eduardo Habkost  wrote:
> Hi,
>
> While working at the CPU index vs APIC ID changes, I stumbled upon
> another not-very-well-defined interface between SeaBIOS and QEMU, and I
> would like to clarify the semantics and constraints of some FW_CFG
> entries.
>
> First, the facts/assumptions:
>
> - There's no concept of "CPU index" or "CPU identifier" that SeaBIOS and
>   QEMU agree upon, except for the APIC ID. All SeaBIOS can really see
>   are the CPU APIC IDs, on boot or on CPU hotplug.
> - The APIC ID is already a perfectly good CPU identifier, that is
>   present on bare metal hardware too.
>   - Adding a new kind of "CPU identifier" in addition to the APIC ID
> would just make things more complex.
>   - The only problem with APIC IDs is that they may not be contiguous.
>
> That said, I would like to clarify the meaning of:
>
> - FW_CFG_MAX_CPUS
>
> What are the basic semantics and expectations about FW_CFG_MAX_CPUS?

FYI: This originates from Sparc and PPC, it says how many SMP CPUs
there are in the system. There we don't have (at least now) any CPU
IDs and of course no APIC.

But I have no idea what x86 should use. As a general rule, what would
happen on a real machine should be emulated, but QEMU can also assist
BIOS (for example to skip some complex HW probes).

> Considering that the APIC IDs may not be contiguous, is it supposed to
> be:
>
> a) the maximum number of CPUs that will be ever online, doesn't matter
>their APIC IDs, or
> b) a value so that every CPU has APIC ID < MAX_CPUS.
>
> A practical example: suppose we have a machine with 18 CPUs with the
> following APIC IDs: 0x00, 0x01, 0x02, 0x04, 0x05, 0x06, 0x08, 0x09,
> 0x0a, 0x10, 0x11, 0x12, 0x14, 0x15, 0x16, 0x18, 0x19, 0x1a.
>
> (That's the expected result for a machine with 2 sockets, 3 cores per
> socket, 3 threads per core.)
>
> In that case, should FW_CFG_MAX_CPUS be: a) 18, or b) 27 (0x1b)?
>
> If it should be 18, it will require additional work on SeaBIOS to make:
> - CPU hotplug work
> - SRAT/MADT/SSDT tables be built with Processor ID != APIC ID
> - SRAT/MADT/SSDT tables be kept stable if the system is hibernated and
>   resumed after a CPU is hot-plugged.
>
> (Probably in that case I would suggest introducing a FW_CFG_MAX_APIC_ID
> entry, so that SeaBIOS can still build the ACPI tables more easily).
>
>
> - FW_CFG_NUMA
>
> The first problem with FW_CFG_NUMA is that it depends on FW_CFG_MAX_CPUS
> (so it inherits the same questions above). The second is that
> FW_CFG_NUMA is a CPU-based table, but there's nothing SeaBIOS can use to
> know what CPUs FW_CFG_NUMA is refering to, except for the APIC IDs. So,
> should FW_CFG_NUMA be indexed by APIC IDs?
>
>
> - My proposal:
>
> My proposal is to try to keep things simple, and just use the following
> rule:
>
>  - Never have a CPU with APIC ID > FW_CFG_MAX_CPUS.
>
> This way:
> - The SeaBIOS ACPI code can be kept simple.
> - The current CPU hotplug interface can work as-is (up to 256 CPUs),
>   based on APIC IDs.
> - The current FW_CFG_NUMA interface can work as-is, indexed by APIC IDs.
> - The ACPI tables can be easily kept stable between hibernate and
>   resume, after CPU hotplug.
>
> This is the direction I am trying to go, and I am sending this just to
> make sure nobody is against it, and to not surprise anybody when I send
> a QEMU patch to make FW_CFG_MAX_CPUS be based on APIC IDs.
>
>
> My second proposal would be to introduce a FW_CFG_MAX_APIC_ID entry, so
> the SeaBIOS ACPI code can be kept simple.
>
> My third proposal would be to introduce a FW_CFG CPU Index => APIC ID
> table, but I really wouldn't like to introduce a new type of CPU
> identifier to be used between QEMU and SeaBIOS, when the APIC ID is a
> perfectly good unique CPU identifier that already exists in bare metal
> hardware.
>
> --
> Eduardo
>

Re: [Qemu-devel] [PATCH 4/8] qemu-option: add alias support

[Markus Armbruster]

Luiz Capitulino  writes:

> On Sat, 21 Jul 2012 10:11:39 +0200
> Markus Armbruster  wrote:
>
>> Luiz Capitulino  writes:
>> 
>> > Allow for specifying an alias for each option name, see next commits
>> > for examples.
>> >
>> > Signed-off-by: Luiz Capitulino 
>> > ---
>> >  qemu-option.c | 5 +++--
>> >  qemu-option.h | 1 +
>> >  2 files changed, 4 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/qemu-option.c b/qemu-option.c
>> > index 65ba1cf..b2f9e21 100644
>> > --- a/qemu-option.c
>> > +++ b/qemu-option.c
>> > @@ -623,7 +623,8 @@ static const QemuOptDesc *find_desc_by_name(const 
>> > QemuOptDesc *desc,
>> >  int i;
>> >  
>> >  for (i = 0; desc[i].name != NULL; i++) {
>> > -if (strcmp(desc[i].name, name) == 0) {
>> > +if (strcmp(desc[i].name, name) == 0 ||
>> > +(desc[i].alias && strcmp(desc[i].alias, name) == 0)) {
>> >  return &desc[i];
>> >  }
>> >  }
>> > @@ -645,7 +646,7 @@ static void opt_set(QemuOpts *opts, const char *name, 
>> > const char *value,

  static void opt_set(QemuOpts *opts, const char *name, const 
  bool prepend, Error **errp)
  {
  QemuOpt *opt;
  const QemuOptDesc *desc;
  Error *local_err = NULL;

  desc = find_desc_by_name(opts->list->desc, name);
  if (!desc && !opts_accepts_any(opts)) {
  error_set(errp, QERR_INVALID_PARAMETER, name);
  return;
>> >  }
>> >  
>> >  opt = g_malloc0(sizeof(*opt));
>> > -opt->name = g_strdup(name);
>> > +opt->name = g_strdup(desc ? desc->name : name);
>> >  opt->opts = opts;
>> >  if (prepend) {
>> >  QTAILQ_INSERT_HEAD(&opts->head, opt, next);
>> 
>> Are you sure this hunk belongs to this patch?  If yes, please explain
>> why :)
>
> Yes, I think it's fine because the change that makes it necessary to choose
> between desc->name and name is introduced by the hunk above.
>

I think I now get why you have this hunk:

We reach it only if the parameter with this name exists (desc non-null),
or opts accepts anthing (opts_accepts_any(opts).

If it exists, name equals desc->name before your patch.  But afterwards,
it could be either desc->name or desc->alias.  You normalize to
desc->name.

Else, all we can do is stick to name.

Correct?

If yes, then "normal" options and "accept any" options behave
differently: the former set opts->name to the canonical name, even when
the user uses an alias.  The latter set opts->name to whatever the user
uses.  I got a bad feeling about that.

> Of course that it's possible to move this to a separate patch, but I don't
> think it's worth it, as name is always valid with the current code.
>
>> > diff --git a/qemu-option.h b/qemu-option.h
>> > index 951dec3..7106d2f 100644
>> > --- a/qemu-option.h
>> > +++ b/qemu-option.h
>> > @@ -94,6 +94,7 @@ enum QemuOptType {
>> >  
>> >  typedef struct QemuOptDesc {
>> >  const char *name;
>> > +const char *alias;
>> >  enum QemuOptType type;
>> >  const char *help;
>> >  } QemuOptDesc;
>> 

Re: [Qemu-devel] [PATCH] configure: Support system emulation with large memory on w32 hosts

[Blue Swirl]

On Fri, Jul 20, 2012 at 5:19 PM, Stefan Weil  wrote:
> Am 20.07.2012 08:38, schrieb Alexey Kardashevskiy:
>
>> On 20/07/12 16:05, Alexey Kardashevskiy wrote:
>>>
>>> On 20/07/12 15:37, Alexey Kardashevskiy wrote:

 On 20/07/12 15:23, Stefan Weil wrote:
>
> Am 20.07.2012 05:53, schrieb Alexey Kardashevskiy:
>>
>> On 19/07/12 02:37, Stefan Weil wrote:
>>>
>>> 32-bit applications on Windows normally only get virtual memory in
>>> the lower 2 GiB address space.
>>>
>>> Because of memory fragmentation, VirtualAlloc() usually won't get 1
>>> GiB
>>> of contiguous virtual memory in that address space. Therefore running
>>> system emulations with 1 GiB or more RAM will abort with a failure.
>>>
>>> The linker flag --large-address-aware allows addresses in the upper 2
>>> GiB.
>>> With this flag, it is possible to run emulated machines with up to
>>> 2047 MiB RAM.
>>>
>>> Signed-off-by: Stefan Weil
>>> ---
>>>
>>> I tested the executables with large address awareness on a 64 bit
>>> Windows
>>> (works) and with Wine on Debian 32 bit Linux (no longer aborts, but
>>> hangs
>>> when 1024 or more MiB are requested).
>>>
>>> Maybe the support for large addresses is broken in my Wine version.
>>> Please report any different test results.
>>
>>
>> I tried with native WindowsXP, the patch did not help.
>>
>> QEMU was compiled with the new parameter:
>>
>> Install prefixc:/Program Files/QEMU
>> BIOS directoryc:/Program Files/QEMU
>> binary directory  c:/Program Files/QEMU
>> library directory c:/Program Files/QEMU/lib
>> include directory c:/Program Files/QEMU/include
>> config directory  c:/Program Files/QEMU
>> Source path   /home/aik/testken/qemu
>> C compileri686-pc-mingw32-gcc
>> Host C compiler   gcc
>> CFLAGS-O2 -D_FORTIFY_SOURCE=2 -g
>> QEMU_CFLAGS   -m32 -D__USE_MINGW_ANSI_STDIO=1
>> -DWIN32_LEAN_AND_MEAN
>> -DWINVER=0x501 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64
>> -D_LARGEFILE_SOURCE
>> -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings
>> -Wmissing-prototypes -fno-strict-aliasing  -fstack-protector-all
>> -Wendif-labels -Wmissing-include-dirs -Wempty-body -Wnested-externs
>> -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers
>> -Wold-style-declaration -Wold-style-definition -Wtype-limits
>> LDFLAGS   -Wl,--nxcompat -Wl,--no-seh
>> -Wl,--large-address-aware
>> -Wl,--dynamicbase -Wl,--warn-common -m32 -static -g
>> make  make
>> install   install
>> pythonpython
>> smbd  /usr/sbin/smbd
>> host CPU  i386
>> host big endian   no
>> target list   ppc64-softmmu
>> [...]
>>
>>
>>
>> qemu/ppc64-softmmu/qemu-system-ppc64 -M pseries -m 1024 -nographic -L
>> qemu/pc-bios/ -net none -append console=hvc0 -initrd ./1.cpio -kernel
>> ./guest.vmlinux.n -net nic,model=virtio -net
>> user,hostfwd=tcp::-:22
>> -drive
>> file=test-virtio-blk.img,if=virtio,index=0,media=disk,cache=unsafe
>> ***qemu_memalign 200 (512) size 800 (2048)
>> Warning: Disabling some instructions which are not emulated by TCG
>> (0x0, 0x6)
>> ***qemu_vmalloc 4000 (1073741824)
>> Failed to allocate memory: 8
>>
>> This application has requested the Runtime to terminate it in an
>> unusual way.
>> Please contact the application's support team for more information.
>>
>>
>> The lines with *** are mine printf("%s %x(%u)", __func__, etc).
>>
>> It still works with "-m 1023".
>
>
> Do you have enough RAM? Even with 64 bit Windows 7, I am
> limited to -m 7000 with 64 bit QEMU on a 8 GiB host.
> 32 bit QEMU runs there with up to 2047 MiB.



 I put another VirtualAlloc(64MB) call in qemu_vmalloc(1023MB) to check
 if
 it fails but it did not so it is possible to allocate 1087MB but in 2
 pieces.


> On 32 bit Windows XP, I'd expect that you need 2 GiB (or better
> 3 GiB) of physical RAM in your host, and close all other
> programs before you try QEMU.


 How do other programs matter? It is virtual 2GB per process, no?
 Ah, anyway, it has 2.994.340KB of physical RAM, swapping is enabled, the
 task manager shows at least "200" in "Available", one cygwin shell
 is
 running.
 And QEMU can always alloc 1023MB (in one chunk) and never 1024MB (also
 in
 one chunk). For me it looks like XP limitation but I could not find any
 mention of it in MSDN.


> Maybe I can run a test with Windows XP during the weekend.
>
> By the way: the patch must be modified to work for 64 bit QEMU.
> MinGW-w64's ld shows the new parameter in its help, but does
> not sup

Re: [Qemu-devel] [PULL] pci,msi,virtio

[Alex Williamson]

On Mon, 2012-07-23 at 13:16 -0500, Anthony Liguori wrote:
> On 07/23/2012 12:05 PM, Alex Williamson wrote:
> > On Mon, 2012-07-23 at 11:39 -0500, Anthony Liguori wrote:
> >> Andreas Färber  writes:
> >>
> >>> Am 19.07.2012 17:15, schrieb Michael S. Tsirkin:
>  The following changes since commit 
>  80aa796bf38b7ef21daa42673b4711510c450d8a:
> 
> pci_bridge_dev: fix error path in pci_bridge_dev_initfn() (2012-06-11 
>  22:55:13 +0300)
> 
>  are available in the git repository at:
> 
> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_anthony
> 
>  for you to fetch changes up to 932d4a42afa28829fadf3cbfbb0507cc09aafd8b:
> 
> msi/msix: added API to set MSI message address and data (2012-07-19 
>  17:56:42 +0300)
> 
>  
>  pci,msi,virtio
> 
>  This pull includes preparation patches mostly by Jan and Alex
>  that should help merge device assignment down the road.
>  And there's a new API needed for emulating POWER firmware.
> 
>  So no new functionality and some unused APIs but it looks like
>  merging will help people make progress.
> 
>  Signed-off-by: Michael S. Tsirkin
> >>>
> >>> Usually, PULLs are expected to carry the individual patches as replies.
> >>>
> >>> But more important, did something go wrong with rebasing before sending
> >>> out the PULL? June 11 is more than a month ago. And if I try to rebase
> >>> my pci_host branch on your "pci" branch it tries to replay loads of
> >>> really old post-1.1 commits (e.g., my "Pass PowerPCCPU to...") and
> >>> fails... am I doing something wrong? "for_anthony" tag and "pci" branch
> >>> seem to match in date at least.
> >>
> >>
> >> It's a tag, not a branch.
> >>
> >> I had to add an explicit remote to pull tags in.  I'm not sure if
> >> there's a better way to do it in git.
> >
> > Looks like it merged ok for me, but you'll need this to complete the
> > PCIUnregisterFunc conversion.  Not sure how this built in MST's tree.
> > Anthony, it'd be great if we could merge and fix this in your tree since
> > MST is out of the office.  Thanks,
> >
> > Alex
> 
> I can't fix it without rewriting git history or breaking bisection which 
> isn't 
> something I'm really willing to do.
> 
> When does MST get back?  If it's not for a while, I can merge it all manually 
> with a fixup.

Next week, so (unfortunately for me) probably not worth you messing with
it.  Thanks,

Alex


> > commit 34e37573f1a983dab43673d664eb500bc87a46d4
> > Author: Alex Williamson
> > Date:   Mon Jul 23 10:59:43 2012 -0600
> >
> >  esp: Update PCIUnregisterFunc function
> >
> >  Function prototype changed in f90c2bcd
> >
> >  Signed-off-by: Alex Williamson
> >
> > diff --git a/hw/esp.c b/hw/esp.c
> > index c6422ad..a011347 100644
> > --- a/hw/esp.c
> > +++ b/hw/esp.c
> > @@ -1153,13 +1153,11 @@ static int esp_pci_scsi_init(PCIDevice *dev)
> >   return 0;
> >   }
> >
> > -static int esp_pci_scsi_uninit(PCIDevice *d)
> > +static void esp_pci_scsi_uninit(PCIDevice *d)
> >   {
> >   PCIESPState *pci = DO_UPCAST(PCIESPState, dev, d);
> >
> >   memory_region_destroy(&pci->io);
> > -
> > -return 0;
> >   }
> >
> >   static void esp_pci_class_init(ObjectClass *klass, void *data)
> >
> >
> >
> >
> 
> 

Re: [Qemu-devel] [PATCH 13/22] Add migration capabilities

[Luiz Capitulino]

On Fri, 13 Jul 2012 09:23:35 +0200
Juan Quintela  wrote:

> From: Orit Wasserman 
> 
> Add migration capabilities that can be queried by the management.
> The management can query the source QEMU and the destination QEMU in order to
> verify both support some migration capability (currently only XBZRLE).
> The management can enable a capability for the next migration by using
> migrate_set_parameter command.

Please, split this into one command per-patch. Otherwise it's difficult to
review.

Have libvirt folks acked this approach btw? It looks fine to me, but we need
their ack too.

More comments below.

> 
> Signed-off-by: Orit Wasserman 
> Signed-off-by: Juan Quintela 
> ---
>  hmp-commands.hx  |   16 
>  hmp.c|   64 
>  hmp.h|2 ++
>  migration.c  |   72 
> --
>  migration.h  |2 ++
>  monitor.c|7 ++
>  qapi-schema.json |   53 ++--
>  qmp-commands.hx  |   71 ++---
>  8 files changed, 280 insertions(+), 7 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index f5d9d91..9245bef 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -861,6 +861,20 @@ Set maximum tolerated downtime (in seconds) for 
> migration.
>  ETEXI
> 
>  {
> +.name   = "migrate_set_parameter",
> +.args_type  = "capability:s,state:b",
> +.params = "",

Please, fill in params.

> +.help   = "Enable/Disable the usage of a capability for 
> migration",
> +.mhandler.cmd = hmp_migrate_set_parameter,
> +},
> +
> +STEXI
> +@item migrate_set_parameter @var{capability} @var{state}
> +@findex migrate_set_parameter
> +Enable/Disable the usage of a capability @var{capability} for migration.
> +ETEXI
> +
> +{
>  .name   = "client_migrate_info",
>  .args_type  = 
> "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?",
>  .params = "protocol hostname port tls-port cert-subject",
> @@ -1419,6 +1433,8 @@ show CPU statistics
>  show user network stack connection states
>  @item info migrate
>  show migration status
> +@item info migration_capabilities
> +show migration capabilities
>  @item info balloon
>  show balloon information
>  @item info qtree
> diff --git a/hmp.c b/hmp.c
> index 4c6d4ae..b0440e6 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -131,9 +131,19 @@ void hmp_info_mice(Monitor *mon)
>  void hmp_info_migrate(Monitor *mon)
>  {
>  MigrationInfo *info;
> +MigrationCapabilityInfoList *cap;
> 
>  info = qmp_query_migrate(NULL);
> 
> +if (info->has_capabilities && info->capabilities) {
> +monitor_printf(mon, "capabilities: ");
> +for (cap = info->capabilities; cap; cap = cap->next) {
> +monitor_printf(mon, "%s: %s ",
> +   
> MigrationCapability_lookup[cap->value->capability],
> +   cap->value->state ? "on" : "off");
> +}
> +monitor_printf(mon, "\n");

Why is this is needed? Isn't info migration-capabilities good enough?
Besides, info migrate should only contain information about current migration
process...

> +}
>  if (info->has_status) {
>  monitor_printf(mon, "Migration status: %s\n", info->status);
>  }
> @@ -161,6 +171,25 @@ void hmp_info_migrate(Monitor *mon)
>  qapi_free_MigrationInfo(info);
>  }
> 
> +void hmp_info_migration_capabilities(Monitor *mon)
> +{
> +MigrationCapabilityInfoList *caps_list, *cap;
> +
> +caps_list = qmp_query_migration_capabilities(NULL);
> +if (!caps_list) {
> +monitor_printf(mon, "No migration capabilities found\n");
> +return;
> +}
> +
> +for (cap = caps_list; cap; cap = cap->next) {
> +monitor_printf(mon, "%s: %s ",
> +   MigrationCapability_lookup[cap->value->capability],
> +   cap->value->state ? "on" : "off");
> +}
> +
> +qapi_free_MigrationCapabilityInfoList(caps_list);
> +}
> +
>  void hmp_info_cpus(Monitor *mon)
>  {
>  CpuInfoList *cpu_list, *cpu;
> @@ -735,6 +764,41 @@ void hmp_migrate_set_speed(Monitor *mon, const QDict 
> *qdict)
>  qmp_migrate_set_speed(value, NULL);
>  }
> 
> +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
> +{
> +const char *cap = qdict_get_str(qdict, "capability");
> +bool state = qdict_get_bool(qdict, "state");
> +Error *err = NULL;
> +MigrationCapabilityInfoList *params = NULL;
> +int i;
> +
> +for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) {
> +if (strcmp(cap, MigrationCapability_lookup[i]) == 0) {
> +if (!params) {
> +params = g_malloc0(sizeof(*params));
> +}
> +params->value = g_malloc0(sizeof(*params->value));
> +params->value->capability = i;
> +params->val

Re: [Qemu-devel] [PULL] pci,msi,virtio

[Anthony Liguori]

On 07/23/2012 12:05 PM, Alex Williamson wrote:

On Mon, 2012-07-23 at 11:39 -0500, Anthony Liguori wrote:

Andreas Färber  writes:


Am 19.07.2012 17:15, schrieb Michael S. Tsirkin:

The following changes since commit 80aa796bf38b7ef21daa42673b4711510c450d8a:

   pci_bridge_dev: fix error path in pci_bridge_dev_initfn() (2012-06-11 
22:55:13 +0300)

are available in the git repository at:

   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_anthony

for you to fetch changes up to 932d4a42afa28829fadf3cbfbb0507cc09aafd8b:

   msi/msix: added API to set MSI message address and data (2012-07-19 17:56:42 
+0300)


pci,msi,virtio

This pull includes preparation patches mostly by Jan and Alex
that should help merge device assignment down the road.
And there's a new API needed for emulating POWER firmware.

So no new functionality and some unused APIs but it looks like
merging will help people make progress.

Signed-off-by: Michael S. Tsirkin


Usually, PULLs are expected to carry the individual patches as replies.

But more important, did something go wrong with rebasing before sending
out the PULL? June 11 is more than a month ago. And if I try to rebase
my pci_host branch on your "pci" branch it tries to replay loads of
really old post-1.1 commits (e.g., my "Pass PowerPCCPU to...") and
fails... am I doing something wrong? "for_anthony" tag and "pci" branch
seem to match in date at least.



It's a tag, not a branch.

I had to add an explicit remote to pull tags in.  I'm not sure if
there's a better way to do it in git.


Looks like it merged ok for me, but you'll need this to complete the
PCIUnregisterFunc conversion.  Not sure how this built in MST's tree.
Anthony, it'd be great if we could merge and fix this in your tree since
MST is out of the office.  Thanks,

Alex


I can't fix it without rewriting git history or breaking bisection which isn't 
something I'm really willing to do.


When does MST get back?  If it's not for a while, I can merge it all manually 
with a fixup.


Regards,

Anthony Liguori



commit 34e37573f1a983dab43673d664eb500bc87a46d4
Author: Alex Williamson
Date:   Mon Jul 23 10:59:43 2012 -0600

 esp: Update PCIUnregisterFunc function

 Function prototype changed in f90c2bcd

 Signed-off-by: Alex Williamson

diff --git a/hw/esp.c b/hw/esp.c
index c6422ad..a011347 100644
--- a/hw/esp.c
+++ b/hw/esp.c
@@ -1153,13 +1153,11 @@ static int esp_pci_scsi_init(PCIDevice *dev)
  return 0;
  }

-static int esp_pci_scsi_uninit(PCIDevice *d)
+static void esp_pci_scsi_uninit(PCIDevice *d)
  {
  PCIESPState *pci = DO_UPCAST(PCIESPState, dev, d);

  memory_region_destroy(&pci->io);
-
-return 0;
  }

  static void esp_pci_class_init(ObjectClass *klass, void *data)

Re: [Qemu-devel] [RFC] introduce a dynamic library to expose qemu block API

[Blue Swirl]

On Wed, Jul 18, 2012 at 8:51 AM, Wenchao Xia  wrote:
>   Hi, following is API draft, prototypes were taken from qemu/block.h,
> and the API prefix is changed frpm bdrv to qbdrvs, to declare related
> object is BlockDriverState, not BlockDriver. One issue here is it may
> require include block_int.h, which is not LGPL2 licensed yet.
>   API format is kept mostly the same with qemu generic block layer, to
> make it easier for implement, and easier to make qemu migrate on it if
> possible.
>
>
> /* structure init and uninit */
> BlockDriverState *qbdrvs_new(const char *device_name);
> void qbdrvs_delete(BlockDriverState *bs);
>
>
> /* file open and close */
> int qbdrvs_open(BlockDriverState *bs, const char *filename, int flags,
>   BlockDriver *drv);

How are the errors passed?

Alternative version with file descriptor or struct FILE instead of
filename might become useful but can be added later.

> void qbdrvs_close(BlockDriverState *bs);
> int qbdrvs_img_create(const char *filename, const char *fmt,
> const char *base_filename, const char *base_fmt,
> char *options, uint64_t img_size, int flags);

'const char *options'

>
>
> /* sync access */
> int qbdrvs_read(BlockDriverState *bs, int64_t sector_num,
>   uint8_t *buf, int nb_sectors);
> int qbdrvs_write(BlockDriverState *bs, int64_t sector_num,
>const uint8_t *buf, int nb_sectors);

Do we want to use sectors here? How about just raw byte offsets and
number of bytes? I'd leave any hardware details out and just provide
file semantics (open/read/write/close). Future QEMU refactorings could
make supporting HW info inconvenient. If HW details (geometry etc., VM
state) are needed, there should be a separate API.

Vectored I/O might be useful too.

>
>
> /* info retrieve */
> //sector, size and geometry info
> int qbdrvs_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);

Currently BlockDriverInfo does not look very useful for outside users,
with the exception of dirty state. How about accessors instead:
bool qbdrvs_is_dirty(BlockDriverState *bs);

Also the format (QCOW) is needed so that the user can use backing file
functions:
const char *qbdrvs_get_format(BlockDriverState *bs);

> int64_t qbdrvs_getlength(BlockDriverState *bs);
> int64_t qbdrvs_get_allocated_file_size(BlockDriverState *bs);
> void qbdrvs_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);

Also this function shouldn't be needed if we don't give HW details
with this API.

> //image type
> const char *qbdrvs_get_format_name(BlockDriverState *bs);
> //backing file info
> void qbdrvs_get_backing_filename(BlockDriverState *bs,
>char *filename, int filename_size);
> void qbdrvs_get_full_backing_filename(BlockDriverState *bs,
> char *dest, size_t sz);

These are specific to QCOW etc., so
bool qbdrvs_has_backing_files(BlockDriverState *bs)?

>
>
> /* advanced image content access */
> int qbdrvs_is_allocated(BlockDriverState *bs, int64_t sector_num, int
> nb_sectors,
>   int *pnum);
> int qbdrvs_discard(BlockDriverState *bs, int64_t sector_num, int
> nb_sectors);

Again, some files do not have a concept of allocation.

> int qbdrvs_has_zero_init(BlockDriverState *bs);
>
>
>> Il 16/07/2012 10:16, Wenchao Xia ha scritto:


>>>Really thanks for the investigation, I paid quite sometime to dig out
>>> which license is compatible to LGPL, this have sorted it out.
>>>The coroutine and structure inside is quite a challenge.
>>
>>
>> Coroutines are really just a small complication in the program flow if
>> all you support is synchronous access to files (i.e. no HTTP etc.).
>> Their usage should be completely transparent.
>>
>>> What about
>>> provide the library first in nbd + sync access, and waiting for the
>>> library employer response? If it is good to use, then replace implement
>>> code to native qemu block layer code, change code's license, while keep
>>> API unchanged.
>>
>>
>> You can start by proposing the API.
>>
>> Paolo
>>
>
>
> --
> Best Regards
>
> Wenchao Xia
>
>
>

[Qemu-devel] [Bug 918791] Re: qemu-kvm dies when using vmvga driver and unity in the guest

[Serge Hallyn]

@houstonbofh

which release is your host?  Please show the result of 'virsh dumpxml
MACHINEID | grep machine'

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/918791

Title:
  qemu-kvm dies when using vmvga driver and unity in the guest

Status in QEMU:
  New
Status in “qemu-kvm” package in Ubuntu:
  Fix Released
Status in “xserver-xorg-video-vmware” package in Ubuntu:
  Invalid
Status in “qemu-kvm” source package in Oneiric:
  Fix Committed
Status in “xserver-xorg-video-vmware” source package in Oneiric:
  Invalid
Status in “qemu-kvm” source package in Precise:
  Fix Released
Status in “xserver-xorg-video-vmware” source package in Precise:
  Invalid

Bug description:
  =
  SRU Justification:
  1. impact: kvm crashes
  2. Development fix: don't allow attempts to set_bit to negative offsets
  3. Stable fix: same as development fix
  4. Test case (see below)
  5. Regression potential: if the patch is wrong, graphics for vmware vga over 
vnc could get messed up
  =

  12.04's qemu-kvm has been unstable for me and Marc Deslauriers and I
  figured out it has something to do with the interaction of qemu-kvm,
  unity and the vmvga driver. This is a regression over qemu-kvm in
  11.10.

  TEST CASE:
  1. start a VM that uses unity (eg, 11.04, 11.10 or 12.04). My tests use 
unity-2d on an amd64 host and amd64 guests
  2. on 11.04 and 11.10, open empathy via the messaging indicator and click 
'Chat'. On 12.04, open empathy via the messaging indicator and click 'Chat', 
close the empathy wizard, move the empathy window over the unity luancher (so 
it autohides), then do 'ctrl+alt+t' to open a terminal

  When the launcher tries to auto(un)hide, qemu-kvm dies with this:
  [10574.958149] do_general_protection: 132 callbacks suppressed
  [10574.958154] kvm[13192] general protection ip:7fab9680ea0f sp:74440148 
error:0 in qemu-system-x86_64[7fab966c4000+2c9000]

  Relevant libvirt xml:
  
    
    
  

  If I change to using 'cirrus', then qemu-kvm no longer crashes. Eg:
  
    
    
    
  

  The workaround is therefore to use the cirrus driver instead of vmvga,
  however being able to kill qemu-kvm in this manner is not ideal. Also,
  unfortunately unity-2d does not run with with cirrus driver under
  11.04, so the security and SRU teams are unable to properly test
  updates in GUI applications under unity when using the current 12.04
  qemu-kvm.

  I tried to report this via apport, but apport complained about a CRC
  error, so I could not.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/918791/+subscriptions

Re: [Qemu-devel] [PATCH 1/8] qemu-option: qemu_opt_set_bool(): fix code duplication

[Markus Armbruster]

Luiz Capitulino  writes:

> On Sat, 21 Jul 2012 10:09:09 +0200
> Markus Armbruster  wrote:
>
>> Luiz Capitulino  writes:
>> 
>> > Call qemu_opt_set() instead of duplicating opt_set().
>> >
>> > Signed-off-by: Luiz Capitulino 
>> > ---
>> >  qemu-option.c | 28 +---
>> >  1 file changed, 1 insertion(+), 27 deletions(-)
>> >
>> > diff --git a/qemu-option.c b/qemu-option.c
>> > index bb3886c..2cb2835 100644
>> > --- a/qemu-option.c
>> > +++ b/qemu-option.c
>> > @@ -677,33 +677,7 @@ void qemu_opt_set_err(QemuOpts *opts, const char
>> > *name, const char *value,
>> >  
>> >  int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
>> >  {
>> > -QemuOpt *opt;
>> > -const QemuOptDesc *desc = opts->list->desc;
>> > -int i;
>> > -
>> > -for (i = 0; desc[i].name != NULL; i++) {
>> > -if (strcmp(desc[i].name, name) == 0) {
>> > -break;
>> > -}
>> > -}
>> > -if (desc[i].name == NULL) {
>> > -if (i == 0) {
>> > -/* empty list -> allow any */;
>> > -} else {
>> > -qerror_report(QERR_INVALID_PARAMETER, name);
>> > -return -1;
>> > -}
>> > -}
>> > -
>> > -opt = g_malloc0(sizeof(*opt));
>> > -opt->name = g_strdup(name);
>> > -opt->opts = opts;
>> > -QTAILQ_INSERT_TAIL(&opts->head, opt, next);
>> > -if (desc[i].name != NULL) {
>> > -opt->desc = desc+i;
>> > -}
>> > -opt->value.boolean = !!val;
>> > -return 0;
>> > +return qemu_opt_set(opts, name, val ? "on" : "off");
>> >  }
>> >  
>> >  int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
>> 
>> Does a bit more than just obvious code de-duplication.  Two things in
>> particular:
>> 
>> * Error reporting
>> 
>>   Old: qerror_report(); return -1
>> 
>>   New: opt_set() and qemu_opt_set() cooperate, like this:
>>opt_set(): error_set(); return;
>>qemu_opt_set():
>> if (error_is_set(&local_err)) {
>> qerror_report_err(local_err);
>> error_free(local_err);
>> return -1;
>> }
>> 
>>   I guess the net effect is the same.  Not sure it's worth mentioning in
>>   the commit message.
>
> The end result of calling qemu_opt_set_bool() is the same. The difference
> between qerror_report() and qerror_report_err() is that the former gets error
> information from the error call, while the latter gets error information
> from the Error object. But they do the same thing.
>
>> * New sets opt->str to either "on" or "off" depending on val, then lets
>>   reconstructs the value with qemu_opt_parse().  Which can't fail then.
>>   I figure the latter part has no further impact.  But what about
>>   setting opt->str?  Is this a bug fix?
>
> I don't remember if opt->str is read after the QemuOpt object is built. If
> it's, then yes, this is a bug fix. Otherwise it just make the final
> QemuOpt object more 'conforming'.

Uses of opt->str, and what happens when it isn't set:

* qemu_opt_get(): returns NULL, which means "not set".  Bug can bite
  when value isn't the default value.

* qemu_opt_parse(): passes NULL to parse_option_bool(), which treats it
  like "on".  Wrong if the value is actually false.  Bug can bite when
  qemu_opts_validate() runs after qemu_opt_set_bool().

* qemu_opt_del(): passes NULL to g_free(), which is just fine.

* qemu_opt_foreach(): passes NULL to the callback, which is unlikely to
  be prepared for it.

* qemu_opts_print(): prints NULL, which crashes on some systems.

* qemu_opts_to_qdict(): passes NULL to qstring_from_str(), which
  crashes.

Right now, the only use of qemu_opt_set_bool() is the one in vl.c.
Can't see how to break it, but I didn't look hard.

I recommend to document the bug fix in the commit message.

[Qemu-devel] [PATCH] linux-user: Move target_to_host_errno_table[] setup out of ioctl loop

[Peter Maydell]

The code to initialise the target_to_host_errno_table[] array was
accidentally inside the loop through checking and initialising all
the supported ioctls. This was harmless but meant that we reinitialised the
array several hundred times on startup.

Signed-off-by: Peter Maydell 
---
The code seems to have been incorrectly placed like this since it was
first committed...

 linux-user/syscall.c |   10 +-
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 539af3f..9f9ad9a 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -4594,6 +4594,11 @@ void syscall_init(void)
 #undef STRUCT
 #undef STRUCT_SPECIAL
 
+/* Build target_to_host_errno_table[] table from
+ * host_to_target_errno_table[]. */
+for (i=0; i < ERRNO_TABLE_SIZE; i++)
+target_to_host_errno_table[host_to_target_errno_table[i]] = i;
+
 /* we patch the ioctl size if necessary. We rely on the fact that
no ioctl has all the bits at '1' in the size field */
 ie = ioctl_entries;
@@ -4613,11 +4618,6 @@ void syscall_init(void)
 (size << TARGET_IOC_SIZESHIFT);
 }
 
-/* Build target_to_host_errno_table[] table from
- * host_to_target_errno_table[]. */
-for (i=0; i < ERRNO_TABLE_SIZE; i++)
-target_to_host_errno_table[host_to_target_errno_table[i]] = i;
-
 /* automatic consistency check if same arch */
 #if (defined(__i386__) && defined(TARGET_I386) && defined(TARGET_ABI32)) || \
 (defined(__x86_64__) && defined(TARGET_X86_64))
-- 
1.7.5.4

[Qemu-devel] [PATCH] linux-user: Fix SNDCTL_DSP_MAP{IN, OUT}BUF ioctl definitions

[Peter Maydell]

Fix the SNDCTL_DSP_MAP{IN,OUT}BUF ioctl definitions so that they
refer to a suitably defined target struct layout rather than hardcoding
the ioctl number. This fixes complaints from the syscall_init()
consistency check when running an x86_64-to-x86_64 linux-user qemu.

Signed-off-by: Peter Maydell 
---
 linux-user/ioctls.h|4 ++--
 linux-user/syscall_defs.h  |4 ++--
 linux-user/syscall_types.h |3 +++
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
index eb96a08..8a47767 100644
--- a/linux-user/ioctls.h
+++ b/linux-user/ioctls.h
@@ -186,8 +186,8 @@
   IOCTL(SNDCTL_DSP_GETISPACE, IOC_R, MK_PTR(MK_STRUCT(STRUCT_audio_buf_info)))
   IOCTL(SNDCTL_DSP_GETOSPACE, IOC_R, MK_PTR(MK_STRUCT(STRUCT_audio_buf_info)))
   IOCTL(SNDCTL_DSP_GETTRIGGER, IOC_R, MK_PTR(TYPE_INT))
-  IOCTL(SNDCTL_DSP_MAPINBUF, IOC_R, MK_PTR(TYPE_INT))
-  IOCTL(SNDCTL_DSP_MAPOUTBUF, IOC_R, MK_PTR(TYPE_INT))
+  IOCTL(SNDCTL_DSP_MAPINBUF, IOC_R, MK_PTR(MK_STRUCT(STRUCT_buffmem_desc)))
+  IOCTL(SNDCTL_DSP_MAPOUTBUF, IOC_R, MK_PTR(MK_STRUCT(STRUCT_buffmem_desc)))
   IOCTL(SNDCTL_DSP_NONBLOCK, 0, TYPE_NULL)
   IOCTL(SNDCTL_DSP_POST, 0, TYPE_NULL)
   IOCTL(SNDCTL_DSP_RESET, 0, TYPE_NULL)
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 0b239c4..ed829e9 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -2163,8 +2163,8 @@ struct target_eabi_flock64 {
 #define TARGET_SNDCTL_DSP_GETTRIGGER  TARGET_IOR('P',16, int)
 #define TARGET_SNDCTL_DSP_GETIPTR TARGET_IORU('P',17)
 #define TARGET_SNDCTL_DSP_GETOPTR TARGET_IORU('P',18)
-#define TARGET_SNDCTL_DSP_MAPINBUF0x80085013
-#define TARGET_SNDCTL_DSP_MAPOUTBUF   0x80085014
+#define TARGET_SNDCTL_DSP_MAPINBUFTARGET_IORU('P', 19)
+#define TARGET_SNDCTL_DSP_MAPOUTBUF   TARGET_IORU('P', 20)
 #define TARGET_SNDCTL_DSP_NONBLOCK0x500e
 #define TARGET_SNDCTL_DSP_SAMPLESIZE  0xc0045005
 #define TARGET_SNDCTL_DSP_SETDUPLEX   0x5016
diff --git a/linux-user/syscall_types.h b/linux-user/syscall_types.h
index 601618d..44b6a58 100644
--- a/linux-user/syscall_types.h
+++ b/linux-user/syscall_types.h
@@ -77,6 +77,9 @@ STRUCT(audio_buf_info,
 STRUCT(count_info,
TYPE_INT, TYPE_INT, TYPE_INT)
 
+STRUCT(buffmem_desc,
+   TYPE_PTRVOID, TYPE_INT)
+
 STRUCT(mixer_info,
MK_ARRAY(TYPE_CHAR, 16), MK_ARRAY(TYPE_CHAR, 32), TYPE_INT, 
MK_ARRAY(TYPE_INT, 10))
 
-- 
1.7.5.4

[Qemu-devel] [PATCH] linux-user: Fix incorrect TARGET_BLKBSZGET, TARGET_BLKBSZSET

[Peter Maydell]

The definitions for the ioctl numbers TARGET_BLKBSZGET and
TARGET_BLKBSZSET had the wrong size parameters (they are defined
with size_t, not int, even though the ioctl implementations themselves
read and write integers). Since commit 354a0008 we now have an
ioctl wrapper definition for BLKBSZGET and so on an x86-64-to-x86-64
linux-user binary we were triggering the mismatch warning in
syscall_init().

Signed-off-by: Peter Maydell 
---
 linux-user/syscall_defs.h |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index a79b67d..0b239c4 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -832,8 +832,8 @@ struct target_pollfd {
 #define TARGET_BLKSECTGET TARGET_IO(0x12,103)/* get max sectors per request 
(ll_rw_blk.c) */
 #define TARGET_BLKSSZGET  TARGET_IO(0x12,104)/* get block device sector size */
 /* A jump here: 108-111 have been used for various private purposes. */
-#define TARGET_BLKBSZGET  TARGET_IOR(0x12,112,int)
-#define TARGET_BLKBSZSET  TARGET_IOW(0x12,113,int)
+#define TARGET_BLKBSZGET  TARGET_IOR(0x12, 112, abi_ulong)
+#define TARGET_BLKBSZSET  TARGET_IOW(0x12, 113, abi_ulong)
 #define TARGET_BLKGETSIZE64 TARGET_IOR(0x12,114,abi_ulong)
  /* return device size in bytes
 (u64 *arg) */
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH] kvm: Move kvm_allows_irq0_override() to target-i386

[Peter Maydell]

On 23 July 2012 15:30, Avi Kivity  wrote:
> But I was only joking.  Nested virtualization is interesting technically
> but so far I haven't seen any huge or even small uptake.

Yes; that (as I understand it) is why it wasn't an expected use
case for the architecture extensions. The other related thing that
might be surprising for x86-background people is that being
able to present the guest with a virtual CPU that looks like
a pre-virtualization CPU (eg the A9) isn't really an intended
use case either. (The ARM world has much less of the 'everything
must be fully backwards compatible for existing OSes' than x86...)

-- PMM

Re: [Qemu-devel] [PATCH] kvm: Move kvm_allows_irq0_override() to target-i386

[Jan Kiszka]

On 2012-07-23 19:41, Peter Maydell wrote:
> On 23 July 2012 17:55, Jan Kiszka  wrote:
>> On 2012-07-23 17:19, Peter Maydell wrote:
>>> OK, let's see if we can get some agreement about naming here.
>>>
>>> First, some test-functions I think we definitely need:
>>>
>>>  kvm_interrupts_are_async()
>>>-- true if interrupt delivery is asynchronous
>>>   default false in kvm_init, set true in kvm_irqchip_create,
>>>   architectures may set it true in kvm_arch_init [ARM will
>>>   do so; PPC might want to do so]
>>>
>>>  kvm_irqchip_in_kernel()
>>>-- the user-settable option, actual behaviour is arch specific
>>>   on x86, true means (as it does now) LAPIC,IOAPIC,PIT in kernel
>>>   on ARM, we ignore this setting and just DTRT
>>
>> You should reject kernel_irqchip=off as long as you only support an
>> in-kernel GIC model.
> 
> Agreed. (At the moment we still have code in QEMU for out-of-kernel
> GIC models, which is legacy from before the VGIC implementation;
> depending on whether that actually vanishes from the kernel ABI
> or not I may include the QEMU code which makes arm_pic.c handle
> injecting interrupts to KVM via ioctl. But in that case we would
> reject =off for A15 and =on for non-A15 (slightly pointlessly
> since non-A15 will fail the "must be an A15" check.))
> 
>>>   on PPC, used as a convenience setting for whether to use
>>>   an in-kernel model of the interrupt controller
>>>   Shouldn't be used in non-target-specific code
>>>
>>> and two I'm not quite so sure about:
>>>
>>>  kvm_has_msi_routing()
>>>-- true if we can do routing of MSIs
>>
>> GSI, not MSI.
>>
>>>   set true only if x86 and kvm_irqchip_in_kernel
>>
>> It means that the target architecture supports routing of various
>> interrupt sources (userspace, irqfds, in-kernel device models) to
>> different in-kernel IRQ sinks (CPU cores, irqchip models, whatever).
>> Interrupt messages via (binary-state) irqfd depend on it.
> 
> So I still don't actually understand what this interrupt routing
> is (as far as the kernel is concerned). Surely the way this should
> work is that you use KVM_IRQFD to say "this fd goes to this
> interrupt sink" (with some sensible scheme for encoding what the
> interrupt destination actually is, like an (irqchip,pin) tuple
> or something). Why does the kernel need to care about irq
> routing?

- to define where to inject if you have multiple irqchips to address, or
  if there is a special sink that reattaches payload (e.g. MSI
  messages) to a binary source like irqfd is
- to configure board-specific re-directions (like that IRQ0 override on
  x86)

> 
>>> kvm-all.c:kvm_irqchip_set_irq():
>>>  -- (just an assert) should be kvm_interrupts_are_async()
>>
>> The name kvm_irqchip_set_irq implies so far that it injects into an
>> in-kernel irqchip model. Either different functions for archs that don't
>> follow this concept need to be provided, or this function requires
>> renaming (kvm_set_irq_async or so).
> 
> Yes, renaming the function would probably be reasonable.
> (Currently my QEMU code for using the VGIC just does a direct
> ioctl itself though, because kvm_irqchip_set_irq() doesn't actually
> do very much and it's a bit awkward that it insists on fishing
> the ioctl number out from the KVMState.)

The latter has to do with legacy and lacking support for certain IOCTLs
on older kernels. However, it shouldn't hurt you to take the generic
route for the sake of consistency and a to ease instrumentations etc.

> 
>>> kvm-all.c:kvm_irqchip_assign_irqfd():
>>>  -- should be true if kvm_has_irqfds()
>>
>> The same issue. Plus there is that naming conflict again if we should
>> ever see irqfd without some in-kernel irqchip. But even s390 would have
>> a logical "irqchip" for me at the point it may route interrupt messages
>> from devices directly to the CPUs.
> 
> I don't think you can have irqfds without an irqchip because where
> would you be sending the interrupts?

To the CPU cores. But even that requires routing and arbitration if
there are multiple receivers. So I think s390 has an in-kernel (or
"in-hardware") irqchip, conceptually.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] configure: fix ALSA configure test

[Peter Maydell]

On 23 July 2012 18:40, Blue Swirl  wrote:
> On Tue, Jul 17, 2012 at 7:28 PM, Peter Maydell  
> wrote:
>> The trouble is that the warnings and errors here don't cause the
>> build to fail noisily; that's a big distinction IMHO.
>> I suppose we could make compile_prog do something like:
>>  * run the compile test
>
> Unfortunately that would break cross compiling.

Sorry, that was slightly unclear phrasing. By "run the test"
I meant "do the check that we can compile the test code"
"run the binary produced". This series won't break cross
compiling.

-- PMM

Re: [Qemu-devel] [PATCH v3 2/2] bitops: fix types

[Peter Maydell]

On 23 July 2012 18:33, Blue Swirl  wrote:
> I'm getting a strong feeling that it's a bad idea to reuse any Linux
> kernel sources since they are seen as divine and untouchable, unlike
> for example BSD queue macros.

We should also try to avoid deviations in our queue macros,
and I think we do (eg commit 6095aa8 added functionality by
moving us closer into sync with the BSD macros rather than
by reinventing the wheel which was IIRC what the initial pre-code-review
patch did).

-- PMM

Re: [Qemu-devel] [PATCH] kvm: Move kvm_allows_irq0_override() to target-i386

[Peter Maydell]

On 23 July 2012 17:55, Jan Kiszka  wrote:
> On 2012-07-23 17:19, Peter Maydell wrote:
>> OK, let's see if we can get some agreement about naming here.
>>
>> First, some test-functions I think we definitely need:
>>
>>  kvm_interrupts_are_async()
>>-- true if interrupt delivery is asynchronous
>>   default false in kvm_init, set true in kvm_irqchip_create,
>>   architectures may set it true in kvm_arch_init [ARM will
>>   do so; PPC might want to do so]
>>
>>  kvm_irqchip_in_kernel()
>>-- the user-settable option, actual behaviour is arch specific
>>   on x86, true means (as it does now) LAPIC,IOAPIC,PIT in kernel
>>   on ARM, we ignore this setting and just DTRT
>
> You should reject kernel_irqchip=off as long as you only support an
> in-kernel GIC model.

Agreed. (At the moment we still have code in QEMU for out-of-kernel
GIC models, which is legacy from before the VGIC implementation;
depending on whether that actually vanishes from the kernel ABI
or not I may include the QEMU code which makes arm_pic.c handle
injecting interrupts to KVM via ioctl. But in that case we would
reject =off for A15 and =on for non-A15 (slightly pointlessly
since non-A15 will fail the "must be an A15" check.))

>>   on PPC, used as a convenience setting for whether to use
>>   an in-kernel model of the interrupt controller
>>   Shouldn't be used in non-target-specific code
>>
>> and two I'm not quite so sure about:
>>
>>  kvm_has_msi_routing()
>>-- true if we can do routing of MSIs
>
> GSI, not MSI.
>
>>   set true only if x86 and kvm_irqchip_in_kernel
>
> It means that the target architecture supports routing of various
> interrupt sources (userspace, irqfds, in-kernel device models) to
> different in-kernel IRQ sinks (CPU cores, irqchip models, whatever).
> Interrupt messages via (binary-state) irqfd depend on it.

So I still don't actually understand what this interrupt routing
is (as far as the kernel is concerned). Surely the way this should
work is that you use KVM_IRQFD to say "this fd goes to this
interrupt sink" (with some sensible scheme for encoding what the
interrupt destination actually is, like an (irqchip,pin) tuple
or something). Why does the kernel need to care about irq
routing?

>> kvm-all.c:kvm_irqchip_set_irq():
>>  -- (just an assert) should be kvm_interrupts_are_async()
>
> The name kvm_irqchip_set_irq implies so far that it injects into an
> in-kernel irqchip model. Either different functions for archs that don't
> follow this concept need to be provided, or this function requires
> renaming (kvm_set_irq_async or so).

Yes, renaming the function would probably be reasonable.
(Currently my QEMU code for using the VGIC just does a direct
ioctl itself though, because kvm_irqchip_set_irq() doesn't actually
do very much and it's a bit awkward that it insists on fishing
the ioctl number out from the KVMState.)

>> kvm-all.c:kvm_irqchip_assign_irqfd():
>>  -- should be true if kvm_has_irqfds()
>
> The same issue. Plus there is that naming conflict again if we should
> ever see irqfd without some in-kernel irqchip. But even s390 would have
> a logical "irqchip" for me at the point it may route interrupt messages
> from devices directly to the CPUs.

I don't think you can have irqfds without an irqchip because where
would you be sending the interrupts?

-- PMM

Re: [Qemu-devel] [PATCH] configure: fix ALSA configure test

[Blue Swirl]

On Tue, Jul 17, 2012 at 7:28 PM, Peter Maydell  wrote:
> On 17 July 2012 20:24, Stefan Weil  wrote:
>> The arguments why -Werror is a bad idea for some configure tests
>> are reasonable.
>>
>> Nevertheless the QEMU community was able to produce thousands of
>> lines of code which compile without a warning, so we should be able
>> to create warning and error free code for a handful of configure
>> tests.
>
> The trouble is that the warnings and errors here don't cause the
> build to fail noisily; that's a big distinction IMHO.
> I suppose we could make compile_prog do something like:
>  * run the compile test

Unfortunately that would break cross compiling.

>  * if it fails => test failure as now
>  * if it succeeds (and we're doing a Werror build at all),
>rerun the same test with -Werror
>  * if that fails, abort configure with an error message
> Then we would have the same "make the problem obvious" effect
> that plain -Werror provides for our main compilation.
>
>> The 4 patches above are valid and can be applied with or without
>> -Werror, therefore qemu-trivial or whoever does not have to wait for
>> Peter's patch.
>
> Yes, I agree we might as well fix these errors since we've now
> noticed them, regardless of whether or not we apply my patch
> (which I've just sent).
>
> -- PMM

Re: [Qemu-devel] [PATCHv4 0/4] Sandboxing Qemu guests with Libseccomp

[Blue Swirl]

On Mon, Jul 23, 2012 at 12:59 PM, Eduardo Otubo
 wrote:
> On Tue, Jul 17, 2012 at 04:19:11PM -0300, Eduardo Otubo wrote:
>> Hello all,
>>
>> This patch is an effort to sandbox Qemu guests using Libseccomp[0]. The 
>> patches
>> that follows are pretty simple and straightforward. I added the correct 
>> options
>> and checks to the configure script and the basic calls to libseccomp in the
>> main loop at vl.c. Details of each one are in the emails of the patch set.
>>
>> This support limits the system call footprint of the entire QEMU process to a
>> limited set of syscalls, those that we know QEMU uses. The idea is to limit 
>> the
>> allowable syscalls, therefore limiting the impact that an attacked guest 
>> could
>> have on the host system.
>>
>> It's important to note that the libseccomp itself needs the seccomp mode 2
>> feature in the kernel, which is only available in kernel versions older (or
>> equal) than 3.5-rc1.
>>
>> v2: Files separated in qemu-seccomp.c and qemu-seccomp.h for a cleaner
>> implementation. The development was tested with the 3.5-rc1 kernel.
>>
>> v3: As we discussed in previous emails in this mailing list, this feature is
>> not supposed to replace existing security feature, but add another layer 
>> to
>> the whole. The whitelist should contain all the syscalls QEMU needs. And 
>> as
>> stated by Will Drewry's commit message[1]: "Filter programs will be 
>> inherited
>> across fork/clone and execve.", the same white list should be passed 
>> along from
>> the father process to the child, then execve() shouldn't be a problem. 
>> Note
>> that there's a feature PR_SET_NO_NEW_PRIVS in seccomp mode 2 in the 
>> kernel,
>> this prevents processes from gaining privileges on execve. For example, 
>> this
>> will prevent qemu (if running unprivileged) from executing setuid 
>> programs[2].
>>
>> v4: Introducing "debug" mode on libseccomp support. The "debug" mode will set
>> the flag SCMP_ACT_TRAP when calling seccomp_start(). It will verbosely
>> print a message to the stderr in the form "seccomp: illegal system call
>> execution trapped: XXX" and resume the execution. This is really just 
>> used as
>> debug mode, it helps users and developers to full fill the whitelist.
>>
>> As always, comments are more than welcome.
>
> Hello folks,
>
> Does anyone got a chance to take a look at these? Thanks in advance :)

The patches look OK as the first step.

I think the next step (1.3?) should be to adjust the code to launch a
couple of threads with different sets of allowed system calls based on
their needs.

>
>>
>> Regards,
>>
>> [0] - http://sourceforge.net/projects/libseccomp/
>> [1] - 
>> http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=commit;h=e2cfabdfd075648216f99c2c03821cf3f47c1727
>> [2] - https://lkml.org/lkml/2012/4/12/457
>>
>> Eduardo Otubo (4):
>>   Adding support for libseccomp in configure and Makefile
>>   Adding qemu-seccomp.[ch]
>>   Adding qemu-seccomp-debug.[ch]
>>   Adding seccomp calls to vl.c
>>
>>  Makefile.objs|   10 
>>  configure|   34 ++
>>  qemu-seccomp-debug.c |   95 +
>>  qemu-seccomp-debug.h |   38 +++
>>  qemu-seccomp.c   |  126 
>> ++
>>  qemu-seccomp.h   |   22 +
>>  vl.c |   31 +
>>  7 files changed, 356 insertions(+)
>>  create mode 100644 qemu-seccomp-debug.c
>>  create mode 100644 qemu-seccomp-debug.h
>>  create mode 100644 qemu-seccomp.c
>>  create mode 100644 qemu-seccomp.h
>>
>> --
>> 1.7.9.5
>>
>
> --
> Eduardo Otubo
> Software Engineer
> Linux Technology Center
> IBM Systems & Technology Group
> Mobile: +55 19 8135 0885
> eot...@linux.vnet.ibm.com
>

Re: [Qemu-devel] question about qemu migrate.

[Sheldon]

On 07/23/2012 06:26 AM, Amos Kong wrote:

On Sun, Jul 22, 2012 at 10:59 PM, Sheldon  wrote:


(qemu) migrate -d exec:cat>/tmp/vm.out
 -incoming fd:

Load the suspend vm.out by  # qemu-kvm  incoming fd/tmp/vm.out2

Is it satisfied for you?

Thank for your patience.

Load the suspend vm.out by  # qemu-kvm  incoming fdActual, I have not do it successfully.  For I can not load the suspend 
vm.out, the hmp terminal report "load of migration failed"


When "migrate" with -b option (full copy of disk), the suspend vm.out 
will copy the source image.

So if I Load the suspend vm.out, does the qemu need the source image?




2. I execute "migrate" with -b option, seems that the "migrate" runs
at background.
I cat get the "migrate" information by qmp , there is a { "execute":
"query-migrate" } command in qmp-command.



But I can not get the the "migrate" information by hmp, there is not
a "query-migrate" command in hmp-command.

(qemu) info migrate

info commands(for hmp) always correspond with query commands(for qmp),
some query-cmds are not implemented right now (eg. query-network)


hmpqmp
------
info status -> query-status
info block  -> query-block
info pci-> query-pci


got it. Thank you again.

Re: [Qemu-devel] [PATCH v3 2/2] bitops: fix types

[Blue Swirl]

On Tue, Jul 17, 2012 at 12:36 PM, Markus Armbruster  wrote:
> Peter Maydell  writes:
>
>> On 14 July 2012 13:34, Blue Swirl  wrote:
>>> bitops.h uses inconsistently 'unsigned long' and 'int' for bit numbers.
>>>
>>> Unify to 'unsigned long' because it generates better code on x86_64.
>>> Adjust asserts accordingly.
>>
>> Still disagree with this patch, for the record.
>
> So do I.
>
> Changing tried-and-true code for some unproven performance gain is a bad
> idea.

No it isn't and that is not the case.

>
> In this particular case, it additionally deviates from the code's
> source.

I'm getting a strong feeling that it's a bad idea to reuse any Linux
kernel sources since they are seen as divine and untouchable, unlike
for example BSD queue macros. Perhaps BSD have also bit field ops
which we could use instead? There's also the licensing problem with
GPLv2only in some cases.

>
> If you feel your patch is a worthwhile improvement, please take it to
> LKML, so the kernel and future borrowers of this code can profit.
> Copying free code without at least trying to contribute improvements
> back to the source isn't proper.

The original kernel functions use 'unsigned long'. The bit field
functions introduced by Peter use 'int' but those are not in the
kernel tree, so there's nothing to fix (except the two first hunks)
from the kernel point of view. Also 'volatile' makes sense when kernel
is banging HW registers.

[Qemu-devel] [Bug 918791] Re: qemu-kvm dies when using vmvga driver and unity in the guest

[houstonbofh]

It looks like this bug is still around.  I was running a 12.04 guest
with the vmvga graphics, and on logging the emulation just dies.
Running 0.14.0+noroms-0ubuntu7+dnjl1~lucid0  I have done almost no
troubleshooting on this yet, and the vga driver works well enough for
now.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/918791

Title:
  qemu-kvm dies when using vmvga driver and unity in the guest

Status in QEMU:
  New
Status in “qemu-kvm” package in Ubuntu:
  Fix Released
Status in “xserver-xorg-video-vmware” package in Ubuntu:
  Invalid
Status in “qemu-kvm” source package in Oneiric:
  Fix Committed
Status in “xserver-xorg-video-vmware” source package in Oneiric:
  Invalid
Status in “qemu-kvm” source package in Precise:
  Fix Released
Status in “xserver-xorg-video-vmware” source package in Precise:
  Invalid

Bug description:
  =
  SRU Justification:
  1. impact: kvm crashes
  2. Development fix: don't allow attempts to set_bit to negative offsets
  3. Stable fix: same as development fix
  4. Test case (see below)
  5. Regression potential: if the patch is wrong, graphics for vmware vga over 
vnc could get messed up
  =

  12.04's qemu-kvm has been unstable for me and Marc Deslauriers and I
  figured out it has something to do with the interaction of qemu-kvm,
  unity and the vmvga driver. This is a regression over qemu-kvm in
  11.10.

  TEST CASE:
  1. start a VM that uses unity (eg, 11.04, 11.10 or 12.04). My tests use 
unity-2d on an amd64 host and amd64 guests
  2. on 11.04 and 11.10, open empathy via the messaging indicator and click 
'Chat'. On 12.04, open empathy via the messaging indicator and click 'Chat', 
close the empathy wizard, move the empathy window over the unity luancher (so 
it autohides), then do 'ctrl+alt+t' to open a terminal

  When the launcher tries to auto(un)hide, qemu-kvm dies with this:
  [10574.958149] do_general_protection: 132 callbacks suppressed
  [10574.958154] kvm[13192] general protection ip:7fab9680ea0f sp:74440148 
error:0 in qemu-system-x86_64[7fab966c4000+2c9000]

  Relevant libvirt xml:
  
    
    
  

  If I change to using 'cirrus', then qemu-kvm no longer crashes. Eg:
  
    
    
    
  

  The workaround is therefore to use the cirrus driver instead of vmvga,
  however being able to kill qemu-kvm in this manner is not ideal. Also,
  unfortunately unity-2d does not run with with cirrus driver under
  11.04, so the security and SRU teams are unable to properly test
  updates in GUI applications under unity when using the current 12.04
  qemu-kvm.

  I tried to report this via apport, but apport complained about a CRC
  error, so I could not.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/918791/+subscriptions

Re: [Qemu-devel] [PATCH V4 1/3] block: Add bdrv_are_busy()

[Luiz Capitulino]

On Mon, 23 Jul 2012 16:22:58 +0200
benoit.ca...@gmail.com wrote:

> From: Benoît Canet 
> 
> bdrv_are_busy will be used to check if any of the bs are in use
> or if one of them have a running block job.
> 
> The first user will be qmp_migrate().
> 
> Signed-off-by: Benoit Canet 
> ---
>  block.c |   13 +
>  block.h |2 ++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/block.c b/block.c
> index ce7eb8f..bc8f160 100644
> --- a/block.c
> +++ b/block.c
> @@ -4027,6 +4027,19 @@ out:
>  return ret;
>  }
>  
> +int bdrv_are_busy(void)
> +{
> +BlockDriverState *bs;
> +
> +QTAILQ_FOREACH(bs, &bdrv_states, list) {
> +if (bs->job || bdrv_in_use(bs)) {
> +return -EBUSY;
> +}
> +}

IMO, this should return true/false. The name is a bit misleading too, as it
gives the impression that are existing bdrvs are busy. I'd call it
bdrv_any_busy() or bdrv_any_in_use().

> +
> +return 0;
> +}
> +
>  void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
> int64_t speed, BlockDriverCompletionFunc *cb,
> void *opaque, Error **errp)
> diff --git a/block.h b/block.h
> index c89590d..0a3de2f 100644
> --- a/block.h
> +++ b/block.h
> @@ -337,6 +337,8 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
>  void bdrv_set_in_use(BlockDriverState *bs, int in_use);
>  int bdrv_in_use(BlockDriverState *bs);
>  
> +int bdrv_are_busy(void);
> +
>  enum BlockAcctType {
>  BDRV_ACCT_READ,
>  BDRV_ACCT_WRITE,

Re: [Qemu-devel] [PATCH 01/16] net: Add a hub net client

[Markus Armbruster]

Andreas Färber  writes:

> Hm, I'm not so much into those documents [cc'ing Blue], but we used to
> be stricter about ANSI C some years back (which iirc forbids
> non-constant expressions in initializers?). FWIW we have since switched

It doesn't, except when the variable has static storage duration.

> to C99 struct initializers and use QOM cast macros (that translate to a
> function call) in initializers. -ansi -pedantic is unlikely to get far.

Re: [Qemu-devel] [PATCH 2/2] fdc-test: Check RELATIVE SEEK beyond track 0/80

[Blue Swirl]

On Tue, Jul 17, 2012 at 9:03 AM, Pavel Hrdina  wrote:
> On 07/16/2012 04:36 PM, Kevin Wolf wrote:
>>
>> TODO This needs to be checked against a real drive
>>
>> Signed-off-by: Kevin Wolf
>> ---
>>   tests/fdc-test.c |   48 +---
>>   1 files changed, 37 insertions(+), 11 deletions(-)
>>
>> diff --git a/tests/fdc-test.c b/tests/fdc-test.c
>> index fa74411..56e745a 100644
>> --- a/tests/fdc-test.c
>> +++ b/tests/fdc-test.c
>> @@ -94,13 +94,17 @@ static uint8_t floppy_recv(void)
>>   }
>> /* pcn: Present Cylinder Number */
>> -static void ack_irq(uint8_t *pcn)
>> +static void ack_irq(uint8_t *st0, uint8_t *pcn)
>>   {
>>   uint8_t ret;
>> g_assert(get_irq(FLOPPY_IRQ));
>>   floppy_send(CMD_SENSE_INT);
>> -floppy_recv();
>> +
>> +ret = floppy_recv();
>> +if (st0 != NULL) {
>> +*st0 = ret;
>> +}
>> ret = floppy_recv();
>>   if (pcn != NULL) {
>> @@ -175,7 +179,7 @@ static void send_seek(int cyl)
>>   floppy_send(head << 2 | drive);
>>   g_assert(!get_irq(FLOPPY_IRQ));
>>   floppy_send(cyl);
>> -ack_irq(NULL);
>> +ack_irq(NULL, NULL);
>>   }
>> static uint8_t cmos_read(uint8_t reg)
>> @@ -295,29 +299,51 @@ static void test_relative_seek(void)
>>   {
>>   uint8_t drive = 0;
>>   uint8_t head = 0;
>> -uint8_t cyl = 1;
>>   uint8_t pcn;
>> +uint8_t st0;
>> /* Send seek to track 0 */
>>   send_seek(0);
>>   -/* Send relative seek to increase track by 1 */
>> +/* Send relative seek to increase track by 3 */
>>   floppy_send(CMD_RELATIVE_SEEK_IN);
>>   floppy_send(head << 2 | drive);
>>   g_assert(!get_irq(FLOPPY_IRQ));
>> -floppy_send(cyl);
>> +floppy_send(3);
>>   -ack_irq(&pcn);
>> -g_assert(pcn == 1);
>> +ack_irq(&st0, &pcn);
>> +g_assert_cmpint(pcn, ==, 3);
>> +g_assert_cmpint(st0, ==, 0x20);
>> /* Send relative seek to decrease track by 1 */
>>   floppy_send(CMD_RELATIVE_SEEK_OUT);
>>   floppy_send(head << 2 | drive);
>>   g_assert(!get_irq(FLOPPY_IRQ));
>> -floppy_send(cyl);
>> +floppy_send(1);
>> +
>> +ack_irq(&st0, &pcn);
>> +g_assert_cmpint(pcn, ==, 2);
>> +g_assert_cmpint(st0, ==, 0x20);
>> +
>> +/* Send relative seek to beyond track 0 */
>> +floppy_send(CMD_RELATIVE_SEEK_OUT);
>> +floppy_send(head << 2 | drive);
>> +g_assert(!get_irq(FLOPPY_IRQ));
>> +floppy_send(42);
>> +
>> +ack_irq(&st0, &pcn);
>> +g_assert_cmpint(pcn, ==, 0);
>> +g_assert_cmpint(st0, ==, 0x70);
>> +
>> +/* Send try relative seek to beyond track 80 */
>> +floppy_send(CMD_RELATIVE_SEEK_IN);
>> +floppy_send(head << 2 | drive);
>> +g_assert(!get_irq(FLOPPY_IRQ));
>> +floppy_send(200);
>>   -ack_irq(&pcn);
>> -g_assert(pcn == 0);
>> +ack_irq(&st0, &pcn);
>> +g_assert_cmpint(pcn, ==, 79);
>> +g_assert_cmpint(st0, ==, 0x50);
>>   }
>> /* success if no crash or abort */
>
> I tested it on the real floppy and the behavior is more complicated.

This reminds me of an idea: it could be interesting to make qtest test
real hardware, for example using a serial port to feed commands and
get responses. The same protocol which libqtest uses could be
interpreted by a boot loader level program or kernel module.

>
> Let's say that we start on the track 0 and the floppy media has 80 tracks.
>
> You send the "relative_seek_in" by 1 end the result is
> st0: 0x20
> rcn: 1.
> Then you issue the "read_id" command and you get
> st0: 0x00
> st1: 0x00
> pcn: 1.
>
> Then you send the "relative_seek_in" by 100 and the result is
> st0: 0x20
> rcn: 101
> but "read_id" returns
> st0: 0x40
> st1: 0x01
> pcn: 0.
> This probably means, that you are out of available tracks.
>
> Then you send the "relative_seek_out" by 100 and the result is
> st: 0x70
> rcn: 20
> but "read_id" returns
> st0: 0x00
> st1: 0x00
> pcn: 0.
> You return reading heads to the track 0, but the "relative_seek_out" returns
> that you are on the relative track 20.
>
> To get the floppy drive fully working, you have to send the "seek" to the
> track 0. If you don't do it, every seek to different track then the track 0
> will always seek to the track 0 and set rcn to track what you want. In this
> "buggy" state only the "relative_seek" will change the real track. The
> command "read_id" returns the real position of the reading heads. For
> example:
>
> Now you send "seek" to track 2 and you get
> st0: 0x20
> pcn: 2
> but if you send "read_id" the result is
> st0: 0x00
> st1: 0x00
> pcn: 0
>
> Pavel
>
>

Re: [Qemu-devel] [PATCH V4 2/3] qerror: Add error telling that block dev usage prevents migration

[Luiz Capitulino]

On Mon, 23 Jul 2012 16:22:59 +0200
benoit.ca...@gmail.com wrote:

> From: Benoît Canet 
> 
> Signed-off-by: Benoit Canet 
> ---
>  qerror.c |4 
>  qerror.h |3 +++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/qerror.c b/qerror.c
> index 92c4eff..d2e76ca 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -283,6 +283,10 @@ static const QErrorStringTable qerror_table[] = {
>  .desc  = "Could not set password",
>  },
>  {
> +.error_fmt = QERR_BLOCK_DEV_IN_USE_MIGRATION_PREVENTED,
> +.desc  = "Block device in use migration prevented",
> +},

Let's avoid an specific error. We have two options:

 1. Use QERR_MIGRATION_NOT_SUPPORTED

 2. Add QERR_MIGRATION_BLOCKED

> +{
>  .error_fmt = QERR_TOO_MANY_FILES,
>  .desc  = "Too many open files",
>  },
> diff --git a/qerror.h b/qerror.h
> index b4c8758..2ef40b5 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -233,6 +233,9 @@ QError *qobject_to_qerror(const QObject *obj);
>  #define QERR_SET_PASSWD_FAILED \
>  "{ 'class': 'SetPasswdFailed', 'data': {} }"
>  
> +#define QERR_BLOCK_DEV_IN_USE_MIGRATION_PREVENTED \
> +"{ 'class': 'BlockDevInUseMigrationPrevented', 'data': {} }"
> +
>  #define QERR_TOO_MANY_FILES \
>  "{ 'class': 'TooManyFiles', 'data': {} }"
>  

Re: [Qemu-devel] [PULL] pci,msi,virtio

[Alex Williamson]

On Mon, 2012-07-23 at 11:39 -0500, Anthony Liguori wrote:
> Andreas Färber  writes:
> 
> > Am 19.07.2012 17:15, schrieb Michael S. Tsirkin:
> >> The following changes since commit 
> >> 80aa796bf38b7ef21daa42673b4711510c450d8a:
> >> 
> >>   pci_bridge_dev: fix error path in pci_bridge_dev_initfn() (2012-06-11 
> >> 22:55:13 +0300)
> >> 
> >> are available in the git repository at:
> >> 
> >>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_anthony
> >> 
> >> for you to fetch changes up to 932d4a42afa28829fadf3cbfbb0507cc09aafd8b:
> >> 
> >>   msi/msix: added API to set MSI message address and data (2012-07-19 
> >> 17:56:42 +0300)
> >> 
> >> 
> >> pci,msi,virtio
> >> 
> >> This pull includes preparation patches mostly by Jan and Alex
> >> that should help merge device assignment down the road.
> >> And there's a new API needed for emulating POWER firmware.
> >> 
> >> So no new functionality and some unused APIs but it looks like
> >> merging will help people make progress.
> >> 
> >> Signed-off-by: Michael S. Tsirkin 
> >
> > Usually, PULLs are expected to carry the individual patches as replies.
> >
> > But more important, did something go wrong with rebasing before sending
> > out the PULL? June 11 is more than a month ago. And if I try to rebase
> > my pci_host branch on your "pci" branch it tries to replay loads of
> > really old post-1.1 commits (e.g., my "Pass PowerPCCPU to...") and
> > fails... am I doing something wrong? "for_anthony" tag and "pci" branch
> > seem to match in date at least.
> 
> 
> It's a tag, not a branch.
> 
> I had to add an explicit remote to pull tags in.  I'm not sure if
> there's a better way to do it in git.

Looks like it merged ok for me, but you'll need this to complete the
PCIUnregisterFunc conversion.  Not sure how this built in MST's tree.
Anthony, it'd be great if we could merge and fix this in your tree since
MST is out of the office.  Thanks,

Alex

commit 34e37573f1a983dab43673d664eb500bc87a46d4
Author: Alex Williamson 
Date:   Mon Jul 23 10:59:43 2012 -0600

esp: Update PCIUnregisterFunc function

Function prototype changed in f90c2bcd

Signed-off-by: Alex Williamson 

diff --git a/hw/esp.c b/hw/esp.c
index c6422ad..a011347 100644
--- a/hw/esp.c
+++ b/hw/esp.c
@@ -1153,13 +1153,11 @@ static int esp_pci_scsi_init(PCIDevice *dev)
 return 0;
 }
 
-static int esp_pci_scsi_uninit(PCIDevice *d)
+static void esp_pci_scsi_uninit(PCIDevice *d)
 {
 PCIESPState *pci = DO_UPCAST(PCIESPState, dev, d);
 
 memory_region_destroy(&pci->io);
-
-return 0;
 }
 
 static void esp_pci_class_init(ObjectClass *klass, void *data)

Re: [Qemu-devel] Can't Build i386-bsd-user on Freebsd

[Blue Swirl]

On Tue, Jul 17, 2012 at 2:07 AM, Paramjot Oberoi  wrote:
> Hey all,
>
> I'm having trouble building user mode BSD emulation on FreeBSD. I've tried
> 1.0.1, 1.1.1, and stable from GIT. I build by doing a: "./configure
> --target-list=i386-bsd-user", and then make with "gmake". The first error I
> get is in regards to CTLTYPE_QUAD.
>
> /usr/home/qemu-1.0.1/bsd-user/syscall.c: In function 'sysctl_oldcvt':
> /usr/home/qemu-1.0.1/bsd-user/syscall.c:214: error: 'CTLTYPE_QUAD'
> undeclared (first use in this function)
> /usr/home/qemu-1.0.1/bsd-user/syscall.c:214: error: (Each undeclared
> identifier is reported only once
> /usr/home/qemu-1.0.1/bsd-user/syscall.c:214: error: for each function it
> appears in.)
> gmake[1]: *** [syscall.o] Error 1
> gmake: *** [subdir-i386-bsd-user] Error 2
>
> To fix this error I added an #ifdef/#endif around the switch statement as
> specified in this thread:
> http://comments.gmane.org/gmane.comp.emulators.qemu/104657. Now I'm stuck
> with the following error:
>
>   CCi386-bsd-user/helper.o
>   CCi386-bsd-user/cpu.o
>   CCi386-bsd-user/disas.o
>   CCi386-bsd-user/ioport-user.o
>   LINK  i386-bsd-user/qemu-i386
> /usr/local/lib/libgthread-2.0.so: could not read symbols: File in wrong
> format

The format of the object files do not match that of the GThreads
library. Can you compile any programs with it? For example:

$ cat gthread.c
#include 
int main(void)
{
  g_thread_init(0);
  return 0;
}
$ gcc `pkg-config --cflags gthread-2.0` gthread.c -o gthread
`pkg-config --libs gthread-2.0`

> gmake[1]: *** [qemu-i386] Error 1
> gmake: *** [subdir-i386-bsd-user] Error 2
>
> Any advice on how to get it to build? I'm running 32-bit FreeBSD. Thanks in
> advance, I appreciate it .

Re: [Qemu-devel] [PATCH 12/12] raw-win32: add emulated AIO support

[Paolo Bonzini]

Il 23/07/2012 18:35, Blue Swirl ha scritto:
>> > +struct qemu_paiocb {
> QEMUPAIOCB

RawWin32AIOData. :)

>> > +BlockDriverState *bs;
>> > +HANDLE hfile;
>> > +struct iovec *aio_iov;
>> > +int aio_niov;
>> > +size_t aio_nbytes;
>> > +off_t aio_offset;
>> > +int aio_type;
>> > +};
>> > +
>> >  typedef struct BDRVRawState {
>> >  HANDLE hfile;
>> >  int type;
>> >  char drive_path[16]; /* format: "d:\" */
>> >  } BDRVRawState;
>> >
>> > +/*
>> > + * Read/writes the data to/from a given linear buffer.
>> > + *
>> > + * Returns the number of bytes handles or -errno in case of an error. 
>> > Short
>> > + * reads are only returned if the end of the file is reached.
>> > + */
>> > +static size_t handle_aiocb_rw(struct qemu_paiocb *aiocb)
>> > +{
>> > +size_t offset = 0;
>> > +int i;
>> > +
>> > +for (i = 0; i < aiocb->aio_niov; i++) {
>> > +OVERLAPPED ov;
>> > +DWORD ret, ret_count, len;
>> > +
>> > +memset(&ov, 0, sizeof(ov));
>> > +ov.Offset = (aiocb->aio_offset + offset);
>> > +ov.OffsetHigh = (aiocb->aio_offset + offset) >> 32;
>> > +len = aiocb->aio_iov[i].iov_len;
>> > +if (aiocb->aio_type & QEMU_AIO_WRITE) {
>> > +ret = WriteFile(aiocb->hfile, aiocb->aio_iov[i].iov_base,
>> > +len, &ret_count, &ov);
>> > +} else {
>> > +ret = ReadFile(aiocb->hfile, aiocb->aio_iov[i].iov_base,
>> > +   len, &ret_count, &ov);
>> > +}
>> > +if (!ret) {
>> > +ret_count = 0;
>> > +}
>> > +if (ret_count != len) {
>> > +break;
>> > +}
>> > +offset += len;
>> > +}
>> > +
>> > +return offset;
>> > +}
>> > +
>> > +static int aio_worker(void *arg)
>> > +{
>> > +struct qemu_paiocb *aiocb = arg;
>> > +ssize_t ret = 0;
>> > +size_t count;
>> > +
>> > +switch (aiocb->aio_type & QEMU_AIO_TYPE_MASK) {
>> > +case QEMU_AIO_READ:
>> > +count = handle_aiocb_rw(aiocb);
>> > +if (count < aiocb->aio_nbytes && aiocb->bs->growable) {
>> > +/* A short read means that we have reached EOF. Pad the buffer
>> > + * with zeros for bytes after EOF. */
>> > +QEMUIOVector qiov;
>> > +
>> > +qemu_iovec_init_external(&qiov, aiocb->aio_iov,
>> > + aiocb->aio_niov);
>> > +qemu_iovec_memset_skip(&qiov, 0, aiocb->aio_nbytes - count, 
>> > count);
>> > +
>> > +count = aiocb->aio_nbytes;
>> > +}
>> > +if (count == aiocb->aio_nbytes) {
>> > +ret = 0;
>> > +} else {
>> > +ret = -EINVAL;
>> > +}
>> > +break;
>> > +case QEMU_AIO_WRITE:
>> > +count = handle_aiocb_rw(aiocb);
>> > +if (count == aiocb->aio_nbytes) {
>> > +count = 0;
>> > +} else {
>> > +count = -EINVAL;
>> > +}
>> > +break;
>> > +case QEMU_AIO_FLUSH:
>> > +if (!FlushFileBuffers(aiocb->hfile)) {
>> > +return -EIO;
>> > +}
>> > +break;
>> > +default:
>> > +fprintf(stderr, "invalid aio request (0x%x)\n", aiocb->aio_type);
> Assert instead?

Yeah, this is cut-and-pasted from posix-aio-compat.c, I'll fix both.

Paolo

Re: [Qemu-devel] [PATCH] kvm: Move kvm_allows_irq0_override() to target-i386

[Jan Kiszka]

On 2012-07-23 17:19, Peter Maydell wrote:
> On 23 July 2012 13:26, Avi Kivity  wrote:
>> On 07/21/2012 11:54 AM, Peter Maydell wrote:
>>> The reason I want to get rid of common-code uses of kvm_irqchip_in_kernel()
>>> is because I think they're all similar to this -- the common code is
>>> using the check as a proxy for something else, and it should be directly
>>> asking about that something else. The only bits of code that should
>>> care about "is the irqchip in kernel?" are:
>>>  * target-specific device/machine setup code which needs to know
>>>which apic/etc to instantiate
>>>  * target-specific x86 code which has this weird synchronous IRQ
>>>delivery model for irqchip-not-in-kernel
>>> (Obviously I might have missed something, I'm flailing around
>>> trying to understand this code :-))
>>
>> Agree naming should be improved.  In fact the early series I pushed to
>> decompose local apic, ioapic, and pic, but that didn't happen.  If it
>> did we'd probably not have this conversation.
> 
> OK, let's see if we can get some agreement about naming here.
> 
> First, some test-functions I think we definitely need:
> 
>  kvm_interrupts_are_async()
>-- true if interrupt delivery is asynchronous
>   default false in kvm_init, set true in kvm_irqchip_create,
>   architectures may set it true in kvm_arch_init [ARM will
>   do so; PPC might want to do so]
> 
>  kvm_irqchip_in_kernel()
>-- the user-settable option, actual behaviour is arch specific
>   on x86, true means (as it does now) LAPIC,IOAPIC,PIT in kernel
>   on ARM, we ignore this setting and just DTRT

You should reject kernel_irqchip=off as long as you only support an
in-kernel GIC model.

>   on PPC, used as a convenience setting for whether to use
>   an in-kernel model of the interrupt controller
>   Shouldn't be used in non-target-specific code
> 
> and two I'm not quite so sure about:
> 
>  kvm_has_msi_routing()
>-- true if we can do routing of MSIs

GSI, not MSI.

>   set true only if x86 and kvm_irqchip_in_kernel

It means that the target architecture supports routing of various
interrupt sources (userspace, irqfds, in-kernel device models) to
different in-kernel IRQ sinks (CPU cores, irqchip models, whatever).
Interrupt messages via (binary-state) irqfd depend on it.

> 
>  kvm_has_irqfds()
>-- true if kernel supports IRQFDs
>   currently true only if x86 and kvm_irqchip_in_kernel

Note that this and the above are currently static feature tests, not
mode checks (i.e. they are true even if kernel_irqchip=off). The
"kvm_has" namespace is reserved for such tests.

> 
> 
> Second, current uses of kvm_irqchip_in_kernel():
> 
> hw/kvmvapic.c, hw/pc.c, hw/pc_piix.c, target-i386/kvm.c:
>  -- these are all x86 specific and can continue to use
> kvm_irqchip_in_kernel()
> 
> cpus.c:cpu_thread_is_idle()
>  -- should use !kvm_interrupts_are_async() [because halt is
> in userspace iff we're using the synchronous interrupt model]
> 
> kvm-all.c:kvm_irqchip_set_irq():
>  -- (just an assert) should be kvm_interrupts_are_async()

The name kvm_irqchip_set_irq implies so far that it injects into an
in-kernel irqchip model. Either different functions for archs that don't
follow this concept need to be provided, or this function requires
renaming (kvm_set_irq_async or so).

> 
> kvm-all.c:kvm_irqchip_add_msi_route():
>  -- should be kvm_have_msi_routing()

Only if you change the semantics of kvm_has_gsi_routing (and rename it).

> 
> kvm-all.c:kvm_irqchip_assign_irqfd():
>  -- should be true if kvm_has_irqfds()

The same issue. Plus there is that naming conflict again if we should
ever see irqfd without some in-kernel irqchip. But even s390 would have
a logical "irqchip" for me at the point it may route interrupt messages
from devices directly to the CPUs.

> 
> kvm-all.c:kvm_allows_irq0_override():
>  -- this still seems to me to be a completely x86 specific concept;
> it should move to a source file in target-x86 and then it
> can continue to use kvm_irqchip_in_kernel()
> 
> hw/virtio-pci.c:virtio_pci_set_guest_notifiers()
>  -- not entirely sure about this one but I think it
> should be testing kvm_has_msi_routing().

It depends on full irqfd support, which includes IRQ routing to allow
MSI via irqfd. Something like kvm_msi_via_irqfd_available.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [RFC][PATCH v3 1/3] configure: Add CONFIG_QEMU_LDST_OPTIMIZATION for TCG qemu_ld/st optimization

[Blue Swirl]

On Tue, Jul 17, 2012 at 2:06 AM, YeongKyoon Lee
 wrote:
> The reason why softmmu condition in configure is that softmmu is thought to 
> be a logical prerequisite of ldst optimization.
> Current patch causes compilation error if removed the condition above.
> To avoid compilation error, it needs more macros in other sources, such as, 
> tcg.c and/or tcg-target.c, because those files are used both by softmmu and 
> by non-softmmu targets.
>
> For example, tcg_out_qemu_ldst_slow_path() call site in tcg.c is wrapped only 
> by CONFIG_QEMU_LDST_OPTIMIZATION, while it calls ldst specific function 
> wrapped by CONFIG_SOFTMMU in tcg/i386/tcg-target.c.
> I'm not sure which one is better, CONFIG_SOFTMMU pre-condition in configure 
> or more those macros in tcg sources.
>
> How do you think about it?

I'd rather remove the configure check and add #ifdeffery to
tcg-target.c. This is how user mode vs. softmmu is handled now wrt.
loads and stores.

>
> --- Original Message ---
> Sender : Blue Swirl
> Date : 2012-07-14 22:13 (GMT+09:00)
> Title : Re: [Qemu-devel] [RFC][PATCH v3 1/3] configure: Add 
> CONFIG_QEMU_LDST_OPTIMIZATION for TCG qemu_ld/st optimization
>
> On Sat, Jul 14, 2012 at 10:23 AM, Yeongkyoon Lee
> wrote:
>> Enable CONFIG_QEMU_LDST_OPTIMIZATION for TCG qemu_ld/st optimization only 
>> when
>> a target uses softmmu and a host is i386 or x86_64.
>> ---
>>  configure |8 
>>  1 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 500fe24..5b39c80 100755
>> --- a/configure
>> +++ b/configure
>> @@ -3700,6 +3700,14 @@ case "$target_arch2" in
>>;;
>>  esac
>>
>> +case "$cpu" in
>> +  i386|x86_64)
>> +if [ "$target_softmmu" = "yes" ] ; then
>
> I suppose this check is not needed, user emulators will not use the
> memory access helpers or TLB.
>
>> +  echo "CONFIG_QEMU_LDST_OPTIMIZATION=y" >> $config_target_mak
>> +fi
>> +  ;;
>> +esac
>> +
>>  echo "TARGET_SHORT_ALIGNMENT=$target_short_alignment" >> $config_target_mak
>>  echo "TARGET_INT_ALIGNMENT=$target_int_alignment" >> $config_target_mak
>>  echo "TARGET_LONG_ALIGNMENT=$target_long_alignment" >> $config_target_mak
>> --
>> 1.7.4.1
>>

Re: [Qemu-devel] [PATCH 1/5] scsi-disk: removable hard disks support START/STOP

[Paolo Bonzini]

Il 23/07/2012 18:44, Blue Swirl ha scritto:
>> > Support for START/STOP UNIT right now is limited to CD-ROMs.  This is 
>> > wrong,
>> > since removable hard disks (in the real world: SD card readers) also 
>> > support
>> > it in pretty much the same way.
> I remember vaguely tuning a set of large SCSI hard disks
> (non-removable) so that they all didn't start immediately at the same
> time (which could have burned out the PSU) but only with START UNIT
> command. I think Linux or maybe even the BIOS started the drives
> (nicely in sequence) before accessing the drive.

Yes, all disks do start/stop.  Removable disks support load and eject in
addition.  I'll fix the commit message.

Paolo

Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions

[Blue Swirl]

On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost  wrote:
> On Sat, Jul 14, 2012 at 09:14:30AM +, Blue Swirl wrote:
> [...]
>> >> > diff --git a/tests/Makefile b/tests/Makefile
>> >> > index b605e14..89bd890 100644
>> >> > --- a/tests/Makefile
>> >> > +++ b/tests/Makefile
>> >> > @@ -15,6 +15,7 @@ check-unit-y += 
>> >> > tests/test-string-output-visitor$(EXESUF)
>> >> >  check-unit-y += tests/test-coroutine$(EXESUF)
>> >> >  check-unit-y += tests/test-visitor-serialization$(EXESUF)
>> >> >  check-unit-y += tests/test-iov$(EXESUF)
>> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF)
>> >>
>> >> This probably tries to build the cpuid test also for non-x86 targets
>> >> and break them all.
>> >
>> > I don't think there's any concept of "targets" for the check-unit tests.
>>
>> How about:
>> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF)
>
> test-x86-cpuid is not a qtest test case.

Why not? I don't think it is a unit test either, judging from what the
other unit tests do.

>
>>
>> > I had to do the following, to be able to make a test that uses the
>> > target-i386 code:
>> >
>> >> > +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386
>> >
>> > Any suggestions to avoid this hack would be welcome.
>>
>> Maybe it would be simpler to adjust #include path in the file.
>
> Using the full path on the #include line would break in case
> target-i386/topology.h include other files from the target-i386
> directory.

That's fragile. Maybe the target-xyz files should use the full path.

>
> --
> Eduardo

Re: [Qemu-devel] [PULL] pci,msi,virtio

[Andreas Färber]

Am 23.07.2012 18:39, schrieb Anthony Liguori:
> Andreas Färber  writes:
> 
>> Am 19.07.2012 17:15, schrieb Michael S. Tsirkin:
>>> The following changes since commit 80aa796bf38b7ef21daa42673b4711510c450d8a:
>>>
>>>   pci_bridge_dev: fix error path in pci_bridge_dev_initfn() (2012-06-11 
>>> 22:55:13 +0300)
>>>
>>> are available in the git repository at:
>>>
>>>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_anthony
>>>
>>> for you to fetch changes up to 932d4a42afa28829fadf3cbfbb0507cc09aafd8b:
>>>
>>>   msi/msix: added API to set MSI message address and data (2012-07-19 
>>> 17:56:42 +0300)
>>>
>>> 
>>> pci,msi,virtio
>>>
>>> This pull includes preparation patches mostly by Jan and Alex
>>> that should help merge device assignment down the road.
>>> And there's a new API needed for emulating POWER firmware.
>>>
>>> So no new functionality and some unused APIs but it looks like
>>> merging will help people make progress.
>>>
>>> Signed-off-by: Michael S. Tsirkin 
>>
>> Usually, PULLs are expected to carry the individual patches as replies.
>>
>> But more important, did something go wrong with rebasing before sending
>> out the PULL? June 11 is more than a month ago. And if I try to rebase
>> my pci_host branch on your "pci" branch it tries to replay loads of
>> really old post-1.1 commits (e.g., my "Pass PowerPCCPU to...") and
>> fails... am I doing something wrong? "for_anthony" tag and "pci" branch
>> seem to match in date at least.
> 
> 
> It's a tag, not a branch.

The current tag is a "subset" of the pci branch though, it seems:

http://git.kernel.org/?p=virt/kvm/mst/qemu.git;a=shortlog;h=refs/heads/pci

I could of course try to cherry-pick my patches individually rather than
using git-rebase, but backporting them so far seemed strange enough to
ask first. :)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/5] scsi-disk: removable hard disks support START/STOP

[Blue Swirl]

On Mon, Jul 16, 2012 at 2:25 PM, Paolo Bonzini  wrote:
> Support for START/STOP UNIT right now is limited to CD-ROMs.  This is wrong,
> since removable hard disks (in the real world: SD card readers) also support
> it in pretty much the same way.

I remember vaguely tuning a set of large SCSI hard disks
(non-removable) so that they all didn't start immediately at the same
time (which could have burned out the PSU) but only with START UNIT
command. I think Linux or maybe even the BIOS started the drives
(nicely in sequence) before accessing the drive.

>
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/scsi-disk.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index bcec66b..42bae3b 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -1251,7 +1251,7 @@ static int scsi_disk_emulate_start_stop(SCSIDiskReq *r)
>  bool start = req->cmd.buf[4] & 1;
>  bool loej = req->cmd.buf[4] & 2; /* load on start, eject on !start */
>
> -if (s->qdev.type == TYPE_ROM && loej) {
> +if ((s->features & (1 << SCSI_DISK_F_REMOVABLE)) && loej) {
>  if (!start && !s->tray_open && s->tray_locked) {
>  scsi_check_condition(r,
>   bdrv_is_inserted(s->qdev.conf.bs)
> --
> 1.7.10.4
>
>
>

[Qemu-devel] [PATCH] vl.c: Exit QEMU early if no machine is found

[riegamaths]

From: Dunrong Huang 

We check whether the variable machine is NULL or not before accessing
it. If machine is NULL, exit QEMU with an error, this can avoids a
segfault error.

Signed-off-by: Dunrong Huang 
---
 vl.c |   10 +-
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/vl.c b/vl.c
index 8904db1..0d145dc 100644
--- a/vl.c
+++ b/vl.c
@@ -3205,6 +3205,11 @@ int main(int argc, char **argv, char **envp)
 }
 loc_set_none();
 
+if (machine == NULL) {
+fprintf(stderr, "No machine found.\n");
+exit(1);
+}
+
 if (machine->hw_version) {
 qemu_set_version(machine->hw_version);
 }
@@ -3247,11 +3252,6 @@ int main(int argc, char **argv, char **envp)
 data_dir = CONFIG_QEMU_DATADIR;
 }
 
-if (machine == NULL) {
-fprintf(stderr, "No machine found.\n");
-exit(1);
-}
-
 /*
  * Default to max_cpus = smp_cpus, in case the user doesn't
  * specify a max_cpus value.
-- 
1.7.8.6

Re: [Qemu-devel] [PULL] pci,msi,virtio

[Anthony Liguori]

Andreas Färber  writes:

> Am 19.07.2012 17:15, schrieb Michael S. Tsirkin:
>> The following changes since commit 80aa796bf38b7ef21daa42673b4711510c450d8a:
>> 
>>   pci_bridge_dev: fix error path in pci_bridge_dev_initfn() (2012-06-11 
>> 22:55:13 +0300)
>> 
>> are available in the git repository at:
>> 
>>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_anthony
>> 
>> for you to fetch changes up to 932d4a42afa28829fadf3cbfbb0507cc09aafd8b:
>> 
>>   msi/msix: added API to set MSI message address and data (2012-07-19 
>> 17:56:42 +0300)
>> 
>> 
>> pci,msi,virtio
>> 
>> This pull includes preparation patches mostly by Jan and Alex
>> that should help merge device assignment down the road.
>> And there's a new API needed for emulating POWER firmware.
>> 
>> So no new functionality and some unused APIs but it looks like
>> merging will help people make progress.
>> 
>> Signed-off-by: Michael S. Tsirkin 
>
> Usually, PULLs are expected to carry the individual patches as replies.
>
> But more important, did something go wrong with rebasing before sending
> out the PULL? June 11 is more than a month ago. And if I try to rebase
> my pci_host branch on your "pci" branch it tries to replay loads of
> really old post-1.1 commits (e.g., my "Pass PowerPCCPU to...") and
> fails... am I doing something wrong? "for_anthony" tag and "pci" branch
> seem to match in date at least.


It's a tag, not a branch.

I had to add an explicit remote to pull tags in.  I'm not sure if
there's a better way to do it in git.

Regards,

Anthony Liguori

>
> Regards,
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3 1/2] bitops: drop volatile qualifier

[Blue Swirl]

On Mon, Jul 16, 2012 at 2:40 PM, Eric Blake  wrote:
> On 07/14/2012 06:34 AM, Blue Swirl wrote:
>> Qualifier 'volatile' is not useful for applications, it's too strict
>> for single threaded code but does not give the real atomicity guarantees
>> needed for multithreaded code.
>>
>> Drop them and now useless casts.
>>
>
>> -static inline void set_bit(int nr, volatile unsigned long *addr)
>> +static inline void set_bit(int nr, unsigned long *addr)
>>  {
>>   unsigned long mask = BIT_MASK(nr);
>> - unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
>> +unsigned long *p = addr + BIT_WORD(nr);
>
> The diff looks weird, because you are converting TAB to space on your
> affected lines but not cleaning up the neighboring lines.  Is it worth a
> preliminary patch to whitespace-sanitize things?

That is close to reformatting, this is the usual way these days.

>
> --
> Eric Blake   ebl...@redhat.com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>

Re: [Qemu-devel] [PATCH 12/12] raw-win32: add emulated AIO support

[Blue Swirl]

On Mon, Jul 16, 2012 at 10:42 AM, Paolo Bonzini  wrote:
> The thread pool can be used under Win32 in the same way as in raw-posix.c.
> Move the existing synchronous code into callbacks, and pass the return
> code back.
>
> Signed-off-by: Paolo Bonzini 
> ---
>  block/raw-win32.c |  189 
> +++--
>  1 file changed, 140 insertions(+), 49 deletions(-)
>
> diff --git a/block/raw-win32.c b/block/raw-win32.c
> index e4b0b75..a50d636 100644
> --- a/block/raw-win32.c
> +++ b/block/raw-win32.c
> @@ -25,6 +25,9 @@
>  #include "qemu-timer.h"
>  #include "block_int.h"
>  #include "module.h"
> +#include "raw-aio.h"
> +#include "trace.h"
> +#include "thread-pool.h"
>  #include 
>  #include 
>
> @@ -32,12 +35,130 @@
>  #define FTYPE_CD 1
>  #define FTYPE_HARDDISK 2
>
> +struct qemu_paiocb {

QEMUPAIOCB

> +BlockDriverState *bs;
> +HANDLE hfile;
> +struct iovec *aio_iov;
> +int aio_niov;
> +size_t aio_nbytes;
> +off_t aio_offset;
> +int aio_type;
> +};
> +
>  typedef struct BDRVRawState {
>  HANDLE hfile;
>  int type;
>  char drive_path[16]; /* format: "d:\" */
>  } BDRVRawState;
>
> +/*
> + * Read/writes the data to/from a given linear buffer.
> + *
> + * Returns the number of bytes handles or -errno in case of an error. Short
> + * reads are only returned if the end of the file is reached.
> + */
> +static size_t handle_aiocb_rw(struct qemu_paiocb *aiocb)
> +{
> +size_t offset = 0;
> +int i;
> +
> +for (i = 0; i < aiocb->aio_niov; i++) {
> +OVERLAPPED ov;
> +DWORD ret, ret_count, len;
> +
> +memset(&ov, 0, sizeof(ov));
> +ov.Offset = (aiocb->aio_offset + offset);
> +ov.OffsetHigh = (aiocb->aio_offset + offset) >> 32;
> +len = aiocb->aio_iov[i].iov_len;
> +if (aiocb->aio_type & QEMU_AIO_WRITE) {
> +ret = WriteFile(aiocb->hfile, aiocb->aio_iov[i].iov_base,
> +len, &ret_count, &ov);
> +} else {
> +ret = ReadFile(aiocb->hfile, aiocb->aio_iov[i].iov_base,
> +   len, &ret_count, &ov);
> +}
> +if (!ret) {
> +ret_count = 0;
> +}
> +if (ret_count != len) {
> +break;
> +}
> +offset += len;
> +}
> +
> +return offset;
> +}
> +
> +static int aio_worker(void *arg)
> +{
> +struct qemu_paiocb *aiocb = arg;
> +ssize_t ret = 0;
> +size_t count;
> +
> +switch (aiocb->aio_type & QEMU_AIO_TYPE_MASK) {
> +case QEMU_AIO_READ:
> +count = handle_aiocb_rw(aiocb);
> +if (count < aiocb->aio_nbytes && aiocb->bs->growable) {
> +/* A short read means that we have reached EOF. Pad the buffer
> + * with zeros for bytes after EOF. */
> +QEMUIOVector qiov;
> +
> +qemu_iovec_init_external(&qiov, aiocb->aio_iov,
> + aiocb->aio_niov);
> +qemu_iovec_memset_skip(&qiov, 0, aiocb->aio_nbytes - count, 
> count);
> +
> +count = aiocb->aio_nbytes;
> +}
> +if (count == aiocb->aio_nbytes) {
> +ret = 0;
> +} else {
> +ret = -EINVAL;
> +}
> +break;
> +case QEMU_AIO_WRITE:
> +count = handle_aiocb_rw(aiocb);
> +if (count == aiocb->aio_nbytes) {
> +count = 0;
> +} else {
> +count = -EINVAL;
> +}
> +break;
> +case QEMU_AIO_FLUSH:
> +if (!FlushFileBuffers(aiocb->hfile)) {
> +return -EIO;
> +}
> +break;
> +default:
> +fprintf(stderr, "invalid aio request (0x%x)\n", aiocb->aio_type);

Assert instead?

> +ret = -EINVAL;
> +break;
> +}
> +
> +g_slice_free(struct qemu_paiocb, aiocb);
> +return ret;
> +}
> +
> +static BlockDriverAIOCB *paio_submit(BlockDriverState *bs, HANDLE hfile,
> +int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
> +BlockDriverCompletionFunc *cb, void *opaque, int type)
> +{
> +struct qemu_paiocb *acb = g_slice_new(struct qemu_paiocb);
> +
> +acb->bs = bs;
> +acb->hfile = hfile;
> +acb->aio_type = type;
> +
> +if (qiov) {
> +acb->aio_iov = qiov->iov;
> +acb->aio_niov = qiov->niov;
> +}
> +acb->aio_nbytes = nb_sectors * 512;
> +acb->aio_offset = sector_num * 512;
> +
> +trace_paio_submit(acb, opaque, sector_num, nb_sectors, type);
> +return thread_pool_submit_aio(aio_worker, acb, cb, opaque);
> +}
> +
>  int qemu_ftruncate64(int fd, int64_t length)
>  {
>  LARGE_INTEGER li;
> @@ -109,59 +230,29 @@ static int raw_open(BlockDriverState *bs, const char 
> *filename, int flags)
>  return 0;
>  }
>
> -static int raw_read(BlockDriverState *bs, int64_t sector_num,
> -uint8_t *buf, int nb_sectors)
> +static BlockDriverAIOCB *raw_aio_readv(BlockDriverState *bs,
> + int64_t 

Re: [Qemu-devel] [PATCH 01/16] net: Add a hub net client

[Andreas Färber]

Am 23.07.2012 17:23, schrieb Stefan Hajnoczi:
> On Mon, Jul 23, 2012 at 4:16 PM, Laszlo Ersek  wrote:
>> On 07/23/12 16:52, Stefan Hajnoczi wrote:
>>> On Mon, Jul 23, 2012 at 3:05 PM, Laszlo Ersek  wrote:
>>
 The idea is, rather than

 unsigned int id = hub->num_ports++;

 it should say

 unsigned int id;
 /* ... */
 id = hub->num_ports++;
>>>
>>> "Be careful to not obfuscate the code by initializing variables in the
>>> declarations.  Use this feature only thoughtfully.  DO NOT use
>>> function calls in initializers."
>>>
>>> This?
>>>
>>> Do you know the rationale?  It's not clear to me how this "obfuscates" the 
>>> code.
>>
>> I think the rationale is that (a) people tend to reorganize definitions,
>> and the expressions in the initializer lists may depend on the original
>> order, (b) even worse with removal of variables, (c) many people have a
>> "conceptual divide" between the definition block and the logic below it,
>> and the first should set constant defaults at most. (Naturally this
>> prevents C99/C++ style mixing of definitions and code as well, at least
>> without explicit braces.)
>>
>> I'm one of those people, but again I'm not sure if qemu has any
>> guideline on this.
>>
>>
>>> Messing around with side-effects in a series of variable declarations
>>> with intializers could be bug-prone.  But here there is only one
>>> initializer so it's not a problem.
>>>
>>> Regarding QEMU, there's no coding style rule against initializers.
>>> Personally I think that's a good thing because it keeps the code
>>> concise - people reading it don't have to keep the declaration in
>>> their mind until they hit the initializer later on.
>>
>> Well I keep the definitions at the top of the block so I can very easily
>> return to them visually (and be sure they do nothing else than define
>> variables / declare externs), but it's getting philosophical :)
>>
>> I have nothing against this initializer as-is, I just wanted to voice
>> *if* there's such a guideline in qemu *then* it should be followed :)
> 
> Yes, I understand - it's a question of style or flamewar fodder :).  I
> double-checked that there is no guideline in ./CODING_STYLE and
> ./HACKING.

Hm, I'm not so much into those documents [cc'ing Blue], but we used to
be stricter about ANSI C some years back (which iirc forbids
non-constant expressions in initializers?). FWIW we have since switched
to C99 struct initializers and use QOM cast macros (that translate to a
function call) in initializers. -ansi -pedantic is unlikely to get far.

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH v2] qxl: add QXL_IO_MONITORS_CONFIG_ASYNC

[Alon Levy]

bumps spice-protocol to 0.12.0 for new IO.
revision bumped to 4 for new IO support, enabled for spice-server >= 0.11.1

RHBZ: 770842

Signed-off-by: Alon Levy 
---
v2: fixed interface_async_complete_io to not complain about unexpected async io.

 configure|2 +-
 hw/qxl.c |   30 +-
 hw/qxl.h |4 
 trace-events |1 +
 4 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index cef0a71..5fcd315 100755
--- a/configure
+++ b/configure
@@ -2630,7 +2630,7 @@ EOF
   spice_cflags=$($pkg_config --cflags spice-protocol spice-server 2>/dev/null)
   spice_libs=$($pkg_config --libs spice-protocol spice-server 2>/dev/null)
   if $pkg_config --atleast-version=0.8.2 spice-server >/dev/null 2>&1 && \
- $pkg_config --atleast-version=0.8.1 spice-protocol > /dev/null 2>&1 && \
+ $pkg_config --atleast-version=0.12.0 spice-protocol > /dev/null 2>&1 && \
  compile_prog "$spice_cflags" "$spice_libs" ; then
 spice="yes"
 libs_softmmu="$libs_softmmu $spice_libs"
diff --git a/hw/qxl.c b/hw/qxl.c
index 3a883ce..0440440 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -249,6 +249,18 @@ static void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl, 
qxl_async_io async)
 }
 }
 
+#if SPICE_SERVER_VERSION >= 0x000b01 /* 0.11.1 */
+static void qxl_spice_monitors_config_async(PCIQXLDevice *qxl)
+{
+trace_qxl_spice_monitors_config(qxl->id);
+spice_qxl_monitors_config_async(&qxl->ssd.qxl,
+qxl->ram->monitors_config,
+MEMSLOT_GROUP_GUEST,
+(uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_IO,
+  QXL_IO_MONITORS_CONFIG_ASYNC));
+}
+#endif
+
 void qxl_spice_reset_image_cache(PCIQXLDevice *qxl)
 {
 trace_qxl_spice_reset_image_cache(qxl->id);
@@ -538,6 +550,7 @@ static const char *io_port_to_string(uint32_t io_port)
 = "QXL_IO_DESTROY_ALL_SURFACES_ASYNC",
 [QXL_IO_FLUSH_SURFACES_ASYNC]   = "QXL_IO_FLUSH_SURFACES_ASYNC",
 [QXL_IO_FLUSH_RELEASE]  = "QXL_IO_FLUSH_RELEASE",
+[QXL_IO_MONITORS_CONFIG_ASYNC]  = "QXL_IO_MONITORS_CONFIG_ASYNC",
 };
 return io_port_to_string[io_port];
 }
@@ -819,6 +832,7 @@ static void interface_async_complete_io(PCIQXLDevice *qxl, 
QXLCookie *cookie)
 case QXL_IO_DESTROY_PRIMARY_ASYNC:
 case QXL_IO_UPDATE_AREA_ASYNC:
 case QXL_IO_FLUSH_SURFACES_ASYNC:
+case QXL_IO_MONITORS_CONFIG_ASYNC:
 break;
 case QXL_IO_CREATE_PRIMARY_ASYNC:
 qxl_create_guest_primary_complete(qxl);
@@ -1333,7 +1347,7 @@ static void ioport_write(void *opaque, target_phys_addr_t 
addr,
 io_port, io_port_to_string(io_port));
 /* be nice to buggy guest drivers */
 if (io_port >= QXL_IO_UPDATE_AREA_ASYNC &&
-io_port <= QXL_IO_DESTROY_ALL_SURFACES_ASYNC) {
+io_port <= QXL_IO_MONITORS_CONFIG_ASYNC) {
 qxl_send_events(d, QXL_INTERRUPT_IO_CMD);
 }
 return;
@@ -1361,6 +1375,9 @@ static void ioport_write(void *opaque, target_phys_addr_t 
addr,
 io_port = QXL_IO_DESTROY_ALL_SURFACES;
 goto async_common;
 case QXL_IO_FLUSH_SURFACES_ASYNC:
+#if SPICE_SERVER_VERSION >= 0x000b01 /* 0.11.1 */
+case QXL_IO_MONITORS_CONFIG_ASYNC:
+#endif
 async_common:
 async = QXL_ASYNC;
 qemu_mutex_lock(&d->async_lock);
@@ -1490,6 +1507,11 @@ async_common:
 d->mode = QXL_MODE_UNDEFINED;
 qxl_spice_destroy_surfaces(d, async);
 break;
+#if SPICE_SERVER_VERSION >= 0x000b01 /* 0.11.1 */
+case QXL_IO_MONITORS_CONFIG_ASYNC:
+qxl_spice_monitors_config_async(d);
+break;
+#endif
 default:
 qxl_set_guest_bug(d, "%s: unexpected ioport=0x%x\n", __func__, 
io_port);
 }
@@ -1785,9 +1807,15 @@ static int qxl_init_common(PCIQXLDevice *qxl)
 io_size = 16;
 break;
 case 3: /* qxl-3 */
+pci_device_rev = QXL_REVISION_STABLE_V10;
+io_size = 32; /* PCI region size must be pow2 */
+break;
+#if SPICE_SERVER_VERSION >= 0x000b01 /* 0.11.1 */
+case 4: /* qxl-4 */
 pci_device_rev = QXL_DEFAULT_REVISION;
 io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
 break;
+#endif
 default:
 error_report("Invalid revision %d for qxl device (max %d)",
  qxl->revision, QXL_DEFAULT_REVISION);
diff --git a/hw/qxl.h b/hw/qxl.h
index 172baf6..c1aadaa 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -128,7 +128,11 @@ typedef struct PCIQXLDevice {
 }   \
 } while (0)
 
+#if SPICE_SERVER_VERSION >= 0x000b01 /* 0.11.1 */
+#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V12
+#else
 #define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
+#endif
 
 /* qxl.c */
 void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
diff --git a/trace-events b/trace-events
index 2a5f074..3db04ad 100644
--- a/trace-events

Re: [Qemu-devel] [PATCH V4 1/3] linux-user: pass sockaddr from host to target

[Peter Maydell]

On 24 July 2012 00:49, Jing Huang  wrote:
>
> Signed-off-by: Jing Huang 
> Reviewed-by: Peter Maydell 
> ---
>  linux-user/syscall.c |   15 +--
>  1 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 539af3f..3319bb8 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1256,7 +1256,6 @@ static inline abi_long 
> host_to_target_sockaddr(abi_ulong target_addr,
>  return 0;
>  }
>
> -/* ??? Should this also swap msgh->name?  */
>  static inline abi_long target_to_host_cmsg(struct msghdr *msgh,
> struct target_msghdr *target_msgh)
>  {

This is removing the comment from the wrong function: there's
an identical one over host_to_target_cmsg() which is the one
most immediately addressed by this patch. You can remove both
comments, though.

> @@ -1873,10 +1872,22 @@ static abi_long do_sendrecvmsg(int fd, abi_ulong 
> target_msg,
>  if (!is_error(ret)) {
>  len = ret;
>  ret = host_to_target_cmsg(msgp, &msg);
> -if (!is_error(ret))
> +if (!is_error(ret)) {
> +msgp->msg_namelen = tswap32(msg.msg_namelen);
> +if (msg.msg_name != NULL) {
> +ret = host_to_target_sockaddr(tswapal(msgp->msg_name),
> +msg.msg_name, 
> msg.msg_namelen);
> +if (ret) {
> +goto out;
> +}
> +}
> +
>  ret = len;
> +}
>  }
>  }
> +
> +out:
>  unlock_iovec(vec, target_vec, count, !send);
>  unlock_user_struct(msgp, target_msg, send ? 0 : 1);
>  return ret;
> --
> 1.7.8.6
>

-- PMM

Re: [Qemu-devel] [QEMU PATCH 0/2] virtio-blk: writeback cache enable improvements

[Paolo Bonzini]

Il 03/07/2012 15:20, Paolo Bonzini ha scritto:
> These patches let virtio-blk use the new support for toggling the cache
> mode between writethrough and writeback.
> 
> The first patch introduces a new feature bit and configuration field to
> do this.  The second patch disables writeback caching for guests that do
> not negotiate VIRTIO_BLK_F_WCACHE (meaning that they cannot send flush
> requests), so that limited or older guests are now safe wrt power losses.
> VIRTIO_BLK_F_FLUSH has been introduced in Linux 2.6.32 (in 2009) and was
> backported to RHEL/CentOS 5.6 (in 2010).
> 
> The Windows drivers (which work by emulating SCSI on top of virtio-blk)
> have bugs in this area, which I reported on the Red Hat Bugzilla as
> bugs 837321 and 837324.  With these patches they will suffer a
> performance hit but gain correctness.
> 
> Paolo Bonzini (2):
>   virtio-blk: support VIRTIO_BLK_F_CONFIG_WCE
>   virtio-blk: disable write cache if not negotiated

Ping - Anthony, mst?

Paolo

Re: [Qemu-devel] [PATCH V4 3/3] linux-user: make host_to_target_cmsg support SO_TIMESTAMP cmsg_type

[Peter Maydell]

On 24 July 2012 00:52, Jing Huang  wrote:
>
> Signed-off-by: Jing Huang 
> ---
>  linux-user/syscall.c |   20 
>  1 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 9b498d0..ce70459 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1348,16 +1348,28 @@ static inline abi_long host_to_target_cmsg(struct 
> target_msghdr *target_msgh,
>  target_cmsg->cmsg_type = tswap32(cmsg->cmsg_type);
>  target_cmsg->cmsg_len = tswapal(TARGET_CMSG_LEN(len));
>
> -if (cmsg->cmsg_level != TARGET_SOL_SOCKET || cmsg->cmsg_type != 
> SCM_RIGHTS) {
> -gemu_log("Unsupported ancillary data: %d/%d\n", 
> cmsg->cmsg_level, cmsg->cmsg_type);
> -memcpy(target_data, data, len);
> -} else {
> +if ((cmsg->cmsg_level == TARGET_SOL_SOCKET) &&
> +(cmsg->cmsg_type == SCM_RIGHTS)) {
>  int *fd = (int *)data;
>  int *target_fd = (int *)target_data;
>  int i, numfds = len / sizeof(int);
>
>  for (i = 0; i < numfds; i++)
>  target_fd[i] = tswap32(fd[i]);
> +} else if ((cmsg->cmsg_level == TARGET_SOL_SOCKET) &&
> +(cmsg->cmsg_type == SO_TIMESTAMP) &&
> +(len == sizeof(struct timeval))) {
> +/* copy struct timeval to target */
> +struct timeval *tv = (struct timeval *)data;
> +struct target_timeval *target_tv =
> +(struct target_timeval *)target_data;
> +
> +tv->tv_sec = tswapl(target_tv->tv_sec);
> +tv->tv_usec = tswapl(target_tv->tv_usec);

This is the wrong way round -- the source is in tv and you need
to fill in target_tv.

(This bug means that the Ubuntu x86-64 ping binary complains:
"Warning: time of day goes back (-1336371523147153us), taking countermeasures."
because the first time round it gets garbage rather than a valid timestamp.)

Looks OK otherwise.

-- PMM

Re: [Qemu-devel] [PATCH] eepro100: fix simplified mode

[Stefan Weil]

Am 23.07.2012 11:25, schrieb initcr...@gmail.com:

From: Christian Schilling 

A driver using simplified mode that works on real hardware
did not work in qemu.

Signed-off-by: Christian Schilling 
---
  hw/eepro100.c |7 +++
  1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 6279ae3..4a48372 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -774,6 +774,13 @@ static void tx_command(EEPRO100State *s)
  #if 0
  uint16_t tx_buffer_el = lduw_le_pci_dma(&s->dev, tbd_address + 6);
  #endif
+if (tbd_array == 0x) {
+/* In simpliyfied mode there is no tbd_array. Instead the packet 
data
+ * starts right after the tcb_bytes field, and the packet size is
+ * equal to tcb_bytes */
+tx_buffer_size = tcb_bytes;
+tx_buffer_address = tbd_address;
+}
  tbd_address += 8;
  TRACE(RXTX, logout
  ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n",


Do you really think that's a trivial patch?

I have a different fix for simplified mode in my QEMU tree:

http://repo.or.cz/w/qemu/ar7.git/blob/HEAD:/hw/eepro100.c

That version is implemented according to the Intel specifications
and avoids hacks for specific guest drivers.

Maybe you can give it a try.

If it works for you, I can put the changes needed for simplified mode
in a patch for QEMU git master.

Regards,

Stefan Weil

Re: [Qemu-devel] [PATCH 4/8] qemu-option: add alias support

[Luiz Capitulino]

On Sat, 21 Jul 2012 10:11:39 +0200
Markus Armbruster  wrote:

> Luiz Capitulino  writes:
> 
> > Allow for specifying an alias for each option name, see next commits
> > for examples.
> >
> > Signed-off-by: Luiz Capitulino 
> > ---
> >  qemu-option.c | 5 +++--
> >  qemu-option.h | 1 +
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/qemu-option.c b/qemu-option.c
> > index 65ba1cf..b2f9e21 100644
> > --- a/qemu-option.c
> > +++ b/qemu-option.c
> > @@ -623,7 +623,8 @@ static const QemuOptDesc *find_desc_by_name(const 
> > QemuOptDesc *desc,
> >  int i;
> >  
> >  for (i = 0; desc[i].name != NULL; i++) {
> > -if (strcmp(desc[i].name, name) == 0) {
> > +if (strcmp(desc[i].name, name) == 0 ||
> > +(desc[i].alias && strcmp(desc[i].alias, name) == 0)) {
> >  return &desc[i];
> >  }
> >  }
> > @@ -645,7 +646,7 @@ static void opt_set(QemuOpts *opts, const char *name, 
> > const char *value,
> >  }
> >  
> >  opt = g_malloc0(sizeof(*opt));
> > -opt->name = g_strdup(name);
> > +opt->name = g_strdup(desc ? desc->name : name);
> >  opt->opts = opts;
> >  if (prepend) {
> >  QTAILQ_INSERT_HEAD(&opts->head, opt, next);
> 
> Are you sure this hunk belongs to this patch?  If yes, please explain
> why :)

Yes, I think it's fine because the change that makes it necessary to choose
between desc->name and name is introduced by the hunk above.

Of course that it's possible to move this to a separate patch, but I don't
think it's worth it, as name is always valid with the current code.

> 
> > diff --git a/qemu-option.h b/qemu-option.h
> > index 951dec3..7106d2f 100644
> > --- a/qemu-option.h
> > +++ b/qemu-option.h
> > @@ -94,6 +94,7 @@ enum QemuOptType {
> >  
> >  typedef struct QemuOptDesc {
> >  const char *name;
> > +const char *alias;
> >  enum QemuOptType type;
> >  const char *help;
> >  } QemuOptDesc;
> 

Re: [Qemu-devel] [PULL] pci,msi,virtio

[Andreas Färber]

Am 19.07.2012 17:15, schrieb Michael S. Tsirkin:
> The following changes since commit 80aa796bf38b7ef21daa42673b4711510c450d8a:
> 
>   pci_bridge_dev: fix error path in pci_bridge_dev_initfn() (2012-06-11 
> 22:55:13 +0300)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_anthony
> 
> for you to fetch changes up to 932d4a42afa28829fadf3cbfbb0507cc09aafd8b:
> 
>   msi/msix: added API to set MSI message address and data (2012-07-19 
> 17:56:42 +0300)
> 
> 
> pci,msi,virtio
> 
> This pull includes preparation patches mostly by Jan and Alex
> that should help merge device assignment down the road.
> And there's a new API needed for emulating POWER firmware.
> 
> So no new functionality and some unused APIs but it looks like
> merging will help people make progress.
> 
> Signed-off-by: Michael S. Tsirkin 

Usually, PULLs are expected to carry the individual patches as replies.

But more important, did something go wrong with rebasing before sending
out the PULL? June 11 is more than a month ago. And if I try to rebase
my pci_host branch on your "pci" branch it tries to replay loads of
really old post-1.1 commits (e.g., my "Pass PowerPCCPU to...") and
fails... am I doing something wrong? "for_anthony" tag and "pci" branch
seem to match in date at least.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/8] qemu-option: qemu_opt_set_bool(): fix code duplication

[Luiz Capitulino]

On Sat, 21 Jul 2012 10:09:09 +0200
Markus Armbruster  wrote:

> Luiz Capitulino  writes:
> 
> > Call qemu_opt_set() instead of duplicating opt_set().
> >
> > Signed-off-by: Luiz Capitulino 
> > ---
> >  qemu-option.c | 28 +---
> >  1 file changed, 1 insertion(+), 27 deletions(-)
> >
> > diff --git a/qemu-option.c b/qemu-option.c
> > index bb3886c..2cb2835 100644
> > --- a/qemu-option.c
> > +++ b/qemu-option.c
> > @@ -677,33 +677,7 @@ void qemu_opt_set_err(QemuOpts *opts, const char
> > *name, const char *value,
> >  
> >  int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
> >  {
> > -QemuOpt *opt;
> > -const QemuOptDesc *desc = opts->list->desc;
> > -int i;
> > -
> > -for (i = 0; desc[i].name != NULL; i++) {
> > -if (strcmp(desc[i].name, name) == 0) {
> > -break;
> > -}
> > -}
> > -if (desc[i].name == NULL) {
> > -if (i == 0) {
> > -/* empty list -> allow any */;
> > -} else {
> > -qerror_report(QERR_INVALID_PARAMETER, name);
> > -return -1;
> > -}
> > -}
> > -
> > -opt = g_malloc0(sizeof(*opt));
> > -opt->name = g_strdup(name);
> > -opt->opts = opts;
> > -QTAILQ_INSERT_TAIL(&opts->head, opt, next);
> > -if (desc[i].name != NULL) {
> > -opt->desc = desc+i;
> > -}
> > -opt->value.boolean = !!val;
> > -return 0;
> > +return qemu_opt_set(opts, name, val ? "on" : "off");
> >  }
> >  
> >  int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
> 
> Does a bit more than just obvious code de-duplication.  Two things in
> particular:
> 
> * Error reporting
> 
>   Old: qerror_report(); return -1
> 
>   New: opt_set() and qemu_opt_set() cooperate, like this:
>opt_set(): error_set(); return;
>qemu_opt_set():
> if (error_is_set(&local_err)) {
> qerror_report_err(local_err);
> error_free(local_err);
> return -1;
> }
> 
>   I guess the net effect is the same.  Not sure it's worth mentioning in
>   the commit message.

The end result of calling qemu_opt_set_bool() is the same. The difference
between qerror_report() and qerror_report_err() is that the former gets error
information from the error call, while the latter gets error information
from the Error object. But they do the same thing.

> * New sets opt->str to either "on" or "off" depending on val, then lets
>   reconstructs the value with qemu_opt_parse().  Which can't fail then.
>   I figure the latter part has no further impact.  But what about
>   setting opt->str?  Is this a bug fix?

I don't remember if opt->str is read after the QemuOpt object is built. If
it's, then yes, this is a bug fix. Otherwise it just make the final
QemuOpt object more 'conforming'.

[Qemu-devel] [PATCH V4 3/3] linux-user: make host_to_target_cmsg support SO_TIMESTAMP cmsg_type

[Jing Huang]

Signed-off-by: Jing Huang 
---
 linux-user/syscall.c |   20 
 1 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 9b498d0..ce70459 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1348,16 +1348,28 @@ static inline abi_long host_to_target_cmsg(struct 
target_msghdr *target_msgh,
 target_cmsg->cmsg_type = tswap32(cmsg->cmsg_type);
 target_cmsg->cmsg_len = tswapal(TARGET_CMSG_LEN(len));
 
-if (cmsg->cmsg_level != TARGET_SOL_SOCKET || cmsg->cmsg_type != 
SCM_RIGHTS) {
-gemu_log("Unsupported ancillary data: %d/%d\n", cmsg->cmsg_level, 
cmsg->cmsg_type);
-memcpy(target_data, data, len);
-} else {
+if ((cmsg->cmsg_level == TARGET_SOL_SOCKET) &&
+(cmsg->cmsg_type == SCM_RIGHTS)) {
 int *fd = (int *)data;
 int *target_fd = (int *)target_data;
 int i, numfds = len / sizeof(int);
 
 for (i = 0; i < numfds; i++)
 target_fd[i] = tswap32(fd[i]);
+} else if ((cmsg->cmsg_level == TARGET_SOL_SOCKET) &&
+(cmsg->cmsg_type == SO_TIMESTAMP) &&
+(len == sizeof(struct timeval))) {
+/* copy struct timeval to target */
+struct timeval *tv = (struct timeval *)data;
+struct target_timeval *target_tv =
+(struct target_timeval *)target_data;
+
+tv->tv_sec = tswapl(target_tv->tv_sec);
+tv->tv_usec = tswapl(target_tv->tv_usec);
+} else {
+gemu_log("Unsupported ancillary data: %d/%d\n",
+cmsg->cmsg_level, cmsg->cmsg_type);
+memcpy(target_data, data, len);
 }
 
 cmsg = CMSG_NXTHDR(msgh, cmsg);
-- 
1.7.8.6

[Qemu-devel] [PATCH V4 2/3] linux-user: make do_setsockopt support SOL_RAW ICMP_FILTER socket option

[Jing Huang]

Signed-off-by: Jing Huang 
Reviewed-by: Peter Maydell 
---
 linux-user/syscall.c |   20 
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 3319bb8..9b498d0 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -60,6 +60,7 @@ int __clone2(int (*fn)(void *), void *child_stack_base,
 #include 
 #include 
 #include 
+#include 
 #include "qemu-common.h"
 #ifdef TARGET_GPROF
 #include 
@@ -1441,6 +1442,25 @@ static abi_long do_setsockopt(int sockfd, int level, int 
optname,
 goto unimplemented;
 }
 break;
+case SOL_RAW:
+switch (optname) {
+case ICMP_FILTER:
+/* struct icmp_filter takes an u32 value */
+if (optlen < sizeof(uint32_t)) {
+return -TARGET_EINVAL;
+}
+
+if (get_user_u32(val, optval_addr)) {
+return -TARGET_EFAULT;
+}
+ret = get_errno(setsockopt(sockfd, level, optname,
+   &val, sizeof(val)));
+break;
+
+default:
+goto unimplemented;
+}
+break;
 case TARGET_SOL_SOCKET:
 switch (optname) {
 /* Options with 'int' argument.  */
-- 
1.7.8.6

[Qemu-devel] [PATCH V4 1/3] linux-user: pass sockaddr from host to target

[Jing Huang]

Signed-off-by: Jing Huang 
Reviewed-by: Peter Maydell 
---
 linux-user/syscall.c |   15 +--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 539af3f..3319bb8 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1256,7 +1256,6 @@ static inline abi_long host_to_target_sockaddr(abi_ulong 
target_addr,
 return 0;
 }
 
-/* ??? Should this also swap msgh->name?  */
 static inline abi_long target_to_host_cmsg(struct msghdr *msgh,
struct target_msghdr *target_msgh)
 {
@@ -1873,10 +1872,22 @@ static abi_long do_sendrecvmsg(int fd, abi_ulong 
target_msg,
 if (!is_error(ret)) {
 len = ret;
 ret = host_to_target_cmsg(msgp, &msg);
-if (!is_error(ret))
+if (!is_error(ret)) {
+msgp->msg_namelen = tswap32(msg.msg_namelen);
+if (msg.msg_name != NULL) {
+ret = host_to_target_sockaddr(tswapal(msgp->msg_name),
+msg.msg_name, msg.msg_namelen);
+if (ret) {
+goto out;
+}
+}
+
 ret = len;
+}
 }
 }
+
+out:
 unlock_iovec(vec, target_vec, count, !send);
 unlock_user_struct(msgp, target_msg, send ? 0 : 1);
 return ret;
-- 
1.7.8.6

[Qemu-devel] [PATCH V4 0/3] Fix ping issues in linux-user guest

[Jing Huang]

The following changes since commit 61dc008f3529fa74a63aad1907438dad857e255a:
Revert "audio: Make PC speaker audio card available by default"

These patches fix ping issues in linux-user guest:

- do_sebdrecv() could not pass sockaddr to target after executing
recvmsg(). Therefore, ping will shows: "64 bytes from 0.0.0.0:
icmp_req=1 ttl=64 time=1.16 ms" for example.

- do_setsockopt() do not support SOL_RAW option which which is used in
ping program.

- host_to_target_cmsg() report unsupported ancillary data qemu-log
message when cmsg_type equals SO_TIMESTAMP.

Please review,

Jing.


Reviewed-by: Peter Maydell 

*** BLURB HERE ***

Jing Huang (3):
  linux-user: pass sockaddr from host to target
  linux-user: make do_setsockopt support SOL_RAW ICMP_FILTER socket
option
  linux-user: make host_to_target_cmsg support SO_TIMESTAMP cmsg_type

 linux-user/syscall.c |   55 -
 1 files changed, 49 insertions(+), 6 deletions(-)

-- 
1.7.8.6