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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.06.2023 07:38, Penny Zheng wrote:
> 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" ?

Indeed.

> As having a declaration is not enough for vm_init()(when
> !HAS_VMAP), we provide static inline empty stub here.

Really you change the existing vm_init() to an empty stub already; no
need to have a 2nd one (if that was a consideration).

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

You'll need vzalloc() to fall back to xzalloc() anyway when !HAS_VMAP,
I suppose.

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

Is this design decision written down anywhere? It looks pretty limiting
to me ...

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

Which suggests that the movement _may_ be needed, but doing it here is
likely premature.

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

Perhaps, but that doesn't mean the #ifdef-ary here is the only
solution. I'm afraid the approach so far doesn't really make clear
in what overall direction you want to move. Possibly it may help
if you split the patch ...

Jan