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

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> On Feb 7, 2022, at 10:11 AM, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 05.02.2022 22:29, George Dunlap wrote:
>>> On Jul 5, 2021, at 5:09 PM, Jan Beulich <JBeulich@xxxxxxxx> 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