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