From patchwork Tue Apr 19 13:57:32 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Octavian Purdila X-Patchwork-Id: 8880391 Return-Path: X-Original-To: patchwork-linux-acpi@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id CE719BF29F for ; Tue, 19 Apr 2016 13:57:54 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id A837220121 for ; Tue, 19 Apr 2016 13:57:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6271D20268 for ; Tue, 19 Apr 2016 13:57:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754456AbcDSN5r (ORCPT ); Tue, 19 Apr 2016 09:57:47 -0400 Received: from mail-wm0-f48.google.com ([74.125.82.48]:34981 "EHLO mail-wm0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754030AbcDSN5p (ORCPT ); Tue, 19 Apr 2016 09:57:45 -0400 Received: by mail-wm0-f48.google.com with SMTP id e201so14968715wme.0 for ; Tue, 19 Apr 2016 06:57:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc; bh=7IBLXM/I1AYNzTjx+Iqvy8j9YYxOmuO2GBvRXwbWSkU=; b=M7yvkJHjrpL1ww43eV5t06Eh+FOwiPeUNUYZlSjgD2SlHQlWfnEQwA4y6ytsJ7c90j aSQV3+J3w8p57A+1383V+tcG4NUnfcRbTK243KSRFe4v9VdXkxo9AT4IvmnLtCZ5+w+Y JT0jL0xTQqAu9S7ZA71w7cXJ1+pKmnPuFtwovvCK/EhGXWZt0daICf9ACp1SWgrSoI3c znewFzEclie5bzaWxSQ+WUCf15pK9w5y/GoZYt72LGx8AdkWmN1lrX9J+Y/Ek01NBfHZ 6GAKFfU35OrB6Vb93pIEc0+H2+ADKPDjc6ZjwYPsHNFG7cCYAuOvuLbv4rBP9elLlTdB d79w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc; bh=7IBLXM/I1AYNzTjx+Iqvy8j9YYxOmuO2GBvRXwbWSkU=; b=DrE3XdOR+As1hQZx9ZVHip4VONPoT3UH9+jbw8ES7jh0NCFm899VyIxWHrlrB0cL8N E7D/0iLNnAW2uHrM0eM8HRW583mzizjk99yk5uDdHElBoqNKKDoq3e+QBV+ACyu/JRCZ FOlLHEMx/yYtpkCZd4PWhY0cP8/UQ/+sIFX9rrNH2OQzNlIqYmxg+rvqYUvOfr+uqTuV ThbtUT6DP1sISgDSSYi5XfDBB33MMcSm3RnSvw6vRDBmnctRzBA2MO1cnMNcRb/gNoOQ clE9WIN7s9Xd9fWWErovEdVca6u5EUVhDMTP5Nl4yntp+d67kacvU1SiD/BgMfeGml2o Ydmw== X-Gm-Message-State: AOPr4FXCAcoyo1A0gKjNi6cQ8Q3a1VhXpsLya1CF2WgUMmOxrgemeZBf38hrircFrNY+GwlKxznomE0OfgrxXzn7 MIME-Version: 1.0 X-Received: by 10.28.145.205 with SMTP id t196mr23823881wmd.60.1461074252106; Tue, 19 Apr 2016 06:57:32 -0700 (PDT) Received: by 10.194.42.138 with HTTP; Tue, 19 Apr 2016 06:57:32 -0700 (PDT) In-Reply-To: References: Date: Tue, 19 Apr 2016 16:57:32 +0300 Message-ID: Subject: Re: [PATCH v2 3/3] ACPI / tables: Convert the initrd table override mechanisms to the table upgrade mechanism From: Octavian Purdila To: Lv Zheng Cc: "Rafael J. Wysocki" , "Rafael J. Wysocki" , Len Brown , Lv Zheng , lkml , "linux-acpi@vger.kernel.org" Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org X-Spam-Status: No, score=-7.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Apr 13, 2016 at 7:01 PM, Octavian Purdila wrote: > On Mon, Apr 11, 2016 at 5:13 AM, Lv Zheng wrote: >> >> This patch converts the initrd table override mechanism to the table >> upgrade mechanism by restricting its usage to the tables released with >> compatibility and more recent revision. >> >> This use case has been encouraged by the ACPI specification: >> 1. OEMID: >> An OEM-supplied string that identifies the OEM. >> 2. OEM Table ID: >> An OEM-supplied string that the OEM uses to identify the particular data >> table. This field is particularly useful when defining a definition >> block to distinguish definition block functions. OEM assigns each >> dissimilar table a new OEM Table Id. >> 3. OEM Revision: >> An OEM-supplied revision number. Larger numbers are assumed to be newer >> revisions. >> For OEMs, good practices will ensure consistency when assigning OEMID and >> OEM Table ID fields in any table. The intent of these fields is to allow >> for a binary control system that support services can use. Because many >> support function can be automated, it is useful when a tool can >> programatically determine which table release is a compatible and more >> recent revision of a prior table on the same OEMID and OEM Table ID. >> >> The facility can now be used by the vendors to upgrade wrong tables for bug >> fixing purpose, thus lockdep disabling taint is not suitable for it and it >> should be a default 'y' option to implement the spec encouraged use case. >> > > I agree that we should not force lockdep disabling. I wonder if we > should add a new taint (like the one I proposed in the overlay patch > set) to see in bug reports that the ACPI tables have been modified. > > Also, do we need a new config option? IMO it would be better if we can > keep the existing config option and do the following: > > * if CONFIG_ACPI_INITRD_TABLE_OVERRIDE is set: allow arbitrary > overrides (preserve the current behavior) > > * if CONFIG_ACPI_INITRD_TABLE_OVERRIDE is not set: allow upgrades > based on the revision number > > * allow adding new tables regardless of CONFIG_ACPI_INITRD_TABLE_OVERRIDE Here is possible implementation for that (compile tested only): break; @@ -497,7 +503,7 @@ static void __init acpi_table_initrd_init(void *data, size_t size) size -= offset; if (file.size < sizeof(struct acpi_table_header)) { - pr_err("ACPI OVERRIDE: Table smaller than ACPI header [%s%s]\n", + pr_err(PREFIX "initrd: Table smaller than ACPI header [%s%s]\n", cpio_path, file.name); continue; } @@ -509,17 +515,17 @@ static void __init acpi_table_initrd_init(void *data, size_t size) break; if (!table_sigs[sig]) { - pr_err("ACPI OVERRIDE: Unknown signature [%s%s]\n", + pr_err(PREFIX "initrd: Unknown signature [%s%s]\n", cpio_path, file.name); continue; } if (file.size != table->length) { - pr_err("ACPI OVERRIDE: File length does not match table length [%s%s]\n", + pr_err(PREFIX "initrd: File length does not match table length [%s%s]\n", cpio_path, file.name); continue; } if (acpi_table_checksum(file.data, table->length)) { - pr_err("ACPI OVERRIDE: Bad table checksum [%s%s]\n", + pr_err(PREFIX "initrd: Bad table checksum [%s%s]\n", cpio_path, file.name); continue; } @@ -593,6 +599,8 @@ acpi_table_initrd_override(struct acpi_table_header *existing_table, int table_index = 0; struct acpi_table_header *table; u32 table_length; + const char *action; + bool taint; *length = 0; *address = 0; @@ -611,17 +619,29 @@ acpi_table_initrd_override(struct acpi_table_header *existing_table, table_length = table->length; /* Only override tables matched */ - if (test_bit(table_index, acpi_initrd_installed) || - memcmp(existing_table->signature, table->signature, 4) || + if (memcmp(existing_table->signature, table->signature, 4) || memcmp(table->oem_table_id, existing_table->oem_table_id, ACPI_OEM_TABLE_ID_SIZE)) { acpi_os_unmap_memory(table, ACPI_HEADER_SIZE); goto next_table; } + if (existing_table->oem_revision >= table->oem_revision) { + action = "override"; + taint = TAINT_OVERRIDDEN_ACPI_TABLE; +#ifndef CONFIG_ACPI_INITRD_TABLE_OVERRIDE + acpi_os_unmap_memory(table, ACPI_HEADER_SIZE); + goto next_table; +#endif + } else { + taint = TAINT_OVERLAY_ACPI_TABLE; + action = "upgrade"; + } + + *length = table_length; *address = acpi_tables_addr + table_offset; - acpi_table_taint(existing_table); + acpi_table_taint(existing_table, taint, action); acpi_os_unmap_memory(table, ACPI_HEADER_SIZE); set_bit(table_index, acpi_initrd_installed); break; @@ -662,7 +682,7 @@ static void __init acpi_table_initrd_scan(void) goto next_table; } - acpi_table_taint(table); + acpi_table_taint(table, TAINT_OVERLAY_ACPI_TABLE, "install"); acpi_os_unmap_memory(table, ACPI_HEADER_SIZE); acpi_install_table(acpi_tables_addr + table_offset, TRUE); set_bit(table_index, acpi_initrd_installed); @@ -671,25 +691,6 @@ next_table: table_index++; } } -#else -static void __init acpi_table_initrd_init(void *data, size_t size) -{ -} - -static acpi_status -acpi_table_initrd_override(struct acpi_table_header *existing_table, - acpi_physical_address *address, - u32 *table_length) -{ - *table_length = 0; - *address = 0; - return AE_OK; -} - -static void __init acpi_table_initrd_scan(void) -{ -} -#endif /* CONFIG_ACPI_INITRD_TABLE_OVERRIDE */ acpi_status acpi_os_physical_table_override(struct acpi_table_header *existing_table, @@ -714,7 +715,8 @@ acpi_os_table_override(struct acpi_table_header *existing_table, *new_table = (struct acpi_table_header *)AmlCode; #endif if (*new_table != NULL) - acpi_table_taint(existing_table); + acpi_table_taint(existing_table, TAINT_OVERRIDDEN_ACPI_TABLE, + "override"); return AE_OK; } --- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 2e74dbf..d72edd6 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -435,14 +435,20 @@ static void __init check_multiple_madt(void) return; } -static void acpi_table_taint(struct acpi_table_header *table) +static void acpi_table_taint(struct acpi_table_header *table, int taint, + const char *action) { - pr_warn("Override [%4.4s-%8.8s], this is unsafe: tainting kernel\n", - table->signature, table->oem_table_id); - add_taint(TAINT_OVERRIDDEN_ACPI_TABLE, LOCKDEP_NOW_UNRELIABLE); + enum lockdep_ok lockdep = LOCKDEP_STILL_OK; + + pr_info(PREFIX "table %s [%4.4s-%6.6s-%8.8s]\n", action, + table->signature, table->oem_id, table->oem_table_id); + + if (taint == TAINT_OVERRIDDEN_ACPI_TABLE) + lockdep = LOCKDEP_NOW_UNRELIABLE; + + add_taint(taint, lockdep); } -#ifdef CONFIG_ACPI_INITRD_TABLE_OVERRIDE static u64 acpi_tables_addr; static int all_tables_size; @@ -471,9 +477,9 @@ static const char * const table_sigs[] = { #define ACPI_HEADER_SIZE sizeof(struct acpi_table_header) -#define ACPI_OVERRIDE_TABLES 64 -static struct cpio_data __initdata acpi_initrd_files[ACPI_OVERRIDE_TABLES]; -static DECLARE_BITMAP(acpi_initrd_installed, ACPI_OVERRIDE_TABLES); +#define ACPI_INITRD_TABLES 64 +static struct cpio_data __initdata acpi_initrd_files[ACPI_INITRD_TABLES]; +static DECLARE_BITMAP(acpi_initrd_installed, ACPI_INITRD_TABLES); #define MAP_CHUNK_SIZE (NR_FIX_BTMAPS << PAGE_SHIFT) @@ -488,7 +494,7 @@ static void __init acpi_table_initrd_init(void *data, size_t size) if (data == NULL || size == 0) return; - for (no = 0; no < ACPI_OVERRIDE_TABLES; no++) { + for (no = 0; no < ACPI_INITRD_TABLES; no++) { file = find_cpio_data(cpio_path, data, size, &offset); if (!file.data)