From patchwork Fri May 17 05:09:32 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chen Gong X-Patchwork-Id: 2581091 Return-Path: X-Original-To: patchwork-linux-acpi@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id EC9DFDF215 for ; Fri, 17 May 2013 05:15:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752827Ab3EQFPw (ORCPT ); Fri, 17 May 2013 01:15:52 -0400 Received: from mga01.intel.com ([192.55.52.88]:20256 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751490Ab3EQFPv (ORCPT ); Fri, 17 May 2013 01:15:51 -0400 Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga101.fm.intel.com with ESMTP; 16 May 2013 22:15:50 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.87,690,1363158000"; d="asc'?scan'208";a="335536961" Received: from gchen-sby.bj.intel.com (HELO localhost) ([10.238.158.225]) by fmsmga001.fm.intel.com with ESMTP; 16 May 2013 22:15:49 -0700 Date: Fri, 17 May 2013 01:09:32 -0400 From: Chen Gong To: Borislav Petkov Cc: tony.luck@intel.com, linux-acpi@vger.kernel.org Subject: Re: [PATCH 1/2] ACPI/APEI: Add parameter check before error injection Message-ID: <20130517050932.GA30122@gchen.bj.intel.com> Mail-Followup-To: Borislav Petkov , tony.luck@intel.com, linux-acpi@vger.kernel.org References: <1368671347-22415-1-git-send-email-gong.chen@linux.intel.com> <1368671347-22415-2-git-send-email-gong.chen@linux.intel.com> <20130516110503.GB31373@pd.tnic> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20130516110503.GB31373@pd.tnic> X-PGP-Key-ID: A43922C7 User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org On Thu, May 16, 2013 at 01:05:03PM +0200, Borislav Petkov wrote: > Date: Thu, 16 May 2013 13:05:03 +0200 > From: Borislav Petkov > To: Chen Gong > Cc: tony.luck@intel.com, linux-acpi@vger.kernel.org > Subject: Re: [PATCH 1/2] ACPI/APEI: Add parameter check before error > injection > User-Agent: Mutt/1.5.21 (2010-09-15) > > On Wed, May 15, 2013 at 10:29:06PM -0400, Chen Gong wrote: > > When param1 is enabled in EINJ but not assigned with a valid > > value, sometimes it will cause the error like below: > > > > APEI: Can not request [mem 0x7aaa7000-0x7aaa7007] for APEI EINJ Trigger > > registers > > > > It is because some firmware will access target address specified in > > param1 to trigger the error when injecting memory error. This will > > cause resource conflict with regular memory. So It must be removed > > from trigger table resources, but uncorrected param1/param2 > > combination will stop this action. Add extra check to avoid > > happening this error. > > > > Signed-off-by: Chen Gong > > --- > > drivers/acpi/apei/einj.c | 29 +++++++++++++++++++++++++---- > > kernel/resource.c | 1 + > > 2 files changed, 26 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c > > index 8d457b5..015546e 100644 > > --- a/drivers/acpi/apei/einj.c > > +++ b/drivers/acpi/apei/einj.c > > @@ -32,6 +32,7 @@ > > #include > > #include > > #include > > +#include > > #include > > > > #include "apei-internal.h" > > @@ -511,11 +512,31 @@ static int __einj_error_inject(u32 type, u64 param1, u64 param2) > > /* Inject the specified hardware error */ > > static int einj_error_inject(u32 type, u64 param1, u64 param2) > > { > > - int rc; > > + int rc = 0; > > + unsigned long pfn; > > + bool checkparam = false; > > + u64 param2_mask = 0xfffffffffffff000; > > > > - mutex_lock(&einj_mutex); > > - rc = __einj_error_inject(type, param1, param2); > > - mutex_unlock(&einj_mutex); > > + /* ensure param1/param2 existed & injection is memory related */ > > + if (param_extension || acpi5) { > > + if (type & 0x80000000) { > > if (type & BIT(31)) { > > better readable. > > even better if you define what bit 31 is > > #define INJ_TYPE BIT(31) > > > + if (vendor_flags == SETWA_FLAGS_MEM) > > + checkparam = true; > > + } else if (type & 0x38) > > what is 0x38? Also a macro define with readable name pls. > > > + checkparam = true; > > + } > > + if (checkparam) { > > + pfn = PFN_DOWN(param1 & param2); > > + if (!page_is_ram(pfn) || > > + ((param2 & param2_mask) != param2_mask)) > > Hmm, shouldn't this be: > > (param2 & param2_mask) != param2 > > ? > > > + rc = -EINVAL; > > + } > > This checkparam variable looks unneeded to me: > > if (type & INJ_TYPE) { > if (vendor_flags == SETWA_FLAGS_MEM || type & 0x38) { > ... > > Ok, I went and rewrote the function to show you what I mean. I've > also reversed the logic to flatten the indentation level (completely > untested, of course): > > static int einj_error_inject(u32 type, u64 param1, u64 param2) > { > int rc; > unsigned long pfn; > > /* ensure param1/param2 exist and we inject to real memory */ > if (!(param_extension || acpi5)) > goto inject; > > if (type & INJ_TYPE) > if (vendor_flags != SETWA_FLAGS_MEM) > goto inject; > else if (!(type & THREE_BIT_MASK)) > goto inject; > > pfn = PFN_DOWN(param1 & param2); > if (!page_is_ram(pfn) || ((param2 & PAGE_MASK) != param2)) > return -EINVAL; > > inject: > mutex_lock(&einj_mutex); > rc = __einj_error_inject(type, param1, param2); > mutex_unlock(&einj_mutex); > > return rc; > } > > -- > Regards/Gruss, > Boris. > > Sent from a fat crate under my desk. Formatting is fine. > -- Hi, Boris Thanks for your review. I agree with you about readability enhancement. IMHO, I don't think your attached patch has obvious improvment. I paste my updated patch here and please continue to review. I will send updated patch based on your further review. In this patch, I update previous confused definition at the same time. They are very little and no ill-efect so I don't write a new patch for it. diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c index 8d457b5..dba59be 100644 --- a/drivers/acpi/apei/einj.c +++ b/drivers/acpi/apei/einj.c @@ -32,6 +32,7 @@ #include #include #include +#include #include #include "apei-internal.h" @@ -41,6 +42,11 @@ #define SPIN_UNIT 100 /* 100ns */ /* Firmware should respond within 1 milliseconds */ #define FIRMWARE_TIMEOUT (1 * NSEC_PER_MSEC) +#define ACPI5_VENDOR_BIT BIT(31) +#define PAGE_MASK 0xfffffffffffff000 +#define MEM_ERROR_MASK (ACPI_EINJ_MEMORY_CORRECTABLE | \ + ACPI_EINJ_MEMORY_UNCORRECTABLE | \ + ACPI_EINJ_MEMORY_FATAL) /* * ACPI version 5 provides a SET_ERROR_TYPE_WITH_ADDRESS action. @@ -367,7 +373,7 @@ static int __einj_error_trigger(u64 trigger_paddr, u32 type, * This will cause resource conflict with regular memory. So * remove it from trigger table resources. */ - if ((param_extension || acpi5) && (type & 0x0038) && param2) { + if ((param_extension || acpi5) && (type & MEM_ERROR_MASK) && param2) { struct apei_resources addr_resources; apei_resources_init(&addr_resources); trigger_param_region = einj_get_trigger_parameter_region( @@ -427,7 +433,7 @@ static int __einj_error_inject(u32 type, u64 param1, u64 param2) struct set_error_type_with_address *v5param = einj_param; tototototoram->type = type; - if (type & 0x80000000) { + if (type & ACPI5_VENDOR_BIT) { switch (vendor_flags) { case SETWA_FLAGS_APICID: v5param->apicid = param1; @@ -512,6 +518,25 @@ static int __einj_error_inject(u32 type, u64 param1, u64 param2) static int einj_error_inject(u32 type, u64 param1, u64 param2) { int rc; + unsigned long pfn; + bool checkparam; + + /* ensure param1/param2 existed & injection is memory related */ + if (param_extension || acpi5) { + checkparam = false; + if (type & ACPI5_VENDOR_BIT) { + if (vendor_flags == SETWA_FLAGS_MEM) + checkparam = true; + } else if (type & MEM_ERROR_MASK) + checkparam = true; + + if (checkparam) { + pfn = PFN_DOWN(param1 & param2); + if (!page_is_ram(pfn) || + ((param2 & PAGE_MASK) != PAGE_MASK)) + return -EINVAL; + } + } mutex_lock(&einj_mutex); rc = __einj_error_inject(type, param1, param2); @@ -590,7 +615,7 @@ static int error_type_set(void *data, u64 val) * Vendor defined types have 0x80000000 bit set, and * are not enumerated by ACPI_EINJ_GET_ERROR_TYPE */ - vendor = val & 0x80000000; + vendor = val & ACPI5_VENDOR_BIT; tval = val & 0x7fffffff; /* Only one error type can be specified */ diff --git a/kernel/resource.c b/kernel/resource.c index d738698..77bf11a 100644 --- a/kernel/resource.c +++ b/kernel/resource.c @@ -409,6 +409,7 @@ int __weak page_is_ram(unsigned long pfn) { return walk_system_ram_range(pfn, 1, NULL, __is_ram) == 1; } +EXPORT_SYMBOL_GPL(page_is_ram); void __weak arch_remove_reservations(struct resource *avail) {