[Qemu-devel] [PATCH] qapi: pad GenericList value fields to 64 bits

[Qemu-devel] [PATCH] qapi: pad GenericList value fields to 64 bits

[Michael Roth]

With the introduction of native list types, we now have types such as
int64List where the 'value' field is not a pointer, but the actual
64-bit value.

On 32-bit architectures, this can lead to situations where 'next' field
offset in GenericList does not correspond to the 'next' field in the
types that we cast to GenericList when using the visit_next_list()
interface, causing issues when we attempt to traverse linked list
structures of these types.

To fix this, pad the 'value' field of GenericList and other
schema-defined/native *List types out to 64-bits.

This is less memory-efficient for 32-bit architectures, but allows us to
continue to rely on list-handling interfaces that target GenericList to
simply visitor implementations.

In the future we can improve efficiency by defaulting to using native C
array backends to handle list of non-pointer types, which would be more
memory efficient in itself and allow us to roll back this change.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 include/qapi/visitor.h          |    5 ++++-
 scripts/qapi-types.py           |   10 ++++++++--
 tests/test-qmp-output-visitor.c |    5 ++++-
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 1fef18c..28c21d8 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -18,7 +18,10 @@
 
 typedef struct GenericList
 {
-    void *value;
+    union {
+        void *value;
+        uint64_t padding;
+    };
     struct GenericList *next;
 } GenericList;
 
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index fd42d71..ddcfed9 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -22,7 +22,10 @@ def generate_fwd_struct(name, members, builtin_type=False):
 
 typedef struct %(name)sList
 {
-    %(type)s value;
+    union {
+        %(type)s value;
+        uint64_t padding;
+    };
     struct %(name)sList *next;
 } %(name)sList;
 ''',
@@ -35,7 +38,10 @@ typedef struct %(name)s %(name)s;
 
 typedef struct %(name)sList
 {
-    %(name)s *value;
+    union {
+        %(name)s *value;
+        uint64_t padding;
+    };
     struct %(name)sList *next;
 } %(name)sList;
 ''',
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 0942a41..b2fa9a7 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -295,7 +295,10 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
 
 typedef struct TestStructList
 {
-    TestStruct *value;
+    union {
+        TestStruct *value;
+        uint64_t padding;
+    };
     struct TestStructList *next;
 } TestStructList;
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] qapi: pad GenericList value fields to 64 bits

[Stefan Weil]

Am 27.05.2013 05:20, schrieb Michael Roth:
> With the introduction of native list types, we now have types such as
> int64List where the 'value' field is not a pointer, but the actual
> 64-bit value.
>
> On 32-bit architectures, this can lead to situations where 'next' field
> offset in GenericList does not correspond to the 'next' field in the
> types that we cast to GenericList when using the visit_next_list()
> interface, causing issues when we attempt to traverse linked list
> structures of these types.
>
> To fix this, pad the 'value' field of GenericList and other
> schema-defined/native *List types out to 64-bits.
>
> This is less memory-efficient for 32-bit architectures, but allows us to
> continue to rely on list-handling interfaces that target GenericList to
> simply visitor implementations.
>
> In the future we can improve efficiency by defaulting to using native C
> array backends to handle list of non-pointer types, which would be more
> memory efficient in itself and allow us to roll back this change.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  include/qapi/visitor.h          |    5 ++++-
>  scripts/qapi-types.py           |   10 ++++++++--
>  tests/test-qmp-output-visitor.c |    5 ++++-
>  3 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 1fef18c..28c21d8 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -18,7 +18,10 @@
>  
>  typedef struct GenericList
>  {
> -    void *value;
> +    union {
> +        void *value;
> +        uint64_t padding;
> +    };
>      struct GenericList *next;
>  } GenericList;
>  
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index fd42d71..ddcfed9 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -22,7 +22,10 @@ def generate_fwd_struct(name, members, builtin_type=False):
>  
>  typedef struct %(name)sList
>  {
> -    %(type)s value;
> +    union {
> +        %(type)s value;
> +        uint64_t padding;
> +    };
>      struct %(name)sList *next;
>  } %(name)sList;
>  ''',
> @@ -35,7 +38,10 @@ typedef struct %(name)s %(name)s;
>  
>  typedef struct %(name)sList
>  {
> -    %(name)s *value;
> +    union {
> +        %(name)s *value;
> +        uint64_t padding;
> +    };
>      struct %(name)sList *next;
>  } %(name)sList;
>  ''',
> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index 0942a41..b2fa9a7 100644
> --- a/tests/test-qmp-output-visitor.c
> +++ b/tests/test-qmp-output-visitor.c
> @@ -295,7 +295,10 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
>  
>  typedef struct TestStructList
>  {
> -    TestStruct *value;
> +    union {
> +        TestStruct *value;
> +        uint64_t padding;
> +    };
>      struct TestStructList *next;
>  } TestStructList;
>  

Looks good. Would reordering of value, next work, too
(without memory overhead for 32 bit systems)?

 typedef struct GenericList
 {
    struct GenericList *next;
    void *value;
 } GenericList;

 typedef struct %(name)sList
 {
    struct %(name)sList *next;
    %(type)s value;
 } %(name)sList;


...

It looks like memory allocation (g_malloc0) for GenericList
was also wrong in the old code (this is fixed with your patch).

Re: [Qemu-devel] [PATCH] qapi: pad GenericList value fields to 64 bits

[mdroth]

On Mon, May 27, 2013 at 06:38:35AM +0200, Stefan Weil wrote:
> Am 27.05.2013 05:20, schrieb Michael Roth:
> > With the introduction of native list types, we now have types such as
> > int64List where the 'value' field is not a pointer, but the actual
> > 64-bit value.
> >
> > On 32-bit architectures, this can lead to situations where 'next' field
> > offset in GenericList does not correspond to the 'next' field in the
> > types that we cast to GenericList when using the visit_next_list()
> > interface, causing issues when we attempt to traverse linked list
> > structures of these types.
> >
> > To fix this, pad the 'value' field of GenericList and other
> > schema-defined/native *List types out to 64-bits.
> >
> > This is less memory-efficient for 32-bit architectures, but allows us to
> > continue to rely on list-handling interfaces that target GenericList to
> > simply visitor implementations.
> >
> > In the future we can improve efficiency by defaulting to using native C
> > array backends to handle list of non-pointer types, which would be more
> > memory efficient in itself and allow us to roll back this change.
> >
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> >  include/qapi/visitor.h          |    5 ++++-
> >  scripts/qapi-types.py           |   10 ++++++++--
> >  tests/test-qmp-output-visitor.c |    5 ++++-
> >  3 files changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> > index 1fef18c..28c21d8 100644
> > --- a/include/qapi/visitor.h
> > +++ b/include/qapi/visitor.h
> > @@ -18,7 +18,10 @@
> >  
> >  typedef struct GenericList
> >  {
> > -    void *value;
> > +    union {
> > +        void *value;
> > +        uint64_t padding;
> > +    };
> >      struct GenericList *next;
> >  } GenericList;
> >  
> > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> > index fd42d71..ddcfed9 100644
> > --- a/scripts/qapi-types.py
> > +++ b/scripts/qapi-types.py
> > @@ -22,7 +22,10 @@ def generate_fwd_struct(name, members, builtin_type=False):
> >  
> >  typedef struct %(name)sList
> >  {
> > -    %(type)s value;
> > +    union {
> > +        %(type)s value;
> > +        uint64_t padding;
> > +    };
> >      struct %(name)sList *next;
> >  } %(name)sList;
> >  ''',
> > @@ -35,7 +38,10 @@ typedef struct %(name)s %(name)s;
> >  
> >  typedef struct %(name)sList
> >  {
> > -    %(name)s *value;
> > +    union {
> > +        %(name)s *value;
> > +        uint64_t padding;
> > +    };
> >      struct %(name)sList *next;
> >  } %(name)sList;
> >  ''',
> > diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> > index 0942a41..b2fa9a7 100644
> > --- a/tests/test-qmp-output-visitor.c
> > +++ b/tests/test-qmp-output-visitor.c
> > @@ -295,7 +295,10 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
> >  
> >  typedef struct TestStructList
> >  {
> > -    TestStruct *value;
> > +    union {
> > +        TestStruct *value;
> > +        uint64_t padding;
> > +    };
> >      struct TestStructList *next;
> >  } TestStructList;
> >  
> 
> Looks good. Would reordering of value, next work, too
> (without memory overhead for 32 bit systems)?
> 
>  typedef struct GenericList
>  {
>     struct GenericList *next;
>     void *value;
>  } GenericList;
> 
>  typedef struct %(name)sList
>  {
>     struct %(name)sList *next;
>     %(type)s value;
>  } %(name)sList;

Hmm, that should fix the issue as far as casting goes, but there's also
the issue of allocating memory:

> 
> 
> ...
> 
> It looks like memory allocation (g_malloc0) for GenericList
> was also wrong in the old code (this is fixed with your patch).
> 

Yup, input visitors are expected to allocate memory for storage of the
lists, and currently do so based on sizeof(GenericList), so we'd still
need to address that problem if we took the above approach. It wouldn't
take much: we'd probably modify visit_start_list() to accept an
additional argument for how large a list container we need it to
allocate.

But this is kind of a 1-off thing specifically for non-pointer list
types, and the ones in-tree, (u)int{8,16,32,64}/bool/double, should be
the only ones we ever need, so i'd like to avoid complicating the qapi
interface/visitor implementations to support them, especially since I
plan on switching them to using an C array backend in the future instead
of linked lists, which should address the memory efficiency issues.

Re: [Qemu-devel] [PATCH] qapi: pad GenericList value fields to 64 bits

[Luiz Capitulino]

On Sun, 26 May 2013 22:20:58 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> With the introduction of native list types, we now have types such as
> int64List where the 'value' field is not a pointer, but the actual
> 64-bit value.
> 
> On 32-bit architectures, this can lead to situations where 'next' field
> offset in GenericList does not correspond to the 'next' field in the
> types that we cast to GenericList when using the visit_next_list()
> interface, causing issues when we attempt to traverse linked list
> structures of these types.
> 
> To fix this, pad the 'value' field of GenericList and other
> schema-defined/native *List types out to 64-bits.
> 
> This is less memory-efficient for 32-bit architectures, but allows us to
> continue to rely on list-handling interfaces that target GenericList to
> simply visitor implementations.
> 
> In the future we can improve efficiency by defaulting to using native C
> array backends to handle list of non-pointer types, which would be more
> memory efficient in itself and allow us to roll back this change.

I'm also concerned with the small complexity this change is adding.
How hard would it be to do the proper solution with arrays instead?

> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  include/qapi/visitor.h          |    5 ++++-
>  scripts/qapi-types.py           |   10 ++++++++--
>  tests/test-qmp-output-visitor.c |    5 ++++-
>  3 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 1fef18c..28c21d8 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -18,7 +18,10 @@
>  
>  typedef struct GenericList
>  {
> -    void *value;
> +    union {
> +        void *value;
> +        uint64_t padding;
> +    };
>      struct GenericList *next;
>  } GenericList;
>  
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index fd42d71..ddcfed9 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -22,7 +22,10 @@ def generate_fwd_struct(name, members, builtin_type=False):
>  
>  typedef struct %(name)sList
>  {
> -    %(type)s value;
> +    union {
> +        %(type)s value;
> +        uint64_t padding;
> +    };
>      struct %(name)sList *next;
>  } %(name)sList;
>  ''',
> @@ -35,7 +38,10 @@ typedef struct %(name)s %(name)s;
>  
>  typedef struct %(name)sList
>  {
> -    %(name)s *value;
> +    union {
> +        %(name)s *value;
> +        uint64_t padding;
> +    };
>      struct %(name)sList *next;
>  } %(name)sList;
>  ''',
> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index 0942a41..b2fa9a7 100644
> --- a/tests/test-qmp-output-visitor.c
> +++ b/tests/test-qmp-output-visitor.c
> @@ -295,7 +295,10 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
>  
>  typedef struct TestStructList
>  {
> -    TestStruct *value;
> +    union {
> +        TestStruct *value;
> +        uint64_t padding;
> +    };
>      struct TestStructList *next;
>  } TestStructList;
>  

Re: [Qemu-devel] [PATCH] qapi: pad GenericList value fields to 64 bits

[mdroth]

On Wed, May 29, 2013 at 01:32:52PM -0400, Luiz Capitulino wrote:
> On Sun, 26 May 2013 22:20:58 -0500
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > With the introduction of native list types, we now have types such as
> > int64List where the 'value' field is not a pointer, but the actual
> > 64-bit value.
> > 
> > On 32-bit architectures, this can lead to situations where 'next' field
> > offset in GenericList does not correspond to the 'next' field in the
> > types that we cast to GenericList when using the visit_next_list()
> > interface, causing issues when we attempt to traverse linked list
> > structures of these types.
> > 
> > To fix this, pad the 'value' field of GenericList and other
> > schema-defined/native *List types out to 64-bits.
> > 
> > This is less memory-efficient for 32-bit architectures, but allows us to
> > continue to rely on list-handling interfaces that target GenericList to
> > simply visitor implementations.
> > 
> > In the future we can improve efficiency by defaulting to using native C
> > array backends to handle list of non-pointer types, which would be more
> > memory efficient in itself and allow us to roll back this change.
> 
> I'm also concerned with the small complexity this change is adding.
> How hard would it be to do the proper solution with arrays instead?

It's not *too* bad, we'd need patches 9-11 from here:

http://lists.gnu.org/archive/html/qemu-devel/2012-10/threads.html#05755

Along with code generation bits, and then all the unit test stuff.

I think we should be able to get it in for 1.6, but I'd rather not leave
32-bit busted in the meantime.

> 
> > 
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> >  include/qapi/visitor.h          |    5 ++++-
> >  scripts/qapi-types.py           |   10 ++++++++--
> >  tests/test-qmp-output-visitor.c |    5 ++++-
> >  3 files changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> > index 1fef18c..28c21d8 100644
> > --- a/include/qapi/visitor.h
> > +++ b/include/qapi/visitor.h
> > @@ -18,7 +18,10 @@
> >  
> >  typedef struct GenericList
> >  {
> > -    void *value;
> > +    union {
> > +        void *value;
> > +        uint64_t padding;
> > +    };
> >      struct GenericList *next;
> >  } GenericList;
> >  
> > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> > index fd42d71..ddcfed9 100644
> > --- a/scripts/qapi-types.py
> > +++ b/scripts/qapi-types.py
> > @@ -22,7 +22,10 @@ def generate_fwd_struct(name, members, builtin_type=False):
> >  
> >  typedef struct %(name)sList
> >  {
> > -    %(type)s value;
> > +    union {
> > +        %(type)s value;
> > +        uint64_t padding;
> > +    };
> >      struct %(name)sList *next;
> >  } %(name)sList;
> >  ''',
> > @@ -35,7 +38,10 @@ typedef struct %(name)s %(name)s;
> >  
> >  typedef struct %(name)sList
> >  {
> > -    %(name)s *value;
> > +    union {
> > +        %(name)s *value;
> > +        uint64_t padding;
> > +    };
> >      struct %(name)sList *next;
> >  } %(name)sList;
> >  ''',
> > diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> > index 0942a41..b2fa9a7 100644
> > --- a/tests/test-qmp-output-visitor.c
> > +++ b/tests/test-qmp-output-visitor.c
> > @@ -295,7 +295,10 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
> >  
> >  typedef struct TestStructList
> >  {
> > -    TestStruct *value;
> > +    union {
> > +        TestStruct *value;
> > +        uint64_t padding;
> > +    };
> >      struct TestStructList *next;
> >  } TestStructList;
> >  
> 
> 

Re: [Qemu-devel] [PATCH] qapi: pad GenericList value fields to 64 bits

[Luiz Capitulino]

On Wed, 29 May 2013 13:12:18 -0500
mdroth <mdroth@linux.vnet.ibm.com> wrote:

> On Wed, May 29, 2013 at 01:32:52PM -0400, Luiz Capitulino wrote:
> > On Sun, 26 May 2013 22:20:58 -0500
> > Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> > 
> > > With the introduction of native list types, we now have types such as
> > > int64List where the 'value' field is not a pointer, but the actual
> > > 64-bit value.
> > > 
> > > On 32-bit architectures, this can lead to situations where 'next' field
> > > offset in GenericList does not correspond to the 'next' field in the
> > > types that we cast to GenericList when using the visit_next_list()
> > > interface, causing issues when we attempt to traverse linked list
> > > structures of these types.
> > > 
> > > To fix this, pad the 'value' field of GenericList and other
> > > schema-defined/native *List types out to 64-bits.
> > > 
> > > This is less memory-efficient for 32-bit architectures, but allows us to
> > > continue to rely on list-handling interfaces that target GenericList to
> > > simply visitor implementations.
> > > 
> > > In the future we can improve efficiency by defaulting to using native C
> > > array backends to handle list of non-pointer types, which would be more
> > > memory efficient in itself and allow us to roll back this change.
> > 
> > I'm also concerned with the small complexity this change is adding.
> > How hard would it be to do the proper solution with arrays instead?
> 
> It's not *too* bad, we'd need patches 9-11 from here:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2012-10/threads.html#05755
> 
> Along with code generation bits, and then all the unit test stuff.
> 
> I think we should be able to get it in for 1.6, but I'd rather not leave
> 32-bit busted in the meantime.

Ok, I've applied this patch to the QMP branch.

> 
> > 
> > > 
> > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > ---
> > >  include/qapi/visitor.h          |    5 ++++-
> > >  scripts/qapi-types.py           |   10 ++++++++--
> > >  tests/test-qmp-output-visitor.c |    5 ++++-
> > >  3 files changed, 16 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> > > index 1fef18c..28c21d8 100644
> > > --- a/include/qapi/visitor.h
> > > +++ b/include/qapi/visitor.h
> > > @@ -18,7 +18,10 @@
> > >  
> > >  typedef struct GenericList
> > >  {
> > > -    void *value;
> > > +    union {
> > > +        void *value;
> > > +        uint64_t padding;
> > > +    };
> > >      struct GenericList *next;
> > >  } GenericList;
> > >  
> > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> > > index fd42d71..ddcfed9 100644
> > > --- a/scripts/qapi-types.py
> > > +++ b/scripts/qapi-types.py
> > > @@ -22,7 +22,10 @@ def generate_fwd_struct(name, members, builtin_type=False):
> > >  
> > >  typedef struct %(name)sList
> > >  {
> > > -    %(type)s value;
> > > +    union {
> > > +        %(type)s value;
> > > +        uint64_t padding;
> > > +    };
> > >      struct %(name)sList *next;
> > >  } %(name)sList;
> > >  ''',
> > > @@ -35,7 +38,10 @@ typedef struct %(name)s %(name)s;
> > >  
> > >  typedef struct %(name)sList
> > >  {
> > > -    %(name)s *value;
> > > +    union {
> > > +        %(name)s *value;
> > > +        uint64_t padding;
> > > +    };
> > >      struct %(name)sList *next;
> > >  } %(name)sList;
> > >  ''',
> > > diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> > > index 0942a41..b2fa9a7 100644
> > > --- a/tests/test-qmp-output-visitor.c
> > > +++ b/tests/test-qmp-output-visitor.c
> > > @@ -295,7 +295,10 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
> > >  
> > >  typedef struct TestStructList
> > >  {
> > > -    TestStruct *value;
> > > +    union {
> > > +        TestStruct *value;
> > > +        uint64_t padding;
> > > +    };
> > >      struct TestStructList *next;
> > >  } TestStructList;
> > >  
> > 
> > 
>