qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [PATCH v5 1/2] ACPI: Cleanup -acpitable option code


From: Zheng, Lv
Subject: Re: [Qemu-arm] [PATCH v5 1/2] ACPI: Cleanup -acpitable option code
Date: Mon, 15 Aug 2016 05:23:34 +0000

Hi, Igor

> From: Igor Mammedov [mailto:address@hidden
> Subject: Re: [PATCH v5 1/2] ACPI: Cleanup -acpitable option code
> 
> On Thu, 11 Aug 2016 17:36:38 +0800
> Lv Zheng <address@hidden> 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 <address@hidden>
> > ---
> >  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
> > address@hidden -acpitable
> address@hidden,address@hidden,address@hidden,address@hidden,address@hidden
> [,address@hidden,address@hidden,address@hidden:@var{file2}]...]
> > address@hidden -acpitable
> address@hidden:@var{file2}]...[,address@hidden,address@hidden,address@hidden,address@hidden
> str}][,address@hidden [,address@hidden,address@hidden
> >  @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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]