[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: |
Igor Mammedov |
Subject: |
Re: [Qemu-arm] [PATCH v5 1/2] ACPI: Cleanup -acpitable option code |
Date: |
Fri, 12 Aug 2016 16:51:00 +0200 |
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?
> * 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
> +# 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
>
> ##
> # @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,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
- [Qemu-arm] [PATCH v3 0/2] ACPI: Add FADT revision support, (continued)
- [Qemu-arm] [PATCH v3 0/2] ACPI: Add FADT revision support, Lv Zheng, 2016/08/11
- [Qemu-arm] [PATCH v4 0/2] ACPI: Add FADT revision support, Lv Zheng, 2016/08/11
- [Qemu-arm] [PATCH v5 0/2] ACPI: Add FADT revision support, Lv Zheng, 2016/08/11
- [Qemu-arm] [PATCH v5 2/2] ACPI: Add -acpitable fadt= to allow FADT revision changes, Lv Zheng, 2016/08/11
- Re: [Qemu-arm] [PATCH v5 2/2] ACPI: Add -acpitable fadt= to allow FADT revision changes, Igor Mammedov, 2016/08/11
- Re: [Qemu-arm] [PATCH v5 2/2] ACPI: Add -acpitable fadt= to allow FADT revision changes, Zheng, Lv, 2016/08/11
- Re: [Qemu-arm] [PATCH v5 2/2] ACPI: Add -acpitable fadt= to allow FADT revision changes, Michael S. Tsirkin, 2016/08/11
- Re: [Qemu-arm] [PATCH v5 2/2] ACPI: Add -acpitable fadt= to allow FADT revision changes, Zheng, Lv, 2016/08/14
- Re: [Qemu-arm] [PATCH v5 2/2] ACPI: Add -acpitable fadt= to allow FADT revision changes, Michael S. Tsirkin, 2016/08/14
- Re: [Qemu-arm] [PATCH v5 2/2] ACPI: Add -acpitable fadt= to allow FADT revision changes, Zheng, Lv, 2016/08/14
- Re: [Qemu-arm] [PATCH v5 2/2] ACPI: Add -acpitable fadt= to allow FADT revision changes, Michael S. Tsirkin, 2016/08/14
- Re: [Qemu-arm] [PATCH v5 2/2] ACPI: Add -acpitable fadt= to allow FADT revision changes, Zheng, Lv, 2016/08/14