From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54892) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4riD-0004qC-4Z for qemu-devel@nongnu.org; Tue, 16 Jun 2015 10:19:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z4riA-0001PT-E8 for qemu-devel@nongnu.org; Tue, 16 Jun 2015 10:19:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41130) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4riA-0001Ol-6y for qemu-devel@nongnu.org; Tue, 16 Jun 2015 10:19:34 -0400 Date: Tue, 16 Jun 2015 16:19:30 +0200 From: "Michael S. Tsirkin" Message-ID: <20150616161800-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> <20150615201153-mutt-send-email-mst@redhat.com> <557F7CDF.5080203@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <557F7CDF.5080203@linaro.org> 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: Shannon Zhao Cc: Peter Maydell , Andrew Jones , QEMU Developers , Igor Mammedov On Tue, Jun 16, 2015 at 09:33:19AM +0800, Shannon Zhao wrote: > > > On 2015/6/16 2:13, Michael S. Tsirkin wrote: > > 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. > > But if we use build_append_int_noprefix, we have to bother about the > unused fields of the struct and have lots of > build_append_int_noprefix(table, 0, 1/2/4/8). With a struct you have a bunch of reserved fields - is that very different? > > We did this for some structures and I'm thinking it's a good direction > > generally. > > > > -- > Shannon