Re: [Qemu-devel] [PATCH 0/8] Add GTK UI to enable basic accessibility (v2)

[Jan Kiszka]

On 2012-02-27 00:46, Anthony Liguori wrote:
> I realize UIs are the third rail of QEMU development, but over the years I've
> gotten a lot of feedback from users about our UI.  I think everyone struggles
> with the SDL interface and its lack of discoverability but it's worse than I
> think most people realize for users that rely on accessibility tools.
> 
> The two pieces of feedback I've gotten the most re: accessibility are the lack
> of QEMU's enablement for screen readers and the lack of configurable
> accelerators.
> 
> Since we render our own terminal using a fixed sized font, we don't respect
> system font settings which means we ignore if the user has configured large
> print.
> 
> We also don't integrate at all with screen readers which means that for blind
> users, the virtual consoles may as well not even exist.
> 
> We also don't allow any type of configuration of accelerators.  For users with
> limited dexterity (this is actually more common than you would think), they 
> may
> use an input device that only inputs one key at a time.  Holding down two keys
> at once is not possible for these users.
> 
> These are solved problems though and while we could reinvent all of this
> ourselves with SDL, we would be crazy if we did.  Modern toolkits, like GTK,
> solve these problems.
> 
> By using GTK, we can leverage VteTerminal for screen reader integration and 
> font
> configuration.  We can also use GTK's accelerator support to make accelerators
> configurable (Gnome provides a global accelerator configuration interface).
> 
> I'm not attempting to make a pretty desktop virtualization UI.  Maybe we'll go
> there eventually but that's not what this series is about.
> 
> This is just attempting to use a richer toolkit such that we can enable basic
> accessibility support.  As a consequence, the UI is much more usable even for 
> a
> user without accessibility requirements so it's a win-win.
> 
> Also available at:
> 
> https://github.com/aliguori/qemu/tree/gtk.2
> 
> ---
> v1 -> v2
>  - Add internationalization support.  I don't actually speak any other 
> languages
>so I added a placeholder for a German translation.  This can be tested with
>LANGUAGE=de_DE.UTF-8 qemu-system-x86_64
>  - Fixed the terminal size for VteTerminal widgets.  I think the behavior 
> makes
>sense now.
>  - Fixed lots of issues raised in review comments (see individual patches)
> 
> Known Issues:
>  - I saw the X crash once.  I think it has to do with widget sizes.  I need to
>work harder to reproduce.
>  - I've not recreated the reported memory leak yet.
>  - I haven't added backwards compatibility code for older VteTerminal widgets
>yet.

Looks quite nice but still has some rough edges:
- full screen doesn't work, at least here
- lacking support for auto-grabbing in absolute mouse mode
- unscaling (ctrl-alt-u) is lacking
- window not resizable (except in broken full-screen mode)

Will see if I find some time to look into this.

Is this also working properly under Windows? Otherwise we probably can't
deprecate SDL - or would have to provide a native Windows GUI.

As we have a menu now, I would suggest to add some handy monitor
commands there as well, like reset or powerdown.

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 2/3] Allow larger return values from get_image_size()

[Markus Armbruster]

David Gibson  writes:

> Currently get_image_size(), used to find the size of files, returns an int.
> But for modern systems, int may be only 32-bit and we can have files
> larger than that.
>
> This patch, therefore, changes the return type of get_image_size() to off_t
> (the same as the return type from lseek() itself).  It also audits all the
> callers of get_image_size() to make sure they process the new unsigned
> return type correctly.
>
> This leaves load_image_targphys() with a limited return type, but one thing
> at a time (that function has far more callers to be audited, so it will
> take longer to fix).

I'm afraid this replaces the single, well-known integer overflow in
get_image_size()'s conversion of lseek() value to int by many unknown
overflows in get_image_size()'s users.  One example below.  Didn't look
for more.

If you need a wider get_image_size(), please make sure its users are
prepared for it!

Is the any use for image sizes exceeding size_t?  Arent such images
impossible to load?

[...]
> diff --git a/hw/pc.c b/hw/pc.c
> index b9f4bc7..cb41955 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -672,7 +672,8 @@ static void load_linux(void *fw_cfg,
> target_phys_addr_t max_ram_size)
>  {
>  uint16_t protocol;
> -int setup_size, kernel_size, initrd_size = 0, cmdline_size;
> +int setup_size, kernel_size, cmdline_size;
> +off_t initrd_size = 0;
>  uint32_t initrd_max;
>  uint8_t header[8192], *setup, *kernel, *initrd_data;
>  target_phys_addr_t real_addr, prot_addr, cmdline_addr, initrd_addr = 0;
> @@ -795,7 +796,7 @@ static void load_linux(void *fw_cfg,
>   }
>  
>   initrd_size = get_image_size(initrd_filename);
> -if (initrd_size < 0) {
> +if (initrd_size == -1) {

Needless churn.

>  fprintf(stderr, "qemu: error reading initrd %s\n",
>  initrd_filename);
>  exit(1);
   }

   initrd_addr = (initrd_max-initrd_size) & ~4095;

   initrd_data = g_malloc(initrd_size);

Integer overflow in conversion from off_t initrd_size to the argument
type size_t[*].

   load_image(initrd_filename, initrd_data);

You could try replacing load_image() by a function that returns a
pointer to the malloced image contents.

[...]


[*] Technically some glib-defined type, because they're into making up
their own "portable" types for everything.

Re: [Qemu-devel] [PATCH 7/8] gtk: add translation support

[Paolo Bonzini]

On 02/27/2012 12:46 AM, Anthony Liguori wrote:
> The de_DE translation is just a placeholder so that I could test the
> infrastructure.

Here is an it_IT translation that you can use instead.

Paolo
# Italian translation for QEMU.
# This file is put in the public domain.
# Paolo Bonzini , 2012.
#
msgid ""
msgstr ""
"Project-Id-Version: QEMU 1.0.50\n"
"Report-Msgid-Bugs-To: qemu-devel@nongnu.org\n"
"POT-Creation-Date: 2012-02-26 11:30-0600\n"
"PO-Revision-Date: 2012-02-27 08:23+0100\n"
"Last-Translator: Paolo Bonzini \n"
"Language-Team: Italian \n"
"Language: \n"
"MIME-Version: 1.0\n"
"Content-Type: text/plain; charset=UTF-8\n"
"Content-Transfer-Encoding: 8bit\n"
"Plural-Forms: nplurals=2; plural=n != 1;\n"

#: ../ui/gtk.c:769
msgid "_File"
msgstr "_File"

#: ../ui/gtk.c:779
msgid "_View"
msgstr "_Visualizza"

#: ../ui/gtk.c:781
msgid "_Full Screen"
msgstr "_Schermo intero"

#: ../ui/gtk.c:805
msgid "_Grab Input"
msgstr "_Cattura input"

#: ../ui/gtk.c:831
msgid "Show _Tabs"
msgstr "Mostra _tab"

Re: [Qemu-devel] [PATCH] qed: replace vm_clock with rt_clock for qemu-tool compatibility

[Paolo Bonzini]

On 02/27/2012 08:35 AM, Zhi Yong Wu wrote:
> Since vm_clock is created via qemu_init_main_loop(), when QED read
> vm_clock, why will this call abort()?
> Can you elaborate this? what is its call path?
> 

It will crash in cpu_get_clock() (in qemu-tool.c).

Paolo

Re: [Qemu-devel] [PATCH] qed: replace vm_clock with rt_clock for qemu-tool compatibility

[Zhi Yong Wu]

On Mon, Feb 27, 2012 at 4:42 PM, Paolo Bonzini  wrote:
> On 02/27/2012 08:35 AM, Zhi Yong Wu wrote:
>> Since vm_clock is created via qemu_init_main_loop(), when QED read
>> vm_clock, why will this call abort()?
>> Can you elaborate this? what is its call path?
>>
>
> It will crash in cpu_get_clock() (in qemu-tool.c).
nice, got it. thanks.
>
> Paolo



-- 
Regards,

Zhi Yong Wu

Re: [Qemu-devel] [PATCH 2/3] Allow larger return values from get_image_size()

[David Gibson]

On Mon, Feb 27, 2012 at 09:21:25AM +0100, Markus Armbruster wrote:
> David Gibson  writes:
> 
> > Currently get_image_size(), used to find the size of files, returns an int.
> > But for modern systems, int may be only 32-bit and we can have files
> > larger than that.
> >
> > This patch, therefore, changes the return type of get_image_size() to off_t
> > (the same as the return type from lseek() itself).  It also audits all the
> > callers of get_image_size() to make sure they process the new unsigned
> > return type correctly.
> >
> > This leaves load_image_targphys() with a limited return type, but one thing
> > at a time (that function has far more callers to be audited, so it will
> > take longer to fix).
> 
> I'm afraid this replaces the single, well-known integer overflow in
> get_image_size()'s conversion of lseek() value to int by many unknown
> overflows in get_image_size()'s users.  One example below.  Didn't look
> for more.
> 
> If you need a wider get_image_size(), please make sure its users are
> prepared for it!

Actually, I have no such need at all, but when I fixed another bug in
loader.c, someone whinged about me not changing get_image_size(), so
here it is.

> Is the any use for image sizes exceeding size_t?  Arent such images
> impossible to load?

Well, possibly not.

> 
> [...]
> > diff --git a/hw/pc.c b/hw/pc.c
> > index b9f4bc7..cb41955 100644
> > --- a/hw/pc.c
> > +++ b/hw/pc.c
> > @@ -672,7 +672,8 @@ static void load_linux(void *fw_cfg,
> > target_phys_addr_t max_ram_size)
> >  {
> >  uint16_t protocol;
> > -int setup_size, kernel_size, initrd_size = 0, cmdline_size;
> > +int setup_size, kernel_size, cmdline_size;
> > +off_t initrd_size = 0;
> >  uint32_t initrd_max;
> >  uint8_t header[8192], *setup, *kernel, *initrd_data;
> >  target_phys_addr_t real_addr, prot_addr, cmdline_addr, initrd_addr = 0;
> > @@ -795,7 +796,7 @@ static void load_linux(void *fw_cfg,
> > }
> >  
> > initrd_size = get_image_size(initrd_filename);
> > -if (initrd_size < 0) {
> > +if (initrd_size == -1) {
> 
> Needless churn.

No, it's not.  Now that initrd_size is unsigned initrd_size < 0 would
return false always (and give a "comparison is always false due to
limited range of data type" warning).

> 
> >  fprintf(stderr, "qemu: error reading initrd %s\n",
> >  initrd_filename);
> >  exit(1);
>}
> 
>initrd_addr = (initrd_max-initrd_size) & ~4095;
> 
>initrd_data = g_malloc(initrd_size);
> 
> Integer overflow in conversion from off_t initrd_size to the argument
> type size_t[*].

Hm, true.

Ok, well, I give up.  Someone who actually needs it can fix it.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH]qemu: deal with guest paniced event

[Jan Kiszka]

On 2012-02-27 04:05, Wen Congyang wrote:
> When the host knows the guest is paniced, it will set
> exit_reason to KVM_EXIT_GUEST_PANIC. So if qemu receive
> this exit_reason, we can send a event to tell management
> application that the guest is paniced.
> 
> Signed-off-by: Wen Congyang 
> ---
>  kvm-all.c |3 +++
>  linux-headers/linux/kvm.h |1 +
>  monitor.c |3 +++
>  monitor.h |1 +
>  4 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index c4babda..ae428ab 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1190,6 +1190,9 @@ int kvm_cpu_exec(CPUState *env)
>  (uint64_t)run->hw.hardware_exit_reason);
>  ret = -1;
>  break;
> +case KVM_EXIT_GUEST_PANIC:
> +monitor_protocol_event(QEVENT_GUEST_PANICED, NULL);
> +break;
>  case KVM_EXIT_INTERNAL_ERROR:
>  ret = kvm_handle_internal_error(env, run);
>  break;
> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> index f6b5343..45dd031 100644
> --- a/linux-headers/linux/kvm.h
> +++ b/linux-headers/linux/kvm.h
> @@ -163,6 +163,7 @@ struct kvm_pit_config {
>  #define KVM_EXIT_OSI  18
>  #define KVM_EXIT_PAPR_HCALL19
>  #define KVM_EXIT_S390_UCONTROL 20
> +#define KVM_EXIT_GUEST_PANIC   21
>  
>  /* For KVM_EXIT_INTERNAL_ERROR */
>  #define KVM_INTERNAL_ERROR_EMULATION 1

linux-headers are supposed to be synchronized in a separate patch,
naming the upstream or kvm.git hash they pull in. IOW: the KVM ABI
change has to be applied first.

Jan



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] SPARC64: unable to boot OpenBIOS from git master

[Mark Cave-Ayland]

Hi all,

I've been experimenting with SPARC64 under QEMU, and with current git 
master I am unable to boot OpenBIOS at all with the following error:


OpenBIOS for Sparc64
Unhandled Exception 0x0032
PC = 0xffd19d84 NPC = 0xffd19d88
Stopping execution

Using git bisect indicates that the problem lies with the following commit:


commit d5f27e88699f14c802d66c01de70e5ea37b7153a
Author: Michael S. Tsirkin 
Date:   Tue Feb 21 15:57:58 2012 +0200

pci: set memory type for memory behind the bridge

As we make upper bits in IO and prefetcheable memory
registers writeable, we should declare support
for 64 bit prefetcheable memory and 32 bit io
in the bridge.

This changes the default for apb, dec, but I'm guessing
they got the defaults wrong by accident.
Alternatively, we could let bridges declare lack of
64 bit support and make the upper bits read-only zero.

With this applied, we can drop these bits
from express code.

Reported-by: Gerd Hoffmann 
Signed-off-by: Michael S. Tsirkin 

Could someone familiar with apb,dec ack this please?
Signed-off-by: Anthony Liguori 


Does anyone have an idea as to whether this is something that needs to 
be fixed in QEMU or OpenBIOS?



Many thanks,

Mark.

Re: [Qemu-devel] [PATCH v6 0/7] qxl: fix hangs caused by qxl_render_update

[Gerd Hoffmann]

On 02/24/12 22:19, Alon Levy wrote:
> v5->v6:
>  rebased
>  dropped vga/console patches
>  addressed checkpatch complaints
>  
> Alon Levy (7):
>   qxl: fix spice+sdl no cursor regression
>   sdl: remove NULL check, g_malloc0 can't fail
>   qxl: drop qxl_spice_update_area_async definition
>   qxl: require spice >= 0.8.2
>   qxl: remove flipped
>   qxl: introduce QXLCookie
>   qxl: make qxl_render_update async

Added to spice patch queue.

thanks,
  Gerd

Re: [Qemu-devel] [PATCH 1/2 v2] Add blkmirror block driver

[Federico Simoncelli]

- Original Message -
> From: "Luiz Capitulino" 
> To: "Federico Simoncelli" 
> Cc: qemu-devel@nongnu.org, mtosa...@redhat.com, pbonz...@redhat.com, 
> kw...@redhat.com, arm...@redhat.com
> Sent: Friday, February 24, 2012 7:17:22 PM
> Subject: Re: [PATCH 1/2 v2] Add blkmirror block driver
> 
> On Fri, 24 Feb 2012 16:49:03 +
> Federico Simoncelli  wrote:
> 
> > +/* Parse the raw image filename */
> > +filename2 = g_malloc(strlen(filename)+1);
> > +escape = 0;
> > +for (i = n = 0; i < strlen(filename); i++) {
> > +if (!escape && filename[i] == ':') {
> > +break;
> > +}
> > +if (!escape && filename[i] == '\\') {
> > +escape = 1;
> > +} else {
> > +escape = 0;
> > +}
> > +
> > +if (!escape) {
> > +filename2[n++] = filename[i];
> > +}
> > +}
> > +filename2[n] = '\0';
> 
> You're escaping only the first image name string, is that
> intentional?

Yes, it was also documented here by Marcelo:

> > diff --git a/docs/blkmirror.txt b/docs/blkmirror.txt
> > new file mode 100644
> > index 000..cf73f3f
> > --- /dev/null
> > +++ b/docs/blkmirror.txt
> > @@ -0,0 +1,16 @@
> > +Block mirror driver
> > +---
> > +
> > +This driver will mirror writes to two distinct images.
> > +It's used internally by live block copy.
> > +
> > +Format
> > +--
> > +
> > +blkmirror:/image1.img:/image2.img
> > +
> > +'\' (backslash) can be used to escape colon processing
> > +as a separator, in the first image filename.
> > +Backslashes themselves also can be escaped as '\\'.
> > +
> > +

Anyway this format is encapsulated by blockdev-migrate.

-- 
Federico

Re: [Qemu-devel] [PATCH][v14] megasas: LSI Megaraid SAS HBA emulation

[Hannes Reinecke]

Hi Micheal,

thanks for your review.
You'll find the answers inline.

On 02/23/2012 04:34 PM, Michael S. Tsirkin wrote:
> On Tue, Feb 21, 2012 at 10:36:43AM +0100, Hannes Reinecke wrote:
>> This patch adds an emulation for the LSI Megaraid SAS 8708EM2 HBA.
>> I've tested it to work with Linux, Windows Vista, and Windows7.
>> MSI-X support is currently broken; have to investigate.
>>
[ .. ]
> 
> So Alex asked whether I can merge this, which made me
> take a look. I don't know much about what this does
> so just general comments on all of the code.
> 
> Also, some issues related to msix - want to rip that code
> out for now since it does not work anyway?
> 
Well, I left it in as a reminder how things _should_ be coded.
I was hoping to get it to work eventually.

But then I didn't so maybe you're right.

> qemu has two styles for struct and enum types:
> 1. documented: typedef struct CamelCase CamelCase;
> 2. undocumented but still widely used: struct lower_case; (no typedef)
> *_t type typedef is used for numeric types such as target_phys_addr_t.
> 
> This code mixes them in arbitrary ways. Pls don't do that,
> pls be consistent.
> 
Ok, done.

> 
> 
> 
>> ---
>>  Makefile.objs   |1 +
>>  default-configs/pci.mak |1 +
>>  hw/megasas.c| 1865 
>> +++
>>  hw/mfi.h| 1281 
>>  hw/pci_ids.h|3 +-
>>  5 files changed, 3150 insertions(+), 1 deletions(-)
>>  create mode 100644 hw/megasas.c
>>  create mode 100644 hw/mfi.h
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 391e524..5841998 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -283,6 +283,7 @@ hw-obj-$(CONFIG_AHCI) += ide/ich.o
>>  
>>  # SCSI layer
>>  hw-obj-$(CONFIG_LSI_SCSI_PCI) += lsi53c895a.o
>> +hw-obj-$(CONFIG_MEGASAS_SCSI_PCI) += megasas.o
>>  hw-obj-$(CONFIG_ESP) += esp.o
>>  
>>  hw-obj-y += dma-helpers.o sysbus.o isa-bus.o
>> diff --git a/default-configs/pci.mak b/default-configs/pci.mak
>> index 9d3e1db..4b49c00 100644
>> --- a/default-configs/pci.mak
>> +++ b/default-configs/pci.mak
>> @@ -10,6 +10,7 @@ CONFIG_EEPRO100_PCI=y
>>  CONFIG_PCNET_PCI=y
>>  CONFIG_PCNET_COMMON=y
>>  CONFIG_LSI_SCSI_PCI=y
>> +CONFIG_MEGASAS_SCSI_PCI=y
>>  CONFIG_RTL8139_PCI=y
>>  CONFIG_E1000_PCI=y
>>  CONFIG_IDE_CORE=y
>> diff --git a/hw/megasas.c b/hw/megasas.c
>> new file mode 100644
>> index 000..083c3d3
>> --- /dev/null
>> +++ b/hw/megasas.c
>> @@ -0,0 +1,1865 @@
>> +/*
>> + * QEMU MegaRAID SAS 8708EM2 Host Bus Adapter emulation
> 
> Link to documentation/hardware spec/driver code?
> 
I would if I had some.
The only reference I had is the Linux megaraid_sas.c driver.

>> + *
>> + * Copyright (c) 2009-2012 Hannes Reinecke, SUSE Labs
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, see 
>> .
>> + */
>> +
>> +#include "hw.h"
>> +#include "pci.h"
>> +#include "dma.h"
>> +#include "msix.h"
>> +#include "iov.h"
>> +#include "scsi.h"
>> +#include "scsi-defs.h"
>> +#include "block_int.h"
>> +
>> +#include "mfi.h"
>> +
>> +/* Static definitions */
> 
> Remove above comment.
> 
Ok.

>> +#define MEGASAS_VERSION "1.60"
>> +#define MEGASAS_MAX_FRAMES 2048 /* Firmware limit at 65535 */
>> +#define MEGASAS_DEFAULT_FRAMES 1000 /* Windows requires this */
>> +#define MEGASAS_MAX_SGE 128 /* Firmware limit */
>> +#define MEGASAS_DEFAULT_SGE 80
>> +#define MEGASAS_MAX_SECTORS 0x  /* No real limit */
>> +#define MEGASAS_MAX_ARRAYS 128
>> +
>> +#define MEGASAS_FLAG_USE_JBOD  0
>> +#define MEGASAS_MASK_USE_JBOD  (1 << MEGASAS_FLAG_USE_JBOD)
>> +#define MEGASAS_FLAG_USE_MSIX  1
>> +#define MEGASAS_MASK_USE_MSIX  (1 << MEGASAS_FLAG_USE_MSIX)
>> +#define MEGASAS_FLAG_USE_QUEUE64   2
>> +#define MEGASAS_MASK_USE_QUEUE64   (1 << MEGASAS_FLAG_USE_QUEUE64)
>> +
>> +const char *megasas_raid_modes[] = {
>> +"raid", "jbod"
>> +};
>> +
>> +const char *mfi_frame_desc[] = {
>> +"MFI init", "LD Read", "LD Write", "LD SCSI", "PD SCSI",
>> +"MFI Doorbell", "MFI Abort", "MFI SMP", "MFI Stop"};
> 
> Can't the above go into mfi.h?
> 
Hmm. Well.

The problem with mfi.h is that it's not actually _my_ file, but
rather copied over from NetBSD. I felt a bit stupid doing a recoding
of all the values which are already present in NetBSD ...
Hence th

[Qemu-devel] Fail to share Samba directory with guest

[Jun Koi]

hi,

on qemu 1.0.1, i am trying to share a host directory with the Windows
guest like below:

qemu-system-i386 -enable-kvm -m 1000 -net nic,model=rtl8139 -net
user,smb=/tmp img.winxp

but in the guest, \\10.0.2.4 doesnt show me any shared directory.

i already run Samba on the host (default configuration).

did i miss something, or is it a bug??

thanks,
Jun

Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver

[Stefan Hajnoczi]

On Thu, Feb 23, 2012 at 4:14 PM, Stefan Hajnoczi  wrote:
> On Wed, Feb 22, 2012 at 5:13 PM, Federico Simoncelli
>  wrote:
>> From: Marcelo Tosatti 
>>
>> Mirrored writes are used by live block copy.
>
> I think the right approach is to create a single blkmirror driver that
> also includes blkverify functionality.  The code is basically the same
> except blkverify also compares reads - just use a flag to
> enable/disable that behavior.
>
> Feel free to rename the blkverify driver to blkmirror if you wish.

Federico: Any response to this?  I still think blkmirror and blkverify
do essentially the same thing and should be unified.

Stefan

Re: [Qemu-devel] [PATCH] qed: replace vm_clock with rt_clock for qemu-tool compatibility

[Kevin Wolf]

Am 27.02.2012 09:42, schrieb Paolo Bonzini:
> On 02/27/2012 08:35 AM, Zhi Yong Wu wrote:
>> Since vm_clock is created via qemu_init_main_loop(), when QED read
>> vm_clock, why will this call abort()?
>> Can you elaborate this? what is its call path?
>>
> 
> It will crash in cpu_get_clock() (in qemu-tool.c).

The fix isn't very nice if it makes migration impossible. I'd like to
introduce a similar timer in qcow2 which does support migration and
breaking it is not an option. So what about (completely untested)...

diff --git a/qemu-tool.c b/qemu-tool.c
index 183a583..edb84f5 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -61,7 +61,7 @@ void monitor_protocol_event(MonitorEvent event,
QObject *data)

 int64_t cpu_get_clock(void)
 {
-abort();
+return 0;
 }

 int64_t cpu_get_icount(void)

Kevin

Re: [Qemu-devel] [PATCH 2/3] Allow larger return values from get_image_size()

[Markus Armbruster]

David Gibson  writes:

> On Mon, Feb 27, 2012 at 09:21:25AM +0100, Markus Armbruster wrote:
>> David Gibson  writes:
>> 
>> > Currently get_image_size(), used to find the size of files, returns an int.
>> > But for modern systems, int may be only 32-bit and we can have files
>> > larger than that.
>> >
>> > This patch, therefore, changes the return type of get_image_size() to off_t
>> > (the same as the return type from lseek() itself).  It also audits all the
>> > callers of get_image_size() to make sure they process the new unsigned
>> > return type correctly.
>> >
>> > This leaves load_image_targphys() with a limited return type, but one thing
>> > at a time (that function has far more callers to be audited, so it will
>> > take longer to fix).
>> 
>> I'm afraid this replaces the single, well-known integer overflow in
>> get_image_size()'s conversion of lseek() value to int by many unknown
>> overflows in get_image_size()'s users.  One example below.  Didn't look
>> for more.
>> 
>> If you need a wider get_image_size(), please make sure its users are
>> prepared for it!
>
> Actually, I have no such need at all, but when I fixed another bug in
> loader.c, someone whinged about me not changing get_image_size(), so
> here it is.

Heh.

>> Is the any use for image sizes exceeding size_t?  Arent such images
>> impossible to load?
>
> Well, possibly not.
>
>> 
>> [...]
>> > diff --git a/hw/pc.c b/hw/pc.c
>> > index b9f4bc7..cb41955 100644
>> > --- a/hw/pc.c
>> > +++ b/hw/pc.c
>> > @@ -672,7 +672,8 @@ static void load_linux(void *fw_cfg,
>> > target_phys_addr_t max_ram_size)
>> >  {
>> >  uint16_t protocol;
>> > -int setup_size, kernel_size, initrd_size = 0, cmdline_size;
>> > +int setup_size, kernel_size, cmdline_size;
>> > +off_t initrd_size = 0;
>> >  uint32_t initrd_max;
>> >  uint8_t header[8192], *setup, *kernel, *initrd_data;
>> >  target_phys_addr_t real_addr, prot_addr, cmdline_addr, initrd_addr = 
>> > 0;
>> > @@ -795,7 +796,7 @@ static void load_linux(void *fw_cfg,
>> >}
>> >  
>> >initrd_size = get_image_size(initrd_filename);
>> > -if (initrd_size < 0) {
>> > +if (initrd_size == -1) {
>> 
>> Needless churn.
>
> No, it's not.  Now that initrd_size is unsigned initrd_size < 0 would
> return false always (and give a "comparison is always false due to
> limited range of data type" warning).

off_t is signed:
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html

>> >  fprintf(stderr, "qemu: error reading initrd %s\n",
>> >  initrd_filename);
>> >  exit(1);
>>}
>> 
>>initrd_addr = (initrd_max-initrd_size) & ~4095;
>> 
>>initrd_data = g_malloc(initrd_size);
>> 
>> Integer overflow in conversion from off_t initrd_size to the argument
>> type size_t[*].
>
> Hm, true.
>
> Ok, well, I give up.  Someone who actually needs it can fix it.

Pity.  Thanks anyway!

[Qemu-devel] Boot failure with MS-Dos 6.22 (due to bad BIOS build?)

[Daniel P. Berrange]

I'm seeing current QEMU GIT fail to boot MS-Dos 6.22 with the following
crash:

# qemu-system-x86_64 -fda ~/MS-DOS\ 6.22.img  -m 1 -curses
iPXE v1.0.0-591-g7aee315
 iPXE (http://ipxe.org) 00:03.0 C900 PCI2.10 
PnP PMM++ C900

 Booting from Floppy..
.qemu: fatal: Trying to execute code outside 
RAM or ROM at 0x0001000e

EAX= EBX= ECX=c934 EDX=0068
ESI=6801 EDI= EBP=002b ESP=fff5
EIP= EFL=0002 [---] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0040 0400  9300
CS =f000 000f  9b00
SS =9ec4 0009ec40  9300
DS =9ec4 0009ec40  9300
FS =   9300
GS =   9300
LDT=   8200
TR =   8b00
GDT= 000fcd78 0037
IDT=  03ff
CR0=0010 CR2= CR3= CR4=
DR0= DR1= DR2= 
DR3= 
DR6=0ff0 DR7=0400
CCS=00d0 CCD=0068 CCO=SARL
EFER=
FCW=037f FSW= [ST=0] FTW=00 MXCSR=1f80
FPR0=  FPR1= 
FPR2=  FPR3= 
FPR4=  FPR5= 
FPR6=  FPR7= 
XMM00= XMM01=
XMM02= XMM03=
XMM04= XMM05=
XMM06= XMM07=
Aborted


Git bisect blames this

  commit 41bd360325168b3c1db78eb7311420a1607d521f
  Author: Jan Kiszka 
  Date:   Sun Jan 15 17:48:25 2012 +0100

seabios: Update to release 1.6.3.1

User visible changes in seabios:
 - Probe HPET existence (fix for -no-hpet)
 - Probe PCI existence (fix for -machine isapc)
 - usb: fix boot paths

Signed-off-by: Jan Kiszka 


I tried to bisect Seabios, but every revision in Seabios upstream works
fine.

Then I noticed, that if I rebuild the BIOS, from the exact same revision
1.6.3.1 revision that is committed in 'seabios' submodule in QEMU, then
it works fine. So AFAICT, it is not the Seabios source code at fault,
but rather the binary build we have commited to GIT. Should/can we rebuild
the bios.bin in GIT ?

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] build: replace librt check function

[Ian Campbell]

On Thu, 2012-02-23 at 13:34 +, Roger Pau Monné wrote:
> 2012/2/22 Anthony Liguori :
> > On 02/20/2012 06:11 AM, Roger Pau Monne wrote:
> >>
> >> Replace clock_gettime with timer_gettime, since at least under
> >> uclibc 0.9.33 the clock_getttime function can be used without linking
> >> against librt (although the manual page states the opposite).
> >>
> >> Signed-off-by: Roger Pau Monne
> >
> >
> > I don't think this is against qemu.git.  Please do not send patches to
> > qemu-devel that are not against qemu.git without clearly indicating this.
> 
> Sorry, this is against qemu-xen-upstream, should I add a tag to my
> patches and resend them to qemu-devel?

qemu-xen-upstream is based upon the upstream qemu stable branch, not the
development branch.

However patches for qemu-xen-upstream should have already been applied
upstream. This means that patches should be against the mainline (e.g.
development branch) version of qemu and sent to qemu-devel. Once
committed there they will be backported as required to the stable branch
maintained by Xen.org as qemu-xen-upstream.

(Stefano, is that correct? Is there somewhere we can (or do!) document
this? Perhaps http://wiki.xen.org/xenwiki/QEMUUpstream linked to from
Submitting_Xen_Patches? The backport policy should also be covered, e.g.
http://marc.info/?l=xen-devel&m=132872001218724 )

Ian.

> 
> Thanks, Roger.
> 
> > Regards,
> >
> > Anthony Liguori
> >
> >
> >> ---
> >>  configure |3 ++-
> >>  1 files changed, 2 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/configure b/configure
> >> index 7bcd547..fb99632 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -2438,7 +2438,8 @@ fi
> >>  cat>  $TMPC< >>  #include
> >>  #include
> >> -int main(void) { clockid_t id; return clock_gettime(id, NULL); }
> >> +int main(void) { timer_t tid; struct itimerspec it; \
> >> + return timer_gettime(tid,&it); }
> >>  EOF
> >>
> >>  if compile_prog "" "" ; then
> >
> >
> 
> ___
> Xen-devel mailing list
> xen-de...@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] build: replace librt check function

[Roger Pau Monné]

2012/2/27 Ian Campbell :
> On Thu, 2012-02-23 at 13:34 +, Roger Pau Monné wrote:
>> 2012/2/22 Anthony Liguori :
>> > On 02/20/2012 06:11 AM, Roger Pau Monne wrote:
>> >>
>> >> Replace clock_gettime with timer_gettime, since at least under
>> >> uclibc 0.9.33 the clock_getttime function can be used without linking
>> >> against librt (although the manual page states the opposite).
>> >>
>> >> Signed-off-by: Roger Pau Monne
>> >
>> >
>> > I don't think this is against qemu.git.  Please do not send patches to
>> > qemu-devel that are not against qemu.git without clearly indicating this.
>>
>> Sorry, this is against qemu-xen-upstream, should I add a tag to my
>> patches and resend them to qemu-devel?
>
> qemu-xen-upstream is based upon the upstream qemu stable branch, not the
> development branch.
>
> However patches for qemu-xen-upstream should have already been applied
> upstream. This means that patches should be against the mainline (e.g.
> development branch) version of qemu and sent to qemu-devel. Once
> committed there they will be backported as required to the stable branch
> maintained by Xen.org as qemu-xen-upstream.

Ok, I'm going to rework this against qemu development branch, thanks
for the tip.

> (Stefano, is that correct? Is there somewhere we can (or do!) document
> this? Perhaps http://wiki.xen.org/xenwiki/QEMUUpstream linked to from
> Submitting_Xen_Patches? The backport policy should also be covered, e.g.
> http://marc.info/?l=xen-devel&m=132872001218724 )
>
> Ian.
>
>>
>> Thanks, Roger.
>>
>> > Regards,
>> >
>> > Anthony Liguori
>> >
>> >
>> >> ---
>> >>  configure |    3 ++-
>> >>  1 files changed, 2 insertions(+), 1 deletions(-)
>> >>
>> >> diff --git a/configure b/configure
>> >> index 7bcd547..fb99632 100755
>> >> --- a/configure
>> >> +++ b/configure
>> >> @@ -2438,7 +2438,8 @@ fi
>> >>  cat>  $TMPC<> >>  #include
>> >>  #include
>> >> -int main(void) { clockid_t id; return clock_gettime(id, NULL); }
>> >> +int main(void) { timer_t tid; struct itimerspec it; \
>> >> +                 return timer_gettime(tid,&it); }
>> >>  EOF
>> >>
>> >>  if compile_prog "" "" ; then
>> >
>> >
>>
>> ___
>> Xen-devel mailing list
>> xen-de...@lists.xen.org
>> http://lists.xen.org/xen-devel
>
>

[Qemu-devel] [PATCH] qxl: properly handle upright and non-shared surfaces

[Gerd Hoffmann]

Although qxl creates a shared displaysurface when the qxl surface is
upright and doesn't need to be flipped there is no guarantee that the
surface doesn't become unshared for some reason.  Rename qxl_flip to
qxl_blit and fix it to handle both flip and non-flip cases.

Signed-off-by: Gerd Hoffmann 
---
 hw/qxl-render.c |   20 +---
 1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index f323a4d..25857f6 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -21,25 +21,31 @@
 
 #include "qxl.h"
 
-static void qxl_flip(PCIQXLDevice *qxl, QXLRect *rect)
+static void qxl_blit(PCIQXLDevice *qxl, QXLRect *rect)
 {
 uint8_t *src;
 uint8_t *dst = qxl->vga.ds->surface->data;
 int len, i;
 
-if (qxl->guest_primary.qxl_stride > 0) {
+if (is_buffer_shared(qxl->vga.ds->surface)) {
 return;
 }
 if (!qxl->guest_primary.data) {
 dprint(qxl, 1, "%s: initializing guest_primary.data\n", __func__);
 qxl->guest_primary.data = memory_region_get_ram_ptr(&qxl->vga.vram);
 }
-dprint(qxl, 1, "%s: stride %d, [%d, %d, %d, %d]\n", __func__,
+dprint(qxl, 2, "%s: stride %d, [%d, %d, %d, %d]\n", __func__,
 qxl->guest_primary.qxl_stride,
 rect->left, rect->right, rect->top, rect->bottom);
 src = qxl->guest_primary.data;
-src += (qxl->guest_primary.surface.height - rect->top - 1) *
-qxl->guest_primary.abs_stride;
+if (qxl->guest_primary.qxl_stride < 0) {
+/* qxl surface is upside down, walk src scanlines
+ * in reverse order to flip it */
+src += (qxl->guest_primary.surface.height - rect->top - 1) *
+qxl->guest_primary.abs_stride;
+} else {
+src += rect->top * qxl->guest_primary.abs_stride;
+}
 dst += rect->top  * qxl->guest_primary.abs_stride;
 src += rect->left * qxl->guest_primary.bytes_pp;
 dst += rect->left * qxl->guest_primary.bytes_pp;
@@ -48,7 +54,7 @@ static void qxl_flip(PCIQXLDevice *qxl, QXLRect *rect)
 for (i = rect->top; i < rect->bottom; i++) {
 memcpy(dst, src, len);
 dst += qxl->guest_primary.abs_stride;
-src -= qxl->guest_primary.abs_stride;
+src += qxl->guest_primary.qxl_stride;
 }
 }
 
@@ -132,7 +138,7 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice 
*qxl)
 if (qemu_spice_rect_is_empty(qxl->dirty+i)) {
 break;
 }
-qxl_flip(qxl, qxl->dirty+i);
+qxl_blit(qxl, qxl->dirty+i);
 dpy_update(vga->ds,
qxl->dirty[i].left, qxl->dirty[i].top,
qxl->dirty[i].right - qxl->dirty[i].left,
-- 
1.7.1

Re: [Qemu-devel] [PATCH][v14] megasas: LSI Megaraid SAS HBA emulation

[Michael S. Tsirkin]

On Mon, Feb 27, 2012 at 10:17:21AM +0100, Hannes Reinecke wrote:
> Hi Micheal,
> 
> thanks for your review.
> You'll find the answers inline.
> 
> On 02/23/2012 04:34 PM, Michael S. Tsirkin wrote:
> > On Tue, Feb 21, 2012 at 10:36:43AM +0100, Hannes Reinecke wrote:
> >> This patch adds an emulation for the LSI Megaraid SAS 8708EM2 HBA.
> >> I've tested it to work with Linux, Windows Vista, and Windows7.
> >> MSI-X support is currently broken; have to investigate.
> >>
> [ .. ]
> > 
> > So Alex asked whether I can merge this, which made me
> > take a look. I don't know much about what this does
> > so just general comments on all of the code.
> > 
> > Also, some issues related to msix - want to rip that code
> > out for now since it does not work anyway?
> > 
> Well, I left it in as a reminder how things _should_ be coded.
> I was hoping to get it to work eventually.
> 
> But then I didn't so maybe you're right.
> 
> > qemu has two styles for struct and enum types:
> > 1. documented: typedef struct CamelCase CamelCase;
> > 2. undocumented but still widely used: struct lower_case; (no typedef)
> > *_t type typedef is used for numeric types such as target_phys_addr_t.
> > 
> > This code mixes them in arbitrary ways. Pls don't do that,
> > pls be consistent.
> > 
> Ok, done.
> 
> > 
> > 
> > 
> >> ---
> >>  Makefile.objs   |1 +
> >>  default-configs/pci.mak |1 +
> >>  hw/megasas.c| 1865 
> >> +++
> >>  hw/mfi.h| 1281 
> >>  hw/pci_ids.h|3 +-
> >>  5 files changed, 3150 insertions(+), 1 deletions(-)
> >>  create mode 100644 hw/megasas.c
> >>  create mode 100644 hw/mfi.h
> >>
> >> diff --git a/Makefile.objs b/Makefile.objs
> >> index 391e524..5841998 100644
> >> --- a/Makefile.objs
> >> +++ b/Makefile.objs
> >> @@ -283,6 +283,7 @@ hw-obj-$(CONFIG_AHCI) += ide/ich.o
> >>  
> >>  # SCSI layer
> >>  hw-obj-$(CONFIG_LSI_SCSI_PCI) += lsi53c895a.o
> >> +hw-obj-$(CONFIG_MEGASAS_SCSI_PCI) += megasas.o
> >>  hw-obj-$(CONFIG_ESP) += esp.o
> >>  
> >>  hw-obj-y += dma-helpers.o sysbus.o isa-bus.o
> >> diff --git a/default-configs/pci.mak b/default-configs/pci.mak
> >> index 9d3e1db..4b49c00 100644
> >> --- a/default-configs/pci.mak
> >> +++ b/default-configs/pci.mak
> >> @@ -10,6 +10,7 @@ CONFIG_EEPRO100_PCI=y
> >>  CONFIG_PCNET_PCI=y
> >>  CONFIG_PCNET_COMMON=y
> >>  CONFIG_LSI_SCSI_PCI=y
> >> +CONFIG_MEGASAS_SCSI_PCI=y
> >>  CONFIG_RTL8139_PCI=y
> >>  CONFIG_E1000_PCI=y
> >>  CONFIG_IDE_CORE=y
> >> diff --git a/hw/megasas.c b/hw/megasas.c
> >> new file mode 100644
> >> index 000..083c3d3
> >> --- /dev/null
> >> +++ b/hw/megasas.c
> >> @@ -0,0 +1,1865 @@
> >> +/*
> >> + * QEMU MegaRAID SAS 8708EM2 Host Bus Adapter emulation
> > 
> > Link to documentation/hardware spec/driver code?
> > 
> I would if I had some.
> The only reference I had is the Linux megaraid_sas.c driver.
> 
> >> + *
> >> + * Copyright (c) 2009-2012 Hannes Reinecke, SUSE Labs
> >> + *
> >> + * This library is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU Lesser General Public
> >> + * License as published by the Free Software Foundation; either
> >> + * version 2 of the License, or (at your option) any later version.
> >> + *
> >> + * This library is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> + * Lesser General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU Lesser General Public
> >> + * License along with this library; if not, see 
> >> .
> >> + */
> >> +
> >> +#include "hw.h"
> >> +#include "pci.h"
> >> +#include "dma.h"
> >> +#include "msix.h"
> >> +#include "iov.h"
> >> +#include "scsi.h"
> >> +#include "scsi-defs.h"
> >> +#include "block_int.h"
> >> +
> >> +#include "mfi.h"
> >> +
> >> +/* Static definitions */
> > 
> > Remove above comment.
> > 
> Ok.
> 
> >> +#define MEGASAS_VERSION "1.60"
> >> +#define MEGASAS_MAX_FRAMES 2048 /* Firmware limit at 65535 */
> >> +#define MEGASAS_DEFAULT_FRAMES 1000 /* Windows requires this */
> >> +#define MEGASAS_MAX_SGE 128 /* Firmware limit */
> >> +#define MEGASAS_DEFAULT_SGE 80
> >> +#define MEGASAS_MAX_SECTORS 0x  /* No real limit */
> >> +#define MEGASAS_MAX_ARRAYS 128
> >> +
> >> +#define MEGASAS_FLAG_USE_JBOD  0
> >> +#define MEGASAS_MASK_USE_JBOD  (1 << MEGASAS_FLAG_USE_JBOD)
> >> +#define MEGASAS_FLAG_USE_MSIX  1
> >> +#define MEGASAS_MASK_USE_MSIX  (1 << MEGASAS_FLAG_USE_MSIX)
> >> +#define MEGASAS_FLAG_USE_QUEUE64   2
> >> +#define MEGASAS_MASK_USE_QUEUE64   (1 << MEGASAS_FLAG_USE_QUEUE64)
> >> +
> >> +const char *megasas_raid_modes[] = {
> >> +"raid", "jbod"
> >> +};
> >> +
> >> +const char *mfi_frame_desc[] = {
> >> +"MFI init", "LD R

Re: [Qemu-devel] [PATCH 4/8] Add universal DMA helper functions

[Michael S. Tsirkin]

On Mon, Feb 27, 2012 at 11:22:43AM +1100, David Gibson wrote:
> On Sun, Feb 26, 2012 at 12:04:49PM +0200, Michael S. Tsirkin wrote:
> > On Fri, Feb 24, 2012 at 02:27:39PM +1100, David Gibson wrote:
> > > Not that long ago, every device implementation using DMA directly
> > > accessed guest memory using cpu_physical_memory_*().  This meant that
> > > adding support for a guest visible IOMMU would require changing every
> > > one of these devices to go through IOMMU translation.
> > > 
> > > Shortly before qemu 1.0, I made a start on fixing this by providing
> > > helper functions for PCI DMA.  These are currently just stubs which
> > > call the direct access functions, but mean that an IOMMU can be
> > > implemented in one place, rather than for every PCI device.
> > > 
> > > Clearly, this doesn't help for non PCI devices, which could also be
> > > IOMMU translated on some platforms.  It is also problematic for the
> > > devices which have both PCI and non-PCI version (e.g. OHCI, AHCI) - we
> > > cannot use the the pci_dma_*() functions, because they assume the
> > > presence of a PCIDevice, but we don't want to have to check between
> > > pci_dma_*() and cpu_physical_memory_*() every time we do a DMA in the
> > > device code.
> > > 
> > > This patch makes the first step on addressing both these problems, by
> > > introducing new (stub) dma helper functions which can be used for any
> > > DMA capable device.
> > > 
> > > These dma functions take a DMAContext *, a new (currently empty)
> > > variable describing the DMA address space in which the operation is to
> > > take place.  NULL indicates untranslated DMA directly into guest
> > > physical address space.  The intention is that in future non-NULL
> > > values will given information about any necessary IOMMU translation.
> > > 
> > > DMA using devices must obtain a DMAContext (or, potentially, contexts)
> > > from their bus or platform.  For now this patch just converts the PCI
> > > wrappers to be implemented in terms of the universal wrappers,
> > > converting other drivers can take place over time.
> > > 
> > > Cc: Michael S. Tsirkin 
> > > Cc: Joerg Rodel 
> > > Cc: Eduard - Gabriel Munteanu 
> > > Cc: Richard Henderson 
> > > 
> > > Signed-off-by: David Gibson 
> > 
> > I'm a bit confused with all the stubbing going on.
> > Is this the final form of the pci_* functions or just
> > a stub? If the final form, we probably should just
> > open-code them - they don't buy us much.
> > If not, let's add a comment?
> 
> Well.. it's the intended final form of pci_dma_*() - which do become
> trivial wrappers, yes.

I'd say let's drop them then (in a follow-up patch).  The topic is
confusing enough without having to wade through layers of wrappers :)

> It's _not_ the intended final form of dma_*(),
> which need to grow code to do actual IOMMU translation.  I'll add a
> comment about this in the next round.
> 
> -- 
> David Gibson  | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au| minimalist, thank you.  NOT _the_ 
> _other_
>   | _way_ _around_!
> http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH 4/8] gtk: add virtual console support (v2)

[Kevin Wolf]

Am 27.02.2012 00:46, schrieb Anthony Liguori:
> This enables VteTerminal to be used to render the text consoles.  VteTerminal 
> is
> the same widget used by gnome-terminal which means it's VT100 emulation is as
> good as they come.
> 
> It's also screen reader accessible, supports copy/paste, proper scrolling and
> most of the other features you would expect from a terminal widget.
> 
> Signed-off-by: Anthony Liguori 
> ---
> v1 -> v2
>  - make sure to activate the menu item when switching tabs
>  - fix sizing of non-0 pages
> ---
>  console.c |4 +-
>  console.h |4 +-
>  ui/gtk.c  |  160 
> +
>  3 files changed, 164 insertions(+), 4 deletions(-)
> 
> diff --git a/console.c b/console.c
> index 6434ed0..c12f02a 100644
> --- a/console.c
> +++ b/console.c
> @@ -1551,9 +1551,9 @@ static CharDriverState *text_console_init(QemuOpts 
> *opts)
>  
>  static VcHandler *vc_handler = text_console_init;
>  
> -int vc_init(QemuOpts *opts, CharDriverState **_chr)
> +CharDriverState *vc_init(QemuOpts *opts)
>  {
> -return vc_handler(opts, _chr);
> +return vc_handler(opts);
>  }
>  
>  void register_vc_handler(VcHandler *handler)
> diff --git a/console.h b/console.h
> index 9b4b390..27d7929 100644
> --- a/console.h
> +++ b/console.h
> @@ -363,9 +363,9 @@ void qemu_console_resize(DisplayState *ds, int width, int 
> height);
>  void qemu_console_copy(DisplayState *ds, int src_x, int src_y,
> int dst_x, int dst_y, int w, int h);
>  
> -typedef int (VcHandler)(QemuOpts *, CharDriverState **);
> +typedef CharDriverState *(VcHandler)(QemuOpts *);
>  
> -int vc_init(QemuOpts *opts, CharDriverState **_chr);
> +CharDriverState *vc_init(QemuOpts *opts);
>  void register_vc_handler(VcHandler *handler);
>  
>  /* sdl.c */
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 591a987..0579a55 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -56,6 +56,8 @@
>  #define dprintf(fmt, ...) do { } while (0)
>  #endif
>  
> +#define MAX_VCS 10
> +
>  typedef struct VirtualConsole
>  {
>  GtkWidget *menu_item;
> @@ -79,6 +81,9 @@ typedef struct GtkDisplayState
>  GtkWidget *view_menu;
>  GtkWidget *vga_item;
>  
> +int nb_vcs;
> +VirtualConsole vc[MAX_VCS];
> +
>  GtkWidget *show_tabs_item;
>  
>  GtkWidget *vbox;
> @@ -400,6 +405,15 @@ static void gd_menu_switch_vc(GtkMenuItem *item, void 
> *opaque)
>  
>  if (gtk_check_menu_item_get_active(GTK_CHECK_MENU_ITEM(s->vga_item))) {
>  gtk_notebook_set_current_page(GTK_NOTEBOOK(s->notebook), 0);
> +} else {
> +int i;
> +
> +for (i = 0; i < s->nb_vcs; i++) {
> +if 
> (gtk_check_menu_item_get_active(GTK_CHECK_MENU_ITEM(s->vc[i].menu_item))) {
> +gtk_notebook_set_current_page(GTK_NOTEBOOK(s->notebook), i + 
> 1);
> +break;
> +}
> +}
>  }
>  }
>  
> @@ -418,16 +432,154 @@ static void gd_change_page(GtkNotebook *nb, gpointer 
> arg1, guint arg2,
> gpointer data)
>  {
>  GtkDisplayState *s = data;
> +guint last_page;
>  
>  if (!gtk_widget_get_realized(s->notebook)) {
>  return;
>  }
>  
> +last_page = gtk_notebook_get_current_page(nb);
> +
> +if (last_page) {
> +gtk_widget_set_size_request(s->vc[last_page - 1].terminal, -1, -1);
> +}
> +
> +if (arg2 == 0) {
> +gtk_check_menu_item_set_active(GTK_CHECK_MENU_ITEM(s->vga_item), 
> TRUE);
> +} else {
> +VirtualConsole *vc = &s->vc[arg2 - 1];
> +VteTerminal *term = VTE_TERMINAL(vc->terminal);
> +int width, height;
> +
> +width = 80 * vte_terminal_get_char_width(term);
> +height = 25 * vte_terminal_get_char_height(term);
> +
> +gtk_check_menu_item_set_active(GTK_CHECK_MENU_ITEM(vc->menu_item), 
> TRUE);
> +gtk_widget_set_size_request(vc->terminal, width, height);
> +}
> +
>  gd_update_cursor(s, TRUE);
>  }
>  
> +/** Virtual Console Callbacks **/
> +
> +static int gd_vc_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
> +{
> +VirtualConsole *vc = chr->opaque;
> +
> +return write(vc->fd, buf, len);
> +}
> +
> +static int nb_vcs;
> +static CharDriverState *vcs[MAX_VCS];
> +
> +static CharDriverState *gd_vc_handler(QemuOpts *opts)
> +{
> +CharDriverState *chr;
> +
> +chr = g_malloc0(sizeof(*chr));
> +chr->chr_write = gd_vc_chr_write;
> +
> +vcs[nb_vcs++] = chr;
> +
> +return chr;
> +}
> +
>  void early_gtk_display_init(void)
>  {
> +register_vc_handler(gd_vc_handler);
> +}
> +
> +static gboolean gd_vc_in(GIOChannel *chan, GIOCondition cond, void *opaque)
> +{
> +VirtualConsole *vc = opaque;
> +uint8_t buffer[1024];
> +ssize_t len;
> +
> +len = read(vc->fd, buffer, sizeof(buffer));
> +if (len <= 0) {
> +return FALSE;
> +}
> +
> +qemu_chr_be_write(vc->chr, buffer, len);
> +
> +return TRUE;
> +}
> +
> +static GSList

[Qemu-devel] [PATCH] TCG: Convert global variables to be TLS.

[Evgeny Voevodin]

I suggest to send this patch to mailing list. Approve?

This patch converts some TCG data to be TLS on the way to make 
TCG multithreaded. This work was made in assumption that qemu-tls.h defines
a general direction to let each VCPU run in its own thread wile being processed
by TCG.

Evgeny Voevodin (1):
  TCG: Convert global variables to be TLS.

 bsd-user/main.c|1 +
 cpus.c |2 +
 darwin-user/main.c |1 +
 exec.c |  121 ++-
 linux-user/main.c  |1 +
 qemu-common.h  |1 +
 6 files changed, 77 insertions(+), 50 deletions(-)

-- 
1.7.5.4

[Qemu-devel] [PATCH] TCG: Convert global variables to be TLS.

[Evgeny Voevodin]

This commit converts code_gen_buffer, code_gen_ptr, tbs, nb_tbs to
TLS. We need this if we want TCG to become multithreaded.

Initialization of code_gen_buffer and code_gen_ptr is moved to new
tcg_gen_buffer_init() function. This is done because we do not need
to allocate and initialize TCG buffers for IO thread. Initialization
is now done in qemu_tcg_cpu_thread_fn() by each HW thread
individually.

Also tcg_enabled() returns a variable instead of
(code_gen_buffer != NULL) since if called from IO thread, this will
always return FALSE.

Also some code format changes.

Signed-off-by: Evgeny Voevodin 
---
 bsd-user/main.c|1 +
 cpus.c |2 +
 darwin-user/main.c |1 +
 exec.c |  121 ++-
 linux-user/main.c  |1 +
 qemu-common.h  |1 +
 6 files changed, 77 insertions(+), 50 deletions(-)

diff --git a/bsd-user/main.c b/bsd-user/main.c
index cc7d4a3..11e4540 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -906,6 +906,7 @@ int main(int argc, char **argv)
 #endif
 }
 tcg_exec_init(0);
+tcg_gen_buffer_init();
 cpu_exec_init_all();
 /* NOTE: we need to init the CPU at this stage to get
qemu_host_page_size */
diff --git a/cpus.c b/cpus.c
index f45a438..6190250 100644
--- a/cpus.c
+++ b/cpus.c
@@ -746,6 +746,8 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
 {
 CPUState *env = arg;
 
+tcg_gen_buffer_init();
+
 qemu_tcg_init_cpu_signals();
 qemu_thread_get_self(env->thread);
 
diff --git a/darwin-user/main.c b/darwin-user/main.c
index 9b57c20..8618a52 100644
--- a/darwin-user/main.c
+++ b/darwin-user/main.c
@@ -851,6 +851,7 @@ int main(int argc, char **argv)
 #endif
 }
 tcg_exec_init(0);
+tcg_gen_buffer_init();
 cpu_exec_init_all();
 /* NOTE: we need to init the CPU at this stage to get
qemu_host_page_size */
diff --git a/exec.c b/exec.c
index b81677a..51a93d9 100644
--- a/exec.c
+++ b/exec.c
@@ -79,10 +79,10 @@
 
 #define SMC_BITMAP_USE_THRESHOLD 10
 
-static TranslationBlock *tbs;
+static DEFINE_TLS(TranslationBlock*, tbs);
 static int code_gen_max_blocks;
 TranslationBlock *tb_phys_hash[CODE_GEN_PHYS_HASH_SIZE];
-static int nb_tbs;
+static DEFINE_TLS(int, nb_tbs);
 /* any access to the tbs or the page table must use this lock */
 spinlock_t tb_lock = SPIN_LOCK_UNLOCKED;
 
@@ -103,11 +103,12 @@ spinlock_t tb_lock = SPIN_LOCK_UNLOCKED;
 #endif
 
 uint8_t code_gen_prologue[1024] code_gen_section;
-static uint8_t *code_gen_buffer;
+static bool code_gen_enabled;
+static DEFINE_TLS(uint8_t*, code_gen_buffer);
 static unsigned long code_gen_buffer_size;
 /* threshold to flush the translated code buffer */
 static unsigned long code_gen_buffer_max_size;
-static uint8_t *code_gen_ptr;
+static DEFINE_TLS(uint8_t*, code_gen_ptr);
 
 #if !defined(CONFIG_USER_ONLY)
 int phys_ram_fd;
@@ -469,18 +470,17 @@ static void tlb_unprotect_code_phys(CPUState *env, 
ram_addr_t ram_addr,
 #endif
 
 #ifdef USE_STATIC_CODE_GEN_BUFFER
-static uint8_t static_code_gen_buffer[DEFAULT_CODE_GEN_BUFFER_SIZE]
-   __attribute__((aligned (CODE_GEN_ALIGN)));
+static DEFINE_TLS(uint8_t [DEFAULT_CODE_GEN_BUFFER_SIZE],
+static_code_gen_buffer) __attribute__((aligned(CODE_GEN_ALIGN)));
 #endif
 
-static void code_gen_alloc(unsigned long tb_size)
+static void code_gen_alloc(void)
 {
 #ifdef USE_STATIC_CODE_GEN_BUFFER
-code_gen_buffer = static_code_gen_buffer;
+tls_var(code_gen_buffer) = tls_var(static_code_gen_buffer);
 code_gen_buffer_size = DEFAULT_CODE_GEN_BUFFER_SIZE;
-map_exec(code_gen_buffer, code_gen_buffer_size);
+map_exec(tls_var(code_gen_buffer), code_gen_buffer_size);
 #else
-code_gen_buffer_size = tb_size;
 if (code_gen_buffer_size == 0) {
 #if defined(CONFIG_USER_ONLY)
 code_gen_buffer_size = DEFAULT_CODE_GEN_BUFFER_SIZE;
@@ -522,10 +522,10 @@ static void code_gen_alloc(unsigned long tb_size)
 }
 start = (void *)0x9000UL;
 #endif
-code_gen_buffer = mmap(start, code_gen_buffer_size,
+tls_var(code_gen_buffer) = mmap(start, code_gen_buffer_size,
PROT_WRITE | PROT_READ | PROT_EXEC,
flags, -1, 0);
-if (code_gen_buffer == MAP_FAILED) {
+if (tls_var(code_gen_buffer) == MAP_FAILED) {
 fprintf(stderr, "Could not allocate dynamic translator buffer\n");
 exit(1);
 }
@@ -553,24 +553,31 @@ static void code_gen_alloc(unsigned long tb_size)
 code_gen_buffer_size = (512 * 1024 * 1024);
 }
 #endif
-code_gen_buffer = mmap(addr, code_gen_buffer_size,
+tls_var(code_gen_buffer) = mmap(addr, code_gen_buffer_size,
PROT_WRITE | PROT_READ | PROT_EXEC, 
flags, -1, 0);
-if (code_gen_buffer == MAP_FAILED) {
+if (tls_var(code_gen_buffer) == MAP_FAILED) {
 fprintf(stderr, "Could n

Re: [Qemu-devel] [PATCH v2 1/2] qapi: Introduce blockdev-group-snapshot-sync command

[Stefan Hajnoczi]

On Sun, Feb 26, 2012 at 7:31 PM, Jeff Cody  wrote:

Do you have automated tests for this feature?

> +/*
> + * Add new bs contents at the top of an image chain while the chain is live,
> + * while keeping required fields on the top layer.

Please also document the swap behavior.  It's pretty important for the
caller to realize that once this function returns, their
BlockDriverState arguments with have swapped.

> + * It is assumed that bs_new already points to an existing image,
> + * with the correct backing filename of top->backing_file

Not sure what this means.  Isn't bs_new going to use bs_top as its
backing file?  Why "top->backing_file"?

> +void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
> +{
> +    BlockDriverState tmp;
> +
> +    /* the new bs must not be in bdrv_states */
> +    bdrv_make_anon(bs_new);
> +
> +    tmp = *bs_new;
> +    tmp.backing_hd = bs_new;

This is tricky, would be nice to comment that we will swap bs_new and
bs_top later, therefore we need a pointer to bs_new here even though
it doesn't make sense yet.

> +
> +    /* there are some fields that need to stay on the top layer: */
> +
> +    /* dev info */
> +    tmp.dev_ops          = bs_top->dev_ops;
> +    tmp.dev_opaque       = bs_top->dev_opaque;
> +    tmp.dev              = bs_top->dev;
> +    tmp.buffer_alignment = bs_top->buffer_alignment;
> +    tmp.copy_on_read     = bs_top->copy_on_read;
> +
> +    /* i/o timing parameters */
> +    tmp.slice_time        = bs_top->slice_time;
> +    tmp.slice_start       = bs_top->slice_start;
> +    tmp.slice_end         = bs_top->slice_end;
> +    tmp.io_limits         = bs_top->io_limits;
> +    tmp.io_base           = bs_top->io_base;
> +    tmp.throttled_reqs    = bs_top->throttled_reqs;
> +    tmp.block_timer       = bs_top->block_timer;
> +    tmp.io_limits_enabled = bs_top->io_limits_enabled;

Please add a comment into BlockDriverState to warn that changes to
fields may require updates to this function too!

> +        /* We will manually add the backing_hd field to the bs later */
> +        states->new_bs = bdrv_new("");
> +        ret = bdrv_open(states->new_bs, snapshot_file,
> +                        flags | BDRV_O_NO_BACKING, drv);
> +        states->is_open = true;

What is the point of is_open?  If we failed then new_bs is actually
not open here.

I think you can remove this field and just do the following in close_and_fail:

if (states->new_bs) {
bdrv_delete(states->new_bs);
}

(BTW I think close_and_fail is currently leaking new_bs because it
only closes it and does not delete it!)

Re: [Qemu-devel] [PATCH] qed: replace vm_clock with rt_clock for qemu-tool compatibility

[Stefan Hajnoczi]

On Mon, Feb 27, 2012 at 9:31 AM, Kevin Wolf  wrote:
> Am 27.02.2012 09:42, schrieb Paolo Bonzini:
>> On 02/27/2012 08:35 AM, Zhi Yong Wu wrote:
>>> Since vm_clock is created via qemu_init_main_loop(), when QED read
>>> vm_clock, why will this call abort()?
>>> Can you elaborate this? what is its call path?
>>>
>>
>> It will crash in cpu_get_clock() (in qemu-tool.c).
>
> The fix isn't very nice if it makes migration impossible. I'd like to
> introduce a similar timer in qcow2 which does support migration and
> breaking it is not an option. So what about (completely untested)...
>
> diff --git a/qemu-tool.c b/qemu-tool.c
> index 183a583..edb84f5 100644
> --- a/qemu-tool.c
> +++ b/qemu-tool.c
> @@ -61,7 +61,7 @@ void monitor_protocol_event(MonitorEvent event,
> QObject *data)
>
>  int64_t cpu_get_clock(void)
>  {
> -    abort();
> +    return 0;
>  }

This is what we used to do, I'm okay with it.

Stefan

Re: [Qemu-devel] [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate commands

[Federico Simoncelli]

- Original Message -
> From: "Luiz Capitulino" 
> To: "Federico Simoncelli" 
> Cc: qemu-devel@nongnu.org, mtosa...@redhat.com, pbonz...@redhat.com, 
> kw...@redhat.com, arm...@redhat.com
> Sent: Friday, February 24, 2012 8:01:43 PM
> Subject: Re: [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate 
> commands
> 
> On Fri, 24 Feb 2012 16:49:04 +
> Federico Simoncelli  wrote:
> 
> > Signed-off-by: Federico Simoncelli 
> 
> Btw, would be nice to have a 0/2 intro email describing the feature
> and changelog
> info.

Yes the 0/2 (actually 0/3) was sent at the beginning of the thread so you might
have missed it because you were added later on but I'm sure you can still find
it in the archives.

> 
> > +BlockDriver *drv;
> > +int i, j, escape;
> > +char new_filename[2048], *filename;
> 
> I'd use PATH_MAX for new_filename's size.

Maybe PATH_MAX * 2 + 1? (To handle the case where all the characters should be
escaped).

> > +escape = 0;
> > +for (i = 0, j = 0; j < strlen(new_image_file); j++) {
> > + loop:
> > +if (!(i < sizeof(new_filename) - 2)) {
> > +error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> > +  "new-image-file", "shorter filename");
> > +return;
> > +}
> > +
> > +if (new_image_file[j] == ':' || new_image_file[j] == '\\')
> > {
> > +if (!escape) {
> > +new_filename[i++] = '\\', escape = 1;
> > +goto loop;
> > +} else {
> > +escape = 0;
> > +}
> > +}
> > +
> > +new_filename[i++] = new_image_file[j];
> > +}
> 
> IMO, you could require the filename arguments to be escaped already.

Can you be more explicit, who should escape it?
The only thing that comes to my mind right now is requiring the input of
blockdev-migrate already escaped but that would expose an internal format.
(I'd not recommend it).

> > +void qmp_blockdev_migrate(const char *device, bool incremental,
> > +  const char *destination, bool
> > has_new_image_file,
> > +  const char *new_image_file, Error
> > **errp)
> > +{
> > +BlockDriverState *bs;
> > +
> > +bs = bdrv_find(device);
> > +if (!bs) {
> > +error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> > +return;
> > +}
> > +if (bdrv_in_use(bs)) {
> > +error_set(errp, QERR_DEVICE_IN_USE, device);
> > +return;
> > +}
> > +
> > +if (incremental) {
> > +if (!has_new_image_file) {
> > +error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> > +  "incremental", "a new image file");
> > +} else {
> > +qmp_blockdev_mirror(device, destination,
> > new_image_file, errp);
> > +}
> > +} else {
> > +error_set(errp, QERR_NOT_SUPPORTED);
> > +}
> 
> The command documentation says that 'incremental' and
> 'new_image_file' are
> optionals, but the code makes them required. Why?

If I didn't make any mistake in the code I'm just enforcing that when
you specify "incremental" you also need a new image.
There are still other valid cases where they are optional.

-- 
Federico

Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver

[Paolo Bonzini]

On 02/27/2012 10:23 AM, Stefan Hajnoczi wrote:
> > I think the right approach is to create a single blkmirror driver that
> > also includes blkverify functionality.  The code is basically the same
> > except blkverify also compares reads - just use a flag to
> > enable/disable that behavior.
> >
> > Feel free to rename the blkverify driver to blkmirror if you wish.
>
> Federico: Any response to this?  I still think blkmirror and blkverify
> do essentially the same thing and should be unified.

Once non-incremental mode is added, I suspect blkmirror will diverge
from blkverify significantly.  In particular, we would need to track
where have writes been done in the destination.  We also would need to
hooks for block/stream.c, or perhaps a completely separate
implementation of streaming.

Also, blkverify doesn't support cancellation.  I know we do quite poorly
in this area, but I'd rather not make it worse...

Paolo

Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver

[Stefan Hajnoczi]

On Mon, Feb 27, 2012 at 11:37 AM, Paolo Bonzini  wrote:
> On 02/27/2012 10:23 AM, Stefan Hajnoczi wrote:
>> > I think the right approach is to create a single blkmirror driver that
>> > also includes blkverify functionality.  The code is basically the same
>> > except blkverify also compares reads - just use a flag to
>> > enable/disable that behavior.
>> >
>> > Feel free to rename the blkverify driver to blkmirror if you wish.
>>
>> Federico: Any response to this?  I still think blkmirror and blkverify
>> do essentially the same thing and should be unified.
>
> Once non-incremental mode is added, I suspect blkmirror will diverge
> from blkverify significantly.  In particular, we would need to track
> where have writes been done in the destination.  We also would need to
> hooks for block/stream.c, or perhaps a completely separate
> implementation of streaming.

I'm not familiar with non-incremental mode, could you please explain
it or link to it?

> Also, blkverify doesn't support cancellation.  I know we do quite poorly
> in this area, but I'd rather not make it worse...

That's why I suggested unifying them - take the best of both approaches.

Stefan

Re: [Qemu-devel] [PATCH] TCG: Convert global variables to be TLS.

[Evgeny Voevodin]

On 27.02.2012 15:06, Evgeny Voevodin wrote:


This patch converts some TCG data to be TLS on the way to make
TCG multithreaded. This work was made in assumption that qemu-tls.h defines
a general direction to let each VCPU run in its own thread wile being processed
by TCG.

Evgeny Voevodin (1):
   TCG: Convert global variables to be TLS.

  bsd-user/main.c|1 +
  cpus.c |2 +
  darwin-user/main.c |1 +
  exec.c |  121 ++-
  linux-user/main.c  |1 +
  qemu-common.h  |1 +
  6 files changed, 77 insertions(+), 50 deletions(-)



Just realised that the patch is buggy and does not work for -smp 1. 
Anyway, it is useful to get any suggestions on the general approach.


--
Kind regards,
Evgeny Voevodin,
Leading Software Engineer,
ASWG, Moscow R&D center, Samsung Electronics
e-mail: e.voevo...@samsung.com

Re: [Qemu-devel] [PATCH 5/6] usb-redir: Return USB_RET_NAK when we've no data for an interrupt endpoint

[Gerd Hoffmann]

On 02/26/12 16:14, Hans de Goede wrote:
> +if (status)
> +return usbredir_handle_status(dev, status, 0);

Fails checkpatch.pl, braces needed here.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver

[Paolo Bonzini]

On 02/27/2012 12:42 PM, Stefan Hajnoczi wrote:
>> >
>> > Once non-incremental mode is added, I suspect blkmirror will diverge
>> > from blkverify significantly.  In particular, we would need to track
>> > where have writes been done in the destination.  We also would need to
>> > hooks for block/stream.c, or perhaps a completely separate
>> > implementation of streaming.
> I'm not familiar with non-incremental mode, could you please explain
> it or link to it?

Non-incremental mode is "pre-copy" migration.  It would stream in the
background from the source to the destination.  In this case:

- you need to differentiate streaming writes from other writes.  When
streaming, you do not want to issue spurious writes to the source;

- you can skip streaming anything that has been written to the
destination already.  This means that you need: 1) a bitmap similar to
is_allocated; 2) to widen writes to a cluster; 3) serialization similar
to copy-on-read.

I'm not yet sure how much of this will be generalized in the block layer
and block/stream.c, and how much will be specific to blkmirror, but in
general none of this applies to blkverify.

Paolo

Re: [Qemu-devel] [PATCH 4/8] Add universal DMA helper functions

[Eduard - Gabriel Munteanu]

On Mon, Feb 27, 2012 at 12:33:49PM +0200, Michael S. Tsirkin wrote:
> On Mon, Feb 27, 2012 at 11:22:43AM +1100, David Gibson wrote:
> > On Sun, Feb 26, 2012 at 12:04:49PM +0200, Michael S. Tsirkin wrote:
> > > On Fri, Feb 24, 2012 at 02:27:39PM +1100, David Gibson wrote:

[snip]

> > > 
> > > I'm a bit confused with all the stubbing going on.
> > > Is this the final form of the pci_* functions or just
> > > a stub? If the final form, we probably should just
> > > open-code them - they don't buy us much.
> > > If not, let's add a comment?
> > 
> > Well.. it's the intended final form of pci_dma_*() - which do become
> > trivial wrappers, yes.
> 
> I'd say let's drop them then (in a follow-up patch).  The topic is
> confusing enough without having to wade through layers of wrappers :)
>

Drop them how? Using dma_* stuff directly? That might work, but I
remember others suggesting we should use a specialized PCI wrapper.
Perhaps it makes sense if some other bus, or PCI itself at some point,
needs to do something special.

> > It's _not_ the intended final form of dma_*(),
> > which need to grow code to do actual IOMMU translation.  I'll add a
> > comment about this in the next round.
> > 
> > -- 
> > David Gibson| I'll have my music baroque, and my 
> > code
> > david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ 
> > _other_
> > | _way_ _around_!
> > http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate commands

[Luiz Capitulino]

On Mon, 27 Feb 2012 06:29:39 -0500 (EST)
Federico Simoncelli  wrote:

> - Original Message -
> > From: "Luiz Capitulino" 
> > To: "Federico Simoncelli" 
> > Cc: qemu-devel@nongnu.org, mtosa...@redhat.com, pbonz...@redhat.com, 
> > kw...@redhat.com, arm...@redhat.com
> > Sent: Friday, February 24, 2012 8:01:43 PM
> > Subject: Re: [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate 
> > commands
> > 
> > On Fri, 24 Feb 2012 16:49:04 +
> > Federico Simoncelli  wrote:
> > 
> > > Signed-off-by: Federico Simoncelli 
> > 
> > Btw, would be nice to have a 0/2 intro email describing the feature
> > and changelog
> > info.
> 
> Yes the 0/2 (actually 0/3) was sent at the beginning of the thread so you 
> might
> have missed it because you were added later on but I'm sure you can still find
> it in the archives.

I only found v1 iirc, but this is not important right now, as you're going to
post v3 right? And that's going to have the intro :)

> > 
> > > +BlockDriver *drv;
> > > +int i, j, escape;
> > > +char new_filename[2048], *filename;
> > 
> > I'd use PATH_MAX for new_filename's size.
> 
> Maybe PATH_MAX * 2 + 1? (To handle the case where all the characters should be
> escaped).

Well, I was discussing this with Eric and he thinks that a value of 4096
should be fine. I personally prefer using PATH_MAX because I don't believe
I'm better at choosing a random value for this vs. using what the system
provides me.

Feel free to choose what you think is the best for this case.

> > > +escape = 0;
> > > +for (i = 0, j = 0; j < strlen(new_image_file); j++) {
> > > + loop:
> > > +if (!(i < sizeof(new_filename) - 2)) {
> > > +error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> > > +  "new-image-file", "shorter filename");
> > > +return;
> > > +}
> > > +
> > > +if (new_image_file[j] == ':' || new_image_file[j] == '\\')
> > > {
> > > +if (!escape) {
> > > +new_filename[i++] = '\\', escape = 1;
> > > +goto loop;
> > > +} else {
> > > +escape = 0;
> > > +}
> > > +}
> > > +
> > > +new_filename[i++] = new_image_file[j];
> > > +}
> > 
> > IMO, you could require the filename arguments to be escaped already.
> 
> Can you be more explicit, who should escape it?

Paolo thinks this should be done by the block layer, fine with me.

> The only thing that comes to my mind right now is requiring the input of
> blockdev-migrate already escaped but that would expose an internal format.
> (I'd not recommend it).
> 
> > > +void qmp_blockdev_migrate(const char *device, bool incremental,
> > > +  const char *destination, bool
> > > has_new_image_file,
> > > +  const char *new_image_file, Error
> > > **errp)
> > > +{
> > > +BlockDriverState *bs;
> > > +
> > > +bs = bdrv_find(device);
> > > +if (!bs) {
> > > +error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> > > +return;
> > > +}
> > > +if (bdrv_in_use(bs)) {
> > > +error_set(errp, QERR_DEVICE_IN_USE, device);
> > > +return;
> > > +}
> > > +
> > > +if (incremental) {
> > > +if (!has_new_image_file) {
> > > +error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> > > +  "incremental", "a new image file");
> > > +} else {
> > > +qmp_blockdev_mirror(device, destination,
> > > new_image_file, errp);
> > > +}
> > > +} else {
> > > +error_set(errp, QERR_NOT_SUPPORTED);
> > > +}
> > 
> > The command documentation says that 'incremental' and
> > 'new_image_file' are
> > optionals, but the code makes them required. Why?
> 
> If I didn't make any mistake in the code I'm just enforcing that when
> you specify "incremental" you also need a new image.
> There are still other valid cases where they are optional.

Which operation will be performed if 'incremental' is not passed? If
it's passed, which operation will be performed if 'new_image_file' is not?

[Qemu-devel] [PATCH v2] TCG: Convert global variables to be TLS.

[Evgeny Voevodin]

This commit converts code_gen_buffer, code_gen_ptr, tbs, nb_tbs to
TLS. We need this if we want TCG to become multithreaded.

Initialization of code_gen_buffer and code_gen_ptr is moved to new
tcg_gen_buffer_init() function. This is done because we do not need
to allocate and initialize TCG buffers for IO thread. Initialization
is now done in qemu_tcg_cpu_thread_fn() by each HW thread
individually.

Also tcg_enabled() returns a variable instead of
(code_gen_buffer != NULL) since if called from IO thread, this will
always return FALSE.

Also some code format changes.

Signed-off-by: Evgeny Voevodin 
---
 bsd-user/main.c|1 +
 cpus.c |2 +
 darwin-user/main.c |1 +
 exec.c |  123 +++-
 linux-user/main.c  |1 +
 qemu-common.h  |1 +
 6 files changed, 79 insertions(+), 50 deletions(-)

diff --git a/bsd-user/main.c b/bsd-user/main.c
index cc7d4a3..11e4540 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -906,6 +906,7 @@ int main(int argc, char **argv)
 #endif
 }
 tcg_exec_init(0);
+tcg_gen_buffer_init();
 cpu_exec_init_all();
 /* NOTE: we need to init the CPU at this stage to get
qemu_host_page_size */
diff --git a/cpus.c b/cpus.c
index f45a438..6190250 100644
--- a/cpus.c
+++ b/cpus.c
@@ -746,6 +746,8 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
 {
 CPUState *env = arg;
 
+tcg_gen_buffer_init();
+
 qemu_tcg_init_cpu_signals();
 qemu_thread_get_self(env->thread);
 
diff --git a/darwin-user/main.c b/darwin-user/main.c
index 9b57c20..8618a52 100644
--- a/darwin-user/main.c
+++ b/darwin-user/main.c
@@ -851,6 +851,7 @@ int main(int argc, char **argv)
 #endif
 }
 tcg_exec_init(0);
+tcg_gen_buffer_init();
 cpu_exec_init_all();
 /* NOTE: we need to init the CPU at this stage to get
qemu_host_page_size */
diff --git a/exec.c b/exec.c
index b81677a..cf673a5 100644
--- a/exec.c
+++ b/exec.c
@@ -79,10 +79,10 @@
 
 #define SMC_BITMAP_USE_THRESHOLD 10
 
-static TranslationBlock *tbs;
+static DEFINE_TLS(TranslationBlock*, tbs);
 static int code_gen_max_blocks;
 TranslationBlock *tb_phys_hash[CODE_GEN_PHYS_HASH_SIZE];
-static int nb_tbs;
+static DEFINE_TLS(int, nb_tbs);
 /* any access to the tbs or the page table must use this lock */
 spinlock_t tb_lock = SPIN_LOCK_UNLOCKED;
 
@@ -103,11 +103,12 @@ spinlock_t tb_lock = SPIN_LOCK_UNLOCKED;
 #endif
 
 uint8_t code_gen_prologue[1024] code_gen_section;
-static uint8_t *code_gen_buffer;
+static bool code_gen_enabled;
+static DEFINE_TLS(uint8_t*, code_gen_buffer);
 static unsigned long code_gen_buffer_size;
 /* threshold to flush the translated code buffer */
 static unsigned long code_gen_buffer_max_size;
-static uint8_t *code_gen_ptr;
+static DEFINE_TLS(uint8_t*, code_gen_ptr);
 
 #if !defined(CONFIG_USER_ONLY)
 int phys_ram_fd;
@@ -469,18 +470,17 @@ static void tlb_unprotect_code_phys(CPUState *env, 
ram_addr_t ram_addr,
 #endif
 
 #ifdef USE_STATIC_CODE_GEN_BUFFER
-static uint8_t static_code_gen_buffer[DEFAULT_CODE_GEN_BUFFER_SIZE]
-   __attribute__((aligned (CODE_GEN_ALIGN)));
+static DEFINE_TLS(uint8_t [DEFAULT_CODE_GEN_BUFFER_SIZE],
+static_code_gen_buffer) __attribute__((aligned(CODE_GEN_ALIGN)));
 #endif
 
-static void code_gen_alloc(unsigned long tb_size)
+static void code_gen_alloc(void)
 {
 #ifdef USE_STATIC_CODE_GEN_BUFFER
-code_gen_buffer = static_code_gen_buffer;
+tls_var(code_gen_buffer) = tls_var(static_code_gen_buffer);
 code_gen_buffer_size = DEFAULT_CODE_GEN_BUFFER_SIZE;
-map_exec(code_gen_buffer, code_gen_buffer_size);
+map_exec(tls_var(code_gen_buffer), code_gen_buffer_size);
 #else
-code_gen_buffer_size = tb_size;
 if (code_gen_buffer_size == 0) {
 #if defined(CONFIG_USER_ONLY)
 code_gen_buffer_size = DEFAULT_CODE_GEN_BUFFER_SIZE;
@@ -522,10 +522,10 @@ static void code_gen_alloc(unsigned long tb_size)
 }
 start = (void *)0x9000UL;
 #endif
-code_gen_buffer = mmap(start, code_gen_buffer_size,
+tls_var(code_gen_buffer) = mmap(start, code_gen_buffer_size,
PROT_WRITE | PROT_READ | PROT_EXEC,
flags, -1, 0);
-if (code_gen_buffer == MAP_FAILED) {
+if (tls_var(code_gen_buffer) == MAP_FAILED) {
 fprintf(stderr, "Could not allocate dynamic translator buffer\n");
 exit(1);
 }
@@ -553,24 +553,30 @@ static void code_gen_alloc(unsigned long tb_size)
 code_gen_buffer_size = (512 * 1024 * 1024);
 }
 #endif
-code_gen_buffer = mmap(addr, code_gen_buffer_size,
+tls_var(code_gen_buffer) = mmap(addr, code_gen_buffer_size,
PROT_WRITE | PROT_READ | PROT_EXEC, 
flags, -1, 0);
-if (code_gen_buffer == MAP_FAILED) {
+if (tls_var(code_gen_buffer) == MAP_FAILED) {
 fprintf(stderr, "Could 

Re: [Qemu-devel] [PATCH 5/6] usb-redir: Return USB_RET_NAK when we've no data for an interrupt endpoint

[Gerd Hoffmann]

On 02/27/12 12:45, Gerd Hoffmann wrote:
> On 02/26/12 16:14, Hans de Goede wrote:
>> +if (status)
>> +return usbredir_handle_status(dev, status, 0);
> 
> Fails checkpatch.pl, braces needed here.

Fixed & applied (all patches).

cheers,
  Gerd

[Qemu-devel] [PATCH 1/3] build: replace librt check function

[Roger Pau Monne]

Replace clock_gettime with timer_gettime, since at least under
uclibc 0.9.33 the clock_getttime function can be used without linking
against librt (although the manual page states the opposite).

Signed-off-by: Roger Pau Monne 
---
 configure |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/configure b/configure
index f9d5330..68eb3fa 100755
--- a/configure
+++ b/configure
@@ -2513,7 +2513,8 @@ fi
 cat > $TMPC <
 #include 
-int main(void) { return clock_gettime(CLOCK_REALTIME, NULL); }
+int main(void) { timer_t tid; struct itimerspec it; \
+ return timer_gettime(tid, &it); }
 EOF
 
 if compile_prog "" "" ; then
-- 
1.7.9

[Qemu-devel] [PATCH 3/3] build: check if libm is needed in configure

[Roger Pau Monne]

Remove the hardcoded use of libm and instead rely on configure to
check for it. It is needed at least for qemu-ga and qemu-system.

Signed-off-by: Roger Pau Monne 
---
 Makefile.target |4 
 configure   |   14 ++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/Makefile.target b/Makefile.target
index 68a5641..c230aff 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -42,10 +42,6 @@ PROGS+=$(QEMU_PROGW)
 endif
 STPFILES=
 
-ifndef CONFIG_HAIKU
-LIBS+=-lm
-endif
-
 config-target.h: config-target.h-timestamp
 config-target.h-timestamp: config-target.mak
 
diff --git a/configure b/configure
index 790d495..b0cb175 100755
--- a/configure
+++ b/configure
@@ -2524,6 +2524,20 @@ elif compile_prog "" "-lrt" ; then
   libs_qga="-lrt $libs_qga"
 fi
 
+##
+# Do we need libm
+cat > $TMPC <
+int main(void) { double a, b; return modf(a, &b);}
+EOF
+
+if compile_prog "" "" ; then
+  :
+elif compile_prog "" "-lm" ; then
+  LIBS="-lm $LIBS"
+  libs_qga="-lm $libs_qga"
+fi
+
 if test "$darwin" != "yes" -a "$mingw32" != "yes" -a "$solaris" != yes -a \
 "$aix" != "yes" -a "$haiku" != "yes" ; then
 libs_softmmu="-lutil $libs_softmmu"
-- 
1.7.9

[Qemu-devel] [PATCH 2/3] build: add librt to libs_qga

[Roger Pau Monne]

librt is needed to link qemu-ga.

Signed-off-by: Roger Pau Monne 
---
 configure |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/configure b/configure
index 68eb3fa..790d495 100755
--- a/configure
+++ b/configure
@@ -2521,6 +2521,7 @@ if compile_prog "" "" ; then
   :
 elif compile_prog "" "-lrt" ; then
   LIBS="-lrt $LIBS"
+  libs_qga="-lrt $libs_qga"
 fi
 
 if test "$darwin" != "yes" -a "$mingw32" != "yes" -a "$solaris" != yes -a \
-- 
1.7.9

Re: [Qemu-devel] [PATCH v2] TCG: Convert global variables to be TLS.

[Peter Maydell]

On 27 February 2012 12:13, Evgeny Voevodin  wrote:
> This commit converts code_gen_buffer, code_gen_ptr, tbs, nb_tbs to
> TLS. We need this if we want TCG to become multithreaded.

I'm sceptical about doing this kind of thing as a change on its
own. A true multithreaded TCG is a large project, and unless we're
going to commit to doing that I don't see much value in making
some variables per-thread when we might instead need to do
larger refactorings (properly encapsulating the codegen
caches as qom objects, maybe?).

-- PMM

Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] build: replace librt check function

[Stefano Stabellini]

On Mon, 27 Feb 2012, Ian Campbell wrote:
> On Thu, 2012-02-23 at 13:34 +, Roger Pau Monné wrote:
> > 2012/2/22 Anthony Liguori :
> > > On 02/20/2012 06:11 AM, Roger Pau Monne wrote:
> > >>
> > >> Replace clock_gettime with timer_gettime, since at least under
> > >> uclibc 0.9.33 the clock_getttime function can be used without linking
> > >> against librt (although the manual page states the opposite).
> > >>
> > >> Signed-off-by: Roger Pau Monne
> > >
> > >
> > > I don't think this is against qemu.git.  Please do not send patches to
> > > qemu-devel that are not against qemu.git without clearly indicating this.
> > 
> > Sorry, this is against qemu-xen-upstream, should I add a tag to my
> > patches and resend them to qemu-devel?
> 
> qemu-xen-upstream is based upon the upstream qemu stable branch, not the
> development branch.
> 
> However patches for qemu-xen-upstream should have already been applied
> upstream. This means that patches should be against the mainline (e.g.
> development branch) version of qemu and sent to qemu-devel. Once
> committed there they will be backported as required to the stable branch
> maintained by Xen.org as qemu-xen-upstream.
> 
> (Stefano, is that correct? Is there somewhere we can (or do!) document
> this? Perhaps http://wiki.xen.org/xenwiki/QEMUUpstream linked to from
> Submitting_Xen_Patches? The backport policy should also be covered, e.g.
> http://marc.info/?l=xen-devel&m=132872001218724 )

Yes, it is correct.

I added few lines to
http://wiki.xen.org/wiki?title=Submitting_Xen_Patches to explain this
concept. I have also linked http://wiki.qemu.org/Contribute/SubmitAPatch
that is the reference on how to submit patches to qemu-devel.

Re: [Qemu-devel] qemu assertion failed with usb on current git master!

[Gerd Hoffmann]

>>> qemu-system-x86_64: /home/erik/qemu/hw/usb.c:358 usb_packet_complete:
>>> Assertion 'p->state == USB_PACKET_QUEUED' failed.
>>
>> Stacktrace?
>> What kind of device?
>>

> Hi Gerd,
> 
> attached the usb logger dump as requested.
> Stacktrace - I tried that without real success using the gdbserver - I had
> issues with the symbol resolving, no idea what went wrong...

You don't need gdbserver, that one is for debugging the guest, not for
debugging qemu.

Just use gdb on the qemu core dump.  And please also print p (packet
pointer) which fails the assertion, so we can match it with the packet
pointers printed in the log.

thanks,
  Gerd

Re: [Qemu-devel] [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate commands

[Paolo Bonzini]

On 02/27/2012 01:12 PM, Luiz Capitulino wrote:
> > If I didn't make any mistake in the code I'm just enforcing that when
> > you specify "incremental" you also need a new image.
> > There are still other valid cases where they are optional.
> 
> Which operation will be performed if 'incremental' is not passed? If
> it's passed, which operation will be performed if 'new_image_file' is not?

There are four cases:

1) incremental=false, new_image_file not passed:
   right now fail; in the future:
   - create an image on dest with no backing file;
   - writes will be mirrored to current_image_file and dest
   - start streaming from current_image_file to dest

2) incremental=false, new_image_file passed:
   right now fail; in the future:
   - create an image on dest with no backing file;
   - live-snapshot based on current_image_file to new_image_file;
   - writes will be mirrored to new_image_file and dest
   - start streaming from current_image_file to dest

3) incremental=true, new_image_file not passed:
   - error

4) incremental=true, new_image_file passed:
   - no images will be created
   - writes will be mirrored to new_image_file and dest

Paolo

Re: [Qemu-devel] [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate commands

[Luiz Capitulino]

On Mon, 27 Feb 2012 13:49:17 +0100
Paolo Bonzini  wrote:

> On 02/27/2012 01:12 PM, Luiz Capitulino wrote:
> > > If I didn't make any mistake in the code I'm just enforcing that when
> > > you specify "incremental" you also need a new image.
> > > There are still other valid cases where they are optional.
> > 
> > Which operation will be performed if 'incremental' is not passed? If
> > it's passed, which operation will be performed if 'new_image_file' is not?
> 
> There are four cases:
> 
> 1) incremental=false, new_image_file not passed:
>right now fail; in the future:
>- create an image on dest with no backing file;
>- writes will be mirrored to current_image_file and dest
>- start streaming from current_image_file to dest
> 
> 2) incremental=false, new_image_file passed:
>right now fail; in the future:
>- create an image on dest with no backing file;
>- live-snapshot based on current_image_file to new_image_file;
>- writes will be mirrored to new_image_file and dest
>- start streaming from current_image_file to dest
>
> 
> 3) incremental=true, new_image_file not passed:
>- error
> 
> 4) incremental=true, new_image_file passed:
>- no images will be created
>- writes will be mirrored to new_image_file and dest

IMHO, this is asking for two commands, where cases 1 & 2 is one of them
and cases 3 & 4 is the other one. Note how 'incremental' goes away and
'new_image_file' really becomes an optional.

Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver

[Stefan Hajnoczi]

On Mon, Feb 27, 2012 at 11:48 AM, Paolo Bonzini  wrote:
> On 02/27/2012 12:42 PM, Stefan Hajnoczi wrote:
>>> >
>>> > Once non-incremental mode is added, I suspect blkmirror will diverge
>>> > from blkverify significantly.  In particular, we would need to track
>>> > where have writes been done in the destination.  We also would need to
>>> > hooks for block/stream.c, or perhaps a completely separate
>>> > implementation of streaming.
>> I'm not familiar with non-incremental mode, could you please explain
>> it or link to it?
>
> Non-incremental mode is "pre-copy" migration.  It would stream in the
> background from the source to the destination.  In this case:
>
> - you need to differentiate streaming writes from other writes.  When
> streaming, you do not want to issue spurious writes to the source;
>
> - you can skip streaming anything that has been written to the
> destination already.  This means that you need: 1) a bitmap similar to
> is_allocated; 2) to widen writes to a cluster; 3) serialization similar
> to copy-on-read.
>
> I'm not yet sure how much of this will be generalized in the block layer
> and block/stream.c, and how much will be specific to blkmirror, but in
> general none of this applies to blkverify.

Agreed but I'm not sure it has to do with blkmirror either.  blkmirror
and blkverify perform the same function except that blkverify mirrors
reads too and checks that they match.

Who knows when and if pre-copy will be (re)implemented, it's not a
good argument to say let's duplicate block mirroring because we're not
sure about the design of a feature feature yet which might want to
hook in here.

Stefan

Re: [Qemu-devel] [PATCH 0/8] Add GTK UI to enable basic accessibility (v2)

[Anthony Liguori]

On 02/27/2012 02:21 AM, Jan Kiszka wrote:

On 2012-02-27 00:46, Anthony Liguori wrote:

I realize UIs are the third rail of QEMU development, but over the years I've
gotten a lot of feedback from users about our UI.  I think everyone struggles
with the SDL interface and its lack of discoverability but it's worse than I
think most people realize for users that rely on accessibility tools.

The two pieces of feedback I've gotten the most re: accessibility are the lack
of QEMU's enablement for screen readers and the lack of configurable
accelerators.

Since we render our own terminal using a fixed sized font, we don't respect
system font settings which means we ignore if the user has configured large
print.

We also don't integrate at all with screen readers which means that for blind
users, the virtual consoles may as well not even exist.

We also don't allow any type of configuration of accelerators.  For users with
limited dexterity (this is actually more common than you would think), they may
use an input device that only inputs one key at a time.  Holding down two keys
at once is not possible for these users.

These are solved problems though and while we could reinvent all of this
ourselves with SDL, we would be crazy if we did.  Modern toolkits, like GTK,
solve these problems.

By using GTK, we can leverage VteTerminal for screen reader integration and font
configuration.  We can also use GTK's accelerator support to make accelerators
configurable (Gnome provides a global accelerator configuration interface).

I'm not attempting to make a pretty desktop virtualization UI.  Maybe we'll go
there eventually but that's not what this series is about.

This is just attempting to use a richer toolkit such that we can enable basic
accessibility support.  As a consequence, the UI is much more usable even for a
user without accessibility requirements so it's a win-win.

Also available at:

https://github.com/aliguori/qemu/tree/gtk.2

---
v1 ->  v2
  - Add internationalization support.  I don't actually speak any other 
languages
so I added a placeholder for a German translation.  This can be tested with
LANGUAGE=de_DE.UTF-8 qemu-system-x86_64
  - Fixed the terminal size for VteTerminal widgets.  I think the behavior makes
sense now.
  - Fixed lots of issues raised in review comments (see individual patches)

Known Issues:
  - I saw the X crash once.  I think it has to do with widget sizes.  I need to
work harder to reproduce.
  - I've not recreated the reported memory leak yet.
  - I haven't added backwards compatibility code for older VteTerminal widgets
yet.


Looks quite nice but still has some rough edges:
- full screen doesn't work, at least here


How does it fail?  It works for me.  What distro are you on?


- lacking support for auto-grabbing in absolute mouse mode


What do you mean by auto grabbing?


- unscaling (ctrl-alt-u) is lacking


Since we scale by 25% up and down, I figured it wasn't strictly necessary 
anymore because it's very easy to zoom back to the original size.  It's easy 
enough to add though.



- window not resizable (except in broken full-screen mode)


That's intentional.



Will see if I find some time to look into this.

Is this also working properly under Windows? Otherwise we probably can't
deprecate SDL - or would have to provide a native Windows GUI.


It should, but it doesn't right now most likely because of the glib event loop 
being broken on win32.



As we have a menu now, I would suggest to add some handy monitor
commands there as well, like reset or powerdown.


Absolutely.  I wanted to start with something very simple though first.

Regards,

Anthony Liguori


Jan

Re: [Qemu-devel] [PATCH 7/8] gtk: add translation support

[Anthony Liguori]

On 02/27/2012 02:32 AM, Paolo Bonzini wrote:

On 02/27/2012 12:46 AM, Anthony Liguori wrote:

The de_DE translation is just a placeholder so that I could test the
infrastructure.


Here is an it_IT translation that you can use instead.


Grazie!

Regards,

Anthony Liguori



Paolo

[Qemu-devel] [PATCH] qed: do not evict in-use L2 table cache entries

[Stefan Hajnoczi]

The L2 table cache reduces QED metadata reads that would be required
when translating LBAs to offsets into the image file.  Since requests
execute in parallel it is possible to share an L2 table between multiple
requests.

There is a potential data corruption issue when an in-use L2 table is
evicted from the cache because the following situation occurs:

  1. An allocating write performs an update to L2 table "A".

  2. Another request needs L2 table "B" and causes table "A" to be
 evicted.

  3. A new read request needs L2 table "A" but it is not cached.

As a result the L2 update from #1 can overlap with the L2 fetch from #3.
We must avoid doing overlapping I/O requests here since the worst case
outcome is that the L2 fetch completes before the L2 update and yields
stale data.  In that case we would effectively discard the L2 update and
lose data clusters!

Thanks to Benoît Canet  for extensive testing
and debugging which lead to discovery of this bug.

Reported-by: Benoît Canet 
Signed-off-by: Stefan Hajnoczi 
---
Please include this in -stable once it has been merged into qemu.git/master.

 block/qed-l2-cache.c |   22 ++
 1 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/block/qed-l2-cache.c b/block/qed-l2-cache.c
index 02b81a2..e9b2aae 100644
--- a/block/qed-l2-cache.c
+++ b/block/qed-l2-cache.c
@@ -161,11 +161,25 @@ void qed_commit_l2_cache_entry(L2TableCache *l2_cache, 
CachedL2Table *l2_table)
 return;
 }
 
+/* Evict an unused cache entry so we have space.  If all entries are in use
+ * we can grow the cache temporarily and we try to shrink back down later.
+ */
 if (l2_cache->n_entries >= MAX_L2_CACHE_SIZE) {
-entry = QTAILQ_FIRST(&l2_cache->entries);
-QTAILQ_REMOVE(&l2_cache->entries, entry, node);
-l2_cache->n_entries--;
-qed_unref_l2_cache_entry(entry);
+CachedL2Table *next;
+QTAILQ_FOREACH_SAFE(entry, &l2_cache->entries, node, next) {
+if (entry->ref > 1) {
+continue;
+}
+
+QTAILQ_REMOVE(&l2_cache->entries, entry, node);
+l2_cache->n_entries--;
+qed_unref_l2_cache_entry(entry);
+
+/* Stop evicting when we've shrunk back to max size */
+if (l2_cache->n_entries < MAX_L2_CACHE_SIZE) {
+break;
+}
+}
 }
 
 l2_cache->n_entries++;
-- 
1.7.9

Re: [Qemu-devel] [PATCH 0/8] Add GTK UI to enable basic accessibility (v2)

[Jan Kiszka]

On 2012-02-27 14:10, Anthony Liguori wrote:
> On 02/27/2012 02:21 AM, Jan Kiszka wrote:
>> On 2012-02-27 00:46, Anthony Liguori wrote:
>>> I realize UIs are the third rail of QEMU development, but over the
>>> years I've
>>> gotten a lot of feedback from users about our UI.  I think everyone
>>> struggles
>>> with the SDL interface and its lack of discoverability but it's worse
>>> than I
>>> think most people realize for users that rely on accessibility tools.
>>>
>>> The two pieces of feedback I've gotten the most re: accessibility are
>>> the lack
>>> of QEMU's enablement for screen readers and the lack of configurable
>>> accelerators.
>>>
>>> Since we render our own terminal using a fixed sized font, we don't
>>> respect
>>> system font settings which means we ignore if the user has configured
>>> large
>>> print.
>>>
>>> We also don't integrate at all with screen readers which means that
>>> for blind
>>> users, the virtual consoles may as well not even exist.
>>>
>>> We also don't allow any type of configuration of accelerators.  For
>>> users with
>>> limited dexterity (this is actually more common than you would
>>> think), they may
>>> use an input device that only inputs one key at a time.  Holding down
>>> two keys
>>> at once is not possible for these users.
>>>
>>> These are solved problems though and while we could reinvent all of this
>>> ourselves with SDL, we would be crazy if we did.  Modern toolkits,
>>> like GTK,
>>> solve these problems.
>>>
>>> By using GTK, we can leverage VteTerminal for screen reader
>>> integration and font
>>> configuration.  We can also use GTK's accelerator support to make
>>> accelerators
>>> configurable (Gnome provides a global accelerator configuration
>>> interface).
>>>
>>> I'm not attempting to make a pretty desktop virtualization UI.  Maybe
>>> we'll go
>>> there eventually but that's not what this series is about.
>>>
>>> This is just attempting to use a richer toolkit such that we can
>>> enable basic
>>> accessibility support.  As a consequence, the UI is much more usable
>>> even for a
>>> user without accessibility requirements so it's a win-win.
>>>
>>> Also available at:
>>>
>>> https://github.com/aliguori/qemu/tree/gtk.2
>>>
>>> ---
>>> v1 ->  v2
>>>   - Add internationalization support.  I don't actually speak any
>>> other languages
>>> so I added a placeholder for a German translation.  This can be
>>> tested with
>>> LANGUAGE=de_DE.UTF-8 qemu-system-x86_64
>>>   - Fixed the terminal size for VteTerminal widgets.  I think the
>>> behavior makes
>>> sense now.
>>>   - Fixed lots of issues raised in review comments (see individual
>>> patches)
>>>
>>> Known Issues:
>>>   - I saw the X crash once.  I think it has to do with widget sizes. 
>>> I need to
>>> work harder to reproduce.
>>>   - I've not recreated the reported memory leak yet.
>>>   - I haven't added backwards compatibility code for older
>>> VteTerminal widgets
>>> yet.
>>
>> Looks quite nice but still has some rough edges:
>> - full screen doesn't work, at least here
> 
> How does it fail? 

The window changes to resizable mode, but remains a decorated window.

> It works for me.  What distro are you on?

OpenSUSE 11.4, gnome2.

> 
>> - lacking support for auto-grabbing in absolute mouse mode
> 
> What do you mean by auto grabbing?

That all keyboard inputs are grabbed when the mouse is in the window and
you don't need to press ctrl-alt-g explicitly. And the reverse should
happen when the mouse reaches the window border. Just like under SDL,
give it a try.

> 
>> - unscaling (ctrl-alt-u) is lacking
> 
> Since we scale by 25% up and down, I figured it wasn't strictly
> necessary anymore because it's very easy to zoom back to the original
> size.  It's easy enough to add though.

It's mandatory for usability IMHO. You find this "back to 1:1 view" in
almost every application that supports scaling of its view, and we even
have a pre-defined shortcut for it for some releases now.

> 
>> - window not resizable (except in broken full-screen mode)
> 
> That's intentional.

And a mistake IMHO. Definitely for the text consoles, but one can also
argue about the guest graphic console. I think Stefano once introduced
this for some Xen use case, maybe he can comment on it.

BTW, "VGA" is the wrong term for the graphic console. Maybe there is the
real front-end name available somewhere and could be used instead.

> 
>>
>> Will see if I find some time to look into this.
>>
>> Is this also working properly under Windows? Otherwise we probably can't
>> deprecate SDL - or would have to provide a native Windows GUI.
> 
> It should, but it doesn't right now most likely because of the glib
> event loop being broken on win32.

Sounds great.

> 
>> As we have a menu now, I would suggest to add some handy monitor
>> commands there as well, like reset or powerdown.
> 
> Absolutely.  I wanted to start with something very simple though first.
> 
> Regards,
> 
> Anthony Liguori
> 
>> Jan
>>

[Qemu-devel] KVM call agenda for Tuesday 28th

[Juan Quintela]

Hi

Please send in any agenda items you are interested in covering.

Cheers,

Juan.

Re: [Qemu-devel] [PATCH 8/8] Make dma_addr_t 64 bit always

[David Gibson]

On Sun, Feb 26, 2012 at 09:46:52PM +, Blue Swirl wrote:
> On Fri, Feb 24, 2012 at 04:57, David Gibson  
> wrote:
> > On Fri, Feb 24, 2012 at 02:27:43PM +1100, David Gibson wrote:
> >
> > Oops, ignore this one folks.  As is probably obvious, this was a
> > testing patch not meant to go into the main series.
> 
> Actually I'm not sure what would be the correct way to calculate the
> size. For example, on Sparc32 the virtual address space and CPU
> registers are 32 bits, physical address space 36 bits, but device
> virtual memory address space (DVMA, used by devices to talk to IOMMU)
> is only 32 bits.

Well, yes, in fact I think we probably do need to use 64-bit addresses
always (or at least whenever we have any kind of IOMMU support
available), because it's almost impossible to be certain that a
platform could never have a 64-bit bus of some kind plugged in.

But that's a matter for another day.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH 0/8] Add GTK UI to enable basic accessibility (v2)

[Anthony Liguori]

On 02/27/2012 07:26 AM, Jan Kiszka wrote:

On 2012-02-27 14:10, Anthony Liguori wrote:

On 02/27/2012 02:21 AM, Jan Kiszka wrote:

On 2012-02-27 00:46, Anthony Liguori wrote:

I realize UIs are the third rail of QEMU development, but over the
years I've
gotten a lot of feedback from users about our UI.  I think everyone
struggles
with the SDL interface and its lack of discoverability but it's worse
than I
think most people realize for users that rely on accessibility tools.

The two pieces of feedback I've gotten the most re: accessibility are
the lack
of QEMU's enablement for screen readers and the lack of configurable
accelerators.

Since we render our own terminal using a fixed sized font, we don't
respect
system font settings which means we ignore if the user has configured
large
print.

We also don't integrate at all with screen readers which means that
for blind
users, the virtual consoles may as well not even exist.

We also don't allow any type of configuration of accelerators.  For
users with
limited dexterity (this is actually more common than you would
think), they may
use an input device that only inputs one key at a time.  Holding down
two keys
at once is not possible for these users.

These are solved problems though and while we could reinvent all of this
ourselves with SDL, we would be crazy if we did.  Modern toolkits,
like GTK,
solve these problems.

By using GTK, we can leverage VteTerminal for screen reader
integration and font
configuration.  We can also use GTK's accelerator support to make
accelerators
configurable (Gnome provides a global accelerator configuration
interface).

I'm not attempting to make a pretty desktop virtualization UI.  Maybe
we'll go
there eventually but that's not what this series is about.

This is just attempting to use a richer toolkit such that we can
enable basic
accessibility support.  As a consequence, the UI is much more usable
even for a
user without accessibility requirements so it's a win-win.

Also available at:

https://github.com/aliguori/qemu/tree/gtk.2

---
v1 ->   v2
   - Add internationalization support.  I don't actually speak any
other languages
 so I added a placeholder for a German translation.  This can be
tested with
 LANGUAGE=de_DE.UTF-8 qemu-system-x86_64
   - Fixed the terminal size for VteTerminal widgets.  I think the
behavior makes
 sense now.
   - Fixed lots of issues raised in review comments (see individual
patches)

Known Issues:
   - I saw the X crash once.  I think it has to do with widget sizes.
I need to
 work harder to reproduce.
   - I've not recreated the reported memory leak yet.
   - I haven't added backwards compatibility code for older
VteTerminal widgets
 yet.


Looks quite nice but still has some rough edges:
- full screen doesn't work, at least here


How does it fail?


The window changes to resizable mode, but remains a decorated window.


It works for me.  What distro are you on?


OpenSUSE 11.4, gnome2.


Okay, I'll setup a system and test it out.


- lacking support for auto-grabbing in absolute mouse mode


What do you mean by auto grabbing?


That all keyboard inputs are grabbed when the mouse is in the window and
you don't need to press ctrl-alt-g explicitly. And the reverse should
happen when the mouse reaches the window border. Just like under SDL,
give it a try.


Right, this was intentional.  I think it's weird that a window would steal input 
when the mouse moves over it breaking desktop accelerators like alt tab.  If 
you've ever alt-tab cycled through windows when QEMU is running, if you're 
unlucky enough to have the mouse where a QEMU window may be, the QEMU instance 
will steal input and prevent alt-tab from working anymore.



- unscaling (ctrl-alt-u) is lacking


Since we scale by 25% up and down, I figured it wasn't strictly
necessary anymore because it's very easy to zoom back to the original
size.  It's easy enough to add though.


It's mandatory for usability IMHO. You find this "back to 1:1 view" in
almost every application that supports scaling of its view, and we even
have a pre-defined shortcut for it for some releases now.


I'll add it.


- window not resizable (except in broken full-screen mode)


That's intentional.


And a mistake IMHO. Definitely for the text consoles, but one can also
argue about the guest graphic console. I think Stefano once introduced
this for some Xen use case, maybe he can comment on it.


If we added resize, I wouldn't want to scale-to-size.  I find this behavior to 
be more trouble than it's worth.  I'd want to render the VGA screen with a black 
border only scaling based on the zoom settings.



BTW, "VGA" is the wrong term for the graphic console. Maybe there is the
real front-end name available somewhere and could be used instead.


Any suggestions?  Display?  Graphics?

Or were you thinking something like Cirrus VGA?

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 0/8] Add GTK UI to enable basic accessibility (v2)

[Jan Kiszka]

On 2012-02-27 14:33, Anthony Liguori wrote:
>> That all keyboard inputs are grabbed when the mouse is in the window and
>> you don't need to press ctrl-alt-g explicitly. And the reverse should
>> happen when the mouse reaches the window border. Just like under SDL,
>> give it a try.
> 
> Right, this was intentional.  I think it's weird that a window would steal 
> input 
> when the mouse moves over it breaking desktop accelerators like alt tab.  If 
> you've ever alt-tab cycled through windows when QEMU is running, if you're 
> unlucky enough to have the mouse where a QEMU window may be, the QEMU 
> instance 
> will steal input and prevent alt-tab from working anymore.

This would be a usability regression. It is very unhandy to switch
between grabbed and ungrabbed, specifically for alt-tab, when working
with guests vs. host windows. Look e.g. at how rdeskop works in this
regard. That's why I introduced this feature to QEMU, and I would not
want to see it die again with the introduction of GTK.

 - window not resizable (except in broken full-screen mode)
>>>
>>> That's intentional.
>>
>> And a mistake IMHO. Definitely for the text consoles, but one can also
>> argue about the guest graphic console. I think Stefano once introduced
>> this for some Xen use case, maybe he can comment on it.
> 
> If we added resize, I wouldn't want to scale-to-size.  I find this behavior 
> to 
> be more trouble than it's worth.  I'd want to render the VGA screen with a 
> black 
> border only scaling based on the zoom settings.

OK for aspect-ratio-correct scaling of the guest view from my POV. And
if there is a use case for the old behavior, we could still add a switch
to the menu for selecting the scaling mode.

> 
>> BTW, "VGA" is the wrong term for the graphic console. Maybe there is the
>> real front-end name available somewhere and could be used instead.
> 
> Any suggestions?  Display?  Graphics?
> 
> Or were you thinking something like Cirrus VGA?

VGA is (mostly) x86. Also, we may once have multiple guest screens,
maybe even multiple types of them. So picking up some telling front-end
name would likely scale best.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver

[Paolo Bonzini]

On 02/27/2012 02:09 PM, Stefan Hajnoczi wrote:
> > Non-incremental mode is "pre-copy" migration.  It would stream in the
> > background from the source to the destination.  In this case:
> >
> > - you need to differentiate streaming writes from other writes.  When
> > streaming, you do not want to issue spurious writes to the source;
> >
> > - you can skip streaming anything that has been written to the
> > destination already.  This means that you need: 1) a bitmap similar to
> > is_allocated; 2) to widen writes to a cluster; 3) serialization similar
> > to copy-on-read.
> >
> > I'm not yet sure how much of this will be generalized in the block layer
> > and block/stream.c, and how much will be specific to blkmirror, but in
> > general none of this applies to blkverify.
> 
> Agreed but I'm not sure it has to do with blkmirror either.  blkmirror
> and blkverify perform the same function except that blkverify mirrors
> reads too and checks that they match.
> 
> Who knows when and if pre-copy will be (re)implemented, it's not a
> good argument to say let's duplicate block mirroring because we're not
> sure about the design of a feature feature yet which might want to
> hook in here.

It's not a duplicate, it just happens that two very simple drivers share
1 operation out of 4 (open, read, write, flush).  There are other
differences, for example:

1) blkverify hardcodes raw for one format and auto-detects the other.
blkmirror needs to have a specified format for both, and I don't think
starting another bikeshedding discussion on blkverify backwards
compatibility is a good idea;

2) blkverify doesn't flush the raw file;

3) blkverify in the future could add callbacks to create snapshots or
load/save imgstate, and forward them to the test file; this doesn't make
sense for blkmirror.

Paolo

Re: [Qemu-devel] [PATCH][v14] megasas: LSI Megaraid SAS HBA emulation

[Andreas Färber]

Am 23.02.2012 16:34, schrieb Michael S. Tsirkin:
> On Tue, Feb 21, 2012 at 10:36:43AM +0100, Hannes Reinecke wrote:
>> diff --git a/hw/megasas.c b/hw/megasas.c
>> new file mode 100644
>> index 000..083c3d3
>> --- /dev/null
>> +++ b/hw/megasas.c

>> +static void megaraid1078_register_types(void)
>> +{
>> +type_register_static(&megasas_info);
>> +}
>> +
>> +type_init(megaraid1078_register_types);
> 
> 
> why not megasas_ ?

And while at it, no semicolon after type_init() please. It defines a
function and I recently cleaned up all callers.

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 0/8] Add GTK UI to enable basic accessibility (v2)

[Anthony Liguori]

On 02/27/2012 07:42 AM, Jan Kiszka wrote:

On 2012-02-27 14:33, Anthony Liguori wrote:

That all keyboard inputs are grabbed when the mouse is in the window and
you don't need to press ctrl-alt-g explicitly. And the reverse should
happen when the mouse reaches the window border. Just like under SDL,
give it a try.


Right, this was intentional.  I think it's weird that a window would steal input
when the mouse moves over it breaking desktop accelerators like alt tab.  If
you've ever alt-tab cycled through windows when QEMU is running, if you're
unlucky enough to have the mouse where a QEMU window may be, the QEMU instance
will steal input and prevent alt-tab from working anymore.


This would be a usability regression. It is very unhandy to switch
between grabbed and ungrabbed, specifically for alt-tab, when working
with guests vs. host windows. Look e.g. at how rdeskop works in this
regard. That's why I introduced this feature to QEMU, and I would not
want to see it die again with the introduction of GTK.


I'll add a "Grab on Hover" menu item to enable the behavior.  It's a good excuse 
to look at gconf to store GUI preferences too.



- window not resizable (except in broken full-screen mode)


That's intentional.


And a mistake IMHO. Definitely for the text consoles, but one can also
argue about the guest graphic console. I think Stefano once introduced
this for some Xen use case, maybe he can comment on it.


If we added resize, I wouldn't want to scale-to-size.  I find this behavior to
be more trouble than it's worth.  I'd want to render the VGA screen with a black
border only scaling based on the zoom settings.


OK for aspect-ratio-correct scaling of the guest view from my POV. And
if there is a use case for the old behavior, we could still add a switch
to the menu for selecting the scaling mode.


Okay, I'll look into it.


BTW, "VGA" is the wrong term for the graphic console. Maybe there is the
real front-end name available somewhere and could be used instead.


Any suggestions?  Display?  Graphics?

Or were you thinking something like Cirrus VGA?


VGA is (mostly) x86. Also, we may once have multiple guest screens,
maybe even multiple types of them. So picking up some telling front-end
name would likely scale best.


Display 0?

I think Monitor would be more natural but obviously that would conflict with the 
human monitor.


Regards,

Anthony Liguori


Jan

Re: [Qemu-devel] [PATCH 0/8] Add GTK UI to enable basic accessibility (v2)

[Jan Kiszka]

On 2012-02-27 14:50, Anthony Liguori wrote:
> On 02/27/2012 07:42 AM, Jan Kiszka wrote:
>> On 2012-02-27 14:33, Anthony Liguori wrote:
 That all keyboard inputs are grabbed when the mouse is in the window and
 you don't need to press ctrl-alt-g explicitly. And the reverse should
 happen when the mouse reaches the window border. Just like under SDL,
 give it a try.
>>>
>>> Right, this was intentional.  I think it's weird that a window would steal 
>>> input
>>> when the mouse moves over it breaking desktop accelerators like alt tab.  If
>>> you've ever alt-tab cycled through windows when QEMU is running, if you're
>>> unlucky enough to have the mouse where a QEMU window may be, the QEMU 
>>> instance
>>> will steal input and prevent alt-tab from working anymore.
>>
>> This would be a usability regression. It is very unhandy to switch
>> between grabbed and ungrabbed, specifically for alt-tab, when working
>> with guests vs. host windows. Look e.g. at how rdeskop works in this
>> regard. That's why I introduced this feature to QEMU, and I would not
>> want to see it die again with the introduction of GTK.
> 
> I'll add a "Grab on Hover" menu item to enable the behavior.  It's a good 
> excuse 
> to look at gconf to store GUI preferences too.

Perfect.

> 
>> - window not resizable (except in broken full-screen mode)
>
> That's intentional.

 And a mistake IMHO. Definitely for the text consoles, but one can also
 argue about the guest graphic console. I think Stefano once introduced
 this for some Xen use case, maybe he can comment on it.
>>>
>>> If we added resize, I wouldn't want to scale-to-size.  I find this behavior 
>>> to
>>> be more trouble than it's worth.  I'd want to render the VGA screen with a 
>>> black
>>> border only scaling based on the zoom settings.
>>
>> OK for aspect-ratio-correct scaling of the guest view from my POV. And
>> if there is a use case for the old behavior, we could still add a switch
>> to the menu for selecting the scaling mode.
> 
> Okay, I'll look into it.
> 
 BTW, "VGA" is the wrong term for the graphic console. Maybe there is the
 real front-end name available somewhere and could be used instead.
>>>
>>> Any suggestions?  Display?  Graphics?
>>>
>>> Or were you thinking something like Cirrus VGA?
>>
>> VGA is (mostly) x86. Also, we may once have multiple guest screens,
>> maybe even multiple types of them. So picking up some telling front-end
>> name would likely scale best.
> 
> Display 0?
> 
> I think Monitor would be more natural but obviously that would conflict with 
> the 
> human monitor.

It might also be something else than a "monitor" that visualizes guest
graphic-like output.

Another effect I just noticed: The scroll position of the monitor
console is not always properly restored when switching from other
consoles. Looks like it can move down, leaving the visible range after
some switches.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] usb: Resolve warnings about unassigned bus on usb device creation

[Jan Kiszka]

On 2012-02-02 18:59, Jan Kiszka wrote:
> When creating an USB device the old way, there is no way to specify the
> target bus. Thus the warning issued by usb_create makes no sense and
> rather confuses our users.
> 
> Resolve this by passing a bus reference to the usbdevice_init handler
> and letting those handlers forward it to usb_create.
> 
> Signed-off-by: Jan Kiszka 
> ---
>  hw/usb-bt.c |4 ++--
>  hw/usb-bus.c|   18 --
>  hw/usb-msd.c|4 ++--
>  hw/usb-net.c|4 ++--
>  hw/usb-serial.c |8 
>  hw/usb.h|7 ---
>  usb-bsd.c   |4 ++--
>  usb-linux.c |4 ++--
>  vl.c|7 ---
>  9 files changed, 26 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/usb-bt.c b/hw/usb-bt.c
> index bf8c470..291242f 100644
> --- a/hw/usb-bt.c
> +++ b/hw/usb-bt.c
> @@ -498,14 +498,14 @@ static int usb_bt_initfn(USBDevice *dev)
>  return 0;
>  }
>  
> -USBDevice *usb_bt_init(HCIInfo *hci)
> +USBDevice *usb_bt_init(USBBus *bus, HCIInfo *hci)
>  {
>  USBDevice *dev;
>  struct USBBtState *s;
>  
>  if (!hci)
>  return NULL;
> -dev = usb_create_simple(NULL /* FIXME */, "usb-bt-dongle");
> +dev = usb_create_simple(bus, "usb-bt-dongle");
>  if (!dev) {
>  return NULL;
>  }
> diff --git a/hw/usb-bus.c b/hw/usb-bus.c
> index aeef908..aae5b0c 100644
> --- a/hw/usb-bus.c
> +++ b/hw/usb-bus.c
> @@ -203,14 +203,15 @@ typedef struct LegacyUSBFactory
>  {
>  const char *name;
>  const char *usbdevice_name;
> -USBDevice *(*usbdevice_init)(const char *params);
> +USBDevice *(*usbdevice_init)(USBBus *bus, const char *params);
>  } LegacyUSBFactory;
>  
>  static GSList *legacy_usb_factory;
>  
>  void usb_qdev_register(DeviceInfo *info,
> const char *usbdevice_name,
> -   USBDevice *(*usbdevice_init)(const char *params))
> +   USBDevice *(*usbdevice_init)(USBBus *bus,
> +const char *params))
>  {
>  info->bus_info = &usb_bus_info;
>  info->init = usb_qdev_init;
> @@ -231,17 +232,6 @@ USBDevice *usb_create(USBBus *bus, const char *name)
>  {
>  DeviceState *dev;
>  
> -#if 1
> -/* temporary stopgap until all usb is properly qdev-ified */
> -if (!bus) {
> -bus = usb_bus_find(-1);
> -if (!bus)
> -return NULL;
> -error_report("%s: no bus specified, using \"%s\" for \"%s\"",
> -__FUNCTION__, bus->qbus.name, name);
> -}
> -#endif
> -
>  dev = qdev_create(&bus->qbus, name);
>  return USB_DEVICE(dev);
>  }
> @@ -572,7 +562,7 @@ USBDevice *usbdevice_create(const char *cmdline)
>  }
>  return usb_create_simple(bus, f->name);
>  }
> -return f->usbdevice_init(params);
> +return f->usbdevice_init(bus, params);
>  }
>  
>  static TypeInfo usb_device_type_info = {
> diff --git a/hw/usb-msd.c b/hw/usb-msd.c
> index ceb01e0..823f072 100644
> --- a/hw/usb-msd.c
> +++ b/hw/usb-msd.c
> @@ -568,7 +568,7 @@ static int usb_msd_initfn(USBDevice *dev)
>  return 0;
>  }
>  
> -static USBDevice *usb_msd_init(const char *filename)
> +static USBDevice *usb_msd_init(USBBus *bus, const char *filename)
>  {
>  static int nr=0;
>  char id[8];
> @@ -611,7 +611,7 @@ static USBDevice *usb_msd_init(const char *filename)
>  }
>  
>  /* create guest device */
> -dev = usb_create(NULL /* FIXME */, "usb-storage");
> +dev = usb_create(bus, "usb-storage");
>  if (!dev) {
>  return NULL;
>  }
> diff --git a/hw/usb-net.c b/hw/usb-net.c
> index 57b58ac..c9884e1 100644
> --- a/hw/usb-net.c
> +++ b/hw/usb-net.c
> @@ -1353,7 +1353,7 @@ static int usb_net_initfn(USBDevice *dev)
>  return 0;
>  }
>  
> -static USBDevice *usb_net_init(const char *cmdline)
> +static USBDevice *usb_net_init(USBBus *bus, const char *cmdline)
>  {
>  USBDevice *dev;
>  QemuOpts *opts;
> @@ -1371,7 +1371,7 @@ static USBDevice *usb_net_init(const char *cmdline)
>  return NULL;
>  }
>  
> -dev = usb_create(NULL /* FIXME */, "usb-net");
> +dev = usb_create(bus, "usb-net");
>  if (!dev) {
>  return NULL;
>  }
> diff --git a/hw/usb-serial.c b/hw/usb-serial.c
> index de49607..8c7861d 100644
> --- a/hw/usb-serial.c
> +++ b/hw/usb-serial.c
> @@ -492,7 +492,7 @@ static int usb_serial_initfn(USBDevice *dev)
>  return 0;
>  }
>  
> -static USBDevice *usb_serial_init(const char *filename)
> +static USBDevice *usb_serial_init(USBBus *bus, const char *filename)
>  {
>  USBDevice *dev;
>  CharDriverState *cdrv;
> @@ -535,7 +535,7 @@ static USBDevice *usb_serial_init(const char *filename)
>  if (!cdrv)
>  return NULL;
>  
> -dev = usb_create(NULL /* FIXME */, "usb-serial");
> +dev = usb_create(bus, "usb-serial");
>  if (!dev) {
>  return NULL;
>  }
> @@ -549,7 +549,7 @@ static USBDevice *usb_serial_init(co

Re: [Qemu-devel] [PATCH 2/6] slirp: Fix assertion failure on rejected DHCP requests

[Jan Kiszka]

On 2012-02-24 01:23, David Gibson wrote:
> The guest network stack might DHCPREQUEST an address that the slirp built
> in dhcp server can't let it have - for example if the guest has an old
> leases file from another network configuration.  In this case the dhcp
> server should and does reject the request and prepares to send a DHCPNAK
> to the client.
> 
> However, in this case the daddr variable in bootp_reply() is set to
> 0.0.0.0.  Shortly afterwards, it unconditionally attempts to pre-insert the
> new client address into the ARP table.  This causes an assertion failure in
> arp_address_add() because of the 0.0.0.0 address.
> 
> According to RFC2131, DHCPNAK messages for clients on the same subnet
> must be sent to the broadcast address (S3.2, subpoint 2).
> 
> Cc: Jan Kiszka 
> 
> Signed-off-by: David Gibson 

Thanks, applied to the slirp queue.

Jan

> ---
>  slirp/bootp.c |3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/slirp/bootp.c b/slirp/bootp.c
> index efd1fe7..64eac7d 100644
> --- a/slirp/bootp.c
> +++ b/slirp/bootp.c
> @@ -200,7 +200,8 @@ static void bootp_reply(Slirp *slirp, const struct 
> bootp_t *bp)
>  daddr.sin_addr = preq_addr;
>  memcpy(bc->macaddr, client_ethaddr, ETH_ALEN);
>  } else {
> -daddr.sin_addr.s_addr = 0;
> +/* DHCPNAKs should be sent to broadcast */
> +daddr.sin_addr.s_addr = 0x;
>  }
>  } else {
>  bc = find_addr(slirp, &daddr.sin_addr, bp->bp_hwaddr);

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] usb: Resolve warnings about unassigned bus on usb device creation

[Gerd Hoffmann]

On 02/27/12 14:57, Jan Kiszka wrote:
> On 2012-02-02 18:59, Jan Kiszka wrote:
>> When creating an USB device the old way, there is no way to specify the
>> target bus. Thus the warning issued by usb_create makes no sense and
>> rather confuses our users.
>>
>> Resolve this by passing a bus reference to the usbdevice_init handler
>> and letting those handlers forward it to usb_create.
>>
>> Signed-off-by: Jan Kiszka 
> 
> Ping.
> 

Somehow slipped through.  Looks good, but needs a rebase.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 3/6] USB OHCI bug fixes

[Gerd Hoffmann]

On 02/24/12 01:23, David Gibson wrote:
> From: Wei Yang 
> 
> This patch fixes two bugs in the OHCI device where the device writes
> back data to system memory that should be exclusively under the
> control of the guest side driver.

Looks good.  Fails checkpatch though.

What is your merge plan btw?  Wanna pick me this up for the usb queue?
Or do you need just my ack?

cheers,
  Gerd

[Qemu-devel] [PATCH v2] usb: Resolve warnings about unassigned bus on usb device creation

[Jan Kiszka]

When creating an USB device the old way, there is no way to specify the
target bus. Thus the warning issued by usb_create makes no sense and
rather confuses our users.

Resolve this by passing a bus reference to the usbdevice_init handler
and letting those handlers forward it to usb_create.

Signed-off-by: Jan Kiszka 
---

Trivial rebase over master

 hw/usb-bt.c |4 ++--
 hw/usb-bus.c|   18 --
 hw/usb-msd.c|4 ++--
 hw/usb-net.c|4 ++--
 hw/usb-serial.c |8 
 hw/usb.h|7 ---
 usb-bsd.c   |4 ++--
 usb-linux.c |4 ++--
 vl.c|7 ---
 9 files changed, 26 insertions(+), 34 deletions(-)

diff --git a/hw/usb-bt.c b/hw/usb-bt.c
index 649bdcf..23c39ec 100644
--- a/hw/usb-bt.c
+++ b/hw/usb-bt.c
@@ -498,14 +498,14 @@ static int usb_bt_initfn(USBDevice *dev)
 return 0;
 }
 
-USBDevice *usb_bt_init(HCIInfo *hci)
+USBDevice *usb_bt_init(USBBus *bus, HCIInfo *hci)
 {
 USBDevice *dev;
 struct USBBtState *s;
 
 if (!hci)
 return NULL;
-dev = usb_create_simple(NULL /* FIXME */, "usb-bt-dongle");
+dev = usb_create_simple(bus, "usb-bt-dongle");
 if (!dev) {
 return NULL;
 }
diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index ae79a45..70b7ebc 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -203,13 +203,14 @@ typedef struct LegacyUSBFactory
 {
 const char *name;
 const char *usbdevice_name;
-USBDevice *(*usbdevice_init)(const char *params);
+USBDevice *(*usbdevice_init)(USBBus *bus, const char *params);
 } LegacyUSBFactory;
 
 static GSList *legacy_usb_factory;
 
 void usb_legacy_register(const char *typename, const char *usbdevice_name,
- USBDevice *(*usbdevice_init)(const char *params))
+ USBDevice *(*usbdevice_init)(USBBus *bus,
+  const char *params))
 {
 if (usbdevice_name) {
 LegacyUSBFactory *f = g_malloc0(sizeof(*f));
@@ -224,17 +225,6 @@ USBDevice *usb_create(USBBus *bus, const char *name)
 {
 DeviceState *dev;
 
-#if 1
-/* temporary stopgap until all usb is properly qdev-ified */
-if (!bus) {
-bus = usb_bus_find(-1);
-if (!bus)
-return NULL;
-error_report("%s: no bus specified, using \"%s\" for \"%s\"",
-__FUNCTION__, bus->qbus.name, name);
-}
-#endif
-
 dev = qdev_create(&bus->qbus, name);
 return USB_DEVICE(dev);
 }
@@ -565,7 +555,7 @@ USBDevice *usbdevice_create(const char *cmdline)
 }
 return usb_create_simple(bus, f->name);
 }
-return f->usbdevice_init(params);
+return f->usbdevice_init(bus, params);
 }
 
 static void usb_device_class_init(ObjectClass *klass, void *data)
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 5fbd2d0..c6f08a0 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -568,7 +568,7 @@ static int usb_msd_initfn(USBDevice *dev)
 return 0;
 }
 
-static USBDevice *usb_msd_init(const char *filename)
+static USBDevice *usb_msd_init(USBBus *bus, const char *filename)
 {
 static int nr=0;
 char id[8];
@@ -611,7 +611,7 @@ static USBDevice *usb_msd_init(const char *filename)
 }
 
 /* create guest device */
-dev = usb_create(NULL /* FIXME */, "usb-storage");
+dev = usb_create(bus, "usb-storage");
 if (!dev) {
 return NULL;
 }
diff --git a/hw/usb-net.c b/hw/usb-net.c
index 49d5d4d..22b8201 100644
--- a/hw/usb-net.c
+++ b/hw/usb-net.c
@@ -1353,7 +1353,7 @@ static int usb_net_initfn(USBDevice *dev)
 return 0;
 }
 
-static USBDevice *usb_net_init(const char *cmdline)
+static USBDevice *usb_net_init(USBBus *bus, const char *cmdline)
 {
 USBDevice *dev;
 QemuOpts *opts;
@@ -1371,7 +1371,7 @@ static USBDevice *usb_net_init(const char *cmdline)
 return NULL;
 }
 
-dev = usb_create(NULL /* FIXME */, "usb-net");
+dev = usb_create(bus, "usb-net");
 if (!dev) {
 return NULL;
 }
diff --git a/hw/usb-serial.c b/hw/usb-serial.c
index 52676e8..0aae379 100644
--- a/hw/usb-serial.c
+++ b/hw/usb-serial.c
@@ -492,7 +492,7 @@ static int usb_serial_initfn(USBDevice *dev)
 return 0;
 }
 
-static USBDevice *usb_serial_init(const char *filename)
+static USBDevice *usb_serial_init(USBBus *bus, const char *filename)
 {
 USBDevice *dev;
 CharDriverState *cdrv;
@@ -535,7 +535,7 @@ static USBDevice *usb_serial_init(const char *filename)
 if (!cdrv)
 return NULL;
 
-dev = usb_create(NULL /* FIXME */, "usb-serial");
+dev = usb_create(bus, "usb-serial");
 if (!dev) {
 return NULL;
 }
@@ -549,7 +549,7 @@ static USBDevice *usb_serial_init(const char *filename)
 return dev;
 }
 
-static USBDevice *usb_braille_init(const char *unused)
+static USBDevice *usb_braille_init(USBBus *bus, const char *unused)
 {
 USBDevice *dev;
 CharDriverState *cdrv;
@@ -558,7 +558,7 @@ static USBDevice *usb_braille_init(const char *unused)
 if (!

Re: [Qemu-devel] [PATCH] qed: replace vm_clock with rt_clock for qemu-tool compatibility

[Zhi Yong Wu]

On Sun, Feb 26, 2012 at 10:55 PM, Stefan Hajnoczi
 wrote:
> The QED dirty bit timer marks the file clean after allocating writes
> have drained.  This is cheaper than clearing/setting the dirty bit on
> each allocating write because the timer introduces a grace period which
> can be extended if more allocating writes arrive.
>
> The vm_clock was used in an attempt to prevent modifying the image file
Why can vm_clock do this work but other clock can not? If you're
available, can you elaborate what the difference among the three
clocks is? such as vm_clock, rt_clock, and host_clock.

> when live migration has stopped the VM.  Unfortunately vm_clock is
> unavailable in the qemu-tool environment and will abort(3)!
>
> Since QED currently does not support live migration, just replace
> vm_clock with rt_clock and add comments explaining the migration
> blocker.
>
> Signed-off-by: Stefan Hajnoczi 
> ---
> Zhi Yong: This patch is needed in addition to the qemu_init_main_loop() 
> patches
> you sent recently.  Without this patch QED may read the vm_clock, which calls
> abort(3) in qemu-tool.c.  Together, our patches make QED work again in 
> qemu-img
> and qemu-io.
>
>  block/qed.c |   16 +++-
>  1 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/block/qed.c b/block/qed.c
> index a041d31..fdb90e3 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -353,10 +353,7 @@ static void qed_start_need_check_timer(BDRVQEDState *s)
>  {
>     trace_qed_start_need_check_timer(s);
>
> -    /* Use vm_clock so we don't alter the image file while suspended for
> -     * migration.
> -     */
> -    qemu_mod_timer(s->need_check_timer, qemu_get_clock_ns(vm_clock) +
> +    qemu_mod_timer(s->need_check_timer, qemu_get_clock_ns(rt_clock) +
>                    get_ticks_per_sec() * QED_NEED_CHECK_TIMEOUT);
>  }
>
> @@ -494,9 +491,18 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
>         }
>     }
>
> -    s->need_check_timer = qemu_new_timer_ns(vm_clock,
> +    s->need_check_timer = qemu_new_timer_ns(rt_clock,
>                                             qed_need_check_timer_cb, s);
>
> +    /* There are two issues with live migration:
> +     *
> +     * 1. The destination will open the image file and see the dirty bit is
> +     *    set, causing it to "repair" the image while the source still has it
> +     *    open for writing.
> +     *
> +     * 2. The timer used for clearing the dirty bit uses rt_clock and can in
> +     *    theory fire when the VM is not running during migration.
> +     */
>     error_set(&s->migration_blocker,
>               QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
>               "qed", bs->device_name, "live migration");
> --
> 1.7.9
>
>



-- 
Regards,

Zhi Yong Wu

Re: [Qemu-devel] [PATCH 0/8] Add GTK UI to enable basic accessibility (v2)

[Kevin Wolf]

Am 27.02.2012 00:46, schrieb Anthony Liguori:
> I realize UIs are the third rail of QEMU development, but over the years I've
> gotten a lot of feedback from users about our UI.  I think everyone struggles
> with the SDL interface and its lack of discoverability but it's worse than I
> think most people realize for users that rely on accessibility tools.
> 
> The two pieces of feedback I've gotten the most re: accessibility are the lack
> of QEMU's enablement for screen readers and the lack of configurable
> accelerators.
> 
> Since we render our own terminal using a fixed sized font, we don't respect
> system font settings which means we ignore if the user has configured large
> print.
> 
> We also don't integrate at all with screen readers which means that for blind
> users, the virtual consoles may as well not even exist.
> 
> We also don't allow any type of configuration of accelerators.  For users with
> limited dexterity (this is actually more common than you would think), they 
> may
> use an input device that only inputs one key at a time.  Holding down two keys
> at once is not possible for these users.
> 
> These are solved problems though and while we could reinvent all of this
> ourselves with SDL, we would be crazy if we did.  Modern toolkits, like GTK,
> solve these problems.
> 
> By using GTK, we can leverage VteTerminal for screen reader integration and 
> font
> configuration.  We can also use GTK's accelerator support to make accelerators
> configurable (Gnome provides a global accelerator configuration interface).
> 
> I'm not attempting to make a pretty desktop virtualization UI.  Maybe we'll go
> there eventually but that's not what this series is about.
> 
> This is just attempting to use a richer toolkit such that we can enable basic
> accessibility support.  As a consequence, the UI is much more usable even for 
> a
> user without accessibility requirements so it's a win-win.
> 
> Also available at:
> 
> https://github.com/aliguori/qemu/tree/gtk.2
> 
> ---
> v1 -> v2
>  - Add internationalization support.  I don't actually speak any other 
> languages
>so I added a placeholder for a German translation.  This can be tested with
>LANGUAGE=de_DE.UTF-8 qemu-system-x86_64

Looks like I need to 'make install' before I can use the translations? I
don't have any qemu version installed into the system, but always run it
from to build directory (I would only confuse the different trees if
something was installed). Shouldn't it be possible that qemu finds the
right files just like it does with the BIOS etc.?

>  - Fixed the terminal size for VteTerminal widgets.  I think the behavior 
> makes
>sense now.
>  - Fixed lots of issues raised in review comments (see individual patches)
> 
> Known Issues:
>  - I saw the X crash once.  I think it has to do with widget sizes.  I need to
>work harder to reproduce.
>  - I've not recreated the reported memory leak yet.
>  - I haven't added backwards compatibility code for older VteTerminal widgets
>yet.

- F10 still activates the menu (same for Alt-F/V)

Kevin

Re: [Qemu-devel] [PATCH v2 1/2] qapi: Introduce blockdev-group-snapshot-sync command

[Jeff Cody]

On 02/27/2012 06:13 AM, Stefan Hajnoczi wrote:
> On Sun, Feb 26, 2012 at 7:31 PM, Jeff Cody  wrote:
> 
> Do you have automated tests for this feature?
> 

No, not yet.  The testing has been manual.

>> +/*
>> + * Add new bs contents at the top of an image chain while the chain is live,
>> + * while keeping required fields on the top layer.
> 
> Please also document the swap behavior.  It's pretty important for the
> caller to realize that once this function returns, their
> BlockDriverState arguments with have swapped.

Good point.  How about this:

/*
 * Add new bs contents at the top of an image chain while the chain is
 * live, while keeping required fields on the top layer.
 *
 * This will modify the BlockDriverState fields, and swap contents
 * between bs_new and bs_top.  Both bs_new and bs_top are modified.
 *
 * A new image file is not created.  It is assumed that bs_new already
 * points to an existing image, with the correct backing filename of
 * bs_top->backing_file.
 */


> 
>> + * It is assumed that bs_new already points to an existing image,
>> + * with the correct backing filename of top->backing_file
> 
> Not sure what this means.  Isn't bs_new going to use bs_top as its
> backing file?  Why "top->backing_file"?

Sorry, that should have been 'bs_top->backing_file'. The image file is
not created by this function.  I added some more explanation, and
corrected that typo, in the above comment block.  Let me know if you
think it still needs more clarification.


> 
>> +void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
>> +{
>> +BlockDriverState tmp;
>> +
>> +/* the new bs must not be in bdrv_states */
>> +bdrv_make_anon(bs_new);
>> +
>> +tmp = *bs_new;
>> +tmp.backing_hd = bs_new;
> 
> This is tricky, would be nice to comment that we will swap bs_new and
> bs_top later, therefore we need a pointer to bs_new here even though
> it doesn't make sense yet.

OK, thanks. I will add some clarifying comments - you are right, it is a
bit tricky and counter-intuitive.  It may also be clearer if this is
done closer to the actual swap ('*bs_top = tmp').  I will move it down
to above the swap.

> 
>> +
>> +/* there are some fields that need to stay on the top layer: */
>> +
>> +/* dev info */
>> +tmp.dev_ops  = bs_top->dev_ops;
>> +tmp.dev_opaque   = bs_top->dev_opaque;
>> +tmp.dev  = bs_top->dev;
>> +tmp.buffer_alignment = bs_top->buffer_alignment;
>> +tmp.copy_on_read = bs_top->copy_on_read;
>> +
>> +/* i/o timing parameters */
>> +tmp.slice_time= bs_top->slice_time;
>> +tmp.slice_start   = bs_top->slice_start;
>> +tmp.slice_end = bs_top->slice_end;
>> +tmp.io_limits = bs_top->io_limits;
>> +tmp.io_base   = bs_top->io_base;
>> +tmp.throttled_reqs= bs_top->throttled_reqs;
>> +tmp.block_timer   = bs_top->block_timer;
>> +tmp.io_limits_enabled = bs_top->io_limits_enabled;
> 
> Please add a comment into BlockDriverState to warn that changes to
> fields may require updates to this function too!

OK; I will add a blanket warning at the top to inspect bdrv_append()
when adding new fields, to see if they need updated in bdrv_append().

> 
>> +/* We will manually add the backing_hd field to the bs later */
>> +states->new_bs = bdrv_new("");
>> +ret = bdrv_open(states->new_bs, snapshot_file,
>> +flags | BDRV_O_NO_BACKING, drv);
>> +states->is_open = true;
> 
> What is the point of is_open?  If we failed then new_bs is actually
> not open here.

Previous entries may have been opened. You are right, however, in that
it is not needed at all. We can rely on new_bs being non-NULL.  I will
remove the 'is_open' references.

> 
> I think you can remove this field and just do the following in close_and_fail:
> 
> if (states->new_bs) {
> bdrv_delete(states->new_bs);
> }
> 
> (BTW I think close_and_fail is currently leaking new_bs because it
> only closes it and does not delete it!)
> 

You are right. That should be bdrv_delete(), and not bdrv_close().  I
will fix that, and also rename close_and_fail to delete_and_fail for
good measure.

Thanks,
Jeff

Re: [Qemu-devel] [PATCHv3 0/4] standard pci bridge device

[Gerd Hoffmann]

On 02/20/12 23:52, Michael S. Tsirkin wrote:
> Here's a new version of the patch.
> 
> TODOs:
> - windows guest testing
> 
> Changes from v2:
> - added slot id capability
> - migration support
> - misc fixes
> - fix checkpatch errors

64bit prefetch memory window works now:

00:10.0 PCI bridge: Red Hat, Inc. Device 0001 (prog-if 00 [Normal decode])
Physical Slot: 16
Flags: bus master, 66MHz, fast devsel, latency 0
Memory at f5126000 (32-bit, non-prefetchable) [size=256]
Bus: primary=00, secondary=01, subordinate=01, sec-latency=0
Memory behind bridge: f500-f50f
Prefetchable memory behind bridge: f800-fbff
Capabilities: [48] Slot ID: 0 slots, First+, chassis 01
Capabilities: [40] Hot-plug capable
Kernel modules: shpchp

Looks good to me, did only light testing though.

cheers,
  Gerd

[Qemu-devel] drive transactions (was Re: [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate commands)

[Paolo Bonzini]

On 02/27/2012 02:06 PM, Luiz Capitulino wrote:
> IMHO, this is asking for two commands, where cases 1 & 2 is one of them
> and cases 3 & 4 is the other one. Note how 'incremental' goes away and
> 'new_image_file' really becomes an optional.

I really would have no idea on naming except perhaps "drive_migrate" and
"funny_drive_migrate_for_ovirt".  But actually I must admit that it's a
rat's nest.

First, there's no reason why live-snapshotting to new_image_file
shouldn't be handled within QEMU.  That would make the above table much
more orthogonal.  However, oVirt likes to create its own snapshots.

Perhaps we do need to rethink this together with group snapshots.

There are four kinds of operations that we do on block devices:
(a) create an image.  This is part of what blockdev-snapshot does.
(b) switch a block device to a new image.  drive-reopen does this.
(c) add mirroring to a new destination.
(d) activate streaming from a base image

drive-migrate does (b) and (c), will do (a) and (d) when we add
pre-copy, and should do (a) right now if Federico wasn't an oVirt
developer. :)

Thinking more about it, the commands we _do_ need are:
- start a transaction
- switch to a new image
- add mirroring to a new destination
- commit a transaction
- rollback a transaction
- query transaction errors

Creating an image can be done outside a transaction for now because we
only support live external snapshots.  Streaming can also be started
outside a transaction, because it doesn't need to be started atomically.

If we have the above elementary commands, blockdev-snapshot-sync becomes
sugar for this:

(create image)
blockdev-start-transaction if no active transaction
drive-reopen
blockdev-commit-transaction if no active transaction

Jeff's group snapshot can be realized as this:

blockdev-begin-transaction
blockdev-snapshot-sync
...
blockdev-snapshot-sync
blockdev-commit-transaction

And for drive-migrate, let's look at the above 3 cases:

> > 1) incremental=false, new_image_file not passed:
> >right now fail; in the future:
> >- create an image on dest with no backing file;
> >- writes will be mirrored to current_image_file and dest
> >- start streaming from current_image_file to dest

This is a new command "drive-mirror device dest", which does:

(create image for dest)
blockdev-begin-transaction if no active transaction
drive-mirror device dest
blockdev-commit-transaction if no active transaction

The command does this:

- mirror writes to current_image_file and dest
- start streaming from current_image_file to dest

The second part can be suppressed with a boolean argument.

> > 2) incremental=false, new_image_file passed:
> >right now fail; in the future:
> >- create an image on dest with no backing file;
> >- live-snapshot based on current_image_file to new_image_file;
> >- writes will be mirrored to new_image_file and dest
> >- start streaming from current_image_file to dest

Atomicity is not needed here, so the user can simply issue:

blockdev-snapshot-sync device new-image-file
drive-mirror device dest

> > 4) incremental=true, new_image_file passed:
> >- no images will be created
> >- writes will be mirrored to new_image_file and dest

No need to provide this from within QEMU, because libvirt/oVirt can do
the dance using elementary operations:

blockdev-begin-transaction
drive-reopen device new-image-file
drive-mirror streaming=false device dest
blockdev-commit-transaction

No strange optional arguments, no proliferation of commands, etc.  The
only downside is that if someone tries to do (4) without transactions
(or without stopping the VM) they'll get corruption because atomicity is
required.

Paolo

Re: [Qemu-devel] [PATCH 0/8] Add GTK UI to enable basic accessibility (v2)

[Anthony Liguori]

On 02/27/2012 08:24 AM, Kevin Wolf wrote:

Am 27.02.2012 00:46, schrieb Anthony Liguori:

I realize UIs are the third rail of QEMU development, but over the years I've
gotten a lot of feedback from users about our UI.  I think everyone struggles
with the SDL interface and its lack of discoverability but it's worse than I
think most people realize for users that rely on accessibility tools.

The two pieces of feedback I've gotten the most re: accessibility are the lack
of QEMU's enablement for screen readers and the lack of configurable
accelerators.

Since we render our own terminal using a fixed sized font, we don't respect
system font settings which means we ignore if the user has configured large
print.

We also don't integrate at all with screen readers which means that for blind
users, the virtual consoles may as well not even exist.

We also don't allow any type of configuration of accelerators.  For users with
limited dexterity (this is actually more common than you would think), they may
use an input device that only inputs one key at a time.  Holding down two keys
at once is not possible for these users.

These are solved problems though and while we could reinvent all of this
ourselves with SDL, we would be crazy if we did.  Modern toolkits, like GTK,
solve these problems.

By using GTK, we can leverage VteTerminal for screen reader integration and font
configuration.  We can also use GTK's accelerator support to make accelerators
configurable (Gnome provides a global accelerator configuration interface).

I'm not attempting to make a pretty desktop virtualization UI.  Maybe we'll go
there eventually but that's not what this series is about.

This is just attempting to use a richer toolkit such that we can enable basic
accessibility support.  As a consequence, the UI is much more usable even for a
user without accessibility requirements so it's a win-win.

Also available at:

https://github.com/aliguori/qemu/tree/gtk.2

---
v1 ->  v2
  - Add internationalization support.  I don't actually speak any other 
languages
so I added a placeholder for a German translation.  This can be tested with
LANGUAGE=de_DE.UTF-8 qemu-system-x86_64


Looks like I need to 'make install' before I can use the translations? I
don't have any qemu version installed into the system, but always run it
from to build directory (I would only confuse the different trees if
something was installed). Shouldn't it be possible that qemu finds the
right files just like it does with the BIOS etc.?


It's a little harder because of the way gettext works.  You can only have a 
single search path AFAICT.


We could add a --with-localedir= to configure and then rearrange the po/ 
structure.  Then you would do something like:


./configure --with-localedir=$(pwd)/po

And it would use translations from your build directory.


  - Fixed the terminal size for VteTerminal widgets.  I think the behavior makes
sense now.
  - Fixed lots of issues raised in review comments (see individual patches)

Known Issues:
  - I saw the X crash once.  I think it has to do with widget sizes.  I need to
work harder to reproduce.
  - I've not recreated the reported memory leak yet.
  - I haven't added backwards compatibility code for older VteTerminal widgets
yet.


- F10 still activates the menu (same for Alt-F/V)


This is expected behavior although Grab on Hover will cause it to behave like 
SDL does.


F10/Alt-F/V are menu accelerators and we need to allow menu accelerators for 
things like Ctrl-Alt-2 to work.


More importantly, it should be possible to navigate the GUI without a mouse 
(this is an accessibility requirement).  Without supporting Alt-F, there's no 
way to navigate the menu from the keyboard.


That said, we probably do want to add a Send Key menu for sending key sequences 
that are used as accelerators.


Regards,

Anthony Liguo



Kevin

Re: [Qemu-devel] [PATCH v2 1/2] qapi: Introduce blockdev-group-snapshot-sync command

[Stefan Hajnoczi]

On Mon, Feb 27, 2012 at 2:26 PM, Jeff Cody  wrote:
> On 02/27/2012 06:13 AM, Stefan Hajnoczi wrote:
>> On Sun, Feb 26, 2012 at 7:31 PM, Jeff Cody  wrote:
>>
>> Do you have automated tests for this feature?
>>
>
> No, not yet.  The testing has been manual.

For image streaming I used the Python unittest framework along with
QMP/qmp.py to create tests.  I am going to submit it as a qemu-iotest.
 We really need something along the lines of a harness with QMP
support so that these block layer features can be tested.  I will CC
you on the email.

>
>>> +/*
>>> + * Add new bs contents at the top of an image chain while the chain is 
>>> live,
>>> + * while keeping required fields on the top layer.
>>
>> Please also document the swap behavior.  It's pretty important for the
>> caller to realize that once this function returns, their
>> BlockDriverState arguments with have swapped.
>
> Good point.  How about this:
>
> /*
>  * Add new bs contents at the top of an image chain while the chain is
>  * live, while keeping required fields on the top layer.
>  *
>  * This will modify the BlockDriverState fields, and swap contents
>  * between bs_new and bs_top.  Both bs_new and bs_top are modified.

Looks good.

>>> + * It is assumed that bs_new already points to an existing image,
>>> + * with the correct backing filename of top->backing_file
>>
>> Not sure what this means.  Isn't bs_new going to use bs_top as its
>> backing file?  Why "top->backing_file"?
>
> Sorry, that should have been 'bs_top->backing_file'. The image file is
> not created by this function.  I added some more explanation, and
> corrected that typo, in the above comment block.  Let me know if you
> think it still needs more clarification.

I still don't follow.  Old bs_top's image file itself becomes the
backing file, not bs_top->backing_file.  Perhaps I'm misinterpreting
because of how swap changes bs_top and bs_new, but I'm reading it from
the perspective of the caller when they pass in bs_top.

The rest looks good.

Stefan

Re: [Qemu-devel] [PATCH v2] usb: Resolve warnings about unassigned bus on usb device creation

[Gerd Hoffmann]

On 02/27/12 15:18, Jan Kiszka wrote:
> When creating an USB device the old way, there is no way to specify the
> target bus. Thus the warning issued by usb_create makes no sense and
> rather confuses our users.
> 
> Resolve this by passing a bus reference to the usbdevice_init handler
> and letting those handlers forward it to usb_create.

Patch added to usb patch queue.

thanks,
  Gerd

Re: [Qemu-devel] [PATCH] qed: replace vm_clock with rt_clock for qemu-tool compatibility

[Stefan Hajnoczi]

On Mon, Feb 27, 2012 at 2:20 PM, Zhi Yong Wu  wrote:
> On Sun, Feb 26, 2012 at 10:55 PM, Stefan Hajnoczi
>  wrote:
>> The QED dirty bit timer marks the file clean after allocating writes
>> have drained.  This is cheaper than clearing/setting the dirty bit on
>> each allocating write because the timer introduces a grace period which
>> can be extended if more allocating writes arrive.
>>
>> The vm_clock was used in an attempt to prevent modifying the image file
> Why can vm_clock do this work but other clock can not? If you're
> available, can you elaborate what the difference among the three
> clocks is? such as vm_clock, rt_clock, and host_clock.

See qemu-timer.h.

Stefan

Re: [Qemu-devel] drive transactions (was Re: [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate commands)

[Anthony Liguori]

On 02/27/2012 08:39 AM, Paolo Bonzini wrote:

On 02/27/2012 02:06 PM, Luiz Capitulino wrote:

IMHO, this is asking for two commands, where cases 1&  2 is one of them
and cases 3&  4 is the other one. Note how 'incremental' goes away and
'new_image_file' really becomes an optional.


I really would have no idea on naming except perhaps "drive_migrate" and
"funny_drive_migrate_for_ovirt".  But actually I must admit that it's a
rat's nest.

First, there's no reason why live-snapshotting to new_image_file
shouldn't be handled within QEMU.  That would make the above table much
more orthogonal.  However, oVirt likes to create its own snapshots.

Perhaps we do need to rethink this together with group snapshots.

There are four kinds of operations that we do on block devices:
(a) create an image.  This is part of what blockdev-snapshot does.
(b) switch a block device to a new image.  drive-reopen does this.
(c) add mirroring to a new destination.
(d) activate streaming from a base image

drive-migrate does (b) and (c), will do (a) and (d) when we add
pre-copy, and should do (a) right now if Federico wasn't an oVirt
developer. :)

Thinking more about it, the commands we _do_ need are:
- start a transaction
- switch to a new image
- add mirroring to a new destination
- commit a transaction
- rollback a transaction
- query transaction errors


I think a better way to think of this is as a batch submission.  It would be 
relatively easy to model in QMP too (just have a batch-command that has a list 
of commands as it's argument).


The difference between batch submission and a transaction is atomic rollback. 
But I don't think atomic rollback is really needed here.


Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver

[Stefan Hajnoczi]

On Mon, Feb 27, 2012 at 1:47 PM, Paolo Bonzini  wrote:
> On 02/27/2012 02:09 PM, Stefan Hajnoczi wrote:
>> > Non-incremental mode is "pre-copy" migration.  It would stream in the
>> > background from the source to the destination.  In this case:
>> >
>> > - you need to differentiate streaming writes from other writes.  When
>> > streaming, you do not want to issue spurious writes to the source;
>> >
>> > - you can skip streaming anything that has been written to the
>> > destination already.  This means that you need: 1) a bitmap similar to
>> > is_allocated; 2) to widen writes to a cluster; 3) serialization similar
>> > to copy-on-read.
>> >
>> > I'm not yet sure how much of this will be generalized in the block layer
>> > and block/stream.c, and how much will be specific to blkmirror, but in
>> > general none of this applies to blkverify.
>>
>> Agreed but I'm not sure it has to do with blkmirror either.  blkmirror
>> and blkverify perform the same function except that blkverify mirrors
>> reads too and checks that they match.
>>
>> Who knows when and if pre-copy will be (re)implemented, it's not a
>> good argument to say let's duplicate block mirroring because we're not
>> sure about the design of a feature feature yet which might want to
>> hook in here.
>
> It's not a duplicate, it just happens that two very simple drivers share
> 1 operation out of 4 (open, read, write, flush).  There are other
> differences, for example:
>
> 1) blkverify hardcodes raw for one format and auto-detects the other.
> blkmirror needs to have a specified format for both, and I don't think
> starting another bikeshedding discussion on blkverify backwards
> compatibility is a good idea;

Backwards compatibility isn't needed here, it's a debugging tool and
some distros even disable it.

Allowing another format for the reference image is a feature that
would be nice to have.  It allows direct qcow2 to vmdk comparison, for
example, if we ever hit a bug that benefits from this type of
comparison.

> 2) blkverify doesn't flush the raw file;

It's fine to flush the reference file.  This is an accidental difference :).

> 3) blkverify in the future could add callbacks to create snapshots or
> load/save imgstate, and forward them to the test file; this doesn't make
> sense for blkmirror.

I guess what I'm saying is, if we need to copy-paste in order to fork
them in the future that's fine, but why maintain duplicates in the
mean-time?  Please make the codebase nice today.  We can always extend
it in the future, we're not forced to keep them unified forever if it
turns out we want to split them.  But as it stands they are
essentially the same thing.

Stefan

Re: [Qemu-devel] qemu assertion failed with usb on current git master!

[Erik Rull]

On February 27, 2012 at 1:48 PM Gerd Hoffmann  wrote:

> >>> qemu-system-x86_64: /home/erik/qemu/hw/usb.c:358 usb_packet_complete:
> >>> Assertion 'p->state == USB_PACKET_QUEUED' failed.
> >>
> >> Stacktrace?
> >> What kind of device?
> >>
>
> > Hi Gerd,
> >
> > attached the usb logger dump as requested.
> > Stacktrace - I tried that without real success using the gdbserver - I
had
> > issues with the symbol resolving, no idea what went wrong...
>
> You don't need gdbserver, that one is for debugging the guest, not for
> debugging qemu.
>
> Just use gdb on the qemu core dump.  And please also print p (packet
> pointer) which fails the assertion, so we can match it with the packet
> pointers printed in the log.
>
> thanks,
>   Gerd
>


I'm really sorry, but I don't understand what's happening - I copied the
qemu executable on my target system before executing it, but gdb complains
that the core file does not match the executable! But except the file paths
they are identical.

Here the gdb output:

warning: core file may not match specified executable file.
Core was generated by `/disc/qemu-system-x86_64 -machine kernel_irqchip=on
-serial /dev/ttyS2 -usb -de'.
Program terminated with signal 6, Aborted.
#0  0xe424 in __kernel_vsyscall ()
(gdb) bt
#0  0xe424 in __kernel_vsyscall ()
#1  0xb705f671 in ?? ()
#2  0xb714bff4 in ?? ()
#3  0xb6faf6b0 in ?? ()
#4  0xbfe8b514 in ?? ()
#5  0xb7060cf9 in ?? ()
#6  0x0006 in ?? ()
#7  0xbfe8b488 in ?? ()
#8  0x in ?? ()
(gdb) p
The history is empty.
(gdb)

I don't know how to proceed here.
Don't you run into this problem (crash on USB plug in) on your system?
I tested it with a Linux guest, there it does not crash! Only with a
Windows XP guest!

Best regards,

Erik

Re: [Qemu-devel] drive transactions (was Re: [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate commands)

[Paolo Bonzini]

On 02/27/2012 03:46 PM, Anthony Liguori wrote:
> I think a better way to think of this is as a batch submission.  It
> would be relatively easy to model in QMP too (just have a batch-command
> that has a list of commands as it's argument).
> 
> The difference between batch submission and a transaction is atomic
> rollback. But I don't think atomic rollback is really needed here.

A transaction enforces atomicity at the block layer level.  It's
different from batch commands in two ways:

* bdrv_drain_all/bdrv_flush needs to be called at the beginning of the
commit.  This may not be the case with batch commands.

* with batch commands, atomicity happens by chance because VCPUs cannot
send I/O while the monitor is holding the global mutex.

Paolo

Re: [Qemu-devel] [PATCH 2/2] Add the blockdev-reopen and blockdev-migrate commands

[Markus Armbruster]

Kevin Wolf  writes:

> Am 24.02.2012 12:37, schrieb Federico Simoncelli:
[...]
>> diff --git a/blockdev.c b/blockdev.c
>> index 2c132a3..1df2542 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
[...]
>> +void qmp_blockdev_migrate(const char *device, BlockMigrateOp mode,
>> +  const char *destination, bool has_new_image_file,
>> +  const char *new_image_file, Error **errp)
>> +{
>> +BlockDriverState *bs;
>> +BlockDriver *drv;
>> +int i, j, escape;
>> +char filename[2048];
>> +
>> +bs = bdrv_find(device);
>> +if (!bs) {
>> +error_set(errp, QERR_DEVICE_NOT_FOUND, device);
>> +return;
>> +}
>> +if (bdrv_in_use(bs)) {
>> +error_set(errp, QERR_DEVICE_IN_USE, device);
>> +return;
>> +}
>> +
>> +if (mode == BLOCK_MIGRATE_OP_MIRROR) {
>
> Move into a separate function?
>
>> +drv = bdrv_find_format("blkmirror");
>> +if (!drv) {
>> +error_set(errp, QERR_INVALID_BLOCK_FORMAT, "blkmirror");
>> +return;
>> +}
>> +
>> +if (!has_new_image_file) {
>> +new_image_file = bs->filename;
>> +}
>> +
>> +pstrcpy(filename, sizeof(filename), "blkmirror:");
>> +i = strlen("blkmirror:");
>> +
>> +escape = 0;
>> +for (j = 0; j < strlen(new_image_file); j++) {
>> + loop:
>> +if (!(i < sizeof(filename) - 2)) {
>> +error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> +  "new-image-file", "shorter filename");
>> +return;
>> +}
>> +
>> +if (new_image_file[j] == ':' || new_image_file[j] == '\\') {
>
> Markus suggested that using comma for the separator is better even
> though it requires escaping. It would allow to parse the option string
> with QemuOpts.

Yes, I did.  While I'm no particular friend of QemuOpts, inventing more
ad hoc syntaxes is even worse.

On the other hand, many block drivers already do, often a colon syntax
similar to yours.  But not all.  I guess consistency is a lost cause
there, as is generality (support for arbitrary filenames).  Maybe it's
best to concede defeat, do another ad hoc syntax now, and hope we can
replace the whole -drive mess by something saner eventually.

>> +if (!escape) {
>> +filename[i++] = '\\', escape = 1;
>> +goto loop;
>> +} else {
>> +escape = 0;
>> +}
>> +}
>> +
>> +filename[i++] = new_image_file[j];
>> +}
>
> Looks like a string helper for qemu-option.c (it contains the parser, so
> keeping the escaping nearby would make sense).
>
>> +
>> +if (i + strlen(destination) + 2 > sizeof(filename)) {
>> +error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> +  "destination", "shorter filename");
>> +return;
>> +}
>> +
>> +filename[i++] = ':';
>> +pstrcpy(filename + i, sizeof(filename) - i - 2, destination);
>> +
>> +change_blockdev_image(device, filename, "blkmirror", false, errp);
>> +} else if (mode == BLOCK_MIGRATE_OP_STREAM) {
>> +error_set(errp, QERR_NOT_SUPPORTED);
>
> Why even define it then?
>
>> +}
>> +}
>> +
>>  static void eject_device(BlockDriverState *bs, int force, Error **errp)
>>  {
>>  if (bdrv_in_use(bs)) {
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 573b823..ccb1f62 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -886,6 +886,42 @@ Snapshot device, using snapshot file as target if 
>> provided
>>  ETEXI
>>  
>>  {
>> +.name   = "blkdev_reopen",
>
> NACK on the name. Let's reserve blkdev_*/blockdev_* for the proper API

Yes, please.

> (that we'll release with the qemu version that comes with Hurd 1.0).

/me hides.

> drive_* would align well with the existing drive_add/del commands.

Agree.

[...]
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index d02ee86..c86796a 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1136,6 +1136,69 @@
>>'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
>>  
>>  ##
>> +# @blockdev-reopen
>> +#
>> +# Assigns a new image file to a device.
>> +#
>> +# @device: the name of the device for which we are chainging the image file.
>> +#
>> +# @new-image-file: the target of the new image. If the file doesn't exists 
>> the
>> +#  command will fail.
>> +#
>> +# @format: #optional the format of the new image, default is 'qcow2'.
>> +#
>> +# Returns: nothing on success
>> +#  If @device is not a valid block device, DeviceNotFound
>> +#  If @new-image-file can't be opened, OpenFileFailed
>> +#  If @format is invalid, InvalidBlockFormat
>> +#
>> +# Since 1.1
>> +##
>> +
>> +{ 'command': 'blockdev-reopen',
>> +  'data': { 'device': 'str', 'new-image-file': 'str', '*format': 'str' } }
>
> Sam

Re: [Qemu-devel] drive transactions (was Re: [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate commands)

[Anthony Liguori]

On 02/27/2012 08:54 AM, Paolo Bonzini wrote:

On 02/27/2012 03:46 PM, Anthony Liguori wrote:

I think a better way to think of this is as a batch submission.  It
would be relatively easy to model in QMP too (just have a batch-command
that has a list of commands as it's argument).

The difference between batch submission and a transaction is atomic
rollback. But I don't think atomic rollback is really needed here.


A transaction enforces atomicity at the block layer level.  It's
different from batch commands in two ways:

* bdrv_drain_all/bdrv_flush needs to be called at the beginning of the
commit.  This may not be the case with batch commands.

* with batch commands, atomicity happens by chance because VCPUs cannot
send I/O while the monitor is holding the global mutex.


If the commands are designed correctly, then this works well.  The problem is 
that the current commands are not designed well.  For instance, multi-snapshot 
could look like:


block-freeze ide0-hd0
block-freeze ide1-hd1
block-reopen ide0-hd0 my-new-file0.qcow2
block-reopen ide1-hd1 my-new-file1.qcow2
block-unfreeze ide1-hd1
block-unfreeze ide1-hd0

This would work regardless of whether the commands were implemented 
asynchronously within QEMU too.


Regards,

Anthony Liguori



Paolo

Re: [Qemu-devel] [PATCH 0/8] Add GTK UI to enable basic accessibility (v2)

[Kevin Wolf]

Am 27.02.2012 15:39, schrieb Anthony Liguori:
> On 02/27/2012 08:24 AM, Kevin Wolf wrote:
>> Am 27.02.2012 00:46, schrieb Anthony Liguori:
>>> I realize UIs are the third rail of QEMU development, but over the years 
>>> I've
>>> gotten a lot of feedback from users about our UI.  I think everyone 
>>> struggles
>>> with the SDL interface and its lack of discoverability but it's worse than I
>>> think most people realize for users that rely on accessibility tools.
>>>
>>> The two pieces of feedback I've gotten the most re: accessibility are the 
>>> lack
>>> of QEMU's enablement for screen readers and the lack of configurable
>>> accelerators.
>>>
>>> Since we render our own terminal using a fixed sized font, we don't respect
>>> system font settings which means we ignore if the user has configured large
>>> print.
>>>
>>> We also don't integrate at all with screen readers which means that for 
>>> blind
>>> users, the virtual consoles may as well not even exist.
>>>
>>> We also don't allow any type of configuration of accelerators.  For users 
>>> with
>>> limited dexterity (this is actually more common than you would think), they 
>>> may
>>> use an input device that only inputs one key at a time.  Holding down two 
>>> keys
>>> at once is not possible for these users.
>>>
>>> These are solved problems though and while we could reinvent all of this
>>> ourselves with SDL, we would be crazy if we did.  Modern toolkits, like GTK,
>>> solve these problems.
>>>
>>> By using GTK, we can leverage VteTerminal for screen reader integration and 
>>> font
>>> configuration.  We can also use GTK's accelerator support to make 
>>> accelerators
>>> configurable (Gnome provides a global accelerator configuration interface).
>>>
>>> I'm not attempting to make a pretty desktop virtualization UI.  Maybe we'll 
>>> go
>>> there eventually but that's not what this series is about.
>>>
>>> This is just attempting to use a richer toolkit such that we can enable 
>>> basic
>>> accessibility support.  As a consequence, the UI is much more usable even 
>>> for a
>>> user without accessibility requirements so it's a win-win.
>>>
>>> Also available at:
>>>
>>> https://github.com/aliguori/qemu/tree/gtk.2
>>>
>>> ---
>>> v1 ->  v2
>>>   - Add internationalization support.  I don't actually speak any other 
>>> languages
>>> so I added a placeholder for a German translation.  This can be tested 
>>> with
>>> LANGUAGE=de_DE.UTF-8 qemu-system-x86_64
>>
>> Looks like I need to 'make install' before I can use the translations? I
>> don't have any qemu version installed into the system, but always run it
>> from to build directory (I would only confuse the different trees if
>> something was installed). Shouldn't it be possible that qemu finds the
>> right files just like it does with the BIOS etc.?
> 
> It's a little harder because of the way gettext works.  You can only have a 
> single search path AFAICT.
> 
> We could add a --with-localedir= to configure and then rearrange the po/ 
> structure.  Then you would do something like:
> 
> ./configure --with-localedir=$(pwd)/po
> 
> And it would use translations from your build directory.

Yeah, would be better than nothing.

>>>   - Fixed the terminal size for VteTerminal widgets.  I think the behavior 
>>> makes
>>> sense now.
>>>   - Fixed lots of issues raised in review comments (see individual patches)
>>>
>>> Known Issues:
>>>   - I saw the X crash once.  I think it has to do with widget sizes.  I 
>>> need to
>>> work harder to reproduce.
>>>   - I've not recreated the reported memory leak yet.
>>>   - I haven't added backwards compatibility code for older VteTerminal 
>>> widgets
>>> yet.
>>
>> - F10 still activates the menu (same for Alt-F/V)
> 
> This is expected behavior although Grab on Hover will cause it to behave like 
> SDL does.

At least grabbing the keyboard manually with Ctrl-Alt-G doesn't disable
the accelerators, so even then you can't use these keys inside the VM.

> F10/Alt-F/V are menu accelerators and we need to allow menu accelerators for 
> things like Ctrl-Alt-2 to work.
> 
> More importantly, it should be possible to navigate the GUI without a mouse 
> (this is an accessibility requirement).  Without supporting Alt-F, there's no 
> way to navigate the menu from the keyboard.
> 
> That said, we probably do want to add a Send Key menu for sending key 
> sequences 
> that are used as accelerators.

Probably. But not for things as common as Alt-F, Alt-V or F10. Having to
use sendkey or the menus for these would be ridiculous and make SDL the
friendlier user interface again.

Can accelerators only be enabled or disabled all at once? Or could we
say that we disable everything that doesn't include Ctrl-Alt and provide
an alternative accelerator for activating the menu, like Ctrl_Alt-M?

Kevin

Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver

[Stefan Hajnoczi]

On Mon, Feb 27, 2012 at 2:49 PM, Stefan Hajnoczi  wrote:
> On Mon, Feb 27, 2012 at 1:47 PM, Paolo Bonzini  wrote:
>> On 02/27/2012 02:09 PM, Stefan Hajnoczi wrote:
>> 3) blkverify in the future could add callbacks to create snapshots or
>> load/save imgstate, and forward them to the test file; this doesn't make
>> sense for blkmirror.
>
> I guess what I'm saying is, if we need to copy-paste in order to fork
> them in the future that's fine, but why maintain duplicates in the
> mean-time?  Please make the codebase nice today.  We can always extend
> it in the future, we're not forced to keep them unified forever if it
> turns out we want to split them.  But as it stands they are
> essentially the same thing.

BTW, I feel that I may just be missing context on what the plans are
around mirroring and block migration.  Is there a plan or discussions
that haven't hit qemu-devel (maybe they were done in ovirt or
internally)?

Stefan

Re: [Qemu-devel] drive transactions (was Re: [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate commands)

[Paolo Bonzini]

On 02/27/2012 03:59 PM, Anthony Liguori wrote:
> The problem is that the current commands are not designed well.  For
> instance, multi-snapshot could look like:
> 
> block-freeze ide0-hd0
> block-freeze ide1-hd1
> block-reopen ide0-hd0 my-new-file0.qcow2
> block-reopen ide1-hd1 my-new-file1.qcow2
> block-unfreeze ide1-hd1
> block-unfreeze ide1-hd0
> 
> This would work regardless of whether the commands were implemented
> asynchronously within QEMU too.

This looks good, too.  Positive: maps well to fsfreeze/thaw with help
from the guest agent.  Negative: you have to specify the devices three
times.  Overall, I think I like it.

However, you need to add freeze/unfreeze capabilities to the block
layer.  Not hard, but one more thing to do.

Paolo

Re: [Qemu-devel] drive transactions (was Re: [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate commands)

[Anthony Liguori]

On 02/27/2012 09:03 AM, Paolo Bonzini wrote:

On 02/27/2012 03:59 PM, Anthony Liguori wrote:

The problem is that the current commands are not designed well.  For
instance, multi-snapshot could look like:

block-freeze ide0-hd0
block-freeze ide1-hd1
block-reopen ide0-hd0 my-new-file0.qcow2
block-reopen ide1-hd1 my-new-file1.qcow2
block-unfreeze ide1-hd1
block-unfreeze ide1-hd0

This would work regardless of whether the commands were implemented
asynchronously within QEMU too.


This looks good, too.  Positive: maps well to fsfreeze/thaw with help
from the guest agent.  Negative: you have to specify the devices three
times.  Overall, I think I like it.

However, you need to add freeze/unfreeze capabilities to the block
layer.  Not hard, but one more thing to do.


Right.  But it also generalizes to other QMP operations which is potentially 
interesting.


And providing mechanisms like this gives more flexibility to management tools to 
implement interesting features without constantly chancing new QMP commands.


Regards,

Anthony Liguori



Paolo

Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver

[Paolo Bonzini]

On 02/27/2012 03:59 PM, Stefan Hajnoczi wrote:
>> > I guess what I'm saying is, if we need to copy-paste in order to fork
>> > them in the future that's fine, but why maintain duplicates in the
>> > mean-time?  Please make the codebase nice today.  We can always extend
>> > it in the future, we're not forced to keep them unified forever if it
>> > turns out we want to split them.  But as it stands they are
>> > essentially the same thing.
> BTW, I feel that I may just be missing context on what the plans are
> around mirroring and block migration.  Is there a plan or discussions
> that haven't hit qemu-devel (maybe they were done in ovirt or
> internally)?

Not that I know of.  Me, I'm not on any oVirt list. :)

Paolo

Re: [Qemu-devel] [PATCH] kvm: notify host when guest paniced

[Jan Kiszka]

On 2012-02-27 04:01, Wen Congyang wrote:
> We can know the guest is paniced when the guest runs on xen.
> But we do not have such feature on kvm. This patch implemnts
> this feature, and the implementation is the same as xen:
> register panic notifier, and call hypercall when the guest
> is paniced.
> 
> Signed-off-by: Wen Congyang 
> ---
>  arch/x86/kernel/kvm.c|   12 
>  arch/x86/kvm/svm.c   |8 ++--
>  arch/x86/kvm/vmx.c   |8 ++--
>  arch/x86/kvm/x86.c   |   13 +++--
>  include/linux/kvm.h  |1 +
>  include/linux/kvm_para.h |1 +
>  6 files changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index f0c6fd6..b928d1d 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -331,6 +331,17 @@ static struct notifier_block kvm_pv_reboot_nb = {
>   .notifier_call = kvm_pv_reboot_notify,
>  };
>  
> +static int
> +kvm_pv_panic_notify(struct notifier_block *nb, unsigned long code, void 
> *unused)
> +{
> + kvm_hypercall0(KVM_HC_GUEST_PANIC);
> + return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block kvm_pv_panic_nb = {
> + .notifier_call = kvm_pv_panic_notify,
> +};
> +

You should split up host and guest-side changes.

>  static u64 kvm_steal_clock(int cpu)
>  {
>   u64 steal;
> @@ -417,6 +428,7 @@ void __init kvm_guest_init(void)
>  
>   paravirt_ops_setup();
>   register_reboot_notifier(&kvm_pv_reboot_nb);
> + atomic_notifier_chain_register(&panic_notifier_list, &kvm_pv_panic_nb);
>   for (i = 0; i < KVM_TASK_SLEEP_HASHSIZE; i++)
>   spin_lock_init(&async_pf_sleepers[i].lock);
>   if (kvm_para_has_feature(KVM_FEATURE_ASYNC_PF))
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 0b7690e..38b4705 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1900,10 +1900,14 @@ static int halt_interception(struct vcpu_svm *svm)
>  
>  static int vmmcall_interception(struct vcpu_svm *svm)
>  {
> + int ret;
> +
>   svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
>   skip_emulated_instruction(&svm->vcpu);
> - kvm_emulate_hypercall(&svm->vcpu);
> - return 1;
> + ret = kvm_emulate_hypercall(&svm->vcpu);
> +
> + /* Ignore the error? */
> + return ret == 0 ? 0 : 1;

Why can't kvm_emulate_hypercall return the right value?

>  }
>  
>  static unsigned long nested_svm_get_tdp_cr3(struct kvm_vcpu *vcpu)
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 66147ca..1b57ebb 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -4582,9 +4582,13 @@ static int handle_halt(struct kvm_vcpu *vcpu)
>  
>  static int handle_vmcall(struct kvm_vcpu *vcpu)
>  {
> + int ret;
> +
>   skip_emulated_instruction(vcpu);
> - kvm_emulate_hypercall(vcpu);
> - return 1;
> + ret = kvm_emulate_hypercall(vcpu);
> +
> + /* Ignore the error? */
> + return ret == 0 ? 0 : 1;
>  }
>  
>  static int handle_invd(struct kvm_vcpu *vcpu)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c9d99e5..3fc2853 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4923,7 +4923,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
>   u64 param, ingpa, outgpa, ret;
>   uint16_t code, rep_idx, rep_cnt, res = HV_STATUS_SUCCESS, rep_done = 0;
>   bool fast, longmode;
> - int cs_db, cs_l;
> + int cs_db, cs_l, r = 1;
>  
>   /*
>* hypercall generates UD from non zero cpl and real mode
> @@ -4964,6 +4964,10 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
>   case HV_X64_HV_NOTIFY_LONG_SPIN_WAIT:
>   kvm_vcpu_on_spin(vcpu);
>   break;
> + case KVM_HC_GUEST_PANIC:
> + vcpu->run->exit_reason = KVM_EXIT_GUEST_PANIC;
> + r = 0;
> + break;

That's the wrong place. This is a KVM hypercall, not a HyperV one.

>   default:
>   res = HV_STATUS_INVALID_HYPERCALL_CODE;
>   break;
> @@ -4977,7 +4981,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
>   kvm_register_write(vcpu, VCPU_REGS_RAX, ret & 0x);
>   }
>  
> - return 1;
> + return r;
>  }
>  
>  int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> @@ -5013,6 +5017,11 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>   case KVM_HC_VAPIC_POLL_IRQ:
>   ret = 0;
>   break;
> + case KVM_HC_GUEST_PANIC:
> + ret = 0;
> + vcpu->run->exit_reason = KVM_EXIT_GUEST_PANIC;
> + r = 0;
> + break;
>   default:
>   ret = -KVM_ENOSYS;
>   break;
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index acbe429..8f0e31b 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -163,6 +163,7 @@ struct kvm_pit_config {
>  #define KVM_EXIT_OSI  18
>  #define KVM_EXIT_PAPR_HCALL19
>  #define KVM_EXIT_S390_UCONTROL 20
> +#define KVM_EXIT_GUEST_PANIC

Re: [Qemu-devel] drive transactions (was Re: [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate commands)

[Kevin Wolf]

Am 27.02.2012 15:59, schrieb Anthony Liguori:
> On 02/27/2012 08:54 AM, Paolo Bonzini wrote:
>> On 02/27/2012 03:46 PM, Anthony Liguori wrote:
>>> I think a better way to think of this is as a batch submission.  It
>>> would be relatively easy to model in QMP too (just have a batch-command
>>> that has a list of commands as it's argument).
>>>
>>> The difference between batch submission and a transaction is atomic
>>> rollback. But I don't think atomic rollback is really needed here.
>>
>> A transaction enforces atomicity at the block layer level.  It's
>> different from batch commands in two ways:
>>
>> * bdrv_drain_all/bdrv_flush needs to be called at the beginning of the
>> commit.  This may not be the case with batch commands.
>>
>> * with batch commands, atomicity happens by chance because VCPUs cannot
>> send I/O while the monitor is holding the global mutex.
> 
> If the commands are designed correctly, then this works well.  The problem is 
> that the current commands are not designed well.  For instance, 
> multi-snapshot 
> could look like:
> 
> block-freeze ide0-hd0
> block-freeze ide1-hd1
> block-reopen ide0-hd0 my-new-file0.qcow2
> block-reopen ide1-hd1 my-new-file1.qcow2
> block-unfreeze ide1-hd1
> block-unfreeze ide1-hd0
> 
> This would work regardless of whether the commands were implemented 
> asynchronously within QEMU too.

What if block-reopen ide1-hd1 fails? In case of failure, you want both
drives to point to their old image, not the first one to the new image
and the second one to the old image.

Kevin

[Qemu-devel] ARM brk bug

[Bernhard M. Wiedemann]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Hi,

I found that running a debian arm5 bash with qemu runs into varying
problems with -R but works without. Also works fine on both armv5 and
armv7hf hardware.


This happened with both master and 1.0 builds:

curl www.zq1.de/~bernhard/temp/debian-bash-bug-nss-minimal.tar.gz |\
  tar xz
cd debian-bash-bug-nss-minimal
path/to/qemu/arm-linux-user/qemu-arm -R 500M -L . bin/bash
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
Segmentation fault


with chroot and our openSUSE qemu-arm-binfmt (which only does some
argv[0] magic) I saw it working after rm lib/libnss*

but otherwise it failed with messages like
bash: xmalloc: ../bash/variables.c:1971: cannot allocate 2 bytes (8192
bytes allocated)


Ciao
Bernhard M.
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.18 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk9LnmsACgkQSTYLOx37oWT6FgCdEgIriKRVmwkaMQU6jBGN/ABP
s/EAn1cZj4KytK9iC3i6EqqfIXFyEUVX
=DCV8
-END PGP SIGNATURE-

Re: [Qemu-devel] [PATCH] qed: replace vm_clock with rt_clock for qemu-tool compatibility

[Zhi Yong Wu]

On Mon, Feb 27, 2012 at 10:41 PM, Stefan Hajnoczi  wrote:
> On Mon, Feb 27, 2012 at 2:20 PM, Zhi Yong Wu  wrote:
>> On Sun, Feb 26, 2012 at 10:55 PM, Stefan Hajnoczi
>>  wrote:
>>> The QED dirty bit timer marks the file clean after allocating writes
>>> have drained.  This is cheaper than clearing/setting the dirty bit on
>>> each allocating write because the timer introduces a grace period which
>>> can be extended if more allocating writes arrive.
>>>
>>> The vm_clock was used in an attempt to prevent modifying the image file
>> Why can vm_clock do this work but other clock can not? If you're
>> available, can you elaborate what the difference among the three
>> clocks is? such as vm_clock, rt_clock, and host_clock.
>
> See qemu-timer.h.
thanks
>
> Stefan



-- 
Regards,

Zhi Yong Wu

Re: [Qemu-devel] [PATCH v2 1/2] qapi: Introduce blockdev-group-snapshot-sync command

[Jeff Cody]

On 02/27/2012 09:40 AM, Stefan Hajnoczi wrote:
> On Mon, Feb 27, 2012 at 2:26 PM, Jeff Cody  wrote:
>> On 02/27/2012 06:13 AM, Stefan Hajnoczi wrote:
>>> On Sun, Feb 26, 2012 at 7:31 PM, Jeff Cody  wrote:
>>>
>>> Do you have automated tests for this feature?
>>>
>>
>> No, not yet.  The testing has been manual.
> 
> For image streaming I used the Python unittest framework along with
> QMP/qmp.py to create tests.  I am going to submit it as a qemu-iotest.
>  We really need something along the lines of a harness with QMP
> support so that these block layer features can be tested.  I will CC
> you on the email.

Excellent, thanks.

> 
>>
 +/*
 + * Add new bs contents at the top of an image chain while the chain is 
 live,
 + * while keeping required fields on the top layer.
>>>
>>> Please also document the swap behavior.  It's pretty important for the
>>> caller to realize that once this function returns, their
>>> BlockDriverState arguments with have swapped.
>>
>> Good point.  How about this:
>>
>> /*
>>  * Add new bs contents at the top of an image chain while the chain is
>>  * live, while keeping required fields on the top layer.
>>  *
>>  * This will modify the BlockDriverState fields, and swap contents
>>  * between bs_new and bs_top.  Both bs_new and bs_top are modified.
> 
> Looks good.
> 
 + * It is assumed that bs_new already points to an existing image,
 + * with the correct backing filename of top->backing_file
>>>
>>> Not sure what this means.  Isn't bs_new going to use bs_top as its
>>> backing file?  Why "top->backing_file"?
>>
>> Sorry, that should have been 'bs_top->backing_file'. The image file is
>> not created by this function.  I added some more explanation, and
>> corrected that typo, in the above comment block.  Let me know if you
>> think it still needs more clarification.
> 
> I still don't follow.  Old bs_top's image file itself becomes the
> backing file, not bs_top->backing_file.  Perhaps I'm misinterpreting
> because of how swap changes bs_top and bs_new, but I'm reading it from
> the perspective of the caller when they pass in bs_top.
> 

Maybe it would be better to just replace that part of the comment with
something that says "This function does not create the image file".

The function bdrv_append() will neither create the image file, or set
(in the case of qcow2) the extended headers of the file to have the
backing filename.  It is only concerned with the bs contents.

For example, in group snapshots in blockdev.c, the image file is first
created, and then we do the swap:

/* create new image w/backing file */
ret = bdrv_img_create(snapshot_file, format,
  states->old_bs->filename,
  drv->format_name, NULL, -1, flags);

...

ret = bdrv_open(states->new_bs, snapshot_file,
flags | BDRV_O_NO_BACKING, drv);

...

bdrv_append(states->new_bs, states->old_bs);




> The rest looks good.
> 
> Stefan

Re: [Qemu-devel] [PATCH][v14] megasas: LSI Megaraid SAS HBA emulation

[Hannes Reinecke]

On 02/27/2012 11:31 AM, Michael S. Tsirkin wrote:
> On Mon, Feb 27, 2012 at 10:17:21AM +0100, Hannes Reinecke wrote:
[ .. ]
>>
>> The problem with mfi.h is that it's not actually _my_ file, but
>> rather copied over from NetBSD. I felt a bit stupid doing a recoding
>> of all the values which are already present in NetBSD ...
>> Hence the name 'mfi.h', and the different copyright.
>> For the same reason I try to keep the differences between the
>> NetBSD and my version as small as possible.
>>
>> If you have a better solution on how I should handle this
>> I'm willing to listen ...
> 
> Document where's the file from as suggested below.
> 
Yes, done.

 +if (megasas_use_msix(s) &&
 +msix_init(&s->dev, 15, &s->mmio_io, 0, 0x2000)) {
 +s->flags &= ~MEGASAS_MASK_USE_MSIX;
>>>
>>> You'd want an error message here. maybe even fail init.
>>>
>> But why? The driver continues happily without MSI-X.
>> And the failure message will be printed out via trace events,
>> in case someone is interested.
> 
> It is important that the configuration is fully specified
> by the flags.
> For example, otherwise migration to another host where
> MSIX does work will then fail.
> 
> 
Ok, thanks for the explanation.
But I'll be #ifdef'inf MSI-X support for the time being anyway.

 diff --git a/hw/mfi.h b/hw/mfi.h
 new file mode 100644
 index 000..4790c7c
 --- /dev/null
 +++ b/hw/mfi.h
>>>
>>> Sorry if this was discussed already, where is this
>>> code from? freebsd? it seems to have this:
>>> http://gitorious.org/freebsd/freebsd/blobs/HEAD/sys/dev/mfi/mfireg.h
>> Yes, that's the case.
>>
>>> Want to name the file the same and add a link?
>>> This would be an explanation why we keep the
>>> file in a weird style incompatible with qemu.
>>>
>>> Still some things I think are better off fixed.
>>> Noted below.
>>>
 +
 +/* Controller properties */
 +struct mfi_ctrl_props {
 +uint16_t seq_num;
 +uint16_t pred_fail_poll_interval;
 +uint16_t intr_throttle_cnt;
 +uint16_t intr_throttle_timeout;
 +uint8_t rebuild_rate;
 +uint8_t patrol_read_rate;
 +uint8_t bgi_rate;
 +uint8_t cc_rate;
 +uint8_t recon_rate;
 +uint8_t cache_flush_interval;
 +uint8_t spinup_drv_cnt;
 +uint8_t spinup_delay;
 +uint8_t cluster_enable;
 +uint8_t coercion_mode;
 +uint8_t alarm_enable;
 +uint8_t disable_auto_rebuild;
 +uint8_t disable_battery_warn;
 +uint8_t ecc_bucket_size;
 +uint16_t ecc_bucket_leak_rate;
 +uint8_t restore_hotspare_on_insertion;
 +uint8_t expose_encl_devices;
 +uint8_t maintainPdFailHistory;
 +uint8_t disallowHostRequestReordering;
 +uint8_t abortCCOnError;
 +uint8_t loadBalanceMode;
 +uint8_t disableAutoDetectBackplane;
 +uint8_t snapVDSpace;
 +struct {
 +/* set TRUE to disable copyBack (0=copyback enabled) */
 +uint32_t copyBackDisabled:1,
 +SMARTerEnabled:1,
 +prCorrectUnconfiguredAreas:1,
 +useFdeOnly:1,
 +disableNCQ:1,
 +SSDSMARTerEnabled:1,
 +SSDPatrolReadEnabled:1,
 +enableSpinDownUnconfigured:1,
 +autoEnhancedImport:1,
 +enableSecretKeyControl:1,
 +disableOnlineCtrlReset:1,
 +allowBootWithPinnedCache:1,
 +disableSpinDownHS:1,
 +enableJBOD:1,
 +reserved:18;
 +} OnOffProperties;
>>>
>>> Using bitfields for anything where you care about endian-ness
>>> is IMO wrong, and you normally do care for BE host + LE guest.
>>> No idea what bsd does to handle this.
>>>
>> Well, I don't really have a choice. That the firmware interface,
>> which is using this kind of construct.
>> So I'm getting passed in a bitfield, which I then have to evaluate.
>> I should be okay as I'll be doing a byteshuffle before evaluation.
>> But yes, this interface is absolutely horrible.
> 
> Bits are pretty comment as an interface.
> The sane thing to do is to have some macros or enums
> specifying the bits. Then you can do
> le32_to_cpu(x) & MFI_DISABLE_SPIN_DOWN_HS
> 
Yes, that's a good idea. Will be doing it.

> I don't see the shuffle in code.
> E.g. info->properties.OnOffProperties.enableJBOD = 1
> and no shuffle in sight.  Add a comment
> here and where you do the shuffle?
> 
Oh. Looks like wishful thinking here.
I _thought_ I did ...

Anyway, will be fixing it up.

Thanks for the review.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] drive transactions (was Re: [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate commands)

[Anthony Liguori]

On 02/27/2012 09:17 AM, Kevin Wolf wrote:

Am 27.02.2012 15:59, schrieb Anthony Liguori:

On 02/27/2012 08:54 AM, Paolo Bonzini wrote:

On 02/27/2012 03:46 PM, Anthony Liguori wrote:

I think a better way to think of this is as a batch submission.  It
would be relatively easy to model in QMP too (just have a batch-command
that has a list of commands as it's argument).

The difference between batch submission and a transaction is atomic
rollback. But I don't think atomic rollback is really needed here.


A transaction enforces atomicity at the block layer level.  It's
different from batch commands in two ways:

* bdrv_drain_all/bdrv_flush needs to be called at the beginning of the
commit.  This may not be the case with batch commands.

* with batch commands, atomicity happens by chance because VCPUs cannot
send I/O while the monitor is holding the global mutex.


If the commands are designed correctly, then this works well.  The problem is
that the current commands are not designed well.  For instance, multi-snapshot
could look like:

block-freeze ide0-hd0
block-freeze ide1-hd1
block-reopen ide0-hd0 my-new-file0.qcow2
block-reopen ide1-hd1 my-new-file1.qcow2
block-unfreeze ide1-hd1
block-unfreeze ide1-hd0

This would work regardless of whether the commands were implemented
asynchronously within QEMU too.


What if block-reopen ide1-hd1 fails? In case of failure, you want both
drives to point to their old image, not the first one to the new image
and the second one to the old image.


Then you get an error with the block devices still frozen.  You can execute 
another command to reopen back to the old image to roll back the transaction.


Pushing the rollback logic to the client does make the client interface a bit 
more complicated and adds latency to the error path but it's much easier than 
building a complex transaction infrastructure.


And there are examples of this in the wild too.  LDAP uses a similar mechanism.

Regards,

Anthony Liguori



Kevin

Re: [Qemu-devel] Boot failure with MS-Dos 6.22 (due to bad BIOS build?)

[Jan Kiszka]

On 2012-02-27 10:51, Daniel P. Berrange wrote:
> I'm seeing current QEMU GIT fail to boot MS-Dos 6.22 with the following
> crash:
> 
> # qemu-system-x86_64 -fda ~/MS-DOS\ 6.22.img  -m 1 -curses
> iPXE v1.0.0-591-g7aee315
>  iPXE (http://ipxe.org) 00:03.0 C900 PCI2.10 
> PnP PMM++ C900
> 
>  Booting from Floppy..
> .qemu: fatal: Trying to execute code outside 
> RAM or ROM at 0x0001000e
> 
> EAX= EBX= ECX=c934 EDX=0068
> ESI=6801 EDI= EBP=002b ESP=fff5
> EIP= EFL=0002 [---] CPL=0 II=0 A20=1 SMM=0 HLT=0
> ES =0040 0400  9300
> CS =f000 000f  9b00
> SS =9ec4 0009ec40  9300
> DS =9ec4 0009ec40  9300
> FS =   9300
> GS =   9300
> LDT=   8200
> TR =   8b00
> GDT= 000fcd78 0037
> IDT=  03ff
> CR0=0010 CR2= CR3= CR4=
> DR0= DR1= DR2= 
> DR3= 
> DR6=0ff0 DR7=0400
> CCS=00d0 CCD=0068 CCO=SARL
> EFER=
> FCW=037f FSW= [ST=0] FTW=00 MXCSR=1f80
> FPR0=  FPR1= 
> FPR2=  FPR3= 
> FPR4=  FPR5= 
> FPR6=  FPR7= 
> XMM00= XMM01=
> XMM02= XMM03=
> XMM04= XMM05=
> XMM06= XMM07=
> Aborted
> 
> 
> Git bisect blames this
> 
>   commit 41bd360325168b3c1db78eb7311420a1607d521f
>   Author: Jan Kiszka 
>   Date:   Sun Jan 15 17:48:25 2012 +0100
> 
> seabios: Update to release 1.6.3.1
> 
> User visible changes in seabios:
>  - Probe HPET existence (fix for -no-hpet)
>  - Probe PCI existence (fix for -machine isapc)
>  - usb: fix boot paths
> 
> Signed-off-by: Jan Kiszka 
> 
> 
> I tried to bisect Seabios, but every revision in Seabios upstream works
> fine.
> 
> Then I noticed, that if I rebuild the BIOS, from the exact same revision
> 1.6.3.1 revision that is committed in 'seabios' submodule in QEMU, then
> it works fine. So AFAICT, it is not the Seabios source code at fault,
> but rather the binary build we have commited to GIT. Should/can we rebuild
> the bios.bin in GIT ?

Probably not without understanding what causes this strange
inconsistency. If Seabios builds without errors and then later on fails,
this is also a bug.

Kevin, what information do you need to assess my tool chain?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] [PULL] slirp: Fix for requeuing crash & assert on DHCPNAK, cleanups

[Jan Kiszka]

The following changes since commit b4bd0b168e9f4898b98308f4a8a089f647a86d16:

  audio: Add some fall through comments (2012-02-25 18:16:11 +0400)

are available in the git repository at:
  git://git.kiszka.org/qemu.git queues/slirp

David Gibson (1):
  slirp: Fix assertion failure on rejected DHCP requests

Jan Kiszka (3):
  slirp: Clean up ifs_init
  slirp: Fix requeuing of batchq packets in if_start
  slirp: Refactor if_start

 slirp/bootp.c |3 +-
 slirp/if.c|  107 +++--
 slirp/if.h|2 -
 slirp/mbuf.h  |5 +++
 4 files changed, 58 insertions(+), 59 deletions(-)

Re: [Qemu-devel] [PATCH v2 1/2] qapi: Introduce blockdev-group-snapshot-sync command

[Stefan Hajnoczi]

On Mon, Feb 27, 2012 at 3:23 PM, Jeff Cody  wrote:
> On 02/27/2012 09:40 AM, Stefan Hajnoczi wrote:
>> On Mon, Feb 27, 2012 at 2:26 PM, Jeff Cody  wrote:
>>> On 02/27/2012 06:13 AM, Stefan Hajnoczi wrote:
 On Sun, Feb 26, 2012 at 7:31 PM, Jeff Cody  wrote:
> + * It is assumed that bs_new already points to an existing image,
> + * with the correct backing filename of top->backing_file

 Not sure what this means.  Isn't bs_new going to use bs_top as its
 backing file?  Why "top->backing_file"?
>>>
>>> Sorry, that should have been 'bs_top->backing_file'. The image file is
>>> not created by this function.  I added some more explanation, and
>>> corrected that typo, in the above comment block.  Let me know if you
>>> think it still needs more clarification.
>>
>> I still don't follow.  Old bs_top's image file itself becomes the
>> backing file, not bs_top->backing_file.  Perhaps I'm misinterpreting
>> because of how swap changes bs_top and bs_new, but I'm reading it from
>> the perspective of the caller when they pass in bs_top.
>>
>
> Maybe it would be better to just replace that part of the comment with
> something that says "This function does not create the image file".
>
> The function bdrv_append() will neither create the image file, or set
> (in the case of qcow2) the extended headers of the file to have the
> backing filename.  It is only concerned with the bs contents.

Makes sense.

Stefan

[Qemu-devel] [Bug 932487] Re: win32: git rev 59f971d crashes when accessing disk (coroutine issue)

[Eric Lassauge]

More or less same crash for me:
Using built-in specs.
COLLECT_GCC=d:\MinGW\bin\gcc.exe
COLLECT_LTO_WRAPPER=d:/mingw/bin/../libexec/gcc/mingw32/4.6.2/lto-wrapper.exe
Target: mingw32
Configured with: ../gcc-4.6.2/configure 
--enable-languages=c,c++,ada,fortran,objc,obj-c++ --disable-sjlj-exceptions 
--with-dwarf2 --enable-shared --enable-libgomp --disable-win32-registry 
--enable-libstdcxx-debug --enable-version-specific-runtime-libs --build=mingw32 
--prefix=/mingw
Thread model: win32
gcc version 4.6.2 (GCC) 

Host WinXP SP3 - Qemu-1.0.1

Maybe it can help:

Some stack frame (breakpoint set with command "where; continue" on
function qemu_coroutine_switch():

Breakpoint 1, qemu_coroutine_switch (from_=0x1989f34, to_=0x209df00, 
action=COROUTINE_YIELD) at coroutine-win32.c:41
41  {
#0  qemu_coroutine_switch (from_=0x1989f34, to_=0x209df00, 
action=COROUTINE_YIELD) at coroutine-win32.c:41
#1  0x004c3fe6 in _fu6882stack_chk_guard () at qemu-coroutine.c:31
#2  0x00410e1e in _fu528stack_chk_guard () at block.c:2518
#3  0x00403152 in _fu35stack_chk_guard () at async.c:71
#4  0x004a7a8e in _fu5545stack_chk_guard () at main-loop.c:472
#5  0x004a27db in main_loop () at 
d:\Documents\lassauge\Software\dev\Qemu\qemu-1.0.1\vl.c:1481
#6  _fu5383stack_chk_guard () at 
d:\Documents\lassauge\Software\dev\Qemu\qemu-1.0.1\vl.c:3485
#7  0x004a3b2a in _fu5385stack_chk_guard () at 
d:\Documents\lassauge\Software\dev\Qemu\qemu-1.0.1\vl.c:102
#8  0x005ddcf9 in console_main (argc=20, argv=0x1985d00) at 
./src/main/win32/SDL_win32_main.c:315
#9  0x005dddbb in WinMain@16 (hInst=0x40, hPrev=0x0, szCmdLine=0x241f18 "-L 
Bios -k fr -vga std -soundhw es1370 -boot 
menu=on,splash=bootsplash.bmp,splash-time=5000 -rtc base=localtime,clock=host 
-name linux-0.2 -drive file=linux-0.2.img,media=disk,cache=writeback 
-no-acpi"..., sw=10) at ./src/main/win32/SDL_win32_main.c:398
#10 0x005dd45a in main (argc=) at ../mingw/main.c:73
[Switching to Thread 5316.0xda0]

Breakpoint 1, qemu_coroutine_switch (from_=0x1989f34, to_=0x1bcf900, 
action=COROUTINE_YIELD) at coroutine-win32.c:41
41  {
#0  qemu_coroutine_switch (from_=0x1989f34, to_=0x1bcf900, 
action=COROUTINE_YIELD) at coroutine-win32.c:41
#1  0x004c3fe6 in _fu6882stack_chk_guard () at qemu-coroutine.c:31
#2  0x0041543d in _fu757stack_chk_guard () at block.c:2657
#3  0x00472b95 in _fu3751stack_chk_guard ()
#4  0x00554e1b in _fu11201stack_chk_guard () at 
d:\Documents\lassauge\Software\dev\Qemu\qemu-1.0.1\memory.c:446
#5  0x0054e5a8 in _fu10980stack_chk_guard () at 
d:\Documents\lassauge\Software\dev\Qemu\qemu-1.0.1\ioport.c:211
#6  0x0054eb9d in ioport_write (data=, address=503, index=0) at 
d:\Documents\lassauge\Software\dev\Qemu\qemu-1.0.1\ioport.c:82
#7  _fu10998stack_chk_guard () at 
d:\Documents\lassauge\Software\dev\Qemu\qemu-1.0.1\ioport.c:274
#8  0x026680cf in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)

Program received signal SIGILL, Illegal instruction.
0x68ac12ca in ?? () from d:\documents\lassauge\qemu-windows\libssp-0.dll
(gdb) at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0xbaadf011
Cannot access memory at address 0x0

Continuing.

Program received signal SIGILL, Illegal instruction.
0x68ac12ca in ?? () from d:\documents\lassauge\qemu-windows\libssp-0.dll
(gdb) where
#0  0x68ac12ca in ?? () from d:\documents\lassauge\qemu-windows\libssp-0.dll
#1  0x68ac1322 in libssp-0!__stack_chk_fail () from 
d:\documents\lassauge\qemu-windows\libssp-0.dll
#2  0x0044a399 in _fu2073stack_chk_guard () at coroutine-win32.c:50
#3  0x0049dc77 in _fu5254stack_chk_guard () at 
d:\Docume

Re: [Qemu-devel] ARM brk bug

[Peter Maydell]

On 27 February 2012 15:16, Bernhard M. Wiedemann  wrote:
> I found that running a debian arm5 bash with qemu runs into varying
> problems with -R but works without. Also works fine on both armv5 and
> armv7hf hardware.
>
>
> This happened with both master and 1.0 builds:
>
> curl www.zq1.de/~bernhard/temp/debian-bash-bug-nss-minimal.tar.gz |\
>  tar xz
> cd debian-bash-bug-nss-minimal
> path/to/qemu/arm-linux-user/qemu-arm -R 500M -L . bin/bash
> qemu: uncaught target signal 11 (Segmentation fault) - core dumped
> Segmentation fault

> but otherwise it failed with messages like
> bash: xmalloc: ../bash/variables.c:1971: cannot allocate 2 bytes (8192
> bytes allocated)

So, er, don't do that then? This looks suspiciously like we're
failing an mmap() (because of the limited guest address space you've
asked for with -R) and then bash is either failing to handle it
and crashing or printing a message about the allocation failure.

-- PMM

Re: [Qemu-devel] [PATCH v2 1/2] qapi: Introduce blockdev-group-snapshot-sync command

[Kevin Wolf]

Am 26.02.2012 20:31, schrieb Jeff Cody:
> This is a QAPI/QMP only command to take a snapshot of a group of
> devices. This is similar to the blockdev-snapshot-sync command, except
> blockdev-group-snapshot-sync accepts a list devices, filenames, and
> formats.
> 
> It is attempted to keep the snapshot of the group atomic; if the
> creation or open of any of the new snapshots fails, then all of
> the new snapshots are abandoned, and the name of the snapshot image
> that failed is returned.  The failure case should not interrupt
> any operations.
> 
> Rather than use bdrv_close() along with a subsequent bdrv_open() to
> perform the pivot, the original image is never closed and the new
> image is placed 'in front' of the original image via manipulation
> of the BlockDriverState fields.  Thus, once the new snapshot image
> has been successfully created, there are no more failure points
> before pivoting to the new snapshot.
> 
> This allows the group of disks to remain consistent with each other,
> even across snapshot failures.
> 
> Signed-off-by: Jeff Cody 
> ---
>  block.c  |   47 
>  block.h  |1 +
>  blockdev.c   |  128 
> ++
>  qapi-schema.json |   38 
>  4 files changed, 214 insertions(+), 0 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 3621d11..0045ab1 100644
> --- a/block.c
> +++ b/block.c
> @@ -880,6 +880,53 @@ void bdrv_make_anon(BlockDriverState *bs)
>  bs->device_name[0] = '\0';
>  }
>  
> +/*
> + * Add new bs contents at the top of an image chain while the chain is live,
> + * while keeping required fields on the top layer.
> + *
> + * It is assumed that bs_new already points to an existing image,
> + * with the correct backing filename of top->backing_file
> + */
> +void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
> +{
> +BlockDriverState tmp;
> +
> +/* the new bs must not be in bdrv_states */
> +bdrv_make_anon(bs_new);
> +
> +tmp = *bs_new;
> +tmp.backing_hd = bs_new;
> +
> +/* there are some fields that need to stay on the top layer: */
> +
> +/* dev info */
> +tmp.dev_ops  = bs_top->dev_ops;
> +tmp.dev_opaque   = bs_top->dev_opaque;
> +tmp.dev  = bs_top->dev;
> +tmp.buffer_alignment = bs_top->buffer_alignment;
> +tmp.copy_on_read = bs_top->copy_on_read;
> +
> +/* i/o timing parameters */
> +tmp.slice_time= bs_top->slice_time;
> +tmp.slice_start   = bs_top->slice_start;
> +tmp.slice_end = bs_top->slice_end;
> +tmp.io_limits = bs_top->io_limits;
> +tmp.io_base   = bs_top->io_base;
> +tmp.throttled_reqs= bs_top->throttled_reqs;
> +tmp.block_timer   = bs_top->block_timer;
> +tmp.io_limits_enabled = bs_top->io_limits_enabled;

What about iostatus_enabled/iostatus? Possibly on_read/write_error and
geometry, too. (Maybe the correct answer is that you're doing the right
thing, but I wanted to bring it up)

> +
> +/* keep the same entry in bdrv_states */
> +pstrcpy(tmp.device_name, sizeof(tmp.device_name), bs_top->device_name);
> +tmp.list = bs_top->list;
> +
> +/* swap contents of the fixed new bs and the current top */
> +*bs_new = *bs_top;
> +*bs_top = tmp;
> +
> +bdrv_detach_dev(bs_new, bs_new->dev);
> +}

The last line would actually deserve a comment /* clear the copied
fields in the new backing file */, which makes clear that there are
probably some more fields missing in this section.

> +
>  void bdrv_delete(BlockDriverState *bs)
>  {
>  assert(!bs->dev);
> diff --git a/block.h b/block.h
> index cae289b..190a780 100644
> --- a/block.h
> +++ b/block.h
> @@ -114,6 +114,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
>  int bdrv_create_file(const char* filename, QEMUOptionParameter *options);
>  BlockDriverState *bdrv_new(const char *device_name);
>  void bdrv_make_anon(BlockDriverState *bs);
> +void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
>  void bdrv_delete(BlockDriverState *bs);
>  int bdrv_parse_cache_flags(const char *mode, int *flags);
>  int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
> diff --git a/blockdev.c b/blockdev.c
> index 05e7c5e..560f7e8 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -714,6 +714,134 @@ void qmp_blockdev_snapshot_sync(const char *device, 
> const char *snapshot_file,
>  }
>  }
>  
> +
> +/* New and old BlockDriverState structs for group snapshots */
> +typedef struct BlkGroupSnapshotStates {
> +BlockDriverState *old_bs;
> +BlockDriverState *new_bs;
> +bool is_open;
> +QSIMPLEQ_ENTRY(BlkGroupSnapshotStates) entry;
> +} BlkGroupSnapshotStates;
> +
> +/*
> + * 'Atomic' group snapshots.  The snapshots are taken as a set, and if any 
> fail
> + *  then we do not pivot any of the devices in the group, and abandon the
> + *  snapshots
> + */
> +void qmp_

Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model

[Paul Brook]

> On Tue, Feb 21, 2012 at 11:04 PM, Paul Brook  wrote:
> >> > +static inline int64_t is_between(int64_t x, int64_t a, int64_t b)
> >> > +{
> >> > +if (a < b) {
> >> > +return x > a && x <= b;
> >> > +}
> >> > +return x < a && x >= b;
> >> > +}
> >> 
> >> This looks slightly odd -- should the boundary condition for whether
> >> a value equal to the max/min really change depending on :whether a
> >> or b is greater?
> 
> The function determines whether x is in-between a and b exclusive of
> a, inclusive of b, so it is consistent with itself in that regard.
> 
> > This is a ugly hack.  Instead of figuring out whether we have a count-up
> > or count-down timer the code checks for both, and have the "in_between"
> > function magically DTRT.  I haven't followed the paths through in enough
> > detail to figure out whether it gets all the corner cases right.
> 
> Is it really a "hack"?? For count up b will always be greater than a,
> and for count down the reverse. I suppose I could assert these
> conditions at the call site for peace of mind? The invocation from
> cadence_timer_run doesn't care whether it is count up of count down,
> it really does just only care if the match value is in-between the
> current timer value and the next timer value, which is exactly what
> this function determines.

When you explain it like this, it makes a more sense.  But this isn't 
immediately obvious from the code.  It took me at least a couple of readings 
to figure out what was going on. This is exactly the sort of thing that should 
be described in comments.  A function with a very generic name is used in a 
way that has fairly subtle implications.  There's a good chance someone[1] 
will come along in a few months/years, reuse this function and "fix" the 
wierdness at the same time.

Annother non-obvious detail is the way you handle overflow.  Specifically you 
check a range both plus and minus the wrap value before wrapping the final 
count.  This is certainly confusing/surprising when you first encounter it.  
Very large steps result in overlapping ranges, which triggers [in this case 
harmless] warning bells.

Thinking about that, I realised why I don't like the following line:

> +s->reg_value = (uint32_t)((x + interval) % interval);

This assumes x > -interval, which is not always true.

Paul

[1] "someone" includes me.  After I've forgotten this obscure detail.

Re: [Qemu-devel] qemu assertion failed with usb on current git master!

[Gerd Hoffmann]

  Hi,

> I'm really sorry, but I don't understand what's happening - I copied the
> qemu executable on my target system before executing it, but gdb complains
> that the core file does not match the executable! But except the file paths
> they are identical.

> warning: core file may not match specified executable file.
> Core was generated by `/disc/qemu-system-x86_64 -machine kernel_irqchip=on
> -serial /dev/ttyS2 -usb -de'.
> Program terminated with signal 6, Aborted.
> #0  0xe424 in __kernel_vsyscall ()

Strange.  The backtrace is bogus too.

> I don't know how to proceed here.

Lets try plan b: add a printf right before the assert:

--- a/hw/usb.c
+++ b/hw/usb.c
@@ -356,6 +356,7 @@ void usb_packet_complete(USBDevice *dev, USBPacket *p)

 while (!QTAILQ_EMPTY(&ep->queue)) {
 p = QTAILQ_FIRST(&ep->queue);
+fprintf(stderr, "%s: packet %p\n", __func__, p);
 assert(p->state == USB_PACKET_QUEUED);
 ret = usb_process_one(p);
 if (ret == USB_RET_ASYNC) {


> Don't you run into this problem (crash on USB plug in) on your system?
> I tested it with a Linux guest, there it does not crash! Only with a
> Windows XP guest!

I test with Linux most of the time, but even with windows xp guest it
doesn't reproduce here.

cheers,
  Gerd

Re: [Qemu-devel] [SeaBIOS] Boot failure with MS-Dos 6.22 (due to bad BIOS build?)

[Peter Stuge]

Jan Kiszka wrote:
> > Then I noticed, that if I rebuild the BIOS, from the exact same revision
> > 1.6.3.1 revision that is committed in 'seabios' submodule in QEMU, then
> > it works fine. So AFAICT, it is not the Seabios source code at fault,
> > but rather the binary build we have commited to GIT. Should/can we rebuild
> > the bios.bin in GIT ?
> 
> Probably not without understanding what causes this strange
> inconsistency. If Seabios builds without errors and then later on fails,
> this is also a bug.
> 
> Kevin, what information do you need to assess my tool chain?

In the coreboot project we have more than 10 years of experience from
distribution toolchains consistently being too broken to build a
working coreboot image. The same problems apply to SeaBIOS.

As you know, distribution toolchains are heavily patched, presumably
to add some value to the distribution. The patches work fine when the
toolchains should output userland binaries or the odd kernel. They
fail frequently and in countless ways when used to produce bare metal
binaries.

Within coreboot it is much less effort to build an i386-elf cross
toolchain than to mess with the hundreds if not thousands of issues
in the distribution toolchains. The same applies to SeaBIOS of
course. The script we use in coreboot is here:

http://review.coreboot.org/gitweb?p=coreboot.git;a=tree;f=util/crossgcc

If you want to investigate and spend time on motivating distributions
to unbreak their toolchains that's awesome, but be prepared to spend
many weeks disassembling binaries and reverse engineering the toolchain.


//Peter

Re: [Qemu-devel] [Bug 932487] Re: win32: git rev 59f971d crashes when accessing disk (coroutine issue)

[Paolo Bonzini]

On 02/17/2012 01:53 AM, Roy Tam wrote:
>> > Please try these modified configure option which adds the compiler flag 
>> > needed for multithreading:
>> > --extra-cflags="-O0 -pipe -mthreads". For me, -mthreads solved the problem.
>> >
> Yes "-mthreads" switch does workaround the issue.
> But using "-mthreads" making resulting binaries depend on
> mingwm10.dll, which is not good.
> 

Is "-D_MT" enough instead of -mthreads?

Paolo

Re: [Qemu-devel] Fail to share Samba directory with guest

[Shu Ming]

On 2012-2-27 17:21, Jun Koi wrote:

hi,

on qemu 1.0.1, i am trying to share a host directory with the Windows
guest like below:

qemu-system-i386 -enable-kvm -m 1000 -net nic,model=rtl8139 -net
user,smb=/tmp img.winxp

but in the guest, \\10.0.2.4 doesnt show me any shared directory.

i already run Samba on the host (default configuration).

did i miss something, or is it a bug??


So 10.0.2.4 is your host IP with samba server?   And what's the network 
the guest belongs to?




thanks,
Jun




--
Shu Ming
IBM China Systems and Technology Laboratory