From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:58129) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QyfMd-0004Dh-ID for qemu-devel@nongnu.org; Wed, 31 Aug 2011 03:37:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QyfMc-0005jO-3O for qemu-devel@nongnu.org; Wed, 31 Aug 2011 03:37:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48552) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QyfMb-0005jA-Qc for qemu-devel@nongnu.org; Wed, 31 Aug 2011 03:37:34 -0400 Message-ID: <4E5DE55D.3070501@redhat.com> Date: Wed, 31 Aug 2011 09:40:13 +0200 From: Kevin Wolf 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> <4E5D1D0C.6040804@mail.berlios.de> <731DACCB-4F4A-4213-B7EC-2AB1D6989B08@suse.de> In-Reply-To: <731DACCB-4F4A-4213-B7EC-2AB1D6989B08@suse.de> Content-Type: text/plain; charset=ISO-8859-1 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: Alexander Graf Cc: Isaku Yamahata , TeLeMan , QEMU Developers , Blue Swirl , Jan Kiszka , Gerd Hoffmann , Aurelien Jarno Am 30.08.2011 20:29, schrieb Alexander Graf: > > On 30.08.2011, at 19:25, Stefan Weil wrote: > >> 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. > > I think that's the better approach to the partial commit. Just introduce QEMU_PACKED, provide the script/sed cmdline you ran over the tree and replace it in every file. That makes more sense to commit than the partial conversion. > > But please wait for a second opinion here :) I agree. Kevin