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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.02.2022 15:45, George Dunlap wrote:
>> 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.

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