[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qapi: pad GenericList value fields to 64 bits
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH] qapi: pad GenericList value fields to 64 bits |
Date: |
Wed, 29 May 2013 16:15:23 -0400 |
On Wed, 29 May 2013 13:12:18 -0500
mdroth <address@hidden> 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 <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.
Ok, I've applied this patch to the QMP branch.
>
> >
> > >
> > > 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;
> > >
> >
> >
>