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 <drjo...@redhat.com> 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. >>> We did this for some structures and I'm thinking it's a good direction >>> generally. >>> >> >> -- >> Shannon -- Shannon