from:"Kaneda, Erik"

RE: [PATCH v1 1/1] libnvdimm: Don't use GUID APIs against raw buffer

2021-04-19 Thread Kaneda, Erik

+Bob and Rafael

> -Original Message-
> From: Dan Williams 
> Sent: Friday, April 16, 2021 3:09 PM
> To: Andy Shevchenko 
> Cc: linux-nvdimm ; Linux Kernel Mailing List
> ; Verma, Vishal L
> ; Jiang, Dave ; Weiny, Ira
> ; Kaneda, Erik 
> Subject: Re: [PATCH v1 1/1] libnvdimm: Don't use GUID APIs against raw
> buffer
> 
> On Fri, Apr 16, 2021 at 1:42 PM Andy Shevchenko
>  wrote:
> >
> > On Fri, Apr 16, 2021 at 01:08:06PM -0700, Dan Williams wrote:
> > > [ add Erik ]
> > >
> > > On Fri, Apr 16, 2021 at 10:36 AM Andy Shevchenko
> > >  wrote:
> > > >
> > > > On Thu, Apr 15, 2021 at 05:37:54PM +0300, Andy Shevchenko wrote:
> > > > > Strictly speaking the comparison between guid_t and raw buffer
> > > > > is not correct. Return to plain memcmp() since the data structures
> > > > > haven't changed to use uuid_t / guid_t the current state of affairs
> > > > > is inconsistent. Either it should be changed altogether or left
> > > > > as is.
> > > >
> > > > Dan, please review this one as well. I think here you may agree with me.
> > >
> > > You know, this is all a problem because ACPICA is using a raw buffer.
> >
> > And this is fine. It might be any other representation as well.
> >
> > > Erik, would it be possible to use the guid_t type in ACPICA? That
> > > would allow NFIT to drop some ugly casts.
> >
> > guid_t is internal kernel type. If we ever decide to deviate from the 
> > current
> > representation it wouldn't be possible in case a 3rd party is using it 1:1
> > (via typedef or so).
> 
Hi Dan,

> I'm thinking something like ACPICA defining that space as a union with
> the correct typing just for Linux.

I'm assuming that you mean using something like guid_t type for ACPI data table 
fields like NFIT rather than objects returned by ACPI namespace object such as 
_DSD.

For ACPI data tables, we could to use macros so that different operating 
systems can provide their own definitions for a guid. For ACPICA, it will 
expands to a 16-byte array. Linux can have it's own definition that contains a 
union or their own guid type (guid_t). As long as the OS-supplied definition is 
16bytes, I don't think there would be an issue.

Bob, do you have any thoughts on this?
Erik

RE: [Devel] Re: [PATCH] ACPICA: Fix a typo

2021-03-29 Thread Kaneda, Erik




> -Original Message-
> From: Rafael J. Wysocki 
> Sent: Monday, March 29, 2021 5:48 AM
> To: Bhaskar Chowdhury ; Kaneda, Erik
> 
> Cc: Wysocki, Rafael J ; ACPI Devel Maling List
> ; open list:ACPI COMPONENT ARCHITECTURE
> (ACPICA) ; Linux Kernel Mailing List  ker...@vger.kernel.org>; Randy Dunlap 
> Subject: [Devel] Re: [PATCH] ACPICA: Fix a typo
> 
> On Fri, Mar 26, 2021 at 1:22 AM Bhaskar Chowdhury
>  wrote:
> >
> >
> > s/optimzation/optimization/
> >
> > Signed-off-by: Bhaskar Chowdhury 
> > ---
> >  include/acpi/acoutput.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/acpi/acoutput.h b/include/acpi/acoutput.h
> > index 1538a6853822..1b4c45815695 100644
> > --- a/include/acpi/acoutput.h
> > +++ b/include/acpi/acoutput.h
> > @@ -362,7 +362,7 @@
> >   *
> >   * A less-safe version of the macros is provided for optional use if the
> >   * compiler uses excessive CPU stack (for example, this may happen in the
> > - * debug case if code optimzation is disabled.)
> > + * debug case if code optimization is disabled.)
> >   */
> >
> >  /* Exit trace helper macro */
> > --
> 
> Erik, could you pick up this patch, please?  It is simple enough IMV ...

No problem, I'll pick it up

Erik
> ___
> Devel mailing list -- de...@acpica.org
> To unsubscribe send an email to devel-le...@acpica.org
> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s

RE: [PATCH] ACPICA: Fix a typo

2021-03-26 Thread Kaneda, Erik




> -Original Message-
> From: Bhaskar Chowdhury 
> Sent: Friday, March 26, 2021 12:49 PM
> To: Moore, Robert 
> Cc: Kaneda, Erik ; Wysocki, Rafael J
> ; l...@kernel.org; linux-a...@vger.kernel.org;
> de...@acpica.org; linux-kernel@vger.kernel.org; rdun...@infradead.org
> Subject: Re: [PATCH] ACPICA: Fix a typo
> 
> On 14:56 Fri 26 Mar 2021, Moore, Robert wrote:
> >Please make a pull request for this on our github.
> >Thanks,
> >Bob
> >
> 
> A pull request for this trivial spelling fix? Kindly be reasonable , it is
> just a single spelling fix, had it been many ,the suggestion could stand.
>
> Kindly, also , let me know aren't we  applying patches from the ML , or is
> there some specific rule for this project. I might be missing the basic stuff.

The code under drivers/acpi/acpica is from a separate open source project 
located here:

https://github.com/acpica/acpica

You submitting a pull request on the github facilitates our maintenance.
Please make the pull request there so that the codebases are easy to maintain.

For more info, read Documentation/driver-api/acpi/linuxized-acpica.rst

Erik
> 
> Thanks,
> Bhaskar
> >
> >-Original Message-
> >From: Bhaskar Chowdhury 
> >Sent: Thursday, March 25, 2021 5:19 PM
> >To: Moore, Robert ; Kaneda, Erik
> ; Wysocki, Rafael J ;
> l...@kernel.org; linux-a...@vger.kernel.org; de...@acpica.org; linux-
> ker...@vger.kernel.org
> >Cc: rdun...@infradead.org; Bhaskar Chowdhury 
> >Subject: [PATCH] ACPICA: Fix a typo
> >
> >
> >s/optimzation/optimization/
> >
> >Signed-off-by: Bhaskar Chowdhury 
> >---
> > include/acpi/acoutput.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/include/acpi/acoutput.h b/include/acpi/acoutput.h index
> 1538a6853822..1b4c45815695 100644
> >--- a/include/acpi/acoutput.h
> >+++ b/include/acpi/acoutput.h
> >@@ -362,7 +362,7 @@
> >  *
> >  * A less-safe version of the macros is provided for optional use if the
> >  * compiler uses excessive CPU stack (for example, this may happen in the
> >- * debug case if code optimzation is disabled.)
> >+ * debug case if code optimization is disabled.)
> >  */
> >
> > /* Exit trace helper macro */
> >--
> >2.26.2
> >

RE: slub freelist issue / BUG: unable to handle page fault for address: 000000003ffe0018

2021-03-23 Thread Kaneda, Erik



> -Original Message-
> From: Rafael J. Wysocki 
> Sent: Tuesday, March 23, 2021 12:03 PM
> To: Kirill A. Shutemov ; Kaneda, Erik
> 
> Cc: Wysocki, Rafael J ; Vegard Nossum
> ; Vlastimil Babka ; Rafael J.
> Wysocki ; Moore, Robert ;
> Kees Cook ; Christoph Lameter ;
> Andrew Morton ; Marco Elver
> ; Waiman Long ; LKML  ker...@vger.kernel.org>; Linux MM ; ACPI Devel
> Maling List ; Len Brown ;
> Steven Rostedt ; Jan Kiszka
> 
> Subject: Re: slub freelist issue / BUG: unable to handle page fault for
> address: 3ffe0018
> 
> On Tue, Mar 23, 2021 at 7:32 PM Kirill A. Shutemov 
> wrote:
> >
> > On Fri, Jun 12, 2020 at 02:26:58PM +0200, Rafael J. Wysocki wrote:
> > > On 6/11/2020 3:40 AM, Kaneda, Erik wrote:
> > > >
> > > > > -Original Message-
> > > > > From: Vegard Nossum 
> > > > > Sent: Friday, June 5, 2020 7:45 AM
> > > > > To: Vlastimil Babka ; Rafael J. Wysocki
> > > > > ; Moore, Robert ;
> Kaneda,
> > > > > Erik 
> > > > > Cc: Kees Cook ; Wysocki, Rafael J
> > > > > ; Christoph Lameter ;
> Andrew
> > > > > Morton ; Marco Elver
> ;
> > > > > Waiman Long ; LKML  > > > > ker...@vger.kernel.org>; Linux MM ; ACPI
> Devel
> > > > > Maling List ; Len Brown
> ;
> > > > > Steven Rostedt 
> > > > > Subject: Re: slub freelist issue / BUG: unable to handle page fault 
> > > > > for
> > > > > address: 3ffe0018
> > > > >
> > > > > On 2020-06-05 16:08, Vlastimil Babka wrote:
> > > > > > On 6/5/20 3:12 PM, Rafael J. Wysocki wrote:
> > > > > > > On Fri, Jun 5, 2020 at 2:48 PM Vegard Nossum
> > > > >  wrote:
> > > > > > > > On 2020-06-05 11:36, Vegard Nossum wrote:
> > > > > > > > > On 2020-06-05 11:11, Vlastimil Babka wrote:
> > > > > > > > > > On 6/4/20 8:46 PM, Vlastimil Babka wrote:
> > > > > > > > > > > On 6/4/20 7:57 PM, Kees Cook wrote:
> > > > > > > > > > > > On Thu, Jun 04, 2020 at 07:20:18PM +0200, Vegard
> Nossum wrote:
> > > > > > > > > > > > > On 2020-06-04 19:18, Vlastimil Babka wrote:
> > > > > > > > > > > > > > On 6/4/20 7:14 PM, Vegard Nossum wrote:
> > > > > > > > > > > > > > > Hi all,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I ran into a boot problem with latest linus/master
> > > > > > > > > > > > > > > (6929f71e46bdddbf1c4d67c2728648176c67c555)
> that manifests
> > > > > like this:
> > > > > > > > > > > > > > Hi, what's the .config you use?
> > > > > > > > > > > > > Pretty much x86_64 defconfig minus a few options (PCI,
> USB,
> > > > > > > > > > > > > ...)
> > > > > > > > > > > > Oh yes indeed. I immediately crash in the same way with
> this config.
> > > > > > > > > > > > I'll
> > > > > > > > > > > > start digging...
> > > > > > > > > > > >
> > > > > > > > > > > > (defconfig finishes boot)
> > > > > > > > > > > This is funny, booting with slub_debug=F results in:
> > > > > > > > > > > I'm not sure if it's ACPI or ftrace wrong here, but looks 
> > > > > > > > > > > like
> > > > > > > > > > > the changed free pointer offset merely exposes a bug in
> something
> > > > > > > > > > > else.
> > > > > > > > > > So, with Kees' patch reverted, booting with slub_debug=F
> (or even
> > > > > > > > > > more specific slub_debug=F,ftrace_event_field) also hits
> this bug
> > > > > > > > > > below. I wanted to bisect it, but v5.7 was also bad, and 
> > > > > > > > > > also
> > > > > > > > > > v5.6. Didn't try further in history. So it's not new at 
> > > > > > > > > > all, and
> > > > > > > > > > likely

RE: [PATCH] Enable ACPI_ADR_SPACE_PCI_CONFIG in acpi_gbl_default_address_spaces only when ACPI_PCI_CONFIGURED is defined

2021-03-03 Thread Kaneda, Erik

This looks good to me. Bob, do you have any comments?

Erik

> -Original Message-
> From: Weidong Cui 
> Sent: Monday, February 8, 2021 7:18 PM
> To: Moore, Robert ; Kaneda, Erik
> ; Wysocki, Rafael J ;
> Len Brown 
> Cc: Weidong Cui ; Xinyang Ge
> ; linux-a...@vger.kernel.org; de...@acpica.org; linux-
> ker...@vger.kernel.org
> Subject: [PATCH] Enable ACPI_ADR_SPACE_PCI_CONFIG in
> acpi_gbl_default_address_spaces only when ACPI_PCI_CONFIGURED is
> defined
> 
> Signed-off-by: Weidong Cui 
> Signed-off-by: Xinyang Ge 
> ---
>  drivers/acpi/acpica/evhandler.c | 2 ++
>  include/acpi/acconfig.h | 4 
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/acpi/acpica/evhandler.c b/drivers/acpi/acpica/evhandler.c
> index 5884eba04..4c25ad433 100644
> --- a/drivers/acpi/acpica/evhandler.c
> +++ b/drivers/acpi/acpica/evhandler.c
> @@ -26,7 +26,9 @@ acpi_ev_install_handler(acpi_handle obj_handle,
>  u8 acpi_gbl_default_address_spaces[ACPI_NUM_DEFAULT_SPACES] = {
>   ACPI_ADR_SPACE_SYSTEM_MEMORY,
>   ACPI_ADR_SPACE_SYSTEM_IO,
> +#ifdef ACPI_PCI_CONFIGURED
>   ACPI_ADR_SPACE_PCI_CONFIG,
> +#endif
>   ACPI_ADR_SPACE_DATA_TABLE
>  };
> 
> diff --git a/include/acpi/acconfig.h b/include/acpi/acconfig.h
> index a225eff49..790999028 100644
> --- a/include/acpi/acconfig.h
> +++ b/include/acpi/acconfig.h
> @@ -162,7 +162,11 @@
>  /* Maximum space_ids for Operation Regions */
> 
>  #define ACPI_MAX_ADDRESS_SPACE  255
> +#ifdef ACPI_PCI_CONFIGURED
>  #define ACPI_NUM_DEFAULT_SPACES 4
> +#else
> +#define ACPI_NUM_DEFAULT_SPACES 3
> +#endif
> 
>  /* Array sizes.  Used for range checking also */
> 
> --
> 2.24.3 (Apple Git-128)

RE: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using existing buffer"

2021-02-08 Thread Kaneda, Erik



> -Original Message-
> From: Ard Biesheuvel 
> Sent: Monday, February 8, 2021 11:14 AM
> To: Kaneda, Erik 
> Cc: Rafael J. Wysocki ; Shawn Guo
> ; Linux ARM  ker...@lists.infradead.org>; ACPI Devel Maling List  a...@vger.kernel.org>; Linux Kernel Mailing List  ker...@vger.kernel.org>; open list:ACPI COMPONENT ARCHITECTURE
> (ACPICA) ; Wysocki, Rafael J
> ; Len Brown ; Moore,
> Robert 
> Subject: Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using
> existing buffer"
> 
> On Mon, 8 Feb 2021 at 20:07, Kaneda, Erik  wrote:
> >
> >
> >
> > > -Original Message-
> > > From: Rafael J. Wysocki 
> > > Sent: Monday, February 8, 2021 5:01 AM
> > > To: Shawn Guo ; Ard Biesheuvel
> > > ; Kaneda, Erik 
> > > Cc: Linux ARM ; ACPI Devel Maling
> > > List ; Linux Kernel Mailing List  > > ker...@vger.kernel.org>; open list:ACPI COMPONENT ARCHITECTURE
> > > (ACPICA) ; Wysocki, Rafael J
> > > ; Len Brown ; Moore,
> > > Robert 
> > > Subject: Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by
> using
> > > existing buffer"
> > >
> > > On Sat, Feb 6, 2021 at 11:49 AM Shawn Guo 
> wrote:
> > > >
> > > > On Sat, Feb 06, 2021 at 09:49:37AM +0100, Ard Biesheuvel wrote:
> > > > > This reverts commit 32cf1a12cad43358e47dac8014379c2f33dfbed4.
> > > > >
> >
> > Hi Bob, Ard and Rafael,
> >
> > > > > The 'exisitng buffer' in this case is the firmware provided table, and
> > > > > we should not modify that in place. This fixes a crash on arm64 with
> > > > > initrd table overrides, in which case the DSDT is not mapped with
> > > > > read/write permissions.
> >
> > Since this code runs on basically every _HID and _CID invocation, I would
> have expected this kind of revert to come in for kernels that do not use 
> initrd
> override... So it sounds like there is a difference between how pages are
> mapped for initrd table overrides and tables exposed through the XSDT for
> ARM.. I think it would be easier for us to make these fixes in the future if 
> we
> could all come to a consensus on whether if we should assume that these
> pages are writable or not.
> >
> > Should we assume that all ACPI tables are non-writable and read only
> regardless of initrd override and architecture?
> >
> 
> ACPI tables are measured into the TPM on measured boot systems, and
> checksummed, so I don't think we should ever modify them in place.

I'm not knowledgeable on TPM but I'm curious - what happens when the TPM 
detects that these ACPI tables are modified?

> 
> But if we need code like this, it should be conditional at the very
> least, i.e., it should only rewrite _HIDs and _CIDs if they are
> incorrect to begin with.

I agree that this would be a more efficient approach

Erik

RE: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using existing buffer"

2021-02-08 Thread Kaneda, Erik

> -Original Message-
> From: Rafael J. Wysocki 
> Sent: Monday, February 8, 2021 5:01 AM
> To: Shawn Guo ; Ard Biesheuvel
> ; Kaneda, Erik 
> Cc: Linux ARM ; ACPI Devel Maling
> List ; Linux Kernel Mailing List  ker...@vger.kernel.org>; open list:ACPI COMPONENT ARCHITECTURE
> (ACPICA) ; Wysocki, Rafael J
> ; Len Brown ; Moore,
> Robert 
> Subject: Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using
> existing buffer"
> 
> On Sat, Feb 6, 2021 at 11:49 AM Shawn Guo  wrote:
> >
> > On Sat, Feb 06, 2021 at 09:49:37AM +0100, Ard Biesheuvel wrote:
> > > This reverts commit 32cf1a12cad43358e47dac8014379c2f33dfbed4.
> > >

Hi Bob, Ard and Rafael,

> > > The 'exisitng buffer' in this case is the firmware provided table, and
> > > we should not modify that in place. This fixes a crash on arm64 with
> > > initrd table overrides, in which case the DSDT is not mapped with
> > > read/write permissions.

Since this code runs on basically every _HID and _CID invocation, I would have 
expected this kind of revert to come in for kernels that do not use initrd 
override... So it sounds like there is a difference between how pages are 
mapped for initrd table overrides and tables exposed through the XSDT for ARM.. 
I think it would be easier for us to make these fixes in the future if we could 
all come to a consensus on whether if we should assume that these pages are 
writable or not.

Should we assume that all ACPI tables are non-writable and read only regardless 
of initrd override and architecture?

> > >
> > > Cc: Robert Moore 
> > > Cc: Erik Kaneda 
> > > Cc: "Rafael J. Wysocki" 
> > > Cc: Len Brown 
> > > Reported-by: Shawn Guo 
> > > Signed-off-by: Ard Biesheuvel 
> >
> > Tested-by: Shawn Guo 
> 
> Applied, thanks!
> 
> Erik, the upstream will need to sync up with this revert.

Ok sounds good.

Erik

RE: [Devel] Re: [PATCH] ACPICA: Common: Fix a typo

2021-01-25 Thread Kaneda, Erik




> -Original Message-
> From: Rafael J. Wysocki 
> Sent: Monday, January 25, 2021 8:37 AM
> To: Christophe JAILLET 
> Cc: Kaneda, Erik ; Wysocki, Rafael J
> ; ACPI Devel Maling List  a...@vger.kernel.org>; open list:ACPI COMPONENT ARCHITECTURE
> (ACPICA) ; Linux Kernel Mailing List  ker...@vger.kernel.org>; kernel-janit...@vger.kernel.org
> Subject: [Devel] Re: [PATCH] ACPICA: Common: Fix a typo
> 
> On Sun, Jan 24, 2021 at 12:35 PM Christophe JAILLET
>  wrote:
> >
> > This module is 'cmfsize', not 'cfsize'.
> > Fix it.
> >
> > Signed-off-by: Christophe JAILLET 
> 
> I'm leaving this one to Bob and Erik, thanks!

Pull request available here: https://github.com/acpica/acpica/pull/661
Once this is merged, it will be included as a part of the next ACPICA release 
and as a part of Linuxized patch series.

Thanks,
Erik
> 
> > ---
> >  tools/power/acpi/common/cmfsize.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/power/acpi/common/cmfsize.c
> b/tools/power/acpi/common/cmfsize.c
> > index 9ea2c0aeb86c..185b8c588e1d 100644
> > --- a/tools/power/acpi/common/cmfsize.c
> > +++ b/tools/power/acpi/common/cmfsize.c
> > @@ -1,7 +1,7 @@
> >  // SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> >
> /**
> 
> >   *
> > - * Module Name: cfsize - Common get file size function
> > + * Module Name: cmfsize - Common get file size function
> >   *
> >   * Copyright (C) 2000 - 2021, Intel Corp.
> >   *
> > --
> > 2.27.0
> >
> ___
> Devel mailing list -- de...@acpica.org
> To unsubscribe send an email to devel-le...@acpica.org
> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s

RE: [PATCH] ACPICA: fix -Wfallthrough

2021-01-21 Thread Kaneda, Erik



> -Original Message-
> From: Nick Desaulniers 
> Sent: Thursday, January 21, 2021 11:08 AM
> To: Rafael J. Wysocki ; Kaneda, Erik
> 
> Cc: Jon Hunter ; Moore, Robert
> ; Wysocki, Rafael J ;
> Gustavo A . R . Silva ; clang-built-linux  li...@googlegroups.com>; Len Brown ; ACPI Devel
> Maling List ; open list:ACPI COMPONENT
> ARCHITECTURE (ACPICA) ; Linux Kernel Mailing List
> ; linux-tegra 
> Subject: Re: [PATCH] ACPICA: fix -Wfallthrough
> 
> On Thu, Jan 21, 2021 at 11:03 AM Rafael J. Wysocki 
> wrote:
> >
> > On Thu, Jan 21, 2021 at 11:08 AM Jon Hunter 
> wrote:
> > >
> > >
> > > On 11/11/2020 02:11, Nick Desaulniers wrote:
> > > > The "fallthrough" pseudo-keyword was added as a portable way to
> denote
> > > > intentional fallthrough. This code seemed to be using a mix of
> > > > fallthrough comments that GCC recognizes, and some kind of lint
> marker.
> > > > I'm guessing that linter hasn't been run in a while from the mixed use
> > > > of the marker vs comments.
> > > >
> > > > Signed-off-by: Nick Desaulniers 
> > >
> > >
> > > I know this is not the exact version that was merged, I can't find it on
> > > the list, but looks like the version that was merged [0],
> >
> > It would be this patch:
> >
> > https://patchwork.kernel.org/project/linux-
> acpi/patch/20210115184826.2250-4-erik.kan...@intel.com/
> >
> > Nick, Erik?
> 
> oh, shit, looks like a line was dropped.  Here's what I sent upstream:
> https://github.com/acpica/acpica/pull/650/files#diff-
> cccd96e900e01f7224c81508cbddfb1af6fcfbff959d6bfb55123e1b9cad4e38R154
> 3
> Note in the patch Rafael links to that line is missing and there's
> instead an #ifdef that's empty.  Was this line accidentally dropped?

Looks like this line was dropped by ACPICA's Linux-ize scripts. I'll re-add it 
and send again.

Rafael, do you want me to re-send the series or do you want me to resend the 
specific commit? I don't mind either way.

Thanks,
Erik
> 
> >
> > > is causing build errors with older toolchains (GCC v6) ...
> > >
> > > /dvs/git/dirty/git-master_l4t-
> upstream/kernel/drivers/acpi/acpica/dscontrol.c: In function
> ‘acpi_ds_exec_begin_control_op’:
> > > /dvs/git/dirty/git-master_l4t-
> upstream/kernel/drivers/acpi/acpica/dscontrol.c:65:3: error:
> ‘ACPI_FALLTHROUGH’ undeclared (first use in this function)
> > >ACPI_FALLTHROUGH;
> > >^~~~
> > > /dvs/git/dirty/git-master_l4t-
> upstream/kernel/drivers/acpi/acpica/dscontrol.c:65:3: note: each
> undeclared identifier is reported only once for each function it appears in
> > > /dvs/git/dirty/git-master_l4t-
> upstream/kernel/scripts/Makefile.build:287: recipe for target
> 'drivers/acpi/acpica/dscontrol.o' failed
> > >
> > > Cheers
> > > Jon
> > >
> > > [0] https://github.com/acpica/acpica/commit/4b9135f5
> > >
> > > --
> > > nvpublic
> 
> 
> 
> --
> Thanks,
> ~Nick Desaulniers

RE: [PATCH] ACPICA: fix -Wfallthrough

2021-01-21 Thread Kaneda, Erik



> -Original Message-
> From: Nick Desaulniers 
> Sent: Thursday, January 21, 2021 11:08 AM
> To: Rafael J. Wysocki ; Kaneda, Erik
> 
> Cc: Jon Hunter ; Moore, Robert
> ; Wysocki, Rafael J ;
> Gustavo A . R . Silva ; clang-built-linux  li...@googlegroups.com>; Len Brown ; ACPI Devel
> Maling List ; open list:ACPI COMPONENT
> ARCHITECTURE (ACPICA) ; Linux Kernel Mailing List
> ; linux-tegra 
> Subject: Re: [PATCH] ACPICA: fix -Wfallthrough
> 
> On Thu, Jan 21, 2021 at 11:03 AM Rafael J. Wysocki 
> wrote:
> >
> > On Thu, Jan 21, 2021 at 11:08 AM Jon Hunter 
> wrote:
> > >
> > >
> > > On 11/11/2020 02:11, Nick Desaulniers wrote:
> > > > The "fallthrough" pseudo-keyword was added as a portable way to
> denote
> > > > intentional fallthrough. This code seemed to be using a mix of
> > > > fallthrough comments that GCC recognizes, and some kind of lint
> marker.
> > > > I'm guessing that linter hasn't been run in a while from the mixed use
> > > > of the marker vs comments.
> > > >
> > > > Signed-off-by: Nick Desaulniers 
> > >
> > >
> > > I know this is not the exact version that was merged, I can't find it on
> > > the list, but looks like the version that was merged [0],
> >
> > It would be this patch:
> >
> > https://patchwork.kernel.org/project/linux-
> acpi/patch/20210115184826.2250-4-erik.kan...@intel.com/
> >
> > Nick, Erik?
> 
> oh, shit, looks like a line was dropped.  Here's what I sent upstream:
> https://github.com/acpica/acpica/pull/650/files#diff-
> cccd96e900e01f7224c81508cbddfb1af6fcfbff959d6bfb55123e1b9cad4e38R154
> 3
> Note in the patch Rafael links to that line is missing and there's
> instead an #ifdef that's empty.  Was this line accidentally dropped?

Let me take a look...
> 
> >
> > > is causing build errors with older toolchains (GCC v6) ...
> > >
> > > /dvs/git/dirty/git-master_l4t-
> upstream/kernel/drivers/acpi/acpica/dscontrol.c: In function
> ‘acpi_ds_exec_begin_control_op’:
> > > /dvs/git/dirty/git-master_l4t-
> upstream/kernel/drivers/acpi/acpica/dscontrol.c:65:3: error:
> ‘ACPI_FALLTHROUGH’ undeclared (first use in this function)
> > >ACPI_FALLTHROUGH;
> > >^~~~
> > > /dvs/git/dirty/git-master_l4t-
> upstream/kernel/drivers/acpi/acpica/dscontrol.c:65:3: note: each
> undeclared identifier is reported only once for each function it appears in
> > > /dvs/git/dirty/git-master_l4t-
> upstream/kernel/scripts/Makefile.build:287: recipe for target
> 'drivers/acpi/acpica/dscontrol.o' failed
> > >
> > > Cheers
> > > Jon
> > >
> > > [0] https://github.com/acpica/acpica/commit/4b9135f5
> > >
> > > --
> > > nvpublic
> 
> 
> 
> --
> Thanks,
> ~Nick Desaulniers

RE: [RFC PATCH v3 02/16] cxl/acpi: Add an acpi_cxl module for the CXL interconnect

2021-01-13 Thread Kaneda, Erik



> -Original Message-
> From: Dan Williams 
> Sent: Tuesday, January 12, 2021 11:44 AM
> To: Jonathan Cameron 
> Cc: Widawsky, Ben ; linux-...@vger.kernel.org;
> Verma, Vishal L ; Linux Kernel Mailing List  ker...@vger.kernel.org>; Linux PCI ; Weiny, Ira
> ; Kelley, Sean V ; Wysocki,
> Rafael J ; Bjorn Helgaas ;
> Jon Masters ; Chris Browy  design.com>; Randy Dunlap ; Christoph Hellwig
> ; daniel@alibaba-inc.com; Moore, Robert
> ; Kaneda, Erik 
> Subject: Re: [RFC PATCH v3 02/16] cxl/acpi: Add an acpi_cxl module for the
> CXL interconnect
> 
> On Tue, Jan 12, 2021 at 10:44 AM Jonathan Cameron
>  wrote:
> >
> > On Mon, 11 Jan 2021 14:51:06 -0800
> > Ben Widawsky  wrote:
> >
> > > From: Vishal Verma 
> > >
> > > Add an acpi_cxl module to coordinate the ACPI portions of the CXL
> > > (Compute eXpress Link) interconnect. This driver binds to ACPI0017
> > > objects in the ACPI tree, and coordinates access to the resources
> > > provided by the ACPI CEDT (CXL Early Discovery Table).
> > >
> > > It also coordinates operations of the root port _OSC object to notify
> > > platform firmware that the OS has native support for the CXL
> > > capabilities of endpoints.
> > >
> > > Note: the actbl1.h changes are speculative. The expectation is that they
> > > will arrive through the ACPICA tree in due time.
> >
> > I would pull the ACPICA changes out into a precursor patch.
> 
> 
> >
> > >
> > > Cc: Ben Widawsky 
> > > Cc: Dan Williams 
> > > Signed-off-by: Vishal Verma 
> > > Signed-off-by: Ben Widawsky 
> >
> > Hi,
> >
> > I think it would be good to also add CEDT to the list in 
> > drivers/acpi/tables.c
> > so that we can dump it from /sys/firmware/acpi/tables/ and potentially
> > override it from an initrd.
> 
> ACPICA changes will eventually come through the ACPI tree not this patch
> set.
> 
> 
> >
> > https://elixir.bootlin.com/linux/v5.11-
> rc3/source/drivers/acpi/tables.c#L482
> > Can be very helpful whilst debugging.  Related to that, anyone know if
> anyone
> > has acpica patches so we can have iasl -d work on the table?  Would
> probably
> > be useful but I'd rather not duplicate work if it's already done.
> >
> 
> The supplemental tables described here:
> 
> https://www.uefi.org/acpi
> 
> ...do eventually make there way into ACPICA. Added Bob and Erik in
> case they can comment on when CEDT and CDAT support will be picked up.

We would be happy to add support. I think Ben has reached out to me earlier 
about something like this but I haven’t had a chance to implement... Sorry 
about the delay.. How soon is the iASL/ACPICA support needed for CDAT and CDET?

Erik

RE: [PATCH] ACPICA: Fix a soft-lockup on large systems

2020-09-29 Thread Kaneda, Erik




> -Original Message-
> From: Qian Cai 
> Sent: Tuesday, September 29, 2020 11:35 AM
> To: Moore, Robert ; Kaneda, Erik
> ; Wysocki, Rafael J 
> Cc: Len Brown ; linux-a...@vger.kernel.org;
> de...@acpica.org; linux-kernel@vger.kernel.org; Paul E. McKenney
> 
> Subject: [PATCH] ACPICA: Fix a soft-lockup on large systems
> 
> It could take a long time in the loop of acpi_ns_walk_namespace() on
> large systems due to there are many nodes in ACPI namespace, and then
> trigger a soft-lockup. Fix it by adding cond_resched() within the loop.
> 
> [   70.533393] watchdog: BUG: soft lockup - CPU#25 stuck for 22s!
> [swapper/0:1]
> [   70.533438] Modules linked in:
> [   70.533468] irq event stamp: 26257732
> [   70.533489] hardirqs last  enabled at (26257731): []
> __slab_alloc+0xa8/0xc8
> [   70.533505] hardirqs last disabled at (26257732): []
> el1_irq+0x7c/0x140
> el1_irq at arch/arm64/kernel/entry.S:650
> [   70.533520] softirqs last  enabled at (26197382): []
> efi_header_end+0xa90/0x10bc
> [   70.533535] softirqs last disabled at (26197377): []
> irq_exit+0x2c4/0x348
> [   70.533551] CPU: 25 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc7-next-
> 20200929 #1
> [   70.533563] Hardware name: HPE Apollo 70 /C01_APACHE_MB
>  ,
> BIOS L50_5.13_1.15 05/08/2020
> [   70.533577] pstate: 6049 (nZCv daif +PAN -UAO -TCO BTYPE=--)
> [   70.533593] pc : arch_local_irq_restore+0x4/0x8
> [   70.533605] lr : __slab_alloc+0xb0/0xc8
> [   70.533617] sp : 05bd61a0
> [   70.533628] x29: 05bd61a0 x28: 000e
> [   70.533653] x27: 0087b41c8ff8 x26: 0080
> [   70.533677] x25: a000190db000 x24: 000e788d6c10
> [   70.533700] x23: a00010cb41c8 x22: 
> [   70.533723] x21: 00012b20 x20: 0087b41cde10
> [   70.533746] x19:  x18: 1fffe001cf1193ca
> [   70.533768] x17:  x16: 
> [   70.533791] x15: 00071ccc x14: 00071ccc
> [   70.533813] x13: 80b7ac1d x12: 1fffe0b7ac1c
> [   70.533836] x11: 1fffe0b7ac1c x10: 80b7ac1c
> [   70.533859] x9 : dfffa000 x8 : 05bd60e7
> [   70.533882] x7 : f200 x6 : dfffa000
> [   70.533904] x5 : f2f2f200 x4 : f2f2f2f2
> [   70.533927] x3 : 1fffe0b6ddca x2 : a00019bf2000
> [   70.533950] x1 : 0190a943 x0 : 
> [   70.533973] Call trace:
> [   70.533987]  arch_local_irq_restore+0x4/0x8
> [   70.534000]  kmem_cache_alloc+0x35c/0x3c0
> [   70.534015]  fill_pool+0x278/0x588
> [   70.534028]  __debug_object_init+0x8c/0x1100
> [   70.534041]  debug_object_activate+0x234/0x448
> [   70.534055]  call_rcu+0x38/0x630
> [   70.534070]  put_object+0x84/0xc0
> [   70.534082]  __delete_object+0xc4/0x110
> [   70.534095]  delete_object_full+0x18/0x20
> [   70.534110]  kmemleak_free+0x2c/0x38
> [   70.534122]  slab_free_freelist_hook+0x15c/0x240
> [   70.534135]  kmem_cache_free+0x10c/0x518
> [   70.534150]  acpi_os_release_object+0xc/0x18
> [   70.534165]  acpi_ut_delete_object_desc+0xa8/0xac
> [   70.534177]  acpi_ut_update_ref_count.part.2+0x33c/0x788
> [   70.534190]  acpi_ut_update_object_reference+0x304/0x42c
> [   70.534203]  acpi_ut_remove_reference+0x64/0x74
> [   70.534218]  acpi_ds_store_object_to_local+0x2d8/0x300
> [   70.534231]  acpi_ex_store+0x600/0x658
> [   70.534244]  acpi_ex_opcode_1A_0T_1R+0x3e4/0xb34
> [   70.534257]  acpi_ds_exec_end_op+0x338/0xad0
> [   70.534270]  acpi_ps_parse_loop+0xdb4/0x1020
> [   70.534282]  acpi_ps_parse_aml+0x1f0/0x614
> [   70.534295]  acpi_ps_execute_method+0x500/0x508
> [   70.534308]  acpi_ns_evaluate+0x680/0x7b4
> [   70.534320]  acpi_ut_evaluate_object+0xc4/0x30c
> [   70.534333]  acpi_rs_get_method_data+0x84/0xd8
> [   70.534345]  acpi_walk_resources+0x13c/0x17c
> [   70.534359]  __acpi_dev_get_resources+0x150/0x1d8
> [   70.534371]  acpi_dev_get_resources+0x14/0x20
> [   70.534384]  acpi_init_device_object+0x698/0x10b8
> [   70.534396]  acpi_add_single_object+0xf8/0x1028
> [   70.534408]  acpi_bus_check_add+0x160/0x3f8
> [   70.534421]  acpi_ns_walk_namespace+0x1f4/0x298
> [   70.534433]  acpi_walk_namespace+0xa4/0xe8
> [   70.534446]  acpi_bus_scan+0xe0/0xf0
> [   70.534460]  acpi_scan_init+0x218/0x51c
> [   70.534472]  acpi_init+0x45c/0x4e4
> [   70.534485]  do_one_initcall+0x168/0xb60
> [   70.534498]  kernel_init_freeable+0x698/0x724
> [   70.534511]  kernel_init+0x10/0x11c
> [   70.534524]  ret_from_fork+0x10/0x18
> [  113.641710] rcu: INFO: rcu_sched self-detected stall on CPU
> [  113.641774] rcu: 25-: (6495 ticks this GP)
> idle=cbe/1/0x4002 softirq=772/772 fqs=3246
> [

RE: [PATCH] ACPI: actypes.h: drop a duplicated word

2020-07-27 Thread Kaneda, Erik



> -Original Message-
> From: Rafael J. Wysocki 
> Sent: Monday, July 27, 2020 5:44 AM
> To: Randy Dunlap ; Kaneda, Erik
> 
> Cc: Linux Kernel Mailing List ; Rafael J.
> Wysocki ; Len Brown ; ACPI Devel
> Maling List ; Moore, Robert
> 
> Subject: Re: [PATCH] ACPI: actypes.h: drop a duplicated word
> 
> On Sun, Jul 19, 2020 at 2:27 AM Randy Dunlap 
> wrote:
> >
> > Drop the repeated word "an" in a comment.
> >
> > Signed-off-by: Randy Dunlap 
> > Cc: "Rafael J. Wysocki" 
> > Cc: Len Brown 
> > Cc: linux-a...@vger.kernel.org
> > ---
> >  include/acpi/actypes.h |2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > --- linux-next-20200717.orig/include/acpi/actypes.h
> > +++ linux-next-20200717/include/acpi/actypes.h
> > @@ -824,7 +824,7 @@ typedef u8 acpi_adr_space_type;
> >   *
> >   * Note: A Data Table region is a special type of operation region
> >   * that has its own AML opcode. However, internally, the AML
> > - * interpreter simply creates an operation region with an an address
> > + * interpreter simply creates an operation region with an address
> >   * space type of ACPI_ADR_SPACE_DATA_TABLE.
> >   */
> >  #define ACPI_ADR_SPACE_DATA_TABLE   (acpi_adr_space_type) 0x7E
> /* Internal to ACPICA only */
> 
> This is an ACPICA header file, so it's better to route the change
> through the upstream.
> 
> Erik, can you pick up this one, please?

Done.

I made a pull request here: https://github.com/acpica/acpica/pull/618
It will be in the next ACPICA release.

Thanks!
Erik

RE: [RFT][PATCH v2 3/4] ACPICA: Preserve memory opregion mappings if supported by OS

2020-06-26 Thread Kaneda, Erik




> -Original Message-
> From: Rafael J. Wysocki 
> Sent: Monday, June 22, 2020 7:02 AM
> To: Williams, Dan J ; Kaneda, Erik
> 
> Cc: Wysocki, Rafael J ; Len Brown
> ; Borislav Petkov ; Weiny, Ira
> ; James Morse ; Myron
> Stowe ; Andy Shevchenko
> ; linux-kernel@vger.kernel.org; linux-
> a...@vger.kernel.org; linux-nvd...@lists.01.org; Moore, Robert
> 
> Subject: [RFT][PATCH v2 3/4] ACPICA: Preserve memory opregion mappings
> if supported by OS
> 
> From: "Rafael J. Wysocki" 
> 
> The ACPICA's strategy with respect to the handling of memory mappings
> associated with memory operation regions is to avoid mapping the
> entire region at once which may be problematic at least in principle
> (for example, it may lead to conflicts with overlapping mappings
> having different attributes created by drivers).  It may also be
> wasteful, because memory opregions on some systems take up vast
> chunks of address space while the fields in those regions actually
> accessed by AML are sparsely distributed.
> 
> For this reason, a one-page "window" is mapped for a given opregion
> on the first memory access through it and if that "window" does not
> cover an address range accessed through that opregion subsequently,
> it is unmapped and a new "window" is mapped to replace it.  Next,
> if the new "window" is not sufficient to access memory through the
> opregion in question in the future, it will be replaced with yet
> another "window" and so on.  That may lead to a suboptimal sequence
> of memory mapping and unmapping operations, for example if two fields
> in one opregion separated from each other by a sufficiently wide
> chunk of unused address space are accessed in an alternating pattern.
> 
> The situation may still be suboptimal if the deferred unmapping
> introduced previously is supported by the OS layer.  For instance,
> the alternating memory access pattern mentioned above may produce
> a relatively long list of mappings to release with substantial
> duplication among the entries in it, which could be avoided if
> acpi_ex_system_memory_space_handler() did not release the mapping
> used by it previously as soon as the current access was not covered
> by it.
> 
> In order to improve that, modify acpi_ex_system_memory_space_handler()
> to take advantage of the memory mappings reference counting at the OS
> level if a suitable interface is provided.
> 
Hi,

> Namely, if ACPI_USE_FAST_PATH_MAPPING is set, the OS is expected to
> implement acpi_os_map_memory_fast_path() that will return NULL if
> there is no mapping covering the given address range known to it.
> If such a mapping is there, however, its reference counter will be
> incremented and a pointer representing the requested virtual address
> will be returned right away without any additional consequences.

I do not fully understand why this is under a #ifdef. Is this to support 
operating systems that might not want to add support for this behavior?

Also, instead of using the terminology fast_path, I think it would be easier to 
use terminology that describes the mechanism..
It might be easier for other Operating systems to understand something like 
acpi_os_map_preserved_memory or acpi_os_map_sysmem_opregion_memory.

Thanks,
Erik
> 
> That allows acpi_ex_system_memory_space_handler() to acquire
> additional references to all new memory mappings with the help
> of acpi_os_map_memory_fast_path() so as to retain them until the
> memory opregions associated with them go away.  The function will
> still use a new "window" mapping if the current one does not
> cover the address range at hand, but it will avoid unmapping the
> current one right away by adding it to a list of "known" mappings
> associated with the given memory opregion which will be deleted at
> the opregion deactivation time.  The mappings in that list can be
> used every time a "new window" is needed so as to avoid overhead
> related to the mapping and unmapping of memory.
> 
> Signed-off-by: Rafael J. Wysocki 
> ---
>  drivers/acpi/acpica/acinterp.h |   4 +
>  drivers/acpi/acpica/evrgnini.c |   7 +-
>  drivers/acpi/acpica/exregion.c | 159
> -
>  3 files changed, 162 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/acinterp.h b/drivers/acpi/acpica/acinterp.h
> index 1f1026fb06e9..db9c279baa2e 100644
> --- a/drivers/acpi/acpica/acinterp.h
> +++ b/drivers/acpi/acpica/acinterp.h
> @@ -479,8 +479,12 @@ void acpi_ex_pci_cls_to_string(char *dest, u8
> class_code[3]);
> 
>  u8 acpi_is_valid_space_id(u8 space_id);
> 
> +struct acpi_mem_space_context
> *acpi_ex_alloc_mem_space_context(vo

RE: [PATCH] acpi: disallow loading configfs acpi tables when locked down

2020-06-17 Thread Kaneda, Erik



> -Original Message-
> From: linux-acpi-ow...@vger.kernel.org  ow...@vger.kernel.org> On Behalf Of Ard Biesheuvel
> Sent: Wednesday, June 17, 2020 1:38 AM
> To: Jason A. Donenfeld 
> Cc: Len Brown ; Rafael J. Wysocki ;
> LKML ; ACPI Devel Maling List  a...@vger.kernel.org>; Kernel Hardening  harden...@lists.openwall.com>
> Subject: Re: [PATCH] acpi: disallow loading configfs acpi tables when locked
> down
> 
> On Wed, 17 Jun 2020 at 00:21, Jason A. Donenfeld 
> wrote:
> >
> > Hi Rafael, Len,
> >
> > Looks like I should have CC'd you on this patch. This is probably
> > something we should get into 5.8-rc2, so that it can then get put into
> > stable kernels, as some people think this is security sensitive.
> > Bigger picture is this:
> >
> > https://data.zx2c4.com/american-unsigned-language-2.gif
> > https://data.zx2c4.com/american-unsigned-language-2-fedora-5.8.png
> >
Hi,

> > Also, somebody mentioned to me that Microsoft's ACPI implementation
> > disallows writes to system memory as a security mitigation. I haven't
> > looked at what that actually entails, but I wonder if entirely
> > disabling support for ACPI_ADR_SPACE_SYSTEM_MEMORY would be
> sensible.

No, Windows uses these as well. They might have some restriction on which areas 
are allowed though.
With that said, there are plenty of use cases (SMI handlers, power management, 
etc) where this is needed.
Disabling SystemMemory would definitely break existing systems.

Erik
> > I haven't looked at too many DSDTs. Would that break real hardware, or
> > does nobody do that? Alternatively, the range of acceptable addresses
> > for SystemMemory could exclude kernel memory. Would that break
> > anything? Have you heard about Microsoft's mitigation to know more
> > details on what they figured out they could safely restrict without
> > breaking hardware? Either way, food for thought I suppose.
> >
> 
> ACPI_ADR_SPACE_SYSTEM_MEMORY may be used for everything that is
> memory
> mapped, i.e., PCIe ECAM space, GPIO control registers etc.
> 
> I agree that using ACPI_ADR_SPACE_SYSTEM_MEMORY for any memory that
> is
> under the kernel's control is a bad idea, and this should be easy to
> filter out: the SystemMemory address space handler needs the ACPI
> support routines to map the physical addresses used by AML into
> virtual kernel addresses, so all these accesses go through
> acpi_os_ioremap(). So as a first step, it should be reasonable to put
> a lockdown check there, and fail any access to OS owned memory if
> lockdown is enabled, and print a warning if it is not.

RE: [Devel] Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

2020-06-16 Thread Kaneda, Erik



> -Original Message-
> From: Nick Desaulniers 
> Sent: Thursday, June 11, 2020 10:06 AM
> To: Kaneda, Erik 
> Cc: Jung-uk Kim ; Wysocki, Rafael J
> ; Ard Biesheuvel ;
> dvyu...@google.com; gli...@google.com; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> lorenzo.pieral...@arm.com; mark.rutl...@arm.com; p...@google.com;
> r...@rjwysocki.net; w...@kernel.org; sta...@vger.kernel.org; linux-
> a...@vger.kernel.org; de...@acpica.org
> Subject: Re: [Devel] Re: [PATCH] ACPICA: fix UBSAN warning using
> __builtin_offsetof
> 
> On Thu, Jun 11, 2020 at 9:45 AM Kaneda, Erik 
> wrote:
> >
> > > From: Jung-uk Kim 
> > >
> > > Actually, I think we should let platform-specific acfoo.h decide what to
> > > do here, i.e.,
> >
> > That's a better solution. For Linux, it would look something like this:
> >
> > --- a/include/acpi/actypes.h
> > +++ b/include/acpi/actypes.h
> > @@ -508,10 +508,15 @@ typedef u64 acpi_integer;
> >
> >  #define ACPI_TO_POINTER(i)  ACPI_CAST_PTR (void, (acpi_size) 
> > (i))
> >  #define ACPI_TO_INTEGER(p)  ACPI_PTR_DIFF (p, (void *) 0)
> > -#define ACPI_OFFSET(d, f)   ACPI_PTR_DIFF (&(((d *) 0)->f), 
> > (void *)
> 0)
> >  #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
> >  #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
> >
> > +/* Platforms may want to define their own ACPI_OFFSET */
> > +
> > +#ifndef ACPI_OFFSET
> > +#define ACPI_OFFSET(d, f)   ACPI_PTR_DIFF (&(((d *) 0)->f), 
> > (void
> *) 0)
> > +#endif
> > +
> >  /* Optimizations for 4-character (32-bit) acpi_name manipulation */
> >
> >  #ifndef ACPI_MISALIGNMENT_NOT_SUPPORTED
> > diff --git a/include/acpi/platform/aclinux.h
> b/include/acpi/platform/aclinux.h
> > index 987e2af7c335..5d1ca6015fce 100644
> > --- a/include/acpi/platform/aclinux.h
> > +++ b/include/acpi/platform/aclinux.h
> > @@ -71,6 +71,11 @@
> >  #undef ACPI_DEBUG_DEFAULT
> >  #define ACPI_DEBUG_DEFAULT  (ACPI_LV_INFO | ACPI_LV_REPAIR)
> >
> > +/* Use gcc's builtin offset instead of the default */
> > +
> > +#undef ACPI_OFFSET
> > +#define ACPI_OFFSET(a,b)__builtin_offsetof(a,b)
> > +
> >  #ifndef CONFIG_ACPI
> >
> 
Hi,

Sorry about the delayed response.

> Looks good at first glance.  Wouldn't actypes.h need to include
> platform/acenv.h first though?  Otherwise you put some header
> inclusion order dependency on folks who include actypes.h to first
> include acenv.h otherwise we're not getting the definition in terms of
> __builtin_offsetof.

Actypes.h is intended for ACPICA use. For files using ACPI_OFFSET, they include 
headers like acpi.h that ends up including aclinux.h before including 
actypes.h. For those using ACPI_OFFSET, precedence will be given to the 
definition in aclinux.h before falling back to the definition in actypes.

Erik

> 
> --
> Thanks,
> ~Nick Desaulniers

RE: [RFT][PATCH 2/3] ACPICA: Remove unused memory mappings on interpreter exit

2020-06-11 Thread Kaneda, Erik




> -Original Message-
> From: Rafael J. Wysocki 
> Sent: Wednesday, June 10, 2020 5:22 AM
> To: Williams, Dan J 
> Cc: Kaneda, Erik ; Wysocki, Rafael J
> ; Len Brown ; Borislav
> Petkov ; Weiny, Ira ; James Morse
> ; Myron Stowe ;
> Andy Shevchenko ; linux-
> ker...@vger.kernel.org; linux-a...@vger.kernel.org; linux-
> nvd...@lists.01.org; Moore, Robert 
> Subject: [RFT][PATCH 2/3] ACPICA: Remove unused memory mappings on
> interpreter exit
> 
> From: "Rafael J. Wysocki" 
> 
> For transient memory opregions that are created dynamically under
> the namespace and interpreter mutexes and go away quickly, there
> still is the problem that removing their memory mappings may take
> significant time and so doing that while holding the mutexes should
> be avoided.
> 
> For example, unmapping a chunk of memory associated with a memory
> opregion in Linux involves running synchronize_rcu_expedited()
> which really should not be done with the namespace mutex held.
> 
> To address that problem, notice that the unused memory mappings left
> behind by the "dynamic" opregions that went away need not be unmapped
> right away when the opregion is deactivated.  Instead, they may be
> unmapped when exiting the interpreter, after the namespace and
> interpreter mutexes have been dropped (there's one more place dealing
> with opregions in the debug code that can be treated analogously).
> 
> Accordingly, change acpi_ev_system_memory_region_setup() to put
> the unused mappings into a global list instead of unmapping them
> right away and add acpi_ev_system_release_memory_mappings() to
> be called when leaving the interpreter in order to unmap the
> unused memory mappings in the global list (which is protected
> by the namespace mutex).
> 
> Signed-off-by: Rafael J. Wysocki 
> ---
>  drivers/acpi/acpica/acevents.h |  2 ++
>  drivers/acpi/acpica/dbtest.c   |  3 ++
>  drivers/acpi/acpica/evrgnini.c | 51
> --
>  drivers/acpi/acpica/exutils.c  |  3 ++
>  drivers/acpi/acpica/utxface.c  | 23 +++
>  include/acpi/acpixf.h  |  1 +
>  6 files changed, 80 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/acevents.h b/drivers/acpi/acpica/acevents.h
> index 79f292687bd6..463eb9124765 100644
> --- a/drivers/acpi/acpica/acevents.h
> +++ b/drivers/acpi/acpica/acevents.h
> @@ -197,6 +197,8 @@ acpi_ev_execute_reg_method(union
> acpi_operand_object *region_obj, u32 function);
>  /*
>   * evregini - Region initialization and setup
>   */
> +void acpi_ev_system_release_memory_mappings(void);
> +
>  acpi_status
>  acpi_ev_system_memory_region_setup(acpi_handle handle,
>  u32 function,
> diff --git a/drivers/acpi/acpica/dbtest.c b/drivers/acpi/acpica/dbtest.c
> index 6db44a5ac786..7dac6dae5c48 100644
> --- a/drivers/acpi/acpica/dbtest.c
> +++ b/drivers/acpi/acpica/dbtest.c
> @@ -8,6 +8,7 @@
>  #include 
>  #include "accommon.h"
>  #include "acdebug.h"
> +#include "acevents.h"
>  #include "acnamesp.h"
>  #include "acpredef.h"
>  #include "acinterp.h"
> @@ -768,6 +769,8 @@ acpi_db_test_field_unit_type(union
> acpi_operand_object *obj_desc)
>   acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
>   acpi_ut_release_mutex(ACPI_MTX_INTERPRETER);
> 
> + acpi_ev_system_release_memory_mappings();
> +
>   bit_length = obj_desc->common_field.bit_length;
>   byte_length =
> ACPI_ROUND_BITS_UP_TO_BYTES(bit_length);
> 
> diff --git a/drivers/acpi/acpica/evrgnini.c b/drivers/acpi/acpica/evrgnini.c
> index 48a5e6eaf9b9..946c4eef054d 100644
> --- a/drivers/acpi/acpica/evrgnini.c
> +++ b/drivers/acpi/acpica/evrgnini.c
> @@ -16,6 +16,52 @@
>  #define _COMPONENT  ACPI_EVENTS
>  ACPI_MODULE_NAME("evrgnini")
> 
> +#ifdef ACPI_OS_MAP_MEMORY_FAST_PATH
> +static struct acpi_mem_mapping *unused_memory_mappings;
> +
> +/*
> **
> + *
> + * FUNCTION:acpi_ev_system_release_memory_mappings
> + *
> + * PARAMETERS:  None
> + *
> + * RETURN:  None
> + *
> + * DESCRIPTION: Release all of the unused memory mappings in the queue
> + *  under the interpreter mutex.
> + *
> +
> **
> /
> +void acpi_ev_system_release_memory_mappings(void)
> +{
> + struct acpi_mem_mapping *mapping;
> +
> +
>   ACPI_FUNCTION_TRACE(acpi_ev_system_release_memory_mappin
> gs);
> +
> + acpi_ut_acquire_mut

RE: [Devel] Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

2020-06-11 Thread Kaneda, Erik




> -Original Message-
> From: Jung-uk Kim 
> Sent: Wednesday, June 10, 2020 4:46 PM
> To: Nick Desaulniers ; Kaneda, Erik
> 
> Cc: Wysocki, Rafael J ; Ard Biesheuvel
> ; dvyu...@google.com; gli...@google.com; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> lorenzo.pieral...@arm.com; mark.rutl...@arm.com; p...@google.com;
> r...@rjwysocki.net; w...@kernel.org; sta...@vger.kernel.org; linux-
> a...@vger.kernel.org; de...@acpica.org
> Subject: [Devel] Re: [PATCH] ACPICA: fix UBSAN warning using
> __builtin_offsetof
> 
> On 20. 6. 10., Nick Desaulniers wrote:
> > On Wed, Jun 10, 2020 at 4:07 PM Kaneda, Erik 
> wrote:
> >>
> >> +JKim (for FreeBSD's perspective)
> >>
> >>> -Original Message-
> >>> From: Nick Desaulniers 
> >>> Sent: Monday, June 1, 2020 4:18 PM
> >>> To: Moore, Robert ; Kaneda, Erik
> >>> ; Wysocki, Rafael J
> ;
> >>> Len Brown 
> >>> Cc: Ard Biesheuvel ; dvyu...@google.com;
> >>> gli...@google.com; guohan...@huawei.com; linux-arm-
> >>> ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> >>> lorenzo.pieral...@arm.com; mark.rutl...@arm.com;
> >>> ndesaulni...@google.com; p...@google.com; r...@rjwysocki.net;
> >>> w...@kernel.org; sta...@vger.kernel.org; linux-a...@vger.kernel.org;
> >>> de...@acpica.org
> >>> Subject: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
> >>>
> >>> Will reported UBSAN warnings:
> >>> UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
> >>> UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
> >>>
> >> Hi,
> >>
> >>> Looks like the emulated offsetof macro ACPI_OFFSET is causing these.
> We
> >>> can avoid this by using the compiler builtin, __builtin_offsetof.
> >>>
> >> This doesn't really fly because __builtin_offsetof is a compiler extension.
> >>
> >> It looks like a lot of stddef.h files do this:
> >>
> >> #define offsetof(a,b) __builtin_offset(a,b)
> >>
> >> So does anyone have objections to ACPI_OFFSET being defined to
> offsetof()?
> >>
> >> This will allow a host OS project project to use their own definitions of
> offsetof in place of ACPICA's.
> >> If they don't have a definition for offsetof, we can supply the old one as 
> >> a
> fallback.
> >>
> >> Here's a patch:
> >>
> >> --- a/include/acpi/actypes.h
> >> +++ b/include/acpi/actypes.h
> >> @@ -504,11 +504,17 @@ typedef u64 acpi_integer;
> >>  #define ACPI_SUB_PTR(t, a, b)   ACPI_CAST_PTR (t, (ACPI_CAST_PTR
> (u8, (a)) - (acpi_size)(b)))
> >>  #define ACPI_PTR_DIFF(a, b) ((acpi_size) (ACPI_CAST_PTR (u8, 
> >> (a))
> - ACPI_CAST_PTR (u8, (b
> >>
> >> +/* Use an existing definiton for offsetof */
> >
> > s/definiton/definition/
> >
> >> +
> >> +#ifndef offsetof
> >> +#define offsetof(d,f)   ACPI_PTR_DIFF (&(((d *) 0)->f), 
> >> (void *)
> 0)
> >> +#endif
> >
> > If this header doesn't explicitly include  or
> > , won't translation units that include
> >  get different definitions of ACPI_OFFSET based on
> > whether they explicitly or transitively included  before
> > including ?  Theoretically, a translation unit in the
> > kernel could include actypes.h, have no includes of linux/stddef.h,
> > then get UBSAN errors again from using this definition?
> >
> > I don't mind using offsetof in place of the builtin (since it
> > typically will be implemented in terms of the builtin, or is at least
> > for the case specific to the Linux kernel). But if it's used, we
> > should include the header that defines it properly, and we should not
> > use the host's  IMO.  Is there a platform specific way to
> > include the platform's stddef.h here?
> >
> > Maybe linux/stddef.h should be included in
> > include/acpi/platform/aclinux.h, then include/acpi/platform/acenv.h
> > included in include/acpi/actypes.h, such that ACPI_OFFSET is defined
> > in terms of offsetof defined from that transitive dependency of
> > headers? (or do we get a circular inclusion trying to do that?)
> 
Hi,

> Actually, I think we should let platform-specific acfoo.h decide what to
> do here, i.e.,

That's a better solution. For Linux, it would look something like this:

--- a/include/acpi/actypes

RE: slub freelist issue / BUG: unable to handle page fault for address: 000000003ffe0018

2020-06-10 Thread Kaneda, Erik



> -Original Message-
> From: Vegard Nossum 
> Sent: Friday, June 5, 2020 7:45 AM
> To: Vlastimil Babka ; Rafael J. Wysocki
> ; Moore, Robert ; Kaneda,
> Erik 
> Cc: Kees Cook ; Wysocki, Rafael J
> ; Christoph Lameter ; Andrew
> Morton ; Marco Elver ;
> Waiman Long ; LKML  ker...@vger.kernel.org>; Linux MM ; ACPI Devel
> Maling List ; Len Brown ;
> Steven Rostedt 
> Subject: Re: slub freelist issue / BUG: unable to handle page fault for
> address: 3ffe0018
> 
> On 2020-06-05 16:08, Vlastimil Babka wrote:
> > On 6/5/20 3:12 PM, Rafael J. Wysocki wrote:
> >> On Fri, Jun 5, 2020 at 2:48 PM Vegard Nossum
>  wrote:
> >>>
> >>> On 2020-06-05 11:36, Vegard Nossum wrote:
> >>>>
> >>>> On 2020-06-05 11:11, Vlastimil Babka wrote:
> >>>>> On 6/4/20 8:46 PM, Vlastimil Babka wrote:
> >>>>>> On 6/4/20 7:57 PM, Kees Cook wrote:
> >>>>>>> On Thu, Jun 04, 2020 at 07:20:18PM +0200, Vegard Nossum wrote:
> >>>>>>>> On 2020-06-04 19:18, Vlastimil Babka wrote:
> >>>>>>>>> On 6/4/20 7:14 PM, Vegard Nossum wrote:
> >>>>>>>>>>
> >>>>>>>>>> Hi all,
> >>>>>>>>>>
> >>>>>>>>>> I ran into a boot problem with latest linus/master
> >>>>>>>>>> (6929f71e46bdddbf1c4d67c2728648176c67c555) that manifests
> like this:
> >>>>>>>>>
> >>>>>>>>> Hi, what's the .config you use?
> >>>>>>>>
> >>>>>>>> Pretty much x86_64 defconfig minus a few options (PCI, USB,
> >>>>>>>> ...)
> >>>>>>>
> >>>>>>> Oh yes indeed. I immediately crash in the same way with this config.
> >>>>>>> I'll
> >>>>>>> start digging...
> >>>>>>>
> >>>>>>> (defconfig finishes boot)
> >>>>>>
> >>>>>> This is funny, booting with slub_debug=F results in:
> >>>>>> I'm not sure if it's ACPI or ftrace wrong here, but looks like
> >>>>>> the changed free pointer offset merely exposes a bug in something
> >>>>>> else.
> >>>>>
> >>>>> So, with Kees' patch reverted, booting with slub_debug=F (or even
> >>>>> more specific slub_debug=F,ftrace_event_field) also hits this bug
> >>>>> below. I wanted to bisect it, but v5.7 was also bad, and also
> >>>>> v5.6. Didn't try further in history. So it's not new at all, and
> >>>>> likely very specific to your config+QEMU? (and related to the ACPI
> >>>>> error messages that precede it?).
> >>>>
> >>>> I see it too, but not on v5.0. I can bisect it.
> >>>
> >>> commit 67a72420a326b45514deb3f212085fb2cd1595b5
> >>> Author: Bob Moore 
> >>> Date:   Fri Aug 16 14:43:21 2019 -0700
> >>>
> >>>   ACPICA: Increase total number of possible Owner IDs
> >>>
> >>>   ACPICA commit 1f1652dad88b9d767767bc1f7eb4f7d99e6b5324
> >>>
> >>>   From 255 to 4095 possible IDs.
> >>>
> >>>   Link: https://github.com/acpica/acpica/commit/1f1652da
> >>>   Reported-by: Hedi Berriche 
> >>>   Signed-off-by: Bob Moore 
> >>>   Signed-off-by: Erik Schmauss 
> >>>   Signed-off-by: Rafael J. Wysocki 
> >>
> >> Bob, Erik, did we miss something in that patch?
> >
> > Maybe the patch just changes layout in a way that exposes the bug.
> >
> > Anyway the "ftrace_event_field" cache is not really involved, this is
> > just because of slab merging. After adding "slub_nomerge" to
> > "slub_debug=F", it starts making more sense, as the cache becomes
> > Acpi-Namespace
> >
> > [0.140408] [ cut here ]
> > [0.140837] cache_from_obj: Wrong slab cache. Acpi-Namespace but
> object is from kmalloc-64
> > [0.141406] WARNING: CPU: 0 PID: 1 at mm/slab.h:524
> kmem_cache_free+0x1d3/0x250
> > [0.142105] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.7.0+ #45
> > [0.142393] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS rel-1.13.0-0-gf21b5a4-rebuilt.opensuse.org 04/01/2014
> > [0.142393] RIP: 0010:kme

RE: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

2020-06-10 Thread Kaneda, Erik

+JKim (for FreeBSD's perspective)

> -Original Message-
> From: Nick Desaulniers 
> Sent: Monday, June 1, 2020 4:18 PM
> To: Moore, Robert ; Kaneda, Erik
> ; Wysocki, Rafael J ;
> Len Brown 
> Cc: Ard Biesheuvel ; dvyu...@google.com;
> gli...@google.com; guohan...@huawei.com; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> lorenzo.pieral...@arm.com; mark.rutl...@arm.com;
> ndesaulni...@google.com; p...@google.com; r...@rjwysocki.net;
> w...@kernel.org; sta...@vger.kernel.org; linux-a...@vger.kernel.org;
> de...@acpica.org
> Subject: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
> 
> Will reported UBSAN warnings:
> UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
> UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
> 
Hi,

> Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We
> can avoid this by using the compiler builtin, __builtin_offsetof.
> 
This doesn't really fly because __builtin_offsetof is a compiler extension.

It looks like a lot of stddef.h files do this:

#define offsetof(a,b) __builtin_offset(a,b)

So does anyone have objections to ACPI_OFFSET being defined to offsetof()?

This will allow a host OS project project to use their own definitions of 
offsetof in place of ACPICA's.
If they don't have a definition for offsetof, we can supply the old one as a 
fallback.

Here's a patch:

--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -504,11 +504,17 @@ typedef u64 acpi_integer;
 #define ACPI_SUB_PTR(t, a, b)   ACPI_CAST_PTR (t, (ACPI_CAST_PTR (u8, 
(a)) - (acpi_size)(b)))
 #define ACPI_PTR_DIFF(a, b) ((acpi_size) (ACPI_CAST_PTR (u8, (a)) 
- ACPI_CAST_PTR (u8, (b

+/* Use an existing definiton for offsetof */
+
+#ifndef offsetof
+#define offsetof(d,f)   ACPI_PTR_DIFF (&(((d *) 0)->f), (void 
*) 0)
+#endif
+
 /* Pointer/Integer type conversions */

 #define ACPI_TO_POINTER(i)  ACPI_CAST_PTR (void, (acpi_size) (i))
 #define ACPI_TO_INTEGER(p)  ACPI_PTR_DIFF (p, (void *) 0)
-#define ACPI_OFFSET(d, f)   ACPI_PTR_DIFF (&(((d *) 0)->f), (void 
*) 0)
+#define ACPI_OFFSET(d, f)   offsetof (d,f)
 #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
 #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)

Thanks,
Erik

> The non-kernel runtime of UBSAN would print:
> runtime error: member access within null pointer of type for this macro.
> 
> Link: https://lore.kernel.org/lkml/20200521100952.GA5360@willie-the-truck/
> Cc: sta...@vger.kernel.org
> Reported-by: Will Deacon 
> Suggested-by: Ard Biesheuvel 
> Signed-off-by: Nick Desaulniers 
> ---
>  include/acpi/actypes.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index
> 4defed58ea33..04359c70b198 100644
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -508,7 +508,7 @@ typedef u64 acpi_integer;
> 
>  #define ACPI_TO_POINTER(i)  ACPI_CAST_PTR (void, (acpi_size) (i))
>  #define ACPI_TO_INTEGER(p)  ACPI_PTR_DIFF (p, (void *) 0)
> -#define ACPI_OFFSET(d, f)   ACPI_PTR_DIFF (&(((d *) 0)->f), 
> (void *)
> 0)
> +#define ACPI_OFFSET(d, f)   __builtin_offsetof(d, f)
>  #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
>  #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
> 
> --
> 2.27.0.rc2.251.g90737beb825-goog

RE: [PATCH v2] ACPICA: Replace one-element array with flexible-array

2020-06-10 Thread Kaneda, Erik



> -Original Message-
> From: linux-acpi-ow...@vger.kernel.org  ow...@vger.kernel.org> On Behalf Of Kaneda, Erik
> Sent: Wednesday, June 3, 2020 4:13 PM
> To: Rafael J. Wysocki ; Gustavo A. R. Silva
> 
> Cc: Moore, Robert ; Wysocki, Rafael J
> ; Len Brown ; ACPI Devel
> Maling List ; open list:ACPI COMPONENT
> ARCHITECTURE (ACPICA) ; Linux Kernel Mailing List
> ; Gustavo A. R. Silva
> 
> Subject: RE: [PATCH v2] ACPICA: Replace one-element array with flexible-
> array
> 
> 
> 
> > -Original Message-
> > From: linux-acpi-ow...@vger.kernel.org  > ow...@vger.kernel.org> On Behalf Of Rafael J. Wysocki
> > Sent: Wednesday, June 3, 2020 6:04 AM
> > To: Gustavo A. R. Silva ; Kaneda, Erik
> > 
> > Cc: Moore, Robert ; Wysocki, Rafael J
> > ; Len Brown ; ACPI Devel
> > Maling List ; open list:ACPI COMPONENT
> > ARCHITECTURE (ACPICA) ; Linux Kernel Mailing List
> > ; Gustavo A. R. Silva
> > 
> > Subject: Re: [PATCH v2] ACPICA: Replace one-element array with
> > flexible- array
> >
> > On Tue, Jun 2, 2020 at 11:34 PM Gustavo A. R. Silva
> > 
> > wrote:
> > >
> > > The current codebase makes use of one-element arrays in the
> > > following
> > > form:
> > >
> > > struct something {
> > > int length;
> > > u8 data[1];
> > > };
> > >
> > > struct something *instance;
> > >
> > > instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
> > > instance->length = size;
> > > memcpy(instance->data, source, size);
> > >
> > > but the preferred mechanism to declare variable-length types such as
> > > these ones is a flexible array member[1][2], introduced in C99:
> > >
> > > struct foo {
> > > int stuff;
> > > struct boo array[];
> > > };
> > >
> > > By making use of the mechanism above, we will get a compiler warning
> > > in case the flexible array does not occur last in the structure,
> > > which will help us prevent some kind of undefined behavior bugs from
> > > being inadvertently introduced[3] to the codebase from now on.
> > >
> > > This issue was found with the help of Coccinelle and audited _manually_.
> > >
> > > [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> > > [2] https://github.com/KSPP/linux/issues/21
> > > [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
> > >
> > > Signed-off-by: Gustavo A. R. Silva 
> >
> > Erik, can you take this to the upstream, please?
> Yes, We'll have to make additional changes to other structures with one-
> element arrays but we'll get this in time for the upcoming ACPICA release.

Pull request available here: https://github.com/acpica/acpica/pull/608

Erik
> 
> Thanks,
> Erik
> >
> > > ---
> > > Changes in v2:
> > >  - Don't use struct_size() for now.
> > >  - Update subject line and changelog text.
> > >
> > >  drivers/acpi/acpica/utids.c | 2 +-
> > >  include/acpi/actypes.h  | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/acpi/acpica/utids.c
> > > b/drivers/acpi/acpica/utids.c index 3bb06935a2ad3..225f3c60203c7
> > > 100644
> > > --- a/drivers/acpi/acpica/utids.c
> > > +++ b/drivers/acpi/acpica/utids.c
> > > @@ -263,7 +263,7 @@ acpi_ut_execute_CID(struct
> acpi_namespace_node
> > *device_node,
> > >  * 3) Size of the actual CID strings
> > >  */
> > > cid_list_size = sizeof(struct acpi_pnp_device_id_list) +
> > > -   ((count - 1) * sizeof(struct acpi_pnp_device_id)) +
> > > +   count * sizeof(struct acpi_pnp_device_id) +
> > > string_area_size;
> > >
> > > cid_list = ACPI_ALLOCATE_ZEROED(cid_list_size);
> > > diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index
> > > 4defed58ea338..c7bcda0ad366a 100644
> > > --- a/include/acpi/actypes.h
> > > +++ b/include/acpi/actypes.h
> > > @@ -1145,7 +1145,7 @@ struct acpi_pnp_device_id {  struct
> > > acpi_pnp_device_id_list {
> > > u32 count;  /* Number of IDs in Ids array */
> > > u32 list_size;  /* Size of list, including ID strings */
> > > -   struct acpi_pnp_device_id ids[1];   /* ID array */
> > > +   struct acpi_pnp_device_id ids[];/* ID array */
> > >  };
> > >
> > >  /*
> > > --
> > > 2.27.0
> > >

RE: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

2020-06-08 Thread Kaneda, Erik



> -Original Message-
> From: Nick Desaulniers 
> Sent: Tuesday, June 2, 2020 11:47 AM
> To: Kaneda, Erik 
> Cc: Moore, Robert ; Wysocki, Rafael J
> ; Len Brown ; Ard
> Biesheuvel ; dvyu...@google.com; gli...@google.com;
> guohan...@huawei.com; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org; lorenzo.pieral...@arm.com;
> mark.rutl...@arm.com; p...@google.com; r...@rjwysocki.net;
> w...@kernel.org; sta...@vger.kernel.org; linux-a...@vger.kernel.org;
> de...@acpica.org
> Subject: Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
> 
> On Mon, Jun 1, 2020 at 5:03 PM Kaneda, Erik 
> wrote:
> >
> >
> > Hi,
> >
> > > Will reported UBSAN warnings:
> > > UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
> > > UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
> > >
> > > Looks like the emulated offsetof macro ACPI_OFFSET is causing these.
> > > We can avoid this by using the compiler builtin, __builtin_offsetof.
> >
> > I'll take a look at this tomorrow
> > >
> > > The non-kernel runtime of UBSAN would print:
> > > runtime error: member access within null pointer of type for this macro.
> >
> > actypes.h is owned by ACPICA so we typically do not allow
> > compiler-specific extensions because the code is intended to be
> > compiled using the C99 standard without compiler extensions. We could
> > allow this sort of thing in a Linux-specific header file like
> include/acpi/platform/aclinux.h but I'll take a look at the error as well..
> 
Hi,

> If I'm not allowed to touch that header, it looks like I can include
>  (rather than my host's ) to get a definition of

Why not use your host's stddef.h?

> `offsetof` thats implemented in terms of `__builtin_offsetof`.  I should be
> able to use that to replace uses of ACPI_OFFSET.  Are any of these off limits?

Yes, the idea is to define ACPI_OFFSET in a way that you want so that we don't 
touch the uses below.

Erik
> 
> $ grep -rn ACPI_OFFSET
> arch/arm64/include/asm/acpi.h:34:#define
> ACPI_MADT_GICC_MIN_LENGTH
> ACPI_OFFSET(  \ arch/arm64/include/asm/acpi.h:41:#define
> ACPI_MADT_GICC_SPE (ACPI_OFFSET(struct acpi_madt_generic_interrupt, \
> include/acpi/actbl.h:376:#define ACPI_FADT_OFFSET(f) (u16)
> ACPI_OFFSET (struct acpi_table_fadt, f)
> drivers/acpi/acpica/acresrc.h:84:#define ACPI_RS_OFFSET(f)
>   (u8) ACPI_OFFSET (struct acpi_resource,f)
> drivers/acpi/acpica/acresrc.h:85:#define AML_OFFSET(f)
>   (u8) ACPI_OFFSET (union aml_resource,f)
> drivers/acpi/acpica/acinterp.h:17:#define ACPI_EXD_OFFSET(f)
> (u8) ACPI_OFFSET (union acpi_operand_object,f)
> drivers/acpi/acpica/acinterp.h:18:#define ACPI_EXD_NSOFFSET(f)
> (u8) ACPI_OFFSET (struct acpi_namespace_node,f)
> drivers/acpi/acpica/rsdumpinfo.c:16:#define ACPI_RSD_OFFSET(f)
>  (u8) ACPI_OFFSET (union acpi_resource_data,f)
> drivers/acpi/acpica/rsdumpinfo.c:17:#define ACPI_PRT_OFFSET(f)
>  (u8) ACPI_OFFSET (struct acpi_pci_routing_table,f)
> 
> --
> Thanks,
> ~Nick Desaulniers

RE: slub freelist issue / BUG: unable to handle page fault for address: 000000003ffe0018

2020-06-05 Thread Kaneda, Erik



> -Original Message-
> From: Vegard Nossum 
> Sent: Friday, June 5, 2020 7:45 AM
> To: Vlastimil Babka ; Rafael J. Wysocki
> ; Moore, Robert ; Kaneda,
> Erik 
> Cc: Kees Cook ; Wysocki, Rafael J
> ; Christoph Lameter ; Andrew
> Morton ; Marco Elver ;
> Waiman Long ; LKML  ker...@vger.kernel.org>; Linux MM ; ACPI Devel
> Maling List ; Len Brown ;
> Steven Rostedt 
> Subject: Re: slub freelist issue / BUG: unable to handle page fault for
> address: 3ffe0018
> 
> On 2020-06-05 16:08, Vlastimil Babka wrote:
> > On 6/5/20 3:12 PM, Rafael J. Wysocki wrote:
> >> On Fri, Jun 5, 2020 at 2:48 PM Vegard Nossum
>  wrote:
> >>>
> >>> On 2020-06-05 11:36, Vegard Nossum wrote:
> >>>>
> >>>> On 2020-06-05 11:11, Vlastimil Babka wrote:
> >>>>> On 6/4/20 8:46 PM, Vlastimil Babka wrote:
> >>>>>> On 6/4/20 7:57 PM, Kees Cook wrote:
> >>>>>>> On Thu, Jun 04, 2020 at 07:20:18PM +0200, Vegard Nossum wrote:
> >>>>>>>> On 2020-06-04 19:18, Vlastimil Babka wrote:
> >>>>>>>>> On 6/4/20 7:14 PM, Vegard Nossum wrote:
> >>>>>>>>>>
> >>>>>>>>>> Hi all,
> >>>>>>>>>>
> >>>>>>>>>> I ran into a boot problem with latest linus/master
> >>>>>>>>>> (6929f71e46bdddbf1c4d67c2728648176c67c555) that manifests
> like this:
> >>>>>>>>>
> >>>>>>>>> Hi, what's the .config you use?
> >>>>>>>>
> >>>>>>>> Pretty much x86_64 defconfig minus a few options (PCI, USB,
> >>>>>>>> ...)
> >>>>>>>
> >>>>>>> Oh yes indeed. I immediately crash in the same way with this config.
> >>>>>>> I'll
> >>>>>>> start digging...
> >>>>>>>
> >>>>>>> (defconfig finishes boot)
> >>>>>>
> >>>>>> This is funny, booting with slub_debug=F results in:
> >>>>>> I'm not sure if it's ACPI or ftrace wrong here, but looks like
> >>>>>> the changed free pointer offset merely exposes a bug in something
> >>>>>> else.
> >>>>>
> >>>>> So, with Kees' patch reverted, booting with slub_debug=F (or even
> >>>>> more specific slub_debug=F,ftrace_event_field) also hits this bug
> >>>>> below. I wanted to bisect it, but v5.7 was also bad, and also
> >>>>> v5.6. Didn't try further in history. So it's not new at all, and
> >>>>> likely very specific to your config+QEMU? (and related to the ACPI
> >>>>> error messages that precede it?).
> >>>>
> >>>> I see it too, but not on v5.0. I can bisect it.
> >>>
> >>> commit 67a72420a326b45514deb3f212085fb2cd1595b5
> >>> Author: Bob Moore 
> >>> Date:   Fri Aug 16 14:43:21 2019 -0700
> >>>
> >>>   ACPICA: Increase total number of possible Owner IDs
> >>>
> >>>   ACPICA commit 1f1652dad88b9d767767bc1f7eb4f7d99e6b5324
> >>>
> >>>   From 255 to 4095 possible IDs.
> >>>
> >>>   Link: https://github.com/acpica/acpica/commit/1f1652da
> >>>   Reported-by: Hedi Berriche 
> >>>   Signed-off-by: Bob Moore 
> >>>   Signed-off-by: Erik Schmauss 
> >>>   Signed-off-by: Rafael J. Wysocki 
> >>
> >> Bob, Erik, did we miss something in that patch?
> >
> > Maybe the patch just changes layout in a way that exposes the bug.
> >
> > Anyway the "ftrace_event_field" cache is not really involved, this is
> > just because of slab merging. After adding "slub_nomerge" to
> > "slub_debug=F", it starts making more sense, as the cache becomes
> > Acpi-Namespace
> >
> > [0.140408] [ cut here ]
> > [0.140837] cache_from_obj: Wrong slab cache. Acpi-Namespace but
> object is from kmalloc-64
> > [0.141406] WARNING: CPU: 0 PID: 1 at mm/slab.h:524
> kmem_cache_free+0x1d3/0x250
> > [0.142105] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.7.0+ #45
> > [0.142393] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS rel-1.13.0-0-gf21b5a4-rebuilt.opensuse.org 04/01/2014
> > [0.142393] RIP: 0010:kme

RE: [PATCH v2] ACPICA: Replace one-element array with flexible-array

2020-06-03 Thread Kaneda, Erik



> -Original Message-
> From: linux-acpi-ow...@vger.kernel.org  ow...@vger.kernel.org> On Behalf Of Rafael J. Wysocki
> Sent: Wednesday, June 3, 2020 6:04 AM
> To: Gustavo A. R. Silva ; Kaneda, Erik
> 
> Cc: Moore, Robert ; Wysocki, Rafael J
> ; Len Brown ; ACPI Devel
> Maling List ; open list:ACPI COMPONENT
> ARCHITECTURE (ACPICA) ; Linux Kernel Mailing List
> ; Gustavo A. R. Silva
> 
> Subject: Re: [PATCH v2] ACPICA: Replace one-element array with flexible-
> array
> 
> On Tue, Jun 2, 2020 at 11:34 PM Gustavo A. R. Silva 
> wrote:
> >
> > The current codebase makes use of one-element arrays in the following
> > form:
> >
> > struct something {
> > int length;
> > u8 data[1];
> > };
> >
> > struct something *instance;
> >
> > instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
> > instance->length = size;
> > memcpy(instance->data, source, size);
> >
> > but the preferred mechanism to declare variable-length types such as
> > these ones is a flexible array member[1][2], introduced in C99:
> >
> > struct foo {
> > int stuff;
> > struct boo array[];
> > };
> >
> > By making use of the mechanism above, we will get a compiler warning
> > in case the flexible array does not occur last in the structure, which
> > will help us prevent some kind of undefined behavior bugs from being
> > inadvertently introduced[3] to the codebase from now on.
> >
> > This issue was found with the help of Coccinelle and audited _manually_.
> >
> > [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> > [2] https://github.com/KSPP/linux/issues/21
> > [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
> >
> > Signed-off-by: Gustavo A. R. Silva 
> 
> Erik, can you take this to the upstream, please?
Yes, We'll have to make additional changes to other structures with one-element 
arrays but we'll get this in time for the upcoming ACPICA release.

Thanks,
Erik
> 
> > ---
> > Changes in v2:
> >  - Don't use struct_size() for now.
> >  - Update subject line and changelog text.
> >
> >  drivers/acpi/acpica/utids.c | 2 +-
> >  include/acpi/actypes.h  | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/acpica/utids.c b/drivers/acpi/acpica/utids.c
> > index 3bb06935a2ad3..225f3c60203c7 100644
> > --- a/drivers/acpi/acpica/utids.c
> > +++ b/drivers/acpi/acpica/utids.c
> > @@ -263,7 +263,7 @@ acpi_ut_execute_CID(struct acpi_namespace_node
> *device_node,
> >  * 3) Size of the actual CID strings
> >  */
> > cid_list_size = sizeof(struct acpi_pnp_device_id_list) +
> > -   ((count - 1) * sizeof(struct acpi_pnp_device_id)) +
> > +   count * sizeof(struct acpi_pnp_device_id) +
> > string_area_size;
> >
> > cid_list = ACPI_ALLOCATE_ZEROED(cid_list_size);
> > diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index
> > 4defed58ea338..c7bcda0ad366a 100644
> > --- a/include/acpi/actypes.h
> > +++ b/include/acpi/actypes.h
> > @@ -1145,7 +1145,7 @@ struct acpi_pnp_device_id {  struct
> > acpi_pnp_device_id_list {
> > u32 count;  /* Number of IDs in Ids array */
> > u32 list_size;  /* Size of list, including ID strings */
> > -   struct acpi_pnp_device_id ids[1];   /* ID array */
> > +   struct acpi_pnp_device_id ids[];/* ID array */
> >  };
> >
> >  /*
> > --
> > 2.27.0
> >

RE: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

2020-06-01 Thread Kaneda, Erik



> -Original Message-
> From: Nick Desaulniers 
> Sent: Monday, June 1, 2020 4:18 PM
> To: Moore, Robert ; Kaneda, Erik
> ; Wysocki, Rafael J ;
> Len Brown 
> Cc: Ard Biesheuvel ; dvyu...@google.com;
> gli...@google.com; guohan...@huawei.com; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> lorenzo.pieral...@arm.com; mark.rutl...@arm.com;
> ndesaulni...@google.com; p...@google.com; r...@rjwysocki.net;
> w...@kernel.org; sta...@vger.kernel.org; linux-a...@vger.kernel.org;
> de...@acpica.org
> Subject: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
>
Hi,
 
> Will reported UBSAN warnings:
> UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
> UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
> 
> Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We
> can avoid this by using the compiler builtin, __builtin_offsetof.

I'll take a look at this tomorrow
> 
> The non-kernel runtime of UBSAN would print:
> runtime error: member access within null pointer of type for this macro.

actypes.h is owned by ACPICA so we typically do not allow compiler-specific
extensions because the code is intended to be compiled using the C99 standard
without compiler extensions. We could allow this sort of thing in a 
Linux-specific
header file like include/acpi/platform/aclinux.h but I'll take a look at the 
error as well..

Erik
> 
> Link: https://lore.kernel.org/lkml/20200521100952.GA5360@willie-the-truck/
> Cc: sta...@vger.kernel.org
> Reported-by: Will Deacon 
> Suggested-by: Ard Biesheuvel 
> Signed-off-by: Nick Desaulniers 
> ---
>  include/acpi/actypes.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index
> 4defed58ea33..04359c70b198 100644
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -508,7 +508,7 @@ typedef u64 acpi_integer;
> 
>  #define ACPI_TO_POINTER(i)  ACPI_CAST_PTR (void, (acpi_size) (i))
>  #define ACPI_TO_INTEGER(p)  ACPI_PTR_DIFF (p, (void *) 0)
> -#define ACPI_OFFSET(d, f)   ACPI_PTR_DIFF (&(((d *) 0)->f), 
> (void *)
> 0)
> +#define ACPI_OFFSET(d, f)   __builtin_offsetof(d, f)
>  #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
>  #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
> 
> --
> 2.27.0.rc2.251.g90737beb825-goog

RE: [PATCH] ACPICA: Replace one-element array and use struct_size() helper

2020-05-21 Thread Kaneda, Erik




> -Original Message-
> From: linux-acpi-ow...@vger.kernel.org  ow...@vger.kernel.org> On Behalf Of Kees Cook
> Sent: Wednesday, May 20, 2020 8:41 AM
> To: Rafael J. Wysocki 
> Cc: Gustavo A. R. Silva ; Moore, Robert
> ; Kaneda, Erik ; Wysocki,
> Rafael J ; Len Brown ; ACPI
> Devel Maling List ; open list:ACPI COMPONENT
> ARCHITECTURE (ACPICA) ; Linux Kernel Mailing List
> ; Gustavo A. R. Silva
> 
> Subject: Re: [PATCH] ACPICA: Replace one-element array and use
> struct_size() helper
> 
> On Wed, May 20, 2020 at 11:15:18AM +0200, Rafael J. Wysocki wrote:
> > On Wed, May 20, 2020 at 12:46 AM Gustavo A. R. Silva
> >  wrote:
> > >
> > > On Tue, May 19, 2020 at 12:25:13PM +0200, Rafael J. Wysocki wrote:
> > > > On Tue, May 19, 2020 at 12:22 AM Gustavo A. R. Silva
> > > >  wrote:
> > > > >
> > > > > The current codebase makes use of one-element arrays in the
> > > > > following
> > > > > form:
> > > > >
> > > > > struct something {
> > > > > int length;
> > > > > u8 data[1];
> > > > > };
> > > > >
> > > > > struct something *instance;
> > > > >
> > > > > instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
> > > > > instance->length = size;
> > > > > memcpy(instance->data, source, size);
> > > > >
> > > > > but the preferred mechanism to declare variable-length types
> > > > > such as these ones is a flexible array member[1][2], introduced in 
> > > > > C99:
> > > > >
> > > > > struct foo {
> > > > > int stuff;
> > > > > struct boo array[];
> > > > > };
> > > > >
> > > > > By making use of the mechanism above, we will get a compiler
> > > > > warning in case the flexible array does not occur last in the
> > > > > structure, which will help us prevent some kind of undefined
> > > > > behavior bugs from being inadvertently introduced[3] to the
> codebase from now on.
> > > >
> > > > However, the ACPICA code in the kernel comes from an external
> > > > project and changes of this type are generally not applicable to
> > > > it unless accepted upstream.
> > >
> > > Hi Rafael,
> > >
> > > By _accepted upstream_, in this case, you mean the adoption of the
> > > flexible-arrays in the whole codebase, first?
> >
> > I meant whether or not the patch is accepted by the ACPICA upstream.
> 
> Is that here? https://github.com/acpica/acpica/commits/master
> 
> >
> > >  If this is the case
> > > notice that there are hundreds of these flexible-array conversions
> > > in mainline, already:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/l
> > > og/?qt=grep&q=flexible-array
> > >
> > > Is this what you mean?
> >
> > I'm not actually sure what you mean here.
> 
> I think this was just a misunderstanding about what "upstream" meant. :)
> 
> I hope ACPICA will take these changes -- it seems like we keep running into
> these issues with the kernel's language feature clean-ups and ACPICA
> upstream, though each have been resolved so far! :) Flexible array members
> are a C99 feature, so it's hardly a new way to express things.
> In fact, it looks like ACPICA already builds with -c99 by default:
> https://github.com/acpica/acpica/blob/master/generate/unix/Makefile.conf
> ig#L202
> https://github.com/acpica/acpica/blob/master/generate/efi/Makefile.config
> #L93
> 
> MSVC has supported them (called "unsized arrays") since 7.1 in 2003.
> 
> Gustavo, can you build a merge request for the ACPICA project directly?

The flexible array members will be fine.

The struct_size helper uses the typeof operator which is a GCC extension.

ACPICA codebase is intended to be compiled by many different compilers (not
just GCC and MSVC) so we cannot support struct_size.

Erik
> 
> --
> Kees Cook

RE: [PATCH v1 1/1] libnvdimm: Don't use GUID APIs against raw buffer

RE: [Devel] Re: [PATCH] ACPICA: Fix a typo

RE: [PATCH] ACPICA: Fix a typo

RE: slub freelist issue / BUG: unable to handle page fault for address: 000000003ffe0018

RE: [PATCH] Enable ACPI_ADR_SPACE_PCI_CONFIG in acpi_gbl_default_address_spaces only when ACPI_PCI_CONFIGURED is defined

RE: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using existing buffer"

RE: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using existing buffer"

RE: [Devel] Re: [PATCH] ACPICA: Common: Fix a typo

RE: [PATCH] ACPICA: fix -Wfallthrough

RE: [PATCH] ACPICA: fix -Wfallthrough

RE: [RFC PATCH v3 02/16] cxl/acpi: Add an acpi_cxl module for the CXL interconnect

RE: [PATCH] ACPICA: Fix a soft-lockup on large systems

RE: [PATCH] ACPI: actypes.h: drop a duplicated word

RE: [RFT][PATCH v2 3/4] ACPICA: Preserve memory opregion mappings if supported by OS

RE: [PATCH] acpi: disallow loading configfs acpi tables when locked down

RE: [Devel] Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

RE: [RFT][PATCH 2/3] ACPICA: Remove unused memory mappings on interpreter exit

RE: [Devel] Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

RE: slub freelist issue / BUG: unable to handle page fault for address: 000000003ffe0018

RE: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

RE: [PATCH v2] ACPICA: Replace one-element array with flexible-array

RE: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

RE: slub freelist issue / BUG: unable to handle page fault for address: 000000003ffe0018

RE: [PATCH v2] ACPICA: Replace one-element array with flexible-array

RE: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

RE: [PATCH] ACPICA: Replace one-element array and use struct_size() helper

26 matches

Site Navigation

Mail list logo

Footer information