[Spice-devel] [PATCH spice-common v2] codegen: Make the compiler work out better way to write unaligned memory

[Christophe Fergeau]

Has this been tested on actual problematic hardware, and improved
performance? I remember last time this was discussed, the outcome was
not always what was expected.

Christophe

On Mon, Sep 18, 2017 at 02:52:57AM -0400, Frediano Ziglio wrote:
> ping
> 
> I think I sent a reply after the new path which could be confusing.
> 
> > 
> > Instead of assuming that the system can safely do unaligned access
> > to memory use packed structures to allow the compiler generate
> > best code possible.
> > A packed structure tells the compiler to not leave padding inside it
> > and that the structure can be unaligned so any field can be unaligned
> > having to generate proper access code based on architecture.
> > For instance ARM7 can use unaligned access but not for 64 bit
> > numbers (currently these accesses are emulated by Linux kernel
> > with obvious performance consequences).
> > 
> > This changes the current methods from:
> > 
> > #ifdef WORDS_BIGENDIAN
> > #define read_uint32(ptr) ((uint32_t)SPICE_BYTESWAP32(*((uint32_t *)(ptr))))
> > #define write_uint32(ptr, val) *(uint32_t *)(ptr) =
> > SPICE_BYTESWAP32((uint32_t)val)
> > #else
> > #define read_uint32(ptr) (*((uint32_t *)(ptr)))
> > #define write_uint32(ptr, val) (*((uint32_t *)(ptr))) = val
> > #endif
> > 
> > to:
> > #include <spice/start-packed.h>
> > typedef struct SPICE_ATTR_PACKED {
> >     uint32_t v;
> > } uint32_unaligned_t;
> > #include <spice/end-packed.h>
> > 
> > #ifdef WORDS_BIGENDIAN
> > #define read_uint32(ptr) ((uint32_t)SPICE_BYTESWAP32(((uint32_unaligned_t
> > *)(ptr))->v))
> > #define write_uint32(ptr, val) ((uint32_unaligned_t *)(ptr))->v =
> > SPICE_BYTESWAP32((uint32_t)val)
> > #else
> > #define read_uint32(ptr) (((uint32_unaligned_t *)(ptr))->v)
> > #define write_uint32(ptr, val) (((uint32_unaligned_t *)(ptr))->v) = val
> > #endif
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  python_modules/demarshal.py | 33 +++++++++++++++++++++++----------
> >  1 file changed, 23 insertions(+), 10 deletions(-)
> > 
> > Changes since v1:
> > - improved commit message;
> > - added SPICE_ATTR_PACKED to structure definition for compatibility;
> > - use intX_unaligned_t instead of intX_unaligned_p.
> > 
> > diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
> > index 1ea131d..da87d44 100644
> > --- a/python_modules/demarshal.py
> > +++ b/python_modules/demarshal.py
> > @@ -40,24 +40,37 @@ def write_parser_helpers(writer):
> >  
> >      writer = writer.function_helper()
> >  
> > +    writer.writeln("#include <spice/start-packed.h>")
> > +    for size in [16, 32, 64]:
> > +        for sign in ["", "u"]:
> > +            type = "%sint%d" % (sign, size)
> > +            writer.begin_block("typedef struct SPICE_ATTR_PACKED")
> > +            writer.variable_def("%s_t" % type, "v")
> > +            writer.end_block(newline=False)
> > +            writer.writeln(" %s_unaligned_t;" % type)
> > +    writer.writeln("#include <spice/end-packed.h>")
> > +    writer.newline()
> > +
> > +    for sign in ["", "u"]:
> > +        type = "%sint8" % sign
> > +        writer.macro("read_%s" % type, "ptr", "(*((%s_t *)(ptr)))" % type)
> > +        writer.macro("write_%s" % type, "ptr, val", "*(%s_t *)(ptr) = val" %
> > (type))
> > +    writer.newline()
> > +
> >      writer.writeln("#ifdef WORDS_BIGENDIAN")
> > -    for size in [8, 16, 32, 64]:
> > +    for size in [16, 32, 64]:
> >          for sign in ["", "u"]:
> >              utype = "uint%d" % (size)
> >              type = "%sint%d" % (sign, size)
> >              swap = "SPICE_BYTESWAP%d" % size
> > -            if size == 8:
> > -                writer.macro("read_%s" % type, "ptr", "(*((%s_t *)(ptr)))" %
> > type)
> > -                writer.macro("write_%s" % type, "ptr, val", "*(%s_t *)(ptr)
> > = val" % (type))
> > -            else:
> > -                writer.macro("read_%s" % type, "ptr", "((%s_t)%s(*((%s_t
> > *)(ptr))))" % (type, swap, utype))
> > -                writer.macro("write_%s" % type, "ptr, val", "*(%s_t *)(ptr)
> > = %s((%s_t)val)" % (utype, swap, utype))
> > +            writer.macro("read_%s" % type, "ptr",
> > "((%s_t)%s(((%s_unaligned_t *)(ptr))->v))" % (type, swap, utype))
> > +            writer.macro("write_%s" % type, "ptr, val", "((%s_unaligned_t
> > *)(ptr))->v = %s((%s_t)val)" % (utype, swap, utype))
> >      writer.writeln("#else")
> > -    for size in [8, 16, 32, 64]:
> > +    for size in [16, 32, 64]:
> >          for sign in ["", "u"]:
> >              type = "%sint%d" % (sign, size)
> > -            writer.macro("read_%s" % type, "ptr", "(*((%s_t *)(ptr)))" %
> > type)
> > -            writer.macro("write_%s" % type, "ptr, val", "(*((%s_t *)(ptr)))
> > = val" % type)
> > +            writer.macro("read_%s" % type, "ptr", "(((%s_unaligned_t
> > *)(ptr))->v)" % type)
> > +            writer.macro("write_%s" % type, "ptr, val", "(((%s_unaligned_t
> > *)(ptr))->v) = val" % type)
> >      writer.writeln("#endif")
> >  
> >      for size in [8, 16, 32, 64]:
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel