Re: [Qemu-devel] [PATCH v5 2/2] ACPI: Add -acpitable fadt= to allow FADT revision changes

[Paolo Bonzini <pbonzini@redhat.com>]

On 15/08/2016 03:42, Zheng, Lv wrote:
> Hi, Paolo
> 
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> Subject: Re: [PATCH v5 2/2] ACPI: Add -acpitable fadt= to allow FADT
>> revision changes
>>
>>
>>
>> On 11/08/2016 11:36, Lv Zheng wrote:
>>>
>>> -    error_setg(errp, "'-acpitable' requires one of 'data' or 'file'");
>>> +    val = qemu_opt_get((QemuOpts *)opts, "fadt");
>>> +    if (val) {
>>> +        unsigned long rev;
>>
>> Don't use qemu_opt_get.  Add the field to AcpiTableOptions in
>> qapi-schema.json, and then use hdrs->has_fadt, hdrs->fadt.
> 
> 
> 1. If I do so, users may be confused when only -acpitable fadt=3 is
>     specified (no user tables are provided), qemu may exit because of 
>     "data or file option missing".
> 2. If we doesn't want qemu exit in the above case, code in
>     acpi_table_add() will be too complicated.
> 3. If I put fadt into AcpiTableOptions in the schema,
>     hdrs->has_fadt/hdrs->fadt will become 2 more useless options
>     (just like hders->file/hdrs->data/hdrs->has_file/hdrs->has_data,
>     see comments of acpi_table_install()) passed to acpi_table_install().
> 
> That's why I enhanced -acpitable to convert it into an option with 3
> mandatory sub-options:
> -acpitable data/file ....
> -acpitable fadt ....

But most arguments of -acpitable (e.g. oem_id) apply to FADT as well.  
In fact "-acpitable fadt=3" perhaps could be written as "-acpitable fadt,rev=3".

So one possibility if you add '*fadt': 'bool' to AcpiTableOptions is the following:

    if (hdrs->has_file + hdrs->has_data + hdrs->has_fadt > 1) {
        error_setg(&err, "'-acpitable' requires one of 'data' or 'file' or 'fadt'");
        goto out;
    }
    if (hdrs->has_fadt && hdrs->fadt) {
        fadt_options = hdrs;
        return;
    }

Paolo