From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53845) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z57D2-0008TP-DO for qemu-devel@nongnu.org; Wed, 17 Jun 2015 02:52:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z57Cx-0008Bm-8s for qemu-devel@nongnu.org; Wed, 17 Jun 2015 02:52:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36327) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z57Cx-0008Ai-1f for qemu-devel@nongnu.org; Wed, 17 Jun 2015 02:52:23 -0400 Date: Wed, 17 Jun 2015 08:52:19 +0200 From: "Michael S. Tsirkin" Message-ID: <20150617084902-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> <20150616161800-mutt-send-email-mst@redhat.com> <5580C827.10600@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5580C827.10600@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 Wed, Jun 17, 2015 at 09:06:47AM +0800, Shannon Zhao wrote: > > > On 2015/6/16 22:19, Michael S. Tsirkin wrote: > > 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? > > > > Not only about the reserved fields, but also the fields which ARM > doesn't use or x86 doesn't use. For example, xpm1a_event_block in struct > AcpiFadtDescriptorRev5_1 is not used for ARM now, if we use > build_append_int_noprefix, we should add lots of > build_append_int_noprefix(table, 0, 1/2/4/8). But if we use struct, we > just need to care them when we define it, rather than every time we use. So the advice above assumes that you have a wrapper function for building each struct. Then you would just pass 0 as parameters as appropriate. But I am not claiming we need to switch all code away from structs. If you like it like this, keep it around. > >>> We did this for some structures and I'm thinking it's a good direction > >>> generally. > >>> > >> > >> -- > >> Shannon > > -- > Shannon