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

[Luiz Capitulino <lcapitulino@redhat.com>]

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;
> > >  
> > 
> > 
>