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

From: Wei Xu <xuwei5@hisilicon.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>, Wei Liu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Linuxarm <linuxarm@huawei.com>,
	Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>,
	"Zengtao \(B\)" <prime.zeng@hisilicon.com>,
	xen-devel@lists.xenproject.org, Volodymyr_Babchuk@epam.com
Subject: Re: [Xen-devel] [PATCH] arm/acpi: Add __acpi_unmap_table function for ARM
Date: Wed, 22 Jan 2020 17:04:28 +0800	[thread overview]
Message-ID: <5E28101C.2030405@hisilicon.com> (raw)
In-Reply-To: <e5082f00-ad1a-ab2b-bc1f-b58714d08a02@suse.com>

Hi Jan,

On 2020/1/22 16:24, Jan Beulich wrote:
> 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.

Got it.
I will check how to clear the fixmap on X86.
Thanks!

Best Regards,
Wei

> 
> Jan
> 
> .
> 

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