Re: [PATCH 2/5] x86/p2m: collapse the two ->write_p2m_entry() hooks

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

On Tue, Nov 10, 2020 at 02:51:11PM +0100, Jan Beulich wrote:
> On 10.11.2020 12:06, Roger Pau Monné wrote:
> > On Wed, Oct 28, 2020 at 10:22:58AM +0100, Jan Beulich wrote:
> >> @@ -1132,7 +1132,13 @@ void p2m_pt_init(struct p2m_domain *p2m)
> >>      p2m->recalc = do_recalc;
> >>      p2m->change_entry_type_global = p2m_pt_change_entry_type_global;
> >>      p2m->change_entry_type_range = p2m_pt_change_entry_type_range;
> >> -    p2m->write_p2m_entry = write_p2m_entry;
> >> +
> >> +    /* Still too early to use paging_mode_hap(). */
> >> +    if ( hap_enabled(p2m->domain) )
> >> +        hap_p2m_init(p2m);
> >> +    else if ( IS_ENABLED(CONFIG_SHADOW_PAGING) )
> >> +        shadow_p2m_init(p2m);
> > 
> > There's already some logic in p2m_initialise that checks for
> > hap_enabled for EPT specific initialization. Do you think you could
> > move this there so that it's more contained?
> > 
> > I think having the initialization condition sprinkled all over the
> > different functions makes the logic more complicated to follow.
> > 
> > Also, should hap_p2m_init be limited to HAP and PT, as opposed to HAP
> > and EPT which doesn't use the helper AFAICT?
> 
> It is limited to HAP and PT - we're in p2m_pt_init() here. This is
> also why I don't want to move it to e.g. p2m_initialise(), as that
> would be the wrong layer.

All those sub-initializations make the code slightly harder to follow,
but I guess it's fine if we want to keep it layered in this way.

> > Maybe it would be clearer to unify shadow_write_p2m_entry with
> > hap_write_p2m_entry and call it p2m_pt_write_p2m_entry to match the
> > rest of the p2m PT helpers?
> 
> This looks to go along the lines of what I'd put up as a post-
> commit-message remark in "x86/p2m: collapse the two
> ->write_p2m_entry() hooks". The nested handler is perhaps the
> bigger problem with such merging, plus it would feel a little like
> a layering violation (which is why I did put up the question
> instead of doing it right away).

Right, we could look into it on further patches:

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.