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

[Rafael J. Wysocki]

On Sat, Jun 27, 2020 at 12:53 AM Kaneda, Erik <erik.kaneda@xxxxxxxxx> wrote:
>
>
>
> > -----Original Message-----
> > From: Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>
> > Sent: Monday, June 22, 2020 7:02 AM
> > To: Williams, Dan J <dan.j.williams@xxxxxxxxx>; Kaneda, Erik
> > <erik.kaneda@xxxxxxxxx>
> > Cc: Wysocki, Rafael J <rafael.j.wysocki@xxxxxxxxx>; Len Brown
> > <lenb@xxxxxxxxxx>; Borislav Petkov <bp@xxxxxxxxx>; Weiny, Ira
> > <ira.weiny@xxxxxxxxx>; James Morse <james.morse@xxxxxxx>; Myron
> > Stowe <myron.stowe@xxxxxxxxxx>; Andy Shevchenko
> > <andriy.shevchenko@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; linux-
> > acpi@xxxxxxxxxxxxxxx; linux-nvdimm@xxxxxxxxxxxx; Moore, Robert
> > <robert.moore@xxxxxxxxx>
> > Subject: [RFT][PATCH v2 3/4] ACPICA: Preserve memory opregion mappings
> > if supported by OS
> >
> > From: "Rafael J. Wysocki" <rafael.j.wysocki@xxxxxxxxx>
> >
> > 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?

Yes, and to protect the ones that have not added support for it just yet.

Without the "fast-path" mapping support, ACPICA has no way to obtain
additional references to known-existing mappings and the new code
won't work as expected without it, so it is better to avoid building
that code at all in those cases IMO.

> 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.

Well, the naming is not particularly important to me to be honest, but
this is mostly about being able to get a new reference to a
known-existing memory mapping.

So something like acpi_os_ref_memory_map() perhaps?

But I'm thinking that this can be implemented without the "fast-path"
mapping support too, let me try to do that.

Cheers!