Re: [Qemu-devel] [PATCH v8 07/17] qapi: Rework collision assertions

[Markus Armbruster]

Eric Blake  writes:

> On 11/02/2015 08:37 AM, Markus Armbruster wrote:
>
>> 
>> Not checked: variant's members don't collide with non-variant members.
>> I think this check got lost when we simplified
>> QAPISchemaObjectTypeVariants to hold a single member.
>
> Yep, I found the culprit: in your v2 proposal for QAPISchema, you had:
>
> +class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
> +def __init__(self, name, typ, flat):
> +QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
> +assert isinstance(flat, bool)
> +self.flat = flat
> +def check(self, schema, tag_type, seen):
> +QAPISchemaObjectTypeMember.check(self, schema, [], seen)
> +assert self.name in tag_type.values
> +if self.flat:
> +self.type.check(schema)
> +assert isinstance(self.type, QAPISchemaObjectType)
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg00394.html
>
> but the 'if self.flat' clause was lost in v3:
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg00450.html

Quoting v3's change log:

  - Lower simple variants to flat ones as described on
qapi-code-gen.txt.  Replace QAPISchemaObjectType's .flat by
.simple_union_type(), for use by pre-existing code-generation
warts.

> I am in fact reinstating it here, but for v9, will do it in a separate
> patch rather than blended in with the rest of the changes.

Any "is this union flat or simple" check signals a flaw.  It's either a
pointless difference in generated code (these should all be marked TODO
by now), or something's wrong with the desugaring of simple to flat
unions.

In this case, it looks like a collision check was lost when I switched
from "simple unions are its own separate type" to "simple unions are
sugar for flat unions".

Reinstating it makes sense, except for the "if self.flat" part.  If the
check's correct for flat unions, it must be correct for desugared simple
unions, or else we screwed up the desugaring.

For a genuine flat union case

'tag-value': 'FlatVariant'

self.type is the object type 'FlatVariant'.  Its members are the case's
variant members, and they can clash with the flat union's non-variant
members.

For a desugared simple union case

'tag-value': 'SimpleVariant'

self.type is an object type with a single member 'data' of type
'SimpleVariant'.  Its member 'data' is the case's only variant member,
and it can clash with the simple union's non-variant members.  In theory
only, because the only non-variant member is the implicit tag, and it's
called 'type'.

Therefore, the if self.flat is superfluous.  Good, because otherwise our
desugaring must be flawed.

> [wow - we've been hammering at this since July?]

First RFC was in April, but we started hammering in earnest only in
summer.

Re: [Qemu-devel] [PATCH 0/2] block: Introduce "json-file:" protocol

[Fam Zheng]

On Tue, 11/03 07:35, Daniel P. Berrange wrote:
> On Tue, Nov 03, 2015 at 03:01:16PM +0800, Fam Zheng wrote:
> > This would be a safer channel when we want to provide block options that
> > contain sensitive information, such as fields for authentication.
> 
> If passing of security sensitive data is the motivation for this,
> then I don't think we want it really. This approach merely avoids
> the config being visible in the process listing, which is really just
> one problem. When people file bugs they are going to need to provide
> the block device config, and that still going to contain sensitive
> info and thus leak. Similarly apps generating block config like libvirt
> are still going to be logging the block config they generate, which is
> again going to leak secrets. Finally, we need the ability to pass
> security sensitive data to QEMU in many other areas besides block devices
> so need a more general mechanism
> 
> I've proposed a way to provide secrets to QEMU in a way that is
> usable across all QEMU backends, that I think is a much better
> approach because it totally decouples the sensitive data from
> the rest of the config data, rather than having it inline
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg04365.html
> 

Apparently I've overlooked your post, thanks for pointing out. I think your
solution covers everything this series has to do.

Fam

Re: [Qemu-devel] [PATCH 0/2] block: Introduce "json-file:" protocol

[Daniel P. Berrange]

On Tue, Nov 03, 2015 at 03:01:16PM +0800, Fam Zheng wrote:
> This would be a safer channel when we want to provide block options that
> contain sensitive information, such as fields for authentication.

If passing of security sensitive data is the motivation for this,
then I don't think we want it really. This approach merely avoids
the config being visible in the process listing, which is really just
one problem. When people file bugs they are going to need to provide
the block device config, and that still going to contain sensitive
info and thus leak. Similarly apps generating block config like libvirt
are still going to be logging the block config they generate, which is
again going to leak secrets. Finally, we need the ability to pass
security sensitive data to QEMU in many other areas besides block devices
so need a more general mechanism

I've proposed a way to provide secrets to QEMU in a way that is
usable across all QEMU backends, that I think is a much better
approach because it totally decouples the sensitive data from
the rest of the config data, rather than having it inline

  https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg04365.html

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

Re: [Qemu-devel] RFC: libyajl for JSON

[Markus Armbruster]

Eric Blake  writes:

> On 11/02/2015 12:10 PM, Markus Armbruster wrote:
>
 * Single-quoted strings
>
>> 
>>> So that is an absolute must for whatever parser we choose.  My first
>>> glance at libyajl is that it does NOT currently allow single quotes (not
>>> even with any of its existing options), so we'd have to pre-process
>>> input before feeding it to yajl.
>
>>>
>>> I'll ask on the yajl mailing lists, to get a feel for community
>>> receptiveness, before attempting to actually write patches on that front.
>> 
>> Makes sense.
>
> On IRC, I got pointed to a couple of forks that have already done this,
> such as:
>
> https://github.com/ludocode/yajl/commits/master
>
> although the upstream author Lloyd was not on the channel at the time.
> Meanwhile, there's 47 open issues on the upstream repo:
>
> https://github.com/lloyd/yajl/issues
>
> which implies slow response by the author, but at least he WAS asking if
> anyone would like to help him with maintenance:
>
> https://github.com/lloyd/yajl/issues/161
>
> |  lloyd commented on Sep 24
> | ok, give me a minute to hand you a patch on a branch to review.
>
> |  lloyd commented on Sep 24
> | ok, I'll merge down and roll a new release within a day.
>
> [still hasn't happened yet...]
>
> |  lamont-granquist commented on Sep 25
> | hey @lloyd would you consider adding other maintainers?
>
> |  lloyd commented on Oct 1
> | I would absolutely consider additional maintainers. Who's interested?
>
> so I don't know what the future holds for extending things upstream.
>
> To date, I don't have a github account, by conscious personal choice; so
> I can't really take him up on that offer directly.  So far, I've been
> trying the mailing list to see if he answers that in addition to the
> github PR stream, but the archives show it to be pretty full of spam:
> http://librelist.com/browser/yajl/

I don't do github, either.

>>> Yes; the yajl parser has 4 modes of parse operation based on which of
>>> three callbacks you implement: double-only (yajl_double), long long-only
>>> (yajl_integer), double-and-int (both yajl_double and yajl_integer, not
>>> sure which one has precedence if input satisfies both formats), or
>>> custom number (yajl_number, which is given a string, and you turn it
>>> into the format of your choice).  Likewise, the yajl formatter has 2
>>> functions: yajl_gen_double() (formats doubles) and yajl_gen_number()
>>> (you provide literal strings formatted how you like).
>> 
>> Good.
>
> And one of the open bugs on the tracker was the same one we've
> encountered ourselves: yajl is locale-sensitive when using strtod() for
> parsing and when using printf() for printing doubles:
>
> https://github.com/lloyd/yajl/issues/79
>
> [I would love for C/POSIX to add strtod_l and printf_l, but that's a
> thread for another day]
>
>> 
 More extensions or pitfalls might be lurking in our parser.  Therefore,
 replacing our parser by a suitable library is not without risk.  I guess
 we could do it over a full development cycle.  No way for 2.5.

>>>
>>> Absolutely not for 2.5; we've missed softfreeze, and this would count as
>>> a new feature.
>
> Another interesting bug report against yajl: one reporter made the point
> [1] that yajl is already a superset of the canonical RFC 4627 definition
> of JSON [2], because while the RFC states that a JSON document has this
> terminal state:
>   JSON-text = object / array
> yajl will accept _any_ JSON value (not just objects/arrays) as the
> overall document.
>
> [1] https://github.com/lloyd/yajl/issues/154
> [2] https://www.ietf.org/rfc/rfc4627.txt

I believe our parser does the same.

> So at this point, I want to see if lloyd makes any progress towards an
> actual yajl release and/or adding a co-maintainer, before even trying to
> get formal upstream support for single quoting.  We could always create
> a git submodule with our own choice of fork (since there are already
> forks that do single-quote parsing) - but the mantra of 'upstream first'
> has a lot of merit (I'm reluctant to fork without good reason).

The value proposition of replacing our flawed JSON parser isn't in
saving big on maintenance, it's in not having to find and fix its flaws.

If the replacement needs a lot of work to fit our needs, the value
proposition becomes negative.

A JSON parser shouldn't require much maintenance, as JSON is simple,
doesn't change, and parsing has few system dependencies.

That said, maintaining fork of an unresponsive upstream is always
awkward.

Wait and see how yajl makes progress sounds sensible to me.  However, I
already hate the idea of releasing 2.5 with the colossal memory waste
unfixed.  I'm willing to sit on my hands for 2.5 only because it's not a
regression.  I'm unwilling to leave it unfixed much longer.

Point is, the longer we wait, the more we'll get invested in our own
parser.

Re: [Qemu-devel] [PATCH 00/19] buffer/vnc: improve vnc buffer hsndling

[Peter Lieven]

Am 30.10.2015 um 12:09 schrieb Gerd Hoffmann:

   Hi,

This series has a bunch of improvements in the vnc buffer handling,
to reduce the memory footprint.  Some of the changes are applied to
the buffer helper functions which Daniel separated out of the vnc code
recently.

Most patches have been on the list before, based on a older version of
Daniel's "separate out buffer code" patches.  Now I finally managed to
rebase and adapt the changes to the latest version which landed in
master meanwhile.  I don't expect major issues showing up here and plan
to have a pull request with this in time for 2.5-rc0.

Peter, if you have anything pending not yet in here please rebase and
resend.

please review,
   Gerd

Gerd Hoffmann (14):
   buffer: add buffer_init
   buffer: add buffer_move_empty
   buffer: add buffer_move
   buffer: add buffer_shrink
   buffer: add tracing
   vnc: attach names to buffers
   vnc: kill jobs queue buffer
   vnc-jobs: move buffer reset, use new buffer move
   vnc: zap dead code
   vnc: add vnc_width+vnc_height helpers
   vnc: factor out vnc_update_server_surface
   vnc: use vnc_{width,height} in vnc_set_area_dirty
   vnc: only alloc server surface with clients connected
   vnc: fix local state init

All above: Reviewed-by: Peter Lieven 



Peter Lieven (5):
   buffer: make the Buffer capacity increase in powers of two
   vnc: recycle empty vs->output buffer
   buffer: factor out buffer_req_size
   buffer: factor out buffer_adj_size
   buffer: allow a buffer to shrink gracefully


The last Patch isn't the latest version. I have one with improved comments here:

https://github.com/plieven/qemu/commit/e599748ab1ef381d4b1c88bf1ea1454dd89353fb

I also had another improvement:

https://github.com/plieven/qemu/commit/2b4180a5f4ec29a59de692e9aa512b7b4d8023e7

which limits the number of memmove operation in qio_buffer_advance.

Peter

Re: [Qemu-devel] [PATCH 19/19] buffer: allow a buffer to shrink gracefully

[Peter Lieven]

Am 30.10.2015 um 12:10 schrieb Gerd Hoffmann:

From: Peter Lieven 

the idea behind this patch is to allow the buffer to shrink, but
make this a seldom operation. The buffers average size is measured
exponentionally smoothed with am alpha of 1/128.

Signed-off-by: Peter Lieven 
Signed-off-by: Gerd Hoffmann 
---
  include/qemu/buffer.h |  1 +
  util/buffer.c | 34 --
  2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/include/qemu/buffer.h b/include/qemu/buffer.h
index 0a69b3a..dead9b7 100644
--- a/include/qemu/buffer.h
+++ b/include/qemu/buffer.h
@@ -37,6 +37,7 @@ struct Buffer {
  char *name;
  size_t capacity;
  size_t offset;
+uint64_t avg_size;
  uint8_t *buffer;
  };
  
diff --git a/util/buffer.c b/util/buffer.c

index fe5a44e..5461f86 100644
--- a/util/buffer.c
+++ b/util/buffer.c
@@ -23,6 +23,7 @@
  
  #define BUFFER_MIN_INIT_SIZE 4096

  #define BUFFER_MIN_SHRINK_SIZE  65536
+#define BUFFER_AVG_SIZE_SHIFT   7
  
  static size_t buffer_req_size(Buffer *buffer, size_t len)

  {
@@ -37,6 +38,11 @@ static void buffer_adj_size(Buffer *buffer, size_t len)
  buffer->buffer = g_realloc(buffer->buffer, buffer->capacity);
  trace_buffer_resize(buffer->name ?: "unnamed",
  old, buffer->capacity);
+
+/* make it even harder for the buffer to shrink, reset average size
+ * to currenty capacity if it is larger than the average. */
+buffer->avg_size = MAX(buffer->avg_size,
+   buffer->capacity << BUFFER_AVG_SIZE_SHIFT);
  }
  
  void buffer_init(Buffer *buffer, const char *name, ...)

@@ -48,16 +54,30 @@ void buffer_init(Buffer *buffer, const char *name, ...)
  va_end(ap);
  }
  
+static uint64_t buffer_get_avg_size(Buffer *buffer)

+{
+return buffer->avg_size >> BUFFER_AVG_SIZE_SHIFT;
+}
+
  void buffer_shrink(Buffer *buffer)
  {
-/*
- * Only shrink in case the used size is *much* smaller than the
- * capacity, to avoid bumping up & down the buffers all the time.
+size_t new;
+
+/* Calculate the average size of the buffer as
+ * avg_size = avg_size * ( 1 - a ) + required_size * a
+ * where a is 1 / 2 ^ QIO_BUFFER_AVG_SIZE_SHIFT. */
+buffer->avg_size *= (1 << BUFFER_AVG_SIZE_SHIFT) - 1;
+buffer->avg_size >>= BUFFER_AVG_SIZE_SHIFT;
+buffer->avg_size += buffer_req_size(buffer, 0);
+
+/* And then only shrink if the average size of the buffer is much
+ * too big, to avoid bumping up & down the buffers all the time.
   * realloc() isn't exactly cheap ...
   */
-if (buffer->offset < (buffer->capacity >> 3) &&
-buffer->capacity > BUFFER_MIN_SHRINK_SIZE) {
-return;
+new = buffer_req_size(buffer, buffer_get_avg_size(buffer));
+if (new < buffer->capacity >> 3 &&
+new >= BUFFER_MIN_SHRINK_SIZE) {
+buffer_adj_size(buffer, buffer_get_avg_size(buffer));
  }
  
  buffer_adj_size(buffer, 0);

@@ -83,6 +103,7 @@ uint8_t *buffer_end(Buffer *buffer)
  void buffer_reset(Buffer *buffer)
  {
  buffer->offset = 0;
+buffer_shrink(buffer);
  }
  
  void buffer_free(Buffer *buffer)

@@ -107,6 +128,7 @@ void buffer_advance(Buffer *buffer, size_t len)
  memmove(buffer->buffer, buffer->buffer + len,
  (buffer->offset - len));
  buffer->offset -= len;
+buffer_shrink(buffer);
  }
  
  void buffer_move_empty(Buffer *to, Buffer *from)


This isn't the last version of this patch. Please have a look at:

https://github.com/plieven/qemu/commit/e599748ab1ef381d4b1c88bf1ea1454dd89353fb

Peter

Re: [Qemu-devel] [PATCH] qom/object: fix 2 comment typos

[Cao jin]

On 11/02/2015 07:34 PM, Peter Maydell wrote:

On 2 November 2015 at 09:04, Cao jin  wrote:

Signed-off-by: Cao jin 
---
  include/qom/object.h | 2 +-
  qom/object.c | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index be7280c..9e5162a 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -511,7 +511,7 @@ struct TypeInfo
  /**
   * OBJECT_CLASS_CHECK:
   * @class: The C type to use for the return value.
- * @obj: A derivative of @type to cast.
+ * @obj: A derivative class of @class to cast.
   * @name: the QOM typename of @class.


I think this is right but not sufficient. Since OBJECT_CLASS_CHECK's
second parameter is not an object but a class, the name "obj" is
rather misleading and should be changed. My suggestion would be
to use

  * @classtype: The C type to use for the return value.
  * @class: A derivative class of @classtype to cast.
  * @name: the QOM typename of @class.

(and the macro definition parameter names should change to match).



Yes, Agree. The comment seems just a copy of macro OBJECT_CHECK`s. Will 
modify it in v2:)



   *
   * A type safe version of @object_class_dynamic_cast_assert.  This macro is
diff --git a/qom/object.c b/qom/object.c
index 11cd86b..fc6e161 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -204,7 +204,7 @@ static bool type_is_ancestor(TypeImpl *type, TypeImpl 
*target_type)
  {
  assert(target_type);

-/* Check if typename is a direct ancestor of type */
+/* Check if target_type is a direct ancestor of type */
  while (type) {
  if (type == target_type) {
  return true;


This change is good.

thanks
-- PMM



--
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 1/4] ide/atapi: make PIO read requests async

[Peter Lieven]

Am 03.11.2015 um 01:48 schrieb John Snow:


On 10/12/2015 08:27 AM, Peter Lieven wrote:

PIO read requests on the ATAPI interface used to be sync blk requests.
This has two significant drawbacks. First the main loop hangs util an
I/O request is completed and secondly if the I/O request does not
complete (e.g. due to an unresponsive storage) Qemu hangs completely.

Signed-off-by: Peter Lieven 
---
  hw/ide/atapi.c | 93 --
  1 file changed, 84 insertions(+), 9 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 747f466..2271ea2 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -105,11 +105,16 @@ static void cd_data_to_raw(uint8_t *buf, int lba)
  memset(buf, 0, 288);
  }
  
-static int cd_read_sector(IDEState *s, int lba, uint8_t *buf, int sector_size)

+static int
+cd_read_sector_sync(IDEState *s, int lba, uint8_t *buf)
  {
  int ret;
  
-switch(sector_size) {

+#ifdef DEBUG_IDE_ATAPI
+printf("cd_read_sector_sync: lba=%d\n", lba);
+#endif
+
+switch (s->cd_sector_size) {
  case 2048:
  block_acct_start(blk_get_stats(s->blk), &s->acct,
   4 * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ);
@@ -129,9 +134,71 @@ static int cd_read_sector(IDEState *s, int lba, uint8_t 
*buf, int sector_size)
  ret = -EIO;
  break;
  }
+
+if (!ret) {
+s->lba++;
+s->io_buffer_index = 0;
+}
+
  return ret;
  }
  
+static void cd_read_sector_cb(void *opaque, int ret)

+{
+IDEState *s = opaque;
+
+block_acct_done(blk_get_stats(s->blk), &s->acct);
+
+#ifdef DEBUG_IDE_ATAPI
+printf("cd_read_sector_cb: lba=%d ret=%d\n", s->lba, ret);
+#endif
+
+if (ret < 0) {
+ide_atapi_io_error(s, ret);
+return;
+}
+
+if (s->cd_sector_size == 2352) {
+cd_data_to_raw(s->io_buffer, s->lba);
+}
+
+s->lba++;
+s->io_buffer_index = 0;
+s->status &= ~BUSY_STAT;
+
+ide_atapi_cmd_reply_end(s);
+}
+
+static int cd_read_sector(IDEState *s, int lba, void *buf)
+{
+if (s->cd_sector_size != 2048 && s->cd_sector_size != 2352) {
+return -EINVAL;
+}
+
+s->iov.iov_base = buf;
+if (s->cd_sector_size == 2352) {
+buf += 16;
+}
+
+s->iov.iov_len = 4 * BDRV_SECTOR_SIZE;
+qemu_iovec_init_external(&s->qiov, &s->iov, 1);
+
+#ifdef DEBUG_IDE_ATAPI
+printf("cd_read_sector: lba=%d\n", lba);
+#endif
+
+if (blk_aio_readv(s->blk, (int64_t)lba << 2, &s->qiov, 4,
+  cd_read_sector_cb, s) == NULL) {
+return -EIO;
+}
+
+block_acct_start(blk_get_stats(s->blk), &s->acct,
+ 4 * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ);
+
+s->status |= BUSY_STAT;
+return 0;
+}
+
  void ide_atapi_cmd_ok(IDEState *s)
  {
  s->error = 0;
@@ -182,18 +249,27 @@ void ide_atapi_cmd_reply_end(IDEState *s)
  ide_atapi_cmd_ok(s);
  ide_set_irq(s->bus);
  #ifdef DEBUG_IDE_ATAPI
-printf("status=0x%x\n", s->status);
+printf("end of transfer, status=0x%x\n", s->status);
  #endif
  } else {
  /* see if a new sector must be read */
  if (s->lba != -1 && s->io_buffer_index >= s->cd_sector_size) {
-ret = cd_read_sector(s, s->lba, s->io_buffer, s->cd_sector_size);
-if (ret < 0) {
-ide_atapi_io_error(s, ret);
+if (!s->elementary_transfer_size) {
+ret = cd_read_sector(s, s->lba, s->io_buffer);
+if (ret < 0) {
+ide_atapi_io_error(s, ret);
+}
  return;
+} else {
+/* rebuffering within an elementary transfer is
+ * only possible with a sync request because we
+ * end up with a race condition otherwise */
+ret = cd_read_sector_sync(s, s->lba, s->io_buffer);
+if (ret < 0) {
+ide_atapi_io_error(s, ret);
+return;
+}
  }
-s->lba++;
-s->io_buffer_index = 0;
  }
  if (s->elementary_transfer_size > 0) {
  /* there are some data left to transmit in this elementary
@@ -275,7 +351,6 @@ static void ide_atapi_cmd_read_pio(IDEState *s, int lba, 
int nb_sectors,
  s->io_buffer_index = sector_size;
  s->cd_sector_size = sector_size;
  
-s->status = READY_STAT | SEEK_STAT;

  ide_atapi_cmd_reply_end(s);
  }
  


This patch looks good to me, apart from Stefan's other comments. Will
you be sending a V3? I can try to merge it for this week before the hard
freeze hits.


I will look at this today.

Peter

[Qemu-devel] [PATCH v2] qom/object: fix 2 comment typos

[Cao jin]

Also change the misleading definition of macro OBJECT_CLASS_CHECK

Signed-off-by: Cao jin 
---
changelog v2:
modified according to peter`s suggestion

 include/qom/object.h | 10 +-
 qom/object.c |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index be7280c..0bb89d4 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -510,16 +510,16 @@ struct TypeInfo
 
 /**
  * OBJECT_CLASS_CHECK:
- * @class: The C type to use for the return value.
- * @obj: A derivative of @type to cast.
- * @name: the QOM typename of @class.
+ * @class_type: The C type to use for the return value.
+ * @class: A derivative class of @class_type to cast.
+ * @name: the QOM typename of @class_type.
  *
  * A type safe version of @object_class_dynamic_cast_assert.  This macro is
  * typically wrapped by each type to perform type safe casts of a class to a
  * specific class type.
  */
-#define OBJECT_CLASS_CHECK(class, obj, name) \
-((class *)object_class_dynamic_cast_assert(OBJECT_CLASS(obj), (name), \
+#define OBJECT_CLASS_CHECK(class_type, class, name) \
+((class_type *)object_class_dynamic_cast_assert(OBJECT_CLASS(class), 
(name), \
__FILE__, __LINE__, __func__))
 
 /**
diff --git a/qom/object.c b/qom/object.c
index 11cd86b..fc6e161 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -204,7 +204,7 @@ static bool type_is_ancestor(TypeImpl *type, TypeImpl 
*target_type)
 {
 assert(target_type);
 
-/* Check if typename is a direct ancestor of type */
+/* Check if target_type is a direct ancestor of type */
 while (type) {
 if (type == target_type) {
 return true;
-- 
2.1.0

[Qemu-devel] [PATCH 0/2] block: Introduce "json-file:" protocol

[Fam Zheng]

This would be a safer channel when we want to provide block options that
contain sensitive information, such as fields for authentication.



Fam Zheng (2):
  block: Add "json-file:" pseudo protocol
  iotests: Add tests for "json-file:" pseudo protocol

 block.c| 58 +-
 tests/qemu-iotests/089 | 27 +
 tests/qemu-iotests/089.out | 15 
 3 files changed, 90 insertions(+), 10 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/2] block: Add "json-file:" pseudo protocol

[Fam Zheng]

When opening the image, "json-file:/path/to/json/file" has the same as
"json-file:$(cat /path/to/json/file)". The advantage is that sensitive
information that would be exposed in /proc/$PID/cmdline or log files,
is no longer visible this way.

Signed-off-by: Fam Zheng 
---
 block.c | 58 --
 1 file changed, 52 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index e9f40dc..785f317 100644
--- a/block.c
+++ b/block.c
@@ -962,10 +962,6 @@ static QDict *parse_json_filename(const char *filename, 
Error **errp)
 {
 QObject *options_obj;
 QDict *options;
-int ret;
-
-ret = strstart(filename, "json:", &filename);
-assert(ret);
 
 options_obj = qobject_from_json(filename);
 if (!options_obj) {
@@ -985,6 +981,36 @@ static QDict *parse_json_filename(const char *filename, 
Error **errp)
 return options;
 }
 
+static char *read_json_file(const char *filename, Error **errp)
+{
+BlockDriverState *file = NULL;
+QDict *options;
+int r;
+int64_t sectors;
+char *buf;
+
+options = qdict_new();
+qdict_put(options, "driver",
+  qstring_from_str("raw"));
+r = bdrv_open(&file, filename, NULL, options, 0, errp);
+if (r) {
+return NULL;
+}
+sectors = DIV_ROUND_UP(bdrv_getlength(file), BDRV_SECTOR_SIZE);
+if (sectors > (1024 * 1024 / BDRV_SECTOR_SIZE)) {
+error_setg(errp, "JSON file is too large");
+return NULL;
+}
+buf = g_malloc0(sectors << BDRV_SECTOR_BITS);
+r = bdrv_read(file, 0, (uint8_t *)buf, sectors);
+if (r) {
+error_setg(errp, "Failed to read JSON file");
+g_free(buf);
+return NULL;
+}
+return buf;
+}
+
 /*
  * Fills in default options for opening images and converts the legacy
  * filename/flags pair to option QDict entries.
@@ -1002,8 +1028,28 @@ static int bdrv_fill_options(QDict **options, const char 
**pfilename,
 Error *local_err = NULL;
 
 /* Parse json: pseudo-protocol */
-if (filename && g_str_has_prefix(filename, "json:")) {
-QDict *json_options = parse_json_filename(filename, &local_err);
+if (filename &&
+(g_str_has_prefix(filename, "json:") ||
+ g_str_has_prefix(filename, "json-file:"))) {
+char *json_str;
+QDict *json_options;
+int ret;
+
+if (g_str_has_prefix(filename, "json-file:")) {
+strstart(filename, "json-file:", &filename);
+json_str = read_json_file(filename, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return -EIO;
+}
+} else {
+ret = strstart(filename, "json:", &filename);
+assert(ret);
+
+json_str = g_strdup(filename);
+}
+json_options = parse_json_filename(json_str, &local_err);
+g_free(json_str);
 if (local_err) {
 error_propagate(errp, local_err);
 return -EINVAL;
-- 
2.4.3

[Qemu-devel] [PATCH 2/2] iotests: Add tests for "json-file:" pseudo protocol

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/089 | 27 +++
 tests/qemu-iotests/089.out | 15 +++
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/089 b/tests/qemu-iotests/089
index 3e0038d..6115563 100755
--- a/tests/qemu-iotests/089
+++ b/tests/qemu-iotests/089
@@ -67,9 +67,7 @@ $QEMU_IO -c 'write -P 42 0 512' -c 'write -P 23 512 512' \
 
 $QEMU_IMG convert -f raw -O $IMGFMT "$TEST_IMG.base" "$TEST_IMG"
 
-$QEMU_IO_PROG --cache $CACHEMODE \
- -c 'read -P 42 0 512' -c 'read -P 23 512 512' \
- -c 'read -P 66 1024 512' "json:{
+img_opts="{
 \"driver\": \"$IMGFMT\",
 \"file\": {
 \"driver\": \"$IMGFMT\",
@@ -77,7 +75,15 @@ $QEMU_IO_PROG --cache $CACHEMODE \
 \"filename\": \"$TEST_IMG\"
 }
 }
-}" | _filter_qemu_io
+}"
+
+echo "$img_opts" > $TEST_IMG.json
+
+for f in "json:$img_opts" "json-file:$TEST_IMG.json"; do
+$QEMU_IO_PROG --cache $CACHEMODE \
+ -c 'read -P 42 0 512' -c 'read -P 23 512 512' \
+ -c 'read -P 66 1024 512' "$f" | _filter_qemu_io
+done
 
 # This should fail (see test 072)
 $QEMU_IO -c 'read -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
@@ -126,6 +132,19 @@ $QEMU_IO -c "open -o driver=qcow2 
json:{\"file.filename\":\"$TEST_IMG\"}" \
 $QEMU_IO -c "open -o driver=qcow2 
json:{\"driver\":\"raw\",\"file.filename\":\"$TEST_IMG\"}" \
  -c "info" 2>&1 | _filter_testdir | _filter_imgfmt
 
+echo
+echo "=== Test invalid json-file: ==="
+echo
+
+echo "This is not a valid json file#^*)@" > $TEST_IMG.json
+TEST_IMG="json-file:$TEST_IMG.json" _img_info
+
+echo
+echo "=== Test large json-file: ==="
+echo
+
+$QEMU_IMG create $TEST_IMG.json 1G
+TEST_IMG="json-file:$TEST_IMG.json" _img_info
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/089.out b/tests/qemu-iotests/089.out
index 5b541a3..6847f60 100644
--- a/tests/qemu-iotests/089.out
+++ b/tests/qemu-iotests/089.out
@@ -15,6 +15,12 @@ read 512/512 bytes at offset 512
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 512/512 bytes at offset 1024
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 1024
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 Pattern verification failed at offset 0, 512 bytes
 read 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -53,4 +59,13 @@ Format specific information:
 lazy refcounts: false
 refcount bits: 16
 corrupt: false
+
+=== Test invalid json-file: ===
+
+qemu-img: Could not open 'json-TEST_DIR/t.IMGFMT.json': Could not parse the 
JSON options
+
+=== Test large json-file: ===
+
+Formatting '/home/fam/build/last/tests/qemu-iotests/scratch/t.qcow2.json', 
fmt=raw size=1073741824
+qemu-img: Could not open 'json-TEST_DIR/t.IMGFMT.json': JSON file is too large
 *** done
-- 
2.4.3

Re: [Qemu-devel] Status Buildbot

[Timo Benk]

On Monday 02 November 2015 17:29:44 Stefan Hajnoczi wrote:
> On Mon, Nov 02, 2015 at 10:37:06AM +0100, Timo Benk wrote:
> > we at B1 Systems GmbH are currently hosting the buildbot infrastructure. I 
> > have mailed some
> > former contacts and it seems that this service is no longer needed anymore.
> > 
> > Can you confirm that?
> 
> It's me again :).
> 
> I announced the end of buildbot usage during QEMU Summit 2015.  The
> meeting notes are here:
> https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg01048.html

Ok, i will shutdown the buildbot servers.

Can you remove the wiki pages at http://wiki.qemu.org/Buildbot as well?

Greetings,
-timo

-- 
Timo Benk
Linux / Unix Consultant & Trainer
Tel.: +49-178-8066638
Mail: b...@b1-systems.de

B1 Systems GmbH
Osterfeldstraße 7 / 85088 Vohburg / http://www.b1-systems.de
GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt,HRB 3537


signature.asc
Description: This is a digitally signed message part.

Re: [Qemu-devel] [PATCH v1 1/4] qemu-iotests: refine common.config

[tu bo]

Hi Eric:

thanks for your review.

On 10/31/2015 01:36 AM, Eric Blake wrote:

On 10/30/2015 01:13 AM, Bo Tu wrote:

Be easier to read, and be slightly shorter.

You mentioned a very short "what" in the subject line (good), but the
"why" in the commit body ("easier to read, shorter") is rather terse and
subjective.  It would be nicer to go into details (change the definition
of default_alias_machine) or give a sample of what changes (use grep
instead of awk).


I agree with you. Adding more details as below,
Replacing sed with awk, then it's easier to read.
Replacing "[ ! -z "$default_alias_machine" ]" with "[[ $default_alias_machine 
]]",
then it's slightly shorter.



[meta-comment]

When sending a series, please include a 0/4 cover letter.  You may want
to do:
git config format.coverLetter auto
to make it automatic when using git format-patch/send-email.


My understanding is that cover letter is needed if the patch set is a little 
bit complicated.
Cover letter is not needed if patch set just has minor change and comment is 
already in the
git message for every patch.
If my understanding above is wrong, please correct me. I just hope to be more 
clear about process :-)


Suggested-By: Sascha Silbe 
Reviewed-by: Sascha Silbe 
Signed-off-by: Bo Tu 
---
  tests/qemu-iotests/common.config | 7 +++
  1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index 596bb2b..0a165e8 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -129,10 +129,9 @@ export QEMU_IO=_qemu_io_wrapper
  export QEMU_NBD=_qemu_nbd_wrapper
  
  default_machine=$($QEMU -machine \? | awk '/(default)/{print $1}')

-default_alias_machine=$($QEMU -machine \? |\
-awk -v var_default_machine="$default_machine"\)\
-'{if ($(NF-2)=="(alias"&&$(NF-1)=="of"&&$(NF)==var_default_machine){print 
$1}}')
-if [ ! -z "$default_alias_machine" ]; then
+default_alias_machine=$($QEMU -machine \? \

As long as we are touching this, we ought to switch to using '-machine
help' rather than the deprecated '-machine \?'.


thanks to point this.




+| grep -F "(alias of ${default_machine})" |cut -d ' ' -f 1 |head -n 1)

Why are you moving the | across lines?


thanks to point this



If we are rewriting to avoid awk, why not do it with a single sed
process, rather than a grep|cut|head pipeline?  For that matter, why not
rewrite default_machine to also avoid awk?


The goal is not to avoid awk. The line of default_machine is easy to read, but 
the
line of default_alias_machine as below is not easy to read, that's why Sascha
suggest me to change it for the line of default_alias_machine.

/-default_alias_machine=$($QEMU -machine \? |\ -awk -v 
var_default_machine="$default_machine"\)\ -'{if 
($(NF-2)=="(alias"&&$(NF-1)=="of"&&$(NF)==var_default_machine){print $1}}')/




default_machine=$($QEMU -machine help | sed -n '/(default)/ s/ .*//p')
default_alias_machine=$($QEMU -machine help | \
   sed -n "/(alias of $default_machine)"' { s/ .*//p; q; }')

(which happens to work even if $default_machine contains '.', but might
get a bit dicey if the machine names could ever contain ?, [, *, or
other regex metacharacters)


I like the single sed process. However, I got error message as below after 
running it,
/sed: -e expression #1, char 38: unknown command: `.' /I guess you missed a '/' 
which is marked as red as below, So I change it as below,
/default_machine=$($QEMU -machine help | sed -n '/(default)/ s/ .*//p') 
default_alias_machine=$($QEMU -machine help | \ sed -n "/(alias of 
$default_machine)"/' { s/ .*//p; q; }') /





+if [ -n "$default_alias_machine" ]; then

Could shorten this to:

if [[ $default_alias_machine ]]; then


thanks to point this.



since we are already using bash.

Re: [Qemu-devel] [PATCH v7 03/35] acpi: add aml_create_field

[Shannon Zhao]

On 2015/11/2 17:13, Xiao Guangrong wrote:
> Implement CreateField term which is used by NVDIMM _DSM method in later patch
> 
> Signed-off-by: Xiao Guangrong 
> ---
>  hw/acpi/aml-build.c | 13 +
>  include/hw/acpi/aml-build.h |  1 +
>  2 files changed, 14 insertions(+)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index a72214d..9fe5e7b 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1151,6 +1151,19 @@ Aml *aml_sizeof(Aml *arg)
>  return var;
>  }
>  
> +/* ACPI 1.0b: 16.2.5.2 Named Objects Encoding: DefCreateField */
> +Aml *aml_create_field(Aml *srcbuf, Aml *index, Aml *len, const char *name)
> +{
> +Aml *var = aml_alloc();
> +build_append_byte(var->buf, 0x5B); /* ExtOpPrefix */
> +build_append_byte(var->buf, 0x13); /* CreateFieldOp */
> +aml_append(var, srcbuf);
> +aml_append(var, index);
> +aml_append(var, len);
> +build_append_namestring(var->buf, "%s", name);
> +return var;
> +}
> +
>  void
>  build_header(GArray *linker, GArray *table_data,
>   AcpiTableHeader *h, const char *sig, int len, uint8_t rev)
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 7296efb..7e1c43b 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -276,6 +276,7 @@ Aml *aml_touuid(const char *uuid);
>  Aml *aml_unicode(const char *str);
>  Aml *aml_derefof(Aml *arg);
>  Aml *aml_sizeof(Aml *arg);
> +Aml *aml_create_field(Aml *srcbuf, Aml *index, Aml *len, const char *name);
>  
Maybe this could be moved together with existing aml_create_dword_field.

>  void
>  build_header(GArray *linker, GArray *table_data,
> 

-- 
Shannon

Re: [Qemu-devel] [RFC PATCH 0/5] Introduce Intel 82574 GbE Controller Emulation (e1000e)

[Jason Wang]

On 11/02/2015 03:49 PM, Dmitry Fleytman wrote:
>
>> On 2 Nov 2015, at 05:35 AM, Jason Wang > > wrote:
>>
>>
>>
>> On 10/31/2015 01:52 PM, Dmitry Fleytman wrote:
>>> Hello Jason,
>>>
>>> Thanks for reviewing. See my answers inline.
>>>
>>>
 On 30 Oct 2015, at 07:28 AM, Jason Wang >>> 
 > wrote:



 On 10/28/2015 01:44 PM, Jason Wang wrote:
>
> On 10/26/2015 01:00 AM, Leonid Bloch wrote:
>> Hello qemu-devel,
>>
>> This patch series is an RFC for the new networking device emulation
>> we're developing for QEMU.
>>
>> This new device emulates the Intel 82574 GbE Controller and works
>> with unmodified Intel e1000e drivers from the Linux/Windows kernels.
>>
>> The status of the current series is "Functional Device Ready, work
>> on Extended Features in Progress".
>>
>> More precisely, these patches represent a functional device, which
>> is recognized by the standard Intel drivers, and is able to transfer
>> TX/RX packets with CSO/TSO offloads, according to the spec.
>>
>> Extended features not supported yet (work in progress):
>> 1. TX/RX Interrupt moderation mechanisms
>> 2. RSS
>> 3. Full-featured multi-queue (use of multiqueued network backend)
>>
>> Also, there will be some code refactoring and performance
>> optimization efforts.
>>
>> This series was tested on Linux (Fedora 22) and Windows (2012R2)
>> guests, using Iperf, with TX/RX and TCP/UDP streams, and various
>> packet sizes.
>>
>> More thorough testing, including data streams with different MTU
>> sizes, and Microsoft Certification (HLK) tests, are pending missing
>> features' development.
>>
>> See commit messages (esp. "net: Introduce e1000e device emulation")
>> for more information about the development approaches and the
>> architecture options chosen for this device.
>>
>> This series is based upon v2.3.0 tag of the upstream QEMU repository,
>> and it will be rebased to latest before the final submission.
>>
>> Please share your thoughts - any feedback is highly welcomed :)
>>
>> Best Regards,
>> Dmitry Fleytman.
> Thanks for the series. Will go through this in next few days.

 Have a quick glance at the series, got the following questions:

 - Though e1000e differs from e1000 in many places, I still see lots of
 code duplications. We need consider to reuse e1000.c (or at least part
 of). I believe we don't want to fix a bug twice in two places in the
 future and I expect hundreds of lines could be saved through this way.
>>>
>>> That’s a good question :)
>>>
>>> This is how we started, we had a common “core” code base meant to
>>> implement all common logic (this split is still present in the patches
>>> - there are e1000e_core.c and e1000e.c files).
>>> Unfortunately at some point it turned out that there are more
>>> differences that commons. We noticed that the code becomes filled with
>>> many minor differences handling.
>>> This also made the code base more complicated and harder to follow.
>>>
>>> So at some point of time it was decided to split the code base and
>>> revert all changes done to the e1000 device (except a few
>>> fixes/improvements Leonid submitted a few days ago).
>>>
>>> Although there was common code between devices, total SLOC of e1000
>>> and e1000e devices became smaller after the split.
>>>
>>> Amount of code that may be shared between devices will be even smaller
>>> after we complete the implementation which still misses a few features
>>> (see cover letter) that will change many things.
>>>
>>> Still after the device implementation is done, we plan to review code
>>> similarities again to see if there are possibilities for code sharing.
>>
>> I see, but if we can try to re-use or unify the codes from beginning, it
>> would be a little bit easier. Looks like the differences were mainly:
>>
>> 1) MSI-X support
>> 2) offloading support through virtio-net header
>> 3) trace points
>> 4) other new functions through e1000e specific registers
>>
>> So we could first unify the code through implementing the support of 2
>> and 3 for e1000. For MSI-X and other e1000e specific new functions, it
>> could be done through:
>>
>> 1) model specific callbacks, e.g realize, transmission and reception
>> 2) A new register flags e.g PHY_RW_E1000E which means the register is
>> for e1000e only. Or even model specific wirteops and readops
>> 3) For other subtle differences, it could be done in the code by
>> checking the model explicitly.
>>
>> What's your opinion? (A good example of code sharing is freebsd's e1000
>> driver which covers both 8254x and 8257x).
>
>
> Hi Jason,
>
> This is exactly how we started.
>
> Issues that made us split the code base were following:
>
> 1. The majority of registers are different. Ev

Re: [Qemu-devel] [PATCH qemu] pseries: Update SLOF firmware image to qemu-slof-20151103

[Alexey Kardashevskiy]

On 11/03/2015 01:22 PM, Alexey Kardashevskiy wrote:

The changes are:
1. supports recent binutils;
2. 64bit BARs behind PCI bridges supported;
3. Many fixes for USB keyboard support - keys, XHCI;
4. virtio-vga support.



Forgot mention that this binary was build using:

gcc version 5.2.1 20151001 (GCC)
GNU ld (GNU Binutils) 2.25.51.20150930

This might explain why the patch is so big.




The full changelog is:
   > version: update to 20151103
   > documentation: Add a clause about signing off
   > qemu/js2x/client: Support binutils >= 2.25.1
   > Fix special keys on USB
   > Fix function keys on USB
   > pci-scan: program 64-bit mem bar range in pci-bridge bar
   > Allow to build SLOF on Little Endian host
   > usb-xhci: add keyboard support
   > usb-xhci: ready the link trb early
   > usb-xhci: scan usb high speed ports
   > usb-xhci: bulk improve event handling loop
   > usb-xhci: return on allocation failure
   > usb-xhci: add delay in shutdown path
   > usb-xhci: event trbs does not need link trb
   > usb-hid: refactor usb key reading
   > takeover: Fix header includes
   > board-js2x: Add missing file dma-function.fs
   > vga: Add support for virtio-vga
   > qemu-vga: Use MMIO BAR instead of legacy IO ports
   > slof: Change call_c() function to a proper assembler function

Signed-off-by: Alexey Kardashevskiy 
---
  pc-bios/README   |   2 +-
  pc-bios/slof.bin | Bin 915584 -> 913256 bytes
  roms/SLOF|   2 +-
  3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/pc-bios/README b/pc-bios/README
index e4154ab..d260c1b 100644
--- a/pc-bios/README
+++ b/pc-bios/README
@@ -17,7 +17,7 @@
  - SLOF (Slimline Open Firmware) is a free IEEE 1275 Open Firmware
implementation for certain IBM POWER hardware.  The sources are at
https://github.com/aik/SLOF, and the image currently in qemu is
-  built from git tag qemu-slof-20150813.
+  built from git tag qemu-slof-20151103.

  - sgabios (the Serial Graphics Adapter option ROM) provides a means for
legacy x86 software to communicate with an attached serial console as




--
Alexey

Re: [Qemu-devel] [Qemu-arm] [PATCH] ARM: ACPI: Fix MPIDR value in ACPI table

[Peter Crosthwaite]

On Sat, Oct 31, 2015 at 5:04 PM, Peter Maydell  wrote:
> On 31 October 2015 at 18:53, Peter Crosthwaite
>  wrote:
>> On Sat, Oct 31, 2015 at 2:50 AM, Shannon Zhao  
>> wrote:
>>> From: Shannon Zhao 
>>>
>>> Use mp_affinity of ARMCPU as the CPU MPIDR instead of the CPU index.
>>>
>>> Signed-off-by: Shannon Zhao 
>>> ---
>>> This patch is based on below patch.
>>> http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg06919.html
>>>
>>>  hw/arm/virt-acpi-build.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index 29a1980..1c621cb 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -451,13 +451,15 @@ build_madt(GArray *table_data, GArray *linker, 
>>> VirtGuestInfo *guest_info,
>>>  for (i = 0; i < guest_info->smp_cpus; i++) {
>>>  AcpiMadtGenericInterrupt *gicc = acpi_data_push(table_data,
>>>   sizeof *gicc);
>>> +ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
>>> +
>>>  gicc->type = ACPI_APIC_GENERIC_INTERRUPT;
>>>  gicc->length = sizeof(*gicc);
>>>  if (guest_info->gic_version == 2) {
>>>  gicc->base_address = memmap[VIRT_GIC_CPU].base;
>>>  }
>>>  gicc->cpu_interface_number = i;
>>> -gicc->arm_mpidr = i;
>>> +gicc->arm_mpidr = armcpu->mp_affinity;
>>
>> As a general rule, hw/ should not be reaching into the CPU state
>> struct like this. What is the real HW equivalent of this query?
>
> This is just doing the same thing the hw/arm/virt.c code does
> to populate the dt... (In real firmware it would presumably
> either (a) have a fixed ACPI table or (b) maybe read the MPIDR,
> but neither of those really fits here I think.)

So, I think this is just another case of the MPIDR information flow
going the wrong way. It should go from board to all of CPU, DT and now
this. I guess we can just fix this incrementally when we fix the
implicit setting of MPIDR in mach-virt.

Regards,
Peter

>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH for-2.5 v3 1/1] hw/misc: Add support for ADC controller in Xilinx Zynq 7000

[Guenter Roeck]

On 11/02/2015 08:25 PM, Peter Crosthwaite wrote:

From: Guenter Roeck 

Add support for the Xilinx XADC core used in Zynq 7000.


Hi Peter,

Wow ... thanks for doing my job!

Owe you a beer or two.

Guenter


References:
- Zynq-7000 All Programmable SoC Technical Reference Manual
- 7 Series FPGAs and Zynq-7000 All Programmable SoC XADC
   Dual 12-Bit 1 MSPS Analog-to-Digital Converter

Tested with Linux using QEMU machine xilinx-zynq-a9 with devicetree
files zynq-zc702.dtb and zynq-zc706.dtb, and kernel configuration
multi_v7_defconfig.

Signed-off-by: Guenter Roeck 
[ PC changes:
   * Changed macro names to match TRM where possible
   * Made programmers model macro scheme consistent
   * Dropped XADC_ZYNQ_ prefix on local macros
   * Fix ALM field width
   * Update threshold-comparison interrupts in _update_ints()
   * factored out DFIFO pushes into helper. Renamed to "push/pop"
   * Changed xadc_reg to 10 bits and added OOB check.
   * Reduced scope of MCTL reset to just stop channel coms.
   * Added dummy read data to write commands
   * Changed _ to - seperators in string names and filenames
   * Dropped  in header comment
   * Catchall'ed _update_ints() in _write handler.
   * Minor whitespace changes.
]
Signed-off-by: Peter Crosthwaite 
---
v3:
 See [PC changes] in commit message
v2:
 Use extract32()
 Merge zynq_xadc_reset() and _zynq_xadc_reset() into one function
 Use "xlnx,zynq_xadc"
 Move device model to include/hw/misc/zynq_xadc.h
 irq -> qemu_irq
 xadc_dfifo_depth -> xadc_dfifo_entries
 Dropped unnecessary comments
 Merged zynq_xadc_realize() into zynq_xadc_init()

  hw/arm/xilinx_zynq.c|   6 +
  hw/misc/Makefile.objs   |   1 +
  hw/misc/zynq-xadc.c | 301 
  include/hw/misc/zynq-xadc.h |  46 +++
  4 files changed, 354 insertions(+)
  create mode 100644 hw/misc/zynq-xadc.c
  create mode 100644 include/hw/misc/zynq-xadc.h

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 82a9db8..1c1a445 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -24,6 +24,7 @@
  #include "hw/block/flash.h"
  #include "sysemu/block-backend.h"
  #include "hw/loader.h"
+#include "hw/misc/zynq-xadc.h"
  #include "hw/ssi.h"
  #include "qemu/error-report.h"

@@ -264,6 +265,11 @@ static void zynq_init(MachineState *machine)
  sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xE0101000);
  sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[79-IRQ_OFFSET]);

+dev = qdev_create(NULL, TYPE_ZYNQ_XADC);
+qdev_init_nofail(dev);
+sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8007100);
+sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[39-IRQ_OFFSET]);
+
  dev = qdev_create(NULL, "pl330");
  qdev_prop_set_uint8(dev, "num_chnls",  8);
  qdev_prop_set_uint8(dev, "num_periph_req",  4);
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 4aa76ff..aeb6b7d 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -36,6 +36,7 @@ obj-$(CONFIG_OMAP) += omap_sdrc.o
  obj-$(CONFIG_OMAP) += omap_tap.o
  obj-$(CONFIG_SLAVIO) += slavio_misc.o
  obj-$(CONFIG_ZYNQ) += zynq_slcr.o
+obj-$(CONFIG_ZYNQ) += zynq-xadc.o
  obj-$(CONFIG_STM32F2XX_SYSCFG) += stm32f2xx_syscfg.o

  obj-$(CONFIG_PVPANIC) += pvpanic.o
diff --git a/hw/misc/zynq-xadc.c b/hw/misc/zynq-xadc.c
new file mode 100644
index 000..ba86056
--- /dev/null
+++ b/hw/misc/zynq-xadc.c
@@ -0,0 +1,301 @@
+/*
+ * ADC registers for Xilinx Zynq Platform
+ *
+ * Copyright (c) 2015 Guenter Roeck
+ * Based on hw/misc/zynq_slcr.c, written by Michal Simek
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see .
+ */
+
+#include "hw/hw.h"
+#include "hw/misc/zynq-xadc.h"
+#include "qemu/timer.h"
+#include "sysemu/sysemu.h"
+
+enum {
+CFG= 0x000 / 4,
+INT_STS,
+INT_MASK,
+MSTS,
+CMDFIFO,
+RDFIFO,
+MCTL,
+};
+
+#define CFG_ENABLE  BIT(31)
+#define CFG_CFIFOTH_SHIFT   20
+#define CFG_CFIFOTH_LENGTH  4
+#define CFG_DFIFOTH_SHIFT   16
+#define CFG_DFIFOTH_LENGTH  4
+#define CFG_WEDGE   BIT(13)
+#define CFG_REDGE   BIT(12)
+#define CFG_TCKRATE_SHIFT   8
+#define CFG_TCKRATE_LENGTH  2
+
+#define CFG_TCKRATE_DIV(x)  (0x1 << (x - 1))
+
+#define CFG_IGAP_SHIFT  0
+#define CFG_IGAP_LENGTH 5
+
+#define INT_CFIFO_LTH   BIT(9)
+#define INT_DFIFO_GTH   BIT(8)
+#define INT_OT  BIT(7)
+#define INT_ALM_SHIFT   0
+#define INT_ALM_LENGTH  7
+#define INT_ALM_MASK(((1 << INT_ALM_LENGTH) - 1) << INT_ALM_SHIFT)
+
+#define INT_AL

[Qemu-devel] [PATCH for-2.5 v3 2/3] arm: highbank: Defeature CPU override

[Peter Crosthwaite]

This board should not support CPU model override. This allows for
easier patching of the board with being able to rely on the CPU
type being correct.

Signed-off-by: Peter Crosthwaite 
---

 hw/arm/highbank.c | 21 +++--
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index be04b27..f2e248b 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -223,15 +223,13 @@ static void calxeda_init(MachineState *machine, enum 
cxmachines machine_id)
 MemoryRegion *sysmem;
 char *sysboot_filename;
 
-if (!cpu_model) {
-switch (machine_id) {
-case CALXEDA_HIGHBANK:
-cpu_model = "cortex-a9";
-break;
-case CALXEDA_MIDWAY:
-cpu_model = "cortex-a15";
-break;
-}
+switch (machine_id) {
+case CALXEDA_HIGHBANK:
+cpu_model = "cortex-a9";
+break;
+case CALXEDA_MIDWAY:
+cpu_model = "cortex-a15";
+break;
 }
 
 for (n = 0; n < smp_cpus; n++) {
@@ -240,11 +238,6 @@ static void calxeda_init(MachineState *machine, enum 
cxmachines machine_id)
 ARMCPU *cpu;
 Error *err = NULL;
 
-if (!oc) {
-error_report("Unable to find CPU definition");
-exit(1);
-}
-
 cpuobj = object_new(object_class_get_name(oc));
 cpu = ARM_CPU(cpuobj);
 
-- 
1.9.1

[Qemu-devel] [PATCH for-2.5 v3 1/3] arm: boot: Add secure_board_setup flag

[Peter Crosthwaite]

Add a flag that when set, will cause the primary CPU to start in secure
mode, even if the overall boot is non-secure. This is useful for when
there is a board-setup blob that needs to run from secure mode, but
device and secondary CPU init should still be done as-normal for a non-
secure boot.

Signed-off-by: Peter Crosthwaite 
---
changed since v2:
Assert if running KVM and board_setup_secure is set

 hw/arm/boot.c| 8 +++-
 include/hw/arm/arm.h | 6 ++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index b0879a5..f671454 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -495,7 +495,8 @@ static void do_cpu_reset(void *opaque)
 }
 
 /* Set to non-secure if not a secure boot */
-if (!info->secure_boot) {
+if (!info->secure_boot &&
+(cs != first_cpu || !info->secure_board_setup)) {
 /* Linux expects non-secure state */
 env->cp15.scr_el3 |= SCR_NS;
 }
@@ -598,6 +599,11 @@ static void arm_load_kernel_notify(Notifier *notifier, 
void *data)
 struct arm_boot_info *info =
 container_of(n, struct arm_boot_info, load_kernel_notifier);
 
+/* It is the boards job to make sure secure_board_setup is actually
+ * possible
+ */
+assert(!info->secure_board_setup || tcg_enabled());
+
 /* Load the kernel.  */
 if (!info->kernel_filename || info->firmware_loaded) {
 
diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index 9217b70..60dc919 100644
--- a/include/hw/arm/arm.h
+++ b/include/hw/arm/arm.h
@@ -97,6 +97,12 @@ struct arm_boot_info {
 hwaddr board_setup_addr;
 void (*write_board_setup)(ARMCPU *cpu,
   const struct arm_boot_info *info);
+
+/* If set, the board specific loader/setup blob will be run from secure
+ * mode, regardless of secure_boot. The blob becomes responsible for
+ * changing to non-secure state if implementing a non-secure boot
+ */
+bool secure_board_setup;
 };
 
 /**
-- 
1.9.1

[Qemu-devel] [PATCH for-2.5 v3 3/3] arm: highbank: Implement PSCI and dummy monitor

[Peter Crosthwaite]

Firstly, enable monitor mode and PSCI, both are which are features of
this board.

In addition to PSCI, this board also uses SMC for cache maintainence
ops. This means we need a secure monitor to catch these and nop them.
Use the ARM boot board-setup feature to implement this. All traps to
monitor mode implement the nop.

As a KVM CPU cannot run in secure mode, do not do the board-setup if
not running TCG. Report a warning explaining the limitation is this
case.

Signed-off-by: Peter Crosthwaite 
---
Changed since v2:
Change to spinlock/movs trap table and drop fallthrough (PMM review)
Do not do board-setup if KVM is enabled. Issue a warning.
Changed since v1:
fallthrough all of trap table to nop implementation
use movw for table address
leave loader at 0
Move MVBAR (and blob to non-zero)
Split nop implementation from MVBAR setup
set secure boot for board
implement NS switch in blob
Changed since RFC:
Use bootloader callback to load blob.
Change "firmware" to "board-setup" for consistency.
Tweak commit message.

 hw/arm/highbank.c | 69 +++
 1 file changed, 59 insertions(+), 10 deletions(-)

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index f2e248b..30e8054 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -32,10 +32,52 @@
 #define SMP_BOOT_REG0x40
 #define MPCORE_PERIPHBASE   0xfff1
 
+#define MVBAR_ADDR  0x200
+
 #define NIRQ_GIC160
 
 /* Board init.  */
 
+/* MVBAR_ADDR is limited by precision of movw */
+
+QEMU_BUILD_BUG_ON(MVBAR_ADDR >= (1 << 16));
+
+#define ARMV7_IMM16(x) (extract32((x),  0, 12) | \
+extract32((x), 12,  4) << 16)
+
+static void hb_write_board_setup(ARMCPU *cpu,
+ const struct arm_boot_info *info)
+{
+int n;
+uint32_t board_setup_blob[] = {
+/* MVBAR_ADDR */
+/* Default unimplemented and unused vectors to spin. Makes it
+ * easier to debug (as opposed to the CPU running away).
+ */
+0xeafe, /* notused1: b notused */
+0xeafe, /* notused2: b notused */
+0xe1b0f00e, /* smc: movs pc, lr - exception return */
+0xeafe, /* prefetch_abort: b prefetch_abort */
+0xeafe, /* data_abort: b data_abort */
+0xeafe, /* notused3: b notused3 */
+0xeafe, /* irq: b irq */
+0xeafe, /* fiq: b fiq */
+#define BOARD_SETUP_ADDR (MVBAR_ADDR + 8 * sizeof(uint32_t))
+0xe300 + ARMV7_IMM16(MVBAR_ADDR), /* movw r0, MVBAR_ADDR */
+0xee0c0f30, /* mcr p15, 0, r0, c12, c0, 1 - set MVBAR */
+0xee110f11, /* mrc p15, 0, r0, c1 , c1, 0 - get SCR */
+0xe3810001, /* orr r0, #1 - set NS */
+0xee010f11, /* mcr p15, 0, r0, c1 , c1, 0 - set SCR */
+0xe1600070, /* smc - go to monitor mode to flush NS change */
+0xe12fff1e, /* bx lr - return to caller */
+};
+for (n = 0; n < ARRAY_SIZE(board_setup_blob); n++) {
+board_setup_blob[n] = tswap32(board_setup_blob[n]);
+}
+rom_add_blob_fixed("board-setup", board_setup_blob,
+   sizeof(board_setup_blob), MVBAR_ADDR);
+}
+
 static void hb_write_secondary(ARMCPU *cpu, const struct arm_boot_info *info)
 {
 int n;
@@ -241,16 +283,13 @@ static void calxeda_init(MachineState *machine, enum 
cxmachines machine_id)
 cpuobj = object_new(object_class_get_name(oc));
 cpu = ARM_CPU(cpuobj);
 
-/* By default A9 and A15 CPUs have EL3 enabled.  This board does not
- * currently support EL3 so the CPU EL3 property is disabled before
- * realization.
- */
-if (object_property_find(cpuobj, "has_el3", NULL)) {
-object_property_set_bool(cpuobj, false, "has_el3", &err);
-if (err) {
-error_report_err(err);
-exit(1);
-}
+object_property_set_int(cpuobj, QEMU_PSCI_CONDUIT_SMC,
+"psci-conduit", &error_abort);
+
+if (n) {
+/* Secondary CPUs start in PSCI powered-down state */
+object_property_set_bool(cpuobj, true,
+ "start-powered-off", &error_abort);
 }
 
 if (object_property_find(cpuobj, "reset-cbar", NULL)) {
@@ -371,6 +410,16 @@ static void calxeda_init(MachineState *machine, enum 
cxmachines machine_id)
 highbank_binfo.loader_start = 0;
 highbank_binfo.write_secondary_boot = hb_write_secondary;
 highbank_binfo.secondary_cpu_reset_hook = hb_reset_secondary;
+if (tcg_enabled()) {
+highbank_binfo.board_setup_addr = BOARD_SETUP_ADDR;
+highbank_binfo.write_board_setup = hb_write_board_setup;
+highbank_binfo.secure_board_setup = true;
+} else {
+error_report("WARNING: TCG unavailable - "
+ "unable to load built-in Monitor support.\n"
+ "Some guests (such as Linux) may not boot\n

[Qemu-devel] [PATCH for-2.5 v3 0/3] ARM: Highbank: Add monitor support

[Peter Crosthwaite]

Hi,

This adds dummy monitor support to the Highbank board. It is needed by
the Highbank kernel which expects a monitor to be present.

A feature is added to arm/boot's board_setup feature, that allows the
board_setup entry point to be entered in secure mode (which is needed
to configure a monitor).

This feature does not play nice with -cpu override, but cpu override
is not valid for well-defined ARM SoCs. So defeature -cpu for Highbank.

Regards,
Peter

See indiv. patches for detailed change logs.

Changed since v2:
Defeature -cpu for Highbank (new patch)
Rework board_setup blob implementation (PMM review)
Conditionalise feature on TCG
Dropped initial board_setup and Zynq patches (Merged)
Changed since v1:
Addressed PMM review.
Added secure_board_setup flag (P4)
Added Zynq patch first, then Highbank


Peter Crosthwaite (3):
  arm: boot: Add secure_board_setup flag
  arm: highbank: Defeature CPU override
  arm: highbank: Implement PSCI and dummy monitor

 hw/arm/boot.c|  8 -
 hw/arm/highbank.c| 90 ++--
 include/hw/arm/arm.h |  6 
 3 files changed, 79 insertions(+), 25 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH for-2.5 v3 1/1] hw/misc: Add support for ADC controller in Xilinx Zynq 7000

[Peter Crosthwaite]

From: Guenter Roeck 

Add support for the Xilinx XADC core used in Zynq 7000.

References:
- Zynq-7000 All Programmable SoC Technical Reference Manual
- 7 Series FPGAs and Zynq-7000 All Programmable SoC XADC
  Dual 12-Bit 1 MSPS Analog-to-Digital Converter

Tested with Linux using QEMU machine xilinx-zynq-a9 with devicetree
files zynq-zc702.dtb and zynq-zc706.dtb, and kernel configuration
multi_v7_defconfig.

Signed-off-by: Guenter Roeck 
[ PC changes:
  * Changed macro names to match TRM where possible
  * Made programmers model macro scheme consistent
  * Dropped XADC_ZYNQ_ prefix on local macros
  * Fix ALM field width
  * Update threshold-comparison interrupts in _update_ints()
  * factored out DFIFO pushes into helper. Renamed to "push/pop"
  * Changed xadc_reg to 10 bits and added OOB check.
  * Reduced scope of MCTL reset to just stop channel coms.
  * Added dummy read data to write commands
  * Changed _ to - seperators in string names and filenames
  * Dropped  in header comment
  * Catchall'ed _update_ints() in _write handler.
  * Minor whitespace changes.
]
Signed-off-by: Peter Crosthwaite 
---
v3:
See [PC changes] in commit message
v2:
Use extract32()
Merge zynq_xadc_reset() and _zynq_xadc_reset() into one function
Use "xlnx,zynq_xadc"
Move device model to include/hw/misc/zynq_xadc.h
irq -> qemu_irq
xadc_dfifo_depth -> xadc_dfifo_entries
Dropped unnecessary comments
Merged zynq_xadc_realize() into zynq_xadc_init()

 hw/arm/xilinx_zynq.c|   6 +
 hw/misc/Makefile.objs   |   1 +
 hw/misc/zynq-xadc.c | 301 
 include/hw/misc/zynq-xadc.h |  46 +++
 4 files changed, 354 insertions(+)
 create mode 100644 hw/misc/zynq-xadc.c
 create mode 100644 include/hw/misc/zynq-xadc.h

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 82a9db8..1c1a445 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -24,6 +24,7 @@
 #include "hw/block/flash.h"
 #include "sysemu/block-backend.h"
 #include "hw/loader.h"
+#include "hw/misc/zynq-xadc.h"
 #include "hw/ssi.h"
 #include "qemu/error-report.h"
 
@@ -264,6 +265,11 @@ static void zynq_init(MachineState *machine)
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xE0101000);
 sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[79-IRQ_OFFSET]);
 
+dev = qdev_create(NULL, TYPE_ZYNQ_XADC);
+qdev_init_nofail(dev);
+sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8007100);
+sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[39-IRQ_OFFSET]);
+
 dev = qdev_create(NULL, "pl330");
 qdev_prop_set_uint8(dev, "num_chnls",  8);
 qdev_prop_set_uint8(dev, "num_periph_req",  4);
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 4aa76ff..aeb6b7d 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -36,6 +36,7 @@ obj-$(CONFIG_OMAP) += omap_sdrc.o
 obj-$(CONFIG_OMAP) += omap_tap.o
 obj-$(CONFIG_SLAVIO) += slavio_misc.o
 obj-$(CONFIG_ZYNQ) += zynq_slcr.o
+obj-$(CONFIG_ZYNQ) += zynq-xadc.o
 obj-$(CONFIG_STM32F2XX_SYSCFG) += stm32f2xx_syscfg.o
 
 obj-$(CONFIG_PVPANIC) += pvpanic.o
diff --git a/hw/misc/zynq-xadc.c b/hw/misc/zynq-xadc.c
new file mode 100644
index 000..ba86056
--- /dev/null
+++ b/hw/misc/zynq-xadc.c
@@ -0,0 +1,301 @@
+/*
+ * ADC registers for Xilinx Zynq Platform
+ *
+ * Copyright (c) 2015 Guenter Roeck
+ * Based on hw/misc/zynq_slcr.c, written by Michal Simek
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see .
+ */
+
+#include "hw/hw.h"
+#include "hw/misc/zynq-xadc.h"
+#include "qemu/timer.h"
+#include "sysemu/sysemu.h"
+
+enum {
+CFG= 0x000 / 4,
+INT_STS,
+INT_MASK,
+MSTS,
+CMDFIFO,
+RDFIFO,
+MCTL,
+};
+
+#define CFG_ENABLE  BIT(31)
+#define CFG_CFIFOTH_SHIFT   20
+#define CFG_CFIFOTH_LENGTH  4
+#define CFG_DFIFOTH_SHIFT   16
+#define CFG_DFIFOTH_LENGTH  4
+#define CFG_WEDGE   BIT(13)
+#define CFG_REDGE   BIT(12)
+#define CFG_TCKRATE_SHIFT   8
+#define CFG_TCKRATE_LENGTH  2
+
+#define CFG_TCKRATE_DIV(x)  (0x1 << (x - 1))
+
+#define CFG_IGAP_SHIFT  0
+#define CFG_IGAP_LENGTH 5
+
+#define INT_CFIFO_LTH   BIT(9)
+#define INT_DFIFO_GTH   BIT(8)
+#define INT_OT  BIT(7)
+#define INT_ALM_SHIFT   0
+#define INT_ALM_LENGTH  7
+#define INT_ALM_MASK(((1 << INT_ALM_LENGTH) - 1) << INT_ALM_SHIFT)
+
+#define INT_ALL (INT_CFIFO_LTH | INT_DFIFO_GTH | INT_OT | INT_ALM_MASK)
+
+#define MSTS_CFIFO_LVL_SHIFT16
+#define MSTS_CFIFO_LVL_LENGTH   4
+#define MSTS_DFIFO_LVL_SHIFT12
+#defi

Re: [Qemu-devel] [PATCH v7 09/35] exec: allow file_ram_alloc to work on file

[Xiao Guangrong]

On 11/03/2015 05:12 AM, Paolo Bonzini wrote:



On 02/11/2015 10:13, Xiao Guangrong wrote:

Currently, file_ram_alloc() only works on directory - it creates a file
under @path and do mmap on it

This patch tries to allow it to work on file directly, if @path is a
directory it works as before, otherwise it treats @path as the target
file then directly allocate memory from it

Signed-off-by: Xiao Guangrong 
---
  exec.c | 80 ++
  1 file changed, 51 insertions(+), 29 deletions(-)

diff --git a/exec.c b/exec.c
index 9075f4d..db0fdaf 100644
--- a/exec.c
+++ b/exec.c
@@ -1174,14 +1174,60 @@ void qemu_mutex_unlock_ramlist(void)
  }

  #ifdef __linux__
+static bool path_is_dir(const char *path)
+{
+struct stat fs;
+
+return stat(path, &fs) == 0 && S_ISDIR(fs.st_mode);
+}
+
+static int open_ram_file_path(RAMBlock *block, const char *path, size_t size)
+{
+char *filename;
+char *sanitized_name;
+char *c;
+int fd;
+
+if (!path_is_dir(path)) {
+int flags = (block->flags & RAM_SHARED) ? O_RDWR : O_RDONLY;
+
+flags |= O_EXCL;
+return open(path, flags);
+}
+
+/* Make name safe to use with mkstemp by replacing '/' with '_'. */
+sanitized_name = g_strdup(memory_region_name(block->mr));
+for (c = sanitized_name; *c != '\0'; c++) {
+if (*c == '/') {
+*c = '_';
+}
+}
+filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
+   sanitized_name);
+g_free(sanitized_name);
+fd = mkstemp(filename);
+if (fd >= 0) {
+unlink(filename);
+/*
+ * ftruncate is not supported by hugetlbfs in older
+ * hosts, so don't bother bailing out on errors.
+ * If anything goes wrong with it under other filesystems,
+ * mmap will fail.
+ */
+if (ftruncate(fd, size)) {
+perror("ftruncate");
+}
+}
+g_free(filename);
+
+return fd;
+}
+
  static void *file_ram_alloc(RAMBlock *block,
  ram_addr_t memory,
  const char *path,
  Error **errp)
  {
-char *filename;
-char *sanitized_name;
-char *c;
  void *area;
  int fd;
  uint64_t pagesize;
@@ -1212,38 +1258,14 @@ static void *file_ram_alloc(RAMBlock *block,
  goto error;
  }

-/* Make name safe to use with mkstemp by replacing '/' with '_'. */
-sanitized_name = g_strdup(memory_region_name(block->mr));
-for (c = sanitized_name; *c != '\0'; c++) {
-if (*c == '/')
-*c = '_';
-}
-
-filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
-   sanitized_name);
-g_free(sanitized_name);
+memory = ROUND_UP(memory, pagesize);

-fd = mkstemp(filename);
+fd = open_ram_file_path(block, path, memory);
  if (fd < 0) {
  error_setg_errno(errp, errno,
   "unable to create backing store for path %s", path);
-g_free(filename);
  goto error;
  }
-unlink(filename);
-g_free(filename);
-
-memory = ROUND_UP(memory, pagesize);
-
-/*
- * ftruncate is not supported by hugetlbfs in older
- * hosts, so don't bother bailing out on errors.
- * If anything goes wrong with it under other filesystems,
- * mmap will fail.
- */
-if (ftruncate(fd, memory)) {
-perror("ftruncate");
-}

  area = qemu_ram_mmap(fd, memory, pagesize, block->flags & RAM_SHARED);
  if (area == MAP_FAILED) {



I was going to send tomorrow a pull request for a similar patch,
"backends/hostmem-file: Allow to specify full pathname for backing file".

The main difference seems to be your usage of O_EXCL.  Can you explain
why you added it?


It' used if we pass a block device as a NVDIMM backend memory:
 O_EXCL can be used without O_CREAT if pathname refers to a block device.  If 
the block device
 is in use by the system (e.g., mounted), open() fails with the error EBUSY

Re: [Qemu-devel] [PATCH] hw/arm/virt-acpi-build: _CCA attribute is compulsary

[Shannon Zhao]

Hi Graeme,

On 2015/11/2 18:39, Graeme Gregory wrote:
> According to ACPI specification 6.2.17 _CCA (Cache Coherency Attribute)
> this attribute is compulsary on ARM systems. Add this attribute to
> the PCI host bridges as required.
> 

To ACPI 5.1 this object is not compulsory and if not supplied it has
default value for it. But to ACPI 6.0 it must be supplied on ARM systems.
Regarding this change, ACPI 6.0 fixes 5.1 for this object, right?

> Without this the kernel will produce the error
> [Firmware Bug]: PCI device :00:00.0 fail to setup DMA.
> 
> Signed-off-by: Graeme Gregory 
> ---
>  hw/arm/virt-acpi-build.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 1aaff1f..1430125 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -180,6 +180,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const 
> MemMapEntry *memmap, int irq,
>  aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
>  aml_append(dev, aml_name_decl("_UID", aml_string("PCI0")));
>  aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0 Device")));
> +aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
>  
>  /* Declare the PCI Routing Table. */
>  Aml *rt_pkg = aml_package(nr_pcie_buses * PCI_NUM_PINS);
> 

-- 
Shannon

Re: [Qemu-devel] [PATCH 1/4] ide/atapi: make PIO read requests async

[John Snow]

On 10/12/2015 08:27 AM, Peter Lieven wrote:
> PIO read requests on the ATAPI interface used to be sync blk requests.
> This has two significant drawbacks. First the main loop hangs util an
> I/O request is completed and secondly if the I/O request does not
> complete (e.g. due to an unresponsive storage) Qemu hangs completely.
> 
> Signed-off-by: Peter Lieven 
> ---
>  hw/ide/atapi.c | 93 
> --
>  1 file changed, 84 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 747f466..2271ea2 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -105,11 +105,16 @@ static void cd_data_to_raw(uint8_t *buf, int lba)
>  memset(buf, 0, 288);
>  }
>  
> -static int cd_read_sector(IDEState *s, int lba, uint8_t *buf, int 
> sector_size)
> +static int
> +cd_read_sector_sync(IDEState *s, int lba, uint8_t *buf)
>  {
>  int ret;
>  
> -switch(sector_size) {
> +#ifdef DEBUG_IDE_ATAPI
> +printf("cd_read_sector_sync: lba=%d\n", lba);
> +#endif
> +
> +switch (s->cd_sector_size) {
>  case 2048:
>  block_acct_start(blk_get_stats(s->blk), &s->acct,
>   4 * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ);
> @@ -129,9 +134,71 @@ static int cd_read_sector(IDEState *s, int lba, uint8_t 
> *buf, int sector_size)
>  ret = -EIO;
>  break;
>  }
> +
> +if (!ret) {
> +s->lba++;
> +s->io_buffer_index = 0;
> +}
> +
>  return ret;
>  }
>  
> +static void cd_read_sector_cb(void *opaque, int ret)
> +{
> +IDEState *s = opaque;
> +
> +block_acct_done(blk_get_stats(s->blk), &s->acct);
> +
> +#ifdef DEBUG_IDE_ATAPI
> +printf("cd_read_sector_cb: lba=%d ret=%d\n", s->lba, ret);
> +#endif
> +
> +if (ret < 0) {
> +ide_atapi_io_error(s, ret);
> +return;
> +}
> +
> +if (s->cd_sector_size == 2352) {
> +cd_data_to_raw(s->io_buffer, s->lba);
> +}
> +
> +s->lba++;
> +s->io_buffer_index = 0;
> +s->status &= ~BUSY_STAT;
> +
> +ide_atapi_cmd_reply_end(s);
> +}
> +
> +static int cd_read_sector(IDEState *s, int lba, void *buf)
> +{
> +if (s->cd_sector_size != 2048 && s->cd_sector_size != 2352) {
> +return -EINVAL;
> +}
> +
> +s->iov.iov_base = buf;
> +if (s->cd_sector_size == 2352) {
> +buf += 16;
> +}
> +
> +s->iov.iov_len = 4 * BDRV_SECTOR_SIZE;
> +qemu_iovec_init_external(&s->qiov, &s->iov, 1);
> +
> +#ifdef DEBUG_IDE_ATAPI
> +printf("cd_read_sector: lba=%d\n", lba);
> +#endif
> +
> +if (blk_aio_readv(s->blk, (int64_t)lba << 2, &s->qiov, 4,
> +  cd_read_sector_cb, s) == NULL) {
> +return -EIO;
> +}
> +
> +block_acct_start(blk_get_stats(s->blk), &s->acct,
> + 4 * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ);
> +
> +s->status |= BUSY_STAT;
> +return 0;
> +}
> +
>  void ide_atapi_cmd_ok(IDEState *s)
>  {
>  s->error = 0;
> @@ -182,18 +249,27 @@ void ide_atapi_cmd_reply_end(IDEState *s)
>  ide_atapi_cmd_ok(s);
>  ide_set_irq(s->bus);
>  #ifdef DEBUG_IDE_ATAPI
> -printf("status=0x%x\n", s->status);
> +printf("end of transfer, status=0x%x\n", s->status);
>  #endif
>  } else {
>  /* see if a new sector must be read */
>  if (s->lba != -1 && s->io_buffer_index >= s->cd_sector_size) {
> -ret = cd_read_sector(s, s->lba, s->io_buffer, s->cd_sector_size);
> -if (ret < 0) {
> -ide_atapi_io_error(s, ret);
> +if (!s->elementary_transfer_size) {
> +ret = cd_read_sector(s, s->lba, s->io_buffer);
> +if (ret < 0) {
> +ide_atapi_io_error(s, ret);
> +}
>  return;
> +} else {
> +/* rebuffering within an elementary transfer is
> + * only possible with a sync request because we
> + * end up with a race condition otherwise */
> +ret = cd_read_sector_sync(s, s->lba, s->io_buffer);
> +if (ret < 0) {
> +ide_atapi_io_error(s, ret);
> +return;
> +}
>  }
> -s->lba++;
> -s->io_buffer_index = 0;
>  }
>  if (s->elementary_transfer_size > 0) {
>  /* there are some data left to transmit in this elementary
> @@ -275,7 +351,6 @@ static void ide_atapi_cmd_read_pio(IDEState *s, int lba, 
> int nb_sectors,
>  s->io_buffer_index = sector_size;
>  s->cd_sector_size = sector_size;
>  
> -s->status = READY_STAT | SEEK_STAT;
>  ide_atapi_cmd_reply_end(s);
>  }
>  
> 

This patch looks good to me, apart from Stefan's other comments. Will
you be sending a V3? I can try to merge it for this week before the hard
freeze hits.

--js

[Qemu-devel] [PATCH v3 1/5] fw_cfg: move internal function call docs to header file

[Gabriel L. Somlo]

Move documentation for fw_cfg functions internal to qemu from
docs/specs/fw_cfg.txt to the fw_cfg.h header file, next to their
prototype declarations, formatted as doc-comments.
NOTE: Documentation for fw_cfg_add_callback() is completely
dropped by this patch, as that function has been eliminated
by commit 023e3148.

Suggested-by: Peter Maydell 
Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Marc Marí 
Cc: Jordan Justen 
Cc: Paolo Bonzini 
Cc: Peter Maydell 
Signed-off-by: Gabriel Somlo 
Reviewed-by: Laszlo Ersek 
---
 docs/specs/fw_cfg.txt |  85 +-
 include/hw/nvram/fw_cfg.h | 129 ++
 2 files changed, 130 insertions(+), 84 deletions(-)

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index b8c794f..2099ad9 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -192,90 +192,7 @@ To check the result, read the "control" field:
 today due to implementation not being async,
 but may in the future).
 
-= Host-side API =
-
-The following functions are available to the QEMU programmer for adding
-data to a fw_cfg device during guest initialization (see fw_cfg.h for
-each function's complete prototype):
-
-== fw_cfg_add_bytes() ==
-
-Given a selector key value, starting pointer, and size, create an item
-as a raw "blob" of the given size, available by selecting the given key.
-The data referenced by the starting pointer is only linked, NOT copied,
-into the data structure of the fw_cfg device.
-
-== fw_cfg_add_string() ==
-
-Instead of a starting pointer and size, this function accepts a pointer
-to a NUL-terminated ascii string, and inserts a newly allocated copy of
-the string (including the NUL terminator) into the fw_cfg device data
-structure.
-
-== fw_cfg_add_iXX() ==
-
-Insert an XX-bit item, where XX may be 16, 32, or 64. These functions
-will convert a 16-, 32-, or 64-bit integer to little-endian, then add
-a dynamically allocated copy of the appropriately sized item to fw_cfg
-under the given selector key value.
-
-== fw_cfg_modify_iXX() ==
-
-Modify the value of an XX-bit item (where XX may be 16, 32, or 64).
-Similarly to the corresponding fw_cfg_add_iXX() function set, convert
-a 16-, 32-, or 64-bit integer to little endian, create a dynamically
-allocated copy of the required size, and replace the existing item at
-the given selector key value with the newly allocated one. The previous
-item, assumed to have been allocated during an earlier call to
-fw_cfg_add_iXX() or fw_cfg_modify_iXX() (of the same width XX), is freed
-before the function returns.
-
-== fw_cfg_add_file() ==
-
-Given a filename (i.e., fw_cfg item name), starting pointer, and size,
-create an item as a raw "blob" of the given size. Unlike fw_cfg_add_bytes()
-above, the next available selector key (above 0x0020, FW_CFG_FILE_FIRST)
-will be used, and a new entry will be added to the file directory structure
-(at key 0x0019), containing the item name, blob size, and automatically
-assigned selector key value. The data referenced by the starting pointer
-is only linked, NOT copied, into the fw_cfg data structure.
-
-== fw_cfg_add_file_callback() ==
-
-Like fw_cfg_add_file(), but additionally sets pointers to a callback
-function (and opaque argument), which will be executed host-side by
-QEMU each time a byte is read by the guest from this particular item.
-
-NOTE: The callback function is given the opaque argument set by
-fw_cfg_add_file_callback(), but also the current data offset,
-allowing it the option of only acting upon specific offset values
-(e.g., 0, before the first data byte of the selected item is
-returned to the guest).
-
-== fw_cfg_modify_file() ==
-
-Given a filename (i.e., fw_cfg item name), starting pointer, and size,
-completely replace the configuration item referenced by the given item
-name with the new given blob. If an existing blob is found, its
-callback information is removed, and a pointer to the old data is
-returned to allow the caller to free it, helping avoid memory leaks.
-If a configuration item does not already exist under the given item
-name, a new item will be created as with fw_cfg_add_file(), and NULL
-is returned to the caller. In any case, the data referenced by the
-starting pointer is only linked, NOT copied, into the fw_cfg data
-structure.
-
-== fw_cfg_add_callback() ==
-
-Like fw_cfg_add_bytes(), but additionally sets pointers to a callback
-function (and opaque argument), which will be executed host-side by
-QEMU each time a guest-side write operation to this particular item
-completes fully overwriting the item's data.
-
-NOTE: This function is deprecated, and will be completely removed
-starting with QEMU v2.4.
-
-== Externally Provided Items ==
+= Externally Provided Items =
 
 As of v2.4, "file" fw_cfg items (i.e., items with selector keys above
 FW_CFG_FILE_FIRST, and with a corresponding entry in the fw_cfg file
diff --git a/include/hw/n

[Qemu-devel] [PATCH v3 0/5] fw_cfg: spec update, misc. cleanup, optimize read

[Gabriel L. Somlo]

New since v2:

- Patches 1-3: updated to address Laszlo's suggestions for better
  and more accurate change descriptions in commit logs, comments,
  etc.

- Patch 4: Per Laszlo's recommendation, this has been split into
  two separate components for improved legibility:

- Patch 4/5: Introduces the new generic read method, and
  replaces the existing MMIO logic

- Patch 5/5: Replaces the IOPort read logic, and cleans
  up the remaining unused bits of code.

Thanks again,
  --Gabriel

>This series' main purpose is to update (and simplify) the specified
>read callback behavior. An earlier standalone patch to move qemu function
>call API documentation into fw_cfg.h should logically be part of the series.
>
>Here's the summary of what each patch does:
>
>- Patch 1/4 is an updated version of the standalone v1 patch
>  I sent out earlier; it moves all the qemu-internal host-side
>  function call api documentation out of docs/specs/fw_cfg.txt,
>  and into the fw_cfg.h header file, next to the prototype of
>  each documented api function.
>
>- Patch 2/4 modifies the specified behavior of read callbacks
>  (from being invoked once per byte read, to being invoked once,
>   before ANY data is read, specifically once each time an item
>   is selected).
>
>- Patch 3/4 additionally removes the now-redundant offset argument
>  from the read callback prototype.
>
>- Finally, 4/4 consolidates (non-DMA) reads, minimizing the number
>  of times redundant sanity checks are performed, particularly for
>  wide (> byte) sized reads.

Gabriel L. Somlo (5):
  fw_cfg: move internal function call docs to header file
  fw_cfg: amend callback behavior spec to once per select
  fw_cfg: remove offset argument from callback prototype
  fw_cfg: add generic non-DMA read method
  fw_cfg: replace ioport data read with generic method

 docs/specs/fw_cfg.txt |  85 +-
 hw/arm/virt-acpi-build.c  |   2 +-
 hw/i386/acpi-build.c  |   2 +-
 hw/nvram/fw_cfg.c |  58 -
 include/hw/nvram/fw_cfg.h | 128 +-
 trace-events  |   2 +-
 6 files changed, 151 insertions(+), 126 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v3 4/5] fw_cfg: add generic non-DMA read method

[Gabriel L. Somlo]

Introduce fw_cfg_data_read(), a generic read method which works
on all access widths (1 through 8 bytes, inclusive), and can be
used during both IOPort and MMIO read accesses.

To maintain legibility, only fw_cfg_data_mem_read() (the MMIO
data read method) is replaced by this patch. The new method
essentially unwinds the fw_cfg_data_mem_read() + fw_cfg_read()
combo, but without unnecessarily repeating all the validity
checks performed by the latter on each byte being read.

This patch also modifies the trace_fw_cfg_read prototype to
accept a 64-bit value argument, allowing it to work properly
with the new read method, but also remain backward compatible
with existing call sites.

Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Marc Marí 
Signed-off-by: Gabriel Somlo 
---
 hw/nvram/fw_cfg.c | 33 +++--
 trace-events  |  2 +-
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index c2d3a0a..8aa980c 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -274,6 +274,24 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key)
 return ret;
 }
 
+static uint64_t fw_cfg_data_read(void *opaque, hwaddr addr, unsigned size)
+{
+FWCfgState *s = opaque;
+int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
+FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
+uint64_t value = 0;
+
+assert(size <= sizeof(value));
+if (s->cur_entry != FW_CFG_INVALID && e->data) {
+while (size-- && s->cur_offset < e->len) {
+value = (value << 8) | e->data[s->cur_offset++];
+}
+}
+
+trace_fw_cfg_read(s, value);
+return value;
+}
+
 static uint8_t fw_cfg_read(FWCfgState *s)
 {
 int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
@@ -290,19 +308,6 @@ static uint8_t fw_cfg_read(FWCfgState *s)
 return ret;
 }
 
-static uint64_t fw_cfg_data_mem_read(void *opaque, hwaddr addr,
- unsigned size)
-{
-FWCfgState *s = opaque;
-uint64_t value = 0;
-unsigned i;
-
-for (i = 0; i < size; ++i) {
-value = (value << 8) | fw_cfg_read(s);
-}
-return value;
-}
-
 static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
   uint64_t value, unsigned size)
 {
@@ -483,7 +488,7 @@ static const MemoryRegionOps fw_cfg_ctl_mem_ops = {
 };
 
 static const MemoryRegionOps fw_cfg_data_mem_ops = {
-.read = fw_cfg_data_mem_read,
+.read = fw_cfg_data_read,
 .write = fw_cfg_data_mem_write,
 .endianness = DEVICE_BIG_ENDIAN,
 .valid = {
diff --git a/trace-events b/trace-events
index 72136b9..5073040 100644
--- a/trace-events
+++ b/trace-events
@@ -196,7 +196,7 @@ ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Read 
diagnostic %"PRId64"= %02x
 
 # hw/nvram/fw_cfg.c
 fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d"
-fw_cfg_read(void *s, uint8_t ret) "%p = %d"
+fw_cfg_read(void *s, uint64_t ret) "%p = %"PRIx64
 fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd 
bytes)"
 
 # hw/block/hd-geometry.c
-- 
2.4.3

[Qemu-devel] [PATCH v3 3/5] fw_cfg: remove offset argument from callback prototype

[Gabriel L. Somlo]

Read callbacks are now only invoked at item selection, before any
data is read. As such, the value of the offset argument passed to
the callback will always be 0. Also, the two callback instances
currently in use both leave their offset argument unused.

This patch removes the offset argument from the fw_cfg read callback
prototype, and from the currently available instances. The unused
(write) callback prototype is also removed (write support was removed
earlier, in commit 023e3148).

Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Marc Marí 
Signed-off-by: Gabriel Somlo 
Reviewed-by: Laszlo Ersek 
---
 hw/arm/virt-acpi-build.c  | 2 +-
 hw/i386/acpi-build.c  | 2 +-
 hw/nvram/fw_cfg.c | 2 +-
 include/hw/nvram/fw_cfg.h | 3 +--
 4 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 1aaff1f..4eed24d 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -626,7 +626,7 @@ static void acpi_ram_update(MemoryRegion *mr, GArray *data)
 memory_region_set_dirty(mr, 0, size);
 }
 
-static void virt_acpi_build_update(void *build_opaque, uint32_t offset)
+static void virt_acpi_build_update(void *build_opaque)
 {
 AcpiBuildState *build_state = build_opaque;
 AcpiBuildTables tables;
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 95e0c65..29e30ce 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1818,7 +1818,7 @@ static void acpi_ram_update(MemoryRegion *mr, GArray 
*data)
 memory_region_set_dirty(mr, 0, size);
 }
 
-static void acpi_build_update(void *build_opaque, uint32_t offset)
+static void acpi_build_update(void *build_opaque)
 {
 AcpiBuildState *build_state = build_opaque;
 AcpiBuildTables tables;
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 6e6414b..c2d3a0a 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -266,7 +266,7 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key)
 arch = !!(key & FW_CFG_ARCH_LOCAL);
 e = &s->entries[arch][key & FW_CFG_ENTRY_MASK];
 if (e->read_callback) {
-e->read_callback(e->callback_opaque, s->cur_offset);
+e->read_callback(e->callback_opaque);
 }
 }
 
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index a1cfaa4..664eaf6 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -70,8 +70,7 @@ typedef struct FWCfgDmaAccess {
 uint64_t address;
 } QEMU_PACKED FWCfgDmaAccess;
 
-typedef void (*FWCfgCallback)(void *opaque, uint8_t *data);
-typedef void (*FWCfgReadCallback)(void *opaque, uint32_t offset);
+typedef void (*FWCfgReadCallback)(void *opaque);
 
 /**
  * fw_cfg_add_bytes:
-- 
2.4.3

[Qemu-devel] [PATCH v3 2/5] fw_cfg: amend callback behavior spec to once per select

[Gabriel L. Somlo]

Currently, the fw_cfg internal API specifies that if an item was set up
with a read callback, the callback must be run each time a byte is read
from the item. This behavior is both wasteful (most items do not have a
read callback set), and impractical for bulk transfers (e.g., DMA read).

At the time of this writing, the only items configured with a callback
are "/etc/table-loader", "/etc/acpi/tables", and "/etc/acpi/rsdp". They
all share the same callback functions: virt_acpi_build_update() on ARM
(in hw/arm/virt-acpi-build.c), and acpi_build_update() on i386 (in
hw/i386/acpi.c). Both of these callbacks are one-shot (i.e. they return
without doing anything at all after the first time they are invoked with
a given build_state; since build_state is also shared across all three
items mentioned above, the callback only ever runs *once*, the first
time either of the listed items is read).

This patch amends the specification for fw_cfg_add_file_callback() to
state that any available read callback will only be invoked once each
time the item is selected. This change has no practical effect on the
current behavior of QEMU, and it enables us to significantly optimize
the behavior of fw_cfg reads during guest firmware setup, eliminating
a large amount of redundant callback checks and invocations.

Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Marc Marí 
Signed-off-by: Gabriel Somlo 
---
 hw/nvram/fw_cfg.c | 19 ++-
 include/hw/nvram/fw_cfg.h | 10 +++---
 2 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 73b0a81..6e6414b 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -252,7 +252,8 @@ static void fw_cfg_write(FWCfgState *s, uint8_t value)
 
 static int fw_cfg_select(FWCfgState *s, uint16_t key)
 {
-int ret;
+int arch, ret;
+FWCfgEntry *e;
 
 s->cur_offset = 0;
 if ((key & FW_CFG_ENTRY_MASK) >= FW_CFG_MAX_ENTRY) {
@@ -261,6 +262,12 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key)
 } else {
 s->cur_entry = key;
 ret = 1;
+/* entry successfully selected, now run callback if present */
+arch = !!(key & FW_CFG_ARCH_LOCAL);
+e = &s->entries[arch][key & FW_CFG_ENTRY_MASK];
+if (e->read_callback) {
+e->read_callback(e->callback_opaque, s->cur_offset);
+}
 }
 
 trace_fw_cfg_select(s, key, ret);
@@ -276,9 +283,6 @@ static uint8_t fw_cfg_read(FWCfgState *s)
 if (s->cur_entry == FW_CFG_INVALID || !e->data || s->cur_offset >= e->len)
 ret = 0;
 else {
-if (e->read_callback) {
-e->read_callback(e->callback_opaque, s->cur_offset);
-}
 ret = e->data[s->cur_offset++];
 }
 
@@ -371,10 +375,6 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
 len = (e->len - s->cur_offset);
 }
 
-if (e->read_callback) {
-e->read_callback(e->callback_opaque, s->cur_offset);
-}
-
 /* If the access is not a read access, it will be a skip access,
  * tested before.
  */
@@ -513,7 +513,8 @@ static void fw_cfg_reset(DeviceState *d)
 {
 FWCfgState *s = FW_CFG(d);
 
-fw_cfg_select(s, 0);
+/* we never register a read callback for FW_CFG_SIGNATURE */
+fw_cfg_select(s, FW_CFG_SIGNATURE);
 }
 
 /* Save restore 32 bit int as uint16_t
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 4b5e196..a1cfaa4 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -183,13 +183,9 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename, 
void *data,
  * structure residing at key value FW_CFG_FILE_DIR, containing the item name,
  * data size, and assigned selector key value.
  * Additionally, set a callback function (and argument) to be called each
- * time a byte is read by the guest from this particular item, or, in the
- * case of DMA, each time a read or skip request overlaps with the valid
- * offset range of the item.
- * NOTE: In addition to the opaque argument set here, the callback function
- * takes the current data offset as an additional argument, allowing it the
- * option of only acting upon specific offset values (e.g., 0, before the
- * first data byte of the selected item is returned to the guest).
+ * time this item is selected (by having its selector key either written to
+ * the fw_cfg control register, or passed to QEMU in FWCfgDmaAccess.control
+ * with FW_CFG_DMA_CTL_SELECT).
  */
 void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
   FWCfgReadCallback callback, void 
*callback_opaque,
-- 
2.4.3

[Qemu-devel] [PATCH v3 5/5] fw_cfg: replace ioport data read with generic method

[Gabriel L. Somlo]

IOPort read access is limited to one byte at a time by
fw_cfg_comb_valid(). As such, fw_cfg_comb_read() may safely
ignore its size argument (which will always be 1), and simply
call its fw_cfg_read() helper function once, returning 8 bits
via the least significant byte of a 64-bit return value.

This patch replaces fw_cfg_comb_read() with the generic method
fw_cfg_data_read(), and removes the unused fw_cfg_read() helper.

When called with size = 1, fw_cfg_data_read() acts exactly like
fw_cfg_read(), performing the same set of sanity checks, and
executing the while loop at most once (subject to the current
read offset being within range).

Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Marc Marí 
Signed-off-by: Gabriel Somlo 
---
 hw/nvram/fw_cfg.c | 24 +---
 1 file changed, 1 insertion(+), 23 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 8aa980c..7d216f0 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -292,22 +292,6 @@ static uint64_t fw_cfg_data_read(void *opaque, hwaddr 
addr, unsigned size)
 return value;
 }
 
-static uint8_t fw_cfg_read(FWCfgState *s)
-{
-int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
-FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
-uint8_t ret;
-
-if (s->cur_entry == FW_CFG_INVALID || !e->data || s->cur_offset >= e->len)
-ret = 0;
-else {
-ret = e->data[s->cur_offset++];
-}
-
-trace_fw_cfg_read(s, ret);
-return ret;
-}
-
 static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
   uint64_t value, unsigned size)
 {
@@ -456,12 +440,6 @@ static bool fw_cfg_ctl_mem_valid(void *opaque, hwaddr addr,
 return is_write && size == 2;
 }
 
-static uint64_t fw_cfg_comb_read(void *opaque, hwaddr addr,
- unsigned size)
-{
-return fw_cfg_read(opaque);
-}
-
 static void fw_cfg_comb_write(void *opaque, hwaddr addr,
   uint64_t value, unsigned size)
 {
@@ -499,7 +477,7 @@ static const MemoryRegionOps fw_cfg_data_mem_ops = {
 };
 
 static const MemoryRegionOps fw_cfg_comb_mem_ops = {
-.read = fw_cfg_comb_read,
+.read = fw_cfg_data_read,
 .write = fw_cfg_comb_write,
 .endianness = DEVICE_LITTLE_ENDIAN,
 .valid.accepts = fw_cfg_comb_valid,
-- 
2.4.3

Re: [Qemu-devel] [PATCH] configure: add missing --disable-modules option

[Fam Zheng]

On Mon, 11/02 14:06, Stefan Hajnoczi wrote:
> According to ./configure all options should have both --enable-foo and
> --disable-foo:
> 
>   # Always add --enable-foo and --disable-foo command line args.
>   # Distributions want to ensure that several features are compiled in, and it
>   # is impossible without a --enable-foo that exits if a feature is not found.

Actually the document is wrong about module support, the default is off:

> Optional features, enabled with --enable-FEATURE and
> disabled with --disable-FEATURE, default is enabled if available:

Maybe we should move it out of this section instead?

What about vnc-tls? It's described in the same place, but there is no option
for it.

Fam

Re: [Qemu-devel] [PATCH] qemu-img: add check for zero-length job len

[Jeff Cody]

On Mon, Nov 02, 2015 at 06:28:20PM -0500, John Snow wrote:
> The mirror job doesn't update its total length until
> it has already started running, so we should translate
> a zero-length job-len as meaning 0%.
> 
> Otherwise, we may get divide-by-zero faults.
> 
> Signed-off-by: John Snow 
> ---
>  qemu-img.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 3025776..38b4888 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -656,7 +656,8 @@ static void run_block_job(BlockJob *job, Error **errp)
>  
>  do {
>  aio_poll(aio_context, true);
> -qemu_progress_print((float)job->offset / job->len * 100.f, 0);
> +qemu_progress_print(job->len ?
> +((float)job->offset / job->len * 100.f) : 0.00, 
> 0);
>  } while (!job->ready);
>  
>  block_job_complete_sync(job, errp);
> -- 
> 2.4.3
>

Reviewed-by: Jeff Cody 

Re: [Qemu-devel] [PATCH] qcow2: avoid misaligned 64bit bswap

[Eric Blake]

On 11/02/2015 04:32 PM, John Snow wrote:
> If we create a buffer directly on the stack by using 12 bytes, there's
> no guarantee the 64bit value we want to swap will be aligned, which
> could cause errors with undefined behavior.
> 
> Spotted with clang -fsanitize=undefined and observed in iotests 15, 26,
> 44, 115 and 121.
> 
> Signed-off-by: John Snow 
> ---
>  block/qcow2-refcount.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)

Reviewed-by: Eric Blake 

-- 
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] qemu-img: add check for zero-length job len

[John Snow]

On 11/02/2015 06:43 PM, Eric Blake wrote:
> On 11/02/2015 04:28 PM, John Snow wrote:
>> The mirror job doesn't update its total length until
>> it has already started running, so we should translate
>> a zero-length job-len as meaning 0%.
>>
>> Otherwise, we may get divide-by-zero faults.
>>
>> Signed-off-by: John Snow 
>> ---
>>  qemu-img.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> And indeed, this has tripped up libvirt in the past :)
> 
> My only concern is what if you truly have a 0-length job?  For example,
> when doing two block-stream commands with identical arguments in a row,
> the second block-stream has no work to do, but can complete instantly.
> 
> Will this result in such a job never reporting 100% complete?  If so,
> that's bad.
> 

A few lines below the context:

/* A block job may finish instantaneously without publishing any
progress,

 * so just signal completion here */
qemu_progress_print(100.f, 0);


> If you can answer my concerns that we don't have a design bug, then the
> code changes look correct, and you can add:
> 
> Reviewed-by: Eric Blake 
> 
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 3025776..38b4888 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -656,7 +656,8 @@ static void run_block_job(BlockJob *job, Error **errp)
>>  
>>  do {
>>  aio_poll(aio_context, true);
>> -qemu_progress_print((float)job->offset / job->len * 100.f, 0);
>> +qemu_progress_print(job->len ?
>> +((float)job->offset / job->len * 100.f) : 0.00, 
>> 0);
> 
> Also, note that this promotes to double rather than float; maybe you
> want to use 0.f instead of 0.00 to keep the ternary as a float?  But it
> shouldn't make a difference in practice.
> 

Yes, oops -- but harmless.

Re: [Qemu-devel] [PATCH] qemu-img: add check for zero-length job len

[Eric Blake]

On 11/02/2015 04:28 PM, John Snow wrote:
> The mirror job doesn't update its total length until
> it has already started running, so we should translate
> a zero-length job-len as meaning 0%.
> 
> Otherwise, we may get divide-by-zero faults.
> 
> Signed-off-by: John Snow 
> ---
>  qemu-img.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

And indeed, this has tripped up libvirt in the past :)

My only concern is what if you truly have a 0-length job?  For example,
when doing two block-stream commands with identical arguments in a row,
the second block-stream has no work to do, but can complete instantly.

Will this result in such a job never reporting 100% complete?  If so,
that's bad.

If you can answer my concerns that we don't have a design bug, then the
code changes look correct, and you can add:

Reviewed-by: Eric Blake 

> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 3025776..38b4888 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -656,7 +656,8 @@ static void run_block_job(BlockJob *job, Error **errp)
>  
>  do {
>  aio_poll(aio_context, true);
> -qemu_progress_print((float)job->offset / job->len * 100.f, 0);
> +qemu_progress_print(job->len ?
> +((float)job->offset / job->len * 100.f) : 0.00, 
> 0);

Also, note that this promotes to double rather than float; maybe you
want to use 0.f instead of 0.00 to keep the ternary as a float?  But it
shouldn't make a difference in practice.

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH] qcow2: avoid misaligned 64bit bswap

[John Snow]

If we create a buffer directly on the stack by using 12 bytes, there's
no guarantee the 64bit value we want to swap will be aligned, which
could cause errors with undefined behavior.

Spotted with clang -fsanitize=undefined and observed in iotests 15, 26,
44, 115 and 121.

Signed-off-by: John Snow 
---
 block/qcow2-refcount.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 4b81c8d..6e0e5bd 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -560,13 +560,16 @@ static int alloc_refcount_block(BlockDriverState *bs,
 }
 
 /* Hook up the new refcount table in the qcow2 header */
-uint8_t data[12];
-cpu_to_be64w((uint64_t*)data, table_offset);
-cpu_to_be32w((uint32_t*)(data + 8), table_clusters);
+struct QEMU_PACKED {
+uint64_t d64;
+uint32_t d32;
+} data;
+cpu_to_be64w(&data.d64, table_offset);
+cpu_to_be32w(&data.d32, table_clusters);
 BLKDBG_EVENT(bs->file, BLKDBG_REFBLOCK_ALLOC_SWITCH_TABLE);
 ret = bdrv_pwrite_sync(bs->file->bs,
offsetof(QCowHeader, refcount_table_offset),
-   data, sizeof(data));
+   &data, sizeof(data));
 if (ret < 0) {
 goto fail_table;
 }
-- 
2.4.3

[Qemu-devel] [PATCH] qemu-img: add check for zero-length job len

[John Snow]

The mirror job doesn't update its total length until
it has already started running, so we should translate
a zero-length job-len as meaning 0%.

Otherwise, we may get divide-by-zero faults.

Signed-off-by: John Snow 
---
 qemu-img.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 3025776..38b4888 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -656,7 +656,8 @@ static void run_block_job(BlockJob *job, Error **errp)
 
 do {
 aio_poll(aio_context, true);
-qemu_progress_print((float)job->offset / job->len * 100.f, 0);
+qemu_progress_print(job->len ?
+((float)job->offset / job->len * 100.f) : 0.00, 0);
 } while (!job->ready);
 
 block_job_complete_sync(job, errp);
-- 
2.4.3

[Qemu-devel] [PATCH v3] seccomp: add cacheflush to whitelist

[Andrew Jones]

cacheflush is an arm-specific syscall that qemu built for arm
uses. Add it to the whitelist, but only if we're linking with
a recent enough libseccomp.

Signed-off-by: Andrew Jones 
---
v3: deal with major and minor version number bumps
v2: only add cacheflush if libseccomp supports it

 qemu-seccomp.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index 80d034a8d5190..c831fe83ad500 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -16,6 +16,14 @@
 #include 
 #include "sysemu/seccomp.h"
 
+#if SCMP_VER_MAJOR >= 3
+  #define HAVE_CACHEFLUSH
+#elif SCMP_VER_MAJOR == 2 && SCMP_VER_MINOR >= 3
+  #define HAVE_CACHEFLUSH
+#elif SCMP_VER_MAJOR == 2 && SCMP_VER_MINOR == 2 && SCMP_VER_MICRO >= 3
+  #define HAVE_CACHEFLUSH
+#endif
+
 struct QemuSeccompSyscall {
 int32_t num;
 uint8_t priority;
@@ -238,7 +246,10 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] 
= {
 { SCMP_SYS(inotify_init1), 240 },
 { SCMP_SYS(inotify_add_watch), 240 },
 { SCMP_SYS(mbind), 240 },
-{ SCMP_SYS(memfd_create), 240 }
+{ SCMP_SYS(memfd_create), 240 },
+#ifdef HAVE_CACHEFLUSH
+{ SCMP_SYS(cacheflush), 240 },
+#endif
 };
 
 int seccomp_start(void)
-- 
1.8.3.1

[Qemu-devel] [PATCH v8.5 2/4] qapi: Check for QMP collisions of flat union branches

[Eric Blake]

Right now, our ad hoc parser ensures that we cannot have a
flat union that introduces any QMP member names that would
conflict with the non-variant QMP members already present
from the union's base type (see flat-union-clash-member.json).
However, we want QAPISchema*.check() to make the same check,
so we can later reduce some of the ad hoc checks.

Basically, all branches of a flat union must be qapi structs
with no variants, at which point those members appear in the
same JSON object as all non-variant members.  We already have
a map 'seen' of all non-variant members passed in to
QAPISchemaObjectTypeVariants.check(), which we clone for each
particular variant (since the members of one variant do not
clash with another); all that is additionally needed is to
actually check the each member of the variant type do not add
any collisions.

For simple unions, the same code happens to work (however, our
synthesized wrapper type has a single member 'data' which will
never collide with the one non-variant member 'type').

But for alternates, we do NOT want to check the type members
for adding collisions (an alternate has no parent JSON object
that is merging in member names, the way a flat union does), so
we have to pass in an extra flag to distinguish whether we are
working with a union or an alternate.  The flag is temporary; a
later patch will rework how alternates are laid out by creating
a new subclass of QAPISchemaObjectTypeMember, and detecting the
use of this subclass for variants.tag_member will serve the
same purpose.

Note that an early proposal [1] for what eventually became
commit ac88219a had QAPISchemaObjectTypeVariant.check() ensure
that the variant type was complete, although it was later
removed [2]; the checks added here happen to match what that
earlier attempt meant to do.

[1] https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg00394.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg00450.html

Signed-off-by: Eric Blake 

---
v9: new patch, split off from v8 7/17
---
 scripts/qapi.py | 25 +++--
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index fff4adb..3cf051f 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1047,7 +1047,8 @@ class QAPISchemaObjectTypeVariants(object):
 self.tag_member = tag_member
 self.variants = variants

-def check(self, schema, seen):
+# TODO drop 'union' param once tag_member is sufficient to spot alternates
+def check(self, schema, seen, union=True):
 if self.tag_name:# flat union
 self.tag_member = seen[self.tag_name]
 assert self.tag_member
@@ -1055,17 +1056,29 @@ class QAPISchemaObjectTypeVariants(object):
 assert self.tag_member in seen.itervalues()
 assert isinstance(self.tag_member.type, QAPISchemaEnumType)
 for v in self.variants:
-vseen = dict(seen)
-v.check(schema, self.tag_member.type, vseen)
+# Reset seen array for each variant, since QMP names from one
+# branch do not affect another branch
+v.check(schema, self.tag_member.type, dict(seen), union)


 class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
 def __init__(self, name, typ):
 QAPISchemaObjectTypeMember.__init__(self, name, typ, False)

-def check(self, schema, tag_type, seen):
-QAPISchemaObjectTypeMember.check(self, schema, seen)
+# TODO drop 'union' param once tag_type is sufficient to spot alternates
+def check(self, schema, tag_type, seen, union):
+QAPISchemaObjectTypeMember.check(self, schema, dict(seen))
 assert self.name in tag_type.values
+if union:
+# If this variant is used within a union, then each member
+# field must avoid collisions with the non-variant members
+# already present in the union.
+assert isinstance(self.type, QAPISchemaObjectType)
+assert not self.type.variants   # not implemented
+self.type.check(schema)
+for m in self.type.members:
+assert c_name(m.name) not in seen
+seen[m.name] = m

 # This function exists to support ugly simple union special cases
 # TODO get rid of them, and drop the function
@@ -1088,7 +1101,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
 def check(self, schema):
 seen = {}
 self.variants.tag_member.check(schema, seen)
-self.variants.check(schema, seen)
+self.variants.check(schema, seen, False)

 def json_type(self):
 return 'value'
-- 
2.4.3

[Qemu-devel] [PATCH v8.5 4/4] qapi: Consolidate collision detection code

[Eric Blake]

Rather than having three separate places populate the seen map,
it is easier to just factor out a subset of Member.check()
that does this as a new method check_collision(), and have the
remaining places in ObjectType.check() and Variant.check()
call into it.  This likewise means a new helper method
ObjectType.check_collision().  Later patches can then change
the handling of the seen array in just one place, when moving
away from ad hoc parser tests.

Note that there was some discrepancy in the existing code on
whether name or c_name(name) could not previously be in the
seen map; a future patch will clean this up to consistently
populate the map via c_name().

Signed-off-by: Eric Blake 

---
v9: new patch, split off from v8 7/17; name change from
ObjectType.check_qmp() to check_collision(), and new method
Member.check_collision(). I'm open to naming suggestions.
---
 scripts/qapi.py | 31 ++-
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 10bf16f..b519d30 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -981,18 +981,21 @@ class QAPISchemaObjectType(QAPISchemaType):
 seen = OrderedDict()
 if self._base_name:
 self.base = schema.lookup_type(self._base_name)
-assert isinstance(self.base, QAPISchemaObjectType)
-assert not self.base.variants   # not implemented
-self.base.check(schema)
-for m in self.base.members:
-assert c_name(m.name) not in seen
-seen[m.name] = m
+self.base.check_collision(schema, seen)
 for m in self.local_members:
 m.check(schema, seen)
 if self.variants:
 self.variants.check(schema, seen)
 self.members = seen.values()

+# Check that the members of this type do not cause duplicate JSON fields,
+# and update seen to track the members seen so far
+def check_collision(self, schema, seen):
+assert not self.variants   # not implemented
+self.check(schema)
+for m in self.members:
+m.check_collision(seen)
+
 def is_implicit(self):
 # See QAPISchema._make_implicit_object_type()
 return self.name[0] == ':'
@@ -1026,9 +1029,16 @@ class QAPISchemaObjectTypeMember(object):
 self.optional = optional

 def check(self, schema, seen):
-assert self.name not in seen
 self.type = schema.lookup_type(self._type_name)
+self.check_collision(seen)
+
+# Check that this member does not collide with anything in seen (the
+# set of non-variant members when called from QAPISchemaObjectType,
+# or the set of tag values when called from QAPISchemaObjectTypeVariant),
+# and update seen accordingly.
+def check_collision(self, seen):
 assert self.type
+assert self.name not in seen
 seen[self.name] = self


@@ -1074,12 +1084,7 @@ class 
QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
 # If this variant is used within a union, then each member
 # field must avoid collisions with the non-variant members
 # already present in the union.
-assert isinstance(self.type, QAPISchemaObjectType)
-assert not self.type.variants   # not implemented
-self.type.check(schema)
-for m in self.type.members:
-assert c_name(m.name) not in seen
-seen[m.name] = m
+self.type.check_collision(schema, seen)

 # This function exists to support ugly simple union special cases
 # TODO get rid of them, and drop the function
-- 
2.4.3

[Qemu-devel] [PATCH v8.5 3/4] qapi: Fix check for variant tag values collision

[Eric Blake]

Now that commit e4ba22b3 has separated the C representation of
qapi unions so that tag values no longer collide with non-variant
members, we must adjust QAPISchemaObjectTypeVariant.check() to
match.  The fix is conceptually simple - track a separate
dictionary of tag names we have seen so far, different from the
dictionary of non-variant names.  And while the non-variant seen
array gets reset for each new variant (because the JSON object
does not have collisions between separate branches), the map of
tag names is not reset.

Signed-off-by: Eric Blake 

---
v9: new patch, split off from v8 7/17
---
 scripts/qapi.py | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 3cf051f..10bf16f 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1055,10 +1055,11 @@ class QAPISchemaObjectTypeVariants(object):
 else:# simple union or alternate
 assert self.tag_member in seen.itervalues()
 assert isinstance(self.tag_member.type, QAPISchemaEnumType)
+cases = {}
 for v in self.variants:
 # Reset seen array for each variant, since QMP names from one
 # branch do not affect another branch
-v.check(schema, self.tag_member.type, dict(seen), union)
+v.check(schema, self.tag_member.type, dict(seen), cases, union)


 class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
@@ -1066,8 +1067,8 @@ class 
QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
 QAPISchemaObjectTypeMember.__init__(self, name, typ, False)

 # TODO drop 'union' param once tag_type is sufficient to spot alternates
-def check(self, schema, tag_type, seen, union):
-QAPISchemaObjectTypeMember.check(self, schema, dict(seen))
+def check(self, schema, tag_type, seen, cases, union):
+QAPISchemaObjectTypeMember.check(self, schema, cases)
 assert self.name in tag_type.values
 if union:
 # If this variant is used within a union, then each member
-- 
2.4.3

[Qemu-devel] [PATCH v8.5 0/4] rework of 7/17

[Eric Blake]

Based on the confusion I caused Markus, I've tried to split
the v8 patch 7/17 into smaller pieces that are hopefully
easier to review in isolation.

Eric Blake (4):
  qapi: Drop all_members parameter from check()
  qapi: Check for QMP collisions of flat union branches
  qapi: Fix check for variant tag values collision
  qapi: Consolidate collision detection code

 scripts/qapi.py | 62 -
 1 file changed, 39 insertions(+), 23 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v8.5 1/4] qapi: Drop all_members parameter from check()

[Eric Blake]

The implementation of QAPISchemaObjectTypeMember.check() always
adds the member currently being checked to both the all_members
and seen parameters. However, the three callers of this method
pass in the following parameters:

QAPISchemaObjectType.check():
  - all_members contains all non-variant members seen to date,
  for use in populating self.members
  - seen contains all non-variant members seen to date, for
  use in checking for collisions

QAPISchemaObjectTypeVariant.check():
  - all_members is a throwaway empty list
  - seen is a throwaway dictionary created as a copy by
  QAPISchemaObjectVariants.check() (since the members of
  one variant cannot collide with those from another), for
  use in checking for collisions (technically, we no longer
  need to check for collisions between tag values and QMP
  key names, but that's a cleanup for another patch)

QAPISchemaAlternateType.check():
  - all_members is a throwaway empty list
  - seen is a throwaway empty dict

Therefore, in the one case where we care about all_members
after seen has been populated, we know that it contains the
same members as seen.values(); changing seen to be an
OrderedDict() is sufficient to pick up this information with
one less parameter being passed around.

Signed-off-by: Eric Blake 
---
v9: new patch, split off from v8 7/17
---
 scripts/qapi.py | 29 +
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 6b6ba5f..fff4adb 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -972,28 +972,26 @@ class QAPISchemaObjectType(QAPISchemaType):
 self.variants = variants
 self.members = None

+# Finish construction, and validate that all members are usable
 def check(self, schema):
 assert self.members is not False# not running in cycles
 if self.members:
 return
 self.members = False# mark as being checked
+seen = OrderedDict()
 if self._base_name:
 self.base = schema.lookup_type(self._base_name)
 assert isinstance(self.base, QAPISchemaObjectType)
 assert not self.base.variants   # not implemented
 self.base.check(schema)
-members = list(self.base.members)
-else:
-members = []
-seen = {}
-for m in members:
-assert c_name(m.name) not in seen
-seen[m.name] = m
+for m in self.base.members:
+assert c_name(m.name) not in seen
+seen[m.name] = m
 for m in self.local_members:
-m.check(schema, members, seen)
+m.check(schema, seen)
 if self.variants:
-self.variants.check(schema, members, seen)
-self.members = members
+self.variants.check(schema, seen)
+self.members = seen.values()

 def is_implicit(self):
 # See QAPISchema._make_implicit_object_type()
@@ -1027,11 +1025,10 @@ class QAPISchemaObjectTypeMember(object):
 self.type = None
 self.optional = optional

-def check(self, schema, all_members, seen):
+def check(self, schema, seen):
 assert self.name not in seen
 self.type = schema.lookup_type(self._type_name)
 assert self.type
-all_members.append(self)
 seen[self.name] = self


@@ -1050,7 +1047,7 @@ class QAPISchemaObjectTypeVariants(object):
 self.tag_member = tag_member
 self.variants = variants

-def check(self, schema, members, seen):
+def check(self, schema, seen):
 if self.tag_name:# flat union
 self.tag_member = seen[self.tag_name]
 assert self.tag_member
@@ -1067,7 +1064,7 @@ class 
QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
 QAPISchemaObjectTypeMember.__init__(self, name, typ, False)

 def check(self, schema, tag_type, seen):
-QAPISchemaObjectTypeMember.check(self, schema, [], seen)
+QAPISchemaObjectTypeMember.check(self, schema, seen)
 assert self.name in tag_type.values

 # This function exists to support ugly simple union special cases
@@ -1090,8 +1087,8 @@ class QAPISchemaAlternateType(QAPISchemaType):

 def check(self, schema):
 seen = {}
-self.variants.tag_member.check(schema, [], seen)
-self.variants.check(schema, [], seen)
+self.variants.tag_member.check(schema, seen)
+self.variants.check(schema, seen)

 def json_type(self):
 return 'value'
-- 
2.4.3

[Qemu-devel] SMM error in 2.4 changelog

[William Dauchy]

Hello,

I think there might be a mistake in the 2.4 changelog:
http://wiki.qemu.org/ChangeLog/2.4
"Support for system management mode (requires Linux 4.1)."

But I believe it's included in Linux v4.2 see
https://lwn.net/Articles/648995/ for the merge window.

Sorry for the noise if it is not the right way to report a
wiki/changelog mistake.

Best regards,
-- 
William

Re: [Qemu-devel] [PATCH v2] seccomp: add cacheflush to whitelist

[Andrew Jones]

On Mon, Nov 02, 2015 at 08:37:15PM +, Peter Maydell wrote:
> On 2 November 2015 at 19:04, Andrew Jones  wrote:
> > On Mon, Nov 02, 2015 at 06:09:41PM +, Peter Maydell wrote:
> >> On 2 November 2015 at 17:56, Andrew Jones  wrote:
> >> > cacheflush is an arm-specific syscall that qemu built for arm
> >> > uses. Add it to the whitelist, but only if we're linking with
> >> > a recent enough libseccomp.
> >> >
> >> > Signed-off-by: Andrew Jones 
> >> > ---
> >> > v2: only add cacheflush if libseccomp supports it
> >> >
> >> >  qemu-seccomp.c | 9 -
> >> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> >> > index 80d034a8d5190..e76097e958779 100644
> >> > --- a/qemu-seccomp.c
> >> > +++ b/qemu-seccomp.c
> >> > @@ -16,6 +16,10 @@
> >> >  #include 
> >> >  #include "sysemu/seccomp.h"
> >> >
> >> > +#if SCMP_VER_MAJOR >= 2 && SCMP_VER_MINOR >= 2 && SCMP_VER_MICRO >= 3
> >> > +#define HAVE_CACHEFLUSH
> >> > +#endif
> >>
> >> This will claim that a hypothetical future version 3.0.0 does not
> >> have cacheflush...
> >
> > Indeed. Sigh... In that case, how about just
> >
> > #if defined(TARGET_ARM) || defined(TARGET_AARCH64)
> > { SCMP_SYS(cacheflush), 240 },
> > #endif
> 
> You want to be checking based on the host architecture,
> not the target architecture. Also, not doing the check based
> on seccomp version means there's no hint in the code that the
> ifdefs become obsolete if we raise our cross-architecture
> minimum seccomp version requirement in the future, so really
> a version check is better I think.

Right. OK, I'll stop flinging junk and pull a better version
check together.

Thanks,
drew

> 
> thanks
> -- PMM
> 

[Qemu-devel] anybody using MMIO tracing?

[Hollis Blanchard]

I'm trying to use the memory_region_ops_read/write tracepoints. They 
produce output like this:


   memory_region_ops_write 0.000 pid=8861 mr=0x185b1e8 addr=0x0
   value=0x3 size=0x4
   memory_region_ops_write 165.000 pid=8861 mr=0x185b1e8 addr=0x80
   value=0x size=0x4
   memory_region_ops_write 155.000 pid=8861 mr=0x1914240 addr=0x0
   value=0x3 size=0x4
   memory_region_ops_write 2.000 pid=8861 mr=0x185b320 addr=0x0
   value=0x3 size=0x4
   memory_region_ops_write 134.000 pid=8861 mr=0x1914240 addr=0x4
   value=0x80 size=0x4

How do I discover which devices are represented by MemoryRegions 
0x185b1e8 and 0x1914240? Or alternatively how do I discover the full 
addresses? Thanks.


--
Hollis Blanchard
Mentor Graphics Emulation Division

Re: [Qemu-devel] [PATCH v8 07/17] qapi: Rework collision assertions

[Eric Blake]

On 11/02/2015 08:37 AM, Markus Armbruster wrote:

> 
> Not checked: variant's members don't collide with non-variant members.
> I think this check got lost when we simplified
> QAPISchemaObjectTypeVariants to hold a single member.

Yep, I found the culprit: in your v2 proposal for QAPISchema, you had:

+class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
+def __init__(self, name, typ, flat):
+QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
+assert isinstance(flat, bool)
+self.flat = flat
+def check(self, schema, tag_type, seen):
+QAPISchemaObjectTypeMember.check(self, schema, [], seen)
+assert self.name in tag_type.values
+if self.flat:
+self.type.check(schema)
+assert isinstance(self.type, QAPISchemaObjectType)

https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg00394.html

but the 'if self.flat' clause was lost in v3:

https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg00450.html

I am in fact reinstating it here, but for v9, will do it in a separate
patch rather than blended in with the rest of the changes.

[wow - we've been hammering at this since July?]

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [RFC v2] Add support for KVM_CAP_SPLIT_IRQCHIP

[Matt Gingell]

Hi Jan, 

Would you be able to look this over? I’d like to get this into shape to commit, 
either now or once the corresponding kernel patch goes in.

Thanks,
Matt

Add support for KVM_CAP_SPLIT_IRQCHIP

Adds a new alternative 'split' to the -machine kernel-irqchip option.
When split mode is specified:

 1.) KVM_CAP_SPLIT_IRQCHIP is enabled.

 2.) The PIC, PIT, and IOAPIC are implemented in userspace while the
 LAPIC is implemented by KVM.

 3.) The software IOAPIC delivers interrupts to the KVM LAPIC via
 kvm_set_irq. Interrupt delivery is configured via the MSI routing
 table, for which routes are reserved in target-i386/kvm.c then
 configured in hw/intc/ioapic.c

 4.) KVM delivers IOAPIC EOIs via a new exit KVM_EXIT_IOAPIC_EOI,
 which is handled in target-i386/kvm.c and relayed to the software
 IOAPIC via ioapic_eoi_broadcast.
---
 hw/core/machine.c | 48 
 hw/i386/pc.c  |  7 --
 hw/i386/pc_piix.c |  4 +--
 hw/intc/ioapic.c  | 63 +--
 include/hw/boards.h   |  2 ++
 include/hw/i386/pc.h  | 11 +
 include/sysemu/kvm.h  | 13 ++
 kvm-all.c | 34 -
 linux-headers/linux/kvm.h | 10 ++--
 qapi/common.json  | 16 
 qemu-options.hx   |  3 ++-
 target-i386/cpu.c |  3 ++-
 target-i386/kvm.c | 25 +--
 13 files changed, 206 insertions(+), 33 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index f4db340..3c14e78 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -11,6 +11,7 @@
  */
 
 #include "hw/boards.h"
+#include "qapi-visit.h"
 #include "qapi/visitor.h"
 #include "hw/sysbus.h"
 #include "sysemu/sysemu.h"
@@ -31,12 +32,34 @@ static void machine_set_accel(Object *obj, const char 
*value, Error **errp)
 ms->accel = g_strdup(value);
 }
 
-static void machine_set_kernel_irqchip(Object *obj, bool value, Error **errp)
+static void machine_set_kernel_irqchip(Object *obj, Visitor *v,
+   void *opaque, const char *name,
+   Error **errp)
 {
-MachineState *ms = MACHINE(obj);
-
-ms->kernel_irqchip_allowed = value;
-ms->kernel_irqchip_required = value;
+OnOffSplit mode;
+MachineState *ms = MACHINE(obj);
+
+visit_type_OnOffSplit(v, &mode, name, errp);
+switch (mode) {
+case ON_OFF_SPLIT_ON:
+ms->kernel_irqchip_allowed = true;
+ms->kernel_irqchip_required = true;
+ms->kernel_irqchip_split = false;
+break;
+case ON_OFF_SPLIT_OFF:
+ms->kernel_irqchip_allowed = false;
+ms->kernel_irqchip_required = false;
+ms->kernel_irqchip_split = false;
+break;
+case ON_OFF_SPLIT_SPLIT:
+ms->kernel_irqchip_allowed = true;
+ms->kernel_irqchip_required = true;
+ms->kernel_irqchip_split = true;
+break;
+default:
+error_report("Option 'kernel-irqchip' must be one of on|off|split");
+exit(1);
+}
 }
 
 static void machine_get_kvm_shadow_mem(Object *obj, Visitor *v,
@@ -341,12 +364,12 @@ static void machine_initfn(Object *obj)
 object_property_set_description(obj, "accel",
 "Accelerator list",
 NULL);
-object_property_add_bool(obj, "kernel-irqchip",
- NULL,
- machine_set_kernel_irqchip,
- NULL);
+object_property_add(obj, "kernel-irqchip", "OnOffSplit",
+NULL,
+machine_set_kernel_irqchip,
+NULL, NULL, NULL);
 object_property_set_description(obj, "kernel-irqchip",
-"Use KVM in-kernel irqchip",
+"Configure KVM in-kernel irqchip",
 NULL);
 object_property_add(obj, "kvm-shadow-mem", "int",
 machine_get_kvm_shadow_mem,
@@ -477,6 +500,11 @@ bool machine_kernel_irqchip_required(MachineState *machine)
 return machine->kernel_irqchip_required;
 }
 
+bool machine_kernel_irqchip_split(MachineState *machine)
+{
+return machine->kernel_irqchip_split;
+}
+
 int machine_kvm_shadow_mem(MachineState *machine)
 {
 return machine->kvm_shadow_mem;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index efbd41a..13db224 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -66,6 +66,7 @@
 #include "hw/mem/pc-dimm.h"
 #include "qapi/visitor.h"
 #include "qapi-visit.h"
+#include "qom/cpu.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ
@@ -1530,10 +1531,11 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq 
*gsi,
 qemu_register_boot_set(pc_boot_set, *rtc_state);
 
 if (!xen_enabled()) {
-if (kvm_irqchip_in_kernel()) {
+if (kvm_pit_i

Re: [Qemu-devel] [PATCH v7 09/35] exec: allow file_ram_alloc to work on file

[Paolo Bonzini]

On 02/11/2015 10:13, Xiao Guangrong wrote:
> Currently, file_ram_alloc() only works on directory - it creates a file
> under @path and do mmap on it
> 
> This patch tries to allow it to work on file directly, if @path is a
> directory it works as before, otherwise it treats @path as the target
> file then directly allocate memory from it
> 
> Signed-off-by: Xiao Guangrong 
> ---
>  exec.c | 80 
> ++
>  1 file changed, 51 insertions(+), 29 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 9075f4d..db0fdaf 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1174,14 +1174,60 @@ void qemu_mutex_unlock_ramlist(void)
>  }
>  
>  #ifdef __linux__
> +static bool path_is_dir(const char *path)
> +{
> +struct stat fs;
> +
> +return stat(path, &fs) == 0 && S_ISDIR(fs.st_mode);
> +}
> +
> +static int open_ram_file_path(RAMBlock *block, const char *path, size_t size)
> +{
> +char *filename;
> +char *sanitized_name;
> +char *c;
> +int fd;
> +
> +if (!path_is_dir(path)) {
> +int flags = (block->flags & RAM_SHARED) ? O_RDWR : O_RDONLY;
> +
> +flags |= O_EXCL;
> +return open(path, flags);
> +}
> +
> +/* Make name safe to use with mkstemp by replacing '/' with '_'. */
> +sanitized_name = g_strdup(memory_region_name(block->mr));
> +for (c = sanitized_name; *c != '\0'; c++) {
> +if (*c == '/') {
> +*c = '_';
> +}
> +}
> +filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
> +   sanitized_name);
> +g_free(sanitized_name);
> +fd = mkstemp(filename);
> +if (fd >= 0) {
> +unlink(filename);
> +/*
> + * ftruncate is not supported by hugetlbfs in older
> + * hosts, so don't bother bailing out on errors.
> + * If anything goes wrong with it under other filesystems,
> + * mmap will fail.
> + */
> +if (ftruncate(fd, size)) {
> +perror("ftruncate");
> +}
> +}
> +g_free(filename);
> +
> +return fd;
> +}
> +
>  static void *file_ram_alloc(RAMBlock *block,
>  ram_addr_t memory,
>  const char *path,
>  Error **errp)
>  {
> -char *filename;
> -char *sanitized_name;
> -char *c;
>  void *area;
>  int fd;
>  uint64_t pagesize;
> @@ -1212,38 +1258,14 @@ static void *file_ram_alloc(RAMBlock *block,
>  goto error;
>  }
>  
> -/* Make name safe to use with mkstemp by replacing '/' with '_'. */
> -sanitized_name = g_strdup(memory_region_name(block->mr));
> -for (c = sanitized_name; *c != '\0'; c++) {
> -if (*c == '/')
> -*c = '_';
> -}
> -
> -filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
> -   sanitized_name);
> -g_free(sanitized_name);
> +memory = ROUND_UP(memory, pagesize);
>  
> -fd = mkstemp(filename);
> +fd = open_ram_file_path(block, path, memory);
>  if (fd < 0) {
>  error_setg_errno(errp, errno,
>   "unable to create backing store for path %s", path);
> -g_free(filename);
>  goto error;
>  }
> -unlink(filename);
> -g_free(filename);
> -
> -memory = ROUND_UP(memory, pagesize);
> -
> -/*
> - * ftruncate is not supported by hugetlbfs in older
> - * hosts, so don't bother bailing out on errors.
> - * If anything goes wrong with it under other filesystems,
> - * mmap will fail.
> - */
> -if (ftruncate(fd, memory)) {
> -perror("ftruncate");
> -}
>  
>  area = qemu_ram_mmap(fd, memory, pagesize, block->flags & RAM_SHARED);
>  if (area == MAP_FAILED) {
> 

I was going to send tomorrow a pull request for a similar patch,
"backends/hostmem-file: Allow to specify full pathname for backing file".

The main difference seems to be your usage of O_EXCL.  Can you explain
why you added it?

Paolo

Re: [Qemu-devel] [PATCH v10 00/14] block: incremental backup transactions using BlockJobTxn

[John Snow]

Ping!

To clarify, I *do* intend this series to be for-2.5, and I think it does
qualify as such.

If this *doesn't* go in, we have to revise our documentation just a
pinch to undo some of Kashyap's changes that have already been merged in.

Patches 5, 10, 11 and 12 are awaiting review.

--js

On 10/23/2015 07:56 PM, John Snow wrote:
> Welcome to V10!
> 
> Where'd 8 and 9 go? Private off-list missives from Fam.
> Now you, I, and everyone on qemu-devel are staring at V10.
> 
> What's new in V10?
> 
> I replaced the per-action "transactional-cancel" parameter with
> a per-transaction paremeter named "err-cancel" which is implemented
> as an enum in case we want to add new behaviors in the future, such
> as a "jobs only" cancel mode.
> 
> For now, it's "all" or "none", and if you use it with actions that
> do not support the latent transactional cancel, you will receive
> an error for your troubles.
> 
> This version primarily changed patches 10,11 and replaced it with
> patches 10-12 that are cut a little differently.
> 
> This is based on top of the work by Stefan Hajnoczi and Fam Zheng.
> 
> Recap: motivation for block job transactions
> 
> If an incremental backup block job fails then we reclaim the bitmap so
> the job can be retried.  The problem comes when multiple jobs are started as
> part of a qmp 'transaction' command.  We need to group these jobs in a
> transaction so that either all jobs complete successfully or all bitmaps are
> reclaimed.
> 
> Without transactions, there is a case where some jobs complete successfully 
> and
> throw away their bitmaps, making it impossible to retry the backup by 
> rerunning
> the command if one of the jobs fails.
> 
> How does this implementation work?
> --
> These patches add a BlockJobTxn object with the following API:
> 
>   txn = block_job_txn_new();
>   block_job_txn_add_job(txn, job1);
>   block_job_txn_add_job(txn, job2);
> 
> The jobs either both complete successfully or they both fail/cancel.  If the
> user cancels job1 then job2 will also be cancelled and vice versa.
> 
> Jobs objects stay alive waiting for other jobs to complete, even if the
> coroutines have returned.  They can be cancelled by the user during this time.
> Job blockers are still in effect and no other block job can run on this device
> in the meantime (since QEMU currently only allows 1 job per device).  This is
> the main drawback to this approach but reasonable since you probably don't 
> want
> to run other jobs/operations until you're sure the backup was successful (you
> won't be able to retry a failed backup if there's a new job running).
> 
> [History]
> 
> v10: Replaced per-action parameter with per-transaction properties.
>  Patches 10,11 were split into 10-12.
> 
> v9: this version fixes a reference count problem with job->bs,
> in patch 05.
> 
> v8: Rebase on to master.
> Minor fixes addressing John Snow's comments.
> 
> v7: Add Eric's rev-by in 1, 11.
> Add Max's rev-by in 4, 5, 9, 10, 11.
> Add John's rev-by in 5, 6, 8.
> Fix wording for 6. [John]
> Fix comment of block_job_txn_add_job() in 9. [Max]
> Remove superfluous hunks, and document default value in 11. [Eric]
> Update Makefile dep in 14. [Max]
> 
> 
> 
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch block-transpop
> https://github.com/jnsnow/qemu/tree/block-transpop
> 
> This version is tagged block-transpop-v10:
> https://github.com/jnsnow/qemu/releases/tag/block-transpop-v10
> 
> Fam Zheng (6):
>   backup: Extract dirty bitmap handling as a separate function
>   blockjob: Introduce reference count and fix reference to job->bs
>   blockjob: Add .commit and .abort block job actions
>   blockjob: Add "completed" and "ret" in BlockJob
>   blockjob: Simplify block_job_finish_sync
>   block: Add block job transactions
> 
> John Snow (7):
>   qapi: Add transaction support to block-dirty-bitmap operations
>   iotests: add transactional incremental backup test
>   block: rename BlkTransactionState and BdrvActionOps
>   block/backup: Rely on commit/abort for cleanup
>   block: Add BlockJobTxn support to backup_run
>   block: add transactional properties
>   iotests: 124 - transactional failure test
> 
> Stefan Hajnoczi (1):
>   tests: add BlockJobTxn unit test
> 
>  block.c|  19 +-
>  block/backup.c |  50 --
>  block/mirror.c |   2 +-
>  blockdev.c | 430 
> ++---
>  blockjob.c | 188 
>  docs/bitmaps.md|   6 +-
>  include/block/block.h  |   2 +-
>  include/block/block_int.h  |   6 +-
>  include/block/blockjob.h   |  85 -
>  qapi-schema.json   |  54 +-
>  qemu-img.c |   3 -
>  qmp-commands.hx

Re: [Qemu-devel] [PATCH v2 2/4] fw_cfg: amend callback behavior spec to once per select

[Gabriel L. Somlo]

On Mon, Nov 02, 2015 at 03:16:47PM +0100, Laszlo Ersek wrote:
> Comments below:
> 
> On 10/28/15 18:20, Gabriel L. Somlo wrote:
> > Currently, the fw_cfg internal API specifies that if an item was set up
> > with a read callback, the callback must be run each time a byte is read
> > from the item. This behavior is both wasteful (most items do not have a
> > read callback set), and impractical for bulk transfers (e.g., DMA read).
> > 
> > At the time of this writing, the only items configured with a callback
> > are "/etc/table-loader", "/etc/acpi/tables", and "/etc/acpi/rsdp". They
> > all share the same callback functions: virt_acpi_build_update() on arm
> 
> (1) I suggest "ARM".

OK.

> 
> > (in hw/arm/virt-acpi-build.c), and acpi_build_update() on i386 (in
> > hw/i386/acpi.c). Both of these callbacks are one-shot (i.e. they return
> > without doing anything at all after the first time they are called on
> > each distinct item).
> 
> (2) Shouldn't this be:
> 
> ... after the first time they are called, regardless of the
> associated item being read

Ha, you're right -- build-state is shared across all blobs, so the
callback only really fires once for the whole guest VM.

Thanks for catching that !

> > 
> > This patch amends the specification for fw_cfg_add_file_callback() to
> > state that any available read callback will only be invoked once each
> > time the item is selected. This change has no practical effect on the
> > current behavior of QEMU, and it enables us to significantly optimize
> > the behavior of fw_cfg reads during guest firmware setup, eliminating
> > a large amount of redundant callback checks and invocations.
> > 
> > Cc: Laszlo Ersek 
> > Cc: Gerd Hoffmann 
> > Cc: Marc Marí 
> > Signed-off-by: Gabriel Somlo 
> > ---
> >  hw/nvram/fw_cfg.c | 16 
> >  include/hw/nvram/fw_cfg.h |  8 ++--
> >  2 files changed, 10 insertions(+), 14 deletions(-)
> > 
> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > index 73b0a81..31fa5c8 100644
> > --- a/hw/nvram/fw_cfg.c
> > +++ b/hw/nvram/fw_cfg.c
> > @@ -252,7 +252,8 @@ static void fw_cfg_write(FWCfgState *s, uint8_t value)
> >  
> >  static int fw_cfg_select(FWCfgState *s, uint16_t key)
> >  {
> > -int ret;
> > +int arch, ret;
> > +FWCfgEntry *e;
> >  
> >  s->cur_offset = 0;
> >  if ((key & FW_CFG_ENTRY_MASK) >= FW_CFG_MAX_ENTRY) {
> > @@ -261,6 +262,12 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key)
> >  } else {
> >  s->cur_entry = key;
> >  ret = 1;
> > +/* entry successfully selected, now run callback if present */
> > +arch = !!(key & FW_CFG_ARCH_LOCAL);
> > +e = &s->entries[arch][key & FW_CFG_ENTRY_MASK];
> 
> Seems to match the logic in fw_cfg_read().
> 
> > +if (e->read_callback) {
> > +e->read_callback(e->callback_opaque, s->cur_offset);
> > +}
> 
> The offset is constant 0 here, but that's fine.
> 
> >  }
> >  
> >  trace_fw_cfg_select(s, key, ret);
> > @@ -276,9 +283,6 @@ static uint8_t fw_cfg_read(FWCfgState *s)
> >  if (s->cur_entry == FW_CFG_INVALID || !e->data || s->cur_offset >= 
> > e->len)
> >  ret = 0;
> >  else {
> > -if (e->read_callback) {
> > -e->read_callback(e->callback_opaque, s->cur_offset);
> > -}
> >  ret = e->data[s->cur_offset++];
> >  }
> >  
> > @@ -371,10 +375,6 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
> >  len = (e->len - s->cur_offset);
> >  }
> >  
> > -if (e->read_callback) {
> > -e->read_callback(e->callback_opaque, s->cur_offset);
> > -}
> > -
> >  /* If the access is not a read access, it will be a skip 
> > access,
> >   * tested before.
> >   */
> > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> > index 422e2e9..47ff118 100644
> > --- a/include/hw/nvram/fw_cfg.h
> > +++ b/include/hw/nvram/fw_cfg.h
> > @@ -183,12 +183,8 @@ void fw_cfg_add_file(FWCfgState *s, const char 
> > *filename, void *data,
> >   * structure residing at key value FW_CFG_FILE_DIR, containing the item 
> > name,
> >   * data size, and assigned selector key value.
> >   * Additionally, set a callback function (and argument) to be called each
> > - * time a byte is read by the guest from this particular item, or once per
> > - * each DMA guest read operation.
> > - * NOTE: In addition to the opaque argument set here, the callback function
> > - * takes the current data offset as an additional argument, allowing it the
> > - * option of only acting upon specific offset values (e.g., 0, before the
> > - * first data byte of the selected item is returned to the guest).
> > + * time this item is selected (by having its selector key written to the
> > + * fw_cfg control register).
> 
> (3) This should be more precise. Selection doesn't only occur via an
> explicit write to the control register. Kevin sugg

Re: [Qemu-devel] [PATCH v2 1/4] fw_cfg: move internal function call docs to header file

[Laszlo Ersek]

On 11/02/15 21:36, Gabriel L. Somlo wrote:
> On Mon, Nov 02, 2015 at 02:41:58PM +0100, Laszlo Ersek wrote:
>> Three (well, two n' half) comments:
>>
>> On 10/28/15 18:20, Gabriel L. Somlo wrote:
>>> Move documentation for fw_cfg functions internal to qemu from
>>> docs/specs/fw_cfg.txt to the fw_cfg.h header file, next to their
>>> prototype declarations, formatted as doc-comments.
>>>
>>> Suggested-by: Peter Maydell 
>>> Cc: Laszlo Ersek 
>>> Cc: Gerd Hoffmann 
>>> Cc: Marc Marí 
>>> Cc: Jordan Justen 
>>> Cc: Paolo Bonzini 
>>> Cc: Peter Maydell 
>>> Signed-off-by: Gabriel Somlo 
>>> ---
>>>  docs/specs/fw_cfg.txt |  85 +-
>>>  include/hw/nvram/fw_cfg.h | 128 
>>> ++
>>>  2 files changed, 129 insertions(+), 84 deletions(-)
>>>
>>> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
>>> index b8c794f..2099ad9 100644
>>> --- a/docs/specs/fw_cfg.txt
>>> +++ b/docs/specs/fw_cfg.txt
>>> @@ -192,90 +192,7 @@ To check the result, read the "control" field:
>>>  today due to implementation not being async,
>>>  but may in the future).
>>>  
>>> -= Host-side API =
>>> -
>>> -The following functions are available to the QEMU programmer for adding
>>> -data to a fw_cfg device during guest initialization (see fw_cfg.h for
>>> -each function's complete prototype):
>>> -
>>> -== fw_cfg_add_bytes() ==
>>> -
>>> -Given a selector key value, starting pointer, and size, create an item
>>> -as a raw "blob" of the given size, available by selecting the given key.
>>> -The data referenced by the starting pointer is only linked, NOT copied,
>>> -into the data structure of the fw_cfg device.
>>> -
>>> -== fw_cfg_add_string() ==
>>> -
>>> -Instead of a starting pointer and size, this function accepts a pointer
>>> -to a NUL-terminated ascii string, and inserts a newly allocated copy of
>>> -the string (including the NUL terminator) into the fw_cfg device data
>>> -structure.
>>> -
>>> -== fw_cfg_add_iXX() ==
>>> -
>>> -Insert an XX-bit item, where XX may be 16, 32, or 64. These functions
>>> -will convert a 16-, 32-, or 64-bit integer to little-endian, then add
>>> -a dynamically allocated copy of the appropriately sized item to fw_cfg
>>> -under the given selector key value.
>>> -
>>> -== fw_cfg_modify_iXX() ==
>>> -
>>> -Modify the value of an XX-bit item (where XX may be 16, 32, or 64).
>>> -Similarly to the corresponding fw_cfg_add_iXX() function set, convert
>>> -a 16-, 32-, or 64-bit integer to little endian, create a dynamically
>>> -allocated copy of the required size, and replace the existing item at
>>> -the given selector key value with the newly allocated one. The previous
>>> -item, assumed to have been allocated during an earlier call to
>>> -fw_cfg_add_iXX() or fw_cfg_modify_iXX() (of the same width XX), is freed
>>> -before the function returns.
>>> -
>>> -== fw_cfg_add_file() ==
>>> -
>>> -Given a filename (i.e., fw_cfg item name), starting pointer, and size,
>>> -create an item as a raw "blob" of the given size. Unlike fw_cfg_add_bytes()
>>> -above, the next available selector key (above 0x0020, FW_CFG_FILE_FIRST)
>>> -will be used, and a new entry will be added to the file directory structure
>>> -(at key 0x0019), containing the item name, blob size, and automatically
>>> -assigned selector key value. The data referenced by the starting pointer
>>> -is only linked, NOT copied, into the fw_cfg data structure.
>>> -
>>> -== fw_cfg_add_file_callback() ==
>>> -
>>> -Like fw_cfg_add_file(), but additionally sets pointers to a callback
>>> -function (and opaque argument), which will be executed host-side by
>>> -QEMU each time a byte is read by the guest from this particular item.
>>> -
>>> -NOTE: The callback function is given the opaque argument set by
>>> -fw_cfg_add_file_callback(), but also the current data offset,
>>> -allowing it the option of only acting upon specific offset values
>>> -(e.g., 0, before the first data byte of the selected item is
>>> -returned to the guest).
>>> -
>>> -== fw_cfg_modify_file() ==
>>> -
>>> -Given a filename (i.e., fw_cfg item name), starting pointer, and size,
>>> -completely replace the configuration item referenced by the given item
>>> -name with the new given blob. If an existing blob is found, its
>>> -callback information is removed, and a pointer to the old data is
>>> -returned to allow the caller to free it, helping avoid memory leaks.
>>> -If a configuration item does not already exist under the given item
>>> -name, a new item will be created as with fw_cfg_add_file(), and NULL
>>> -is returned to the caller. In any case, the data referenced by the
>>> -starting pointer is only linked, NOT copied, into the fw_cfg data
>>> -structure.
>>> -
>>> -== fw_cfg_add_callback() ==
>>> -
>>> -Like fw_cfg_add_bytes(), but additionally sets pointers to a callback
>>> -function (and opaque argument), which will be executed host-side by
>>> -

Re: [Qemu-devel] [PATCH v2 1/4] fw_cfg: move internal function call docs to header file

[Gabriel L. Somlo]

On Mon, Nov 02, 2015 at 02:41:58PM +0100, Laszlo Ersek wrote:
> Three (well, two n' half) comments:
> 
> On 10/28/15 18:20, Gabriel L. Somlo wrote:
> > Move documentation for fw_cfg functions internal to qemu from
> > docs/specs/fw_cfg.txt to the fw_cfg.h header file, next to their
> > prototype declarations, formatted as doc-comments.
> > 
> > Suggested-by: Peter Maydell 
> > Cc: Laszlo Ersek 
> > Cc: Gerd Hoffmann 
> > Cc: Marc Marí 
> > Cc: Jordan Justen 
> > Cc: Paolo Bonzini 
> > Cc: Peter Maydell 
> > Signed-off-by: Gabriel Somlo 
> > ---
> >  docs/specs/fw_cfg.txt |  85 +-
> >  include/hw/nvram/fw_cfg.h | 128 
> > ++
> >  2 files changed, 129 insertions(+), 84 deletions(-)
> > 
> > diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> > index b8c794f..2099ad9 100644
> > --- a/docs/specs/fw_cfg.txt
> > +++ b/docs/specs/fw_cfg.txt
> > @@ -192,90 +192,7 @@ To check the result, read the "control" field:
> >  today due to implementation not being async,
> >  but may in the future).
> >  
> > -= Host-side API =
> > -
> > -The following functions are available to the QEMU programmer for adding
> > -data to a fw_cfg device during guest initialization (see fw_cfg.h for
> > -each function's complete prototype):
> > -
> > -== fw_cfg_add_bytes() ==
> > -
> > -Given a selector key value, starting pointer, and size, create an item
> > -as a raw "blob" of the given size, available by selecting the given key.
> > -The data referenced by the starting pointer is only linked, NOT copied,
> > -into the data structure of the fw_cfg device.
> > -
> > -== fw_cfg_add_string() ==
> > -
> > -Instead of a starting pointer and size, this function accepts a pointer
> > -to a NUL-terminated ascii string, and inserts a newly allocated copy of
> > -the string (including the NUL terminator) into the fw_cfg device data
> > -structure.
> > -
> > -== fw_cfg_add_iXX() ==
> > -
> > -Insert an XX-bit item, where XX may be 16, 32, or 64. These functions
> > -will convert a 16-, 32-, or 64-bit integer to little-endian, then add
> > -a dynamically allocated copy of the appropriately sized item to fw_cfg
> > -under the given selector key value.
> > -
> > -== fw_cfg_modify_iXX() ==
> > -
> > -Modify the value of an XX-bit item (where XX may be 16, 32, or 64).
> > -Similarly to the corresponding fw_cfg_add_iXX() function set, convert
> > -a 16-, 32-, or 64-bit integer to little endian, create a dynamically
> > -allocated copy of the required size, and replace the existing item at
> > -the given selector key value with the newly allocated one. The previous
> > -item, assumed to have been allocated during an earlier call to
> > -fw_cfg_add_iXX() or fw_cfg_modify_iXX() (of the same width XX), is freed
> > -before the function returns.
> > -
> > -== fw_cfg_add_file() ==
> > -
> > -Given a filename (i.e., fw_cfg item name), starting pointer, and size,
> > -create an item as a raw "blob" of the given size. Unlike fw_cfg_add_bytes()
> > -above, the next available selector key (above 0x0020, FW_CFG_FILE_FIRST)
> > -will be used, and a new entry will be added to the file directory structure
> > -(at key 0x0019), containing the item name, blob size, and automatically
> > -assigned selector key value. The data referenced by the starting pointer
> > -is only linked, NOT copied, into the fw_cfg data structure.
> > -
> > -== fw_cfg_add_file_callback() ==
> > -
> > -Like fw_cfg_add_file(), but additionally sets pointers to a callback
> > -function (and opaque argument), which will be executed host-side by
> > -QEMU each time a byte is read by the guest from this particular item.
> > -
> > -NOTE: The callback function is given the opaque argument set by
> > -fw_cfg_add_file_callback(), but also the current data offset,
> > -allowing it the option of only acting upon specific offset values
> > -(e.g., 0, before the first data byte of the selected item is
> > -returned to the guest).
> > -
> > -== fw_cfg_modify_file() ==
> > -
> > -Given a filename (i.e., fw_cfg item name), starting pointer, and size,
> > -completely replace the configuration item referenced by the given item
> > -name with the new given blob. If an existing blob is found, its
> > -callback information is removed, and a pointer to the old data is
> > -returned to allow the caller to free it, helping avoid memory leaks.
> > -If a configuration item does not already exist under the given item
> > -name, a new item will be created as with fw_cfg_add_file(), and NULL
> > -is returned to the caller. In any case, the data referenced by the
> > -starting pointer is only linked, NOT copied, into the fw_cfg data
> > -structure.
> > -
> > -== fw_cfg_add_callback() ==
> > -
> > -Like fw_cfg_add_bytes(), but additionally sets pointers to a callback
> > -function (and opaque argument), which will be executed host-side by
> > -QEMU each time a guest-side write operation t

Re: [Qemu-devel] [PATCH v2] seccomp: add cacheflush to whitelist

[Peter Maydell]

On 2 November 2015 at 19:04, Andrew Jones  wrote:
> On Mon, Nov 02, 2015 at 06:09:41PM +, Peter Maydell wrote:
>> On 2 November 2015 at 17:56, Andrew Jones  wrote:
>> > cacheflush is an arm-specific syscall that qemu built for arm
>> > uses. Add it to the whitelist, but only if we're linking with
>> > a recent enough libseccomp.
>> >
>> > Signed-off-by: Andrew Jones 
>> > ---
>> > v2: only add cacheflush if libseccomp supports it
>> >
>> >  qemu-seccomp.c | 9 -
>> >  1 file changed, 8 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/qemu-seccomp.c b/qemu-seccomp.c
>> > index 80d034a8d5190..e76097e958779 100644
>> > --- a/qemu-seccomp.c
>> > +++ b/qemu-seccomp.c
>> > @@ -16,6 +16,10 @@
>> >  #include 
>> >  #include "sysemu/seccomp.h"
>> >
>> > +#if SCMP_VER_MAJOR >= 2 && SCMP_VER_MINOR >= 2 && SCMP_VER_MICRO >= 3
>> > +#define HAVE_CACHEFLUSH
>> > +#endif
>>
>> This will claim that a hypothetical future version 3.0.0 does not
>> have cacheflush...
>
> Indeed. Sigh... In that case, how about just
>
> #if defined(TARGET_ARM) || defined(TARGET_AARCH64)
> { SCMP_SYS(cacheflush), 240 },
> #endif

You want to be checking based on the host architecture,
not the target architecture. Also, not doing the check based
on seccomp version means there's no hint in the code that the
ifdefs become obsolete if we raise our cross-architecture
minimum seccomp version requirement in the future, so really
a version check is better I think.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 0/2] qemu-iotests: fix cleanup of background processes

[Max Reitz]

On 02.11.2015 21:31, Max Reitz wrote:
> On 30.10.2015 20:25, Jeff Cody wrote:
>> Changes from v2:
>>
>> * Pulled patch 2 into this series
>>
>> * Patch 1: Moved non-empty test conditionals inside
>>pid file existance check (thanks Max)
>>
>>Added a fix in for patch 058, for its
>>self-launched qemu-nbd instance (thanks Max)
>>
>> * Patch 2: Only print the valgrind logs if the exit error
>>matches (thanks Max) 
>>
>> Changes from v1:
>>
>> * use 'read' instead of 'cat' (thanks Eric)
>> * quote variable in variable test (thanks Eric)
>>
>> Jeff Cody (2):
>>   qemu-iotests: fix cleanup of background processes
>>   qemu-iotests: fix -valgrind option for check
>>
>>  tests/qemu-iotests/039.out   | 30 +-
>>  tests/qemu-iotests/058   | 12 
>>  tests/qemu-iotests/061.out   | 12 ++--
>>  tests/qemu-iotests/137.out   |  6 +-
>>  tests/qemu-iotests/common|  9 ++---
>>  tests/qemu-iotests/common.config | 32 +---
>>  tests/qemu-iotests/common.qemu   | 18 --
>>  tests/qemu-iotests/common.rc | 18 +-
>>  8 files changed, 96 insertions(+), 41 deletions(-)
> 
> Thanks, applied to my block tree with the three occurrences of ! -n in
> patch 1 changed to -z:

Err, of course it's the other way around, ! -z to -n.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3 0/2] qemu-iotests: fix cleanup of background processes

[Max Reitz]

On 30.10.2015 20:25, Jeff Cody wrote:
> Changes from v2:
> 
> * Pulled patch 2 into this series
> 
> * Patch 1: Moved non-empty test conditionals inside
>pid file existance check (thanks Max)
> 
>Added a fix in for patch 058, for its
>self-launched qemu-nbd instance (thanks Max)
> 
> * Patch 2: Only print the valgrind logs if the exit error
>matches (thanks Max) 
> 
> Changes from v1:
> 
> * use 'read' instead of 'cat' (thanks Eric)
> * quote variable in variable test (thanks Eric)
> 
> Jeff Cody (2):
>   qemu-iotests: fix cleanup of background processes
>   qemu-iotests: fix -valgrind option for check
> 
>  tests/qemu-iotests/039.out   | 30 +-
>  tests/qemu-iotests/058   | 12 
>  tests/qemu-iotests/061.out   | 12 ++--
>  tests/qemu-iotests/137.out   |  6 +-
>  tests/qemu-iotests/common|  9 ++---
>  tests/qemu-iotests/common.config | 32 +---
>  tests/qemu-iotests/common.qemu   | 18 --
>  tests/qemu-iotests/common.rc | 18 +-
>  8 files changed, 96 insertions(+), 41 deletions(-)

Thanks, applied to my block tree with the three occurrences of ! -n in
patch 1 changed to -z:

https://github.com/XanClic/qemu/commits/block


Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v8 32/54] Postcopy: Maintain sentmap and calculate discard

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> > +/*  functions for postcopy * */
> > +
> > +/*
> > + * Callback from postcopy_each_ram_send_discard for each RAMBlock
> > + * start,end: Indexes into the bitmap for the first and last bit
> > + *representing the named block
> > + */
> > +static int postcopy_send_discard_bm_ram(MigrationState *ms,
> > +PostcopyDiscardState *pds,
> > +unsigned long start, unsigned long 
> > end)
> > +{
> > +unsigned long current;
> > +
> > +for (current = start; current <= end; ) {
> > +unsigned long set = find_next_bit(ms->sentmap, end + 1, current);
> > +
> > +if (set <= end) {
> > +unsigned long zero = find_next_zero_bit(ms->sentmap,
> > +end + 1, set + 1);
> > +
> > +if (zero > end) {
> > +zero = end + 1;
> > +}
> > +postcopy_discard_send_range(ms, pds, set, zero - 1);
> > +current = zero + 1;
> > +} else {
> > +current = set;
> > +}
> > +}
> 
> I think I undrestood the logic  here at the end
> 
> But if we change the meaning of postcopy_discard_send_range() from
> (begin, end), to (begin, length), I think everything goes clearer, no?
> 
> if (set <= end) {
> unsigned long zero = find_next_zero_bit(ms->sentmap,
> end + 1, set + 1);
> unsigned long length;
> 
> if (zero > end) {
> length = end - set;
> } else {
> lenght = zero - 1 - set;
> current = zero + 1;
> }
> postcopy_discard_send_range(ms, pds, set, len);
> } else {
> current = set;
> }
> }
> 
> Y would clame that if we call one zero, the other would be called one.
> Or change to set/unset, but that is just me.  Yes, I haven't tested, and
> it is possible that there is a off-by-one somewhere...
> 
> Looking at postocpy_eand_ram_send_discard, I even think that it would be
> a good idea to pass length to this function.

OK, so I've ended up with (build tested only so far):
/*
 * Callback from postcopy_each_ram_send_discard for each RAMBlock
 * Note: At this point the 'unsentmap' is the processed bitmap combined
 *   with the dirtymap; so a '1' means it's either dirty or unsent.
 * start,length: Indexes into the bitmap for the first bit
 *representing the named block and length in target-pages
 */
static int postcopy_send_discard_bm_ram(MigrationState *ms,
PostcopyDiscardState *pds,
unsigned long start,
unsigned long length)
{
unsigned long end = start + length; /* one after the end */
unsigned long current;

for (current = start; current < end; ) {
unsigned long one = find_next_bit(ms->unsentmap, end, current);

if (one <= end) {
unsigned long zero = find_next_zero_bit(ms->unsentmap,
end, one + 1);
unsigned long discard_length;

if (zero >= end) {
discard_length = end - one;
} else {
discard_length = zero - one;
}
postcopy_discard_send_range(ms, pds, one, discard_length);
current = one + discard_length;
} else {
current = one;
}
}

return 0;
}

Dave

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

Re: [Qemu-devel] RFC: libyajl for JSON

[Eric Blake]

On 11/02/2015 12:10 PM, Markus Armbruster wrote:

>>> * Single-quoted strings

> 
>> So that is an absolute must for whatever parser we choose.  My first
>> glance at libyajl is that it does NOT currently allow single quotes (not
>> even with any of its existing options), so we'd have to pre-process
>> input before feeding it to yajl.

>>
>> I'll ask on the yajl mailing lists, to get a feel for community
>> receptiveness, before attempting to actually write patches on that front.
> 
> Makes sense.

On IRC, I got pointed to a couple of forks that have already done this,
such as:

https://github.com/ludocode/yajl/commits/master

although the upstream author Lloyd was not on the channel at the time.
Meanwhile, there's 47 open issues on the upstream repo:

https://github.com/lloyd/yajl/issues

which implies slow response by the author, but at least he WAS asking if
anyone would like to help him with maintenance:

https://github.com/lloyd/yajl/issues/161

|  lloyd commented on Sep 24
| ok, give me a minute to hand you a patch on a branch to review.

|  lloyd commented on Sep 24
| ok, I'll merge down and roll a new release within a day.

[still hasn't happened yet...]

|  lamont-granquist commented on Sep 25
| hey @lloyd would you consider adding other maintainers?

|  lloyd commented on Oct 1
| I would absolutely consider additional maintainers. Who's interested?

so I don't know what the future holds for extending things upstream.

To date, I don't have a github account, by conscious personal choice; so
I can't really take him up on that offer directly.  So far, I've been
trying the mailing list to see if he answers that in addition to the
github PR stream, but the archives show it to be pretty full of spam:
http://librelist.com/browser/yajl/

>>
>> Yes; the yajl parser has 4 modes of parse operation based on which of
>> three callbacks you implement: double-only (yajl_double), long long-only
>> (yajl_integer), double-and-int (both yajl_double and yajl_integer, not
>> sure which one has precedence if input satisfies both formats), or
>> custom number (yajl_number, which is given a string, and you turn it
>> into the format of your choice).  Likewise, the yajl formatter has 2
>> functions: yajl_gen_double() (formats doubles) and yajl_gen_number()
>> (you provide literal strings formatted how you like).
> 
> Good.

And one of the open bugs on the tracker was the same one we've
encountered ourselves: yajl is locale-sensitive when using strtod() for
parsing and when using printf() for printing doubles:

https://github.com/lloyd/yajl/issues/79

[I would love for C/POSIX to add strtod_l and printf_l, but that's a
thread for another day]

> 
>>> More extensions or pitfalls might be lurking in our parser.  Therefore,
>>> replacing our parser by a suitable library is not without risk.  I guess
>>> we could do it over a full development cycle.  No way for 2.5.
>>>
>>
>> Absolutely not for 2.5; we've missed softfreeze, and this would count as
>> a new feature.

Another interesting bug report against yajl: one reporter made the point
[1] that yajl is already a superset of the canonical RFC 4627 definition
of JSON [2], because while the RFC states that a JSON document has this
terminal state:
  JSON-text = object / array
yajl will accept _any_ JSON value (not just objects/arrays) as the
overall document.

[1] https://github.com/lloyd/yajl/issues/154
[2] https://www.ietf.org/rfc/rfc4627.txt

So at this point, I want to see if lloyd makes any progress towards an
actual yajl release and/or adding a co-maintainer, before even trying to
get formal upstream support for single quoting.  We could always create
a git submodule with our own choice of fork (since there are already
forks that do single-quote parsing) - but the mantra of 'upstream first'
has a lot of merit (I'm reluctant to fork without good reason).

-- 
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 v1 0/4] AHCI patches + Allwinner SATA

[John Snow]

On 10/27/2015 12:02 AM, Peter Crosthwaite wrote:
> This patch series adds bare-minimum Allwinner SATA support.
> 
> P1 is a trivial to help debug AHCI.
> 
> Changed since RFC:
> Addressed Beniamino review.
> Rebased to avoid bad deps (John Snow review)
> 
> Regards,
> Peter
> 
> 
> Peter Crosthwaite (4):
>   ahci: Add some MMIO debug printfs
>   ahci: split realize and init
>   ahci: Add allwinner AHCI
>   arm: allwinner-a10: Add SATA
> 
>  hw/arm/allwinner-a10.c |  11 +++
>  hw/ide/ahci.c  | 152 
> +++--
>  hw/ide/ahci.h  |  19 +-
>  hw/ide/ich.c   |  10 ++-
>  include/hw/arm/allwinner-a10.h |   4 ++
>  include/qemu/typedefs.h|   1 +
>  6 files changed, 176 insertions(+), 21 deletions(-)
> 

I patched up some minor context on 3/4.

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js

Re: [Qemu-devel] [PATCH] ide: remove hardcoded 2GiB transactional limit

[John Snow]

On 10/27/2015 12:50 PM, Stefan Hajnoczi wrote:
> On Mon, Oct 26, 2015 at 07:38:02PM -0400, John Snow wrote:
>> Not that you can request a >2GiB transaction, but that's why checking
>> for it makes no sense anymore.
>>
>> With the newer 'limit' parameter to prepare_buf, we no longer need a
>> static limit. The maximum limit is still 2GiB, but the limit parameter
>> is set to the current transaction size, which cannot surpass 32MiB
>> (512 * 65536). If the PRDT surpasses the transactional size, then,
>> we'll just carry out the normative underflow handling pathways instead
>> of needing an extra, strange pathway that worries about hitting some
>> logistical cap for the largest sglist we can support -- we'll never
>> even attempt to build one that big anymore.
>>
>> Reported-by: Kevin Wolf 
>> Signed-off-by: John Snow 
>> ---
>>  hw/ide/ahci.c | 30 ++
>>  hw/ide/internal.h |  2 +-
>>  hw/ide/pci.c  |  7 ---
>>  3 files changed, 15 insertions(+), 24 deletions(-)
> 
> Acked-by: Stefan Hajnoczi 
> 

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js

Re: [Qemu-devel] RFC: libyajl for JSON

[Markus Armbruster]

Eric Blake  writes:

> On 11/02/2015 01:40 AM, Markus Armbruster wrote:
>> Eric Blake  writes:
>> 
>>> Loaded question in response to
>>> https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06988.html, but
>> 
>> Discussion of our parser's enormous appetite for wasting RAM.  Fixable,
>> but it's work, and it's not its only defect.
>> 
>
>>> On the other hand, one of the "features" of qemu's hand-rolled json
>>> parser is the ability to do qobject_from_jsonf("{'foo':%s}", "bar")
>>> (that is, we extended JSON with our notion of single-quoted strings, and
>>> with our notion of %s and similar escape sequences for piecing together
>>> multiple inputs into a single input stream without having to first
>>> g_strdup_printf our pieces into a single string).  I don't know if
>>> libyajl lets us add extensions to the language it parses.
>> 
>> Actually two separate extensions:
>> 
>> * Single-quoted strings
>> 
>>   The extension's purpose is avoiding quotes in C.  Example:
>> 
>>   "{ 'execute': 'migrate_set_speed', 'arguments': { 'value': 10 } }"
>> 
>>   is easier to read than
>> 
>>   "{ \"execute\": \"migrate_set_speed\", \"arguments\": { \"value\": 10 
>> } }"
>> 
>>   Most actual uses are in tests.
>
> Except that we have documented that QMP clients may use it; and indeed:
>
> $ printf "{'execute':'qmp_capabilities'}\n{'execute':'quit'}" | \
>./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -qmp stdio
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2},
> "package": ""}, "capabilities": []}}
> {"return": {}}
> {"return": {}}
> {"timestamp": {"seconds": 1446478389, "microseconds": 265886}, "event":
> "SHUTDOWN"}

Unnecessary mistake :(

> So that is an absolute must for whatever parser we choose.  My first
> glance at libyajl is that it does NOT currently allow single quotes (not
> even with any of its existing options), so we'd have to pre-process
> input before feeding it to yajl.

I'm not sure reusing an existing parser makes much sense if we need to
preprocess anyway.  After all, parsing JSON isn't exactly a moon shot,
at least if you know what you're doing.

>>   JSON5, a proposed extension to JSON also supports single-quoted
>>   strings.  So does Python.
>
> It would be an interesting task to see if yajl would accept patches to
> parse JSON5 as additional options (for example, yajl already has options
> on whether to diagnose or ignore UTF8 errors, and on whether to allow /*
> */ javascript comments).  But then we'd be requiring an
> as-yet-unreleased version of libyajl rather than being able to rely on
> what distros already have, at least for a few years; or we'd have to
> carry new-enough yajl as a git submodule within qemu.git.
>
> I'll ask on the yajl mailing lists, to get a feel for community
> receptiveness, before attempting to actually write patches on that front.

Makes sense.

>> * Optional interpolation
>> 
>>   If you pass a va_list to the parser, it replaces certain escape
>>   sequences by values from that va_list.  The escape sequences are a
>>   subset of printf conversion specifiers, to enable compile-time
>>   checking with __attribute__((format...)).
>> 
>>   We used to rely on this for creating "rich" error objects, but those
>>   are long gone (commit df1e608).  Perhaps two dozen uses are left.
>
> The testsuite is probably the biggest user, but we still have uses
> throughout the code base.
>
> Based on my look at libyajl, I think we CAN get away with stream
> interpolation - yajl maintains state such that you do NOT have to feed
> it the entire stream in one go.  So this one is not insurmountable; we
> could rewrite our qobject_from_jsonf() to make multiple calls into yajl
> without having to pre-format a single string.

Sounds complicated...

>>   We could convert them to use "normal" interpolation, i.e. first format
>>   the arguments as JSON, then interpolate with g_strdup_printf() or
>>   similar, then parse.  Formatting only to parse it right back is
>>   inelegant.  Also inefficient, but that probably doesn't matter here.
>
> Or indeed, this is still an option.
>
>> 
>>   The current interface could be retained as convenience function to
>>   interpolate and feed the result to the JSOn parser.
>> 
>>   Alternatively, we could build the QObject manually instead.  More
>>   efficient than either kind of interpolation, but a good deal less
>>   readable.
>
> At any rate, it is certainly less of a show-stopper when compared to the
> need for supporting single-quoted strings.
>
>> Got one more, actually a pitfall, not an extension:
>> 
>> * Representation of JSON numbers
>> 
>>   RFC 7159 doesn't specify how numbers are to be represented.
>> 
>>   Many JSON implementations represent any JSON number as double.  This
>>   can represent signed integers up to 53 bits exactly.
>> 
>>   Our parser represents numbers as int64_t when possible, else as
>>   double.
>> 
>>   Unlike the extensions above, this one is essential: any parser that
>>

Re: [Qemu-devel] [PATCH v2] seccomp: add cacheflush to whitelist

[Andrew Jones]

On Mon, Nov 02, 2015 at 06:09:41PM +, Peter Maydell wrote:
> On 2 November 2015 at 17:56, Andrew Jones  wrote:
> > cacheflush is an arm-specific syscall that qemu built for arm
> > uses. Add it to the whitelist, but only if we're linking with
> > a recent enough libseccomp.
> >
> > Signed-off-by: Andrew Jones 
> > ---
> > v2: only add cacheflush if libseccomp supports it
> >
> >  qemu-seccomp.c | 9 -
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> > index 80d034a8d5190..e76097e958779 100644
> > --- a/qemu-seccomp.c
> > +++ b/qemu-seccomp.c
> > @@ -16,6 +16,10 @@
> >  #include 
> >  #include "sysemu/seccomp.h"
> >
> > +#if SCMP_VER_MAJOR >= 2 && SCMP_VER_MINOR >= 2 && SCMP_VER_MICRO >= 3
> > +#define HAVE_CACHEFLUSH
> > +#endif
> 
> This will claim that a hypothetical future version 3.0.0 does not
> have cacheflush...

Indeed. Sigh... In that case, how about just

#if defined(TARGET_ARM) || defined(TARGET_AARCH64)
{ SCMP_SYS(cacheflush), 240 },
#endif

Thanks,
drew

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH] target-arm: Fix arm_debug_excp_handler() for singlestep enabled

[Peter Maydell]

On 2 November 2015 at 17:51, Sergey Fedorov  wrote:
> CPU singlestep is done by generating a debug internal exception. Do not
> raise a real CPU exception in case of singlestepping.
>
> Signed-off-by: Sergey Fedorov 
> ---
>  target-arm/op_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 7929c71..67d9ffb 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -909,7 +909,7 @@ void arm_debug_excp_handler(CPUState *cs)
>  uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
>  bool same_el = (arm_debug_target_el(env) == arm_current_el(env));
>
> -if (cpu_breakpoint_test(cs, pc, BP_GDB)) {
> +if (cs->singlestep_enabled || cpu_breakpoint_test(cs, pc, BP_GDB)) {
>  return;
>  }

So I think this will mean that if we're gdbstub-single-stepping then
an architectural breakpoint on the insn we're stepping won't fire.

Does using a test

if (!cpu_breakpoint_test(cs, pc, BP_CPU)) {
return;
}

fix the singlestep bug too? If so I think it would probably be
preferable.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Clean up DISAS_UPDATE usage in AArch32 translation code

[Peter Maydell]

On 2 November 2015 at 18:16, Sergey Fedorov  wrote:
> AArch32 translation code does not distinguish between DISAS_UPDATE and
> DISAS_JUMP. Thus, we cannot use any of them without first updating PC in
> CPU state. Furthermore, it is too complicated to update PC in CPU state
> before PC gets updated in disas context. So it is hardly possible to
> correctly end TB early if is is not likely to be executed before calling
> disas_*_insn(), e.g. just after calling breakpoint check helper.
>
> Modify DISAS_UPDATE and DISAS_JUMP usage in AArch32 translation and
> apply to them the same semantic as AArch64 translation does:
>  - DISAS_UPDATE: update PC in CPU state when finishing translation
>  - DISAS_JUMP:   preserve current PC value in CPU state when finishing
>  translation

Is this fixing the breakpoint related bug? If so the commit message
should say so. Otherwise it just looks like cleanup...

(I'll review the patch tomorrow.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8 32/54] Postcopy: Maintain sentmap and calculate discard

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:

> > @@ -662,6 +672,24 @@ static int save_zero_page(QEMUFile *f, RAMBlock 
> > *block, ram_addr_t offset,
> >  }
> >  
> >  /**
> > + * ram_find_block_by_id: Find a ramblock by name.
> > + *
> > + * Returns: The RAMBlock with matching ID, or NULL.
> > + */
> > +static RAMBlock *ram_find_block_by_id(const char *id)
> > +{
> > +RAMBlock *block;
> > +
> > +QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
> > +if (!strcmp(id, block->idstr)) {
> > +return block;
> > +}
> > +}
> > +
> > +return NULL;
> > +}
> 
> We don't have this function already.
> 
> Once here, could we split it in its own patch and use it in ram_load?
> 
> 
> QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
> if (!strncmp(id, block->idstr, sizeof(id))) {
> if (length != block->used_length) {
> Error *local_err = NULL;
> 
> ret = qemu_ram_resize(block->offset, length, 
> &local_err);
> if (local_err) {
> error_report_err(local_err);
> }
> }
> ram_control_load_hook(f, RAM_CONTROL_BLOCK_REG,
>   block->idstr);
> break;
> }
> }
> 
> if (!block) {
> error_report("Unknown ramblock \"%s\", cannot "
>  "accept migration", id);
> ret = -EINVAL;
> }
> 
> 
> We could also use it in:
> 
> host_from_stream_offset

Done; replaced both uses and it's now called 'qemu_ram_block_by_name'

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

[Qemu-devel] [PATCH] target-arm: Clean up DISAS_UPDATE usage in AArch32 translation code

[Sergey Fedorov]

AArch32 translation code does not distinguish between DISAS_UPDATE and
DISAS_JUMP. Thus, we cannot use any of them without first updating PC in
CPU state. Furthermore, it is too complicated to update PC in CPU state
before PC gets updated in disas context. So it is hardly possible to
correctly end TB early if is is not likely to be executed before calling
disas_*_insn(), e.g. just after calling breakpoint check helper.

Modify DISAS_UPDATE and DISAS_JUMP usage in AArch32 translation and
apply to them the same semantic as AArch64 translation does:
 - DISAS_UPDATE: update PC in CPU state when finishing translation
 - DISAS_JUMP:   preserve current PC value in CPU state when finishing
 translation

Signed-off-by: Sergey Fedorov 
---
 target-arm/translate.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 9f1d740..ec740fd 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -870,7 +870,7 @@ static inline void gen_bx_im(DisasContext *s, uint32_t addr)
 {
 TCGv_i32 tmp;
 
-s->is_jmp = DISAS_UPDATE;
+s->is_jmp = DISAS_JUMP;
 if (s->thumb != (addr & 1)) {
 tmp = tcg_temp_new_i32();
 tcg_gen_movi_i32(tmp, addr & 1);
@@ -883,7 +883,7 @@ static inline void gen_bx_im(DisasContext *s, uint32_t addr)
 /* Set PC and Thumb state from var.  var is marked as dead.  */
 static inline void gen_bx(DisasContext *s, TCGv_i32 var)
 {
-s->is_jmp = DISAS_UPDATE;
+s->is_jmp = DISAS_JUMP;
 tcg_gen_andi_i32(cpu_R[15], var, ~1);
 tcg_gen_andi_i32(var, var, 1);
 store_cpu_field(var, thumb);
@@ -1062,7 +1062,7 @@ static void gen_exception_insn(DisasContext *s, int 
offset, int excp,
 static inline void gen_lookup_tb(DisasContext *s)
 {
 tcg_gen_movi_i32(cpu_R[15], s->pc & ~1);
-s->is_jmp = DISAS_UPDATE;
+s->is_jmp = DISAS_JUMP;
 }
 
 static inline void gen_add_data_offset(DisasContext *s, unsigned int insn,
@@ -4096,7 +4096,7 @@ static void gen_exception_return(DisasContext *s, 
TCGv_i32 pc)
 tmp = load_cpu_field(spsr);
 gen_set_cpsr(tmp, CPSR_ERET_MASK);
 tcg_temp_free_i32(tmp);
-s->is_jmp = DISAS_UPDATE;
+s->is_jmp = DISAS_JUMP;
 }
 
 /* Generate a v6 exception return.  Marks both values as dead.  */
@@ -4105,7 +4105,7 @@ static void gen_rfe(DisasContext *s, TCGv_i32 pc, 
TCGv_i32 cpsr)
 gen_set_cpsr(cpsr, CPSR_ERET_MASK);
 tcg_temp_free_i32(cpsr);
 store_reg(s, 15, pc);
-s->is_jmp = DISAS_UPDATE;
+s->is_jmp = DISAS_JUMP;
 }
 
 static void gen_nop_hint(DisasContext *s, int val)
@@ -9035,7 +9035,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int 
insn)
 tmp = load_cpu_field(spsr);
 gen_set_cpsr(tmp, CPSR_ERET_MASK);
 tcg_temp_free_i32(tmp);
-s->is_jmp = DISAS_UPDATE;
+s->is_jmp = DISAS_JUMP;
 }
 }
 break;
@@ -11488,8 +11488,10 @@ void gen_intermediate_code(CPUARMState *env, 
TranslationBlock *tb)
 gen_goto_tb(dc, 1, dc->pc);
 break;
 default:
-case DISAS_JUMP:
 case DISAS_UPDATE:
+gen_set_pc_im(dc, dc->pc);
+/* fall through */
+case DISAS_JUMP:
 /* indicate that the hash table must be used to find the next TB */
 tcg_gen_exit_tb(0);
 break;
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2] seccomp: add cacheflush to whitelist

[Peter Maydell]

On 2 November 2015 at 17:56, Andrew Jones  wrote:
> cacheflush is an arm-specific syscall that qemu built for arm
> uses. Add it to the whitelist, but only if we're linking with
> a recent enough libseccomp.
>
> Signed-off-by: Andrew Jones 
> ---
> v2: only add cacheflush if libseccomp supports it
>
>  qemu-seccomp.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> index 80d034a8d5190..e76097e958779 100644
> --- a/qemu-seccomp.c
> +++ b/qemu-seccomp.c
> @@ -16,6 +16,10 @@
>  #include 
>  #include "sysemu/seccomp.h"
>
> +#if SCMP_VER_MAJOR >= 2 && SCMP_VER_MINOR >= 2 && SCMP_VER_MICRO >= 3
> +#define HAVE_CACHEFLUSH
> +#endif

This will claim that a hypothetical future version 3.0.0 does not
have cacheflush...

thanks
-- PMM

[Qemu-devel] [PATCH v2] seccomp: add cacheflush to whitelist

[Andrew Jones]

cacheflush is an arm-specific syscall that qemu built for arm
uses. Add it to the whitelist, but only if we're linking with
a recent enough libseccomp.

Signed-off-by: Andrew Jones 
---
v2: only add cacheflush if libseccomp supports it

 qemu-seccomp.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index 80d034a8d5190..e76097e958779 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -16,6 +16,10 @@
 #include 
 #include "sysemu/seccomp.h"
 
+#if SCMP_VER_MAJOR >= 2 && SCMP_VER_MINOR >= 2 && SCMP_VER_MICRO >= 3
+#define HAVE_CACHEFLUSH
+#endif
+
 struct QemuSeccompSyscall {
 int32_t num;
 uint8_t priority;
@@ -238,7 +242,10 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] 
= {
 { SCMP_SYS(inotify_init1), 240 },
 { SCMP_SYS(inotify_add_watch), 240 },
 { SCMP_SYS(mbind), 240 },
-{ SCMP_SYS(memfd_create), 240 }
+{ SCMP_SYS(memfd_create), 240 },
+#ifdef HAVE_CACHEFLUSH
+{ SCMP_SYS(cacheflush), 240 },
+#endif
 };
 
 int seccomp_start(void)
-- 
1.8.3.1

[Qemu-devel] [PATCH] target-arm: Fix arm_debug_excp_handler() for singlestep enabled

[Sergey Fedorov]

CPU singlestep is done by generating a debug internal exception. Do not
raise a real CPU exception in case of singlestepping.

Signed-off-by: Sergey Fedorov 
---
 target-arm/op_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 7929c71..67d9ffb 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -909,7 +909,7 @@ void arm_debug_excp_handler(CPUState *cs)
 uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
 bool same_el = (arm_debug_target_el(env) == arm_current_el(env));
 
-if (cpu_breakpoint_test(cs, pc, BP_GDB)) {
+if (cs->singlestep_enabled || cpu_breakpoint_test(cs, pc, BP_GDB)) {
 return;
 }
 
-- 
1.9.1

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] dataplane: simplify indirect descriptor read

[Stefan Hajnoczi]

On Wed, Oct 28, 2015 at 05:48:02PM +0200, Michael S. Tsirkin wrote:
> Use address_space_read to make sure we handle the case of an indirect
> descriptor crossing DIMM boundary correctly.
> 
> Signed-off-by: Michael S. Tsirkin 
> ---
> 
> Warning: compile-tested only.

Test (with your follow-up patch squashed in) with 6 4KB seqeuential read
processes on running successfully.

I didn't test the DIMM boundary case.  Igor: what is the easiest way to
reproduce the DIMM boundary crossing?


signature.asc
Description: PGP signature

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] dataplane: simplify indirect descriptor read

[Stefan Hajnoczi]

On Wed, Oct 28, 2015 at 05:48:02PM +0200, Michael S. Tsirkin wrote:
> Use address_space_read to make sure we handle the case of an indirect
> descriptor crossing DIMM boundary correctly.
> 
> Signed-off-by: Michael S. Tsirkin 
> ---
> 
> Warning: compile-tested only.
> 
>  hw/virtio/dataplane/vring.c | 28 ++--
>  1 file changed, 18 insertions(+), 10 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [Qemu-block] [PATCH] fixup! dataplane: support non-contigious s/g

[Stefan Hajnoczi]

On Wed, Oct 28, 2015 at 11:18:42PM +0200, Michael S. Tsirkin wrote:
> Should fix issues Stefan reported.
> 
> ---
> 
> Built only.
> 
>  hw/virtio/dataplane/vring.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Squashed in to the previous commit.

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v8 32/54] Postcopy: Maintain sentmap and calculate discard

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:

> > +/*
> > + * Transmit the set of pages to be discarded after precopy to the target
> > + * these are pages that:
> > + * a) Have been previously transmitted but are now dirty again
> > + * b) Pages that have never been transmitted, this ensures that
> > + *any pages on the destination that have been mapped by background
> > + *tasks get discarded (transparent huge pages is the specific 
> > concern)
> > + * Hopefully this is pretty sparse
> > + */
> > +int ram_postcopy_send_discard_bitmap(MigrationState *ms)
> > +{
> > +int ret;
> > +
> > +rcu_read_lock();
> > +
> > +/* This should be our last sync, the src is now paused */
> > +migration_bitmap_sync();
> > +
> > +/*
> > + * Update the sentmap to be sentmap = ~sentmap | dirty
> > + */
> > +bitmap_complement(ms->sentmap, ms->sentmap,
> > +   last_ram_offset() >> TARGET_PAGE_BITS);
> > +
> > +bitmap_or(ms->sentmap, ms->sentmap, migration_bitmap,
> > +   last_ram_offset() >> TARGET_PAGE_BITS);
> 
> This bitmaps are really big, I don't know how long would take to do this
> operations here, but I think that we can avoid (at least) the
> bitmap_complement.  We can change the bitmap name to notsentbitmap, init
> it to one and clear it each time that we sent a page, no?

Done, it's now 'unsentmap' - although I suspect the complement step is
probably one of the simpler steps in the process, I'm not sure it's a vast
saving.

> We can also do the bitmap_or() at migration_sync_bitmap() time, at that
> point, we shouldn't be on the critical path?

Given that we're doing the bitmap_sync immediately before the OR, I don't
understand that line; are you talking about a modified migration_bitmap_sync?

Dave

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

Re: [Qemu-devel] Status Buildbot

[Stefan Hajnoczi]

On Mon, Nov 02, 2015 at 10:37:06AM +0100, Timo Benk wrote:
> we at B1 Systems GmbH are currently hosting the buildbot infrastructure. I 
> have mailed some
> former contacts and it seems that this service is no longer needed anymore.
> 
> Can you confirm that?

It's me again :).

I announced the end of buildbot usage during QEMU Summit 2015.  The
meeting notes are here:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg01048.html

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v2 2/2] block: test 'blockdev-snapshot' using a file BDS as the overlay

[Eric Blake]

On 11/02/2015 10:07 AM, Max Reitz wrote:
> On 02.11.2015 13:15, Alberto Garcia wrote:
>> This test checks that it is not possible to create a snapshot using as
>> the overlay node a BDS that does not support backing images.
> 
> I don't think that works in English. I may be wrong, of course.
> 
> "a snapshot using a BDS that does not support backing images as the
> overlay node", "a snapshot with the overlay node being a BDS that...",
> "a snapshot using a BDS as the overlay node that...", or something like
> that might work.
> 

How about:

This test checks that it is not possible to create a snapshot if the
requested overlay node is a BDS which does not support backing images.

>> +++ b/tests/qemu-iotests/085
>> @@ -103,7 +103,8 @@ function add_snapshot_image()
>> { 'options':
>>   { 'driver': 'qcow2', 'node-name': 'snap_"${1}"', 
>> "${extra_params}"
>> 'file':
>> -   { 'driver': 'file', 'filename': '"${snapshot_file}"' } } } }"
>> +   { 'driver': 'file', 'filename': '"${snapshot_file}"',
>> + 'node-name': 'file_"${1}"' } } } }"
> 
> Pre-existing, but do those "" actually do anything?
> 

Actually, the "" are wrong.  Look at the full context: we have:

cmd="..."${snapshot_file}"..."

which means the expansion of $snapshot_file is _unquoted_.  We really
want either:

cmd='...'"${snapshot_file}"'...'

(if we wanted to write the command with " instead of ' for internal
string quoting), or:

cmd="...${snapshot_file}..."


I suspect that it crept in because locally we have ' in the ..., and
'${...}' in isolation is usually wrong (which is why you have to look at
the full string, and not just the local change).

> Since the latter is mainly out of curiosity, and because English too not
> my mother language is, which is why I not the one be should, who himself
> over that complains*:

LOL

-- 
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 v8 07/17] qapi: Rework collision assertions

[Eric Blake]

On 11/02/2015 08:37 AM, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> Now that we have separate namespaces for QMP vs. tag values,
> 
> What's the "QMP namespace"?

I guess I need to be more explicit :)

In the generated C struct for a qapi object, we now have two separate
namespaces:

struct Foo {
Type1 name1; /* namespace for QMP (aka non-variant) members */
Type2 name2;
union {
Type1 name1; /* namespace for tag values */
Type2 name2;
} u;
};


> 
>> we can simplify how the QAPISchema*.check() methods check for
>> collisions.
> 
> I *guess* the point of this patch is dropping checks that are obsolete
> now tag values no longer collide with non-variant member names in
> generated C, with simplifications on top.  Correct?
> 

Almost. This is not actually dropping any of the old ad hoc checks in
the parser, but you are correct that it IS fixing the check() methods to
quit asserting things that are no longer forbidden now that tag values
no longer collide with non-variant member names.

>>  Each QAPISchemaObjectTypeMember check() call is
>> given a single set of names it must not collide with; this set
>> is either the QMP names (when this member is used by an
>> ObjectType) or the case names (when this member is used by an
>> ObjectTypeVariants).  We no longer need an all_members
>> parameter, as it can be computed by seen.values().

This is point [1] mentioned below.

>>  When used
>> by a union, QAPISchemaObjectTypeVariant must also perform a
>> QMP collision check for each member of its corresponding type.
> 
> I'm afraid this explanation is next to impossible to understand unless
> you know how the checking works.  I should know, because I wrote it, but
> actually don't, at least not by heart.  So let me relearn how checking
> works before your patch.

I guess I get to pick and choose from your wording, to try and make my
commit body more comprehensible.

> 
> We're talking about a single invocation of QAPISchemaObjectType.check().
> Its job is to compute self.members and self.base, and do sanity
> checking, which includes checking for collisions.  It has two variables
> of interest:
> 
> * members is where we accumulate the list of members that'll become
>   self.members.  It's initialized to the inherited members (empty if no
>   base, obviously).

True pre-patch; eliminated by point [1] mentioned above, by observing
that seen.values() is identical to members after processing
local_members, and that we don't further modify seen when processing
variants.

> 
> * seen is a dictionary mapping names to members.  This is merely for
>   collision checking.  It's initialized to contain the inherited members
>   (whose names must be distinct, by induction, because we make sure the
>   base type's check() completes first).
> 
> For each local member m of self, we call m.check(schema, members, seen).
> This is actually QAPISchemaObjectTypeMember.check(), and it uses members
> and seen as follows:
> 
> * Assert m.name doesn't collide with another member's name, i.e. not in
>   seen.
> 
> * Append m to members.
> 
> * Insert m.name: m into seen.
> 
> Straightforward so far.  Coming up next: variants.
> 
> Each variant's members are represented as a single member with the tag
> value as name.  Its type is an object type that has the variant's
> members.
> 
> For each variant v:
> 
> * Copy seen to vseen.  It holds the non-variant members.
> 
> * Call QAPISchemaObjectTypeMember.check(schema, [], vseen), which boils
>   down to assert v.name doesn't collide with a non-variant member's
>   name.  This check is obsolete.
> 
> * Assert v.name is a member of tag_type.
> 
> Not checked: variant's name doesn't collide with another variant's name.
> That's because we copy seen inside the loop instead of before the loop.
> 
> Not checked: variant's members don't collide with non-variant members.
> I think this check got lost when we simplified
> QAPISchemaObjectTypeVariants to hold a single member.

Perhaps so; our testsuite wasn't as complete at that time.  Or maybe
it's because our ad hoc checks in the parsing portion were preventing us
from realizing that we needed to repeat things in check() methods.

> 
> Note that the real checking happens in check_union(), and the checks
> here are just sanity checks.  As long as that's the case, holes here are
> harmless.  However, we need them plugged them when we move the real
> checking from check_union() to the .check().

Yep, and that's what this patch is trying to do.

> 
> Looks like variant checking should be thrown out and redone.
> 
> I still don't get your description.  Guess I have to read the patch
> after all ;)
> 
>> The new ObjectType.check_qmp() is an idempotent subset of
>> check(), and can be called multiple times over different seen
>> sets (useful, since the members of one type can be applied
>> into more than one other location via inheritance or flat
>> union variants).
> 
> I think I get this argument.  Excep

Re: [Qemu-devel] [PATCH v4 4/4] iotests: Add tests for the x-blockdev-del command

[Max Reitz]

On 02.11.2015 15:51, Alberto Garcia wrote:
> Signed-off-by: Alberto Garcia 
> ---
>  tests/qemu-iotests/139 | 414 
> +
>  tests/qemu-iotests/139.out |   5 +
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 420 insertions(+)
>  create mode 100644 tests/qemu-iotests/139
>  create mode 100644 tests/qemu-iotests/139.out

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 1/2] block: Disallow snapshots if the overlay doesn't support backing files

[Alberto Garcia]

On Mon 02 Nov 2015 05:11:46 PM CET, Eric Blake  wrote:
>> @@ -1667,6 +1667,11 @@ static void 
>> external_snapshot_prepare(BlkTransactionState *common,
>>  
>>  if (state->new_bs->backing != NULL) {
>>  error_setg(errp, "The snapshot already has a backing image");
>> +return;
>> +}
>> +
>> +if (!state->new_bs->drv->supports_backing) {
>> +error_setg(errp, "The snapshot does not support backing images");
>>  }
>>  }
>
> You could avoid the 'return' by doing:
>
> if (state->new_bs->backing) {
> error_setg(...);
> } else if (!state->new_bs->drv->supports_backing) {
> error_setg(...);
> }

There's three more checks immediately before these lines, so I would
have had to turn everything into a series of if / else if, which is a
bit more unreadable in this case in my opinion. That's why I decided to
leave it like it is in the patch.

Berto

Re: [Qemu-devel] [PATCH v7 13/35] hostmem-file: use whole file size if possible

[Vladimir Sementsov-Ogievskiy]

On 02.11.2015 12:13, Xiao Guangrong wrote:

Use the whole file size if @size is not specified which is useful
if we want to directly pass a file to guest

Signed-off-by: Xiao Guangrong 
---
  backends/hostmem-file.c | 22 ++
  1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index 9097a57..ea355c1 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -38,15 +38,29 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error 
**errp)
  {
  HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
  
-if (!backend->size) {

-error_setg(errp, "can't create backend with size 0");
-return;
-}
  if (!fb->mem_path) {
  error_setg(errp, "mem-path property not set");
  return;
  }
  
+if (!backend->size) {

+Error *local_err = NULL;
+
+/*
+ * use the whole file size if @size is not specified.
+ */
+backend->size = qemu_file_getlength(fb->mem_path, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+}
+
+if (!backend->size) {
+error_setg(errp, "can't create backend on the file whose size is 0");
+return;
+}
+
  backend->force_prealloc = mem_prealloc;
  memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
   object_get_canonical_path(OBJECT(backend)),


why not just

+if (!backend->size) {
+/*
+ * use the whole file size if @size is not specified.
+ */
+backend->size = qemu_file_getlength(fb->mem_path, errp);
+if (*errp) {
+return;
+}
+}


what the purpose of propagating?

--
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.

Re: [Qemu-devel] [PATCH v2 2/2] block: test 'blockdev-snapshot' using a file BDS as the overlay

[Max Reitz]

On 02.11.2015 13:15, Alberto Garcia wrote:
> This test checks that it is not possible to create a snapshot using as
> the overlay node a BDS that does not support backing images.

I don't think that works in English. I may be wrong, of course.

"a snapshot using a BDS that does not support backing images as the
overlay node", "a snapshot with the overlay node being a BDS that...",
"a snapshot using a BDS as the overlay node that...", or something like
that might work.

> Signed-off-by: Alberto Garcia 
> ---
>  tests/qemu-iotests/085 | 12 +++-
>  tests/qemu-iotests/085.out |  4 
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
> index 9484117..ccde2ae 100755
> --- a/tests/qemu-iotests/085
> +++ b/tests/qemu-iotests/085
> @@ -103,7 +103,8 @@ function add_snapshot_image()
> { 'options':
>   { 'driver': 'qcow2', 'node-name': 'snap_"${1}"', 
> "${extra_params}"
> 'file':
> -   { 'driver': 'file', 'filename': '"${snapshot_file}"' } } } }"
> +   { 'driver': 'file', 'filename': '"${snapshot_file}"',
> + 'node-name': 'file_"${1}"' } } } }"

Pre-existing, but do those "" actually do anything?

Since the latter is mainly out of curiosity, and because English too not
my mother language is, which is why I not the one be should, who himself
over that complains*:

Reviewed-by: Max Reitz 

(Although I would indeed prefer the commit message to be parsable more
easily.)

*Man, writing that was hard.

>  _send_qemu_cmd $h "${cmd}" "return"
>  }
>  
> @@ -187,6 +188,15 @@ add_snapshot_image ${SNAPSHOTS}
>  blockdev_snapshot ${SNAPSHOTS}
>  
>  echo
> +echo === Invalid command - cannot create a snapshot using a file BDS ===
> +echo
> +
> +_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
> + 'arguments': { 'node':'virtio0',
> +'overlay':'file_"${SNAPSHOTS}"' }
> +   }" "error"
> +
> +echo
>  echo === Invalid command - snapshot node used as active layer ===
>  echo
>  
> diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
> index 52292ea..01c78d6 100644
> --- a/tests/qemu-iotests/085.out
> +++ b/tests/qemu-iotests/085.out
> @@ -62,6 +62,10 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
> backing_file=TEST_DIR/
>  {"return": {}}
>  {"return": {}}
>  
> +=== Invalid command - cannot create a snapshot using a file BDS ===
> +
> +{"error": {"class": "GenericError", "desc": "The snapshot does not support 
> backing images"}}
> +
>  === Invalid command - snapshot node used as active layer ===
>  
>  {"error": {"class": "GenericError", "desc": "The snapshot is already in use 
> by virtio0"}}
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 1/2] block: Disallow snapshots if the overlay doesn't support backing files

[Max Reitz]

On 02.11.2015 13:15, Alberto Garcia wrote:
> This addresses scenarios like this one:
> 
>   { 'execute': 'blockdev-add', 'arguments':
> { 'options': { 'driver': 'qcow2',
>'node-name': 'new0',
>'file': { 'driver': 'file',
>  'filename': 'new.qcow2',
>  'node-name': 'file0' } } } }
> 
>   { 'execute': 'blockdev-snapshot', 'arguments':
> { 'node': 'virtio0',
>   'overlay': 'file0' } }
> 
> Signed-off-by: Alberto Garcia 
> ---
>  blockdev.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index a567a05..2774bf5 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1667,6 +1667,11 @@ static void 
> external_snapshot_prepare(BlkTransactionState *common,
>  
>  if (state->new_bs->backing != NULL) {
>  error_setg(errp, "The snapshot already has a backing image");
> +return;

This is...

> +}
> +
> +if (!state->new_bs->drv->supports_backing) {
> +error_setg(errp, "The snapshot does not support backing images");

...why a return statement might be good here, too.

With or without:

Reviewed-by: Max Reitz 

>  }
>  }
>  
> 




signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [Bug 1512134] Re: Multiboot v1 memory map offset wrong

[Max Reitz]

Hi,

Have you tested your code with GRUB (Legacy) itself? Looking at code
from one of my own hobby kernels, and from a hobby kernel that has
largely not been written by me, both are interpreting the fields as qemu
is (i.e. mmap_addr points to a multiboot_mmap_entry, and not to
multiboot_mmap_entry + 4), and I know both to work just fine with GRUB.

I considered the -4 offset to signify that the size field simply does
not count itself, i.e. the size of one multiboot_mmap_entry is $size +
4.

Max

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

Title:
  Multiboot v1 memory map offset wrong

Status in QEMU:
  New

Bug description:
  I'm developping a multiboot kernel for multiboot v1
  My multiboot header contains the V1 magic (0x1BADB002) and the flags 
0x00010243  (with enabled memory detection, and boot loader name)

  
  When booted in multiboot,
  Qemu gives me two pointers:
  unsigned long mmap_length;
  unsigned long mmap_addr;

  mmap_addr shall points to this structure:
  struct multiboot_mmap_entry
  {
  multiboot_uint32_t size;
  multiboot_uint64_t addr;
  multiboot_uint64_t len;
  multiboot_uint32_t type;
  } 

  
  According to the multiboot v1 specification, mmap_addr should not point  to 
the start of this structure, but instead, should point to the "addr "field.

  Work-arround:
  Detect if qemu is used using bootloader_name field.
  If it is, do NOT apply a -4 offset to mmap_addr

  http://git.savannah.gnu.org/cgit/grub.git/tree/doc/multiboot.texi?h=multiboot

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

Re: [Qemu-devel] [PATCH v2 4/4] fw_cfg: streamline (non-DMA) read operations

[Eric Blake]

On 11/02/2015 07:38 AM, Laszlo Ersek wrote:

> 
> (1) Can you please split this patch in two? Maybe I'm just particularly
> slow today, but I feel that it would help me review this patch if I
> could look at each .read conversion in separation. I'd like to see that
> each conversion, individually, is unobservable from the guest.
> 
> The first patch would introduce the new function and convert one of the
> callbacks. The second patch would convert the other callback and remove
> the old function. (Un-called static functions would break the compile,
> so the removal cannot be left for a third patch.)

You can mark static functions with __attribute__((__unused__)), and gcc
will then let you leave them for a later cleanup patch.  I'm not sure if
clang behaves similarly, though.

-- 
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 v3 1/2] qemu-iotests: fix cleanup of background processes

[Eric Blake]

On 11/02/2015 09:03 AM, Max Reitz wrote:

 +++ b/tests/qemu-iotests/058
 @@ -32,11 +32,17 @@ status=1   # failure is the default!
  
  nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket
  nbd_snapshot_img="nbd:unix:$nbd_unix_socket"
 +rm -f "${TEST_DIR}/qemu-nbd.pid"
  
  _cleanup_nbd()
  {
 -if [ -n "$NBD_SNAPSHOT_PID" ]; then
 -kill "$NBD_SNAPSHOT_PID"
 +local NBD_SNAPSHOT_PID
 +if [ -f "${TEST_DIR}/qemu-nbd.pid" ]; then
 +read NBD_SNAPSHOT_PID < "${TEST_DIR}/qemu-nbd.pid"
 +rm -f "${TEST_DIR}/qemu-nbd.pid"
 +if [ -n "$NBD_SNAPSHOT_PID" ]; then
>>>
>>> No, I won't complain about using ! -z "" elsewhere and -n "" here. :-)
>>
>> The little pedant in me screams "but I will!", and the little prankster
>> next to him is clapping enthusiastically.
>>
>> Kidding aside: not worth a respin, but could be cleaned up on commit
>> (maintainer's discretion).
> 
> Oh, if that's the case, then I have another thing for you: The use of ==
> in patch 2! ;-)
> 
> (I'm a bit disappointed Eric doesn't have a mail filter for
> #!/bin/(ba)?sh ... if.*== for his mail client.)

I already know that most (if not all) of qemu-iotests is specifically
/bin/bash.  But if we want to, we can ditch -n and -z, and just use:

if [[ $NBD_SNAPSHOT_PID ]]; then

and similarly.  In fact, I actually prefer embracing bash-isms when we
know we are using bash, to make it obvious that we know we are not
catering to /bin/sh.

-- 
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 v7 12/35] util: let qemu_fd_getlength support block device

[Xiao Guangrong]

On 11/03/2015 12:11 AM, Vladimir Sementsov-Ogievskiy wrote:

On 02.11.2015 12:13, Xiao Guangrong wrote:

lseek can not work for all block devices as the man page says:
| Some devices are incapable of seeking and POSIX does not specify
| which devices must support lseek().

This patch tries to add the support on Linux by using BLKGETSIZE64
ioctl

Signed-off-by: Xiao Guangrong 
---
  util/osdep.c | 20 
  1 file changed, 20 insertions(+)

diff --git a/util/osdep.c b/util/osdep.c
index 5a61e19..b20c793 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -45,6 +45,11 @@
  extern int madvise(caddr_t, size_t, int);
  #endif
+#ifdef CONFIG_LINUX
+#include 
+#include 
+#endif
+
  #include "qemu-common.h"
  #include "qemu/sockets.h"
  #include "qemu/error-report.h"
@@ -433,6 +438,21 @@ int64_t qemu_fd_getlength(int fd)
  {
  int64_t size;
+#ifdef CONFIG_LINUX
+struct stat stat_buf;
+if (fstat(fd, &stat_buf) < 0) {
+return -errno;
+}
+
+if ((S_ISBLK(stat_buf.st_mode)) && !ioctl(fd, BLKGETSIZE64, &size)) {
+/* The size of block device is larger than max int64_t? */
+if (size < 0) {
+return -EOVERFLOW;
+}
+return size;
+}
+#endif
+
  size = lseek(fd, 0, SEEK_END);
  if (size < 0) {
  return -errno;


Reviewed-by: Vladimir Sementsov-Ogievskiy 

just a question: is there any use for stat.st_size ? Is it always worse then 
lseek?


The man page says:
The  st_size field gives the size of the file (if it is a regular file or a 
symbolic link)
in bytes.  The size of a symbolic link is the length of the pathname it 
contains, without a
terminating null byte.

So it can not work on symbolic link.


Does it work for
blk?



Quickly checked with a program written by myself and 'stat' command, the answer 
is NO. :)


also, "This patch tries to add..". Hmm. It looks like this patch is not sure 
about will it success.
I'd prefer "This patch adds", but this is not important



Thanks for your sharing. I did not know the different, now, i got it. :)

Re: [Qemu-devel] [PATCH v7 20/35] dimm: get mapped memory region from DIMMDeviceClass->get_memory_region

[Vladimir Sementsov-Ogievskiy]

On 02.11.2015 18:06, Xiao Guangrong wrote:



On 11/02/2015 10:26 PM, Vladimir Sementsov-Ogievskiy wrote:

On 02.11.2015 16:08, Xiao Guangrong wrote:



On 11/02/2015 08:19 PM, Vladimir Sementsov-Ogievskiy wrote:

On 02.11.2015 12:13, Xiao Guangrong wrote:

Curretly, the memory region of backed memory is directly mapped to
guest's address space, however, it is not true for nvdimm device

This patch let dimm device realize this fact and use
DIMMDeviceClass->get_memory_region method to get the mapped memory
region

Current code did not check the return value of get_memory_region 
as it

assumed the backend memory of pc-dimm is always properly initialized,
we make get_memory_region internally catch the case if something is
wrong


but here you call not pc-dimm's get_memory_region, but common 
ddc->get_memory_region, which may be
nvdimm or possibly other future dimm, so, why not check it here? And 
than pc_dimm_get_memory_region

may be left untouched (error_abort is ok, because errp is unused).


Hmm, because 'here' is not the only place calling ->get_memory_region, 
this method has

multiple callers:

$ git grep "\->get_memory_region"
hw/i386/pc.c:MemoryRegion *mr = ddc->get_memory_region(dimm);
hw/i386/pc.c:MemoryRegion *mr = ddc->get_memory_region(dimm);
hw/mem/dimm.c:mr = ddc->get_memory_region(dimm);
hw/mem/nvdimm.c:ddc->get_memory_region = nvdimm_get_memory_region;
hw/mem/pc-dimm.c:ddc->get_memory_region = pc_dimm_get_memory_region;
hw/ppc/spapr.c:MemoryRegion *mr = ddc->get_memory_region(dimm);

memory region validation is also done for NVDIMM in nvdimm device.

Ok, then it should be documented by a comment in dimm.h, where 
DIMMDeviceClass is defined, that this function should not fail


--
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.

Re: [Qemu-devel] [PATCH v7 12/35] util: let qemu_fd_getlength support block device

[Vladimir Sementsov-Ogievskiy]

On 02.11.2015 12:13, Xiao Guangrong wrote:

lseek can not work for all block devices as the man page says:
| Some devices are incapable of seeking and POSIX does not specify
| which devices must support lseek().

This patch tries to add the support on Linux by using BLKGETSIZE64
ioctl

Signed-off-by: Xiao Guangrong 
---
  util/osdep.c | 20 
  1 file changed, 20 insertions(+)

diff --git a/util/osdep.c b/util/osdep.c
index 5a61e19..b20c793 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -45,6 +45,11 @@
  extern int madvise(caddr_t, size_t, int);
  #endif
  
+#ifdef CONFIG_LINUX

+#include 
+#include 
+#endif
+
  #include "qemu-common.h"
  #include "qemu/sockets.h"
  #include "qemu/error-report.h"
@@ -433,6 +438,21 @@ int64_t qemu_fd_getlength(int fd)
  {
  int64_t size;
  
+#ifdef CONFIG_LINUX

+struct stat stat_buf;
+if (fstat(fd, &stat_buf) < 0) {
+return -errno;
+}
+
+if ((S_ISBLK(stat_buf.st_mode)) && !ioctl(fd, BLKGETSIZE64, &size)) {
+/* The size of block device is larger than max int64_t? */
+if (size < 0) {
+return -EOVERFLOW;
+}
+return size;
+}
+#endif
+
  size = lseek(fd, 0, SEEK_END);
  if (size < 0) {
  return -errno;


Reviewed-by: Vladimir Sementsov-Ogievskiy 

just a question: is there any use for stat.st_size ? Is it always worse 
then lseek? Does it work for blk?


also, "This patch tries to add..". Hmm. It looks like this patch is not 
sure about will it success. I'd prefer "This patch adds", but this is 
not important


--
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.

Re: [Qemu-devel] [PATCH v2 1/2] block: Disallow snapshots if the overlay doesn't support backing files

[Eric Blake]

On 11/02/2015 05:15 AM, Alberto Garcia wrote:
> This addresses scenarios like this one:
> 
>   { 'execute': 'blockdev-add', 'arguments':
> { 'options': { 'driver': 'qcow2',
>'node-name': 'new0',
>'file': { 'driver': 'file',
>  'filename': 'new.qcow2',
>  'node-name': 'file0' } } } }
> 
>   { 'execute': 'blockdev-snapshot', 'arguments':
> { 'node': 'virtio0',
>   'overlay': 'file0' } }
> 
> Signed-off-by: Alberto Garcia 
> ---
>  blockdev.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index a567a05..2774bf5 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1667,6 +1667,11 @@ static void 
> external_snapshot_prepare(BlkTransactionState *common,
>  
>  if (state->new_bs->backing != NULL) {
>  error_setg(errp, "The snapshot already has a backing image");
> +return;
> +}
> +
> +if (!state->new_bs->drv->supports_backing) {
> +error_setg(errp, "The snapshot does not support backing images");
>  }
>  }

You could avoid the 'return' by doing:

if (state->new_bs->backing) {
error_setg(...);
} else if (!state->new_bs->drv->supports_backing) {
error_setg(...);
}

but I don't care enough to require a respin.

Reviewed-by: Eric Blake 

-- 
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 v3 1/2] qemu-iotests: fix cleanup of background processes

[Max Reitz]

On 02.11.2015 08:37, Markus Armbruster wrote:
> Max Reitz  writes:
> 
>> On 30.10.2015 20:25, Jeff Cody wrote:
>>> Commit 934659c switched the iotests to run qemu and qemu-nbd from a bash
>>> subshell, in order to catch segfaults.  Unfortunately, this means the
>>> process PID cannot be captured via '$!'. We stopped killing qemu and
>>> qemu-nbd processes, leaving a lot of orphaned, running qemu processes
>>> after executing iotests.
>>>
>>> Since the process is using exec in the subshell, the PID is the
>>> same as the subshell PID.
>>>
>>> Track these PIDs for cleanup using pidfiles in the $TEST_DIR. Only
>>> track the qemu PID, however, if requested - not all usage requires
>>> killing the process.
>>>
>>> Reported-by: John Snow 
>>> Signed-off-by: Jeff Cody 
>>> ---
>>>  tests/qemu-iotests/058   | 12 
>>>  tests/qemu-iotests/common.config | 14 --
>>>  tests/qemu-iotests/common.qemu   | 18 --
>>>  tests/qemu-iotests/common.rc |  8 +---
>>>  4 files changed, 37 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/058 b/tests/qemu-iotests/058
>>> index f2bdd0b..63a6598 100755
>>> --- a/tests/qemu-iotests/058
>>> +++ b/tests/qemu-iotests/058
>>> @@ -32,11 +32,17 @@ status=1# failure is the default!
>>>  
>>>  nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket
>>>  nbd_snapshot_img="nbd:unix:$nbd_unix_socket"
>>> +rm -f "${TEST_DIR}/qemu-nbd.pid"
>>>  
>>>  _cleanup_nbd()
>>>  {
>>> -if [ -n "$NBD_SNAPSHOT_PID" ]; then
>>> -kill "$NBD_SNAPSHOT_PID"
>>> +local NBD_SNAPSHOT_PID
>>> +if [ -f "${TEST_DIR}/qemu-nbd.pid" ]; then
>>> +read NBD_SNAPSHOT_PID < "${TEST_DIR}/qemu-nbd.pid"
>>> +rm -f "${TEST_DIR}/qemu-nbd.pid"
>>> +if [ -n "$NBD_SNAPSHOT_PID" ]; then
>>
>> No, I won't complain about using ! -z "" elsewhere and -n "" here. :-)
> 
> The little pedant in me screams "but I will!", and the little prankster
> next to him is clapping enthusiastically.
> 
> Kidding aside: not worth a respin, but could be cleaned up on commit
> (maintainer's discretion).

Oh, if that's the case, then I have another thing for you: The use of ==
in patch 2! ;-)

(I'm a bit disappointed Eric doesn't have a mail filter for
#!/bin/(ba)?sh ... if.*== for his mail client.)

Max

> 
>> Reviewed-by: Max Reitz 
>>
>>> +kill "$NBD_SNAPSHOT_PID"
>>> +fi
>>>  fi
>>>  rm -f "$nbd_unix_socket"
>>>  }




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v7 09/35] exec: allow file_ram_alloc to work on file

[Vladimir Sementsov-Ogievskiy]

On 02.11.2015 18:25, Xiao Guangrong wrote:



On 11/02/2015 11:12 PM, Vladimir Sementsov-Ogievskiy wrote:

On 02.11.2015 12:13, Xiao Guangrong wrote:

Currently, file_ram_alloc() only works on directory - it creates a file
under @path and do mmap on it

This patch tries to allow it to work on file directly, if @path is a


It isn't try to allow, it allows, as I understand)...


Err... Sorry for my English, but what is the different between:
”This patch tries to allow it to work on file directly“ and
"This patch allows it to work on file directly"

:(


Not sure that everyone is interested in this nit-picking discussion..

A allows B: if A then B
A tries to allow B: if A then _may be_ B

In any way it doesn't matter.






directory it works as before, otherwise it treats @path as the target
file then directly allocate memory from it

Signed-off-by: Xiao Guangrong 
---
  exec.c | 80 
++

  1 file changed, 51 insertions(+), 29 deletions(-)

diff --git a/exec.c b/exec.c
index 9075f4d..db0fdaf 100644
--- a/exec.c
+++ b/exec.c
@@ -1174,14 +1174,60 @@ void qemu_mutex_unlock_ramlist(void)
  }
  #ifdef __linux__
+static bool path_is_dir(const char *path)
+{
+struct stat fs;
+
+return stat(path, &fs) == 0 && S_ISDIR(fs.st_mode);
+}
+
+static int open_ram_file_path(RAMBlock *block, const char *path, 
size_t size)

+{
+char *filename;
+char *sanitized_name;
+char *c;
+int fd;
+
+if (!path_is_dir(path)) {
+int flags = (block->flags & RAM_SHARED) ? O_RDWR : O_RDONLY;
+
+flags |= O_EXCL;
+return open(path, flags);
+}
+
+/* Make name safe to use with mkstemp by replacing '/' with 
'_'. */

+sanitized_name = g_strdup(memory_region_name(block->mr));
+for (c = sanitized_name; *c != '\0'; c++) {
+if (*c == '/') {
+*c = '_';
+}
+}
+filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
+   sanitized_name);
+g_free(sanitized_name);


one empty line will be very nice here, and it was in master branch


+fd = mkstemp(filename);
+if (fd >= 0) {
+unlink(filename);
+/*
+ * ftruncate is not supported by hugetlbfs in older
+ * hosts, so don't bother bailing out on errors.
+ * If anything goes wrong with it under other filesystems,
+ * mmap will fail.
+ */
+if (ftruncate(fd, size)) {
+perror("ftruncate");
+}
+}
+g_free(filename);
+
+return fd;
+}
+
  static void *file_ram_alloc(RAMBlock *block,
  ram_addr_t memory,
  const char *path,
  Error **errp)
  {
-char *filename;
-char *sanitized_name;
-char *c;
  void *area;
  int fd;
  uint64_t pagesize;
@@ -1212,38 +1258,14 @@ static void *file_ram_alloc(RAMBlock *block,
  goto error;
  }
-/* Make name safe to use with mkstemp by replacing '/' with 
'_'. */

-sanitized_name = g_strdup(memory_region_name(block->mr));
-for (c = sanitized_name; *c != '\0'; c++) {
-if (*c == '/')
-*c = '_';
-}
-
-filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
-   sanitized_name);
-g_free(sanitized_name);
+memory = ROUND_UP(memory, pagesize);
-fd = mkstemp(filename);
+fd = open_ram_file_path(block, path, memory);
  if (fd < 0) {
  error_setg_errno(errp, errno,
   "unable to create backing store for path 
%s", path);

-g_free(filename);
  goto error;
  }
-unlink(filename);
-g_free(filename);
-
-memory = ROUND_UP(memory, pagesize);
-
-/*
- * ftruncate is not supported by hugetlbfs in older
- * hosts, so don't bother bailing out on errors.
- * If anything goes wrong with it under other filesystems,
- * mmap will fail.
- */
-if (ftruncate(fd, memory)) {
-perror("ftruncate");
-}
  area = qemu_ram_mmap(fd, memory, pagesize, block->flags & 
RAM_SHARED);

  if (area == MAP_FAILED) {



Reviewed-by: Vladimir Sementsov-Ogievskiy 


Thanks for your review.





--
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.

Re: [Qemu-devel] [kvm-unit-tests PATCHv5 3/3] arm: pmu: Add CPI checking

[Andrew Jones]

On Fri, Oct 30, 2015 at 03:32:43PM -0400, Christopher Covington wrote:
> Hi Drew,
> 
> On 10/30/2015 09:00 AM, Andrew Jones wrote:
> > On Wed, Oct 28, 2015 at 03:12:55PM -0400, Christopher Covington wrote:
> >> Calculate the numbers of cycles per instruction (CPI) implied by ARM
> >> PMU cycle counter values. The code includes a strict checking facility
> >> intended for the -icount option in TCG mode but it is not yet enabled
> >> in the configuration file. Enabling it must wait on infrastructure
> >> improvements which allow for different tests to be run on TCG versus
> >> KVM.
> >>
> >> Signed-off-by: Christopher Covington 
> >> ---
> >>  arm/pmu.c | 103 
> >> +-
> >>  1 file changed, 102 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arm/pmu.c b/arm/pmu.c
> >> index 4334de4..76a 100644
> >> --- a/arm/pmu.c
> >> +++ b/arm/pmu.c
> >> @@ -43,6 +43,23 @@ static inline unsigned long get_pmccntr(void)
> >>asm volatile("mrc p15, 0, %0, c9, c13, 0" : "=r" (cycles));
> >>return cycles;
> >>  }
> >> +
> >> +/*
> >> + * Extra instructions inserted by the compiler would be difficult to 
> >> compensate
> >> + * for, so hand assemble everything between, and including, the PMCR 
> >> accesses
> >> + * to start and stop counting.
> >> + */
> >> +static inline void loop(int i, uint32_t pmcr)
> >> +{
> >> +  asm volatile(
> >> +  "   mcr p15, 0, %[pmcr], c9, c12, 0\n"
> >> +  "1: subs%[i], %[i], #1\n"
> >> +  "   bgt 1b\n"
> >> +  "   mcr p15, 0, %[z], c9, c12, 0\n"
> >> +  : [i] "+r" (i)
> >> +  : [pmcr] "r" (pmcr), [z] "r" (0)
> >> +  : "cc");
> >> +}
> >>  #elif defined(__aarch64__)
> >>  static inline uint32_t get_pmcr(void)
> >>  {
> >> @@ -64,6 +81,23 @@ static inline unsigned long get_pmccntr(void)
> >>asm volatile("mrs %0, pmccntr_el0" : "=r" (cycles));
> >>return cycles;
> >>  }
> >> +
> >> +/*
> >> + * Extra instructions inserted by the compiler would be difficult to 
> >> compensate
> >> + * for, so hand assemble everything between, and including, the PMCR 
> >> accesses
> >> + * to start and stop counting.
> >> + */
> >> +static inline void loop(int i, uint32_t pmcr)
> >> +{
> >> +  asm volatile(
> >> +  "   msr pmcr_el0, %[pmcr]\n"
> >> +  "1: subs%[i], %[i], #1\n"
> >> +  "   b.gt1b\n"
> >> +  "   msr pmcr_el0, xzr\n"
> >> +  : [i] "+r" (i)
> >> +  : [pmcr] "r" (pmcr)
> >> +  : "cc");
> >> +}
> >>  #endif
> >>  
> >>  struct pmu_data {
> >> @@ -131,12 +165,79 @@ static bool check_cycles_increase(void)
> >>return true;
> >>  }
> >>  
> >> -int main(void)
> >> +/*
> >> + * Execute a known number of guest instructions. Only odd instruction 
> >> counts
> >> + * greater than or equal to 3 are supported by the in-line assembly code. 
> >> The
> >> + * control register (PMCR_EL0) is initialized with the provided value 
> >> (allowing
> >> + * for example for the cycle counter or event counters to be reset). At 
> >> the end
> >> + * of the exact instruction loop, zero is written to PMCR_EL0 to disable
> >> + * counting, allowing the cycle counter or event counters to be read at 
> >> the
> >> + * leisure of the calling code.
> >> + */
> >> +static void measure_instrs(int num, uint32_t pmcr)
> >> +{
> >> +  int i = (num - 1) / 2;
> >> +
> >> +  assert(num >= 3 && ((num - 1) % 2 == 0));
> >> +  loop(i, pmcr);
> >> +}
> >> +
> >> +/*
> >> + * Measure cycle counts for various known instruction counts. Ensure that 
> >> the
> >> + * cycle counter progresses (similar to check_cycles_increase() but with 
> >> more
> >> + * instructions and using reset and stop controls). If supplied a 
> >> positive,
> >> + * nonzero CPI parameter, also strictly check that every measurement 
> >> matches
> >> + * it. Strict CPI checking is used to test -icount mode.
> >> + */
> >> +static bool check_cpi(int cpi)
> >> +{
> >> +  struct pmu_data pmu = {0};
> >> +
> >> +  pmu.cycle_counter_reset = 1;
> >> +  pmu.enable = 1;
> >> +
> >> +  if (cpi > 0)
> >> +  printf("Checking for CPI=%d.\n", cpi);
> >> +  printf("instrs : cycles0 cycles1 ...\n");
> >> +
> >> +  for (int i = 3; i < 300; i += 32) {
> >> +  int avg, sum = 0;
> >> +
> >> +  printf("%d :", i);
> >> +  for (int j = 0; j < NR_SAMPLES; j++) {
> >> +  int cycles;
> >> +
> >> +  measure_instrs(i, pmu.pmcr_el0);
> >> +  cycles = get_pmccntr();
> >> +  printf(" %d", cycles);
> >> +
> >> +  if (!cycles || (cpi > 0 && cycles != i * cpi)) {
> >> +  printf("\n");
> >> +  return false;
> >> +  }
> >> +
> >> +  sum += cycles;
> >> +  }
> >> +  avg = sum / NR_SAMPLES;
> >> +  printf(" sum=%d avg=%d avg_ipc=%d avg_cpi=%d\n",
> >> +  sum, avg, i / avg, avg / i);
> >> +  }
> >> +
> >> +  return true;
> >> +}
> >> +

Re: [Qemu-devel] [PATCH v7 08/35] exec: allow memory to be allocated from any kind of path

[Vladimir Sementsov-Ogievskiy]

On 02.11.2015 18:22, Xiao Guangrong wrote:



On 11/02/2015 10:51 PM, Vladimir Sementsov-Ogievskiy wrote:

On 02.11.2015 12:13, Xiao Guangrong wrote:
Currently file_ram_alloc() is designed for hugetlbfs, however, the 
memory
of nvdimm can come from either raw pmem device eg, /dev/pmem, or the 
file

locates at DAX enabled filesystem

So this patch let it work on any kind of path


No, this patch doesn't change any logic, but only fix variable name 
and some error messages.


Yes, it is.

'let it work' in my thought exactly was "fix variable name and some 
error messages"... okay,

if it confused you, how about change it to:

"This patch fixes variable name and some error messages to let it be 
aware of normal

path"


My english is not very good, I don't know figures of speech. For me 
"patch let it work" means that without this patch it will not work))
Your new variant is ok for me, or better (imo) "This patch fixes 
variable name and some error messages to be suitable for any kind of path"








Signed-off-by: Xiao Guangrong 
---
  exec.c | 24 
  1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/exec.c b/exec.c
index 9de38be..9075f4d 100644
--- a/exec.c
+++ b/exec.c
@@ -1184,25 +1184,25 @@ static void *file_ram_alloc(RAMBlock *block,
  char *c;
  void *area;
  int fd;
-uint64_t hpagesize;
+uint64_t pagesize;
  Error *local_err = NULL;
-hpagesize = qemu_file_get_page_size(path, &local_err);
+pagesize = qemu_file_get_page_size(path, &local_err);
  if (local_err) {
  error_propagate(errp, local_err);
  goto error;
  }
-if (hpagesize == getpagesize()) {
-fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path);
+if (pagesize == getpagesize()) {
+fprintf(stderr, "Memory is not allocated from HugeTlbfs.\n");


Why do you remove path from error message? It is good additional 
information.. What if we have

several memory file backends?


Good catch, will change it to:
fprintf(stderr, "Memory is not allocated from HugeTlbfs on path 
%s.\n", path);


BTW, if no other big change in the further, i will post the new 
version just for of this patch,





  }
-block->mr->align = hpagesize;
+block->mr->align = pagesize;
-if (memory < hpagesize) {
+if (memory < pagesize) {
  error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be 
equal to "

-   "or larger than huge page size 0x%" PRIx64,
-   memory, hpagesize);
+   "or larger than page size 0x%" PRIx64,
+   memory, pagesize);
  goto error;
  }
@@ -1226,14 +1226,14 @@ static void *file_ram_alloc(RAMBlock *block,
  fd = mkstemp(filename);
  if (fd < 0) {
  error_setg_errno(errp, errno,
- "unable to create backing store for 
hugepages");
+ "unable to create backing store for path 
%s", path);

  g_free(filename);
  goto error;
  }
  unlink(filename);
  g_free(filename);
-memory = ROUND_UP(memory, hpagesize);
+memory = ROUND_UP(memory, pagesize);
  /*
   * ftruncate is not supported by hugetlbfs in older
@@ -1245,10 +1245,10 @@ static void *file_ram_alloc(RAMBlock *block,
  perror("ftruncate");
  }
-area = qemu_ram_mmap(fd, memory, hpagesize, block->flags & 
RAM_SHARED);
+area = qemu_ram_mmap(fd, memory, pagesize, block->flags & 
RAM_SHARED);

  if (area == MAP_FAILED) {
  error_setg_errno(errp, errno,
- "unable to map backing store for hugepages");
+ "unable to map backing store for path %s", 
path);

  close(fd);
  goto error;
  }





With these two fixes (any commit message variant):

Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.

Re: [Qemu-devel] [PATCH] qemu-sockets: do not test path with access() before unlinking

[Paolo Bonzini]

On 02/11/2015 16:18, Cole Robinson wrote:
>> > -if ((access(un.sun_path, F_OK) == 0) &&
>> > -unlink(un.sun_path) < 0) {
>> > +if (unlink(un.sun_path) < 0) {
>> >  error_setg_errno(errp, errno,
>> >   "Failed to unlink socket %s", un.sun_path);
>> >  goto err;
>> > 
> This is a serious semantic change, after this patch you will get:
> 
> $ ./x86_64-softmmu/qemu-system-x86_64 -vnc unix:/tmp/idontexist.sock
> qemu-system-x86_64: -vnc unix:/tmp/idontexist.sock: Failed to start VNC
> server: Failed to unlink socket /tmp/idontexist.sock: No such file or 
> directory
> 
> Previously it would 'just work'. Common libvirt usage depends on this as well
> 
> Yeah there's a TOCTTOU race here, but it's very minor: if sun_path is created
> after the access() check, qemu is just going to fail to start since bind()
> will barf if the unix socket path exists.

You're right.  I misread how to test the change.

The right change is what Markus proposed.

Paolo

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Paolo Bonzini]

On 02/11/2015 16:13, Peter Maydell wrote:
> On 2 November 2015 at 14:48, Paolo Bonzini  wrote:
>>
>>
>> On 02/11/2015 15:09, Peter Maydell wrote:
> diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c
> index 383cc8b..45fc7db 100644
> --- a/target-sparc/vis_helper.c
> +++ b/target-sparc/vis_helper.c
> @@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr, uint64_t rs2)
>  for (word = 0; word < 2; word++) {
>  uint32_t val;
>  int32_t src = rs2 >> (word * 32);
> -int64_t scaled = src << scale;
> +int64_t scaled = (int64_t)src << scale;
>  int64_t from_fixed = scaled >> 16;
>>> This will now shift left into the sign bit of a signed integer,
>>> which is undefined behaviour.
>>
>> Why "now"?  It would have done the same before.
> 
> True, but I was reviewing the new code rather than the
> code you were taking away :-)
> 
> Incidentally, that manual says the fpackfix and fpack32 insns
> use a 4 bit GSR.scale_factor value, but our code is masking
> by 0x1f in helper_fpack32 and helper_fpackfix. Which is right?

I don't know... That manual even says that GSR has no bit defined above
bit 6 (where scale_factor is 3 to 6).

It's possible that a newer processor defines a 5-bit scale factor; I
honestly have no idea.

Paolo

Re: [Qemu-devel] [PATCH] qemu-sockets: do not test path with access() before unlinking

[Markus Armbruster]

Cole Robinson  writes:

> On 11/02/2015 09:10 AM, Paolo Bonzini wrote:
>> Using access() is a time-of-check/time-of-use race condition.  It is
>> okay to use them to provide better error messages, but that is pretty
>> much it.
>> 
>> This is not one such case, so just drop the call.
>> 
>> Signed-off-by: Paolo Bonzini 
>> ---
>>  util/qemu-sockets.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>> 
>> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
>> index 9142917..2833c70 100644
>> --- a/util/qemu-sockets.c
>> +++ b/util/qemu-sockets.c
>> @@ -751,8 +751,7 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
>>  qemu_opt_set(opts, "path", un.sun_path, &error_abort);
>>  }
>>  
>> -if ((access(un.sun_path, F_OK) == 0) &&
>> -unlink(un.sun_path) < 0) {
>> +if (unlink(un.sun_path) < 0) {
>>  error_setg_errno(errp, errno,
>>   "Failed to unlink socket %s", un.sun_path);
>>  goto err;
>> 
>
> This is a serious semantic change, after this patch you will get:
>
> $ ./x86_64-softmmu/qemu-system-x86_64 -vnc unix:/tmp/idontexist.sock
> qemu-system-x86_64: -vnc unix:/tmp/idontexist.sock: Failed to start VNC
> server: Failed to unlink socket /tmp/idontexist.sock: No such file or 
> directory
>
> Previously it would 'just work'. Common libvirt usage depends on this as well
>
> Yeah there's a TOCTTOU race here, but it's very minor: if sun_path is created
> after the access() check, qemu is just going to fail to start since bind()
> will barf if the unix socket path exists.

As Paolo says, access() is almost never a solution to anything.  Here,
the proper solution should be something like

if (unlink(un.sun_path) < 0 && errno != ENOENT) {

Re: [Qemu-devel] RFC: libyajl for JSON

[Eric Blake]

On 11/02/2015 01:40 AM, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> Loaded question in response to
>> https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06988.html, but
> 
> Discussion of our parser's enormous appetite for wasting RAM.  Fixable,
> but it's work, and it's not its only defect.
> 

>> On the other hand, one of the "features" of qemu's hand-rolled json
>> parser is the ability to do qobject_from_jsonf("{'foo':%s}", "bar")
>> (that is, we extended JSON with our notion of single-quoted strings, and
>> with our notion of %s and similar escape sequences for piecing together
>> multiple inputs into a single input stream without having to first
>> g_strdup_printf our pieces into a single string).  I don't know if
>> libyajl lets us add extensions to the language it parses.
> 
> Actually two separate extensions:
> 
> * Single-quoted strings
> 
>   The extension's purpose is avoiding quotes in C.  Example:
> 
>   "{ 'execute': 'migrate_set_speed', 'arguments': { 'value': 10 } }"
> 
>   is easier to read than
> 
>   "{ \"execute\": \"migrate_set_speed\", \"arguments\": { \"value\": 10 } 
> }"
> 
>   Most actual uses are in tests.

Except that we have documented that QMP clients may use it; and indeed:

$ printf "{'execute':'qmp_capabilities'}\n{'execute':'quit'}" | \
   ./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2},
"package": ""}, "capabilities": []}}
{"return": {}}
{"return": {}}
{"timestamp": {"seconds": 1446478389, "microseconds": 265886}, "event":
"SHUTDOWN"}

So that is an absolute must for whatever parser we choose.  My first
glance at libyajl is that it does NOT currently allow single quotes (not
even with any of its existing options), so we'd have to pre-process
input before feeding it to yajl.

> 
>   JSON5, a proposed extension to JSON also supports single-quoted
>   strings.  So does Python.

It would be an interesting task to see if yajl would accept patches to
parse JSON5 as additional options (for example, yajl already has options
on whether to diagnose or ignore UTF8 errors, and on whether to allow /*
*/ javascript comments).  But then we'd be requiring an
as-yet-unreleased version of libyajl rather than being able to rely on
what distros already have, at least for a few years; or we'd have to
carry new-enough yajl as a git submodule within qemu.git.

I'll ask on the yajl mailing lists, to get a feel for community
receptiveness, before attempting to actually write patches on that front.

> 
> * Optional interpolation
> 
>   If you pass a va_list to the parser, it replaces certain escape
>   sequences by values from that va_list.  The escape sequences are a
>   subset of printf conversion specifiers, to enable compile-time
>   checking with __attribute__((format...)).
> 
>   We used to rely on this for creating "rich" error objects, but those
>   are long gone (commit df1e608).  Perhaps two dozen uses are left.

The testsuite is probably the biggest user, but we still have uses
throughout the code base.

Based on my look at libyajl, I think we CAN get away with stream
interpolation - yajl maintains state such that you do NOT have to feed
it the entire stream in one go.  So this one is not insurmountable; we
could rewrite our qobject_from_jsonf() to make multiple calls into yajl
without having to pre-format a single string.

> 
>   We could convert them to use "normal" interpolation, i.e. first format
>   the arguments as JSON, then interpolate with g_strdup_printf() or
>   similar, then parse.  Formatting only to parse it right back is
>   inelegant.  Also inefficient, but that probably doesn't matter here.

Or indeed, this is still an option.

> 
>   The current interface could be retained as convenience function to
>   interpolate and feed the result to the JSOn parser.
> 
>   Alternatively, we could build the QObject manually instead.  More
>   efficient than either kind of interpolation, but a good deal less
>   readable.

At any rate, it is certainly less of a show-stopper when compared to the
need for supporting single-quoted strings.

> 
> Got one more, actually a pitfall, not an extension:
> 
> * Representation of JSON numbers
> 
>   RFC 7159 doesn't specify how numbers are to be represented.
> 
>   Many JSON implementations represent any JSON number as double.  This
>   can represent signed integers up to 53 bits exactly.
> 
>   Our parser represents numbers as int64_t when possible, else as
>   double.
> 
>   Unlike the extensions above, this one is essential: any parser that
>   can't do it is useless for us.  Can yajl do it?

Yes; the yajl parser has 4 modes of parse operation based on which of
three callbacks you implement: double-only (yajl_double), long long-only
(yajl_integer), double-and-int (both yajl_double and yajl_integer, not
sure which one has precedence if input satisfies both formats), or
custom number (yajl_number, which is given a string, and you tu

Re: [Qemu-devel] [PATCH v8 07/17] qapi: Rework collision assertions

[Markus Armbruster]

Eric Blake  writes:

> Now that we have separate namespaces for QMP vs. tag values,

What's the "QMP namespace"?

> we can simplify how the QAPISchema*.check() methods check for
> collisions.

I *guess* the point of this patch is dropping checks that are obsolete
now tag values no longer collide with non-variant member names in
generated C, with simplifications on top.  Correct?

>  Each QAPISchemaObjectTypeMember check() call is
> given a single set of names it must not collide with; this set
> is either the QMP names (when this member is used by an
> ObjectType) or the case names (when this member is used by an
> ObjectTypeVariants).  We no longer need an all_members
> parameter, as it can be computed by seen.values().  When used
> by a union, QAPISchemaObjectTypeVariant must also perform a
> QMP collision check for each member of its corresponding type.

I'm afraid this explanation is next to impossible to understand unless
you know how the checking works.  I should know, because I wrote it, but
actually don't, at least not by heart.  So let me relearn how checking
works before your patch.

We're talking about a single invocation of QAPISchemaObjectType.check().
Its job is to compute self.members and self.base, and do sanity
checking, which includes checking for collisions.  It has two variables
of interest:

* members is where we accumulate the list of members that'll become
  self.members.  It's initialized to the inherited members (empty if no
  base, obviously).

* seen is a dictionary mapping names to members.  This is merely for
  collision checking.  It's initialized to contain the inherited members
  (whose names must be distinct, by induction, because we make sure the
  base type's check() completes first).

For each local member m of self, we call m.check(schema, members, seen).
This is actually QAPISchemaObjectTypeMember.check(), and it uses members
and seen as follows:

* Assert m.name doesn't collide with another member's name, i.e. not in
  seen.

* Append m to members.

* Insert m.name: m into seen.

Straightforward so far.  Coming up next: variants.

Each variant's members are represented as a single member with the tag
value as name.  Its type is an object type that has the variant's
members.

For each variant v:

* Copy seen to vseen.  It holds the non-variant members.

* Call QAPISchemaObjectTypeMember.check(schema, [], vseen), which boils
  down to assert v.name doesn't collide with a non-variant member's
  name.  This check is obsolete.

* Assert v.name is a member of tag_type.

Not checked: variant's name doesn't collide with another variant's name.
That's because we copy seen inside the loop instead of before the loop.

Not checked: variant's members don't collide with non-variant members.
I think this check got lost when we simplified
QAPISchemaObjectTypeVariants to hold a single member.

Note that the real checking happens in check_union(), and the checks
here are just sanity checks.  As long as that's the case, holes here are
harmless.  However, we need them plugged them when we move the real
checking from check_union() to the .check().

Looks like variant checking should be thrown out and redone.

I still don't get your description.  Guess I have to read the patch
after all ;)

> The new ObjectType.check_qmp() is an idempotent subset of
> check(), and can be called multiple times over different seen
> sets (useful, since the members of one type can be applied
> into more than one other location via inheritance or flat
> union variants).

I think I get this argument.  Except I don't get why the method's named
check_qmp().

> The code needs a temporary hack of passing a 'union' flag
> through Variants.check(), since we do not inline the branches
> of an alternate type into a parent QMP object.

What a "QMP object"?  Sounds like a JSON object on the QMP wire, but
that makes no sense :)

> A later patch
> will rework how alternates are laid out, by adding a new
> subclass, and that will allow us to drop the extra parameter.
>
> There are no changes to generated code.
>
> Future patches will enhance testsuite coverage, improve error
> message quality on actual collisions, and move collision
> checks out of ad hoc parse code into the check() methods.
>
> Signed-off-by: Eric Blake 
>
> ---
> v8: rebase to earlier patches, defer positive test additions to
> later in series
> v7: new patch, although it is a much cleaner implementation of
> what was attempted by subset B v8 15/18
> https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg03042.html
> ---
>  scripts/qapi.py | 55 +--
>  1 file changed, 33 insertions(+), 22 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 84ac151..c571709 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -972,28 +972,28 @@ class QAPISchemaObjectType(QAPISchemaType):
>  self.variants = variants
>  s