[v4,2/2] ACPICA: Preserve memory opregion mappings
diff mbox series

Message ID 1794490.F2OrUDcHQn@kreacher
State New
Headers show
Series
  • ACPI: ACPICA / OSL: Avoid unmapping ACPI memory inside of the AML interpreter
Related show

Commit Message

Rafael J. Wysocki June 29, 2020, 4:33 p.m. UTC
From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

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 acess 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 preserve all of the memory mappings created by it until the memory
regions associated with them go away.

Accordingly, update acpi_ev_system_memory_region_setup() to unmap all
memory associated with memory opregions that go away.

Reported-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/acpica/evrgnini.c | 14 ++++----
 drivers/acpi/acpica/exregion.c | 65 ++++++++++++++++++++++++----------
 include/acpi/actypes.h         | 12 +++++--
 3 files changed, 64 insertions(+), 27 deletions(-)

Comments

Al Stone June 29, 2020, 8:57 p.m. UTC | #1
On 29 Jun 2020 18:33, Rafael J. Wysocki wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> 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 acess 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 preserve all of the memory mappings created by it until the memory
> regions associated with them go away.
> 
> Accordingly, update acpi_ev_system_memory_region_setup() to unmap all
> memory associated with memory opregions that go away.
> 
> Reported-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/acpica/evrgnini.c | 14 ++++----
>  drivers/acpi/acpica/exregion.c | 65 ++++++++++++++++++++++++----------
>  include/acpi/actypes.h         | 12 +++++--
>  3 files changed, 64 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/evrgnini.c b/drivers/acpi/acpica/evrgnini.c
> index aefc0145e583..89be3ccdad53 100644
> --- a/drivers/acpi/acpica/evrgnini.c
> +++ b/drivers/acpi/acpica/evrgnini.c
> @@ -38,6 +38,7 @@ acpi_ev_system_memory_region_setup(acpi_handle handle,
>  	union acpi_operand_object *region_desc =
>  	    (union acpi_operand_object *)handle;
>  	struct acpi_mem_space_context *local_region_context;
> +	struct acpi_mem_mapping *mm;
>  
>  	ACPI_FUNCTION_TRACE(ev_system_memory_region_setup);
>  
> @@ -46,13 +47,14 @@ acpi_ev_system_memory_region_setup(acpi_handle handle,
>  			local_region_context =
>  			    (struct acpi_mem_space_context *)*region_context;
>  
> -			/* Delete a cached mapping if present */
> +			/* Delete memory mappings if present */
>  
> -			if (local_region_context->mapped_length) {
> -				acpi_os_unmap_memory(local_region_context->
> -						     mapped_logical_address,
> -						     local_region_context->
> -						     mapped_length);
> +			while (local_region_context->first_mm) {
> +				mm = local_region_context->first_mm;
> +				local_region_context->first_mm = mm->next_mm;
> +				acpi_os_unmap_memory(mm->logical_address,
> +						     mm->length);
> +				ACPI_FREE(mm);
>  			}
>  			ACPI_FREE(local_region_context);
>  			*region_context = NULL;
> diff --git a/drivers/acpi/acpica/exregion.c b/drivers/acpi/acpica/exregion.c
> index d15a66de26c0..fd68f2134804 100644
> --- a/drivers/acpi/acpica/exregion.c
> +++ b/drivers/acpi/acpica/exregion.c
> @@ -41,6 +41,7 @@ acpi_ex_system_memory_space_handler(u32 function,
>  	acpi_status status = AE_OK;
>  	void *logical_addr_ptr = NULL;
>  	struct acpi_mem_space_context *mem_info = region_context;
> +	struct acpi_mem_mapping *mm = mem_info->cur_mm;
>  	u32 length;
>  	acpi_size map_length;

I think this needs to be:

        acpi_size map_length = mem_info->length;

since it now gets used in the ACPI_ERROR() call below.  I'm getting
a "maybe used unitialized" error on compilation.

>  	acpi_size page_boundary_map_length;
> @@ -96,20 +97,38 @@ acpi_ex_system_memory_space_handler(u32 function,
>  	 * Is 1) Address below the current mapping? OR
>  	 *    2) Address beyond the current mapping?
>  	 */
> -	if ((address < mem_info->mapped_physical_address) ||
> -	    (((u64) address + length) > ((u64)
> -					 mem_info->mapped_physical_address +
> -					 mem_info->mapped_length))) {
> +	if (!mm || (address < mm->physical_address) ||
> +	    ((u64) address + length > (u64) mm->physical_address + mm->length)) {
>  		/*
> -		 * The request cannot be resolved by the current memory mapping;
> -		 * Delete the existing mapping and create a new one.
> +		 * The request cannot be resolved by the current memory mapping.
> +		 *
> +		 * Look for an existing saved mapping covering the address range
> +		 * at hand.  If found, save it as the current one and carry out
> +		 * the access.
>  		 */
> -		if (mem_info->mapped_length) {
> +		for (mm = mem_info->first_mm; mm; mm = mm->next_mm) {
> +			if (mm == mem_info->cur_mm)
> +				continue;
> +
> +			if (address < mm->physical_address)
> +				continue;
> +
> +			if ((u64) address + length >
> +					(u64) mm->physical_address + mm->length)
> +				continue;
>  
> -			/* Valid mapping, delete it */
> +			mem_info->cur_mm = mm;
> +			goto access;
> +		}
>  
> -			acpi_os_unmap_memory(mem_info->mapped_logical_address,
> -					     mem_info->mapped_length);
> +		/* Create a new mappings list entry */
> +		mm = ACPI_ALLOCATE_ZEROED(sizeof(*mm));
> +		if (!mm) {
> +			ACPI_ERROR((AE_INFO,
> +				    "Unable to save memory mapping at 0x%8.8X%8.8X, size %u",
> +				    ACPI_FORMAT_UINT64(address),
> +				    (u32)map_length));
> +			return_ACPI_STATUS(AE_NO_MEMORY);
>  		}
>  
>  		/*
> @@ -143,29 +162,39 @@ acpi_ex_system_memory_space_handler(u32 function,
>  
>  		/* Create a new mapping starting at the address given */
>  
> -		mem_info->mapped_logical_address =
> -		    acpi_os_map_memory(address, map_length);
> -		if (!mem_info->mapped_logical_address) {
> +		logical_addr_ptr = acpi_os_map_memory(address, map_length);
> +		if (!logical_addr_ptr) {
>  			ACPI_ERROR((AE_INFO,
>  				    "Could not map memory at 0x%8.8X%8.8X, size %u",
>  				    ACPI_FORMAT_UINT64(address),
>  				    (u32)map_length));
> -			mem_info->mapped_length = 0;
> +			ACPI_FREE(mm);
>  			return_ACPI_STATUS(AE_NO_MEMORY);
>  		}
>  
>  		/* Save the physical address and mapping size */
>  
> -		mem_info->mapped_physical_address = address;
> -		mem_info->mapped_length = map_length;
> +		mm->logical_address = logical_addr_ptr;
> +		mm->physical_address = address;
> +		mm->length = map_length;
> +
> +		/*
> +		 * Add the new entry to the mappigs list and save it as the
> +		 * current mapping.
> +		 */
> +		mm->next_mm = mem_info->first_mm;
> +		mem_info->first_mm = mm;
> +
> +		mem_info->cur_mm = mm;
>  	}
>  
> +access:
>  	/*
>  	 * Generate a logical pointer corresponding to the address we want to
>  	 * access
>  	 */
> -	logical_addr_ptr = mem_info->mapped_logical_address +
> -	    ((u64) address - (u64) mem_info->mapped_physical_address);
> +	logical_addr_ptr = mm->logical_address +
> +		((u64) address - (u64) mm->physical_address);
>  
>  	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>  			  "System-Memory (width %u) R/W %u Address=%8.8X%8.8X\n",
> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> index aa236b9e6f24..d005e35ab399 100644
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -1201,12 +1201,18 @@ struct acpi_pci_id {
>  	u16 function;
>  };
>  
> +struct acpi_mem_mapping {
> +	acpi_physical_address physical_address;
> +	u8 *logical_address;
> +	acpi_size length;
> +	struct acpi_mem_mapping *next_mm;
> +};
> +
>  struct acpi_mem_space_context {
>  	u32 length;
>  	acpi_physical_address address;
> -	acpi_physical_address mapped_physical_address;
> -	u8 *mapped_logical_address;
> -	acpi_size mapped_length;
> +	struct acpi_mem_mapping *cur_mm;
> +	struct acpi_mem_mapping *first_mm;
>  };
>  
>  /*
> -- 
> 2.26.2
> 
> 
> 
>
Rafael J. Wysocki June 30, 2020, 11:44 a.m. UTC | #2
On Mon, Jun 29, 2020 at 10:57 PM Al Stone <ahs3@redhat.com> wrote:
>
> On 29 Jun 2020 18:33, Rafael J. Wysocki wrote:
> > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> >
> > 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 acess 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 preserve all of the memory mappings created by it until the memory
> > regions associated with them go away.
> >
> > Accordingly, update acpi_ev_system_memory_region_setup() to unmap all
> > memory associated with memory opregions that go away.
> >
> > Reported-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/acpica/evrgnini.c | 14 ++++----
> >  drivers/acpi/acpica/exregion.c | 65 ++++++++++++++++++++++++----------
> >  include/acpi/actypes.h         | 12 +++++--
> >  3 files changed, 64 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/acpi/acpica/evrgnini.c b/drivers/acpi/acpica/evrgnini.c
> > index aefc0145e583..89be3ccdad53 100644
> > --- a/drivers/acpi/acpica/evrgnini.c
> > +++ b/drivers/acpi/acpica/evrgnini.c
> > @@ -38,6 +38,7 @@ acpi_ev_system_memory_region_setup(acpi_handle handle,
> >       union acpi_operand_object *region_desc =
> >           (union acpi_operand_object *)handle;
> >       struct acpi_mem_space_context *local_region_context;
> > +     struct acpi_mem_mapping *mm;
> >
> >       ACPI_FUNCTION_TRACE(ev_system_memory_region_setup);
> >
> > @@ -46,13 +47,14 @@ acpi_ev_system_memory_region_setup(acpi_handle handle,
> >                       local_region_context =
> >                           (struct acpi_mem_space_context *)*region_context;
> >
> > -                     /* Delete a cached mapping if present */
> > +                     /* Delete memory mappings if present */
> >
> > -                     if (local_region_context->mapped_length) {
> > -                             acpi_os_unmap_memory(local_region_context->
> > -                                                  mapped_logical_address,
> > -                                                  local_region_context->
> > -                                                  mapped_length);
> > +                     while (local_region_context->first_mm) {
> > +                             mm = local_region_context->first_mm;
> > +                             local_region_context->first_mm = mm->next_mm;
> > +                             acpi_os_unmap_memory(mm->logical_address,
> > +                                                  mm->length);
> > +                             ACPI_FREE(mm);
> >                       }
> >                       ACPI_FREE(local_region_context);
> >                       *region_context = NULL;
> > diff --git a/drivers/acpi/acpica/exregion.c b/drivers/acpi/acpica/exregion.c
> > index d15a66de26c0..fd68f2134804 100644
> > --- a/drivers/acpi/acpica/exregion.c
> > +++ b/drivers/acpi/acpica/exregion.c
> > @@ -41,6 +41,7 @@ acpi_ex_system_memory_space_handler(u32 function,
> >       acpi_status status = AE_OK;
> >       void *logical_addr_ptr = NULL;
> >       struct acpi_mem_space_context *mem_info = region_context;
> > +     struct acpi_mem_mapping *mm = mem_info->cur_mm;
> >       u32 length;
> >       acpi_size map_length;
>
> I think this needs to be:
>
>         acpi_size map_length = mem_info->length;
>
> since it now gets used in the ACPI_ERROR() call below.

No, it's better to print the length value in the message.

>  I'm getting a "maybe used unitialized" error on compilation.

Thanks for reporting!

I've updated the commit in the acpica-osl branch with the fix.
Al Stone June 30, 2020, 3:31 p.m. UTC | #3
On 30 Jun 2020 13:44, Rafael J. Wysocki wrote:
> On Mon, Jun 29, 2020 at 10:57 PM Al Stone <ahs3@redhat.com> wrote:
> >
> > On 29 Jun 2020 18:33, Rafael J. Wysocki wrote:
> > > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > >
> > > 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 acess 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 preserve all of the memory mappings created by it until the memory
> > > regions associated with them go away.
> > >
> > > Accordingly, update acpi_ev_system_memory_region_setup() to unmap all
> > > memory associated with memory opregions that go away.
> > >
> > > Reported-by: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/acpi/acpica/evrgnini.c | 14 ++++----
> > >  drivers/acpi/acpica/exregion.c | 65 ++++++++++++++++++++++++----------
> > >  include/acpi/actypes.h         | 12 +++++--
> > >  3 files changed, 64 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/acpi/acpica/evrgnini.c b/drivers/acpi/acpica/evrgnini.c
> > > index aefc0145e583..89be3ccdad53 100644
> > > --- a/drivers/acpi/acpica/evrgnini.c
> > > +++ b/drivers/acpi/acpica/evrgnini.c
> > > @@ -38,6 +38,7 @@ acpi_ev_system_memory_region_setup(acpi_handle handle,
> > >       union acpi_operand_object *region_desc =
> > >           (union acpi_operand_object *)handle;
> > >       struct acpi_mem_space_context *local_region_context;
> > > +     struct acpi_mem_mapping *mm;
> > >
> > >       ACPI_FUNCTION_TRACE(ev_system_memory_region_setup);
> > >
> > > @@ -46,13 +47,14 @@ acpi_ev_system_memory_region_setup(acpi_handle handle,
> > >                       local_region_context =
> > >                           (struct acpi_mem_space_context *)*region_context;
> > >
> > > -                     /* Delete a cached mapping if present */
> > > +                     /* Delete memory mappings if present */
> > >
> > > -                     if (local_region_context->mapped_length) {
> > > -                             acpi_os_unmap_memory(local_region_context->
> > > -                                                  mapped_logical_address,
> > > -                                                  local_region_context->
> > > -                                                  mapped_length);
> > > +                     while (local_region_context->first_mm) {
> > > +                             mm = local_region_context->first_mm;
> > > +                             local_region_context->first_mm = mm->next_mm;
> > > +                             acpi_os_unmap_memory(mm->logical_address,
> > > +                                                  mm->length);
> > > +                             ACPI_FREE(mm);
> > >                       }
> > >                       ACPI_FREE(local_region_context);
> > >                       *region_context = NULL;
> > > diff --git a/drivers/acpi/acpica/exregion.c b/drivers/acpi/acpica/exregion.c
> > > index d15a66de26c0..fd68f2134804 100644
> > > --- a/drivers/acpi/acpica/exregion.c
> > > +++ b/drivers/acpi/acpica/exregion.c
> > > @@ -41,6 +41,7 @@ acpi_ex_system_memory_space_handler(u32 function,
> > >       acpi_status status = AE_OK;
> > >       void *logical_addr_ptr = NULL;
> > >       struct acpi_mem_space_context *mem_info = region_context;
> > > +     struct acpi_mem_mapping *mm = mem_info->cur_mm;
> > >       u32 length;
> > >       acpi_size map_length;
> >
> > I think this needs to be:
> >
> >         acpi_size map_length = mem_info->length;
> >
> > since it now gets used in the ACPI_ERROR() call below.
> 
> No, it's better to print the length value in the message.

Yeah, that was the other option.

> >  I'm getting a "maybe used unitialized" error on compilation.
> 
> Thanks for reporting!
> 
> I've updated the commit in the acpica-osl branch with the fix.

Thanks, Rafael.

Do you have a generic way of testing this?  I can see a way to do it
-- timing a call of a method in a dynamically loaded SSDT -- but if
you had a test case laying around, I could continue to be lazy :).
Rafael J. Wysocki June 30, 2020, 3:52 p.m. UTC | #4
On Tue, Jun 30, 2020 at 5:31 PM Al Stone <ahs3@redhat.com> wrote:
>
> On 30 Jun 2020 13:44, Rafael J. Wysocki wrote:
> > On Mon, Jun 29, 2020 at 10:57 PM Al Stone <ahs3@redhat.com> wrote:
> > >
> > > On 29 Jun 2020 18:33, Rafael J. Wysocki wrote:
> > > > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > > >
> > > > 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 acess 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 preserve all of the memory mappings created by it until the memory
> > > > regions associated with them go away.
> > > >
> > > > Accordingly, update acpi_ev_system_memory_region_setup() to unmap all
> > > > memory associated with memory opregions that go away.
> > > >
> > > > Reported-by: Dan Williams <dan.j.williams@intel.com>
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > ---
> > > >  drivers/acpi/acpica/evrgnini.c | 14 ++++----
> > > >  drivers/acpi/acpica/exregion.c | 65 ++++++++++++++++++++++++----------
> > > >  include/acpi/actypes.h         | 12 +++++--
> > > >  3 files changed, 64 insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/acpica/evrgnini.c b/drivers/acpi/acpica/evrgnini.c
> > > > index aefc0145e583..89be3ccdad53 100644
> > > > --- a/drivers/acpi/acpica/evrgnini.c
> > > > +++ b/drivers/acpi/acpica/evrgnini.c
> > > > @@ -38,6 +38,7 @@ acpi_ev_system_memory_region_setup(acpi_handle handle,
> > > >       union acpi_operand_object *region_desc =
> > > >           (union acpi_operand_object *)handle;
> > > >       struct acpi_mem_space_context *local_region_context;
> > > > +     struct acpi_mem_mapping *mm;
> > > >
> > > >       ACPI_FUNCTION_TRACE(ev_system_memory_region_setup);
> > > >
> > > > @@ -46,13 +47,14 @@ acpi_ev_system_memory_region_setup(acpi_handle handle,
> > > >                       local_region_context =
> > > >                           (struct acpi_mem_space_context *)*region_context;
> > > >
> > > > -                     /* Delete a cached mapping if present */
> > > > +                     /* Delete memory mappings if present */
> > > >
> > > > -                     if (local_region_context->mapped_length) {
> > > > -                             acpi_os_unmap_memory(local_region_context->
> > > > -                                                  mapped_logical_address,
> > > > -                                                  local_region_context->
> > > > -                                                  mapped_length);
> > > > +                     while (local_region_context->first_mm) {
> > > > +                             mm = local_region_context->first_mm;
> > > > +                             local_region_context->first_mm = mm->next_mm;
> > > > +                             acpi_os_unmap_memory(mm->logical_address,
> > > > +                                                  mm->length);
> > > > +                             ACPI_FREE(mm);
> > > >                       }
> > > >                       ACPI_FREE(local_region_context);
> > > >                       *region_context = NULL;
> > > > diff --git a/drivers/acpi/acpica/exregion.c b/drivers/acpi/acpica/exregion.c
> > > > index d15a66de26c0..fd68f2134804 100644
> > > > --- a/drivers/acpi/acpica/exregion.c
> > > > +++ b/drivers/acpi/acpica/exregion.c
> > > > @@ -41,6 +41,7 @@ acpi_ex_system_memory_space_handler(u32 function,
> > > >       acpi_status status = AE_OK;
> > > >       void *logical_addr_ptr = NULL;
> > > >       struct acpi_mem_space_context *mem_info = region_context;
> > > > +     struct acpi_mem_mapping *mm = mem_info->cur_mm;
> > > >       u32 length;
> > > >       acpi_size map_length;
> > >
> > > I think this needs to be:
> > >
> > >         acpi_size map_length = mem_info->length;
> > >
> > > since it now gets used in the ACPI_ERROR() call below.
> >
> > No, it's better to print the length value in the message.
>
> Yeah, that was the other option.
>
> > >  I'm getting a "maybe used unitialized" error on compilation.
> >
> > Thanks for reporting!
> >
> > I've updated the commit in the acpica-osl branch with the fix.
>
> Thanks, Rafael.
>
> Do you have a generic way of testing this?  I can see a way to do it
> -- timing a call of a method in a dynamically loaded SSDT -- but if
> you had a test case laying around, I could continue to be lazy :).

I don't check the timing, but instrument the code to see if what
happens is what is expected.

Now, the overhead reduction resulting from this change in Linux is
quite straightforward: Every time the current mapping doesn't cover
the request at hand, an unmap is carried out by the original code,
which involves a linear search through acpi_ioremaps, and which
generally is (at least a bit) more expensive than the linear search
through the list of opregion-specific mappings introduced by the
$subject patch, because quite likely the acpi_ioremaps list holds more
items.  And, of course, if the opregion in question holds many fields
and they are not covered by one mapping, each of them needs to be
mapped just once per the opregion life cycle.
Al Stone June 30, 2020, 7:57 p.m. UTC | #5
On 30 Jun 2020 17:52, Rafael J. Wysocki wrote:
> On Tue, Jun 30, 2020 at 5:31 PM Al Stone <ahs3@redhat.com> wrote:
> >
> > On 30 Jun 2020 13:44, Rafael J. Wysocki wrote:
> > > On Mon, Jun 29, 2020 at 10:57 PM Al Stone <ahs3@redhat.com> wrote:
> > > >
> > > > On 29 Jun 2020 18:33, Rafael J. Wysocki wrote:
> > > > > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > > > >
> > > > > 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 acess 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 preserve all of the memory mappings created by it until the memory
> > > > > regions associated with them go away.
> > > > >
> > > > > Accordingly, update acpi_ev_system_memory_region_setup() to unmap all
> > > > > memory associated with memory opregions that go away.
> > > > >
> > > > > Reported-by: Dan Williams <dan.j.williams@intel.com>
> > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > ---
> > > > >  drivers/acpi/acpica/evrgnini.c | 14 ++++----
> > > > >  drivers/acpi/acpica/exregion.c | 65 ++++++++++++++++++++++++----------
> > > > >  include/acpi/actypes.h         | 12 +++++--
> > > > >  3 files changed, 64 insertions(+), 27 deletions(-)
> > > > >
> > > > > diff --git a/drivers/acpi/acpica/evrgnini.c b/drivers/acpi/acpica/evrgnini.c
> > > > > index aefc0145e583..89be3ccdad53 100644
> > > > > --- a/drivers/acpi/acpica/evrgnini.c
> > > > > +++ b/drivers/acpi/acpica/evrgnini.c
> > > > > @@ -38,6 +38,7 @@ acpi_ev_system_memory_region_setup(acpi_handle handle,
> > > > >       union acpi_operand_object *region_desc =
> > > > >           (union acpi_operand_object *)handle;
> > > > >       struct acpi_mem_space_context *local_region_context;
> > > > > +     struct acpi_mem_mapping *mm;
> > > > >
> > > > >       ACPI_FUNCTION_TRACE(ev_system_memory_region_setup);
> > > > >
> > > > > @@ -46,13 +47,14 @@ acpi_ev_system_memory_region_setup(acpi_handle handle,
> > > > >                       local_region_context =
> > > > >                           (struct acpi_mem_space_context *)*region_context;
> > > > >
> > > > > -                     /* Delete a cached mapping if present */
> > > > > +                     /* Delete memory mappings if present */
> > > > >
> > > > > -                     if (local_region_context->mapped_length) {
> > > > > -                             acpi_os_unmap_memory(local_region_context->
> > > > > -                                                  mapped_logical_address,
> > > > > -                                                  local_region_context->
> > > > > -                                                  mapped_length);
> > > > > +                     while (local_region_context->first_mm) {
> > > > > +                             mm = local_region_context->first_mm;
> > > > > +                             local_region_context->first_mm = mm->next_mm;
> > > > > +                             acpi_os_unmap_memory(mm->logical_address,
> > > > > +                                                  mm->length);
> > > > > +                             ACPI_FREE(mm);
> > > > >                       }
> > > > >                       ACPI_FREE(local_region_context);
> > > > >                       *region_context = NULL;
> > > > > diff --git a/drivers/acpi/acpica/exregion.c b/drivers/acpi/acpica/exregion.c
> > > > > index d15a66de26c0..fd68f2134804 100644
> > > > > --- a/drivers/acpi/acpica/exregion.c
> > > > > +++ b/drivers/acpi/acpica/exregion.c
> > > > > @@ -41,6 +41,7 @@ acpi_ex_system_memory_space_handler(u32 function,
> > > > >       acpi_status status = AE_OK;
> > > > >       void *logical_addr_ptr = NULL;
> > > > >       struct acpi_mem_space_context *mem_info = region_context;
> > > > > +     struct acpi_mem_mapping *mm = mem_info->cur_mm;
> > > > >       u32 length;
> > > > >       acpi_size map_length;
> > > >
> > > > I think this needs to be:
> > > >
> > > >         acpi_size map_length = mem_info->length;
> > > >
> > > > since it now gets used in the ACPI_ERROR() call below.
> > >
> > > No, it's better to print the length value in the message.
> >
> > Yeah, that was the other option.
> >
> > > >  I'm getting a "maybe used unitialized" error on compilation.
> > >
> > > Thanks for reporting!
> > >
> > > I've updated the commit in the acpica-osl branch with the fix.
> >
> > Thanks, Rafael.
> >
> > Do you have a generic way of testing this?  I can see a way to do it
> > -- timing a call of a method in a dynamically loaded SSDT -- but if
> > you had a test case laying around, I could continue to be lazy :).
> 
> I don't check the timing, but instrument the code to see if what
> happens is what is expected.

Ah, okay.  Thanks.

> Now, the overhead reduction resulting from this change in Linux is
> quite straightforward: Every time the current mapping doesn't cover
> the request at hand, an unmap is carried out by the original code,
> which involves a linear search through acpi_ioremaps, and which
> generally is (at least a bit) more expensive than the linear search
> through the list of opregion-specific mappings introduced by the
> $subject patch, because quite likely the acpi_ioremaps list holds more
> items.  And, of course, if the opregion in question holds many fields
> and they are not covered by one mapping, each of them needs to be
> mapped just once per the opregion life cycle.

Right.  What I was debating as a generic test was something to try to
force an OpRegion through mapping and unmapping repeatedly with the
current code to determine a rough average elapsed time.  Then, apply
the patch to see what the change does.  Granted, a completely synthetic
scenario, and specifically designed to exaggerate the overhead, but
I'm just curious.
Verma, Vishal L July 16, 2020, 7:22 p.m. UTC | #6
On Mon, 2020-06-29 at 18:33 +0200, Rafael J. Wysocki wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> 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 acess 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 preserve all of the memory mappings created by it until the memory
> regions associated with them go away.
> 
> Accordingly, update acpi_ev_system_memory_region_setup() to unmap all
> memory associated with memory opregions that go away.
> 
> Reported-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/acpica/evrgnini.c | 14 ++++----
>  drivers/acpi/acpica/exregion.c | 65 ++++++++++++++++++++++++----------
>  include/acpi/actypes.h         | 12 +++++--
>  3 files changed, 64 insertions(+), 27 deletions(-)
> 

Hi Rafael,

Picking up from Dan while he's out - I had these patches tested by the
original reporter, and they work fine. I see you had them staged in the
acpica-osl branch. Is that slated to go in during the 5.9 merge window?

You can add:
Tested-by: Xiang Li <xiang.z.li@intel.com>
Rafael J. Wysocki July 19, 2020, 7:14 p.m. UTC | #7
On Thu, Jul 16, 2020 at 9:22 PM Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
>
> On Mon, 2020-06-29 at 18:33 +0200, Rafael J. Wysocki wrote:
> > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> >
> > 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 acess 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 preserve all of the memory mappings created by it until the memory
> > regions associated with them go away.
> >
> > Accordingly, update acpi_ev_system_memory_region_setup() to unmap all
> > memory associated with memory opregions that go away.
> >
> > Reported-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/acpica/evrgnini.c | 14 ++++----
> >  drivers/acpi/acpica/exregion.c | 65 ++++++++++++++++++++++++----------
> >  include/acpi/actypes.h         | 12 +++++--
> >  3 files changed, 64 insertions(+), 27 deletions(-)
> >
>
> Hi Rafael,
>
> Picking up from Dan while he's out - I had these patches tested by the
> original reporter, and they work fine. I see you had them staged in the
> acpica-osl branch. Is that slated to go in during the 5.9 merge window?

Yes, it is.

> You can add:
> Tested-by: Xiang Li <xiang.z.li@intel.com>

Thank you!

Patch
diff mbox series

diff --git a/drivers/acpi/acpica/evrgnini.c b/drivers/acpi/acpica/evrgnini.c
index aefc0145e583..89be3ccdad53 100644
--- a/drivers/acpi/acpica/evrgnini.c
+++ b/drivers/acpi/acpica/evrgnini.c
@@ -38,6 +38,7 @@  acpi_ev_system_memory_region_setup(acpi_handle handle,
 	union acpi_operand_object *region_desc =
 	    (union acpi_operand_object *)handle;
 	struct acpi_mem_space_context *local_region_context;
+	struct acpi_mem_mapping *mm;
 
 	ACPI_FUNCTION_TRACE(ev_system_memory_region_setup);
 
@@ -46,13 +47,14 @@  acpi_ev_system_memory_region_setup(acpi_handle handle,
 			local_region_context =
 			    (struct acpi_mem_space_context *)*region_context;
 
-			/* Delete a cached mapping if present */
+			/* Delete memory mappings if present */
 
-			if (local_region_context->mapped_length) {
-				acpi_os_unmap_memory(local_region_context->
-						     mapped_logical_address,
-						     local_region_context->
-						     mapped_length);
+			while (local_region_context->first_mm) {
+				mm = local_region_context->first_mm;
+				local_region_context->first_mm = mm->next_mm;
+				acpi_os_unmap_memory(mm->logical_address,
+						     mm->length);
+				ACPI_FREE(mm);
 			}
 			ACPI_FREE(local_region_context);
 			*region_context = NULL;
diff --git a/drivers/acpi/acpica/exregion.c b/drivers/acpi/acpica/exregion.c
index d15a66de26c0..fd68f2134804 100644
--- a/drivers/acpi/acpica/exregion.c
+++ b/drivers/acpi/acpica/exregion.c
@@ -41,6 +41,7 @@  acpi_ex_system_memory_space_handler(u32 function,
 	acpi_status status = AE_OK;
 	void *logical_addr_ptr = NULL;
 	struct acpi_mem_space_context *mem_info = region_context;
+	struct acpi_mem_mapping *mm = mem_info->cur_mm;
 	u32 length;
 	acpi_size map_length;
 	acpi_size page_boundary_map_length;
@@ -96,20 +97,38 @@  acpi_ex_system_memory_space_handler(u32 function,
 	 * Is 1) Address below the current mapping? OR
 	 *    2) Address beyond the current mapping?
 	 */
-	if ((address < mem_info->mapped_physical_address) ||
-	    (((u64) address + length) > ((u64)
-					 mem_info->mapped_physical_address +
-					 mem_info->mapped_length))) {
+	if (!mm || (address < mm->physical_address) ||
+	    ((u64) address + length > (u64) mm->physical_address + mm->length)) {
 		/*
-		 * The request cannot be resolved by the current memory mapping;
-		 * Delete the existing mapping and create a new one.
+		 * The request cannot be resolved by the current memory mapping.
+		 *
+		 * Look for an existing saved mapping covering the address range
+		 * at hand.  If found, save it as the current one and carry out
+		 * the access.
 		 */
-		if (mem_info->mapped_length) {
+		for (mm = mem_info->first_mm; mm; mm = mm->next_mm) {
+			if (mm == mem_info->cur_mm)
+				continue;
+
+			if (address < mm->physical_address)
+				continue;
+
+			if ((u64) address + length >
+					(u64) mm->physical_address + mm->length)
+				continue;
 
-			/* Valid mapping, delete it */
+			mem_info->cur_mm = mm;
+			goto access;
+		}
 
-			acpi_os_unmap_memory(mem_info->mapped_logical_address,
-					     mem_info->mapped_length);
+		/* Create a new mappings list entry */
+		mm = ACPI_ALLOCATE_ZEROED(sizeof(*mm));
+		if (!mm) {
+			ACPI_ERROR((AE_INFO,
+				    "Unable to save memory mapping at 0x%8.8X%8.8X, size %u",
+				    ACPI_FORMAT_UINT64(address),
+				    (u32)map_length));
+			return_ACPI_STATUS(AE_NO_MEMORY);
 		}
 
 		/*
@@ -143,29 +162,39 @@  acpi_ex_system_memory_space_handler(u32 function,
 
 		/* Create a new mapping starting at the address given */
 
-		mem_info->mapped_logical_address =
-		    acpi_os_map_memory(address, map_length);
-		if (!mem_info->mapped_logical_address) {
+		logical_addr_ptr = acpi_os_map_memory(address, map_length);
+		if (!logical_addr_ptr) {
 			ACPI_ERROR((AE_INFO,
 				    "Could not map memory at 0x%8.8X%8.8X, size %u",
 				    ACPI_FORMAT_UINT64(address),
 				    (u32)map_length));
-			mem_info->mapped_length = 0;
+			ACPI_FREE(mm);
 			return_ACPI_STATUS(AE_NO_MEMORY);
 		}
 
 		/* Save the physical address and mapping size */
 
-		mem_info->mapped_physical_address = address;
-		mem_info->mapped_length = map_length;
+		mm->logical_address = logical_addr_ptr;
+		mm->physical_address = address;
+		mm->length = map_length;
+
+		/*
+		 * Add the new entry to the mappigs list and save it as the
+		 * current mapping.
+		 */
+		mm->next_mm = mem_info->first_mm;
+		mem_info->first_mm = mm;
+
+		mem_info->cur_mm = mm;
 	}
 
+access:
 	/*
 	 * Generate a logical pointer corresponding to the address we want to
 	 * access
 	 */
-	logical_addr_ptr = mem_info->mapped_logical_address +
-	    ((u64) address - (u64) mem_info->mapped_physical_address);
+	logical_addr_ptr = mm->logical_address +
+		((u64) address - (u64) mm->physical_address);
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 			  "System-Memory (width %u) R/W %u Address=%8.8X%8.8X\n",
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index aa236b9e6f24..d005e35ab399 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -1201,12 +1201,18 @@  struct acpi_pci_id {
 	u16 function;
 };
 
+struct acpi_mem_mapping {
+	acpi_physical_address physical_address;
+	u8 *logical_address;
+	acpi_size length;
+	struct acpi_mem_mapping *next_mm;
+};
+
 struct acpi_mem_space_context {
 	u32 length;
 	acpi_physical_address address;
-	acpi_physical_address mapped_physical_address;
-	u8 *mapped_logical_address;
-	acpi_size mapped_length;
+	struct acpi_mem_mapping *cur_mm;
+	struct acpi_mem_mapping *first_mm;
 };
 
 /*