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

From: Jan Beulich <jbeulich@suse.com>
To: George Dunlap <George.Dunlap@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Andrew Cooper <Andrew.Cooper3@citrix.com>, Wei Liu <wl@xen.org>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH 08/16] x86/P2M: PoD, altp2m, and nested-p2m are HVM-only
Date: Mon, 7 Feb 2022 16:23:26 +0100	[thread overview]
Message-ID: <1425efc7-42e9-bb50-8db7-356fe1db1477@suse.com> (raw)
In-Reply-To: <E8945018-F8D5-4930-8E5B-ACBCB039BC2F@citrix.com>

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