From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58561) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4Yst-0003VW-MT for qemu-devel@nongnu.org; Mon, 15 Jun 2015 14:13:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z4Ysm-0005SU-QU for qemu-devel@nongnu.org; Mon, 15 Jun 2015 14:13:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47127) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4Ysm-0005SL-Kd for qemu-devel@nongnu.org; Mon, 15 Jun 2015 14:13:16 -0400 Date: Mon, 15 Jun 2015 20:13:13 +0200 From: "Michael S. Tsirkin" Message-ID: <20150615201153-mutt-send-email-mst@redhat.com> References: <1433929959-29530-1-git-send-email-drjones@redhat.com> <1433929959-29530-3-git-send-email-drjones@redhat.com> <20150615180904-mutt-send-email-mst@redhat.com> <20150615163221.GA30395@hawk.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Igor Mammedov , Andrew Jones , QEMU Developers , Shannon Zhao On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote: > On 15 June 2015 at 17:32, Andrew Jones wrote: > > On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote: > >> On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote: > >> > I'm still confused about when fields in these ACPI structs > >> > need to be converted to little-endian, and when they don't. > >> > Is there a rule-of-thumb I can use when I'm looking at patches? > > >> Normally it's all LE unless it's a single byte value. > >> Did not check this specific table. > >> We really need to add sparse support to check > >> endian-ness matches, or re-write it > >> all using byte_add so there's no duplication of info. > > > Everything used in the table is either a single byte, or I used le32, > > Well, I didn't bother for the pci_{device,vendor}_id assignments, as > > they're 0xffff anyway. I can change those two to make them more explicit, > > if that's preferred. > > Yep, I just looked over the struct definition, so since this > has been reviewed I'll apply it to target-arm.next. > > You could probably make it easier to review and write > code that has to do these endianness swaps with something > like > > #define acpi_struct_assign(FIELD, VAL) \ > ((FIELD) = \ > __builtin_choose_expr(sizeof(FIELD) == 1, VAL, \ > __builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \ > __builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \ > __builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \ > abort)))) > > (untested, but based on some code in linux-user/qemu.h). > > Then it's always > > acpi_struct_assign(spcr->field, value); > > whether the field is 1, 2, 4 or 8 bytes. > > Not my bit of the codebase though, so I'll leave it to the > ACPI maintainers to decide how much they like magic macros :-) > > thanks > -- PMM We don't much. One can use build_append_int_noprefix and just avoid structs altogether. We did this for some structures and I'm thinking it's a good direction generally. -- MST