Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Julien/Jan,

On 19/08/2024 10:58, Julien Grall wrote:
> On 19/08/2024 10:55, Julien Grall wrote:
> > Hi Ayan,
> >
> > On 19/08/2024 10:45, Ayan Kumar Halder wrote:
> > > I am ok with this. This has the benefit that the change can be 
> > > contained within arch/arm if we do the following :-
> > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > > index cb2c0a16b8..26f7406278 100644
> > > --- a/xen/arch/arm/setup.c
> > > +++ b/xen/arch/arm/setup.c
> > > @@ -329,7 +329,9 @@ void asmlinkage __init start_xen(unsigned long 
> > > boot_phys_offset,
> > >       setup_mm();
> > >
> > > +#ifdef CONFIG_MMU
> > >       vm_init();
> > > +#endif
> > >
> > >       /* Parse the ACPI tables for possible boot-time configuration */
> > >       acpi_boot_table_init();
> > >
> > > Are we ok with this ?
> > >
> > > The definition of vm_init() is in xen/include/xen/vmap.h. If I 
> > > enclose it using any CONFIG_XXX (like I have done in the current 
> > > patch), then I need to introduce it in common/Kconfig and define it 
> > > for x86 and PPC. I would prefer to contain the change within 
> > > arch/arm only if possible.
> > Just to clarify, are you suggesting to just protect the call 
> > vm_init(). In other word, common/vmap.c would still be included in 
> > the final binary for the MPU?
> > If yes, then I think it would be a bit odd... Someone could still 
> > call vmap() and this would not break until runtime.
> > So I don't see how we could get away from modifying the common code.
> Readying my previous reply again. I think the confusion comes from:
>
> > But maybe ARCH_VMAP was an incorrect suggestion. It might be better 
> to gate with the !MMU (IIRC this would imply MPU).
> This was specifically referring to the branch predictor Kconfig. This 
> was not a suggestion to avoid introducing ARCH_VMAP.
Thanks for clarifying this. Yes, I misunderstood your previous comment. 
So I will do :-
1. HARDEN_BRANCH_PREDICTOR will depend on MMU.

2. ARCH_VMAP will be selected by PPC and RISCV. The reason is below.

3. xen/common/vmap.c will be conditionally compiled on ARCH_VMAP and the 
"#ifdef VMAP_VIRT_START .. endif" will be from removed within the file. 
As VMAP_VIRT_START is defined by RISCV and PPC, thus #2 is needed.
Julien, Jan :- Please let me know if you are ok with #3. This was in 
response to Michal's comment. While his suggestion makes sense, I am not 
sure if extending the changes to other architectures is the correct 
approach. Or do you prefer keeping xen/common/vmap.c unchanged.
Kind regards,

Ayan


> Cheers,