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

[Wei Xu <xuwei5@hisilicon.com>]

Hi Jan,

On 2020/1/21 19:02, Jan Beulich wrote:
> On 21.01.2020 10:49, Wei Xu wrote:
>> Add __acpi_unmap_table function for ARM and invoke it at acpi_os_unmap_memory
>> to make sure the related fixmap has been cleared before using it for a
>> different mapping.
> 
> How can it possibly be that this is needed for Arm only?

Sorry, I think Julien has helped to explain this.

> 
>> --- a/xen/arch/arm/acpi/lib.c
>> +++ b/xen/arch/arm/acpi/lib.c
>> @@ -49,6 +49,31 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>>       return ((char *) base + offset);
>>   }
>>   
>> +void __acpi_unmap_table(void __iomem * virt, unsigned long size)
>> +{
>> +    unsigned long base, end;
>> +    int idx;
> 
> unsigned int

OK.
I will change the type.

> 
>> +    base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN);
>> +    end = FIXMAP_ADDR(FIXMAP_ACPI_END);
>> +
>> +    if ( (unsigned long)virt < base || (unsigned long)virt > end )
>> +    {
>> +        return;
>> +    }
> 
> Stray braces.

OK.
I will remove the braces.

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

	diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
	--- a/xen/drivers/acpi/osl.c
	+++ b/xen/drivers/acpi/osl.c
	@@ -114,10 +114,10 @@ void acpi_os_unmap_memory(void __iomem * virt, acpi_size size)
					return;
			}

			if (system_state >= SYS_STATE_boot)
					vunmap((void *)((unsigned long)virt & PAGE_MASK));
	+
	+       __acpi_unmap_table(virt, size);
	 }

	 acpi_status acpi_os_read_port(acpi_io_address port, u32 * value, u32 width)
	diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
	--- a/xen/include/xen/acpi.h
	+++ b/xen/include/xen/acpi.h
	@@ -68,7 +68,13 @@ typedef int (*acpi_table_entry_handler) (struct acpi_subtable_header *header, co

	 unsigned int acpi_get_processor_id (unsigned int cpu);
	 char * __acpi_map_table (paddr_t phys_addr, unsigned long size);
	+
	+#ifdef CONFIG_ARM
	+void __acpi_unmap_table(void __iomem *virt, unsigned long size);
	+#else
	+void __acpi_unmap_table(void __iomem *virt, unsigned long size) { }
	+#endif /* CONFIG_ARM */


>> --- a/xen/include/xen/acpi.h
>> +++ b/xen/include/xen/acpi.h
>> @@ -68,6 +68,7 @@ typedef int (*acpi_table_entry_handler) (struct acpi_subtable_header *header, co
>>   
>>   unsigned int acpi_get_processor_id (unsigned int cpu);
>>   char * __acpi_map_table (paddr_t phys_addr, unsigned long size);
>> +void __acpi_unmap_table(void __iomem * virt, unsigned long size);
>>   int acpi_boot_init (void);
>>   int acpi_boot_table_init (void);
>>   int acpi_numa_init (void);
> 
> Best noticable here, your mailer has mangled the patch. The way
> of this mangling makes me guess you used Thunderbird to send the
> patch, in which case you'll need to adjust its settings (iirc it
> was mailnews.wraplength which needed setting to zero).

Yes, I used Thunderbird and did not set that :(.
Thanks!

Best Regards,
Wei

> 
> Jan
> 
> .
> 


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