| Message ID | 5E26C935.9080107@hisilicon.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Xen-devel] arm/acpi: Add __acpi_unmap_table function for ARM | expand |
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
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
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,
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 >
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 > > . >
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, >
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
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 --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);
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 .