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 <address@hidden> 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 <address@hidden>
> > ---
> >  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;
> >  
> 
>