Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table

[Igor Mammedov]

On Tue, 16 Jun 2015 09:33:19 +0800
Shannon Zhao <address@hidden> 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 <address@hidden> 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).
that would be drop in replacement for struct
(i.e. you'll just use build_append_int_noprefix instead of struct)

It's easier to review either since it repeats table descriptions
from spec practically 1:1 and there is no need to invent names for
struct fields anymore.

this approach is used in aml_build.c and so far works well.

> 
> > We did this for some structures and I'm thinking it's a good direction
> > generally.
> > 
>