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....@citrix.com> Thanks, Roger.
