Re: [Qemu-devel] [PATCH] w32: Fix build with older gcc (unresolved symbol)

[Stefan Weil]

Am 10.05.2013 22:14, schrieb Igor Mitsyanko:
> On 29.03.2013 21:20, Stefan Weil wrote:
>> The cross i586-mingw32msvc-gcc 4.4.4 from Debian Squeeze does not
>> support
>> __sync_val_compare_and_swap by default.
>>
>> Using -march=i686 fixes that and should also result in better code.
>>
>> Signed-off-by: Stefan Weil 
>> ---
>>
>> Maybe this modification is also needed for native gcc-4.4 and older
>> on Linux i386. If yes, we can move the new script code out of the
>> MinGW conditional code.
>>
>> Newer versions of gcc obviously use -march=i686 by default and
>> don't need the patch, but it also won't do any harm for those
>> versions.
>>
>> Stefan
>>
>
> mingw is built with --build=mingw32 and looks like it defaults to
> -march=i386 (I have gcc version 4.7.2).
> Default build on windows is broken without this patch, it should be
> applied to 1.5 probably.
>
> Tested-by: Igor Mitsyanko 
>

With latest QEMU, -march=i486 is used and there should be no problem.
If there still is a problem, we have to look for the reason.

Could you please post the output from configure?

Regards,
Stefan

Re: [Qemu-devel] [PATCH 20/40] pci: use memory core for iommu support

[liu ping fan]

On Wed, May 8, 2013 at 2:30 AM, Peter Maydell  wrote:
> On 7 May 2013 15:16, Paolo Bonzini  wrote:
>> From: Avi Kivity 
>>
>> Use the new iommu support in the memory core for iommu support.  The only
>> user, spapr, is also converted, but it still provides a DMAContext
>> interface until the non-PCI bits switch to AddressSpace.
>>
>> Cc: Michael S. Tsirkin 
>> Signed-off-by: Avi Kivity 
>> Signed-off-by: Paolo Bonzini 
>> ---
>>  hw/pci/pci.c |   53 
>> ++
>>  hw/ppc/spapr_pci.c   |   12 +++---
>>  include/hw/pci/pci.h |7 -
>>  include/hw/pci/pci_bus.h |5 ++-
>>  4 files changed, 46 insertions(+), 31 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 16ed118..3eb397c 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -279,6 +279,16 @@ int pci_find_domain(const PCIBus *bus)
>>  return -1;
>>  }
>>
>> +static MemoryRegion *pci_default_iommu(PCIBus *bus, void *opaque, int devfn)
>> +{
>> +/* FIXME: inherit memory region from bus creator */
>> +return get_system_memory();
>> +}
>
> This seems a bit misnamed since it doesn't actually need to
> return an iommu MemoryRegion (and in fact in most cases it
> won't). What it's actually returning is "this is the memory
> region representing the view for bus master DMA devices to
> DMA into", I think.
>
Maybe pci_fake_iommu ?

> Also, technically speaking get_system_memory() is never the
> right answer, though in practice it's good enough for our
> purposes. (returning get_system_memory() would allow a bus
> master DMA device to access back into the PCI bus by
> DMAing to the address in system memory where the PCI host
> controller is mapped, which I'm guessing is not possible
> on real hardware.)
>
I think although it is rare case, but PCI device can fetch another's
data on its pci-ram, expecially pci-spec does not forbid it.

Regards,
Pingfan

> As you kind of imply with the FIXME comment here, I think
> what we probably actually eventually want is for pci_bus_init()
> and friends to have a parameter for the DMA MemoryRegion
> (but what this patch does is fine for now).
>
> Once these patches go in I could use this to do the
> versatile_pci SMAP registers, though that's more for
> completeness than anything else.
>
>> +
>> +static void pci_default_iommu_dtor(MemoryRegion *mr)
>> +{
>> +}
>> +
>>  static void pci_bus_init(PCIBus *bus, DeviceState *parent,
>>   const char *name,
>>   MemoryRegion *address_space_mem,
>> @@ -289,6 +299,7 @@ static void pci_bus_init(PCIBus *bus, DeviceState 
>> *parent,
>>  bus->devfn_min = devfn_min;
>>  bus->address_space_mem = address_space_mem;
>>  bus->address_space_io = address_space_io;
>> +pci_setup_iommu(bus, pci_default_iommu, NULL, NULL);
>>
>>  /* host bridge */
>>  QLIST_INIT(&bus->child);
>> @@ -801,21 +812,15 @@ static PCIDevice *do_pci_register_device(PCIDevice 
>> *pci_dev, PCIBus *bus,
>>   PCI_SLOT(devfn), PCI_FUNC(devfn), name, 
>> bus->devices[devfn]->name);
>>  return NULL;
>>  }
>> +
>>  pci_dev->bus = bus;
>> -if (bus->dma_context_fn) {
>> -pci_dev->dma = bus->dma_context_fn(bus, bus->dma_context_opaque, 
>> devfn);
>> -} else {
>> -/* FIXME: Make dma_context_fn use MemoryRegions instead, so this 
>> path is
>> - * taken unconditionally */
>> -/* FIXME: inherit memory region from bus creator */
>> -memory_region_init_alias(&pci_dev->bus_master_enable_region, "bus 
>> master",
>> - get_system_memory(), 0,
>> - memory_region_size(get_system_memory()));
>> -memory_region_set_enabled(&pci_dev->bus_master_enable_region, 
>> false);
>> -address_space_init(&pci_dev->bus_master_as, 
>> &pci_dev->bus_master_enable_region);
>> -pci_dev->dma = g_new(DMAContext, 1);
>> -dma_context_init(pci_dev->dma, &pci_dev->bus_master_as);
>> -}
>> +pci_dev->iommu = bus->iommu_fn(bus, bus->iommu_opaque, devfn);
>> +memory_region_init_alias(&pci_dev->bus_master_enable_region, "bus 
>> master",
>> + pci_dev->iommu, 0, 
>> memory_region_size(pci_dev->iommu));
>> +memory_region_set_enabled(&pci_dev->bus_master_enable_region, false);
>> +address_space_init(&pci_dev->bus_master_as, 
>> &pci_dev->bus_master_enable_region);
>> +pci_dev->dma = g_new(DMAContext, 1);
>> +dma_context_init(pci_dev->dma, &pci_dev->bus_master_as);
>>
>>  pci_dev->devfn = devfn;
>>  pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
>> @@ -870,12 +875,12 @@ static void do_pci_unregister_device(PCIDevice 
>> *pci_dev)
>>  pci_dev->bus->devices[pci_dev->devfn] = NULL;
>>  pci_config_free(pci_dev);
>>
>> -if (!pci_dev->bus->dma_context_fn) {
>> -address_space_destroy(&pci_dev->bus_master_as);
>> -memory_region_destroy(&pci

Re: [Qemu-devel] [PATCH v2 2/3] block: add block-backup QMP command

[Eric Blake]

On 04/29/2013 01:42 AM, Stefan Hajnoczi wrote:
> @block-backup
> 
> +++ b/qapi-schema.json
> @@ -1715,6 +1715,37 @@
>  '*speed': 'int' } }
>  
>  ##
> +# @block-backup
> +#
> +# Start a point-in-time copy of a block device to a new destination.  The
> +# status of ongoing block backup operations can be checked with
> +# query-block-jobs.  The operation can be stopped before it has completed 
> using
> +# the block-job-cancel command.

Still might be worth mentioning that 'query-block-jobs' will list it as
a job of type 'backup'.

> +#
> +# @device: the name of the device whose writes should be mirrored.
> +#
> +# @target: the target of the new image. If the file exists, or if it
> +#  is a device, the existing file/device will be used as the new
> +#  destination.  If it does not exist, a new file will be created.
> +#
> +# @format: #optional the format of the new destination, default is to
> +#  probe if @mode is 'existing', else the format of the source
> +#
> +# @mode: #optional whether and how QEMU should create a new image, default is
> +#'absolute-paths'.
> +#
> +# @speed: #optional the maximum speed, in bytes per second
> +#
> +# Returns: nothing on success
> +#  If @device is not a valid block device, DeviceNotFound
> +#
> +# Since 1.6
> +##
> +{ 'command': 'block-backup',
> +  'data': { 'device': 'str', 'target': 'str', '*format': 'str',

Hmm - wondering if we should add an enum type for supported disk formats
instead of using free-form strings.  The wire representation would be
the same, and now's the time to do it before we add introspection (it's
more than just this command impacted).

-- 
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 v2 2/3] block: add block-backup QMP command

[Eric Blake]

On 05/08/2013 06:49 AM, Kevin Wolf wrote:
> Am 29.04.2013 um 09:42 hat Stefan Hajnoczi geschrieben:
>> @block-backup
>>

> drive-backup would probably be a more consistent naming. We would then
> still have block-backup for a future low-level command that doesn't
> create everything by itself but takes an existing BlockDriverState (e.g.
> created by blockdev-add).

At least it would match why we named a command 'drive-mirror' instead of
'block-mirror'.

Hmm, looking at qapi-schema.json, I wonder if we can rename
'BlockdevAction' to 'TransactionAction' as used in the @transaction
command.  It wouldn't change what is sent over the wire in JSON, and
until we have full introspection, there is no visibility into the type
name used.  Changing the name now would let it be more generic to adding
future transaction items that are not blockdev related.

> 
> We should also make it transactionable from the beginning, as we don't
> have schema introspection yet. This way we allow to assume that if the
> standalone command exists, the transaction subcommand exists as well.

Agreed - existence of a command at the same time the command is made
transactionable serves as a nice substitute for not having full
introspection into the 'BlockdevAction' union type, whereas if we
introduce the command now but not transaction support until 1.7, life
becomes tougher to know when it can be used where (although I HOPE we
have introspection in 1.6).

-- 
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 v4] Add 'maxqdepth' as an option to tty character devices.

[Eric Blake]

On 05/07/2013 04:39 PM, John Baboval wrote:
> From: "John V. Baboval" 
> 
> This parameter will cause writes to tty backed chardevs to return
> -EAGAIN if the backing tty has buffered more than the specified
> number of characters. When data is sent, the TIOCOUTQ ioctl is invoked
> to determine the current TTY output buffer depth.
> 

> +++ b/qapi-schema.json
> @@ -3182,11 +3182,14 @@
>  #
>  # @device: The name of the special file for the device,
>  #  i.e. /dev/ttyS0 on Unix or COM1: on Windows
> -# @type: What kind of device this is.
> +#
> +# @maxqdepth: #optional The maximum depth of the underlying tty
> +# output queue (Unix) (Since 1.6)
>  #
>  # Since: 1.4
>  ##
> -{ 'type': 'ChardevHostdev', 'data': { 'device' : 'str' } }
> +{ 'type': 'ChardevHostdev', 'data': { 'device': 'str',
> +  '*maxqdepth' : 'int' } }
>  

Thanks; this interface change looks better (I'd still like to see
someone working on introspection, but it doesn't have to be you).  I'll
still leave the implementation details to others more qualified for that
part of the review.  In particular, since you are claiming this optional
attribute is Linux-only, that means we'd need introspection to know
whether a given qemu build supports the field (compiled on Linux) or not
(compiled on mingw), not just whether the qemu is new enough (1.6) or
older (1.4).

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Anthony Liguori]

Paolo Bonzini  writes:

> Il 10/05/2013 19:41, Anthony Liguori ha scritto:
>> Paolo Bonzini  writes:
>> 
>>> Il 10/05/2013 16:39, Anthony Liguori ha scritto:
 I just oppose the notion of disabling casts and *especially* only
 disabling casts for official builds.
>>>
>>> This actually happens all the time.  Exactly this kind of type-safe cast
>>> is disabled in releases of GCC, but enabled when building from svn trunk.
>> 
>> Let's assume for a moment that you are right and this behavior is what
>> we should have.  Let's also assume there is a real regression here
>> which has yet to have been established.
>
> Aurelien timed the effect of my patch two hours before you sent this
> message.  If it's not a regression in 1.5 (which is quite obvious from
> the profile), it is a regression from the introduction of CPU classes
> (1.3 or 1.4), when this code didn't exist at all.
>
> And in 1.5 we introduced virtio-net casts as well (or did mst sneak in
> his change anyway? ;)). If 10% is the effect of a few hundred
> interrupts/sec, perhaps the same effect is visible on a few thousand
> packets/sec.  I wouldn't bet against that one week from release.

The thing is, none of these casts should be for more than 1 level and
the patch I provided makes those casts almost free.

I believe the reason it didn't fix the problem for Aurelien is because
the string addresses were different because of different compilation
units.  I guess binutils doesn't collapse strings when linking.

>> As soon as we open up 1.6 for development, we face an immediate
>> regression in performance.  So we need to fix the real problem here
>> anyway.
>
> No, we don't.  We will simply have to live with a debugging vs.
> production tradeoff.

I appreciate all of the arguments below.  I'm very concerned about
reducing safety checks but am sympathetic to performance concerns.

Here's what I would like to do.  I'll apply 1-6 of your series which
gives us the infrastructure to disable qom casts.  That at least let's
the code get tested in case we need it.

I will hold off applying patch 7 hoping that the patch I provided to
Aurelien solves the problem.  If it doesn't and we're unable to find a
better solution, I'll apply patch 7 for the release.

Either way, the infrastructure is there for a distro to make a decision
although I think disabling qom casts is an extremely bad decision to
make.

Given the v2 version of my patch, I'm quite confident that casting in
the vast majority of circumstances would avoid a g_hash_table lookup so
that should eliminate any performance concerns.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH for-1.5] qom: optimize casting to leaf class and parent class

[Anthony Liguori]

Aurelien Jarno  writes:

> On Fri, May 10, 2013 at 01:47:55PM -0500, Anthony Liguori wrote:
>> Most QOM types use type_register_static but we still strdup the
>> passed data.  However, the original pointers are useful because
>> GCC is pretty good about collapsing strings so its very likely any
>> use of the pointer will end up being that same address.
>> 
>> IOW, with a little trickery, we can compare types by just comparing
>> strings and in fact that's what we do here.
>> 
>> We do this for the two most common cases, casting to a leaf class
>> or to the parent class.
>> 
>> With these two changes, I see a decrease from around 2 hash table
>> lookups to only a thousand with no run time lookups at all.
>> 
>> Cc: Paolo Bonzini 
>> Cc: Aurelien Jarno 
>> Cc: Andreas Färber 
>> Reported-by: Aurelien Jarno 
>> Signed-off-by: Anthony Liguori 
>> ---
>> Aurelien, could you please try this patch with your PPC test case?
>> ---
>>  qom/object.c | 16 ++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>> 
>> diff --git a/qom/object.c b/qom/object.c
>> index 75e6aac..5ecfd28 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -132,7 +132,13 @@ TypeImpl *type_register(const TypeInfo *info)
>>  
>>  TypeImpl *type_register_static(const TypeInfo *info)
>>  {
>> -return type_register(info);
>> +TypeImpl *impl;
>> +
>> +impl = type_register(info);
>> +impl->name = info->name;
>> +impl->parent = info->parent;
>> +
>> +return impl;
>>  }
>>  
>>  static TypeImpl *type_get_by_name(const char *name)
>> @@ -449,10 +455,16 @@ Object *object_dynamic_cast_assert(Object *obj, const 
>> char *typename)
>>  ObjectClass *object_class_dynamic_cast(ObjectClass *class,
>> const char *typename)
>>  {
>> -TypeImpl *target_type = type_get_by_name(typename);
>> +TypeImpl *target_type;
>>  TypeImpl *type = class->type;
>>  ObjectClass *ret = NULL;
>>  
>> +if (type->name == typename || type->parent == typename) {
>> +return class;
>> +}
>> +
>> +target_type = type_get_by_name(typename);
>> +
>>  if (!target_type) {
>>  /* target class type unknown, so fail the cast */
>>  return NULL;
>
> Unfortunately it doesn't fix the problem. I only see a 0.5% improvement,
> which might be in the noise. I still see g_hash_table_lookup and
> g_str_hash quite high in perf top.

I was afraid of this.  I assume the cast comes somewhere other than
where the type was registered.

This patch should address that.  Could you post an image too?  Then I
don't have to keep bugging you with updated patches.

>From 269442decf0f137cf7d09c32442fcd16dd319901 Mon Sep 17 00:00:00 2001
From: Anthony Liguori 
Date: Fri, 10 May 2013 13:43:11 -0500
Subject: [PATCH] qom: optimize casting to leaf class and parent class (v2)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Most QOM types use type_register_static but we still strdup the
passed data.  However, the original pointers are useful because
GCC is pretty good about collapsing strings so its very likely any
use of the pointer will end up being that same address.

IOW, with a little trickery, we can compare types by just comparing
strings and in fact that's what we do here.

We do this for the two most common cases, casting to a leaf class
or to the parent class.

With these two changes, I see a decrease from around 2 hash table
lookups to only a thousand with no run time lookups at all.

Cc: Paolo Bonzini 
Cc: Aurelien Jarno 
Cc: Andreas Färber 
Reported-by: Aurelien Jarno 
Signed-off-by: Anthony Liguori 
---
v1 -> v2:
 - use strcmp() instead of ==
---
 qom/object.c | 17 +++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 75e6aac..99b5b33 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -132,7 +132,13 @@ TypeImpl *type_register(const TypeInfo *info)
 
 TypeImpl *type_register_static(const TypeInfo *info)
 {
-return type_register(info);
+TypeImpl *impl;
+
+impl = type_register(info);
+impl->name = info->name;
+impl->parent = info->parent;
+
+return impl;
 }
 
 static TypeImpl *type_get_by_name(const char *name)
@@ -449,10 +455,17 @@ Object *object_dynamic_cast_assert(Object *obj, const char *typename)
 ObjectClass *object_class_dynamic_cast(ObjectClass *class,
const char *typename)
 {
-TypeImpl *target_type = type_get_by_name(typename);
+TypeImpl *target_type;
 TypeImpl *type = class->type;
 ObjectClass *ret = NULL;
 
+if (strcmp(type->name, typename) == 0 ||
+strcmp(type->parent, typename) == 0) {
+return class;
+}
+
+target_type = type_get_by_name(typename);
+
 if (!target_type) {
 /* target class type unknown, so fail the cast */
 return NULL;
-- 
1.8.0


Regards,

Anthony Liguori

>
> -- 
> Aurelien JarnoGPG: 1024D/F

[Qemu-devel] [PATCH 11/11] qapi: add native list coverage for QMP input visitor tests

[Michael Roth]

This exercises schema-generated visitors for native list types and does
some sanity checking on validity of deserialized data.

Signed-off-by: Michael Roth 
---
 tests/test-qmp-input-visitor.c |  338 
 1 file changed, 338 insertions(+)

diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index b308cf9..2741eef 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -61,6 +61,31 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data,
 return v;
 }
 
+/* similar to visitor_input_test_init(), but does not expect a string
+ * literal/format json_string argument and so can be used for
+ * programatically generated strings (and we can't pass in programatically
+ * generated strings via %s format parameters since qobject_from_jsonv()
+ * will wrap those in double-quotes and treat the entire object as a
+ * string)
+ */
+static Visitor *visitor_input_test_init_raw(TestInputVisitorData *data,
+const char *json_string)
+{
+Visitor *v;
+
+data->obj = qobject_from_json(json_string);
+
+g_assert(data->obj != NULL);
+
+data->qiv = qmp_input_visitor_new(data->obj);
+g_assert(data->qiv != NULL);
+
+v = qmp_input_get_visitor(data->qiv);
+g_assert(v != NULL);
+
+return v;
+}
+
 static void test_visitor_in_int(TestInputVisitorData *data,
 const void *unused)
 {
@@ -277,6 +302,287 @@ static void test_visitor_in_union(TestInputVisitorData 
*data,
 qapi_free_UserDefUnion(tmp);
 }
 
+static void test_native_list_integer_helper(TestInputVisitorData *data,
+const void *unused,
+UserDefNativeListUnionKind kind)
+{
+UserDefNativeListUnion *cvalue = NULL;
+Error *err = NULL;
+Visitor *v;
+GString *gstr_list = g_string_new("");
+GString *gstr_union = g_string_new("");
+int i;
+
+for (i = 0; i < 32; i++) {
+g_string_append_printf(gstr_list, "%d", i);
+if (i != 31) {
+g_string_append(gstr_list, ", ");
+}
+}
+g_string_append_printf(gstr_union,  "{ 'type': '%s', 'data': [ %s ] }",
+   UserDefNativeListUnionKind_lookup[kind],
+   gstr_list->str);
+v = visitor_input_test_init_raw(data,  gstr_union->str);
+
+visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &err);
+g_assert(err == NULL);
+g_assert(cvalue != NULL);
+g_assert_cmpint(cvalue->kind, ==, kind);
+
+switch (kind) {
+case USER_DEF_NATIVE_LIST_UNION_KIND_INTEGER: {
+intList *elem = NULL;
+for (i = 0, elem = cvalue->integer; elem; elem = elem->next, i++) {
+g_assert_cmpint(elem->value, ==, i);
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_S8: {
+int8List *elem = NULL;
+for (i = 0, elem = cvalue->s8; elem; elem = elem->next, i++) {
+g_assert_cmpint(elem->value, ==, i);
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_S16: {
+int16List *elem = NULL;
+for (i = 0, elem = cvalue->s16; elem; elem = elem->next, i++) {
+g_assert_cmpint(elem->value, ==, i);
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_S32: {
+int32List *elem = NULL;
+for (i = 0, elem = cvalue->s32; elem; elem = elem->next, i++) {
+g_assert_cmpint(elem->value, ==, i);
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_S64: {
+int64List *elem = NULL;
+for (i = 0, elem = cvalue->s64; elem; elem = elem->next, i++) {
+g_assert_cmpint(elem->value, ==, i);
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_U8: {
+uint8List *elem = NULL;
+for (i = 0, elem = cvalue->u8; elem; elem = elem->next, i++) {
+g_assert_cmpint(elem->value, ==, i);
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_U16: {
+uint16List *elem = NULL;
+for (i = 0, elem = cvalue->u16; elem; elem = elem->next, i++) {
+g_assert_cmpint(elem->value, ==, i);
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_U32: {
+uint32List *elem = NULL;
+for (i = 0, elem = cvalue->u32; elem; elem = elem->next, i++) {
+g_assert_cmpint(elem->value, ==, i);
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_U64: {
+uint64List *elem = NULL;
+for (i = 0, elem = cvalue->u64; elem; elem = elem->next, i++) {
+g_assert_cmpint(elem->value, ==, i);
+}
+break;
+}
+default:
+g_assert(false);
+}
+
+g_string_free(gstr_union, true);
+g_string_free(gstr_list, true);
+qapi_free_UserDefNativeListUnion(cvalue);
+}
+
+static void test_visitor_in_native_list

[Qemu-devel] [PATCH 09/11] qapi: add native list coverage for visitor serialization tests

[Michael Roth]

Signed-off-by: Michael Roth 
---
 tests/test-visitor-serialization.c |  451 ++--
 1 file changed, 433 insertions(+), 18 deletions(-)

diff --git a/tests/test-visitor-serialization.c 
b/tests/test-visitor-serialization.c
index fed6810..ee7916b 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -23,6 +23,25 @@
 #include "qapi/qmp-output-visitor.h"
 #include "qapi/string-input-visitor.h"
 #include "qapi/string-output-visitor.h"
+#include "qapi-types.h"
+#include "qapi-visit.h"
+#include "qapi/dealloc-visitor.h"
+
+enum PrimitiveTypeKind {
+PTYPE_STRING = 0,
+PTYPE_BOOLEAN,
+PTYPE_NUMBER,
+PTYPE_INTEGER,
+PTYPE_U8,
+PTYPE_U16,
+PTYPE_U32,
+PTYPE_U64,
+PTYPE_S8,
+PTYPE_S16,
+PTYPE_S32,
+PTYPE_S64,
+PTYPE_EOL,
+};
 
 typedef struct PrimitiveType {
 union {
@@ -40,26 +59,42 @@ typedef struct PrimitiveType {
 int64_t s64;
 intmax_t max;
 } value;
-enum {
-PTYPE_STRING = 0,
-PTYPE_BOOLEAN,
-PTYPE_NUMBER,
-PTYPE_INTEGER,
-PTYPE_U8,
-PTYPE_U16,
-PTYPE_U32,
-PTYPE_U64,
-PTYPE_S8,
-PTYPE_S16,
-PTYPE_S32,
-PTYPE_S64,
-PTYPE_EOL,
-} type;
+enum PrimitiveTypeKind type;
 const char *description;
 } PrimitiveType;
 
+typedef struct PrimitiveList {
+union {
+strList *strings;
+boolList *booleans;
+numberList *numbers;
+intList *integers;
+int8List *s8_integers;
+int16List *s16_integers;
+int32List *s32_integers;
+int64List *s64_integers;
+uint8List *u8_integers;
+uint16List *u16_integers;
+uint32List *u32_integers;
+uint64List *u64_integers;
+} value;
+enum PrimitiveTypeKind type;
+const char *description;
+} PrimitiveList;
+
 /* test helpers */
 
+typedef void (*VisitorFunc)(Visitor *v, void **native, Error **errp);
+
+static void dealloc_helper(void *native_in, VisitorFunc visit, Error **errp)
+{
+QapiDeallocVisitor *qdv = qapi_dealloc_visitor_new();
+
+visit(qapi_dealloc_get_visitor(qdv), &native_in, errp);
+
+qapi_dealloc_visitor_cleanup(qdv);
+}
+
 static void visit_primitive_type(Visitor *v, void **native, Error **errp)
 {
 PrimitiveType *pt = *native;
@@ -105,6 +140,51 @@ static void visit_primitive_type(Visitor *v, void 
**native, Error **errp)
 }
 }
 
+static void visit_primitive_list(Visitor *v, void **native, Error **errp)
+{
+PrimitiveList *pl = *native;
+switch (pl->type) {
+case PTYPE_STRING:
+visit_type_strList(v, &pl->value.strings, NULL, errp);
+break;
+case PTYPE_BOOLEAN:
+visit_type_boolList(v, &pl->value.booleans, NULL, errp);
+break;
+case PTYPE_NUMBER:
+visit_type_numberList(v, &pl->value.numbers, NULL, errp);
+break;
+case PTYPE_INTEGER:
+visit_type_intList(v, &pl->value.integers, NULL, errp);
+break;
+case PTYPE_S8:
+visit_type_int8List(v, &pl->value.s8_integers, NULL, errp);
+break;
+case PTYPE_S16:
+visit_type_int16List(v, &pl->value.s16_integers, NULL, errp);
+break;
+case PTYPE_S32:
+visit_type_int32List(v, &pl->value.s32_integers, NULL, errp);
+break;
+case PTYPE_S64:
+visit_type_int64List(v, &pl->value.s64_integers, NULL, errp);
+break;
+case PTYPE_U8:
+visit_type_uint8List(v, &pl->value.u8_integers, NULL, errp);
+break;
+case PTYPE_U16:
+visit_type_uint16List(v, &pl->value.u16_integers, NULL, errp);
+break;
+case PTYPE_U32:
+visit_type_uint32List(v, &pl->value.u32_integers, NULL, errp);
+break;
+case PTYPE_U64:
+visit_type_uint64List(v, &pl->value.u64_integers, NULL, errp);
+break;
+default:
+g_assert(false);
+}
+}
+
 typedef struct TestStruct
 {
 int64_t integer;
@@ -206,12 +286,11 @@ static void visit_nested_struct_list(Visitor *v, void 
**native, Error **errp)
 
 /* test cases */
 
-typedef void (*VisitorFunc)(Visitor *v, void **native, Error **errp);
-
 typedef enum VisitorCapabilities {
 VCAP_PRIMITIVES = 1,
 VCAP_STRUCTURES = 2,
 VCAP_LISTS = 4,
+VCAP_PRIMITIVE_LISTS = 8,
 } VisitorCapabilities;
 
 typedef struct SerializeOps {
@@ -270,6 +349,328 @@ static void test_primitives(gconstpointer opaque)
 g_free(pt_copy);
 }
 
+static void test_primitive_lists(gconstpointer opaque)
+{
+TestArgs *args = (TestArgs *) opaque;
+const SerializeOps *ops = args->ops;
+PrimitiveType *pt = args->test_data;
+PrimitiveList pl = { .value = { 0 } };
+PrimitiveList pl_copy = { .value = { 0 } };
+PrimitiveList *pl_copy_ptr = &pl_copy;
+Error *err = NULL;
+void *serialize_data;
+void *cur_head = NULL;
+int i;
+
+pl.type = pl_copy.type = pt->type;
+
+/* build up our list of primitive type

[Qemu-devel] [PATCH 10/11] qapi: add native list coverage for QMP output visitor tests

[Michael Roth]

This exercises schema-generated visitors for native list types and does
some sanity checking on validity of serialized data.

Signed-off-by: Michael Roth 
---
 qapi-schema-test.json   |   15 ++
 tests/test-qmp-output-visitor.c |  332 +++
 2 files changed, 347 insertions(+)

diff --git a/qapi-schema-test.json b/qapi-schema-test.json
index 9eae350..4434fa3 100644
--- a/qapi-schema-test.json
+++ b/qapi-schema-test.json
@@ -32,6 +32,21 @@
 { 'union': 'UserDefUnion',
   'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
 
+# for testing native lists
+{ 'union': 'UserDefNativeListUnion',
+  'data': { 'integer': ['int'],
+'s8': ['int8'],
+'s16': ['int16'],
+'s32': ['int32'],
+'s64': ['int64'],
+'u8': ['uint8'],
+'u16': ['uint16'],
+'u32': ['uint32'],
+'u64': ['uint64'],
+'number': ['number'],
+'boolean': ['bool'],
+'string': ['str'] } }
+
 # testing commands
 { 'command': 'user_def_cmd', 'data': {} }
 { 'command': 'user_def_cmd1', 'data': {'ud1a': 'UserDefOne'} }
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 71367e6..0942a41 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -431,6 +431,314 @@ static void test_visitor_out_union(TestOutputVisitorData 
*data,
 QDECREF(qdict);
 }
 
+static void init_native_list(UserDefNativeListUnion *cvalue)
+{
+int i;
+switch (cvalue->kind) {
+case USER_DEF_NATIVE_LIST_UNION_KIND_INTEGER: {
+intList **list = &cvalue->integer;
+for (i = 0; i < 32; i++) {
+*list = g_new0(intList, 1);
+(*list)->value = i;
+(*list)->next = NULL;
+list = &(*list)->next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_S8: {
+int8List **list = &cvalue->s8;
+for (i = 0; i < 32; i++) {
+*list = g_new0(int8List, 1);
+(*list)->value = i;
+(*list)->next = NULL;
+list = &(*list)->next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_S16: {
+int16List **list = &cvalue->s16;
+for (i = 0; i < 32; i++) {
+*list = g_new0(int16List, 1);
+(*list)->value = i;
+(*list)->next = NULL;
+list = &(*list)->next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_S32: {
+int32List **list = &cvalue->s32;
+for (i = 0; i < 32; i++) {
+*list = g_new0(int32List, 1);
+(*list)->value = i;
+(*list)->next = NULL;
+list = &(*list)->next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_S64: {
+int64List **list = &cvalue->s64;
+for (i = 0; i < 32; i++) {
+*list = g_new0(int64List, 1);
+(*list)->value = i;
+(*list)->next = NULL;
+list = &(*list)->next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_U8: {
+uint8List **list = &cvalue->u8;
+for (i = 0; i < 32; i++) {
+*list = g_new0(uint8List, 1);
+(*list)->value = i;
+(*list)->next = NULL;
+list = &(*list)->next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_U16: {
+uint16List **list = &cvalue->u16;
+for (i = 0; i < 32; i++) {
+*list = g_new0(uint16List, 1);
+(*list)->value = i;
+(*list)->next = NULL;
+list = &(*list)->next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_U32: {
+uint32List **list = &cvalue->u32;
+for (i = 0; i < 32; i++) {
+*list = g_new0(uint32List, 1);
+(*list)->value = i;
+(*list)->next = NULL;
+list = &(*list)->next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_U64: {
+uint64List **list = &cvalue->u64;
+for (i = 0; i < 32; i++) {
+*list = g_new0(uint64List, 1);
+(*list)->value = i;
+(*list)->next = NULL;
+list = &(*list)->next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_BOOLEAN: {
+boolList **list = &cvalue->boolean;
+for (i = 0; i < 32; i++) {
+*list = g_new0(boolList, 1);
+(*list)->value = (i % 3 == 0);
+(*list)->next = NULL;
+list = &(*list)->next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_STRING: {
+strList **list = &cvalue->string;
+for (i = 0; i < 32; i++) {
+*list = g_new0(strList, 1);
+(*list)->value = g_strdup_printf("%d", i);
+(*list)->next = NULL;
+list = &(*list)->next;
+}
+break;
+}
+case USER_D

[Qemu-devel] [PATCH 08/11] qapi: fix visitor serialization tests for numbers/doubles

[Michael Roth]

We never actually stored the stringified double values into the strings
before we did the comparisons. This left number/double values completely
uncovered in test-visitor-serialization tests.

Fixing this exposed a bug in our handling of large whole number values
in QEMU's JSON parser which is now fixed.

Simplify the code while we're at it by dropping the
calc_float_string_storage() craziness in favor of GStrings.

Signed-off-by: Michael Roth 
---
 tests/test-visitor-serialization.c |   25 -
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/tests/test-visitor-serialization.c 
b/tests/test-visitor-serialization.c
index 8c8adac..fed6810 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -229,17 +229,6 @@ typedef struct TestArgs {
 void *test_data;
 } TestArgs;
 
-#define FLOAT_STRING_PRECISION 6 /* corresponding to n in %.nf formatting */
-static gsize calc_float_string_storage(double value)
-{
-int whole_value = value;
-gsize i = 0;
-do {
-i++;
-} while (whole_value /= 10);
-return i + 2 + FLOAT_STRING_PRECISION;
-}
-
 static void test_primitives(gconstpointer opaque)
 {
 TestArgs *args = (TestArgs *) opaque;
@@ -248,7 +237,6 @@ static void test_primitives(gconstpointer opaque)
 PrimitiveType *pt_copy = g_malloc0(sizeof(*pt_copy));
 Error *err = NULL;
 void *serialize_data;
-char *double1, *double2;
 
 pt_copy->type = pt->type;
 ops->serialize(pt, &serialize_data, visit_primitive_type, &err);
@@ -260,14 +248,17 @@ static void test_primitives(gconstpointer opaque)
 g_assert_cmpstr(pt->value.string, ==, pt_copy->value.string);
 g_free((char *)pt_copy->value.string);
 } else if (pt->type == PTYPE_NUMBER) {
+GString *double_expected = g_string_new("");
+GString *double_actual = g_string_new("");
 /* we serialize with %f for our reference visitors, so rather than 
fuzzy
  * floating math to test "equality", just compare the formatted values
  */
-double1 = g_malloc0(calc_float_string_storage(pt->value.number));
-double2 = g_malloc0(calc_float_string_storage(pt_copy->value.number));
-g_assert_cmpstr(double1, ==, double2);
-g_free(double1);
-g_free(double2);
+g_string_printf(double_expected, "%.6f", pt->value.number);
+g_string_printf(double_actual, "%.6f", pt_copy->value.number);
+g_assert_cmpstr(double_actual->str, ==, double_expected->str);
+
+g_string_free(double_expected, true);
+g_string_free(double_actual, true);
 } else if (pt->type == PTYPE_BOOLEAN) {
 g_assert_cmpint(!!pt->value.max, ==, !!pt->value.max);
 } else {
-- 
1.7.9.5

[Qemu-devel] [PATCH 06/11] json-parser: fix handling of large whole number values

[Michael Roth]

Currently our JSON parser assumes that numbers lacking a fractional
value are integers and attempts to store them as QInt/int64 values. This
breaks in the case where the number overflows/underflows int64 values (which
is still valid JSON)

Fix this by detecting such cases and using a QFloat to store the value
instead.

Signed-off-by: Michael Roth 
---
 qobject/json-parser.c |   26 +++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 05279c1..e7947b3 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -640,9 +640,29 @@ static QObject *parse_literal(JSONParserContext *ctxt)
 case JSON_STRING:
 obj = QOBJECT(qstring_from_escaped_str(ctxt, token));
 break;
-case JSON_INTEGER:
-obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 
10)));
-break;
+case JSON_INTEGER: {
+/* A possibility exists that this is a whole-valued float where the
+ * fractional part was left out due to being 0 (.0). It's not a big
+ * deal to treat these as ints in the parser, so long as users of the
+ * resulting QObject know to expect a QInt in place of a QFloat in
+ * cases like these.
+ *
+ * However, in some cases these values will overflow/underflow a
+ * QInt/int64 container, thus we should assume these are to be handled
+ * as QFloats/doubles rather than silently changing their values.
+ *
+ * strtoll() indicates these instances by setting errno to ERANGE
+ */
+int64_t value;
+
+errno = 0; /* strtoll doesn't set errno on success */
+value = strtoll(token_get_value(token), NULL, 10);
+if (errno != ERANGE) {
+obj = QOBJECT(qint_from_int(value));
+break;
+}
+/* fall through to JSON_FLOAT */
+}
 case JSON_FLOAT:
 /* FIXME dependent on locale */
 obj = QOBJECT(qfloat_from_double(strtod(token_get_value(token), 
NULL)));
-- 
1.7.9.5

[Qemu-devel] [PATCH 07/11] qapi: add QMP input test for large integers

[Michael Roth]

Large integers previously got capped to LLONG_MAX/LLONG_MIN so we could
store them as int64_t. This could lead to silent errors occuring.

Now, we use a double to handle these cases.

Add a test to confirm that QMPInputVisitor handles this as expected if
we're expected an integer value: errors for out of range integer values
that got promoted to doubles in this fashion.

Signed-off-by: Michael Roth 
---
 tests/test-qmp-input-visitor.c |   20 
 1 file changed, 20 insertions(+)

diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 955a4c0..b308cf9 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -75,6 +75,24 @@ static void test_visitor_in_int(TestInputVisitorData *data,
 g_assert_cmpint(res, ==, value);
 }
 
+static void test_visitor_in_int_overflow(TestInputVisitorData *data,
+ const void *unused)
+{
+int64_t res = 0;
+Error *errp = NULL;
+Visitor *v;
+
+/* this will overflow a Qint/int64, so should be deserialized into
+ * a QFloat/double field instead, leading to an error if we pass it
+ * to visit_type_int. confirm this.
+ */
+v = visitor_input_test_init(data, "%f", DBL_MAX);
+
+visit_type_int(v, &res, NULL, &errp);
+g_assert(error_is_set(&errp));
+error_free(errp);
+}
+
 static void test_visitor_in_bool(TestInputVisitorData *data,
  const void *unused)
 {
@@ -292,6 +310,8 @@ int main(int argc, char **argv)
 
 input_visitor_test_add("/visitor/input/int",
&in_visitor_data, test_visitor_in_int);
+input_visitor_test_add("/visitor/input/int_overflow",
+   &in_visitor_data, test_visitor_in_int_overflow);
 input_visitor_test_add("/visitor/input/bool",
&in_visitor_data, test_visitor_in_bool);
 input_visitor_test_add("/visitor/input/number",
-- 
1.7.9.5

[Qemu-devel] [PATCH 03/11] qapi: qapi-visit.py, native list support

[Michael Roth]

Teach visitor generators about native types so they can generate the
appropriate visitor routines.

Signed-off-by: Michael Roth 
---
 scripts/qapi-visit.py |   34 +-
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 4c4de4b..6cac05a 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -202,12 +202,14 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, 
const char *name, Error **
 
 return ret
 
-def generate_declaration(name, members, genlist=True):
-ret = mcgen('''
+def generate_declaration(name, members, genlist=True, builtin_type=False):
+ret = ""
+if not builtin_type:
+ret += mcgen('''
 
 void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error 
**errp);
 ''',
-name=name)
+name=name)
 
 if genlist:
 ret += mcgen('''
@@ -235,8 +237,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s * obj, const 
char *name, Error **e
 name=name)
 
 try:
-opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:o:",
-   ["source", "header", "prefix=", 
"output-dir="])
+opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:",
+   ["source", "header", "builtins", "prefix=",
+"output-dir="])
 except getopt.GetoptError, err:
 print str(err)
 sys.exit(1)
@@ -248,6 +251,7 @@ h_file = 'qapi-visit.h'
 
 do_c = False
 do_h = False
+do_builtins = False
 
 for o, a in opts:
 if o in ("-p", "--prefix"):
@@ -258,6 +262,8 @@ for o, a in opts:
 do_c = True
 elif o in ("-h", "--header"):
 do_h = True
+elif o in ("-b", "--builtins"):
+do_builtins = True
 
 if not do_c and not do_h:
 do_c = True
@@ -324,11 +330,29 @@ fdecl.write(mcgen('''
 
 #include "qapi/visitor.h"
 #include "%(prefix)sqapi-types.h"
+
 ''',
   prefix=prefix, guard=guardname(h_file)))
 
 exprs = parse_schema(sys.stdin)
 
+# to avoid header dependency hell, we always generate declarations
+# for built-in types in our header files and simply guard them
+fdecl.write(guardstart("QAPI_VISIT_BUILTIN_VISITOR_DECL"))
+for typename in builtin_types:
+fdecl.write(generate_declaration(typename, None, genlist=True,
+ builtin_type=True))
+fdecl.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DECL"))
+
+# ...this doesn't work for cases where we link in multiple objects that
+# have the functions defined, so we use -b option to provide control
+# over these cases
+if do_builtins:
+fdef.write(guardstart("QAPI_VISIT_BUILTIN_VISITOR_DEF"))
+for typename in builtin_types:
+fdef.write(generate_visit_list(typename, None))
+fdef.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DEF"))
+
 for expr in exprs:
 if expr.has_key('type'):
 ret = generate_visit_struct(expr['type'], expr['data'])
-- 
1.7.9.5

[Qemu-devel] [PATCH 05/11] qapi: fix leak in unit tests

[Michael Roth]

qmp_output_get_qobject() increments the qobject's reference count. Since
we currently pass this straight into qobject_to_json() so we can feed
the data into a QMP input visitor, we never actually free the underlying
qobject when qmp_output_visitor_cleanup() is called. This causes leaks
on all of the QMP serialization tests.

Fix this by holding a pointer to the qobject and decref'ing it before
returning from qmp_deserialize().

Signed-off-by: Michael Roth 
---
 tests/test-visitor-serialization.c |9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/test-visitor-serialization.c 
b/tests/test-visitor-serialization.c
index e84926f..8c8adac 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -657,11 +657,16 @@ static void qmp_deserialize(void **native_out, void 
*datap,
 VisitorFunc visit, Error **errp)
 {
 QmpSerializeData *d = datap;
-QString *output_json = qobject_to_json(qmp_output_get_qobject(d->qov));
-QObject *obj = qobject_from_json(qstring_get_str(output_json));
+QString *output_json;
+QObject *obj_orig, *obj;
+
+obj_orig = qmp_output_get_qobject(d->qov);
+output_json = qobject_to_json(obj_orig);
+obj = qobject_from_json(qstring_get_str(output_json));
 
 QDECREF(output_json);
 d->qiv = qmp_input_visitor_new(obj);
+qobject_decref(obj_orig);
 qobject_decref(obj);
 visit(qmp_input_get_visitor(d->qiv), native_out, errp);
 }
-- 
1.7.9.5

[Qemu-devel] [PATCH 04/11] qapi: enable generation of native list code

[Michael Roth]

Also, fix a dependency issue with libqemuutil: qemu-sockets.c needs
qapi-types.c/qapi-visit.c

Signed-off-by: Michael Roth 
---
 Makefile |6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 7dc0204..9695c9d 100644
--- a/Makefile
+++ b/Makefile
@@ -178,7 +178,7 @@ Makefile: $(version-obj-y) $(version-lobj-y)
 # Build libraries
 
 libqemustub.a: $(stub-obj-y)
-libqemuutil.a: $(util-obj-y)
+libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o
 
 ##
 
@@ -215,10 +215,10 @@ $(SRC_PATH)/qga/qapi-schema.json 
$(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
 
 qapi-types.c qapi-types.h :\
 $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
-   $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py 
$(gen-out-type) -o "." < $<, "  GEN   $@")
+   $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py 
$(gen-out-type) -o "." -b < $<, "  GEN   $@")
 qapi-visit.c qapi-visit.h :\
 $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
-   $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py 
$(gen-out-type) -o "."  < $<, "  GEN   $@")
+   $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py 
$(gen-out-type) -o "." -b < $<, "  GEN   $@")
 qmp-commands.h qmp-marshal.c :\
 $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py 
$(gen-out-type) -m -o "." < $<, "  GEN   $@")
-- 
1.7.9.5

[Qemu-devel] [PATCH 02/11] qapi: qapi-visit.py, fix list handling for union types

[Michael Roth]

Currently we assume non-list types when generating visitor routines for
union types. This is broken, since values like ['Type'] need to mapped
to 'TypeList'.

We already have a type_name() function to handle this that we use for
generating struct visitors, so use that here as well.

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

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index a276540..4c4de4b 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -174,7 +174,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const 
char *name, Error **
 ''',
 abbrev = de_camel_case(name).upper(),
 enum = c_fun(de_camel_case(key),False).upper(),
-c_type=members[key],
+c_type=type_name(members[key]),
 c_name=c_fun(key))
 
 ret += mcgen('''
-- 
1.7.9.5

[Qemu-devel] [PATCH 01/11] qapi: qapi-types.py, native list support

[Michael Roth]

Teach type generators about native types so they can generate the
appropriate linked list types.

Signed-off-by: Michael Roth 
---
 scripts/qapi-types.py |   45 ++---
 scripts/qapi.py   |   23 +++
 2 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 9e19920..fd42d71 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -16,8 +16,21 @@ import os
 import getopt
 import errno
 
-def generate_fwd_struct(name, members):
+def generate_fwd_struct(name, members, builtin_type=False):
+if builtin_type:
+return mcgen('''
+
+typedef struct %(name)sList
+{
+%(type)s value;
+struct %(name)sList *next;
+} %(name)sList;
+''',
+ type=c_type(name),
+ name=name)
+
 return mcgen('''
+
 typedef struct %(name)s %(name)s;
 
 typedef struct %(name)sList
@@ -164,6 +177,7 @@ void qapi_free_%(type)s(%(c_type)s obj);
 
 def generate_type_cleanup(name):
 ret = mcgen('''
+
 void qapi_free_%(type)s(%(c_type)s obj)
 {
 QapiDeallocVisitor *md;
@@ -184,8 +198,9 @@ void qapi_free_%(type)s(%(c_type)s obj)
 
 
 try:
-opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:o:",
-   ["source", "header", "prefix=", 
"output-dir="])
+opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:",
+   ["source", "header", "builtins",
+"prefix=", "output-dir="])
 except getopt.GetoptError, err:
 print str(err)
 sys.exit(1)
@@ -197,6 +212,7 @@ h_file = 'qapi-types.h'
 
 do_c = False
 do_h = False
+do_builtins = False
 
 for o, a in opts:
 if o in ("-p", "--prefix"):
@@ -207,6 +223,8 @@ for o, a in opts:
 do_c = True
 elif o in ("-h", "--header"):
 do_h = True
+elif o in ("-b", "--builtins"):
+do_builtins = True
 
 if not do_c and not do_h:
 do_c = True
@@ -282,6 +300,11 @@ fdecl.write(mcgen('''
 exprs = parse_schema(sys.stdin)
 exprs = filter(lambda expr: not expr.has_key('gen'), exprs)
 
+fdecl.write(guardstart("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
+for typename in builtin_types:
+fdecl.write(generate_fwd_struct(typename, None, builtin_type=True))
+fdecl.write(guardend("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
+
 for expr in exprs:
 ret = "\n"
 if expr.has_key('type'):
@@ -298,6 +321,22 @@ for expr in exprs:
 continue
 fdecl.write(ret)
 
+# to avoid header dependency hell, we always generate declarations
+# for built-in types in our header files and simply guard them
+fdecl.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
+for typename in builtin_types:
+fdecl.write(generate_type_cleanup_decl(typename + "List"))
+fdecl.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
+
+# ...this doesn't work for cases where we link in multiple objects that
+# have the functions defined, so we use -b option to provide control
+# over these cases
+if do_builtins:
+fdef.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DEF"))
+for typename in builtin_types:
+fdef.write(generate_type_cleanup(typename + "List"))
+fdef.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DEF"))
+
 for expr in exprs:
 ret = "\n"
 if expr.has_key('type'):
diff --git a/scripts/qapi.py b/scripts/qapi.py
index afc5f32..02ad668 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -11,6 +11,12 @@
 
 from ordereddict import OrderedDict
 
+builtin_types = [
+'str', 'int', 'number', 'bool',
+'int8', 'int16', 'int32', 'int64',
+'uint8', 'uint16', 'uint32', 'uint64'
+]
+
 def tokenize(data):
 while len(data):
 ch = data[0]
@@ -242,3 +248,20 @@ def guardname(filename):
 for substr in [".", " ", "-"]:
 guard = guard.replace(substr, "_")
 return guard.upper() + '_H'
+
+def guardstart(name):
+return mcgen('''
+
+#ifndef %(name)s
+#define %(name)s
+
+''',
+ name=guardname(name))
+
+def guardend(name):
+return mcgen('''
+
+#endif /* %(name)s */
+
+''',
+ name=guardname(name))
-- 
1.7.9.5

[Qemu-devel] [PATCH v3 00/11] qapi: add support for lists of native types

[Michael Roth]

These patches apply on top of qemu.git master, and can also be obtained from:
git://github.com/mdroth/qemu.git qapi-native-lists

Sending this now since a number of series have popped up in the past that
wanted this, and Amos has some pending patches (query-mac-tables) that rely
on this as well.

These patches add support for specifying lists of native qapi types
(int/bool/str/number/int8/uint8/etc) like so:

  { 'type': 'foo',
'data': { 'bar': ['int'] }}

for a 'bar' field that is a list of type 'int',

  { 'type': 'foo2',
'data': { 'bar2': ['str'] }}

for a 'bar2' field that is a list of type 'str', and so on.

This uses linked list types for the native C representations, just as we do
for complex schema-defined types. In the future we may add schema annotations
of some sort to specify a more natural/efficient array type for the C
representations, but this should serve the majority of uses-cases for now.

v2->v3:
 * added native list support for fixed-width integer types (Amos)
 * added fixed-width integer list coverage to serialization,
   QMPOutputVisitor, and QmpInputVisitor unit tests
 * added unit test to QmpInputVisitor to test new overflow handling when
   integers types are expected (Eric)
 * clarified terminology/documentation in json parser fix (Laszlo)
 * fixed whitespace/newlines in code generator output (Luiz)

v1->v2:
 * fixed do-nothing float tests in pre-existing code and updated new
   unit tests accordingly (Laszlo)
 * added a fix for a bug in json parser that was exposed by above change
 * fixed misuse of string format parameters for float testing (Laszlo)
 * fixed extra characters in comment (Laszlo)
 * removed unused variant from UserDefNativeListUnion QAPI type

 Makefile   |6 +-
 qapi-schema-test.json  |   15 ++
 qobject/json-parser.c  |   26 +-
 scripts/qapi-types.py  |   45 +++-
 scripts/qapi-visit.py  |   36 ++-
 scripts/qapi.py|   23 ++
 tests/test-qmp-input-visitor.c |  358 ++
 tests/test-qmp-output-visitor.c|  332 
 tests/test-visitor-serialization.c |  485 +---
 9 files changed, 1274 insertions(+), 52 deletions(-)

Re: [Qemu-devel] [PATCH 04/10] qapi: enable generation of native list code

[mdroth]

On Fri, May 10, 2013 at 11:32:48AM -0500, mdroth wrote:
> On Fri, May 10, 2013 at 10:10:03AM -0400, Luiz Capitulino wrote:
> > On Thu,  9 May 2013 21:20:56 -0500
> > Michael Roth  wrote:
> > 
> > > Also, fix a dependency issue with libqemuutil: qemu-sockets.c needs
> > > qapi-types.c/qapi-visit.c
> > > 
> > > Signed-off-by: Michael Roth 
> > > ---
> > >  Makefile |6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index 7dc0204..9695c9d 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -178,7 +178,7 @@ Makefile: $(version-obj-y) $(version-lobj-y)
> > >  # Build libraries
> > >  
> > >  libqemustub.a: $(stub-obj-y)
> > > -libqemuutil.a: $(util-obj-y)
> > > +libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o
> > 
> > Don't we want this in for 1.5?
> > 
> 
> I don't think it's causing any issues currently since it's not causing
> undefined reference errors upstream. users of libqemuutil that make use
> of qemu-sockets seem to be pulling qapi-types/qapi-visit in through other
> dependencies.
> 
> I only noticed it because I was attempting to generate the native list
> code via tests/Makefile and running into redefinition conflicts with
> qapi-types.o/qapi-visit.o, then noticed the qemu-sockets.o issue when I
> attempted to remove the qapi-types/qapi-visit dependency from
> tests/test-visitor-serialization
> 
> Now that we're generating the native list code from top-level Makefile,
> it actually doesn't seem to be needed by this series anymore, so maybe
> I'll pull it out for now. I think a better fix would be to have
> qapi/Makefile.obj add these to $util-obj-y directly anyway.

^ this wasn't quite right, we do have a new dependency on
qapi-types/qapi-visit in the visitor tests for native list types, so
I've left this in place.

> 
> > >  
> > >  ##
> > >  
> > > @@ -215,10 +215,10 @@ $(SRC_PATH)/qga/qapi-schema.json 
> > > $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
> > >  
> > >  qapi-types.c qapi-types.h :\
> > >  $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> > > - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py 
> > > $(gen-out-type) -o "." < $<, "  GEN   $@")
> > > + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py 
> > > $(gen-out-type) -o "." -b < $<, "  GEN   $@")
> > >  qapi-visit.c qapi-visit.h :\
> > >  $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
> > > - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py 
> > > $(gen-out-type) -o "."  < $<, "  GEN   $@")
> > > + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py 
> > > $(gen-out-type) -o "." -b < $<, "  GEN   $@")
> > >  qmp-commands.h qmp-marshal.c :\
> > >  $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py 
> > > $(qapi-py)
> > >   $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py 
> > > $(gen-out-type) -m -o "." < $<, "  GEN   $@")
> > 

Re: [Qemu-devel] [PATCH for-1.5] qom: optimize casting to leaf class and parent class

[Aurelien Jarno]

On Fri, May 10, 2013 at 01:47:55PM -0500, Anthony Liguori wrote:
> Most QOM types use type_register_static but we still strdup the
> passed data.  However, the original pointers are useful because
> GCC is pretty good about collapsing strings so its very likely any
> use of the pointer will end up being that same address.
> 
> IOW, with a little trickery, we can compare types by just comparing
> strings and in fact that's what we do here.
> 
> We do this for the two most common cases, casting to a leaf class
> or to the parent class.
> 
> With these two changes, I see a decrease from around 2 hash table
> lookups to only a thousand with no run time lookups at all.
> 
> Cc: Paolo Bonzini 
> Cc: Aurelien Jarno 
> Cc: Andreas Färber 
> Reported-by: Aurelien Jarno 
> Signed-off-by: Anthony Liguori 
> ---
> Aurelien, could you please try this patch with your PPC test case?
> ---
>  qom/object.c | 16 ++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index 75e6aac..5ecfd28 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -132,7 +132,13 @@ TypeImpl *type_register(const TypeInfo *info)
>  
>  TypeImpl *type_register_static(const TypeInfo *info)
>  {
> -return type_register(info);
> +TypeImpl *impl;
> +
> +impl = type_register(info);
> +impl->name = info->name;
> +impl->parent = info->parent;
> +
> +return impl;
>  }
>  
>  static TypeImpl *type_get_by_name(const char *name)
> @@ -449,10 +455,16 @@ Object *object_dynamic_cast_assert(Object *obj, const 
> char *typename)
>  ObjectClass *object_class_dynamic_cast(ObjectClass *class,
> const char *typename)
>  {
> -TypeImpl *target_type = type_get_by_name(typename);
> +TypeImpl *target_type;
>  TypeImpl *type = class->type;
>  ObjectClass *ret = NULL;
>  
> +if (type->name == typename || type->parent == typename) {
> +return class;
> +}
> +
> +target_type = type_get_by_name(typename);
> +
>  if (!target_type) {
>  /* target class type unknown, so fail the cast */
>  return NULL;

Unfortunately it doesn't fix the problem. I only see a 0.5% improvement,
which might be in the noise. I still see g_hash_table_lookup and
g_str_hash quite high in perf top.

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

[Qemu-devel] [PATCH for-1.5] isapc: Fix non-KVM qemu boot (read/write memory for isapc BIOS)

[Jordan Justen]

The isapc machine with seabios currently requires the BIOS region
to be read/write memory rather than read-only memory.

KVM currently cannot support the BIOS as a ROM region, but qemu
in non-KVM mode can. Based on this, isapc machine currently only
works with KVM.

To work-around this isapc issue, this change avoids marking the
BIOS as readonly for isapc.

This change also will allow KVM to start supporting ROM mode
via KVM_CAP_READONLY_MEM.

Signed-off-by: Jordan Justen 
Reviewed-by: Paolo Bonzini 
---
 hw/block/pc_sysfw.c |   14 ++
 hw/i386/pc_piix.c   |5 +
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/hw/block/pc_sysfw.c b/hw/block/pc_sysfw.c
index aad8614..90894af 100644
--- a/hw/block/pc_sysfw.c
+++ b/hw/block/pc_sysfw.c
@@ -39,6 +39,7 @@
 typedef struct PcSysFwDevice {
 SysBusDevice busdev;
 uint8_t rom_only;
+uint8_t isapc_ram_fw;
 } PcSysFwDevice;
 
 static void pc_isa_bios_init(MemoryRegion *rom_memory,
@@ -139,7 +140,7 @@ static void pc_system_flash_init(MemoryRegion *rom_memory,
 pc_isa_bios_init(rom_memory, flash_mem, size);
 }
 
-static void old_pc_system_rom_init(MemoryRegion *rom_memory)
+static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
 {
 char *filename;
 MemoryRegion *bios, *isa_bios;
@@ -163,7 +164,9 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
 bios = g_malloc(sizeof(*bios));
 memory_region_init_ram(bios, "pc.bios", bios_size);
 vmstate_register_ram_global(bios);
-memory_region_set_readonly(bios, true);
+if (!isapc_ram_fw) {
+memory_region_set_readonly(bios, true);
+}
 ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
 if (ret != 0) {
 bios_error:
@@ -186,7 +189,9 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
 0x10 - isa_bios_size,
 isa_bios,
 1);
-memory_region_set_readonly(isa_bios, true);
+if (!isapc_ram_fw) {
+memory_region_set_readonly(isa_bios, true);
+}
 
 /* map all the bios at the top of memory */
 memory_region_add_subregion(rom_memory,
@@ -216,7 +221,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
 qdev_init_nofail(DEVICE(sysfw_dev));
 
 if (sysfw_dev->rom_only) {
-old_pc_system_rom_init(rom_memory);
+old_pc_system_rom_init(rom_memory, sysfw_dev->isapc_ram_fw);
 return;
 }
 
@@ -255,6 +260,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
 }
 
 static Property pcsysfw_properties[] = {
+DEFINE_PROP_UINT8("isapc_ram_fw", PcSysFwDevice, isapc_ram_fw, 0),
 DEFINE_PROP_UINT8("rom_only", PcSysFwDevice, rom_only, 1),
 DEFINE_PROP_END_OF_LIST(),
 };
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index f7c80ad..c1a49ec 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -716,6 +716,11 @@ static QEMUMachine isapc_machine = {
 .property = "rom_only",
 .value= stringify(1),
 },
+{
+.driver   = "pc-sysfw",
+.property = "isapc_ram_fw",
+.value= stringify(1),
+},
 { /* end of list */ }
 },
 DEFAULT_MACHINE_OPTIONS,
-- 
1.7.10.4

Re: [Qemu-devel] VFIO VGA test branches

[Alex Williamson]

On Fri, 2013-05-10 at 14:31 -0700, Justin Gottula wrote:
> Hi,
> 
> The kernel won't compile with CONFIG_HOTPLUG_PCI=m:
> 
> drivers/pci/hotplug/pci_hotplug_core.c:548:5: error: redefinition of
> ‘pci_hp_reset_slot’
>  int pci_hp_reset_slot(struct hotplug_slot *hotplug, int probe)
>  ^
> In file included from drivers/pci/hotplug/pci_hotplug_core.c:41:0:
> include/linux/pci_hotplug.h:141:19: note: previous definition of
> ‘pci_hp_reset_slot’ was here
>  static inline int pci_hp_reset_slot(struct hotplug_slot *slot, int probe)
>^
> make[3]: *** [drivers/pci/hotplug/pci_hotplug_core.o] Error 1

Oops, thanks.  Hopefully you can use =y or off for now.  These trees
aren't yet ready for upstream.  Thanks,

Alex

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Paolo Bonzini]

Il 10/05/2013 19:41, Anthony Liguori ha scritto:
> Paolo Bonzini  writes:
> 
>> Il 10/05/2013 16:39, Anthony Liguori ha scritto:
>>> I just oppose the notion of disabling casts and *especially* only
>>> disabling casts for official builds.
>>
>> This actually happens all the time.  Exactly this kind of type-safe cast
>> is disabled in releases of GCC, but enabled when building from svn trunk.
> 
> Let's assume for a moment that you are right and this behavior is what
> we should have.  Let's also assume there is a real regression here
> which has yet to have been established.

Aurelien timed the effect of my patch two hours before you sent this
message.  If it's not a regression in 1.5 (which is quite obvious from
the profile), it is a regression from the introduction of CPU classes
(1.3 or 1.4), when this code didn't exist at all.

And in 1.5 we introduced virtio-net casts as well (or did mst sneak in
his change anyway? ;)). If 10% is the effect of a few hundred
interrupts/sec, perhaps the same effect is visible on a few thousand
packets/sec.  I wouldn't bet against that one week from release.

(In fact, I'm not going to bet against that after release, either.  I'll
propose to disable QOM cast checking in Fedora and RHEL).

> As soon as we open up 1.6 for development, we face an immediate
> regression in performance.  So we need to fix the real problem here
> anyway.

No, we don't.  We will simply have to live with a debugging vs.
production tradeoff.  You will need to remember using the appropriate
option when doing performance tests on development releases.  We can
print a message if necessary, but it's really common practice.  What
about lockdep or might_sleep debugging or similar checks that Fedora
only enables for the kernel during development phases?

> I strongly suspect that if there is a problem, that optimizing
> leaf/concrete casts will take care of it.  Otherwise, a small lookup
> cache ought to do the trick too.  We can even use pointer comparisons
> for the lookup cache which should make it extremely cheap.

Still more expensive than no code at all.

> Let's independently evaluate your proposal for 1.6.

No, my proposal has to be evaluated for 1.5, and perhaps reverted later.
 We have a potentially large *set* of regressions (large amounts of QOM
casts were introduced in 1.5) that we found after -rc1, and the big
hammer is the only way to deal with it.

> Of course, these changes never make it into the tree which is an
> indication that the mechanism works very well :-)

Yes, it works *great*.  I don't want to give the impression that I think
the opposite.  But how often have you seen them abort in the distro
QEMU, as opposed to a development version?  And how often have you seen
"CRITICAL: foo != NULL" or "CRITICAL: GTK_IS_WIDGET (foo)" from a GTK+
app?  For me, it is never vs. quite often.

It works great simply because it's very cheap to add and makes debugging
much nicer.

> Note that the cast macros are a big improvement in code readability.
> The only real replacement would be static casts which would downgrade
> safety.

Yes, we should keep the cast macros, no doubt about that.  And the
checks, for development.  But adding tracing, while removing the actual
checks, is a pretty good speed compromise IMHO.  And leaving the checks
in before the freeze matches what most developers probably desire, and
avoids bitrotting of the check code.

> If you want to debate the merits of the safety, that's fine.
> 
>> Type-safe casts make sense in GTK+/GObject where: 1) type-safe casts
>> return NULL and log a "critical" error, they do not abort;  2) all
>> functions fail with another "critical" error if they are passed NULL.
>> We do neither, so we're just trading a crash now for a crash very soon
>> after (our call stacks tend to be relatively shallow, with some
>> exceptions such as the block layer).
> 
> The big assumption here is that dereference a NULL results in
> predictable failure.  This is not always the case and can lead to
> security vulnerabilities.

That's not what I was talking about.  I was talking about code like this:

void
gtk_window_set_wmclass (GtkWindow *window,
const gchar *wmclass_name,
const gchar *wmclass_class)
{
  GtkWindowPrivate *priv;

  g_return_if_fail (GTK_IS_WINDOW (window));
  priv = window->priv;

  ...
}

that avoids NULL pointer dereferences before they happen, while still
doing the checks.

Anyway, yes: the checks can catch some security vulnerabilities.  But
they won't catch many uses-after-free, they won't catch wrong
refcounting, they won't catch the _actual_ vulnerabilities that we had,
nor probably the ones that we could have.  Every segfault we fix could
potentially become a guest->host exploit with enough skill, the guest
has control of an enormous part of the QEMU address space.  But we fix
segfaults, not QOM cast failures.

Before QOM casts, I don't remember seeing many such failures _in the
tree_.  They

Re: [Qemu-devel] [PATCH] w32: Fix build with older gcc (unresolved symbol)

[Igor Mitsyanko]

On 29.03.2013 21:20, Stefan Weil wrote:

The cross i586-mingw32msvc-gcc 4.4.4 from Debian Squeeze does not support
__sync_val_compare_and_swap by default.

Using -march=i686 fixes that and should also result in better code.

Signed-off-by: Stefan Weil 
---

Maybe this modification is also needed for native gcc-4.4 and older
on Linux i386. If yes, we can move the new script code out of the
MinGW conditional code.

Newer versions of gcc obviously use -march=i686 by default and
don't need the patch, but it also won't do any harm for those
versions.

Stefan



mingw is built with --build=mingw32 and looks like it defaults to 
-march=i386 (I have gcc version 4.7.2).

Default build on windows is broken without this patch, it should be
applied to 1.5 probably.

Tested-by: Igor Mitsyanko 



  configure |5 +
  1 file changed, 5 insertions(+)

diff --git a/configure b/configure
index f2af714..70c2219 100755
--- a/configure
+++ b/configure
@@ -562,6 +562,11 @@ if test "$mingw32" = "yes" ; then
QEMU_CFLAGS="-DWIN32_LEAN_AND_MEAN -DWINVER=0x501 $QEMU_CFLAGS"
# enable C99/POSIX format strings (needs mingw32-runtime 3.15 or later)
QEMU_CFLAGS="-D__USE_MINGW_ANSI_STDIO=1 $QEMU_CFLAGS"
+  if test "$cpu" = "i386"; then
+# We need something better than i386 for __sync_val_compare_and_swap
+# and can expect that QEMU will only run on i686 or later.
+QEMU_CFLAGS="-march=i686 $QEMU_CFLAGS"
+  fi
LIBS="-lwinmm -lws2_32 -liphlpapi $LIBS"
  cat > $TMPC << EOF
  int main(void) { return 0; }

Re: [Qemu-devel] [PATCH v2 0/2] qga umask fix addenda

[mdroth]

On Fri, May 10, 2013 at 09:53:27PM +0200, Laszlo Ersek wrote:
> On 05/10/13 21:30, mdroth wrote:
> > On Wed, May 08, 2013 at 05:31:34PM +0200, Laszlo Ersek wrote:
> >> I should have paid more attention to portability and error path cleanup
> >> in the CVE-2013-2007 fix.
> >>
> >> (We continue to assume, like the rest of qemu code, that
> >> qemu_set_cloexec() never fails internally. This should be a reasonable
> >> assumption when the input fd is valid.)
> >>
> >> Laszlo Ersek (2):
> >>   qga: distinguish binary modes in "guest_file_open_modes" map
> >>   qga: unlink just created guest-file if fchmod() or fdopen() fails on
> >> it
> > 
> > Thanks, applied to qga branch:
> > 
> > https://github.com/mdroth/qemu/commits/qga
> 
> Thanks!
> 
> Can you reword the second commit to include Eric's R-b?
> 

Sure, missed that one. Should be fixed in tree now.

> 
> Thanks!
> Laszlo
> 

[Qemu-devel] [PATCH V2] osdep.h: include sys/types.h for ssize_t definition

[Igor Mitsyanko]

sys/types.h is taken out from "ifdef __OpenBSD__" guard. It should be
safe for other systems, according to following survey:
http://hacks.owlfolio.org/header-survey/

This fixes build for CONFIG_IOVEC-less systems (mingw).

Signed-off-by: Igor Mitsyanko 
---
 include/qemu/osdep.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 42545bc..3bcd4ab 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -5,8 +5,8 @@
 #include 
 #include 
 #include 
-#ifdef __OpenBSD__
 #include 
+#ifdef __OpenBSD__
 #include 
 #endif
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 0/2] qga umask fix addenda

[Laszlo Ersek]

On 05/10/13 21:30, mdroth wrote:
> On Wed, May 08, 2013 at 05:31:34PM +0200, Laszlo Ersek wrote:
>> I should have paid more attention to portability and error path cleanup
>> in the CVE-2013-2007 fix.
>>
>> (We continue to assume, like the rest of qemu code, that
>> qemu_set_cloexec() never fails internally. This should be a reasonable
>> assumption when the input fd is valid.)
>>
>> Laszlo Ersek (2):
>>   qga: distinguish binary modes in "guest_file_open_modes" map
>>   qga: unlink just created guest-file if fchmod() or fdopen() fails on
>> it
> 
> Thanks, applied to qga branch:
> 
> https://github.com/mdroth/qemu/commits/qga

Thanks!

Can you reword the second commit to include Eric's R-b?


Thanks!
Laszlo

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Anthony Liguori]

Aurelien Jarno  writes:

> On Fri, May 10, 2013 at 12:41:07PM -0500, Anthony Liguori wrote:
>> Paolo Bonzini  writes:
>> 
>> > Il 10/05/2013 16:39, Anthony Liguori ha scritto:
>> >> I just oppose the notion of disabling casts and *especially* only
>> >> disabling casts for official builds.
>> >
>> > This actually happens all the time.  Exactly this kind of type-safe cast
>> > is disabled in releases of GCC, but enabled when building from svn trunk.
>> 
>> Let's assume for a moment that you are right and this behavior is what
>> we should have.  Let's also assume there is a real regression here
>> which has yet to have been established.
>
> I have pointed out in another mail of this thread, that this type
> checking account for about 9% of slowdown. Isn't that enough to say it's
> a regression?

Yes, it's a regression.  I hadn't seen your note when I wrote the above.

I believe http://article.gmane.org/gmane.comp.emulators.qemu/210976
fixes the problem without eliminating any checking.

> I still have to compare to 1.4, but it's not very fair, as a
> significant work has been but on TCG to improve the performances,
> especially on the target-ppc code.
>
> I am currently preparing an image so that people can test that
> themselves.

I can reproduce large numbers of casts FWIW so no need to on my end.
I've confirmed the above patch fixes it.

>> Disabling casts doesn't give us a long term fix.  One of the solutions
>> above does and patches also exist.
>
> Why do you insist on using dynamic cast, on a hot path?

It's a question of safety.

> It was using a
> simple pointer before QOM in 1.4, it won't be less safe with if we use
> a static cast instead of a dynamic one in 1.5.

The patch I posted should make it equally fast.

> With your arguments, we should drop all dataplane code, as it doesn't
> use the QEMU core code. People should just not complain about the
> performance.

No one is complaining about performance.

I merely stated that if performance is a problem, give us a chance to
fix it before completely disabling a feature.

And indeed, now I have a fix that doesn't disable anything.

Regards,

Anthony Liguori

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

Re: [Qemu-devel] [PATCH v2 0/2] qga umask fix addenda

[mdroth]

On Wed, May 08, 2013 at 05:31:34PM +0200, Laszlo Ersek wrote:
> I should have paid more attention to portability and error path cleanup
> in the CVE-2013-2007 fix.
> 
> (We continue to assume, like the rest of qemu code, that
> qemu_set_cloexec() never fails internally. This should be a reasonable
> assumption when the input fd is valid.)
> 
> Laszlo Ersek (2):
>   qga: distinguish binary modes in "guest_file_open_modes" map
>   qga: unlink just created guest-file if fchmod() or fdopen() fails on
> it

Thanks, applied to qga branch:

https://github.com/mdroth/qemu/commits/qga

> 
>  qga/commands-posix.c |   25 +++--
>  1 files changed, 19 insertions(+), 6 deletions(-)
> 

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Aurelien Jarno]

On Fri, May 10, 2013 at 12:41:07PM -0500, Anthony Liguori wrote:
> Paolo Bonzini  writes:
> 
> > Il 10/05/2013 16:39, Anthony Liguori ha scritto:
> >> I just oppose the notion of disabling casts and *especially* only
> >> disabling casts for official builds.
> >
> > This actually happens all the time.  Exactly this kind of type-safe cast
> > is disabled in releases of GCC, but enabled when building from svn trunk.
> 
> Let's assume for a moment that you are right and this behavior is what
> we should have.  Let's also assume there is a real regression here
> which has yet to have been established.

I have pointed out in another mail of this thread, that this type
checking account for about 9% of slowdown. Isn't that enough to say it's
a regression?

I still have to compare to 1.4, but it's not very fair, as a
significant work has been but on TCG to improve the performances,
especially on the target-ppc code.

I am currently preparing an image so that people can test that
themselves.

> As soon as we open up 1.6 for development, we face an immediate
> regression in performance.  So we need to fix the real problem here
> anyway.
> 
> I strongly suspect that if there is a problem, that optimizing
> leaf/concrete casts will take care of it.  Otherwise, a small lookup
> cache ought to do the trick too.  We can even use pointer comparisons
> for the lookup cache which should make it extremely cheap.
>
>
> Disabling casts doesn't give us a long term fix.  One of the solutions
> above does and patches also exist.

Why do you insist on using dynamic cast, on a hot path? It was using a
simple pointer before QOM in 1.4, it won't be less safe with if we use
a static cast instead of a dynamic one in 1.5.

With your arguments, we should drop all dataplane code, as it doesn't
use the QEMU core code. People should just not complain about the
performance.

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

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Anthony Liguori]

Aurelien Jarno  writes:

> On Fri, May 10, 2013 at 02:16:34PM +0200, Paolo Bonzini wrote:
>> Cast debugging can have a substantial cost (20% or more, measured by
>> Aurelien on qemu-system-ppc64).  Instead of adding special-cased "fast
>> casts" in the hot paths, we can just disable it in releases.  At the
>> same time, add tracing facilities that simplify the analysys of those
>> problems that cast debugging would reveal.
>> 
>> At least patches 1-7 are for 1.5.
>> 
>> Paolo Bonzini (9):
>>   qom: improve documentation of cast functions
>>   qom: allow casting of a NULL class
>>   qom: add a fast path to object_class_dynamic_cast
>>   qom: pass file/line/function to asserting casts
>>   qom: trace asserting casts
>>   qom: allow turning cast debugging off
>>   build: disable QOM cast debugging for official releases
>>   qom: simplify object_class_dynamic_cast, part 1
>>   qom: simplify object_class_dynamic_cast, part 2
>> 
>>  configure| 20 --
>>  include/qom/object.h | 40 ++-
>>  qom/object.c | 77 
>> ++--
>>  trace-events |  3 ++
>>  4 files changed, 99 insertions(+), 41 deletions(-)
>> 
>
> I have tested this series with qemu-system-ppc64, on a Core i7 2600 CPU.
> The process was set to a single core using taskset. Inside the guest
> (Debian ppc64 from debian-ports), I ran the command three times:
>
>   lintian g++-4.8_4.8.0-6_ppc64.deb
>
> I used lintian as it's a perl code, that trigger the discussion about
> sparc/ppc comparison.
>
> First of all with this patch series, the object_class_dynamic_cast calls
> went down to below 0.1% when using perf top. Before the patch series was
> applied, the command took in average on 3 runs 142.4s. With the patch
> series, it went down to 129.8s, so almost 9% faster.

I just posted another patch which I believe will also reduce this
overhead without eliminating the checks.

We do a staggering number of casts...  The patch I posted makes the
overwhelming majority of them nothing more than a single pointer
comparison and a couple derefs.

Regards,

Anthony Liguori

>
> To improve the performance a bit more, and come back to the same kind of
> code as before, we should move simple accessors from qom/*.c to
> include/qom/*.h and mark them as inline, so that they can be removed by
> the compiler. Currently, even if the function is simple it's still a
> call/ret in the hot path instead of a simple pointer addition.

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

[Qemu-devel] [PATCH for-1.5] qom: optimize casting to leaf class and parent class

[Anthony Liguori]

Most QOM types use type_register_static but we still strdup the
passed data.  However, the original pointers are useful because
GCC is pretty good about collapsing strings so its very likely any
use of the pointer will end up being that same address.

IOW, with a little trickery, we can compare types by just comparing
strings and in fact that's what we do here.

We do this for the two most common cases, casting to a leaf class
or to the parent class.

With these two changes, I see a decrease from around 2 hash table
lookups to only a thousand with no run time lookups at all.

Cc: Paolo Bonzini 
Cc: Aurelien Jarno 
Cc: Andreas Färber 
Reported-by: Aurelien Jarno 
Signed-off-by: Anthony Liguori 
---
Aurelien, could you please try this patch with your PPC test case?
---
 qom/object.c | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 75e6aac..5ecfd28 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -132,7 +132,13 @@ TypeImpl *type_register(const TypeInfo *info)
 
 TypeImpl *type_register_static(const TypeInfo *info)
 {
-return type_register(info);
+TypeImpl *impl;
+
+impl = type_register(info);
+impl->name = info->name;
+impl->parent = info->parent;
+
+return impl;
 }
 
 static TypeImpl *type_get_by_name(const char *name)
@@ -449,10 +455,16 @@ Object *object_dynamic_cast_assert(Object *obj, const 
char *typename)
 ObjectClass *object_class_dynamic_cast(ObjectClass *class,
const char *typename)
 {
-TypeImpl *target_type = type_get_by_name(typename);
+TypeImpl *target_type;
 TypeImpl *type = class->type;
 ObjectClass *ret = NULL;
 
+if (type->name == typename || type->parent == typename) {
+return class;
+}
+
+target_type = type_get_by_name(typename);
+
 if (!target_type) {
 /* target class type unknown, so fail the cast */
 return NULL;
-- 
1.8.0

[Qemu-devel] [PATCH] 9p: Be robust against paths without FS_IOC_GETVERSION

[Gabriel de Perthuis]

9P optionally uses the FS_IOC_GETVERSION ioctl to get information about
a file's version (sometimes called generation number).

The code checks for supported filesystems at mount time, but some paths
may come from other mounted filesystems.

Change it to treat unsupported paths the same as unsupported
filesystems, returning 0 in both cases.

Note: ENOTTY is the error code for an unsupported ioctl.

This fix allows booting a linux kernel with the same / filesystem as the
host; otherwise the boot fails when mounting devtmpfs.

Signed-off-by: Gabriel de Perthuis 
Reviewed-by: Aneesh Kumar K.V 
---

Here it is with an expanded commit message.

 hw/9pfs/cofile.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c
index 2efebf3..194c130 100644
--- a/hw/9pfs/cofile.c
+++ b/hw/9pfs/cofile.c
@@ -36,10 +36,14 @@ int v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t 
st_mode,
 err = -errno;
 }
 });
 v9fs_path_unlock(s);
 }
+/* The ioctl may not be supported depending on the path */
+if (err == -ENOTTY) {
+err = 0;
+}
 return err;
 }
 
 int v9fs_co_lstat(V9fsPDU *pdu, V9fsPath *path, struct stat *stbuf)
 {
-- 
1.8.2.1.419.ga0b97c6

Re: [Qemu-devel] [PATCH for-1.5 8/9] qom: simplify object_class_dynamic_cast, part 1

[Anthony Liguori]

Paolo Bonzini  writes:

> Access everything from the class.
>
> Signed-off-by: Paolo Bonzini 
> ---
>  qom/object.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index f5f416b..f82f12c 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -457,15 +457,13 @@ ObjectClass *object_class_dynamic_cast(ObjectClass 
> *class,
>  {
>  ObjectClass *ret = NULL;
>  TypeImpl *target_type;
> -TypeImpl *type;
>  
>  if (!class) {
>  return NULL;
>  }
>  
>  /* A simple fast path that can trigger a lot for leaf classes.  */
> -type = class->type;
> -if (type->name == typename) {
> +if (class->type->name == typename) {

This won't trigger because we unconditionally strdup().  Can you add
this the bit from my patch that sets ->name appropriately?

Aurelien, can you try the resulting series with
--{enable,disable}-qom-casts?  My suspicion is that this is the primary
source of speed up.

If you can make an image available too, I can try this myself.

Regards,

Anthony Liguori

>  return class;
>  }
>  
> @@ -475,7 +473,7 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
>  return NULL;
>  }
>  
> -if (type->class->interfaces &&
> +if (class->interfaces &&
>  type_is_ancestor(target_type, type_interface)) {
>  int found = 0;
>  GSList *i;
> @@ -493,7 +491,7 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
>  if (found > 1) {
>  ret = NULL;
>  }
> -} else if (type_is_ancestor(type, target_type)) {
> +} else if (type_is_ancestor(class->type, target_type)) {
>  ret = class;
>  }
>  
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Anthony Liguori]

Paolo Bonzini  writes:

> Il 10/05/2013 16:39, Anthony Liguori ha scritto:
>> I just oppose the notion of disabling casts and *especially* only
>> disabling casts for official builds.
>
> This actually happens all the time.  Exactly this kind of type-safe cast
> is disabled in releases of GCC, but enabled when building from svn trunk.

Let's assume for a moment that you are right and this behavior is what
we should have.  Let's also assume there is a real regression here
which has yet to have been established.

As soon as we open up 1.6 for development, we face an immediate
regression in performance.  So we need to fix the real problem here
anyway.

I strongly suspect that if there is a problem, that optimizing
leaf/concrete casts will take care of it.  Otherwise, a small lookup
cache ought to do the trick too.  We can even use pointer comparisons
for the lookup cache which should make it extremely cheap.

Disabling casts doesn't give us a long term fix.  One of the solutions
above does and patches also exist.

So first, let's establish if there really is a performance issue here
and if so, let's find a long term solution.

Let's independently evaluate your proposal for 1.6.  You may in fact be
right that it's the right thing to do long term, but I'm quite confident
that it's not the right solution to the potential issue here.

> I have hardly seen any of these failures _during development_, much less
> on a release.

It's the most common failure that I catch in my own testing.  Most often
around hotplug which tends to break the most.

Of course, these changes never make it into the tree which is an
indication that the mechanism works very well :-)

>  I appreciate the advantage of type-safe casts, but in
> QEMU they are a solution in search of a problem.  They are cheap to
> implement (though not that cheap to execute ;)) so it's perfectly fine
> to have them, but they are not _needed_; disabling them is anyway a good
> build-time option to have.

Note that the cast macros are a big improvement in code readability.
The only real replacement would be static casts which would downgrade
safety.

If you want to debate the merits of the safety, that's fine.

> Type-safe casts make sense in GTK+/GObject where: 1) type-safe casts
> return NULL and log a "critical" error, they do not abort;  2) all
> functions fail with another "critical" error if they are passed NULL.
> We do neither, so we're just trading a crash now for a crash very soon
> after (our call stacks tend to be relatively shallow, with some
> exceptions such as the block layer).

The big assumption here is that dereference a NULL results in
predictable failure.  This is not always the case and can lead to
security vulnerabilities.

> Also, in GTK+/GObject the code paths are unpredictable because they
> depend on user interaction, and a crash can lead to data loss.  By
> contrast, in QEMU most of the code is hardly ever run, and the possible
> paths are very few because driver writers tend to use always the same
> path.  The day someone is bringing up a new guest OS and encounters such
> a crash, we'll tell them to either build from git, or to use
> --enable-qom-cast-debug.
>
> I'm sure it will be a long time before that happens...

If you are concerned about the performance, provide a concrete example
of poor performance and I will fix it.

If we find one that is unfixable, then we should consider your
proposal.

Regards,

Anthony Liguori

>
> Paolo
>
>> Regards,
>> 
>> Anthony Liguori
>> 
 Either way, it would be nice to see the call sites of those
 most-impacting dynamic casts! So far I held back my APIC RFC since I'm
 not sure how to reproducibly profile things.
>>>
>>> It's interrupts (both sending and returning from them).
>>>
>>> Paolo
>> 
>> 
>> 

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Paolo Bonzini]

Il 10/05/2013 18:18, Andreas Färber ha scritto:
> Am 10.05.2013 17:56, schrieb Aurelien Jarno:
>> To improve the performance a bit more, and come back to the same kind of
>> code as before, we should move simple accessors from qom/*.c to
>> include/qom/*.h and mark them as inline, so that they can be removed by
>> the compiler. Currently, even if the function is simple it's still a
>> call/ret in the hot path instead of a simple pointer addition.
> 
> Could you be more concrete? cpu_reset() and the ELF write functions are
> the only such things that comes to my mind from qom/cpu.c, and they will
> not be called regularly during your Perl execution. You mean something
> from qom/object.c?

Yes, the _assert versions of the casts could be made inline.

Paolo

Re: [Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Andreas Färber]

Am 10.05.2013 18:20, schrieb Peter Maydell:
> On 10 May 2013 17:14, Andreas Färber  wrote:
>> Personally I wouldn't oppose dropping these checks for release builds as
>> proposed by Paolo in his series; for me, the value of POWERPC_CPU() is
>> being closer to an OO cast than any container_of()-style expressions.
>>
>> But I can also see Anthony's point that we should try to optimize
>> dynamic_cast rather than circumventing it.
> 
> I don't think we should be doing anything dynamically at all.
> We know at compile time that we've been passed a CPUPPCState*,
> and we know that we always get from that to a CPUState*
> by subtracting a compile-time-constant offset. Nothing about
> this is dynamic at all and anything we do at runtime beyond
> that subtraction is pure overhead.

No one doubts that for CPU.

But if you think of AXI, I2C and the general connecter inheritance vs.
aggregation discussion, it becomes much more hairy! In particular
Anthony's solution to the I/O port VGA vs. ISA problem was dropping
ISADevice as base type and turning ISA into an interface on a chipset
object. Then you may know which interfaces are available on your device,
but you still need some special non-C cast, aka dynamic_cast.

So in the end we want C++ (replace with favorite native OO language) but
we can't switch because not all devices are QOM'ified yet, and when
QOM'ifying them we get complaints about bad QOM performance.
Either we ignore performance hits and hurry up with the conversion or we
address performance hits to at least some degree, otherwise we're not
going to reach a satisfactory solution...

Andreas

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

Re: [Qemu-devel] [PATCH 04/10] qapi: enable generation of native list code

[mdroth]

On Fri, May 10, 2013 at 10:10:03AM -0400, Luiz Capitulino wrote:
> On Thu,  9 May 2013 21:20:56 -0500
> Michael Roth  wrote:
> 
> > Also, fix a dependency issue with libqemuutil: qemu-sockets.c needs
> > qapi-types.c/qapi-visit.c
> > 
> > Signed-off-by: Michael Roth 
> > ---
> >  Makefile |6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 7dc0204..9695c9d 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -178,7 +178,7 @@ Makefile: $(version-obj-y) $(version-lobj-y)
> >  # Build libraries
> >  
> >  libqemustub.a: $(stub-obj-y)
> > -libqemuutil.a: $(util-obj-y)
> > +libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o
> 
> Don't we want this in for 1.5?
> 

I don't think it's causing any issues currently since it's not causing
undefined reference errors upstream. users of libqemuutil that make use
of qemu-sockets seem to be pulling qapi-types/qapi-visit in through other
dependencies.

I only noticed it because I was attempting to generate the native list
code via tests/Makefile and running into redefinition conflicts with
qapi-types.o/qapi-visit.o, then noticed the qemu-sockets.o issue when I
attempted to remove the qapi-types/qapi-visit dependency from
tests/test-visitor-serialization

Now that we're generating the native list code from top-level Makefile,
it actually doesn't seem to be needed by this series anymore, so maybe
I'll pull it out for now. I think a better fix would be to have
qapi/Makefile.obj add these to $util-obj-y directly anyway.

> >  
> >  ##
> >  
> > @@ -215,10 +215,10 @@ $(SRC_PATH)/qga/qapi-schema.json 
> > $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
> >  
> >  qapi-types.c qapi-types.h :\
> >  $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> > -   $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py 
> > $(gen-out-type) -o "." < $<, "  GEN   $@")
> > +   $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py 
> > $(gen-out-type) -o "." -b < $<, "  GEN   $@")
> >  qapi-visit.c qapi-visit.h :\
> >  $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
> > -   $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py 
> > $(gen-out-type) -o "."  < $<, "  GEN   $@")
> > +   $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py 
> > $(gen-out-type) -o "." -b < $<, "  GEN   $@")
> >  qmp-commands.h qmp-marshal.c :\
> >  $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py 
> > $(qapi-py)
> > $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py 
> > $(gen-out-type) -m -o "." < $<, "  GEN   $@")
> 

Re: [Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Peter Maydell]

On 10 May 2013 17:14, Andreas Färber  wrote:
> Personally I wouldn't oppose dropping these checks for release builds as
> proposed by Paolo in his series; for me, the value of POWERPC_CPU() is
> being closer to an OO cast than any container_of()-style expressions.
>
> But I can also see Anthony's point that we should try to optimize
> dynamic_cast rather than circumventing it.

I don't think we should be doing anything dynamically at all.
We know at compile time that we've been passed a CPUPPCState*,
and we know that we always get from that to a CPUState*
by subtracting a compile-time-constant offset. Nothing about
this is dynamic at all and anything we do at runtime beyond
that subtraction is pure overhead.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Andreas Färber]

Am 10.05.2013 17:56, schrieb Aurelien Jarno:
> To improve the performance a bit more, and come back to the same kind of
> code as before, we should move simple accessors from qom/*.c to
> include/qom/*.h and mark them as inline, so that they can be removed by
> the compiler. Currently, even if the function is simple it's still a
> call/ret in the hot path instead of a simple pointer addition.

Could you be more concrete? cpu_reset() and the ELF write functions are
the only such things that comes to my mind from qom/cpu.c, and they will
not be called regularly during your Perl execution. You mean something
from qom/object.c?

Andreas

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

Re: [Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Andreas Färber]

Am 10.05.2013 17:32, schrieb Alexander Graf:
> 
> On 10.05.2013, at 17:23, Andreas Färber wrote:
> 
>> Am 10.05.2013 17:06, schrieb Anthony Liguori:
>>> Andreas Färber  writes:
>>>
 A transition from CPUPPCState to PowerPCCPU can be considered safe,
 just like PowerPCCPU::env access in the opposite direction.

 This should slightly improve interrupt performance.

 Reported-by: Anthony Liguori 
 Signed-off-by: Andreas Färber 
>>>
>>> Another option would be to leave it and do something like:
>>>
>>> diff --git a/qom/object.c b/qom/object.c
>>> index 75e6aac..cba1d88 100644
>>> --- a/qom/object.c
>>> +++ b/qom/object.c
>>> @@ -126,8 +126,13 @@ static TypeImpl *type_register_internal(const TypeInfo 
>>> *info)
>>>
>>> TypeImpl *type_register(const TypeInfo *info)
>>> {
>>> +TypeImpl *impl;
>>> +
>>> assert(info->parent);
>>> -return type_register_internal(info);
>>> +impl = type_register_internal(info);
>>> +g_free(impl->name);
>>> +impl->name = info->name;
>>> +return impl;
>>> }
>>>
>>> TypeImpl *type_register_static(const TypeInfo *info)
>>> @@ -449,10 +490,16 @@ Object *object_dynamic_cast_assert(Object *obj, const 
>>> char *typename)
>>> ObjectClass *object_class_dynamic_cast(ObjectClass *class,
>>>const char *typename)
>>> {
>>> -TypeImpl *target_type = type_get_by_name(typename);
>>> +TypeImpl *target_type;
>>> TypeImpl *type = class->type;
>>> ObjectClass *ret = NULL;
>>>
>>> +if (type->name == typename) {
>>> +return class;
>>> +}
>>> +
>>> +target_type = type_get_by_name(typename);
>>> +
>>> if (!target_type) {
>>> /* target class type unknown, so fail the cast */
>>> return NULL;
>>>
>>> Which makes casting an object to it's concrete class free.
>>
>> Doesn't help here since concrete class is POWER7_v2.1-ppc64-cpu whereas
>> we're casting to ppc64-cpu, with two-level hierarchy by now:
>> POWER7_v2.1 -> POWER7 -> ppc64 -> device -> object.
> 
> How much performance penalty do we get from this?

Not sure which "this" you are referring to, but in general dynamic_cast
does a check for interfaces (which we don't have) and then iterates
through the hierarchy with string comparisons, i.e. negative, negative,
positive for POWERPC_CPU(). My original patch here dropped this penalty
for ppc_env_get_cpu(); CPU() would still result in negative, negative,
negative, positive.

Personally I wouldn't oppose dropping these checks for release builds as
proposed by Paolo in his series; for me, the value of POWERPC_CPU() is
being closer to an OO cast than any container_of()-style expressions.

But I can also see Anthony's point that we should try to optimize
dynamic_cast rather than circumventing it.

Andreas

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

[Qemu-devel] [RFC 7/7] pl181.c: convert to async IO SD card interface

[Igor Mitsyanko]

Signed-off-by: Igor Mitsyanko 
---
 hw/sd/pl181.c | 302 +-
 1 file changed, 192 insertions(+), 110 deletions(-)

diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index 2caacc2..a8a3510 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -20,7 +20,7 @@ do { printf("pl181: " fmt , ## __VA_ARGS__); } while (0)
 #define DPRINTF(fmt, ...) do {} while(0)
 #endif
 
-#define PL181_FIFO_LEN 16
+#define PL181_FIFO_LEN 64
 
 typedef struct {
 SysBusDevice busdev;
@@ -39,23 +39,23 @@ typedef struct {
 uint32_t status;
 uint32_t mask[2];
 int32_t fifo_pos;
-int32_t fifo_len;
 /* The linux 2.6.21 driver is buggy, and misbehaves if new data arrives
while it is reading the FIFO.  We hack around this be defering
subsequent transfers until after the driver polls the status word.
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=4446/1
  */
 int32_t linux_hack;
-uint32_t fifo[PL181_FIFO_LEN];
 qemu_irq irq[2];
 /* GPIO outputs for 'card is readonly' and 'card inserted' */
 qemu_irq cardstatus[2];
+qemu_irq sbit_received;
+qemu_irq busy_deasrt;
 } pl181_state;
 
 static const VMStateDescription vmstate_pl181 = {
 .name = "pl181",
-.version_id = 1,
-.minimum_version_id = 1,
+.version_id = 2,
+.minimum_version_id = 2,
 .fields = (VMStateField[]) {
 VMSTATE_UINT32(clock, pl181_state),
 VMSTATE_UINT32(power, pl181_state),
@@ -70,9 +70,7 @@ static const VMStateDescription vmstate_pl181 = {
 VMSTATE_UINT32(status, pl181_state),
 VMSTATE_UINT32_ARRAY(mask, pl181_state, 2),
 VMSTATE_INT32(fifo_pos, pl181_state),
-VMSTATE_INT32(fifo_len, pl181_state),
 VMSTATE_INT32(linux_hack, pl181_state),
-VMSTATE_UINT32_ARRAY(fifo, pl181_state, PL181_FIFO_LEN),
 VMSTATE_END_OF_LIST()
 }
 };
@@ -133,32 +131,110 @@ static void pl181_update(pl181_state *s)
 }
 }
 
+static inline unsigned int pl181_get_blk_size(pl181_state *s)
+{
+if (s->datactrl & PL181_DATA_MODE) {
+return 1;
+} else {
+return 1 << ((s->datactrl >> 4) & 0xf);
+}
+}
+
 static void pl181_fifo_push(pl181_state *s, uint32_t value)
 {
-int n;
+unsigned i;
+unsigned int blkdat_left = s->datacnt & (pl181_get_blk_size(s) - 1);
 
-if (s->fifo_len == PL181_FIFO_LEN) {
-fprintf(stderr, "pl181: FIFO overflow\n");
-return;
+if (blkdat_left == 0 && s->fifo_pos == 0) {
+/* Start of block */
+blkdat_left = pl181_get_blk_size(s);
+}
+
+for (i = 0; i < sizeof(uint32_t); ++i) {
+if (s->fifo_pos >= PL181_FIFO_LEN) {
+qemu_log_mask(LOG_GUEST_ERROR, "pl181_fifo_push: fifo overflow\n");
+break;
+}
+
+sd_write_data(s->card, value >> i * 8);
+
+if (blkdat_left > PL181_FIFO_LEN) {
+--blkdat_left;
+--s->datacnt;
+} else {
+++s->fifo_pos;
+}
+}
+
+if (s->fifo_pos == 0) {
+s->status &= ~(PL181_STATUS_TXDATAAVLBL | PL181_STATUS_TXFIFOFULL);
+s->status |= PL181_STATUS_TXFIFOEMPTY | PL181_STATUS_TXFIFOHALFEMPTY;
+} else {
+s->status &= ~(PL181_STATUS_TXFIFOEMPTY | PL181_STATUS_TXFIFOHALFEMPTY 
|
+PL181_STATUS_TXFIFOFULL);
+s->status |= PL181_STATUS_TXDATAAVLBL;
+
+if (s->fifo_pos == PL181_FIFO_LEN) {
+s->status |= PL181_STATUS_TXFIFOFULL;
+} else if (s->fifo_pos <= (PL181_FIFO_LEN / 2)) {
+s->status |= PL181_STATUS_TXFIFOHALFEMPTY;
+}
 }
-n = (s->fifo_pos + s->fifo_len) & (PL181_FIFO_LEN - 1);
-s->fifo_len++;
-s->fifo[n] = value;
-DPRINTF("FIFO push %08x\n", (int)value);
+
+DPRINTF("FIFO push %08x: datacnt=%u, fifo_pos=%u\n",
+(int)value, s->datacnt, s->fifo_pos);
+pl181_update(s);
 }
 
 static uint32_t pl181_fifo_pop(pl181_state *s)
 {
-uint32_t value;
+uint32_t value = 0;
+uint32_t blksz_mask = pl181_get_blk_size(s) - 1;
+unsigned i;
 
-if (s->fifo_len == 0) {
-fprintf(stderr, "pl181: FIFO underflow\n");
-return 0;
+for (i = 0; i < sizeof(uint32_t); ++i) {
+if (s->fifo_pos == 0) {
+qemu_log_mask(LOG_GUEST_ERROR, "pl181_fifo_pop: fifo underflow\n");
+break;
+}
+
+value |= sd_read_data(s->card) << i * 8;
+
+if (s->datacnt & blksz_mask) {
+--s->datacnt;
+} else {
+--s->fifo_pos;
+}
+}
+
+if (s->fifo_pos == 0) {
+s->status |= PL181_STATUS_RXFIFOEMPTY | PL181_STATUS_DATABLOCKEND;
+s->status &= ~(PL181_STATUS_RXDATAAVLBL | PL181_STATUS_RXFIFOFULL |
+PL181_STATUS_RXFIFOHALFFULL);
+
+if (s->datacnt == 0) {
+s->status |= PL181_STATUS_DATAEND;
+s->status &= ~PL181_STATUS_RX_FIFO;
+s->datactrl &= ~PL181_DATA_ENABLE;
+DPRINTF("Dat

[Qemu-devel] [RFC 5/7] sd.c: introduce async read operation

[Igor Mitsyanko]

It will only be used if start bit and databusy callbacks were initialized
by user of SD card model.

Signed-off-by: Igor Mitsyanko 
---
 hw/sd/sd.c | 85 --
 1 file changed, 77 insertions(+), 8 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a0bbbaa..659ec56 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -685,6 +685,44 @@ static void sd_lock_command(SDState *sd)
 sd->card_status &= ~CARD_IS_LOCKED;
 }
 
+static void sd_bdrv_read_done(void *opaque, int ret)
+{
+SDState *sd = opaque;
+uint32_t io_len, offset;
+uint64_t end_sector;
+
+DPRINTF("sd_bdrv_read_done ret = %d, \n", ret);
+sd->aiocb = NULL;
+
+if (ret != 0) {
+return;
+}
+
+if (sd->state != sd_sendingdata_state) {
+DPRINTF("Transfer was aborted\n");
+return;
+}
+
+io_len = (sd->ocr & (1 << 30)) ? 512 : sd->blk_len;
+end_sector = (sd->data_start + io_len - 1) >> BDRV_SECTOR_BITS;
+offset = (sd->data_start + sd->transf_cnt) & ~BDRV_SECTOR_MASK;
+
+if (end_sector >
+   (sd->data_start + sd->transf_cnt) >> BDRV_SECTOR_BITS) {
+memcpy(sd->data, sd->buf + offset, BDRV_SECTOR_SIZE - offset);
+sd->transf_cnt += BDRV_SECTOR_SIZE - offset;
+sd->aiocb = bdrv_aio_readv(sd->bdrv, end_sector, &sd->qiov, 1,
+sd_bdrv_read_done, sd);
+return;
+} else {
+memcpy(sd->data + sd->transf_cnt, sd->buf + offset,
+io_len - sd->transf_cnt);
+sd->transf_cnt += io_len - sd->transf_cnt;
+}
+
+qemu_irq_raise(sd->start_bit_cb);
+}
+
 static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
 {
@@ -891,8 +929,13 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
 sd->data_start = req.arg;
 sd->data_offset = 0;
 
-if (sd->data_start + sd->blk_len > sd->size)
+if (sd->data_start + sd->blk_len > sd->size) {
 sd->card_status |= ADDRESS_ERROR;
+} else if (sd->iov.iov_base) {
+sd->aiocb = bdrv_aio_readv(sd->bdrv,
+   sd->data_start >> BDRV_SECTOR_BITS,
+   &sd->qiov, 1, sd_bdrv_read_done, 
sd);
+}
 return sd_r0;
 
 default:
@@ -904,6 +947,11 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
 switch (sd->state) {
 case sd_sendingdata_state:
 sd->state = sd_transfer_state;
+sd->transf_cnt = 0;
+if (sd->aiocb) {
+bdrv_aio_cancel(sd->aiocb);
+sd->aiocb = NULL;
+}
 return sd_r1b;
 
 case sd_receivingdata_state:
@@ -969,8 +1017,13 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
 sd->data_start = addr;
 sd->data_offset = 0;
 
-if (sd->data_start + sd->blk_len > sd->size)
+if (sd->data_start + sd->blk_len > sd->size) {
 sd->card_status |= ADDRESS_ERROR;
+} else {
+sd->aiocb = bdrv_aio_readv(sd->bdrv,
+   sd->data_start >> BDRV_SECTOR_BITS,
+   &sd->qiov, 1, sd_bdrv_read_done, 
sd);
+}
 return sd_r1;
 
 default:
@@ -985,8 +1038,13 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
 sd->data_start = addr;
 sd->data_offset = 0;
 
-if (sd->data_start + sd->blk_len > sd->size)
+if (sd->data_start + sd->blk_len > sd->size) {
 sd->card_status |= ADDRESS_ERROR;
+} else {
+sd->aiocb = bdrv_aio_readv(sd->bdrv,
+   sd->data_start >> BDRV_SECTOR_BITS,
+   &sd->qiov, 1, sd_bdrv_read_done, 
sd);
+}
 return sd_r1;
 
 default:
@@ -1706,16 +1764,21 @@ uint8_t sd_read_data(SDState *sd)
 break;
 
 case 11:   /* CMD11:  READ_DAT_UNTIL_STOP */
-if (sd->data_offset == 0)
+if (sd->data_offset == 0 && sd->iov.iov_base == NULL) {
 BLK_READ_BLOCK(sd->data_start, io_len);
+}
 ret = sd->data[sd->data_offset ++];
 
 if (sd->data_offset >= io_len) {
 sd->data_start += io_len;
 sd->data_offset = 0;
+sd->transf_cnt = 0;
 if (sd->data_start + io_len > sd->size) {
 sd->card_status |= ADDRESS_ERROR;
-break;
+} else if (sd->iov.iov_base) {
+sd->aiocb = bdrv_aio_readv(sd->bdrv,
+   sd->data_start >> BDRV_SECTOR_BITS,
+   &sd->qiov, 1, sd_bdrv_read_done, 
sd);
 }
 }
 break;
@@ -1728,8 +1791,9 @@ uint8_t sd_read_data(SDState *sd)
   

[Qemu-devel] [RFC 6/7] sd.c: introduce async write interface

[Igor Mitsyanko]

Signed-off-by: Igor Mitsyanko 
---
 hw/sd/sd.c | 145 -
 1 file changed, 133 insertions(+), 12 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 659ec56..615ab61 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -958,6 +958,11 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
 sd->state = sd_programming_state;
 /* Bzzztt  Operation complete.  */
 sd->state = sd_transfer_state;
+sd->transf_cnt = 0;
+if (sd->aiocb) {
+bdrv_aio_cancel(sd->aiocb);
+sd->aiocb = NULL;
+}
 return sd_r1b;
 
 default:
@@ -1600,6 +1605,90 @@ static void sd_blk_write(SDState *sd, uint64_t addr, 
uint32_t len)
 #define APP_READ_BLOCK(a, len) memset(sd->data, 0xec, len)
 #define APP_WRITE_BLOCK(a, len)
 
+static void sd_write_done(void *opaque, int ret)
+{
+SDState *sd = opaque;
+
+DPRINTF("sd_write_done: ret = %d\n", ret);
+sd->aiocb = NULL;
+
+if (ret != 0) {
+return;
+}
+
+switch (sd->current_cmd) {
+case 24:/* CMD24:  WRITE_SINGLE_BLOCK */
+sd->blk_written++;
+sd->csd[14] |= 0x40;
+/* Bzzztt  Operation complete.  */
+sd->state = sd_transfer_state;
+qemu_irq_raise(sd->datbusy_cb);
+break;
+case 25:/* CMD25:  WRITE_MULTIPLE_BLOCK */
+sd->blk_written++;
+sd->data_start += sd->blk_len;
+sd->data_offset = 0;
+sd->transf_cnt = 0;
+sd->csd[14] |= 0x40;
+/* Bzzztt  Operation complete.  */
+sd->state = sd_receivingdata_state;
+qemu_irq_raise(sd->datbusy_cb);
+break;
+default:
+DPRINTF("unknown command\n");
+break;
+}
+}
+
+static void sd_read_next_sector(void *opaque, int ret);
+
+static void sd_sector_read_done(void *opaque, int ret)
+{
+SDState *sd = opaque;
+uint64_t end;
+unsigned offset, start;
+
+DPRINTF("sd_sector_read_done ret = %d\n", ret);
+sd->aiocb = NULL;
+
+if (ret != 0) {
+return;
+}
+
+start = sd->data_start + sd->transf_cnt;
+end = sd->data_start + sd->blk_len;
+offset = start & ~BDRV_SECTOR_MASK;
+
+if (end > ((start & BDRV_SECTOR_MASK) + BDRV_SECTOR_SIZE)) {
+memcpy(sd->buf + offset, sd->data, BDRV_SECTOR_SIZE - offset);
+sd->transf_cnt += BDRV_SECTOR_SIZE - offset;
+sd->aiocb = bdrv_aio_writev(sd->bdrv, start >> BDRV_SECTOR_BITS,
+&sd->qiov, 1, sd_read_next_sector, sd);
+} else {
+memcpy(sd->buf + offset, sd->data + sd->transf_cnt,
+sd->blk_len - sd->transf_cnt);
+sd->transf_cnt += sd->blk_len - sd->transf_cnt;
+sd->aiocb = bdrv_aio_writev(sd->bdrv, start >> BDRV_SECTOR_BITS,
+&sd->qiov, 1, sd_write_done, sd);
+}
+}
+
+static void sd_read_next_sector(void *opaque, int ret)
+{
+SDState *sd = opaque;
+
+DPRINTF("sd_read_next_sector ret = %d\n", ret);
+sd->aiocb = NULL;
+
+if (ret != 0) {
+return;
+}
+
+sd->aiocb = bdrv_aio_readv(sd->bdrv,
+   (sd->data_start >> BDRV_SECTOR_BITS) + 1,
+   &sd->qiov, 1, sd_sector_read_done, sd);
+}
+
 void sd_write_data(SDState *sd, uint8_t value)
 {
 int i;
@@ -1621,11 +1710,27 @@ void sd_write_data(SDState *sd, uint8_t value)
 if (sd->data_offset >= sd->blk_len) {
 /* TODO: Check CRC before committing */
 sd->state = sd_programming_state;
-BLK_WRITE_BLOCK(sd->data_start, sd->data_offset);
-sd->blk_written ++;
-sd->csd[14] |= 0x40;
-/* Bzzztt  Operation complete.  */
-sd->state = sd_transfer_state;
+
+if (sd->iov.iov_base) {
+if ((sd->data_start & ~BDRV_SECTOR_MASK) ||
+sd->blk_len < BDRV_SECTOR_SIZE) {
+sd->aiocb = bdrv_aio_readv(sd->bdrv,
+   sd->data_start >> BDRV_SECTOR_BITS,
+   &sd->qiov, 1, sd_sector_read_done, sd);
+break;
+}
+
+memcpy(sd->buf, sd->data, sd->blk_len);
+sd->aiocb = bdrv_aio_writev(sd->bdrv,
+sd->data_start >> BDRV_SECTOR_BITS,
+&sd->qiov, 1, sd_write_done, sd);
+} else {
+BLK_WRITE_BLOCK(sd->data_start, sd->data_offset);
+sd->blk_written ++;
+sd->csd[14] |= 0x40;
+/* Bzzztt  Operation complete.  */
+sd->state = sd_transfer_state;
+}
 }
 break;
 
@@ -1645,14 +1750,30 @@ void sd_write_data(SDState *sd, uint8_t value)
 if (sd->data_offset >= sd->blk_len) {
 /* TODO: Ch

[Qemu-devel] [RFC 4/7] sd.c: use callbacks as a flag to use async IO

[Igor Mitsyanko]

This is temporary to distinguish between sync and async users.

Signed-off-by: Igor Mitsyanko 
---
 hw/sd/sd.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 2e75201..a0bbbaa 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -530,6 +530,10 @@ void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq 
insert,
 sd->datbusy_cb = endbusy;
 qemu_set_irq(readonly, sd->bdrv ? bdrv_is_read_only(sd->bdrv) : 0);
 qemu_set_irq(insert, sd->bdrv ? bdrv_is_inserted(sd->bdrv) : 0);
+
+if (sd->start_bit_cb && sd->datbusy_cb) {
+sd->iov.iov_base = sd->buf;
+}
 }
 
 static void sd_erase(SDState *sd)
-- 
1.8.1.4

[Qemu-devel] [RFC 1/7] sd.c: introduce AIO related members in SD state

[Igor Mitsyanko]

New state members will be used for async IO implementation later.

Signed-off-by: Igor Mitsyanko 
---
 hw/sd/sd.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 2e0ef3e..1dd1331 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -112,6 +112,10 @@ struct SDState {
 uint8_t *buf;
 
 bool enable;
+
+QEMUIOVector qiov;
+struct iovec iov;
+BlockDriverAIOCB *aiocb;
 };
 
 static void sd_set_mode(SDState *sd)
@@ -403,6 +407,11 @@ static void sd_reset(SDState *sd, BlockDriverState *bdrv)
 uint64_t size;
 uint64_t sect;
 
+if (sd->aiocb) {
+bdrv_aio_cancel(sd->aiocb);
+sd->aiocb = NULL;
+}
+
 if (bdrv) {
 bdrv_get_geometry(bdrv, §);
 } else {
@@ -496,11 +505,15 @@ SDState *sd_init(BlockDriverState *bs, bool is_spi)
 sd->buf = qemu_blockalign(bs, 512);
 sd->spi = is_spi;
 sd->enable = true;
+sd->aiocb = NULL;
+sd->iov.iov_base = NULL;
+sd->iov.iov_len = BDRV_SECTOR_SIZE;
 sd_reset(sd, bs);
 if (sd->bdrv) {
 bdrv_attach_dev_nofail(sd->bdrv, sd);
 bdrv_set_dev_ops(sd->bdrv, &sd_block_ops, sd);
 }
+qemu_iovec_init_external(&sd->qiov, &sd->iov, 1);
 vmstate_register(NULL, -1, &sd_vmstate, sd);
 return sd;
 }
-- 
1.8.1.4

[Qemu-devel] [RFC 3/7] sd.c: introduce "start bit" and "busy deasserted" callbacks

[Igor Mitsyanko]

Start bit callback models arrival of first bit of data on card's DAT line.

Busy deasserted callback models releasing of DAT0 line by card when it
transitions from a programming state to a writing data state.

Both of them will be used for async IO later.

Signed-off-by: Igor Mitsyanko 
---
 hw/sd/omap_mmc.c|  6 +++---
 hw/sd/pl181.c   |  2 +-
 hw/sd/pxa2xx_mmci.c |  2 +-
 hw/sd/sd.c  | 24 +---
 hw/sd/sdhci.c   |  4 ++--
 include/hw/sd.h |  3 ++-
 6 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index d4079cd..94580bb 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -620,7 +620,7 @@ struct omap_mmc_s *omap2_mmc_init(struct 
omap_target_agent_s *ta,
 s->card = sd_init(bd, 0);
 
 s->cdet = qemu_allocate_irqs(omap_mmc_cover_cb, s, 1)[0];
-sd_set_cb(s->card, NULL, s->cdet);
+sd_set_cb(s->card, NULL, s->cdet, NULL, NULL);
 
 return s;
 }
@@ -628,11 +628,11 @@ struct omap_mmc_s *omap2_mmc_init(struct 
omap_target_agent_s *ta,
 void omap_mmc_handlers(struct omap_mmc_s *s, qemu_irq ro, qemu_irq cover)
 {
 if (s->cdet) {
-sd_set_cb(s->card, ro, s->cdet);
+sd_set_cb(s->card, ro, s->cdet, NULL, NULL);
 s->coverswitch = cover;
 qemu_set_irq(cover, s->cdet_state);
 } else
-sd_set_cb(s->card, ro, cover);
+sd_set_cb(s->card, ro, cover, NULL, NULL);
 }
 
 void omap_mmc_enable(struct omap_mmc_s *s, int enable)
diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index 2527296..2caacc2 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -471,7 +471,7 @@ static void pl181_reset(DeviceState *d)
 s->mask[1] = 0;
 
 /* We can assume our GPIO outputs have been wired up now */
-sd_set_cb(s->card, s->cardstatus[0], s->cardstatus[1]);
+sd_set_cb(s->card, s->cardstatus[0], s->cardstatus[1], NULL, NULL);
 }
 
 static int pl181_init(SysBusDevice *dev)
diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 2db1cab..64a3050 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -549,5 +549,5 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
 void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
 qemu_irq coverswitch)
 {
-sd_set_cb(s->card, readonly, coverswitch);
+sd_set_cb(s->card, readonly, coverswitch, NULL, NULL);
 }
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 775a55c..2e75201 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -108,6 +108,8 @@ struct SDState {
 uint8_t data[512];
 qemu_irq readonly_cb;
 qemu_irq inserted_cb;
+qemu_irq start_bit_cb;
+qemu_irq datbusy_cb;
 BlockDriverState *bdrv;
 uint8_t *buf;
 
@@ -519,10 +521,13 @@ SDState *sd_init(BlockDriverState *bs, bool is_spi)
 return sd;
 }
 
-void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
+void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert,
+   qemu_irq startbit, qemu_irq endbusy)
 {
 sd->readonly_cb = readonly;
 sd->inserted_cb = insert;
+sd->start_bit_cb = startbit;
+sd->datbusy_cb = endbusy;
 qemu_set_irq(readonly, sd->bdrv ? bdrv_is_read_only(sd->bdrv) : 0);
 qemu_set_irq(insert, sd->bdrv ? bdrv_is_inserted(sd->bdrv) : 0);
 }
@@ -762,6 +767,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
 sd->state = sd_sendingdata_state;
 sd->data_start = 0;
 sd->data_offset = 0;
+qemu_irq_raise(sd->start_bit_cb);
 return sd_r1;
 
 default:
@@ -841,6 +847,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
 memcpy(sd->data, sd->csd, 16);
 sd->data_start = addr;
 sd->data_offset = 0;
+qemu_irq_raise(sd->start_bit_cb);
 return sd_r1;
 
 default:
@@ -863,6 +870,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
 memcpy(sd->data, sd->cid, 16);
 sd->data_start = addr;
 sd->data_offset = 0;
+qemu_irq_raise(sd->start_bit_cb);
 return sd_r1;
 
 default:
@@ -,6 +1119,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
 *(uint32_t *) sd->data = sd_wpbits(sd, req.arg);
 sd->data_start = addr;
 sd->data_offset = 0;
+qemu_irq_raise(sd->start_bit_cb);
 return sd_r1b;
 
 default:
@@ -1201,10 +1210,12 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
 switch (sd->state) {
 case sd_transfer_state:
 sd->data_offset = 0;
-if (req.arg & 1)
+if (req.arg & 1) {
 sd->state = sd_sendingdata_state;
-else
+qemu_irq_raise(sd->start_bit_cb);
+} else {
 sd->state = sd_receivingdata_state;
+}
 return sd_r1;
 
 default:
@@ -1251,6 +1262,7 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
 sd->state = sd_sendingdata_state;

[Qemu-devel] [RFC 2/7] sd.c: introduce variable for trekking valid data

[Igor Mitsyanko]

Initialize it appropriately when various commands are processed.

Signed-off-by: Igor Mitsyanko 
---
 hw/sd/sd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 1dd1331..775a55c 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -116,6 +116,7 @@ struct SDState {
 QEMUIOVector qiov;
 struct iovec iov;
 BlockDriverAIOCB *aiocb;
+uint32_t transf_cnt;
 };
 
 static void sd_set_mode(SDState *sd)
-- 
1.8.1.4

[Qemu-devel] [RFC 0/7] Convert SD card model to AIO

[Igor Mitsyanko]

This is an initial attempt to change our SD card model to use asynchronious
input/output API instead of synchronious one. This will require converting of
every user also. Right now I've converted only PL181 model, and I'll wait
for some feedback on taken approach before I'll continue with other users.

New async SD interface is built on two callbacks:
1 Callback which models arrival of start bit on DAT line from card (start bit)
2 Callback modeling deasserting of DAT0 line by card when it has finished
programming data (data busy end)

This is based on SD card specification.
Start bit is issued for every new block of data coming from card.
DAT0 line is kept low by card to signal busy state, while data block is being
programmed by card and it can't receive new data.

Furthermore, I decided to drop data buffering on SD controller side. We already
have two buffers in SD card model, no need to have another one in controller 
model.
We can avoid intermediate copying from card's buffer to controller's buffer, and
read/write directly from card.

Tested by running this Fedora image 
https://fedoraproject.org/wiki/Architectures/ARM/F18/Versatile_Express
on versatile board.

Igor Mitsyanko (7):
  sd.c: introduce AIO related members in SD state
  sd.c: introduce variable for trekking valid data
  sd.c: introduce "start bit" and "busy deasserted" callbacks
  sd.c: use callbacks as a flag to use async IO
  sd.c: introduce async read operation
  sd.c: introduce async write interface
  pl181.c: convert to async IO SD card interface

 hw/sd/omap_mmc.c|   6 +-
 hw/sd/pl181.c   | 302 +---
 hw/sd/pxa2xx_mmci.c |   2 +-
 hw/sd/sd.c  | 272 ++
 hw/sd/sdhci.c   |   4 +-
 include/hw/sd.h |   3 +-
 6 files changed, 449 insertions(+), 140 deletions(-)

-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values

[mdroth]

On Fri, May 10, 2013 at 11:17:17AM -0400, Luiz Capitulino wrote:
> On Thu,  9 May 2013 21:20:58 -0500
> Michael Roth  wrote:
> 
> > Currently our JSON parser assumes that numbers lacking a mantissa are
> > integers and attempts to store them as QInt/int64 values. This breaks in
> > the case where the number overflows/underflows int64 values (which is
> > still valid JSON)
> 
> Anthony wanted to fix this by moving to another wire format :)
> 
> But, how this patch related to this series?

In v1 Laszlo pointed out that the QFloat tests I added in
test-visitor-serialization were actually non-functional, and those tests were
based on pre-existing code. So I added a patch to fix the pre-existing
code as a pre-cursor to the new unit tests based on it. But fixing that code
exposed the json-parser bug, so I added that fix as a precursor to the
precursor :)

Same with mem leak fixes, etc, to ensure the tests were functional and
leak-free within this series.

> 
> > 
> > Fix this by detecting such cases and using a QFloat to store the value
> > instead.
> > 
> > Signed-off-by: Michael Roth 
> > ---
> >  qobject/json-parser.c |   26 +++---
> >  1 file changed, 23 insertions(+), 3 deletions(-)
> > 
> > diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> > index 05279c1..4d14e71 100644
> > --- a/qobject/json-parser.c
> > +++ b/qobject/json-parser.c
> > @@ -640,9 +640,29 @@ static QObject *parse_literal(JSONParserContext *ctxt)
> >  case JSON_STRING:
> >  obj = QOBJECT(qstring_from_escaped_str(ctxt, token));
> >  break;
> > -case JSON_INTEGER:
> > -obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 
> > 10)));
> > -break;
> > +case JSON_INTEGER: {
> > +/* A possibility exists that this is a whole-valued float where the
> > + * mantissa was left out due to being 0 (.0). It's not a big deal 
> > to
> > + * treat these as ints in the parser, so long as users of the
> > + * resulting QObject know to expect a QInt in place of a QFloat in
> > + * cases like these.
> > + *
> > + * However, in some cases these values will overflow/underflow a
> > + * QInt/int64 container, thus we should assume these are to be 
> > handled
> > + * as QFloats/doubles rather than silently changing their values.
> > + *
> > + * strtoll() indicates these instances by setting errno to ERANGE
> > + */
> > +int64_t value;
> > +
> > +errno = 0; /* strtoll doesn't set errno on success */
> > +value = strtoll(token_get_value(token), NULL, 10);
> > +if (errno != ERANGE) {
> > +obj = QOBJECT(qint_from_int(value));
> > +break;
> > +}
> > +/* fall through to JSON_FLOAT */
> > +}
> >  case JSON_FLOAT:
> >  /* FIXME dependent on locale */
> >  obj = QOBJECT(qfloat_from_double(strtod(token_get_value(token), 
> > NULL)));
> 

Re: [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types

[Laszlo Ersek]

On 05/10/13 17:30, Luiz Capitulino wrote:
> On Thu,  9 May 2013 21:20:52 -0500
> Michael Roth  wrote:
> 
>> These patches apply on top of qemu.git master, and can also be obtained from:
>> git://github.com/mdroth/qemu.git qapi-native-lists
>>
>> Sending this now since a number of series have popped up in the past that
>> wanted this, and Amos has some pending patches (query-mac-tables) that rely
>> on this as well.
>>
>> These patches add support for specifying lists of native qapi types
>> (int/bool/str/number) like so:
>>
>>   { 'type': 'foo',
>> 'data': { 'bar': ['int'] }}
>>
>> for a 'bar' field that is a list of type 'int',
>>
>>   { 'type': 'foo2',
>> 'data': { 'bar2': ['str'] }}
>>
>> for a 'bar2' field that is a list of type 'str', and so on.
>>
>> This uses linked list types for the native C representations, just as we do
>> for complex schema-defined types. In the future we may add schema annotations
>> of some sort to specify a more natural/efficient array type for the C
>> representations, but this should serve the majority of uses-cases for now.
> 
> Series looks good to me. I'd drop patch 06/10 if it's not required
> for this series though.

I guess:

On 05/10/13 04:20, Michael Roth wrote:
> v1->v2:
>  * fixed do-nothing float tests in pre-existing code and updated new
>unit tests accordingly (Laszlo)
>  * added a fix for a bug in json parser that was exposed by above change

Laszlo

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Aurelien Jarno]

On Fri, May 10, 2013 at 02:16:34PM +0200, Paolo Bonzini wrote:
> Cast debugging can have a substantial cost (20% or more, measured by
> Aurelien on qemu-system-ppc64).  Instead of adding special-cased "fast
> casts" in the hot paths, we can just disable it in releases.  At the
> same time, add tracing facilities that simplify the analysys of those
> problems that cast debugging would reveal.
> 
> At least patches 1-7 are for 1.5.
> 
> Paolo Bonzini (9):
>   qom: improve documentation of cast functions
>   qom: allow casting of a NULL class
>   qom: add a fast path to object_class_dynamic_cast
>   qom: pass file/line/function to asserting casts
>   qom: trace asserting casts
>   qom: allow turning cast debugging off
>   build: disable QOM cast debugging for official releases
>   qom: simplify object_class_dynamic_cast, part 1
>   qom: simplify object_class_dynamic_cast, part 2
> 
>  configure| 20 --
>  include/qom/object.h | 40 ++-
>  qom/object.c | 77 
> ++--
>  trace-events |  3 ++
>  4 files changed, 99 insertions(+), 41 deletions(-)
> 

I have tested this series with qemu-system-ppc64, on a Core i7 2600 CPU.
The process was set to a single core using taskset. Inside the guest
(Debian ppc64 from debian-ports), I ran the command three times:

  lintian g++-4.8_4.8.0-6_ppc64.deb

I used lintian as it's a perl code, that trigger the discussion about
sparc/ppc comparison.

First of all with this patch series, the object_class_dynamic_cast calls
went down to below 0.1% when using perf top. Before the patch series was
applied, the command took in average on 3 runs 142.4s. With the patch
series, it went down to 129.8s, so almost 9% faster.

To improve the performance a bit more, and come back to the same kind of
code as before, we should move simple accessors from qom/*.c to
include/qom/*.h and mark them as inline, so that they can be removed by
the compiler. Currently, even if the function is simple it's still a
call/ret in the hot path instead of a simple pointer addition.

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

Re: [Qemu-devel] [PATCH 01/10] qapi: qapi-types.py, native list support

[mdroth]

On Fri, May 10, 2013 at 10:07:45AM -0400, Luiz Capitulino wrote:
> On Thu,  9 May 2013 21:20:53 -0500
> Michael Roth  wrote:
> 
> > Teach type generators about native types so they can generate the
> > appropriate linked list types.
> > 
> > Signed-off-by: Michael Roth 
> > ---
> >  scripts/qapi-types.py |   43 ---
> >  scripts/qapi.py   |   21 +
> >  2 files changed, 61 insertions(+), 3 deletions(-)
> > 
> > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> > index 9e19920..96cb26d 100644
> > --- a/scripts/qapi-types.py
> > +++ b/scripts/qapi-types.py
> > @@ -16,7 +16,18 @@ import os
> >  import getopt
> >  import errno
> >  
> > -def generate_fwd_struct(name, members):
> > +def generate_fwd_struct(name, members, builtin_type=False):
> > +if builtin_type:
> > +return mcgen('''
> > +typedef struct %(name)sList
> > +{
> > +%(type)s value;
> > +struct %(name)sList *next;
> > +} %(name)sList;
> > +''',
> 
> Sorry for the utterly minor comment, but as you're going to respin please
> add a newline before ''' so that we get the declarations properly separated
> when generated.
> 
> > + type=c_type(name),
> > + name=name)
> > +
> >  return mcgen('''
> >  typedef struct %(name)s %(name)s;
> >  
> > @@ -164,6 +175,7 @@ void qapi_free_%(type)s(%(c_type)s obj);
> >  
> >  def generate_type_cleanup(name):
> >  ret = mcgen('''
> > +
> >  void qapi_free_%(type)s(%(c_type)s obj)
> >  {
> >  QapiDeallocVisitor *md;
> > @@ -184,8 +196,9 @@ void qapi_free_%(type)s(%(c_type)s obj)
> >  
> >  
> >  try:
> > -opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:o:",
> > -   ["source", "header", "prefix=", 
> > "output-dir="])
> > +opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:",
> > +   ["source", "header", "builtins",
> > +"prefix=", "output-dir="])
> >  except getopt.GetoptError, err:
> >  print str(err)
> >  sys.exit(1)
> > @@ -197,6 +210,7 @@ h_file = 'qapi-types.h'
> >  
> >  do_c = False
> >  do_h = False
> > +do_builtins = False
> >  
> >  for o, a in opts:
> >  if o in ("-p", "--prefix"):
> > @@ -207,6 +221,8 @@ for o, a in opts:
> >  do_c = True
> >  elif o in ("-h", "--header"):
> >  do_h = True
> > +elif o in ("-b", "--builtins"):
> > +do_builtins = True
> >  
> >  if not do_c and not do_h:
> >  do_c = True
> > @@ -282,6 +298,11 @@ fdecl.write(mcgen('''
> >  exprs = parse_schema(sys.stdin)
> >  exprs = filter(lambda expr: not expr.has_key('gen'), exprs)
> >  
> > +fdecl.write(guardstart("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
> > +for typename in builtin_types:
> > +fdecl.write(generate_fwd_struct(typename, None, builtin_type=True))
> > +fdecl.write(guardend("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
> > +
> >  for expr in exprs:
> >  ret = "\n"
> >  if expr.has_key('type'):
> > @@ -298,6 +319,22 @@ for expr in exprs:
> >  continue
> >  fdecl.write(ret)
> >  
> > +# to avoid header dependency hell, we always generate declarations
> > +# for built-in types in our header files and simply guard them
> > +fdecl.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
> > +for typename in builtin_types:
> > +fdecl.write(generate_type_cleanup_decl(typename + "List"))
> > +fdecl.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
> 
> I'm not sure I got why you're doing this. Is it because you're going to
> generate them in more .h files? This is a bit ugly :(
> 

The issue is that things like the types generated from
qapi-schema-test.json or qga/qapi-schema.json may end up referencing
intList/strList/visit_type_intList/etc, which we'll also have declared for
use in the main qapi-schema.json. qapi-schema.json of course can't
depend on tests or qga, so we generate the definitions for the builtin types
when running the code generators on qapi-schema.json.

For everyone else, tests/qga/etc, to be able to use/link against those
definitions we need to include the declarations by either #include'ing
qapi-types.h/qapi-visit.h etc, or by always declaring them and simply
adding a guard.

In this case I've taken the latter approach since hard-coding a
reference in the code generators to header files created by separate calls
to the code generators seemed less modular. qapi-schema-test.json should
be capable of generating self-contained code if need be (and the only
reason we don't make it self-contained is that test-cases rely on
libqemuutil to build, which actually does have a dependency on
qapi-schema.json due to qemu-sockets.

> > +
> > +# ...this doesn't work for cases where we link in multiple objects that
> > +# have the functions defined, so we use -b option to provide control
> > +# over these cases
> > +if do_builtins:
> > +fdef.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DEF"))
> > +for typename in buil

Re: [Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Andreas Färber]

[resending after bounce]

Am 10.05.2013 17:06, schrieb Anthony Liguori:
> Andreas Färber  writes:
> 
>> A transition from CPUPPCState to PowerPCCPU can be considered safe,
>> just like PowerPCCPU::env access in the opposite direction.
>>
>> This should slightly improve interrupt performance.
>>
>> Reported-by: Anthony Liguori 
>> Signed-off-by: Andreas Färber 
> 
> Another option would be to leave it and do something like:
> 
> diff --git a/qom/object.c b/qom/object.c
> index 75e6aac..cba1d88 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -126,8 +126,13 @@ static TypeImpl *type_register_internal(const TypeInfo 
> *info)
>  
>  TypeImpl *type_register(const TypeInfo *info)
>  {
> +TypeImpl *impl;
> +
>  assert(info->parent);
> -return type_register_internal(info);
> +impl = type_register_internal(info);
> +g_free(impl->name);
> +impl->name = info->name;
> +return impl;
>  }
>  
>  TypeImpl *type_register_static(const TypeInfo *info)
> @@ -449,10 +490,16 @@ Object *object_dynamic_cast_assert(Object *obj, const 
> char *typename)
>  ObjectClass *object_class_dynamic_cast(ObjectClass *class,
> const char *typename)
>  {
> -TypeImpl *target_type = type_get_by_name(typename);
> +TypeImpl *target_type;
>  TypeImpl *type = class->type;
>  ObjectClass *ret = NULL;
>  
> +if (type->name == typename) {
> +return class;
> +}
> +
> +target_type = type_get_by_name(typename);
> +
>  if (!target_type) {
>  /* target class type unknown, so fail the cast */
>  return NULL;
> 
> Which makes casting an object to it's concrete class free.

Doesn't help here since concrete class is POWER7_v2.1-ppc64-cpu whereas
we're casting to ppc64-cpu, with two-level hierarchy by now:
POWER7_v2.1 -> POWER7 -> ppc64 -> device -> object.

Regards,
Andreas

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

[Qemu-devel] [Bug 1175513] Re: Qemu 1.5-git gpu clock control doesn`t work after guest reboot

[commiethebeastie]

And all work after killing qemu with ^C.

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

Title:
  Qemu 1.5-git gpu clock control doesn`t work after guest reboot

Status in The Linux Kernel:
  New
Status in QEMU:
  New

Bug description:
  I run qemu from git with such command:

  qemu-system-x86_64 -nodefaults -m 4096 -smp 8,cores=4,threads=2,sockets=1 
-cpu 'kvm64' -device usb-mouse -M q35 -vga qxl -no-hpet -boot once=c,menu=on 
-device vfio-pci,host=02:00.0,x-vga=on \
  -enable-kvm -monitor stdio -chardev 
socket,path=/tmp/qga.sock,server,nowait,id=qga0 -device virtio-serial -device 
virtserialport,chardev=qga0,name=org.qemu.guest_agent.0 -net 
nic,vlan=0,model=e1000 -net tap,ifname=tap0,script=/etc/guest-ifup -usb -device 
intel-hda -device hda-duplex \
  -drive 
file='/home//qemu/win7',if=none,id=drive-virtio-disk0,cache=writeback,aio=native,format=qed,discard=on
 -device virtio-blk-pci,drive=drive-virtio-disk0,id=virtio-disk \
  -drive 
file='/dev/sr0',if=none,id=drive-ide1-0-0,media=cdrom,snapshot=off,format=raw 
-device ide-drive,bus=ide.1,unit=0,drive=drive-ide1-0-0,id=ide1-0-0 \
  -spice port=5930,disable-ticketing

  Before guest (Windows 7) reboot, videocard works in 3D mode with full
  frequency. But after reboot videocard works in 3D only with powersafe
  frequency. Then I must reboot host for recover gpu clock control.

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

Re: [Qemu-devel] [PATCH 0/2] pci-assign: MSI affinity support

[Alex Williamson]

On Fri, 2013-05-10 at 14:40 +0200, Jan Kiszka wrote:
> On 2013-05-09 18:35, Alex Williamson wrote:
> > I posted these about 6 months ago and Jan felt we should implement
> > MSI notifiers like we have for MSI-X.  That still hasn't happened.
> 
> Device assignments are the only currently known users - and you provide
> this feature, so...
> 
> POWER does this nice configuration of MSI messages via a side channel.
> Not that it already fires the MSI-X notifiers properly, but a generic
> notifier based approach is the right way to abstract away the different
> modification channels (instead of encoding them at the consumer side
> like in your patches).
> 
> Moreover, having different designs for MSI and MSI-X is just ugly.

OTOH, if device assignment is the only one that needs this and it's
trivial to implement there, why would we bloat the common code with
notifiers and caching vector messages.  I'm not that thrilled with the
MSI-X notifiers to go copy them into MSI anyway, they completely miss
important things like an overall device MSI-X enable or disable.

For this much code:

 hw/i386/kvm/pci-assign.c |   78 +++---
 1 file changed, 53 insertions(+), 25 deletions(-)

we get a feature that's immediately useful to a guest.  Seems like a win
to me.  Thanks,

Alex

Re: [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types

[Luiz Capitulino]

On Fri, 10 May 2013 17:40:20 +0200
Laszlo Ersek  wrote:

> On 05/10/13 17:30, Luiz Capitulino wrote:
> > On Thu,  9 May 2013 21:20:52 -0500
> > Michael Roth  wrote:
> > 
> >> These patches apply on top of qemu.git master, and can also be obtained 
> >> from:
> >> git://github.com/mdroth/qemu.git qapi-native-lists
> >>
> >> Sending this now since a number of series have popped up in the past that
> >> wanted this, and Amos has some pending patches (query-mac-tables) that rely
> >> on this as well.
> >>
> >> These patches add support for specifying lists of native qapi types
> >> (int/bool/str/number) like so:
> >>
> >>   { 'type': 'foo',
> >> 'data': { 'bar': ['int'] }}
> >>
> >> for a 'bar' field that is a list of type 'int',
> >>
> >>   { 'type': 'foo2',
> >> 'data': { 'bar2': ['str'] }}
> >>
> >> for a 'bar2' field that is a list of type 'str', and so on.
> >>
> >> This uses linked list types for the native C representations, just as we do
> >> for complex schema-defined types. In the future we may add schema 
> >> annotations
> >> of some sort to specify a more natural/efficient array type for the C
> >> representations, but this should serve the majority of uses-cases for now.
> > 
> > Series looks good to me. I'd drop patch 06/10 if it's not required
> > for this series though.
> 
> I guess:

I see, although it's still a separate issue (which I'd personally fix
in a different series).

> 
> On 05/10/13 04:20, Michael Roth wrote:
> > v1->v2:
> >  * fixed do-nothing float tests in pre-existing code and updated new
> >unit tests accordingly (Laszlo)
> >  * added a fix for a bug in json parser that was exposed by above change
> 
> Laszlo
> 

Re: [Qemu-devel] [PATCH] osdep.h: include sys/types.h for ssize_t definition

[Igor Mitsyanko]

On 05/10/2013 07:24 PM, Peter Maydell wrote:

On 10 May 2013 16:16, Igor Mitsyanko  wrote:

This fixes build for mingw32

Signed-off-by: Igor Mitsyanko 
---
  include/qemu/osdep.h | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 42545bc..17946a3 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -163,6 +163,8 @@ int qemu_create_pidfile(const char *filename);
  int qemu_get_thread_id(void);

  #ifndef CONFIG_IOVEC
+#include 
+

This is quite a long way down to have a system #include.

Can we just take the existing include of sys/types.h at
the top of the file out of its current #ifdef __OpenBSD__
guard ?

(http://hacks.owlfolio.org/header-survey/ has a full row
of greens for sys/types.h so this should be safe.)

thanks
-- PMM


Nice link.
Long way, yes, but there's "#include " even further down in 
this file. But I guess taking existing sys/types.h is still better, I'll 
resend it.

[Qemu-devel] [PATCH V2 0/2] Fix memory migration for exynos 4210 SoC

[Igor Mitsyanko]

Fix issues in exynos4210 code which were blocking proper memory
migration.

V1->V2
PATCH1 now doesn't use memory_region_init_ram_ptr at all. Instead, it
converts chipid memory to an mmio region.
PATCH3 is dropped until hard freeze is over. It better be sent separately
anyway.

Igor Mitsyanko (2):
  hw/arm/exynos4210.c: convert chipid_and_omr to an mmio region
  exynos4210.c: register rom_mem for memory migration

 hw/arm/exynos4210.c | 28 +---
 1 file changed, 25 insertions(+), 3 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH V2 2/2] exynos4210.c: register rom_mem for memory migration

[Igor Mitsyanko]

From: Igor Mitsyanko 

Even if we do not register newly created RAM MemoryRegion for migration with
vmstate_register_ram_global() function, ram_save_setup() still saves this region
to snapshot file with empty idstr=="". Consequently this results in error during
VM loading in ram_load().
Register rom_mem for migration.

Signed-off-by: Igor Mitsyanko 
---
 hw/arm/exynos4210.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index c998fbb..be597db 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -254,6 +254,7 @@ Exynos4210State *exynos4210_init(MemoryRegion *system_mem,
 /* Internal ROM */
 memory_region_init_ram(&s->irom_mem, "exynos4210.irom",
EXYNOS4210_IROM_SIZE);
+vmstate_register_ram_global(&s->irom_mem);
 memory_region_set_readonly(&s->irom_mem, true);
 memory_region_add_subregion(system_mem, EXYNOS4210_IROM_BASE_ADDR,
 &s->irom_mem);
-- 
1.8.1.4

[Qemu-devel] [PATCH V2 1/2] hw/arm/exynos4210.c: convert chipid_and_omr to an mmio region

[Igor Mitsyanko]

From: Igor Mitsyanko 

Exynos SoC was misusing memory_region_init_ram_ptr(): this interface can safely
be used only for memory regions which size is a multiple of target page size.
Change chipid_and_omr memory to an mmio region to fix this.

Signed-off-by: Igor Mitsyanko 
---
 hw/arm/exynos4210.c | 27 ---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index 6c2dca5..c998fbb 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -84,6 +84,28 @@
 static uint8_t chipid_and_omr[] = { 0x11, 0x02, 0x21, 0x43,
 0x09, 0x00, 0x00, 0x00 };
 
+static uint64_t exynos4210_chipid_and_omr_read(void *opaque, hwaddr offset,
+   unsigned size)
+{
+assert(offset < sizeof(chipid_and_omr));
+return chipid_and_omr[offset];
+}
+
+static void exynos4210_chipid_and_omr_write(void *opaque, hwaddr offset,
+uint64_t value, unsigned size)
+{
+return;
+}
+
+static const MemoryRegionOps exynos4210_chipid_and_omr_ops = {
+.read = exynos4210_chipid_and_omr_read,
+.write = exynos4210_chipid_and_omr_write,
+.endianness = DEVICE_NATIVE_ENDIAN,
+.impl = {
+.max_access_size = 1,
+}
+};
+
 void exynos4210_write_secondary(ARMCPU *cpu,
 const struct arm_boot_info *info)
 {
@@ -224,9 +246,8 @@ Exynos4210State *exynos4210_init(MemoryRegion *system_mem,
 /*** Memory ***/
 
 /* Chip-ID and OMR */
-memory_region_init_ram_ptr(&s->chipid_mem, "exynos4210.chipid",
-sizeof(chipid_and_omr), chipid_and_omr);
-memory_region_set_readonly(&s->chipid_mem, true);
+memory_region_init_io(&s->chipid_mem, &exynos4210_chipid_and_omr_ops,
+NULL, "exynos4210.chipid", sizeof(chipid_and_omr));
 memory_region_add_subregion(system_mem, EXYNOS4210_CHIPID_ADDR,
 &s->chipid_mem);
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types

[Luiz Capitulino]

On Thu,  9 May 2013 21:20:52 -0500
Michael Roth  wrote:

> These patches apply on top of qemu.git master, and can also be obtained from:
> git://github.com/mdroth/qemu.git qapi-native-lists
> 
> Sending this now since a number of series have popped up in the past that
> wanted this, and Amos has some pending patches (query-mac-tables) that rely
> on this as well.
> 
> These patches add support for specifying lists of native qapi types
> (int/bool/str/number) like so:
> 
>   { 'type': 'foo',
> 'data': { 'bar': ['int'] }}
> 
> for a 'bar' field that is a list of type 'int',
> 
>   { 'type': 'foo2',
> 'data': { 'bar2': ['str'] }}
> 
> for a 'bar2' field that is a list of type 'str', and so on.
> 
> This uses linked list types for the native C representations, just as we do
> for complex schema-defined types. In the future we may add schema annotations
> of some sort to specify a more natural/efficient array type for the C
> representations, but this should serve the majority of uses-cases for now.

Series looks good to me. I'd drop patch 06/10 if it's not required
for this series though.

Re: [Qemu-devel] [PATCH] osdep.h: include sys/types.h for ssize_t definition

[Peter Maydell]

On 10 May 2013 16:16, Igor Mitsyanko  wrote:
> This fixes build for mingw32
>
> Signed-off-by: Igor Mitsyanko 
> ---
>  include/qemu/osdep.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 42545bc..17946a3 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -163,6 +163,8 @@ int qemu_create_pidfile(const char *filename);
>  int qemu_get_thread_id(void);
>
>  #ifndef CONFIG_IOVEC
> +#include 
> +

This is quite a long way down to have a system #include.

Can we just take the existing include of sys/types.h at
the top of the file out of its current #ifdef __OpenBSD__
guard ?

(http://hacks.owlfolio.org/header-survey/ has a full row
of greens for sys/types.h so this should be safe.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values

[Luiz Capitulino]

On Thu,  9 May 2013 21:20:58 -0500
Michael Roth  wrote:

> Currently our JSON parser assumes that numbers lacking a mantissa are
> integers and attempts to store them as QInt/int64 values. This breaks in
> the case where the number overflows/underflows int64 values (which is
> still valid JSON)

Anthony wanted to fix this by moving to another wire format :)

But, how this patch related to this series?

> 
> Fix this by detecting such cases and using a QFloat to store the value
> instead.
> 
> Signed-off-by: Michael Roth 
> ---
>  qobject/json-parser.c |   26 +++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> index 05279c1..4d14e71 100644
> --- a/qobject/json-parser.c
> +++ b/qobject/json-parser.c
> @@ -640,9 +640,29 @@ static QObject *parse_literal(JSONParserContext *ctxt)
>  case JSON_STRING:
>  obj = QOBJECT(qstring_from_escaped_str(ctxt, token));
>  break;
> -case JSON_INTEGER:
> -obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 
> 10)));
> -break;
> +case JSON_INTEGER: {
> +/* A possibility exists that this is a whole-valued float where the
> + * mantissa was left out due to being 0 (.0). It's not a big deal to
> + * treat these as ints in the parser, so long as users of the
> + * resulting QObject know to expect a QInt in place of a QFloat in
> + * cases like these.
> + *
> + * However, in some cases these values will overflow/underflow a
> + * QInt/int64 container, thus we should assume these are to be 
> handled
> + * as QFloats/doubles rather than silently changing their values.
> + *
> + * strtoll() indicates these instances by setting errno to ERANGE
> + */
> +int64_t value;
> +
> +errno = 0; /* strtoll doesn't set errno on success */
> +value = strtoll(token_get_value(token), NULL, 10);
> +if (errno != ERANGE) {
> +obj = QOBJECT(qint_from_int(value));
> +break;
> +}
> +/* fall through to JSON_FLOAT */
> +}
>  case JSON_FLOAT:
>  /* FIXME dependent on locale */
>  obj = QOBJECT(qfloat_from_double(strtod(token_get_value(token), 
> NULL)));

[Qemu-devel] [PATCH] osdep.h: include sys/types.h for ssize_t definition

[Igor Mitsyanko]

This fixes build for mingw32

Signed-off-by: Igor Mitsyanko 
---
 include/qemu/osdep.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 42545bc..17946a3 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -163,6 +163,8 @@ int qemu_create_pidfile(const char *filename);
 int qemu_get_thread_id(void);
 
 #ifndef CONFIG_IOVEC
+#include 
+
 struct iovec {
 void *iov_base;
 size_t iov_len;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 05/10] qapi: fix leak in unit tests

[Luiz Capitulino]

On Thu,  9 May 2013 21:20:57 -0500
Michael Roth  wrote:

> qmp_output_get_qobject() increments the qobject's reference count. Since
> we currently pass this straight into qobject_to_json() so we can feed
> the data into a QMP input visitor, we never actually free the underlying
> qobject when qmp_output_visitor_cleanup() is called. This causes leaks
> on all of the QMP serialization tests.
> 
> Fix this by holding a pointer to the qobject and decref'ing it before
> returning from qmp_deserialize().
> 
> Signed-off-by: Michael Roth 

I've cherry-picked this one into the qmp branch for 1.5.

> ---
>  tests/test-visitor-serialization.c |9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/test-visitor-serialization.c 
> b/tests/test-visitor-serialization.c
> index e84926f..8c8adac 100644
> --- a/tests/test-visitor-serialization.c
> +++ b/tests/test-visitor-serialization.c
> @@ -657,11 +657,16 @@ static void qmp_deserialize(void **native_out, void 
> *datap,
>  VisitorFunc visit, Error **errp)
>  {
>  QmpSerializeData *d = datap;
> -QString *output_json = qobject_to_json(qmp_output_get_qobject(d->qov));
> -QObject *obj = qobject_from_json(qstring_get_str(output_json));
> +QString *output_json;
> +QObject *obj_orig, *obj;
> +
> +obj_orig = qmp_output_get_qobject(d->qov);
> +output_json = qobject_to_json(obj_orig);
> +obj = qobject_from_json(qstring_get_str(output_json));
>  
>  QDECREF(output_json);
>  d->qiv = qmp_input_visitor_new(obj);
> +qobject_decref(obj_orig);
>  qobject_decref(obj);
>  visit(qmp_input_get_visitor(d->qiv), native_out, errp);
>  }

Re: [Qemu-devel] CPU scheduling with TCG in SMP system emulation mode

[Sebastian Huber]

On 02/22/2013 01:46 AM, Max Filippov wrote:

Hello.

Do I understand it right that there's no dedicated mechanism
other than icount that would switch current CPU in emulated
SMP system and that in the absence of icount such scheduling
is a side effect of interrupt delivery to the current CPU?



In case I start two CPUs and execute a while (1) loop in both of them with 
interrupts disabled.  Will Qemu ever switch from one CPU to the other?


--
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax : +49 89 189 47 41-09
E-Mail  : sebastian.hu...@embedded-brains.de
PGP : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

Re: [Qemu-devel] [RFC PATCH v5 3/3] Force auto-convegence of live migration

[Anthony Liguori]

Chegu Vinod  writes:

> On 5/10/2013 6:07 AM, Anthony Liguori wrote:
>> Chegu Vinod  writes:
>>
>>>   If a user chooses to turn on the auto-converge migration capability
>>>   these changes detect the lack of convergence and throttle down the
>>>   guest. i.e. force the VCPUs out of the guest for some duration
>>>   and let the migration thread catchup and help converge.
>>>
>>>   Verified the convergence using the following :
>>>   - SpecJbb2005 workload running on a 20VCPU/256G guest(~80% busy)
>>>   - OLTP like workload running on a 80VCPU/512G guest (~80% busy)
>>>
>>>   Sample results with SpecJbb2005 workload : (migrate speed set to 20Gb and
>>>   migrate downtime set to 4seconds).
>> Would it make sense to separate out the "slow the VCPU down" part of
>> this?
>>
>> That would give a management tool more flexibility to create policies
>> around slowing the VCPU down to encourage migration.
>
> I believe one can always enhance libvirt tools to monitor the migration 
> statistics and control the shares/entitlements of the vcpus via 
> cgroups..thereby slowing the guest down to allow for convergence  (I had 
> that listed in my earlier versions of the patches as an option and also 
> noted that it requires external (i.e. tool driven) monitoring and 
> triggers...and that this alternative was kind of automatic after the 
> initial setting of the capability).
>
> Is that what you meant by your comment above (or) are you talking about 
> something outside the scope of cgroups and from an implementation point 
> of view also outside the migration code path...i.e. a new knob that an 
> external tool can use to just throttle down the vcpus of a guest ?

I'm saying, a knob to throttle the guest vcpus within QEMU that could be
used by management tools to encourage convergence.

For instance, consider an imaginary "vcpu_throttle" command that took a
number between 0 and 1 that throttled VCPU performance accordingly.

Then migration would look like:

0) throttle = 1.0
1) call migrate command to start migration
2) query progress until you decide you aren't converging
3) throttle *= 0.75; call vcpu_throttle $throttle
4) goto (2)

Now I'm not opposed to a series like this that adds this sort of policy
to QEMU itself too but I want to make sure the pieces are exposed for a
management tool to implement its own policies too.

Regards,

Anthony Liguori

>
> Thanks
> Vinod
>
>
>
>>
>> In fact, I wonder if we need anything in the migration path if we just
>> expose the "slow the VCPU down" bit as a feature.
>>
>> Slow the VCPU down is not quite the same as setting priority of the VCPU
>> thread largely because of the QBL so I recognize the need to have
>> something for this in QEMU.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>>   (qemu) info migrate
>>>   capabilities: xbzrle: off auto-converge: off  <
>>>   Migration status: active
>>>   total time: 1487503 milliseconds
>>>   expected downtime: 519 milliseconds
>>>   transferred ram: 383749347 kbytes
>>>   remaining ram: 2753372 kbytes
>>>   total ram: 268444224 kbytes
>>>   duplicate: 65461532 pages
>>>   skipped: 64901568 pages
>>>   normal: 95750218 pages
>>>   normal bytes: 383000872 kbytes
>>>   dirty pages rate: 67551 pages
>>>
>>>   ---
>>>   
>>>   (qemu) info migrate
>>>   capabilities: xbzrle: off auto-converge: on   <
>>>   Migration status: completed
>>>   total time: 241161 milliseconds
>>>   downtime: 6373 milliseconds
>>>   transferred ram: 28235307 kbytes
>>>   remaining ram: 0 kbytes
>>>   total ram: 268444224 kbytes
>>>   duplicate: 64946416 pages
>>>   skipped: 64903523 pages
>>>   normal: 7044971 pages
>>>   normal bytes: 28179884 kbytes
>>>
>>> Signed-off-by: Chegu Vinod 
>>> ---
>>>   arch_init.c   |   68 
>>> +
>>>   include/migration/migration.h |4 ++
>>>   migration.c   |1 +
>>>   3 files changed, 73 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch_init.c b/arch_init.c
>>> index 49c5dc2..29788d6 100644
>>> --- a/arch_init.c
>>> +++ b/arch_init.c
>>> @@ -49,6 +49,7 @@
>>>   #include "trace.h"
>>>   #include "exec/cpu-all.h"
>>>   #include "hw/acpi/acpi.h"
>>> +#include "sysemu/cpus.h"
>>>   
>>>   #ifdef DEBUG_ARCH_INIT
>>>   #define DPRINTF(fmt, ...) \
>>> @@ -104,6 +105,8 @@ int graphic_depth = 15;
>>>   #endif
>>>   
>>>   const uint32_t arch_type = QEMU_ARCH;
>>> +static bool mig_throttle_on;
>>> +
>>>   
>>>   /***/
>>>   /* ram save/restore */
>>> @@ -378,8 +381,15 @@ static void migration_bitmap_sync(void)
>>>   uint64_t num_dirty_pages_init = migration_dirty_pages;
>>>   MigrationState *s = migrate_get_current();
>>>   static int64_t start_time;
>>> +static int64_t bytes_xfer_prev;
>>>   static int64_t num_dirty_pages_period;
>>>   int64_t end_time;
>>> +int64_t bytes_xfer_now;
>>> +static int dirty_rate_high_cnt;
>>> +
>>> +if (!bytes_xfer_prev) {
>>> +   

Re: [Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Paolo Bonzini]

Il 10/05/2013 17:06, Anthony Liguori ha scritto:
> Andreas Färber  writes:
> 
>> A transition from CPUPPCState to PowerPCCPU can be considered safe,
>> just like PowerPCCPU::env access in the opposite direction.
>>
>> This should slightly improve interrupt performance.
>>
>> Reported-by: Anthony Liguori 
>> Signed-off-by: Andreas Färber 
> 
> Another option would be to leave it and do something like:

... patch 3 in my series. ;)

Paolo

Re: [Qemu-devel] [RFC PATCH v5 3/3] Force auto-convegence of live migration

[Anthony Liguori]

"Daniel P. Berrange"  writes:

> On Fri, May 10, 2013 at 08:07:51AM -0500, Anthony Liguori wrote:
>> Chegu Vinod  writes:
>> 
>> >  If a user chooses to turn on the auto-converge migration capability
>> >  these changes detect the lack of convergence and throttle down the
>> >  guest. i.e. force the VCPUs out of the guest for some duration
>> >  and let the migration thread catchup and help converge.
>> >
>> >  Verified the convergence using the following :
>> >  - SpecJbb2005 workload running on a 20VCPU/256G guest(~80% busy)
>> >  - OLTP like workload running on a 80VCPU/512G guest (~80% busy)
>> >
>> >  Sample results with SpecJbb2005 workload : (migrate speed set to 20Gb and
>> >  migrate downtime set to 4seconds).
>> 
>> Would it make sense to separate out the "slow the VCPU down" part of
>> this?
>> 
>> That would give a management tool more flexibility to create policies
>> around slowing the VCPU down to encourage migration.
>> 
>> In fact, I wonder if we need anything in the migration path if we just
>> expose the "slow the VCPU down" bit as a feature.
>> 
>> Slow the VCPU down is not quite the same as setting priority of the VCPU
>> thread largely because of the QBL so I recognize the need to have
>> something for this in QEMU.
>
> Rather than the priority, could you perhaps do the VCPU slow down
> using  cfs_quota_us + cfs_period_us settings though ? These let you
> place hard caps on schedular time afforded to vCPUs and we can already
> control those via libvirt + cgroups.

The problem with the bandwidth controller is the same with priorities.
You can end up causing lock holder pre-emption which would negatively
impact migration performance.

It's far better for QEMU to voluntarily give up some time knowing that
it's not holding the QBL since then migration can continue without
impact.

Regards,

Anthony Liguori

>
> 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] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Paolo Bonzini]

Il 10/05/2013 16:47, Andreas Färber ha scritto:
> Am 10.05.2013 16:42, schrieb Peter Maydell:
>> On 10 May 2013 15:39, Andreas Färber  wrote:
>>> A transition from CPUPPCState to PowerPCCPU can be considered safe,
>>> just like PowerPCCPU::env access in the opposite direction.
>>>
>>> This should slightly improve interrupt performance.
>>
>>>  static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
>>>  {
>>> -return POWERPC_CPU(container_of(env, PowerPCCPU, env));
>>> +return container_of(env, PowerPCCPU, env);
>>>  }

This doesn't fix the same in problem in ENV_GET_CPU.  It would fix half
of the problem, but not the other half.

I don't want to think in advance of what casts are in hot paths.  The
less I think about such useless details, the less bug I will put in my
code.  I _want_ to code defensively (and have gdb pinpoint problems fast
while developing), I just don't want that to come at a performance cost
for users.

Algorithmic invariants are what you need to assert against.  As far as
type-safety is concerned, people are used to SIGSEGVs and it doesn't
change anything to them if they get SIGABRTs instead.  *If* the
type-safety is a major concern, at least patch 4 of my series should be
a no brainer.  And with the tracing support introduced by patch 5, the
price of having a little less type-safety should be more than acceptable.

>> So if this is worthwhile shouldn't we be doing it for
>> all our CPUs?
> 
> I thought ppc were the exception, but you're right there's 15
> occurrences remaining, i.e. all targets do it that way currently.
> 
> Don't have time right now for large cross-tree cleanups, so feel free to
> profile with and without this patch.

Well, we have a week to get this in.

Paolo

Re: [Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Anthony Liguori]

Andreas Färber  writes:

> A transition from CPUPPCState to PowerPCCPU can be considered safe,
> just like PowerPCCPU::env access in the opposite direction.
>
> This should slightly improve interrupt performance.
>
> Reported-by: Anthony Liguori 
> Signed-off-by: Andreas Färber 

Another option would be to leave it and do something like:

diff --git a/qom/object.c b/qom/object.c
index 75e6aac..cba1d88 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -126,8 +126,13 @@ static TypeImpl *type_register_internal(const TypeInfo 
*info)
 
 TypeImpl *type_register(const TypeInfo *info)
 {
+TypeImpl *impl;
+
 assert(info->parent);
-return type_register_internal(info);
+impl = type_register_internal(info);
+g_free(impl->name);
+impl->name = info->name;
+return impl;
 }
 
 TypeImpl *type_register_static(const TypeInfo *info)
@@ -449,10 +490,16 @@ Object *object_dynamic_cast_assert(Object *obj, const 
char *typename)
 ObjectClass *object_class_dynamic_cast(ObjectClass *class,
const char *typename)
 {
-TypeImpl *target_type = type_get_by_name(typename);
+TypeImpl *target_type;
 TypeImpl *type = class->type;
 ObjectClass *ret = NULL;
 
+if (type->name == typename) {
+return class;
+}
+
+target_type = type_get_by_name(typename);
+
 if (!target_type) {
 /* target class type unknown, so fail the cast */
 return NULL;

Which makes casting an object to it's concrete class free.

Regards,

Anthony Liguori

> ---
>  target-ppc/cpu-qom.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
> index eb03a00..f62181f 100644
> --- a/target-ppc/cpu-qom.h
> +++ b/target-ppc/cpu-qom.h
> @@ -91,7 +91,7 @@ typedef struct PowerPCCPU {
>  
>  static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
>  {
> -return POWERPC_CPU(container_of(env, PowerPCCPU, env));
> +return container_of(env, PowerPCCPU, env);
>  }
>  
>  #define ENV_GET_CPU(e) CPU(ppc_env_get_cpu(e))
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Andreas Färber]

Am 10.05.2013 16:46, schrieb Aurelien Jarno:
> We have changed a pointer access in a hot path (and more are likely to
> come as far as I know) to a complex function. The correct way to fix
> that is to remove the complex function.

FWIW I had started to investigate whether I can tweak TCG to use
CPUFooState for its offset calculations as-is but to call helpers with
FooCPU to get code in a uniform shape, not constantly coding casts as
first thing in helper functions. We'd need a pointer similar to
CPUState::env_ptr in the back part of CPU_COMMON, make FooCPU available
as ArchCPU define and tweak the helper templates to support cpu type
argument in addition to env. So far just an idea.

Andreas

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

Re: [Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Anthony Liguori]

Peter Maydell  writes:

> On 10 May 2013 15:39, Andreas Färber  wrote:
>> A transition from CPUPPCState to PowerPCCPU can be considered safe,
>> just like PowerPCCPU::env access in the opposite direction.
>>
>> This should slightly improve interrupt performance.
>
>>  static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
>>  {
>> -return POWERPC_CPU(container_of(env, PowerPCCPU, env));
>> +return container_of(env, PowerPCCPU, env);
>>  }
>
> So if this is worthwhile shouldn't we be doing it for
> all our CPUs?

Ack.

Regards,

Anthony Liguori

>
> thanks
> -- PMM

[Qemu-devel] [PATCH] ui/gtk.c: do not use gdk_display_warp_pointer when GTK ver >3.0

[Igor Mitsyanko]

Commit 9697f5d2d38e5dd1e64e8e0d64436e6d44e7b1fe "gtk: custom cursor support"
introduced unconditional usage of gdk_display_warp_pointer(). This function
is marked as deprecated since GTK-3.0, and triggers warning (error with -Werror)
during compilation.
Conditionally change gdk_display_warp_pointer() method usage to gdk_device_warp
usage, as suggested by compiler.

Signed-off-by: Igor Mitsyanko 
---
 ui/gtk.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/ui/gtk.c b/ui/gtk.c
index e12f228..841f912 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -331,6 +331,24 @@ static void gd_refresh(DisplayChangeListener *dcl)
 graphic_hw_update(dcl->con);
 }
 
+#if GTK_CHECK_VERSION(3, 0, 0)
+static void gd_mouse_set(DisplayChangeListener *dcl,
+ int x, int y, int visible)
+{
+GtkDisplayState *s = container_of(dcl, GtkDisplayState, dcl);
+GdkDisplay *dpy;
+GdkDeviceManager *mgr;
+gint x_root, y_root;
+
+dpy = gtk_widget_get_display(s->drawing_area);
+mgr = gdk_display_get_device_manager(dpy);
+gdk_window_get_root_coords(gtk_widget_get_window(s->drawing_area),
+   x, y, &x_root, &y_root);
+gdk_device_warp(gdk_device_manager_get_client_pointer(mgr),
+gtk_widget_get_screen(s->drawing_area),
+x, y);
+}
+#else
 static void gd_mouse_set(DisplayChangeListener *dcl,
  int x, int y, int visible)
 {
@@ -343,6 +361,7 @@ static void gd_mouse_set(DisplayChangeListener *dcl,
  gtk_widget_get_screen(s->drawing_area),
  x_root, y_root);
 }
+#endif
 
 static void gd_cursor_define(DisplayChangeListener *dcl,
  QEMUCursor *c)
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Paolo Bonzini]

Il 10/05/2013 16:39, Anthony Liguori ha scritto:
> I just oppose the notion of disabling casts and *especially* only
> disabling casts for official builds.

This actually happens all the time.  Exactly this kind of type-safe cast
is disabled in releases of GCC, but enabled when building from svn trunk.

I have hardly seen any of these failures _during development_, much less
on a release.  I appreciate the advantage of type-safe casts, but in
QEMU they are a solution in search of a problem.  They are cheap to
implement (though not that cheap to execute ;)) so it's perfectly fine
to have them, but they are not _needed_; disabling them is anyway a good
build-time option to have.

Type-safe casts make sense in GTK+/GObject where: 1) type-safe casts
return NULL and log a "critical" error, they do not abort; 2) all
functions fail with another "critical" error if they are passed NULL.
We do neither, so we're just trading a crash now for a crash very soon
after (our call stacks tend to be relatively shallow, with some
exceptions such as the block layer).

Also, in GTK+/GObject the code paths are unpredictable because they
depend on user interaction, and a crash can lead to data loss.  By
contrast, in QEMU most of the code is hardly ever run, and the possible
paths are very few because driver writers tend to use always the same
path.  The day someone is bringing up a new guest OS and encounters such
a crash, we'll tell them to either build from git, or to use
--enable-qom-cast-debug.

I'm sure it will be a long time before that happens...

Paolo

> Regards,
> 
> Anthony Liguori
> 
>>> Either way, it would be nice to see the call sites of those
>>> most-impacting dynamic casts! So far I held back my APIC RFC since I'm
>>> not sure how to reproducibly profile things.
>>
>> It's interrupts (both sending and returning from them).
>>
>> Paolo
> 
> 
> 

Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values

[mdroth]

On Fri, May 10, 2013 at 08:08:05AM -0600, Eric Blake wrote:
> On 05/10/2013 06:47 AM, Laszlo Ersek wrote:
> 
> > The pre-patch code for JSON_INTEGER:
> > 
> > obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 10)));
> > 
> > doesn't check for errors at all. (I assume that JSON_INTEGER is selected
> > by the parser, token_get_type(), based on syntax purely.)
> > 
> > I thought when the pre-patch version encounters an int-looking decimal
> > string that's actually too big in magnitude for an int, you'd simply end
> > up with LLONG_MIN or LLONG_MAX, but no error. strtoll() clamps the
> > value, errno is lost, and qint_from_int() sees nothing wrong.
> 
> Oh, right.  _That's_ why libvirt had to add checks that it wasn't
> passing 0x8000ULL as a positive number - because the qemu
> parser was silently clamping it to 0x7fffLL, which is not
> what libvirt wanted.  So the code was NOT erroring out with an overflow
> message, but was acting on the wrong integer.
> 
> > 
> > With the patch, you end up with a float instead of an int-typed
> > LLONG_MIN/LLONG_MAX, and also no error.
> 
> Ah, but here we have a difference - beforehand, the code was passing a
> valid (albeit wrong value) qint, so the rest of the qemu code was
> oblivious to the fact that the QMP message contained an overflow.  But
> now the code is passing a qdouble, and the rest of the qemu code may be
> unprepared to handle it when expecting a qint.

Yup, new error cases can be triggered, but in the case of
QmpInputVisitor this is handled appropriately (will add a test case to
confirm), and none of our other input visitors act on QObjects, and this
ambiguity isn't present for output visitors.

We also have monitor events that call qobject_from_json() to marshall
event payloads, but these are essentially open-coded QmpInputVisitors
where the JSON values come from native C types. The only case where I
can see this triggering the change is if they did something like:

  obj = qobject_from_jsonf("{'myInt': %f}", whole_valued_float);

which would be evil, and thankfully such cases don't appear to exist:

mdroth@loki:~/w/qemu.git$ ack-grep qobject_from_json | grep "%f"
tests/check-qjson.c:987:obj = qobject_from_jsonf("%f", valuef);
mdroth@loki:~/w/qemu.git$

(the 'valuef' above is not whole-valued, and the output is expected to
be a QFloat)

I'm not aware of any other cases to consider, but I might've missed
something.

> 
> > 
> >> At any rate, libvirt already checks that all numbers that fall outside
> >> the range of int64_t are never passed over qmp when passing an int
> >> argument (and yes, this is annoying, in that large 64-bit unsigned
> >> numbers have to be passed as negative numbers, rather than exceeding
> >> INT64_MAX), so libvirt should not be triggering this newly exposed code
> >> path.  But even if libvirt doesn't plan on triggering it, I'd still feel
> >> better if your commit message documented evidence of testing what
> >> happens in this case.  For example, compare what
> >> {"execute":"add-fd","arguments":{"fdset-id":""}}
> >> does before and after this patch.
> > 
> > That would be likely interesting to test, yes.
> 
> add-fd may not be the best candidate (it expects an fd to be passed at
> the same time, and does its own checking that it does not get a negative
> number); but I'm sure there's plenty of other candidates (add-cpu is
> another possibility that comes quickly to mind) - basically, pick a
> command that takes an explicit 'int' argument, and overflow that
> argument to see what happens when the command now has to deal with a
> qdouble.

Command params will end up getting marshalled in QObject prior to being
passed into commands:

mi = qmp_input_visitor_new_strict(QOBJECT(args));
v = qmp_input_get_visitor(mi);
visit_start_optional(v, &has_fdset_id, "fdset-id", errp);
if (has_fdset_id) {
visit_type_int(v, &fdset_id, "fdset-id", errp);
}
visit_end_optional(v, errp);
visit_start_optional(v, &has_opaque, "opaque", errp);
if (has_opaque) {
visit_type_str(v, &opaque, "opaque", errp);
}
visit_end_optional(v, errp);
qmp_input_visitor_cleanup(mi);

if (error_is_set(errp)) {
goto out;
}
retval = qmp_add_fd(has_fdset_id, fdset_id, has_opaque, opaque, errp);

so i think a check in tests-qmp-input-visitor that verifies that values that
exceed LLONG_MAX/LLONG_MIN will get added into the QObject as QFloats
and trigger a type error when being passed to visit_type_int() should
cover the cases in question.

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

Re: [Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Andreas Färber]

Am 10.05.2013 16:42, schrieb Peter Maydell:
> On 10 May 2013 15:39, Andreas Färber  wrote:
>> A transition from CPUPPCState to PowerPCCPU can be considered safe,
>> just like PowerPCCPU::env access in the opposite direction.
>>
>> This should slightly improve interrupt performance.
> 
>>  static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
>>  {
>> -return POWERPC_CPU(container_of(env, PowerPCCPU, env));
>> +return container_of(env, PowerPCCPU, env);
>>  }
> 
> So if this is worthwhile shouldn't we be doing it for
> all our CPUs?

I thought ppc were the exception, but you're right there's 15
occurrences remaining, i.e. all targets do it that way currently.

Don't have time right now for large cross-tree cleanups, so feel free to
profile with and without this patch.

Andreas

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

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Aurelien Jarno]

On Fri, May 10, 2013 at 09:27:28AM -0500, Anthony Liguori wrote:
> Paolo Bonzini  writes:
> 
> > Il 10/05/2013 15:01, Anthony Liguori ha scritto:
> >> Paolo Bonzini  writes:
> >> 
> >>> Cast debugging can have a substantial cost (20% or more, measured by
> >>> Aurelien on qemu-system-ppc64).
> >> 
> >> [Needs citation]
> >
> > Sure: http://permalink.gmane.org/gmane.comp.emulators.qemu/210830
> >
> >  49,73%  perf-10672.map   [.] 0x7f7853ab4e0f
> >  13,23%  qemu-system-ppc64[.] cpu_ppc_exec
> >  13,16%  libglib-2.0.so.0.3200.4  [.] g_hash_table_lookup
> >   8,18%  libglib-2.0.so.0.3200.4  [.] g_str_hash
> >   2,47%  qemu-system-ppc64[.] object_class_dynamic_cast
> >   1,97%  qemu-system-ppc64[.] type_is_ancestor
> >   1,05%  libglib-2.0.so.0.3200.4  [.] g_str_equal
> >
> > That's ~27%.
> >
> >>> Instead of adding special-cased "fast
> >>> casts" in the hot paths, we can just disable it in releases.  At the
> >>> same time, add tracing facilities that simplify the analysys of those
> >>> problems that cast debugging would reveal.
> >> 
> >> I'd prefer not to disable but instead focus on improving performance.
> >
> > For 1.5?  This is a regression in 1.5 due to more and more usage of
> > foo_env_on_cpu.
> 
> If this is a regression, then we should be able to show:
> 
> 1) workload XYZ gets X in 1.4 and Y in 1.5
> 
> 2) applying your series demonstrates that we go back to X performance

I will work on that.

> I'm not convinced this is a true set of statements.  13% of time spent
> doing casts could simply suggest that the vcpu is largely idle and has
> nothing better to do.

It's not in KVM, but in the QEMU case. The emulated CPU was fully loaded
during this perf trace, and the qemu-system-ppc64 process was taking
about 100% of the host CPU. This means that this 13% of the time doing
cast can be used to emulate more instructions.

> At any rate, given (1), we can at least try some other less invasive
> approachs to fixing the problem.  Just removing the unnecessary cast
> might end up doing it.

We have changed a pointer access in a hot path (and more are likely to
come as far as I know) to a complex function. The correct way to fix
that is to remove the complex function.

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

Re: [Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Peter Maydell]

On 10 May 2013 15:39, Andreas Färber  wrote:
> A transition from CPUPPCState to PowerPCCPU can be considered safe,
> just like PowerPCCPU::env access in the opposite direction.
>
> This should slightly improve interrupt performance.

>  static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
>  {
> -return POWERPC_CPU(container_of(env, PowerPCCPU, env));
> +return container_of(env, PowerPCCPU, env);
>  }

So if this is worthwhile shouldn't we be doing it for
all our CPUs?

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-1.5] Revert "pc: Kill the "use flash device for BIOS unless KVM" misfeature"

[Anthony Liguori]

Paolo Bonzini  writes:

> This reverts commit 9953f8822cc316eec9962f0a2858c3439a80adec.
> While Markus's analysis is entirely correct, there are 1.6 patches
> that fix the bug for real and without requiring machine type hacks.
> Let's think of the children who will have to read this code, and
> avoid a complicated mess of semantics that differ between <1.5,
> 1.5, and >1.5.
>
> Conflicts:
>   hw/i386/pc_piix.c
>   hw/i386/pc_q35.c
>   include/hw/i386/pc.h
>
> Signed-off-by: Paolo Bonzini 

Acked-by: Anthony Liguori 

I was hestitant to apply this but felt that the new semantics would be
more reasonable.   However, since it looks like we're closer to having
executable flash than I expected we were, I agree that having special
semantics for 1.6 is undesirable.

I'll give Markus a chance to chime in though.

Regards,

Anthony Liguori


> ---
>  hw/block/pc_sysfw.c  | 8 
>  hw/i386/pc_piix.c| 3 ---
>  hw/i386/pc_q35.c | 1 -
>  include/hw/i386/pc.h | 5 -
>  4 files changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/hw/block/pc_sysfw.c b/hw/block/pc_sysfw.c
> index aad8614..4f17668 100644
> --- a/hw/block/pc_sysfw.c
> +++ b/hw/block/pc_sysfw.c
> @@ -209,7 +209,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
>   * TODO This device exists only so that users can switch between
>   * use of flash and ROM for the BIOS.  The ability to switch was
>   * created because flash doesn't work with KVM.  Once it does, we
> - * should drop this device for new machine types.
> + * should drop this device.
>   */
>  sysfw_dev = (PcSysFwDevice*) qdev_create(NULL, "pc-sysfw");
>  
> @@ -226,9 +226,9 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
> Use old rom based firmware initialization for KVM. */
>  /*
>   * This is a Bad Idea, because it makes enabling/disabling KVM
> - * guest-visible.  Do it only in bug-compatibility mode.
> + * guest-visible.  Let's fix it for real in QEMU 1.6.
>   */
> -if (pc_sysfw_flash_vs_rom_bug_compatible && kvm_enabled()) {
> +if (kvm_enabled()) {
>  if (pflash_drv != NULL) {
>  fprintf(stderr, "qemu: pflash cannot be used with kvm 
> enabled\n");
>  exit(1);
> @@ -255,7 +255,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
>  }
>  
>  static Property pcsysfw_properties[] = {
> -DEFINE_PROP_UINT8("rom_only", PcSysFwDevice, rom_only, 1),
> +DEFINE_PROP_UINT8("rom_only", PcSysFwDevice, rom_only, 0),
>  DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index f7c80ad..43ab480 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -248,7 +248,6 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
>  
>  static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
>  {
> -pc_sysfw_flash_vs_rom_bug_compatible = true;
>  has_pvpanic = false;
>  x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
>  pc_init_pci(args);
> @@ -257,7 +256,6 @@ static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
>  static void pc_init_pci_1_3(QEMUMachineInitArgs *args)
>  {
>  enable_compat_apic_id_mode();
> -pc_sysfw_flash_vs_rom_bug_compatible = true;
>  has_pvpanic = false;
>  pc_init_pci(args);
>  }
> @@ -267,7 +265,6 @@ static void pc_init_pci_1_2(QEMUMachineInitArgs *args)
>  {
>  disable_kvm_pv_eoi();
>  enable_compat_apic_id_mode();
> -pc_sysfw_flash_vs_rom_bug_compatible = true;
>  has_pvpanic = false;
>  pc_init_pci(args);
>  }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 4160e2b..dd95921 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -210,7 +210,6 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>  
>  static void pc_q35_init_1_4(QEMUMachineInitArgs *args)
>  {
> -pc_sysfw_flash_vs_rom_bug_compatible = true;
>  has_pvpanic = false;
>  x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
>  pc_q35_init(args);
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 417afe4..2bd7090 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -169,7 +169,6 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, 
> int irq, NICInfo *nd)
>  }
>  
>  /* pc_sysfw.c */
> -extern bool pc_sysfw_flash_vs_rom_bug_compatible;
>  void pc_system_firmware_init(MemoryRegion *rom_memory);
>  
>  /* pvpanic.c */
> @@ -239,10 +238,6 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
>  .property = "romfile",\
>  .value= "pxe-virtio.rom",\
>  },{\
> -.driver   = "pc-sysfw",\
> -.property = "rom_only",\
> -.value= stringify(0),\
> -},{\
>  .driver   = "486-" TYPE_X86_CPU,\
>  .property = "model",\
>  .value= stringify(0),\
> -- 
> 1.8.1.4

[Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Andreas Färber]

A transition from CPUPPCState to PowerPCCPU can be considered safe,
just like PowerPCCPU::env access in the opposite direction.

This should slightly improve interrupt performance.

Reported-by: Anthony Liguori 
Signed-off-by: Andreas Färber 
---
 target-ppc/cpu-qom.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
index eb03a00..f62181f 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -91,7 +91,7 @@ typedef struct PowerPCCPU {
 
 static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
 {
-return POWERPC_CPU(container_of(env, PowerPCCPU, env));
+return container_of(env, PowerPCCPU, env);
 }
 
 #define ENV_GET_CPU(e) CPU(ppc_env_get_cpu(e))
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Anthony Liguori]

Paolo Bonzini  writes:

> Il 10/05/2013 15:23, Andreas Färber ha scritto:
>> Am 10.05.2013 15:08, schrieb Paolo Bonzini:
>>> Il 10/05/2013 15:01, Anthony Liguori ha scritto:
 I'd prefer not to disable but instead focus on improving performance.
>>>
>>> For 1.5?  This is a regression in 1.5 due to more and more usage of
>>> foo_env_on_cpu.
>> 
>> If CPUs were the only reason, we could simply change those inlines and
>> ENV_GET_CPU() macro to use a C cast. No complicated interface scenarios
>> requiring a dynamic cast are used for CPUs so far to my knowledge.
>
> Almost nothing really requires a dynamic cast in QEMU.  Only interface
> casts do, and there's just a couple of uses of interfaces.
>
> And I wrote in the cover letter that what I want is really avoid the
> need for "fast casts" in the hot paths.
>
> Can you guys actually read the commit messages?

Yeah, the vast majority of the series is a nice cleanup.

I just oppose the notion of disabling casts and *especially* only
disabling casts for official builds.

I don't see a reason to do something like this without even attempting
to improve performance first.

Regards,

Anthony Liguori

>> Either way, it would be nice to see the call sites of those
>> most-impacting dynamic casts! So far I held back my APIC RFC since I'm
>> not sure how to reproducibly profile things.
>
> It's interrupts (both sending and returning from them).
>
> Paolo

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Anthony Liguori]

Paolo Bonzini  writes:

> Il 10/05/2013 15:01, Anthony Liguori ha scritto:
>> Paolo Bonzini  writes:
>> 
>>> Cast debugging can have a substantial cost (20% or more, measured by
>>> Aurelien on qemu-system-ppc64).
>> 
>> [Needs citation]
>
> Sure: http://permalink.gmane.org/gmane.comp.emulators.qemu/210830
>
>  49,73%  perf-10672.map   [.] 0x7f7853ab4e0f
>  13,23%  qemu-system-ppc64[.] cpu_ppc_exec
>  13,16%  libglib-2.0.so.0.3200.4  [.] g_hash_table_lookup
>   8,18%  libglib-2.0.so.0.3200.4  [.] g_str_hash
>   2,47%  qemu-system-ppc64[.] object_class_dynamic_cast
>   1,97%  qemu-system-ppc64[.] type_is_ancestor
>   1,05%  libglib-2.0.so.0.3200.4  [.] g_str_equal
>
> That's ~27%.
>
>>> Instead of adding special-cased "fast
>>> casts" in the hot paths, we can just disable it in releases.  At the
>>> same time, add tracing facilities that simplify the analysys of those
>>> problems that cast debugging would reveal.
>> 
>> I'd prefer not to disable but instead focus on improving performance.
>
> For 1.5?  This is a regression in 1.5 due to more and more usage of
> foo_env_on_cpu.

If this is a regression, then we should be able to show:

1) workload XYZ gets X in 1.4 and Y in 1.5

2) applying your series demonstrates that we go back to X performance

I'm not convinced this is a true set of statements.  13% of time spent
doing casts could simply suggest that the vcpu is largely idle and has
nothing better to do.

At any rate, given (1), we can at least try some other less invasive
approachs to fixing the problem.  Just removing the unnecessary cast
might end up doing it.

Regards,

Anthony Liguori

>
> Paolo
>
>> I suspect any performance overhead is resolving the type string to
>> typeimpl.  One work around is to have a dynamic cast that takes a
>> typeimpl and then use a function that returns a static similar to how
>> glib works.
>> 
>> If you've got a reproducible case where the overhead is high, it should
>> be easy to check.
>> 
>> Regards,
>> 
>> Anthony Liguori
>> 
>>>
>>> At least patches 1-7 are for 1.5.
>>>
>>> Paolo Bonzini (9):
>>>   qom: improve documentation of cast functions
>>>   qom: allow casting of a NULL class
>>>   qom: add a fast path to object_class_dynamic_cast
>>>   qom: pass file/line/function to asserting casts
>>>   qom: trace asserting casts
>>>   qom: allow turning cast debugging off
>>>   build: disable QOM cast debugging for official releases
>>>   qom: simplify object_class_dynamic_cast, part 1
>>>   qom: simplify object_class_dynamic_cast, part 2
>>>
>>>  configure| 20 --
>>>  include/qom/object.h | 40 ++-
>>>  qom/object.c | 77 
>>> ++--
>>>  trace-events |  3 ++
>>>  4 files changed, 99 insertions(+), 41 deletions(-)
>>>
>>> -- 
>>> 1.8.1.4
>> 

Re: [Qemu-devel] [RFC PATCH v5 2/3] Add 'auto-converge' migration capability

[Eric Blake]

On 05/10/2013 01:43 AM, Paolo Bonzini wrote:
>> +++ b/qapi-schema.json
>> @@ -602,10 +602,13 @@
>>  #  This feature allows us to minimize migration traffic for certain 
>> work
>>  #  loads, by sending compressed difference of the pages
>>  #
>> +# @auto-converge: Migration supports automatic throttling down of guest
>> +#  to force convergence. (since 1.6)
> 
> If enabled, QEMU will automatically throttle down the guest to speed up
> convergence of RAM migration.

Ooh, I do like Paolo's wording better than mine.  But either one is
reasonable, so feel free to add:

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 04/10] qapi: enable generation of native list code

[Luiz Capitulino]

On Thu,  9 May 2013 21:20:56 -0500
Michael Roth  wrote:

> Also, fix a dependency issue with libqemuutil: qemu-sockets.c needs
> qapi-types.c/qapi-visit.c
> 
> Signed-off-by: Michael Roth 
> ---
>  Makefile |6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 7dc0204..9695c9d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -178,7 +178,7 @@ Makefile: $(version-obj-y) $(version-lobj-y)
>  # Build libraries
>  
>  libqemustub.a: $(stub-obj-y)
> -libqemuutil.a: $(util-obj-y)
> +libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o

Don't we want this in for 1.5?

>  
>  ##
>  
> @@ -215,10 +215,10 @@ $(SRC_PATH)/qga/qapi-schema.json 
> $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
>  
>  qapi-types.c qapi-types.h :\
>  $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py 
> $(gen-out-type) -o "." < $<, "  GEN   $@")
> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py 
> $(gen-out-type) -o "." -b < $<, "  GEN   $@")
>  qapi-visit.c qapi-visit.h :\
>  $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
> - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py 
> $(gen-out-type) -o "."  < $<, "  GEN   $@")
> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py 
> $(gen-out-type) -o "." -b < $<, "  GEN   $@")
>  qmp-commands.h qmp-marshal.c :\
>  $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
>   $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py 
> $(gen-out-type) -m -o "." < $<, "  GEN   $@")

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Aurelien Jarno]

On Fri, May 10, 2013 at 03:30:17PM +0200, Paolo Bonzini wrote:
> Il 10/05/2013 15:23, Andreas Färber ha scritto:
> > Am 10.05.2013 15:08, schrieb Paolo Bonzini:
> >> Il 10/05/2013 15:01, Anthony Liguori ha scritto:
> >>> I'd prefer not to disable but instead focus on improving performance.
> >>
> >> For 1.5?  This is a regression in 1.5 due to more and more usage of
> >> foo_env_on_cpu.
> > 
> > If CPUs were the only reason, we could simply change those inlines and
> > ENV_GET_CPU() macro to use a C cast. No complicated interface scenarios
> > requiring a dynamic cast are used for CPUs so far to my knowledge.
> 
> Almost nothing really requires a dynamic cast in QEMU.  Only interface
> casts do, and there's just a couple of uses of interfaces.
> 
> And I wrote in the cover letter that what I want is really avoid the
> need for "fast casts" in the hot paths.
> 
> Can you guys actually read the commit messages?
> 
> > Either way, it would be nice to see the call sites of those
> > most-impacting dynamic casts! So far I held back my APIC RFC since I'm
> > not sure how to reproducibly profile things.
> 
> It's interrupts (both sending and returning from them).
> 

More precisely in target-ppc/excp_helper.c:
- in ppc_hw_interrupt
- in helper_store_msr
- in do_rfi (called from helper_rfi)
- in helper_msgsnd

Sot it's at least twice per interruption, which means doing hash table
lookup and string comparison through glib a few hundred to a few
thousand times per second. Before the QOMification, it was a simple
pointer access.

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

Re: [Qemu-devel] Profiling sparc64 emulation

[Anthony Liguori]

Paolo Bonzini  writes:

> Il 09/05/2013 20:30, Aurelien Jarno ha scritto:
>>  13,16%  libglib-2.0.so.0.3200.4  [.] g_hash_table_lookup
>>   8,18%  libglib-2.0.so.0.3200.4  [.] g_str_hash
>>   2,47%  qemu-system-ppc64[.] object_class_dynamic_cast
>>   1,97%  qemu-system-ppc64[.] type_is_ancestor
>
> That's worrisome, but should be easy to fix... can you make a callgraph
> profile?

So percentage of a profiling run doesn't imply a performance regression.

Are there real performance numbers here?

Regards,

Anthony Liguori

>
> Paolo

Re: [Qemu-devel] [RFC PATCH v5 3/3] Force auto-convegence of live migration

[Daniel P. Berrange]

On Fri, May 10, 2013 at 08:07:51AM -0500, Anthony Liguori wrote:
> Chegu Vinod  writes:
> 
> >  If a user chooses to turn on the auto-converge migration capability
> >  these changes detect the lack of convergence and throttle down the
> >  guest. i.e. force the VCPUs out of the guest for some duration
> >  and let the migration thread catchup and help converge.
> >
> >  Verified the convergence using the following :
> >  - SpecJbb2005 workload running on a 20VCPU/256G guest(~80% busy)
> >  - OLTP like workload running on a 80VCPU/512G guest (~80% busy)
> >
> >  Sample results with SpecJbb2005 workload : (migrate speed set to 20Gb and
> >  migrate downtime set to 4seconds).
> 
> Would it make sense to separate out the "slow the VCPU down" part of
> this?
> 
> That would give a management tool more flexibility to create policies
> around slowing the VCPU down to encourage migration.
> 
> In fact, I wonder if we need anything in the migration path if we just
> expose the "slow the VCPU down" bit as a feature.
> 
> Slow the VCPU down is not quite the same as setting priority of the VCPU
> thread largely because of the QBL so I recognize the need to have
> something for this in QEMU.

Rather than the priority, could you perhaps do the VCPU slow down
using  cfs_quota_us + cfs_period_us settings though ? These let you
place hard caps on schedular time afforded to vCPUs and we can already
control those via libvirt + cgroups.

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 PATCH v5 3/3] Force auto-convegence of live migration

[Chegu Vinod]

On 5/10/2013 6:07 AM, Anthony Liguori wrote:

Chegu Vinod  writes:


  If a user chooses to turn on the auto-converge migration capability
  these changes detect the lack of convergence and throttle down the
  guest. i.e. force the VCPUs out of the guest for some duration
  and let the migration thread catchup and help converge.

  Verified the convergence using the following :
  - SpecJbb2005 workload running on a 20VCPU/256G guest(~80% busy)
  - OLTP like workload running on a 80VCPU/512G guest (~80% busy)

  Sample results with SpecJbb2005 workload : (migrate speed set to 20Gb and
  migrate downtime set to 4seconds).

Would it make sense to separate out the "slow the VCPU down" part of
this?

That would give a management tool more flexibility to create policies
around slowing the VCPU down to encourage migration.


I believe one can always enhance libvirt tools to monitor the migration 
statistics and control the shares/entitlements of the vcpus via 
cgroups..thereby slowing the guest down to allow for convergence  (I had 
that listed in my earlier versions of the patches as an option and also 
noted that it requires external (i.e. tool driven) monitoring and 
triggers...and that this alternative was kind of automatic after the 
initial setting of the capability).


Is that what you meant by your comment above (or) are you talking about 
something outside the scope of cgroups and from an implementation point 
of view also outside the migration code path...i.e. a new knob that an 
external tool can use to just throttle down the vcpus of a guest ?


Thanks
Vinod





In fact, I wonder if we need anything in the migration path if we just
expose the "slow the VCPU down" bit as a feature.

Slow the VCPU down is not quite the same as setting priority of the VCPU
thread largely because of the QBL so I recognize the need to have
something for this in QEMU.

Regards,

Anthony Liguori


  (qemu) info migrate
  capabilities: xbzrle: off auto-converge: off  <
  Migration status: active
  total time: 1487503 milliseconds
  expected downtime: 519 milliseconds
  transferred ram: 383749347 kbytes
  remaining ram: 2753372 kbytes
  total ram: 268444224 kbytes
  duplicate: 65461532 pages
  skipped: 64901568 pages
  normal: 95750218 pages
  normal bytes: 383000872 kbytes
  dirty pages rate: 67551 pages

  ---
  
  (qemu) info migrate

  capabilities: xbzrle: off auto-converge: on   <
  Migration status: completed
  total time: 241161 milliseconds
  downtime: 6373 milliseconds
  transferred ram: 28235307 kbytes
  remaining ram: 0 kbytes
  total ram: 268444224 kbytes
  duplicate: 64946416 pages
  skipped: 64903523 pages
  normal: 7044971 pages
  normal bytes: 28179884 kbytes

Signed-off-by: Chegu Vinod 
---
  arch_init.c   |   68 +
  include/migration/migration.h |4 ++
  migration.c   |1 +
  3 files changed, 73 insertions(+), 0 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 49c5dc2..29788d6 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -49,6 +49,7 @@
  #include "trace.h"
  #include "exec/cpu-all.h"
  #include "hw/acpi/acpi.h"
+#include "sysemu/cpus.h"
  
  #ifdef DEBUG_ARCH_INIT

  #define DPRINTF(fmt, ...) \
@@ -104,6 +105,8 @@ int graphic_depth = 15;
  #endif
  
  const uint32_t arch_type = QEMU_ARCH;

+static bool mig_throttle_on;
+
  
  /***/

  /* ram save/restore */
@@ -378,8 +381,15 @@ static void migration_bitmap_sync(void)
  uint64_t num_dirty_pages_init = migration_dirty_pages;
  MigrationState *s = migrate_get_current();
  static int64_t start_time;
+static int64_t bytes_xfer_prev;
  static int64_t num_dirty_pages_period;
  int64_t end_time;
+int64_t bytes_xfer_now;
+static int dirty_rate_high_cnt;
+
+if (!bytes_xfer_prev) {
+bytes_xfer_prev = ram_bytes_transferred();
+}
  
  if (!start_time) {

  start_time = qemu_get_clock_ms(rt_clock);
@@ -404,6 +414,23 @@ static void migration_bitmap_sync(void)
  
  /* more than 1 second = 1000 millisecons */

  if (end_time > start_time + 1000) {
+if (migrate_auto_converge()) {
+/* The following detection logic can be refined later. For now:
+   Check to see if the dirtied bytes is 50% more than the approx.
+   amount of bytes that just got transferred since the last time we
+   were in this routine. If that happens N times (for now N==5)
+   we turn on the throttle down logic */
+bytes_xfer_now = ram_bytes_transferred();
+if (s->dirty_pages_rate &&
+((num_dirty_pages_period*TARGET_PAGE_SIZE) >
+((bytes_xfer_now - bytes_xfer_prev)/2))) {
+if (dirty_rate_high_cnt++ > 5) {
+DPRINTF("Unable to converge. Throtting down guest\n");
+mig_throttle_on = true;
+}
+

Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values

[Eric Blake]

On 05/10/2013 06:47 AM, Laszlo Ersek wrote:

> The pre-patch code for JSON_INTEGER:
> 
> obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 10)));
> 
> doesn't check for errors at all. (I assume that JSON_INTEGER is selected
> by the parser, token_get_type(), based on syntax purely.)
> 
> I thought when the pre-patch version encounters an int-looking decimal
> string that's actually too big in magnitude for an int, you'd simply end
> up with LLONG_MIN or LLONG_MAX, but no error. strtoll() clamps the
> value, errno is lost, and qint_from_int() sees nothing wrong.

Oh, right.  _That's_ why libvirt had to add checks that it wasn't
passing 0x8000ULL as a positive number - because the qemu
parser was silently clamping it to 0x7fffLL, which is not
what libvirt wanted.  So the code was NOT erroring out with an overflow
message, but was acting on the wrong integer.

> 
> With the patch, you end up with a float instead of an int-typed
> LLONG_MIN/LLONG_MAX, and also no error.

Ah, but here we have a difference - beforehand, the code was passing a
valid (albeit wrong value) qint, so the rest of the qemu code was
oblivious to the fact that the QMP message contained an overflow.  But
now the code is passing a qdouble, and the rest of the qemu code may be
unprepared to handle it when expecting a qint.

> 
>> At any rate, libvirt already checks that all numbers that fall outside
>> the range of int64_t are never passed over qmp when passing an int
>> argument (and yes, this is annoying, in that large 64-bit unsigned
>> numbers have to be passed as negative numbers, rather than exceeding
>> INT64_MAX), so libvirt should not be triggering this newly exposed code
>> path.  But even if libvirt doesn't plan on triggering it, I'd still feel
>> better if your commit message documented evidence of testing what
>> happens in this case.  For example, compare what
>> {"execute":"add-fd","arguments":{"fdset-id":""}}
>> does before and after this patch.
> 
> That would be likely interesting to test, yes.

add-fd may not be the best candidate (it expects an fd to be passed at
the same time, and does its own checking that it does not get a negative
number); but I'm sure there's plenty of other candidates (add-cpu is
another possibility that comes quickly to mind) - basically, pick a
command that takes an explicit 'int' argument, and overflow that
argument to see what happens when the command now has to deal with a
qdouble.

-- 
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 01/10] qapi: qapi-types.py, native list support

[Luiz Capitulino]

On Thu,  9 May 2013 21:20:53 -0500
Michael Roth  wrote:

> Teach type generators about native types so they can generate the
> appropriate linked list types.
> 
> Signed-off-by: Michael Roth 
> ---
>  scripts/qapi-types.py |   43 ---
>  scripts/qapi.py   |   21 +
>  2 files changed, 61 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 9e19920..96cb26d 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -16,7 +16,18 @@ import os
>  import getopt
>  import errno
>  
> -def generate_fwd_struct(name, members):
> +def generate_fwd_struct(name, members, builtin_type=False):
> +if builtin_type:
> +return mcgen('''
> +typedef struct %(name)sList
> +{
> +%(type)s value;
> +struct %(name)sList *next;
> +} %(name)sList;
> +''',

Sorry for the utterly minor comment, but as you're going to respin please
add a newline before ''' so that we get the declarations properly separated
when generated.

> + type=c_type(name),
> + name=name)
> +
>  return mcgen('''
>  typedef struct %(name)s %(name)s;
>  
> @@ -164,6 +175,7 @@ void qapi_free_%(type)s(%(c_type)s obj);
>  
>  def generate_type_cleanup(name):
>  ret = mcgen('''
> +
>  void qapi_free_%(type)s(%(c_type)s obj)
>  {
>  QapiDeallocVisitor *md;
> @@ -184,8 +196,9 @@ void qapi_free_%(type)s(%(c_type)s obj)
>  
>  
>  try:
> -opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:o:",
> -   ["source", "header", "prefix=", 
> "output-dir="])
> +opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:",
> +   ["source", "header", "builtins",
> +"prefix=", "output-dir="])
>  except getopt.GetoptError, err:
>  print str(err)
>  sys.exit(1)
> @@ -197,6 +210,7 @@ h_file = 'qapi-types.h'
>  
>  do_c = False
>  do_h = False
> +do_builtins = False
>  
>  for o, a in opts:
>  if o in ("-p", "--prefix"):
> @@ -207,6 +221,8 @@ for o, a in opts:
>  do_c = True
>  elif o in ("-h", "--header"):
>  do_h = True
> +elif o in ("-b", "--builtins"):
> +do_builtins = True
>  
>  if not do_c and not do_h:
>  do_c = True
> @@ -282,6 +298,11 @@ fdecl.write(mcgen('''
>  exprs = parse_schema(sys.stdin)
>  exprs = filter(lambda expr: not expr.has_key('gen'), exprs)
>  
> +fdecl.write(guardstart("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
> +for typename in builtin_types:
> +fdecl.write(generate_fwd_struct(typename, None, builtin_type=True))
> +fdecl.write(guardend("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
> +
>  for expr in exprs:
>  ret = "\n"
>  if expr.has_key('type'):
> @@ -298,6 +319,22 @@ for expr in exprs:
>  continue
>  fdecl.write(ret)
>  
> +# to avoid header dependency hell, we always generate declarations
> +# for built-in types in our header files and simply guard them
> +fdecl.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
> +for typename in builtin_types:
> +fdecl.write(generate_type_cleanup_decl(typename + "List"))
> +fdecl.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))

I'm not sure I got why you're doing this. Is it because you're going to
generate them in more .h files? This is a bit ugly :(

> +
> +# ...this doesn't work for cases where we link in multiple objects that
> +# have the functions defined, so we use -b option to provide control
> +# over these cases
> +if do_builtins:
> +fdef.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DEF"))
> +for typename in builtin_types:
> +fdef.write(generate_type_cleanup(typename + "List"))
> +fdef.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DEF"))
> +
>  for expr in exprs:
>  ret = "\n"
>  if expr.has_key('type'):
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index afc5f32..0ac8c2b 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -11,6 +11,10 @@
>  
>  from ordereddict import OrderedDict
>  
> +builtin_types = [
> +'str', 'int', 'number', 'bool'
> +]
> +
>  def tokenize(data):
>  while len(data):
>  ch = data[0]
> @@ -242,3 +246,20 @@ def guardname(filename):
>  for substr in [".", " ", "-"]:
>  guard = guard.replace(substr, "_")
>  return guard.upper() + '_H'
> +
> +def guardstart(name):
> +return mcgen('''
> +
> +#ifndef %(name)s
> +#define %(name)s
> +
> +''',
> + name=guardname(name))
> +
> +def guardend(name):
> +return mcgen('''
> +
> +#endif /* %(name)s */
> +
> +''',
> + name=guardname(name))

Re: [Qemu-devel] [PATCH 20/40] pci: use memory core for iommu support

[Paolo Bonzini]

Il 10/05/2013 15:07, Alexey Kardashevskiy ha scritto:
>> @@ -2234,10 +2239,12 @@ static void pci_device_class_init(ObjectClass 
>> *klass, void *data)
>>  k->props = pci_props;
>>  }
>>  
>> -void pci_setup_iommu(PCIBus *bus, PCIDMAContextFunc fn, void *opaque)
>> +void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, PCIIOMMUDestructorFunc 
>> dtor,
>> + void *opaque)
>>  {
>> -bus->dma_context_fn = fn;
>> -bus->dma_context_opaque = opaque;
>> +bus->iommu_fn = fn;

Or perhaps just use pci_default_iommu if fn == NULL.

Paolo

Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values

[mdroth]

On Fri, May 10, 2013 at 02:47:18PM +0200, Laszlo Ersek wrote:
> On 05/10/13 14:22, Eric Blake wrote:
> > On 05/09/2013 08:20 PM, Michael Roth wrote:
> >> Currently our JSON parser assumes that numbers lacking a mantissa are
> >> integers and attempts to store them as QInt/int64 values. This breaks in
> >> the case where the number overflows/underflows int64 values (which is
> >> still valid JSON)
> >>
> >> Fix this by detecting such cases and using a QFloat to store the value
> >> instead.
> >>
> >> Signed-off-by: Michael Roth 
> >> ---
> >>  qobject/json-parser.c |   26 +++---
> >>  1 file changed, 23 insertions(+), 3 deletions(-)
> > 
> > This changes the error message handed back to QMP clients, and possibly
> > exposes problems in other qemu code that receives the result of json
> > parses.  Previously, for an 'int' argument, if you passed in a too-large
> > number, you got an error that the argument was too large for int.  Now,
> > the number is accepted as a double; are we guaranteed that in a context
> > that expects a qint, when that code is now handed a qfloat (a case which
> > was previously impossible because qint_from_int protected it), that the
> > code will still behave correctly?
> 
> I tried to consider this while reviewing... Maybe I was wrong.
> 
> The pre-patch code for JSON_INTEGER:
> 
> obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 10)));
> 
> doesn't check for errors at all. (I assume that JSON_INTEGER is selected
> by the parser, token_get_type(), based on syntax purely.)
> 
> I thought when the pre-patch version encounters an int-looking decimal
> string that's actually too big in magnitude for an int, you'd simply end
> up with LLONG_MIN or LLONG_MAX, but no error. strtoll() clamps the
> value, errno is lost, and qint_from_int() sees nothing wrong.
> 
> With the patch, you end up with a float instead of an int-typed
> LLONG_MIN/LLONG_MAX, and also no error.
> 

This is correct, but it does make code acting on the resulting QObject
capable of checking for this scenario and acting accordingly in whatever
manner is appropriate. I think that's appropriate, since there's no error
as far as the JSON spec goes, but further up the stack we may have stricter
requirements on what the valid ranges are for various fields.

For instance, in the case of QmpInputVisitor, these cases will
trigger an error case that was previously being bypassed when
out-of-range values were provided in place of 'int' due to the parser
silently capping the max/min values:

in qmp_input_type_int():

QObject *qobj = qmp_input_get_object(qiv, name);

if (!qobj || qobject_type(qobj) != QTYPE_QINT) {
error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
  "integer");
return;
}

We'd now trigger this error for such inputs (since we'd have QTYPE_QFLOAT),
which is stricter checking than before, but as intended since we map int to
int64_t and expect values in that range (though we could stand to be clearer
about this in the QAPI documentation)

> > At any rate, libvirt already checks that all numbers that fall outside
> > the range of int64_t are never passed over qmp when passing an int
> > argument (and yes, this is annoying, in that large 64-bit unsigned
> > numbers have to be passed as negative numbers, rather than exceeding
> > INT64_MAX), so libvirt should not be triggering this newly exposed code
> > path.  But even if libvirt doesn't plan on triggering it, I'd still feel
> > better if your commit message documented evidence of testing what
> > happens in this case.  For example, compare what
> > {"execute":"add-fd","arguments":{"fdset-id":""}}
> > does before and after this patch.
> 
> That would be likely interesting to test, yes.

That would trigger the error mentioned above, as opposed to silently changing
the value to LLONG_MAX. I'll see about covering this case somewhere in
tests/test-qmp-input-visitor.

> 
> Laszlo
> 

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Paolo Bonzini]

Il 10/05/2013 15:23, Andreas Färber ha scritto:
> Am 10.05.2013 15:08, schrieb Paolo Bonzini:
>> Il 10/05/2013 15:01, Anthony Liguori ha scritto:
>>> I'd prefer not to disable but instead focus on improving performance.
>>
>> For 1.5?  This is a regression in 1.5 due to more and more usage of
>> foo_env_on_cpu.
> 
> If CPUs were the only reason, we could simply change those inlines and
> ENV_GET_CPU() macro to use a C cast. No complicated interface scenarios
> requiring a dynamic cast are used for CPUs so far to my knowledge.

Almost nothing really requires a dynamic cast in QEMU.  Only interface
casts do, and there's just a couple of uses of interfaces.

And I wrote in the cover letter that what I want is really avoid the
need for "fast casts" in the hot paths.

Can you guys actually read the commit messages?

> Either way, it would be nice to see the call sites of those
> most-impacting dynamic casts! So far I held back my APIC RFC since I'm
> not sure how to reproducibly profile things.

It's interrupts (both sending and returning from them).

Paolo

Re: [Qemu-devel] [PATCH v2 0/2] qga umask fix addenda

[Luiz Capitulino]

On Wed,  8 May 2013 17:31:34 +0200
Laszlo Ersek  wrote:

> I should have paid more attention to portability and error path cleanup
> in the CVE-2013-2007 fix.
> 
> (We continue to assume, like the rest of qemu code, that
> qemu_set_cloexec() never fails internally. This should be a reasonable
> assumption when the input fd is valid.)

Series:

Reviewed-by: Luiz Capitulino 

> 
> Laszlo Ersek (2):
>   qga: distinguish binary modes in "guest_file_open_modes" map
>   qga: unlink just created guest-file if fchmod() or fdopen() fails on
> it
> 
>  qga/commands-posix.c |   25 +++--
>  1 files changed, 19 insertions(+), 6 deletions(-)
> 
> 

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Andreas Färber]

Am 10.05.2013 15:08, schrieb Paolo Bonzini:
> Il 10/05/2013 15:01, Anthony Liguori ha scritto:
>> I'd prefer not to disable but instead focus on improving performance.
> 
> For 1.5?  This is a regression in 1.5 due to more and more usage of
> foo_env_on_cpu.

If CPUs were the only reason, we could simply change those inlines and
ENV_GET_CPU() macro to use a C cast. No complicated interface scenarios
requiring a dynamic cast are used for CPUs so far to my knowledge.

Either way, it would be nice to see the call sites of those
most-impacting dynamic casts! So far I held back my APIC RFC since I'm
not sure how to reproducibly profile things.

Andreas

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

Re: [Qemu-devel] [PATCH 20/40] pci: use memory core for iommu support

[Alexey Kardashevskiy]

On 05/08/2013 12:16 AM, Paolo Bonzini wrote:
> From: Avi Kivity 
> 
> Use the new iommu support in the memory core for iommu support.  The only
> user, spapr, is also converted, but it still provides a DMAContext
> interface until the non-PCI bits switch to AddressSpace.
> 
> Cc: Michael S. Tsirkin 
> Signed-off-by: Avi Kivity 
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/pci/pci.c |   53 
> ++
>  hw/ppc/spapr_pci.c   |   12 +++---
>  include/hw/pci/pci.h |7 -
>  include/hw/pci/pci_bus.h |5 ++-
>  4 files changed, 46 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 16ed118..3eb397c 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -279,6 +279,16 @@ int pci_find_domain(const PCIBus *bus)
>  return -1;
>  }
>  
> +static MemoryRegion *pci_default_iommu(PCIBus *bus, void *opaque, int devfn)
> +{
> +/* FIXME: inherit memory region from bus creator */
> +return get_system_memory();
> +}
> +
> +static void pci_default_iommu_dtor(MemoryRegion *mr)
> +{
> +}
> +
>  static void pci_bus_init(PCIBus *bus, DeviceState *parent,
>   const char *name,
>   MemoryRegion *address_space_mem,
> @@ -289,6 +299,7 @@ static void pci_bus_init(PCIBus *bus, DeviceState *parent,
>  bus->devfn_min = devfn_min;
>  bus->address_space_mem = address_space_mem;
>  bus->address_space_io = address_space_io;
> +pci_setup_iommu(bus, pci_default_iommu, NULL, NULL);
>  
>  /* host bridge */
>  QLIST_INIT(&bus->child);
> @@ -801,21 +812,15 @@ static PCIDevice *do_pci_register_device(PCIDevice 
> *pci_dev, PCIBus *bus,
>   PCI_SLOT(devfn), PCI_FUNC(devfn), name, 
> bus->devices[devfn]->name);
>  return NULL;
>  }
> +
>  pci_dev->bus = bus;
> -if (bus->dma_context_fn) {
> -pci_dev->dma = bus->dma_context_fn(bus, bus->dma_context_opaque, 
> devfn);
> -} else {
> -/* FIXME: Make dma_context_fn use MemoryRegions instead, so this 
> path is
> - * taken unconditionally */
> -/* FIXME: inherit memory region from bus creator */
> -memory_region_init_alias(&pci_dev->bus_master_enable_region, "bus 
> master",
> - get_system_memory(), 0,
> - memory_region_size(get_system_memory()));
> -memory_region_set_enabled(&pci_dev->bus_master_enable_region, false);
> -address_space_init(&pci_dev->bus_master_as, 
> &pci_dev->bus_master_enable_region);
> -pci_dev->dma = g_new(DMAContext, 1);
> -dma_context_init(pci_dev->dma, &pci_dev->bus_master_as);
> -}
> +pci_dev->iommu = bus->iommu_fn(bus, bus->iommu_opaque, devfn);


I tried to create a PCI bus on the default PHB and put e1000 onto it like this:

-device pci-bridge,chassis_nr=1  -device e1000,bus=pci.0,addr=1.0


And I got a crash as bus->iommu_fn==NULL.

Is it because we need this bridge to use parent's iommu_fn/iommu or
pci-bridge is not supposed to be created this way or ...?



I made small patch just to get one step further, the info qree is below.
Now our system firmware fails (enters infinite loop) but this is a
different story.


diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 64a983c..a156053 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -811,7 +811,11 @@ static PCIDevice *do_pci_register_device(PCIDevice
*pci_dev, PCIBus *bus,
 }

 pci_dev->bus = bus;
-pci_dev->iommu = bus->iommu_fn(bus, bus->iommu_opaque, devfn);
+if (bus->iommu_fn)
+pci_dev->iommu = bus->iommu_fn(bus, bus->iommu_opaque, devfn);
+else
+pci_dev->iommu = bus->parent_dev->iommu;
+
 memory_region_init_alias(&pci_dev->bus_master_enable_region, "bus master",
  pci_dev->iommu, 0,
memory_region_size(pci_dev->iommu));
 memory_region_set_enabled(&pci_dev->bus_master_enable_region, false);



  dev: spapr-pci-host-bridge, id ""
index = 0
buid = 0x8002000
liobn = 0x8000
mem_win_addr = 0x100a000
mem_win_size = 0x2000
io_win_addr = 0x1008000
io_win_size = 0x1
msi_win_addr = 0x1009000
irq 0
bus: pci
  type PCI
  dev: pci-bridge, id ""
chassis_nr = 1
msi = on
addr = 00.0
romfile = 
rombar = 1
multifunction = off
command_serr_enable = on
class PCI bridge, addr 00:00.0, pci id 1b36:0001 (sub :)
bar 0: mem at 0x [0xfe]
bus: pci.0
  type PCI
  dev: e1000, id ""
mac = 52:54:00:12:34:57
vlan = 
netdev = 
bootindex = -1
autonegotiation = on
addr = 01.0
romfile = "efi-e1000.rom"
rombar = 1
multifunction = off
command_serr_enable = on
class Ethernet controller, addr 00:01.0, pci id

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Paolo Bonzini]

Il 10/05/2013 15:01, Anthony Liguori ha scritto:
> Paolo Bonzini  writes:
> 
>> Cast debugging can have a substantial cost (20% or more, measured by
>> Aurelien on qemu-system-ppc64).
> 
> [Needs citation]

Sure: http://permalink.gmane.org/gmane.comp.emulators.qemu/210830

 49,73%  perf-10672.map   [.] 0x7f7853ab4e0f
 13,23%  qemu-system-ppc64[.] cpu_ppc_exec
 13,16%  libglib-2.0.so.0.3200.4  [.] g_hash_table_lookup
  8,18%  libglib-2.0.so.0.3200.4  [.] g_str_hash
  2,47%  qemu-system-ppc64[.] object_class_dynamic_cast
  1,97%  qemu-system-ppc64[.] type_is_ancestor
  1,05%  libglib-2.0.so.0.3200.4  [.] g_str_equal

That's ~27%.

>> Instead of adding special-cased "fast
>> casts" in the hot paths, we can just disable it in releases.  At the
>> same time, add tracing facilities that simplify the analysys of those
>> problems that cast debugging would reveal.
> 
> I'd prefer not to disable but instead focus on improving performance.

For 1.5?  This is a regression in 1.5 due to more and more usage of
foo_env_on_cpu.

Paolo

> I suspect any performance overhead is resolving the type string to
> typeimpl.  One work around is to have a dynamic cast that takes a
> typeimpl and then use a function that returns a static similar to how
> glib works.
> 
> If you've got a reproducible case where the overhead is high, it should
> be easy to check.
> 
> Regards,
> 
> Anthony Liguori
> 
>>
>> At least patches 1-7 are for 1.5.
>>
>> Paolo Bonzini (9):
>>   qom: improve documentation of cast functions
>>   qom: allow casting of a NULL class
>>   qom: add a fast path to object_class_dynamic_cast
>>   qom: pass file/line/function to asserting casts
>>   qom: trace asserting casts
>>   qom: allow turning cast debugging off
>>   build: disable QOM cast debugging for official releases
>>   qom: simplify object_class_dynamic_cast, part 1
>>   qom: simplify object_class_dynamic_cast, part 2
>>
>>  configure| 20 --
>>  include/qom/object.h | 40 ++-
>>  qom/object.c | 77 
>> ++--
>>  trace-events |  3 ++
>>  4 files changed, 99 insertions(+), 41 deletions(-)
>>
>> -- 
>> 1.8.1.4
>