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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> 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).

Jan