From mboxrd@z Thu Jan 1 00:00:00 1970 From: Borislav Petkov Subject: Re: [PATCH v6 14/18] ACPI / APEI: Split ghes_read_estatus() to read CPER length Date: Fri, 12 Oct 2018 19:25:34 +0200 Message-ID: <20181012172533.GG580@zn.tnic> References: <20180921221705.6478-1-james.morse@arm.com> <20180921221705.6478-15-james.morse@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20180921221705.6478-15-james.morse@arm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu To: James Morse Cc: jonathan.zhang@cavium.com, Rafael Wysocki , Tony Luck , linux-mm@kvack.org, Marc Zyngier , Catalin Marinas , Tyler Baicar , Will Deacon , Dongjiu Geng , linux-acpi@vger.kernel.org, Punit Agrawal , Naoya Horiguchi , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, Len Brown List-Id: linux-acpi@vger.kernel.org On Fri, Sep 21, 2018 at 11:17:01PM +0100, James Morse wrote: > ghes_read_estatus() reads the record address, then the record's > header, then performs some sanity checks before reading the > records into the provided estatus buffer. > > We either need to know the size of the records before we call > ghes_read_estatus(), or always provide a worst-case sized buffer, > as happens today. > > Add a function to peek at the record's header to find the size. This > will let the NMI path allocate the right amount of memory before reading > the records, instead of using the worst-case size, and having to copy > the records. > > Split ghes_read_estatus() to create ghes_peek_estatus() which > returns the address and size of the CPER records. > > Signed-off-by: James Morse > --- > drivers/acpi/apei/ghes.c | 55 ++++++++++++++++++++++++++++++---------- > 1 file changed, 41 insertions(+), 14 deletions(-) > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index 3028487d43a3..055176ed68ac 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -298,11 +298,12 @@ static void ghes_copy_tofrom_phys(void *buffer, u64 paddr, u32 len, > } > } > > -static int ghes_read_estatus(struct ghes *ghes, > - struct acpi_hest_generic_status *estatus, > - u64 *buf_paddr, int fixmap_idx) > +/* read the CPER block returning its address and size */ Make that comment a proper sentence: "./* ... Read the CPER ... and size. */ > +static int ghes_peek_estatus(struct ghes *ghes, int fixmap_idx, > + u64 *buf_paddr, u32 *buf_len) > { I find the functionality split a bit strange: ghes_peek_estatus() does peek *and* verify sizes. The latter belongs maybe better in ghes_read_estatus(). Together with the cper_estatus_check_header() call. Or maybe into a separate __ghes_check_estatus() to separate it all nicely. > struct acpi_hest_generic *g = ghes->generic; > + struct acpi_hest_generic_status estatus; > u32 len; > int rc; > > @@ -317,26 +318,23 @@ static int ghes_read_estatus(struct ghes *ghes, > if (!*buf_paddr) > return -ENOENT; > > - ghes_copy_tofrom_phys(estatus, *buf_paddr, > - sizeof(*estatus), 1, fixmap_idx); > - if (!estatus->block_status) { > + ghes_copy_tofrom_phys(&estatus, *buf_paddr, > + sizeof(estatus), 1, fixmap_idx); > + if (!estatus.block_status) { > *buf_paddr = 0; > return -ENOENT; > } > > rc = -EIO; > - len = cper_estatus_len(estatus); > - if (len < sizeof(*estatus)) > + len = cper_estatus_len(&estatus); > + if (len < sizeof(estatus)) > goto err_read_block; > if (len > ghes->generic->error_block_length) > goto err_read_block; > - if (cper_estatus_check_header(estatus)) > - goto err_read_block; > - ghes_copy_tofrom_phys(estatus + 1, > - *buf_paddr + sizeof(*estatus), > - len - sizeof(*estatus), 1, fixmap_idx); > - if (cper_estatus_check(estatus)) > + if (cper_estatus_check_header(&estatus)) > goto err_read_block; > + *buf_len = len; > + > rc = 0; > > err_read_block: > @@ -346,6 +344,35 @@ static int ghes_read_estatus(struct ghes *ghes, > return rc; > } > > +static int __ghes_read_estatus(struct acpi_hest_generic_status *estatus, > + u64 buf_paddr, size_t buf_len, > + int fixmap_idx) > +{ > + ghes_copy_tofrom_phys(estatus, buf_paddr, buf_len, 1, fixmap_idx); > + if (cper_estatus_check(estatus)) { > + if (printk_ratelimit()) > + pr_warning(FW_WARN GHES_PFX > + "Failed to read error status block!\n"); Then you won't have to have two identical messages: "Failed to read error status block!\n" which, when one sees them, is hard to figure out where exactly in the code that happened. > + return -EIO; > + } > + > + return 0; > +} > + > +static int ghes_read_estatus(struct ghes *ghes, > + struct acpi_hest_generic_status *estatus, > + u64 *buf_paddr, int fixmap_idx) > +{ > + int rc; > + u32 buf_len; > + > + rc = ghes_peek_estatus(ghes, fixmap_idx, buf_paddr, &buf_len); Also, if we have a __ghes_read_estatus() helper now, maybe prefixing ghes_peek_estatus() with "__" would make sense too... -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by kanga.kvack.org (Postfix) with ESMTP id 86F646B027C for ; Fri, 12 Oct 2018 13:25:42 -0400 (EDT) Received: by mail-wr1-f71.google.com with SMTP id c16-v6so8007413wrr.8 for ; Fri, 12 Oct 2018 10:25:42 -0700 (PDT) Received: from mail.skyhub.de (mail.skyhub.de. [2a01:4f8:190:11c2::b:1457]) by mx.google.com with ESMTPS id 30-v6si1573536wrr.440.2018.10.12.10.25.41 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 12 Oct 2018 10:25:41 -0700 (PDT) Date: Fri, 12 Oct 2018 19:25:34 +0200 From: Borislav Petkov Subject: Re: [PATCH v6 14/18] ACPI / APEI: Split ghes_read_estatus() to read CPER length Message-ID: <20181012172533.GG580@zn.tnic> References: <20180921221705.6478-1-james.morse@arm.com> <20180921221705.6478-15-james.morse@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20180921221705.6478-15-james.morse@arm.com> Sender: owner-linux-mm@kvack.org List-ID: To: James Morse Cc: linux-acpi@vger.kernel.org, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, linux-mm@kvack.org, Marc Zyngier , Christoffer Dall , Will Deacon , Catalin Marinas , Naoya Horiguchi , Rafael Wysocki , Len Brown , Tony Luck , Tyler Baicar , Dongjiu Geng , Xie XiuQi , Punit Agrawal , jonathan.zhang@cavium.com On Fri, Sep 21, 2018 at 11:17:01PM +0100, James Morse wrote: > ghes_read_estatus() reads the record address, then the record's > header, then performs some sanity checks before reading the > records into the provided estatus buffer. > > We either need to know the size of the records before we call > ghes_read_estatus(), or always provide a worst-case sized buffer, > as happens today. > > Add a function to peek at the record's header to find the size. This > will let the NMI path allocate the right amount of memory before reading > the records, instead of using the worst-case size, and having to copy > the records. > > Split ghes_read_estatus() to create ghes_peek_estatus() which > returns the address and size of the CPER records. > > Signed-off-by: James Morse > --- > drivers/acpi/apei/ghes.c | 55 ++++++++++++++++++++++++++++++---------- > 1 file changed, 41 insertions(+), 14 deletions(-) > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index 3028487d43a3..055176ed68ac 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -298,11 +298,12 @@ static void ghes_copy_tofrom_phys(void *buffer, u64 paddr, u32 len, > } > } > > -static int ghes_read_estatus(struct ghes *ghes, > - struct acpi_hest_generic_status *estatus, > - u64 *buf_paddr, int fixmap_idx) > +/* read the CPER block returning its address and size */ Make that comment a proper sentence: "./* ... Read the CPER ... and size. */ > +static int ghes_peek_estatus(struct ghes *ghes, int fixmap_idx, > + u64 *buf_paddr, u32 *buf_len) > { I find the functionality split a bit strange: ghes_peek_estatus() does peek *and* verify sizes. The latter belongs maybe better in ghes_read_estatus(). Together with the cper_estatus_check_header() call. Or maybe into a separate __ghes_check_estatus() to separate it all nicely. > struct acpi_hest_generic *g = ghes->generic; > + struct acpi_hest_generic_status estatus; > u32 len; > int rc; > > @@ -317,26 +318,23 @@ static int ghes_read_estatus(struct ghes *ghes, > if (!*buf_paddr) > return -ENOENT; > > - ghes_copy_tofrom_phys(estatus, *buf_paddr, > - sizeof(*estatus), 1, fixmap_idx); > - if (!estatus->block_status) { > + ghes_copy_tofrom_phys(&estatus, *buf_paddr, > + sizeof(estatus), 1, fixmap_idx); > + if (!estatus.block_status) { > *buf_paddr = 0; > return -ENOENT; > } > > rc = -EIO; > - len = cper_estatus_len(estatus); > - if (len < sizeof(*estatus)) > + len = cper_estatus_len(&estatus); > + if (len < sizeof(estatus)) > goto err_read_block; > if (len > ghes->generic->error_block_length) > goto err_read_block; > - if (cper_estatus_check_header(estatus)) > - goto err_read_block; > - ghes_copy_tofrom_phys(estatus + 1, > - *buf_paddr + sizeof(*estatus), > - len - sizeof(*estatus), 1, fixmap_idx); > - if (cper_estatus_check(estatus)) > + if (cper_estatus_check_header(&estatus)) > goto err_read_block; > + *buf_len = len; > + > rc = 0; > > err_read_block: > @@ -346,6 +344,35 @@ static int ghes_read_estatus(struct ghes *ghes, > return rc; > } > > +static int __ghes_read_estatus(struct acpi_hest_generic_status *estatus, > + u64 buf_paddr, size_t buf_len, > + int fixmap_idx) > +{ > + ghes_copy_tofrom_phys(estatus, buf_paddr, buf_len, 1, fixmap_idx); > + if (cper_estatus_check(estatus)) { > + if (printk_ratelimit()) > + pr_warning(FW_WARN GHES_PFX > + "Failed to read error status block!\n"); Then you won't have to have two identical messages: "Failed to read error status block!\n" which, when one sees them, is hard to figure out where exactly in the code that happened. > + return -EIO; > + } > + > + return 0; > +} > + > +static int ghes_read_estatus(struct ghes *ghes, > + struct acpi_hest_generic_status *estatus, > + u64 *buf_paddr, int fixmap_idx) > +{ > + int rc; > + u32 buf_len; > + > + rc = ghes_peek_estatus(ghes, fixmap_idx, buf_paddr, &buf_len); Also, if we have a __ghes_read_estatus() helper now, maybe prefixing ghes_peek_estatus() with "__" would make sense too... -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. From mboxrd@z Thu Jan 1 00:00:00 1970 From: bp@alien8.de (Borislav Petkov) Date: Fri, 12 Oct 2018 19:25:34 +0200 Subject: [PATCH v6 14/18] ACPI / APEI: Split ghes_read_estatus() to read CPER length In-Reply-To: <20180921221705.6478-15-james.morse@arm.com> References: <20180921221705.6478-1-james.morse@arm.com> <20180921221705.6478-15-james.morse@arm.com> Message-ID: <20181012172533.GG580@zn.tnic> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Fri, Sep 21, 2018 at 11:17:01PM +0100, James Morse wrote: > ghes_read_estatus() reads the record address, then the record's > header, then performs some sanity checks before reading the > records into the provided estatus buffer. > > We either need to know the size of the records before we call > ghes_read_estatus(), or always provide a worst-case sized buffer, > as happens today. > > Add a function to peek at the record's header to find the size. This > will let the NMI path allocate the right amount of memory before reading > the records, instead of using the worst-case size, and having to copy > the records. > > Split ghes_read_estatus() to create ghes_peek_estatus() which > returns the address and size of the CPER records. > > Signed-off-by: James Morse > --- > drivers/acpi/apei/ghes.c | 55 ++++++++++++++++++++++++++++++---------- > 1 file changed, 41 insertions(+), 14 deletions(-) > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index 3028487d43a3..055176ed68ac 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -298,11 +298,12 @@ static void ghes_copy_tofrom_phys(void *buffer, u64 paddr, u32 len, > } > } > > -static int ghes_read_estatus(struct ghes *ghes, > - struct acpi_hest_generic_status *estatus, > - u64 *buf_paddr, int fixmap_idx) > +/* read the CPER block returning its address and size */ Make that comment a proper sentence: "./* ... Read the CPER ... and size. */ > +static int ghes_peek_estatus(struct ghes *ghes, int fixmap_idx, > + u64 *buf_paddr, u32 *buf_len) > { I find the functionality split a bit strange: ghes_peek_estatus() does peek *and* verify sizes. The latter belongs maybe better in ghes_read_estatus(). Together with the cper_estatus_check_header() call. Or maybe into a separate __ghes_check_estatus() to separate it all nicely. > struct acpi_hest_generic *g = ghes->generic; > + struct acpi_hest_generic_status estatus; > u32 len; > int rc; > > @@ -317,26 +318,23 @@ static int ghes_read_estatus(struct ghes *ghes, > if (!*buf_paddr) > return -ENOENT; > > - ghes_copy_tofrom_phys(estatus, *buf_paddr, > - sizeof(*estatus), 1, fixmap_idx); > - if (!estatus->block_status) { > + ghes_copy_tofrom_phys(&estatus, *buf_paddr, > + sizeof(estatus), 1, fixmap_idx); > + if (!estatus.block_status) { > *buf_paddr = 0; > return -ENOENT; > } > > rc = -EIO; > - len = cper_estatus_len(estatus); > - if (len < sizeof(*estatus)) > + len = cper_estatus_len(&estatus); > + if (len < sizeof(estatus)) > goto err_read_block; > if (len > ghes->generic->error_block_length) > goto err_read_block; > - if (cper_estatus_check_header(estatus)) > - goto err_read_block; > - ghes_copy_tofrom_phys(estatus + 1, > - *buf_paddr + sizeof(*estatus), > - len - sizeof(*estatus), 1, fixmap_idx); > - if (cper_estatus_check(estatus)) > + if (cper_estatus_check_header(&estatus)) > goto err_read_block; > + *buf_len = len; > + > rc = 0; > > err_read_block: > @@ -346,6 +344,35 @@ static int ghes_read_estatus(struct ghes *ghes, > return rc; > } > > +static int __ghes_read_estatus(struct acpi_hest_generic_status *estatus, > + u64 buf_paddr, size_t buf_len, > + int fixmap_idx) > +{ > + ghes_copy_tofrom_phys(estatus, buf_paddr, buf_len, 1, fixmap_idx); > + if (cper_estatus_check(estatus)) { > + if (printk_ratelimit()) > + pr_warning(FW_WARN GHES_PFX > + "Failed to read error status block!\n"); Then you won't have to have two identical messages: "Failed to read error status block!\n" which, when one sees them, is hard to figure out where exactly in the code that happened. > + return -EIO; > + } > + > + return 0; > +} > + > +static int ghes_read_estatus(struct ghes *ghes, > + struct acpi_hest_generic_status *estatus, > + u64 *buf_paddr, int fixmap_idx) > +{ > + int rc; > + u32 buf_len; > + > + rc = ghes_peek_estatus(ghes, fixmap_idx, buf_paddr, &buf_len); Also, if we have a __ghes_read_estatus() helper now, maybe prefixing ghes_peek_estatus() with "__" would make sense too... -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.