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

[Michael S. Tsirkin]

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 <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).
> > 
> > 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