Re: [PATCH 08/22] x86/mm: avoid passing a domain parameter to L4 init function

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jul 29, 2024 at 02:36:39PM +0100, Alejandro Vallejo wrote:
> On Fri Jul 26, 2024 at 4:21 PM BST, Roger Pau Monne wrote:
> > In preparation for the function being called from contexts where no domain 
> > is
> > present.
> >
> > No functional change intended.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> >  xen/arch/x86/include/asm/mm.h  |  4 +++-
> >  xen/arch/x86/mm.c              | 24 +++++++++++++-----------
> >  xen/arch/x86/mm/hap/hap.c      |  3 ++-
> >  xen/arch/x86/mm/shadow/hvm.c   |  3 ++-
> >  xen/arch/x86/mm/shadow/multi.c |  7 +++++--
> >  xen/arch/x86/pv/dom0_build.c   |  3 ++-
> >  xen/arch/x86/pv/domain.c       |  3 ++-
> >  7 files changed, 29 insertions(+), 18 deletions(-)
> >
> > diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
> > index b3853ae734fa..076e7009dc99 100644
> > --- a/xen/arch/x86/include/asm/mm.h
> > +++ b/xen/arch/x86/include/asm/mm.h
> > @@ -375,7 +375,9 @@ int devalidate_page(struct page_info *page, unsigned 
> > long type,
> >  
> >  void init_xen_pae_l2_slots(l2_pgentry_t *l2t, const struct domain *d);
> >  void init_xen_l4_slots(l4_pgentry_t *l4t, mfn_t l4mfn,
> > -                       const struct domain *d, mfn_t sl4mfn, bool ro_mpt);
> > +                       mfn_t sl4mfn, const struct page_info *perdomain_l3,
> > +                       bool ro_mpt, bool maybe_compat, bool 
> > short_directmap);
> > +
> 
> The comment currently in the .c file should probably be here instead, and
> updated for the new arguments. That said, I'm skeptical 3 booleans is 
> something
> desirable. It induces a lot of complexity at the call sites (which of the 8
> forms of init_xen_l4_slots() do I need here?) and a lot of cognitive overload.
> 
> I can't propose a solution because I'm still wrapping my head around how the
> layout (esp. compat layout) fits together. Maybe the booleans can be mapped to
> an enum? It would also help interpret the callsites as it'd no longer be a
> sequence of contextless booleans, but a readable identifier.

We have the following possible combinations:

          RO MPT  COMPAT XLAT  SHORT DMAP
PV64        ?         N             Y
PV32        N         Y             Y
HVM         N         Y             N


So we would need:

enum l4_domain_type {
    PV64,
    PV64_RO_MPT,
    PV32,
    HVM
};

I can see about replacing those last 3 booleans with the proposed
enum.

Thanks, Roger.