Re: [PATCH 10/22] x86/mapcache: initialise the mapcache for the idle domain

[Elias El Yandouzi <eliasely@xxxxxxxxxx>]

Hi,

On 22/12/2022 13:06, Jan Beulich wrote:
> On 16.12.2022 12:48, Julien Grall wrote:
> > From: Hongyan Xia <hongyxia@xxxxxxxxxx>
> >
> > In order to use the mapcache in the idle domain, we also have to
> > populate its page tables in the PERDOMAIN region, and we need to move
> > mapcache_domain_init() earlier in arch_domain_create().
> >
> > Note, commit 'x86: lift mapcache variable to the arch level' has
> > initialised the mapcache for HVM domains. With this patch, PV, HVM,
> > idle domains now all initialise the mapcache.
> But they can't use it yet, can they? This needs saying explicitly, or
> else one is going to make wrong implications.
Yes, I tried to use the mapcache right after the idle vCPU gets 
scheduled and it worked. So, I believe it is enough.
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -732,6 +732,8 @@ int arch_domain_create(struct domain *d,
> >   
> >       spin_lock_init(&d->arch.e820_lock);
> >   
> > +    mapcache_domain_init(d);
> > +
> >       /* Minimal initialisation for the idle domain. */
> >       if ( unlikely(is_idle_domain(d)) )
> >       {
> > @@ -829,8 +831,6 @@ int arch_domain_create(struct domain *d,
> >   
> >       psr_domain_init(d);
> >   
> > -    mapcache_domain_init(d);
> You move this ahead of error paths taking the "goto out" route, so
> adjustments to affected error paths are going to be needed to avoid
> memory leaks.
Correct, I'll fix that.

> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -5963,6 +5963,9 @@ int create_perdomain_mapping(struct domain *d, unsigned 
> > long va,
> >           l3tab = __map_domain_page(pg);
> >           clear_page(l3tab);
> >           d->arch.perdomain_l3_pg = pg;
> > +        if ( is_idle_domain(d) )
> > +            idle_pg_table[l4_table_offset(PERDOMAIN_VIRT_START)] =
> > +                l4e_from_page(pg, __PAGE_HYPERVISOR_RW);
> Hmm, having an idle domain check here isn't very nice. I agree putting
> it in arch_domain_create()'s respective conditional isn't very neat
> either, but personally I'd consider this at least a little less bad.
> And the layering violation aspect isn't much worse than that of setting
> d->arch.ctxt_switch there as well.
Why do you think it would be less bad to move it in 
arch_domain_create()? To me, it would make things worse as it would 
spread the mapping stuff across different functions.
--
Elias