diff mbox series

arm/acpi: Add __acpi_unmap_table function for ARM

Message ID 5E26C935.9080107@hisilicon.com (mailing list archive)
State New, archived
Headers show
Series arm/acpi: Add __acpi_unmap_table function for ARM | expand

Commit Message

Wei Xu Jan. 21, 2020, 9:49 a.m. UTC
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.

Signed-off-by: Wei Xu<xuwei5@hisilicon.com>
---
  xen/arch/arm/acpi/lib.c | 25 +++++++++++++++++++++++++
  xen/drivers/acpi/osl.c  |  2 ++
  xen/include/xen/acpi.h  |  1 +
  3 files changed, 28 insertions(+)

-- 2.8.1 .

Comments

Alexandru Stefan ISAILA Jan. 21, 2020, 10:01 a.m. UTC | #1
On 21.01.2020 11: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.
> 
> Signed-off-by: Wei Xu<xuwei5@hisilicon.com>
> ---
>   xen/arch/arm/acpi/lib.c | 25 +++++++++++++++++++++++++
>   xen/drivers/acpi/osl.c  |  2 ++
>   xen/include/xen/acpi.h  |  1 +
>   3 files changed, 28 insertions(+)
> 
> diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c
> index 4fc6e17..69e87ec 100644
> --- 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)

You have a stray space here after "*"

> +{
> +    unsigned long base, end;
> +    int idx;
> +
> +    base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN);
> +    end = FIXMAP_ADDR(FIXMAP_ACPI_END);

Maybe the initialization can be moved to the declaration?


Alex
Jan Beulich Jan. 21, 2020, 11:02 a.m. UTC | #2
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?

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

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

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

> --- 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).

Jan
Julien Grall Jan. 21, 2020, 11:25 a.m. UTC | #3
Hi Jan,

On 21/01/2020 11: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?

Let me give some background (I will let Wei dealing with the rest of the 
patches). On Arm, I made the decision to forbid a mapping replacement in 
the page-tables code. This means that if you want to re-use the same 
fixmap, then you need to clear it first.

The reason for the requirement is quite simple. On Arm, you need to use 
a break-before-make sequence any time you replace a valid entry by 
another valid entry (there is a couple of case where it is not needed).

As the sequence name suggests it, we will have a small window where the 
virtual address will point to nothing. This may result to an abort if 
another CPU is accessing the address at the same time.

In the fixmap case below, this should never happen. But now imagine 
shattering a superpage...

So rather than trying to allow in some cases the modification of a 
mapping, we just forbid for everything but permission changes. This is 
much simpler to reason and a much saner interface.

Similarly, I think this is much saner to call have a clear_fixmap() for 
each set_fixmap().

Cheers,
Wei Xu Jan. 22, 2020, 1:51 a.m. UTC | #4
Hi Alexandru,

On 2020/1/21 18:01, Alexandru Stefan ISAILA wrote:
> 
> 
> On 21.01.2020 11: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.
>>
>> Signed-off-by: Wei Xu<xuwei5@hisilicon.com>
>> ---
>>   xen/arch/arm/acpi/lib.c | 25 +++++++++++++++++++++++++
>>   xen/drivers/acpi/osl.c  |  2 ++
>>   xen/include/xen/acpi.h  |  1 +
>>   3 files changed, 28 insertions(+)
>>
>> diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c
>> index 4fc6e17..69e87ec 100644
>> --- 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)
> 
> You have a stray space here after "*"

Sorry, I will remove it.

> 
>> +{
>> +    unsigned long base, end;
>> +    int idx;
>> +
>> +    base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN);
>> +    end = FIXMAP_ADDR(FIXMAP_ACPI_END);
> 
> Maybe the initialization can be moved to the declaration?

OK.
I will move it.

Thanks!

Best Regards,
Wei

> 
> 
> Alex
>
Wei Xu Jan. 22, 2020, 5:57 a.m. UTC | #5
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
> 
> .
>
Wei Xu Jan. 22, 2020, 5:58 a.m. UTC | #6
Hi Julien,

On 2020/1/21 19:25, Julien Grall wrote:
> Hi Jan,
> 
> On 21/01/2020 11: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?
> 
> Let me give some background (I will let Wei dealing with the rest of the patches). On Arm, I made the decision to forbid a mapping replacement in the page-tables code. This means that if you want to re-use the same fixmap, then you need to clear it first.
> 
> The reason for the requirement is quite simple. On Arm, you need to use a break-before-make sequence any time you replace a valid entry by another valid entry (there is a couple of case where it is not needed).
> 
> As the sequence name suggests it, we will have a small window where the virtual address will point to nothing. This may result to an abort if another CPU is accessing the address at the same time.
> 
> In the fixmap case below, this should never happen. But now imagine shattering a superpage...
> 
> So rather than trying to allow in some cases the modification of a mapping, we just forbid for everything but permission changes. This is much simpler to reason and a much saner interface.
> 
> Similarly, I think this is much saner to call have a clear_fixmap() for each set_fixmap().

Thanks to explain!

Best Regards,
Wei

> 
> Cheers,
>
Jan Beulich Jan. 22, 2020, 8:24 a.m. UTC | #7
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
Wei Xu Jan. 22, 2020, 9:04 a.m. UTC | #8
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
> 
> .
>
diff mbox series

Patch

diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c
index 4fc6e17..69e87ec 100644
--- 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;
+
+    base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN);
+    end = FIXMAP_ADDR(FIXMAP_ACPI_END);
+
+    if ( (unsigned long)virt < base || (unsigned long)virt > end )
+    {
+        return;
+    }
+
+    idx = FIXMAP_ACPI_BEGIN + ((unsigned long)virt - base) / PAGE_SIZE;
+    clear_fixmap(idx);
+
+    while ( size > PAGE_SIZE )
+    {
+        if ( ++idx > FIXMAP_ACPI_END )
+            return;
+        clear_fixmap(idx);
+        size -= PAGE_SIZE;
+    }
+}
+
  /* True to indicate PSCI 0.2+ is implemented */
  bool __init acpi_psci_present(void)
  {
diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
index 4c8bb78..18666c7 100644
--- 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));
  }
diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
index 5cfa060..acb00a2 100644
--- 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);