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

[Shannon Zhao <zhaoshenglong@huawei.com>]

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 <drjones@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.
> 
> 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.

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

-- 
Shannon