RE: [bug report] system panic at nfit_get_smbios_id+0x6e/0xf0 [nfit] during boot
> -Original Message- > From: Yi Zhang > Sent: Wednesday, May 5, 2021 8:05 PM > To: Williams, Dan J ; Moore, Robert > > Cc: linux-nvdimm ; Kaneda, Erik > ; Wysocki, Rafael J > Subject: Re: [bug report] system panic at nfit_get_smbios_id+0x6e/0xf0 > [nfit] during boot > > On Sat, May 1, 2021 at 2:05 PM Dan Williams > wrote: > > > > On Fri, Apr 30, 2021 at 7:28 PM Yi Zhang wrote: > > > > > > Hi > > > > > > With the latest Linux tree, my DCPMM server boot failed with the > > > bellow panic log, pls help check it, let me know if you need any test > > > for it. > > > > So v5.12 is ok but v5.12+ is not? > > > > Might you be able to bisect? > > Hi Dan > This issue was introduced with this patch, let me know if you need more info. > > commit cf16b05c607bd716a0a5726dc8d577a89fdc1777 > Author: Bob Moore > Date: Tue Apr 6 14:30:15 2021 -0700 > > ACPICA: ACPI 6.4: NFIT: add Location Cookie field > > Also, update struct size to reflect these changes in nfit core driver. > > ACPICA commit af60199a9a1de9e6844929fd4cc22334522ed195 > > Link: https://github.com/acpica/acpica/commit/af60199a > Cc: Dan Williams > Signed-off-by: Bob Moore > Signed-off-by: Erik Kaneda > Signed-off-by: Rafael J. Wysocki > It's likely that this change forced the nfit driver's code to parse the ACPI table so that it assumes that the location cookie field is always enabled and the NFIT was parsed incorrectly. Does the NFIT table on this platform contain a valid cookie field? > > > > If not can you send the nfit.gz from this command: > > > > acpidump -n NFIT | gzip -c > nfit.gz > > > > Also can you send the full dmesg? I don't suppose you see a message of > > this format before this failure: > > > > dev_err(acpi_desc->dev, "SPA %d missing DCR %d\n", > > spa->range_index, dcr); > > ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
RE: [PATCH v1 1/1] libnvdimm: Don't use GUID APIs against raw buffer
+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 ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
RE: [RFT][PATCH v2 3/4] ACPICA: Preserve memory opregion mappings if supported by OS
> -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-ker...@vger.kernel.org; linux- > a...@vger.kernel.org; linux-nvdimm@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: [RFT][PATCH 2/3] ACPICA: Remove unused memory mappings on interpreter exit
> -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
