Initialize and bring down altp2m only when it is supported by the platform,
e.g. VMX. Also guard p2m_altp2m_propagate_change().
The puspose of that is the possiblity to disable altp2m support and exclude its
code from the build completely, when it's not supported by the target platform.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
CC: Tamas K Lengyel <tamas@tklengyel.com>
---
 xen/arch/x86/mm/p2m-basic.c | 19 +++++++++++--------
 xen/arch/x86/mm/p2m-ept.c   |  2 +-
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-basic.c b/xen/arch/x86/mm/p2m-basic.c
index 8599bd15c6..90106997d7 100644
--- a/xen/arch/x86/mm/p2m-basic.c
+++ b/xen/arch/x86/mm/p2m-basic.c
@@ -126,13 +126,15 @@ int p2m_init(struct domain *d)
         return rc;
     }
 
-    rc = p2m_init_altp2m(d);
-    if ( rc )
+    if ( hvm_altp2m_supported() )
     {
-        p2m_teardown_hostp2m(d);
-        p2m_teardown_nestedp2m(d);
+        rc = p2m_init_altp2m(d);
+        if ( rc )
+        {
+            p2m_teardown_hostp2m(d);
+            p2m_teardown_nestedp2m(d);
+        }
     }
-
     return rc;
 }
 
@@ -195,11 +197,12 @@ void p2m_final_teardown(struct domain *d)
 {
     if ( is_hvm_domain(d) )
     {
+        if ( hvm_altp2m_supported() )
+            p2m_teardown_altp2m(d);
         /*
-         * We must tear down both of them unconditionally because
-         * we initialise them unconditionally.
+         * We must tear down nestedp2m unconditionally because
+         * we initialise it unconditionally.
          */
-        p2m_teardown_altp2m(d);
         p2m_teardown_nestedp2m(d);
     }
 
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index f83610cb8c..d264df5b14 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -986,7 +986,7 @@ out:
     if ( is_epte_present(&old_entry) )
         ept_free_entry(p2m, &old_entry, target);
 
-    if ( entry_written && p2m_is_hostp2m(p2m) )
+    if ( entry_written && p2m_is_hostp2m(p2m) && hvm_altp2m_supported())
     {
         ret = p2m_altp2m_propagate_change(d, _gfn(gfn), mfn, order, p2mt, p2ma);
         if ( !rc )
-- 
2.25.1

On 15.05.2024 11:03, Sergiy Kibrik wrote:
> --- a/xen/arch/x86/mm/p2m-basic.c
> +++ b/xen/arch/x86/mm/p2m-basic.c
> @@ -126,13 +126,15 @@ int p2m_init(struct domain *d)
>          return rc;
>      }
>  
> -    rc = p2m_init_altp2m(d);
> -    if ( rc )
> +    if ( hvm_altp2m_supported() )
>      {
> -        p2m_teardown_hostp2m(d);
> -        p2m_teardown_nestedp2m(d);
> +        rc = p2m_init_altp2m(d);
> +        if ( rc )
> +        {
> +            p2m_teardown_hostp2m(d);
> +            p2m_teardown_nestedp2m(d);
> +        }

With less code churn and less indentation:

    rc = hvm_altp2m_supported() ? p2m_init_altp2m(d) : 0;
    if ( rc )
    {
        p2m_teardown_hostp2m(d);
        p2m_teardown_nestedp2m(d);
    }

>      }
> -
>      return rc;
>  }

Please don't remove the blank line ahead of the main return of a function.

> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -986,7 +986,7 @@ out:
>      if ( is_epte_present(&old_entry) )
>          ept_free_entry(p2m, &old_entry, target);
>  
> -    if ( entry_written && p2m_is_hostp2m(p2m) )
> +    if ( entry_written && p2m_is_hostp2m(p2m) && hvm_altp2m_supported())

I agree with Stefano's ordering comment here, btw.

Jan

On Wed, 15 May 2024, Sergiy Kibrik wrote:
> Initialize and bring down altp2m only when it is supported by the platform,
> e.g. VMX. Also guard p2m_altp2m_propagate_change().
> The puspose of that is the possiblity to disable altp2m support and exclude its
> code from the build completely, when it's not supported by the target platform.
> 
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> CC: Tamas K Lengyel <tamas@tklengyel.com>
> ---
>  xen/arch/x86/mm/p2m-basic.c | 19 +++++++++++--------
>  xen/arch/x86/mm/p2m-ept.c   |  2 +-
>  2 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m-basic.c b/xen/arch/x86/mm/p2m-basic.c
> index 8599bd15c6..90106997d7 100644
> --- a/xen/arch/x86/mm/p2m-basic.c
> +++ b/xen/arch/x86/mm/p2m-basic.c
> @@ -126,13 +126,15 @@ int p2m_init(struct domain *d)
>          return rc;
>      }
>  
> -    rc = p2m_init_altp2m(d);
> -    if ( rc )
> +    if ( hvm_altp2m_supported() )
>      {
> -        p2m_teardown_hostp2m(d);
> -        p2m_teardown_nestedp2m(d);
> +        rc = p2m_init_altp2m(d);
> +        if ( rc )
> +        {
> +            p2m_teardown_hostp2m(d);
> +            p2m_teardown_nestedp2m(d);
> +        }
>      }
> -
>      return rc;
>  }
>  
> @@ -195,11 +197,12 @@ void p2m_final_teardown(struct domain *d)
>  {
>      if ( is_hvm_domain(d) )
>      {
> +        if ( hvm_altp2m_supported() )
> +            p2m_teardown_altp2m(d);
>          /*
> -         * We must tear down both of them unconditionally because
> -         * we initialise them unconditionally.
> +         * We must tear down nestedp2m unconditionally because
> +         * we initialise it unconditionally.
>           */
> -        p2m_teardown_altp2m(d);
>          p2m_teardown_nestedp2m(d);
>      }
>  
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index f83610cb8c..d264df5b14 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -986,7 +986,7 @@ out:
>      if ( is_epte_present(&old_entry) )
>          ept_free_entry(p2m, &old_entry, target);
>  
> -    if ( entry_written && p2m_is_hostp2m(p2m) )
> +    if ( entry_written && p2m_is_hostp2m(p2m) && hvm_altp2m_supported())
>      {
>          ret = p2m_altp2m_propagate_change(d, _gfn(gfn), mfn, order, p2mt, p2ma);
>          if ( !rc )

This is a matter of taste but I would put hvm_altp2m_supported() first.

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>