Re: [PATCH 08/16] x86/P2M: PoD, altp2m, and nested-p2m are HVM-only

[Jan Beulich <jbeulich@suse.com>]

On 05.02.2022 22:29, George Dunlap wrote:
>> On Jul 5, 2021, at 5:09 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> --- a/xen/arch/x86/mm/p2m-pod.c
>> +++ b/xen/arch/x86/mm/p2m-pod.c
>> @@ -1135,6 +1135,12 @@ p2m_pod_demand_populate(struct p2m_domai
>>     mfn_t mfn;
>>     unsigned long i;
>>
>> +    if ( !p2m_is_hostp2m(p2m) )
>> +    {
>> +        ASSERT_UNREACHABLE();
>> +        return false;
>> +    }
>> +
>>     ASSERT(gfn_locked_by_me(p2m, gfn));
>>     pod_lock(p2m);
> 
> Why this check rather than something which explicitly says HVM?

Checking for just HVM is too lax here imo. PoD operations should
never be invoked for alternative or nested p2ms; see the various
uses of p2m_get_hostp2m() in p2m-pod.c. However, looking at the
call sites again, I no longer see why I did put in
ASSERT_UNREACHABLE() here. IOW ...

> If you really mean to check for HVM here but are just using this as a shortcut, it needs a comment.

... it's not just a shortcut, yet it feels as if even then you'd
want a comment attached. I'm not really sure though what such a
comment might say which goes beyond what the use p2m_is_hostp2m()
already communicates.

> With that addressed:
> 
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>

Thanks, but as per above I'll wait with making use of this.

Jan