There's no need to initialize respective data for PV domains. Note that
p2m_teardown_{alt,nested}p2m() will handle the lack-of-initialization
case fine.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -102,6 +102,9 @@ static int p2m_initialise(struct domain
     p2m->default_access = p2m_access_rwx;
     p2m->p2m_class = p2m_host;
 
+    if ( !is_hvm_domain(d) )
+        return 0;
+
     p2m_pod_init(p2m);
     p2m_nestedp2m_init(p2m);
 
@@ -259,7 +262,7 @@ int p2m_init(struct domain *d)
     int rc;
 
     rc = p2m_init_hostp2m(d);
-    if ( rc )
+    if ( rc || !is_hvm_domain(d) )
         return rc;
 
 #ifdef CONFIG_HVM
--- a/xen/arch/x86/mm/p2m.h
+++ b/xen/arch/x86/mm/p2m.h
@@ -17,6 +17,8 @@
 
 #include <xen/mem_access.h>
 
+void p2m_pod_init(struct p2m_domain *p2m);
+
 int p2m_add_identity_entry(struct domain *d, unsigned long gfn,
                            p2m_access_t p2ma, unsigned int flag);
 int p2m_remove_identity_entry(struct domain *d, unsigned long gfn);
--- 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);
 
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -679,8 +679,6 @@ static inline long p2m_pod_entry_count(c
     return p2m->pod.entry_count;
 }
 
-void p2m_pod_init(struct p2m_domain *p2m);
-
 #else
 
 static inline bool
@@ -709,8 +707,6 @@ static inline long p2m_pod_entry_count(c
     return 0;
 }
 
-static inline void p2m_pod_init(struct p2m_domain *p2m) {}
-
 #endif
 
 

> On Jul 5, 2021, at 5:09 PM, Jan Beulich <JBeulich@suse.com> wrote:
> 
> There's no need to initialize respective data for PV domains. Note that
> p2m_teardown_{alt,nested}p2m() will handle the lack-of-initialization
> case fine.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -102,6 +102,9 @@ static int p2m_initialise(struct domain
>     p2m->default_access = p2m_access_rwx;
>     p2m->p2m_class = p2m_host;
> 
> +    if ( !is_hvm_domain(d) )
> +        return 0;
> +
>     p2m_pod_init(p2m);
>     p2m_nestedp2m_init(p2m);


> 
> @@ -259,7 +262,7 @@ int p2m_init(struct domain *d)
>     int rc;
> 
>     rc = p2m_init_hostp2m(d);
> -    if ( rc )
> +    if ( rc || !is_hvm_domain(d) )
>         return rc;
> 
> #ifdef CONFIG_HVM
> --- a/xen/arch/x86/mm/p2m.h
> +++ b/xen/arch/x86/mm/p2m.h
> @@ -17,6 +17,8 @@
> 
> #include <xen/mem_access.h>
> 
> +void p2m_pod_init(struct p2m_domain *p2m);
> +
> int p2m_add_identity_entry(struct domain *d, unsigned long gfn,
>                            p2m_access_t p2ma, unsigned int flag);
> int p2m_remove_identity_entry(struct domain *d, unsigned long gfn);
> --- 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?

If you really mean to check for HVM here but are just using this as a shortcut, it needs a comment.

With that addressed:

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

 -George

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. However, looking at the
call sites again, I no longer see why I did put in
ASSERT_UNREACHABLE() here. IOW ...

> If you really mean to check for HVM here but are just using this as a shortcut, it needs a comment.

... it's not just a shortcut, yet it feels as if even then you'd
want a comment attached. I'm not really sure though what such a
comment might say which goes beyond what the use p2m_is_hostp2m()
already communicates.

> With that addressed:
> 
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>

Thanks, but as per above I'll wait with making use of this.

Jan

> 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.

 -George

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