Re: [Xen-devel] [PATCH] arm/acpi: Add __acpi_unmap_table function for ARM

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.01.2020 06:57, Wei Xu wrote:
> On 2020/1/21 19:02, Jan Beulich wrote:
>> On 21.01.2020 10:49, Wei Xu wrote:
>>> --- a/xen/drivers/acpi/osl.c
>>> +++ b/xen/drivers/acpi/osl.c
>>> @@ -114,6 +114,8 @@ void acpi_os_unmap_memory(void __iomem * virt, 
>>> acpi_size size)
>>>             return;
>>>     }
>>>   
>>> +   __acpi_unmap_table(virt, size);
>>> +
>>>     if (system_state >= SYS_STATE_boot)
>>>             vunmap((void *)((unsigned long)virt & PAGE_MASK));
>>
>> How can it possibly be correct to call both vunmap() and your new
>> function? And how is this, having jsut an Arm implementation,
>> going to compile for x86? Seeing that x86 gets away without this,
>> may I suggest that you look at the x86 code to see why that is,
>> and then consider whether the same model makes sense for Arm? And
>> if it doesn't, check whether the new Arm model would make sense
>> to also use on x86?
>>
> 
> Sorry, I thought acpi_os_unmap_memory is specific for ARM.
> Just now I checked map_pages_to_xen in arch/x86/mm.c and did not find any 
> place
> to forbid the modification of a mapping. Maybe clearing mapping before 
> modification
> is not necessary for X86. Do you think is it OK to add a empty stub function
> for the other cases except ARM and invoke it after vunmap as following?

No. This is still doing two unmaps when system_state >= SYS_STATE_boot.
At the very least this need to go in an "else" block to the existing
if(). There also shouldn't be a blanket empty stub function. Even on
x86 it would be _better_ (albeit not strictly needed) if the unmap
indeed zapped the fixmap mappings. And for potential future ports it
would be outright dangerous to have such an empty stub.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel