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

[Andrew Jones]

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.

I've had a chat with Igor about this, and thought about it, and I'm now
in the build_append camp. It took me a while to see that point of view,
as it isn't a pattern I'm familiar with. However, the pattern, which is

* table generation code is a sequence of build_appends, commented with
  strings matching strings in the spec
* table generation code has the minimal number of parameters necessary
  to be useful for all users, all other table values have default values
  filled (typically zero)
* the parameters to the table generation code can be packed in a param
  struct that has descriptive member names, allowing the caller to
  still use the struct initializing type pattern, and to more cleanly
  manage the parameters

with the benefits of

* structs (the param structs) stay small
* callers don't have to worry about endianness
* maximum code sharing across users
* no need to try and reproduce the spec as a struct definition, which
  can easily explode with comments/enums trying to describe each field
  accurately

So, I think it would be nice to convert ARM's ACPI generation over to
this type of pattern, which may allow more merging of ARM and x86.
However, that said, it'd be good to get the endianness fix patch and
the gicv2m patch in sooner than later. Maybe we should start with those
patches (just using cpu_to_le*), and then consider a rework of the
struct based table generation, before continuing to extend it.

Thanks,
drew