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

[Wei Xu <xuwei5@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel