> On Feb 7, 2022, at 10:11 AM, Jan Beulich <[email protected]> wrote: > > 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.
The fact remains that it doesn’t match what the patch descriptions says, and you’re making me, the reviewer, guess why you changed it — along with anyone else coming back to try to figure out why the code was this way. If you want me to approve of the decision to make the check more strict than simply HVM, then you need to make it clear why you’re doing it. Adding a sentence in the commit message should be fine. -George
