On Tue, Jul 10, 2018 at 06:28:34AM -0400, Frediano Ziglio wrote: > > > > On Mon, Jul 09, 2018 at 04:48:46AM -0400, Frediano Ziglio wrote: > > > > > > > > On Tue, Jul 03, 2018 at 11:38:56AM -0400, Frediano Ziglio wrote: > > > > > > > > > > > > On Thu, Jun 28, 2018 at 09:43:47PM +0100, Frediano Ziglio wrote: > > > > > > > This avoids compilers detect misaligned access. > > > > > > > > > > > > Maybe "This helps some compilers to realize QXLRam is correctly > > > > > > aligned"? > > > > > > > > > > > > > > > > Well, the current code tells the compiler that this structure > > > > > is NOT aligned. > > > > > But we actually know the opposite > > > > > > > > Well, we know that in QEMU and Xspice, this is correctly aligned (but > > > > one need to look at how ram offset is computed in order to see that), so > > > > we add the annotation to spice-protocol. Since this is quite odd, the > > > > commit log would indeed need to be quite descriptive. > > > > > > > > Christophe > > > > > > > > > > Suggestions? > > > > I would rephrase like this: > > > > > This avoids compilers detect misaligned access. > > > > Without such a hint, some compilers (clang) will generate warnings for > > trying > > to > > do atomic operations on misaligned memory ? > > > > Actually for a bug code does not link as is referring to a undefined > function. In Qemu an atomic operation is performed on int_pending which > causes the problem. Atomic operator and unaligned access does not > play well together although x86/x64 make an exceptions (beside potentially > taking literally hundreds of CPU ticks). Note that QXL (in this case I'm > talking about virtual QXL card, not SPICE interface) is provided only > in x86/x64 so this would be a real issue only if you are emulating the > CPU on a no x86/x64 system. > > OT: about endianness and QXL doing some minor portability attempts > I realized that commands to QXL interface can arrive either in host > encoding or little endian so for instance if you need to run a > x64 machine on a big endian host machine you could receive for the > same VM command in 2 different endianness! Nothing that we support > at the moment, mainly a curiosity. > > > > The structure is allocated at the beginning of a page so surely > > > will be 4 bytes aligned. > > > > In QEMU and Xspice, the offset of the structure will be rounded to a 4kB > > boundary, so the structure will end up 4 bytes aligned as well with > > these users. We are not aware of other users of QXLRam, but if there > > was, it's likely the struct would be at least naturally aligned, so this > > change should not impact anyone. > > > > Not sure if Xspice aligns this structure to 4kB. > But in QXLRam there are some fields that potentially are accessed by > 2 thread which could be in theory an issue if not aligned. For instance > incrementing 255 from another thread you could get a 0. > > > Aligning to 4 bytes maintains the size of the structure unchanged > > avoiding possible ABI changes. > > > > I assume I should copy these last 2 sentences in the commit message.
Yes, all of this were suggested additions/changes to the commit message as you asked for previously ;) Christophe
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
