Re: [Qemu-devel] [PATCH 1/7] backends: Fix warning from Sparse

[Michael Tokarev]

10.03.2015 09:22, Stefan Weil wrote:

> No, that was my mistake. Please excuse also the cris patch which I sent twice.
> I did not notice that it already had caused trouble, nor did I notice that I 
> had already sent it.
> In my local patch queue (~ 50 patches) there was even a 2nd cris patch
> which removed the unused variables...

No problems with that (except that we bothered Peter twice
with this cris thing already, and I really mean "we", since
it escaped my testing and accurate review too, and it was
twice too, so I'm as guilty if not more).

And this situation suggests that I should process and submit
patches a) more carefully and b) faster.  So it is, again,
good portion of my fault.  Not doing trivial-patches merging
for 3 weeks and rushing a whole lot in one go without that
good review isn't going to work well.

Thank you for the good work!  And it really is good, while
boring...

/mjt

Re: [Qemu-devel] [PATCH 1/3] pcie_aer: fix typos in pcie_aer_inject_error comment

[Michael Tokarev]

10.03.2015 04:49, Chen Fan пишет:
> Refer to "PCI Express Base Spec3.0", this comments can't
> fit the description in spec, so we should fix them.
> 
> Signed-off-by: Chen Fan 
> ---
>  hw/pci/pcie_aer.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> index 1f4be16..7ca077a 100644
> --- a/hw/pci/pcie_aer.c
> +++ b/hw/pci/pcie_aer.c
> @@ -618,11 +618,11 @@ static bool pcie_aer_inject_uncor_error(PCIEAERInject 
> *inj, bool is_fatal)
>   * non-Function specific error must be recorded in all functions.
>   * It is the responsibility of the caller of this function.
>   * It is also caller's responsibility to determine which function should
> - * report the rerror.
> + * report the error.
>   *
>   * 6.2.4 Error Logging
> - * 6.2.5 Sqeunce of Device Error Signaling and Logging Operations
> - * table 6-2: Flowchard Showing Sequence of Device Error Signaling and 
> Logging
> + * 6.2.5 Sequence of Device Error Signaling and Logging Operations
> + * table 6-2: Flowchart Showing Sequence of Device Error Signaling and 
> Logging
>   *Operations

this 6-2 is not a table, it is "Figure 6-2" on page 479, unless, ofcourse, you 
mean
"Table 6-2: General PCI Express Error List" on page 481.

I can fix this when applying.

Thanks,

/mjt

>   */
>  int pcie_aer_inject_error(PCIDevice *dev, const PCIEAERErr *err)
> 

Re: [Qemu-devel] Using tap device for qemu

[Shannon Zhao]

On 2015/3/10 12:59, Gayathri Nagarajan wrote:
> I have a problem with networking in qemu. I want to use the Linux tap device 
> for qemu network. For that
> 
> *1. I created a bridge -*
> 
> $ sudo brctl addbr br0 
> 
> *2. I used the following qemu-ifup script*
> 
> #!/bin/sh
> set -x
> 
> switch=br0
> 
> if [ -n "$1" ];then
> /usr/bin/sudo /usr/sbin/tunctl -u `whoami` -t $1
> /usr/bin/sudo /sbin/ip link set $1 up
> sleep 0.5s
> /usr/bin/sudo /usr/sbin/brctl addif $switch $1
> exit 0
> else
> echo "Error: no interface specified"
> exit 1
> fi
> 
> *3. When I run qemu using*
> 
> $ qemu-system-i386 -m 200M -cdrom images/ss.iso -serial stdio -device 
> e1000,netdev=net0,mac=DE:AD:BE:EF:00:C0 -netdev 
> tap,id=net0,script=/home/user/qemu-ifup
> 
> */home/user/qemu-ifup:could not launch network script*
> 

Please check whether qemu-ifup exits at this directory and qemu-ifup has 
executable permission.

> *qemu-system-i386: -netdev tap,id=net0,script=/home/user/qemu-ifup: Device 
> 'tap' could not be initialized*
> 
> 
> How do I resolve this problem?
> 
> Thank you
> 
> gayathri
> 
> 


-- 
Thanks,
Shannon

Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 for-2.3 00/25] hw/pc: implement multiple primary busses for pc machines

[Alexey Kardashevskiy]

On 03/10/2015 06:21 AM, Marcel Apfelbaum wrote:

On 03/09/2015 06:55 PM, Gerd Hoffmann wrote:

On Mo, 2015-03-09 at 18:26 +0200, Marcel Apfelbaum wrote:

On 03/09/2015 04:19 PM, Gerd Hoffmann wrote:

Hi,


My series is based on commit 09d219a. Try please on top of this commit.


Ok, that works.  Going to play with that now ;)

Good luck! ... and tell me what you think :)
If you need any help with the command line of the pxb device, let me know,.


First thing I've noticed:  You need to define a numa node so you can
pass a valid numa node to the pxb-device.  Guess that is ok as the whole
point of this is to assign pci devices to numa nodes.  More complete
test instructions would be nice though.

Exactly, this is by design. But you can also use it without specifying the
NUMA node...

A detailed command line would be:

[qemu-bin + storage options]
-bios [seabios-dir]/out/bios.bin -L [seabios-dir]/out/
-m 2G
-object memory-backend-ram,size=1024M,policy=bind,host-nodes=0,id=ram-node0
-numa node,nodeid=0,cpus=0,memdev=ram-node0
-object
memory-backend-ram,size=1024M,policy=interleave,host-nodes=0,id=ram-node1
-numa node,nodeid=1,cpus=1,memdev=ram-node1
-device pxb-device,id=bridge1,bus=pci.0,numa_node=1,bus_nr=4 -netdev
user,id=nd-device e1000,bus=bridge1,addr=0x4,netdev=nd
-device pxb-device,id=bridge2,bus=pci.0,numa_node=0,bus_nr=8 -device
e1000,bus=bridge2,addr=0x3
-device pxb-device,id=bridge3,bus=pci.0,bus_nr=40 -drive
if=none,id=drive0,file=[img] -device
virtio-blk-pci,drive=drive0,scsi=off,bus=bridge3,addr=1



I replayed this patchset on top of 09d219a "acpi: update generated files" 
and got this:



qemu-system-x86_64: -object 
memory-backend-ram,size=1024M,policy=bind,host-nodes=0,id=ram-node0: NUMA 
node binding are not supported by this QEMU
qemu-system-x86_64: -object 
memory-backend-ram,size=1024M,policy=interleave,host-nodes=0,id=ram-node1: 
NUMA node binding are not supported by this QEMU



This is my exact command line:

/scratch/alexey/p/qemu-build/x86_x86_64/x86_64-softmmu/qemu-system-x86_64 \
-L /home/alexey/p/qemu/pc-bios/ \
-hda x86/fc19_24GB_x86.qcow2 \
-enable-kvm \
-kernel x86/vmlinuz-3.12.11-201.fc19.x86_64 \
-initrd x86/initramfs-3.12.11-201.fc19.x86_64.img \
-append "root=/dev/sda3 console=ttyS0" \
-nographic \
-nodefaults \
-chardev stdio,id=id2,signal=off,mux=on \
-device isa-serial,id=id3,chardev=id2 \
-mon id=id4,chardev=id2,mode=readline \
-m 2G \
-object memory-backend-ram,size=1024M,policy=bind,host-nodes=0,id=ram-node0 \
-numa node,nodeid=0,cpus=0,memdev=ram-node0 \
-object 
memory-backend-ram,size=1024M,policy=interleave,host-nodes=0,id=ram-node1 \

-numa node,nodeid=1,cpus=1,memdev=ram-node1 \
-device pxb-device,id=bridge1,bus=pci.0,numa_node=1,bus_nr=4 \
-netdev user,id=nd-device e1000,bus=bridge1,addr=0x4,netdev=nd \
-device pxb-device,id=bridge2,bus=pci.0,numa_node=0,bus_nr=8 \
-device e1000,bus=bridge2,addr=0x3 \
-device pxb-device,id=bridge3,bus=pci.0,bus_nr=40 \
-drive if=none,id=drive0,file=debian_lenny_powerpc_desktop.qcow2 \
-device virtio-blk-pci,drive=drive0,scsi=off,bus=bridge3,addr=1 \


What am I missing here?


What I actually wanted to find out (instead of asking what I am doing now) 
is is this PXB device a PCI device sitting on the same PCI host bus adapter 
(1) or it is a separate PHB (2) with its own PCI domain (new  in 
:00:00.0 PCI address)? I would think it is (1) but then what exactly do 
you call "A primary PCI bus" here (that's my ignorance speaking, yes  :) )? 
Thanks.





Here you have:
  - 2 NUMA nodes for the guest, 0 and 1. (both mapped to the same NUMA node
in host, but you can and should put it in different host NUMA nodes)
  - a pxb host bridge attached to NUMA 1 with an e1000 behind it
  - a pxb host bridge attached to NUMA 0 with an e1000 behind it
  - a pxb host bridge not attached to any NUMA with a hard drive behind it.

As you can see, since you already "decide" NUMA mapping at command line, it
is "natural" also to attach the pxbs to the NUMA nodes.



Second thing:  Booting with an unpatched seabios has bad effects:

[root@localhost ~]# cat /proc/iomem
-000f : PCI Bus :10
   -0fff : reserved
   1000-0009fbff : System RAM
   0009fc00-0009 : reserved
   000c-000c91ff : Video ROM
   000c9800-000ca1ff : Adapter ROM
   000ca800-000ccbff : Adapter ROM
   000f-000f : reserved
 000f-000f : System ROM
0010-3ffd : System RAM
   0100-0174bde4 : Kernel code
   0174bde5-01d30cff : Kernel data
   01eaa000-0202afff : Kernel bss
3ffe-3fff : reserved
fd00-fdff : :00:02.0
   fd00-fdff : bochs-drm
febc-febd : :00:03.0
   febc-febd : e1000
febf-febf0fff : :00:02.0
   febf-febf0fff : bochs-drm
fec0-fec003ff : IOAPIC 0
fed0-fed003ff : HPET 0
   fed0-fed003ff : PNP0103:00
fee0-fee00fff : Local APIC
feffc000-feff : reserved
fffc- : reserved

"PCI Bus :10" is bogus and "PCI Bus 00

Re: [Qemu-devel] [PATCH] target-moxie: Fix warnings from Sparse (one-bit signed bitfield)

[Michael Tokarev]

Applied to -trivial, thank you!

/mjt

Re: [Qemu-devel] [PATCH 1/7] backends: Fix warning from Sparse

[Stefan Weil]

Am 10.03.2015 um 07:04 schrieb Michael Tokarev:

Applied all 7 to -trivial.

Is it intentional that there's no cover letter for the series?

Thanks,

/mjt


No, that was my mistake. Please excuse also the cris patch which I sent 
twice.
I did not notice that it already had caused trouble, nor did I notice 
that I had already sent it.

In my local patch queue (~ 50 patches) there was even a 2nd cris patch
which removed the unused variables...

Regards
Stefan

Re: [Qemu-devel] [PATCH v2] error: Replace error_report() & error_free() with error_report_err()

[Michael Tokarev]

04.03.2015 13:25, zhanghailiang wrote:
> This is a continuation of the work started in commit 565f65d27:
> "error: Use error_report_err() where appropriate"

Applied to -trivial, thank you!

/mjt

Re: [Qemu-devel] [PATCH] 9pfs-local: simplify/optimize local_mapped_attr_path()

[Michael Tokarev]

05.03.2015 04:13, Fam Zheng wrote:
> On Thu, 03/05 00:03, Michael Tokarev wrote:
>> +const char *name = basename((char*)path);
> 
> checkpatch.pl complained this, s/char*/char */

I've fixed it in git.  Thank you for spotting this.

/mjt

Re: [Qemu-devel] [PATCH v5 13/45] ram_debug_dump_bitmap: Dump a migration bitmap as text

[David Gibson]

On Wed, Feb 25, 2015 at 04:51:36PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> Misses out lines that are all the expected value so the output
> can be quite compact depending on the circumstance.
> 
> Signed-off-by: Dr. David Alan Gilbert 
> ---
>  arch_init.c   | 39 +++
>  include/migration/migration.h |  1 +
>  2 files changed, 40 insertions(+)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 91645cc..fe0df0d 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -776,6 +776,45 @@ static void reset_ram_globals(void)
>  
>  #define MAX_WAIT 50 /* ms, half buffered_file limit */
>  
> +/*
> + * 'expected' is the value you expect the bitmap mostly to be full
> + * of and it won't bother printing lines that are all this value
> + * if 'todump' is null the migration bitmap is dumped.
> + */
> +void ram_debug_dump_bitmap(unsigned long *todump, bool expected)
> +{
> +int64_t ram_pages = last_ram_offset() >> TARGET_PAGE_BITS;
> +
> +int64_t cur;
> +int64_t linelen = 128;
> +char linebuf[129];
> +
> +if (!todump) {
> +todump = migration_bitmap;

Any reason not to just have the caller pass migration_bitmap, if
that's what they want?

> +}
> +
> +for (cur = 0; cur < ram_pages; cur += linelen) {
> +int64_t curb;
> +bool found = false;
> +/*
> + * Last line; catch the case where the line length
> + * is longer than remaining ram
> + */
> +if (cur+linelen > ram_pages) {
> +linelen = ram_pages - cur;
> +}
> +for (curb = 0; curb < linelen; curb++) {
> +bool thisbit = test_bit(cur+curb, todump);
> +linebuf[curb] = thisbit ? '1' : '.';
> +found = found || (thisbit != expected);
> +}
> +if (found) {
> +linebuf[curb] = '\0';
> +fprintf(stderr,  "0x%08" PRIx64 " : %s\n", cur, linebuf);

Might be slightly more readable if it printed GPAs instead of page
numbers.

> +}
> +}
> +}
> +
>  static int ram_save_setup(QEMUFile *f, void *opaque)
>  {
>  RAMBlock *block;
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 5242ead..3776e86 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -156,6 +156,7 @@ uint64_t xbzrle_mig_pages_cache_miss(void);
>  double xbzrle_mig_cache_miss_rate(void);
>  
>  void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
> +void ram_debug_dump_bitmap(unsigned long *todump, bool expected);
>  
>  /**
>   * @migrate_add_blocker - prevent migration from proceeding

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


pgpZ_co5zij6o.pgp
Description: PGP signature

Re: [Qemu-devel] [PATCH v5 12/45] Return path: Source handling of return path

[David Gibson]

On Wed, Feb 25, 2015 at 04:51:35PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> Open a return path, and handle messages that are received upon it.
> 
> Signed-off-by: Dr. David Alan Gilbert 
> ---
>  include/migration/migration.h |   8 ++
>  migration/migration.c | 178 
> +-
>  trace-events  |  13 +++
>  3 files changed, 198 insertions(+), 1 deletion(-)
> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 6775747..5242ead 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -73,6 +73,14 @@ struct MigrationState
>  
>  int state;
>  MigrationParams params;
> +
> +/* State related to return path */
> +struct {
> +QEMUFile *file;
> +QemuThreadrp_thread;
> +bool  error;
> +} rp_state;
> +
>  double mbps;
>  int64_t total_time;
>  int64_t downtime;
> diff --git a/migration/migration.c b/migration/migration.c
> index 80d234c..34cd4fe 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -237,6 +237,23 @@ MigrationCapabilityStatusList 
> *qmp_query_migrate_capabilities(Error **errp)
>  return head;
>  }
>  
> +/*
> + * Return true if we're already in the middle of a migration
> + * (i.e. any of the active or setup states)
> + */
> +static bool migration_already_active(MigrationState *ms)
> +{
> +switch (ms->state) {
> +case MIG_STATE_ACTIVE:
> +case MIG_STATE_SETUP:
> +return true;
> +
> +default:
> +return false;
> +
> +}
> +}
> +
>  static void get_xbzrle_cache_stats(MigrationInfo *info)
>  {
>  if (migrate_use_xbzrle()) {
> @@ -362,6 +379,21 @@ static void migrate_set_state(MigrationState *s, int 
> old_state, int new_state)
>  }
>  }
>  
> +static void migrate_fd_cleanup_src_rp(MigrationState *ms)
> +{
> +QEMUFile *rp = ms->rp_state.file;
> +
> +/*
> + * When stuff goes wrong (e.g. failing destination) on the rp, it can get
> + * cleaned up from a few threads; make sure not to do it twice in 
> parallel
> + */
> +rp = atomic_cmpxchg(&ms->rp_state.file, rp, NULL);

A cmpxchg seems dangerously subtle for such a basic and infrequent
operation, but ok.

> +if (rp) {
> +trace_migrate_fd_cleanup_src_rp();
> +qemu_fclose(rp);
> +}
> +}
> +
>  static void migrate_fd_cleanup(void *opaque)
>  {
>  MigrationState *s = opaque;
> @@ -369,6 +401,8 @@ static void migrate_fd_cleanup(void *opaque)
>  qemu_bh_delete(s->cleanup_bh);
>  s->cleanup_bh = NULL;
>  
> +migrate_fd_cleanup_src_rp(s);
> +
>  if (s->file) {
>  trace_migrate_fd_cleanup();
>  qemu_mutex_unlock_iothread();
> @@ -406,6 +440,11 @@ static void migrate_fd_cancel(MigrationState *s)
>  QEMUFile *f = migrate_get_current()->file;
>  trace_migrate_fd_cancel();
>  
> +if (s->rp_state.file) {
> +/* shutdown the rp socket, so causing the rp thread to shutdown */
> +qemu_file_shutdown(s->rp_state.file);

I missed where qemu_file_shutdown() was implemented.  Does this
introduce a leftover socket dependency?

> +}
> +
>  do {
>  old_state = s->state;
>  if (old_state != MIG_STATE_SETUP && old_state != MIG_STATE_ACTIVE) {
> @@ -658,8 +697,145 @@ int64_t migrate_xbzrle_cache_size(void)
>  return s->xbzrle_cache_size;
>  }
>  
> -/* migration thread support */
> +/*
> + * Something bad happened to the RP stream, mark an error
> + * The caller shall print something to indicate why
> + */
> +static void source_return_path_bad(MigrationState *s)
> +{
> +s->rp_state.error = true;
> +migrate_fd_cleanup_src_rp(s);
> +}
> +
> +/*
> + * Handles messages sent on the return path towards the source VM
> + *
> + */
> +static void *source_return_path_thread(void *opaque)
> +{
> +MigrationState *ms = opaque;
> +QEMUFile *rp = ms->rp_state.file;
> +uint16_t expected_len, header_len, header_com;
> +const int max_len = 512;
> +uint8_t buf[max_len];
> +uint32_t tmp32;
> +int res;
> +
> +trace_source_return_path_thread_entry();
> +while (rp && !qemu_file_get_error(rp) &&
> +migration_already_active(ms)) {
> +trace_source_return_path_thread_loop_top();
> +header_com = qemu_get_be16(rp);
> +header_len = qemu_get_be16(rp);
> +
> +switch (header_com) {
> +case MIG_RP_CMD_SHUT:
> +case MIG_RP_CMD_PONG:
> +expected_len = 4;

Could the knowledge of expected lengths be folded into the switch
below?  Switching twice on the same thing is a bit icky.

> +break;
> +
> +default:
> +error_report("RP: Received invalid cmd 0x%04x length 0x%04x",
> +header_com, header_len);
> +source_return_path_bad(ms);
> +goto out;
> +}
>  
> +if (header_len > expected_len) {
> +

Re: [Qemu-devel] [PATCH v2] arm: fix memory leak

[Michael Tokarev]

This one appears to work better.
Applied to -trivial, thank you!

/mjt

Re: [Qemu-devel] [PATCH 5/8] ioport: Remove unused functions

[Thomas Huth]

On Mon, 09 Mar 2015 20:23:24 +0100
Paolo Bonzini  wrote:

> 
> On 09/03/2015 18:30, Thomas Huth wrote:
> > The functions portio_list_destroy() and portio_list_del()
> > are not used anywhere, so let's remove them.
> > 
> > Signed-off-by: Thomas Huth 
> > Cc: Paolo Bonzini 
> 
> I think it's better to provide a complete API, mimicking memory_region_*
> as much as possible, so I'd rather keep these.

Ok, then let's keep then and drop this patch.

 Thomas

Re: [Qemu-devel] [PATCH 0/2] monitor: Fix and clean up around .user_print

[Michael Tokarev]

06.03.2015 12:09, Markus Armbruster wrote:
> A more thorough cleanup of the area is in the works, but I want to get
> this out quickly, so it makes 2.3.
> 
> I think it can go via -trivial, if Luiz doesn't mind.

Applied to -trivial for now, thank you!

/mjt

Re: [Qemu-devel] [PATCH] block/qapi: Fix Sparse warning

[Michael Tokarev]

08.03.2015 01:16, Stefan Weil wrote:
> Sparse reports this warning:
> 
> block/qapi.c:417:47: warning:
>  too long initializer-string for array of char(no space for nul char)
> 
> Replacing the string by an array of characters fixes this warning.

> -static const char suffixes[NB_SUFFIXES] = "KMGT";
> +static const char suffixes[NB_SUFFIXES] = {'K', 'M', 'G', 'T'};

The latter is a bit uglier to my taste but oh well, damn warnings! :)

Applied to -trivial, thanks!

/mjt

Re: [Qemu-devel] [PATCH v2] error: Replace error_report() & error_free() with error_report_err()

[zhanghailiang]

Hi Michael,

This patch has been reviewed, Please consider for merging.

Thanks,
zhanghailiang

On 2015/3/5 0:50, Markus Armbruster wrote:

zhanghailiang  writes:


This is a continuation of the work started in commit 565f65d27:
"error: Use error_report_err() where appropriate"

Signed-off-by: zhanghailiang 
Reviewed-by: Eric Blake 


Reviewed-by: Markus Armbruster 

Re: [Qemu-devel] [PATCH 1/7] backends: Fix warning from Sparse

[Michael Tokarev]

Applied all 7 to -trivial.

Is it intentional that there's no cover letter for the series?

Thanks,

/mjt

Re: [Qemu-devel] [PATCH v5 10/45] Return path: Control commands

[David Gibson]

On Wed, Feb 25, 2015 at 04:51:33PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> Add two src->dest commands:
>* OPEN_RETURN_PATH - To request that the destination open the return path
>* SEND_PING - Request an acknowledge from the destination
> 
> Signed-off-by: Dr. David Alan Gilbert 

Reviewed-by: David Gibson 

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


pgp1eMuWqu_dk.pgp
Description: PGP signature

Re: [Qemu-devel] [PATCH v5 09/45] Migration commands

[David Gibson]

On Wed, Feb 25, 2015 at 04:51:32PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> Create QEMU_VM_COMMAND section type for sending commands from
> source to destination.  These commands are not intended to convey
> guest state but to control the migration process.
> 
> For use in postcopy.
> 
> Signed-off-by: Dr. David Alan Gilbert 

[snip]
> +/* Send a 'QEMU_VM_COMMAND' type element with the command
> + * and associated data.
> + */
> +void qemu_savevm_command_send(QEMUFile *f,
> +  enum qemu_vm_cmd command,
> +  uint16_t len,
> +  uint8_t *data)
> +{
> +uint32_t tmp = (uint16_t)command;

Erm.. cast to u16, assign to u32, then send as u16?  What's up with
that?

> +qemu_put_byte(f, QEMU_VM_COMMAND);
> +qemu_put_be16(f, tmp);
> +qemu_put_be16(f, len);
> +if (len) {
> +qemu_put_buffer(f, data, len);
> +}
> +qemu_fflush(f);
> +}
> +

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


pgpOuOZAvx5I7.pgp
Description: PGP signature

Re: [Qemu-devel] [PATCH v5 11/45] Return path: Send responses from destination to source

[David Gibson]

On Wed, Feb 25, 2015 at 04:51:34PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> Add migrate_send_rp_message to send a message from destination to source 
> along the return path.
>   (It uses a mutex to let it be called from multiple threads)
> Add migrate_send_rp_shut to send a 'shut' message to indicate
>   the destination is finished with the RP.
> Add migrate_send_rp_ack to send a 'PONG' message in response to a PING
>   Use it in the CMD_PING handler
> 
> Signed-off-by: Dr. David Alan Gilbert 
> ---
>  include/migration/migration.h | 17 
>  migration/migration.c | 45 
> +++
>  savevm.c  |  2 +-
>  trace-events  |  1 +
>  4 files changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index c514dd4..6775747 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -41,6 +41,13 @@ struct MigrationParams {
>  bool shared;
>  };
>  
> +/* Commands sent on the return path from destination to source*/
> +enum mig_rpcomm_cmd {

"command" doesn't seem like quite the right description for these rp
messages.

> +MIG_RP_CMD_INVALID = 0,  /* Must be 0 */
> +MIG_RP_CMD_SHUT, /* sibling will not send any more RP messages */
> +MIG_RP_CMD_PONG, /* Response to a PING; data (seq: be32 ) */
> +};
> +
>  typedef struct MigrationState MigrationState;
>  
>  /* State for the incoming migration */
> @@ -48,6 +55,7 @@ struct MigrationIncomingState {
>  QEMUFile *file;
>  
>  QEMUFile *return_path;
> +QemuMutex  rp_mutex;/* We send replies from multiple threads */
>  };
>  
>  MigrationIncomingState *migration_incoming_get_current(void);
> @@ -169,6 +177,15 @@ int64_t migrate_xbzrle_cache_size(void);
>  
>  int64_t xbzrle_cache_resize(int64_t new_size);
>  
> +/* Sending on the return path - generic and then for each message type */
> +void migrate_send_rp_message(MigrationIncomingState *mis,
> + enum mig_rpcomm_cmd cmd,
> + uint16_t len, uint8_t *data);
> +void migrate_send_rp_shut(MigrationIncomingState *mis,
> +  uint32_t value);
> +void migrate_send_rp_pong(MigrationIncomingState *mis,
> +  uint32_t value);
> +
>  void ram_control_before_iterate(QEMUFile *f, uint64_t flags);
>  void ram_control_after_iterate(QEMUFile *f, uint64_t flags);
>  void ram_control_load_hook(QEMUFile *f, uint64_t flags);
> diff --git a/migration/migration.c b/migration/migration.c
> index a36ea65..80d234c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -78,6 +78,7 @@ MigrationIncomingState 
> *migration_incoming_state_new(QEMUFile* f)
>  {
>  mis_current = g_malloc0(sizeof(MigrationIncomingState));
>  mis_current->file = f;
> +qemu_mutex_init(&mis_current->rp_mutex);
>  
>  return mis_current;
>  }
> @@ -88,6 +89,50 @@ void migration_incoming_state_destroy(void)
>  mis_current = NULL;
>  }
>  
> +/*
> + * Send a message on the return channel back to the source
> + * of the migration.
> + */
> +void migrate_send_rp_message(MigrationIncomingState *mis,
> + enum mig_rpcomm_cmd cmd,
> + uint16_t len, uint8_t *data)

Using (void *) for data would avoid casts in a bunch of the callers.

> +{
> +trace_migrate_send_rp_message((int)cmd, len);
> +qemu_mutex_lock(&mis->rp_mutex);
> +qemu_put_be16(mis->return_path, (unsigned int)cmd);
> +qemu_put_be16(mis->return_path, len);
> +qemu_put_buffer(mis->return_path, data, len);
> +qemu_fflush(mis->return_path);
> +qemu_mutex_unlock(&mis->rp_mutex);
> +}
> +
> +/*
> + * Send a 'SHUT' message on the return channel with the given value
> + * to indicate that we've finished with the RP.  None-0 value indicates
> + * error.
> + */
> +void migrate_send_rp_shut(MigrationIncomingState *mis,
> +  uint32_t value)
> +{
> +uint32_t buf;
> +
> +buf = cpu_to_be32(value);
> +migrate_send_rp_message(mis, MIG_RP_CMD_SHUT, 4, (uint8_t *)&buf);

 ^ sizeof(buf)
 would be safer

> +}
> +
> +/*
> + * Send a 'PONG' message on the return channel with the given value
> + * (normally in response to a 'PING')
> + */
> +void migrate_send_rp_pong(MigrationIncomingState *mis,
> +  uint32_t value)
> +{
> +uint32_t buf;
> +
> +buf = cpu_to_be32(value);
> +migrate_send_rp_message(mis, MIG_RP_CMD_PONG, 4, (uint8_t *)&buf);

It occurs to me that you could define PONG as returning the whole
buffer that PING sends, instead of just 4-bytes.  Might allow for some
more testing of variable sized messages.

> +}
> +
>  void qemu_start_incoming_migration(const char *uri, Er

[Qemu-devel] [PATCH] 9pfs-proxy: tiny cleanups in proxy_pwritev and proxy_preadv

[Michael Tokarev]

Don't compare syscall return with -1, use "<0" condition.
Don't introduce useless local variables when we already
have similar variable
Rename local variable to be consistent with other usages

Signed-off-by: Michael Tokarev 
---
 hw/9pfs/virtio-9p-proxy.c | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
index 59c7445..a01804a 100644
--- a/hw/9pfs/virtio-9p-proxy.c
+++ b/hw/9pfs/virtio-9p-proxy.c
@@ -696,9 +696,9 @@ static ssize_t proxy_preadv(FsContext *ctx, 
V9fsFidOpenState *fs,
 #ifdef CONFIG_PREADV
 return preadv(fs->fd, iov, iovcnt, offset);
 #else
-int err = lseek(fs->fd, offset, SEEK_SET);
-if (err == -1) {
-return err;
+int ret = lseek(fs->fd, offset, SEEK_SET);
+if (err < 0)
+return ret;
 } else {
 return readv(fs->fd, iov, iovcnt);
 }
@@ -714,10 +714,8 @@ static ssize_t proxy_pwritev(FsContext *ctx, 
V9fsFidOpenState *fs,
 #ifdef CONFIG_PREADV
 ret = pwritev(fs->fd, iov, iovcnt, offset);
 #else
-int err = lseek(fs->fd, offset, SEEK_SET);
-if (err == -1) {
-return err;
-} else {
+ret = lseek(fs->fd, offset, SEEK_SET);
+if (ret >= 0) {
 ret = writev(fs->fd, iov, iovcnt);
 }
 #endif
-- 
2.1.4

Re: [Qemu-devel] [PULL v2 00/46] Trivial patches for 2015-03-04

[Michael Tokarev]

08.03.2015 15:43, Peter Maydell wrote:
[..]
> Fails to build on clang, I'm afraid:
> 
> /home/petmay01/linaro/qemu-for-merges/disas/cris.c:1218:33: error:
> unused variable 'cris_cond15s' [-Werror,-Wunused-const-variable]
> static const struct cris_cond15 cris_cond15s[] =
> ^
> 1 error generated.

What a wonderful patchset I have in -trivial.  It
had 2 follow-ups each dropping one more patch.  Now
there's one another bad patch, and more, a bad patch
which has already been submitted previously and rejected
because of the same very reason.

I just re-sent the pull request without the patch in question.

I'm sorry for the noize.

Thanks,

/mjt

Re: [Qemu-devel] Fix for incorrect SYSRET instruction implementation -- anyone looked at this yet?

[Stefan Weil]

Please see my comment below.

Am 09.03.2015 um 23:46 schrieb Bill Paul:

Of all the gin joints in all the towns in all the world, Stefan Weil had to
walk into mine at 15:23:36 on Monday 09 March 2015 and say:


Hi Bill,

sending text only e-mails might help. Usually git send-email is better
than using KMail or other mail software.

You know, I thought I was sending in plain text. Somewhere along the line
KMail lied to me.


Even when using plain text, mailing programs will often wrap lines and 
or do other nasty things.
Create your patch with "git format-patch", add optional personal 
comments after the ---,
use scripts/get_maintainer.pl and scripts/checkpatch.pl on your patch, 
and then send it

with "git send-email".




Use tools like git gui to get a correct signature. Any optional personal
comments should come directly after
the line with ---.

After? Or before?



See http://patchwork.ozlabs.org/patch/447644/for an example.



Could you please re-send your patch? Check it before
sending with scripts/checkpatch.pl.

Maybe this looks like much regulations to fix a bug, but some rules are
necessary to keep a project
like QEMU alive.

I'm going to send it once more because it was entirely my fault that my stupid
mailer kept sending in HTML mode, but that's it.

-Bill


Cheers
Stefan

[Qemu-devel] Ask about patching our MIPS work to Qemu main branch

[魏振伟]

Hi, there,


We are software engineers of Loongson.Inc，a computer company design 
and produce MIPS-compatible computers located in China。


Currently we use Qemu to run X86-64 network programs such as snort on MIPS 
platform.
During Qemu running on MIPS, we found some Qemu bugs such as pagesize,
memory access, multi-thread, etc.  And now we have fixed some of them.


We wonder if these bug patch could port to Qeme official main branch, 
so we write to ask your advice.


Best Regards~


Weizhenwei
2015.03.10

[Qemu-devel] Using tap device for qemu

[Gayathri Nagarajan]

I have a problem with networking in qemu. I want to use the Linux tap
device for qemu network. For that

*1. I created a bridge -*

$ sudo brctl addbr br0

*2. I used the following qemu-ifup script*

#!/bin/sh
set -x

switch=br0

if [ -n "$1" ];then
/usr/bin/sudo /usr/sbin/tunctl -u `whoami` -t $1
/usr/bin/sudo /sbin/ip link set $1 up
sleep 0.5s
/usr/bin/sudo /usr/sbin/brctl addif $switch $1
exit 0
else
echo "Error: no interface specified"
exit 1
fi

*3. When I run qemu using*

$ qemu-system-i386 -m 200M -cdrom images/ss.iso -serial stdio -device
e1000,netdev=net0,mac=DE:AD:BE:EF:00:C0 -netdev
tap,id=net0,script=/home/user/qemu-ifup

*/home/user/qemu-ifup:could not launch network script*

*qemu-system-i386: -netdev tap,id=net0,script=/home/user/qemu-ifup:
Device 'tap' could not be initialized*


How do I resolve this problem?

Thank you

gayathri

[Qemu-devel] [PATCH] cris: remove unused cris_cond15 declarations

[Michael Tokarev]

Signed-off-by: Michael Tokarev 
---
 disas/cris.c  | 11 ---
 target-cris/opcode-cris.h | 10 --
 2 files changed, 21 deletions(-)

diff --git a/disas/cris.c b/disas/cris.c
index 9dfb4e3..0fc7e54 100644
--- a/disas/cris.c
+++ b/disas/cris.c
@@ -1214,17 +1214,6 @@ cris_cc_strings[] =
   "wf"
 };
 
-/* Different names and semantics for condition  (0xf).  */
-const struct cris_cond15 cris_cond15s[] =
-{
-  /* FIXME: In what version did condition "ext" disappear?  */
-  {"ext", cris_ver_v0_3},
-  {"wf", cris_ver_v10},
-  {"sb", cris_ver_v32p},
-  {NULL, 0}
-};
-
-
 /*
  * Local variables:
  * eval: (c-set-style "gnu")
diff --git a/target-cris/opcode-cris.h b/target-cris/opcode-cris.h
index 779d4aa..e7ebb98 100644
--- a/target-cris/opcode-cris.h
+++ b/target-cris/opcode-cris.h
@@ -108,16 +108,6 @@ struct cris_support_reg
 };
 extern const struct cris_support_reg cris_support_regs[];
 
-struct cris_cond15
-{
-  /* The name of the condition.  */
-  const char *const name;
-
-  /* What CPU version this condition name applies to.  */
-  enum cris_insn_version_usage applicable_version;
-};
-extern const struct cris_cond15 cris_conds15[];
-
 /* Opcode-dependent constants.  */
 #define AUTOINCR_BIT (0x04)
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH 2/3] ppc64-softmmu: Remove unsupported FDC from config

[Alexander Graf]

> Am 09.03.2015 um 23:44 schrieb Alexey Kardashevskiy :
> 
>> On 03/10/2015 02:58 PM, David Gibson wrote:
>>> On Tue, Mar 10, 2015 at 02:52:48PM +1100, Alexey Kardashevskiy wrote:
 On 03/10/2015 01:39 AM, Alexey Kardashevskiy wrote:
> On 03/09/2015 11:31 PM, Alexander Graf wrote:
> 
> 
>> On 02.03.15 00:46, Alexey Kardashevskiy wrote:
>> This removes floppy disks support as it is not supported by any PPC64
>> system anyway as the only way to have floppy disk on such systems would
>> be an ISA bus and Linux kernels seems have never had such support.
>> 
>> Signed-off-by: Alexey Kardashevskiy 
> 
> I removed this patch from my queue again. The ppc64-softmmu target can
> execute -M PReP which in turn uses the fdc.
 
 
 Out of curiosity - do you have actual 64bit guests being able to run on
 PReP? :) Current Linux ditched its support...
>>> 
>>> 
>>> Paul suggested that there has never ever been a 64bit PReP CPU so there is
>>> no point in emulating it in QEMU. Or there is some reason for that?
>> 
>> IIUC, qemu-system-ppc64 (roughly speaking) emulates a superset of what
>> qemu-system-ppc does, not a different set of hardware.
> 
> Well, default-configs/ppc-softmmu.mak does not include 
> default-configs/ppc64-softmmu.mak or vice versa so I would say these are 
> pretty independent and I would simply remove CONFIG_PREP* from 
> default-configs/ppc64-softmmu.mak.

Convention so far has been that ppc64 includes ppc includes ppcemb. I don't see 
why we should break that assumption.

However I do agree that we should probably reflect it with includes in the mak 
files.


Alex

> 
> 
> -- 
> Alexey

[Qemu-devel] [PULL v4 00/44] Trivial patches for 2015-03-04

[Michael Tokarev]

This is a v4 (!) pull request for trivial-patches tree which was initially sent
Wed,  4 Mar 2015 20:06:17 +0300.  The following patches has been dropped:

 - migration/rdma: clean up qemu_rdma_dest_init a bit --
   will go to the migration tree

 - microblaze: fix memory leak --
   wrong change, causes compile failure, initially escaped my test coverage

 - disas/cris: Fix warning caused by missing 'static' attribute --
   this makes an unused static variable as static, causing compile warning.
   To my shame, the same patch has been routed and dropped in trivial tree
   before.  Now it is dropped again, I'll hopefully fix the damn thing for
   good in the next pull request.

Please consider pulling.  And please note I used the same tag name and
pull request date as before.

I'm sorry for the noize.

Thanks,

/mjt

The following changes since commit 277263e1b320d759a760ba6c5ea75ec268f929e5:

  Merge remote-tracking branch 'remotes/agraf/tags/signed-ppc-for-upstream' 
into staging (2015-03-09 14:04:14 +)

are available in the git repository at:

  git://git.corpit.ru/qemu.git tags/pull-trivial-patches-2015-03-04

for you to fetch changes up to 438940cbc2eabbe9e403e5249dfa0be6c792c93b:

  9pfs: remove useless return (2015-03-10 08:15:34 +0300)


trivial patches for 2015-03-04


Alberto Garcia (2):
  qerror.h: Swap definitions that were not in alphabetical order
  qmp-commands.hx: Fix several typos

Borislav Petkov (1):
  memsave: Improve and disambiguate error message

Cole Robinson (3):
  qapi-schema: Fix SpiceChannel docs
  gitignore: Track common.env in iotests gitignore
  gitignore: Ignore new tests

Gabriel L. Somlo (1):
  smbios: document cmdline options for smbios type 2-4, 17 structures

Gonglei (18):
  xen-pt: fix Negative array index read
  xen-pt: fix Out-of-bounds read
  block: remove superfluous '\n' around error_report/error_setg
  a9gtimer: remove superfluous '\n' around error_setg
  pl330.c: remove superfluous '\n' around error_setg
  numa: remove superfluous '\n' around error_setg
  Remove superfluous '\n' around error_report()
  vhost-scsi: Remove superfluous '\n' around error_report()
  vfio: Remove superfluous '\n' around error_report()
  xtensa: Remove superfluous '\n' around error_report()
  tpm: Remove superfluous '\n' around error_report()
  arm/digic_boards: Remove superfluous '\n' around error_report()
  vhost: Remove superfluous '\n' around error_report()
  nbd: fix resource leak
  sparc/leon3.c: fix memory leak
  macio: fix possible memory leak
  milkymist.c: fix memory leak
  sysbus: fix memory leak

Markus Armbruster (1):
  xilinx_ethlite: Clean up after commit 2f991ad

Michael Tokarev (3):
  qemu-options: fix/document -incoming options
  e500: fix memory leak
  9pfs: remove useless return

Paolo Bonzini (2):
  cutils: refine strtol error handling in parse_debug_env
  gdbstub: avoid possible NULL pointer dereference

Radim Krčmář (2):
  fix GCC 5.0.0 logical-not-parentheses warnings
  milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning

Stefan Berger (1):
  Add copyright and author after file split

Stefan Weil (5):
  vhost_net: Add missing 'static' attribute
  disas/arm: Fix warnings caused by missing 'static' attribute
  disas/microblaze: Fix warnings caused by missing 'static' attribute
  oslib-posix: Fix compiler warning (-Wclobbered) and simplify the code
  migration: Fix coding style (whitespace issues)

Thomas Huth (3):
  ui: Removed unused functions
  ui/vnc: Remove vnc_stop_worker_thread()
  xen: Remove xen_cmos_set_s3_resume()

Wang Xin (2):
  qemu-char: add cyrillic characters 'numerosign' to VNC keysyms
  qemu-char: add cyrillic key 'numerosign' to Russian keymap

 .gitignore|   1 -
 block/archipelago.c   |   6 +-
 block/nbd.c   |   1 +
 cpus.c|   4 +-
 disas/arm.c   | 128 --
 disas/microblaze.c|  13 +++--
 exec.c|   2 +-
 gdbstub.c |   8 ++-
 hw/9pfs/virtio-9p-local.c |   1 -
 hw/arm/digic_boards.c |   6 +-
 hw/block/nand.c   |   2 +-
 hw/core/sysbus.c  |   2 +
 hw/dma/pl330.c|   4 +-
 hw/ide/pci.c  |   2 +-
 hw/lm32/milkymist.c   |   1 +
 hw/microblaze/boot.c  |   2 +-
 hw/misc/macio/macio.c |   3 +-
 hw/misc/milkymist-pfpu.c  |   2 +-
 hw/net/vhost_net.c|   2 +-
 hw/net/virtio-net.c   |   4 +-
 hw/net/xilinx_ethlite.c   |   1 +
 hw/ppc/e500.c |   1 +
 hw/scsi/vhost-scsi.c  |   6 +-
 hw/sparc/leon3.c  |   1 +
 hw/time

Re: [Qemu-devel] [PATCH] disas/cris: Fix warning caused by missing 'static' attribute

[Michael Tokarev]

01.03.2015 16:07, Stefan Weil wrote:
> Warning from the Sparse static analysis tool:
> 
> disas/cris.c:1218:26: warning:
>  symbol 'cris_cond15s' was not declared. Should it be static?
> 
> Signed-off-by: Stefan Weil 
> ---
>  disas/cris.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/disas/cris.c b/disas/cris.c
> index 9dfb4e3..a034dba 100644
> --- a/disas/cris.c
> +++ b/disas/cris.c
> @@ -1215,7 +1215,7 @@ cris_cc_strings[] =
>  };
>  
>  /* Different names and semantics for condition  (0xf).  */
> -const struct cris_cond15 cris_cond15s[] =
> +static const struct cris_cond15 cris_cond15s[] =

This is the same patch you sent 07-feb, with Message-Id:
<1423258997-30957-2-git-send-email...@weilnetz.de>, and
the same patch which makes at least clang unhappy:

 disas/cris.c:1218:33: error:
 unused variable 'cris_cond15s' [-Werror,-Wunused-const-variable]
 static const struct cris_cond15 cris_cond15s[] =
 ^
 1 error generated.

I'll drop this patch and remove the unused variable
instead.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH 1/5] 9pfs-proxy: simplify v9fs_request(), P1

[Michael Tokarev]

10.03.2015 07:19, Michael Tokarev wrote:
> 08.03.2015 19:39, Aneesh Kumar K.V wrote:
>> Michael Tokarev  writes:
>>
>>> This simplifies code in v9fs_request() a bit by replacing several
>>> ifs with a common variable check and rearranging error/cleanup
>>> code a bit.
>>
>> Is this -V2 of
>> http://mid.gmane.org/b98f675750ef0535cab41225240db1657fc2fe00.1425678142.git@msgid.tls.msk.ru
> 
> No, this one (simplify v9fs_reqeust(), P1) was a first version.
> The above URL points to the v2, a second version.

Actually the url points to v3, not v2, of the patchset.  Yet it
is still the latest ;)

Thanks,

/mjt

Re: [Qemu-devel] [PATCH v3 0/3] RFC: 9pfs-proxy: simplify/cleanup

[Michael Tokarev]

Now when I reviewed the whole thing, I'd drop the error
handling change too, and fold it into even bigger change.
What I'm thinking is to refactor this code to look
significantly different, namely:

For the proxy code:

 o each filesystem method first call proxy_send_request(type, fmt, ...),
   and proxy_send_request() locks the proxy object, marshals the
   arguments according to fmt and sends the request out.

 o after sending the request, each method calls
   proxy_receive_reply(fmt, ...) (or proxy_receive_fd() -- there's
   no need in proxy_receive_status(), it is proxy_recive_reply()
   with fmt=NULL).  This proxy_receive_reply() receives the
   reply according to the fmt argument and unlocks the
   proxy object.

This way. locking code will be split into two methods, but
_all_ filesystem-method-specific code, for each method,
will be in the same function which is the only place which
"knows" everything about the method.

For proxy code it might also be a good idea to have two
v9fs_string objects embedded in the proxy structure, to
be used by the methods, so there's no need to init and
free the local-to-function strings in every method.

At the general level, v9fs_string handling is bad, it
shouldn't really free+alloc a string every time something
gets printed into it.  v9fs_path type should be dropped
completely and replaced with v9fs_string, especially since
one is often being type-cast to another.

marshal/unmarshal interface should be printf-like with %s,
so that the compiler will be able to check if the arguments
are of the right type.  Alternatively, it should be re-
factored to accept just one, typed, argument to pack/unpack,
without vararg part.  Here, an iovec iterator might be used
(to keep current position in iovec) to speed things up.

marshal/unmarshal interface should not allocate/free strings
frantically like it does now -- this allocation/freeing takes
significant time and slows down 9pfs code.  The same is true
for some other parts of 9pfs too.

Also marshal/unmarshal interface - is it really necessary to
support I/O where individual eiements (a file name, size of
that file name, or even all the fields of some guest-visible
structure) are split between several iovec elements?  Can't
we say that a single element (with some definition of "element",
which can be a single string or this string together with its
size in front, one element of a structure like stat or whole
stat structure, etc) must not be split into multiple iovecs?
If it's possible, this code can be simplified and sped up
greatly, for example, we won't need to copy the strings to
a temp buffer (especially multiple times!) and can just point
inside of a single iovec...

That's just some random thoughts.  All without acually understanding
how 9pfs communicates with the guest...  Can you describe this part
briefly please?  Or maybe I should just shut up and pretend I never
seen this code.. ;)  (which is, actually, a good possibility - I don't
really have much time to deal with it, and 9pfs is not my big interest... ;)

Thanks,

/mjt

07.03.2015 00:43, Michael Tokarev wrote:
> Try to make the code a bit less ugly.  First by moving
> errno = -retval to a common place, and second by simplifying
> common place a lot.  What's left is v9fs_receive_response().
> 
> V3: merge several small patches into larger ones,
> drop trivial stuff
> 
> Michael Tokarev (3):
>   9pfs-proxy: simplify error handling
>   fsdev: introduce v9fs_vmarshal() and v9fs_vunmarshal()
>   9pfs-proxy: remove one half of redundrand code
> 
>  fsdev/virtio-9p-marshal.c |  38 +++--
>  fsdev/virtio-9p-marshal.h |   6 +
>  hw/9pfs/virtio-9p-proxy.c | 405 
> +-
>  3 files changed, 77 insertions(+), 372 deletions(-)
> 

Re: [Qemu-devel] [PATCH 2/3] ppc64-softmmu: Remove unsupported FDC from config

[Alexey Kardashevskiy]

On 03/10/2015 02:58 PM, David Gibson wrote:

On Tue, Mar 10, 2015 at 02:52:48PM +1100, Alexey Kardashevskiy wrote:

On 03/10/2015 01:39 AM, Alexey Kardashevskiy wrote:

On 03/09/2015 11:31 PM, Alexander Graf wrote:



On 02.03.15 00:46, Alexey Kardashevskiy wrote:

This removes floppy disks support as it is not supported by any PPC64
system anyway as the only way to have floppy disk on such systems would
be an ISA bus and Linux kernels seems have never had such support.

Signed-off-by: Alexey Kardashevskiy 


I removed this patch from my queue again. The ppc64-softmmu target can
execute -M PReP which in turn uses the fdc.



Out of curiosity - do you have actual 64bit guests being able to run on
PReP? :) Current Linux ditched its support...



Paul suggested that there has never ever been a 64bit PReP CPU so there is
no point in emulating it in QEMU. Or there is some reason for that?


IIUC, qemu-system-ppc64 (roughly speaking) emulates a superset of what
qemu-system-ppc does, not a different set of hardware.


Well, default-configs/ppc-softmmu.mak does not include 
default-configs/ppc64-softmmu.mak or vice versa so I would say these are 
pretty independent and I would simply remove CONFIG_PREP* from 
default-configs/ppc64-softmmu.mak.



--
Alexey

Re: [Qemu-devel] [PATCH 1/3] 9pfs-proxy: simplify error handling

[Michael Tokarev]

08.03.2015 19:27, Aneesh Kumar K.V wrote:
> Michael Tokarev  writes:
[]
>> Actually, after reading almost whole 9pfs and fsdev code, I can
>> say with great confidence this code is nearly hopeless.
> 
> Is that about the 9pfs-proxy code, or the rest of 9pfs. I understand

Well. the marshal/unmarshal interface is in core code as far as
I can see, and it is very fragile at best, as the below example of
its usage shows.  I haven't dug deeper.  So far, it was only the
9pfs proxy code.

> that the error handling can definitely get some cleanup. I mentioned
> that in my previous mail
> mail. http://mid.gmane.org/87oav7iy5v@linux.vnet.ibm.com

I've shown probs in the code itself, not the visible behavour.
Visible behavour is much easier to fix here.

Thanks,

/mjt

>> Patch 3 shows just one (huge) example.  There are so many issues
>> with this code, I'm afraid I don't have know the words to express
>> it.
>>
>> Again, patch 3 shows a good example.  Another example:
>>
>> static int v9fs_receive_status(V9fsProxy *proxy,
>>struct iovec *reply, int *status)
>> ...
>> proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
>> if (header.size != sizeof(int)) {
>> *status = -ENOBUFS;
>> }
>> ...
>> proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status);
>>
>> proxy_unmarshall(), for "d" element, expects an int32_t
>> pointer.  Here we have int pointer, and compare its
>> size with sizeof(int).  This is a generic problem of whole
>> v9fs_(un)marshall interface, which is in the core of 9pfs...
>> this is a status return, which is int32.
>>
>> Oh well.  I've no idea how this code has been accepted.
>> It is absolute crap.
>>
> 
> -aneesh
> 

Re: [Qemu-devel] [PATCH 1/5] 9pfs-proxy: simplify v9fs_request(), P1

[Michael Tokarev]

08.03.2015 19:39, Aneesh Kumar K.V wrote:
> Michael Tokarev  writes:
> 
>> This simplifies code in v9fs_request() a bit by replacing several
>> ifs with a common variable check and rearranging error/cleanup
>> code a bit.
> 
> Is this -V2 of
> http://mid.gmane.org/b98f675750ef0535cab41225240db1657fc2fe00.1425678142.git@msgid.tls.msk.ru

No, this one (simplify v9fs_reqeust(), P1) was a first version.
The above URL points to the v2, a second version.

> I am slightly confused with the patch series. It does split the patch as
> I wanted, but i am not sure which one is the latest, so that i can start
> applying them.

That URL points to last.  I merged several small patches into larger ones.

Initially I thought I just plug a small resource leak so the patch will
be small.  But the more I understood the code, the bigger the changes
were becoming.  So at some point it become unreasonable to keep small
changes since they're related to bigger changes and since bigger changes
touches the same code anyway.

Thanks,

/mjt

[Qemu-devel] qemu-system return value when terminating due to a signal

[Michael Tokarev]

When qemu is terminated by a signal, such as SIGINT (Ctrl+C),
it exits with a zero, successful status.  Is it intentional?

Sure, from the qemu perspective, pulling the plug from the
guest machine is a success.  But maybe it's better to indicate
that the exit was due to interrupt, not because of `exit'
command which is a much better version of pulling the plug?

If yes, what exit code should it return?

Thanks,

/mjt

Re: [Qemu-devel] [PATCH 2/3] ppc64-softmmu: Remove unsupported FDC from config

[David Gibson]

On Tue, Mar 10, 2015 at 02:52:48PM +1100, Alexey Kardashevskiy wrote:
> On 03/10/2015 01:39 AM, Alexey Kardashevskiy wrote:
> >On 03/09/2015 11:31 PM, Alexander Graf wrote:
> >>
> >>
> >>On 02.03.15 00:46, Alexey Kardashevskiy wrote:
> >>>This removes floppy disks support as it is not supported by any PPC64
> >>>system anyway as the only way to have floppy disk on such systems would
> >>>be an ISA bus and Linux kernels seems have never had such support.
> >>>
> >>>Signed-off-by: Alexey Kardashevskiy 
> >>
> >>I removed this patch from my queue again. The ppc64-softmmu target can
> >>execute -M PReP which in turn uses the fdc.
> >
> >
> >Out of curiosity - do you have actual 64bit guests being able to run on
> >PReP? :) Current Linux ditched its support...
> 
> 
> Paul suggested that there has never ever been a 64bit PReP CPU so there is
> no point in emulating it in QEMU. Or there is some reason for that?

IIUC, qemu-system-ppc64 (roughly speaking) emulates a superset of what
qemu-system-ppc does, not a different set of hardware.

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


pgpjTOrIPojIg.pgp
Description: PGP signature

Re: [Qemu-devel] [PATCH 2/3] ppc64-softmmu: Remove unsupported FDC from config

[Alexey Kardashevskiy]

On 03/10/2015 01:39 AM, Alexey Kardashevskiy wrote:

On 03/09/2015 11:31 PM, Alexander Graf wrote:



On 02.03.15 00:46, Alexey Kardashevskiy wrote:

This removes floppy disks support as it is not supported by any PPC64
system anyway as the only way to have floppy disk on such systems would
be an ISA bus and Linux kernels seems have never had such support.

Signed-off-by: Alexey Kardashevskiy 


I removed this patch from my queue again. The ppc64-softmmu target can
execute -M PReP which in turn uses the fdc.



Out of curiosity - do you have actual 64bit guests being able to run on
PReP? :) Current Linux ditched its support...



Paul suggested that there has never ever been a 64bit PReP CPU so there is 
no point in emulating it in QEMU. Or there is some reason for that?




--
Alexey

Re: [Qemu-devel] [PATCH 00/10] spapr: Small patches to prepare for Dynamic DMA windows

[Alexey Kardashevskiy]

On 02/23/2015 07:33 PM, Alexey Kardashevskiy wrote:

These I have in my DDW working tree for quite a while, while I am polishing
others, there could go to some tree already.

Please comment. Thanks!


Alex, ping.

"spapr_pci: Make find_phb()/find_dev() public" won't apply after Gavin's 
EEH patches but the others would, should I repost them together with DDW 
patches or separately or any other suggestion about these? Thanks.






Alexey Kardashevskiy (10):
   spapr_iommu: Disable in-kernel IOMMU tables for >4GB windows
   spapr_iommu: Make H_PUT_TCE_INDIRECT endian-safe
   spapr_pci: Introduce a liobn number generating macros
   spapr_vio: Introduce a liobn number generating macros
   spapr_pci: Define default DMA window size as a macro
   spapr_iommu: Add separate trace points for PCI DMA operations
   spapr_pci: Make find_phb()/find_dev() public
   spapr_iommu: Make spapr_tce_find_by_liobn() public
   spapr_pci: Rework device-tree rendering
   spapr_iommu: Give unique QOM name to TCE table

  hw/ppc/spapr_iommu.c| 44 +
  hw/ppc/spapr_pci.c  | 60 +++--
  hw/ppc/spapr_vio.c  |  2 +-
  include/hw/pci-host/spapr.h |  6 +
  include/hw/ppc/spapr.h  |  7 +-
  trace-events|  4 +++
  6 files changed, 66 insertions(+), 57 deletions(-)




--
Alexey

Re: [Qemu-devel] [RFC] introduce bitmap to bdrv_commit totrackdirtysector

[Zhang Haoyu]

On 2015-03-10 10:58:44, Fam Zheng wrote:
> > Is it worthy to implement it?
> 
> Maybe, I think once we start to assign node-name to each BDS in the tree, this
> will be natural to implement.
> 
Once implemented, to be honest, I really don't see much advantages of 
incremental backup over aforementioned mechanism(external snapshot + active 
block commit),
especially, regarding the case of source vm non-normal shutdown, full copy is 
needed,
and corresponding check mechanism is needed too.
I think so much exceptional scenarios are needed to be considered for 
incremental backup mechanism.
Maybe I missed something.

Thanks,
Zhang Haoyu
> > And does qemu support commit any external snapshot to its backing file?
> 
> Yes.

Re: [Qemu-devel] [PATCH v5 08/45] Return path: socket_writev_buffer: Block even on non-blocking fd's

[David Gibson]

On Wed, Feb 25, 2015 at 04:51:31PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> The return path uses a non-blocking fd so as not to block waiting
> for the (possibly broken) destination to finish returning a message,
> however we still want outbound data to behave in the same way and block.

It's not clear to me from this description exactly where the situation
is that you need to write to the non-blocking socket.  Is it on the
source or the destination?  If the source, why are you writing to the
return path?  If the destination, why are you marking the outgoing
return path as non-blocking?

> Signed-off-by: Dr. David Alan Gilbert 
> ---
>  migration/qemu-file-unix.c | 41 -
>  1 file changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/qemu-file-unix.c b/migration/qemu-file-unix.c
> index 50291cf..218dbd0 100644
> --- a/migration/qemu-file-unix.c
> +++ b/migration/qemu-file-unix.c
> @@ -39,12 +39,43 @@ static ssize_t socket_writev_buffer(void *opaque, struct 
> iovec *iov, int iovcnt,
>  QEMUFileSocket *s = opaque;
>  ssize_t len;
>  ssize_t size = iov_size(iov, iovcnt);
> +ssize_t offset = 0;
> +int err;
>  
> -len = iov_send(s->fd, iov, iovcnt, 0, size);
> -if (len < size) {
> -len = -socket_error();
> -}
> -return len;
> +while (size > 0) {
> +len = iov_send(s->fd, iov, iovcnt, offset, size);
> +
> +if (len > 0) {
> +size -= len;
> +offset += len;
> +}
> +
> +if (size > 0) {
> +err = socket_error();
> +
> +if (err != EAGAIN) {
> +error_report("socket_writev_buffer: Got err=%d for 
> (%zd/%zd)",
> + err, size, len);
> +/*
> + * If I've already sent some but only just got the error, I
> + * could return the amount validly sent so far and wait for 
> the
> + * next call to report the error, but I'd rather flag the 
> error
> + * immediately.
> + */
> +return -err;
> +}
> +
> +/* Emulate blocking */
> +GPollFD pfd;
> +
> +pfd.fd = s->fd;
> +pfd.events = G_IO_OUT | G_IO_ERR;
> +pfd.revents = 0;
> +g_poll(&pfd, 1 /* 1 fd */, -1 /* no timeout */);
> +}
> + }
> +
> +return offset;
>  }
>  
>  static int socket_get_fd(void *opaque)

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


pgputg2g8RRxR.pgp
Description: PGP signature

Re: [Qemu-devel] [PATCH v5 07/45] Return path: Open a return path on QEMUFile for sockets

[David Gibson]

On Wed, Feb 25, 2015 at 04:51:30PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> Postcopy needs a method to send messages from the destination back to
> the source, this is the 'return path'.
> 
> Wire it up for 'socket' QEMUFile's using a dup'd fd.
> 
> Signed-off-by: Dr. David Alan Gilbert 
> ---
>  include/migration/qemu-file.h  |  7 +
>  migration/qemu-file-internal.h |  2 ++
>  migration/qemu-file-unix.c | 58 
> +++---
>  migration/qemu-file.c  | 12 +
>  4 files changed, 70 insertions(+), 9 deletions(-)
> 
> diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
> index 6ae0b03..3c38963 100644
> --- a/include/migration/qemu-file.h
> +++ b/include/migration/qemu-file.h
> @@ -85,6 +85,11 @@ typedef size_t (QEMURamSaveFunc)(QEMUFile *f, void *opaque,
> int *bytes_sent);
>  
>  /*
> + * Return a QEMUFile for comms in the opposite direction
> + */
> +typedef QEMUFile *(QEMURetPathFunc)(void *opaque);
> +
> +/*
>   * Stop any read or write (depending on flags) on the underlying
>   * transport on the QEMUFile.
>   * Existing blocking reads/writes must be woken
> @@ -102,6 +107,7 @@ typedef struct QEMUFileOps {
>  QEMURamHookFunc *after_ram_iterate;
>  QEMURamHookFunc *hook_ram_load;
>  QEMURamSaveFunc *save_page;
> +QEMURetPathFunc *get_return_path;
>  QEMUFileShutdownFunc *shut_down;
>  } QEMUFileOps;
>  
> @@ -188,6 +194,7 @@ int64_t qemu_file_get_rate_limit(QEMUFile *f);
>  int qemu_file_get_error(QEMUFile *f);
>  void qemu_file_set_error(QEMUFile *f, int ret);
>  int qemu_file_shutdown(QEMUFile *f);
> +QEMUFile *qemu_file_get_return_path(QEMUFile *f);
>  void qemu_fflush(QEMUFile *f);
>  
>  static inline void qemu_put_be64s(QEMUFile *f, const uint64_t *pv)
> diff --git a/migration/qemu-file-internal.h b/migration/qemu-file-internal.h
> index d95e853..a39b8e3 100644
> --- a/migration/qemu-file-internal.h
> +++ b/migration/qemu-file-internal.h
> @@ -48,6 +48,8 @@ struct QEMUFile {
>  unsigned int iovcnt;
>  
>  int last_error;
> +
> +struct QEMUFile *return_path;

AFAICT, the only thing this field is used for is an assert, which
seems a bit pointless.  I'd suggest either getting rid of it, or
make qemu_file_get_return_path() safely idempotent by having it only
call the FileOps pointer if QEMUFile::return_path is non-NULL,
otherwise just return the existing return_path.

Setting the field probably belongs better in the wrapper than in the
socket specific callback, too, since there's nothing inherently
related to the socket implementation about 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


pgpVR4b2Kl2gE.pgp
Description: PGP signature

Re: [Qemu-devel] [RFC] introduce bitmap to bdrv_commit to trackdirtysector

[Fam Zheng]

> Is it worthy to implement it?

Maybe, I think once we start to assign node-name to each BDS in the tree, this
will be natural to implement.

> And does qemu support commit any external snapshot to its backing file?

Yes.

Re: [Qemu-devel] [PATCH v5 06/45] Provide runtime Target page information

[David Gibson]

On Wed, Feb 25, 2015 at 04:51:29PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> The migration code generally is built target-independent, however
> there are a few places where knowing the target page size would
> avoid artificially moving stuff into arch_init.
> 
> Provide 'qemu_target_page_bits()' that returns TARGET_PAGE_BITS
> to other bits of code so that they can stay target-independent.
> 
> Signed-off-by: Dr. David Alan Gilbert 

Reviewed-by: David Gibson 

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


pgpF3Z9ZKsrcJ.pgp
Description: PGP signature

Re: [Qemu-devel] [PATCH v5 05/45] Create MigrationIncomingState

[David Gibson]

On Wed, Feb 25, 2015 at 04:51:28PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> There are currently lots of pieces of incoming migration state scattered
> around, and postcopy is adding more, and it seems better to try and keep
> it together.
> 
> allocate MIS in process_incoming_migration_co
> 
> Signed-off-by: Dr. David Alan Gilbert 

Reviewed-by: David Gibson 

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


pgpDpaes8kJK7.pgp
Description: PGP signature

[Qemu-devel] [PATCH] Revert "timer: replace time() with QEMU_CLOCK_HOST"

[Alexey Kardashevskiy]

This reverts commit 2ed1ebcf65edf6757d8904000889ce52cc0a9d1b
as it breaks compile when configured with --enable-profiler:

/home/alexey/p/qemu/vl.c:710:15: error: 'qemu_time' redeclared as different 
kind of symbol
 static time_t qemu_time(void)
   ^
In file included from /home/alexey/p/qemu/include/block/aio.h:23:0,
 from /home/alexey/p/qemu/include/hw/hw.h:13,
 from /home/alexey/p/qemu/vl.c:62:
/home/alexey/p/qemu/include/qemu/timer.h:1005:16: note: previous declaration of 
'qemu_time' was here
 extern int64_t qemu_time, qemu_time_start;
^

Signed-off-by: Alexey Kardashevskiy 
---


I could rename qemu_time() but could not think of any nice and simple name
instead so here is revert :)


---
 vl.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/vl.c b/vl.c
index b47e223..0e59688 100644
--- a/vl.c
+++ b/vl.c
@@ -707,17 +707,13 @@ void vm_start(void)
 /***/
 /* real time host monotonic timer */
 
-static time_t qemu_time(void)
-{
-return qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000;
-}
-
 /***/
 /* host time/date access */
 void qemu_get_timedate(struct tm *tm, int offset)
 {
-time_t ti = qemu_time();
+time_t ti;
 
+time(&ti);
 ti += offset;
 if (rtc_date_offset == -1) {
 if (rtc_utc)
@@ -745,7 +741,7 @@ int qemu_timedate_diff(struct tm *tm)
 else
 seconds = mktimegm(tm) + rtc_date_offset;
 
-return seconds - qemu_time();
+return seconds - time(NULL);
 }
 
 static void configure_rtc_date_offset(const char *startdate, int legacy)
@@ -783,7 +779,7 @@ static void configure_rtc_date_offset(const char 
*startdate, int legacy)
 "'2006-06-17T16:01:21' or '2006-06-17'\n");
 exit(1);
 }
-rtc_date_offset = qemu_time() - rtc_start_date;
+rtc_date_offset = time(NULL) - rtc_start_date;
 }
 }
 
-- 
2.0.0

Re: [Qemu-devel] [RFC] introduce bitmap to bdrv_commit to trackdirtysector

[Zhang Haoyu]

On 2015-03-10 09:54:47, Fam Zheng wrote:
> On Tue, 03/10 09:30, Zhang Haoyu wrote:
> > 
> > On 2015-03-10 08:29:19, Fam Zheng wrote:
> > > On Mon, 03/09 16:14, Zhang Haoyu wrote:
> > > > Hi John, Vladimir 
> > > > We can using active block commit to implement incremental backup 
> > > > without guest disruption,
> > > > e.g., 
> > > > origin <= A <= B <= C <= current BDS,
> > > > a new external snapshot will be produced before every time backup,
> > > > we can migrate A, B, C, ... to destination, 
> > > > then commit_active_start() the unneeded snapshot in source or 
> > > > destination end.
> > > > 
> > > > So, comparing with above mechanism,
> > > > what's the advantages of the incremental backup implemented by John and 
> > > > Vladimir?
> > > 
> > > We can't migrate A, B, C because they are buried in the backing chain 
> > > under
> > > "current BDS". 
> > I think we can backup the incremental image(e.g., A, B, C) to destination 
> > in top mode,
> > although I haven't read the code in detail, it can work theoretically, I 
> > think.
> 
> No, we don't have any command do that.
> 
Is it worthy to implement it?
And does qemu support commit any external snapshot to its backing file?

> > 
> > > Even if we do that, there will be severe IO amplification
> > > compared to the dirty bitmap way.
> > > 
> > Yes, block-commit will produce extra IO.
> > But regarding incremental backup, when guest IO is performed, 
> > will the corresponding dirty bit be synchronized to qcow2 image 
> > simultaneously?
> 
> No, that would have a huge performance penalty. It will only be synced
> at shutdown and or periodically, therefore it has the same implications with
> other cache, such as page cache or block driver metadata cache.
> 
> > if no, if source VM is shut-down in non-normal way, like killed by force or 
> > by mistake or server poweroff suddenly,
> > some dirty bits maybe lost, then full copy is needed.
> 
> Yes, it is a reasonable rule.
> 
> > > 
> > drive-backup is not incremental backup, full copy is needed in every time 
> > backup,
> > so it dosen't meet our requirements.
> 
> I didn't mean drive-backup already provides incremental backup, but we do need
> it to implement it (see the patch series posted by John Snow).
> 
> Thanks,
> Fam

Re: [Qemu-devel] question about live migration with storage

[Zhang Haoyu]

On 2015-01-15 18:08:39, Paolo Bonzini wrote:
> 
> On 15/01/2015 10:56, Zhang Haoyu wrote:
> > I see, when waiting the completion of drive_mirror IO, the coroutine will be
> > switched back to main-thread to poll and process other events, like qmp 
> > request,
> > then after the IO completed, coroutine will be switched back in main-loop 
> > process, right?
> > 
> > Another question:
> > while starting to migrate storage, will the unused sector be allocated and 
> > transferred?
> > regarding the thin-provisioning qcow2 disk, will extra large IO requests be
> > performed, and more data be transferred for the unallocated clusters?
> 
> No.
> 
My test results show that
when using thin-provisioning qcow2 image(created by qemu-img create -f qcow2 
preallocation=metadata),
even the unallocated sectors will be transferred to destination, so much data 
is transferred,
so the qcow2 image in destination is full allocated.

Thanks,
Zhang Haoyu
> Paolo

Re: [Qemu-devel] [PATCH 0/3] fix pci related code typos

[Chen Fan]

Cc: qemu-trivial

On 03/10/2015 09:49 AM, Chen Fan wrote:

Chen Fan (3):
   pcie_aer: fix typos in pcie_aer_inject_error comment
   aer: fix a wrong init PCI_ERR_COR_STATUS w1cmask type register
   pci: fix several trivial typos in comment

  hw/pci/pcie_aer.c | 8 
  include/hw/pci/pci.h  | 2 +-
  include/hw/pci/pcie_aer.h | 2 +-
  3 files changed, 6 insertions(+), 6 deletions(-)

[Qemu-devel] [PATCH 3/3] pci: fix several trivial typos in comment

[Chen Fan]

Signed-off-by: Chen Fan 
---
 include/hw/pci/pci.h  | 2 +-
 include/hw/pci/pcie_aer.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index bdee464..b82de15 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -137,7 +137,7 @@ enum {
 #define PCI_CONFIG_HEADER_SIZE 0x40
 /* Size of the standard PCI config space */
 #define PCI_CONFIG_SPACE_SIZE 0x100
-/* Size of the standart PCIe config space: 4KB */
+/* Size of the standard PCIe config space: 4KB */
 #define PCIE_CONFIG_SPACE_SIZE  0x1000
 
 #define PCI_NUM_PINS 4 /* A-D */
diff --git a/include/hw/pci/pcie_aer.h b/include/hw/pci/pcie_aer.h
index bcac80a..2fb8388 100644
--- a/include/hw/pci/pcie_aer.h
+++ b/include/hw/pci/pcie_aer.h
@@ -51,7 +51,7 @@ struct PCIEAERLog {
 PCIEAERErr *log;
 };
 
-/* aer error message: error signaling message has only error sevirity and
+/* aer error message: error signaling message has only error severity and
source id. See 2.2.8.3 error signaling messages */
 struct PCIEAERMsg {
 /*
-- 
1.9.3

[Qemu-devel] [PATCH 0/3] fix pci related code typos

[Chen Fan]

Chen Fan (3):
  pcie_aer: fix typos in pcie_aer_inject_error comment
  aer: fix a wrong init PCI_ERR_COR_STATUS w1cmask type register
  pci: fix several trivial typos in comment

 hw/pci/pcie_aer.c | 8 
 include/hw/pci/pci.h  | 2 +-
 include/hw/pci/pcie_aer.h | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 2/3] aer: fix a wrong init PCI_ERR_COR_STATUS w1cmask type register

[Chen Fan]

>From pcie spec, the bits attributes are RW1CS in Correctable
Error Status Register, so this patch fix a wrong definition
for PCI_ERR_COR_STATUS register with w1cmask type.

Signed-off-by: Chen Fan 
---
 hw/pci/pcie_aer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index 7ca077a..ece1487 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -123,7 +123,7 @@ int pcie_aer_init(PCIDevice *dev, uint16_t offset)
  PCI_ERR_UNC_SUPPORTED);
 
 pci_long_test_and_set_mask(dev->w1cmask + offset + PCI_ERR_COR_STATUS,
-   PCI_ERR_COR_STATUS);
+   PCI_ERR_COR_SUPPORTED);
 
 pci_set_long(dev->config + offset + PCI_ERR_COR_MASK,
  PCI_ERR_COR_MASK_DEFAULT);
-- 
1.9.3

[Qemu-devel] [PATCH 1/3] pcie_aer: fix typos in pcie_aer_inject_error comment

[Chen Fan]

Refer to "PCI Express Base Spec3.0", this comments can't
fit the description in spec, so we should fix them.

Signed-off-by: Chen Fan 
---
 hw/pci/pcie_aer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index 1f4be16..7ca077a 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -618,11 +618,11 @@ static bool pcie_aer_inject_uncor_error(PCIEAERInject 
*inj, bool is_fatal)
  * non-Function specific error must be recorded in all functions.
  * It is the responsibility of the caller of this function.
  * It is also caller's responsibility to determine which function should
- * report the rerror.
+ * report the error.
  *
  * 6.2.4 Error Logging
- * 6.2.5 Sqeunce of Device Error Signaling and Logging Operations
- * table 6-2: Flowchard Showing Sequence of Device Error Signaling and Logging
+ * 6.2.5 Sequence of Device Error Signaling and Logging Operations
+ * table 6-2: Flowchart Showing Sequence of Device Error Signaling and Logging
  *Operations
  */
 int pcie_aer_inject_error(PCIDevice *dev, const PCIEAERErr *err)
-- 
1.9.3

Re: [Qemu-devel] [RFC] introduce bitmap to bdrv_commit to trackdirty sector

[Fam Zheng]

On Tue, 03/10 09:30, Zhang Haoyu wrote:
> 
> On 2015-03-10 08:29:19, Fam Zheng wrote:
> > On Mon, 03/09 16:14, Zhang Haoyu wrote:
> > > Hi John, Vladimir 
> > > We can using active block commit to implement incremental backup without 
> > > guest disruption,
> > > e.g., 
> > > origin <= A <= B <= C <= current BDS,
> > > a new external snapshot will be produced before every time backup,
> > > we can migrate A, B, C, ... to destination, 
> > > then commit_active_start() the unneeded snapshot in source or destination 
> > > end.
> > > 
> > > So, comparing with above mechanism,
> > > what's the advantages of the incremental backup implemented by John and 
> > > Vladimir?
> > 
> > We can't migrate A, B, C because they are buried in the backing chain under
> > "current BDS". 
> I think we can backup the incremental image(e.g., A, B, C) to destination in 
> top mode,
> although I haven't read the code in detail, it can work theoretically, I 
> think.

No, we don't have any command do that.

> 
> > Even if we do that, there will be severe IO amplification
> > compared to the dirty bitmap way.
> > 
> Yes, block-commit will produce extra IO.
> But regarding incremental backup, when guest IO is performed, 
> will the corresponding dirty bit be synchronized to qcow2 image 
> simultaneously?

No, that would have a huge performance penalty. It will only be synced
at shutdown and or periodically, therefore it has the same implications with
other cache, such as page cache or block driver metadata cache.

> if no, if source VM is shut-down in non-normal way, like killed by force or 
> by mistake or server poweroff suddenly,
> some dirty bits maybe lost, then full copy is needed.

Yes, it is a reasonable rule.

> > 
> drive-backup is not incremental backup, full copy is needed in every time 
> backup,
> so it dosen't meet our requirements.

I didn't mean drive-backup already provides incremental backup, but we do need
it to implement it (see the patch series posted by John Snow).

Thanks,
Fam

Re: [Qemu-devel] [RFC v4 0/9] pass aer error to guest for vfio device

[Chen Fan]

On 03/10/2015 04:34 AM, Alex Williamson wrote:

On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:

For now, for vfio pci passthough devices when qemu receives
an error from host aer report, there just terminate the guest,
but usually user want to know what error occurred but stop the
guest, so this patches add aer capability support for vfio device,
and pass the error to guest, and have guest driver to recover
from the error.
and turning on SERR# for error forwording in bridge control register
patch in seabios has been merged.

v3-v4:
1. add 'x-aer' for user to off aer capability.
2. refactor vfio device to parse extended capabilities.

v2-v3:
1. refactor vfio device to parse extended capability.
2. add global property for piix4 to disable vfio aer cap.

v1-v2:
1. turn on SERR# for bridge control register in firmware.
2. initilize aer capability for vfio device.
3. fix some trivial bug.

Chen Fan (9):
   pcie_aer: fix typos in pcie_aer_inject_error comment
   aer: fix a wrong init PCI_ERR_COR_STATUS w1cmask type register
   vfio: add pcie extanded capability support
   aer: impove pcie_aer_init to support vfio device
   vfio: add aer support for vfio device
   vfio: add 'x-aer' option to disable aer capability
   pcie_aer: expose pcie_aer_msg() interface
   vfio-pci: pass the aer error to guest
   pcie: fix several trivial typos

  hw/pci-bridge/ioh3420.c|   3 +-
  hw/pci-bridge/xio3130_downstream.c |   3 +-
  hw/pci-bridge/xio3130_upstream.c   |   3 +-
  hw/pci/pcie_aer.c  |  17 ++--
  hw/vfio/pci.c  | 160 +++--
  include/hw/pci/pci.h   |   2 +-
  include/hw/pci/pcie_aer.h  |   7 +-
  7 files changed, 174 insertions(+), 21 deletions(-)


I would encourage you to submit any of the trivial typos and spelling
fixes separately, including them in a vfio series is only going to slow
down acceptance since it touches core-pci code, which I do not maintain.
Likewise we'll minimally need ACKs from MST for the PCI changes, but it
may be a wise move to send them separately with full justification as
well.  We're into the QEMU 2.3 freeze, so aside from trivial fixes, the
new functionality will need to wait until after 2.3 is tagged.  Thanks,
I got it, Thanks for your suggestion. I will send the trivial typos 
separately.


Thanks,
Chen




Alex

.

Re: [Qemu-devel] [RFC] introduce bitmap to bdrv_commit to trackdirty sector

[Zhang Haoyu]

On 2015-03-10 08:29:19, Fam Zheng wrote:
> On Mon, 03/09 16:14, Zhang Haoyu wrote:
> > Hi John, Vladimir 
> > We can using active block commit to implement incremental backup without 
> > guest disruption,
> > e.g., 
> > origin <= A <= B <= C <= current BDS,
> > a new external snapshot will be produced before every time backup,
> > we can migrate A, B, C, ... to destination, 
> > then commit_active_start() the unneeded snapshot in source or destination 
> > end.
> > 
> > So, comparing with above mechanism,
> > what's the advantages of the incremental backup implemented by John and 
> > Vladimir?
> 
> We can't migrate A, B, C because they are buried in the backing chain under
> "current BDS". 
I think we can backup the incremental image(e.g., A, B, C) to destination in 
top mode,
although I haven't read the code in detail, it can work theoretically, I think.

> Even if we do that, there will be severe IO amplification
> compared to the dirty bitmap way.
> 
Yes, block-commit will produce extra IO.
But regarding incremental backup, when guest IO is performed, 
will the corresponding dirty bit be synchronized to qcow2 image simultaneously?
If yes, it will impact on IO throughput in two sides, I think,
1) extro IO  2) randomize the IO,
if no, if source VM is shut-down in non-normal way, like killed by force or by 
mistake or server poweroff suddenly,
some dirty bits maybe lost, then full copy is needed.

> For the same reason we have drive-backup (write intercept) instead of a
> "snapshot + drive-mirror" analogue.
> 
drive-backup is not incremental backup, full copy is needed in every time 
backup,
so it dosen't meet our requirements.

Zhang Haoyu
> Fam

Re: [Qemu-devel] [PATCH v5 04/45] Add qemu_get_counted_string to read a string prefixed by a count byte

[David Gibson]

On Wed, Feb 25, 2015 at 04:51:27PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> and use it in loadvm_state and ram_load.
> 
> Signed-off-by: Dr. David Alan Gilbert 

Pure cleanup, no change to migration stream.

Reviewed-by: David Gibson 

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


pgpkI7ghib1s0.pgp
Description: PGP signature

Re: [Qemu-devel] [PATCH v5 02/45] Split header writing out of qemu_save_state_begin

[David Gibson]

On Wed, Feb 25, 2015 at 04:51:25PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> Split qemu_save_state_begin to:
>   qemu_save_state_header   That writes the initial file header.
>   qemu_save_state_beginThat sets up devices and does the first
>device pass.
> 
> Used later in postcopy.
> 
> Signed-off-by: Dr. David Alan Gilbert 

Reviewed-by: David Gibson 

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


pgpNoUpBGOI41.pgp
Description: PGP signature

Re: [Qemu-devel] [PATCH RFC] Implement GIC-500 from GICv3 family for arm64

[Shannon Zhao]

On 2015/3/9 22:41, shlomo.pongr...@toganetworks.com wrote:
> From: Shlomo Pongratz 
> 
> This patch is a first step toward 128 cores support for arm64.
> 
> At first only 64 cores are supported for two reasons:
> First the largest integer type has the size of 64 bits and modifying
> essential data structures in order to support 128 cores will require
> the usage of bitops.
> Second currently the Linux (kernel) can be configured to support
> up to 64 cores thus there is no urgency with 128 cores support.
> 
> Things left to do:
> 
> Currently the booting Linux may got stuck. The probability of getting stuck
> increases with the number of cores. I'll appreciate core review.
> 
> There is a need to support flexible clusters size. The GIC-500 can support
> up to 128 cores, up to 32 clusters and up to 8 cores is a cluster.
> So for example, if one wishes to have 16 cores, the options are:
> 2 clusters of 8 cores each, 4 clusters with 4 cores each
> Currently only the first option is supported.
> There is an issue of passing clock affinity to via the dtb. In the dtb
> 
> interrupt section there are only 24 bit left to affinity since the
> variable is a 32 bit entity and 8 bits are reserved for flags.
> See Documentation/devicetree/bindings/arm/arch_timer.txt.
> Note that this issue is not seems to be critical as when checking
> /proc/irq/3/smp_affinity with 32 cores all 32 bits are one.
> 
> The last issue is to add support for 128 cores. This requires the usage
> of bitops and currently can be tested up to 64 cores.
> 
> Signed-off-by: Shlomo Pongratz 
> ---
>  hw/arm/Makefile.objs   |2 +-
>  hw/arm/virtv2.c|  774 +

Hi,

I think here you want to introduce GICv3 in this patch. So is this necessary to
add a new virtv2 machine? And the codes of this machine mostly are same with 
virt.

Maybe we can add a parameter such as -GICv3 for machine virt to choose GICv3 
for it
and choose GICv2 without this parameter. Then we can reuse more codes.

-- 
Thanks,
Shannon

Re: [Qemu-devel] [PATCH v5 01/45] Start documenting how postcopy works.

[David Gibson]

On Thu, Mar 05, 2015 at 09:21:39AM +, Dr. David Alan Gilbert wrote:
> * David Gibson (da...@gibson.dropbear.id.au) wrote:
> > On Wed, Feb 25, 2015 at 04:51:24PM +, Dr. David Alan Gilbert
> (git) wrote:
[snip]
> > > +=== Enabling postcopy ===
> > > +
> > > +To enable postcopy (prior to the start of migration):
> > > +
> > > +migrate_set_capability x-postcopy-ram on
> > > +
> > > +The migration will still start in precopy mode, however issuing:
> > > +
> > > +migrate_start_postcopy
> > > +
> > > +will now cause the transition from precopy to postcopy.
> > > +It can be issued immediately after migration is started or any
> > > +time later on.  Issuing it after the end of a migration is harmless.
> > 
> > It's not quite clear to me what this means.  Does
> > "migrate_start_postcopy" mean it will immediately transfer execution
> > and transfer any remaining pages postcopy, or does it just mean it
> > will start postcopying once the remaining data to transfer is small
> > enough?
> 
> Yes; it will flip into postcopy soon after issuing that command irrespective
> of the amount of data remaining.
> 
> > What's the reason for this rather awkward two stage activation of
> > postcopy?
> 
> We need to keep track of the pages that are received during the precopy phase,
> and do some madvise and other setups on the destination RAM area before 
> precopy
> starts; and so we need to know we might want to do postcopy - so we need
> to be told early.  In the earliest posted version of my patches I had a
> time-limit setting and after the time limit expired QEMU would switch into
> the second phase of postcopy itself, but Paolo suggested the 
> migrate_start_postcopy:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2014-07/msg00943.html
> 
> and it works out simpler anyway.

Ok, that makes sense.

> > > +=== Postcopy device transfer ===
> > > +
> > > +Loading of device data may cause the device emulation to access guest RAM
> > > +that may trigger faults that have to be resolved by the source, as such
> > > +the migration stream has to be able to respond with page data *during* 
> > > the
> > > +device load, and hence the device data has to be read from the stream 
> > > completely
> > > +before the device load begins to free the stream up.  This is achieved by
> > > +'packaging' the device data into a blob that's read in one go.
> > > +
> > > +Source behaviour
> > > +
> > > +Until postcopy is entered the migration stream is identical to normal
> > > +precopy, except for the addition of a 'postcopy advise' command at
> > > +the beginning, to tell the destination that postcopy might happen.
> > > +When postcopy starts the source sends the page discard data and then
> > > +forms the 'package' containing:
> > > +
> > > +   Command: 'postcopy ram listen'
> > > +   The device state
> > > +  A series of sections, identical to the precopy streams device 
> > > state stream
> > > +  containing everything except postcopiable devices (i.e. RAM)
> > > +   Command: 'postcopy ram run'
> > > +
> > > +The 'package' is sent as the data part of a Command: 'CMD_PACKAGED', and 
> > > the
> > > +contents are formatted in the same way as the main migration stream.
> > 
> > It seems to me the "ram listen", "ram run" and CMD_PACKAGED really
> > have to be used in conjuction this way, they don't really have any use
> > on their own.  So why not make it all CMD_POSTCOPY_TRANSITION and have
> > the "listen" and "run" take effect implicitly at the beginning and end
> > of the device data.
> 
> CMD_PACKAGED seems like something that was generally useful; it's fairly
> complicated on it's own and so it seemed best to keep it separate.

And can you actually think of another use case for it?

The thing that bothers me is that the "listen" and "run" operations
will not work correctly anywhere other than at the beginning and end
of the packaged blob.

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


pgpc1SXicKcEQ.pgp
Description: PGP signature

[Qemu-devel] OVMF whitepaper released

[Laszlo Ersek]

http://people.redhat.com/~lersek/ovmf-whitepaper-c770f8c.txt

(An official Red Hat whitepaper PDF edition, for graphical displays, is
in the works. As time and technical hurdles allow, both the plain text
and the PDF editions shall appear under
 too, replacing the stale article
currently visible there.)

Feedback is welcome.

For public feedback, please trim the address list to
 and yours truly; my intent is to spam
qemu-devel, linux-efi and libvir-list just this once. (Appropriate
levels of embarrassment are already being felt for spamming those.)

Thanks
Laszlo

Re: [Qemu-devel] [RFC] introduce bitmap to bdrv_commit to track dirty sector

[Fam Zheng]

On Mon, 03/09 16:14, Zhang Haoyu wrote:
> Hi John, Vladimir 
> We can using active block commit to implement incremental backup without 
> guest disruption,
> e.g., 
> origin <= A <= B <= C <= current BDS,
> a new external snapshot will be produced before every time backup,
> we can migrate A, B, C, ... to destination, 
> then commit_active_start() the unneeded snapshot in source or destination end.
> 
> So, comparing with above mechanism,
> what's the advantages of the incremental backup implemented by John and 
> Vladimir?

We can't migrate A, B, C because they are buried in the backing chain under
"current BDS". Even if we do that, there will be severe IO amplification
compared to the dirty bitmap way.

For the same reason we have drive-backup (write intercept) instead of a
"snapshot + drive-mirror" analogue.

Fam

Re: [Qemu-devel] [PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/9 18:17, Wei Liu wrote:

On Mon, Mar 09, 2015 at 02:45:36PM +0800, Chen, Tiejun wrote:
[...]



+exit (1);
+}
+} else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
+if (libxl_gfx_passthru_kind_from_string(buf, 
&b_info->u.hvm.gfx_passthru_kind)) {


Ditto.


+fprintf(stderr, "ERROR: invalid value \"%s\" for 
\"gfx_passthru\"\n",


Ditto.


So,

@@ -1959,13 +1959,15 @@ skip_vfb:
  } else if (i == 1) {
  libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
  } else {
-fprintf(stderr, "ERROR: invalid value %ld for
\"gfx_passthru\"\n", l);
+fprintf(stderr, "ERROR: invalid value %ld for"
+" \"gfx_passthru\"\n", l);
  exit (1);
  }
  } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
-if (libxl_gfx_passthru_kind_from_string(buf,
&b_info->u.hvm.gfx_passthru_kind)) {
-fprintf(stderr, "ERROR: invalid value \"%s\" for
\"gfx_passthru\"\n",
-buf);
+if (libxl_gfx_passthru_kind_from_string(buf,
+ &b_info->u.hvm.gfx_passthru_kind)) {
+fprintf(stderr, "ERROR: invalid value \"%s\" for"
+" \"gfx_passthru\"\n", buf);
  exit (1);
  }
  libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);



Your changes are mangled by your email client. But we don't normally
split the format string so that it's easier to grep.


Yeah.



What we normally do is

  fprintf(stderr,
  "FORMAT STRING",
 a, b, c);

If format string still treads over 80 column it's fine. We can live
with that.



Understood.

Thanks
Tiejun

[Qemu-devel] [PATCH] tpm: Move memory initialization into realize function

[Stefan Berger]

From: Stefan Berger 

Move the memory initialization into the DeviceClass realize function due to
isa_address_space crashing if called in the instance init function.
A recent change must have changed the order in which structures are
initialized so that this move is now necessary.

Signed-off-by: Stefan Berger 
---
 hw/tpm/tpm_tis.c | 11 ++-
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index d0bb97f..339e1bf 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -959,18 +959,12 @@ static void tpm_tis_realizefn(DeviceState *dev, Error 
**errp)
 tis->bh = qemu_bh_new(tpm_tis_receive_bh, s);
 
 isa_init_irq(&s->busdev, &tis->irq, tis->irq_num);
-}
-
-static void tpm_tis_initfn(Object *obj)
-{
-ISADevice *dev = ISA_DEVICE(obj);
-TPMState *s = TPM(obj);
 
 memory_region_init_io(&s->mmio, OBJECT(s), &tpm_tis_memory_ops,
   s, "tpm-tis-mmio",
   TPM_TIS_NUM_LOCALITIES << TPM_TIS_LOCALITY_SHIFT);
-memory_region_add_subregion(isa_address_space(dev), TPM_TIS_ADDR_BASE,
-&s->mmio);
+memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
+TPM_TIS_ADDR_BASE, &s->mmio);
 }
 
 static void tpm_tis_class_init(ObjectClass *klass, void *data)
@@ -987,7 +981,6 @@ static const TypeInfo tpm_tis_info = {
 .name = TYPE_TPM_TIS,
 .parent = TYPE_ISA_DEVICE,
 .instance_size = sizeof(TPMState),
-.instance_init = tpm_tis_initfn,
 .class_init  = tpm_tis_class_init,
 };
 
-- 
2.1.0

Re: [Qemu-devel] Crash when using -daemonize

[Stefan Berger]

On 03/09/2015 05:34 PM, Christian Borntraeger wrote:

Am 09.03.2015 um 22:03 schrieb Stefan Berger:

Since an upgrade to Fedora 21, I get crashes with QEMU when using -daemonize. I 
noticed this since libvirt could not QMP probe QEMU.

This is the command line used:

x86_64-softmmu/qemu-system-x86_64 -S -no-user-config -nodefaults -nographic -M 
none -pidfile /tmp/foo -daemonize

Here's the backtrace from the coredump:

#0  0x7fe653d5c8d7 in __GI_raise (sig=sig@entry=6)
 at ../sysdeps/unix/sysv/linux/raise.c:55
#1  0x7fe653d5e53a in __GI_abort () at abort.c:89
#2  0x7fe658c4cb80 in error_exit (err=,
 msg=msg@entry=0x7fe658f58580 <__func__.6036> "qemu_mutex_unlock")
 at util/qemu-thread-posix.c:48
#3  0x7fe658edcab0 in qemu_mutex_unlock (
 mutex=mutex@entry=0x7fe6593a01c0 )
 at util/qemu-thread-posix.c:93
#4  0x7fe658c7a96c in qemu_mutex_unlock_iothread ()
 at /root/qemu/qemu-git.pt/cpus.c:1137
#5  0x7fe658e7695f in os_host_main_loop_wait (timeout=-1)
 at main-loop.c:234
#6  main_loop_wait (nonblocking=) at main-loop.c:494
#7  0x7fe658c4e82e in main_loop () at vl.c:1795
#8  main (argc=, argv=, envp=)
 at vl.c:4354

I am using today's tip of the QEMU git tree, but I don't think that's the 
actual problem.

I think it is.
Paolo has posted a quick fix in the thread of "vl: take iothread lock very 
early".

Can you verify?


Yes, verified. And this solves this problem.

   Stefan

[Qemu-devel] Fix for incorrect SYSRET instruction implementation -- anyone looked at this yet?

[Bill Paul]

I'm certain I'm sending this in plain text mode this time. According to my 
reading of the Intel documentation, the SYSRET instruction is supposed to 
force the RPL bits of the %ss register to 3 when returning to user mode. The 
actual sequence is:

SS.Selector <-- (IA32_STAR[63:48]+8) OR 3; (* RPL forced to 3 *)

However, the code in helper_sysret() leaves them at 0 (in other words, the "OR 
3" part of the above sequence is missing). It does set the privilege level 
bits of %cs correctly though.

This has caused me trouble with some of my VxWorks development: code that runs 
okay on real hardware will crash on QEMU, unless I apply the patch below.

Can someone confirm that this is in fact a real bug? The Intel architecture 
manual seems quite clear about the SYSRET behavior. The bug seems to have been 
around as far back as QEMU 0.10.5.

I am using QEMU 2.2.0 on FreeBSD/amd64 9.1-RELEASE.

-Bill

Signed-off-by: Bill Paul 

---
 target-i386/seg_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
index fa374d0..2bc757a 100644
--- a/target-i386/seg_helper.c
+++ b/target-i386/seg_helper.c
@@ -1043,7 +1043,7 @@ void helper_sysret(CPUX86State *env, int dflag)
DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK);
 env->eip = (uint32_t)env->regs[R_ECX];
 }
-cpu_x86_load_seg_cache(env, R_SS, selector + 8,
+cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3,
0, 0x,
DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |
DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
@@ -1056,7 +1056,7 @@ void helper_sysret(CPUX86State *env, int dflag)
DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK);
 env->eip = (uint32_t)env->regs[R_ECX];
-cpu_x86_load_seg_cache(env, R_SS, selector + 8,
+cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3,
0, 0x,
DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |
DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
-- 
1.8.0

Re: [Qemu-devel] Fix for incorrect SYSRET instruction implementation -- anyone looked at this yet?

[Bill Paul]

Of all the gin joints in all the towns in all the world, Stefan Weil had to 
walk into mine at 15:23:36 on Monday 09 March 2015 and say:

> Hi Bill,
> 
> sending text only e-mails might help. Usually git send-email is better
> than using KMail or other mail software.

You know, I thought I was sending in plain text. Somewhere along the line 
KMail lied to me.

> Use tools like git gui to get a correct signature. Any optional personal
> comments should come directly after
> the line with ---.

After? Or before?

> Could you please re-send your patch? Check it before
> sending with scripts/checkpatch.pl.
> 
> Maybe this looks like much regulations to fix a bug, but some rules are
> necessary to keep a project
> like QEMU alive.

I'm going to send it once more because it was entirely my fault that my stupid 
mailer kept sending in HTML mode, but that's it.

-Bill

--
=
-Bill Paul(510) 749-2329 | Senior Member of Technical Staff,
 wp...@windriver.com | Master of Unix-Fu - Wind River Systems
=
   "I put a dollar in a change machine. Nothing changed." - George Carlin
=

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[John Snow]

On 02/26/2015 05:02 PM, Paolo Bonzini wrote:



On 26/02/2015 22:31, Jordan Hargrave wrote:


My OS initializes DMAR page tables and then enables the IOMMU translation.
Then OS initializes AHCI driver.  Writes VIRTUAL DMA to FIS registers.
eg. FIS DMA address is 0x1 (maps to some hardware physical address
via iommu)

The OS writes 0x00 PORT_FIS_ADDR_HI -> qemu calls map_page (0x00 << 32)
| 0x7fae... 0x7fae is stale, and is not in the IOMMU page map.
Causes a non-recoverable IOMMU fault.


That's a bug in QEMU.  map_page must be skipped unless PORT_CMD_FIS_ON
is set in pr->cmd (also, QEMU is never resetting PORT_CMD_FIS_ON when
PORT_CMD_FIS_RX goes down).

Paolo



map_page must be skipped *if* PORT_CMD_FIS_ON is set -- or rather, that 
produces undefined behavior in the AHCI spec. We can only set these 
fields when the FIS Receive Engine is off. We can only modify the CLB if 
PxCMD.ST is off.


We also never actually reset PORT_CMD_FIS_RX or PORT_CMD_FIS_ON 
ourselves, so the status bit will get reset when the user writes zeroes 
to those bits when writing PxCMD. (We always clear PORT_CMD_FIS_ON and 
PORT_CMD_LIST_ON both upon any write to PxCMD, then re-add these flags 
if we see the user has written PORT_CMD_START or PORT_CMD_FIS_RX.)


So I think I can fix this just by delaying the CLB and FB mappings to 
the PORT_CMD_LIST_ON and PORT_CMD_FIS_ON events. I'll send a patch.

[Qemu-devel] [RFC 2/2] macio: move unaligned DMA write code into separate pmac_dma_write() function

[Mark Cave-Ayland]

Similarly switch the macio IDE routines over to use the new function and
tidy-up the remaining code as required.

Signed-off-by: Mark Cave-Ayland 
---
 hw/ide/macio.c |  267 
 include/hw/ppc/mac_dbdma.h |4 -
 2 files changed, 124 insertions(+), 147 deletions(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 4061bd6..47e64b1 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -143,6 +143,100 @@ static void pmac_dma_read(BlockBackend *blk,
 m->aiocb = blk_aio_readv(blk, sector_num, &io->iov, nsector, cb, io);
 }
 
+static void pmac_dma_write(BlockBackend *blk,
+ int64_t sector_num, int nb_sectors,
+ void (*cb)(void *opaque, int ret), void *opaque)
+{
+DBDMA_io *io = opaque;
+MACIOIDEState *m = io->opaque;
+IDEState *s = idebus_active_if(&m->bus);
+dma_addr_t dma_addr, dma_len;
+void *mem;
+int nsector, remainder;
+int extra = 0;
+
+qemu_iovec_destroy(&io->iov);
+qemu_iovec_init(&io->iov, io->len / MACIO_PAGE_SIZE + 1);
+
+if (io->remainder_len > 0) {
+/* Return remainder of request */
+int transfer = MIN(io->remainder_len, io->len);
+
+MACIO_DPRINTF("--- processing write remainder %x\n", transfer);
+cpu_physical_memory_read(io->addr,
+ &io->remainder + (0x200 - transfer),
+ transfer);
+
+io->remainder_len -= transfer;
+io->len -= transfer;
+io->addr += transfer;
+
+s->io_buffer_index += transfer;
+s->io_buffer_size -= transfer;
+
+if (io->remainder_len != 0) {
+/* Still waiting for remainder */
+return;
+}
+
+MACIO_DPRINTF("--> prepending bounce buffer with size 0x200\n");
+
+/* Sector transfer complete - prepend to request */
+qemu_iovec_add(&io->iov, &io->remainder, 0x200);
+extra = 1;
+}
+
+if (s->drive_kind == IDE_CD) {
+sector_num = (int64_t)(s->lba << 2) + (s->io_buffer_index >> 9);
+} else {
+sector_num = ide_get_sector(s) + (s->io_buffer_index >> 9);
+}
+
+nsector = (io->len >> 9);
+remainder = io->len - (nsector << 9);
+
+MACIO_DPRINTF("--- DMA write transfer - addr: %" HWADDR_PRIx " len: %x\n",
+  io->addr, io->len);
+MACIO_DPRINTF("xxx remainder: %x\n", remainder);
+MACIO_DPRINTF("xxx sector_num: %lx   nsector: %x\n", sector_num, nsector);
+
+dma_addr = io->addr;
+dma_len = io->len;
+mem = dma_memory_map(&address_space_memory, dma_addr, &dma_len,
+ DMA_DIRECTION_TO_DEVICE);
+
+if (!remainder) {
+MACIO_DPRINTF("--- DMA write aligned - addr: %" HWADDR_PRIx
+  " len: %x\n", io->addr, io->len);
+qemu_iovec_add(&io->iov, mem, io->len);
+} else {
+/* Write up to last complete sector */
+MACIO_DPRINTF("--- DMA write unaligned - addr: %" HWADDR_PRIx
+  " len: %x\n", io->addr, (nsector << 9));
+qemu_iovec_add(&io->iov, mem, (nsector << 9));
+
+MACIO_DPRINTF("--- DMA write read- bounce addr: %p "
+  "remainder_len: %x\n", &io->remainder, remainder);
+cpu_physical_memory_read(io->addr + (nsector << 9), &io->remainder,
+ remainder);
+
+io->remainder_len = 0x200 - remainder;
+
+MACIO_DPRINTF("xxx remainder_len: %x\n", io->remainder_len);
+}
+
+s->io_buffer_size -= ((nsector + extra) << 9);
+s->io_buffer_index += ((nsector + extra) << 9);
+
+io->len = 0;
+
+MACIO_DPRINTF("--- Block write transfer   - sector_num: %lx  "
+  "nsector: %x\n", sector_num, nsector + extra);
+
+m->aiocb = blk_aio_writev(blk, sector_num, &io->iov, nsector + extra, cb,
+  io);
+}
+
 static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
 {
 DBDMA_io *io = opaque;
@@ -217,24 +311,19 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
 DBDMA_io *io = opaque;
 MACIOIDEState *m = io->opaque;
 IDEState *s = idebus_active_if(&m->bus);
-int n = 0;
 int64_t sector_num;
-int unaligned;
+int nsector, remainder;
+
+MACIO_DPRINTF("pmac_ide_transfer_cb\n");
 
 if (ret < 0) {
 MACIO_DPRINTF("DMA error\n");
 m->aiocb = NULL;
-qemu_sglist_destroy(&s->sg);
 ide_dma_error(s);
 io->remainder_len = 0;
 goto done;
 }
 
-if (--io->requests) {
-/* More requests still in flight */
-return;
-}
-
 if (!m->dma_active) {
 MACIO_DPRINTF("waiting for data (%#x - %#x - %x)\n",
   s->nsector, io->len, s->status);
@@ -243,155 +332,48 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
 return;
 }
 
-sector_num = ide_get_sector(s);
-MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_

[Qemu-devel] [RFC 0/2] macio: split out unaligned DMA access into separate functions

[Mark Cave-Ayland]

This patchset attempts to separate out the IDE/ATAPI logic from the unaligned
DMA access logic for macio which provides the following benefits:

1) Reduced code complexity

The existing macio IDE/ATAPI functions were becoming extremely difficult to
follow through the various callbacks. By splitting up the functions in this
way it becomes much easier to follow the DMA-specific sections of code.

2) Future-proofing

If/when the block layer becomes able to handle unaligned DMA accesses directly
then it should be possible to switch out pmac_dma_read() and pmac_dma_write()
with their unaligned-capable bdrv_*() equivalents without having to change any
other logic.

3) Fix intermittent CDROM detection under -M g3beige

The code refactoring now correctly handles non-block ATAPI transfers which
fixes the problem with intermittent CDROM detection with Darwin under
-M g3beige.

Signed-off-by: Mark Cave-Ayland 


Mark Cave-Ayland (2):
  macio: move unaligned DMA read code into separate pmac_dma_read()
function
  macio: move unaligned DMA write code into separate pmac_dma_write()
function

 hw/ide/macio.c |  487 +++-
 include/hw/ppc/mac_dbdma.h |4 -
 2 files changed, 254 insertions(+), 237 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [RFC 1/2] macio: move unaligned DMA read code into separate pmac_dma_read() function

[Mark Cave-Ayland]

This considerably helps simplify the complexity of the macio read routines and
by switching macio CDROM accesses to use the new code, fixes the issue with
the CDROM device being detected intermittently by Darwin/OS X.

Signed-off-by: Mark Cave-Ayland 
---
 hw/ide/macio.c |  252 
 1 file changed, 146 insertions(+), 106 deletions(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index f6074f2..4061bd6 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -51,128 +51,165 @@ static const int debug_macio = 0;
 
 #define MACIO_PAGE_SIZE 4096
 
-static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
+static void pmac_dma_read(BlockBackend *blk,
+  int64_t sector_num, int nb_sectors,
+  void (*cb)(void *opaque, int ret), void *opaque)
 {
 DBDMA_io *io = opaque;
 MACIOIDEState *m = io->opaque;
 IDEState *s = idebus_active_if(&m->bus);
-int unaligned;
+dma_addr_t dma_addr, dma_len;
+void *mem;
+int nsector, remainder;
 
-if (ret < 0) {
-m->aiocb = NULL;
-qemu_sglist_destroy(&s->sg);
-ide_atapi_io_error(s, ret);
-io->remainder_len = 0;
-goto done;
+qemu_iovec_destroy(&io->iov);
+qemu_iovec_init(&io->iov, io->len / MACIO_PAGE_SIZE + 1);
+
+if (io->remainder_len > 0) {
+/* Return remainder of request */
+int transfer = MIN(io->remainder_len, io->len);
+
+MACIO_DPRINTF("--- DMA read pop - bounce addr: %p addr: %"
+  HWADDR_PRIx " remainder_len: %x\n",
+  &io->remainder + (0x200 - transfer), io->addr,
+  io->remainder_len);
+
+cpu_physical_memory_write(io->addr,
+  &io->remainder + (0x200 - transfer),
+  transfer);
+
+io->remainder_len -= transfer;
+io->len -= transfer;
+io->addr += transfer;
+
+s->io_buffer_index += transfer;
+s->io_buffer_size -= transfer;
+
+if (io->remainder_len != 0) {
+/* Still waiting for remainder */
+return;
+}
+
+if (io->len == 0) {
+MACIO_DPRINTF("--- finished all read processing; go and finish\n");
+cb(opaque, 0);
+return;
+}
 }
 
-if (!m->dma_active) {
-MACIO_DPRINTF("waiting for data (%#x - %#x - %x)\n",
-  s->nsector, io->len, s->status);
-/* data not ready yet, wait for the channel to get restarted */
-io->processing = false;
-return;
+if (s->drive_kind == IDE_CD) {
+sector_num = (int64_t)(s->lba << 2) + (s->io_buffer_index >> 9);
+} else {
+sector_num = ide_get_sector(s) + (s->io_buffer_index >> 9);
 }
 
-MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size);
+nsector = ((io->len + 0x1ff) >> 9);
+remainder = (nsector << 9) - io->len;
 
-if (s->io_buffer_size > 0) {
-m->aiocb = NULL;
-qemu_sglist_destroy(&s->sg);
+MACIO_DPRINTF("--- DMA read transfer - addr: %" HWADDR_PRIx " len: %x\n",
+  io->addr, io->len);
+
+dma_addr = io->addr;
+dma_len = io->len;
+mem = dma_memory_map(&address_space_memory, dma_addr, &dma_len,
+ DMA_DIRECTION_FROM_DEVICE);
+
+if (!remainder) {
+MACIO_DPRINTF("--- DMA read aligned - addr: %" HWADDR_PRIx
+  " len: %x\n", io->addr, io->len);
+qemu_iovec_add(&io->iov, mem, io->len);
+} else {
+MACIO_DPRINTF("--- DMA read unaligned - addr: %" HWADDR_PRIx
+  " len: %x\n", io->addr, io->len);
+qemu_iovec_add(&io->iov, mem, io->len);
 
-s->packet_transfer_size -= s->io_buffer_size;
+MACIO_DPRINTF("--- DMA read push- bounce addr: %p "
+  "remainder_len: %x\n",
+  &io->remainder + 0x200 - remainder, remainder);
+qemu_iovec_add(&io->iov, &io->remainder + 0x200 - remainder,
+   remainder);
 
-s->io_buffer_index += s->io_buffer_size;
-s->lba += s->io_buffer_index >> 11;
-s->io_buffer_index &= 0x7ff;
+io->remainder_len = remainder;
 }
 
-s->io_buffer_size = MIN(io->len, s->packet_transfer_size);
+s->io_buffer_size -= io->len;
+s->io_buffer_index += io->len;
 
-MACIO_DPRINTF("remainder: %d io->len: %d size: %d\n", io->remainder_len,
-  io->len, s->packet_transfer_size);
-if (io->remainder_len && io->len) {
-/* guest wants the rest of its previous transfer */
-int remainder_len = MIN(io->remainder_len, io->len);
+io->len = 0;
 
-MACIO_DPRINTF("copying remainder %d bytes\n", remainder_len);
+MACIO_DPRINTF("--- Block read transfer   - sector_num: %lx  nsector: %x\n",
+  sector_num, nsector);
 
-cpu_physical_memory_write(io->addr, io->remaind

Re: [Qemu-devel] Fix for incorrect SYSRET instruction implementation -- anyone looked at this yet?

[Stefan Weil]

Hi Bill,

sending text only e-mails might help. Usually git send-email is better 
than using KMail or other mail software.
Use tools like git gui to get a correct signature. Any optional personal 
comments should come directly after
the line with ---. Could you please re-send your patch? Check it before 
sending with scripts/checkpatch.pl.


Maybe this looks like much regulations to fix a bug, but some rules are 
necessary to keep a project

like QEMU alive.

Cc'ing Paolo and Richard because this might be an important bug fix for 
the next release.

(scripts/get_maintainer.pl tells which maintainers should be cc'ed).

Regards
Stefan


Am 09.03.2015 um 23:02 schrieb Bill Paul:


Nobody has commented on this yet. According to my reading of the Intel 
documentation, the SYSRET instruction is supposed to force the RPL 
bits of the %ss register to 3 when returning to user mode. The actual 
sequence is:


SS.Selector <-- (IA32_STAR[63:48]+8) OR 3; (* RPL forced to 3 *)

However, the code in helper_sysret() leaves them at 0 (in other words, 
the "OR 3" part of the above sequence is missing). It does set the 
privilege level bits of %cs correctly though.


This has caused me trouble with some of my VxWorks development: code 
that runs okay on real hardware will crash on QEMU, unless I apply the 
patch below.


Can someone confirm that this is in fact a real bug? The Intel 
architecture manual seems quite clear about the SYSRET behavior. The 
bug seems to have been around as far back as QEMU 0.10.5.


I am using QEMU 2.2.0 on FreeBSD/amd64 9.1-RELEASE.

-Bill

Signed-off-by: Bill Paul 

---

target-i386/seg_helper.c | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c

index fa374d0..2bc757a 100644

--- a/target-i386/seg_helper.c

+++ b/target-i386/seg_helper.c

@@ -1043,7 +1043,7 @@ void helper_sysret(CPUX86State *env, int dflag)

DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK);

env->eip = (uint32_t)env->regs[R_ECX];

}

- cpu_x86_load_seg_cache(env, R_SS, selector + 8,

+ cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3,

0, 0x,

DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |

DESC_S_MASK | (3 << DESC_DPL_SHIFT) |

@@ -1056,7 +1056,7 @@ void helper_sysret(CPUX86State *env, int dflag)

DESC_S_MASK | (3 << DESC_DPL_SHIFT) |

DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK);

env->eip = (uint32_t)env->regs[R_ECX];

- cpu_x86_load_seg_cache(env, R_SS, selector + 8,

+ cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3,

0, 0x,

DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |

DESC_S_MASK | (3 << DESC_DPL_SHIFT) |

--

1.8.0

--

=

-Bill Paul (510) 749-2329 | Senior Member of Technical Staff,

wp...@windriver.com | Master of Unix-Fu - Wind River Systems

=

"I put a dollar in a change machine. Nothing changed." - George Carlin

=

[Qemu-devel] Fix for incorrect SYSRET instruction implementation -- anyone looked at this yet?

[Bill Paul]

Nobody has commented on this yet. According to my reading of the Intel 
documentation, the SYSRET instruction is supposed to force the RPL bits of the 
%ss register to 3 when returning to user mode. The actual sequence is:

SS.Selector <-- (IA32_STAR[63:48]+8) OR 3; (* RPL forced to 3 *)

However, the code in helper_sysret() leaves them at 0 (in other words, the "OR 
3" part of the above sequence is missing). It does set the privilege level 
bits of %cs correctly though.

This has caused me trouble with some of my VxWorks development: code that runs 
okay on real hardware will crash on QEMU, unless I apply the patch below.

Can someone confirm that this is in fact a real bug? The Intel architecture 
manual seems quite clear about the SYSRET behavior. The bug seems to have been 
around as far back as QEMU 0.10.5.

I am using QEMU 2.2.0 on FreeBSD/amd64 9.1-RELEASE.

-Bill

Signed-off-by: Bill Paul 

---
 target-i386/seg_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
index fa374d0..2bc757a 100644
--- a/target-i386/seg_helper.c
+++ b/target-i386/seg_helper.c
@@ -1043,7 +1043,7 @@ void helper_sysret(CPUX86State *env, int dflag)
DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK);
 env->eip = (uint32_t)env->regs[R_ECX];
 }
-cpu_x86_load_seg_cache(env, R_SS, selector + 8,
+cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3,
0, 0x,
DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |
DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
@@ -1056,7 +1056,7 @@ void helper_sysret(CPUX86State *env, int dflag)
DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK);
 env->eip = (uint32_t)env->regs[R_ECX];
-cpu_x86_load_seg_cache(env, R_SS, selector + 8,
+cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3,
0, 0x,
DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |
DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
-- 
1.8.0

-- 
=
-Bill Paul(510) 749-2329 | Senior Member of Technical Staff,
 wp...@windriver.com | Master of Unix-Fu - Wind River Systems
=
   "I put a dollar in a change machine. Nothing changed." - George Carlin
=

Re: [Qemu-devel] Crash when using -daemonize

[Christian Borntraeger]

Am 09.03.2015 um 22:03 schrieb Stefan Berger:
> Since an upgrade to Fedora 21, I get crashes with QEMU when using -daemonize. 
> I noticed this since libvirt could not QMP probe QEMU.
> 
> This is the command line used:
> 
> x86_64-softmmu/qemu-system-x86_64 -S -no-user-config -nodefaults -nographic 
> -M none -pidfile /tmp/foo -daemonize
> 
> Here's the backtrace from the coredump:
> 
> #0  0x7fe653d5c8d7 in __GI_raise (sig=sig@entry=6)
> at ../sysdeps/unix/sysv/linux/raise.c:55
> #1  0x7fe653d5e53a in __GI_abort () at abort.c:89
> #2  0x7fe658c4cb80 in error_exit (err=,
> msg=msg@entry=0x7fe658f58580 <__func__.6036> "qemu_mutex_unlock")
> at util/qemu-thread-posix.c:48
> #3  0x7fe658edcab0 in qemu_mutex_unlock (
> mutex=mutex@entry=0x7fe6593a01c0 )
> at util/qemu-thread-posix.c:93
> #4  0x7fe658c7a96c in qemu_mutex_unlock_iothread ()
> at /root/qemu/qemu-git.pt/cpus.c:1137
> #5  0x7fe658e7695f in os_host_main_loop_wait (timeout=-1)
> at main-loop.c:234
> #6  main_loop_wait (nonblocking=) at main-loop.c:494
> #7  0x7fe658c4e82e in main_loop () at vl.c:1795
> #8  main (argc=, argv=, envp=)
> at vl.c:4354
> 
> I am using today's tip of the QEMU git tree, but I don't think that's the 
> actual problem.

I think it is. 
Paolo has posted a quick fix in the thread of "vl: take iothread lock very 
early".

Can you verify? 

Paolo, what is the status of this fix?



diff --git a/vl.c b/vl.c
index e1ffd0a..af61835 100644
--- a/vl.c
+++ b/vl.c
@@ -3759,7 +3759,9 @@ int main(int argc, char **argv, char **envp)

 loc_set_none();

+qemu_mutex_unlock_iothread();
 os_daemonize();
+qemu_mutex_lock_iothread();

 if (qemu_init_main_loop(&main_loop_err)) {
 error_report_err(main_loop_err);

> 
> Anyone have an idea? I reinstalled glibc, but that doesn't seem to solve what 
> looks like a mutex problem.
> 
>Stefan
> 
> 

[Qemu-devel] Crash when using -daemonize

[Stefan Berger]

Since an upgrade to Fedora 21, I get crashes with QEMU when using 
-daemonize. I noticed this since libvirt could not QMP probe QEMU.


This is the command line used:

x86_64-softmmu/qemu-system-x86_64 -S -no-user-config -nodefaults 
-nographic -M none -pidfile /tmp/foo -daemonize


Here's the backtrace from the coredump:

#0  0x7fe653d5c8d7 in __GI_raise (sig=sig@entry=6)
at ../sysdeps/unix/sysv/linux/raise.c:55
#1  0x7fe653d5e53a in __GI_abort () at abort.c:89
#2  0x7fe658c4cb80 in error_exit (err=,
msg=msg@entry=0x7fe658f58580 <__func__.6036> "qemu_mutex_unlock")
at util/qemu-thread-posix.c:48
#3  0x7fe658edcab0 in qemu_mutex_unlock (
mutex=mutex@entry=0x7fe6593a01c0 )
at util/qemu-thread-posix.c:93
#4  0x7fe658c7a96c in qemu_mutex_unlock_iothread ()
at /root/qemu/qemu-git.pt/cpus.c:1137
#5  0x7fe658e7695f in os_host_main_loop_wait (timeout=-1)
at main-loop.c:234
#6  main_loop_wait (nonblocking=) at main-loop.c:494
#7  0x7fe658c4e82e in main_loop () at vl.c:1795
#8  main (argc=, argv=, envp=)
at vl.c:4354

I am using today's tip of the QEMU git tree, but I don't think that's 
the actual problem.


Anyone have an idea? I reinstalled glibc, but that doesn't seem to solve 
what looks like a mutex problem.


   Stefan

[Qemu-devel] [PULL 5/7] target-i386: Move CPUX86State::cpuid_apic_id to X86CPU::apic_id

[Eduardo Habkost]

The field doesn't need to be inside CPUX86State, and it is not specific
for the CPUID instruction, so move and rename it.

Reviewed-by: Paolo Bonzini 
Reviewed-by: Andreas Färber 
Signed-off-by: Eduardo Habkost 
---
 target-i386/cpu-qom.h |  1 +
 target-i386/cpu.c | 15 +++
 target-i386/cpu.h |  1 -
 target-i386/kvm.c |  2 +-
 4 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index b557b61..4a6f48a 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -93,6 +93,7 @@ typedef struct X86CPU {
 bool expose_kvm;
 bool migratable;
 bool host_features;
+uint32_t apic_id;
 
 /* if true the CPUID code directly forward host cache leaves to the guest 
*/
 bool cache_info_passthrough;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3e1a0e7..6dd74f0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1690,7 +1690,7 @@ static void x86_cpuid_get_apic_id(Object *obj, Visitor 
*v, void *opaque,
   const char *name, Error **errp)
 {
 X86CPU *cpu = X86_CPU(obj);
-int64_t value = cpu->env.cpuid_apic_id;
+int64_t value = cpu->apic_id;
 
 visit_type_int(v, &value, name, errp);
 }
@@ -1723,11 +1723,11 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor 
*v, void *opaque,
 return;
 }
 
-if ((value != cpu->env.cpuid_apic_id) && cpu_exists(value)) {
+if ((value != cpu->apic_id) && cpu_exists(value)) {
 error_setg(errp, "CPU with APIC ID %" PRIi64 " exists", value);
 return;
 }
-cpu->env.cpuid_apic_id = value;
+cpu->apic_id = value;
 }
 
 /* Generic getter for "feature-words" and "filtered-features" properties */
@@ -2252,7 +2252,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
uint32_t count,
 break;
 case 1:
 *eax = env->cpuid_version;
-*ebx = (env->cpuid_apic_id << 24) | 8 << 8; /* CLFLUSH size in quad 
words, Linux wants it. */
+*ebx = (cpu->apic_id << 24) |
+   8 << 8; /* CLFLUSH size in quad words, Linux wants it. */
 *ecx = env->features[FEAT_1_ECX];
 *edx = env->features[FEAT_1_EDX];
 if (cs->nr_cores * cs->nr_threads > 1) {
@@ -2699,7 +2700,6 @@ static void mce_init(X86CPU *cpu)
 #ifndef CONFIG_USER_ONLY
 static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 {
-CPUX86State *env = &cpu->env;
 DeviceState *dev = DEVICE(cpu);
 APICCommonState *apic;
 const char *apic_type = "apic";
@@ -2718,7 +2718,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 
 object_property_add_child(OBJECT(cpu), "apic",
   OBJECT(cpu->apic_state), NULL);
-qdev_prop_set_uint8(cpu->apic_state, "id", env->cpuid_apic_id);
+qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id);
 /* TODO: convert to link<> */
 apic = APIC_COMMON(cpu->apic_state);
 apic->cpu = cpu;
@@ -2914,9 +2914,8 @@ static void x86_cpu_initfn(Object *obj)
 static int64_t x86_cpu_get_arch_id(CPUState *cs)
 {
 X86CPU *cpu = X86_CPU(cs);
-CPUX86State *env = &cpu->env;
 
-return env->cpuid_apic_id;
+return cpu->apic_id;
 }
 
 static bool x86_cpu_get_paging_enabled(const CPUState *cs)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 478450c..38bedc2 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -944,7 +944,6 @@ typedef struct CPUX86State {
 uint32_t cpuid_version;
 FeatureWordArray features;
 uint32_t cpuid_model[12];
-uint32_t cpuid_apic_id;
 
 /* MTRRs */
 uint64_t mtrr_fixed[11];
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 40d6a14..27fe2be 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -430,7 +430,7 @@ static void cpu_update_state(void *opaque, int running, 
RunState state)
 unsigned long kvm_arch_vcpu_id(CPUState *cs)
 {
 X86CPU *cpu = X86_CPU(cs);
-return cpu->env.cpuid_apic_id;
+return cpu->apic_id;
 }
 
 #ifndef KVM_CPUID_SIGNATURE_NEXT
-- 
2.1.0

[Qemu-devel] [PULL 7/7] target-i386: Require APIC ID to be explicitly set before CPU realize

[Eduardo Habkost]

On softmuu, instead of setting APIC ID automatically when creating a
X86CPU, require the property to be set before realizing the object
(which is already done by the CPU creation code on PC).

Keep apic_id = 0 by default on *-user so it can simply create a new CPU
object and realize it without extra steps (so target-i386 will be able
to use cpu_generic_init() eventually).

Reviewed-by: Andreas Färber 
Signed-off-by: Eduardo Habkost 
---
 target-i386/cpu-qom.h |  2 +-
 target-i386/cpu.c | 10 ++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index 4a6f48a..31a0c1e 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -93,7 +93,7 @@ typedef struct X86CPU {
 bool expose_kvm;
 bool migratable;
 bool host_features;
-uint32_t apic_id;
+int64_t apic_id;
 
 /* if true the CPUID code directly forward host cache leaves to the guest 
*/
 bool cache_info_passthrough;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 110716e..50907d0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2757,6 +2757,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
 Error *local_err = NULL;
 static bool ht_warned;
 
+if (cpu->apic_id < 0) {
+error_setg(errp, "apic-id property was not initialized properly");
+return;
+}
+
 if (env->features[FEAT_7_0_EBX] && env->cpuid_level < 7) {
 env->cpuid_level = 7;
 }
@@ -2868,6 +2873,11 @@ static void x86_cpu_initfn(Object *obj)
 
 cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
 
+#ifndef CONFIG_USER_ONLY
+/* Any code creating new X86CPU objects have to set apic-id explicitly */
+cpu->apic_id = -1;
+#endif
+
 x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
 
 /* init various static tables used in TCG mode */
-- 
2.1.0

[Qemu-devel] [PULL 4/7] target-i386: Remove unused APIC ID default code

[Eduardo Habkost]

The existing apic_id = cpu_index code has no visible effect: the PC code
already initializes the APIC ID according to the topology on
pc_new_cpu(), and linux-user memcpy()s the CPU state (including
cpuid_apic_id) on cpu_copy().

Remove the dead code and simply let APIC ID to to be 0 by default. This
doesn't change behavior of PC because apic-id is already explicitly set,
and doesn't affect linux-user because APIC ID was already always 0.

Reviewed-by: Paolo Bonzini 
Reviewed-by: Andreas Färber 
Signed-off-by: Eduardo Habkost 
---
 target-i386/cpu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 2f3a450..3e1a0e7 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2901,7 +2901,6 @@ static void x86_cpu_initfn(Object *obj)
 NULL, NULL, (void *)cpu->filtered_features, NULL);
 
 cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
-env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
 
 x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
 
-- 
2.1.0

[Qemu-devel] [PULL 3/7] target-i386: Eliminate unnecessary get_cpuid_vendor() function

[Eduardo Habkost]

The function was used in only two places. In one of them, the function
made the code less readable by requiring temporary te[bcd]x variables.
In the other one we can simply inline the existing code.

Reviewed-by: Andreas Färber 
Signed-off-by: Eduardo Habkost 
---
 target-i386/cpu.c | 20 ++--
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 80e9b9d..2f3a450 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2213,14 +2213,6 @@ void x86_cpudef_setup(void)
 }
 }
 
-static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
- uint32_t *ecx, uint32_t *edx)
-{
-*ebx = env->cpuid_vendor1;
-*edx = env->cpuid_vendor2;
-*ecx = env->cpuid_vendor3;
-}
-
 void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
uint32_t *eax, uint32_t *ebx,
uint32_t *ecx, uint32_t *edx)
@@ -2254,7 +2246,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
uint32_t count,
 switch(index) {
 case 0:
 *eax = env->cpuid_level;
-get_cpuid_vendor(env, ebx, ecx, edx);
+*ebx = env->cpuid_vendor1;
+*edx = env->cpuid_vendor2;
+*ecx = env->cpuid_vendor3;
 break;
 case 1:
 *eax = env->cpuid_version;
@@ -2447,11 +2441,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
uint32_t count,
  * So dont set it here for Intel to make Linux guests happy.
  */
 if (cs->nr_cores * cs->nr_threads > 1) {
-uint32_t tebx, tecx, tedx;
-get_cpuid_vendor(env, &tebx, &tecx, &tedx);
-if (tebx != CPUID_VENDOR_INTEL_1 ||
-tedx != CPUID_VENDOR_INTEL_2 ||
-tecx != CPUID_VENDOR_INTEL_3) {
+if (env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1 ||
+env->cpuid_vendor2 != CPUID_VENDOR_INTEL_2 ||
+env->cpuid_vendor3 != CPUID_VENDOR_INTEL_3) {
 *ecx |= 1 << 1;/* CmpLegacy bit */
 }
 }
-- 
2.1.0

[Qemu-devel] [PULL 0/7] X86 patches

[Eduardo Habkost]

The following changes since commit 277263e1b320d759a760ba6c5ea75ec268f929e5:

  Merge remote-tracking branch 'remotes/agraf/tags/signed-ppc-for-upstream' 
into staging (2015-03-09 14:04:14 +)

are available in the git repository at:

  https://github.com/ehabkost/qemu.git tags/x86-pull-request

for you to fetch changes up to 9886e834f47adabdbfd54ab606788ce7326e6779:

  target-i386: Require APIC ID to be explicitly set before CPU realize 
(2015-03-09 16:30:03 -0300)


X86 patches queued in the last few weeks. Mostly code cleanup and changes on
code assigning APIC ID.



Eduardo Habkost (7):
  target-i386: Move topology.h to include/hw/i386
  target-i386: Simplify listflags() function
  target-i386: Eliminate unnecessary get_cpuid_vendor() function
  target-i386: Remove unused APIC ID default code
  target-i386: Move CPUX86State::cpuid_apic_id to X86CPU::apic_id
  target-i386: Move APIC ID compatibility code to pc.c
  target-i386: Require APIC ID to be explicitly set before CPU realize

 hw/i386/pc.c|  35 
 {target-i386 => include/hw/i386}/topology.h |   6 +-
 target-i386/cpu-qom.h   |   1 +
 target-i386/cpu.c   | 122 +---
 target-i386/cpu.h   |   2 -
 target-i386/kvm.c   |   2 +-
 tests/Makefile  |   2 -
 tests/test-x86-cpuid.c  |   2 +-
 8 files changed, 78 insertions(+), 94 deletions(-)
 rename {target-i386 => include/hw/i386}/topology.h (97%)

-- 
2.1.0

[Qemu-devel] [PULL 6/7] target-i386: Move APIC ID compatibility code to pc.c

[Eduardo Habkost]

The APIC ID compatibility code is required only for PC, and now that
x86_cpu_initfn() doesn't use x86_cpu_apic_id_from_index() anymore, that
code can be moved to pc.c.

Reviewed-by: Paolo Bonzini 
Reviewed-by: Andreas Färber 
Signed-off-by: Eduardo Habkost 
---
 hw/i386/pc.c  | 35 +++
 target-i386/cpu.c | 34 --
 target-i386/cpu.h |  1 -
 3 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 79eaad5..b5b2aad 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -25,6 +25,8 @@
 #include "hw/i386/pc.h"
 #include "hw/char/serial.h"
 #include "hw/i386/apic.h"
+#include "hw/i386/topology.h"
+#include "sysemu/cpus.h"
 #include "hw/block/fdc.h"
 #include "hw/ide.h"
 #include "hw/pci/pci.h"
@@ -629,6 +631,39 @@ bool e820_get_entry(int idx, uint32_t type, uint64_t 
*address, uint64_t *length)
 return false;
 }
 
+/* Enables contiguous-apic-ID mode, for compatibility */
+static bool compat_apic_id_mode;
+
+void enable_compat_apic_id_mode(void)
+{
+compat_apic_id_mode = true;
+}
+
+/* Calculates initial APIC ID for a specific CPU index
+ *
+ * Currently we need to be able to calculate the APIC ID from the CPU index
+ * alone (without requiring a CPU object), as the QEMU<->Seabios interfaces 
have
+ * no concept of "CPU index", and the NUMA tables on fw_cfg need the APIC ID of
+ * all CPUs up to max_cpus.
+ */
+static uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
+{
+uint32_t correct_id;
+static bool warned;
+
+correct_id = x86_apicid_from_cpu_idx(smp_cores, smp_threads, cpu_index);
+if (compat_apic_id_mode) {
+if (cpu_index != correct_id && !warned) {
+error_report("APIC IDs set in compatibility mode, "
+ "CPU topology won't match the configuration");
+warned = true;
+}
+return cpu_index;
+} else {
+return correct_id;
+}
+}
+
 /* Calculates the limit to CPU APIC ID values
  *
  * This function returns the limit for the APIC ID value, so that all
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 6dd74f0..110716e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -25,7 +25,6 @@
 #include "sysemu/kvm.h"
 #include "sysemu/cpus.h"
 #include "kvm_i386.h"
-#include "hw/i386/topology.h"
 
 #include "qemu/option.h"
 #include "qemu/config-file.h"
@@ -2822,39 +2821,6 @@ out:
 }
 }
 
-/* Enables contiguous-apic-ID mode, for compatibility */
-static bool compat_apic_id_mode;
-
-void enable_compat_apic_id_mode(void)
-{
-compat_apic_id_mode = true;
-}
-
-/* Calculates initial APIC ID for a specific CPU index
- *
- * Currently we need to be able to calculate the APIC ID from the CPU index
- * alone (without requiring a CPU object), as the QEMU<->Seabios interfaces 
have
- * no concept of "CPU index", and the NUMA tables on fw_cfg need the APIC ID of
- * all CPUs up to max_cpus.
- */
-uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
-{
-uint32_t correct_id;
-static bool warned;
-
-correct_id = x86_apicid_from_cpu_idx(smp_cores, smp_threads, cpu_index);
-if (compat_apic_id_mode) {
-if (cpu_index != correct_id && !warned) {
-error_report("APIC IDs set in compatibility mode, "
- "CPU topology won't match the configuration");
-warned = true;
-}
-return cpu_index;
-} else {
-return correct_id;
-}
-}
-
 static void x86_cpu_initfn(Object *obj)
 {
 CPUState *cs = CPU(obj);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 38bedc2..0638d24 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1328,7 +1328,6 @@ void x86_cpu_compat_kvm_no_autodisable(FeatureWord w, 
uint32_t features);
 /* Return name of 32-bit register, from a R_* constant */
 const char *get_register_name_32(unsigned int reg);
 
-uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index);
 void enable_compat_apic_id_mode(void);
 
 #define APIC_DEFAULT_ADDRESS 0xfee0
-- 
2.1.0

[Qemu-devel] [PULL 1/7] target-i386: Move topology.h to include/hw/i386

[Eduardo Habkost]

This will allow the PC code to use the header, and lets us eliminate the
QEMU_INCLUDES hack inside tests/Makefile.

Reviewed-by: Paolo Bonzini 
Reviewed-by: Andreas Färber 
Signed-off-by: Eduardo Habkost 
---
 {target-i386 => include/hw/i386}/topology.h | 6 +++---
 target-i386/cpu.c   | 2 +-
 tests/Makefile  | 2 --
 tests/test-x86-cpuid.c  | 2 +-
 4 files changed, 5 insertions(+), 7 deletions(-)
 rename {target-i386 => include/hw/i386}/topology.h (97%)

diff --git a/target-i386/topology.h b/include/hw/i386/topology.h
similarity index 97%
rename from target-i386/topology.h
rename to include/hw/i386/topology.h
index 07a6c5f..9c6f3a9 100644
--- a/target-i386/topology.h
+++ b/include/hw/i386/topology.h
@@ -21,8 +21,8 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  */
-#ifndef TARGET_I386_TOPOLOGY_H
-#define TARGET_I386_TOPOLOGY_H
+#ifndef HW_I386_TOPOLOGY_H
+#define HW_I386_TOPOLOGY_H
 
 /* This file implements the APIC-ID-based CPU topology enumeration logic,
  * documented at the following document:
@@ -131,4 +131,4 @@ static inline apic_id_t x86_apicid_from_cpu_idx(unsigned 
nr_cores,
 return apicid_from_topo_ids(nr_cores, nr_threads, pkg_id, core_id, smt_id);
 }
 
-#endif /* TARGET_I386_TOPOLOGY_H */
+#endif /* HW_I386_TOPOLOGY_H */
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d543e2b..8fc5727 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -25,7 +25,7 @@
 #include "sysemu/kvm.h"
 #include "sysemu/cpus.h"
 #include "kvm_i386.h"
-#include "topology.h"
+#include "hw/i386/topology.h"
 
 #include "qemu/option.h"
 #include "qemu/config-file.h"
diff --git a/tests/Makefile b/tests/Makefile
index 307035c..7d4b96d 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -239,8 +239,6 @@ $(test-obj-y): QEMU_INCLUDES += -Itests
 QEMU_CFLAGS += -I$(SRC_PATH)/tests
 qom-core-obj = qom/object.o qom/qom-qobject.o qom/container.o
 
-tests/test-x86-cpuid.o: QEMU_INCLUDES += -I$(SRC_PATH)/target-i386
-
 tests/check-qint$(EXESUF): tests/check-qint.o libqemuutil.a
 tests/check-qstring$(EXESUF): tests/check-qstring.o libqemuutil.a
 tests/check-qdict$(EXESUF): tests/check-qdict.o libqemuutil.a
diff --git a/tests/test-x86-cpuid.c b/tests/test-x86-cpuid.c
index 8d9f96a..6cd20d4 100644
--- a/tests/test-x86-cpuid.c
+++ b/tests/test-x86-cpuid.c
@@ -24,7 +24,7 @@
 
 #include 
 
-#include "topology.h"
+#include "hw/i386/topology.h"
 
 static void test_topo_bits(void)
 {
-- 
2.1.0

[Qemu-devel] [PULL 2/7] target-i386: Simplify listflags() function

[Eduardo Habkost]

listflags() had lots of unnecessary complexity. Instead of printing to a
buffer that will be immediately printed, simply call the printing
function directly. Also, remove the fbits and flags arguments that were
always set to the same value. Also, there's no need to list the flags in
reverse order.

Reviewed-by: Paolo Bonzini 
Signed-off-by: Eduardo Habkost 
---
 target-i386/cpu.c | 42 ++
 1 file changed, 14 insertions(+), 28 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 8fc5727..80e9b9d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1911,34 +1911,19 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char 
*features,
 }
 }
 
-/* generate a composite string into buf of all cpuid names in featureset
- * selected by fbits.  indicate truncation at bufsize in the event of overflow.
- * if flags, suppress names undefined in featureset.
+/* Print all cpuid feature names in featureset
  */
-static void listflags(char *buf, int bufsize, uint32_t fbits,
-  const char **featureset, uint32_t flags)
-{
-const char **p = &featureset[31];
-char *q, *b, bit;
-int nc;
-
-b = 4 <= bufsize ? buf + (bufsize -= 3) - 1 : NULL;
-*buf = '\0';
-for (q = buf, bit = 31; fbits && bufsize; --p, fbits &= ~(1 << bit), --bit)
-if (fbits & 1 << bit && (*p || !flags)) {
-if (*p)
-nc = snprintf(q, bufsize, "%s%s", q == buf ? "" : " ", *p);
-else
-nc = snprintf(q, bufsize, "%s[%d]", q == buf ? "" : " ", bit);
-if (bufsize <= nc) {
-if (b) {
-memcpy(b, "...", sizeof("..."));
-}
-return;
-}
-q += nc;
-bufsize -= nc;
+static void listflags(FILE *f, fprintf_function print, const char **featureset)
+{
+int bit;
+bool first = true;
+
+for (bit = 0; bit < 32; bit++) {
+if (featureset[bit]) {
+print(f, "%s%s", first ? "" : " ", featureset[bit]);
+first = false;
 }
+}
 }
 
 /* generate CPU information. */
@@ -1963,8 +1948,9 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) {
 FeatureWordInfo *fw = &feature_word_info[i];
 
-listflags(buf, sizeof(buf), (uint32_t)~0, fw->feat_names, 1);
-(*cpu_fprintf)(f, "  %s\n", buf);
+(*cpu_fprintf)(f, "  ");
+listflags(f, cpu_fprintf, fw->feat_names);
+(*cpu_fprintf)(f, "\n");
 }
 }
 
-- 
2.1.0

Re: [Qemu-devel] qemu crash in coroutine bdrv_co_do_rw

[Christian Borntraeger]

Am 06.03.2015 um 18:23 schrieb Stefan Hajnoczi:
> On Thu, Feb 26, 2015 at 10:29:57AM +0100, Christian Borntraeger wrote:
>> is this some know issue? Under heavy load with lots of dataplane devices I 
>> sometimes get a segfault in the bdrc_co_do_rw routine:
>>
>> #0  bdrv_co_do_rw (opaque=0x0) at /home/cborntra/REPOS/qemu/block.c:4791
>> 4791 if (!acb->is_write) {
>> (gdb) bt
>> #0  bdrv_co_do_rw (opaque=0x0) at /home/cborntra/REPOS/qemu/block.c:4791
>> #1  0x801aeb78 in coroutine_trampoline (i0=, 
>> i1=-725099072) at /home/cborntra/REPOS/qemu/coroutine-ucontext.c:80
>> #2  0x03fffbe1cca2 in __makecontext_ret () from /lib64/libc.so.6
>> Backtrace stopped: previous frame identical to this frame (corrupt stack?)
>> (gdb) up
>> #1  0x801aeb78 in coroutine_trampoline (i0=, 
>> i1=-725099072) at /home/cborntra/REPOS/qemu/coroutine-ucontext.c:80
>> 80   co->entry(co->entry_arg);
>> (gdb) print *co
>> $1 = {entry = 0x801a3c28 , entry_arg = 0x0, caller = 
>> 0x3ffe2fff788, pool_next = {sle_next = 0x3ffd2287990}, co_queue_wakeup = 
>> {tqh_first = 0x0, 
>> tqh_last = 0x3ffd4c7dde0}, co_queue_next = {tqe_next = 0x0, tqe_prev = 
>> 0x0}}
>>
>> As you can see enty_arg is 0, causing the problem. Do you have any quick 
>> idea before I start debugging?
> 
> No, I haven't seen this bug before.  Are you running qemu.git/master?
> 
> Have you tried disabling the coroutine pool (freelist)?
> 
> Stefan
> 

I was able to increase the likelyhood of hitting this (more vCPUs, less guests).

bisect thinks that this makes this shaky:

4d68e86bb10159099da0798f74e7512955f15eec is the first bad commit
commit 4d68e86bb10159099da0798f74e7512955f15eec
Author: Paolo Bonzini 
Date:   Tue Dec 2 12:05:48 2014 +0100

coroutine: rewrite pool to avoid mutex


Christian

Re: [Qemu-devel] [PATCH v2 7/8] qtest/ahci: add qcow2 support to ahci-test

[John Snow]

On 03/09/2015 10:27 AM, Kevin Wolf wrote:

Am 26.02.2015 um 00:06 hat John Snow geschrieben:

This will enable the testing of high offsets without
wasting a lot of disk space, and does not impact the
previous tests.

mkimg and mkqcow2 are added to libqos for other tests.

Signed-off-by: John Snow 
---
  tests/Makefile|  1 +
  tests/ahci-test.c | 16 ++--
  tests/libqos/libqos.c | 37 +
  tests/libqos/libqos.h |  2 ++
  4 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/tests/Makefile b/tests/Makefile
index 307035c..09ecb66 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -413,6 +413,7 @@ GCOV_OPTIONS = -n $(if $(V),-f,)
  $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: 
$(check-qtest-y)
$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
+   QTEST_QEMU_IMG=qemu-img$(EXESUF) \
MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \
gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y),"GTESTER 
$@")
$(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \
diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index cf0b98b..3f93c15 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -39,8 +39,8 @@
  #include "hw/pci/pci_ids.h"
  #include "hw/pci/pci_regs.h"

-/* Test-specific defines. */
-#define TEST_IMAGE_SIZE(64 * 1024 * 1024)
+/* Test-specific defines -- in MiB */
+#define TEST_IMAGE_SIZE_MB (200 * 1024)

  /*** Globals ***/
  static char tmp_path[] = "/tmp/qtest.XX";
@@ -81,7 +81,7 @@ static AHCIQState *ahci_boot(void)
  s = g_malloc0(sizeof(AHCIQState));

  cli = "-drive if=none,id=drive0,file=%s,cache=writeback,serial=%s"
-",format=raw"
+",format=qcow2"
  " -M q35 "
  "-device ide-hd,drive=drive0 "
  "-global ide-hd.ver=%s";
@@ -1051,7 +1051,6 @@ static void create_ahci_io_test(enum IOMode type, enum 
AddrMode addr,
  int main(int argc, char **argv)
  {
  const char *arch;
-int fd;
  int ret;
  int c;
  int i, j, k;
@@ -1088,12 +1087,9 @@ int main(int argc, char **argv)
  return 0;
  }

-/* Create a temporary raw image */
-fd = mkstemp(tmp_path);
-g_assert(fd >= 0);
-ret = ftruncate(fd, TEST_IMAGE_SIZE);
-g_assert(ret == 0);
-close(fd);
+/* Create a temporary qcow2 image */
+close(mkstemp(tmp_path));
+mkqcow2(tmp_path, TEST_IMAGE_SIZE_MB);

  /* Run the tests */
  qtest_add_func("/ahci/sanity", test_sanity);
diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
index bc8beb2..c825486 100644
--- a/tests/libqos/libqos.c
+++ b/tests/libqos/libqos.c
@@ -61,3 +61,40 @@ void qtest_shutdown(QOSState *qs)
  qtest_quit(qs->qts);
  g_free(qs);
  }
+
+void mkimg(const char *file, const char *fmt, unsigned size_mb)
+{
+gchar *cli;
+bool ret;
+int rc;
+GError *err = NULL;
+char *qemu_img_path;
+gchar *out, *out2;
+
+qemu_img_path = getenv("QTEST_QEMU_IMG");
+assert(qemu_img_path);
+
+cli = g_strdup_printf("./%s create -f %s %s %uM", qemu_img_path,
+  fmt, file, size_mb);
+ret = g_spawn_command_line_sync(cli, &out, &out2, &rc, &err);
+if (err) {
+fprintf(stderr, "%s\n", err->message);
+g_error_free(err);
+}
+g_assert(ret && !err);
+
+ret = g_spawn_check_exit_status(rc, &err);


This function only exists since glib 2.34. Dropping the following
patches from the queue:



I'm looking at this some more. The glib code basically does this:

If windows: Set an error if rc is nonzero.
If linux: use the WIFEXITED, WIFSIGNALED or WIFSTOPPED macros to 
determine the domain of the error code. If WIFEXITED, check for nonzero 
status. if WIFSIGNALED, WIFSTOPPED or !WIFEXITED, return an error.


I *think* I can just generalize this all, if I am not interested in 
*why* we failed, to just checking rc to be nonzero. That should be 
adequately multiplatform.


Agree?


pick 23134a5 qtest/ahci: add qcow2 support to ahci-test
pick 6ca5609 qtest/ahci: test different disk sectors
pick e2f0dee qtest/ahci: Add simple flush test
pick 396491b qtest/ahci: Allow override of default CLI options
pick eb8c8bd libqtest: add qmp_eventwait
pick 398bfc3 libqtest: add qmp_async
pick d3f77d1 libqos: add blkdebug_prepare_script
pick d628e51 qtest/ahci: add flush retry test

This is patch 7 and 8 from this series and the complete series "ahci:
rerror/werror=stop resume tests", which seems to depend on them.

Kevin

Re: [Qemu-devel] [RFC v4 0/9] pass aer error to guest for vfio device

[Alex Williamson]

On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:
> For now, for vfio pci passthough devices when qemu receives
> an error from host aer report, there just terminate the guest,
> but usually user want to know what error occurred but stop the
> guest, so this patches add aer capability support for vfio device,
> and pass the error to guest, and have guest driver to recover
> from the error.
> and turning on SERR# for error forwording in bridge control register
> patch in seabios has been merged.
> 
> v3-v4:
>1. add 'x-aer' for user to off aer capability.
>2. refactor vfio device to parse extended capabilities.
> 
> v2-v3:
>1. refactor vfio device to parse extended capability.
>2. add global property for piix4 to disable vfio aer cap.
> 
> v1-v2:
>1. turn on SERR# for bridge control register in firmware.
>2. initilize aer capability for vfio device.
>3. fix some trivial bug.
> 
> Chen Fan (9):
>   pcie_aer: fix typos in pcie_aer_inject_error comment
>   aer: fix a wrong init PCI_ERR_COR_STATUS w1cmask type register
>   vfio: add pcie extanded capability support
>   aer: impove pcie_aer_init to support vfio device
>   vfio: add aer support for vfio device
>   vfio: add 'x-aer' option to disable aer capability
>   pcie_aer: expose pcie_aer_msg() interface
>   vfio-pci: pass the aer error to guest
>   pcie: fix several trivial typos
> 
>  hw/pci-bridge/ioh3420.c|   3 +-
>  hw/pci-bridge/xio3130_downstream.c |   3 +-
>  hw/pci-bridge/xio3130_upstream.c   |   3 +-
>  hw/pci/pcie_aer.c  |  17 ++--
>  hw/vfio/pci.c  | 160 
> +++--
>  include/hw/pci/pci.h   |   2 +-
>  include/hw/pci/pcie_aer.h  |   7 +-
>  7 files changed, 174 insertions(+), 21 deletions(-)
> 

I would encourage you to submit any of the trivial typos and spelling
fixes separately, including them in a vfio series is only going to slow
down acceptance since it touches core-pci code, which I do not maintain.
Likewise we'll minimally need ACKs from MST for the PCI changes, but it
may be a wise move to send them separately with full justification as
well.  We're into the QEMU 2.3 freeze, so aside from trivial fixes, the
new functionality will need to wait until after 2.3 is tagged.  Thanks,

Alex

Re: [Qemu-devel] [RFC v4 8/9] vfio-pci: pass the aer error to guest

[Alex Williamson]

On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:
> when the vfio device encounters an uncorrectable error in host,
> the vfio_pci driver will signal the eventfd registered by this
> vfio device, the results in the qemu eventfd handler getting
> invoked.
> 
> this patch is to pass the error to guest and have the guest driver
> recover from the error.
> 
> Signed-off-by: Chen Fan 
> ---
>  hw/vfio/pci.c | 36 ++--
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 5471437..5669c55 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3241,18 +3241,42 @@ static void vfio_put_device(VFIOPCIDevice *vdev)
>  static void vfio_err_notifier_handler(void *opaque)
>  {
>  VFIOPCIDevice *vdev = opaque;
> +PCIDevice *dev = &vdev->pdev;
> +PCIEAERMsg msg = {
> +.severity = 0,
> +.source_id = (pci_bus_num(dev->bus) << 8) | dev->devfn,
> +};
>  
>  if (!event_notifier_test_and_clear(&vdev->err_notifier)) {
>  return;
>  }
>  
> +/* we should read the error details from the real hardware
> + * configuration spaces, here we only need to do is signaling
> + * to guest an uncorrectable error has occurred.
> + */
> +if (pci_is_express(dev) &&
> +pci_bus_is_express(dev->bus) &&
> +dev->exp.aer_cap) {

Isn't simply testing dev->exp.aer_cap sufficient?

> +uint8_t *aer_cap = dev->config + dev->exp.aer_cap;
> +uint32_t uncor_status;
> +bool isfatal;
> +
> +uncor_status = vfio_pci_read_config(dev,
> +   dev->exp.aer_cap + PCI_ERR_UNCOR_STATUS, 4);
> +
> +isfatal = uncor_status & pci_get_long(aer_cap + PCI_ERR_UNCOR_SEVER);
> +
> +msg.severity = isfatal ? PCI_ERR_ROOT_CMD_FATAL_EN :
> + PCI_ERR_ROOT_CMD_NONFATAL_EN;
> +
> +pcie_aer_msg(dev, &msg);
> +return;
> +}
> +
>  /*
> - * TBD. Retrieve the error details and decide what action
> - * needs to be taken. One of the actions could be to pass
> - * the error to the guest and have the guest driver recover
> - * from the error. This requires that PCIe capabilities be
> - * exposed to the guest. For now, we just terminate the
> - * guest to contain the error.
> + * If the aer capability is not exposed to the guest. we just
> + * terminate the guest to contain the error.
>   */
>  
>  error_report("%s(%04x:%02x:%02x.%x) Unrecoverable error detected.  "

Re: [Qemu-devel] [RFC v4 4/9] aer: impove pcie_aer_init to support vfio device

[Alex Williamson]

On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:
> extend pcie_aer_init arguments to adjust vfio device.

Some discussion of why vfio wants this would be useful.


> Signed-off-by: Chen Fan 
> ---
>  hw/pci-bridge/ioh3420.c| 3 ++-
>  hw/pci-bridge/xio3130_downstream.c | 3 ++-
>  hw/pci-bridge/xio3130_upstream.c   | 3 ++-
>  hw/pci/pcie_aer.c  | 7 ---
>  include/hw/pci/pcie_aer.h  | 4 +++-
>  5 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> index cce2fdd..354574f 100644
> --- a/hw/pci-bridge/ioh3420.c
> +++ b/hw/pci-bridge/ioh3420.c
> @@ -129,7 +129,8 @@ static int ioh3420_initfn(PCIDevice *d)
>  goto err_pcie_cap;
>  }
>  pcie_cap_root_init(d);
> -rc = pcie_aer_init(d, IOH_EP_AER_OFFSET);
> +rc = pcie_aer_init(d, PCI_ERR_VER,
> +   IOH_EP_AER_OFFSET, PCI_ERR_SIZEOF);
>  if (rc < 0) {
>  goto err;
>  }
> diff --git a/hw/pci-bridge/xio3130_downstream.c 
> b/hw/pci-bridge/xio3130_downstream.c
> index b3a6479..407f75f 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -92,7 +92,8 @@ static int xio3130_downstream_initfn(PCIDevice *d)
>  goto err_pcie_cap;
>  }
>  pcie_cap_arifwd_init(d);
> -rc = pcie_aer_init(d, XIO3130_AER_OFFSET);
> +rc = pcie_aer_init(d, PCI_ERR_VER,
> +   XIO3130_AER_OFFSET, PCI_ERR_SIZEOF);
>  if (rc < 0) {
>  goto err;
>  }
> diff --git a/hw/pci-bridge/xio3130_upstream.c 
> b/hw/pci-bridge/xio3130_upstream.c
> index eada582..52b130f 100644
> --- a/hw/pci-bridge/xio3130_upstream.c
> +++ b/hw/pci-bridge/xio3130_upstream.c
> @@ -81,7 +81,8 @@ static int xio3130_upstream_initfn(PCIDevice *d)
>  }
>  pcie_cap_flr_init(d);
>  pcie_cap_deverr_init(d);
> -rc = pcie_aer_init(d, XIO3130_AER_OFFSET);
> +rc = pcie_aer_init(d, PCI_ERR_VER,
> +   XIO3130_AER_OFFSET, PCI_ERR_SIZEOF);
>  if (rc < 0) {
>  goto err;
>  }
> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> index ece1487..a76a943 100644
> --- a/hw/pci/pcie_aer.c
> +++ b/hw/pci/pcie_aer.c
> @@ -94,12 +94,13 @@ static void aer_log_clear_all_err(PCIEAERLog *aer_log)
>  aer_log->log_num = 0;
>  }
>  
> -int pcie_aer_init(PCIDevice *dev, uint16_t offset)
> +int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver,
> +  uint16_t offset, uint16_t size)
>  {
>  PCIExpressDevice *exp;
>  
> -pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR, PCI_ERR_VER,
> -offset, PCI_ERR_SIZEOF);
> +pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR,
> +cap_ver, offset, size);
>  exp = &dev->exp;
>  exp->aer_cap = offset;
>  
> diff --git a/include/hw/pci/pcie_aer.h b/include/hw/pci/pcie_aer.h
> index bcac80a..afa074e 100644
> --- a/include/hw/pci/pcie_aer.h
> +++ b/include/hw/pci/pcie_aer.h
> @@ -87,7 +87,9 @@ struct PCIEAERErr {
>  
>  extern const VMStateDescription vmstate_pcie_aer_log;
>  
> -int pcie_aer_init(PCIDevice *dev, uint16_t offset);
> +int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver,
> +  uint16_t offset, uint16_t size);
> +
>  void pcie_aer_exit(PCIDevice *dev);
>  void pcie_aer_write_config(PCIDevice *dev,
> uint32_t addr, uint32_t val, int len);

Re: [Qemu-devel] [RFC v4 6/9] vfio: add 'x-aer' option to disable aer capability

[Alex Williamson]

On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:
> add 'x-aer' option to disable aer capability if user
> want.

I'm generally one to favor using the x- flag, but we need to figure out
if we need to make this be a supported option or not.  We also need to
decide whether we should include your previous patch that toggled this
support based on machine type.  We don't care about migration
compatibility, but users might depend on specific error behavior and we
don't want to break that for existing machine types.  This would include
only the q35 machine types through QEMU 2.3, I believe.

My impression is that it should probably be supported.  There may be
cases where the guest OS does not support AER and we want to be able to
invoke the old vm_stop() behavior.


> Signed-off-by: Chen Fan 
> ---
>  hw/vfio/pci.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index db4ba23..5471437 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -156,6 +156,8 @@ typedef struct VFIOPCIDevice {
>  uint32_t features;
>  #define VFIO_FEATURE_ENABLE_VGA_BIT 0
>  #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
> +#define VFIO_FEATURE_ENABLE_AER_BIT 1
> +#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT)

Bit 1 is already taken by:

47cbe50 vfio-pci: Enable device request notification support

>  int32_t bootindex;
>  uint8_t pm_cap;
>  bool has_vga;
> @@ -2728,6 +2730,10 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t 
> cap_ver,
>  uint32_t severity;
>  int ret;
>  
> +if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
> +return 0;
> +}
> +
>  pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
>  ret = pcie_aer_init(pdev, cap_ver, pos, size);
>  if (ret) {
> @@ -3569,6 +3575,8 @@ static Property vfio_pci_dev_properties[] = {
> intx.mmap_timeout, 1100),
>  DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
>  VFIO_FEATURE_ENABLE_VGA_BIT, false),
> +DEFINE_PROP_BIT("x-aer", VFIOPCIDevice, features,
> +VFIO_FEATURE_ENABLE_AER_BIT, true),
>  DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
>  /*
>   * TODO - support passed fds... is this necessary?

Re: [Qemu-devel] [RFC v4 3/9] vfio: add pcie extanded capability support

[Alex Williamson]

On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:
> For vfio pcie device, we could expose the extanded capability on

s/extanded/extended/

> PCIE bus. in order to avoid config space broken, we introduce
> a copy config for parsing extended caps. and rebuild the pcie
> extended config space.
> 
> Signed-off-by: Chen Fan 
> ---
>  hw/vfio/pci.c | 83 
> ++-
>  1 file changed, 82 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 84e9d99..96cb52b 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2482,6 +2482,21 @@ static uint8_t vfio_std_cap_max_size(PCIDevice *pdev, 
> uint8_t pos)
>  return next - pos;
>  }
>  
> +
> +static uint16_t vfio_ext_cap_max_size(const uint8_t *config, uint16_t pos)
> +{
> +uint16_t tmp, next = PCIE_CONFIG_SPACE_SIZE - 1;
> +
> +for (tmp = PCI_CONFIG_SPACE_SIZE; tmp;
> +tmp = PCI_EXT_CAP_NEXT(pci_get_long(config + tmp))) {
> +if (tmp > pos && tmp < next) {
> +next = tmp;
> +}
> +}
> +
> +return next - pos;
> +}
> +
>  static void vfio_set_word_bits(uint8_t *buf, uint16_t val, uint16_t mask)
>  {
>  pci_set_word(buf, (pci_get_word(buf) & ~mask) | val);
> @@ -2705,16 +2720,82 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, 
> uint8_t pos)
>  return 0;
>  }
>  
> +static int vfio_add_ext_cap(VFIOPCIDevice *vdev, const uint8_t *config,
> +uint16_t pos)
> +{
> +PCIDevice *pdev = &vdev->pdev;
> +uint32_t header;
> +uint16_t cap_id, next, size;
> +uint8_t cap_ver;
> +int ret;
> +
> +header = pci_get_long(config + pos);
> +cap_id = PCI_EXT_CAP_ID(header);
> +cap_ver = PCI_EXT_CAP_VER(header);
> +next = PCI_EXT_CAP_NEXT(header);
> +
> +/*
> + * If it becomes important to configure extended capabilities to their
> + * actual size, use this as the default when it's something we don't
> + * recognize. Since QEMU doesn't actually handle many of the config
> + * accesses, exact size doesn't seem worthwhile.
> + */
> +size = vfio_ext_cap_max_size(config, pos);
> +
> +pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
> +if (pos == PCI_CONFIG_SPACE_SIZE) {
> +/* Begin the rebuild, we should set the next offset zero. */
> +pci_set_long(pdev->config + pos, PCI_EXT_CAP(cap_id, cap_ver, 0));
> +}
> +
> +/* Use emulated header pointer to allow dropping extended caps */
> +pci_set_long(vdev->emulated_config_bits + pos, 0x);
> +
> +if (next) {
> +ret = vfio_add_ext_cap(vdev, config, next);
> +if (ret) {
> +return ret;
> +}
> +}

This recursion seems pointless.  We use it for the standard capabilities
to make the capability list ordering correct and to avoid the config
space copy that is necessary for this version, but I don't see the
advantage to using it here vs a for-loop in the below function.
Recursion is more complicated, so let's only use it if it provides some
benefit.

> +
> +return 0;
> +}
> +
>  static int vfio_add_capabilities(VFIOPCIDevice *vdev)
>  {
>  PCIDevice *pdev = &vdev->pdev;
> +int ret;
> +uint8_t *config;
>  
>  if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST) ||
>  !pdev->config[PCI_CAPABILITY_LIST]) {
>  return 0; /* Nothing to add */
>  }
>  
> -return vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST]);
> +ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST]);
> +if (ret) {
> +return ret;
> +}
> +
> +/* on PCI bus, it doesn't make sense to expose extended capabilities. */
> +if (!pci_bus_is_express(vdev->pdev.bus) ||
> +!pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) {


Don't we need a pci_is_express(pdev) here too?  Also, we already have
pdev->bus to use as an argument, no need to start with the vdev.

> +return 0;
> +}
> +
> +/*
> + * In order to avoid config space broken, here using a copy config to
> + * parse extended capabilitiess.

spelling

> + */
> +config = g_malloc0(vdev->config_size);
> +if (!config) {
> +return -ENOMEM;
> +}

g_malloc can't fail, no need for this error out condition.  It's
strange, but it's the QEMU way.

> +memcpy(config, pdev->config, vdev->config_size);

There's a g_memdup() function

> +ret = vfio_add_ext_cap(vdev, config, PCI_CONFIG_SPACE_SIZE);
> +
> +g_free(config);
> +return ret;
>  }
>  
>  static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)

Re: [Qemu-devel] [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs

[Christoffer Dall]

On Mon, Mar 09, 2015 at 10:31:19PM +0900, Peter Maydell wrote:
> On 9 March 2015 at 21:56, Christoffer Dall  
> wrote:
> > this function, however, is not used only when migration, but should
> > generally cover the case where you want to synchronize QEMU's state into
> > KVM's state, right?  So while it may not be harmful in currently
> > supported use cases, is there ever a situation where (is_a64(env) && el
> > == 0) and env->spsr != banked_spsr[el], and where env->spsr is
> > out-of-date?
> 
> If EL == 0 then you can't access any SPSR, so env->spsr is by
> definition out of date.
> 
ok, never mind then.

-Christoffer

Re: [Qemu-devel] [PATCH 5/8] ioport: Remove unused functions

[Paolo Bonzini]

On 09/03/2015 18:30, Thomas Huth wrote:
> The functions portio_list_destroy() and portio_list_del()
> are not used anywhere, so let's remove them.
> 
> Signed-off-by: Thomas Huth 
> Cc: Paolo Bonzini 

I think it's better to provide a complete API, mimicking memory_region_*
as much as possible, so I'd rather keep these.

Paolo

> ---
>  include/exec/ioport.h |2 --
>  ioport.c  |   24 
>  2 files changed, 0 insertions(+), 26 deletions(-)
> 
> diff --git a/include/exec/ioport.h b/include/exec/ioport.h
> index 3bd6722..218c835 100644
> --- a/include/exec/ioport.h
> +++ b/include/exec/ioport.h
> @@ -71,10 +71,8 @@ void portio_list_init(PortioList *piolist, Object *owner,
>const struct MemoryRegionPortio *callbacks,
>void *opaque, const char *name);
>  void portio_list_set_flush_coalesced(PortioList *piolist);
> -void portio_list_destroy(PortioList *piolist);
>  void portio_list_add(PortioList *piolist,
>   struct MemoryRegion *address_space,
>   uint32_t addr);
> -void portio_list_del(PortioList *piolist);
>  
>  #endif /* IOPORT_H */
> diff --git a/ioport.c b/ioport.c
> index 783a3ae..712b9e2 100644
> --- a/ioport.c
> +++ b/ioport.c
> @@ -147,19 +147,6 @@ void portio_list_set_flush_coalesced(PortioList *piolist)
>  piolist->flush_coalesced_mmio = true;
>  }
>  
> -void portio_list_destroy(PortioList *piolist)
> -{
> -MemoryRegionPortioList *mrpio;
> -unsigned i;
> -
> -for (i = 0; i < piolist->nr; ++i) {
> -mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, 
> mr);
> -object_unparent(OBJECT(&mrpio->mr));
> -g_free(mrpio);
> -}
> -g_free(piolist->regions);
> -}
> -
>  static const MemoryRegionPortio *find_portio(MemoryRegionPortioList *mrpio,
>   uint64_t offset, unsigned size,
>   bool write)
> @@ -290,14 +277,3 @@ void portio_list_add(PortioList *piolist,
>  /* There will always be an open sub-list.  */
>  portio_list_add_1(piolist, pio_start, count, start, off_low, off_high);
>  }
> -
> -void portio_list_del(PortioList *piolist)
> -{
> -MemoryRegionPortioList *mrpio;
> -unsigned i;
> -
> -for (i = 0; i < piolist->nr; ++i) {
> -mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, 
> mr);
> -memory_region_del_subregion(piolist->address_space, &mrpio->mr);
> -}
> -}
> 

Re: [Qemu-devel] [PATCH v2 0/4] scsi: Error reporting cleanups, realize()

[Paolo Bonzini]

On 09/03/2015 19:17, Markus Armbruster wrote:
> Applies cleanly on master now, and passes make check with my "[PATCH
> RFC 1/1] qtest: Add generic PCI device test" applied on top.
> 
> v2:
> - Rebase: PATCH 01 loses the spapr_vscsi.c hunk, because commit
>   28b07e7 converted it to realize().  Commit message updated.  PATCH
>   03 commit message updated to current error messages.  R-bys
>   retained, hope that's okay.
> 
> Markus Armbruster (4):
>   scsi: Clean up duplicated error in legacy if=scsi code
>   hw: Propagate errors through qdev_prop_set_drive()
>   scsi: Improve error reporting for invalid drive property
>   scsi: Convert remaining PCI HBAs to realize()
> 
>  hw/arm/vexpress.c|  6 +++---
>  hw/arm/virt.c|  6 +++---
>  hw/block/pflash_cfi01.c  |  4 ++--
>  hw/block/pflash_cfi02.c  |  4 ++--
>  hw/core/qdev-properties-system.c | 22 ++
>  hw/scsi/esp-pci.c| 28 +++-
>  hw/scsi/lsi53c895a.c | 13 +++--
>  hw/scsi/megasas.c| 12 +++-
>  hw/scsi/scsi-bus.c   |  6 +++---
>  hw/usb/dev-storage.c |  7 +--
>  include/hw/qdev-properties.h |  4 ++--
>  11 files changed, 47 insertions(+), 65 deletions(-)
> 

Applied, thanks.

Paolo

Re: [Qemu-devel] [PATCH v4 for-2.3 00/25] hw/pc: implement multiple primary busses for pc machines

[Marcel Apfelbaum]

On 03/09/2015 06:55 PM, Gerd Hoffmann wrote:

On Mo, 2015-03-09 at 18:26 +0200, Marcel Apfelbaum wrote:

On 03/09/2015 04:19 PM, Gerd Hoffmann wrote:

Hi,


My series is based on commit 09d219a. Try please on top of this commit.


Ok, that works.  Going to play with that now ;)

Good luck! ... and tell me what you think :)
If you need any help with the command line of the pxb device, let me know,.


First thing I've noticed:  You need to define a numa node so you can
pass a valid numa node to the pxb-device.  Guess that is ok as the whole
point of this is to assign pci devices to numa nodes.  More complete
test instructions would be nice though.

Exactly, this is by design. But you can also use it without specifying the NUMA 
node...

A detailed command line would be:

[qemu-bin + storage options]
-bios [seabios-dir]/out/bios.bin -L [seabios-dir]/out/
-m 2G
-object memory-backend-ram,size=1024M,policy=bind,host-nodes=0,id=ram-node0 
-numa node,nodeid=0,cpus=0,memdev=ram-node0
-object 
memory-backend-ram,size=1024M,policy=interleave,host-nodes=0,id=ram-node1 -numa 
node,nodeid=1,cpus=1,memdev=ram-node1
-device pxb-device,id=bridge1,bus=pci.0,numa_node=1,bus_nr=4 -netdev 
user,id=nd-device e1000,bus=bridge1,addr=0x4,netdev=nd
-device pxb-device,id=bridge2,bus=pci.0,numa_node=0,bus_nr=8 -device 
e1000,bus=bridge2,addr=0x3
-device pxb-device,id=bridge3,bus=pci.0,bus_nr=40 -drive 
if=none,id=drive0,file=[img] -device 
virtio-blk-pci,drive=drive0,scsi=off,bus=bridge3,addr=1

Here you have:
 - 2 NUMA nodes for the guest, 0 and 1. (both mapped to the same NUMA node in 
host, but you can and should put it in different host NUMA nodes)
 - a pxb host bridge attached to NUMA 1 with an e1000 behind it
 - a pxb host bridge attached to NUMA 0 with an e1000 behind it
 - a pxb host bridge not attached to any NUMA with a hard drive behind it.

As you can see, since you already "decide" NUMA mapping at command line, it is 
"natural" also to attach the pxbs to the NUMA nodes.



Second thing:  Booting with an unpatched seabios has bad effects:

[root@localhost ~]# cat /proc/iomem
-000f : PCI Bus :10
   -0fff : reserved
   1000-0009fbff : System RAM
   0009fc00-0009 : reserved
   000c-000c91ff : Video ROM
   000c9800-000ca1ff : Adapter ROM
   000ca800-000ccbff : Adapter ROM
   000f-000f : reserved
 000f-000f : System ROM
0010-3ffd : System RAM
   0100-0174bde4 : Kernel code
   0174bde5-01d30cff : Kernel data
   01eaa000-0202afff : Kernel bss
3ffe-3fff : reserved
fd00-fdff : :00:02.0
   fd00-fdff : bochs-drm
febc-febd : :00:03.0
   febc-febd : e1000
febf-febf0fff : :00:02.0
   febf-febf0fff : bochs-drm
fec0-fec003ff : IOAPIC 0
fed0-fed003ff : HPET 0
   fed0-fed003ff : PNP0103:00
fee0-fee00fff : Local APIC
feffc000-feff : reserved
fffc- : reserved

"PCI Bus :10" is bogus and "PCI Bus :00" isn't there at all.

Yes, you shouldn't use pxb if you are not using the corresponding SeaBIOS.
However, as I understand we always attach a SeaBIOS binary with a QEMU release,
so we should be OK with this.

And this is the reason I wanted bios support *before* the PXB device 
implementation,
but anyway, even if we have them in the same time, as long as the release
has both pxb and BIOS with pxb support, is OK. (I think...)

I appreciate you looking into this and if you need further assistance
don't hesitate to mail me! :)

Thanks,
Marcel



cheers,
   Gerd

Re: [Qemu-devel] [PATCH] pci: Convert pci_nic_init() to Error to avoid qdev_init()

[Markus Armbruster]

Markus Armbruster  writes:

> qdev_init() is deprecated, and will be removed when its callers have
> been weaned off it.
>
> Signed-off-by: Markus Armbruster 
> ---
> Depends on my "[PATCH 00/10] pci: Partial conversion to realize",
> which is in Michael's latest pull request, and on my "[PATCH v2 2/2]
> pci: Give a few helpers internal linkage".

Applies cleanly to master now (commit 277263e).

Re: [Qemu-devel] [PATCH] virtio-pci: Convert to realize()

[Markus Armbruster]

Markus Armbruster  writes:

> Signed-off-by: Markus Armbruster 
> ---
> Depends on my "[PATCH 00/10] pci: Partial conversion to realize",
> which is in Michael's latest pull request.

Applies cleanly to master now (commit 277263e).

Re: [Qemu-devel] [PATCH] macio: Convert to realize()

[Markus Armbruster]

Alexander Graf  writes:

> On 27.02.15 13:43, Markus Armbruster wrote:
>> Convert device models "macio-oldworld" and "macio-newworld".
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>> Depends on my "[PATCH 00/10] pci: Partial conversion to realize",
>> which is in Michael's latest pull request.
>
> Can you please poke me again when it landed?

Applies cleanly to master now (commit 277263e).

[Qemu-devel] [PATCH v2 1/4] scsi: Clean up duplicated error in legacy if=scsi code

[Markus Armbruster]

Commit a818a4b changed scsi_bus_legacy_handle_cmdline() to report
errors from scsi_bus_legacy_add_drive() with error_report() in
addition to returning them.  That's inappropriate.

Two kinds of callers:

1. realize methods (devices "esp", "virtio-scsi-device" and
   "spapr-vscsi")

   The error object gets passed up the call chain until it gets
   reported again and freed.

   Example:

   $ qemu-system-arm -M virt -S -display none \
   > -drive if=scsi,id=foo,bus=1,file=tmp.qcow2 \
   > -device nec-usb-xhci -device usb-storage,drive=foo \
   > -device virtio-scsi-pci
   qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2: Property 
'scsi-disk.drive' can't take value 'foo', it's in use
   qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2: Setting drive 
property failed
   qemu-system-arm: -device virtio-scsi-pci: Setting drive property failed
   qemu-system-arm: -device virtio-scsi-pci: Device initialization failed
   qemu-system-arm: -device virtio-scsi-pci: Device 'virtio-scsi-pci' could not 
be initialized

   The second message in this error cascade comes from
   scsi_bus_legacy_handle_cmdline().  The error object then gets
   passed up to the qdev_init() called from
   virtio_scsi_pci_init_pci(), which reports it again.

2. init methods (devices "am53c974", "dc390", "lsi53c895a",
   "lsi53c810", "megasas", "megasas-gen2")

   init methods need to report their errors with qerror_report().
   These don't.  The inappropriate error_report() papers over the bug.

   error_report() isn't the same as qerror_report() in QMP context,
   but this can't actually happen: QMP can still only hot-plug, and
   callers call scsi_bus_legacy_handle_cmdline() only on cold-plug.
   Except for sysbus_esp_realize(), but that can't be hot-plugged at
   all, as far as I can tell.

Fix the init methods and drop the inappropriate error_report() in
scsi_bus_legacy_handle_cmdline().

Signed-off-by: Markus Armbruster 
Reviewed-by: Peter Crosthwaite 
---
 hw/scsi/esp-pci.c| 2 ++
 hw/scsi/lsi53c895a.c | 3 ++-
 hw/scsi/megasas.c| 2 ++
 hw/scsi/scsi-bus.c   | 1 -
 4 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
index 00b7297..a75fcfa 100644
--- a/hw/scsi/esp-pci.c
+++ b/hw/scsi/esp-pci.c
@@ -28,6 +28,7 @@
 #include "hw/scsi/esp.h"
 #include "trace.h"
 #include "qemu/log.h"
+#include "qapi/qmp/qerror.h"
 
 #define TYPE_AM53C974_DEVICE "am53c974"
 
@@ -369,6 +370,7 @@ static int esp_pci_scsi_init(PCIDevice *dev)
 if (!d->hotplugged) {
 scsi_bus_legacy_handle_cmdline(&s->bus, &err);
 if (err != NULL) {
+qerror_report_err(err);
 error_free(err);
 return -1;
 }
diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index db7d4b8..4ffab03 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -19,7 +19,7 @@
 #include "hw/pci/pci.h"
 #include "hw/scsi/scsi.h"
 #include "sysemu/dma.h"
-#include "qemu/error-report.h"
+#include "qapi/qmp/qerror.h"
 
 //#define DEBUG_LSI
 //#define DEBUG_LSI_REG
@@ -2119,6 +2119,7 @@ static int lsi_scsi_init(PCIDevice *dev)
 if (!d->hotplugged) {
 scsi_bus_legacy_handle_cmdline(&s->bus, &err);
 if (err != NULL) {
+qerror_report_err(err);
 error_free(err);
 return -1;
 }
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 4852237..69ffdbd 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -28,6 +28,7 @@
 #include "hw/scsi/scsi.h"
 #include "block/scsi.h"
 #include "trace.h"
+#include "qapi/qmp/qerror.h"
 
 #include "mfi.h"
 
@@ -2409,6 +2410,7 @@ static int megasas_scsi_init(PCIDevice *dev)
 if (!d->hotplugged) {
 scsi_bus_legacy_handle_cmdline(&s->bus, &err);
 if (err != NULL) {
+qerror_report_err(err);
 error_free(err);
 return -1;
 }
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index dca9576..f8de569 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -273,7 +273,6 @@ void scsi_bus_legacy_handle_cmdline(SCSIBus *bus, Error 
**errp)
 scsi_bus_legacy_add_drive(bus, blk_by_legacy_dinfo(dinfo),
   unit, false, -1, NULL, &err);
 if (err != NULL) {
-error_report("%s", error_get_pretty(err));
 error_propagate(errp, err);
 break;
 }
-- 
1.9.3

[Qemu-devel] [PATCH v2 4/4] scsi: Convert remaining PCI HBAs to realize()

[Markus Armbruster]

These are "am53c974", "dc390", "lsi53c895a", "lsi53c810", "megasas",
"megasas-gen2".

Signed-off-by: Markus Armbruster 
Reviewed-by: Peter Crosthwaite 
---
 hw/scsi/esp-pci.c| 30 +++---
 hw/scsi/lsi53c895a.c | 14 +++---
 hw/scsi/megasas.c| 14 +++---
 3 files changed, 17 insertions(+), 41 deletions(-)

diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
index a75fcfa..8d2242d 100644
--- a/hw/scsi/esp-pci.c
+++ b/hw/scsi/esp-pci.c
@@ -28,7 +28,6 @@
 #include "hw/scsi/esp.h"
 #include "trace.h"
 #include "qemu/log.h"
-#include "qapi/qmp/qerror.h"
 
 #define TYPE_AM53C974_DEVICE "am53c974"
 
@@ -343,13 +342,12 @@ static const struct SCSIBusInfo esp_pci_scsi_info = {
 .cancel = esp_request_cancelled,
 };
 
-static int esp_pci_scsi_init(PCIDevice *dev)
+static void esp_pci_scsi_realize(PCIDevice *dev, Error **errp)
 {
 PCIESPState *pci = PCI_ESP(dev);
 DeviceState *d = DEVICE(dev);
 ESPState *s = &pci->esp;
 uint8_t *pci_conf;
-Error *err = NULL;
 
 pci_conf = dev->config;
 
@@ -368,14 +366,8 @@ static int esp_pci_scsi_init(PCIDevice *dev)
 
 scsi_bus_new(&s->bus, sizeof(s->bus), d, &esp_pci_scsi_info, NULL);
 if (!d->hotplugged) {
-scsi_bus_legacy_handle_cmdline(&s->bus, &err);
-if (err != NULL) {
-qerror_report_err(err);
-error_free(err);
-return -1;
-}
+scsi_bus_legacy_handle_cmdline(&s->bus, errp);
 }
-return 0;
 }
 
 static void esp_pci_scsi_uninit(PCIDevice *d)
@@ -390,7 +382,7 @@ static void esp_pci_class_init(ObjectClass *klass, void 
*data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-k->init = esp_pci_scsi_init;
+k->realize = esp_pci_scsi_realize;
 k->exit = esp_pci_scsi_uninit;
 k->vendor_id = PCI_VENDOR_ID_AMD;
 k->device_id = PCI_DEVICE_ID_AMD_SCSI;
@@ -468,17 +460,19 @@ static void dc390_write_config(PCIDevice *dev,
 }
 }
 
-static int dc390_scsi_init(PCIDevice *dev)
+static void dc390_scsi_realize(PCIDevice *dev, Error **errp)
 {
 DC390State *pci = DC390(dev);
+Error *err = NULL;
 uint8_t *contents;
 uint16_t chksum = 0;
-int i, ret;
+int i;
 
 /* init base class */
-ret = esp_pci_scsi_init(dev);
-if (ret < 0) {
-return ret;
+esp_pci_scsi_realize(dev, &err);
+if (err) {
+error_propagate(errp, err);
+return;
 }
 
 /* EEPROM */
@@ -505,8 +499,6 @@ static int dc390_scsi_init(PCIDevice *dev)
 chksum = 0x1234 - chksum;
 contents[EE_CHKSUM1] = chksum & 0xff;
 contents[EE_CHKSUM2] = chksum >> 8;
-
-return 0;
 }
 
 static void dc390_class_init(ObjectClass *klass, void *data)
@@ -514,7 +506,7 @@ static void dc390_class_init(ObjectClass *klass, void *data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-k->init = dc390_scsi_init;
+k->realize = dc390_scsi_realize;
 k->config_read = dc390_read_config;
 k->config_write = dc390_write_config;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 4ffab03..c5b0cc5 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -19,7 +19,6 @@
 #include "hw/pci/pci.h"
 #include "hw/scsi/scsi.h"
 #include "sysemu/dma.h"
-#include "qapi/qmp/qerror.h"
 
 //#define DEBUG_LSI
 //#define DEBUG_LSI_REG
@@ -2089,12 +2088,11 @@ static const struct SCSIBusInfo lsi_scsi_info = {
 .cancel = lsi_request_cancelled
 };
 
-static int lsi_scsi_init(PCIDevice *dev)
+static void lsi_scsi_realize(PCIDevice *dev, Error **errp)
 {
 LSIState *s = LSI53C895A(dev);
 DeviceState *d = DEVICE(dev);
 uint8_t *pci_conf;
-Error *err = NULL;
 
 pci_conf = dev->config;
 
@@ -2117,14 +2115,8 @@ static int lsi_scsi_init(PCIDevice *dev)
 
 scsi_bus_new(&s->bus, sizeof(s->bus), d, &lsi_scsi_info, NULL);
 if (!d->hotplugged) {
-scsi_bus_legacy_handle_cmdline(&s->bus, &err);
-if (err != NULL) {
-qerror_report_err(err);
-error_free(err);
-return -1;
-}
+scsi_bus_legacy_handle_cmdline(&s->bus, errp);
 }
-return 0;
 }
 
 static void lsi_class_init(ObjectClass *klass, void *data)
@@ -2132,7 +2124,7 @@ static void lsi_class_init(ObjectClass *klass, void *data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-k->init = lsi_scsi_init;
+k->realize = lsi_scsi_realize;
 k->vendor_id = PCI_VENDOR_ID_LSI_LOGIC;
 k->device_id = PCI_DEVICE_ID_LSI_53C895A;
 k->class_id = PCI_CLASS_STORAGE_SCSI;
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 69ffdbd..bf83b65 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -28,7 +28,6 @@
 #include "hw/scsi/scsi.h"
 #include "block/scsi.h"
 #include "trace.h"
-#include "qapi/qmp/qerror.h"
 
 #include "mfi.h"
 
@@ -2321,14 +2320,13 @@ static const st

[Qemu-devel] [PATCH v2 0/4] scsi: Error reporting cleanups, realize()

[Markus Armbruster]

Applies cleanly on master now, and passes make check with my "[PATCH
RFC 1/1] qtest: Add generic PCI device test" applied on top.

v2:
- Rebase: PATCH 01 loses the spapr_vscsi.c hunk, because commit
  28b07e7 converted it to realize().  Commit message updated.  PATCH
  03 commit message updated to current error messages.  R-bys
  retained, hope that's okay.

Markus Armbruster (4):
  scsi: Clean up duplicated error in legacy if=scsi code
  hw: Propagate errors through qdev_prop_set_drive()
  scsi: Improve error reporting for invalid drive property
  scsi: Convert remaining PCI HBAs to realize()

 hw/arm/vexpress.c|  6 +++---
 hw/arm/virt.c|  6 +++---
 hw/block/pflash_cfi01.c  |  4 ++--
 hw/block/pflash_cfi02.c  |  4 ++--
 hw/core/qdev-properties-system.c | 22 ++
 hw/scsi/esp-pci.c| 28 +++-
 hw/scsi/lsi53c895a.c | 13 +++--
 hw/scsi/megasas.c| 12 +++-
 hw/scsi/scsi-bus.c   |  6 +++---
 hw/usb/dev-storage.c |  7 +--
 include/hw/qdev-properties.h |  4 ++--
 11 files changed, 47 insertions(+), 65 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 2/4] hw: Propagate errors through qdev_prop_set_drive()

[Markus Armbruster]

Three kinds of callers:

1. On failure, report the error and abort

   Passing &error_abort does the job.  No functional change.

2. On failure, report the error and exit()

   This is qdev_prop_set_drive_nofail().  Error reporting moves from
   qdev_prop_set_drive() to its caller.  Because hiding away the error
   in the monitor right before exit() isn't helpful, replace
   qerror_report_err() by error_report_err().  Shouldn't make a
   difference, because qdev_prop_set_drive_nofail() should never be
   used in QMP context.

3. On failure, report the error and recover

   This is usb_msd_init() and scsi_bus_legacy_add_drive().  Error
   reporting and freeing the error object moves from
   qdev_prop_set_drive() to its callers.

   Because usb_msd_init() can't run in QMP context, replace
   qerror_report_err() by error_report_err() there.

   No functional change.

   scsi_bus_legacy_add_drive() calling qerror_report_err() is of
   course inappropriate, but this commit merely makes it more obvious.
   The next one will clean it up.

Signed-off-by: Markus Armbruster 
Reviewed-by: Peter Crosthwaite 
---
 hw/arm/vexpress.c|  6 +++---
 hw/arm/virt.c|  6 +++---
 hw/block/pflash_cfi01.c  |  4 ++--
 hw/block/pflash_cfi02.c  |  4 ++--
 hw/core/qdev-properties-system.c | 22 ++
 hw/scsi/scsi-bus.c   |  6 +-
 hw/usb/dev-storage.c |  7 +--
 include/hw/qdev-properties.h |  4 ++--
 8 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 5933454..8496c16 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -515,9 +515,9 @@ static pflash_t *ve_pflash_cfi01_register(hwaddr base, 
const char *name,
 {
 DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
 
-if (di && qdev_prop_set_drive(dev, "drive",
-  blk_by_legacy_dinfo(di))) {
-abort();
+if (di) {
+qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(di),
+&error_abort);
 }
 
 qdev_prop_set_uint32(dev, "num-blocks",
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 69f51ac..93b7605 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -522,9 +522,9 @@ static void create_one_flash(const char *name, hwaddr 
flashbase,
 DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
 const uint64_t sectorlength = 256 * 1024;
 
-if (dinfo && qdev_prop_set_drive(dev, "drive",
- blk_by_legacy_dinfo(dinfo))) {
-abort();
+if (dinfo) {
+qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo),
+&error_abort);
 }
 
 qdev_prop_set_uint32(dev, "num-blocks", flashsize / sectorlength);
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 89d380e..d282695 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -969,8 +969,8 @@ pflash_t *pflash_cfi01_register(hwaddr base,
 {
 DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH01);
 
-if (blk && qdev_prop_set_drive(dev, "drive", blk)) {
-abort();
+if (blk) {
+qdev_prop_set_drive(dev, "drive", blk, &error_abort);
 }
 qdev_prop_set_uint32(dev, "num-blocks", nb_blocs);
 qdev_prop_set_uint64(dev, "sector-length", sector_len);
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 389b4aa..074a005 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -773,8 +773,8 @@ pflash_t *pflash_cfi02_register(hwaddr base,
 {
 DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH02);
 
-if (blk && qdev_prop_set_drive(dev, "drive", blk)) {
-abort();
+if (blk) {
+qdev_prop_set_drive(dev, "drive", blk, &error_abort);
 }
 qdev_prop_set_uint32(dev, "num-blocks", nb_blocs);
 qdev_prop_set_uint32(dev, "sector-length", sector_len);
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index a2e44bd..c413226 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -341,27 +341,25 @@ PropertyInfo qdev_prop_vlan = {
 .set   = set_vlan,
 };
 
-int qdev_prop_set_drive(DeviceState *dev, const char *name,
-BlockBackend *value)
+void qdev_prop_set_drive(DeviceState *dev, const char *name,
+ BlockBackend *value, Error **errp)
 {
-Error *err = NULL;
-object_property_set_str(OBJECT(dev),
-value ? blk_name(value) : "", name, &err);
-if (err) {
-qerror_report_err(err);
-error_free(err);
-return -1;
-}
-return 0;
+object_property_set_str(OBJECT(dev), value ? blk_name(value) : "",
+name, errp);
 }
 
 void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name,
 BlockBackend *value)
 {
-if (qdev_prop_set_drive(dev, name, value) < 0) {
+  

[Qemu-devel] [PATCH v2 3/4] scsi: Improve error reporting for invalid drive property

[Markus Armbruster]

When setting "realized" fails, scsi_bus_legacy_add_drive() passes the
error to qerror_report_err(), then returns an unspecific "Setting
drive property failed" error, which is reported further up the call
chain.

Example:

$ qemu-system-x86_64 -nodefaults -S -display none \
> -drive if=scsi,id=foo,file=tmp.qcow2 -global isa-fdc.driveA=foo
qemu-system-x86_64: -drive if=scsi,id=foo,file=tmp.qcow2: Property 
'scsi-disk.drive' can't take value 'foo', it's in use
qemu-system-x86_64: Setting drive property failed
qemu-system-x86_64: Initialization of device lsi53c895a failed: Device 
initialization failed

Clean up the obvious way: simply return the original error to the
caller.  Gets rid of the second message in the above error cascade.

Signed-off-by: Markus Armbruster 
Reviewed-by: Peter Crosthwaite 
---
 hw/scsi/scsi-bus.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 61c595f..bd2c0e4 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -7,7 +7,6 @@
 #include "sysemu/blockdev.h"
 #include "trace.h"
 #include "sysemu/dma.h"
-#include "qapi/qmp/qerror.h"
 
 static char *scsibus_get_dev_path(DeviceState *dev);
 static char *scsibus_get_fw_dev_path(DeviceState *dev);
@@ -245,9 +244,7 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, 
BlockBackend *blk,
 }
 qdev_prop_set_drive(dev, "drive", blk, &err);
 if (err) {
-qerror_report_err(err);
-error_free(err);
-error_setg(errp, "Setting drive property failed");
+error_propagate(errp, err);
 object_unparent(OBJECT(dev));
 return NULL;
 }
-- 
1.9.3