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

[Andrew Jones]

On Thu, Jun 18, 2015 at 06:28:36PM +0800, Shannon Zhao wrote:
> 
> 
> On 2015/6/17 17:42, Andrew Jones wrote:
> > 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
> 
> But the table generation codes will be huge and have lots of meaningless
> build_append_int_noprefix(table, 0, 1/2/4/8) for the unused fields.
> Maybe the readability is poor.

It should be about the same number of lines as the struct definition,
one line per field, and it'll only appear in one place, an API call in
aml-build.c. Looking at aml-build.c for some examples where this is
already done, e.g. aml_memory32_fixed and aml_interrupt, ones you added
:-), I would agree that, at first look, it's not super easy to see what's
what. However the key to understanding it is to have the spec open.
Knowing that no field is skipped allows a 1:1 mapping of build_append
lines to spec table fields, and good comments help guide the eye for
that mapping as well, even though just counting lines should work.

Note, the build_append_int_noprefix(table, 0, 1/2/4/8) lines are no more
meaningless than the unused fields in a struct definition. The advantage
of dropping the struct in favor of the build_append lines is the benefit
of simplifying the use of the used fields, as there are no more concerns
with endianness, nor the details of a AcpiGenericAddress, etc.

drew