[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 15/52] xen: make VMAP only support in MMU system
On 26.06.2023 05:34, Penny Zheng wrote: > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -27,6 +27,7 @@ config X86 > select HAS_PDX > select HAS_SCHED_GRANULARITY > select HAS_UBSAN > + select HAS_VMAP With this being unconditional, ... > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -1750,12 +1750,14 @@ void __init noreturn __start_xen(unsigned long mbi_p) > end_boot_allocator(); > > system_state = SYS_STATE_boot; > +#ifdef CONFIG_HAS_VMAP > /* > * No calls involving ACPI code should go between the setting of > * SYS_STATE_boot and vm_init() (or else acpi_os_{,un}map_memory() > * will break). > */ > vm_init(); > +#endif ... there's no need to make this code less readable by adding #ifdef. Even for the Arm side I question the #ifdef-ary - it likely would be better to have an empty stub in the header for that case. > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -15,6 +15,7 @@ config CORE_PARKING > config GRANT_TABLE > bool "Grant table support" if EXPERT > default y > + depends on HAS_VMAP Looking back at the commit which added use of vmap.h there, I can't seem to be bale to spot why it did. Where's the dependency there? And even if there is one, I think you don't want to unconditionally turn off a core, guest-visible feature. Instead you would want to identify a way to continue to support the feature in perhaps slightly less efficient a way. > --- a/xen/common/vmap.c > +++ b/xen/common/vmap.c > @@ -331,4 +331,11 @@ void vfree(void *va) > while ( (pg = page_list_remove_head(&pg_list)) != NULL ) > free_domheap_page(pg); > } > + > +void iounmap(void __iomem *va) > +{ > + unsigned long addr = (unsigned long)(void __force *)va; > + > + vunmap((void *)(addr & PAGE_MASK)); > +} Why does this move here? > --- a/xen/include/xen/vmap.h > +++ b/xen/include/xen/vmap.h > @@ -1,4 +1,4 @@ > -#if !defined(__XEN_VMAP_H__) && defined(VMAP_VIRT_START) > +#if !defined(__XEN_VMAP_H__) && (defined(VMAP_VIRT_START) || > !defined(CONFIG_HAS_VMAP)) > #define __XEN_VMAP_H__ > > #include <xen/mm-frame.h> > @@ -25,17 +25,14 @@ void vfree(void *va); > > void __iomem *ioremap(paddr_t, size_t); > > -static inline void iounmap(void __iomem *va) > -{ > - unsigned long addr = (unsigned long)(void __force *)va; > - > - vunmap((void *)(addr & PAGE_MASK)); > -} > +void iounmap(void __iomem *va); > > void *arch_vmap_virt_end(void); > static inline void vm_init(void) > { > +#if defined(VMAP_VIRT_START) > vm_init_type(VMAP_DEFAULT, (void *)VMAP_VIRT_START, > arch_vmap_virt_end()); > +#endif > } Why the (seemingly unrelated) #ifdef-ary here? You've eliminated the problematic caller already. Didn't you mean to add wider scope #ifdef CONFIG_HAS_VMAP to this header? Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |