Re: [Qemu-devel] apparently missing yet another notify_event()

[Michael Tokarev]

On 04.09.2012 10:53, Paolo Bonzini wrote:
> Il 03/09/2012 20:13, Michael Tokarev ha scritto:
[]
>>  qemu -nographic -kernel /boot/vmlinuz-$(uname -r) -append console=ttyS0 
>> -serial pty
>>
>> This will hang with 100% CPU usage until something is sent
>> to the pty.   key is enough.
[]
> Could it be this one?
> 
> 

Removing this "pending" initialization fixes this problem too.

Thanks,

/mjt

Re: [Qemu-devel] apparently missing yet another notify_event()

[Paolo Bonzini]

Il 03/09/2012 20:13, Michael Tokarev ha scritto:
> There's a new bugreport filed against qemu-kvm in debian,
> which looks very similar to what we already had before --
> https://bugs.launchpad.net/qemu/+bug/1021649
> which were fixed by adding qemu_notify_event() call.
> Later on these qemu_notify_event() calls become unnecessary
> as far as I remember.
> 
> But here is a new one.
> 
>  qemu -nographic -kernel /boot/vmlinuz-$(uname -r) -append console=ttyS0 
> -serial pty
> 
> This will hang with 100% CPU usage until something is sent
> to the pty.   key is enough.
> 
> It does the same on qemu and kvm, with and without -enable-kvm,
> and it looks pretty much like another forgotten notify_event.
> And it happens on 1.1 and 1.2-tobe (today qemu/master), so should
> be fixed before 1.2 is released.
> 
> Note: libvirt uses pty-redirected serial port heavily, and this
> is where the users are hit, exactly.
> 
> The longer -serial specification (-chardev pty, -device isa-serial)
> triggers it too.
> 
> What's missing this time?

Could it be this one?



Paolo

Re: [Qemu-devel] [PATCH 1/6] libqblock APIs

[Paolo Bonzini]

Il 04/09/2012 05:15, Wenchao Xia ha scritto:
>>
>> Can you use GError instead?
>>
>   read through the GError doc, GError is defined as following:
> struct GError {
>   GQuark   domain;
>   gint code;
>   gchar   *message;
> };
>   I am worried about the message member, I guess program would be
> aborted if OOM, which I was tring to avoid, so I used char err_msg[1024]
> in my code, and make things simpler.

That's true.  On the other hand, and IMHO, not aborting in the library
code is a non-goal as long as the rest of the block layer still does.

>>>3 QBlockInfoImageStatic. Now it is not folded with location and
>>> format.
>>
>> What does "Static" mean?
>>
>  It is about sorting the information into following kinds:
> 1) static. It is values that defined at creating time/modifying time,
> mostly some settings, and it would not be automatically changed in I/O.
> 2) dynamic. Some information that would changes in I/O and other
> operations, such as allocated_size, snapshots.
> 3) statistics.
>   Now only static one is provided, so I added _static suffix.

Makes sense, thanks for the clarification.  Perhaps QBlockStaticInfo is
a shorter and simpler name?

Paolo

Re: [Qemu-devel] [PATCH 1/6] libqblock APIs

[Wenchao Xia]

Thank u for the careful reviewing of my codes, I will write down
the typo errors you mentioned on a note.


On 09/03/2012 03:18 AM, Wenchao Xia wrote:

   This patch contains the major APIs in the library.
Important APIs:
   1 QBroker. These structure was used to retrieve errors, every thread must
create one first, Later maybe thread related staff could be added into it.
   2 QBlockState. It stands for an block image object.
   3 QBlockInfoImageStatic. Now it is not folded with location and format.
   4 ABI was kept with reserved members.

structure, because it would cause caller more codes to find one member.


Commit message snafu?


 a wrong paste, sorry.


+/**
+ * libqblock_init: Initialize the library
+ */
+void libqblock_init(void);


Is this function safe to call more than once?  Even tighter, is it safe
to call this function simultaneously from multiple threads?


  No, it should be only called once, any other thread should not call
it again, will document it. About the multiple thread user case, qemu
block layer can't support that now, will fix that later.


+
+/**
+ * qb_broker_new: allocate a new broker
+ *
+ * Broker is used to pass operation to libqblock, and got feed back from it.


s/got feed back/get feedback/


+ *
+ * Returns 0 on success, negative value on fail.


Is there any particular interpretation to this negative value, such as
negative errno constant, or always -1?


  Yes, negative values are defined with macros in the header file.


+
+/**
+ * qb_state_new: allocate a new QBloctState struct


s/Bloct/Block/


+ *
+ * Following qblock action were based on this struct


Didn't read well.  Did you mean:

Subsequent qblock actions will use this struct


  Yes.


+ *
+ * Returns 0 if succeed, negative value on fail.


Again, is there any particular meaning to which negative value is returned?


+
+/**
+ * qb_open: open a block object.
+ *
+ * return 0 on success, negative on fail.
+ *
+ * @broker: operation broker.
+ * @qbs: pointer to struct QBlockState.
+ * @loc: location options for open, how to find the image.
+ * @fmt: format options, how to extract the data, only valid member now is
+ fmt->fmt_type, set NULL if you want auto discovery the format.


set to NULL if you want to auto-discover the format

Maybe also add a warning about the inherent security risks of attempting
format auto-discovery (any raw image must NOT be probed, as the raw
image can emulate any other format and cause qemu to chase down chains
where it should not).


  it seems qemu-img could find out that an image is raw correctly by
probing, do you mean give a warning saying that this image is probably
some formats that qemu do not supported, such as virtual box's image?


+ * @flag: behavior control flags.


What flags are currently defined?


  It is the flags defined in the header file, such as LIBQBLOCK_O_RDWR,
will document it.


+ */
+int qb_open(struct QBroker *broker,
+struct QBlockState *qbs,
+struct QBlockOptionLoc *loc,
+struct QBlockOptionFormat *fmt,
+int flag);
+
+/**
+ * qb_close: close a block object.
+ *
+ * qb_flush is automaticlly done inside.


s/automaticlly/automatically/


+/**
+ * qb_create: create a block image or object.
+ *
+ * Note: Create operation would not open the image automatically.
+ *
+ * return negative on fail, 0 on success.
+ *
+ * @broker: operation broker.
+ * @qbs: pointer to struct QBlockState.
+ * @loc: location options for open, how to find the image.
+ * @fmt: format options, how to extract the data.
+ * @flag: behavior control flags.


Again, what flags are defined?


+
+/* sync access */
+/**
+ * qb_read: block sync read.
+ *
+ * return negative on fail, 0 on success.


Shouldn't this instead return the number of successfully read bytes, to
allow for short reads if offset exceeds end-of-file?  If so, should it
return ssize_t instead of int?


  I had this idea before too, let'me check if qemu block layer can
provide this functionality,


+ *
+ * @broker: operation broker.
+ * @qbs: pointer to struct QBlockState.
+ * @buf: buffer that receive the content.


s/receive/receives/


+ * @len: length to read.


Is there a magic length for reading the entire file in one go?


  no, if so where should I put with the result.


+ * @offset: offset in the block data.
+ */
+int qb_read(struct QBroker *broker,
+struct QBlockState *qbs,
+const void *buf,
+size_t len,
+off_t offset);


You do realize that size_t and off_t are not necessarily the same width;
but I'm okay with limiting to size_t reads.


+/**
+ * qb_write: block sync write.
+ *
+ * return negative on fail, 0 on success.


Again, this should probably return number of successfully written bytes,
as an ssize_t.


+ *
+ * @broker: operation broker.
+ * @qbs: pointer to struct QBlockState.
+ * @buf: buffer that receive the content.


s/receive/supplies/


+/* advance image APIs */
+/**
+ * qb_is_allocated: check if

[Qemu-devel] [PATCH (stable, 1.2)] add missing pty_chr_update_read_handler() in qemu_chr_open_pty()

[Michael Tokarev]

Currently pty code does not register i/o handler properly, so that
one have to "ping" the pty in order for qemu to work, or else it
is sitting in main loop doing nothing and using 100% CPU.

 qemu -nographic -kernel /boot/vmlinuz-$(uname -r) -append console=ttyS0 
-serial pty

shows this nicely: the process is eating 100% CPU until someone
connects to the pty in question and sends a char to it.

Fix this by adding a call to pty_chr_update_read_handler() into
qemu_chr_open_pty().  I'm not sure whenever this is the right
thing to do, but at least it fixes the hang for me.

Signed-off-by: Michael Tokarev 
---
 qemu-char.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/qemu-char.c b/qemu-char.c
index 398baf1..35a58bf 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1015,6 +1015,7 @@ static CharDriverState *qemu_chr_open_pty(QemuOpts *opts)
 
 s->fd = master_fd;
 s->timer = qemu_new_timer_ms(rt_clock, pty_chr_timer, chr);
+pty_chr_update_read_handler(chr);
 
 return chr;
 }
-- 
1.7.10.4

Re: [Qemu-devel] boot device order has no effect for virtio-scsi devices

[Paolo Bonzini]

Il 04/09/2012 02:03, ching ha scritto:
> i add boot order and the virtual machine still boot from hard disk instead of 
> cd-rom
> 
> 
>   
>   
>   
>   
>   
> 
> 
>   
>   
>   
>   
>   
>   
> 
> 
> here is the captured command line generated by libvirt (uuid and mac address 
> are masked)

Ah, ok.  libvirt for now supports only booting from LUN 0.  You can use
multiple targets instead of multiple units.

Paolo

[Qemu-devel] [PATCH] target-cris: Fix buffer overflow

[Stefan Weil]

Report from smatch:

target-cris/translate.c:3464 cpu_dump_state(32) error:
 buffer overflow 'env->sregs' 4 <= 255

sregs is declared 'uint32_t sregs[4][16]', so the first index must be
less than 4.

Signed-off-by: Stefan Weil 
---

I did not fix tabs, therefore checkpatch.pl reports an error.

 target-cris/translate.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-cris/translate.c b/target-cris/translate.c
index 1ad9ec7..34c0452 100644
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -3458,7 +3458,7 @@ void cpu_dump_state (CPUCRISState *env, FILE *f, 
fprintf_function cpu_fprintf,
}
srs = env->pregs[PR_SRS];
cpu_fprintf(f, "\nsupport function regs bank %x:\n", srs);
-   if (srs < 256) {
+   if (srs < 4) {
for (i = 0; i < 16; i++) {
cpu_fprintf(f, "s%2.2d=%8.8x ",
i, env->sregs[srs][i]);
-- 
1.7.10

[Qemu-devel] [PATCH] target-arm: Fix potential buffer overflow

[Stefan Weil]

Report from smatch:

target-arm/helper.c:651 arm946_prbs_read(6) error:
 buffer overflow 'env->cp15.c6_region' 8 <= 8
target-arm/helper.c:661 arm946_prbs_write(6) error:
 buffer overflow 'env->cp15.c6_region' 8 <= 8

c7_region is an array with 8 elements, so the index must be less than 8.

Signed-off-by: Stefan Weil 
---
 target-arm/helper.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index dceaa95..e27df96 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -645,7 +645,7 @@ static int pmsav5_insn_ap_read(CPUARMState *env, const 
ARMCPRegInfo *ri,
 static int arm946_prbs_read(CPUARMState *env, const ARMCPRegInfo *ri,
 uint64_t *value)
 {
-if (ri->crm > 8) {
+if (ri->crm >= 8) {
 return EXCP_UDEF;
 }
 *value = env->cp15.c6_region[ri->crm];
@@ -655,7 +655,7 @@ static int arm946_prbs_read(CPUARMState *env, const 
ARMCPRegInfo *ri,
 static int arm946_prbs_write(CPUARMState *env, const ARMCPRegInfo *ri,
  uint64_t value)
 {
-if (ri->crm > 8) {
+if (ri->crm >= 8) {
 return EXCP_UDEF;
 }
 env->cp15.c6_region[ri->crm] = value;
-- 
1.7.10

[Qemu-devel] [KVM][Kemari]:Kemari slows down the VM user experience

[Harshita]

Hello,

As part of implementing Fault Tolerant solution, we are exploring
Kemari. In the process of understanding it, we tested the branch "next"
of git repository. It is working fine. But, once we trigger sync
command, VM response is very slow for the actions performed.

Observation:

Once sync is started vm is responding slowly. (eg., Mouse operation,
keys pressed are shown on the screen after some delay.)
This behavior continues until sync is completed (eg., killing
guest on the primary or resuming guest on secondary, so that no further
sync), and then the guest responds normally.

Our setup:
--
Host is KVM enabled Linux.
Tested with both Linux and Windows guests.
NFS is used to share the disk image.

Steps followed:
1. Start VM on primary(node1)
qemu-system-x86_64 -hda "/mnt/vm_disks/vm_shared.disk.xm" -m "750"
-monitor stdio --enable-kvm

2. Start VM on secondary(node2), to accept synchronization
qemu-system-x86_64 -hda "/mnt/vm_disks/vm_shared.disk.xm" -m "750"
-monitor stdio --enable-kvm -incoming kemari:tcp:0:

3. Started sync using below command, on primary
migrate -d kemari:tcp:node2:

4. 'c' on the virtual terminal of secondary VM to take over completely.

Can you please let us know if anything missing in the setup or any
inputs to improve the situation.

Regards,
Harshita

Re: [Qemu-devel] [PATCH] cadence_uart: Fix buffer overflow

[Peter Crosthwaite]

Thanks Stefan,

Please enqueue to Trivial.

Regards,
Peter

On Sat, Sep 1, 2012 at 7:12 PM, Stefan Weil  wrote:
> Report from smatch:
> hw/cadence_uart.c:413 uart_read(13) error: buffer overflow 's->r' 18 <= 18
>
> This fixes read access to s->r[R_MAX] which is behind the limits of s->r.
>
> Signed-off-by: Stefan Weil 

Reviewed-by: Peter Crosthwaite 

> ---
>  hw/cadence_uart.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/cadence_uart.c b/hw/cadence_uart.c
> index d98e531..f8afc4e 100644
> --- a/hw/cadence_uart.c
> +++ b/hw/cadence_uart.c
> @@ -404,7 +404,7 @@ static uint64_t uart_read(void *opaque, 
> target_phys_addr_t offset,
>  uint32_t c = 0;
>
>  offset >>= 2;
> -if (offset > R_MAX) {
> +if (offset >= R_MAX) {
>  return 0;
>  } else if (offset == R_TX_RX) {
>  uart_read_rx_fifo(s, &c);
> --
> 1.7.10
>

Re: [Qemu-devel] [PATCH 4/4] kvm: i386: Add classic PCI device assignment

[Alex Williamson]

On Mon, 2012-09-03 at 18:59 +0300, Avi Kivity wrote:
> On 08/29/2012 11:49 AM, Peter Maydell wrote:
> > On 29 August 2012 09:47, Jan Kiszka  wrote:
> >> On 2012-08-28 23:26, Peter Maydell wrote:
> >>> Since this is arch-specific we should probably give the
> >>> resulting device a more specific name than "pci-assign",
> >>> which implies that it is (a) ok for any PCI system and
> >>> (b) not something that will be obsolete in the foreseen
> >>> future.
> >>
> >> The name has to be like this for libvirt compatibility. I can provide it
> >> as alias, though, calling the device "kvm-pci-assign" by default. Better?
> > 
> > ...is it likely to ever work for non-x86 PCI devices?
> 
> It used to work for ia64 before it died.  So it's not architecture
> specific by design.

Caveat; ia64 is a different architecture, but it was on Intel VT-d
chipsets, which therefore supported the IOMMU API and ia64 maps guests
the same way.  This is the same problem I ran into trying to name the
vfio iommu driver for x86.  It's not really x86-specific, but it's got
to look and smell pretty similar.  Thanks,

Alex

Re: [Qemu-devel] [PATCH 4/6] libqblock internal used functions

[Wenchao Xia]

于 2012-9-3 21:18, Paolo Bonzini 写道:

Il 03/09/2012 11:18, Wenchao Xia ha scritto:

   This patch contains internal helper codes.

Signed-off-by: Wenchao Xia 
---
  block.c  |2 +-
  block.h  |1 +
  libqblock/libqblock-helper.c |   92 ++
  libqblock/libqblock-helper.h |   57 ++
  4 files changed, 151 insertions(+), 1 deletions(-)
  create mode 100644 libqblock/libqblock-helper.c
  create mode 100644 libqblock/libqblock-helper.h

diff --git a/block.c b/block.c
index 470bdcc..8b312f8 100644
--- a/block.c
+++ b/block.c
@@ -196,7 +196,7 @@ static void bdrv_io_limits_intercept(BlockDriverState *bs,
  }

  /* check if the path starts with ":" */
-static int path_has_protocol(const char *path)
+int path_has_protocol(const char *path)
  {
  const char *p;

diff --git a/block.h b/block.h
index 2e2be11..e7da711 100644
--- a/block.h
+++ b/block.h
@@ -405,4 +405,5 @@ typedef enum {
  #define BLKDBG_EVENT(bs, evt) bdrv_debug_event(bs, evt)
  void bdrv_debug_event(BlockDriverState *bs, BlkDebugEvent event);

+int path_has_protocol(const char *path);
  #endif
diff --git a/libqblock/libqblock-helper.c b/libqblock/libqblock-helper.c
new file mode 100644
index 000..f9e8ce9
--- /dev/null
+++ b/libqblock/libqblock-helper.c
@@ -0,0 +1,92 @@
+#include "libqblock-helper.h"
+#include "libqblock-types.h"
+#include "libqblock-error.h"
+
+const char *fmt2str(enum QBlockFormat fmt)
+{
+const char *ret = NULL;
+switch (fmt) {
+case QB_FMT_COW:
+ret = "cow";
+break;
+case QB_FMT_QED:
+ret = "qed";
+break;
+case QB_FMT_QCOW:
+ret = "qcow";
+break;
+case QB_FMT_QCOW2:
+ret = "qcow2";
+break;
+case QB_FMT_RAW:
+ret = "raw";
+break;
+case QB_FMT_RBD:
+ret = "rbd";
+break;
+case QB_FMT_SHEEPDOG:
+ret = "sheepdog";
+break;
+case QB_FMT_VDI:
+ret = "vdi";
+break;
+case QB_FMT_VMDK:
+ret = "vmdk";
+break;
+case QB_FMT_VPC:
+ret = "vpc";
+break;
+default:
+break;
+}
+return ret;
+}
+
+enum QBlockFormat str2fmt(const char *fmt)
+{
+enum QBlockFormat ret = QB_FMT_NONE;
+if (0 == strcmp(fmt, "cow")) {
+ret = QB_FMT_COW;
+} else if (0 == strcmp(fmt, "qed")) {
+ret = QB_FMT_QED;
+} else if (0 == strcmp(fmt, "qcow")) {
+ret = QB_FMT_QCOW;
+} else if (0 == strcmp(fmt, "qcow2")) {
+ret = QB_FMT_QCOW2;
+} else if (0 == strcmp(fmt, "raw")) {
+ret = QB_FMT_RAW;
+} else if (0 == strcmp(fmt, "rbd")) {
+ret = QB_FMT_RBD;
+} else if (0 == strcmp(fmt, "sheepdog")) {
+ret = QB_FMT_SHEEPDOG;
+} else if (0 == strcmp(fmt, "vdi")) {
+ret = QB_FMT_VDI;
+} else if (0 == strcmp(fmt, "vmdk")) {
+ret = QB_FMT_VMDK;
+} else if (0 == strcmp(fmt, "vpc")) {
+ret = QB_FMT_VPC;
+}
+return ret;
+}
+
+void set_broker_err(struct QBroker *broker, int err_ret,
+   const char *fmt, ...)
+{
+va_list ap;
+
+broker->err_ret = err_ret;
+if (err_ret == QB_ERR_INTERNAL_ERR) {
+broker->err_no = -errno;
+} else {
+broker->err_no = 0;
+}
+
+va_start(ap, fmt);
+vsnprintf(broker->err_msg, sizeof(broker->err_msg), fmt, ap);
+va_end(ap);
+}
+
+void set_broker_err_nomem(struct QBroker *broker)
+{
+set_broker_err(broker, QB_ERR_MEM_ERR, "No Memory.");
+}
diff --git a/libqblock/libqblock-helper.h b/libqblock/libqblock-helper.h
new file mode 100644
index 000..4330472
--- /dev/null
+++ b/libqblock/libqblock-helper.h
@@ -0,0 +1,57 @@
+/*
+ * QEMU block layer library
+ *
+ * Copyright IBM, Corp. 2012
+ *
+ * Authors:
+ *  Wenchao Xia   
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef LIBQBLOCK_HELPER
+#define LIBQBLOCK_HELPER
+
+#include "block.h"
+#include "block_int.h"
+
+#include "libqblock-types.h"
+
+/* this file contains helper function used internally. */
+#define SECTOR_SIZE (512)
+#define SECTOR_SIZE_MASK (0x01ff)
+#define SECTOR_SIZE_BITS_NUM (9)
+#define FUNC_FREE free
+#define FUNC_MALLOC malloc
+#define FUNC_CALLOC calloc
+
+#define CLEAN_FREE(p) { \
+FUNC_FREE(p); \
+(p) = NULL; \
+}
+
+/* details should be hidden to user */
+struct QBlockState {
+BlockDriverState *bdrvs;
+/* internal used file name now, if it is not NULL, it means
+   image was opened.
+*/
+char *filename;
+};
+
+#define QB_ERR_STRING_SIZE (1024)
+struct QBroker {
+/* last error */
+char err_msg[QB_ERR_STRING_SIZE];
+int err_ret; /* last error return of libqblock. */
+int err_no; /* 2nd level of error, errno what below reports */
+};
+
+const char *fmt2str(enum QBlockFormat fmt);
+enum QBlockFormat str2fmt(cons

Re: [Qemu-devel] [PATCH 1/6] libqblock APIs

[Wenchao Xia]

于 2012-9-3 21:18, Paolo Bonzini 写道:

Il 03/09/2012 11:18, Wenchao Xia ha scritto:

   1 QBroker. These structure was used to retrieve errors, every thread must
create one first, Later maybe thread related staff could be added into it.


Can you use GError instead?


  read through the GError doc, GError is defined as following:
struct GError {
  GQuark   domain;
  gint code;
  gchar   *message;
};
  I am worried about the message member, I guess program would be
aborted if OOM, which I was tring to avoid, so I used char err_msg[1024]
in my code, and make things simpler.


   3 QBlockInfoImageStatic. Now it is not folded with location and format.


What does "Static" mean?


 It is about sorting the information into following kinds:
1) static. It is values that defined at creating time/modifying time,
mostly some settings, and it would not be automatically changed in I/O.
2) dynamic. Some information that would changes in I/O and other
operations, such as allocated_size, snapshots.
3) statistics.
  Now only static one is provided, so I added _static suffix.


Paolo




--
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[liu ping fan]

On Mon, Sep 3, 2012 at 6:06 PM, liu ping fan  wrote:
> On Mon, Sep 3, 2012 at 4:52 PM, Avi Kivity  wrote:
>> On 09/03/2012 10:44 AM, liu ping fan wrote:
>

 If we make the refcount/lock internal to the region, we must remove the
 opaque, since the region won't protect it.

 Replacing the opaque with container_of(mr) doesn't help, since we can't
 refcount mr, only mr->impl.

>>> I think you mean if using MemoryRegionImpl, then at this level, we had
>>> better not touch the refcnt of container_of(mr) and should face the
>>> mr->impl->refcnt. Right?
>>
>> I did not understand the second part, sorry.
>>
> My understanding of "Replacing the opaque with container_of(mr)
> doesn't help, since we can't  refcount mr, only
> mr->impl." is that although Object_ref(container_of(mr)) can help us
> to protect it from disappearing. But apparently it is not right place
> to do it it in memory core.   Do I catch you meaning?
>
 We could externalize the refcounting and push it into device code.  This
 means:

memory_region_init_io(&s->mem, dev)

...

object_ref(dev)
memory_region_add_subregion(..., &dev->mr)

...

memory_region_del_subregion(..., &dev->mr)  // implied flush
object_unref(dev)

>>> I think "object_ref(dev)" just another style to push
>>> MemoryRegionOps::object() to device level.  And I think we can bypass
>>> it.   The caller (unplug, pci-reconfig ) of
>>> memory_region_del_subregion() ensure the @dev is valid.
>>> If the implied flush is implemented in synchronize,  _del_subregion()
>>> will guarantee no reader for @dev->mr and @dev exist any longer.
>>
>> The above code has a deadlock.  memory_region_del_subregion() may be
>> called under the device lock (since it may be the result of mmio to the
>> device), and if the flush uses synchronized_rcu(), it will wait forever
>> for the read-side critical section to complete.
>>
> But if _del_subregion() just wait for mr-X quiescent period, while
> calling in mr-Y's read side critical section, then we can avoid
> deadlock.  I saw in pci-mapping, we delete mr-X in mr-Y read side.
>
>>> So I
>>> think we can save both object_ref/unref(dev) for memory system.
>>> The only problem is that whether we can implement it as synchronous or
>>> not,  is it possible that we can launch a  _del_subregion(mr-X) in
>>> mr-X's dispatcher?
>>
>> Yes.  Real cases exist.
>
> Oh, I find the sample code, then, the deadlock is unavoidable in this method.
>>
>> What alternatives remain?
>>
> I think a way out may be async+refcnt
>
If we consider the relationship of MemoryRegionImpl and device as the
one between file and file->private_data  in Linux. Then the creation
of impl will object_ref(dev) and when impl->ref=0, it will
object_unref(dev)
But this is an async model, for those client which need to know the
end of flush, we can adopt callback just like call_rcu().



> Regards,
> pingfan
>> --
>> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[liu ping fan]

On Mon, Sep 3, 2012 at 6:16 PM, Avi Kivity  wrote:
> On 09/03/2012 01:06 PM, liu ping fan wrote:
>> On Mon, Sep 3, 2012 at 4:52 PM, Avi Kivity  wrote:
>>> On 09/03/2012 10:44 AM, liu ping fan wrote:
>>
>
> If we make the refcount/lock internal to the region, we must remove the
> opaque, since the region won't protect it.
>
> Replacing the opaque with container_of(mr) doesn't help, since we can't
> refcount mr, only mr->impl.
>
 I think you mean if using MemoryRegionImpl, then at this level, we had
 better not touch the refcnt of container_of(mr) and should face the
 mr->impl->refcnt. Right?
>>>
>>> I did not understand the second part, sorry.
>>>
>> My understanding of "Replacing the opaque with container_of(mr)
>> doesn't help, since we can't  refcount mr, only
>> mr->impl." is that although Object_ref(container_of(mr)) can help us
>> to protect it from disappearing. But apparently it is not right place
>> to do it it in memory core.   Do I catch you meaning?
>
> We cannot call container_of(mr) in the memory core, because the
> parameters to container_of() are not known.  But that is not the main issue.
>
> Something we can do is make MemoryRegionOps::object() take a mr
> parameter instead of opaque.  MemoryRegionOps::object() then uses
> container_of() to derive the object to ref.
>
> (works with MemoryRegionOps::ref()/MemoryRegionOps::unref() too; Jan,
> this decouples Object from MemoryRegion at the cost of extra boilerplate
> in client code, but it may be worthwhile as a temporary measure while we
> gain more experience with this)
>
Exactly catch your meaning, thanks.
>>
> We could externalize the refcounting and push it into device code.  This
> means:
>
>memory_region_init_io(&s->mem, dev)
>
>...
>
>object_ref(dev)
>memory_region_add_subregion(..., &dev->mr)
>
>...
>
>memory_region_del_subregion(..., &dev->mr)  // implied flush
>object_unref(dev)
>
 I think "object_ref(dev)" just another style to push
 MemoryRegionOps::object() to device level.  And I think we can bypass
 it.   The caller (unplug, pci-reconfig ) of
 memory_region_del_subregion() ensure the @dev is valid.
 If the implied flush is implemented in synchronize,  _del_subregion()
 will guarantee no reader for @dev->mr and @dev exist any longer.
>>>
>>> The above code has a deadlock.  memory_region_del_subregion() may be
>>> called under the device lock (since it may be the result of mmio to the
>>> device), and if the flush uses synchronized_rcu(), it will wait forever
>>> for the read-side critical section to complete.
>>>
>> But if _del_subregion() just wait for mr-X quiescent period, while
>> calling in mr-Y's read side critical section, then we can avoid
>> deadlock.  I saw in pci-mapping, we delete mr-X in mr-Y read side.
>
> Look at cirrus-vga.c, there are many calls to the memory API there.
> While I doubt that we have one region disabling itself (or a subregion
> of itself), that's all code that would be run under the same device
> lock, and so would deadlock.
>
Oh, yes, quiescent time will never come since reader wait for the
lock! Got it, thanks.

pingfan
>
> --
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [RESEND] [PATCH] Properly use backing file argument to qemu-img convert

[Brad Campbell]

On 03/09/12 22:23, Andreas Färber wrote:

Am 03.09.2012 09:46, schrieb Brad Campbell:

  Converting to an image with an output backing file would write out the 
contents
  of the source image whether or not it was already contained in the new backing
  file. This commit ensures that the source file is checked against the new
  backing file and output is only allocated if it obsoletes or extends data
  already in the supplied backing file.


Signed-off-by: Brad Campbell 


For a resend it would've been nice to get the typos out, I'm sure you
mean qemu-img. :) And it would be nice to have that marked as the topic
then so that it's easily recognized ("qemu-img: Bla bla").


To be honest, with a typo that big I would never have picked it up 
myself no matter how many times I went over it. I'll take that 
suggestion for the topic, thanks.



---
  qemu-img.c |   74 ++-
  1 files changed, 57 insertions(+), 17 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index c8a70ff..3fb6cbf 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -629,11 +629,12 @@ static int img_convert(int argc, char **argv)
  int progress = 0, flags;
  const char *fmt, *out_fmt, *cache, *out_baseimg, *out_filename;
  BlockDriver *drv, *proto_drv;
-BlockDriverState **bs = NULL, *out_bs = NULL;
+BlockDriverState **bs = NULL, *out_bs = NULL, *out_bf = NULL;
  int64_t total_sectors, nb_sectors, sector_num, bs_offset;
-uint64_t bs_sectors;
+uint64_t bf_sectors, bs_sectors;
  uint8_t * buf = NULL;
  const uint8_t *buf1;
+uint8_t * bf_buf = NULL;


Should be *bf_buf despite the * buf line above (checkpatch.pl sometimes
mixes up multiplication and declaration of pointer variables).


I'll correct and re-send.


  BlockDriverInfo bdi;
  QEMUOptionParameter *param = NULL, *create_options = NULL;
  QEMUOptionParameter *out_baseimg_param;

[snip]

Note that the merge window for v1.3 is not yet open so you'll have to be
a bit patient before seeing your patch in qemu.git.


I'm not in any hurry, it's just that the mail config on the machine I 
used git send-email from is so broken that the mail to Kevin bounced, so 
I waited a week before I re-sent it just to avoid bombing the list. As 
it turns out it's still broken anyway.


Thanks for the feedback.

Regards,
Brad

Re: [Qemu-devel] [PATCH 2/6] libqblock public type defines

[Wenchao Xia]

于 2012-9-3 21:13, Paolo Bonzini 写道:

Il 03/09/2012 11:18, Wenchao Xia ha scritto:

+union QBlockOption_fmt {
+struct QBlockOption_fmt_cow   o_cow;
+struct QBlockOption_fmt_qed   o_qed;
+struct QBlockOption_fmt_qcow  o_qcow;
+struct QBlockOption_fmt_qcow2 o_qcow2;
+struct QBlockOption_fmt_raw   o_raw;
+struct QBlockOption_fmt_rbd   o_rbd;
+struct QBlockOption_fmt_sheepdog  o_sheepdog;
+struct QBlockOption_fmt_vdi   o_vdi;
+struct QBlockOption_fmt_vmdk  o_vmdk;
+struct QBlockOption_fmt_vpc   o_vpc;
+};
+
+struct QBlockOptionFormat {
+enum QBlockFormat fmt_type;
+union QBlockOption_fmt fmt_op;
+uint8_t reserved[512];
+};


Padding must be in the union not the struct.  For the fourth time.

Paolo


  I must have left it in some other patches, sorry about this key point
missing.



--
Best Regards

Wenchao Xia

Re: [Qemu-devel] boot device order has no effect for virtio-scsi devices

[ching]

On 09/03/2012 04:24 PM, Paolo Bonzini wrote:
> Il 03/09/2012 00:23, ching ha scritto:
>> have anyone tested the boot order of virtio-scsi devices?
>>
>> 
>> hvm
>> 
>> 
>> 
>>   
>>
>>
>> i try to set the boot order with scsi cd-rom first, then scsi harddisk
>>
>> but the virtual machine will always boot with first scsi device only 
>> (unit='0', the scsi harddisk)
>>
>> is it a known problem?
> Use the bootindex property of  instead.
>
> Paolo
>
>

i add boot order and the virtual machine still boot from hard disk instead of 
cd-rom


  
  
  
  
  


  
  
  
  
  
  


here is the captured command line generated by libvirt (uuid and mac address 
are masked)

/usr/bin/qemu-system-x86_64 -enable-kvm -name Linux -S -M pc-1.1 -cpu 
SandyBridge,+osxsave,+pdcm,+xtpr,+tm2,+est,+smx,+vmx,+ds_cpl,+monitor,+dtes64,+pbe,+tm,+ht,+ss,+acpi,+ds,+vme
 -enable-kvm -m 1024 -smp 2,sockets=2,cores=1,threads=1 -uuid  
-no-user-config -nodefaults -chardev 
socket,id=charmonitor,path=/var/lib/libvirt/qemu/Linux.monitor,server,nowait 
-mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc -no-shutdown 
-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 -device 
virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x4 -device 
virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x5 -drive 
file=/Linux.raw_image,if=none,id=drive-scsi0-0-0-0,format=raw,cache=unsafe,aio=native
 -device 
scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=2
 -drive 
file=/xubuntu-12.04-desktop-amd64.iso,if=none,id=drive-scsi0-0-0-1,readonly=on,format=raw,cache=unsafe,aio=native
 -device
scsi-cd,bus=scsi0.0,channel=0,scsi-id=0,lun=1,drive=drive-scsi0-0-0-1,id=scsi0-0-0-1,bootindex=1
 -netdev tap,fd=18,id=hostnet0,vhost=on,vhostfd=19 -device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=xx:xx:xx:xx:xx:xx,bus=pci.0,addr=0x8 
-chardev pty,id=charserial0 -device isa-serial,chardev=charserial0,id=serial0 
-chardev spicevmc,id=charchannel0,name=vdagent -device 
virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=com.redhat.spice.0
 -spice 
port=5900,addr=127.0.0.1,disable-ticketing,image-compression=off,jpeg-wan-compression=never,zlib-glz-wan-compression=never,playback-compression=off,streaming-video=off
 -vga qxl -global qxl-vga.vram_size=67108864 -device 
intel-hda,id=sound0,bus=pci.0,addr=0x3 -device 
hda-micro,id=sound0-codec0,bus=sound0.0,cad=0 -chardev 
spicevmc,id=charredir0,name=usbredir -device 
usb-redir,chardev=charredir0,id=redir0 -device 
virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6

Re: [Qemu-devel] [PATCH 01/21] target-s390x: fix style

[Alexander Graf]

On 03.09.2012, at 15:10, Blue Swirl  wrote:

> On Mon, Sep 3, 2012 at 4:31 AM, Alexander Graf  wrote:
>> 
>> On 02.09.2012, at 13:33, Blue Swirl wrote:
>> 
>>> Before splitting op_helper.c and helper.c in the next patches,
>>> fix style issues. No functional changes.
>>> 
>>> Replace also GCC specific __FUNCTION__ with
>>> standard __func__.
>>> 
>>> Don't init static variable (cpu_s390x_init:inited) with 0.
>>> 
>>> Signed-off-by: Blue Swirl 
>> 
>> 
>> Phew. Have you hooked up with Richard about these changes? He had quite a 
>> big rewrite of the s390 target in the works, and I don't want you guys to 
>> work needlessly on major conflicts :)
> 
> I missed that one. He's rewriting translate.c, while I'm mostly
> touching op_helper.c. I'd expect it would be simpler for Richard to
> rebase on my series, because they only change helper calls but don't
> otherwise touch code. His series rearranges the code heavily and if I
> rebased on that, it would probably make the patch conflicts harder to
> resolve. What do you think?

I also think that way around makes more sense, but mainly because you posted 
patches while he posted an RFC :).

Either way, will hopefully get around to review and/or apply your patches next 
week :)


Alex

Re: [Qemu-devel] [PATCH] hw/wm8750: Fix potential buffer overflow

[Peter Maydell]

On 3 September 2012 21:56, Stefan Weil  wrote:
> Report from smatch:
>
> hw/wm8750.c:369 wm8750_tx(12) error: buffer overflow 's->i2c_data' 2 <= 2
>
> It looks like the preprocessor statements were simply misplaced.
>
> Replace also __FUNCTION__ by __func__ to please checkpatch.pl.
>
> Signed-off-by: Stefan Weil 

Reviewed-by: Peter Maydell 

-- PMM

Re: [Qemu-devel] [PATCH] kvm: Fix warning from static code analysis

[Peter Maydell]

On 3 September 2012 21:40, Stefan Weil  wrote:
> Report from smatch:
>
> kvm-all.c:1373 kvm_init(135) warn:
>  variable dereferenced before check 's' (see line 1360)
>
> 's' cannot by NULL (it was alloced using g_malloc0), so there is no need
> to check it here.
>
> Signed-off-by: Stefan Weil 

Reviewed-by: Peter Maydell 

-- PMM

Re: [Qemu-devel] [PATCH] slirp: Fix error reported by static code analysis and remove wrong type casts

[Peter Maydell]

On 3 September 2012 21:34, Stefan Weil  wrote:
> Report from smatch:
> slirp/tcp_subr.c:127 tcp_respond(17) error:
>  we previously assumed 'tp' could be null (see line 124)
>
> Fix this by checking 'tp' before reading its elements.
>
> The type casts of pointers to long are not related to the smatch report
> but happened to be near that code. Those type casts are not allowed
> when sizeof(pointer) != sizeof(long).

I think these would be better in a separate patch.
> @@ -124,7 +124,7 @@ tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct 
> mbuf *m,
> if (tp)
> win = sbspace(&tp->t_socket->so_rcv);
>  if (m == NULL) {
> -   if ((m = m_get(tp->t_socket->slirp)) == NULL)
> +   if (tp && (m = m_get(tp->t_socket->slirp)) == NULL)
> return;
> tlen = 0;
> m->m_data += IF_MAXLINKHDR;

This doesn't look quite right -- now if tp is NULL we will skip
the assignment to m and dereference a NULL pointer a few lines
further on, right?

I suspect that we need either to be passed our Slirp* explicitly rather
than via tp, or  we have to enforce that tcp_respond() is called with
a non-NULL struct tcpcb*. I think the only cases where tp can be non-NULL
at the moment are the two calls from the dropwithreset code in tcp_input().

-- PMM

[Qemu-devel] [PATCH] hw/wm8750: Fix potential buffer overflow

[Stefan Weil]

Report from smatch:

hw/wm8750.c:369 wm8750_tx(12) error: buffer overflow 's->i2c_data' 2 <= 2

It looks like the preprocessor statements were simply misplaced.

Replace also __FUNCTION__ by __func__ to please checkpatch.pl.

Signed-off-by: Stefan Weil 
---
 hw/wm8750.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/wm8750.c b/hw/wm8750.c
index 11bcec3..44f138f 100644
--- a/hw/wm8750.c
+++ b/hw/wm8750.c
@@ -361,10 +361,10 @@ static int wm8750_tx(I2CSlave *i2c, uint8_t data)
 uint16_t value;
 
 if (s->i2c_len >= 2) {
-printf("%s: long message (%i bytes)\n", __FUNCTION__, s->i2c_len);
 #ifdef VERBOSE
-return 1;
+printf("%s: long message (%i bytes)\n", __func__, s->i2c_len);
 #endif
+return 1;
 }
 s->i2c_data[s->i2c_len ++] = data;
 if (s->i2c_len != 2)
-- 
1.7.10

Re: [Qemu-devel] [PATCH] json-parser: Fix potential NULL pointer segfault

[Luiz Capitulino]

On Mon,  3 Sep 2012 21:19:11 +0200
Stefan Weil  wrote:

> Report from smatch:
> json-parser.c:474 parse_object(62) error: potential null derefence 'dict'.
> json-parser.c:553 parse_array(75) error: potential null derefence 'list'.
> 
> Label 'out' in json-parser.c can be called with list == NULL
> which is passed to QDECREF.
> 
> Modify QDECREF to handle a NULL argument (inline function qobject_decref
> already handles them, too).
> 
> Signed-off-by: Stefan Weil 

Applied to qmp-next branch, thanks!

> ---
> 
> I did not change QINCREF because there are currently no errors caused
> by that rarely used macro.
> 
> This patch can be used instead of the previous patch which fixed
> the problem directly in json-parser.c
> (see http://patchwork.ozlabs.org/patch/181129/).
> 
> Regards,
> Stefan Weil
> 
>  qobject.h |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qobject.h b/qobject.h
> index d42386d..9124649 100644
> --- a/qobject.h
> +++ b/qobject.h
> @@ -71,7 +71,7 @@ typedef struct QObject {
>  
>  /* High-level interface for qobject_decref() */
>  #define QDECREF(obj)  \
> -qobject_decref(QOBJECT(obj))
> +qobject_decref(obj ? QOBJECT(obj) : NULL)
>  
>  /* Initialize an object to default values */
>  #define QOBJECT_INIT(obj, qtype_type)   \

[Qemu-devel] [PATCH] kvm: Fix warning from static code analysis

[Stefan Weil]

Report from smatch:

kvm-all.c:1373 kvm_init(135) warn:
 variable dereferenced before check 's' (see line 1360)

's' cannot by NULL (it was alloced using g_malloc0), so there is no need
to check it here.

Signed-off-by: Stefan Weil 
---
 kvm-all.c |   12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 34b02c1..20b980d 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1370,13 +1370,11 @@ int kvm_init(void)
 return 0;
 
 err:
-if (s) {
-if (s->vmfd >= 0) {
-close(s->vmfd);
-}
-if (s->fd != -1) {
-close(s->fd);
-}
+if (s->vmfd >= 0) {
+close(s->vmfd);
+}
+if (s->fd != -1) {
+close(s->fd);
 }
 g_free(s);
 
-- 
1.7.10

[Qemu-devel] [PATCH] slirp: Fix error reported by static code analysis and remove wrong type casts

[Stefan Weil]

Report from smatch:
slirp/tcp_subr.c:127 tcp_respond(17) error:
 we previously assumed 'tp' could be null (see line 124)

Fix this by checking 'tp' before reading its elements.

The type casts of pointers to long are not related to the smatch report
but happened to be near that code. Those type casts are not allowed
when sizeof(pointer) != sizeof(long).

Signed-off-by: Stefan Weil 
---

Coding style was not fixed by the patch!

 slirp/tcp_subr.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index 025b374..5f3214c 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -114,9 +114,9 @@ tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct 
mbuf *m,
int win = 0;
 
DEBUG_CALL("tcp_respond");
-   DEBUG_ARG("tp = %lx", (long)tp);
-   DEBUG_ARG("ti = %lx", (long)ti);
-   DEBUG_ARG("m = %lx", (long)m);
+   DEBUG_ARG("tp = %p", tp);
+   DEBUG_ARG("ti = %p", ti);
+   DEBUG_ARG("m = %p", m);
DEBUG_ARG("ack = %u", ack);
DEBUG_ARG("seq = %u", seq);
DEBUG_ARG("flags = %x", flags);
@@ -124,7 +124,7 @@ tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct 
mbuf *m,
if (tp)
win = sbspace(&tp->t_socket->so_rcv);
 if (m == NULL) {
-   if ((m = m_get(tp->t_socket->slirp)) == NULL)
+   if (tp && (m = m_get(tp->t_socket->slirp)) == NULL)
return;
tlen = 0;
m->m_data += IF_MAXLINKHDR;
-- 
1.7.10

[Qemu-devel] [PATCH] sparc-dis: Remove redundant NULL check

[Stefan Weil]

Report from smatch:
sparc-dis.c:2664 build_hash_table(14) info:
 redundant null check on hash_buf calling free()

Signed-off-by: Stefan Weil 
---

Coding style was not fixed.

- sw

 sparc-dis.c |3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sparc-dis.c b/sparc-dis.c
index cdd337a..ef28835 100644
--- a/sparc-dis.c
+++ b/sparc-dis.c
@@ -2660,8 +2660,7 @@ build_hash_table (const sparc_opcode **opcode_table,
 
   memset (hash_table, 0, HASH_SIZE * sizeof (hash_table[0]));
   memset (hash_count, 0, HASH_SIZE * sizeof (hash_count[0]));
-  if (hash_buf != NULL)
-free (hash_buf);
+  free (hash_buf);
   hash_buf = malloc (sizeof (* hash_buf) * num_opcodes);
   for (i = num_opcodes - 1; i >= 0; --i)
 {
-- 
1.7.10

Re: [Qemu-devel] [PATCH 16/21] target-arm: switch to AREG0 free mode

[Peter Maydell]

On 3 September 2012 21:10, Blue Swirl  wrote:
> On Mon, Sep 3, 2012 at 7:54 PM, Peter Maydell  
> wrote:
>> I don't want the *file* split, I'd just like to see this *patch*
>> as 4 or 5 separate patches, not one big one.
>
> While converting, it's easier to work on whole files but maybe the
> resulting patch can be still split.

If it really doesn't seem splittable let me know and I'll wade
through this big patch.

-- PMM

[Qemu-devel] [PATCH] ide: Fix error messages from static code analysis (no real error)

[Stefan Weil]

Report from smatch:
hw/ide/core.c:1472 ide_exec_cmd(423) error: buffer overflow 'smart_attributes' 
8 <= 29
hw/ide/core.c:1474 ide_exec_cmd(425) error: buffer overflow 'smart_attributes' 
8 <= 29
hw/ide/core.c:1475 ide_exec_cmd(426) error: buffer overflow 'smart_attributes' 
8 <= 29
...

The upper limit of 30 was never reached because both for loops terminated
when 'smart_attributes' reached end of list, so there was no real buffer
overflow.

Nevertheless, changing the code not only fixes the error report, but also
reduces the size of smart_attributes and simplifies the for loops.

Signed-off-by: Stefan Weil 
---

Note: scripts/checkpatch.pl reports warnings and errors caused by tabs and
wrong indentation. I did not fix that because the whole file is full of tabs.

- sw


 hw/ide/core.c |   11 ++-
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 978dd5e..dc04621 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -53,8 +53,6 @@ static const int smart_attributes[][12] = {
 { 0x0c, 0x03, 0x00, 0x64, 0x64, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
 /* airflow-temperature-celsius */
 { 190,  0x03, 0x00, 0x45, 0x45, 0x1f, 0x00, 0x1f, 0x1f, 0x00, 0x00, 0x32},
-/* end of list */
-{ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
 };
 
 static int ide_handle_rw_error(IDEState *s, int error, int op);
@@ -1468,9 +1466,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
case SMART_READ_THRESH:
memset(s->io_buffer, 0, 0x200);
s->io_buffer[0] = 0x01; /* smart struct version */
-   for (n=0; n<30; n++) {
-   if (smart_attributes[n][0] == 0)
-   break;
+   for (n = 0; n < ARRAY_SIZE(smart_attributes); n++) {
s->io_buffer[2+0+(n*12)] = smart_attributes[n][0];
s->io_buffer[2+1+(n*12)] = smart_attributes[n][11];
}
@@ -1484,10 +1480,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
case SMART_READ_DATA:
memset(s->io_buffer, 0, 0x200);
s->io_buffer[0] = 0x01; /* smart struct version */
-   for (n=0; n<30; n++) {
-   if (smart_attributes[n][0] == 0) {
-   break;
-   }
+   for (n = 0; n < ARRAY_SIZE(smart_attributes); n++) {
int i;
for(i = 0; i < 11; i++) {
s->io_buffer[2+i+(n*12)] = smart_attributes[n][i];
-- 
1.7.10

Re: [Qemu-devel] [PATCH 16/21] target-arm: switch to AREG0 free mode

[Blue Swirl]

On Mon, Sep 3, 2012 at 7:54 PM, Peter Maydell  wrote:
> On 3 September 2012 19:58, Blue Swirl  wrote:
>> On Mon, Sep 3, 2012 at 12:03 AM, Peter Maydell  
>> wrote:
>>> On 3 September 2012 01:01, Peter Maydell  wrote:
>>> That's quite hard to cross-reference when the patch is this big.
>>> I think it would be helpful if you could split it up into patches
>>> touching smaller groups of helpers at once rather than having a
>>> single patch that does them all at once.
>>
>> For x86, Sparc and s390x I used the approach of splitting op_helper.c
>> to smaller files first. I didn't do it for ARM since
>> target-arm/op_helper.c is alread pretty small (<500 lines). It could
>> be split to saturating ops, condition code setting arithmetic ops and
>> misc ops, between 100 and 200 lines each. Would that be OK?
>
> I don't want the *file* split, I'd just like to see this *patch*
> as 4 or 5 separate patches, not one big one.

While converting, it's easier to work on whole files but maybe the
resulting patch can be still split.

>
> (Patch-splitting is a personal preference thing; I generally favour
> lots of little patches over big ones.)

That's just common sense. The conversion logic is just not very helpful here.

>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 16/21] target-arm: switch to AREG0 free mode

[Peter Maydell]

On 3 September 2012 19:58, Blue Swirl  wrote:
> On Mon, Sep 3, 2012 at 12:03 AM, Peter Maydell  
> wrote:
>> On 3 September 2012 01:01, Peter Maydell  wrote:
>> That's quite hard to cross-reference when the patch is this big.
>> I think it would be helpful if you could split it up into patches
>> touching smaller groups of helpers at once rather than having a
>> single patch that does them all at once.
>
> For x86, Sparc and s390x I used the approach of splitting op_helper.c
> to smaller files first. I didn't do it for ARM since
> target-arm/op_helper.c is alread pretty small (<500 lines). It could
> be split to saturating ops, condition code setting arithmetic ops and
> misc ops, between 100 and 200 lines each. Would that be OK?

I don't want the *file* split, I'd just like to see this *patch*
as 4 or 5 separate patches, not one big one.

(Patch-splitting is a personal preference thing; I generally favour
lots of little patches over big ones.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 20/21] target-mips: switch to AREG0 free mode

[Aurelien Jarno]

On Mon, Sep 03, 2012 at 07:15:29PM +, Blue Swirl wrote:
> On Mon, Sep 3, 2012 at 3:50 PM, Aurelien Jarno  wrote:
> > On Sun, Sep 02, 2012 at 05:33:49PM +, Blue Swirl wrote:
> >> Add an explicit CPUState parameter instead of relying on AREG0
> >> and switch to AREG0 free mode.
> >>
> >> Signed-off-by: Blue Swirl 
> >> ---
> >>  configure |2 +-
> >>  target-mips/Makefile.objs |2 -
> >>  target-mips/cpu.h |   16 +-
> >>  target-mips/helper.h  |  410 +-
> >>  target-mips/op_helper.c   | 1065 
> >> -
> >>  target-mips/translate.c   |  754 
> >>  6 files changed, 1163 insertions(+), 1086 deletions(-)
> >
> > Acked-by: Aurelien Jarno 
> >
> > Please commit this patch asap after the 1.2 release, even if the patches
> > for the other targets are not ready, so that it doesn't hold the
> > development.
> 
> Thanks. There's at least Jia Liu's MIPS DSP series, it probably does
> not conflict so much but it hasn't been converted to new AREG0-free
> order either.
> 

I am still reviewing it, but I don't think it is ready yet, so it's
better to get the AREG0 patches first.

-- 
Aurelien Jarno  GPG: 1024D/F1BCDB73
aurel...@aurel32.net http://www.aurel32.net

Re: [Qemu-devel] [PATCH 4/4] kvm: i386: Add classic PCI device assignment

[Blue Swirl]

On Mon, Sep 3, 2012 at 4:14 PM, Avi Kivity  wrote:
> On 08/29/2012 11:27 AM, Markus Armbruster wrote:
>>
>> I don't see a point in making contributors avoid non-problems that might
>> conceivably become trivial problems some day.  Especially when there's
>> no automated help with the avoiding.
>
> -Wpointer-arith

+1

>
>
>
> --
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/6] libqblock public type defines

[Blue Swirl]

On Mon, Sep 3, 2012 at 9:18 AM, Wenchao Xia  wrote:
>   This patch contains public type and defines used in APIs.
>
> Signed-off-by: Wenchao Xia 
> ---
>  libqblock/libqblock-types.h |  228 
> +++
>  1 files changed, 228 insertions(+), 0 deletions(-)
>  create mode 100644 libqblock/libqblock-types.h
>
> diff --git a/libqblock/libqblock-types.h b/libqblock/libqblock-types.h
> new file mode 100644
> index 000..3389bda
> --- /dev/null
> +++ b/libqblock/libqblock-types.h
> @@ -0,0 +1,228 @@
> +#ifndef LIBQBLOCK_TYPES_H
> +#define LIBQBLOCK_TYPES_H
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* this library is designed around this core struct. */
> +struct QBlockState;
> +
> +/* every thread would have a broker. */
> +struct QBroker;
> +
> +/* flag used in open and create */
> +#define LIBQBLOCK_O_RDWR0x0002
> +/* do not use the host page cache */
> +#define LIBQBLOCK_O_NOCACHE 0x0020
> +/* use write-back caching */
> +#define LIBQBLOCK_O_CACHE_WB0x0040
> +/* don't open the backing file */
> +#define LIBQBLOCK_O_NO_BACKING  0x0100
> +/* disable flushing on this disk */
> +#define LIBQBLOCK_O_NO_FLUSH0x0200
> +
> +#define LIBQBLOCK_O_CACHE_MASK \
> +   (LIBQBLOCK_O_NOCACHE | LIBQBLOCK_O_CACHE_WB | LIBQBLOCK_O_NO_FLUSH)
> +
> +#define LIBQBLOCK_O_VALID_MASK \
> +   (LIBQBLOCK_O_RDWR | LIBQBLOCK_O_NOCACHE | LIBQBLOCK_O_CACHE_WB | \
> +LIBQBLOCK_O_NO_BACKING | LIBQBLOCK_O_NO_FLUSH)
> +
> +enum QBlockProtocol {
> +QB_PROTO_NONE = 0,
> +QB_PROTO_FILE,
> +QB_PROTO_MAX
> +};
> +
> +enum QBlockFormat {
> +QB_FMT_NONE = 0,
> +QB_FMT_COW,
> +QB_FMT_QED,
> +QB_FMT_QCOW,
> +QB_FMT_QCOW2,
> +QB_FMT_RAW,
> +QB_FMT_RBD,
> +QB_FMT_SHEEPDOG,
> +QB_FMT_VDI,
> +QB_FMT_VMDK,
> +QB_FMT_VPC,
> +QB_FMT_MAX
> +};
> +
> +struct QBlockOption_prot_file {

QBlockOptionProtFile

> +char *filename;

'const'

> +};
> +
> +union QBlockOption_prot {

QBlockOptionProt

> +struct QBlockOption_prot_file o_file;
> +};
> +
> +/**
> + * struct QBlockOptionLoc: contains information about how to find the image
> + *
> + * @prot_type: protocol type, now only support FILE.
> + * @prot_op: protocol related options.
> + */
> +struct QBlockOptionLoc {
> +enum QBlockProtocol prot_type;
> +union QBlockOption_prot prot_op;
> +uint8_t reserved[512];
> +};
> +
> +/* format related options */
> +struct QBlockOption_fmt_cow {

QBlockOptionFmtCOW

> +size_t virt_size;
> +struct QBlockOptionLoc backing_loc;
> +};
> +
> +struct QBlockOption_fmt_qed {

QBlockOptionFmtQED

etc. for the rest. Don't mix CamelCase with underscore style, struct
names must use CamelCase.

> +size_t virt_size;
> +struct QBlockOptionLoc backing_loc;
> +enum QBlockFormat backing_fmt;
> +size_t cluster_size; /* unit is bytes */
> +size_t table_size; /* unit is clusters */
> +};
> +
> +struct QBlockOption_fmt_qcow {
> +size_t virt_size;
> +struct QBlockOptionLoc backing_loc;
> +bool encrypt;
> +};
> +
> +/* "Compatibility level (0.10 or 1.1)" */
> +enum QBlockOption_fmt_qcow2_cpt {
> +QBO_FMT_QCOW2_CPT_NONE = 0,
> +QBO_FMT_QCOW2_CPT_V010,
> +QBO_FMT_QCOW2_CPT_V110,
> +};
> +
> +/* off or metadata */
> +enum QBlockOption_fmt_qcow2_prealloc {
> +QBO_FMT_QCOW2_PREALLOC_NONE = 0,
> +QBO_FMT_QCOW2_PREALLOC_OFF,
> +QBO_FMT_QCOW2_PREALLOC_METADATA,
> +};
> +
> +struct QBlockOption_fmt_qcow2 {
> +size_t virt_size;
> +struct QBlockOptionLoc backing_loc;
> +enum QBlockFormat backing_fmt;
> +bool encrypt;
> +size_t cluster_size; /* unit is bytes */
> +enum QBlockOption_fmt_qcow2_cpt cpt_lv;
> +enum QBlockOption_fmt_qcow2_prealloc pre_mode;
> +};
> +
> +struct QBlockOption_fmt_raw {
> +size_t virt_size;
> +};
> +
> +struct QBlockOption_fmt_rbd {
> +size_t virt_size;
> +size_t cluster_size;
> +};
> +
> +/* off or full */
> +enum QBlockOption_fmt_sheepdog_prealloc {
> +QBO_FMT_SD_PREALLOC_NONE = 0,
> +QBO_FMT_SD_PREALLOC_OFF,
> +QBO_FMT_SD_PREALLOC_FULL,
> +};
> +
> +struct QBlockOption_fmt_sheepdog {
> +size_t virt_size;
> +struct QBlockOptionLoc backing_loc;
> +enum QBlockOption_fmt_sheepdog_prealloc pre_mode;
> +};
> +
> +enum QBlockOption_fmt_vdi_prealloc {
> +QBO_FMT_VDI_PREALLOC_NONE = 0,
> +QBO_FMT_VDI_PREALLOC_FALSE,
> +QBO_FMT_VDI_PREALLOC_TRUE,
> +};
> +
> +struct QBlockOption_fmt_vdi {
> +size_t virt_size;
> +size_t cluster_size;
> +enum QBlockOption_fmt_vdi_prealloc pre_mode;
> +};
> +
> +/* whether compact to vmdk verion 6 */
> +enum QBlockOption_fmt_vmdk_cpt {
> +QBO_FMT_VMDK_CPT_NONE = 0,
> +QBO_FMT_VMDK_CPT_VMDKV6_FALSE,
> +QBO_FMT_VMDK_CPT_VMDKV6_TRUE,
> +};
> +
> +/* vmdk flat extent format, values:
> +"{monolithicSparse (default) | monolithicFlat | twoGbMaxExtentSparse |
> +twoGbMaxExtentFlat | streamOptimized} */
> +enum QBlockOption_fmt_vmdk_subfmt {
> +QBO_FM

Re: [Qemu-devel] [PATCH 5/6] libqblock test example

[Blue Swirl]

On Mon, Sep 3, 2012 at 9:18 AM, Wenchao Xia  wrote:
>   In this example, user first create two qcow2 images, and then get the
> backing file relationship information of them. Then does write and read
> sync IO on them.
>
> Signed-off-by: Wenchao Xia 
> ---
>  tests/libqblock/libqblock-test.c |  219 
> ++
>  1 files changed, 219 insertions(+), 0 deletions(-)
>  create mode 100644 tests/libqblock/libqblock-test.c
>
> diff --git a/tests/libqblock/libqblock-test.c 
> b/tests/libqblock/libqblock-test.c
> new file mode 100644
> index 000..9a1eac5
> --- /dev/null
> +++ b/tests/libqblock/libqblock-test.c
> @@ -0,0 +1,219 @@
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "libqblock.h"
> +
> +static unsigned char buf_r[1024];
> +static unsigned char buf_w[1024] = {4, 0, 0, 2};
> +
> +struct verify_data {

VerifyData

> +unsigned char *buf_r;
> +unsigned char *buf_w;
> +int len;
> +};
> +
> +static void print_loc(struct QBlockOptionLoc *loc)
> +{
> +switch (loc->prot_type) {
> +case QB_PROTO_NONE:
> +printf("protocol type [none].");
> +break;
> +case QB_PROTO_FILE:
> +printf("protocol type [file], filename [%s].",
> +   loc->prot_op.o_file.filename);
> +break;
> +default:
> +printf("protocol type not supported.");
> +break;
> +}
> +}
> +
> +static void print_info_image_static(struct QBlockInfoImageStatic *info)
> +{
> +printf("===image location:\n");
> +print_loc(&info->loc);
> +printf("\nvirtual_size %" PRId64 ", format type %d,",
> +   info->virt_size, info->fmt_type);
> +printf("allocated size %" PRId64 ", encrypt %d,",
> +   info->allocated_size, info->encrypt);
> +printf("\nbacking image location:\n");
> +print_loc(&info->backing_loc);
> +printf("\n");
> +}
> +
> +static void test_check(struct verify_data *vdata)
> +{
> +int cmp;
> +cmp = memcmp(vdata->buf_r, vdata->buf_w, vdata->len);
> +if (cmp == 0) {
> +printf("compare succeed, %d.\n", vdata->buf_r[24]);
> +} else {
> +printf("!!! compare fail, %d.\n", vdata->buf_r[24]);
> +exit(1);
> +}
> +}
> +
> +int main(int argc, char **argv)
> +{
> +char *filename1, *filename2;
> +struct QBroker *broker = NULL;
> +struct QBlockState *qbs = NULL;
> +struct QBlockOptionLoc *ol = NULL;
> +struct QBlockOptionFormat *of = NULL;
> +struct QBlockInfoImageStatic *info_st = NULL;
> +int ret, flag;
> +int test_offset = 0;
> +int test_len = 512;
> +struct verify_data vdata;
> +char err_str[1024];
> +
> +vdata.buf_r = buf_r;
> +vdata.buf_w = buf_w;
> +vdata.len = test_len;
> +
> +filename1 = (char *)"./qemu_image1";
> +filename2 = (char *)"./qemu_image2";

The casts remove 'const' qualifier, how can that be safe?

> +printf("qemu test, filename1 is %s, filename2 is %s.\n",
> +   filename1, filename2);
> +
> +libqblock_init();
> +
> +ret = qb_broker_new(&broker);
> +if (ret < 0) {
> +goto free;
> +}
> +
> +ret = qb_state_new(broker, &qbs);
> +if (ret < 0) {
> +goto free;
> +}
> +
> +ret = qb_ol_new(broker, &ol);
> +if (ret < 0) {
> +goto free;
> +}
> +
> +ret = qb_of_new(broker, &of);
> +if (ret < 0) {
> +goto free;
> +}
> +
> +/* create a new image */
> +
> +ol->prot_type = QB_PROTO_FILE;
> +ol->prot_op.o_file.filename = filename2;
> +of->fmt_type = QB_FMT_QCOW2;
> +of->fmt_op.o_qcow2.virt_size = 100 * 1024;
> +flag = 0;
> +
> +ret = qb_create(broker, qbs, ol, of, flag);
> +if (ret < 0) {
> +qb_error_get_human_str(broker, err_str, sizeof(err_str));
> +printf("create fail 1. %s.\n", err_str);
> +goto unlink;
> +}
> +
> +ol->prot_type = QB_PROTO_FILE;
> +ol->prot_op.o_file.filename = filename1;
> +of->fmt_type = QB_FMT_QCOW2;
> +of->fmt_op.o_qcow2.backing_loc.prot_type = QB_PROTO_FILE;
> +of->fmt_op.o_qcow2.backing_loc.prot_op.o_file.filename = filename2;
> +flag = 0;
> +ret = qb_create(broker, qbs, ol, of, flag);
> +if (ret < 0) {
> +qb_error_get_human_str(broker, err_str, sizeof(err_str));
> +printf("create fail 2. %s.\n", err_str);
> +goto unlink;
> +}
> +
> +/* get informations */
> +ol->prot_type = QB_PROTO_FILE;
> +ol->prot_op.o_file.filename = filename1;
> +of->fmt_type = QB_FMT_NONE;
> +flag = LIBQBLOCK_O_NO_BACKING;
> +ret = qb_open(broker, qbs, ol, of, flag);
> +if (ret < 0) {
> +qb_error_get_human_str(broker, err_str, sizeof(err_str));
> +printf("info getting, open failed. %s.\n", err_str);
> +goto free;
> +}
> +
> +while (1) {
> +ret = qb_info_image_static_get(broker, qbs, &info_st);
> +if (ret < 0) {
> +qb_error_get_human_str(br

[Qemu-devel] [PATCH] audio: Fix warning from static code analysis

[Stefan Weil]

smatch report:
audio/audio_template.h:416 AUD_open_out(18) warn:
 variable dereferenced before check 'as' (see line 414)

Moving the ldebug statement after the statement which checks 'as'
fixes that warning.

Signed-off-by: Stefan Weil 
---

This patch fails when checked by checkpatch.pl because
it preserves the coding style used for QEMU audio code.

Please apply the patch after QEMU 1.2 was released.

Thanks,

Stefan Weil

 audio/audio_template.h |6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/audio/audio_template.h b/audio/audio_template.h
index 519432a..16f7880 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -410,15 +410,15 @@ SW *glue (AUD_open_, TYPE) (
 SW *old_sw = NULL;
 #endif
 
-ldebug ("open %s, freq %d, nchannels %d, fmt %d\n",
-name, as->freq, as->nchannels, as->fmt);
-
 if (audio_bug (AUDIO_FUNC, !card || !name || !callback_fn || !as)) {
 dolog ("card=%p name=%p callback_fn=%p as=%p\n",
card, name, callback_fn, as);
 goto fail;
 }
 
+ldebug ("open %s, freq %d, nchannels %d, fmt %d\n",
+name, as->freq, as->nchannels, as->fmt);
+
 if (audio_bug (AUDIO_FUNC, audio_validate_settings (as))) {
 audio_print_settings (as);
 goto fail;
-- 
1.7.10

Re: [Qemu-devel] [PATCH 1/6] libqblock APIs

[Blue Swirl]

On Mon, Sep 3, 2012 at 9:18 AM, Wenchao Xia  wrote:
>   This patch contains the major APIs in the library.
> Important APIs:
>   1 QBroker. These structure was used to retrieve errors, every thread must
> create one first, Later maybe thread related staff could be added into it.
>   2 QBlockState. It stands for an block image object.
>   3 QBlockInfoImageStatic. Now it is not folded with location and format.
>   4 ABI was kept with reserved members.
>
> structure, because it would cause caller more codes to find one member.
>
> Signed-off-by: Wenchao Xia 
> ---
>  libqblock/libqblock.c |  859 
> +
>  libqblock/libqblock.h |  251 ++
>  2 files changed, 1110 insertions(+), 0 deletions(-)
>  create mode 100644 libqblock/libqblock.c
>  create mode 100644 libqblock/libqblock.h
>
> diff --git a/libqblock/libqblock.c b/libqblock/libqblock.c
> new file mode 100644
> index 000..3983802
> --- /dev/null
> +++ b/libqblock/libqblock.c
> @@ -0,0 +1,859 @@
> +/*
> + * QEMU block layer library
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + *  Wenchao Xia   
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#include 
> +#include 
> +
> +#include "libqblock.h"
> +#include "libqblock-helper.h"
> +
> +#include "qemu-aio.h"
> +
> +void libqblock_init(void)
> +{
> +bdrv_init();
> +qemu_init_main_loop();
> +}
> +
> +int qb_broker_new(struct QBroker **broker)
> +{
> +*broker = FUNC_CALLOC(1, sizeof(struct QBroker));
> +if (*broker == NULL) {
> +return QB_ERR_MEM_ERR;
> +}
> +return 0;
> +}
> +
> +void qb_broker_delete(struct QBroker **broker)
> +{
> +CLEAN_FREE(*broker);
> +return;
> +}
> +
> +int qb_state_new(struct QBroker *broker,
> + struct QBlockState **qbs)
> +{
> +*qbs = FUNC_CALLOC(1, sizeof(struct QBlockState));
> +if (*qbs == NULL) {
> +set_broker_err_nomem(broker);
> +return broker->err_ret;
> +}
> +(*qbs)->bdrvs = bdrv_new("hda");
> +if ((*qbs)->bdrvs == NULL) {
> +CLEAN_FREE(*qbs);
> +set_broker_err(broker, QB_ERR_INTERNAL_ERR,
> +   "failed to create the driver.");
> +return broker->err_ret;
> +}
> +return 0;
> +}
> +
> +void qb_state_delete(struct QBroker *broker,
> + struct QBlockState **qbs)
> +{
> +CLEAN_FREE((*qbs)->filename);
> +if ((*qbs)->bdrvs != NULL) {
> +bdrv_delete((*qbs)->bdrvs);
> +(*qbs)->bdrvs = NULL;
> +}
> +CLEAN_FREE(*qbs);
> +return;
> +}
> +
> +int qb_ol_new(struct QBroker *broker,
> +  struct QBlockOptionLoc **op)
> +{
> +*op = FUNC_CALLOC(1, sizeof(struct QBlockOptionLoc));
> +if (*op == NULL) {
> +set_broker_err_nomem(broker);
> +return broker->err_ret;
> +}
> +return 0;
> +}
> +
> +void qb_ol_delete(struct QBroker *broker,
> +  struct QBlockOptionLoc **op)
> +{
> +CLEAN_FREE(*op);
> +}
> +
> +int qb_of_new(struct QBroker *broker,
> +  struct QBlockOptionFormat **op)
> +{
> +*op = FUNC_CALLOC(1, sizeof(struct QBlockOptionFormat));
> +if (*op == NULL) {
> +set_broker_err_nomem(broker);
> +return broker->err_ret;
> +}
> +return 0;
> +}
> +
> +void qb_of_delete(struct QBroker *broker,
> +  struct QBlockOptionFormat **op)
> +{
> +CLEAN_FREE(*op);
> +}
> +
> +/* return 0 if every thing is fine */
> +static int loc_check_params(struct QBroker *broker,
> +struct QBlockOptionLoc *loc)
> +{
> +broker->err_ret = 0;
> +
> +switch (loc->prot_type) {
> +case QB_PROTO_FILE:
> +if (loc->prot_op.o_file.filename == NULL) {
> +set_broker_err(broker, QB_ERR_INVALID_PARAM,
> +   "Filename was not set.");
> +goto out;
> +}
> +if (path_has_protocol(loc->prot_op.o_file.filename) > 0) {
> +set_broker_err(broker, QB_ERR_INVALID_PARAM,
> +   "filename [%s] had protocol.",
> +   loc->prot_op.o_file.filename);
> +goto out;
> +}
> +break;
> +default:
> +set_broker_err(broker, QB_ERR_INVALID_PARAM,
> +   "Protocol type [%d] was not valid.",
> +   loc->prot_type);
> +break;
> +}
> +
> + out:
> +return broker->err_ret;
> +}
> +
> +/* translate loc structure to internal filename, returned char* need free,
> + * assuming filename is not NULL. *filename would be set to NULL if no valid
> + * filename found. *filename must be freed later.
> + * return 0 if no error with *filename set.
> + */
> +static int loc2filename(struct QBroker *broker,
> +struct QBlockOptionLoc *loc,
> +char **filename)
> +{
> +  

[Qemu-devel] [PATCH] json-parser: Fix potential NULL pointer segfault

[Stefan Weil]

Report from smatch:
json-parser.c:474 parse_object(62) error: potential null derefence 'dict'.
json-parser.c:553 parse_array(75) error: potential null derefence 'list'.

Label 'out' in json-parser.c can be called with list == NULL
which is passed to QDECREF.

Modify QDECREF to handle a NULL argument (inline function qobject_decref
already handles them, too).

Signed-off-by: Stefan Weil 
---

I did not change QINCREF because there are currently no errors caused
by that rarely used macro.

This patch can be used instead of the previous patch which fixed
the problem directly in json-parser.c
(see http://patchwork.ozlabs.org/patch/181129/).

Regards,
Stefan Weil

 qobject.h |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qobject.h b/qobject.h
index d42386d..9124649 100644
--- a/qobject.h
+++ b/qobject.h
@@ -71,7 +71,7 @@ typedef struct QObject {
 
 /* High-level interface for qobject_decref() */
 #define QDECREF(obj)  \
-qobject_decref(QOBJECT(obj))
+qobject_decref(obj ? QOBJECT(obj) : NULL)
 
 /* Initialize an object to default values */
 #define QOBJECT_INIT(obj, qtype_type)   \
-- 
1.7.10

Re: [Qemu-devel] [PATCH 20/21] target-mips: switch to AREG0 free mode

[Blue Swirl]

On Mon, Sep 3, 2012 at 3:50 PM, Aurelien Jarno  wrote:
> On Sun, Sep 02, 2012 at 05:33:49PM +, Blue Swirl wrote:
>> Add an explicit CPUState parameter instead of relying on AREG0
>> and switch to AREG0 free mode.
>>
>> Signed-off-by: Blue Swirl 
>> ---
>>  configure |2 +-
>>  target-mips/Makefile.objs |2 -
>>  target-mips/cpu.h |   16 +-
>>  target-mips/helper.h  |  410 +-
>>  target-mips/op_helper.c   | 1065 
>> -
>>  target-mips/translate.c   |  754 
>>  6 files changed, 1163 insertions(+), 1086 deletions(-)
>
> Acked-by: Aurelien Jarno 
>
> Please commit this patch asap after the 1.2 release, even if the patches
> for the other targets are not ready, so that it doesn't hold the
> development.

Thanks. There's at least Jia Liu's MIPS DSP series, it probably does
not conflict so much but it hasn't been converted to new AREG0-free
order either.

>
> --
> Aurelien Jarno  GPG: 1024D/F1BCDB73
> aurel...@aurel32.net http://www.aurel32.net

Re: [Qemu-devel] [PATCH 16/21] target-arm: switch to AREG0 free mode

[Blue Swirl]

On Mon, Sep 3, 2012 at 12:03 AM, Peter Maydell  wrote:
> On 3 September 2012 01:01, Peter Maydell  wrote:
>> On 2 September 2012 18:33, Blue Swirl  wrote:
>>> Add an explicit CPUState parameter instead of relying on AREG0
>>> and switch to AREG0 free mode.
>>>
>>> Signed-off-by: Blue Swirl 
>>> ---
>>>  configure|2 +-
>>>  target-arm/Makefile.objs |2 -
>>>  target-arm/cpu.h |   10 ++-
>>>  target-arm/helper.c  |8 +-
>>>  target-arm/helper.h  |   60 +-
>>>  target-arm/op_helper.c   |   92 +---
>>>  target-arm/translate.c   |  148 
>>> +++---
>>>  7 files changed, 158 insertions(+), 164 deletions(-)
>>
>> This is too big to easily review -- it's making a change to a lot
>> of helpers, and in each case that change affects three places
>> (callers, declaration, implementation). That'
>
> Sorry, finger slip meant I sent that half finished. To continue...
>
> That's quite hard to cross-reference when the patch is this big.
> I think it would be helpful if you could split it up into patches
> touching smaller groups of helpers at once rather than having a
> single patch that does them all at once.

For x86, Sparc and s390x I used the approach of splitting op_helper.c
to smaller files first. I didn't do it for ARM since
target-arm/op_helper.c is alread pretty small (<500 lines). It could
be split to saturating ops, condition code setting arithmetic ops and
misc ops, between 100 and 200 lines each. Would that be OK?

It looks like helper.c should be split too (maybe VFP, MMU, CPU init,
CPR), but that's starting to get beyond the scope of the series.

>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 19/21] target-sh4: switch to AREG0 free mode

[Blue Swirl]

On Sun, Sep 2, 2012 at 11:42 PM, Aurelien Jarno  wrote:
> On Sun, Sep 02, 2012 at 05:33:48PM +, Blue Swirl wrote:
>> Add an explicit CPUState parameter instead of relying on AREG0
>> and switch to AREG0 free mode.
>>
>> Signed-off-by: Blue Swirl 
>> ---
>>  configure|2 +-
>>  target-sh4/Makefile.objs |2 -
>>  target-sh4/helper.h  |   84 +++---
>>  target-sh4/op_helper.c   |  182 
>> ++---
>>  target-sh4/translate.c   |  114 -
>>  5 files changed, 195 insertions(+), 189 deletions(-)
>>
>> diff --git a/configure b/configure
>> index d760e07..d69e43e 100755
>> --- a/configure
>> +++ b/configure
>> @@ -3829,7 +3829,7 @@ symlink "$source_path/Makefile.target" 
>> "$target_dir/Makefile"
>>
>>
>>  case "$target_arch2" in
>> -  alpha | arm* | cris | i386 | lm32 | m68k | microblaze* | or32 | s390x | 
>> sparc* | unicore32 | x86_64 | xtensa* | ppc*)
>> +  alpha | arm* | cris | i386 | lm32 | m68k | microblaze* | or32 | s390x | 
>> sh4* | sparc* | unicore32 | x86_64 | xtensa* | ppc*)
>>  echo "CONFIG_TCG_PASS_AREG0=y" >> $config_target_mak
>>;;
>>  esac
>> diff --git a/target-sh4/Makefile.objs b/target-sh4/Makefile.objs
>> index 2e0e093..ca20f21 100644
>> --- a/target-sh4/Makefile.objs
>> +++ b/target-sh4/Makefile.objs
>> @@ -1,4 +1,2 @@
>>  obj-y += translate.o op_helper.o helper.o cpu.o
>>  obj-$(CONFIG_SOFTMMU) += machine.o
>> -
>> -$(obj)/op_helper.o: QEMU_CFLAGS += $(HELPER_CFLAGS)
>> diff --git a/target-sh4/helper.h b/target-sh4/helper.h
>> index 95e3c7c..6e4f108 100644
>> --- a/target-sh4/helper.h
>> +++ b/target-sh4/helper.h
>> @@ -1,54 +1,54 @@
>>  #include "def-helper.h"
>>
>> -DEF_HELPER_0(ldtlb, void)
>> -DEF_HELPER_0(raise_illegal_instruction, void)
>> -DEF_HELPER_0(raise_slot_illegal_instruction, void)
>> -DEF_HELPER_0(raise_fpu_disable, void)
>> -DEF_HELPER_0(raise_slot_fpu_disable, void)
>> -DEF_HELPER_0(debug, void)
>> -DEF_HELPER_1(sleep, void, i32)
>> -DEF_HELPER_1(trapa, void, i32)
>> +DEF_HELPER_1(ldtlb, void, env)
>> +DEF_HELPER_1(raise_illegal_instruction, void, env)
>> +DEF_HELPER_1(raise_slot_illegal_instruction, void, env)
>> +DEF_HELPER_1(raise_fpu_disable, void, env)
>> +DEF_HELPER_1(raise_slot_fpu_disable, void, env)
>> +DEF_HELPER_1(debug, void, env)
>> +DEF_HELPER_2(sleep, void, env, i32)
>> +DEF_HELPER_2(trapa, void, env, i32)
>>
>> -DEF_HELPER_2(movcal, void, i32, i32)
>> -DEF_HELPER_0(discard_movcal_backup, void)
>> -DEF_HELPER_1(ocbi, void, i32)
>> +DEF_HELPER_3(movcal, void, env, i32, i32)
>> +DEF_HELPER_1(discard_movcal_backup, void, env)
>> +DEF_HELPER_2(ocbi, void, env, i32)
>>
>> -DEF_HELPER_2(addv, i32, i32, i32)
>> -DEF_HELPER_2(addc, i32, i32, i32)
>> -DEF_HELPER_2(subv, i32, i32, i32)
>> -DEF_HELPER_2(subc, i32, i32, i32)
>> -DEF_HELPER_2(div1, i32, i32, i32)
>> -DEF_HELPER_2(macl, void, i32, i32)
>> -DEF_HELPER_2(macw, void, i32, i32)
>> +DEF_HELPER_3(addv, i32, env, i32, i32)
>> +DEF_HELPER_3(addc, i32, env, i32, i32)
>> +DEF_HELPER_3(subv, i32, env, i32, i32)
>> +DEF_HELPER_3(subc, i32, env, i32, i32)
>> +DEF_HELPER_3(div1, i32, env, i32, i32)
>> +DEF_HELPER_3(macl, void, env, i32, i32)
>> +DEF_HELPER_3(macw, void, env, i32, i32)
>>
>> -DEF_HELPER_1(ld_fpscr, void, i32)
>> +DEF_HELPER_2(ld_fpscr, void, env, i32)
>>
>>  DEF_HELPER_1(fabs_FT, f32, f32)
>>  DEF_HELPER_1(fabs_DT, f64, f64)
>> -DEF_HELPER_2(fadd_FT, f32, f32, f32)
>> -DEF_HELPER_2(fadd_DT, f64, f64, f64)
>> -DEF_HELPER_1(fcnvsd_FT_DT, f64, f32)
>> -DEF_HELPER_1(fcnvds_DT_FT, f32, f64)
>> +DEF_HELPER_3(fadd_FT, f32, env, f32, f32)
>> +DEF_HELPER_3(fadd_DT, f64, env, f64, f64)
>> +DEF_HELPER_2(fcnvsd_FT_DT, f64, env, f32)
>> +DEF_HELPER_2(fcnvds_DT_FT, f32, env, f64)
>>
>> -DEF_HELPER_2(fcmp_eq_FT, void, f32, f32)
>> -DEF_HELPER_2(fcmp_eq_DT, void, f64, f64)
>> -DEF_HELPER_2(fcmp_gt_FT, void, f32, f32)
>> -DEF_HELPER_2(fcmp_gt_DT, void, f64, f64)
>> -DEF_HELPER_2(fdiv_FT, f32, f32, f32)
>> -DEF_HELPER_2(fdiv_DT, f64, f64, f64)
>> -DEF_HELPER_1(float_FT, f32, i32)
>> -DEF_HELPER_1(float_DT, f64, i32)
>> -DEF_HELPER_3(fmac_FT, f32, f32, f32, f32)
>> -DEF_HELPER_2(fmul_FT, f32, f32, f32)
>> -DEF_HELPER_2(fmul_DT, f64, f64, f64)
>> +DEF_HELPER_3(fcmp_eq_FT, void, env, f32, f32)
>> +DEF_HELPER_3(fcmp_eq_DT, void, env, f64, f64)
>> +DEF_HELPER_3(fcmp_gt_FT, void, env, f32, f32)
>> +DEF_HELPER_3(fcmp_gt_DT, void, env, f64, f64)
>> +DEF_HELPER_3(fdiv_FT, f32, env, f32, f32)
>> +DEF_HELPER_3(fdiv_DT, f64, env, f64, f64)
>> +DEF_HELPER_2(float_FT, f32, env, i32)
>> +DEF_HELPER_2(float_DT, f64, env, i32)
>> +DEF_HELPER_4(fmac_FT, f32, env, f32, f32, f32)
>> +DEF_HELPER_3(fmul_FT, f32, env, f32, f32)
>> +DEF_HELPER_3(fmul_DT, f64, env, f64, f64)
>>  DEF_HELPER_1(fneg_T, f32, f32)
>> -DEF_HELPER_2(fsub_FT, f32, f32, f32)
>> -DEF_HELPER_2(fsub_DT, f64, f64, f64)
>> -DEF_HELPER_1(fsqrt_FT, f32, f32)
>> -DEF_HELPER_1(fsqrt_DT, f64, f64)
>> -DEF_HELPER_1(ftrc_FT, i32, f32)
>> -DEF_HELPER_1(ftrc_DT, i32, f64

Re: [Qemu-devel] [Spice-devel] [PATCH 3/5] Process outstanding commands in the ring after changing capability bits

[Alon Levy]

> From: Søren Sandmann Pedersen 
> 
> When a new client connects, there may be commands in the ring that it
> can't understand, so we need to process these before forwarding new
> commands to the client. By doing this after changing the capability
> bits we ensure that the new client will never see a command that it
> doesn't understand (under the assumption that the guest will read and
> obey the capability bits).


ACK.

We really should have some sort of fence mechanism for this. This patch will 
still work, since the command ring is 32 items long, so it should be relatively 
cheap to flush it (each item is a single comamnd. worse case can be 
32*video_mem). There is still a race - the guest has to handle the interrupt 
before sending any new commands.

In the future we could introduce a new command called QXLFence and have the 
interrupt handler call a new io to return it just before pushing it to the 
ring. The fence would be used only in the server right now, but when the driver 
releases it it can use it to know all commands before it have been processed 
(note that it doesn't mean all those commands have been released).

> ---
>  server/red_worker.c |5 +
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 60b5471..f87967c 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -9493,6 +9493,11 @@ static void
> on_new_display_channel_client(DisplayChannelClient *dcc)
>  }
>  red_channel_client_ack_zero_messages_window(&dcc->common.base);
>  if (worker->surfaces[0].context.canvas) {
> +int ring_is_empty;
> +
> +while (red_process_commands(worker, MAX_PIPE_SIZE,
> &ring_is_empty)) {
> +}
> +
>  red_current_flush(worker, 0);
>  push_new_primary_surface(dcc);
>  red_push_surface_image(dcc, 0);
> --
> 1.7.4
> 
> ___
> Spice-devel mailing list
> spice-de...@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>

Re: [Qemu-devel] [PATCH] qxl: Add set_client_capabilities() interface to QXLInterface

[Alon Levy]

> From: Søren Sandmann Pedersen 
> 
> This new interface lets spice server inform the guest whether
> 
> (a) a client is connected
> (b) what capabilities the client has
> 
> There is a fixed number (464) of bits reserved for capabilities, and
> when the capabilities bits change, the QXL_INTERRUPT_CLIENT interrupt
> is generated.
> 
> Signed-off-by: Soren Sandmann 
> ---
>  hw/qxl.c |   27 +++
>  hw/qxl.h |2 +-
>  2 files changed, 28 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/qxl.c b/hw/qxl.c
> index c2dd3b4..ffe1a76 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -901,6 +901,26 @@ static void interface_async_complete(QXLInstance
> *sin, uint64_t cookie_token)
>  }
>  }
>  
> +#if SPICE_SERVER_VERSION >= 0x000b04
> +
> +/* called from spice server thread context only */
> +static void interface_set_client_capabilities(QXLInstance *sin,
> +   uint8_t client_present,
> +   uint8_t caps[58])
> +{
> +PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> +
> +qxl->shadow_rom.client_present = client_present;
> +memcpy(qxl->shadow_rom.client_capabilities, caps, sizeof(caps));
> +qxl->rom->client_present = client_present;
> +memcpy(qxl->rom->client_capabilities, caps, sizeof(caps));
> +qxl_rom_set_dirty(qxl);
> +
> +qxl_send_events(qxl, QXL_INTERRUPT_CLIENT);
> +}
> +
> +#endif
> +
>  static const QXLInterface qxl_interface = {
>  .base.type   = SPICE_INTERFACE_QXL,
>  .base.description= "qxl gpu",
> @@ -922,6 +942,9 @@ static const QXLInterface qxl_interface = {
>  .flush_resources = interface_flush_resources,
>  .async_complete  = interface_async_complete,
>  .update_area_complete= interface_update_area_complete,
> +#if SPICE_SERVER_VERSION >= 0x000b04
> +.set_client_capabilities = interface_set_client_capabilities,
> +#endif
>  };
>  
>  static void qxl_enter_vga_mode(PCIQXLDevice *d)
> @@ -1785,6 +1808,10 @@ static int qxl_init_common(PCIQXLDevice *qxl)
>  io_size = 16;
>  break;
>  case 3: /* qxl-3 */
> + pci_device_rev = QXL_REVISION_STABLE_V10;
> + io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
> + break;
> +case 4:
>  default:
>  pci_device_rev = QXL_DEFAULT_REVISION;
>  io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
> diff --git a/hw/qxl.h b/hw/qxl.h
> index 172baf6..98d5a64 100644
> --- a/hw/qxl.h
> +++ b/hw/qxl.h
> @@ -128,7 +128,7 @@ typedef struct PCIQXLDevice {
>  }
>\
>  } while (0)
>  
> -#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
> +#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V12

QXL_REVISION_STABLE_V12 is only defined in latest spice-protocol, too new for 
the qemu required version.

Gerd, maybe it's a good idea to require spice-protocol 0.12.1, now that it's 
released? this will remove a lot of cruft.

>  
>  /* qxl.c */
>  void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int
>  group_id);
> --
> 1.7.4
> 
> 
> 

[Qemu-devel] apparently missing yet another notify_event()

[Michael Tokarev]

There's a new bugreport filed against qemu-kvm in debian,
which looks very similar to what we already had before --
https://bugs.launchpad.net/qemu/+bug/1021649
which were fixed by adding qemu_notify_event() call.
Later on these qemu_notify_event() calls become unnecessary
as far as I remember.

But here is a new one.

 qemu -nographic -kernel /boot/vmlinuz-$(uname -r) -append console=ttyS0 
-serial pty

This will hang with 100% CPU usage until something is sent
to the pty.   key is enough.

It does the same on qemu and kvm, with and without -enable-kvm,
and it looks pretty much like another forgotten notify_event.
And it happens on 1.1 and 1.2-tobe (today qemu/master), so should
be fixed before 1.2 is released.

Note: libvirt uses pty-redirected serial port heavily, and this
is where the users are hit, exactly.

The longer -serial specification (-chardev pty, -device isa-serial)
triggers it too.

What's missing this time?

(http://bugs.debian.org/686524 is the original bugreport).

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] qemu-ga: Remove unreachable code after g_error

[Luiz Capitulino]

On Mon, 03 Sep 2012 19:02:20 +0200
Stefan Weil  wrote:

> Am 03.09.2012 18:49, schrieb Luiz Capitulino:
> > On Sat,  1 Sep 2012 09:34:15 +0200
> > Stefan Weil  wrote:
> >
> >> Report from smatch:
> >> qemu-ga.c:117 register_signal_handlers(11) info: ignoring unreachable code.
> >> qemu-ga.c:122 register_signal_handlers(16) info: ignoring unreachable code.
> >>
> >> g_error calls abort which terminates the program.
> >>
> >> Signed-off-by: Stefan Weil 
> >> ---
> >>   qemu-ga.c |2 --
> >>   1 file changed, 2 deletions(-)
> >>
> >> diff --git a/qemu-ga.c b/qemu-ga.c
> >> index 7623079..b747470 100644
> >> --- a/qemu-ga.c
> >> +++ b/qemu-ga.c
> >> @@ -114,12 +114,10 @@ static gboolean register_signal_handlers(void)
> >>   ret = sigaction(SIGINT, &sigact, NULL);
> >>   if (ret == -1) {
> >>   g_error("error configuring signal handler: %s", strerror(errno));
> >> -return false;
> > Good catch, but we should really drop g_error() usage as qemu-ga will not
> > fail gracefully otherwise (will leak the pidfile, for example). We either
> > just drop g_error() or replace it by fprintf().
> 
> Isn't that a classical case of an error which should never occur,
> something which could also be handled by an assert statement?
> 
> I don't expect a graceful exit after such errors. If they occur,
> that's something which must be fixed in the code.
> 
> When I read the documentation of sigaction, I don't see how
> it could fail with the given function arguments.
> 
> Therefore I'd apply the patch as it is.

Yes, taking a look at the sigaction() manpages shows you're obviously
correct. Please, disregard what I've said.

Re: [Qemu-devel] [PATCH] json-parser: Fix potential NULL pointer segfault

[Luiz Capitulino]

On Mon, 03 Sep 2012 19:14:27 +0200
Stefan Weil  wrote:

> Am 03.09.2012 18:53, schrieb Stefan Weil:
> > Am 03.09.2012 18:41, schrieb Luiz Capitulino:
> >> On Sat,  1 Sep 2012 12:52:58 +0200
> >> Stefan Weil  wrote:
> >>
> >>> Report from smatch:
> >>> json-parser.c:474 parse_object(62) error: potential null derefence 
> >>> 'dict'.
> >>> json-parser.c:553 parse_array(75) error: potential null derefence 
> >>> 'list'.
> >>>
> >>> Label out can be called with list == NULL.
> >>>
> >>> Signed-off-by: Stefan Weil 
> >>> ---
> >>>   json-parser.c |8 ++--
> >>>   1 file changed, 6 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/json-parser.c b/json-parser.c
> >>> index 457291b..c31c759 100644
> >>> --- a/json-parser.c
> >>> +++ b/json-parser.c
> >>> @@ -471,7 +471,9 @@ static QObject *parse_object(JSONParserContext 
> >>> *ctxt, va_list *ap)
> >>> out:
> >>>   parser_context_restore(ctxt, saved_ctxt);
> >>> -QDECREF(dict);
> >>> +if (dict) {
> >>> +QDECREF(dict);
> >>> +}
> >>
> >> I prefer changing QDECREF() to a nop if obj is NULL.
> >
> > That's fine for me, too. If everybody agrees, I'll send two new
> > patches: one to change QDECREF, one to remove the if statements
> > from other code locations which use the same pattern as
> > my original patch.
> >
> > Cheers,
> >
> > - sw
> >
> >
> 
> What about modifying QOBJECT to return NULL if called with a NULL pointer?
> That would be a more generic fix for the same problem.

I don't like this because it's not obvious, besides, at least in theory
we'd have to change QOBJECT() users to check its return value.

On the other hand, QDECREF() is expected to do nothing if its argument is
NULL.

[Qemu-devel] [PATCH 4/5] Set a8 capability in the QXL device if supported by the client

[Søren Sandmann]

From: Søren Sandmann Pedersen 

---
 server/red_worker.c |2 ++
 spice-common|2 +-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index f87967c..17d9ef8 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -10377,6 +10377,8 @@ static void handle_new_display_channel(RedWorker 
*worker, RedClient *client, Red
 SET_CAP(caps, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
 if (red_channel_client_test_remote_cap(rcc, 
SPICE_DISPLAY_CAP_COMPOSITE))
 SET_CAP(caps, SPICE_DISPLAY_CAP_COMPOSITE);
+if (red_channel_client_test_remote_cap(rcc, 
SPICE_DISPLAY_CAP_COMPOSITE))
+SET_CAP(caps, SPICE_DISPLAY_CAP_A8_SURFACE);
 
 worker->qxl->st->qif->set_client_capabilities(worker->qxl, TRUE, caps);
 }
diff --git a/spice-common b/spice-common
index 86e286b..04dc2be 16
--- a/spice-common
+++ b/spice-common
@@ -1 +1 @@
-Subproject commit 86e286ba2003c206e700fd70ec67c1cf4ac8d8a6
+Subproject commit 04dc2bee9ecdda7d7966f9267df37ab23bb5a802
-- 
1.7.4

[Qemu-devel] [PATCH 3/5] Process outstanding commands in the ring after changing capability bits

[Søren Sandmann]

From: Søren Sandmann Pedersen 

When a new client connects, there may be commands in the ring that it
can't understand, so we need to process these before forwarding new
commands to the client. By doing this after changing the capability
bits we ensure that the new client will never see a command that it
doesn't understand (under the assumption that the guest will read and
obey the capability bits).
---
 server/red_worker.c |5 +
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index 60b5471..f87967c 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -9493,6 +9493,11 @@ static void 
on_new_display_channel_client(DisplayChannelClient *dcc)
 }
 red_channel_client_ack_zero_messages_window(&dcc->common.base);
 if (worker->surfaces[0].context.canvas) {
+int ring_is_empty;
+
+while (red_process_commands(worker, MAX_PIPE_SIZE, &ring_is_empty)) {
+}
+
 red_current_flush(worker, 0);
 push_new_primary_surface(dcc);
 red_push_surface_image(dcc, 0);
-- 
1.7.4

[Qemu-devel] [PATCH 1/5] client: Advertise A8_SURFACE capability

[Søren Sandmann]

From: Søren Sandmann Pedersen 

---
 client/display_channel.cpp |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/client/display_channel.cpp b/client/display_channel.cpp
index d08072d..49a4c6a 100644
--- a/client/display_channel.cpp
+++ b/client/display_channel.cpp
@@ -652,6 +652,7 @@ DisplayChannel::DisplayChannel(RedClient& client, uint32_t 
id,
 set_draw_handlers();
 
 set_capability(SPICE_DISPLAY_CAP_COMPOSITE);
+set_capability(SPICE_DISPLAY_CAP_A8_SURFACE);
 }
 
 DisplayChannel::~DisplayChannel()
-- 
1.7.4

[Qemu-devel] [PATCH 5/5] Bump spice.h version number to 0.11.4

[Søren Sandmann]

From: Søren Sandmann Pedersen 

No new symbols are added, but there is an addition to QXLInterface:

void (*set_client_capabilities)(QXLInstance *qin,
uint8_t client_present,
uint8_t caps[58]);
---
 server/spice.h |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/server/spice.h b/server/spice.h
index 9d65efa..71adfb6 100644
--- a/server/spice.h
+++ b/server/spice.h
@@ -22,7 +22,7 @@
 #include 
 #include 
 
-#define SPICE_SERVER_VERSION 0x000b02 /* release 0.11.2 */
+#define SPICE_SERVER_VERSION 0x000b04 /* release 0.11.4 */
 
 /* interface base type */
 
-- 
1.7.4

[Qemu-devel] [PATCH 2/5] Add new set_client_capabilities() interface to QXLInstance

[Søren Sandmann]

From: Søren Sandmann Pedersen 

A new interface

  set_client_capabilities (QXLInstance *qin,
   uint8_t client_present,
   uint8_t caps[58]);

is added to QXLInstance, and spice server is changed to call it
whenever a client connects or disconnects. The QXL device in response
is expected to update the client capability bits in the ROM of the
device and raise the QXL_INTERRUPT_CLIENT interrupt.

There is a potential race condition in the case where a client
disconnects and a new client with fewer capabilities connects. There
may be commands in the ring that the new client can't handle. This
case is handled by first changing the capability bits, then processing
all commands in the ring, and then start forwarding commands to the
new client. As long as the guest obeys the capability bits, the new
client will never see anything it doesn't understand.
---
 server/red_worker.c |   24 
 server/spice.h  |3 +++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index 133ba94..60b5471 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -10359,6 +10359,23 @@ static void handle_new_display_channel(RedWorker 
*worker, RedClient *client, Red
 spice_info("jpeg %s", display_channel->enable_jpeg ? "enabled" : 
"disabled");
 spice_info("zlib-over-glz %s", display_channel->enable_zlib_glz_wrap ? 
"enabled" : "disabled");
 
+if (worker->qxl->st->qif->set_client_capabilities) {
+RedChannelClient *rcc = (RedChannelClient *)dcc;
+uint8_t caps[58] = { 0 };
+
+#define SET_CAP(a,c)\
+((a)[(c) / 8] |= (1 << ((c) % 8)))
+
+if (red_channel_client_test_remote_cap(rcc, 
SPICE_DISPLAY_CAP_SIZED_STREAM))
+SET_CAP(caps, SPICE_DISPLAY_CAP_SIZED_STREAM);
+if (red_channel_client_test_remote_cap(rcc, 
SPICE_DISPLAY_CAP_MONITORS_CONFIG))
+SET_CAP(caps, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
+if (red_channel_client_test_remote_cap(rcc, 
SPICE_DISPLAY_CAP_COMPOSITE))
+SET_CAP(caps, SPICE_DISPLAY_CAP_COMPOSITE);
+
+worker->qxl->st->qif->set_client_capabilities(worker->qxl, TRUE, caps);
+}
+
 // todo: tune level according to bandwidth
 display_channel->zlib_level = ZLIB_DEFAULT_COMPRESSION_LEVEL;
 red_display_client_init_streams(dcc);
@@ -11213,9 +11230,16 @@ void handle_dev_display_disconnect(void *opaque, void 
*payload)
 {
 RedWorkerMessageDisplayDisconnect *msg = payload;
 RedChannelClient *rcc = msg->rcc;
+RedWorker *worker = opaque;
 
 spice_info("disconnect display client");
 spice_assert(rcc);
+
+if (worker->qxl->st->qif->set_client_capabilities) {
+uint8_t caps[58] = { 0 };
+worker->qxl->st->qif->set_client_capabilities(worker->qxl, FALSE, 
caps);
+}
+
 red_channel_client_disconnect(rcc);
 }
 
diff --git a/server/spice.h b/server/spice.h
index 0dc9d05..9d65efa 100644
--- a/server/spice.h
+++ b/server/spice.h
@@ -239,6 +239,9 @@ struct QXLInterface {
 void (*update_area_complete)(QXLInstance *qin, uint32_t surface_id,
  struct QXLRect *updated_rects,
  uint32_t num_updated_rects);
+void (*set_client_capabilities)(QXLInstance *qin,
+   uint8_t client_present,
+   uint8_t caps[58]);
 };
 
 struct QXLInstance {
-- 
1.7.4

Re: [Qemu-devel] [PATCH-v2 spice 1/2] Set a8 capability in the QXL device if supported by the client

[Søren Sandmann]

Søren Sandmann  writes:

> Alon Levy  writes:
>
>>> ---
>>>  server/red_worker.c | 2 ++
>>>  spice-common| 2 +-
>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/server/red_worker.c b/server/red_worker.c
>>> index 843f559..23f3464 100644
>>> --- a/server/red_worker.c
>>> +++ b/server/red_worker.c
>>> @@ -10377,6 +10377,8 @@ static void
>>> handle_new_display_channel(RedWorker *worker, RedClient *client, Red
>>>  SET_CAP(caps, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
>>>  if (red_channel_client_test_remote_cap(rcc,
>>>  SPICE_DISPLAY_CAP_COMPOSITE))
>>>  SET_CAP(caps, SPICE_DISPLAY_CAP_COMPOSITE);
>>> +if (red_channel_client_test_remote_cap(rcc,
>>> SPICE_DISPLAY_CAP_COMPOSITE))
>>> +SET_CAP(caps, SPICE_DISPLAY_CAP_A8_SURFACE);
>>
>> Didn't you mean to test remote SPICE_DISPLAY_CAP_A8_SURFACE?
>
> Yes, good catch. I'll fix before pushing.

I realized that I didn't actually send the whole patch series for
spice-server. There are five patches rather than two.


Soren

[Qemu-devel] [PATCH] qxl: Add set_client_capabilities() interface to QXLInterface

[Søren Sandmann]

From: Søren Sandmann Pedersen 

This new interface lets spice server inform the guest whether

(a) a client is connected
(b) what capabilities the client has

There is a fixed number (464) of bits reserved for capabilities, and
when the capabilities bits change, the QXL_INTERRUPT_CLIENT interrupt
is generated.

Signed-off-by: Soren Sandmann 
---
 hw/qxl.c |   27 +++
 hw/qxl.h |2 +-
 2 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index c2dd3b4..ffe1a76 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -901,6 +901,26 @@ static void interface_async_complete(QXLInstance *sin, 
uint64_t cookie_token)
 }
 }
 
+#if SPICE_SERVER_VERSION >= 0x000b04
+
+/* called from spice server thread context only */
+static void interface_set_client_capabilities(QXLInstance *sin,
+ uint8_t client_present,
+ uint8_t caps[58])
+{
+PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+
+qxl->shadow_rom.client_present = client_present;
+memcpy(qxl->shadow_rom.client_capabilities, caps, sizeof(caps));
+qxl->rom->client_present = client_present;
+memcpy(qxl->rom->client_capabilities, caps, sizeof(caps));
+qxl_rom_set_dirty(qxl);
+
+qxl_send_events(qxl, QXL_INTERRUPT_CLIENT);
+}
+
+#endif
+
 static const QXLInterface qxl_interface = {
 .base.type   = SPICE_INTERFACE_QXL,
 .base.description= "qxl gpu",
@@ -922,6 +942,9 @@ static const QXLInterface qxl_interface = {
 .flush_resources = interface_flush_resources,
 .async_complete  = interface_async_complete,
 .update_area_complete= interface_update_area_complete,
+#if SPICE_SERVER_VERSION >= 0x000b04
+.set_client_capabilities = interface_set_client_capabilities,
+#endif
 };
 
 static void qxl_enter_vga_mode(PCIQXLDevice *d)
@@ -1785,6 +1808,10 @@ static int qxl_init_common(PCIQXLDevice *qxl)
 io_size = 16;
 break;
 case 3: /* qxl-3 */
+   pci_device_rev = QXL_REVISION_STABLE_V10;
+   io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
+   break;
+case 4:
 default:
 pci_device_rev = QXL_DEFAULT_REVISION;
 io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
diff --git a/hw/qxl.h b/hw/qxl.h
index 172baf6..98d5a64 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -128,7 +128,7 @@ typedef struct PCIQXLDevice {
 }   \
 } while (0)
 
-#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
+#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V12
 
 /* qxl.c */
 void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
-- 
1.7.4

Re: [Qemu-devel] [Spice-devel] [PATCH-v2 qemu] qxl: Add set_client_capabilities() interface to QXLInterface

[Søren Sandmann]

Søren Sandmann Pedersen  writes:

> @@ -1292,7 +1315,7 @@ static void qxl_set_mode(PCIQXLDevice *d, int modenr, 
> int loadvm)
>  
>  d->mode = QXL_MODE_COMPAT;
>  d->cmdflags = QXL_COMMAND_FLAG_COMPAT;
> -#ifdef QXL_COMMAND_FLAG_COMPAT_16BPP /* new in spice 0.6.1 */
> +#if QXL_COMMAND_FLAG_COMPAT_16BPP /* new in spice 0.6.1 */
>  if (mode->bits == 16) {
>  d->cmdflags |= QXL_COMMAND_FLAG_COMPAT_16BPP;
>  }

This one is obviously a mistake. New patch to follow.


Soren

Re: [Qemu-devel] [PATCH-v2 spice 1/2] Set a8 capability in the QXL device if supported by the client

[Søren Sandmann]

Alon Levy  writes:

>> ---
>>  server/red_worker.c | 2 ++
>>  spice-common| 2 +-
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/server/red_worker.c b/server/red_worker.c
>> index 843f559..23f3464 100644
>> --- a/server/red_worker.c
>> +++ b/server/red_worker.c
>> @@ -10377,6 +10377,8 @@ static void
>> handle_new_display_channel(RedWorker *worker, RedClient *client, Red
>>  SET_CAP(caps, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
>>  if (red_channel_client_test_remote_cap(rcc,
>>  SPICE_DISPLAY_CAP_COMPOSITE))
>>  SET_CAP(caps, SPICE_DISPLAY_CAP_COMPOSITE);
>> +if (red_channel_client_test_remote_cap(rcc,
>> SPICE_DISPLAY_CAP_COMPOSITE))
>> +SET_CAP(caps, SPICE_DISPLAY_CAP_A8_SURFACE);
>
> Didn't you mean to test remote SPICE_DISPLAY_CAP_A8_SURFACE?

Yes, good catch. I'll fix before pushing.

Thanks,
Soren

Re: [Qemu-devel] [PATCH] json-parser: Fix potential NULL pointer segfault

[Stefan Weil]

Am 03.09.2012 18:53, schrieb Stefan Weil:

Am 03.09.2012 18:41, schrieb Luiz Capitulino:

On Sat,  1 Sep 2012 12:52:58 +0200
Stefan Weil  wrote:


Report from smatch:
json-parser.c:474 parse_object(62) error: potential null derefence 
'dict'.
json-parser.c:553 parse_array(75) error: potential null derefence 
'list'.


Label out can be called with list == NULL.

Signed-off-by: Stefan Weil 
---
  json-parser.c |8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/json-parser.c b/json-parser.c
index 457291b..c31c759 100644
--- a/json-parser.c
+++ b/json-parser.c
@@ -471,7 +471,9 @@ static QObject *parse_object(JSONParserContext 
*ctxt, va_list *ap)

out:
  parser_context_restore(ctxt, saved_ctxt);
-QDECREF(dict);
+if (dict) {
+QDECREF(dict);
+}


I prefer changing QDECREF() to a nop if obj is NULL.


That's fine for me, too. If everybody agrees, I'll send two new
patches: one to change QDECREF, one to remove the if statements
from other code locations which use the same pattern as
my original patch.

Cheers,

- sw




What about modifying QOBJECT to return NULL if called with a NULL pointer?
That would be a more generic fix for the same problem.

In either case, the code will be a little larger and slower,
but that should not matter because it is not time critical.

Regards,

Stefan W.

[Qemu-devel] [PATCH V7 8/8] hw/dma.c: replace register_ioport*

[Julien Grall]

This patch replaces all register_ioport* be the new memory API functions.
It permits to use the new Memory stuff like listener.

Signed-off-by: Julien Grall 
---
 hw/dma.c |  108 +
 1 files changed, 72 insertions(+), 36 deletions(-)

diff --git a/hw/dma.c b/hw/dma.c
index 0a9322d..59c0dac 100644
--- a/hw/dma.c
+++ b/hw/dma.c
@@ -58,6 +58,8 @@ static struct dma_cont {
 int dshift;
 struct dma_regs regs[4];
 qemu_irq *cpu_request_exit;
+MemoryRegion channel_io;
+MemoryRegion cont_io;
 } dma_controllers[2];
 
 enum {
@@ -149,7 +151,8 @@ static inline int getff (struct dma_cont *d)
 return ff;
 }
 
-static uint32_t read_chan (void *opaque, uint32_t nport)
+static uint64_t read_chan(void *opaque, target_phys_addr_t nport,
+  unsigned size)
 {
 struct dma_cont *d = opaque;
 int ichan, nreg, iport, ff, val, dir;
@@ -171,7 +174,8 @@ static uint32_t read_chan (void *opaque, uint32_t nport)
 return (val >> (d->dshift + (ff << 3))) & 0xff;
 }
 
-static void write_chan (void *opaque, uint32_t nport, uint32_t data)
+static void write_chan(void *opaque, target_phys_addr_t nport, uint64_t data,
+   unsigned size)
 {
 struct dma_cont *d = opaque;
 int iport, ichan, nreg;
@@ -189,22 +193,23 @@ static void write_chan (void *opaque, uint32_t nport, 
uint32_t data)
 }
 }
 
-static void write_cont (void *opaque, uint32_t nport, uint32_t data)
+static void write_cont(void *opaque, target_phys_addr_t nport, uint64_t data,
+   unsigned size)
 {
 struct dma_cont *d = opaque;
 int iport, ichan = 0;
 
 iport = (nport >> d->dshift) & 0x0f;
 switch (iport) {
-case 0x08:  /* command */
+case 0x01:  /* command */
 if ((data != 0) && (data & CMD_NOT_SUPPORTED)) {
-dolog ("command %#x not supported\n", data);
+dolog("command %#lx not supported\n", data);
 return;
 }
 d->command = data;
 break;
 
-case 0x09:
+case 0x02:
 ichan = data & 3;
 if (data & 4) {
 d->status |= 1 << (ichan + 4);
@@ -216,7 +221,7 @@ static void write_cont (void *opaque, uint32_t nport, 
uint32_t data)
 DMA_run();
 break;
 
-case 0x0a:  /* single mask */
+case 0x03:  /* single mask */
 if (data & 4)
 d->mask |= 1 << (data & 3);
 else
@@ -224,7 +229,7 @@ static void write_cont (void *opaque, uint32_t nport, 
uint32_t data)
 DMA_run();
 break;
 
-case 0x0b:  /* mode */
+case 0x04:  /* mode */
 {
 ichan = data & 3;
 #ifdef DEBUG_DMA
@@ -243,23 +248,23 @@ static void write_cont (void *opaque, uint32_t nport, 
uint32_t data)
 break;
 }
 
-case 0x0c:  /* clear flip flop */
+case 0x05:  /* clear flip flop */
 d->flip_flop = 0;
 break;
 
-case 0x0d:  /* reset */
+case 0x06:  /* reset */
 d->flip_flop = 0;
 d->mask = ~0;
 d->status = 0;
 d->command = 0;
 break;
 
-case 0x0e:  /* clear mask for all channels */
+case 0x07:  /* clear mask for all channels */
 d->mask = 0;
 DMA_run();
 break;
 
-case 0x0f:  /* write mask for all channels */
+case 0x08:  /* write mask for all channels */
 d->mask = data;
 DMA_run();
 break;
@@ -277,7 +282,8 @@ static void write_cont (void *opaque, uint32_t nport, 
uint32_t data)
 #endif
 }
 
-static uint32_t read_cont (void *opaque, uint32_t nport)
+static uint64_t read_cont(void *opaque, target_phys_addr_t nport,
+  unsigned size)
 {
 struct dma_cont *d = opaque;
 int iport, val;
@@ -463,7 +469,7 @@ void DMA_schedule(int nchan)
 static void dma_reset(void *opaque)
 {
 struct dma_cont *d = opaque;
-write_cont (d, (0x0d << d->dshift), 0);
+write_cont(d, (0x06 << d->dshift), 0, 1);
 }
 
 static int dma_phony_handler (void *opaque, int nchan, int dma_pos, int 
dma_len)
@@ -473,38 +479,68 @@ static int dma_phony_handler (void *opaque, int nchan, 
int dma_pos, int dma_len)
 return dma_pos;
 }
 
+
+static const MemoryRegionOps channel_io_ops = {
+.read = read_chan,
+.write = write_chan,
+.endianness = DEVICE_NATIVE_ENDIAN,
+.impl = {
+.min_access_size = 1,
+.max_access_size = 1,
+},
+};
+
+/* IOport from page_base */
+static const MemoryRegionPortio page_portio_list[] = {
+{ 0x01, 3, 1, .write = write_page, .read = read_page, },
+{ 0x07, 1, 1, .write = write_page, .read = read_page, },
+PORTIO_END_OF_LIST(),
+};
+
+/* IOport from pageh_base */
+static const MemoryRegionPortio pageh_portio_list[] = 

Re: [Qemu-devel] [PATCH 0/2 v3] Fix static linking for cURL and SDL

[Yann E. MORIN]

Peter, All,

On Monday 03 September 2012 18:38:48 Peter Maydell wrote:
> On 3 September 2012 17:28, Yann E. MORIN  wrote:
> > On Monday 03 September 2012 17:44:51 Peter Maydell wrote:
> >> Personally I think it might indeed be a good idea to just say
> >> "statically linked softmmu isn't supported" and forbid it, unless
> >> somebody has a good use case for it...
> >
> > I personnally have such a use-case: provide run-everywhere qemu softmmu
> > binaries that do have minimal requirements on the distro they'll be
> > running on, especially when the distros can be very different (in kinds
> > and in age).
> 
> ...have you audited all of qemu's library usage to confirm that it
> doesn't use any of the bits of glibc that don't work in the static
> linking scenario? I'm reasonably confident that the linux-user binaries
> are OK, much less so for the system binaries...

I guess you mean the nsswitch stuff?

I'm using a uClibc-based toolchain that does not have such libs, and allows
a truly static link.

Regards,
Yann E. MORIN.

-- 
.-..--..
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN |  ___   |
| +33 223 225 172 `.---:  X  AGAINST  |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL|   v   conspiracy.  |
'--^---^--^'

Re: [Qemu-devel] [PATCH] qemu-ga: Remove unreachable code after g_error

[Stefan Weil]

Am 03.09.2012 18:49, schrieb Luiz Capitulino:

On Sat,  1 Sep 2012 09:34:15 +0200
Stefan Weil  wrote:


Report from smatch:
qemu-ga.c:117 register_signal_handlers(11) info: ignoring unreachable code.
qemu-ga.c:122 register_signal_handlers(16) info: ignoring unreachable code.

g_error calls abort which terminates the program.

Signed-off-by: Stefan Weil 
---
  qemu-ga.c |2 --
  1 file changed, 2 deletions(-)

diff --git a/qemu-ga.c b/qemu-ga.c
index 7623079..b747470 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -114,12 +114,10 @@ static gboolean register_signal_handlers(void)
  ret = sigaction(SIGINT, &sigact, NULL);
  if (ret == -1) {
  g_error("error configuring signal handler: %s", strerror(errno));
-return false;

Good catch, but we should really drop g_error() usage as qemu-ga will not
fail gracefully otherwise (will leak the pidfile, for example). We either
just drop g_error() or replace it by fprintf().


Isn't that a classical case of an error which should never occur,
something which could also be handled by an assert statement?

I don't expect a graceful exit after such errors. If they occur,
that's something which must be fixed in the code.

When I read the documentation of sigaction, I don't see how
it could fail with the given function arguments.

Therefore I'd apply the patch as it is.

Regards,

- sw

[Qemu-devel] [PATCH V7 1/8] isa: add isa_address_space_io

[Julien Grall]

This function permits to retrieve ISA IO address space.
It will be usefull when we need to pass IO address space as argument.

Signed-off-by: Julien Grall 
---
 hw/isa-bus.c |9 +
 hw/isa.h |1 +
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index f9b2373..c1d8309 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -244,4 +244,13 @@ MemoryRegion *isa_address_space(ISADevice *dev)
 return get_system_memory();
 }
 
+MemoryRegion *isa_address_space_io(ISADevice *dev)
+{
+if (dev) {
+return isa_bus_from_device(dev)->address_space_io;
+}
+
+return isabus->address_space_io;
+}
+
 type_init(isabus_register_types)
diff --git a/hw/isa.h b/hw/isa.h
index dc97052..3891c1f 100644
--- a/hw/isa.h
+++ b/hw/isa.h
@@ -43,6 +43,7 @@ void isa_bus_irqs(ISABus *bus, qemu_irq *irqs);
 qemu_irq isa_get_irq(ISADevice *dev, int isairq);
 void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq);
 MemoryRegion *isa_address_space(ISADevice *dev);
+MemoryRegion *isa_address_space_io(ISADevice *dev);
 ISADevice *isa_create(ISABus *bus, const char *name);
 ISADevice *isa_try_create(ISABus *bus, const char *name);
 ISADevice *isa_create_simple(ISABus *bus, const char *name);
-- 
Julien Grall

[Qemu-devel] [PATCH V7 7/8] hw/pc.c: replace register_ioport*

[Julien Grall]

This patch replaces all register_ioport* with portio_* or
isa_register_portio_list. It permits to use the new Memory
stuff like listener.

Signed-off-by: Julien Grall 
---
 hw/pc.c |   58 +++---
 1 files changed, 43 insertions(+), 15 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 112739a..4abd4e2 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -104,7 +104,8 @@ void gsi_handler(void *opaque, int n, int level)
 qemu_set_irq(s->ioapic_irq[n], level);
 }
 
-static void ioport80_write(void *opaque, uint32_t addr, uint32_t data)
+static void ioport80_write(void *opaque, target_phys_addr_t addr,
+   uint64_t data, unsigned size)
 {
 }
 
@@ -122,7 +123,8 @@ void cpu_set_ferr(CPUX86State *s)
 qemu_irq_raise(ferr_irq);
 }
 
-static void ioportF0_write(void *opaque, uint32_t addr, uint32_t data)
+static void ioportF0_write(void *opaque, target_phys_addr_t addr,
+   uint64_t data, unsigned size)
 {
 qemu_irq_lower(ferr_irq);
 }
@@ -588,6 +590,17 @@ int e820_add_entry(uint64_t address, uint64_t length, 
uint32_t type)
 return index;
 }
 
+static const MemoryRegionPortio bochs_bios_portio_list[] = {
+{ 0x400, 2, 2, .write = bochs_bios_write, }, /* 0x400 */
+{ 0x402, 2, 1, .write = bochs_bios_write, }, /* 0x402 */
+{ 0x500, 1, 1, .write = bochs_bios_write, }, /* 0x500 */
+{ 0x501, 1, 1, .write = bochs_bios_write, }, /* 0x501 */
+{ 0x501, 2, 2, .write = bochs_bios_write, }, /* 0x501 */
+{ 0x503, 1, 1, .write = bochs_bios_write, }, /* 0x503  */
+{ 0x8900, 1, 1, .write = bochs_bios_write, }, /* 0x8900 */
+PORTIO_END_OF_LIST(),
+};
+
 static void *bochs_bios_init(void)
 {
 void *fw_cfg;
@@ -595,18 +608,11 @@ static void *bochs_bios_init(void)
 size_t smbios_len;
 uint64_t *numa_fw_cfg;
 int i, j;
+PortioList *bochs_bios_port_list = g_new(PortioList, 1);
 
-register_ioport_write(0x400, 1, 2, bochs_bios_write, NULL);
-register_ioport_write(0x401, 1, 2, bochs_bios_write, NULL);
-register_ioport_write(0x402, 1, 1, bochs_bios_write, NULL);
-register_ioport_write(0x403, 1, 1, bochs_bios_write, NULL);
-register_ioport_write(0x8900, 1, 1, bochs_bios_write, NULL);
-
-register_ioport_write(0x501, 1, 1, bochs_bios_write, NULL);
-register_ioport_write(0x501, 1, 2, bochs_bios_write, NULL);
-register_ioport_write(0x502, 1, 2, bochs_bios_write, NULL);
-register_ioport_write(0x500, 1, 1, bochs_bios_write, NULL);
-register_ioport_write(0x503, 1, 1, bochs_bios_write, NULL);
+portio_list_init(bochs_bios_port_list, bochs_bios_portio_list,
+ NULL, "bosch-bios");
+portio_list_add(bochs_bios_port_list, get_system_io(), 0x0);
 
 fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
 
@@ -1059,6 +1065,24 @@ static void cpu_request_exit(void *opaque, int irq, int 
level)
 }
 }
 
+static const MemoryRegionOps ioport80_io_ops = {
+.write = ioport80_write,
+.endianness = DEVICE_NATIVE_ENDIAN,
+.impl = {
+.min_access_size = 1,
+.max_access_size = 1,
+},
+};
+
+static const MemoryRegionOps ioportF0_io_ops = {
+.write = ioportF0_write,
+.endianness = DEVICE_NATIVE_ENDIAN,
+.impl = {
+.min_access_size = 1,
+.max_access_size = 1,
+},
+};
+
 void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
   ISADevice **rtc_state,
   ISADevice **floppy,
@@ -1073,10 +1097,14 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq 
*gsi,
 qemu_irq *a20_line;
 ISADevice *i8042, *port92, *vmmouse, *pit = NULL;
 qemu_irq *cpu_exit_irq;
+MemoryRegion *ioport80_io = g_new(MemoryRegion, 1);
+MemoryRegion *ioportF0_io = g_new(MemoryRegion, 1);
 
-register_ioport_write(0x80, 1, 1, ioport80_write, NULL);
+memory_region_init_io(ioport80_io, &ioport80_io_ops, NULL, "ioport80", 1);
+memory_region_add_subregion(isa_bus->address_space_io, 0x80, ioport80_io);
 
-register_ioport_write(0xf0, 1, 1, ioportF0_write, NULL);
+memory_region_init_io(ioportF0_io, &ioportF0_io_ops, NULL, "ioportF0", 1);
+memory_region_add_subregion(isa_bus->address_space_io, 0xf0, ioportF0_io);
 
 /*
  * Check if an HPET shall be created.
-- 
Julien Grall

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Paolo Bonzini]

Il 03/09/2012 18:40, Jan Kiszka ha scritto:
>>> >> And the migration fails.  Needlessly, since icw3 == 0 doesn't affect
>>> >> guest operation.
>> > 
>> > But the point of subsections is to succeed migration in the common case,
>> > assuming there is more than one case that doesn't affect guest operation.
> The point is that the common case is icw3 = 4 (for the master), not 0.
> And if we don't save that case, we must save the rest. If we don't save
> this as well, we lose state information. This can't work.

I was agreeing with you, not Avi. :)

Paolo

Re: [Qemu-devel] [PATCH] json-parser: Fix potential NULL pointer segfault

[Stefan Weil]

Am 03.09.2012 18:41, schrieb Luiz Capitulino:

On Sat,  1 Sep 2012 12:52:58 +0200
Stefan Weil  wrote:


Report from smatch:
json-parser.c:474 parse_object(62) error: potential null derefence 'dict'.
json-parser.c:553 parse_array(75) error: potential null derefence 'list'.

Label out can be called with list == NULL.

Signed-off-by: Stefan Weil 
---
  json-parser.c |8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/json-parser.c b/json-parser.c
index 457291b..c31c759 100644
--- a/json-parser.c
+++ b/json-parser.c
@@ -471,7 +471,9 @@ static QObject *parse_object(JSONParserContext *ctxt, 
va_list *ap)
  
  out:

  parser_context_restore(ctxt, saved_ctxt);
-QDECREF(dict);
+if (dict) {
+QDECREF(dict);
+}


I prefer changing QDECREF() to a nop if obj is NULL.


That's fine for me, too. If everybody agrees, I'll send two new
patches: one to change QDECREF, one to remove the if statements
from other code locations which use the same pattern as
my original patch.

Cheers,

- sw

Re: [Qemu-devel] [PATCH] qapi: Fix potential NULL pointer segfault

[Luiz Capitulino]

On Mon, 03 Sep 2012 18:49:54 +0200
Stefan Weil  wrote:

> Am 03.09.2012 18:34, schrieb Luiz Capitulino:
> > On Mon, 03 Sep 2012 08:57:36 +0200
> > Paolo Bonzini  wrote:
> >
> >> Il 01/09/2012 09:30, Stefan Weil ha scritto:
> >>> Report from smatch:
> >>>
> >>> qapi-visit.c:1640 visit_type_BlockdevAction(8) error:
> >>>   we previously assumed 'obj' could be null (see line 1639)
> >>> qapi-visit.c:2432 visit_type_NetClientOptions(8) error:
> >>>   we previously assumed 'obj' could be null (see line 2431)
> >>>
> >>> Signed-off-by: Stefan Weil 
> >>> ---
> >>>   scripts/qapi-visit.py |2 +-
> >>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> >>> index 2afc5c0..1a669f3 100644
> >>> --- a/scripts/qapi-visit.py
> >>> +++ b/scripts/qapi-visit.py
> >>> @@ -157,7 +157,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, 
> >>> const char *name, Error **
> >>>   if (!error_is_set(errp)) {
> >>>   visit_start_struct(m, (void **)obj, "%(name)s", name, 
> >>> sizeof(%(name)s), &err);
> >>>   if (!err) {
> >>> -if (!obj || *obj) {
> >>> +if (obj && *obj) {
> >>>   visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err);
> >>>   if (!err) {
> >>>   switch ((*obj)->kind) {
> >>>
> >>
> >> Reviewed-by: Paolo Bonzini 
> >
> > Is this for 1.2?
> >
> > Although the fix is pretty obvious, it doesn't seem possible to trigger the
> > segfault today and I believe we're only accepting true bug fixes at this 
> > point
> > (ie. two days from the release).
> 
> As long as nobody has a scenario which triggers the bug,
> there is no need to apply that patch before 1.2 is released.
> 
> That's why I did not add "for 1.2" to the subject line.

Applied to qmp-next, thanks.

Re: [Qemu-devel] [PATCH] qapi: Fix potential NULL pointer segfault

[Stefan Weil]

Am 03.09.2012 18:34, schrieb Luiz Capitulino:

On Mon, 03 Sep 2012 08:57:36 +0200
Paolo Bonzini  wrote:


Il 01/09/2012 09:30, Stefan Weil ha scritto:

Report from smatch:

qapi-visit.c:1640 visit_type_BlockdevAction(8) error:
  we previously assumed 'obj' could be null (see line 1639)
qapi-visit.c:2432 visit_type_NetClientOptions(8) error:
  we previously assumed 'obj' could be null (see line 2431)

Signed-off-by: Stefan Weil 
---
  scripts/qapi-visit.py |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 2afc5c0..1a669f3 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -157,7 +157,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const 
char *name, Error **
  if (!error_is_set(errp)) {
  visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), 
&err);
  if (!err) {
-if (!obj || *obj) {
+if (obj && *obj) {
  visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err);
  if (!err) {
  switch ((*obj)->kind) {



Reviewed-by: Paolo Bonzini 


Is this for 1.2?

Although the fix is pretty obvious, it doesn't seem possible to trigger the
segfault today and I believe we're only accepting true bug fixes at this point
(ie. two days from the release).


As long as nobody has a scenario which triggers the bug,
there is no need to apply that patch before 1.2 is released.

That's why I did not add "for 1.2" to the subject line.

- sw

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

On 2012-09-03 18:33, Paolo Bonzini wrote:
> Il 03/09/2012 18:30, Avi Kivity ha scritto:
>> The values above are what every user of the PIC cascaded on our targets
>> must program to use them. So We will find them in the state once any
>> relevant guest code was able to run (e.g. the BIOS).
>>
>> Suppose the bios has not run yet?

 Then you transmit the subsection.
>> And the migration fails.  Needlessly, since icw3 == 0 doesn't affect
>> guest operation.
> 
> But the point of subsections is to succeed migration in the common case,
> assuming there is more than one case that doesn't affect guest operation.

The point is that the common case is icw3 = 4 (for the master), not 0.
And if we don't save that case, we must save the rest. If we don't save
this as well, we lose state information. This can't work.

Jan

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

Re: [Qemu-devel] [PATCH] qemu-ga: Remove unreachable code after g_error

[Luiz Capitulino]

On Sat,  1 Sep 2012 09:34:15 +0200
Stefan Weil  wrote:

> Report from smatch:
> qemu-ga.c:117 register_signal_handlers(11) info: ignoring unreachable code.
> qemu-ga.c:122 register_signal_handlers(16) info: ignoring unreachable code.
> 
> g_error calls abort which terminates the program.
> 
> Signed-off-by: Stefan Weil 
> ---
>  qemu-ga.c |2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/qemu-ga.c b/qemu-ga.c
> index 7623079..b747470 100644
> --- a/qemu-ga.c
> +++ b/qemu-ga.c
> @@ -114,12 +114,10 @@ static gboolean register_signal_handlers(void)
>  ret = sigaction(SIGINT, &sigact, NULL);
>  if (ret == -1) {
>  g_error("error configuring signal handler: %s", strerror(errno));
> -return false;

Good catch, but we should really drop g_error() usage as qemu-ga will not
fail gracefully otherwise (will leak the pidfile, for example). We either
just drop g_error() or replace it by fprintf().

>  }
>  ret = sigaction(SIGTERM, &sigact, NULL);
>  if (ret == -1) {
>  g_error("error configuring signal handler: %s", strerror(errno));
> -return false;
>  }
>  
>  return true;

[Qemu-devel] [PATCH V7 6/8] hw/serial.c: replace register_ioport*

[Julien Grall]

This patch replaces all register_ioport* with a MemoryRegion. It permits to
use the new Memory stuff like listener.

For more flexibility, the IO address space is passed as an argument.

Signed-off-by: Julien Grall 
---
 hw/mips_mipssim.c |3 ++-
 hw/pc.h   |2 +-
 hw/serial.c   |8 +---
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/mips_mipssim.c b/hw/mips_mipssim.c
index 830f635..a204ab1 100644
--- a/hw/mips_mipssim.c
+++ b/hw/mips_mipssim.c
@@ -215,7 +215,8 @@ mips_mipssim_init (ram_addr_t ram_size,
 /* A single 16450 sits at offset 0x3f8. It is attached to
MIPS CPU INT2, which is interrupt 4. */
 if (serial_hds[0])
-serial_init(0x3f8, env->irq[4], 115200, serial_hds[0]);
+serial_init(0x3f8, env->irq[4], 115200, serial_hds[0],
+get_system_io());
 
 if (nd_table[0].used)
 /* MIPSnet uses the MIPS CPU INT0, which is interrupt 2. */
diff --git a/hw/pc.h b/hw/pc.h
index e4db071..f2b7af5 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -15,7 +15,7 @@
 /* serial.c */
 
 SerialState *serial_init(int base, qemu_irq irq, int baudbase,
- CharDriverState *chr);
+ CharDriverState *chr, MemoryRegion *system_io);
 SerialState *serial_mm_init(MemoryRegion *address_space,
 target_phys_addr_t base, int it_shift,
 qemu_irq irq, int baudbase,
diff --git a/hw/serial.c b/hw/serial.c
index a421d1e..4ed20c0 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -28,6 +28,7 @@
 #include "pc.h"
 #include "qemu-timer.h"
 #include "sysemu.h"
+#include "exec-memory.h"
 
 //#define DEBUG_SERIAL
 
@@ -810,7 +811,7 @@ static const VMStateDescription vmstate_isa_serial = {
 };
 
 SerialState *serial_init(int base, qemu_irq irq, int baudbase,
- CharDriverState *chr)
+ CharDriverState *chr, MemoryRegion *system_io)
 {
 SerialState *s;
 
@@ -823,8 +824,9 @@ SerialState *serial_init(int base, qemu_irq irq, int 
baudbase,
 
 vmstate_register(NULL, base, &vmstate_serial, s);
 
-register_ioport_write(base, 8, 1, serial_ioport_write, s);
-register_ioport_read(base, 8, 1, serial_ioport_read, s);
+memory_region_init_io(&s->io, &serial_io_ops, s, "serial", 8);
+memory_region_add_subregion(system_io, base, &s->io);
+
 return s;
 }
 
-- 
Julien Grall

[Qemu-devel] [PATCH V7 5/8] hw/cirrus_vga.c: replace register_ioport*

[Julien Grall]

This patch replaces all register_ioport* with portio_*. It permits to
use the new Memory stuff like listener.

Signed-off-by: Julien Grall 
---
 hw/cirrus_vga.c |   50 ++
 1 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index e8dcc6b..aa81f0b 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -200,6 +200,7 @@ typedef void (*cirrus_fill_t)(struct CirrusVGAState *s,
 typedef struct CirrusVGAState {
 VGACommonState vga;
 
+MemoryRegion cirrus_vga_io;
 MemoryRegion cirrus_linear_io;
 MemoryRegion cirrus_linear_bitblt_io;
 MemoryRegion cirrus_mmio_io;
@@ -2435,12 +2436,15 @@ static void cirrus_update_memory_access(CirrusVGAState 
*s)
 
 /* I/O ports */
 
-static uint32_t cirrus_vga_ioport_read(void *opaque, uint32_t addr)
+static uint64_t cirrus_vga_ioport_read(void *opaque, target_phys_addr_t addr,
+   unsigned size)
 {
 CirrusVGAState *c = opaque;
 VGACommonState *s = &c->vga;
 int val, index;
 
+addr += 0x3b0;
+
 if (vga_ioport_invalid(s, addr)) {
val = 0xff;
 } else {
@@ -2528,12 +2532,15 @@ static uint32_t cirrus_vga_ioport_read(void *opaque, 
uint32_t addr)
 return val;
 }
 
-static void cirrus_vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
+static void cirrus_vga_ioport_write(void *opaque, target_phys_addr_t addr,
+uint64_t val, unsigned size)
 {
 CirrusVGAState *c = opaque;
 VGACommonState *s = &c->vga;
 int index;
 
+addr += 0x3b0;
+
 /* check port range access depending on color/monochrome mode */
 if (vga_ioport_invalid(s, addr)) {
return;
@@ -2645,7 +2652,7 @@ static uint64_t cirrus_mmio_read(void *opaque, 
target_phys_addr_t addr,
 if (addr >= 0x100) {
 return cirrus_mmio_blt_read(s, addr - 0x100);
 } else {
-return cirrus_vga_ioport_read(s, addr + 0x3c0);
+return cirrus_vga_ioport_read(s, addr + 0x10, size);
 }
 }
 
@@ -2657,7 +2664,7 @@ static void cirrus_mmio_write(void *opaque, 
target_phys_addr_t addr,
 if (addr >= 0x100) {
cirrus_mmio_blt_write(s, addr - 0x100, val);
 } else {
-cirrus_vga_ioport_write(s, addr + 0x3c0, val);
+cirrus_vga_ioport_write(s, addr + 0x10, val, size);
 }
 }
 
@@ -2783,8 +2790,19 @@ static const MemoryRegionOps cirrus_linear_io_ops = {
 },
 };
 
+static const MemoryRegionOps cirrus_vga_io_ops = {
+.read = cirrus_vga_ioport_read,
+.write = cirrus_vga_ioport_write,
+.endianness = DEVICE_LITTLE_ENDIAN,
+.impl = {
+.min_access_size = 1,
+.max_access_size = 1,
+},
+};
+
 static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci,
-   MemoryRegion *system_memory)
+   MemoryRegion *system_memory,
+   MemoryRegion *system_io)
 {
 int i;
 static int inited;
@@ -2816,19 +2834,10 @@ static void cirrus_init_common(CirrusVGAState * s, int 
device_id, int is_pci,
 s->bustype = CIRRUS_BUSTYPE_ISA;
 }
 
-register_ioport_write(0x3c0, 16, 1, cirrus_vga_ioport_write, s);
-
-register_ioport_write(0x3b4, 2, 1, cirrus_vga_ioport_write, s);
-register_ioport_write(0x3d4, 2, 1, cirrus_vga_ioport_write, s);
-register_ioport_write(0x3ba, 1, 1, cirrus_vga_ioport_write, s);
-register_ioport_write(0x3da, 1, 1, cirrus_vga_ioport_write, s);
-
-register_ioport_read(0x3c0, 16, 1, cirrus_vga_ioport_read, s);
-
-register_ioport_read(0x3b4, 2, 1, cirrus_vga_ioport_read, s);
-register_ioport_read(0x3d4, 2, 1, cirrus_vga_ioport_read, s);
-register_ioport_read(0x3ba, 1, 1, cirrus_vga_ioport_read, s);
-register_ioport_read(0x3da, 1, 1, cirrus_vga_ioport_read, s);
+/* Register ioport 0x3b0 - 0x3df */
+memory_region_init_io(&s->cirrus_vga_io, &cirrus_vga_io_ops, s,
+  "cirrus-io", 0x30);
+memory_region_add_subregion(system_io, 0x3b0, &s->cirrus_vga_io);
 
 memory_region_init(&s->low_mem_container,
"cirrus-lowmem-container",
@@ -2896,7 +2905,7 @@ static int vga_initfn(ISADevice *dev)
 s->vram_size_mb = VGA_RAM_SIZE >> 20;
 vga_common_init(s);
 cirrus_init_common(&d->cirrus_vga, CIRRUS_ID_CLGD5430, 0,
-   isa_address_space(dev));
+   isa_address_space(dev), isa_address_space_io(dev));
 s->ds = graphic_console_init(s->update, s->invalidate,
  s->screen_dump, s->text_update,
  s);
@@ -2938,7 +2947,8 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
  /* setup VGA */
  s->vga.vram_size_mb = VGA_RAM_SIZE >> 20;
  vga_common_init(&s->vga);
- cirrus_init_common(s, device_id, 1, pci_address_space(dev));
+ cirrus_init_common(s, device_id, 1, pci_address_space(dev),
+  

[Qemu-devel] [PATCH V7 4/8] hw/acpi_piix4.c: replace register_ioport*

[Julien Grall]

This patch replaces all register_ioport* with the new memory API. It permits
to use the new Memory stuff like listener.

Signed-off-by: Julien Grall 
---
 hw/acpi_piix4.c |  151 +++
 1 files changed, 119 insertions(+), 32 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 1ad45ce..527dfc1 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -41,8 +41,7 @@
 
 #define GPE_BASE 0xafe0
 #define GPE_LEN 4
-#define PCI_UP_BASE 0xae00
-#define PCI_DOWN_BASE 0xae04
+#define PCI_BASE 0xae00
 #define PCI_EJ_BASE 0xae08
 #define PCI_RMV_BASE 0xae0c
 
@@ -55,7 +54,8 @@ struct pci_status {
 
 typedef struct PIIX4PMState {
 PCIDevice dev;
-IORange ioport;
+MemoryRegion pm_io;
+uint32_t pm_io_base;
 ACPIREGS ar;
 
 APMState apm;
@@ -64,6 +64,11 @@ typedef struct PIIX4PMState {
 uint32_t smb_io_base;
 
 MemoryRegion smb_io;
+MemoryRegion acpi_io;
+MemoryRegion acpi_hot_io;
+PortioList pci_hot_port_list;
+MemoryRegion pciej_hot_io;
+MemoryRegion pcirmv_hot_io;
 
 qemu_irq irq;
 qemu_irq smi_irq;
@@ -110,10 +115,10 @@ static void pm_tmr_timer(ACPIREGS *ar)
 pm_update_sci(s);
 }
 
-static void pm_ioport_write(IORange *ioport, uint64_t addr, unsigned width,
-uint64_t val)
+static void pm_ioport_write(void *opaque, target_phys_addr_t addr,
+uint64_t val, unsigned width)
 {
-PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
+PIIX4PMState *s = opaque;
 
 if (width != 2) {
 PIIX4_DPRINTF("PM write port=0x%04x width=%d val=0x%08x\n",
@@ -139,11 +144,11 @@ static void pm_ioport_write(IORange *ioport, uint64_t 
addr, unsigned width,
   (unsigned int)val);
 }
 
-static void pm_ioport_read(IORange *ioport, uint64_t addr, unsigned width,
-uint64_t *data)
+static uint64_t pm_ioport_read(void *opaque, target_phys_addr_t addr,
+   unsigned width)
 {
-PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
-uint32_t val;
+PIIX4PMState *s = opaque;
+uint64_t val;
 
 switch(addr) {
 case 0x00:
@@ -163,12 +168,18 @@ static void pm_ioport_read(IORange *ioport, uint64_t 
addr, unsigned width,
 break;
 }
 PIIX4_DPRINTF("PM readw port=0x%04x val=0x%04x\n", (unsigned int)addr, 
val);
-*data = val;
+
+return val;
 }
 
-static const IORangeOps pm_iorange_ops = {
+static const MemoryRegionOps pm_io_ops = {
 .read = pm_ioport_read,
 .write = pm_ioport_write,
+.endianness = DEVICE_LITTLE_ENDIAN,
+.impl = {
+.min_access_size = 2,
+.max_access_size = 2,
+},
 };
 
 static void apm_ctrl_changed(uint32_t val, void *arg)
@@ -185,7 +196,8 @@ static void apm_ctrl_changed(uint32_t val, void *arg)
 }
 }
 
-static void acpi_dbg_writel(void *opaque, uint32_t addr, uint32_t val)
+static void acpi_dbg_writel(void *opaque, target_phys_addr_t addr,
+uint64_t val, unsigned size)
 {
 PIIX4_DPRINTF("ACPI: DBG: 0x%08x\n", val);
 }
@@ -200,8 +212,18 @@ static void pm_io_space_update(PIIX4PMState *s)
 
 /* XXX: need to improve memory and ioport allocation */
 PIIX4_DPRINTF("PM: mapping to 0x%x\n", pm_io_base);
-iorange_init(&s->ioport, &pm_iorange_ops, pm_io_base, 64);
-ioport_register(&s->ioport);
+
+if (!s->pm_io_base) {
+memory_region_add_subregion(pci_address_space_io(&s->dev),
+pm_io_base, &s->pm_io);
+} else {
+memory_region_set_address(&s->pm_io, pm_io_base);
+}
+
+s->pm_io_base = pm_io_base;
+memory_region_set_enabled(&s->pm_io, true);
+} else {
+memory_region_set_enabled(&s->pm_io, false);
 }
 }
 
@@ -395,6 +417,15 @@ static const MemoryRegionOps smb_io_ops = {
 },
 };
 
+static const MemoryRegionOps acpi_io_ops = {
+.write = acpi_dbg_writel,
+.endianness = DEVICE_LITTLE_ENDIAN,
+.impl = {
+.min_access_size = 4,
+.max_access_size = 4,
+},
+};
+
 static int piix4_pm_initfn(PCIDevice *dev)
 {
 PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev, dev);
@@ -409,7 +440,9 @@ static int piix4_pm_initfn(PCIDevice *dev)
 /* APM */
 apm_init(dev, &s->apm, apm_ctrl_changed, s);
 
-register_ioport_write(ACPI_DBG_IO_ADDR, 4, 4, acpi_dbg_writel, s);
+memory_region_init_io(&s->acpi_io, &acpi_io_ops, s, "piix4-acpi", 4);
+memory_region_add_subregion(pci_address_space_io(dev), ACPI_DBG_IO_ADDR,
+&s->acpi_io);
 
 if (s->kvm_enabled) {
 /* Mark SMM as already inited to prevent SMM from running.  KVM does 
not
@@ -427,6 +460,11 @@ static int piix4_pm_initfn(PCIDevice *dev)
 memory_region_add_subregion(pci_address_space_io(dev), s->smb_io_base,
 &s->smb_io);
 
+/* PM  */
+memory_region_

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Paolo Bonzini]

Il 03/09/2012 18:30, Avi Kivity ha scritto:
> The values above are what every user of the PIC cascaded on our targets
> >>> > must program to use them. So We will find them in the state once any
> >>> > relevant guest code was able to run (e.g. the BIOS).
> >>> > 
>>> >> Suppose the bios has not run yet?
>> > 
>> > Then you transmit the subsection.
> And the migration fails.  Needlessly, since icw3 == 0 doesn't affect
> guest operation.

But the point of subsections is to succeed migration in the common case,
assuming there is more than one case that doesn't affect guest operation.

Paolo

Re: [Qemu-devel] [PATCH] qapi: Fix potential NULL pointer segfault

[Luiz Capitulino]

On Mon, 03 Sep 2012 08:57:36 +0200
Paolo Bonzini  wrote:

> Il 01/09/2012 09:30, Stefan Weil ha scritto:
> > Report from smatch:
> > 
> > qapi-visit.c:1640 visit_type_BlockdevAction(8) error:
> >  we previously assumed 'obj' could be null (see line 1639)
> > qapi-visit.c:2432 visit_type_NetClientOptions(8) error:
> >  we previously assumed 'obj' could be null (see line 2431)
> > 
> > Signed-off-by: Stefan Weil 
> > ---
> >  scripts/qapi-visit.py |2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> > index 2afc5c0..1a669f3 100644
> > --- a/scripts/qapi-visit.py
> > +++ b/scripts/qapi-visit.py
> > @@ -157,7 +157,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, 
> > const char *name, Error **
> >  if (!error_is_set(errp)) {
> >  visit_start_struct(m, (void **)obj, "%(name)s", name, 
> > sizeof(%(name)s), &err);
> >  if (!err) {
> > -if (!obj || *obj) {
> > +if (obj && *obj) {
> >  visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err);
> >  if (!err) {
> >  switch ((*obj)->kind) {
> > 
> 
> Reviewed-by: Paolo Bonzini 

Is this for 1.2?

Although the fix is pretty obvious, it doesn't seem possible to trigger the
segfault today and I believe we're only accepting true bug fixes at this point
(ie. two days from the release).

[Qemu-devel] [PATCH V7 3/8] smb: replace_register_ioport*

[Julien Grall]

This patch fix smb_ioport_* to be compliant with read/write memory callback.
Moreover it replaces all register_ioport* which use theses functions by
the new Memory API.

Signed-off-by: Julien Grall 
---
 hw/acpi_piix4.c |   18 --
 hw/pm_smbus.c   |7 ---
 hw/pm_smbus.h   |6 --
 hw/vt82c686.c   |   18 --
 4 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index fd2fc33..1ad45ce 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -63,6 +63,8 @@ typedef struct PIIX4PMState {
 PMSMBus smb;
 uint32_t smb_io_base;
 
+MemoryRegion smb_io;
+
 qemu_irq irq;
 qemu_irq smi_irq;
 int kvm_enabled;
@@ -383,6 +385,16 @@ static void piix4_pm_machine_ready(Notifier *n, void 
*opaque)
 
 }
 
+static const MemoryRegionOps smb_io_ops = {
+.read = smb_ioport_readb,
+.write = smb_ioport_writeb,
+.endianness = DEVICE_NATIVE_ENDIAN,
+.impl = {
+.min_access_size = 1,
+.max_access_size = 1,
+},
+};
+
 static int piix4_pm_initfn(PCIDevice *dev)
 {
 PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev, dev);
@@ -410,8 +422,10 @@ static int piix4_pm_initfn(PCIDevice *dev)
 pci_conf[0x90] = s->smb_io_base | 1;
 pci_conf[0x91] = s->smb_io_base >> 8;
 pci_conf[0xd2] = 0x09;
-register_ioport_write(s->smb_io_base, 64, 1, smb_ioport_writeb, &s->smb);
-register_ioport_read(s->smb_io_base, 64, 1, smb_ioport_readb, &s->smb);
+
+memory_region_init_io(&s->smb_io, &smb_io_ops, &s->smb, "piix4-smb", 64);
+memory_region_add_subregion(pci_address_space_io(dev), s->smb_io_base,
+&s->smb_io);
 
 acpi_pm_tmr_init(&s->ar, pm_tmr_timer);
 acpi_gpe_init(&s->ar, GPE_LEN);
diff --git a/hw/pm_smbus.c b/hw/pm_smbus.c
index 5d6046d..fe59ca6 100644
--- a/hw/pm_smbus.c
+++ b/hw/pm_smbus.c
@@ -94,7 +94,8 @@ static void smb_transaction(PMSMBus *s)
 s->smb_stat |= 0x04;
 }
 
-void smb_ioport_writeb(void *opaque, uint32_t addr, uint32_t val)
+void smb_ioport_writeb(void *opaque, target_phys_addr_t addr, uint64_t val,
+   unsigned size)
 {
 PMSMBus *s = opaque;
 addr &= 0x3f;
@@ -131,10 +132,10 @@ void smb_ioport_writeb(void *opaque, uint32_t addr, 
uint32_t val)
 }
 }
 
-uint32_t smb_ioport_readb(void *opaque, uint32_t addr)
+uint64_t smb_ioport_readb(void *opaque, target_phys_addr_t addr, unsigned size)
 {
 PMSMBus *s = opaque;
-uint32_t val;
+uint64_t val;
 
 addr &= 0x3f;
 switch(addr) {
diff --git a/hw/pm_smbus.h b/hw/pm_smbus.h
index 4750a40..45b4330 100644
--- a/hw/pm_smbus.h
+++ b/hw/pm_smbus.h
@@ -15,7 +15,9 @@ typedef struct PMSMBus {
 } PMSMBus;
 
 void pm_smbus_init(DeviceState *parent, PMSMBus *smb);
-void smb_ioport_writeb(void *opaque, uint32_t addr, uint32_t val);
-uint32_t smb_ioport_readb(void *opaque, uint32_t addr);
+void smb_ioport_writeb(void *opaque, target_phys_addr_t addr, uint64_t val,
+   unsigned size);
+uint64_t smb_ioport_readb(void *opaque, target_phys_addr_t addr,
+  unsigned size);
 
 #endif /* !PM_SMBUS_H */
diff --git a/hw/vt82c686.c b/hw/vt82c686.c
index 7f11dbe..3ad75ea 100644
--- a/hw/vt82c686.c
+++ b/hw/vt82c686.c
@@ -163,6 +163,7 @@ typedef struct VT686PMState {
 APMState apm;
 PMSMBus smb;
 uint32_t smb_io_base;
+MemoryRegion smb_io;
 } VT686PMState;
 
 typedef struct VT686AC97State {
@@ -405,6 +406,16 @@ static TypeInfo via_mc97_info = {
 .class_init= via_mc97_class_init,
 };
 
+static const MemoryRegionOps smb_io_ops = {
+.read = smb_ioport_readb,
+.write = smb_ioport_writeb,
+.endianness = DEVICE_NATIVE_ENDIAN,
+.impl = {
+.min_access_size = 1,
+.max_access_size = 1,
+},
+};
+
 /* vt82c686 pm init */
 static int vt82c686b_pm_initfn(PCIDevice *dev)
 {
@@ -424,8 +435,11 @@ static int vt82c686b_pm_initfn(PCIDevice *dev)
 pci_conf[0x90] = s->smb_io_base | 1;
 pci_conf[0x91] = s->smb_io_base >> 8;
 pci_conf[0xd2] = 0x90;
-register_ioport_write(s->smb_io_base, 0xf, 1, smb_ioport_writeb, &s->smb);
-register_ioport_read(s->smb_io_base, 0xf, 1, smb_ioport_readb, &s->smb);
+
+memory_region_init_io(&s->smb_io, &smb_io_ops, &s->smb, "vt82c686b-smb",
+  0xf);
+memory_region_add_subregion(pci_address_space_io(dev), s->smb_io_base,
+&s->smb_io);
 
 apm_init(dev, &s->apm, NULL, s);
 
-- 
Julien Grall

Re: [Qemu-devel] [PATCH] json-parser: Fix potential NULL pointer segfault

[Luiz Capitulino]

On Sat,  1 Sep 2012 12:52:58 +0200
Stefan Weil  wrote:

> Report from smatch:
> json-parser.c:474 parse_object(62) error: potential null derefence 'dict'.
> json-parser.c:553 parse_array(75) error: potential null derefence 'list'.
> 
> Label out can be called with list == NULL.
> 
> Signed-off-by: Stefan Weil 
> ---
>  json-parser.c |8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/json-parser.c b/json-parser.c
> index 457291b..c31c759 100644
> --- a/json-parser.c
> +++ b/json-parser.c
> @@ -471,7 +471,9 @@ static QObject *parse_object(JSONParserContext *ctxt, 
> va_list *ap)
>  
>  out:
>  parser_context_restore(ctxt, saved_ctxt);
> -QDECREF(dict);
> +if (dict) {
> +QDECREF(dict);
> +}

I prefer changing QDECREF() to a nop if obj is NULL.

>  return NULL;
>  }
>  
> @@ -550,7 +552,9 @@ static QObject *parse_array(JSONParserContext *ctxt, 
> va_list *ap)
>  
>  out:
>  parser_context_restore(ctxt, saved_ctxt);
> -QDECREF(list);
> +if (list) {
> +QDECREF(list);
> +}
>  return NULL;
>  }
>  

Re: [Qemu-devel] [PATCH 0/2 v3] Fix static linking for cURL and SDL

[Peter Maydell]

On 3 September 2012 17:28, Yann E. MORIN  wrote:
> On Monday 03 September 2012 17:44:51 Peter Maydell wrote:
>> Personally I think it might indeed be a good idea to just say
>> "statically linked softmmu isn't supported" and forbid it, unless
>> somebody has a good use case for it...
>
> I personnally have such a use-case: provide run-everywhere qemu softmmu
> binaries that do have minimal requirements on the distro they'll be
> running on, especially when the distros can be very different (in kinds
> and in age).

...have you audited all of qemu's library usage to confirm that it
doesn't use any of the bits of glibc that don't work in the static
linking scenario? I'm reasonably confident that the linux-user binaries
are OK, much less so for the system binaries...

-- PMM

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Avi Kivity]

On 09/03/2012 07:23 PM, Paolo Bonzini wrote:
> Il 03/09/2012 18:15, Avi Kivity ha scritto:
>>> > The values above are what every user of the PIC cascaded on our targets
>>> > must program to use them. So We will find them in the state once any
>>> > relevant guest code was able to run (e.g. the BIOS).
>>> > 
>> Suppose the bios has not run yet?
> 
> Then you transmit the subsection.

And the migration fails.  Needlessly, since icw3 == 0 doesn't affect
guest operation.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

On 2012-09-03 18:15, Avi Kivity wrote:
> On 09/03/2012 07:02 PM, Jan Kiszka wrote:
> 
> Looks like the optimal condition is ((s->icw3 & ~s->eclr) != 0) (i.e.
> bit set in icw3 but clear in eclr).

 The standard PC values are optimal: 4 for master, 2 for slave.
>>>
>>> Can you explain why?  I saw that icw3 is always ORed with eclr, so my
>>> condition will catch exactly those cases where a change in behaviour
>>> occurs, and no more.
>>
>> The values above are what every user of the PIC cascaded on our targets
>> must program to use them. So We will find them in the state once any
>> relevant guest code was able to run (e.g. the BIOS).
>>
> 
> Suppose the bios has not run yet?

Then we must save the 0.

Your logic will force us to save in all standard cases (ELCR's bit for
IRQ2 must not be set by a guest). So it's not really helpful.

Jan

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

Re: [Qemu-devel] [PATCH 0/2 v3] Fix static linking for cURL and SDL

[Yann E. MORIN]

Hello All,

On Monday 03 September 2012 17:44:51 Peter Maydell wrote:
> On 3 September 2012 16:41, Andreas Färber  wrote:
> > The only use case for QEMU's --static compilation I know is linux-user,
> > and that doesn't need cURL or SDL AFAIK. Shouldn't we rather sanitize
> > our configure-time checks to only look for the actually needed stuff
> > than making sure that unnecessary dependencies are generated nicely?
> 
> In particular it might be nice to be able to build in a single
> run both (a) dynamically linked softmmu targets and (b) statically
> linked linux-user targets (and maybe even also (c) dynamically
> linked linux-user targets?).
> 
> Personally I think it might indeed be a good idea to just say
> "statically linked softmmu isn't supported" and forbid it, unless
> somebody has a good use case for it...

I personnally have such a use-case: provide run-everywhere qemu softmmu
binaries that do have minimal requirements on the distro they'll be
running on, especially when the distros can be very different (in kinds
and in age).

For example, I can use it to provide:
  - a qemu binary
  - a VM image
  - a script
that users can simply drop anywhere they want (without needing root
access) and run the VM, without requiring them to install a myriad
packages, especially on older distros that may lack those packages,
or whose packaged versions are too old for qemu.

Regards,
Yann E. MORIN.

-- 
.-..--..
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN |  ___   |
| +33 223 225 172 `.---:  X  AGAINST  |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL|   v   conspiracy.  |
'--^---^--^'

[Qemu-devel] [PATCH V7 2/8] hw/apm.c: replace register_ioport*

[Julien Grall]

This patch replaces all register_ioport* by a MemorySection.
It permits to use the new Memory stuff like listener.

Moreover, the PCI is added as an argument for apm_init, so we
can register IO inside the pci IO address space.

Signed-off-by: Julien Grall 
---
 hw/acpi_piix4.c |2 +-
 hw/apm.c|   24 +++-
 hw/apm.h|5 -
 hw/vt82c686.c   |2 +-
 4 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index c56220b..fd2fc33 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -395,7 +395,7 @@ static int piix4_pm_initfn(PCIDevice *dev)
 pci_conf[0x3d] = 0x01; // interrupt pin 1
 
 /* APM */
-apm_init(&s->apm, apm_ctrl_changed, s);
+apm_init(dev, &s->apm, apm_ctrl_changed, s);
 
 register_ioport_write(ACPI_DBG_IO_ADDR, 4, 4, acpi_dbg_writel, s);
 
diff --git a/hw/apm.c b/hw/apm.c
index 2aead52..fe7bc21 100644
--- a/hw/apm.c
+++ b/hw/apm.c
@@ -22,6 +22,7 @@
 
 #include "apm.h"
 #include "hw.h"
+#include "pci.h"
 
 //#define DEBUG
 
@@ -35,7 +36,8 @@
 #define APM_CNT_IOPORT  0xb2
 #define APM_STS_IOPORT  0xb3
 
-static void apm_ioport_writeb(void *opaque, uint32_t addr, uint32_t val)
+static void apm_ioport_writeb(void *opaque, target_phys_addr_t addr,
+  uint64_t val, unsigned size)
 {
 APMState *apm = opaque;
 addr &= 1;
@@ -51,7 +53,8 @@ static void apm_ioport_writeb(void *opaque, uint32_t addr, 
uint32_t val)
 }
 }
 
-static uint32_t apm_ioport_readb(void *opaque, uint32_t addr)
+static uint64_t apm_ioport_readb(void *opaque, target_phys_addr_t addr,
+ unsigned size)
 {
 APMState *apm = opaque;
 uint32_t val;
@@ -78,12 +81,23 @@ const VMStateDescription vmstate_apm = {
 }
 };
 
-void apm_init(APMState *apm, apm_ctrl_changed_t callback, void *arg)
+static const MemoryRegionOps apm_ops = {
+.read = apm_ioport_readb,
+.write = apm_ioport_writeb,
+.impl = {
+.min_access_size = 1,
+.max_access_size = 1,
+},
+};
+
+void apm_init(PCIDevice *dev, APMState *apm, apm_ctrl_changed_t callback,
+  void *arg)
 {
 apm->callback = callback;
 apm->arg = arg;
 
 /* ioport 0xb2, 0xb3 */
-register_ioport_write(APM_CNT_IOPORT, 2, 1, apm_ioport_writeb, apm);
-register_ioport_read(APM_CNT_IOPORT, 2, 1, apm_ioport_readb, apm);
+memory_region_init_io(&apm->io, &apm_ops, apm, "apm-io", 2);
+memory_region_add_subregion(pci_address_space_io(dev), APM_CNT_IOPORT,
+&apm->io);
 }
diff --git a/hw/apm.h b/hw/apm.h
index f7c741e..5431b6d 100644
--- a/hw/apm.h
+++ b/hw/apm.h
@@ -4,6 +4,7 @@
 #include 
 #include "qemu-common.h"
 #include "hw.h"
+#include "memory.h"
 
 typedef void (*apm_ctrl_changed_t)(uint32_t val, void *arg);
 
@@ -13,9 +14,11 @@ typedef struct APMState {
 
 apm_ctrl_changed_t callback;
 void *arg;
+MemoryRegion io;
 } APMState;
 
-void apm_init(APMState *s, apm_ctrl_changed_t callback, void *arg);
+void apm_init(PCIDevice *dev, APMState *s, apm_ctrl_changed_t callback,
+  void *arg);
 
 extern const VMStateDescription vmstate_apm;
 
diff --git a/hw/vt82c686.c b/hw/vt82c686.c
index 5d7c00c..7f11dbe 100644
--- a/hw/vt82c686.c
+++ b/hw/vt82c686.c
@@ -427,7 +427,7 @@ static int vt82c686b_pm_initfn(PCIDevice *dev)
 register_ioport_write(s->smb_io_base, 0xf, 1, smb_ioport_writeb, &s->smb);
 register_ioport_read(s->smb_io_base, 0xf, 1, smb_ioport_readb, &s->smb);
 
-apm_init(&s->apm, NULL, s);
+apm_init(dev, &s->apm, NULL, s);
 
 acpi_pm_tmr_init(&s->ar, pm_tmr_timer);
 acpi_pm1_cnt_init(&s->ar);
-- 
Julien Grall

[Qemu-devel] [PATCH V7 0/8] memory: unify ioport registration

[Julien Grall]

This is the seventh version of patch series about ioport registration.

Some part of QEMU still use register_ioport* functions to register ioport.
These functions doesn't allow to use Memory Listener on it.

Modifications between V1 and V2:
   - Remove the use of get_system_io. Instead of use isa and pci IO
 address space.
   - Avoid allocation of PortioList. Use the different device
 structure.
   - Still remove register_ioport* (hw/dma.c, hw/apm.c,
 hw/acpi_piix4.c).
   - Use MemoryRegion when we have only a range of ioport.
   - For some functions, add IO address space as
 argument.
   - Add isa_address_space_io function

Modifications between V2 and V3:
   - Remove some register_ioport_* on hw/vt82c686.c.
   - Split smb ioport part in new patch.
   - Still replace MemoryRegion when we have only a range of ioport.
   - Fix read/write ioports prototype to  be compliant with memory callback.

Modifications between V3 and V4:
   - Fix compilation in hw/dma.c
   - Fix address conversion (hw/dma.c, hw/acpi_piix4.c) with MemorySection.
 Indeed the new version use offset from MemorySection start instead of 0.

Modifications between V4 and V5:
   - Rebase on qemu upstream.
   - Forget some ioport_register_* in acpi_piix4.c.
   - Register 0x3b0 - 0x3df range for cirrus instead of ioport by ioport.

Modifications between V5 and V6:
   - Add read function on cirrus ioport (forget on the previous patch).
   - Rework PM memory range handling.
   - Fix PCI_BASE in acpi_piix4.c (wrong conversion during port).
   - Rewrite isa_address_space_io to use ISA bus address space.
   - Fix compilation in vt82c686.c

Modifications between V6 and V7:
   - acpi_piix4: use memory_region_set_enabled instead of a boolean. I'm not
 sure about this modification (adviced by Avi).
   - Fix device endianess in acpi_piix4 (reported by Avi).
   - Avoid dependencies between patches and reorder it (reported by Jan).
 Some code moved from acpi_piix4 patch to smb/apm patches.

Julien Grall (8):
  isa: add isa_address_space_io
  hw/apm.c: replace register_ioport*
  smb: replace_register_ioport*
  hw/acpi_piix4.c: replace register_ioport*
  hw/cirrus_vga.c: replace register_ioport*
  hw/serial.c: replace register_ioport*
  hw/pc.c: replace register_ioport*
  hw/dma.c: replace register_ioport*

 hw/acpi_piix4.c   |  171 ++---
 hw/apm.c  |   24 ++--
 hw/apm.h  |5 +-
 hw/cirrus_vga.c   |   50 +--
 hw/dma.c  |  108 ++---
 hw/isa-bus.c  |9 +++
 hw/isa.h  |1 +
 hw/mips_mipssim.c |3 +-
 hw/pc.c   |   58 +-
 hw/pc.h   |2 +-
 hw/pm_smbus.c |7 +-
 hw/pm_smbus.h |6 +-
 hw/serial.c   |8 ++-
 hw/vt82c686.c |   20 +-
 14 files changed, 347 insertions(+), 125 deletions(-)

-- 
Julien Grall

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Paolo Bonzini]

Il 03/09/2012 18:15, Avi Kivity ha scritto:
>> > The values above are what every user of the PIC cascaded on our targets
>> > must program to use them. So We will find them in the state once any
>> > relevant guest code was able to run (e.g. the BIOS).
>> > 
> Suppose the bios has not run yet?

Then you transmit the subsection.

BTW this also means that simply checking against eclr|icw3 is wrong; the
right test is:

* against elcr if !s->master

* against elcr|icw3 if s->master

This makes precomputing the value more appealing.

Similarly, perhaps this:

if (s->special_fully_nested_mode && s->master) {
mask &= ~(1 << 2);
}

should be changed to

if (s->special_fully_nested_mode && s->master) {
mask &= ~s->icw3;
}

?

Paolo

Re: [Qemu-devel] [PATCH 2/6] monitor: Adjust qmp_human_monitor_command to new MemCharDriver

[Luiz Capitulino]

On Tue, 04 Sep 2012 00:14:03 +0800
Lei Li  wrote:

> On 08/31/2012 02:51 AM, Luiz Capitulino wrote:
> > On Thu, 23 Aug 2012 13:14:22 +0800
> > Lei Li  wrote:
> >
> >> Signed-off-by: Lei Li 
> >> ---
> >>   monitor.c |8 +++-
> >>   1 files changed, 7 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/monitor.c b/monitor.c
> >> index 480f583..ab4650b 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -642,7 +642,13 @@ char *qmp_human_monitor_command(const char 
> >> *command_line, bool has_cpu_index,
> >>   CharDriverState mchar;
> >>   
> >>   memset(&hmp, 0, sizeof(hmp));
> >> -qemu_chr_init_mem(&mchar);
> >> +
> >> +/* Since the backend of MemCharDriver convert to a circular
> >> + * buffer with fixed size, so should indicate the init memory
> >> + * size.
> >> + *
> >> + * XXX:  is 4096 as init memory enough for this? */
> >> +qemu_chr_init_mem(&mchar, 4096);
> > I'm not sure I like this. The end result will be that hmp commands writing
> > more than 4096 bytes will simply fail or return garbage (if the circular 
> > buffer
> > is changed to allow writing more than it supports) today they would just 
> > work.
> >
> > Although it's always possible to increase the buffer size, we would only 
> > realize
> > this is needed when the bug is triggered, which means it has a high chance
> > of happening in production. IOW, this would be a regression.
> >
> > The only solution I can think of is to make the circular buffer and the
> > current MemoryDriver live in parallel. Actually, you really seem to be
> > adding something else.
> 
> Hi Luiz,
> 
> Thanks for your review. Yes, I agree with that it's not a good solution for 
> the
> old user human command. Make the circular buffer and the current MemoryDriver
> live in parallel, do you mean don't change the current MemoryDriver, choose to
> expose a new char device backend with circular buffer?

Yes, you could add CircularMemoryDriver for example.

> 
> >>   hmp.chr = &mchar;
> >>   
> >>   old_mon = cur_mon;
> >
> 
> 

Re: [Qemu-devel] [PATCH 2/6] monitor: Adjust qmp_human_monitor_command to new MemCharDriver

[Lei Li]

On 08/31/2012 02:51 AM, Luiz Capitulino wrote:

On Thu, 23 Aug 2012 13:14:22 +0800
Lei Li  wrote:


Signed-off-by: Lei Li 
---
  monitor.c |8 +++-
  1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index 480f583..ab4650b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -642,7 +642,13 @@ char *qmp_human_monitor_command(const char *command_line, 
bool has_cpu_index,
  CharDriverState mchar;
  
  memset(&hmp, 0, sizeof(hmp));

-qemu_chr_init_mem(&mchar);
+
+/* Since the backend of MemCharDriver convert to a circular
+ * buffer with fixed size, so should indicate the init memory
+ * size.
+ *
+ * XXX:  is 4096 as init memory enough for this? */
+qemu_chr_init_mem(&mchar, 4096);

I'm not sure I like this. The end result will be that hmp commands writing
more than 4096 bytes will simply fail or return garbage (if the circular buffer
is changed to allow writing more than it supports) today they would just work.

Although it's always possible to increase the buffer size, we would only realize
this is needed when the bug is triggered, which means it has a high chance
of happening in production. IOW, this would be a regression.

The only solution I can think of is to make the circular buffer and the
current MemoryDriver live in parallel. Actually, you really seem to be
adding something else.


Hi Luiz,

Thanks for your review. Yes, I agree with that it's not a good solution for the
old user human command. Make the circular buffer and the current MemoryDriver
live in parallel, do you mean don't change the current MemoryDriver, choose to
expose a new char device backend with circular buffer?


  hmp.chr = &mchar;
  
  old_mon = cur_mon;





--
Lei

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Avi Kivity]

On 09/03/2012 07:02 PM, Jan Kiszka wrote:

 Looks like the optimal condition is ((s->icw3 & ~s->eclr) != 0) (i.e.
 bit set in icw3 but clear in eclr).
>>>
>>> The standard PC values are optimal: 4 for master, 2 for slave.
>> 
>> Can you explain why?  I saw that icw3 is always ORed with eclr, so my
>> condition will catch exactly those cases where a change in behaviour
>> occurs, and no more.
> 
> The values above are what every user of the PIC cascaded on our targets
> must program to use them. So We will find them in the state once any
> relevant guest code was able to run (e.g. the BIOS).
> 

Suppose the bios has not run yet?


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 4/4] kvm: i386: Add classic PCI device assignment

[Avi Kivity]

On 08/29/2012 11:27 AM, Markus Armbruster wrote:
> 
> I don't see a point in making contributors avoid non-problems that might
> conceivably become trivial problems some day.  Especially when there's
> no automated help with the avoiding.

-Wpointer-arith



-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 4/4] kvm: i386: Add classic PCI device assignment

[Avi Kivity]

On 08/28/2012 03:30 AM, Jan Kiszka wrote:
>> 
>> Maybe add case 8: and default: with abort(), also below.
> 
> PIO is never 8 bytes long, the generic layer protects us.

Note: eventually the pio space will be mapped directly to mmio (instead
of being bounced via cpu_inb() in the bridge's mmio handler), and so we
will have to add this restriction to the memory core.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

On 2012-09-03 17:57, Avi Kivity wrote:
> On 09/03/2012 06:54 PM, Jan Kiszka wrote:
>> On 2012-09-03 17:52, Avi Kivity wrote:
>>> On 09/03/2012 06:42 PM, Juan Quintela wrote:
 Avi Kivity  wrote:
> On 09/03/2012 11:40 AM, Andreas Färber wrote:
>> Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
>>> diff --git a/hw/i8259_common.c b/hw/i8259_common.c
>>> index ab3d98b..dcde5f2 100644
>>> --- a/hw/i8259_common.c
>>> +++ b/hw/i8259_common.c
>> [...]
>>> @@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common 
>>> = {
>>>  VMSTATE_UINT8(isr, PICCommonState),
>>>  VMSTATE_UINT8(priority_add, PICCommonState),
>>>  VMSTATE_UINT8(irq_base, PICCommonState),
>>> +VMSTATE_UINT8(icw3, PICCommonState),
>>>  VMSTATE_UINT8(read_reg_select, PICCommonState),
>>>  VMSTATE_UINT8(poll, PICCommonState),
>>>  VMSTATE_UINT8(special_mask, PICCommonState),
>>
>> Additional VMState needs to be versioned by incrementing .version_id and
>> by specifying the new version number here. Otherwise it breaks migration.

 For the subsection, only sending this when icw3 != 0 is enough?  I am
 searching for a test about when we need to send it.
>>>
>>> Looks like the optimal condition is ((s->icw3 & ~s->eclr) != 0) (i.e.
>>> bit set in icw3 but clear in eclr).
>>
>> The standard PC values are optimal: 4 for master, 2 for slave.
> 
> Can you explain why?  I saw that icw3 is always ORed with eclr, so my
> condition will catch exactly those cases where a change in behaviour
> occurs, and no more.

The values above are what every user of the PIC cascaded on our targets
must program to use them. So We will find them in the state once any
relevant guest code was able to run (e.g. the BIOS).

Jan

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

Re: [Qemu-devel] [PATCH 4/4] kvm: i386: Add classic PCI device assignment

[Avi Kivity]

On 08/29/2012 11:49 AM, Peter Maydell wrote:
> On 29 August 2012 09:47, Jan Kiszka  wrote:
>> On 2012-08-28 23:26, Peter Maydell wrote:
>>> Since this is arch-specific we should probably give the
>>> resulting device a more specific name than "pci-assign",
>>> which implies that it is (a) ok for any PCI system and
>>> (b) not something that will be obsolete in the foreseen
>>> future.
>>
>> The name has to be like this for libvirt compatibility. I can provide it
>> as alias, though, calling the device "kvm-pci-assign" by default. Better?
> 
> ...is it likely to ever work for non-x86 PCI devices?

It used to work for ia64 before it died.  So it's not architecture
specific by design.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Avi Kivity]

On 09/03/2012 06:54 PM, Jan Kiszka wrote:
> On 2012-09-03 17:52, Avi Kivity wrote:
>> On 09/03/2012 06:42 PM, Juan Quintela wrote:
>>> Avi Kivity  wrote:
 On 09/03/2012 11:40 AM, Andreas Färber wrote:
> Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
>> diff --git a/hw/i8259_common.c b/hw/i8259_common.c
>> index ab3d98b..dcde5f2 100644
>> --- a/hw/i8259_common.c
>> +++ b/hw/i8259_common.c
> [...]
>> @@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = 
>> {
>>  VMSTATE_UINT8(isr, PICCommonState),
>>  VMSTATE_UINT8(priority_add, PICCommonState),
>>  VMSTATE_UINT8(irq_base, PICCommonState),
>> +VMSTATE_UINT8(icw3, PICCommonState),
>>  VMSTATE_UINT8(read_reg_select, PICCommonState),
>>  VMSTATE_UINT8(poll, PICCommonState),
>>  VMSTATE_UINT8(special_mask, PICCommonState),
>
> Additional VMState needs to be versioned by incrementing .version_id and
> by specifying the new version number here. Otherwise it breaks migration.
>>>
>>> For the subsection, only sending this when icw3 != 0 is enough?  I am
>>> searching for a test about when we need to send it.
>> 
>> Looks like the optimal condition is ((s->icw3 & ~s->eclr) != 0) (i.e.
>> bit set in icw3 but clear in eclr).
> 
> The standard PC values are optimal: 4 for master, 2 for slave.

Can you explain why?  I saw that icw3 is always ORed with eclr, so my
condition will catch exactly those cases where a change in behaviour
occurs, and no more.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

On 2012-09-03 17:52, Avi Kivity wrote:
> On 09/03/2012 06:42 PM, Juan Quintela wrote:
>> Avi Kivity  wrote:
>>> On 09/03/2012 11:40 AM, Andreas Färber wrote:
 Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
> diff --git a/hw/i8259_common.c b/hw/i8259_common.c
> index ab3d98b..dcde5f2 100644
> --- a/hw/i8259_common.c
> +++ b/hw/i8259_common.c
 [...]
> @@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
>  VMSTATE_UINT8(isr, PICCommonState),
>  VMSTATE_UINT8(priority_add, PICCommonState),
>  VMSTATE_UINT8(irq_base, PICCommonState),
> +VMSTATE_UINT8(icw3, PICCommonState),
>  VMSTATE_UINT8(read_reg_select, PICCommonState),
>  VMSTATE_UINT8(poll, PICCommonState),
>  VMSTATE_UINT8(special_mask, PICCommonState),

 Additional VMState needs to be versioned by incrementing .version_id and
 by specifying the new version number here. Otherwise it breaks migration.
>>
>> For the subsection, only sending this when icw3 != 0 is enough?  I am
>> searching for a test about when we need to send it.
> 
> Looks like the optimal condition is ((s->icw3 & ~s->eclr) != 0) (i.e.
> bit set in icw3 but clear in eclr).

The standard PC values are optimal: 4 for master, 2 for slave.

Jan

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

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Avi Kivity]

On 09/03/2012 06:42 PM, Juan Quintela wrote:
> Avi Kivity  wrote:
>> On 09/03/2012 11:40 AM, Andreas Färber wrote:
>>> Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
 diff --git a/hw/i8259_common.c b/hw/i8259_common.c
 index ab3d98b..dcde5f2 100644
 --- a/hw/i8259_common.c
 +++ b/hw/i8259_common.c
>>> [...]
 @@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
  VMSTATE_UINT8(isr, PICCommonState),
  VMSTATE_UINT8(priority_add, PICCommonState),
  VMSTATE_UINT8(irq_base, PICCommonState),
 +VMSTATE_UINT8(icw3, PICCommonState),
  VMSTATE_UINT8(read_reg_select, PICCommonState),
  VMSTATE_UINT8(poll, PICCommonState),
  VMSTATE_UINT8(special_mask, PICCommonState),
>>> 
>>> Additional VMState needs to be versioned by incrementing .version_id and
>>> by specifying the new version number here. Otherwise it breaks migration.
> 
> For the subsection, only sending this when icw3 != 0 is enough?  I am
> searching for a test about when we need to send it.

Looks like the optimal condition is ((s->icw3 & ~s->eclr) != 0) (i.e.
bit set in icw3 but clear in eclr).

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 20/21] target-mips: switch to AREG0 free mode

[Aurelien Jarno]

On Sun, Sep 02, 2012 at 05:33:49PM +, Blue Swirl wrote:
> Add an explicit CPUState parameter instead of relying on AREG0
> and switch to AREG0 free mode.
> 
> Signed-off-by: Blue Swirl 
> ---
>  configure |2 +-
>  target-mips/Makefile.objs |2 -
>  target-mips/cpu.h |   16 +-
>  target-mips/helper.h  |  410 +-
>  target-mips/op_helper.c   | 1065 
> -
>  target-mips/translate.c   |  754 
>  6 files changed, 1163 insertions(+), 1086 deletions(-)

Acked-by: Aurelien Jarno 

Please commit this patch asap after the 1.2 release, even if the patches
for the other targets are not ready, so that it doesn't hold the 
development.

-- 
Aurelien Jarno  GPG: 1024D/F1BCDB73
aurel...@aurel32.net http://www.aurel32.net

Re: [Qemu-devel] [PATCH for-1.2] use --libexecdir instead of ignoring it first and reinventing it later

[Andreas Färber]

Am 16.08.2012 16:46, schrieb Andreas Färber:
> Am 06.06.2012 23:11, schrieb Michael Tokarev:
>> Commit 7b93fadf3a38d1ed65ea5536a52efc2772c6e3b8 "Add basic version
>> of bridge helper" put the bridge helper executable into a fixed
>> ${prefix}/libexec/ location, instead of using ${libexecdir} for
>> this.  At the same time, --libexecdir is being happily ignored
>> by ./configure.  Even more, the same patch sets unused $libexecdir
>> variable in the generated config-host.mak, and uses fixed string
>> (\${prefix}/libexecdir) for the bridge helper binary.
>>
>> Fix this braindamage by introducing $libexecdir variable, using
>> it for the bridge helper binary, and recognizing --libexecdir.
>>
>> This patch is applicable to stable-1.1.
>>
>> Signed-off-by: Michael Tokarev 
>> Cc: Corey Bryant 
>> Cc: Richa Marwaha 
>> Cc: qemu-sta...@nongnu.org
> 
> Ping! This patch still seems missing in master and is needed for v1.1
> and v1.2 packaging.

Ping^2! Could someone please apply this before it's too late?
(optionally with less offensive wording...)

Thanks,
Andreas

>> ---
>>  configure |   10 +++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 38dafec..fc86803 100755
>> --- a/configure
>> +++ b/configure
>> @@ -169,6 +169,7 @@ datadir="\${prefix}/share"
>>  qemu_docdir="\${prefix}/share/doc/qemu"
>>  bindir="\${prefix}/bin"
>>  libdir="\${prefix}/lib"
>> +libexecdir="\${prefix}/libexec"
>>  includedir="\${prefix}/include"
>>  sysconfdir="\${prefix}/etc"
>>  confsuffix="/qemu"
>> @@ -598,6 +599,8 @@ for opt do
>>;;
>>--libdir=*) libdir="$optarg"
>>;;
>> +  --libexecdir=*) libexecdir="$optarg"
>> +  ;;
>>--includedir=*) includedir="$optarg"
>>;;
>>--datadir=*) datadir="$optarg"
>> @@ -608,7 +611,7 @@ for opt do
>>;;
>>--sysconfdir=*) sysconfdir="$optarg"
>>;;
>> -  --sbindir=*|--libexecdir=*|--sharedstatedir=*|--localstatedir=*|\
>> +  --sbindir=*|--sharedstatedir=*|--localstatedir=*|\
>>--oldincludedir=*|--datarootdir=*|--infodir=*|--localedir=*|\
>>--htmldir=*|--dvidir=*|--pdfdir=*|--psdir=*)
>>  # These switches are silently ignored, for compatibility with
>> @@ -2960,6 +2963,7 @@ echo "Install prefix$prefix"
>>  echo "BIOS directory`eval echo $qemu_datadir`"
>>  echo "binary directory  `eval echo $bindir`"
>>  echo "library directory `eval echo $libdir`"
>> +echo "libexec directory `eval echo $libexecdir`"
>>  echo "include directory `eval echo $includedir`"
>>  echo "config directory  `eval echo $sysconfdir`"
>>  if test "$mingw32" = "no" ; then
>> @@ -3064,14 +3068,14 @@ echo all: >> $config_host_mak
>>  echo "prefix=$prefix" >> $config_host_mak
>>  echo "bindir=$bindir" >> $config_host_mak
>>  echo "libdir=$libdir" >> $config_host_mak
>> +echo "libexecdir=$libexecdir" >> $config_host_mak
>>  echo "includedir=$includedir" >> $config_host_mak
>>  echo "mandir=$mandir" >> $config_host_mak
>>  echo "sysconfdir=$sysconfdir" >> $config_host_mak
>>  echo "qemu_confdir=$qemu_confdir" >> $config_host_mak
>>  echo "qemu_datadir=$qemu_datadir" >> $config_host_mak
>>  echo "qemu_docdir=$qemu_docdir" >> $config_host_mak
>> -echo "libexecdir=\${prefix}/libexec" >> $config_host_mak
>> -echo "CONFIG_QEMU_HELPERDIR=\"$prefix/libexec\"" >> $config_host_mak
>> +echo "CONFIG_QEMU_HELPERDIR=\"$libexecdir\"" >> $config_host_mak
>>  
>>  echo "ARCH=$ARCH" >> $config_host_mak
>>  if test "$debug_tcg" = "yes" ; then
>>
> 
> 


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

Re: [Qemu-devel] [PATCH for 1.2] qemu-timer: properly arm alarm timer for timers set by device initialization

[Aurelien Jarno]

On Mon, Sep 03, 2012 at 05:34:32PM +0200, Paolo Bonzini wrote:
> QEMU will hang when fed the following command-line
> 
>   qemu-system-mips -kernel vmlinux-2.6.32-5-4kc-malta -append "console=ttyS0" 
> -nographic -net none
> 
> The -net none is important otherwise it seems some events are generated
> causing the things to work. When it doesn't work, the guest hangs when
> measuring the CPU frequency, after the following line:
> 
>   [0.00] NR_IRQS:256
> 
> Pressing a key on the serial port unblocks it, hinting that the problem
> is due to the recent elimination of the 1 second timeout in the main
> loop.
> 
> The problem is that because init_timer_alarm sets the timer's pending
> flag to true, the alarm timer is never armed until after the first time
> through the main loop.  Thus the bug started when QEMU started testing
> the pending flag in qemu_mod_timer (commit 1828be3, more alarm timer
> cleanup, 2010-03-10).
> 
> But actually, it isn't true at all that a timer is pending when the
> alarm timer is created, and the real bug has been latent forever: the
> fix is to remove the bogus setting of pending flag.
> 
> Reported-by: Aurelien Jarno 
> Signed-off-by: Paolo Bonzini 
> ---
>  qemu-timer.c | 3 ---
>  1 file modificato, 3 rimozioni(-)
> 
> diff --git a/qemu-timer.c b/qemu-timer.c
> index 5aea94e..c7a1551 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -759,11 +759,8 @@ int init_timer_alarm(void)
>  goto fail;
>  }
>  
> -/* first event is at time 0 */
>  atexit(quit_timers);
> -t->pending = true;
>  alarm_timer = t;
> -
>  return 0;
>  
>  fail:
>

Thanks a lot that was quite fast!

Tested-by: Aurelien Jarno 

-- 
Aurelien Jarno  GPG: 1024D/F1BCDB73
aurel...@aurel32.net http://www.aurel32.net

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

On 2012-09-03 17:42, Juan Quintela wrote:
> Avi Kivity  wrote:
>> On 09/03/2012 11:40 AM, Andreas Färber wrote:
>>> Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
 diff --git a/hw/i8259_common.c b/hw/i8259_common.c
 index ab3d98b..dcde5f2 100644
 --- a/hw/i8259_common.c
 +++ b/hw/i8259_common.c
>>> [...]
 @@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
  VMSTATE_UINT8(isr, PICCommonState),
  VMSTATE_UINT8(priority_add, PICCommonState),
  VMSTATE_UINT8(irq_base, PICCommonState),
 +VMSTATE_UINT8(icw3, PICCommonState),
  VMSTATE_UINT8(read_reg_select, PICCommonState),
  VMSTATE_UINT8(poll, PICCommonState),
  VMSTATE_UINT8(special_mask, PICCommonState),
>>>
>>> Additional VMState needs to be versioned by incrementing .version_id and
>>> by specifying the new version number here. Otherwise it breaks migration.
> 
> For the subsection, only sending this when icw3 != 0 is enough?  I am
> searching for a test about when we need to send it.

See my reply on this topic in the other branch of this thread.

> 
>> And incrementing the version ID breaks backwards migration.  The correct
>> solution is subsections, copying Juan and booking a trip to the Mariana
>> trench.
> 
> Book also for me, no need for the return ticket.
> 

Hey, this scenario is rather harmless (famous last words...). ;)

Jan

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

Re: [Qemu-devel] [PATCH 0/2 v3] Fix static linking for cURL and SDL

[Peter Maydell]

On 3 September 2012 16:41, Andreas Färber  wrote:
> The only use case for QEMU's --static compilation I know is linux-user,
> and that doesn't need cURL or SDL AFAIK. Shouldn't we rather sanitize
> our configure-time checks to only look for the actually needed stuff
> than making sure that unnecessary dependencies are generated nicely?

In particular it might be nice to be able to build in a single
run both (a) dynamically linked softmmu targets and (b) statically
linked linux-user targets (and maybe even also (c) dynamically
linked linux-user targets?).

Personally I think it might indeed be a good idea to just say
"statically linked softmmu isn't supported" and forbid it, unless
somebody has a good use case for it...

-- PMM

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Juan Quintela]

Avi Kivity  wrote:
> On 09/03/2012 11:40 AM, Andreas Färber wrote:
>> Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
>>> diff --git a/hw/i8259_common.c b/hw/i8259_common.c
>>> index ab3d98b..dcde5f2 100644
>>> --- a/hw/i8259_common.c
>>> +++ b/hw/i8259_common.c
>> [...]
>>> @@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
>>>  VMSTATE_UINT8(isr, PICCommonState),
>>>  VMSTATE_UINT8(priority_add, PICCommonState),
>>>  VMSTATE_UINT8(irq_base, PICCommonState),
>>> +VMSTATE_UINT8(icw3, PICCommonState),
>>>  VMSTATE_UINT8(read_reg_select, PICCommonState),
>>>  VMSTATE_UINT8(poll, PICCommonState),
>>>  VMSTATE_UINT8(special_mask, PICCommonState),
>> 
>> Additional VMState needs to be versioned by incrementing .version_id and
>> by specifying the new version number here. Otherwise it breaks migration.

For the subsection, only sending this when icw3 != 0 is enough?  I am
searching for a test about when we need to send it.

> And incrementing the version ID breaks backwards migration.  The correct
> solution is subsections, copying Juan and booking a trip to the Mariana
> trench.

Book also for me, no need for the return ticket.

Later, Juan.

Re: [Qemu-devel] [PATCH for 1.2] qemu-timer: properly arm alarm timer for timers set by device initialization

[Jan Kiszka]

On 2012-09-03 17:34, Paolo Bonzini wrote:
> QEMU will hang when fed the following command-line
> 
>   qemu-system-mips -kernel vmlinux-2.6.32-5-4kc-malta -append "console=ttyS0" 
> -nographic -net none
> 
> The -net none is important otherwise it seems some events are generated
> causing the things to work. When it doesn't work, the guest hangs when
> measuring the CPU frequency, after the following line:
> 
>   [0.00] NR_IRQS:256
> 
> Pressing a key on the serial port unblocks it, hinting that the problem
> is due to the recent elimination of the 1 second timeout in the main
> loop.
> 
> The problem is that because init_timer_alarm sets the timer's pending
> flag to true, the alarm timer is never armed until after the first time
> through the main loop.  Thus the bug started when QEMU started testing
> the pending flag in qemu_mod_timer (commit 1828be3, more alarm timer
> cleanup, 2010-03-10).
> 
> But actually, it isn't true at all that a timer is pending when the
> alarm timer is created, and the real bug has been latent forever: the
> fix is to remove the bogus setting of pending flag.
> 
> Reported-by: Aurelien Jarno 
> Signed-off-by: Paolo Bonzini 
> ---
>  qemu-timer.c | 3 ---
>  1 file modificato, 3 rimozioni(-)
> 
> diff --git a/qemu-timer.c b/qemu-timer.c
> index 5aea94e..c7a1551 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -759,11 +759,8 @@ int init_timer_alarm(void)
>  goto fail;
>  }
>  
> -/* first event is at time 0 */
>  atexit(quit_timers);
> -t->pending = true;
>  alarm_timer = t;
> -
>  return 0;
>  
>  fail:
> 

Funnily, I just create the same problem with my "run timer handlers in
signal handler" patch (*). Same solution there.

Reviewed-by: Jan Kiszka 

Jan


(*) I will go for select-based timers once I have time for the necessary
refactorings.

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

Re: [Qemu-devel] [PATCH 0/2 v3] Fix static linking for cURL and SDL

[Andreas Färber]

Hello,

Am 02.09.2012 15:09, schrieb Yann E. MORIN:
> Currently, configure checks for cURL and SDL with either pkg-config (the
> default), or with {curl,sdl}-config (as a fallback).
> 
> But pkg-config and {curl,sdl}-config do not have the same set of options:
>   - to check for shared libs, both use the option: --libs
>   - to check for static libs:
> - pkg-config uses  : --static --libs
> - {curl,sdl}-config use: --static-libs
> 
> To add to the complexity, pkg-config is called through the querry_pkg_config
> wrapper, that already passes --static when static linking is required, but
> there is no such wrapper for {curl,sdl}-config, so we miss the occasion to
> pass --static-libs.
> 
> To fix this:
>   - introduce a new variable QEMU_XXX_CONFIG_LIBS_FLAGS that mirrors the
> behavior of QEMU_PKG_CONFIG_FLAGS; this variable can be used by all
> xxx-config scripts (eg. curl-config, but later sdl-config too).
> Default it to '--libs', which is for shared linking.
>   - properly use either --libs for pkg-config (--static is already taken
> care of in the wrapper), or $QEMU_XXX_CONFIG_LIBS_FLAGS for
> {curl,sdl}-config.

While this patch set looks okay technically, I wonder if this is fixing
the wrong problem...

The only use case for QEMU's --static compilation I know is linux-user,
and that doesn't need cURL or SDL AFAIK. Shouldn't we rather sanitize
our configure-time checks to only look for the actually needed stuff
than making sure that unnecessary dependencies are generated nicely?

Regards,
Andreas

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

[Qemu-devel] [PATCH for 1.2] qemu-timer: properly arm alarm timer for timers set by device initialization

[Paolo Bonzini]

QEMU will hang when fed the following command-line

  qemu-system-mips -kernel vmlinux-2.6.32-5-4kc-malta -append "console=ttyS0" 
-nographic -net none

The -net none is important otherwise it seems some events are generated
causing the things to work. When it doesn't work, the guest hangs when
measuring the CPU frequency, after the following line:

  [0.00] NR_IRQS:256

Pressing a key on the serial port unblocks it, hinting that the problem
is due to the recent elimination of the 1 second timeout in the main
loop.

The problem is that because init_timer_alarm sets the timer's pending
flag to true, the alarm timer is never armed until after the first time
through the main loop.  Thus the bug started when QEMU started testing
the pending flag in qemu_mod_timer (commit 1828be3, more alarm timer
cleanup, 2010-03-10).

But actually, it isn't true at all that a timer is pending when the
alarm timer is created, and the real bug has been latent forever: the
fix is to remove the bogus setting of pending flag.

Reported-by: Aurelien Jarno 
Signed-off-by: Paolo Bonzini 
---
 qemu-timer.c | 3 ---
 1 file modificato, 3 rimozioni(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index 5aea94e..c7a1551 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -759,11 +759,8 @@ int init_timer_alarm(void)
 goto fail;
 }
 
-/* first event is at time 0 */
 atexit(quit_timers);
-t->pending = true;
 alarm_timer = t;
-
 return 0;
 
 fail:
-- 
1.7.11.2