From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:44241) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QyS4A-0002Qv-73 for qemu-devel@nongnu.org; Tue, 30 Aug 2011 13:25:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QyS48-00075T-U1 for qemu-devel@nongnu.org; Tue, 30 Aug 2011 13:25:38 -0400 Received: from v220110690675601.yourvserver.net ([78.47.199.172]:53591) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QyS48-000753-FQ for qemu-devel@nongnu.org; Tue, 30 Aug 2011 13:25:36 -0400 Message-ID: <4E5D1D0C.6040804@mail.berlios.de> Date: Tue, 30 Aug 2011 19:25:32 +0200 From: Stefan Weil MIME-Version: 1.0 References: <1314564200-6872-1-git-send-email-weil@mail.berlios.de> <4E5B1D42.1010605@mail.berlios.de> <4E5BEEBE.6070500@mail.berlios.de> <4E5C94EE.2000705@redhat.com> In-Reply-To: <4E5C94EE.2000705@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 0/7] Fix packing for MinGW with -mms-bitfields List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: TeLeMan , QEMU Developers , Alexander Graf , Blue Swirl , Isaku Yamahata , Jan Kiszka , Gerd Hoffmann Am 30.08.2011 09:44, schrieb Kevin Wolf: > Am 29.08.2011 21:55, schrieb Stefan Weil: >> Am 29.08.2011 10:34, schrieb TeLeMan: >>> On Mon, Aug 29, 2011 at 13:01, Stefan Weil wrote: >>>> Am 28.08.2011 23:43, schrieb Blue Swirl: >>>>> >>>>> On Sun, Aug 28, 2011 at 8:43 PM, Stefan Weil >>>>> wrote: >>>>>> >>>>>> These patches fix the packing of structures which were affected by >>>>>> the new compiler attribute -mms-bitfields (which is needed for >>>>>> glib-2.0). >>>>>> >>>>>> I compiled qemu.exe with and without -mms-bitfields and compared >>>>>> the resulting struct alignment using pahole and codiff. >>>>> >>>>> If a structure is only used internally by QEMU (not used in network, >>>>> disk or guest interfaces), changes in padding don't matter. In fact, >>>>> in those cases it may be better to remove the packing, because then >>>>> the fields may be naturally aligned and that gives better performance >>>>> on most architectures. Could you please check if this is the case for >>>>> any of the structs? >>>> >>>> I did this already, but also forward your question to the maintainers. >>>> Here is my result: >>>> >>>> [PATCH 2/7] block/vvfat: Fix packing for w32: needs packing (disk) >>>> [PATCH 3/7] acpi: Fix packing for w32: needs packing (bios interface) >>>> [PATCH 4/7] hpet: Fix packing for w32: needs packing (bios interface) >>>> [PATCH 5/7] usb: Fix packing for w32: needs packing (usb interface) >>>> [PATCH 6/7] virtio: Fix packing for w32: needs packing? (guest >>>> interface?) >>>> [PATCH 7/7] slirp: Fix packing for w32: needs packing (network >>>> interface) >>>> >>>> All those struct statements need the pack attribute (otherwise the code >>>> would have to be rewritten which is of course always possible). >>> gesn_cdb in atapi.c, VMDK4Header in vmdk.c and many structures in >>> bt.h need be fixed too. >> >> Oops, you are right. Obviously I missed all anonymous structs: >> codiff simply ignores them, and pahole must be called with >> flags -a -A to show them. Who invented packing of structs? >> >> Comparing the output of pahole -a -A is less elegant than using >> codiff, but shows the structs which you mentioned. >> >> I suggest to apply my patch series first because it fixes >> the most important bugs in networking. The remaining >> bugs are in code which is used less often. They will be >> fixed by a second patch series which replaces all remaining >> packed attributes. > > Shouldn't we have a look at every packed structure instead of just > fixing what we notice as broken in the x86 emulator binary with one > given configuration? I think if there is a QEMU_PACKED, we should use it > consistently, or is there a reason not to do so? > > Kevin Hi Kevin, yes, we should use QEMU_PACKED instead of any __attribute__((packed)). The first 7 patches simply introduce QEMU_PACKED and fix the most important bugs for those users who run QEMU on Windows. There was only a bug report for broken networking (fixed by Jan's committed patch and the above slirp patch). These fixes work for all targets, so chances are good that Windows users will have working binaries for the commonly used scenarios with any target - although I only examined qemu.exe. For this reason, these patches should be applied to git master as soon as possible. I did not intend to have a look at every packed structure as was suggested by Alex, Blue and others. I simply wanted to run a global replace (perl -pi -e ...) which replaced the remaining __attributes__. Reviewing every __attribute__ takes much more time of course: there are more than 250 of them. I don't think that a review is really necessary, because usually "packed" is not added just for fun, and most QEMU code was already reviewed. A small rate of unnecessary QEMU_PACKED would do no harm, because only performance suffers a little. If more people agreed that QEMU_PACKED can be introduced mechanically by a script without a new review, I could send a patch very soon. Cheers, Stefan