Hi, Igor
> From: Igor Mammedov [mailto:imamm...@redhat.com]
> Subject: Re: [PATCH v5 1/2] ACPI: Cleanup -acpitable option code
>
> On Thu, 11 Aug 2016 17:36:38 +0800
> Lv Zheng <lv.zh...@intel.com> wrote:
>
> > In -acpitable options, at least/most one data/file sub-option is mandatory,
> > this patch cleans up the code to reflect this in a managed manner so that
> > the follow-up mandatory sub-options can be added to -acpitable.
> >
> > Signed-off-by: Lv Zheng <lv.zh...@intel.com>
> > ---
> > hw/acpi/core.c | 32 +++++++++++++++++++++++---------
> > qapi-schema.json | 26 +++++++-------------------
> > qemu-options.hx | 7 +++++--
> > 3 files changed, 35 insertions(+), 30 deletions(-)
> >
> > diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> > index e890a5d..85e0e94 100644
> > --- a/hw/acpi/core.c
> > +++ b/hw/acpi/core.c
> > @@ -89,8 +89,6 @@ static int acpi_checksum(const uint8_t *data, int len)
> > * It is valid to call this function with
> > * (@blob == NULL && bloblen == 0 && !has_header).
> > *
> > - * @hdrs->file and @hdrs->data are ignored.
> > - *
> unrelated change?
When file/data become mandatory sub-options,
AcpiTableOptions doesn't have to contain file/data/has_file/has_data.
Because the value of the sub-option "file" or "data" has already been obtained
in acpi_table_add() by invoking qemu_opt_get().
Thus I made this change.
And passed the sub-option value via new "const char *file" parameter.
>
> > * SIZE_MAX is considered "infinity" in this function.
> > *
> > * The number of tables that can be installed is not limited, but the
> > 16-bit
> > @@ -229,7 +227,8 @@ static void acpi_table_install(const char unsigned
> > *blob, size_t bloblen,
> > ACPI_TABLE_PFX_SIZE,
> > acpi_payload_size);
> > }
> >
> > -void acpi_table_add(const QemuOpts *opts, Error **errp)
> > +static void acpi_table_from_file(bool has_header, const char *file,
> > + const QemuOpts *opts, Error **errp)
> > {
> > AcpiTableOptions *hdrs = NULL;
> > Error *err = NULL;
> > @@ -249,12 +248,8 @@ void acpi_table_add(const QemuOpts *opts, Error **errp)
> > if (err) {
> > goto out;
> > }
> > - if (hdrs->has_file == hdrs->has_data) {
> > - error_setg(&err, "'-acpitable' requires one of 'data' or 'file'");
> > - goto out;
> > - }
> >
> > - pathnames = g_strsplit(hdrs->has_file ? hdrs->file : hdrs->data, ":",
> > 0);
> > + pathnames = g_strsplit(file, ":", 0);
> > if (pathnames == NULL || pathnames[0] == NULL) {
> > error_setg(&err, "'-acpitable' requires at least one pathname");
> > goto out;
> > @@ -291,7 +286,7 @@ void acpi_table_add(const QemuOpts *opts, Error **errp)
> > close(fd);
> > }
> >
> > - acpi_table_install(blob, bloblen, hdrs->has_file, hdrs, &err);
> > + acpi_table_install(blob, bloblen, has_header, hdrs, &err);
> >
> > out:
> > g_free(blob);
> > @@ -301,6 +296,25 @@ out:
> > error_propagate(errp, err);
> > }
> >
> > +void acpi_table_add(const QemuOpts *opts, Error **errp)
> > +{
> > + const char *val;
> > +
> > + val = qemu_opt_get((QemuOpts *)opts, "file");
> > + if (val) {
> > + acpi_table_from_file(true, val, opts, errp);
> > + return;
> > + }
> > +
> > + val = qemu_opt_get((QemuOpts *)opts, "data");
> > + if (val) {
> > + acpi_table_from_file(false, val, opts, errp);
> > + return;
> > + }
> > +
> > + error_setg(errp, "'-acpitable' requires one of 'data' or 'file'");
> > +}
> > +
> > static bool acpi_table_builtin = false;
> >
> > void acpi_table_add_builtin(const QemuOpts *opts, Error **errp)
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 5658723..75b8b3b 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -3597,17 +3597,17 @@
> > ##
> > # @AcpiTableOptions
> > #
> > -# Specify an ACPI table on the command line to load.
> > +# Specify ACPI table options for the table loaded on the command line.
> > #
> > -# At most one of @file and @data can be specified. The list of files
> > specified
> > -# by any one of them is loaded and concatenated in order.
>
>
> > -# If both are omitted, @data is implied.
> You are removing this bit of documentation, which is still true
I shouldn't remove it.
It seems this implication is still working after applying my changes.
I booted qemu with -acpitable ssdt-local.aml,sig=SSDT and it worked same as
-acpitable file=ssdt-local.aml.
ssdt-local.aml is loaded as an SSDT table.
How is this implication achieved in the code?
>
>
> > +# ACPI table can be loaded via 'file' and 'data' options. At most one of
> > +# 'file' and 'data' can be specified. The list of files specified by any
> > one
> > +# of them is loaded and concatenated in order.
> > #
> > # Other fields / optargs can be used to override fields of the generic ACPI
> > # table header; refer to the ACPI specification 5.0, section 5.2.6 System
> > # Description Table Header. If a header field is not overridden, then the
> > -# corresponding value from the concatenated blob is used (in case of
> > @file), or
> > -# it is filled in with a hard-coded value (in case of @data).
> > +# corresponding value from the concatenated blob is used (in case of
> > 'file'),
> > +# or it is filled in with a hard-coded value (in case of 'data').
> > #
> > # String fields are copied into the matching ACPI member from lowest
> > address
> > # upwards, and silently truncated / NUL-padded to length.
> > @@ -3628,16 +3628,6 @@
> > # @asl_compiler_rev: #optional revision number of the utility that created
> > the
> > # table (4 bytes)
> > #
> > -# @file: #optional colon (:) separated list of pathnames to load and
> > -# concatenate as table data. The resultant binary blob is expected
> > to
> > -# have an ACPI table header. At least one file is required. This
> > field
> > -# excludes @data.
> > -#
> > -# @data: #optional colon (:) separated list of pathnames to load and
> > -# concatenate as table data. The resultant binary blob must not
> > have an
> > -# ACPI table header. At least one file is required. This field
> > excludes
> > -# @file.
> > -#
> > # Since 1.5
> > ##
> > { 'struct': 'AcpiTableOptions',
> > @@ -3648,9 +3638,7 @@
> > '*oem_table_id': 'str',
> > '*oem_rev': 'uint32',
> > '*asl_compiler_id': 'str',
> > - '*asl_compiler_rev': 'uint32',
> > - '*file': 'str',
> > - '*data': 'str' }}
> > + '*asl_compiler_rev': 'uint32' }}
> it's probably is not ok to remove fields here
> as it might break existing users that expect them
That's what I don't know and need to ask.
In the current code base, I can only see AcpiTableOptions used in
acpi_table_install().
While it is commented that hdrs->file/data are useless for acpi_table_install().
Are there any external users for AcpiTableOptions?
Thanks and best regards
Lv
>
> >
> > ##
> > # @CommandLineParameterType:
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index a71aaf8..5fe7f87 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -1493,10 +1493,13 @@ Disable HPET support.
> > ETEXI
> >
> > DEF("acpitable", HAS_ARG, QEMU_OPTION_acpitable,
> > - "-acpitable
> [sig=str][,rev=n][,oem_id=str][,oem_table_id=str][,oem_rev=n][,asl_compiler_id=str][,asl_compiler_rev=
> n][,{data|file}=file1[:file2]...]\n"
> > + "-acpitable {data|file}=file1[:file2]...\n"
> > + " [,sig=str][,rev=n]\n"
> > + " [,oem_id=str][,oem_table_id=str][,oem_rev=n]\n"
> > + " [,asl_compiler_id=str][,asl_compiler_rev=n]\n"
> > " ACPI table description\n", QEMU_ARCH_I386)
> > STEXI
> > -@item -acpitable
> [sig=@var{str}][,rev=@var{n}][,oem_id=@var{str}][,oem_table_id=@var{str}][,oem_rev=@var{n}]
> [,asl_compiler_id=@var{str}][,asl_compiler_rev=@var{n}][,data=@var{file1}[:@var{file2}]...]
> > +@item -acpitable
> data=@var{file1}[:@var{file2}]...[,sig=@var{str}][,rev=@var{n}][,oem_id=@var{str}][,oem_table_id=@var{
> str}][,oem_rev=@var{n}]
> [,asl_compiler_id=@var{str}][,asl_compiler_rev=@var{n}]
> > @findex -acpitable
> > Add ACPI table with specified header fields and context from specified
> > files.
> > For file=, take whole ACPI table from the specified files, including all
