Re: [Qemu-devel] [PATCH 11/11] pseries: Clean up error reporting in htab migration functions

[Thomas Huth]

On 11/12/15 01:11, David Gibson wrote:
> The functions for migrating the hash page table on pseries machine type
> (htab_save_setup() and htab_load()) can report some errors with an
> explicit fprintf() before returning an appropriate eror code.  Change these

s/eror/error/

> to use error_report() instead.
> 
> Signed-off-by: David Gibson 
> ---
>  hw/ppc/spapr.c | 17 +
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index deaf5c0..c93ce09 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1318,8 +1318,9 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
>  spapr->htab_fd = kvmppc_get_htab_fd(false);
>  spapr->htab_fd_stale = false;
>  if (spapr->htab_fd < 0) {
> -fprintf(stderr, "Unable to open fd for reading hash table from 
> KVM: %s\n",
> -strerror(errno));
> +error_report(
> +"Unable to open fd for reading hash table from KVM: %s",
> +strerror(errno));
>  return -1;
>  }
>  }
> @@ -1535,7 +1536,7 @@ static int htab_load(QEMUFile *f, void *opaque, int 
> version_id)
>  int fd = -1;
>  
>  if (version_id < 1 || version_id > 1) {
> -fprintf(stderr, "htab_load() bad version\n");
> +error_report("htab_load() bad version");
>  return -EINVAL;
>  }
>  
> @@ -1556,8 +1557,8 @@ static int htab_load(QEMUFile *f, void *opaque, int 
> version_id)
>  
>  fd = kvmppc_get_htab_fd(true);
>  if (fd < 0) {
> -fprintf(stderr, "Unable to open fd to restore KVM hash table: 
> %s\n",
> -strerror(errno));
> +error_report("Unable to open fd to restore KVM hash table: %s",
> + strerror(errno));
>  }
>  }
>  
> @@ -1577,9 +1578,9 @@ static int htab_load(QEMUFile *f, void *opaque, int 
> version_id)
>  if ((index + n_valid + n_invalid) >
>  (HTAB_SIZE(spapr) / HASH_PTE_SIZE_64)) {
>  /* Bad index in stream */
> -fprintf(stderr, "htab_load() bad index %d (%hd+%hd entries) "
> -"in htab stream (htab_shift=%d)\n", index, n_valid, 
> n_invalid,
> -spapr->htab_shift);
> +error_report(
> +"htab_load() bad index %d (%hd+%hd entries) in htab stream 
> (htab_shift=%d)\n",
> +index, n_valid, n_invalid, spapr->htab_shift);
>  return -EINVAL;
>  }

With the typo fixed in the patch description:

Reviewed-by: Thomas Huth 

Re: [Qemu-devel] [PATCH v2 for 2.6 0/5] QMP wrappers for VM snapshot operations

[Denis V. Lunev]

On 12/04/2015 05:44 PM, Denis V. Lunev wrote:

EFI based VM with pflash storage for NVRAM could not be snapshoted as
libvirt configures storage as 'raw' and writable. OK, this is a libvirt
problem.

Another problem is that libvirt can not detect this failure at all
as it uses HMP for this operation. This create snapshot/delete snapshot
sequence passes silently.

The patchset adds QMP wrappers for the purpose.

Signed-off-by: "Denis V. Lunev" 
CC: Juan Quintela 
CC: Amit Shah 
CC: Markus Armbruster 
CC: Eric Blake 

Changes from v1:
- cosmetic fixes suggested by Markus. I pray I have added all of them :)
- patch 5 is rewritten completely. Original one was deadbeaf

Denis V. Lunev (5):
   migration: split hmp_savevm to do_savevm and hmp_savevm wrapper
   qmp: create qmp_savevm command
   qmp: create qmp_delvm command
   migration: improve error reporting for hmp_loadvm
   qmp: create QMP implementation of loadvm command

  include/sysemu/sysemu.h |   2 +-
  migration/savevm.c  | 100 +++-
  monitor.c   |   7 +++-
  qapi-schema.json|  39 +++
  qmp-commands.hx |  71 ++
  vl.c|   5 ++-
  6 files changed, 185 insertions(+), 39 deletions(-)


ping

Re: [Qemu-devel] [PATCH COLO-Frame v11 27/39] COLO failover: Shutdown related socket fd when do failover

[Hailiang Zhang]

On 2015/12/11 17:18, Dr. David Alan Gilbert wrote:

* Hailiang Zhang (zhang.zhanghaili...@huawei.com) wrote:

On 2015/12/11 4:03, Dr. David Alan Gilbert wrote:

* zhanghailiang (zhang.zhanghaili...@huawei.com) wrote:

If the net connection between COLO's two sides is broken while colo/colo 
incoming
thread is blocked in 'read'/'write' socket fd. It will not detect this error 
until
connect timeout. It will be a long time.

Here we shutdown all the related socket file descriptors to wake up the blocking
operation in failover BH. Besides, we should close the corresponding file 
descriptors
after failvoer BH shutdown them, or there will be an error.

Signed-off-by: zhanghailiang 
Signed-off-by: Li Zhijian 
---
v11:
- Only shutdown fd for once
---
  migration/colo.c | 31 +--
  1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index 4cd7b00..994b80d 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -68,6 +68,14 @@ static void secondary_vm_do_failover(void)
  /* recover runstate to normal migration finish state */
  autostart = true;
  }
+/*
+* Make sure colo incoming thread not block in recv,
+* mis->from_src_file and mis->to_src_file use the same fd,
+* so here we only need to shutdown it for once.
+*/


That assumption is only valid for the current socket transport;
for example I have a (partially-working) RDMA transport where the
forward and reverse paths are quite separate.



Yes, you are right, but we really catch the bad stuck case in test for many 
times,
and it is easy to reproduce. So i think it's necessary to keep this codes.


Yes, I agree you must shutdown from_src_file, but I mean do you also need to
shutdown to_src_file ?  On sockets they share an fd, but that might not always 
be


Yes, in that case, we need to shutdown it, i will add it. Thanks.


true, so you might have to shutdown both QEMUFile's - but only if you can
get blocked on them.




+if (mis->from_src_file) {
+qemu_file_shutdown(mis->from_src_file);
+}

  old_state = failover_set_state(FAILOVER_STATUS_HANDLING,
 FAILOVER_STATUS_COMPLETED);
@@ -92,6 +100,15 @@ static void primary_vm_do_failover(void)
MIGRATION_STATUS_COMPLETED);
  }

+/*
+* Make sure colo thread no block in recv,
+* Besides, s->rp_state.from_dst_file and s->to_dst_file use the
+* same fd, so here we only need to shutdown it for once.
+*/
+if (s->to_dst_file) {
+qemu_file_shutdown(s->to_dst_file);
+}
+
  old_state = failover_set_state(FAILOVER_STATUS_HANDLING,
 FAILOVER_STATUS_COMPLETED);
  if (old_state != FAILOVER_STATUS_HANDLING) {
@@ -333,7 +350,7 @@ static void colo_process_checkpoint(MigrationState *s)

  out:
  current_time = error_time = qemu_clock_get_ms(QEMU_CLOCK_HOST);
-if (ret < 0) {
+if (ret < 0 || (!ret && !failover_request_is_active())) {


Why is this needed - when would you get a 0 return that is actually an error?



If we shutdown the fd, it will return 0, and the ret will be 0, this only happen
when we do failover actively.


OK


  error_report("%s: %s", __func__, strerror(-ret));
  qapi_event_send_colo_exit(COLO_MODE_PRIMARY, COLO_EXIT_REASON_ERROR,
true, strerror(-ret), NULL);
@@ -362,6 +379,11 @@ out:
  qsb_free(buffer);
  buffer = NULL;

+/* Hope this not to be too long to loop here */
+while (failover_get_state() != FAILOVER_STATUS_COMPLETED) {
+;
+}
+/* Must be called after failover BH is completed */


Is this the right patch for this?



Yes, we must ensure the close() called after the shutdown() in failover,
or there maybe an shutdown wrong 'fd' error, the 'fd' maybe just re-used by
other thread after we close here, but before shutdown() in failover. This
is a race case, which is rarely happening. I'll add more comments here.


Thanks, that makes sense; closing the wrong fd can be really hard to debug :-)

Dave



Thanks,
Hailiang



  if (s->rp_state.from_dst_file) {
  qemu_fclose(s->rp_state.from_dst_file);
  }
@@ -534,7 +556,7 @@ void *colo_process_incoming_thread(void *opaque)

  out:
  current_time = error_time = qemu_clock_get_ms(QEMU_CLOCK_HOST);
-if (ret < 0) {
+if (ret < 0 || (!ret && !failover_request_is_active())) {
  error_report("colo incoming thread will exit, detect error: %s",
   strerror(-ret));
  qapi_event_send_colo_exit(COLO_MODE_SECONDARY, COLO_EXIT_REASON_ERROR,
@@ -573,6 +595,11 @@ out:
  */
  colo_release_ram_cache();

+/* Hope this not to be too long to loop here */
+while (failover_get_state() != FAILOVER_STATUS_COMPLETED) {
+;
+}
+/* Must be called after failover BH is completed 

Re: [Qemu-devel] [PATCH 08/11] pseries: Clean up error handling in spapr_rtas_register()

[Thomas Huth]

On 11/12/15 01:11, David Gibson wrote:
> The errors detected in this function indicate problems with the rest of
> the machine type code, rather than configuration or runtime problems.
> 
> Use error_setg() and _abort instead of explicit fprintf() and exit().
> 
> Signed-off-by: David Gibson 
> ---
>  hw/ppc/spapr_rtas.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 34b12a3..db4a675 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -649,15 +649,14 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, 
> sPAPRMachineState *spapr,
>  void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
>  {
>  if (!((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX))) {
> -fprintf(stderr, "RTAS invalid token 0x%x\n", token);
> -exit(1);
> +error_setg(_abort, "RTAS invalid token 0x%x", token);
>  }
>  
>  token -= RTAS_TOKEN_BASE;
>  if (rtas_table[token].name) {
> -fprintf(stderr, "RTAS call \"%s\" is registered already as 0x%x\n",
> -rtas_table[token].name, token);
> -exit(1);
> +error_setg(_abort,
> +   "RTAS call \"%s\" is registered already as 0x%x",
> +   rtas_table[token].name, token);
>  }

As with the last patch, I also think these error_setg(_abort, ...)
are rather ugly. Maybe use error_report() + abort() instead?

 Thomas

Re: [Qemu-devel] [PATCH 0/7] igd passthrough chipset tweaks

[Stefano Stabellini]

On Tue, 8 Dec 2015, Gerd Hoffmann wrote:
>   Hi,
> 
> We have some code in our tree to support pci passthrough of intel
> graphics devices (igd) on xen, which requires some chipset tweaks
> for (a) the host bridge and (b) the lpc/isa-bridge to meat the
> expectations of the guest driver.  For kvm we need pretty much
> the same, also the requirements for vgpu (xengt/kvmgt) are very
> simliar.
> 
> This patch series tackles (a) only, (b) will follow later.  It
> wires up the igd-passthru machine option for tcg/kvm too, moves
> the code to its own file so it is nicely separated, fixes a bunch
> of issues and finally adds q35 support.
> 
> This patch series has seen very light testing, basically doing
> lspci in the guest to check whenever pci config space got updated
> correctly.  Trying actual device assignment needs more pieces
> being in place.  But I suspect even that is more testing than
> the code has seen on xen so far (see patch #6 ...).

I for one don't have a setup to be able to test this at the moment. But
I would appreciate if this kind of changes were Tested-by Tiejun Chen.


> Gerd Hoffmann (7):
>   pc: wire up TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE for !xen
>   pc: move igd support code to igd.c
>   igd: switch TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE to realize
>   igd: TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE: call parent realize
>   igd: use defines for standard pci config space offsets
>   igd: revamp host config read
>   igd: add q35 support
> 
>  hw/i386/pc_piix.c |  11 ++--
>  hw/pci-host/Makefile.objs |   3 ++
>  hw/pci-host/igd.c | 132 
> ++
>  hw/pci-host/piix.c|  88 ---
>  hw/pci-host/q35.c |   6 ++-
>  5 files changed, 145 insertions(+), 95 deletions(-)
>  create mode 100644 hw/pci-host/igd.c
> 
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 01/18] slirp: goto bad in udp_input if sosendto fails

[Thomas Huth]

On 11/12/15 01:15, Samuel Thibault wrote:
> From: Guillaume Subiron 
> 
> Before this patch, if sosendto fails, udp_input is executed as if the
> packet was sent, recording the packet for icmp errors, which does not
> makes sense since the packet was not actually sent, errors would be
> related to a previous packet.
> 
> This patch adds a goto bad to cut the execution of this function.
> 
> Signed-off-by: Guillaume Subiron 
> Signed-off-by: Samuel Thibault 
> ---
>  slirp/debug.h | 2 +-
>  slirp/udp.c   | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/slirp/debug.h b/slirp/debug.h
> index 6cfa61e..c60f967 100644
> --- a/slirp/debug.h
> +++ b/slirp/debug.h
> @@ -5,7 +5,7 @@
>   * terms and conditions of the copyright.
>   */
>  
> -//#define DEBUG 1
> +#define DEBUG 1

Please don't enable the debug code by default.

> diff --git a/slirp/udp.c b/slirp/udp.c
> index fee13b4..ce63414 100644
> --- a/slirp/udp.c
> +++ b/slirp/udp.c
> @@ -218,6 +218,7 @@ udp_input(register struct mbuf *m, int iphlen)
> *ip=save_ip;
> DEBUG_MISC((dfd,"udp tx errno = %d-%s\n",errno,strerror(errno)));
> icmp_error(m, ICMP_UNREACH,ICMP_UNREACH_NET, 0,strerror(errno));
> +   goto bad;
>   }
>  
>   m_free(so->so_m);   /* used for ICMP if error on sorecvfrom */

That change looks sane to me.

 Thomas

Re: [Qemu-devel] [PATCH 01/18] slirp: goto bad in udp_input if sosendto fails

[Samuel Thibault]

Thomas Huth, on Fri 11 Dec 2015 12:54:14 +0100, wrote:
> > @@ -5,7 +5,7 @@
> >   * terms and conditions of the copyright.
> >   */
> >  
> > -//#define DEBUG 1
> > +#define DEBUG 1
> 
> Please don't enable the debug code by default.

Oops, sorry, that wasn't meant to be included in the patch series.

Samuel

Re: [Qemu-devel] [PATCH v3 0/9] Introduce I/O channels framework

[Paolo Bonzini]

On 11/12/2015 11:53, Daniel P. Berrange wrote:
> Ping, unless anyone wants to review this further I'll send a pull
> request for it once 2.6 opens up, so I can focus on getting the
> other dependant patch series reviewed & merged in 2.6 too.

Yes, please do.

Paolo

Re: [Qemu-devel] [PATCH 08/11] qjson: Simplify by using json-output-visitor

[Paolo Bonzini]

On 11/12/2015 00:53, Eric Blake wrote:
> Instead of rolling our own limited JSON outputter, we can just
> wrap the more full-featured JSON output Visitor.
> 
> This slightly changes the output (different spacing), but the
> result is still equivalent JSON contents.
> 
> Signed-off-by: Eric Blake 
> ---
>  qjson.c | 61 ++---
>  1 file changed, 22 insertions(+), 39 deletions(-)

I didn't find out which tree your patches apply to, but this:

diff --git a/migration/savevm.c b/migration/savevm.c
index 0ad1b93..26986d8 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -646,7 +646,7 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, 
int version_id)
 return vmstate_load_state(f, se->vmsd, se->opaque, version_id);
 }
 
-static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se, QJSON 
*vmdesc)
+static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se, Visitor 
*vmdesc)
 {
 int64_t old_offset, size;
 
@@ -655,14 +655,14 @@ static void vmstate_save_old_style(QEMUFile *f, 
SaveStateEntry *se, QJSON *vmdes
 size = qemu_ftell_fast(f) - old_offset;
 
 if (vmdesc) {
-json_prop_int(vmdesc, "size", size);
-json_start_array(vmdesc, "fields");
-json_start_object(vmdesc, NULL);
-json_prop_str(vmdesc, "name", "data");
-json_prop_int(vmdesc, "size", size);
-json_prop_str(vmdesc, "type", "buffer");
-json_end_object(vmdesc);
-json_end_array(vmdesc);
+visit_type_int(vmdesc, size, "size", _abort);
+visit_start_list(vmdesc, NULL, 0, "fields", _abort);
+visit_start_struct(vmdesc, NULL, 0, NULL, _abort);
+visit_type_str(vmdesc, (char **)&"data", "name", _abort);
+visit_type_int(vmdesc, size, "size", _abort);
+visit_type_str(vmdesc, (char **)&"buffer", "type", _abort);
+visit_end_struct(vmdesc);
+visit_end_list(vmdesc);
 }
 }
 
@@ -1028,7 +1028,9 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
 
 void qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only)
 {
-QJSON *vmdesc;
+JsonOutputVisitor *vmdesc_jov;
+Visitor *vmdesc;
+char *vmdesc_str;
 int vmdesc_len;
 SaveStateEntry *se;
 int ret;
@@ -1068,9 +1070,11 @@ void qemu_savevm_state_complete_precopy(QEMUFile *f, 
bool iterable_only)
 return;
 }
 
-vmdesc = qjson_new();
-json_prop_int(vmdesc, "page_size", TARGET_PAGE_SIZE);
-json_start_array(vmdesc, "devices");
+vmdesc_jov = json_output_visitor_new();
+vmdesc = json_output_get_visitor(vmdesc_jov);
+visit_start_struct(vmdesc, NULL, 0, NULL, _abort);
+visit_type_int(vmdesc, TARGET_PAGE_SIZE, "page_size", _abort);
+visit_start_list(vmdesc, NULL, 0, "devices", _abort);
 QTAILQ_FOREACH(se, _state.handlers, entry) {
 
 if ((!se->ops || !se->ops->save_state) && !se->vmsd) {
@@ -1083,15 +1087,15 @@ void qemu_savevm_state_complete_precopy(QEMUFile *f, 
bool iterable_only)
 
 trace_savevm_section_start(se->idstr, se->section_id);
 
-json_start_object(vmdesc, NULL);
-json_prop_str(vmdesc, "name", se->idstr);
-json_prop_int(vmdesc, "instance_id", se->instance_id);
+visit_start_struct(vmdesc, NULL, 0, NULL, _abort);
+visit_type_str(vmdesc, (char **)>idstr, "name", _abort);
+visit_type_int(vmdesc, se->instance_id, "instance_id", _abort);
 
 save_section_header(f, se, QEMU_VM_SECTION_FULL);
 
 vmstate_save(f, se, vmdesc);
 
-json_end_object(vmdesc);
+visit_end_struct(vmdesc);
 trace_savevm_section_end(se->idstr, se->section_id, 0);
 save_section_footer(f, se);
 }
@@ -1101,16 +1105,18 @@ void qemu_savevm_state_complete_precopy(QEMUFile *f, 
bool iterable_only)
 qemu_put_byte(f, QEMU_VM_EOF);
 }
 
-json_end_array(vmdesc);
-qjson_finish(vmdesc);
-vmdesc_len = strlen(qjson_get_str(vmdesc));
+visit_end_list(vmdesc);
+visit_end_struct(vmdesc);
+vmdesc_str = json_output_get_string(vmdesc_jov);
+vmdesc_len = strlen(vmdesc_str);
 
 if (should_send_vmdesc()) {
 qemu_put_byte(f, QEMU_VM_VMDESCRIPTION);
 qemu_put_be32(f, vmdesc_len);
-qemu_put_buffer(f, (uint8_t *)qjson_get_str(vmdesc), vmdesc_len);
+qemu_put_buffer(f, (uint8_t *)vmdesc_str, vmdesc_len);
 }
-object_unref(OBJECT(vmdesc));
+g_free(vmdesc_str);
+json_output_visitor_cleanup(vmdesc_jov);
 
 qemu_fflush(f);
 }

compiles and is pretty much a 1:1 translation from the qjson.c API to the
visitor API (using this patch as a guide).  Feel free to include it and
remove qjson.c.  Alternatively, you can leave out this patch and I'll test
and submit the transition.

Paolo

Re: [Qemu-devel] [PATCH 01/11] qapi: Rename qjson.h to qobject-json.h

[Paolo Bonzini]

On 11/12/2015 00:53, Eric Blake wrote:
> We have two different JSON visitors in the tree; and having both
> named 'qjson.h' can cause include confusion.  Rename the qapi
> version.
> 
> Kill trailing whitespace in the renamed tests/check-qobject-json.c
> to keep checkpatch.pl happy.
> 
> Signed-off-by: Eric Blake 
> ---
>  MAINTAINERS   |  2 +-
>  balloon.c |  2 +-
>  block.c   |  2 +-
>  block/archipelago.c   |  2 +-
>  block/nbd.c   |  2 +-
>  block/quorum.c|  2 +-
>  blockjob.c|  2 +-
>  hw/core/qdev.c|  2 +-
>  hw/misc/pvpanic.c |  2 +-
>  hw/net/virtio-net.c   |  2 +-
>  include/qapi/qmp/{qjson.h => qobject-json.h}  |  0
>  include/qapi/qmp/types.h  |  2 +-
>  monitor.c |  2 +-
>  qapi/qmp-event.c  |  2 +-
>  qemu-img.c|  2 +-
>  qga/main.c|  2 +-
>  qobject/Makefile.objs |  3 ++-
>  qobject/{qjson.c => qobject-json.c}   |  2 +-
>  target-s390x/kvm.c|  2 +-
>  tests/.gitignore  |  2 +-
>  tests/Makefile|  8 
>  tests/{check-qjson.c => check-qobject-json.c} | 14 +++---
>  tests/libqtest.c  |  2 +-
>  ui/spice-core.c   |  2 +-
>  vl.c  |  2 +-
>  25 files changed, 34 insertions(+), 33 deletions(-)
>  rename include/qapi/qmp/{qjson.h => qobject-json.h} (100%)
>  rename qobject/{qjson.c => qobject-json.c} (99%)
>  rename tests/{check-qjson.c => check-qobject-json.c} (99%)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e8cee1e..c943ff4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1155,7 +1155,7 @@ X: include/qapi/qmp/dispatch.h
>  F: tests/check-qdict.c
>  F: tests/check-qfloat.c
>  F: tests/check-qint.c
> -F: tests/check-qjson.c
> +F: tests/check-qobject-json.c
>  F: tests/check-qlist.c
>  F: tests/check-qstring.c
>  T: git git://repo.or.cz/qemu/qmp-unstable.git queue/qmp
> diff --git a/balloon.c b/balloon.c
> index 0f45d1b..5983b4f 100644
> --- a/balloon.c
> +++ b/balloon.c
> @@ -31,7 +31,7 @@
>  #include "trace.h"
>  #include "qmp-commands.h"
>  #include "qapi/qmp/qerror.h"
> -#include "qapi/qmp/qjson.h"
> +#include "qapi/qmp/qobject-json.h"
> 
>  static QEMUBalloonEvent *balloon_event_fn;
>  static QEMUBalloonStatus *balloon_stat_fn;
> diff --git a/block.c b/block.c
> index 9971976..e611002 100644
> --- a/block.c
> +++ b/block.c
> @@ -29,7 +29,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/module.h"
>  #include "qapi/qmp/qerror.h"
> -#include "qapi/qmp/qjson.h"
> +#include "qapi/qmp/qobject-json.h"
>  #include "sysemu/block-backend.h"
>  #include "sysemu/sysemu.h"
>  #include "qemu/notify.h"
> diff --git a/block/archipelago.c b/block/archipelago.c
> index 855655c..80a1bb5 100644
> --- a/block/archipelago.c
> +++ b/block/archipelago.c
> @@ -56,7 +56,7 @@
>  #include "qemu/thread.h"
>  #include "qapi/qmp/qint.h"
>  #include "qapi/qmp/qstring.h"
> -#include "qapi/qmp/qjson.h"
> +#include "qapi/qmp/qobject-json.h"
>  #include "qemu/atomic.h"
> 
>  #include 
> diff --git a/block/nbd.c b/block/nbd.c
> index cd6a587..9009f4f 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -32,7 +32,7 @@
>  #include "qemu/module.h"
>  #include "qemu/sockets.h"
>  #include "qapi/qmp/qdict.h"
> -#include "qapi/qmp/qjson.h"
> +#include "qapi/qmp/qobject-json.h"
>  #include "qapi/qmp/qint.h"
>  #include "qapi/qmp/qstring.h"
> 
> diff --git a/block/quorum.c b/block/quorum.c
> index d162459..1e3a278 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -18,7 +18,7 @@
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qapi/qmp/qint.h"
> -#include "qapi/qmp/qjson.h"
> +#include "qapi/qmp/qobject-json.h"
>  #include "qapi/qmp/qlist.h"
>  #include "qapi/qmp/qstring.h"
>  #include "qapi-event.h"
> diff --git a/blockjob.c b/blockjob.c
> index 80adb9d..84361f7 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -31,7 +31,7 @@
>  #include "block/block_int.h"
>  #include "sysemu/block-backend.h"
>  #include "qapi/qmp/qerror.h"
> -#include "qapi/qmp/qjson.h"
> +#include "qapi/qmp/qobject-json.h"
>  #include "qemu/coroutine.h"
>  #include "qmp-commands.h"
>  #include "qemu/timer.h"
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index b3ad467..a98304d 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -31,7 +31,7 @@
>  #include "qapi/error.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qapi/visitor.h"
> -#include "qapi/qmp/qjson.h"
> +#include "qapi/qmp/qobject-json.h"
>  

Re: [Qemu-devel] [PATCH 0/7] igd passthrough chipset tweaks

[Stefano Stabellini]

Actually CC'ing xen-devel

On Fri, 11 Dec 2015, Stefano Stabellini wrote:
> On Tue, 8 Dec 2015, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > We have some code in our tree to support pci passthrough of intel
> > graphics devices (igd) on xen, which requires some chipset tweaks
> > for (a) the host bridge and (b) the lpc/isa-bridge to meat the
> > expectations of the guest driver.  For kvm we need pretty much
> > the same, also the requirements for vgpu (xengt/kvmgt) are very
> > simliar.
> > 
> > This patch series tackles (a) only, (b) will follow later.  It
> > wires up the igd-passthru machine option for tcg/kvm too, moves
> > the code to its own file so it is nicely separated, fixes a bunch
> > of issues and finally adds q35 support.
> > 
> > This patch series has seen very light testing, basically doing
> > lspci in the guest to check whenever pci config space got updated
> > correctly.  Trying actual device assignment needs more pieces
> > being in place.  But I suspect even that is more testing than
> > the code has seen on xen so far (see patch #6 ...).
> 
> I for one don't have a setup to be able to test this at the moment. But
> I would appreciate if this kind of changes were Tested-by Tiejun Chen.
> 
> 
> > Gerd Hoffmann (7):
> >   pc: wire up TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE for !xen
> >   pc: move igd support code to igd.c
> >   igd: switch TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE to realize
> >   igd: TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE: call parent realize
> >   igd: use defines for standard pci config space offsets
> >   igd: revamp host config read
> >   igd: add q35 support
> > 
> >  hw/i386/pc_piix.c |  11 ++--
> >  hw/pci-host/Makefile.objs |   3 ++
> >  hw/pci-host/igd.c | 132 
> > ++
> >  hw/pci-host/piix.c|  88 ---
> >  hw/pci-host/q35.c |   6 ++-
> >  5 files changed, 145 insertions(+), 95 deletions(-)
> >  create mode 100644 hw/pci-host/igd.c
> > 
> > -- 
> > 1.8.3.1
> > 
> > 
> 

Re: [Qemu-devel] [PATCH COLO-Frame v11 23/39] COLO: Implement failover work for Primary VM

[Hailiang Zhang]

On 2015/12/11 17:22, Dr. David Alan Gilbert wrote:

* Hailiang Zhang (zhang.zhanghaili...@huawei.com) wrote:

On 2015/12/11 2:34, Dr. David Alan Gilbert wrote:

* zhanghailiang (zhang.zhanghaili...@huawei.com) wrote:

For PVM, if there is failover request from users.
The colo thread will exit the loop while the failover BH does the
cleanup work and resumes VM.

Signed-off-by: zhanghailiang 
Signed-off-by: Li Zhijian 
---
v11:
- Don't call migration_end() in primary_vm_do_failover(),
  The cleanup work will be done in migration_thread().
- Remove vm_start() in primary_vm_do_failover() which also been done
   in migraiton_thread()
v10:
- Call migration_end() in primary_vm_do_failover()
---
  include/migration/colo.h |  3 +++
  include/migration/failover.h |  1 +
  migration/colo-failover.c|  7 +-
  migration/colo.c | 56 ++--
  4 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/include/migration/colo.h b/include/migration/colo.h
index ba27719..0b02e95 100644
--- a/include/migration/colo.h
+++ b/include/migration/colo.h
@@ -32,4 +32,7 @@ void *colo_process_incoming_thread(void *opaque);
  bool migration_incoming_in_colo_state(void);

  COLOMode get_colo_mode(void);
+
+/* failover */
+void colo_do_failover(MigrationState *s);
  #endif
diff --git a/include/migration/failover.h b/include/migration/failover.h
index 882c625..fba3931 100644
--- a/include/migration/failover.h
+++ b/include/migration/failover.h
@@ -26,5 +26,6 @@ void failover_init_state(void);
  int failover_set_state(int old_state, int new_state);
  int failover_get_state(void);
  void failover_request_active(Error **errp);
+bool failover_request_is_active(void);

  #endif
diff --git a/migration/colo-failover.c b/migration/colo-failover.c
index 1b1be24..0c525da 100644
--- a/migration/colo-failover.c
+++ b/migration/colo-failover.c
@@ -32,7 +32,7 @@ static void colo_failover_bh(void *opaque)
  error_report("Unkown error for failover, old_state=%d", old_state);
  return;
  }
-/*TODO: Do failover work */
+colo_do_failover(NULL);
  }

  void failover_request_active(Error **errp)
@@ -67,6 +67,11 @@ int failover_get_state(void)
  return atomic_read(_state);
  }

+bool failover_request_is_active(void)
+{
+return ((failover_get_state() != FAILOVER_STATUS_NONE));
+}
+
  void qmp_x_colo_lost_heartbeat(Error **errp)
  {
  if (get_colo_mode() == COLO_MODE_UNKNOWN) {
diff --git a/migration/colo.c b/migration/colo.c
index cedfc63..7a42fc6 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -41,6 +41,42 @@ bool migration_incoming_in_colo_state(void)
  return mis && (mis->state == MIGRATION_STATUS_COLO);
  }

+static bool colo_runstate_is_stopped(void)
+{
+return runstate_check(RUN_STATE_COLO) || !runstate_is_running();
+}
+
+static void primary_vm_do_failover(void)
+{
+MigrationState *s = migrate_get_current();
+int old_state;
+
+if (s->state != MIGRATION_STATUS_FAILED) {
+migrate_set_state(>state, MIGRATION_STATUS_COLO,
+  MIGRATION_STATUS_COMPLETED);
+}


That's a little odd; it will only move to completed if the current
state is MIGRATION_STATUS_COLO, but you only do it if the state isn't
FAILED.  You could remove the if and just call migrate_set_state
like that and it would be safe as long as you really only expect
to have to deal with MIGRATION_STATUS_COLO.



Yes, you are right, i will remove the judgement, and set the state directly.


+old_state = failover_set_state(FAILOVER_STATUS_HANDLING,
+   FAILOVER_STATUS_COMPLETED);
+if (old_state != FAILOVER_STATUS_HANDLING) {
+error_report("Serious error while do failover for Primary VM,"
+ "old_state: %d", old_state);


It's generally better to state the reason rather than say it's 'serious';
so something like:
'Incorrect state (%d) while doing failover for Primary VM'
tells you more, and looks less scary!



Ha, OK, i will fix it.


+return;
+}
+}
+
+void colo_do_failover(MigrationState *s)
+{
+/* Make sure vm stopped while failover */
+if (!colo_runstate_is_stopped()) {
+vm_stop_force_state(RUN_STATE_COLO);
+}


That feels like a race? couldn't we be just at the end
of taking a checkpoint and about to restart when you do
the if, so it reads that it's currently stopped but
then it restarts it by the time you have a chance to
do anything?


Do you mean, after we stopped VM in failover() but before done the failover 
work,
the migration (checkpoint) thread may starts VM just in the middle time ?
The colo_do_failover() is in the context of BH, it holds
the __lock__, so checkpoint thread has no chance to execute vm_start().


What happens if the failover code was executed just before the checkpoint thread
executed the _lock_, when the failover code finishes what happens?
(I'm not 

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[Dmitry Fleytman]

Sent from my iPhone

> On 11 Dec 2015, at 11:10, Jason Wang  wrote:
> 
> 
> 
>> On 12/09/2015 11:28 PM, P J P wrote:
>>   Hello Jason, Dmitry,
>> 
>> +-- On Tue, 8 Dec 2015, P J P wrote --+
>> | |1) VMXNET3_CMD_QUIESCE_DEV
>> | 
>> |   IIUC, it is used to pause the device when the receiver end is unable to 
>> | keee-up with the incoming flow. After a brief period, the operation could 
>> be 
>> | resumed again.
>> | 
>> | |2) VMXNET3_REG_DSAL
>> | 
>> |Shared memory between a driver and the device appears to be set in two 
>> | steps. Firs low address, followed by the high address(VMXNET3_REG_DSAH). I 
>> | guess 's->device_active' needs to be enabled again while setting the 
>> higher 
>> | part of the address.
>> 
>> Please see below another (tested)patch, it fixes the VMXNET3_CMD_QUIESCE_DEV 
>> case above. It's not clear if the device would be initialised and active 
>> while 
>> setting shared memory via VMXNET3_REG_DSAL/DSAH.
> 
> I think it's possible for attacker. Better wait for Dmitry's answer for
> this.

Hi Guys,

I'd like to review this carefully. I'll post my thoughts tomorrow or on Sunday.

Thanks,
Dmitry


> 
>> 
>> ===
>> From 81c4ecb67635435f01397dc21210497e6420bdca Mon Sep 17 00:00:00 2001
>> From: Prasad J Pandit 
>> Date: Wed, 9 Dec 2015 20:45:25 +0530
>> Subject: [PATCH] net: vmxnet3: avoid memory leakage in activate_device
>> 
>> Vmxnet3 device emulator does not check if the device is active
>> before activating it, resulting in memory leakage on the host.
>> This memory leakage could also occur, if the device was paused
>> during flow control and activated thereafter.
>> 
>> Introduced a 'Pause' state for the vmxnet3 device to differentiate
>> it from the earlier active and inactive states. One need not
>> 'activate' the device to resume operation after pause.
>> 
>> This patch adds a check to verify these device states and avoid
>> memory leakage during activating the device.
>> 
>> Reported-by: Qinghao Tang 
>> Signed-off-by: Prasad J Pandit 
>> ---
>> hw/net/vmxnet3.c | 43 +++
>> hw/net/vmxnet3.h |  6 ++
>> 2 files changed, 41 insertions(+), 8 deletions(-)
>> 
>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>> index 37373e5..be2c9e2 100644
>> --- a/hw/net/vmxnet3.c
>> +++ b/hw/net/vmxnet3.c
>> @@ -1195,7 +1195,19 @@ static void vmxnet3_reset_mac(VMXNET3State *s)
>> static void vmxnet3_deactivate_device(VMXNET3State *s)
>> {
>> VMW_CBPRN("Deactivating vmxnet3...");
>> -s->device_active = false;
>> +s->device_active = VMXNET3_DEV_DEACTIVE;
>> +}
>> +
>> +static void vmxnet3_pause_device(VMXNET3State *s)
>> +{
>> +VMW_CBPRN("Pausing vmxnet3...");
>> +s->device_active = VMXNET3_DEV_PAUSE;
>> +}
>> +
>> +static void vmxnet3_resume_device(VMXNET3State *s)
>> +{
>> +VMW_CBPRN("Resuming vmxnet3...");
>> +s->device_active = VMXNET3_DEV_ACTIVE;
>> }
>> 
>> static void vmxnet3_reset(VMXNET3State *s)
>> @@ -1431,6 +1443,12 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>> return;
>> }
>> 
>> +/* Verify if device is active */
>> +if (s->device_active) {
>> +VMW_CFPRN("Vmxnet3 device is active");
>> +return;
>> +}
> 
> What if guest want to activate a paused device?
> 
>> +
>> vmxnet3_adjust_by_guest_type(s);
>> vmxnet3_update_features(s);
>> vmxnet3_update_pm_state(s);
>> @@ -1566,7 +1584,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>> 
>> vmxnet3_reset_mac(s);
>> 
>> -s->device_active = true;
>> +s->device_active = VMXNET3_DEV_ACTIVE;
>> }
>> 
>> static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd)
>> @@ -1627,8 +1645,13 @@ static void vmxnet3_handle_command(VMXNET3State *s, 
>> uint64_t cmd)
>> break;
>> 
>> case VMXNET3_CMD_QUIESCE_DEV:
>> -VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
>> -vmxnet3_deactivate_device(s);
>> +if (s->device_active & VMXNET3_DEV_ACTIVE) {
>> +VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
>> +vmxnet3_pause_device(s);
>> +} else if (s->device_active & VMXNET3_DEV_PAUSE) {
>> +VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - resume the device");
>> +vmxnet3_resume_device(s);
>> +}
> 
> Not sure this is the correct behavior. Is there a link to the spec?
> 
>> break;
>> 
>> case VMXNET3_CMD_GET_CONF_INTR:
>> @@ -1652,12 +1675,16 @@ static uint64_t 
>> vmxnet3_get_command_status(VMXNET3State *s)
>> 
>> switch (s->last_command) {
>> case VMXNET3_CMD_ACTIVATE_DEV:
>> -ret = (s->device_active) ? 0 : -1;
>> +ret = (s->device_active & VMXNET3_DEV_ACTIVE) ? 0 : -1;
>> VMW_CFPRN("Device active: %" PRIx64, ret);
>> break;
>> 
>> -case VMXNET3_CMD_RESET_DEV:
>> case VMXNET3_CMD_QUIESCE_DEV:
>> +ret = 

Re: [Qemu-devel] [PATCH 11/16] pc: Simplify signature of xen_load_linux()

[Stefano Stabellini]

On Tue, 1 Dec 2015, Eduardo Habkost wrote:
> We don't need the FWCfgState return value and the PcGuestInfo
> parameter.
> 
> Signed-off-by: Eduardo Habkost 

Reviewed-by: Stefano Stabellini 


>  hw/i386/pc.c | 5 +
>  hw/i386/pc_piix.c| 2 +-
>  include/hw/i386/pc.h | 3 +--
>  3 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index a219187..a9ec402 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1253,8 +1253,7 @@ void pc_acpi_init(const char *default_dsdt)
>  }
>  }
>  
> -FWCfgState *xen_load_linux(PCMachineState *pcms,
> -   PcGuestInfo *guest_info)
> +void xen_load_linux(PCMachineState *pcms)
>  {
>  int i;
>  FWCfgState *fw_cfg;
> @@ -1271,7 +1270,6 @@ FWCfgState *xen_load_linux(PCMachineState *pcms,
>  rom_add_option(option_rom[i].name, option_rom[i].bootindex);
>  }
>  pcms->fw_cfg = fw_cfg;
> -return fw_cfg;
>  }
>  
>  FWCfgState *pc_memory_init(PCMachineState *pcms,
> @@ -1401,7 +1399,6 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
>  rom_add_option(option_rom[i].name, option_rom[i].bootindex);
>  }
>  pcms->fw_cfg = fw_cfg;
> -return fw_cfg;
>  }
>  
>  qemu_irq pc_allocate_cpu_irq(void)
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index f7bc1c0..f39c086 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -164,7 +164,7 @@ static void pc_init1(MachineState *machine,
> rom_memory, _memory);
>  } else if (machine->kernel_filename != NULL) {
>  /* For xen HVM direct kernel boot, load linux here */
> -xen_load_linux(pcms, guest_info);
> +xen_load_linux(pcms);
>  }
>  
>  gsi_state = g_malloc0(sizeof(*gsi_state));
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 03750bc..2732a72 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -205,8 +205,7 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms);
>  void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
>  MemoryRegion *pci_address_space);
>  
> -FWCfgState *xen_load_linux(PCMachineState *pcms,
> -   PcGuestInfo *guest_info);
> +void xen_load_linux(PCMachineState *pcms);
>  FWCfgState *pc_memory_init(PCMachineState *pcms,
> MemoryRegion *system_memory,
> MemoryRegion *rom_memory,
> -- 
> 2.1.0
> 
> 

Re: [Qemu-devel] [PATCH 2/7] pc: move igd support code to igd.c

[Stefano Stabellini]

On Tue, 8 Dec 2015, Gerd Hoffmann wrote:
> Pure code motion, except for dropping instance_size for
> TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE (no need to set,
> we can inherit it from TYPE_I440FX_PCI_DEVICE).
> 
> Signed-off-by: Gerd Hoffmann 

Acked-by: Stefano Stabellini 


>  hw/pci-host/Makefile.objs |  3 ++
>  hw/pci-host/igd.c | 96 
> +++
>  hw/pci-host/piix.c| 88 ---
>  3 files changed, 99 insertions(+), 88 deletions(-)
>  create mode 100644 hw/pci-host/igd.c
> 
> diff --git a/hw/pci-host/Makefile.objs b/hw/pci-host/Makefile.objs
> index 45f1f0e..e341a49 100644
> --- a/hw/pci-host/Makefile.objs
> +++ b/hw/pci-host/Makefile.objs
> @@ -11,6 +11,9 @@ common-obj-$(CONFIG_PPCE500_PCI) += ppce500.o
>  # ARM devices
>  common-obj-$(CONFIG_VERSATILE_PCI) += versatile.o
>  
> +# igd passthrough support
> +common-obj-$(CONFIG_LINUX) += igd.o
> +
>  common-obj-$(CONFIG_PCI_APB) += apb.o
>  common-obj-$(CONFIG_FULONG) += bonito.o
>  common-obj-$(CONFIG_PCI_PIIX) += piix.o
> diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
> new file mode 100644
> index 000..ef0273b
> --- /dev/null
> +++ b/hw/pci-host/igd.c
> @@ -0,0 +1,96 @@
> +#include "qemu-common.h"
> +#include "hw/pci/pci.h"
> +#include "hw/i386/pc.h"
> +
> +/* IGD Passthrough Host Bridge. */
> +typedef struct {
> +uint8_t offset;
> +uint8_t len;
> +} IGDHostInfo;
> +
> +/* Here we just expose minimal host bridge offset subset. */
> +static const IGDHostInfo igd_host_bridge_infos[] = {
> +{0x08, 2},  /* revision id */
> +{0x2c, 2},  /* sybsystem vendor id */
> +{0x2e, 2},  /* sybsystem id */
> +{0x50, 2},  /* SNB: processor graphics control register */
> +{0x52, 2},  /* processor graphics control register */
> +{0xa4, 4},  /* SNB: graphics base of stolen memory */
> +{0xa8, 4},  /* SNB: base of GTT stolen memory */
> +};
> +
> +static int host_pci_config_read(int pos, int len, uint32_t val)
> +{
> +char path[PATH_MAX];
> +int config_fd;
> +ssize_t size = sizeof(path);
> +/* Access real host bridge. */
> +int rc = snprintf(path, size, 
> "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
> +  0, 0, 0, 0, "config");
> +int ret = 0;
> +
> +if (rc >= size || rc < 0) {
> +return -ENODEV;
> +}
> +
> +config_fd = open(path, O_RDWR);
> +if (config_fd < 0) {
> +return -ENODEV;
> +}
> +
> +if (lseek(config_fd, pos, SEEK_SET) != pos) {
> +ret = -errno;
> +goto out;
> +}
> +do {
> +rc = read(config_fd, (uint8_t *), len);
> +} while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> +if (rc != len) {
> +ret = -errno;
> +}
> +out:
> +close(config_fd);
> +return ret;
> +}
> +
> +static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
> +{
> +uint32_t val = 0;
> +int rc, i, num;
> +int pos, len;
> +
> +num = ARRAY_SIZE(igd_host_bridge_infos);
> +for (i = 0; i < num; i++) {
> +pos = igd_host_bridge_infos[i].offset;
> +len = igd_host_bridge_infos[i].len;
> +rc = host_pci_config_read(pos, len, val);
> +if (rc) {
> +return -ENODEV;
> +}
> +pci_default_write_config(pci_dev, pos, val, len);
> +}
> +
> +return 0;
> +}
> +
> +static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
> +{
> +DeviceClass *dc = DEVICE_CLASS(klass);
> +PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +k->init = igd_pt_i440fx_initfn;
> +dc->desc = "IGD Passthrough Host bridge";
> +}
> +
> +static const TypeInfo igd_passthrough_i440fx_info = {
> +.name  = TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE,
> +.parent= TYPE_I440FX_PCI_DEVICE,
> +.class_init= igd_passthrough_i440fx_class_init,
> +};
> +
> +static void igd_register_types(void)
> +{
> +type_register_static(_passthrough_i440fx_info);
> +}
> +
> +type_init(igd_register_types)
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 715208b..ccacb57 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -744,93 +744,6 @@ static const TypeInfo i440fx_info = {
>  .class_init= i440fx_class_init,
>  };
>  
> -/* IGD Passthrough Host Bridge. */
> -typedef struct {
> -uint8_t offset;
> -uint8_t len;
> -} IGDHostInfo;
> -
> -/* Here we just expose minimal host bridge offset subset. */
> -static const IGDHostInfo igd_host_bridge_infos[] = {
> -{0x08, 2},  /* revision id */
> -{0x2c, 2},  /* sybsystem vendor id */
> -{0x2e, 2},  /* sybsystem id */
> -{0x50, 2},  /* SNB: processor graphics control register */
> -{0x52, 2},  /* processor graphics control register */
> -{0xa4, 4},  /* SNB: graphics base of stolen memory */
> -{0xa8, 4},  /* SNB: base of GTT stolen memory */
> -};
> -
> -static int 

Re: [Qemu-devel] [PATCH 06/11] pseries: Improve error handling in find_unknown_sysbus_device()

[Thomas Huth]

On 11/12/15 01:11, David Gibson wrote:
> Use error_setg() to return an error instead of using an explicit exit().
> 
> Signed-off-by: David Gibson 
> ---
>  hw/ppc/spapr.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0ff09b9..fd16db4 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1107,6 +1107,7 @@ static void spapr_reset_htab(sPAPRMachineState *spapr, 
> Error **errp)
>  
>  static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
>  {
> +Error **errp = opaque;
>  bool matched = false;
>  
>  if (object_dynamic_cast(OBJECT(sbdev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> @@ -1114,9 +1115,9 @@ static int find_unknown_sysbus_device(SysBusDevice 
> *sbdev, void *opaque)
>  }
>  
>  if (!matched) {
> -error_report("Device %s is not supported by this machine yet.",
> - qdev_fw_name(DEVICE(sbdev)));
> -exit(1);
> +error_setg(errp,
> +   "Device %s is not supported by this machine yet",
> +   qdev_fw_name(DEVICE(sbdev)));
>  }
>  
>  return 0;
> @@ -1151,7 +1152,7 @@ static void ppc_spapr_reset(void)
>  uint32_t rtas_limit;
>  
>  /* Check for unknown sysbus devices */
> -foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
> +foreach_dynamic_sysbus_device(find_unknown_sysbus_device, _abort);
>  
>  /* Reset the hash table & recalc the RMA */
>  spapr_reset_htab(spapr, _abort);

Can this error be triggered by the user (by plugging certain devices or
so)? If so, I'm not sure whether error_abort is the right thing to use
here ... it's always ugly if the user can trigger an abort directly.
Maybe error_fatal would be better instead?

 Thomas

Re: [Qemu-devel] [PATCH COLO-Frame v11 25/39] COLO: implement default failover treatment

[Hailiang Zhang]

On 2015/12/11 3:01, Dr. David Alan Gilbert wrote:

* zhanghailiang (zhang.zhanghaili...@huawei.com) wrote:

If we detect some error in colo,  we will wait for some time,
hoping users also detect it. If users don't issue failover command.
We will go into default failover procedure, which the PVM will takeover
work while SVM is exit in default.


I'm not sure this is needed; especially on the SVM.  I don't see any harm
in the SVM waiting forever to be told what to do - it could be told to
failover or quit; I don't see any benefit to it automatically exiting.

In the primary, I can see if you didn't have some automated error
detection system then I can understand it (but I think it's rare);
but you really would want to make that failover delay configurable
so that you could turn it off in a system that did have failure detection;
because automatically restarting the primary after it had caused a failover
to the secondary would be very bad.


Yes, automatically restarting the PVM may cause split-brain. I'll drop
this patch temporarily.

Thanks.
Hailiang



Signed-off-by: zhanghailiang 
Signed-off-by: Li Zhijian 
---
  migration/colo.c | 46 ++
  1 file changed, 46 insertions(+)

diff --git a/migration/colo.c b/migration/colo.c
index f31e957..1e6d3dd 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -19,6 +19,14 @@
  #include "qemu/sockets.h"
  #include "migration/failover.h"

+/*
+ * The delay time before qemu begin the procedure of default failover 
treatment.
+ * Unit: ms
+ * Fix me: This value should be able to change by command
+ * 'migrate-set-parameters'
+ */
+#define DEFAULT_FAILOVER_DELAY 2000
+
  /* colo buffer */
  #define COLO_BUFFER_BASE_SIZE (4 * 1024 * 1024)

@@ -264,6 +272,7 @@ static void colo_process_checkpoint(MigrationState *s)
  {
  QEMUSizedBuffer *buffer = NULL;
  int64_t current_time, checkpoint_time = 
qemu_clock_get_ms(QEMU_CLOCK_HOST);
+int64_t error_time;
  int ret = 0;
  uint64_t value;

@@ -322,8 +331,25 @@ static void colo_process_checkpoint(MigrationState *s)
  }

  out:
+current_time = error_time = qemu_clock_get_ms(QEMU_CLOCK_HOST);
  if (ret < 0) {
  error_report("%s: %s", __func__, strerror(-ret));
+/* Give users time to get involved in this verdict */
+while (current_time - error_time <= DEFAULT_FAILOVER_DELAY) {
+if (failover_request_is_active()) {
+error_report("Primary VM will take over work");
+break;
+}
+usleep(100 * 1000);
+current_time = qemu_clock_get_ms(QEMU_CLOCK_HOST);
+}
+
+qemu_mutex_lock_iothread();
+if (!failover_request_is_active()) {
+error_report("Primary VM will take over work in default");
+failover_request_active(NULL);
+}
+qemu_mutex_unlock_iothread();
  }

  qsb_free(buffer);
@@ -384,6 +410,7 @@ void *colo_process_incoming_thread(void *opaque)
  QEMUFile *fb = NULL;
  QEMUSizedBuffer *buffer = NULL; /* Cache incoming device state */
  uint64_t  total_size;
+int64_t error_time, current_time;
  int ret = 0;
  uint64_t value;

@@ -499,9 +526,28 @@ void *colo_process_incoming_thread(void *opaque)
  }

  out:
+current_time = error_time = qemu_clock_get_ms(QEMU_CLOCK_HOST);
  if (ret < 0) {
  error_report("colo incoming thread will exit, detect error: %s",
   strerror(-ret));
+/* Give users time to get involved in this verdict */
+while (current_time - error_time <= DEFAULT_FAILOVER_DELAY) {
+if (failover_request_is_active()) {
+error_report("Secondary VM will take over work");
+break;
+}
+usleep(100 * 1000);
+current_time = qemu_clock_get_ms(QEMU_CLOCK_HOST);
+}
+/* check flag again*/
+if (!failover_request_is_active()) {
+/*
+* We assume that Primary VM is still alive according to
+* heartbeat, just kill Secondary VM
+*/
+error_report("SVM is going to exit in default!");
+exit(1);
+}
  }

  if (fb) {
--
1.8.3.1



--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

.

Re: [Qemu-devel] [PATCH v3 0/9] Introduce I/O channels framework

[Daniel P. Berrange]

Ping, unless anyone wants to review this further I'll send a pull
request for it once 2.6 opens up, so I can focus on getting the
other dependant patch series reviewed & merged in 2.6 too.

Regards,
Daniel

On Wed, Nov 18, 2015 at 06:42:43PM +, Daniel P. Berrange wrote:
> This series of patches is a followup submission for review of another
> chunk of my large series supporting TLS across chardevs, VNC and
> migration, previously shown here:
> 
>  RFC: https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00829.html
>   v1: https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg04853.html
>   v2: https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg03439.html
> 
> In this series, I have provided only the general I/O channels
> framework, which will ultimately be used by VNC, chardev and
> migration code, which currently work as follows:
> 
>  - VNC: uses raw sockets APIs directly, layering in TLS, SASL
>and websockets where needed. This has resulted in what should
>be fairly generic code for TLS and websockets being entangled
>with the rest of the VNC server code.
> 
>  - Chardevs: uses GLib's GIOChannel framework. This only provides
>implementations for sockets and files. Its API lacks support for
>using struct iovec for read/writes, file descriptor passing,
>misc sockets ioctls/fcntls. While you can work around these
>problems by accessing the underling file descriptor directly,
>this breaks the encapsulation, requiring callers to know about
>specific implementations. It also does not have integration
>with QEMU Error reporting framework. So while the GIOChannel
>is extensible, extending it would result in throwing away
>pretty much the entire of the existing implementations
> 
>  - Migration: uses QEMUFile framework. The provides an abstract
>interface for I/O channels used during migration. It has
>impls for sockets, files, memory buffers & commands. While
>it has struct iovec support for writes, it does not have
>the same for reads. It also doesn't support file descriptor
>passing or control of the sockets ioctls/fcntls. It also
>does not have any explicit event loop integration, expecting
>the callers to directly access the underling file desctriptor
>and setup events on that. This limits its utility in cases
>where you need channels which are not backed by file
>descriptors. It has no integration with QEMU Error object
>for error reporting, has fixed semantics wrt writes
>ie must always do a full write, no partial writes, and
>most strangely forces either input or output mode, but
>refuses to allow both, so no bi-directional channels!
> 
> Out of all of these, the migration code is probably closest
> to what is needed, but is still a good way off from being a
> generic framework that be can reused outside of the migration
> code.
> 
> There is also the GLib GIO library which provides a generic
> framework, but we can't depend on that due to the minimum
> GLib requirement. It also has various missing pieces such as
> file descriptor passing, and no support for struct iovec
> either.
> 
> Hence, this series was born, which tries to take the best of
> the designs for the GIOChannel, QIO and QEMUFile code to
> provide QIOChannel. Right from the start this new framework
> is explicitly isolated from any other QEMU subsystem, so its
> implementation will not get mixed up with specific use cases.
> 
> The QIOChannel abstract base class defines the overall API
> contract
> 
>  - qio_channel_{write,writev,write_full} for writing data. The
>underling impl always uses struct iovec, but non-iov
>APIs are provided as simple wrappers for convenience
> 
>  - qio_channel_{read,readv,read_full} for reading data. The
>underling impl always uses struct iovec, but non-iov
>APIs are provided as simple wrappers for convenience
> 
>  - qio_channel_has_feature to determine which optional
>features the channel supports - eg file descriptor
>passing, nagle, etc
> 
>  - qio_channel_set_{blocking,delay,cork} for various fcntl
>and ioctl controls
> 
>  - qio_channel_{close,shutdown} for closing the I/O stream
>or individual channels
> 
>  - qio_channel_seek for random access channels
> 
>  - qio_channel_{add,create}_watch for integration into the
>main loop for event notifications
> 
>  - qio_channel_wait for blocking of execution pending an
>event notification
> 
>  - qio_channel_yield for switching coroutine until an
>event notification
> 
> All the APIs support Error ** object arguments where needed.
> The object is built using QOM, in order to get reference
> counting and sub-classing with type checking. They are *not*
> user creatable/visible objects though - these are internal
> infrastructure, so we will be free to adapt APIs/objects at
> will over time.
> 
> In this series there are a variety of implementations. Some
> of them provide an actual base layer data 

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[P J P]

  Hello Jason,

+-- On Fri, 11 Dec 2015, Jason Wang wrote --+
| I think it's possible for attacker. Better wait for Dmitry's answer for
| this.

  Okay.
 
| > +/* Verify if device is active */
| > +if (s->device_active) {
| > +VMW_CFPRN("Vmxnet3 device is active");
| > +return;
| > +}
| 
| What if guest want to activate a paused device?

  There is a 'resume' operation defined below.
 
| >  case VMXNET3_CMD_QUIESCE_DEV:
| > -VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
| > -vmxnet3_deactivate_device(s);
| > +if (s->device_active & VMXNET3_DEV_ACTIVE) {
| > +VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
| > +vmxnet3_pause_device(s);
| > +} else if (s->device_active & VMXNET3_DEV_PAUSE) {
| > +VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - resume the device");
| > +vmxnet3_resume_device(s);
| > +}
| 
| Not sure this is the correct behavior. Is there a link to the spec?

  I couldn't find a spec for vmxnet3; I referred the vmxnet3 kernel driver, 
which seems to implement suspend & resume functions.

  -> 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/vmxnet3/vmxnet3_drv.c

In general, Ethernet documents talk about 'pause' frame mechanism to stop NIC 
from buffering more data, till it has space available to process more, when it 
resumes its operation.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH 1/7] pc: wire up TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE for !xen

[Stefano Stabellini]

On Wed, 9 Dec 2015, Eduardo Habkost wrote:
> On Tue, Dec 08, 2015 at 03:07:22PM +0100, Gerd Hoffmann wrote:
> > rename pc_xen_hvm_init_pci to pc_i440fx_init_pci,
> > use it for both xen and non-xen init.
> > 
> > Signed-off-by: Gerd Hoffmann 
> > ---
> >  hw/i386/pc_piix.c | 11 +--
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 2e41efe..ce6c3c5 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -419,10 +419,9 @@ static void pc_init_isa(MachineState *machine)
> >  pc_init1(machine, TYPE_I440FX_PCI_HOST_BRIDGE, TYPE_I440FX_PCI_DEVICE);
> >  }
> >  
> > -#ifdef CONFIG_XEN
> > -static void pc_xen_hvm_init_pci(MachineState *machine)
> > +static void pc_i440fx_init_pci(MachineState *machine)
> >  {
> > -const char *pci_type = has_igd_gfx_passthru ?
> > +const char *pci_type = machine->igd_gfx_passthru ?
> >  TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE : 
> > TYPE_I440FX_PCI_DEVICE;
> >  
> 
> Have you considered removing the has_igd_gfx_passthru global
> completely?

Indeed. It doesn't make much sense anymore.


> >  pc_init1(machine,
> > @@ -430,6 +429,7 @@ static void pc_xen_hvm_init_pci(MachineState *machine)
> >   pci_type);
> >  }
> >  
> > +#ifdef CONFIG_XEN
> >  static void pc_xen_hvm_init(MachineState *machine)
> >  {
> >  PCIBus *bus;
> > @@ -439,7 +439,7 @@ static void pc_xen_hvm_init(MachineState *machine)
> >  exit(1);
> >  }
> >  
> > -pc_xen_hvm_init_pci(machine);
> +pc_i440fx_init_pci(machine);
> >  
> >  bus = pci_find_primary_bus();
> >  if (bus != NULL) {
> > @@ -455,8 +455,7 @@ static void pc_xen_hvm_init(MachineState *machine)
> >  if (compat) { \
> >  compat(machine); \
> >  } \
> > -pc_init1(machine, TYPE_I440FX_PCI_HOST_BRIDGE, \
> > - TYPE_I440FX_PCI_DEVICE); \
> > +pc_i440fx_init_pci(machine); \
> 
> machine->igd_gfx_passthru defaults to false, meaning that in the
> default case the pc_init1() arguments in pc_i440fx_init_pci()
> will be the same as the call being replaced above, keeping
> exactly the same behavior.
> 
> This change breaks compatibility in the unlikely case somebody is
> already using igd-passthru=on in non-xenfv machines. I don't
> think it would make sense to keep a broken igd-passthru option in
> pc-2.5 and older for compatibility if nobody ever used that
> option, but it would be nice to mention that in the commit
> message.
> 
> Reviewed-by: Eduardo Habkost 

I agree

Reviewed-by: Stefano Stabellini 

Re: [Qemu-devel] [PATCH] Fix xbzrle vs last_sent_block update

[Juan Quintela]

"Dr. David Alan Gilbert (git)"  wrote:
> From: "Dr. David Alan Gilbert" 
>
> My fix (84e7b80a) replaced the last_sent_block update that I'd
> removed earlier; however it was too aggressive in the xbzrle case.
>
> save_xbzrle_page might return '0' to mean that the page didn't
> need sending since it was the same as the last sent version;
> in this case we can't update 'last_sent_block' since we didn't
> actually send it.
>
> Symptom: 'Illegal RAM offset 1018000' as we try and send a page
> to the wrong RAMBlock;  potentially that could be a data
> corruption if you were really unlucky.
>
> Fixes: 84e7b80a05c0c44b90533c6cd2f1db5c932ccf77
>
> Signed-off-by: Dr. David Alan Gilbert 

Reviewed-by: Juan Quintela 

Re: [Qemu-devel] [PATCH 05/11] pseries: Cleanup error handling in spapr_vga_init()

[Thomas Huth]

On 11/12/15 01:11, David Gibson wrote:
> Use error_setg() to return an error rather than an explicit exit().
> Previously it was an exit(0) instead of a non-zero exit code, which was
> simply a bug.
> 
> Also improve the error message.
> 
> Signed-off-by: David Gibson 
> ---
>  hw/ppc/spapr.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b478ca6..0ff09b9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1246,7 +1246,7 @@ static void spapr_rtc_create(sPAPRMachineState *spapr)
>  }
>  
>  /* Returns whether we want to use VGA or not */
> -static int spapr_vga_init(PCIBus *pci_bus)
> +static int spapr_vga_init(PCIBus *pci_bus, Error **errp)

While you're at it, you could also change the return type from "int" to
"bool".

>  {
>  switch (vga_interface_type) {
>  case VGA_NONE:
> @@ -1257,9 +1257,9 @@ static int spapr_vga_init(PCIBus *pci_bus)
>  case VGA_VIRTIO:
>  return pci_vga_init(pci_bus) != NULL;
>  default:
> -fprintf(stderr, "This vga model is not supported,"
> -"currently it only supports -vga std\n");
> -exit(0);
> +error_setg(errp,
> +   "Unsupported VGA mode, only -vga std or -vga virtio is 
> supported");
> +return false;
>  }
>  }
>  
> @@ -1926,7 +1926,7 @@ static void ppc_spapr_init(MachineState *machine)
>  }
>  
>  /* Graphics */
> -if (spapr_vga_init(phb->bus)) {
> +if (spapr_vga_init(phb->bus, _fatal)) {
>  spapr->has_graphics = true;
>  machine->usb |= defaults_enabled() && !machine->usb_disabled;
>  }
> 

Reviewed-by: Thomas Huth 

Re: [Qemu-devel] [PATCH 07/11] pseries: Cleanup error handling in spapr_kvm_type()

[Thomas Huth]

On 11/12/15 01:11, David Gibson wrote:
> Use error_setg() and _fatal instead of an explicit exit().
> 
> Signed-off-by: David Gibson 
> ---
>  hw/ppc/spapr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fd16db4..546d2f5 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2035,8 +2035,8 @@ static int spapr_kvm_type(const char *vm_type)
>  return 2;
>  }
>  
> -error_report("Unknown kvm-type specified '%s'", vm_type);
> -exit(1);
> +error_setg(_fatal, "Unknown kvm-type specified '%s'", vm_type);
> +return 0;
>  }

Honestly, I'd rather prefer the original code here. error_setg() should
IMHO be used to set an error in an "flexible" error variable. Using it
with an "hard-coded" error_fatal sounds ugly to me. And as far as I can
see, no other code in QEMU uses error_setg(_fatal, ...) - so we
should maybe not start with this in the spapr code as well.

If you still would like to get rid of the exit() here ... maybe you
could introduce some kind of error_report_fatal() function instead that
exits after reporting the error with error_report() ?

 Thomas

Re: [Qemu-devel] [PATCH 09/11] pseries: Clean up error handling in xics_system_init()

[Thomas Huth]

On 11/12/15 01:11, David Gibson wrote:
> Use the error handling infrastructure to pass an error out from
> try_create_xics() instead of assuming _abort - the caller is in a
> better position to decide on error handling policy.
> 
> Signed-off-by: David Gibson 
> ---
>  hw/ppc/spapr.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 546d2f5..c376748 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -111,7 +111,7 @@ static XICSState *try_create_xics(const char *type, int 
> nr_servers,
>  }
>  
>  static XICSState *xics_system_init(MachineState *machine,
> -   int nr_servers, int nr_irqs)
> +   int nr_servers, int nr_irqs, Error **errp)
>  {
>  XICSState *icp = NULL;
>  
> @@ -129,7 +129,7 @@ static XICSState *xics_system_init(MachineState *machine,
>  }
>  
>  if (!icp) {
> -icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs, _abort);
> +icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs, errp);
>  }
>  
>  return icp;
> @@ -1808,7 +1808,7 @@ static void ppc_spapr_init(MachineState *machine)
>  spapr->icp = xics_system_init(machine,
>DIV_ROUND_UP(max_cpus * 
> kvmppc_smt_threads(),
> smp_threads),
> -  XICS_IRQS);
> +  XICS_IRQS, _fatal);
>  
>  if (smc->dr_lmb_enabled) {
>  spapr_validate_node_memory(machine, _fatal);
> 

Could you maybe explain in the patch description why you changed the
behavior in case of errors from "error_abort" into "error_fatal" ?

 Thomas

Re: [Qemu-devel] [PATCH] Fix xbzrle vs last_sent_block update

[Juan Quintela]

Peter Maydell  wrote:
D> On 10 December 2015 at 16:31, Dr. David Alan Gilbert (git)
>  wrote:
>> From: "Dr. David Alan Gilbert" 
>>
>> My fix (84e7b80a) replaced the last_sent_block update that I'd
>> removed earlier; however it was too aggressive in the xbzrle case.
>>
>> save_xbzrle_page might return '0' to mean that the page didn't
>> need sending since it was the same as the last sent version;
>> in this case we can't update 'last_sent_block' since we didn't
>> actually send it.
>>
>> Symptom: 'Illegal RAM offset 1018000' as we try and send a page
>> to the wrong RAMBlock;  potentially that could be a data
>> corruption if you were really unlucky.
>>
>> Fixes: 84e7b80a05c0c44b90533c6cd2f1db5c932ccf77
>>
>> Signed-off-by: Dr. David Alan Gilbert 
>> ---
>>  migration/ram.c | 11 ++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 1eb155a..0490f00 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -716,6 +716,9 @@ static int save_zero_page(QEMUFile *f, RAMBlock *block, 
>> ram_addr_t offset,
>>   * ram_save_page: Send the given page to the stream
>>   *
>>   * Returns: Number of pages written.
>> + *  < 0 - error
>> + *  >=0 - Number of pages written - this might legally be 0
>> + *if xbzrle noticed the page was the same.
>>   *
>>   * @f: QEMUFile where to send the data
>>   * @block: block that contains the page we want to send
>> @@ -1249,7 +1252,13 @@ static int ram_save_target_page(MigrationState *ms, 
>> QEMUFile *f,
>>  if (unsentmap) {
>>  clear_bit(dirty_ram_abs >> TARGET_PAGE_BITS, unsentmap);
>>  }
>> -last_sent_block = block;
>> +/* Only update last_sent_block if a block was actually sent; xbzrle
>> + * might have decided the page was identical so didn't bother 
>> writing
>> + * to the stream.
>> + */
>> +if (res > 0) {
>> +last_sent_block = block;
>> +}
>>  }
>>
>>  return res;
>
> This sounds like we should probably put this into 2.5; I'm happy
> to do so if it gets review by tomorrow afternoon and Juan/Amit
> agree.

Yeap, did the review by.  Do you want a pull request, or just pick it
directly?

Re: [Qemu-devel] [PATCH 02/18] slirp: Generalizing and neutralizing code before adding IPv6 stuff

[Samuel Thibault]

Thomas Huth, on Fri 11 Dec 2015 14:38:44 +0100, wrote:
> > +switch (iph->ip_v) {
> > +case IPVERSION:
> >  if (iph->ip_dst.s_addr == 0) {
> >  /* 0.0.0.0 can not be a destination address, something went wrong,
> >   * avoid making it worse */
> 
> That indentation looks now broken - shouldn't the if-statement now be
> indented with four more spaces now?

Please see the patch summary: I was asked to provided a non-reindented
patch and then a reindent patch.

> Also, I'd prefer if you could split this patch into two: One for
> renaming the "arp_requested" field, and one for adding the additional
> logic with the switch statement (since there are two different kind of
> changes).

TBH I'm starting to again lose motivation.

Samuel

Re: [Qemu-devel] [PATCH 08/11] qjson: Simplify by using json-output-visitor

[Paolo Bonzini]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256



On 11/12/2015 14:42, Eric Blake wrote:
>>> compiles and is pretty much a 1:1 translation from the qjson.c
>>> API to the visitor API (using this patch as a guide).  Feel
>>> free to include it and remove qjson.c.  Alternatively, you can
>>> leave out this patch and I'll test and submit the transition.
> Should I squash the two together, or keep my current patch and
> drop qjson.c as a separate patch?

As you prefer.

> Also, while reading this, I noticed that qjson.h has signatures
> that match JSON ('name' comes before 'value', when generating a
> "name":value pair), while visitor.h has signatures that are
> backwards ('obj' comes before 'name', even though the output will
> be "name":value-of-obj).  I'm seriously debating about a tree-wide
> coccinelle patch to reverse the order or arguments of all the
> visit_type_* functions.

That would be doable, there aren't many users outside generated code.

Paolo
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iQEcBAEBCAAGBQJWatN5AAoJEL/70l94x66D8PQIAKEzIEUqjfhDMLhov4NwxGs3
LvyW+/w5AyDEZOJMayBuc3y7fSTbLp/eMQPsfBOIvBCMr/aEE4Tij0CzW3JE4FnH
IFCW94F8Q7vq8jTYKFU6222Nk7vUGOLG9y3jQSnLX4RzpRizhWjk73DtkED7YVu3
5J0Ef3+ZEobCZA9zOwQT7+pqc9ah/M6tGsZodvML2FopCk/SsTBauPjoSeP0knHE
1MJsu/KVjn1508yn1SY9d0HJKQx2dM3nzQRtp0xbWx7x8o7u826axpVQieq7YBpb
PaRW/vqiXvdnRkrCRHZAL6PJ4GLyquX42E7HjqCQjibA06fbtqmLoYQA2A/2Pyg=
=EJIn
-END PGP SIGNATURE-

Re: [Qemu-devel] [PATCH 02/18] slirp: Generalizing and neutralizing code before adding IPv6 stuff

[Samuel Thibault]

Thomas Huth, on Fri 11 Dec 2015 14:43:42 +0100, wrote:
> Anyway, it's IMHO a somewhat strange way to structure a patch ... maybe
> it would be nicer to do it in one go instead (after splitting off the
> arp_requested renaming)?

Please discuss about it with the people who asked for it.  I won't
revamp patches until what people prefer is settled.

Personally I prefer the two-patch approach, since the first nicely shows
what behavior change we have, and the second is a no-op patch, as can be
seen with diff -w

Samuel

Re: [Qemu-devel] [PATCH 01/11] ppc: Cleanup error handling in ppc_set_compat()

[Eric Blake]

On 12/10/2015 05:11 PM, David Gibson wrote:
> Current ppc_set_compat() returns -1 for errors, and also (unconditionally)
> reports an error message.  The caller in h_client_architecture_support()
> may then report it again using an outdated fprintf().
> 
> Clean this up by using the modern error reporting mechanisms.
> 
> Signed-off-by: David Gibson 
> ---

> @@ -9208,12 +9208,13 @@ int ppc_set_compat(PowerPCCPU *cpu, uint32_t 
> cpu_version)
>  break;
>  }
>  
> -if (kvm_enabled() && kvmppc_set_compat(cpu, cpu->cpu_version) < 0) {
> -error_report("Unable to set compatibility mode in KVM");
> -ret = -1;
> +if (kvm_enabled()) {
> +ret = kvmppc_set_compat(cpu, cpu->cpu_version);
> +if (ret < 0) {
> +error_setg(errp, "Unable to set CPU compatibility mode in KVM: 
> %s",
> +   strerror(-ret));
> +}

Could use error_setg_errno() here instead of manually calling strerror().

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] How does TCG gen host code for a TB?

[Valerón JC]

I want to trace a bug in tcg, which for me, at some point, generate infinite 
loop TB chains, that's unexpected. and I've found the final TB(head, since 
they're chaining) which run in an infinite loop, and I know a very weird trick 
to 'disable' this bug, so I would like to track the tcg-ops for the TB, hope 
that I can figure what's wrong.

but when I read the tcg_gen_code(), I'm confused...

how does tcg_gen_code() generate codes for one TB? if I read the code 
correctly, gen_intermediate_code() will not flush the tcg_ctx->gen_opc_buf[], 
codes for previous TB are mixed together... and tcg_gen_code() will start gen 
from index 0 of gen_opc_buf, how does it generate codes for the TB just created?

even though there's label for a TB-start(I'm not familiar with this label stuff 
yet), but the fact that the gen_opc_buf not flush every time a new TB is 
generated seems weird to me, won't the new tcg-ops mixed with the ones of 
previous TB? Isn’t the newly generated host-code dedicate to one TB?

Sent from Mail for Windows 10

[Qemu-devel] [RFC PATCH 1/3] linux-headers: Update VFIO headers from linux-next tag ToBeFilled

[Yongji Xie]

Syncup VFIO related linux headers from linux-next tree using
scripts/update-linux-headers.sh.

Integrate two VFIO-PCI ioctl flags:
- VFIO_DEVICE_FLAGS_PCI_PAGE_ALIGNED
- VFIO_DEVICE_FLAGS_PCI_MSIX_MMAP

Signed-off-by: Yongji Xie 
---
 linux-headers/linux/vfio.h |4 
 1 file changed, 4 insertions(+)

diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index aa276bc..3b79d63 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -164,6 +164,10 @@ struct vfio_device_info {
 #define VFIO_DEVICE_FLAGS_PCI  (1 << 1)/* vfio-pci device */
 #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)/* vfio-platform device */
 #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)   /* vfio-amba device */
+/* Platform support all PCI MMIO BARs to be page aligned */
+#define VFIO_DEVICE_FLAGS_PCI_PAGE_ALIGNED  (1 << 4)
+/* platform support mmapping PCI MSI-X vector table */
+#define VFIO_DEVICE_FLAGS_PCI_MSIX_MMAP  (1 << 5)
__u32   num_regions;/* Max region index + 1 */
__u32   num_irqs;   /* Max IRQ index + 1 */
 };
-- 
1.7.9.5

[Qemu-devel] [Qemu-trivial][PATCH] trace: reflect the file name change

[Qinghua Jin]

Some functions was moved from block.c to block/io.c, so the trace-events file 
should reflect that change.


Signed-off-by: Qinghua Jin 
---
 trace-events | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


diff --git a/trace-events b/trace-events
index 2fce98e..bec1aa1 100644
--- a/trace-events
+++ b/trace-events
@@ -59,6 +59,9 @@ virtio_console_chr_event(unsigned int port, int event) "port 
%u, event %d"
 
 # block.c
 bdrv_open_common(void *bs, const char *filename, int flags, const char 
*format_name) "bs %p filename \"%s\" flags %#x format_name \"%s\""
+bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d"
+
+# block/io.c
 multiwrite_cb(void *mcb, int ret) "mcb %p ret %d"
 bdrv_aio_multiwrite(void *mcb, int num_callbacks, int num_reqs) "mcb %p 
num_callbacks %d num_reqs %d"
 bdrv_aio_discard(void *bs, int64_t sector_num, int nb_sectors, void *opaque) 
"bs %p sector_num %"PRId64" nb_sectors %d opaque %p"
@@ -66,7 +69,6 @@ bdrv_aio_flush(void *bs, void *opaque) "bs %p opaque %p"
 bdrv_aio_readv(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs 
%p sector_num %"PRId64" nb_sectors %d opaque %p"
 bdrv_aio_writev(void *bs, int64_t sector_num, int nb_sectors, void *opaque) 
"bs %p sector_num %"PRId64" nb_sectors %d opaque %p"
 bdrv_aio_write_zeroes(void *bs, int64_t sector_num, int nb_sectors, int flags, 
void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d flags %#x opaque %p"
-bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d"
 bdrv_co_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num 
%"PRId64" nb_sectors %d"
 bdrv_co_copy_on_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p 
sector_num %"PRId64" nb_sectors %d"
 bdrv_co_readv_no_serialising(void *bs, int64_t sector_num, int nb_sector) "bs 
%p sector_num %"PRId64" nb_sectors %d"
-- 
2.4.3

[Qemu-devel] [PATCH V2] qemu-img / curl: When fetching Content-Size use GET instead of HEAD.

[Boris Schrijver]

A server can respond different to both methods, or can block one of the two.

Signed-off-by: Boris Schrijver 
Reviewed-by: John Snow 
---
V2: Impovements over V1:
- Don't check for CURLE_WRITE_ERROR, on success CURLE_OK will be returned.
- Use CURLOPT_CUSTOMREQUEST instead of CURLOPT_HTTPGET.
---
 block/curl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/curl.c b/block/curl.c
index 8994182..1677d0c 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -598,6 +598,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, 
int flags,
 curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION,
  curl_header_cb);
 curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s);
+curl_easy_setopt(state->curl, CURLOPT_CUSTOMREQUEST, "GET");
 if (curl_easy_perform(state->curl))
 goto out;
 curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, );
-- 
2.1.4

Re: [Qemu-devel] [PATCH 02/11] pseries: Cleanup error handling of spapr_cpu_init()

[Eric Blake]

On 12/10/2015 05:11 PM, David Gibson wrote:
> Currently spapr_cpu_init() is hardcoded to handle any errors as fatal.
> That works for now, since it's only called from initial setup where an
> error here means we really can't proceed.
> 
> However, we'll want to handle this more flexibly for cpu hotplug in future
> so generalize this using the error reporting infrastructure.  While we're
> at it make a small cleanup in a related part of ppc_spapr_init() to use
> the error infrastructure instead of an old-style explicit fprintf / exit.
> 
> Signed-off-by: David Gibson 
> ---
>  hw/ppc/spapr.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)

> @@ -1633,7 +1634,7 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, 
> PowerPCCPU *cpu)
>  }
>  
>  if (cpu->max_compat) {
> -ppc_set_compat(cpu, cpu->max_compat, _fatal);
> +ppc_set_compat(cpu, cpu->max_compat, errp);
>  }
>  
>  xics_cpu_setup(spapr->icp, cpu);

Pre-patch: you can't reach the xics_cpu_setup() call on error.

Post-patch: depending on what the caller passed in, you can fall through
to xics_cpu_setup() with a potentially incomplete cpu.

I think a more robust solution is probably along the lines of:

Error *err = NULL;
if (cpu->max_compat) {
ppc_set_compat(cpu, cpu->max_compat, );
if (err) {
error_propagate(errp, err);
return;
}
}
xics_cpu_setup(spapr_icp, cpu);

> @@ -1802,10 +1803,9 @@ static void ppc_spapr_init(MachineState *machine)
>  for (i = 0; i < smp_cpus; i++) {
>  cpu = cpu_ppc_init(machine->cpu_model);
>  if (cpu == NULL) {
> -fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> -exit(1);
> +error_setg(_fatal, "Unable to find PowerPC CPU 
> definition");
>  }
> -spapr_cpu_init(spapr, cpu);
> +spapr_cpu_init(spapr, cpu, _fatal);

Of course, in _this_ patch, since the (only) caller is passing
_fatal, you don't hit the situation of fallthrough to
xics_cpu_setup().

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] Fix xbzrle vs last_sent_block update

[Peter Maydell]

On 11 December 2015 at 12:52, Peter Maydell  wrote:
> On 11 December 2015 at 12:25, Juan Quintela  wrote:
>> Peter Maydell  wrote:
>>> This sounds like we should probably put this into 2.5; I'm happy
>>> to do so if it gets review by tomorrow afternoon and Juan/Amit
>>> agree.
>>
>> Yeap, did the review by.  Do you want a pull request, or just pick it
>> directly?
>
> I'll just apply it directly.

Now applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH 08/11] qjson: Simplify by using json-output-visitor

[Eric Blake]

On 12/11/2015 04:10 AM, Paolo Bonzini wrote:
> 
> 
> On 11/12/2015 00:53, Eric Blake wrote:
>> Instead of rolling our own limited JSON outputter, we can just
>> wrap the more full-featured JSON output Visitor.
>>
>> This slightly changes the output (different spacing), but the
>> result is still equivalent JSON contents.
>>
>> Signed-off-by: Eric Blake 
>> ---
>>  qjson.c | 61 ++---
>>  1 file changed, 22 insertions(+), 39 deletions(-)
> 
> I didn't find out which tree your patches apply to, but this:

Mentioned in the cover letter:

+ v8 or later of my qapi subset E patches (at the time of this mail,
only v7 has been posted):
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg01181.html

Also available as a tag at this location (with prerequisites applied):
git fetch git://repo.or.cz/qemu/ericb.git qapi-jsonv1

(note that it is only a tag, not a branch, and you are right that it
doesn't yet apply to any maintainer's tree).


> @@ -1068,9 +1070,11 @@ void qemu_savevm_state_complete_precopy(QEMUFile *f, 
> bool iterable_only)
>  return;
>  }
>  
> -vmdesc = qjson_new();
> -json_prop_int(vmdesc, "page_size", TARGET_PAGE_SIZE);
> -json_start_array(vmdesc, "devices");
> +vmdesc_jov = json_output_visitor_new();
> +vmdesc = json_output_get_visitor(vmdesc_jov);
> +visit_start_struct(vmdesc, NULL, 0, NULL, _abort);
> +visit_type_int(vmdesc, TARGET_PAGE_SIZE, "page_size", _abort);
> +visit_start_list(vmdesc, NULL, 0, "devices", _abort);
>  QTAILQ_FOREACH(se, _state.handlers, entry) {

Nice idea.

> compiles and is pretty much a 1:1 translation from the qjson.c API to the
> visitor API (using this patch as a guide).  Feel free to include it and
> remove qjson.c.  Alternatively, you can leave out this patch and I'll test
> and submit the transition.

Should I squash the two together, or keep my current patch and drop
qjson.c as a separate patch?

Also, while reading this, I noticed that qjson.h has signatures that
match JSON ('name' comes before 'value', when generating a "name":value
pair), while visitor.h has signatures that are backwards ('obj' comes
before 'name', even though the output will be "name":value-of-obj).  I'm
seriously debating about a tree-wide coccinelle patch to reverse the
order or arguments of all the visit_type_* functions.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [Qemu-block] [PATCH V2] qemu-img / curl: When fetching Content-Size use GET instead of HEAD.

[Eric Blake]

[adding qemu-devel; per http://wiki.qemu.org/Contribute/SubmitAPatch,
ALL patches must include qemu-devel, even if they are also copying other
sublists]

On 12/11/2015 01:40 AM, Boris Schrijver wrote:
> A server can respond different to both methods, or can block one of the two.
> 
> Signed-off-by: Boris Schrijver 
> Reviewed-by: John Snow 
> ---
> V2: Impovements over V1:
> - Don't check for CURLE_WRITE_ERROR, on success CURLE_OK will be returned.
> - Use CURLOPT_CUSTOMREQUEST instead of CURLOPT_HTTPGET.
> ---
>  block/curl.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/curl.c b/block/curl.c
> index 8994182..1677d0c 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -598,6 +598,7 @@ static int curl_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION,
>   curl_header_cb);
>  curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s);
> +curl_easy_setopt(state->curl, CURLOPT_CUSTOMREQUEST, "GET");
>  if (curl_easy_perform(state->curl))
>  goto out;
>  curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, );
> 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 02/18] slirp: Generalizing and neutralizing code before adding IPv6 stuff

[Thomas Huth]

On 11/12/15 15:01, Samuel Thibault wrote:
> Just to explain.
> 
> I'm fine with revamping the patches, provided that we eventually
> converge somewhere.
> 
> What I'm afraid of is that splitting patches in yet more many pieces,
> revamping the code yet more (moving code into their own functions, etc.)
> will only make the patch series look even bigger and even less prone to
> be included for lack of reviewer time.
> 
> I agree that the presentation can be looked after. But if that makes the
> patch series much bigger, I'd really prefer to look after that once the
> content gets commited, otherwise I don't see how we'll ever manage to
> get this commited in the coming decade.

Ok, point taken, and I understand your frustration about reworking and
posting this a couple of times without getting the patches accepted.

So maybe it's better to do smaller steps instead: Would it for example
make sense to split the whole series into two parts - first a series
that does all the preparation and cleanup patches. And then once that
has been reviewed and merged, send the second series that adds the real
new IPv6 code.

In the end, it's up to the subsystem mainter how the patches should be
done - so, Jan, what do you think? What way would you recommend to get
the patches ready for being included?

 Thomas

Re: [Qemu-devel] [PATCH] Fix xbzrle vs last_sent_block update

[Peter Maydell]

On 11 December 2015 at 12:25, Juan Quintela  wrote:
> Peter Maydell  wrote:
>> This sounds like we should probably put this into 2.5; I'm happy
>> to do so if it gets review by tomorrow afternoon and Juan/Amit
>> agree.
>
> Yeap, did the review by.  Do you want a pull request, or just pick it
> directly?

I'll just apply it directly.

thanks
-- PMM

Re: [Qemu-devel] How does TCG gen host code for a TB?

[Peter Maydell]

On 11 December 2015 at 03:43, Valerón JC  wrote:
> I want to trace a bug in tcg, which for me, at some point, generate infinite
> loop TB chains, that's unexpected. and I've found the final TB(head, since
> they're chaining) which run in an infinite loop, and I know a very weird
> trick to 'disable' this bug, so I would like to track the tcg-ops for the
> TB, hope that I can figure what's wrong.

If the guest code is an infinite loop then we will generate
a chain of TBs which goes round in a loop too. (Execution will
escape from the loop via longjmp when there is a guest interrupt
or other exception.)

> but when I read the tcg_gen_code(), I'm confused...

> how does tcg_gen_code() generate codes for one TB? if I read the code
> correctly, gen_intermediate_code() will not flush the
> tcg_ctx->gen_opc_buf[], codes for previous TB are mixed together... and
> tcg_gen_code() will start gen from index 0 of gen_opc_buf, how does it
> generate codes for the TB just created?

This code has changed, and tcg_ctx->gen_opc_buf doesn't exist any more.
We store ops in a linked list now rather than an array.
The answer to your question in general is still the same, though:
before calling gen_intermediate_code() we call tcg_func_start(),
which resets the TCGContext to a clean state, including "no temporaries
allocated", "no labels" and "no ops".

thanks
-- PMM

Re: [Qemu-devel] [PATCH 02/18] slirp: Generalizing and neutralizing code before adding IPv6 stuff

[Thomas Huth]

On 11/12/15 14:38, Thomas Huth wrote:
> On 11/12/15 01:15, Samuel Thibault wrote:
>> From: Guillaume Subiron 
>>
>> Basically, this patch replaces "arp" by "resolution" every time "arp"
>> means "mac resolution" and not specifically ARP.
>>
>> Some indentation problems are solved in functions that will be modified
>> in the next patches (ip_input…).
>>
>> In if_encap, a switch is added to prepare for the IPv6 case. Some code
>> is factorized.
>>
>> Signed-off-by: Guillaume Subiron 
>> Signed-off-by: Samuel Thibault 
>> ---
>>  slirp/mbuf.c  |  2 +-
>>  slirp/mbuf.h  |  2 +-
>>  slirp/slirp.c | 24 
>>  3 files changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/slirp/mbuf.c b/slirp/mbuf.c
>> index 795fc29..bc942b6 100644
>> --- a/slirp/mbuf.c
>> +++ b/slirp/mbuf.c
>> @@ -91,7 +91,7 @@ m_get(Slirp *slirp)
>>  m->m_len = 0;
>>  m->m_nextpkt = NULL;
>>  m->m_prevpkt = NULL;
>> -m->arp_requested = false;
>> +m->resolution_requested = false;
>>  m->expiration_date = (uint64_t)-1;
>>  end_error:
>>  DEBUG_ARG("m = %p", m);
>> diff --git a/slirp/mbuf.h b/slirp/mbuf.h
>> index b144f1c..38fedf4 100644
>> --- a/slirp/mbuf.h
>> +++ b/slirp/mbuf.h
>> @@ -79,7 +79,7 @@ struct mbuf {
>>  int m_len;  /* Amount of data in this mbuf */
>>  
>>  Slirp *slirp;
>> -boolarp_requested;
>> +boolresolution_requested;
>>  uint64_t expiration_date;
>>  /* start of dynamic buffer area, must be last element */
>>  union {
>> diff --git a/slirp/slirp.c b/slirp/slirp.c
>> index 35f819a..380ddc3 100644
>> --- a/slirp/slirp.c
>> +++ b/slirp/slirp.c
>> @@ -776,6 +776,8 @@ int if_encap(Slirp *slirp, struct mbuf *ifm)
>>  return 1;
>>  }
>>  
>> +switch (iph->ip_v) {
>> +case IPVERSION:
>>  if (iph->ip_dst.s_addr == 0) {
>>  /* 0.0.0.0 can not be a destination address, something went wrong,
>>   * avoid making it worse */
> 
> That indentation looks now broken - shouldn't the if-statement now be
> indented with four more spaces now?
> 
>> @@ -786,7 +788,7 @@ int if_encap(Slirp *slirp, struct mbuf *ifm)
>>  struct ethhdr *reh = (struct ethhdr *)arp_req;
>>  struct arphdr *rah = (struct arphdr *)(arp_req + ETH_HLEN);
>>  
>> -if (!ifm->arp_requested) {
>> +if (!ifm->resolution_requested) {
>>  /* If the client addr is not known, send an ARP request */
>>  memset(reh->h_dest, 0xff, ETH_ALEN);
>>  memcpy(reh->h_source, special_ethaddr, ETH_ALEN - 4);
>> @@ -812,22 +814,36 @@ int if_encap(Slirp *slirp, struct mbuf *ifm)
>>  rah->ar_tip = iph->ip_dst.s_addr;
>>  slirp->client_ipaddr = iph->ip_dst;
>>  slirp_output(slirp->opaque, arp_req, sizeof(arp_req));
>> -ifm->arp_requested = true;
>> +ifm->resolution_requested = true;
>>  
>>  /* Expire request and drop outgoing packet after 1 second */
>>  ifm->expiration_date = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + 
>> 10ULL;
>>  }
>>  return 0;
>>  } else {
>> -memcpy(eh->h_dest, ethaddr, ETH_ALEN);
>>  memcpy(eh->h_source, special_ethaddr, ETH_ALEN - 4);
>>  /* XXX: not correct */
>>  memcpy(>h_source[2], >vhost_addr, 4);
>>  eh->h_proto = htons(ETH_P_IP);
>> +break;
>> +}
>> +
>> +default:
>> +/* Do not assert while we don't manage IP6VERSION */
>> +/* assert(0); */
>> +break;
>> +}
>> +
>> +memcpy(eh->h_dest, ethaddr, ETH_ALEN);
>> +DEBUG_ARGS((dfd, " src = %02x:%02x:%02x:%02x:%02x:%02x\n",
>> +eh->h_source[0], eh->h_source[1], eh->h_source[2],
>> +eh->h_source[3], eh->h_source[4], eh->h_source[5]));
>> +DEBUG_ARGS((dfd, " dst = %02x:%02x:%02x:%02x:%02x:%02x\n",
>> +eh->h_dest[0], eh->h_dest[1], eh->h_dest[2],
>> +eh->h_dest[3], eh->h_dest[4], eh->h_dest[5]));
>>  memcpy(buf + sizeof(struct ethhdr), ifm->m_data, ifm->m_len);
>>  slirp_output(slirp->opaque, buf, ifm->m_len + ETH_HLEN);
>>  return 1;
>> -}
>>  }
> 
> The lines after the switch statement should also only be indented with 4
> spaces, not with 8.

Ah, well, never mind, I did not see the comment in the patch description
that you fix it up in the next patch.

Anyway, it's IMHO a somewhat strange way to structure a patch ... maybe
it would be nicer to do it in one go instead (after splitting off the
arp_requested renaming)?

 Thomas

Re: [Qemu-devel] [PATCH 02/18] slirp: Generalizing and neutralizing code before adding IPv6 stuff

[Samuel Thibault]

Just to explain.

I'm fine with revamping the patches, provided that we eventually
converge somewhere.

What I'm afraid of is that splitting patches in yet more many pieces,
revamping the code yet more (moving code into their own functions, etc.)
will only make the patch series look even bigger and even less prone to
be included for lack of reviewer time.

I agree that the presentation can be looked after. But if that makes the
patch series much bigger, I'd really prefer to look after that once the
content gets commited, otherwise I don't see how we'll ever manage to
get this commited in the coming decade.

Samuel

Re: [Qemu-devel] How does TCG gen host code for a TB?

[Sergey Fedorov]

On 11.12.2015 06:43, Valerón JC wrote:
> I want to trace a bug in tcg, which for me, at some point, generate
> infinite loop TB chains, that's unexpected. and I've found the final
> TB(head, since they're chaining) which run in an infinite loop, and I
> know a very weird trick to 'disable' this bug, so I would like to
> track the tcg-ops for the TB, hope that I can figure what's wrong.

Hi,

You may find it useful to enable logging of TCG ops generated with '-d
op', as well as input and output assembly with '-d in_asm,out_asm'. '-D
' could be useful to output log to a file. Here are references
to documentation: http://qemu.weilnetz.de/qemu-doc.html#index-_002dd and
http://qemu.weilnetz.de/qemu-doc.html#index-_002dD.

Best,
Sergey

Re: [Qemu-devel] [PATCH 04/18] slirp: Make Socket structure IPv6 compatible

[Thomas Huth]

On 11/12/15 01:15, Samuel Thibault wrote:
> From: Guillaume Subiron 
> 
> This patch replaces foreign and local address/port couples in Socket
> structure by 2 sockaddr_storage which can be casted in sockaddr_in.
> Direct access to address and port is still possible thanks to some
> \#define, so retrocompatibility of the existing code is assured.
> 
> The ss_family field of sockaddr_storage is declared after each socket
> creation.
> 
> The whole structure is also saved/restored when a Qemu session is
> saved/restored.
> 
> Signed-off-by: Guillaume Subiron 
> Signed-off-by: Samuel Thibault 
> ---
[...]
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 66c4196..0937ee0 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -1028,10 +1028,26 @@ static void slirp_sbuf_save(QEMUFile *f, struct sbuf 
> *sbuf)
>  static void slirp_socket_save(QEMUFile *f, struct socket *so)
>  {
>  qemu_put_be32(f, so->so_urgc);
> -qemu_put_be32(f, so->so_faddr.s_addr);
> -qemu_put_be32(f, so->so_laddr.s_addr);
> -qemu_put_be16(f, so->so_fport);
> -qemu_put_be16(f, so->so_lport);
> +qemu_put_be16(f, so->so_ffamily);
> +switch (so->so_ffamily) {
> +case AF_INET:
> +qemu_put_be32(f, so->so_faddr.s_addr);
> +qemu_put_be16(f, so->so_fport);
> +break;
> +default:
> +fprintf(stderr,
> +"so_ffamily unknown, unable to save so_faddr and 
> so_fport\n");
> +}
> +qemu_put_be16(f, so->so_lfamily);
> +switch (so->so_lfamily) {
> +case AF_INET:
> +qemu_put_be32(f, so->so_laddr.s_addr);
> +qemu_put_be16(f, so->so_lport);
> +break;
> +default:
> +fprintf(stderr,
> +"so_ffamily unknown, unable to save so_laddr and 
> so_lport\n");
> +}
>  qemu_put_byte(f, so->so_iptos);
>  qemu_put_byte(f, so->so_emu);
>  qemu_put_byte(f, so->so_type);
> @@ -1151,10 +1167,26 @@ static int slirp_socket_load(QEMUFile *f, struct 
> socket *so)
>  return -ENOMEM;
>  
>  so->so_urgc = qemu_get_be32(f);
> -so->so_faddr.s_addr = qemu_get_be32(f);
> -so->so_laddr.s_addr = qemu_get_be32(f);
> -so->so_fport = qemu_get_be16(f);
> -so->so_lport = qemu_get_be16(f);
> +so->so_ffamily = qemu_get_be16(f);
> +switch (so->so_ffamily) {
> +case AF_INET:
> +so->so_faddr.s_addr = qemu_get_be32(f);
> +so->so_fport = qemu_get_be16(f);
> +break;
> +default:
> +fprintf(stderr,
> +"so_ffamily unknown, unable to restore so_faddr and 
> so_lport\n");
> +}
> +so->so_lfamily = qemu_get_be16(f);
> +switch (so->so_lfamily) {
> +case AF_INET:
> +so->so_laddr.s_addr = qemu_get_be32(f);
> +so->so_lport = qemu_get_be16(f);
> +break;
> +default:
> +fprintf(stderr,
> +"so_ffamily unknown, unable to restore so_laddr and 
> so_lport\n");
> +}

Since you're changing the layout of the save data here, I think you have
to bump the version_id number in the register_savevm() call in
slirp_init() to make it clear that the new layout is incompatible.

>  so->so_iptos = qemu_get_byte(f);
>  so->so_emu = qemu_get_byte(f);
>  so->so_type = qemu_get_byte(f);
> diff --git a/slirp/socket.c b/slirp/socket.c
> index 1673e3a..bf603c9 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
[...]

Remaining parts of the patch look fine to me.

 Thomas

Re: [Qemu-devel] [PATCH 02/18] slirp: Generalizing and neutralizing code before adding IPv6 stuff

[Samuel Thibault]

Thomas Huth, on Fri 11 Dec 2015 15:32:48 +0100, wrote:
> So maybe it's better to do smaller steps instead: Would it for example
> make sense to split the whole series into two parts - first a series
> that does all the preparation and cleanup patches. And then once that
> has been reviewed and merged, send the second series that adds the real
> new IPv6 code.

Ok, that's what we already have: patches 1-9 are refactoring and
support, and 10-18 are ipv6 code.

Samuel

[Qemu-devel] [RFC PATCH 0/3] vfio/pci: Add support for mmapping sub-page MMIO BARs and MSI-X table

[Yongji Xie]

This patch set adds support for two VFIO-PCI ioctl flags:
VFIO_DEVICE_FLAGS_PCI_PAGE_ALIGNED and VFIO_DEVICE_FLAGS_PCI_MSIX_MMAP.
Note that the kernel bits of this two flags are not in upstream and
posted as .

VFIO_DEVICE_FLAGS_PCI_PAGE_ALIGNED indicates that platform support
all PCI MMIO BARs to be page aligned which means any BARs' mmio
page would not be shared with other BARs. So it's allowed to mmap
sub-page(size < PAGE_SIZE) MMIO BARs in QEMU.

VFIO_DEVICE_FLAGS_PCI_MSIX_MMAP indicates that it's safe to mmap MSI-X
table in QEMU.

Besides supporting for these two mmapping cases, we also fix some issues
which would block KVM to create memory slot for these mmapped sub-page
MMIO BARs and mmapped MSI-X table.

Posted kernel patches link:
- https://www.mail-archive.com/kvm@vger.kernel.org/msg124031.html

Yongji Xie (3):
  linux-headers: Update VFIO headers from linux-next tag ToBeFilled
  vfio/pci: Add support for mmapped sub-page MMIO BARs
  vfio/pci: Add support for mmapping MSI-X table

 hw/vfio/pci.c  |   65 
 linux-headers/linux/vfio.h |4 +++
 2 files changed, 64 insertions(+), 5 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH] Keep pty slave file descriptor open until the master is closed

[Ashley Jonathan]

I have experienced a minor difficulty using QEMU with the "-serial pty" option:

If a process opens the slave pts device, writes data to it, then immediately 
closes it, the data doesn't reliably get delivered to the emulated serial port. 
This seems to be because a read of the master pty device returns EIO on Linux 
if no process has the pts device open, even when data is waiting "in the pipe".

A fix seems to be for QEMU to keep the pts file descriptor open until the pty 
is closed, as per the below patch.

---
 qemu-char.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qemu-char.c b/qemu-char.c
index 2969c44..ed03ba0 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1198,6 +1198,7 @@ typedef struct {
 int connected;
 guint timer_tag;
 guint open_tag;
+int slave_fd;
 } PtyCharDriver;
 
 static void pty_chr_update_read_handler_locked(CharDriverState *chr);
@@ -1373,6 +1374,7 @@ static void pty_chr_close(struct CharDriverState *chr)
 
 qemu_mutex_lock(>chr_write_lock);
 pty_chr_state(chr, 0);
+close(s->slave_fd);
 fd = g_io_channel_unix_get_fd(s->fd);
 g_io_channel_unref(s->fd);
 close(fd);
@@ -1401,7 +1403,6 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
 return NULL;
 }
 
-close(slave_fd);
 qemu_set_nonblock(master_fd);
 
 chr = qemu_chr_alloc();
@@ -1422,6 +1423,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
 chr->explicit_be_open = true;
 
 s->fd = io_channel_from_fd(master_fd);
+s->slave_fd = slave_fd;
 s->timer_tag = 0;
 
 return chr;
-- 
2.1.4



---
Jonathan Ashley 
jonathan.ashley AT altran.com

[Qemu-devel] [RFC PATCH 2/3] vfio/pci: Add support for mmapped sub-page MMIO BARs

[Yongji Xie]

The VFIO-PCI ioctl flag VFIO_DEVICE_FLAGS_PCI_PAGE_ALIGNED indicates platform
support all PCI MMIO BARs to be page aligned and sub-page(size < PAGE_SIZE)
MMIO BARs can be mmapped.

But this has an issue for these mmapped sub-page MMIO BARs - KVM would not
allow to create memory slot for them via ioctl KVM_SET_USER_MEMORY_REGION
because these BARs' size are still less than PAGE_SIZE.

So this patch expands these sub-page MMIO BARs to page size in
vfio_pci_write_config(), so that the BARs could be passed to KVM
ioctl KVM_SET_USER_MEMORY_REGION with a valid size. And we also set the
priority of these BARs' memory regions to zero in case of overlap with
page unaligned BARs when guest doesn't support all PCI MMIO BARs to be
page aligned.

Signed-off-by: Yongji Xie 
---
 hw/vfio/pci.c |   46 ++
 1 file changed, 46 insertions(+)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 1fb868c..a7c6171 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1028,6 +1028,44 @@ static const MemoryRegionOps vfio_vga_ops = {
 };
 
 /*
+ * Expand sub-page(size < PAGE_SIZE) MMIO BARs to page size if host support
+ * all MMIO BARs to be page aligned. And we should set the priority of these
+ * BARs' memory regions to zero in case of overlap with page unaligned
+ * BARs when guest doesn't support all MMIO BARs to be page aligned.
+ */
+static void vfio_expand_bar_to_page_size(PCIDevice *pdev)
+{
+VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
+MemoryRegion *mmap_mr;
+MemoryRegion *mr;
+PCIIORegion *r;
+pcibus_t bar_addr;
+int i;
+
+memory_region_transaction_begin();
+for (i = 0; i < PCI_NUM_REGIONS; i++) {
+r = >io_regions[i];
+if (r->size > 0 && r->size < qemu_real_host_page_size) {
+bar_addr = r->addr;
+if (bar_addr != PCI_BAR_UNMAPPED &&
+!(bar_addr & ~qemu_real_host_page_mask)) {
+mr = >bars[i].region.mem;
+mmap_mr = >bars[i].region.mmap_mem;
+if (memory_region_is_mapped(mr) && vdev->bars[i].region.mmap &&
+memory_region_size(mr) < qemu_real_host_page_size) {
+memory_region_del_subregion(r->address_space, mr);
+memory_region_set_size(mr, qemu_real_host_page_size);
+memory_region_set_size(mmap_mr, qemu_real_host_page_size);
+memory_region_add_subregion_overlap(r->address_space,
+bar_addr, mr, 0);
+}
+}
+}
+}
+memory_region_transaction_commit();
+}
+
+/*
  * PCI config space
  */
 uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len)
@@ -1112,6 +1150,14 @@ void vfio_pci_write_config(PCIDevice *pdev,
 } else if (was_enabled && !is_enabled) {
 vfio_msix_disable(vdev);
 }
+} else if (vdev->vbasedev.flags & VFIO_DEVICE_FLAGS_PCI_PAGE_ALIGNED &&
+(ranges_overlap(addr, len, PCI_BASE_ADDRESS_0, 24) ||
+ranges_overlap(addr, l, PCI_ROM_ADDRESS, 4) ||
+ranges_overlap(addr, l, PCI_ROM_ADDRESS1, 4) ||
+range_covers_byte(addr, len, PCI_COMMAND))) {
+pci_default_write_config(pdev, addr, val, len);
+
+vfio_expand_bar_to_page_size(pdev);
 } else {
 /* Write everything to QEMU to keep emulated bits correct */
 pci_default_write_config(pdev, addr, val, len);
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH for-2.5?] qemu-iotests: Reduce racy output in 028

[Peter Maydell]

On 11 December 2015 at 03:27, Eric Blake  wrote:
> On my machine, './check -qcow2 028' was failing about 80% of the time,
> due to a race in how many times the repeated attempts to run 'info
> block-jobs' could occur before the job was done, showing up as a
> failure of fewer '(qemu) ' prompts than in the expected output.

I'm guessing from the git history for tests/qemu-iotests/028.out
that this isn't a recent regression? That suggests to me that we
shouldn't worry about fixing this for 2.5.

PS: gmail displays your email with no line breaks in it at all,
but I suspect that's gmail's fault -- perhaps it got confused by
all the hardcoded ESC characters...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 02/18] slirp: Generalizing and neutralizing code before adding IPv6 stuff

[Thomas Huth]

On 11/12/15 01:15, Samuel Thibault wrote:
> From: Guillaume Subiron 
> 
> Basically, this patch replaces "arp" by "resolution" every time "arp"
> means "mac resolution" and not specifically ARP.
> 
> Some indentation problems are solved in functions that will be modified
> in the next patches (ip_input…).
> 
> In if_encap, a switch is added to prepare for the IPv6 case. Some code
> is factorized.
> 
> Signed-off-by: Guillaume Subiron 
> Signed-off-by: Samuel Thibault 
> ---
>  slirp/mbuf.c  |  2 +-
>  slirp/mbuf.h  |  2 +-
>  slirp/slirp.c | 24 
>  3 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/slirp/mbuf.c b/slirp/mbuf.c
> index 795fc29..bc942b6 100644
> --- a/slirp/mbuf.c
> +++ b/slirp/mbuf.c
> @@ -91,7 +91,7 @@ m_get(Slirp *slirp)
>   m->m_len = 0;
>  m->m_nextpkt = NULL;
>  m->m_prevpkt = NULL;
> -m->arp_requested = false;
> +m->resolution_requested = false;
>  m->expiration_date = (uint64_t)-1;
>  end_error:
>   DEBUG_ARG("m = %p", m);
> diff --git a/slirp/mbuf.h b/slirp/mbuf.h
> index b144f1c..38fedf4 100644
> --- a/slirp/mbuf.h
> +++ b/slirp/mbuf.h
> @@ -79,7 +79,7 @@ struct mbuf {
>   int m_len;  /* Amount of data in this mbuf */
>  
>   Slirp *slirp;
> - boolarp_requested;
> + boolresolution_requested;
>   uint64_t expiration_date;
>   /* start of dynamic buffer area, must be last element */
>   union {
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 35f819a..380ddc3 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -776,6 +776,8 @@ int if_encap(Slirp *slirp, struct mbuf *ifm)
>  return 1;
>  }
>  
> +switch (iph->ip_v) {
> +case IPVERSION:
>  if (iph->ip_dst.s_addr == 0) {
>  /* 0.0.0.0 can not be a destination address, something went wrong,
>   * avoid making it worse */

That indentation looks now broken - shouldn't the if-statement now be
indented with four more spaces now?

> @@ -786,7 +788,7 @@ int if_encap(Slirp *slirp, struct mbuf *ifm)
>  struct ethhdr *reh = (struct ethhdr *)arp_req;
>  struct arphdr *rah = (struct arphdr *)(arp_req + ETH_HLEN);
>  
> -if (!ifm->arp_requested) {
> +if (!ifm->resolution_requested) {
>  /* If the client addr is not known, send an ARP request */
>  memset(reh->h_dest, 0xff, ETH_ALEN);
>  memcpy(reh->h_source, special_ethaddr, ETH_ALEN - 4);
> @@ -812,22 +814,36 @@ int if_encap(Slirp *slirp, struct mbuf *ifm)
>  rah->ar_tip = iph->ip_dst.s_addr;
>  slirp->client_ipaddr = iph->ip_dst;
>  slirp_output(slirp->opaque, arp_req, sizeof(arp_req));
> -ifm->arp_requested = true;
> +ifm->resolution_requested = true;
>  
>  /* Expire request and drop outgoing packet after 1 second */
>  ifm->expiration_date = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + 
> 10ULL;
>  }
>  return 0;
>  } else {
> -memcpy(eh->h_dest, ethaddr, ETH_ALEN);
>  memcpy(eh->h_source, special_ethaddr, ETH_ALEN - 4);
>  /* XXX: not correct */
>  memcpy(>h_source[2], >vhost_addr, 4);
>  eh->h_proto = htons(ETH_P_IP);
> +break;
> +}
> +
> +default:
> +/* Do not assert while we don't manage IP6VERSION */
> +/* assert(0); */
> +break;
> +}
> +
> +memcpy(eh->h_dest, ethaddr, ETH_ALEN);
> +DEBUG_ARGS((dfd, " src = %02x:%02x:%02x:%02x:%02x:%02x\n",
> +eh->h_source[0], eh->h_source[1], eh->h_source[2],
> +eh->h_source[3], eh->h_source[4], eh->h_source[5]));
> +DEBUG_ARGS((dfd, " dst = %02x:%02x:%02x:%02x:%02x:%02x\n",
> +eh->h_dest[0], eh->h_dest[1], eh->h_dest[2],
> +eh->h_dest[3], eh->h_dest[4], eh->h_dest[5]));
>  memcpy(buf + sizeof(struct ethhdr), ifm->m_data, ifm->m_len);
>  slirp_output(slirp->opaque, buf, ifm->m_len + ETH_HLEN);
>  return 1;
> -}
>  }

The lines after the switch statement should also only be indented with 4
spaces, not with 8.

Also, I'd prefer if you could split this patch into two: One for
renaming the "arp_requested" field, and one for adding the additional
logic with the switch statement (since there are two different kind of
changes).

 Thomas

Re: [Qemu-devel] [PATCH v7 13/31] qapi: Drop unused 'kind' for struct/enum visit

[Eric Blake]

On 12/07/2015 08:55 PM, Eric Blake wrote:
> visit_start_struct() and visit_type_enum() had a 'kind' argument
> that was usually set to either the stringized version of the
> corresponding qapi type name, or to NULL (although some clients
> didn't even get that right).  But nothing ever used the argument.
> It's even hard to argue that it would be useful in a debugger,
> as a stack backtrace also tells which type is being visited.
> 
> Therefore, drop the 'kind' argument as dead.  While at it, change
> the signature of visit_start_struct() to place the 'name'
> argument at the end (other than 'errp'), and the 'size' argument
> next to 'obj'; this placement of 'name' matches matches how all
> other functions in visit.h do it (visit_type_enum() places
> 'strings' between 'obj' and 'name'; visit_get_next_type() places
> 'promote_int' between 'type' and 'name').  This also avoids the
> confusion caused by splitting related pieces of information,
> where the old signature an unrelated parameter in between the
> "typename" and sizeof(typename) arguments.

I should probably spell it out better in the commit message; I was going
from:

visit_start_struct(v, obj, [kind,] name, size, err)

to:

visit_start_struct(v, obj, size, [kind,] name, err)

then dropping kind as unused.  But I'm seriously thinking about doing
the argument shuffle in the opposite direction (move name earlier,
rather than later):

visit_start_struct(v, name, obj, [kind,] size, err)

with a global coccinelle patch that changes ALL visit_type_* to put name
before obj, because after all we are tying this to JSON which uses
"name":value and it looks odd that every one of our visitor functions
takes parameters in 'value, name' order.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 06/16] acpi: Save PCMachineState on AcpiBuildState

[Eduardo Habkost]

On Wed, Dec 09, 2015 at 08:37:16PM +0100, Igor Mammedov wrote:
> On Tue, 8 Dec 2015 20:44:38 +0200
> Marcel Apfelbaum  wrote:
> > On 12/08/2015 07:59 PM, Eduardo Habkost wrote:
> > > On Mon, Dec 07, 2015 at 05:39:29PM +0200, Marcel Apfelbaum wrote:
> > >> On 12/02/2015 03:47 AM, Eduardo Habkost wrote:
> > >>> PCMachineState will be used in some of the steps of ACPI table
> > >>> building.
> > >>>
> > >>> Signed-off-by: Eduardo Habkost 
> > >>> ---
> > >>>   hw/i386/acpi-build.c | 8 
> > >>>   1 file changed, 4 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > >>> index 85a5c53..ca11c88 100644
> > >>> --- a/hw/i386/acpi-build.c
> > >>> +++ b/hw/i386/acpi-build.c
> > >>> @@ -1644,7 +1644,7 @@ struct AcpiBuildState {
> > >>>   MemoryRegion *table_mr;
> > >>>   /* Is table patched? */
> > >>>   uint8_t patched;
> > >>> -PcGuestInfo *guest_info;
> > >>> +PCMachineState *pcms;
> > >>>   void *rsdp;
> > >>>   MemoryRegion *rsdp_mr;
> > >>>   MemoryRegion *linker_mr;
> > >>> @@ -1855,7 +1855,7 @@ static void acpi_build_update(void
> > >>> *build_opaque, uint32_t offset)
> > >>>
> > >>>   acpi_build_tables_init();
> > >>>
> > >>> -acpi_build(build_state->guest_info, );
> > >>> +acpi_build(_state->pcms->acpi_guest_info, );
> > >>>
> > >>>   acpi_ram_update(build_state->table_mr, tables.table_data);
> > >>>
> > >>> @@ -1916,12 +1916,12 @@ void acpi_setup(PCMachineState *pcms)
> > >>>
> > >>>   build_state = g_malloc0(sizeof *build_state);
> > >>>
> > >>> -build_state->guest_info = guest_info;
> > >>> +build_state->pcms = pcms;
> > >>
> > >> I am not "sold" on keeping a reference to machine in the
> > >> build_state. We can always query current machine using
> > >> qdev_machine() or something.
> > >>
> > >> Keeping the "guest info" made sense since is used especially for
> > >> ACPI, however the machine has a wider scope. (And not having to
> > >> keep it around is a very good thing!)
> > >
> > > I wouldn't mind using qdev_get_machine() if preferred by the
> > > maintainer of that code, but I like to avoid it when possible. To
> > > me, qdev_get_machine() is just a global variable disguised as a
> > > harder-to-understand API.
> > 
> > Really? Hmm, for me is looking like the other way around :)
> > I see it as "query the QOM tree", instead of "keep the reference
> > around" everywhere. But it may be just a personal preference.
> 
> +1,
> It's not performance critical path so I'd prefer qdev_get_machine()
> instead of keeping extra reference/state.

If both of you think it's better, I will change it in v2. Thanks!

-- 
Eduardo

Re: [Qemu-devel] [PATCH QEMU-XEN v6 4/8] xen: Switch uses of xc_map_foreign_range into xc_map_foreign_pages

[Stefano Stabellini]

On Wed, 9 Dec 2015, Ian Campbell wrote:
> On Thu, 2015-12-03 at 11:23 +, Ian Campbell wrote:
> > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> > index 5e324ef..c96d974 100644
> > --- a/hw/display/xenfb.c
> > +++ b/hw/display/xenfb.c
> > @@ -104,9 +104,8 @@ static int common_bind(struct common *c)
> >  if (xenstore_read_fe_int(>xendev, "event-channel", 
> > >xendev.remote_port) == -1)
> >     return -1;
> >  
> > -c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
> > -      XC_PAGE_SIZE,
> > -      PROT_READ | PROT_WRITE, mfn);
> > +c->page = xc_map_foreign_pages(xen_xc, c->xendev.dom,
> > +   PROT_READ | PROT_WRITE, , 1);
> 
> This doesn't build for i386 userspace, since mfn is a uint64_t but
> xc_map_foreign_pages() wants a xen_pfn_t * (where xen_pfn_t == unsigned
> long on x86).
> 
> Until now that was just a truncation which was already checked for with:
> 
>     uint64_t mfn;
> 
> if (xenstore_read_fe_uint64(>xendev, "page-ref", ) == -1)
> return -1;
> assert(mfn == (xen_pfn_t)mfn);
> 
> I think in principal passing "(xen_pfn_t *)" would ok (since it is a
> singleton array in this case), but I was thinking of going a bit further
> and:
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index 8b86b4a..54585fa 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -95,11 +95,13 @@ struct XenFB {
>  
>  static int common_bind(struct common *c)
>  {
> -uint64_t mfn;
> +uint64_t val;
> +xen_pfn_t mfn;
>  
> -if (xenstore_read_fe_uint64(>xendev, "page-ref", ) == -1)
> +if (xenstore_read_fe_uint64(>xendev, "page-ref", ) == -1)
>   return -1;
> -assert(mfn == (xen_pfn_t)mfn);
> +mfn = (xen_pfn_t)val;
> +assert(val == mfn);
>  
>  if (xenstore_read_fe_int(>xendev, "event-channel", 
> >xendev.remote_port) == -1)
>   return -1;
> 
> Stefano, what do you think/prefer? An alternative to the above

I like this change because it makes the code more obvious

[Qemu-devel] [RFC PATCH 3/3] vfio/pci: Add support for mmapping MSI-X table

[Yongji Xie]

The VFIO-PCI ioctl VFIO_DEVICE_FLAGS_PCI_MSIX_MMAP flag indicate that
platform support mmapping MSI-X table.

With this flag set, we skip some MSI-X table check so that QEMU can
mmap MSI-X table successfully. And we also raise the priority of mmap
memory region in case of overlap with MSI-X/PBA memory region which
is added in msix_init().

Signed-off-by: Yongji Xie 
---
 hw/vfio/pci.c |   19 ++-
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index a7c6171..bec301c 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1339,7 +1339,8 @@ static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, 
bool enabled)
 }
 
 memory_region_set_enabled(>region.mmap_mem, enabled);
-if (vdev->msix && vdev->msix->table_bar == i) {
+if (vdev->msix && vdev->msix->table_bar == i &&
+!(vdev->vbasedev.flags & VFIO_DEVICE_FLAGS_PCI_MSIX_MMAP)) {
 memory_region_set_enabled(>msix->mmap_mem, enabled);
 }
 }
@@ -1357,7 +1358,8 @@ static void vfio_unregister_bar(VFIOPCIDevice *vdev, int 
nr)
 
 memory_region_del_subregion(>region.mem, >region.mmap_mem);
 
-if (vdev->msix && vdev->msix->table_bar == nr) {
+if (vdev->msix && vdev->msix->table_bar == nr &&
+!(vdev->vbasedev.flags & VFIO_DEVICE_FLAGS_PCI_MSIX_MMAP)) {
 memory_region_del_subregion(>region.mem, >msix->mmap_mem);
 }
 }
@@ -1374,7 +1376,8 @@ static void vfio_unmap_bar(VFIOPCIDevice *vdev, int nr)
 
 munmap(bar->region.mmap, memory_region_size(>region.mmap_mem));
 
-if (vdev->msix && vdev->msix->table_bar == nr) {
+if (vdev->msix && vdev->msix->table_bar == nr &&
+!(vdev->vbasedev.flags & VFIO_DEVICE_FLAGS_PCI_MSIX_MMAP)) {
 munmap(vdev->msix->mmap, memory_region_size(>msix->mmap_mem));
 }
 }
@@ -1420,7 +1423,8 @@ static void vfio_map_bar(VFIOPCIDevice *vdev, int nr)
  * We can't mmap areas overlapping the MSIX vector table, so we
  * potentially insert a direct-mapped subregion before and after it.
  */
-if (vdev->msix && vdev->msix->table_bar == nr) {
+if (vdev->msix && vdev->msix->table_bar == nr &&
+!(vdev->vbasedev.flags & VFIO_DEVICE_FLAGS_PCI_MSIX_MMAP)) {
 size = vdev->msix->table_offset & qemu_real_host_page_mask;
 }
 
@@ -1429,9 +1433,14 @@ static void vfio_map_bar(VFIOPCIDevice *vdev, int nr)
   >region.mmap_mem, >region.mmap,
   size, 0, name)) {
 error_report("%s unsupported. Performance may be slow", name);
+} else if (vdev->vbasedev.flags & VFIO_DEVICE_FLAGS_PCI_MSIX_MMAP) {
+memory_region_del_subregion(>region.mem, >region.mmap_mem);
+memory_region_add_subregion_overlap(>region.mem, 0,
+  >region.mmap_mem, 1);
 }
 
-if (vdev->msix && vdev->msix->table_bar == nr) {
+if (vdev->msix && vdev->msix->table_bar == nr &&
+!(vdev->vbasedev.flags & VFIO_DEVICE_FLAGS_PCI_MSIX_MMAP)) {
 uint64_t start;
 
 start = REAL_HOST_PAGE_ALIGN((uint64_t)vdev->msix->table_offset +
-- 
1.7.9.5

[Qemu-devel] tcg/tcg.c ifndef USE_LIVENESS_ANALYSIS code won't compile

[Peter Maydell]

Hi; I noticed while grepping through code that the version of
tcg_liveness_analysis() in tcg/tcg.c for #ifndef USE_LIVENESS_ANALYSIS
won't compile because it's still referring to the no-longer-existent
TCGContext::gen_opc_buf.

Richard, I think this was probably broken as part of your changes to
use a linked list of ops instead. Should we update the dummy function,
or just delete it on the grounds nobody noticed it was broken? :-)

(There are also 3 now-stale comments in the tree that refer to
gen_opc_buf still.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 02/18] slirp: Generalizing and neutralizing code before adding IPv6 stuff

[Thomas Huth]

On 11/12/15 14:43, Thomas Huth wrote:
> On 11/12/15 14:38, Thomas Huth wrote:
>> On 11/12/15 01:15, Samuel Thibault wrote:
>>> From: Guillaume Subiron 
>>>
>>> Basically, this patch replaces "arp" by "resolution" every time "arp"
>>> means "mac resolution" and not specifically ARP.
>>>
>>> Some indentation problems are solved in functions that will be modified
>>> in the next patches (ip_input…).
>>>
>>> In if_encap, a switch is added to prepare for the IPv6 case. Some code
>>> is factorized.
>>>
>>> Signed-off-by: Guillaume Subiron 
>>> Signed-off-by: Samuel Thibault 
>>> ---
>>>  slirp/mbuf.c  |  2 +-
>>>  slirp/mbuf.h  |  2 +-
>>>  slirp/slirp.c | 24 
>>>  3 files changed, 22 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/slirp/mbuf.c b/slirp/mbuf.c
>>> index 795fc29..bc942b6 100644
>>> --- a/slirp/mbuf.c
>>> +++ b/slirp/mbuf.c
>>> @@ -91,7 +91,7 @@ m_get(Slirp *slirp)
>>> m->m_len = 0;
>>>  m->m_nextpkt = NULL;
>>>  m->m_prevpkt = NULL;
>>> -m->arp_requested = false;
>>> +m->resolution_requested = false;
>>>  m->expiration_date = (uint64_t)-1;
>>>  end_error:
>>> DEBUG_ARG("m = %p", m);
>>> diff --git a/slirp/mbuf.h b/slirp/mbuf.h
>>> index b144f1c..38fedf4 100644
>>> --- a/slirp/mbuf.h
>>> +++ b/slirp/mbuf.h
>>> @@ -79,7 +79,7 @@ struct mbuf {
>>> int m_len;  /* Amount of data in this mbuf */
>>>  
>>> Slirp *slirp;
>>> -   boolarp_requested;
>>> +   boolresolution_requested;
>>> uint64_t expiration_date;
>>> /* start of dynamic buffer area, must be last element */
>>> union {
>>> diff --git a/slirp/slirp.c b/slirp/slirp.c
>>> index 35f819a..380ddc3 100644
>>> --- a/slirp/slirp.c
>>> +++ b/slirp/slirp.c
>>> @@ -776,6 +776,8 @@ int if_encap(Slirp *slirp, struct mbuf *ifm)
>>>  return 1;
>>>  }
>>>  
>>> +switch (iph->ip_v) {
>>> +case IPVERSION:
>>>  if (iph->ip_dst.s_addr == 0) {
>>>  /* 0.0.0.0 can not be a destination address, something went wrong,
>>>   * avoid making it worse */
>>
>> That indentation looks now broken - shouldn't the if-statement now be
>> indented with four more spaces now?
>>
>>> @@ -786,7 +788,7 @@ int if_encap(Slirp *slirp, struct mbuf *ifm)
>>>  struct ethhdr *reh = (struct ethhdr *)arp_req;
>>>  struct arphdr *rah = (struct arphdr *)(arp_req + ETH_HLEN);
>>>  
>>> -if (!ifm->arp_requested) {
>>> +if (!ifm->resolution_requested) {
>>>  /* If the client addr is not known, send an ARP request */
>>>  memset(reh->h_dest, 0xff, ETH_ALEN);
>>>  memcpy(reh->h_source, special_ethaddr, ETH_ALEN - 4);
>>> @@ -812,22 +814,36 @@ int if_encap(Slirp *slirp, struct mbuf *ifm)
>>>  rah->ar_tip = iph->ip_dst.s_addr;
>>>  slirp->client_ipaddr = iph->ip_dst;
>>>  slirp_output(slirp->opaque, arp_req, sizeof(arp_req));
>>> -ifm->arp_requested = true;
>>> +ifm->resolution_requested = true;
>>>  
>>>  /* Expire request and drop outgoing packet after 1 second */
>>>  ifm->expiration_date = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) 
>>> + 10ULL;
>>>  }
>>>  return 0;
>>>  } else {
>>> -memcpy(eh->h_dest, ethaddr, ETH_ALEN);
>>>  memcpy(eh->h_source, special_ethaddr, ETH_ALEN - 4);
>>>  /* XXX: not correct */
>>>  memcpy(>h_source[2], >vhost_addr, 4);
>>>  eh->h_proto = htons(ETH_P_IP);
>>> +break;
>>> +}
>>> +
>>> +default:
>>> +/* Do not assert while we don't manage IP6VERSION */
>>> +/* assert(0); */
>>> +break;
>>> +}
>>> +
>>> +memcpy(eh->h_dest, ethaddr, ETH_ALEN);
>>> +DEBUG_ARGS((dfd, " src = %02x:%02x:%02x:%02x:%02x:%02x\n",
>>> +eh->h_source[0], eh->h_source[1], eh->h_source[2],
>>> +eh->h_source[3], eh->h_source[4], eh->h_source[5]));
>>> +DEBUG_ARGS((dfd, " dst = %02x:%02x:%02x:%02x:%02x:%02x\n",
>>> +eh->h_dest[0], eh->h_dest[1], eh->h_dest[2],
>>> +eh->h_dest[3], eh->h_dest[4], eh->h_dest[5]));
>>>  memcpy(buf + sizeof(struct ethhdr), ifm->m_data, ifm->m_len);
>>>  slirp_output(slirp->opaque, buf, ifm->m_len + ETH_HLEN);
>>>  return 1;
>>> -}
>>>  }
>>
>> The lines after the switch statement should also only be indented with 4
>> spaces, not with 8.
> 
> Ah, well, never mind, I did not see the comment in the patch description
> that you fix it up in the next patch.
> 
> Anyway, it's IMHO a somewhat strange way to structure a patch ... maybe
> it would be nicer to do it in one go instead (after splitting off the
> arp_requested renaming)?

Or maybe even move the IPv4-only code into a separate function instead?
... otherwise if_encap() will be very huge once the IPv6 

Re: [Qemu-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability

[Stefano Stabellini]

On Mon, 7 Dec 2015, Jan Beulich wrote:
> >>> On 07.12.15 at 15:56,  wrote:
> > On Mon, 7 Dec 2015, Jan Beulich wrote:
> >> >>> On 07.12.15 at 13:45,  wrote:
> >> > On Tue, 24 Nov 2015, Jan Beulich wrote:
> >> >> Now that the hypervisor intercepts all config space writes and monitors
> >> >> changes to the masking flags, this undoes the main effect of the
> >> >> XSA-129 fix, exposing the masking capability again to guests.
> > 
> > Could you please mention the relevant commit ids in Xen?
> 
> It's just one (which I've now  added a reference to), unless you want
> all the prereqs listed.

One is probably OK.


> > What happens if QEMU, with this change, is running on top of an older
> > Xen that doesn't intercepts all config space writes?
> 
> The security issue would resurface.
> 
> >> >> Signed-off-by: Jan Beulich 
> >> >> ---
> >> >> TBD: We probably need to deal with running on an older hypervisor. I
> >> >>  can't, however, immediately see a way for qemu to find out.
> >> > 
> >> > Actually QEMU has already an infrastructure to detect the hypervisor
> >> > version at compile time, see include/hw/xen/xen_common.h. You could
> >> > #define the right emu_mask depending on the hypervisor.
> >> 
> >> We don't want compile time detection here, but runtime one.
> > 
> > I guess the issue is that a fix was backported to Xen that changed its
> > behaviour in past releases, right?
> 
> No, we shouldn't try to guess whether this is present in any pre-4.6
> hypervisors; we should simply accept that maskable MSI is not
> available for guests there, because ...
> 
> > Is there a way to detect the presence of this fix in Xen, by invoking an
> > hypercall and checking the returned values and error numbers?
> 
> ... there's nothing to (reliably) probe here. This really is just an
> implementation detail of the hypervisor, and hence a version check
> is all we have available.

In that case, I think we should stay on the safe side, and only expose
the masking capability (only take into effects the changes that this
patch makes) for Xen >= 4.7.

What do you think?

[Qemu-devel] [PATCH] checkpatch: Detect newlines in error_report and other error functions

[Jason J. Herne]

We don't want newlines embedded in error messages. This seems to be a common
problem with new code so let's try to catch it with checkpatch.

This does not catch the newline when it is in a multiline statement. This is
quite a bit more difficult and can be handled as follow on work.

Signed-off-by: Jason J. Herne 
---
 scripts/checkpatch.pl | 16 
 1 file changed, 16 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b0f6e11..476ac13 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2511,6 +2511,22 @@ sub process {
WARN("use QEMU instead of Qemu or QEmu\n" . $herecurr);
}
 
+# Qemu error function tests
+# FIXME: This does not work for multiline statements
+   my $qemu_error_funcs = qr{error_setg|
+   error_setg_errno|
+   error_setg_win32|
+   error_set|
+   error_vprintf|
+   error_printf|
+   error_printf_unless_qmp|
+   error_vreport|
+   error_report}x;
+
+   if ($rawline =~ /\b(?:$qemu_error_funcs)\s*\(\s*\".*\\n/) {
+   WARN("Error function text should not contain newlines\n" . 
$herecurr);
+   }
+
 # check for non-portable ffs() calls that have portable alternatives in QEMU
if ($line =~ /\bffs\(/) {
ERROR("use ctz32() instead of ffs()\n" . $herecurr);
-- 
1.9.1

[Qemu-devel] [PATCH 08/10] hw/sd/pxa2xx_mmci: Update to use new SDBus APIs

[Peter Maydell]

Now the PXA2xx MMCI device is QOMified itself, we can
update it to use the SDBus APIs to talk to the SD card.

Signed-off-by: Peter Maydell 
---
 hw/sd/pxa2xx_mmci.c | 80 +++--
 1 file changed, 66 insertions(+), 14 deletions(-)

diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index b6bb390..a30be2b 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -16,10 +16,14 @@
 #include "hw/sd/sd.h"
 #include "hw/qdev.h"
 #include "hw/qdev-properties.h"
+#include "qemu/error-report.h"
 
 #define TYPE_PXA2XX_MMCI "pxa2xx-mmci"
 #define PXA2XX_MMCI(obj) OBJECT_CHECK(PXA2xxMMCIState, (obj), TYPE_PXA2XX_MMCI)
 
+#define TYPE_PXA2XX_MMCI_BUS "pxa2xx-mmci-bus"
+#define PXA2XX_MMCI_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_PXA2XX_MMCI_BUS)
+
 typedef struct PXA2xxMMCIState {
 SysBusDevice parent_obj;
 
@@ -27,9 +31,11 @@ typedef struct PXA2xxMMCIState {
 qemu_irq irq;
 qemu_irq rx_dma;
 qemu_irq tx_dma;
+qemu_irq inserted;
+qemu_irq readonly;
 
 BlockBackend *blk;
-SDState *card;
+SDBus sdbus;
 
 uint32_t status;
 uint32_t clkrt;
@@ -129,7 +135,7 @@ static void pxa2xx_mmci_fifo_update(PXA2xxMMCIState *s)
 
 if (s->cmdat & CMDAT_WR_RD) {
 while (s->bytesleft && s->tx_len) {
-sd_write_data(s->card, s->tx_fifo[s->tx_start ++]);
+sdbus_write_data(>sdbus, s->tx_fifo[s->tx_start++]);
 s->tx_start &= 0x1f;
 s->tx_len --;
 s->bytesleft --;
@@ -139,7 +145,7 @@ static void pxa2xx_mmci_fifo_update(PXA2xxMMCIState *s)
 } else
 while (s->bytesleft && s->rx_len < 32) {
 s->rx_fifo[(s->rx_start + (s->rx_len ++)) & 0x1f] =
-sd_read_data(s->card);
+sdbus_read_data(>sdbus);
 s->bytesleft --;
 s->intreq |= INT_RXFIFO_REQ;
 }
@@ -173,7 +179,7 @@ static void pxa2xx_mmci_wakequeues(PXA2xxMMCIState *s)
 request.arg = s->arg;
 request.crc = 0;   /* FIXME */
 
-rsplen = sd_do_command(s->card, , response);
+rsplen = sdbus_do_command(>sdbus, , response);
 s->intreq |= INT_END_CMD;
 
 memset(s->resp_fifo, 0, sizeof(s->resp_fifo));
@@ -482,32 +488,59 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
 BlockBackend *blk, qemu_irq irq,
 qemu_irq rx_dma, qemu_irq tx_dma)
 {
-DeviceState *dev;
+DeviceState *dev, *carddev;
 SysBusDevice *sbd;
 PXA2xxMMCIState *s;
+Error *err = NULL;
 
 dev = qdev_create(NULL, TYPE_PXA2XX_MMCI);
 s = PXA2XX_MMCI(dev);
-/* Reach into the device and initialize the SD card. This is
- * unclean but will vanish when we update to SDBus APIs.
- */
-s->card = sd_init(s->blk, false);
-if (s->card == NULL) {
-exit(1);
-}
-qdev_init_nofail(dev);
 sbd = SYS_BUS_DEVICE(dev);
 sysbus_mmio_map(sbd, 0, base);
 sysbus_connect_irq(sbd, 0, irq);
 sysbus_connect_irq(sbd, 1, rx_dma);
 sysbus_connect_irq(sbd, 2, tx_dma);
+
+/* Create and plug in the sd card */
+carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
+qdev_prop_set_drive(carddev, "drive", blk, );
+if (err) {
+error_report("failed to init SD card: %s", error_get_pretty(err));
+return NULL;
+}
+object_property_set_bool(OBJECT(carddev), true, "realized", );
+if (err) {
+error_report("failed to init SD card: %s", error_get_pretty(err));
+return NULL;
+}
+
 return s;
 }
 
+static void pxa2xx_mmci_set_inserted(DeviceState *dev, bool inserted)
+{
+PXA2xxMMCIState *s = PXA2XX_MMCI(dev);
+
+qemu_set_irq(s->inserted, inserted);
+}
+
+static void pxa2xx_mmci_set_readonly(DeviceState *dev, bool readonly)
+{
+PXA2xxMMCIState *s = PXA2XX_MMCI(dev);
+
+qemu_set_irq(s->readonly, readonly);
+}
+
 void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
   qemu_irq coverswitch)
 {
-sd_set_cb(s->card, readonly, coverswitch);
+DeviceState *dev = DEVICE(s);
+
+s->readonly = readonly;
+s->inserted = coverswitch;
+
+pxa2xx_mmci_set_inserted(dev, sdbus_get_inserted(>sdbus));
+pxa2xx_mmci_set_readonly(dev, sdbus_get_readonly(>sdbus));
 }
 
 static void pxa2xx_mmci_instance_init(Object *obj)
@@ -524,6 +557,17 @@ static void pxa2xx_mmci_instance_init(Object *obj)
 
 register_savevm(NULL, "pxa2xx_mmci", 0, 0,
 pxa2xx_mmci_save, pxa2xx_mmci_load, s);
+
+qbus_create_inplace(>sdbus, sizeof(s->sdbus),
+TYPE_PXA2XX_MMCI_BUS, DEVICE(obj), "sd-bus");
+}
+
+static void pxa2xx_mmci_bus_class_init(ObjectClass *klass, void *data)
+{
+SDBusClass *sbc = SD_BUS_CLASS(klass);
+
+sbc->set_inserted = pxa2xx_mmci_set_inserted;
+sbc->set_readonly = pxa2xx_mmci_set_readonly;
 }
 
 static const TypeInfo pxa2xx_mmci_info = {
@@ -533,9 +577,17 @@ static const TypeInfo pxa2xx_mmci_info 

[Qemu-devel] [PATCH 06/10] sdhci_sysbus: Create SD card device in users, not the device itself

[Peter Maydell]

Move the creation of the SD card device from the sdhci_sysbus
device itself into the boards that create these devices.
This allows us to remove the cannot_instantiate_with_device_add
notation because we no longer call drive_get_next in the device
model.

Signed-off-by: Peter Maydell 
---
 hw/arm/xilinx_zynq.c | 17 -
 hw/arm/xlnx-ep108.c  | 19 +++
 hw/sd/sdhci.c| 22 --
 3 files changed, 35 insertions(+), 23 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 1c1a445..3195055 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -27,6 +27,7 @@
 #include "hw/misc/zynq-xadc.h"
 #include "hw/ssi.h"
 #include "qemu/error-report.h"
+#include "hw/sd/sd.h"
 
 #define NUM_SPI_FLASHES 4
 #define NUM_QSPI_FLASHES 2
@@ -153,8 +154,10 @@ static void zynq_init(MachineState *machine)
 MemoryRegion *address_space_mem = get_system_memory();
 MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
 MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
-DeviceState *dev;
+DeviceState *dev, *carddev;
 SysBusDevice *busdev;
+DriveInfo *di;
+BlockBackend *blk;
 qemu_irq pic[64];
 Error *err = NULL;
 int n;
@@ -260,11 +263,23 @@ static void zynq_init(MachineState *machine)
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xE010);
 sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[56-IRQ_OFFSET]);
 
+di = drive_get_next(IF_SD);
+blk = di ? blk_by_legacy_dinfo(di) : NULL;
+carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
+qdev_prop_set_drive(carddev, "drive", blk, _fatal);
+object_property_set_bool(OBJECT(carddev), true, "realized", _fatal);
+
 dev = qdev_create(NULL, "generic-sdhci");
 qdev_init_nofail(dev);
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xE0101000);
 sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[79-IRQ_OFFSET]);
 
+di = drive_get_next(IF_SD);
+blk = di ? blk_by_legacy_dinfo(di) : NULL;
+carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
+qdev_prop_set_drive(carddev, "drive", blk, _fatal);
+object_property_set_bool(OBJECT(carddev), true, "realized", _fatal);
+
 dev = qdev_create(NULL, TYPE_ZYNQ_XADC);
 qdev_init_nofail(dev);
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8007100);
diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
index 85b978f..cb95d32 100644
--- a/hw/arm/xlnx-ep108.c
+++ b/hw/arm/xlnx-ep108.c
@@ -34,6 +34,7 @@ static void xlnx_ep108_init(MachineState *machine)
 {
 XlnxEP108 *s = g_new0(XlnxEP108, 1);
 Error *err = NULL;
+int i;
 
 object_initialize(>soc, sizeof(s->soc), TYPE_XLNX_ZYNQMP);
 object_property_add_child(OBJECT(machine), "soc", OBJECT(>soc),
@@ -45,6 +46,24 @@ static void xlnx_ep108_init(MachineState *machine)
 exit(1);
 }
 
+/* Create and plug in the SD cards */
+for (i = 0; i < XLNX_ZYNQMP_NUM_SDHCI; i++) {
+BusState *bus;
+DriveInfo *di = drive_get_next(IF_SD);
+BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL;
+DeviceState *carddev;
+
+bus = qdev_get_child_bus(DEVICE(>soc.sdhci[i]), "sd-bus");
+if (!bus) {
+error_report("No SD bus found for SD card %d", i);
+exit(1);
+}
+carddev = qdev_create(bus, TYPE_SD);
+qdev_prop_set_drive(carddev, "drive", blk, _fatal);
+object_property_set_bool(OBJECT(carddev), true, "realized",
+ _fatal);
+}
+
 if (machine->ram_size > EP108_MAX_RAM_SIZE) {
 error_report("WARNING: RAM size " RAM_ADDR_FMT " above max supported, "
  "reduced to %llx", machine->ram_size, EP108_MAX_RAM_SIZE);
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 17e08a2..c8e3865 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1296,26 +1296,6 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error 
** errp)
 {
 SDHCIState *s = SYSBUS_SDHCI(dev);
 SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-DriveInfo *di;
-BlockBackend *blk;
-DeviceState *carddev;
-
-/* Create and plug in the sd card.
- * FIXME: this should be done by the users of this device so we
- * do not use drive_get_next() here.
- */
-di = drive_get_next(IF_SD);
-blk = di ? blk_by_legacy_dinfo(di) : NULL;
-
-carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
-qdev_prop_set_drive(carddev, "drive", blk, errp);
-if (*errp) {
-return;
-}
-object_property_set_bool(OBJECT(carddev), true, "realized", errp);
-if (*errp) {
-return;
-}
 
 s->buf_maxsz = sdhci_get_fifolen(s);
 s->fifo_buffer = g_malloc0(s->buf_maxsz);
@@ -1333,8 +1313,6 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, 
void *data)
 dc->vmsd = _vmstate;
 dc->props = sdhci_sysbus_properties;
 dc->realize = sdhci_sysbus_realize;
-/* Reason: instance_init() method uses 

[Qemu-devel] [PATCH 10/10] hw/sd/pxa2xx_mmci: Add reset function

[Peter Maydell]

Add a reset function to the pxa2xx_mmci device; previously it had
no handling for system reset at all.

Signed-off-by: Peter Maydell 
---
 hw/sd/pxa2xx_mmci.c | 37 +
 1 file changed, 37 insertions(+)

diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 81fec4d..3df927e 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -510,6 +510,35 @@ void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq 
readonly,
 pxa2xx_mmci_set_readonly(dev, sdbus_get_readonly(>sdbus));
 }
 
+static void pxa2xx_mmci_reset(DeviceState *d)
+{
+PXA2xxMMCIState *s = PXA2XX_MMCI(d);
+
+s->status = 0;
+s->clkrt = 0;
+s->spi = 0;
+s->cmdat = 0;
+s->resp_tout = 0;
+s->read_tout = 0;
+s->blklen = 0;
+s->numblk = 0;
+s->intmask = 0;
+s->intreq = 0;
+s->cmd = 0;
+s->arg = 0;
+s->active = 0;
+s->bytesleft = 0;
+s->tx_start = 0;
+s->tx_len = 0;
+s->rx_start = 0;
+s->rx_len = 0;
+s->resp_len = 0;
+s->cmdreq = 0;
+memset(s->tx_fifo, 0, sizeof(s->tx_fifo));
+memset(s->rx_fifo, 0, sizeof(s->rx_fifo));
+memset(s->resp_fifo, 0, sizeof(s->resp_fifo));
+}
+
 static void pxa2xx_mmci_instance_init(Object *obj)
 {
 PXA2xxMMCIState *s = PXA2XX_MMCI(obj);
@@ -526,6 +555,13 @@ static void pxa2xx_mmci_instance_init(Object *obj)
 TYPE_PXA2XX_MMCI_BUS, DEVICE(obj), "sd-bus");
 }
 
+static void pxa2xx_mmci_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+
+dc->reset = pxa2xx_mmci_reset;
+}
+
 static void pxa2xx_mmci_bus_class_init(ObjectClass *klass, void *data)
 {
 SDBusClass *sbc = SD_BUS_CLASS(klass);
@@ -539,6 +575,7 @@ static const TypeInfo pxa2xx_mmci_info = {
 .parent = TYPE_SYS_BUS_DEVICE,
 .instance_size = sizeof(PXA2xxMMCIState),
 .instance_init = pxa2xx_mmci_instance_init,
+.class_init = pxa2xx_mmci_class_init,
 };
 
 static const TypeInfo pxa2xx_mmci_bus_info = {
-- 
1.9.1

[Qemu-devel] [PULL 0/1] tags/xen-2015-12-11

[Stefano Stabellini]

The following changes since commit 6e0abc251dd4f8eba1f53656dfede12e5840e83b:

  blockdev: Mark {insert, remove}-medium experimental (2015-12-11 15:39:29 
+)

are available in the git repository at:

  git://xenbits.xen.org/people/sstabellini/qemu-dm.git tags/xen-2015-12-11

for you to fetch changes up to 73e72322d336e1a5c5fb32e8cc2310c2ad62d79b:

  xen_disk: treat "vhd" as "vpc" (2015-12-11 16:49:42 +)


Xen 2015/12/11


Stefano Stabellini (1):
  xen_disk: treat "vhd" as "vpc"

 hw/block/xen_disk.c |3 +++
 1 file changed, 3 insertions(+)

Re: [Qemu-devel] [PULL 0/1] tags/xen-2015-12-11

[Peter Maydell]

On 11 December 2015 at 17:01, Stefano Stabellini
 wrote:
> On Fri, 11 Dec 2015, Peter Maydell wrote:
>> On 11 December 2015 at 16:52, Stefano Stabellini
>>  wrote:
>> If this was intended for 2.5 you have missed the boat.
>
> I sent it because it is a bug fix, but waiting for 2.6 also makes
> sense. I'll resend after the release.

For future releases I recommend listing things you're
hoping to get into 2.5 on the wiki's Planning page.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 02/18] slirp: Generalizing and neutralizing code before adding IPv6 stuff

[Thomas Huth]

On 11/12/15 15:55, Samuel Thibault wrote:
> Thomas Huth, on Fri 11 Dec 2015 15:32:48 +0100, wrote:
>> So maybe it's better to do smaller steps instead: Would it for example
>> make sense to split the whole series into two parts - first a series
>> that does all the preparation and cleanup patches. And then once that
>> has been reviewed and merged, send the second series that adds the real
>> new IPv6 code.
> 
> Ok, that's what we already have: patches 1-9 are refactoring and
> support, and 10-18 are ipv6 code.

Sounds good, ... then I'd suggest to sent the preparation patches
separately next time and get them accepted first.

 Thomas

Re: [Qemu-devel] [PATCH for-2.5 1/1] blockdev: Mark {insert, remove}-medium experimental

[Eric Blake]

On 12/11/2015 08:23 AM, Max Reitz wrote:
> While in the long term we want throttling to be its own block filter
> BDS, in the short term we want it to be part of the BB instead of a BDS;
> even in the long term we may want legacy throttling to be automatically
> tied to the BB.
> 
> blockdev-insert-medium and blockdev-remove-medium do not retain
> throttling information in the BB (deliberately so). Therefore, using
> them means tying this information to a BDS, which would break the model
> described above. (The same applies to other flags such as
> detect_zeroes.) We probably want to move this information to the BB or
> its own filter BDS before blockdev-{insert,remove}-medium can be
> considered completely stable.
> 
> Therefore, mark these functions experimental for the time being.
> 
> Suggested-by: Markus Armbruster 
> Signed-off-by: Max Reitz 
> Acked-by: Markus Armbruster 
> Acked-by: Kevin Wolf 
> ---

Reviewed-by: Eric Blake 


> +++ b/qmp-commands.hx
> @@ -4203,13 +4203,13 @@ Example:
>  EQMP
>  
>  {
> -.name   = "blockdev-remove-medium",
> +.name   = "x-blockdev-remove-medium",
>  .args_type  = "device:s",
> -.mhandler.cmd_new = qmp_marshal_blockdev_remove_medium,
> +.mhandler.cmd_new = qmp_marshal_x_blockdev_remove_medium,
>  },
>  
>  SQMP
> -blockdev-remove-medium
> +x-blockdev-remove-medium
>  --

Formatting nit, but not worth holding this up (as it is really our last
chance to get it in 2.5 before baking in something we'd be stuck with).

>  
>  SQMP
> -blockdev-insert-medium
> +x-blockdev-insert-medium
>  --

Ditto.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH for-2.5 1/1] blockdev: Mark {insert, remove}-medium experimental

[Peter Maydell]

On 11 December 2015 at 15:43, Max Reitz  wrote:
> On 2015-12-11 at 16:40, Peter Maydell wrote:
>>
>> On 11 December 2015 at 15:30, Eric Blake  wrote:
>>>
>>> On 12/11/2015 08:23 AM, Max Reitz wrote:


   SQMP
 -blockdev-remove-medium
 +x-blockdev-remove-medium
   --
>>>
>>>
>>> Formatting nit, but not worth holding this up (as it is really our last
>>> chance to get it in 2.5 before baking in something we'd be stuck with).
>>
>>
>> You mean the length of the underline, right? I can fix that up
>> as I apply it.
>
>
> I'd appreciate that, thanks!

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH QEMU-XEN v6 4/8] xen: Switch uses of xc_map_foreign_range into xc_map_foreign_pages

[Stefano Stabellini]

On Fri, 11 Dec 2015, Ian Campbell wrote:
> On Fri, 2015-12-11 at 14:26 +, Stefano Stabellini wrote:
> > On Wed, 9 Dec 2015, Ian Campbell wrote:
> > > On Thu, 2015-12-03 at 11:23 +, Ian Campbell wrote:
> > > > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> > > > index 5e324ef..c96d974 100644
> > > > --- a/hw/display/xenfb.c
> > > > +++ b/hw/display/xenfb.c
> > > > @@ -104,9 +104,8 @@ static int common_bind(struct common *c)
> > > >  if (xenstore_read_fe_int(>xendev, "event-channel", 
> > > > >xendev.remote_port) == -1)
> > > >     return -1;
> > > >  
> > > > -c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
> > > > -      XC_PAGE_SIZE,
> > > > -      PROT_READ | PROT_WRITE, mfn);
> > > > +c->page = xc_map_foreign_pages(xen_xc, c->xendev.dom,
> > > > +   PROT_READ | PROT_WRITE, , 1);
> > > 
> > > This doesn't build for i386 userspace, since mfn is a uint64_t but
> > > xc_map_foreign_pages() wants a xen_pfn_t * (where xen_pfn_t == unsigned
> > > long on x86).
> > > 
> > > Until now that was just a truncation which was already checked for
> > > with:
> > > 
> > >     uint64_t mfn;
> > > 
> > > if (xenstore_read_fe_uint64(>xendev, "page-ref", ) == -1)
> > > return -1;
> > > assert(mfn == (xen_pfn_t)mfn);
> > > 
> > > I think in principal passing "(xen_pfn_t *)" would ok (since it is
> > > a
> > > singleton array in this case), but I was thinking of going a bit
> > > further
> > > and:
> > > 
> > > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> > > index 8b86b4a..54585fa 100644
> > > --- a/hw/display/xenfb.c
> > > +++ b/hw/display/xenfb.c
> > > @@ -95,11 +95,13 @@ struct XenFB {
> > >  
> > >  static int common_bind(struct common *c)
> > >  {
> > > -uint64_t mfn;
> > > +uint64_t val;
> > > +xen_pfn_t mfn;
> > >  
> > > -if (xenstore_read_fe_uint64(>xendev, "page-ref", ) == -1)
> > > +if (xenstore_read_fe_uint64(>xendev, "page-ref", ) == -1)
> > >   return -1;
> > > -assert(mfn == (xen_pfn_t)mfn);
> > > +mfn = (xen_pfn_t)val;
> > > +assert(val == mfn);
> > >  
> > >  if (xenstore_read_fe_int(>xendev, "event-channel", 
> > > >xendev.remote_port) == -1)
> > >   return -1;
> > > 
> > > Stefano, what do you think/prefer? An alternative to the above
> > 
> > I like this change because it makes the code more obvious
> 
> Thanks, with that change may I keep your Reviewed-by?
 
Sure

Re: [Qemu-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability

[Stefano Stabellini]

On Fri, 11 Dec 2015, Jan Beulich wrote:
> >>> On 11.12.15 at 15:33,  wrote:
> > On Mon, 7 Dec 2015, Jan Beulich wrote:
> >> >>> On 07.12.15 at 15:56,  wrote:
> >> > On Mon, 7 Dec 2015, Jan Beulich wrote:
> >> >> >>> On 07.12.15 at 13:45,  wrote:
> >> >> > On Tue, 24 Nov 2015, Jan Beulich wrote:
> >> >> >> TBD: We probably need to deal with running on an older hypervisor. I
> >> >> >>  can't, however, immediately see a way for qemu to find out.
> >> >> > 
> >> >> > Actually QEMU has already an infrastructure to detect the hypervisor
> >> >> > version at compile time, see include/hw/xen/xen_common.h. You could
> >> >> > #define the right emu_mask depending on the hypervisor.
> >> >> 
> >> >> We don't want compile time detection here, but runtime one.
> >> > 
> >> > I guess the issue is that a fix was backported to Xen that changed its
> >> > behaviour in past releases, right?
> >> 
> >> No, we shouldn't try to guess whether this is present in any pre-4.6
> >> hypervisors; we should simply accept that maskable MSI is not
> >> available for guests there, because ...
> >> 
> >> > Is there a way to detect the presence of this fix in Xen, by invoking an
> >> > hypercall and checking the returned values and error numbers?
> >> 
> >> ... there's nothing to (reliably) probe here. This really is just an
> >> implementation detail of the hypervisor, and hence a version check
> >> is all we have available.
> > 
> > In that case, I think we should stay on the safe side, and only expose
> > the masking capability (only take into effects the changes that this
> > patch makes) for Xen >= 4.7.
> > 
> > What do you think?
> 
> That's what I suggested, with the hope of getting a hint where the
> necessary infrastructure is (somehow I didn't find any run time
> version dependent code to clone from).
 
It is not possible to do this at runtime. I think we should do this at
compile time because in any case it is not supported to run a QEMU built
for a given Xen version on a different Xen version.

The infrastructure to do this at compile time is in
./include/hw/xen/xen_common.h

[Qemu-devel] [PATCH 00/10] hw/sd: QOMify sd.c (and pxa2xx_mmci)

[Peter Maydell]

This series attempts to QOMify sd.c (the actual SD card model),
including a proper QOM bus between the controller and the card.

This series removes the experimental x-drive property on sdhci-pci;
the syntax for using that device changes:

instead of using the x-drive property:

  -device sdhci-pci,x-drive=mydrive -drive id=mydrive,[...]

we create a specific sd device (which is autoplugged into the
sdhci-pci device's "sd-bus" bus if you don't manually connect them):

  -device sdhci-pci -drive id=mydrive,[...] -device sd,drive=mydrive


The basic structure of the patch series is:
 * QOMify sd.c itself
 * Add the QOM bus APIs
 * Convert sdhci to use the QOM bus APIs
 * QOMify pxa2xx_mmci
 * Convert pxa2xx_mmci to use the QOM bus APIs


Points worthy of review:
 * is the code in "sdhci_sysbus: Create SD card device in users"
   for xlnx-ep108.c to connect the SD cards up to the two controllers
   inside the SoC doing this in the right way?
 * I chose not to try to make the SD card type a subclass of
   SSI, because the interface we have at the moment didn't really
   seem to line up with what SSI has
 * generally do I have the QOM style right?


Some notes on compatibility/bisection:
 * the old-style non-QOMified controllers (which get their drive
   via blk_by_legacy_dinfo() continue to work through the whole series
 * the only QOMified controller which doesn't do that is sdhci-pci,
   which uses the experimental x-drive property to take a drive. This
   support and property is removed in patch 1; support for the new
   syntax is added in patch 5.
   (Since we're breaking the old syntax anyway I chose to not try to
   introduce the new syntax in the same commit as breaking the old one;
   I think it makes the patches easier to review.)


I don't have any Xilinx test images, so I haven't been able to test
the changes to those boards (beyond confirming that the board doesn't
crash on startup and that 'info qtree' seems to show SD cards
connected to the right things). I have tested sdhci-pci and
the pxa2xx.

Peter Maydell (10):
  hw/sd/sdhci.c: Remove x-drive property
  hw/sd/sd.c: QOMify
  hw/sd/sd.c: Convert sd_reset() function into Device reset method
  hw/sd: Add QOM bus which SD cards plug in to
  hw/sd/sdhci.c: Update to use SDBus APIs
  sdhci_sysbus: Create SD card device in users, not the device itself
  hw/sd/pxa2xx_mmci: convert to SysBusDevice object
  hw/sd/pxa2xx_mmci: Update to use new SDBus APIs
  hw/sd/pxa2xx_mmci: Convert to VMStateDescription
  hw/sd/pxa2xx_mmci: Add reset function

 hw/arm/xilinx_zynq.c  |  17 ++-
 hw/arm/xlnx-ep108.c   |  19 
 hw/sd/Makefile.objs   |   2 +-
 hw/sd/core.c  | 146 
 hw/sd/pxa2xx_mmci.c   | 304 --
 hw/sd/sd.c| 149 -
 hw/sd/sdhci.c |  83 --
 include/hw/sd/sd.h|  63 +++
 include/hw/sd/sdhci.h |   3 +-
 9 files changed, 610 insertions(+), 176 deletions(-)
 create mode 100644 hw/sd/core.c

-- 
1.9.1

[Qemu-devel] [PATCH 04/10] hw/sd: Add QOM bus which SD cards plug in to

[Peter Maydell]

Add a QOM bus for SD cards to plug in to.

Note that since sd_enable() is used only by one board and there
only as part of a broken implementation, we do not provide it in
the SDBus API (but instead add a warning comment about the old
function). Whoever converts OMAP and the nseries boards to QOM
will need to either implement the card switch properly or move
the enable hack into the OMAP MMC controller model.

In the SDBus API, the old-style use of sd_set_cb to register some
qemu_irqs for notification of card insertion and write-protect
toggling is replaced with methods in the SDBusClass which the
card calls on status changes and methods in the SDClass which
the controller can call to find out the current status. The
query methods will allow us to remove the abuse of the 'register
irqs' API by controllers in their reset methods to trigger
the card to tell them about the current status again.

Signed-off-by: Peter Maydell 
---
 hw/sd/Makefile.objs |   2 +-
 hw/sd/core.c| 146 
 hw/sd/sd.c  |  46 +++--
 include/hw/sd/sd.h  |  60 +
 4 files changed, 249 insertions(+), 5 deletions(-)
 create mode 100644 hw/sd/core.c

diff --git a/hw/sd/Makefile.objs b/hw/sd/Makefile.objs
index f1aed83..31c8330 100644
--- a/hw/sd/Makefile.objs
+++ b/hw/sd/Makefile.objs
@@ -1,6 +1,6 @@
 common-obj-$(CONFIG_PL181) += pl181.o
 common-obj-$(CONFIG_SSI_SD) += ssi-sd.o
-common-obj-$(CONFIG_SD) += sd.o
+common-obj-$(CONFIG_SD) += sd.o core.o
 common-obj-$(CONFIG_SDHCI) += sdhci.o
 
 obj-$(CONFIG_MILKYMIST) += milkymist-memcard.o
diff --git a/hw/sd/core.c b/hw/sd/core.c
new file mode 100644
index 000..584eeb5
--- /dev/null
+++ b/hw/sd/core.c
@@ -0,0 +1,146 @@
+/*
+ * SD card bus interface code.
+ *
+ * Copyright (c) 2015 Linaro Limited
+ *
+ * Author:
+ *  Peter Maydell 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "hw/qdev-core.h"
+#include "sysemu/block-backend.h"
+#include "hw/sd/sd.h"
+
+static SDState *get_card(SDBus *sdbus)
+{
+/* We only ever have one child on the bus so just return it */
+BusChild *kid = QTAILQ_FIRST(>qbus.children);
+
+if (!kid) {
+return NULL;
+}
+return SD(kid->child);
+}
+
+int sdbus_do_command(SDBus *sdbus, SDRequest *req, uint8_t *response)
+{
+SDState *card = get_card(sdbus);
+
+if (card) {
+SDClass *sc = SD_GET_CLASS(card);
+
+return sc->do_command(card, req, response);
+}
+
+return 0;
+}
+
+void sdbus_write_data(SDBus *sdbus, uint8_t value)
+{
+SDState *card = get_card(sdbus);
+
+if (card) {
+SDClass *sc = SD_GET_CLASS(card);
+
+sc->write_data(card, value);
+}
+}
+
+uint8_t sdbus_read_data(SDBus *sdbus)
+{
+SDState *card = get_card(sdbus);
+
+if (card) {
+SDClass *sc = SD_GET_CLASS(card);
+
+return sc->read_data(card);
+}
+
+return 0;
+}
+
+bool sdbus_data_ready(SDBus *sdbus)
+{
+SDState *card = get_card(sdbus);
+
+if (card) {
+SDClass *sc = SD_GET_CLASS(card);
+
+return sc->data_ready(card);
+}
+
+return false;
+}
+
+bool sdbus_get_inserted(SDBus *sdbus)
+{
+SDState *card = get_card(sdbus);
+
+if (card) {
+SDClass *sc = SD_GET_CLASS(card);
+
+return sc->get_inserted(card);
+}
+
+return false;
+}
+
+bool sdbus_get_readonly(SDBus *sdbus)
+{
+SDState *card = get_card(sdbus);
+
+if (card) {
+SDClass *sc = SD_GET_CLASS(card);
+
+return sc->get_readonly(card);
+}
+
+return false;
+}
+
+void sdbus_set_inserted(SDBus *sdbus, bool inserted)
+{
+SDBusClass *sbc = SD_BUS_GET_CLASS(sdbus);
+BusState *qbus = BUS(sdbus);
+
+if (sbc->set_inserted) {
+sbc->set_inserted(qbus->parent, inserted);
+}
+}
+
+void sdbus_set_readonly(SDBus *sdbus, bool readonly)
+{
+SDBusClass *sbc = SD_BUS_GET_CLASS(sdbus);
+BusState *qbus = BUS(sdbus);
+
+if (sbc->set_readonly) {
+sbc->set_readonly(qbus->parent, readonly);
+}
+}
+
+static const TypeInfo sd_bus_info = {
+.name = TYPE_SD_BUS,
+.parent = TYPE_BUS,
+.instance_size = sizeof(SDBus),
+.class_size = sizeof(SDBusClass),
+};
+
+static void sd_bus_register_types(void)
+{
+type_register_static(_bus_info);
+}
+
+type_init(sd_bus_register_types)
diff 

[Qemu-devel] [PATCH 02/10] hw/sd/sd.c: QOMify

[Peter Maydell]

Turn the SD card into a QOM device.
This conversion only changes the device itself; the various
functions which are effectively methods on the device are not
touched at this point.

Signed-off-by: Peter Maydell 
---
 hw/sd/sd.c | 99 ++
 include/hw/sd/sd.h |  3 ++
 2 files changed, 80 insertions(+), 22 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 1a9935c..7c79217 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -33,6 +33,8 @@
 #include "sysemu/block-backend.h"
 #include "hw/sd/sd.h"
 #include "qemu/bitmap.h"
+#include "hw/qdev-properties.h"
+#include "qemu/error-report.h"
 
 //#define DEBUG_SD 1
 
@@ -77,6 +79,8 @@ enum SDCardStates {
 };
 
 struct SDState {
+DeviceState parent_obj;
+
 uint32_t mode;/* current card mode, one of SDCardModes */
 int32_t state;/* current card state, one of SDCardStates */
 uint32_t ocr;
@@ -472,34 +476,26 @@ static const VMStateDescription sd_vmstate = {
 }
 };
 
-/* We do not model the chip select pin, so allow the board to select
-   whether card should be in SSI or MMC/SD mode.  It is also up to the
-   board to ensure that ssi transfers only occur when the chip select
-   is asserted.  */
+/* Legacy initialization function for use by non-qdevified callers */
 SDState *sd_init(BlockBackend *blk, bool is_spi)
 {
-SDState *sd;
+DeviceState *dev;
+Error *err = NULL;
 
-if (blk && blk_is_read_only(blk)) {
-fprintf(stderr, "sd_init: Cannot use read-only drive\n");
+dev = qdev_create(NULL, TYPE_SD);
+qdev_prop_set_drive(dev, "drive", blk, );
+if (err) {
+error_report("sd_init failed: %s", error_get_pretty(err));
 return NULL;
 }
-
-sd = (SDState *) g_malloc0(sizeof(SDState));
-sd->buf = blk_blockalign(blk, 512);
-sd->spi = is_spi;
-sd->enable = true;
-sd->blk = blk;
-sd_reset(sd);
-if (sd->blk) {
-/* Attach dev if not already attached.  (This call ignores an
- * error return code if sd->blk is already attached.) */
-/* FIXME ignoring blk_attach_dev() failure is dangerously brittle */
-blk_attach_dev(sd->blk, sd);
-blk_set_dev_ops(sd->blk, _block_ops, sd);
+qdev_prop_set_bit(dev, "spi", is_spi);
+object_property_set_bool(OBJECT(dev), true, "realized", );
+if (err) {
+error_report("sd_init failed: %s", error_get_pretty(err));
+return NULL;
 }
-vmstate_register(NULL, -1, _vmstate, sd);
-return sd;
+
+return SD(dev);
 }
 
 void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
@@ -1765,3 +1761,62 @@ void sd_enable(SDState *sd, bool enable)
 {
 sd->enable = enable;
 }
+
+static void sd_instance_init(Object *obj)
+{
+SDState *sd = SD(obj);
+
+sd->enable = true;
+}
+
+static void sd_realize(DeviceState *dev, Error ** errp)
+{
+SDState *sd = SD(dev);
+
+if (sd->blk && blk_is_read_only(sd->blk)) {
+error_setg(errp, "Cannot use read-only drive as SD card");
+return;
+}
+
+sd->buf = blk_blockalign(sd->blk, 512);
+
+if (sd->blk) {
+blk_set_dev_ops(sd->blk, _block_ops, sd);
+}
+
+sd_reset(sd);
+}
+
+static Property sd_properties[] = {
+DEFINE_PROP_DRIVE("drive", SDState, blk),
+/* We do not model the chip select pin, so allow the board to select
+ * whether card should be in SSI or MMC/SD mode.  It is also up to the
+ * board to ensure that ssi transfers only occur when the chip select
+ * is asserted.  */
+DEFINE_PROP_BOOL("spi", SDState, spi, false),
+DEFINE_PROP_END_OF_LIST()
+};
+
+static void sd_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+
+dc->realize = sd_realize;
+dc->props = sd_properties;
+dc->vmsd = _vmstate;
+}
+
+static const TypeInfo sd_info = {
+.name = TYPE_SD,
+.parent = TYPE_DEVICE,
+.instance_size = sizeof(SDState),
+.class_init = sd_class_init,
+.instance_init = sd_instance_init,
+};
+
+static void sd_register_types(void)
+{
+type_register_static(_info);
+}
+
+type_init(sd_register_types)
diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 79adb5b..6997dfc 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -68,6 +68,9 @@ typedef struct {
 
 typedef struct SDState SDState;
 
+#define TYPE_SD "sd"
+#define SD(obj) OBJECT_CHECK(SDState, (obj), TYPE_SD)
+
 SDState *sd_init(BlockBackend *bs, bool is_spi);
 int sd_do_command(SDState *sd, SDRequest *req,
   uint8_t *response);
-- 
1.9.1

[Qemu-devel] [PATCH 03/10] hw/sd/sd.c: Convert sd_reset() function into Device reset method

[Peter Maydell]

Convert the sd_reset() function into a proper Device reset method.

Signed-off-by: Peter Maydell 
---
 hw/sd/sd.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 7c79217..b4a5a62 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -393,8 +393,9 @@ static inline uint64_t sd_addr_to_wpnum(uint64_t addr)
 return addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
 }
 
-static void sd_reset(SDState *sd)
+static void sd_reset(DeviceState *dev)
 {
+SDState *sd = SD(dev);
 uint64_t size;
 uint64_t sect;
 
@@ -435,7 +436,7 @@ static void sd_cardchange(void *opaque, bool load)
 
 qemu_set_irq(sd->inserted_cb, blk_is_inserted(sd->blk));
 if (blk_is_inserted(sd->blk)) {
-sd_reset(sd);
+sd_reset(DEVICE(sd));
 qemu_set_irq(sd->readonly_cb, sd->wp_switch);
 }
 }
@@ -677,7 +678,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
 
 default:
 sd->state = sd_idle_state;
-sd_reset(sd);
+sd_reset(DEVICE(sd));
 return sd->spi ? sd_r1 : sd_r0;
 }
 break;
@@ -1783,8 +1784,6 @@ static void sd_realize(DeviceState *dev, Error ** errp)
 if (sd->blk) {
 blk_set_dev_ops(sd->blk, _block_ops, sd);
 }
-
-sd_reset(sd);
 }
 
 static Property sd_properties[] = {
@@ -1804,6 +1803,7 @@ static void sd_class_init(ObjectClass *klass, void *data)
 dc->realize = sd_realize;
 dc->props = sd_properties;
 dc->vmsd = _vmstate;
+dc->reset = sd_reset;
 }
 
 static const TypeInfo sd_info = {
-- 
1.9.1

[Qemu-devel] [PULL 1/1] xen_disk: treat "vhd" as "vpc"

[Stefano Stabellini]

The Xen toolstack uses "vhd" to specify a disk in VHD format, however
the name of the driver in QEMU is "vpc". Replace "vhd" with "vpc", so
that QEMU can find the right driver to use for it.

Signed-off-by: Stefano Stabellini 
---
 hw/block/xen_disk.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 8146650..a48e726 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -825,6 +825,9 @@ static int blk_init(struct XenDevice *xendev)
 if (!strcmp("aio", blkdev->fileproto)) {
 blkdev->fileproto = "raw";
 }
+if (!strcmp("vhd", blkdev->fileproto)) {
+blkdev->fileproto = "vpc";
+}
 if (blkdev->mode == NULL) {
 blkdev->mode = xenstore_read_be_str(>xendev, "mode");
 }
-- 
1.7.10.4

Re: [Qemu-devel] [PULL 0/1] tags/xen-2015-12-11

[Stefano Stabellini]

On Fri, 11 Dec 2015, Peter Maydell wrote:
> On 11 December 2015 at 16:52, Stefano Stabellini
>  wrote:
> > The following changes since commit 6e0abc251dd4f8eba1f53656dfede12e5840e83b:
> >
> >   blockdev: Mark {insert, remove}-medium experimental (2015-12-11 15:39:29 
> > +)
> >
> > are available in the git repository at:
> >
> >   git://xenbits.xen.org/people/sstabellini/qemu-dm.git tags/xen-2015-12-11
> >
> > for you to fetch changes up to 73e72322d336e1a5c5fb32e8cc2310c2ad62d79b:
> >
> >   xen_disk: treat "vhd" as "vpc" (2015-12-11 16:49:42 +)
> >
> > 
> > Xen 2015/12/11
> >
> > 
> > Stefano Stabellini (1):
> >   xen_disk: treat "vhd" as "vpc"
> >
> >  hw/block/xen_disk.c |3 +++
> >  1 file changed, 3 insertions(+)
> 
> If this was intended for 2.5 you have missed the boat.

I sent it because it is a bug fix, but waiting for 2.6 also makes
sense. I'll resend after the release.

[Qemu-devel] [PATCH 01/10] hw/sd/sdhci.c: Remove x-drive property

[Peter Maydell]

The following commits will remove support for the old sdhci-pci
command line syntax using the x-drive property:
 -device sdhci-pci,x-drive=mydrive -drive id=mydrive,[...]
and replace it with an explicit sd device:
 -device sdhci-pci -drive id=mydrive,[...] -device sd,drive=mydrive

(This is OK because x-drive is experimental.)

This commit removes the x-drive property so that old style
command lines will fail with a reasonable error message:
  -device sdhci-pci,x-drive=mydrive: Property '.x-drive' not found

Signed-off-by: Peter Maydell 
---
 hw/sd/sdhci.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 8612760..991c9b5 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1221,12 +1221,6 @@ const VMStateDescription sdhci_vmstate = {
 /* Capabilities registers provide information on supported features of this
  * specific host controller implementation */
 static Property sdhci_pci_properties[] = {
-/*
- * We currently fuse controller and card into a single device
- * model, but we intend to separate them.  For that purpose, the
- * properties that belong to the card are marked as experimental.
- */
-DEFINE_PROP_DRIVE("x-drive", SDHCIState, blk),
 DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
 SDHC_CAPAB_REG_DEFAULT),
 DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
-- 
1.9.1

[Qemu-devel] [PATCH 07/10] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Peter Maydell]

Convert the pxa2xx_mmci device to be a sysbus device.

In this commit we only change the device itself, and leave
the interface to the SD card using the old non-SDBus APIs.

Signed-off-by: Peter Maydell 
---
 hw/sd/pxa2xx_mmci.c | 73 -
 1 file changed, 55 insertions(+), 18 deletions(-)

diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index b217080..b6bb390 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -11,16 +11,24 @@
  */
 
 #include "hw/hw.h"
+#include "hw/sysbus.h"
 #include "hw/arm/pxa.h"
 #include "hw/sd/sd.h"
 #include "hw/qdev.h"
+#include "hw/qdev-properties.h"
+
+#define TYPE_PXA2XX_MMCI "pxa2xx-mmci"
+#define PXA2XX_MMCI(obj) OBJECT_CHECK(PXA2xxMMCIState, (obj), TYPE_PXA2XX_MMCI)
+
+typedef struct PXA2xxMMCIState {
+SysBusDevice parent_obj;
 
-struct PXA2xxMMCIState {
 MemoryRegion iomem;
 qemu_irq irq;
 qemu_irq rx_dma;
 qemu_irq tx_dma;
 
+BlockBackend *blk;
 SDState *card;
 
 uint32_t status;
@@ -48,7 +56,7 @@ struct PXA2xxMMCIState {
 int resp_len;
 
 int cmdreq;
-};
+} PXA2xxMMCIState;
 
 #define MMC_STRPCL 0x00/* MMC Clock Start/Stop register */
 #define MMC_STAT   0x04/* MMC Status register */
@@ -474,31 +482,60 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
 BlockBackend *blk, qemu_irq irq,
 qemu_irq rx_dma, qemu_irq tx_dma)
 {
+DeviceState *dev;
+SysBusDevice *sbd;
 PXA2xxMMCIState *s;
 
-s = (PXA2xxMMCIState *) g_malloc0(sizeof(PXA2xxMMCIState));
-s->irq = irq;
-s->rx_dma = rx_dma;
-s->tx_dma = tx_dma;
-
-memory_region_init_io(>iomem, NULL, _mmci_ops, s,
-  "pxa2xx-mmci", 0x0010);
-memory_region_add_subregion(sysmem, base, >iomem);
-
-/* Instantiate the actual storage */
-s->card = sd_init(blk, false);
+dev = qdev_create(NULL, TYPE_PXA2XX_MMCI);
+s = PXA2XX_MMCI(dev);
+/* Reach into the device and initialize the SD card. This is
+ * unclean but will vanish when we update to SDBus APIs.
+ */
+s->card = sd_init(s->blk, false);
 if (s->card == NULL) {
 exit(1);
 }
-
-register_savevm(NULL, "pxa2xx_mmci", 0, 0,
-pxa2xx_mmci_save, pxa2xx_mmci_load, s);
-
+qdev_init_nofail(dev);
+sbd = SYS_BUS_DEVICE(dev);
+sysbus_mmio_map(sbd, 0, base);
+sysbus_connect_irq(sbd, 0, irq);
+sysbus_connect_irq(sbd, 1, rx_dma);
+sysbus_connect_irq(sbd, 2, tx_dma);
 return s;
 }
 
 void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
-qemu_irq coverswitch)
+  qemu_irq coverswitch)
 {
 sd_set_cb(s->card, readonly, coverswitch);
 }
+
+static void pxa2xx_mmci_instance_init(Object *obj)
+{
+PXA2xxMMCIState *s = PXA2XX_MMCI(obj);
+SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+
+memory_region_init_io(>iomem, obj, _mmci_ops, s,
+  "pxa2xx-mmci", 0x0010);
+sysbus_init_mmio(sbd, >iomem);
+sysbus_init_irq(sbd, >irq);
+sysbus_init_irq(sbd, >rx_dma);
+sysbus_init_irq(sbd, >tx_dma);
+
+register_savevm(NULL, "pxa2xx_mmci", 0, 0,
+pxa2xx_mmci_save, pxa2xx_mmci_load, s);
+}
+
+static const TypeInfo pxa2xx_mmci_info = {
+.name = TYPE_PXA2XX_MMCI,
+.parent = TYPE_SYS_BUS_DEVICE,
+.instance_size = sizeof(PXA2xxMMCIState),
+.instance_init = pxa2xx_mmci_instance_init,
+};
+
+static void pxa2xx_mmci_register_types(void)
+{
+type_register_static(_mmci_info);
+}
+
+type_init(pxa2xx_mmci_register_types)
-- 
1.9.1

[Qemu-devel] [PATCH 05/10] hw/sd/sdhci.c: Update to use SDBus APIs

[Peter Maydell]

Update the SDHCI code to use the new SDBus APIs.

This commit introduces the new command line options required
to connect a disk to sdhci-pci:

 -device sdhci-pci -drive id=mydrive,[...] -device sd,drive=mydrive

Signed-off-by: Peter Maydell 
---
 hw/sd/sdhci.c | 95 +++
 include/hw/sd/sdhci.h |  3 +-
 2 files changed, 67 insertions(+), 31 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 991c9b5..17e08a2 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -55,6 +55,9 @@
 } \
 } while (0)
 
+#define TYPE_SDHCI_BUS "sdhci-bus"
+#define SDHCI_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_PXA2XX_MMCI_BUS)
+
 /* Default SD/MMC host controller features information, which will be
  * presented in CAPABILITIES register of generic SD host controller at reset.
  * If not stated otherwise:
@@ -145,9 +148,9 @@ static void sdhci_raise_insertion_irq(void *opaque)
 }
 }
 
-static void sdhci_insert_eject_cb(void *opaque, int irq, int level)
+static void sdhci_set_inserted(DeviceState *dev, bool level)
 {
-SDHCIState *s = (SDHCIState *)opaque;
+SDHCIState *s = (SDHCIState *)dev;
 DPRINT_L1("Card state changed: %s!\n", level ? "insert" : "eject");
 
 if ((s->norintsts & SDHC_NIS_REMOVE) && level) {
@@ -172,9 +175,9 @@ static void sdhci_insert_eject_cb(void *opaque, int irq, 
int level)
 }
 }
 
-static void sdhci_card_readonly_cb(void *opaque, int irq, int level)
+static void sdhci_set_readonly(DeviceState *dev, bool level)
 {
-SDHCIState *s = (SDHCIState *)opaque;
+SDHCIState *s = (SDHCIState *)dev;
 
 if (level) {
 s->prnsts &= ~SDHC_WRITE_PROTECT;
@@ -186,6 +189,8 @@ static void sdhci_card_readonly_cb(void *opaque, int irq, 
int level)
 
 static void sdhci_reset(SDHCIState *s)
 {
+DeviceState *dev = DEVICE(s);
+
 timer_del(s->insert_timer);
 timer_del(s->transfer_timer);
 /* Set all registers to 0. Capabilities registers are not cleared
@@ -193,7 +198,10 @@ static void sdhci_reset(SDHCIState *s)
  * initialization */
 memset(>sdmasysad, 0, (uintptr_t)>capareg - 
(uintptr_t)>sdmasysad);
 
-sd_set_cb(s->card, s->ro_cb, s->eject_cb);
+/* Reset other state based on current card insertion/readonly status */
+sdhci_set_inserted(dev, sdbus_get_inserted(>sdbus));
+sdhci_set_readonly(dev, sdbus_get_readonly(>sdbus));
+
 s->data_count = 0;
 s->stopped_state = sdhc_not_stopped;
 }
@@ -211,7 +219,7 @@ static void sdhci_send_command(SDHCIState *s)
 request.cmd = s->cmdreg >> 8;
 request.arg = s->argument;
 DPRINT_L1("sending CMD%u ARG[0x%08x]\n", request.cmd, request.arg);
-rlen = sd_do_command(s->card, , response);
+rlen = sdbus_do_command(>sdbus, , response);
 
 if (s->cmdreg & SDHC_CMD_RESPONSE) {
 if (rlen == 4) {
@@ -270,7 +278,7 @@ static void sdhci_end_transfer(SDHCIState *s)
 request.cmd = 0x0C;
 request.arg = 0;
 DPRINT_L1("Automatically issue CMD%d %08x\n", request.cmd, 
request.arg);
-sd_do_command(s->card, , response);
+sdbus_do_command(>sdbus, , response);
 /* Auto CMD12 response goes to the upper Response register */
 s->rspreg[3] = (response[0] << 24) | (response[1] << 16) |
 (response[2] << 8) | response[3];
@@ -302,7 +310,7 @@ static void sdhci_read_block_from_card(SDHCIState *s)
 }
 
 for (index = 0; index < (s->blksize & 0x0fff); index++) {
-s->fifo_buffer[index] = sd_read_data(s->card);
+s->fifo_buffer[index] = sdbus_read_data(>sdbus);
 }
 
 /* New data now available for READ through Buffer Port Register */
@@ -395,7 +403,7 @@ static void sdhci_write_block_to_card(SDHCIState *s)
 }
 
 for (index = 0; index < (s->blksize & 0x0fff); index++) {
-sd_write_data(s->card, s->fifo_buffer[index]);
+sdbus_write_data(>sdbus, s->fifo_buffer[index]);
 }
 
 /* Next data can be written through BUFFER DATORT register */
@@ -477,7 +485,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
 while (s->blkcnt) {
 if (s->data_count == 0) {
 for (n = 0; n < block_size; n++) {
-s->fifo_buffer[n] = sd_read_data(s->card);
+s->fifo_buffer[n] = sdbus_read_data(>sdbus);
 }
 }
 begin = s->data_count;
@@ -518,7 +526,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
 s->sdmasysad += s->data_count - begin;
 if (s->data_count == block_size) {
 for (n = 0; n < block_size; n++) {
-sd_write_data(s->card, s->fifo_buffer[n]);
+sdbus_write_data(>sdbus, s->fifo_buffer[n]);
 }
 s->data_count = 0;
 if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
@@ -550,7 +558,7 @@ static void sdhci_sdma_transfer_single_block(SDHCIState *s)
 
 if (s->trnmod 

[Qemu-devel] [PATCH 09/10] hw/sd/pxa2xx_mmci: Convert to VMStateDescription

[Peter Maydell]

Convert the pxa2xx_mmci device from manual save/load
functions to a VMStateDescription structure.

This is a migration compatibility break.

Signed-off-by: Peter Maydell 
---
 hw/sd/pxa2xx_mmci.c | 148 
 1 file changed, 56 insertions(+), 92 deletions(-)

diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index a30be2b..81fec4d 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -43,27 +43,72 @@ typedef struct PXA2xxMMCIState {
 uint32_t cmdat;
 uint32_t resp_tout;
 uint32_t read_tout;
-int blklen;
-int numblk;
+int32_t blklen;
+int32_t numblk;
 uint32_t intmask;
 uint32_t intreq;
-int cmd;
+int32_t cmd;
 uint32_t arg;
 
-int active;
-int bytesleft;
+int32_t active;
+int32_t bytesleft;
 uint8_t tx_fifo[64];
-int tx_start;
-int tx_len;
+uint32_t tx_start;
+uint32_t tx_len;
 uint8_t rx_fifo[32];
-int rx_start;
-int rx_len;
+uint32_t rx_start;
+uint32_t rx_len;
 uint16_t resp_fifo[9];
-int resp_len;
+uint32_t resp_len;
 
-int cmdreq;
+int32_t cmdreq;
 } PXA2xxMMCIState;
 
+static bool pxa2xx_mmci_vmstate_validate(void *opaque, int version_id)
+{
+PXA2xxMMCIState *s = opaque;
+
+return s->tx_start < sizeof(s->tx_fifo)
+&& s->rx_start < sizeof(s->rx_fifo)
+&& s->tx_len <= sizeof(s->tx_fifo)
+&& s->rx_len <= sizeof(s->rx_fifo)
+&& s->resp_len <= sizeof(s->resp_fifo);
+}
+
+
+static const VMStateDescription vmstate_pxa2xx_mmci = {
+.name = "pxa2xx-mmci",
+.version_id = 2,
+.minimum_version_id = 2,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32(status, PXA2xxMMCIState),
+VMSTATE_UINT32(clkrt, PXA2xxMMCIState),
+VMSTATE_UINT32(spi, PXA2xxMMCIState),
+VMSTATE_UINT32(cmdat, PXA2xxMMCIState),
+VMSTATE_UINT32(resp_tout, PXA2xxMMCIState),
+VMSTATE_UINT32(read_tout, PXA2xxMMCIState),
+VMSTATE_INT32(blklen, PXA2xxMMCIState),
+VMSTATE_INT32(numblk, PXA2xxMMCIState),
+VMSTATE_UINT32(intmask, PXA2xxMMCIState),
+VMSTATE_UINT32(intreq, PXA2xxMMCIState),
+VMSTATE_INT32(cmd, PXA2xxMMCIState),
+VMSTATE_UINT32(arg, PXA2xxMMCIState),
+VMSTATE_INT32(cmdreq, PXA2xxMMCIState),
+VMSTATE_INT32(active, PXA2xxMMCIState),
+VMSTATE_INT32(bytesleft, PXA2xxMMCIState),
+VMSTATE_UINT32(tx_start, PXA2xxMMCIState),
+VMSTATE_UINT32(tx_len, PXA2xxMMCIState),
+VMSTATE_UINT32(rx_start, PXA2xxMMCIState),
+VMSTATE_UINT32(rx_len, PXA2xxMMCIState),
+VMSTATE_UINT32(resp_len, PXA2xxMMCIState),
+VMSTATE_VALIDATE("fifo size incorrect", pxa2xx_mmci_vmstate_validate),
+VMSTATE_UINT8_ARRAY(tx_fifo, PXA2xxMMCIState, 64),
+VMSTATE_UINT8_ARRAY(rx_fifo, PXA2xxMMCIState, 32),
+VMSTATE_UINT16_ARRAY(resp_fifo, PXA2xxMMCIState, 9),
+VMSTATE_END_OF_LIST()
+}
+};
+
 #define MMC_STRPCL 0x00/* MMC Clock Start/Stop register */
 #define MMC_STAT   0x04/* MMC Status register */
 #define MMC_CLKRT  0x08/* MMC Clock Rate register */
@@ -405,84 +450,6 @@ static const MemoryRegionOps pxa2xx_mmci_ops = {
 .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static void pxa2xx_mmci_save(QEMUFile *f, void *opaque)
-{
-PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
-int i;
-
-qemu_put_be32s(f, >status);
-qemu_put_be32s(f, >clkrt);
-qemu_put_be32s(f, >spi);
-qemu_put_be32s(f, >cmdat);
-qemu_put_be32s(f, >resp_tout);
-qemu_put_be32s(f, >read_tout);
-qemu_put_be32(f, s->blklen);
-qemu_put_be32(f, s->numblk);
-qemu_put_be32s(f, >intmask);
-qemu_put_be32s(f, >intreq);
-qemu_put_be32(f, s->cmd);
-qemu_put_be32s(f, >arg);
-qemu_put_be32(f, s->cmdreq);
-qemu_put_be32(f, s->active);
-qemu_put_be32(f, s->bytesleft);
-
-qemu_put_byte(f, s->tx_len);
-for (i = 0; i < s->tx_len; i ++)
-qemu_put_byte(f, s->tx_fifo[(s->tx_start + i) & 63]);
-
-qemu_put_byte(f, s->rx_len);
-for (i = 0; i < s->rx_len; i ++)
-qemu_put_byte(f, s->rx_fifo[(s->rx_start + i) & 31]);
-
-qemu_put_byte(f, s->resp_len);
-for (i = s->resp_len; i < 9; i ++)
-qemu_put_be16s(f, >resp_fifo[i]);
-}
-
-static int pxa2xx_mmci_load(QEMUFile *f, void *opaque, int version_id)
-{
-PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
-int i;
-
-qemu_get_be32s(f, >status);
-qemu_get_be32s(f, >clkrt);
-qemu_get_be32s(f, >spi);
-qemu_get_be32s(f, >cmdat);
-qemu_get_be32s(f, >resp_tout);
-qemu_get_be32s(f, >read_tout);
-s->blklen = qemu_get_be32(f);
-s->numblk = qemu_get_be32(f);
-qemu_get_be32s(f, >intmask);
-qemu_get_be32s(f, >intreq);
-s->cmd = qemu_get_be32(f);
-qemu_get_be32s(f, >arg);
-s->cmdreq = qemu_get_be32(f);
-s->active = qemu_get_be32(f);
-s->bytesleft = 

Re: [Qemu-devel] [PULL 0/1] tags/xen-2015-12-11

[Peter Maydell]

On 11 December 2015 at 16:52, Stefano Stabellini
 wrote:
> The following changes since commit 6e0abc251dd4f8eba1f53656dfede12e5840e83b:
>
>   blockdev: Mark {insert, remove}-medium experimental (2015-12-11 15:39:29 
> +)
>
> are available in the git repository at:
>
>   git://xenbits.xen.org/people/sstabellini/qemu-dm.git tags/xen-2015-12-11
>
> for you to fetch changes up to 73e72322d336e1a5c5fb32e8cc2310c2ad62d79b:
>
>   xen_disk: treat "vhd" as "vpc" (2015-12-11 16:49:42 +)
>
> 
> Xen 2015/12/11
>
> 
> Stefano Stabellini (1):
>   xen_disk: treat "vhd" as "vpc"
>
>  hw/block/xen_disk.c |3 +++
>  1 file changed, 3 insertions(+)

If this was intended for 2.5 you have missed the boat.

thanks
-- PMM

Re: [Qemu-devel] [Xen-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability

[Ian Campbell]

On Fri, 2015-12-11 at 16:44 +, Stefano Stabellini wrote:
>  
> It is not possible to do this at runtime. I think we should do this at
> compile time because in any case it is not supported to run a QEMU built
> for a given Xen version on a different Xen version.

I am currently working pretty hard to make this possible in the future, it
would be a shame to add another reason it wasn't possible at this stage.

I proposed (in <1445442435.9563.184.ca...@citrix.com>) that as well as the
various stable libraries extracted from libxenctrl we will probably also
want to have a libxendevicemodel.so at some point, to provide a stable way
to interface with all the stuff which being a DM involves.

Maybe that library could contain a way to get this information? (In which
case it could be hardcoded at compile time now and I'll see what I can do
when I get to producing the library).

For the original issue here, could the flag be exposed as a
XEN_SYSCTL_PHYSCAP_

Ian.

[Qemu-devel] [PATCH for-2.5 1/1] blockdev: Mark {insert, remove}-medium experimental

[Max Reitz]

While in the long term we want throttling to be its own block filter
BDS, in the short term we want it to be part of the BB instead of a BDS;
even in the long term we may want legacy throttling to be automatically
tied to the BB.

blockdev-insert-medium and blockdev-remove-medium do not retain
throttling information in the BB (deliberately so). Therefore, using
them means tying this information to a BDS, which would break the model
described above. (The same applies to other flags such as
detect_zeroes.) We probably want to move this information to the BB or
its own filter BDS before blockdev-{insert,remove}-medium can be
considered completely stable.

Therefore, mark these functions experimental for the time being.

Suggested-by: Markus Armbruster 
Signed-off-by: Max Reitz 
Acked-by: Markus Armbruster 
Acked-by: Kevin Wolf 
---
 blockdev.c | 10 +-
 qapi/block-core.json   | 18 --
 qmp-commands.hx| 24 +++-
 tests/qemu-iotests/118 | 20 ++--
 tests/qemu-iotests/139 |  2 +-
 5 files changed, 43 insertions(+), 31 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 313841b..80932e8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2257,7 +2257,7 @@ void qmp_eject(const char *device, bool has_force, bool 
force, Error **errp)
 return;
 }
 
-qmp_blockdev_remove_medium(device, errp);
+qmp_x_blockdev_remove_medium(device, errp);
 }
 
 void qmp_block_passwd(bool has_device, const char *device,
@@ -2343,7 +2343,7 @@ void qmp_blockdev_close_tray(const char *device, Error 
**errp)
 blk_dev_change_media_cb(blk, true);
 }
 
-void qmp_blockdev_remove_medium(const char *device, Error **errp)
+void qmp_x_blockdev_remove_medium(const char *device, Error **errp)
 {
 BlockBackend *blk;
 BlockDriverState *bs;
@@ -2430,8 +2430,8 @@ static void qmp_blockdev_insert_anon_medium(const char 
*device,
 QTAILQ_INSERT_TAIL(_states, bs, device_list);
 }
 
-void qmp_blockdev_insert_medium(const char *device, const char *node_name,
-Error **errp)
+void qmp_x_blockdev_insert_medium(const char *device, const char *node_name,
+  Error **errp)
 {
 BlockDriverState *bs;
 
@@ -2520,7 +2520,7 @@ void qmp_blockdev_change_medium(const char *device, const 
char *filename,
 goto fail;
 }
 
-qmp_blockdev_remove_medium(device, );
+qmp_x_blockdev_remove_medium(device, );
 if (err) {
 error_propagate(errp, err);
 goto fail;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a07b13f..5a23165 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2081,7 +2081,7 @@
   'data': { 'device': 'str' } }
 
 ##
-# @blockdev-remove-medium:
+# @x-blockdev-remove-medium:
 #
 # Removes a medium (a block driver state tree) from a block device. That block
 # device's tray must currently be open (unless there is no attached guest
@@ -2089,27 +2089,33 @@
 #
 # If the tray is open and there is no medium inserted, this will be a no-op.
 #
+# This command is still a work in progress and is considered experimental.
+# Stay away from it unless you want to help with its development.
+#
 # @device: block device name
 #
 # Since: 2.5
 ##
-{ 'command': 'blockdev-remove-medium',
+{ 'command': 'x-blockdev-remove-medium',
   'data': { 'device': 'str' } }
 
 ##
-# @blockdev-insert-medium:
+# @x-blockdev-insert-medium:
 #
 # Inserts a medium (a block driver state tree) into a block device. That block
 # device's tray must currently be open (unless there is no attached guest
 # device) and there must be no medium inserted already.
 #
+# This command is still a work in progress and is considered experimental.
+# Stay away from it unless you want to help with its development.
+#
 # @device:block device name
 #
 # @node-name: name of a node in the block driver state graph
 #
 # Since: 2.5
 ##
-{ 'command': 'blockdev-insert-medium',
+{ 'command': 'x-blockdev-insert-medium',
   'data': { 'device': 'str',
 'node-name': 'str'} }
 
@@ -2137,8 +2143,8 @@
 #
 # Changes the medium inserted into a block device by ejecting the current 
medium
 # and loading a new image file which is inserted as the new medium (this 
command
-# combines blockdev-open-tray, blockdev-remove-medium, blockdev-insert-medium
-# and blockdev-close-tray).
+# combines blockdev-open-tray, x-blockdev-remove-medium,
+# x-blockdev-insert-medium and blockdev-close-tray).
 #
 # @device:  block device name
 #
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9d8b42f..b6e5e44 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4203,13 +4203,13 @@ Example:
 EQMP
 
 {
-.name   = "blockdev-remove-medium",
+.name   = "x-blockdev-remove-medium",
 .args_type  = "device:s",
-.mhandler.cmd_new = qmp_marshal_blockdev_remove_medium,
+

Re: [Qemu-devel] [PATCH QEMU-XEN v6 4/8] xen: Switch uses of xc_map_foreign_range into xc_map_foreign_pages

[Ian Campbell]

On Fri, 2015-12-11 at 14:26 +, Stefano Stabellini wrote:
> On Wed, 9 Dec 2015, Ian Campbell wrote:
> > On Thu, 2015-12-03 at 11:23 +, Ian Campbell wrote:
> > > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> > > index 5e324ef..c96d974 100644
> > > --- a/hw/display/xenfb.c
> > > +++ b/hw/display/xenfb.c
> > > @@ -104,9 +104,8 @@ static int common_bind(struct common *c)
> > >  if (xenstore_read_fe_int(>xendev, "event-channel", 
> > > >xendev.remote_port) == -1)
> > >   return -1;
> > >  
> > > -c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
> > > -    XC_PAGE_SIZE,
> > > -    PROT_READ | PROT_WRITE, mfn);
> > > +c->page = xc_map_foreign_pages(xen_xc, c->xendev.dom,
> > > +   PROT_READ | PROT_WRITE, , 1);
> > 
> > This doesn't build for i386 userspace, since mfn is a uint64_t but
> > xc_map_foreign_pages() wants a xen_pfn_t * (where xen_pfn_t == unsigned
> > long on x86).
> > 
> > Until now that was just a truncation which was already checked for
> > with:
> > 
> >     uint64_t mfn;
> > 
> > if (xenstore_read_fe_uint64(>xendev, "page-ref", ) == -1)
> > return -1;
> > assert(mfn == (xen_pfn_t)mfn);
> > 
> > I think in principal passing "(xen_pfn_t *)" would ok (since it is
> > a
> > singleton array in this case), but I was thinking of going a bit
> > further
> > and:
> > 
> > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> > index 8b86b4a..54585fa 100644
> > --- a/hw/display/xenfb.c
> > +++ b/hw/display/xenfb.c
> > @@ -95,11 +95,13 @@ struct XenFB {
> >  
> >  static int common_bind(struct common *c)
> >  {
> > -uint64_t mfn;
> > +uint64_t val;
> > +xen_pfn_t mfn;
> >  
> > -if (xenstore_read_fe_uint64(>xendev, "page-ref", ) == -1)
> > +if (xenstore_read_fe_uint64(>xendev, "page-ref", ) == -1)
> >     return -1;
> > -assert(mfn == (xen_pfn_t)mfn);
> > +mfn = (xen_pfn_t)val;
> > +assert(val == mfn);
> >  
> >  if (xenstore_read_fe_int(>xendev, "event-channel", 
> > >xendev.remote_port) == -1)
> >     return -1;
> > 
> > Stefano, what do you think/prefer? An alternative to the above
> 
> I like this change because it makes the code more obvious

Thanks, with that change may I keep your Reviewed-by?

Re: [Qemu-devel] How does TCG gen host code for a TB?

[Valerón JC]

Peter and Sergey, Thank you so much for your help and explanation. 
I appreciate your helps very much.

Sent from Mail for Windows 10



From: Sergey Fedorov
Sent: Friday, December 11, 2015 10:21 PM
To: Valerón JC;qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] How does TCG gen host code for a TB?


On 11.12.2015 06:43, Valerón JC wrote:
> I want to trace a bug in tcg, which for me, at some point, generate
> infinite loop TB chains, that's unexpected. and I've found the final
> TB(head, since they're chaining) which run in an infinite loop, and I
> know a very weird trick to 'disable' this bug, so I would like to
> track the tcg-ops for the TB, hope that I can figure what's wrong.

Hi,

You may find it useful to enable logging of TCG ops generated with '-d
op', as well as input and output assembly with '-d in_asm,out_asm'. '-D
' could be useful to output log to a file. Here are references
to documentation: http://qemu.weilnetz.de/qemu-doc.html#index-_002dd and
http://qemu.weilnetz.de/qemu-doc.html#index-_002dD.

Best,
Sergey

Re: [Qemu-devel] [PATCH 02/18] slirp: Generalizing and neutralizing code before adding IPv6 stuff

[Samuel Thibault]

Laszlo Ersek, on Fri 11 Dec 2015 16:40:32 +0100, wrote:
> > Sounds good, ... then I'd suggest to sent the preparation patches
> > separately next time and get them accepted first.
> 
> And then the next reviewer will say, "nice, but it would be even nicer
> to see what *motivates* these preparatory patches!" :)

Yes, I also had that in mind, just didn't say it :)

Samuel

Re: [Qemu-devel] [PATCH 06/18] slirp: Factorizing and cleaning solookup()

[Thomas Huth]

On 11/12/15 01:15, Samuel Thibault wrote:
> From: Guillaume Subiron 
> 
> This patch makes solookup() compatible with varying address families. Also,
> this function was only compatible with TCP. Having the socket list in
> argument, it is now compatible with UDP too. Finally, some optimization
> code is factorized inside the function (the function look at the last
> returned result before browsing the complete socket list).
> 
> This also adds a sockaddr_equal() function to compare two
> sockaddr_storage.

I'd maybe also split this patch into two - first introduce the
sockaddr_equal() function, then do the other changes. If you do too much
stuff in one patch, it gets more difficult to read.

> Signed-off-by: Guillaume Subiron 
> Signed-off-by: Samuel Thibault 
> ---
[...]
> diff --git a/slirp/socket.h b/slirp/socket.h
> index b27bbb2..644216c 100644
> --- a/slirp/socket.h
> +++ b/slirp/socket.h
> @@ -87,7 +87,28 @@ struct socket {
>  #define SS_HOSTFWD   0x1000  /* Socket describes host->guest 
> forwarding */
>  #define SS_INCOMING  0x2000  /* Connection was initiated by a host 
> on the internet */
>  
> -struct socket * solookup(struct socket *, struct in_addr, u_int, struct 
> in_addr, u_int);
> +static inline int sockaddr_equal(struct sockaddr_storage *a,
> +struct sockaddr_storage *b)
> +{
> +if (a->ss_family != b->ss_family) {
> +return 0;
> +} else {
> +switch (a->ss_family) {
> +case AF_INET:
> +{
> +struct sockaddr_in *a4 = (struct sockaddr_in *) a;
> +struct sockaddr_in *b4 = (struct sockaddr_in *) b;
> +return (a4->sin_addr.s_addr == b4->sin_addr.s_addr
> +&& a4->sin_port == b4->sin_port);
> +}
> +default:
> +assert(0);
> +}
> +}
> +}

Since the first part of the if statement always returns, you could get
rid of one level of indentation here by removing the "else".

 Thomas

Re: [Qemu-devel] [PATCH 11/11] pseries: Clean up error reporting in htab migration functions

[Eric Blake]

On 12/10/2015 05:11 PM, David Gibson wrote:
> The functions for migrating the hash page table on pseries machine type
> (htab_save_setup() and htab_load()) can report some errors with an
> explicit fprintf() before returning an appropriate eror code.  Change these
> to use error_report() instead.
> 
> Signed-off-by: David Gibson 
> ---
>  hw/ppc/spapr.c | 17 +
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 

>  
> @@ -1577,9 +1578,9 @@ static int htab_load(QEMUFile *f, void *opaque, int 
> version_id)
>  if ((index + n_valid + n_invalid) >
>  (HTAB_SIZE(spapr) / HASH_PTE_SIZE_64)) {
>  /* Bad index in stream */
> -fprintf(stderr, "htab_load() bad index %d (%hd+%hd entries) "
> -"in htab stream (htab_shift=%d)\n", index, n_valid, 
> n_invalid,
> -spapr->htab_shift);
> +error_report(
> +"htab_load() bad index %d (%hd+%hd entries) in htab stream 
> (htab_shift=%d)\n",
> +index, n_valid, n_invalid, spapr->htab_shift);

No trailing newline to error_report().

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH for-2.5 1/1] blockdev: Mark {insert, remove}-medium experimental

[Max Reitz]

On 2015-12-11 at 16:40, Peter Maydell wrote:

On 11 December 2015 at 15:30, Eric Blake  wrote:

On 12/11/2015 08:23 AM, Max Reitz wrote:


  SQMP
-blockdev-remove-medium
+x-blockdev-remove-medium
  --


Formatting nit, but not worth holding this up (as it is really our last
chance to get it in 2.5 before baking in something we'd be stuck with).


You mean the length of the underline, right? I can fix that up
as I apply it.


I'd appreciate that, thanks!

Max

Re: [Qemu-devel] [PATCH 03/11] pseries: Clean up hash page table allocation error handling

[Eric Blake]

On 12/10/2015 05:11 PM, David Gibson wrote:
> The spapr_alloc_htab() and spapr_reset_htab() functions currently handle
> all errors with error_setg(_abort, ...).  That's correct for
> spapr_reset_htab() - if anything goes wrong there, there's really nothing
> we can do about it.  For spapr_alloc_htab() _fatal would make more
> sense, since it occurs during initialization.
> 
> But in any case the callers are really better placed to decide on the error
> handling.  So, instead make the functions use the error propagation
> infrastructure.
> 
> While we're at it improve the messages themselves a bit, and clean up the
> indentation a little.
> 
> Signed-off-by: David Gibson 
> ---
>  hw/ppc/spapr.c | 26 ++
>  1 file changed, 18 insertions(+), 8 deletions(-)

> @@ -1030,7 +1030,9 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
>   * For HV KVM, host kernel will return -ENOMEM when requested
>   * HTAB size can't be allocated.
>   */
> -error_setg(_abort, "Failed to allocate HTAB of requested size, 
> try with smaller maxmem");
> +error_setg(errp,
> +"Error allocating KVM hash page table, try smaller maxmem: %s",
> +strerror(-shift));

error_setg_errno() is nicer than calling strerror().

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH for-2.5 0/1] blockdev: Mark {insert, remove}-medium experimental

[Max Reitz]

Hello Peter,

After having talked about the current status of the block layer today,
Markus, Kevin and me agreed to mark the newly introduced QMP commands
blockdev-insert-medium and blockdev-remove-medium experimental after
all (due to possible interference of its current status with future
designs).

Can you please apply this patch directly?

I am sorry for the inconvencience.

Max


Max Reitz (1):
  blockdev: Mark {insert,remove}-medium experimental

 blockdev.c | 10 +-
 qapi/block-core.json   | 18 --
 qmp-commands.hx| 24 +++-
 tests/qemu-iotests/118 | 20 ++--
 tests/qemu-iotests/139 |  2 +-
 5 files changed, 43 insertions(+), 31 deletions(-)

-- 
2.5.0

Re: [Qemu-devel] [PATCH for-2.5 1/1] blockdev: Mark {insert, remove}-medium experimental

[Peter Maydell]

On 11 December 2015 at 15:30, Eric Blake  wrote:
> On 12/11/2015 08:23 AM, Max Reitz wrote:
>>
>>  SQMP
>> -blockdev-remove-medium
>> +x-blockdev-remove-medium
>>  --
>
> Formatting nit, but not worth holding this up (as it is really our last
> chance to get it in 2.5 before baking in something we'd be stuck with).

You mean the length of the underline, right? I can fix that up
as I apply it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 02/18] slirp: Generalizing and neutralizing code before adding IPv6 stuff

[Laszlo Ersek]

meta

On 12/11/15 16:09, Thomas Huth wrote:
> On 11/12/15 15:55, Samuel Thibault wrote:
>> Thomas Huth, on Fri 11 Dec 2015 15:32:48 +0100, wrote:
>>> So maybe it's better to do smaller steps instead: Would it for example
>>> make sense to split the whole series into two parts - first a series
>>> that does all the preparation and cleanup patches. And then once that
>>> has been reviewed and merged, send the second series that adds the real
>>> new IPv6 code.
>>
>> Ok, that's what we already have: patches 1-9 are refactoring and
>> support, and 10-18 are ipv6 code.
> 
> Sounds good, ... then I'd suggest to sent the preparation patches
> separately next time and get them accepted first.

And then the next reviewer will say, "nice, but it would be even nicer
to see what *motivates* these preparatory patches!" :)

Disclaimer: I don't have any technical context for this thread; I just
noticed Samuel's email / frustration. I know that all too well, first
hand, from this list and elsewhere. I don't know how it can be fixed,
but I'm positive it is a *systemic* problem with this development model.

(I didn't contribute much value with this email, but perhaps Samuel will
feel better by seeing some (however unsolicited) confirmation; plus hey
it's Friday.)

Thanks
Laszlo

Re: [Qemu-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability

[Jan Beulich]

>>> On 11.12.15 at 15:33,  wrote:
> On Mon, 7 Dec 2015, Jan Beulich wrote:
>> >>> On 07.12.15 at 15:56,  wrote:
>> > On Mon, 7 Dec 2015, Jan Beulich wrote:
>> >> >>> On 07.12.15 at 13:45,  wrote:
>> >> > On Tue, 24 Nov 2015, Jan Beulich wrote:
>> >> >> TBD: We probably need to deal with running on an older hypervisor. I
>> >> >>  can't, however, immediately see a way for qemu to find out.
>> >> > 
>> >> > Actually QEMU has already an infrastructure to detect the hypervisor
>> >> > version at compile time, see include/hw/xen/xen_common.h. You could
>> >> > #define the right emu_mask depending on the hypervisor.
>> >> 
>> >> We don't want compile time detection here, but runtime one.
>> > 
>> > I guess the issue is that a fix was backported to Xen that changed its
>> > behaviour in past releases, right?
>> 
>> No, we shouldn't try to guess whether this is present in any pre-4.6
>> hypervisors; we should simply accept that maskable MSI is not
>> available for guests there, because ...
>> 
>> > Is there a way to detect the presence of this fix in Xen, by invoking an
>> > hypercall and checking the returned values and error numbers?
>> 
>> ... there's nothing to (reliably) probe here. This really is just an
>> implementation detail of the hypervisor, and hence a version check
>> is all we have available.
> 
> In that case, I think we should stay on the safe side, and only expose
> the masking capability (only take into effects the changes that this
> patch makes) for Xen >= 4.7.
> 
> What do you think?

That's what I suggested, with the hope of getting a hint where the
necessary infrastructure is (somehow I didn't find any run time
version dependent code to clone from).

Jan

Re: [Qemu-devel] [PATCH 08/18] slirp: Adding family argument to tcp_fconnect()

[Thomas Huth]

On 11/12/15 01:15, Samuel Thibault wrote:
> From: Guillaume Subiron 
> 
> This patch simply adds a sa_family_t argument to remove the hardcoded
> "AF_INET" in the call of qemu_socket().
> 
> Signed-off-by: Guillaume Subiron 
> Signed-off-by: Samuel Thibault 
> ---
>  slirp/slirp.h | 2 +-
>  slirp/tcp_input.c | 2 +-
>  slirp/tcp_subr.c  | 5 +++--
>  3 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/slirp/slirp.h b/slirp/slirp.h
> index 6589d7e..5b810e5 100644
> --- a/slirp/slirp.h
> +++ b/slirp/slirp.h
> @@ -332,7 +332,7 @@ void tcp_respond(struct tcpcb *, register struct tcpiphdr 
> *, register struct mbu
>  struct tcpcb * tcp_newtcpcb(struct socket *);
>  struct tcpcb * tcp_close(register struct tcpcb *);
>  void tcp_sockclosed(struct tcpcb *);
> -int tcp_fconnect(struct socket *);
> +int tcp_fconnect(struct socket *, sa_family_t af);
>  void tcp_connect(struct socket *);
>  int tcp_attach(struct socket *);
>  uint8_t tcp_tos(struct socket *);
> diff --git a/slirp/tcp_input.c b/slirp/tcp_input.c
> index 8c4fa62..079eeb9 100644
> --- a/slirp/tcp_input.c
> +++ b/slirp/tcp_input.c
> @@ -581,7 +581,7 @@ findso:
>   goto cont_input;
> }
>  
> -  if ((tcp_fconnect(so) == -1) &&
> +   if ((tcp_fconnect(so, so->so_ffamily) == -1) &&
>  #if defined(_WIN32)
>socket_error() != WSAEWOULDBLOCK
>  #else
> diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
> index 76c716f..8ec2729 100644
> --- a/slirp/tcp_subr.c
> +++ b/slirp/tcp_subr.c
> @@ -324,14 +324,15 @@ tcp_sockclosed(struct tcpcb *tp)
>   * nonblocking.  Connect returns after the SYN is sent, and does
>   * not wait for ACK+SYN.
>   */
> -int tcp_fconnect(struct socket *so)
> +int tcp_fconnect(struct socket *so, sa_family_t af)
>  {
>int ret=0;
>  
>DEBUG_CALL("tcp_fconnect");
>DEBUG_ARG("so = %p", so);
>  
> -  if( (ret = so->s = qemu_socket(AF_INET,SOCK_STREAM,0)) >= 0) {
> +  ret = so->s = qemu_socket(af, SOCK_STREAM, 0);
> +  if (ret >= 0) {
>  int opt, s=so->s;
>  struct sockaddr_storage addr;
>  

Reviewed-by: Thomas Huth 

Re: [Qemu-devel] [PATCH 02/18] slirp: Generalizing and neutralizing code before adding IPv6 stuff

[Eric Blake]

On 12/11/2015 08:40 AM, Laszlo Ersek wrote:
> meta
> 
> On 12/11/15 16:09, Thomas Huth wrote:
>> On 11/12/15 15:55, Samuel Thibault wrote:
>>> Thomas Huth, on Fri 11 Dec 2015 15:32:48 +0100, wrote:
 So maybe it's better to do smaller steps instead: Would it for example
 make sense to split the whole series into two parts - first a series
 that does all the preparation and cleanup patches. And then once that
 has been reviewed and merged, send the second series that adds the real
 new IPv6 code.
>>>
>>> Ok, that's what we already have: patches 1-9 are refactoring and
>>> support, and 10-18 are ipv6 code.
>>
>> Sounds good, ... then I'd suggest to sent the preparation patches
>> separately next time and get them accepted first.
> 
> And then the next reviewer will say, "nice, but it would be even nicer
> to see what *motivates* these preparatory patches!" :)
> 
> Disclaimer: I don't have any technical context for this thread; I just
> noticed Samuel's email / frustration. I know that all too well, first
> hand, from this list and elsewhere. I don't know how it can be fixed,
> but I'm positive it is a *systemic* problem with this development model.

The best defense you have against this is to put more information in a
commit message - any time you have split work to ease review, make sure
to mention it in the commit message.  Any time you choose to merge work
into a single patch for less churn, mention it.  The commit message is
your greatest chance of explaining to reviewers WHY you did it the way
you did; and a reasonable reviewer should be happy enough that you did
the work without making you redo it to match their 'ivory tower'
alternative approach that meets the same end.  If you changed a patch
from an earlier version due to a particular reviewer, feel free to
mention that reviewer in your explanation of changes (although in this
case, doing it in the cover letter or after the --- marker may be better
than in the commit body proper).

Yes, I've also been on the receiving end of frustration of my patch not
being perfect enough, and try to be relatively forgiving of different
styles when they aren't outright forbidden by the code base's written
standards (there's a huge difference between a patch not being
technically correct, and merely not being stylistically ideal according
to my ideals).  Also, projects that automate standards checks (such as
our scripts/checkpatch.pl) are nicer than ones that require contributors
to comply with unwritten rules - but even then you can only encode so
much into an automated checker, and still need to leave room for human
judgment on when to bend the rules.

At any rate, if you ever feel like you are being asked to do too much
churn for no real benefit, please call attention to that fact.  Burn out
is real, and suffering in silence is not beneficial to the project.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 05/10] hw/sd/sdhci.c: Update to use SDBus APIs

[Peter Maydell]

On 11 December 2015 at 19:01, Kevin O'Connor  wrote:
> On Fri, Dec 11, 2015 at 04:37:06PM +, Peter Maydell wrote:
>> Update the SDHCI code to use the new SDBus APIs.
>>
>> This commit introduces the new command line options required
>> to connect a disk to sdhci-pci:
>>
>>  -device sdhci-pci -drive id=mydrive,[...] -device sd,drive=mydrive
>
> I can't review in depth right now, but I did notice
>
> [...]
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -55,6 +55,9 @@
>>  } \
>>  } while (0)
>>
>> +#define TYPE_SDHCI_BUS "sdhci-bus"
>> +#define SDHCI_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_PXA2XX_MMCI_BUS)
>
> the above PXA2XX typo

Oops. We never actually use this macro, so I didn't notice the
cut-n-paste error.

> [...]
>> @@ -1303,6 +1323,7 @@ static void sdhci_sysbus_realize(DeviceState *dev, 
>> Error ** errp)
>>  memory_region_init_io(>iomem, OBJECT(s), _mmio_ops, s, "sdhci",
>>  SDHC_REGISTERS_MAP_SIZE);
>>  sysbus_init_mmio(sbd, >iomem);
>> +
>>  }
>
> and the above white space damage.

Will fix.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 3/4] target-tilegx: Add double floating point implementation

[Chen Gang]

On 12/11/15 05:17, Richard Henderson wrote:
> On 12/10/2015 06:15 AM, Chen Gang wrote:
>> +#define TILEGX_F_MAN_HBIT   (1ULL << 59)
> ...
>> +static uint64_t fr_to_man(float64 d)
>> +{
>> +uint64_t val = get_f64_man(d) << 7;
>> +
>> +if (get_f64_exp(d)) {
>> +val |= TILEGX_F_MAN_HBIT;
>> +}
>> +
>> +return val;
>> +}
> 
> One presumes that "HBIT" is the ieee implicit one bit.
> A better name or better comments would help there.
> 

OK, thanks. And after think of again, I guess, the real hardware does
not use HBIT internally (use the full 64 bits as mantissa without HBIT).

But what I have done is still OK (use 59 bits + 1 HBIT as mantissa), for
59 bits are enough for double mantissa (52 bits). It makes the overflow
processing easier, but has to process mul operation specially.

It we have to try to match the real hardware have done, I shall rewrite
the related code for mantissa. (I guess, we need to match the real
hardware have done).


> Do we know for sure that "7" is the correct number of guard bits?  From the 
> gcc
> implementation of floatsidf, I might guess that the correct number is "4".
> 

According to floatsidf, it seems "4", but after I expanded the bits, I
guess, it is "7".

/*
 * Double exp analyzing: (0x21b00 << 1) - 0x37(55) = 0x3ff
 *
 *   17  16  15  14  13  12  11  10   9   8   76   5   4   3   2   1   0
 *
 *1   0   0   0   0   1   1   0   1   1   00   0   0   0   0   0   0
 *
 *0   0   0   0   0   1   1   0   1   1   1=> 0x37(55)
 *
 *0   1   1   1   1   1   1   1   1   1   1=> 0x3ff
 *
 */

I guess, I need restore this comments in helper_fdouble.c.


>> +static uint32_t get_fdouble_vexp(uint64_t n)
>> +{
>> +return extract32(n, 7, 13);
>> +}
> 
> What's a "vexp"?
> 

It is exp + overflow bit + underflow bit. We can use vexp for internal
calculation, directly, and check uv and ov for the result. I guess the
real hardware will do like this.

The full description of the format is:

typedef union TileGXFPDFmtF {

struct {
uint64_t unknown0 : 7;/* unknown */

uint64_t vexp : 13;  /* vexp = exp | ov | uv */
#if 0 /* it is only the explanation for vexp above */
uint64_t exp : 11;/* exp, 0x21b << 1: 55 + TILEGX_F_EXP_DZERO */
uint64_t ov : 1;  /* overflow for mul, low priority */
uint64_t uv : 1;  /* underflow for mul, high priority */
#endif

uint64_t sign : 1;/* Sign bit for the total value */

uint64_t calc: 2; /* absolute add, sub, or mul */
uint64_t inf: 1;  /* infinit */
uint64_t nan: 1;  /* nan */

/* Come from TILE-Gx ISA document, Table 7-2 for floating point */
uint64_t unordered : 1;   /* The two are unordered */
uint64_t lt : 1;  /* 1st is less than 2nd */
uint64_t le : 1;  /* 1st is less than or equal to 2nd */
uint64_t gt : 1;  /* 1st is greater than 2nd */
uint64_t ge : 1;  /* 1st is greater than or equal to 2nd */
uint64_t eq : 1;  /* The two operands are equal */
uint64_t neq : 1; /* The two operands are not equal */

uint64_t unknown1 : 32;   /* unknown */
} fmt;
uint64_t ll;  /* only for easy using */
} TileGXFPDFmtF;


>> +uint64_t helper_fdouble_unpack_min(CPUTLGState *env,
>> +   uint64_t srca, uint64_t srcb)
>> +{
>> +uint64_t v = 0;
>> +uint32_t expa = get_f64_exp(srca);
>> +uint32_t expb = get_f64_exp(srcb);
>> +
>> +if (float64_is_any_nan(srca) || float64_is_any_nan(srcb)
>> +|| float64_is_infinity(srca) || float64_is_infinity(srcb)) {
>> +return 0;
>> +} else if (expa > expb) {
>> +if (expa - expb < 64) {
>> +set_fdouble_man(, fr_to_man(srcb) >> (expa - expb));
>> +} else {
>> +return 0;
>> +}
>> +} else if (expa < expb) {
>> +if (expb - expa < 64) {
>> +set_fdouble_man(, fr_to_man(srca) >> (expb - expa));
> 
> I very sincerely doubt that a simple right-shift is correct.  In order to
> obtain proper rounding for real computation, a sticky bit is required.  That
> is, set bit 0 if any bits are shifted out.  See the implementation of
> shift64RightJamming in fpu/softfloat-macros.h.
> 

Oh, really, thanks.


>> +uint64_t helper_fdouble_addsub(CPUTLGState *env,
>> +   uint64_t dest, uint64_t srca, uint64_t srcb)
>> +{
>> +if (get_fdouble_calc(srcb) == TILEGX_F_CALC_ADD) {
>> +return dest + srca; /* maybe set addsub overflow bit */
> 
> Definitely not.  That would be part of packing.
> 

If we need to try to match the real hardware have done, the related
implementation above is incorrect.

And for my current implementation (I guess, it should be correct):

typedef union TileGXFPDFmtV {
struct {
uint64_t mantissa : 60;   /* mantissa */

Re: [Qemu-devel] [Qemu-block] ping [PATCH v11] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host

[Eric Blake]

On 12/11/2015 04:40 PM, Programmingkid wrote:

> I guess the commit message is a little out of date. How about this:
> 
> Mac OS X can be picky when it comes to allowing the user
> to use physical devices in QEMU. Most mounted volumes
> appear to be off limits to QEMU. If an issue is detected,
> a message is displayed showing the user how to unmount a
> volume. Also new to this patch is the ability to use DVD
> disc.
> 

The use of "Also" in a commit message is often a sign of trying to do
too much in one patch.  If you are doing two distinct things (adding a
new message, vs. adding the ability to use a DVD disk), then two patches
is probably the best way to approach it.

>> Also, this patch seems garbled.  When saved via Mutt, or when I view
>> it "raw", there are '=20" and '=3D' all around, a sure sign that it is
>> (or once was) not plaintext.
>>
>> I recommend using git format-patch [1] and git send-email [1] to
>> create and send patches - it'll do the Right Thing.
> 
> You really see that? I don't know why. The link I provide to the patch in
> patchworks shows no problems. The patch itself was sent as a plain text
> file.

Your mail was sent with:

> Content-Type: text/plain; charset=us-ascii
> Content-Transfer-Encoding: quoted-printable
> Mime-Version: 1.0 (Apple Message framework v1084)

and the body indeed contains quoted-printable text, such as:

> @@ -1975,32 +1976,46 @@ BlockDriver bdrv_file =3D {
> /* host device */
> =20

Many mailers are good enough to re-render things on the fly
(Thunderbird, for example, does not have those annoying =3D and =20
insertions in the displayed text) - but what gets rendered in the mail
client is different than what the mail client saves to file.

Mails sent by 'git send-email', on the other hand, use:

> MIME-Version: 1.0
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline

Apparently, mutt doesn't quite defang a quoted-printable message
properly, and/or 'git am' doesn't take too well to a quoted-printable
message when trying to apply a saved file as a patch.  Which is all the
more reason why using the same tool as everyone else makes sure that
your patches will interoperate with the receivers' tools the same as the
receiver is already used to handling other patches.


>> Just use g_strdup().
> 
> I like snprintf() because it very well documented.

So is g_strdup().  But more important than being well-documented,
g_strdup() also has the benefit of being correctly usable with less
boilerplate - you can accomplish the same task in fewer lines of code,
and focus the reviewer on your task rather than on the boilerplate.
That, and I've seen a number of programs that incorrectly use snprintf()
(such as incorrectly sizing a buffer, or failing to check for sizing
errors after the fact).  An interface that is hard to use correctly
should be avoided in favor of one that is less mental effort.

> 
> Is this what you had in mind:
> 
> mediaType = g_strdup(matching_array[index]);

Pretty much, coupled with g_free(mediaType) later on.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 05/18] slirp: Factorizing address translation

[Samuel Thibault]

Hello,

Thomas, this is the last refactoring patch which doesn't have a review
yet, right?

Samuel

Samuel Thibault, on Fri 11 Dec 2015 01:15:17 +0100, wrote:
> From: Guillaume Subiron 
> 
> This patch factorizes some duplicate code into a new function,
> sotranslate_out(). This function perform the address translation when a
> packet is transmitted to the host network. If the paquet is destinated
> to the host, the loopback address is used, and if the paquet is
> destinated to the virtual DNS, the real DNS address is used. This code
> is just a copy of the existant, but factorized and ready to manage the
> IPv6 case.
> 
> On the same model, the major part of udp_output() code is moved into a
> new sotranslate_in(). This function is directly used in sorecvfrom(),
> like sotranslate_out() in sosendto().
> udp_output() becoming useless, it is removed and udp_output2() is
> renamed into udp_output(). This adds consistency with the udp6_output()
> function introduced by further patches.
> 
> Lastly, this factorizes some duplicate code into sotranslate_accept(), which
> performs the address translation when a connection is established on the host
> for port forwarding: if it comes from localhost, the host virtual address is
> used instead.
> 
> Signed-off-by: Guillaume Subiron 
> Signed-off-by: Samuel Thibault 
> ---
>  slirp/bootp.c|   2 +-
>  slirp/ip_icmp.c  |  19 ++---
>  slirp/socket.c   | 115 
> +--
>  slirp/socket.h   |   5 +++
>  slirp/tcp_subr.c |  35 -
>  slirp/tftp.c |   6 +--
>  slirp/udp.c  |  37 ++
>  slirp/udp.h  |   3 +-
>  8 files changed, 119 insertions(+), 103 deletions(-)
> 
> diff --git a/slirp/bootp.c b/slirp/bootp.c
> index 1baaab1..0027279 100644
> --- a/slirp/bootp.c
> +++ b/slirp/bootp.c
> @@ -325,7 +325,7 @@ static void bootp_reply(Slirp *slirp, const struct 
> bootp_t *bp)
>  
>  m->m_len = sizeof(struct bootp_t) -
>  sizeof(struct ip) - sizeof(struct udphdr);
> -udp_output2(NULL, m, , , IPTOS_LOWDELAY);
> +udp_output(NULL, m, , , IPTOS_LOWDELAY);
>  }
>  
>  void bootp_input(struct mbuf *m)
> diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
> index 58b7ceb..3a29847 100644
> --- a/slirp/ip_icmp.c
> +++ b/slirp/ip_icmp.c
> @@ -157,7 +157,7 @@ icmp_input(struct mbuf *m, int hlen)
>  goto freeit;
>  } else {
>struct socket *so;
> -  struct sockaddr_in addr;
> +  struct sockaddr_storage addr;
>if ((so = socreate(slirp)) == NULL) goto freeit;
>if (icmp_send(so, m, hlen) == 0) {
>  return;
> @@ -181,20 +181,9 @@ icmp_input(struct mbuf *m, int hlen)
>so->so_state = SS_ISFCONNECTED;
>  
>/* Send the packet */
> -  addr.sin_family = AF_INET;
> -  if ((so->so_faddr.s_addr & slirp->vnetwork_mask.s_addr) ==
> -  slirp->vnetwork_addr.s_addr) {
> - /* It's an alias */
> - if (so->so_faddr.s_addr == slirp->vnameserver_addr.s_addr) {
> -   if (get_dns_addr(_addr) < 0)
> - addr.sin_addr = loopback_addr;
> - } else {
> -   addr.sin_addr = loopback_addr;
> - }
> -  } else {
> - addr.sin_addr = so->so_faddr;
> -  }
> -  addr.sin_port = so->so_fport;
> +  addr = so->fhost.ss;
> +  sotranslate_out(so, );
> +
>if(sendto(so->s, icmp_ping_msg, strlen(icmp_ping_msg), 0,
>   (struct sockaddr *), sizeof(addr)) == -1) {
>   DEBUG_MISC((dfd,"icmp_input udp sendto tx errno = %d-%s\n",
> diff --git a/slirp/socket.c b/slirp/socket.c
> index bf603c9..97948e8 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -438,6 +438,7 @@ void
>  sorecvfrom(struct socket *so)
>  {
>   struct sockaddr_storage addr;
> + struct sockaddr_storage saddr, daddr;
>   socklen_t addrlen = sizeof(struct sockaddr_storage);
>  
>   DEBUG_CALL("sorecvfrom");
> @@ -525,11 +526,17 @@ sorecvfrom(struct socket *so)
>  
>   /*
>* If this packet was destined for CTL_ADDR,
> -  * make it look like that's where it came from, done by udp_output
> +  * make it look like that's where it came from
>*/
> + saddr = addr;
> + sotranslate_in(so, );
> + daddr = so->lhost.ss;
> +
>   switch (so->so_ffamily) {
>   case AF_INET:
> - udp_output(so, m, (struct sockaddr_in *) );
> + udp_output(so, m, (struct sockaddr_in *) ,
> +(struct sockaddr_in *) ,
> +so->so_iptos);
>   break;
>   default:
>   break;
> @@ -544,33 +551,20 @@ sorecvfrom(struct socket *so)
>  int
>  sosendto(struct socket *so, struct mbuf *m)
>  {
> - Slirp *slirp = so->slirp;
>   int ret;
> - struct sockaddr_in addr;
> + struct sockaddr_storage addr;
>  
>   DEBUG_CALL("sosendto");
>   DEBUG_ARG("so = %p", so);

Re: [Qemu-devel] [Qemu-block] ping [PATCH v11] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host

[Programmingkid]

On Dec 11, 2015, at 5:00 PM, Jeff Cody wrote:

> On Thu, Dec 10, 2015 at 09:39:51AM -0500, Programmingkid wrote:
>> https://patchwork.ozlabs.org/patch/550295/
>> 
>> Mac OS X can be picky when it comes to allowing the user
>> to use physical devices in QEMU. Most mounted volumes
>> appear to be off limits to QEMU. If an issue is detected,
>> a message is displayed showing the user how to unmount a
>> volume.
>> 
> 
> This commit message doesn't seem to really match the patch; it is more
> than just a message displayed to the user.  For instance, before it
> looked for just kIOCDMediaClass - now, it searches for kIOCDMediaClass
> and kIODVDMediaClass.

I guess the commit message is a little out of date. How about this:

Mac OS X can be picky when it comes to allowing the user
to use physical devices in QEMU. Most mounted volumes
appear to be off limits to QEMU. If an issue is detected,
a message is displayed showing the user how to unmount a
volume. Also new to this patch is the ability to use DVD
disc.

> 
>> Signed-off-by: John Arbuckle 
>> 
>> ---
>> error_report()'s had their \n, '.', and "Error:" removed.
>> Indentations are now at the left parenthesis rather than
>> at the 80 character mark.
>> Added spaces between the + sign.
>> 
> 
> Also, this patch seems garbled.  When saved via Mutt, or when I view
> it "raw", there are '=20" and '=3D' all around, a sure sign that it is
> (or once was) not plaintext.
> 
> I recommend using git format-patch [1] and git send-email [1] to
> create and send patches - it'll do the Right Thing.

You really see that? I don't know why. The link I provide to the patch in
patchworks shows no problems. The patch itself was sent as a plain text
file.

> 
>> block/raw-posix.c |  135 
>> +++--
>> 1 files changed, 99 insertions(+), 36 deletions(-)
>> 
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index ccfec1c..39e523b 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -43,6 +43,7 @@
>> #include 
>> #include 
>> //#include 
>> +#include 
>> #include 
>> #endif
>> 
>> @@ -1975,32 +1976,46 @@ BlockDriver bdrv_file = {
>> /* host device */
>> 
>> #if defined(__APPLE__) && defined(__MACH__)
>> -static kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator );
>> static kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
>> CFIndex maxPathSize, int flags);
>> -kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator )
>> +static kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator,
>> +   char *mediaType)
>> {
>> kern_return_t   kernResult;
>> mach_port_t masterPort;
>> CFMutableDictionaryRef  classesToMatch;
>> +const char *matching_array[] = {kIODVDMediaClass, kIOCDMediaClass};
>> 
>> kernResult = IOMasterPort( MACH_PORT_NULL,  );
>> if ( KERN_SUCCESS != kernResult ) {
>> printf( "IOMasterPort returned %d\n", kernResult );
>> }
>> 
>> -classesToMatch = IOServiceMatching( kIOCDMediaClass );
>> -if ( classesToMatch == NULL ) {
>> -printf( "IOServiceMatching returned a NULL dictionary.\n" );
>> -} else {
>> -CFDictionarySetValue( classesToMatch, CFSTR( kIOMediaEjectableKey ), 
>> kCFBooleanTrue );
>> -}
>> -kernResult = IOServiceGetMatchingServices( masterPort, classesToMatch, 
>> mediaIterator );
>> -if ( KERN_SUCCESS != kernResult )
>> -{
>> -printf( "IOServiceGetMatchingServices returned %d\n", kernResult );
>> -}
>> +int index;
>> +for (index = 0; index < ARRAY_SIZE(matching_array); index++) {
>> +classesToMatch = IOServiceMatching(matching_array[index]);
>> +if (classesToMatch == NULL) {
>> +error_report("IOServiceMatching returned NULL for %s",
>> + matching_array[index]);
>> +continue;
>> +}
>> +CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
>> + kCFBooleanTrue);
>> +kernResult = IOServiceGetMatchingServices(masterPort, 
>> classesToMatch,
>> +  mediaIterator);
>> +if (kernResult != KERN_SUCCESS) {
>> +error_report("Note: IOServiceGetMatchingServices returned %d",
>> + kernResult);
>> +}
>> 
>> +/* If a match was found, leave the loop */
>> +if (*mediaIterator != 0) {
> 
> Since mediaIterator was not ever initialized in hdev_open, we can't
> assume the value of mediaIterator as being 0 after kernResult !=
> KERN_SUCCESS, can we?  A quick google search [3] shows it ambiguous, so
> best initialize mediaIterator down below...

Sounds like a good suggestion.

> 
> 
>> +DPRINTF("Matching using %s\n", matching_array[index]);
>> +snprintf(mediaType, strlen(matching_array[index]) + 1, 

Re: [Qemu-devel] [PATCH] target-*: Get rid of "PC advancement" trick

[Richard Henderson]

On 12/10/2015 10:47 AM, Sergey Fedorov wrote:

The "PC advancement" trick was used just after recognizing that a
breakpoint exception was going to be generated. This trick has had two
points:
  1. Guarantee that tb->size isn't zero: there are many places where it's
 expected to be non-zero. In fact, that is even stated in the comment
 for this field.
  2. Try to satisfy disassembler's check for instruction length. To this
 end, PC advancement was done for estimated instruction length, but
 actually, didn't work properly in variable-instruction-length cases.

Substitute this trick with checking for TB size at the end of
translation. If we get an empty TB then just set tb->size to 1 and skip
disassembling. Setting tb->size to 1 is enough to get correct behaviour,
whereas an empty TB doesn't obviously need to be disassembled.


This doesn't help when the TB already has instructions, the TB would ordinarily 
cross a page boundary, and the breakpoint is at the page boundary.



r~