Re: [PATCH v2 1/7] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.11.2020 18:44, Julien Grall wrote:
> Hi Jan,
> 
> On 20/11/2020 16:03, Jan Beulich wrote:
>> On 23.10.2020 17:41, Julien Grall wrote:
>>> From: Julien Grall <jgrall@xxxxxxxxxx>
>>>
>>> The functions acpi_os_{un,}map_memory() are meant to be arch-agnostic
>>> while the __acpi_os_{un,}map_memory() are meant to be arch-specific.
>>>
>>> Currently, the former are still containing x86 specific code.
>>>
>>> To avoid this rather strange split, the generic helpers are reworked so
>>> they are arch-agnostic. This requires the introduction of a new helper
>>> __acpi_os_unmap_memory() that will undo any mapping done by
>>> __acpi_os_map_memory().
>>>
>>> Currently, the arch-helper for unmap is basically a no-op so it only
>>> returns whether the mapping was arch specific. But this will change
>>> in the future.
>>>
>>> Note that the x86 version of acpi_os_map_memory() was already able to
>>> able the 1MB region. Hence why there is no addition of new code.
>>>
>>> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
>>> Reviewed-by: Rahul Singh <rahul.singh@xxxxxxx>
>>> Tested-by: Rahul Singh <rahul.singh@xxxxxxx>
>>
>> This change breaks shutdown on x86. Either Dom0 no longer requests S5
>> (in which case I'd expect some data collection there to fail), or Xen
>> refuses the request. As a result, things go the machine_halt() path
>> instead. I've looked over the change again, but couldn't spot anything
>> yet which might explain the behavior. Yet reverting (just the non-Arm
>> parts, so I wouldn't have to revert multiple commits) made things
>> work again.
> 
> Thank you for the report and sorry for the breakage.
> 
> When changing the behavior of __acpi_map_table(), I failed to realize 
> that some x86 code will call it directly rather than 
> acpi_os_map_memory(). This is the case of acpi_fadt_parse_sleep_info() 
> which detects whether ACPI can be used to put the system in sleep state.
> 
> I am tempted to require all the callers requiring to map memory to use 
> the generic implementation acpi_os_{, un}map_memory().
> 
> However, AFAICT, some of the callers (such as acpi_sleep_prepare()) are 
> using __acpi_map_table() because the function never failed before. By 
> using the generic function, all mappings after boot will be using vmap() 
> which may fail.

The FACS mapping can (and perhaps should long have been) be switched
to acpi_os_map_memory(). The other two direct uses of the function,
however, will require more care. I'm in the process or making a
series, also noticing some further shortcomings of the FACS handling.

Jan