On 05.02.2022 22:29, George Dunlap wrote:
>> On Jul 5, 2021, at 5:09 PM, Jan Beulich <jbeul...@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.dun...@citrix.com>
Thanks, but as per above I'll wait with making use of this.
Jan
