On 07.02.2022 15:45, George Dunlap wrote: >> 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.
I've added a paragraph, but already after your first reply I was asking myself whether I actually need that change here. It's more of the "just to be on the safe side" nature, I think. But it's been quite a while since I put this change together, so I may also have forgotten about some subtle aspect. Jan
