Re: [PATCH v3 15/52] xen: make VMAP only support in MMU system

[Penny Zheng <penny.zheng@xxxxxxx>]

Hi Jan

On 2023/6/26 14:00, Jan Beulich wrote:
> 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.
I may misunderstand what you said in last serie and thought #ifdef-ary 
is preferred compared with inline stubs, referring here to recall
'''
Do you really need this and all other inline stubs? Imo the goal ought
to be to have as few of them as possible: The one above won't be
referenced if you further make LIVEPATCH depend on HAS_VMAP (which I
think you need to do anyway), and the only other call to the function
is visible in context above (i.e. won't be used either when !HAS_VMAP).
In other cases merely having a declaration (but no definition) may be
sufficient, as the compiler may be able to eliminate calls.
'''
So maybe the preferring order is "declaration > empty inline stubs > 
#ifdef-ary" ? As having a declaration is not enough for vm_init()(when
!HAS_VMAP), we provide static inline empty stub here.

> > --- 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?
vzalloc() is used in grant_table_init() in xen/common/grantable.c:2023

> 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.
We have discussed in MPU design, normally, xen guest in MPU system
will be a linux guest with no pv(with CONFIG_XEN=n), so advanced 
features like grant table is in no use there.
> > --- 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?
ioremap/iounmap is using vmap, at least in ARM. So for this more
generic interface, I was intending to implement it on MPU system.
In commit "[PATCH v3 19/52] xen/arm: switch to use ioremap_xxx in common 
file", I'm trying to replace all direct vmap interface with ioremap_xxx 
in common files.
Then I, in commit "[PATCH v3 36/52] xen/mpu: implememt ioremap_xxx in 
MPU", will implement MPU version of ioremap_xxx.
> > --- 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?
plz correct me if I'm wrong, in MPU system with !HAS_VMAP, if we don't
#ifdef-ary here, we will have the following compilation error, I guess
it's because that vm_init() is a static inline stub:
```
./include/xen/vmap.h: In function ‘vm_init’:
./include/xen/vmap.h:33:40: error: ‘VMAP_VIRT_START’ undeclared (first 
use in this function); did you mean ‘XEN_VIRT_START’?
   33 |     vm_init_type(VMAP_DEFAULT, (void *)VMAP_VIRT_START, 
arch_vmap_virt_end());
      |                                        ^~~~~~~~~~~~~~~
      |                                        XEN_VIRT_START
```

> Jan