[PATCH 0/6] x86/p2m: restrict more code to build just for HVM

[PATCH 0/6] x86/p2m: restrict more code to build just for HVM

[Jan Beulich]

1: p2m: tidy p2m_add_foreign() a little
2: mm: p2m_add_foreign() is HVM-only
3: p2m: set_{foreign,mmio}_p2m_entry() are HVM-only
4: p2m: {,un}map_mmio_regions() are HVM-only
5: mm: the gva_to_gfn() hook is HVM-only
6: p2m: set_shared_p2m_entry() is MEM_SHARING-only

Jan

[PATCH 1/6] x86/p2m: tidy p2m_add_foreign() a little

[Jan Beulich]

Drop a bogus ASSERT() - we don't typically assert incoming domain
pointers to be non-NULL, and there's no particular reason to do so here.

Replace the open-coded DOMID_SELF check by use of
rcu_lock_remote_domain_by_id(), at the same time covering the request
being made with the current domain's actual ID.

Move the "both domains same" check into just the path where it really
is meaningful.

Swap the order of the two puts, such that
- the p2m lock isn't needlessly held across put_page(),
- a separate put_page() on an error path can be avoided,
- they're inverse to the order of the respective gets.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The DOMID_SELF check being converted also suggests to me that there's an
implication of tdom == current->domain, which would in turn appear to
mean the "both domains same" check could as well be dropped altogether.

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2560,9 +2560,6 @@ int p2m_add_foreign(struct domain *tdom,
     int rc;
     struct domain *fdom;
 
-    ASSERT(tdom);
-    if ( foreigndom == DOMID_SELF )
-        return -EINVAL;
     /*
      * hvm fixme: until support is added to p2m teardown code to cleanup any
      * foreign entries, limit this to hardware domain only.
@@ -2573,13 +2570,15 @@ int p2m_add_foreign(struct domain *tdom,
     if ( foreigndom == DOMID_XEN )
         fdom = rcu_lock_domain(dom_xen);
     else
-        fdom = rcu_lock_domain_by_id(foreigndom);
-    if ( fdom == NULL )
-        return -ESRCH;
+    {
+        rc = rcu_lock_remote_domain_by_id(foreigndom, &fdom);
+        if ( rc )
+            return rc;
 
-    rc = -EINVAL;
-    if ( tdom == fdom )
-        goto out;
+        rc = -EINVAL;
+        if ( tdom == fdom )
+            goto out;
+    }
 
     rc = xsm_map_gmfn_foreign(XSM_TARGET, tdom, fdom);
     if ( rc )
@@ -2593,10 +2592,8 @@ int p2m_add_foreign(struct domain *tdom,
     if ( !page ||
          !p2m_is_ram(p2mt) || p2m_is_shared(p2mt) || p2m_is_hole(p2mt) )
     {
-        if ( page )
-            put_page(page);
         rc = -EINVAL;
-        goto out;
+        goto put_one;
     }
     mfn = page_to_mfn(page);
 
@@ -2625,8 +2622,6 @@ int p2m_add_foreign(struct domain *tdom,
                  gpfn, mfn_x(mfn), fgfn, tdom->domain_id, fdom->domain_id);
 
  put_both:
-    put_page(page);
-
     /*
      * This put_gfn for the above get_gfn for prev_mfn.  We must do this
      * after set_foreign_p2m_entry so another cpu doesn't populate the gpfn
@@ -2634,9 +2629,13 @@ int p2m_add_foreign(struct domain *tdom,
      */
     put_gfn(tdom, gpfn);
 
-out:
+ put_one:
+    put_page(page);
+
+ out:
     if ( fdom )
         rcu_unlock_domain(fdom);
+
     return rc;
 }
 

Re: [PATCH 1/6] x86/p2m: tidy p2m_add_foreign() a little

[Andrew Cooper]

On 15/12/2020 16:25, Jan Beulich wrote:
> Drop a bogus ASSERT() - we don't typically assert incoming domain
> pointers to be non-NULL, and there's no particular reason to do so here.
>
> Replace the open-coded DOMID_SELF check by use of
> rcu_lock_remote_domain_by_id(), at the same time covering the request
> being made with the current domain's actual ID.
>
> Move the "both domains same" check into just the path where it really
> is meaningful.
>
> Swap the order of the two puts, such that
> - the p2m lock isn't needlessly held across put_page(),
> - a separate put_page() on an error path can be avoided,
> - they're inverse to the order of the respective gets.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> The DOMID_SELF check being converted also suggests to me that there's an
> implication of tdom == current->domain, which would in turn appear to
> mean the "both domains same" check could as well be dropped altogether.

I don't see anything conceptually wrong with the toolstack creating a
foreign mapping on behalf of a guest at construction time.  I'd go as
far as to argue that it is an interface shortcoming if this didn't
function correctly.

>
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2560,9 +2560,6 @@ int p2m_add_foreign(struct domain *tdom,
>      int rc;
>      struct domain *fdom;
>  
> -    ASSERT(tdom);
> -    if ( foreigndom == DOMID_SELF )
> -        return -EINVAL;
>      /*
>       * hvm fixme: until support is added to p2m teardown code to cleanup any
>       * foreign entries, limit this to hardware domain only.
> @@ -2573,13 +2570,15 @@ int p2m_add_foreign(struct domain *tdom,
>      if ( foreigndom == DOMID_XEN )
>          fdom = rcu_lock_domain(dom_xen);
>      else
> -        fdom = rcu_lock_domain_by_id(foreigndom);
> -    if ( fdom == NULL )
> -        return -ESRCH;
> +    {
> +        rc = rcu_lock_remote_domain_by_id(foreigndom, &fdom);

It occurs to me that rcu_lock_remote_domain_by_id()'s self error path
ought to be -EINVAL rather than -EPERM.  It's never for permissions
reasons that we restrict to remote domains like this - always for
technical ones.

But that is definitely content for a different patch.

~Andrew

Re: [PATCH 1/6] x86/p2m: tidy p2m_add_foreign() a little

[Jan Beulich]

On 17.12.2020 20:03, Andrew Cooper wrote:
> On 15/12/2020 16:25, Jan Beulich wrote:
>> Drop a bogus ASSERT() - we don't typically assert incoming domain
>> pointers to be non-NULL, and there's no particular reason to do so here.
>>
>> Replace the open-coded DOMID_SELF check by use of
>> rcu_lock_remote_domain_by_id(), at the same time covering the request
>> being made with the current domain's actual ID.
>>
>> Move the "both domains same" check into just the path where it really
>> is meaningful.
>>
>> Swap the order of the two puts, such that
>> - the p2m lock isn't needlessly held across put_page(),
>> - a separate put_page() on an error path can be avoided,
>> - they're inverse to the order of the respective gets.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

>> ---
>> The DOMID_SELF check being converted also suggests to me that there's an
>> implication of tdom == current->domain, which would in turn appear to
>> mean the "both domains same" check could as well be dropped altogether.
> 
> I don't see anything conceptually wrong with the toolstack creating a
> foreign mapping on behalf of a guest at construction time.  I'd go as
> far as to argue that it is an interface shortcoming if this didn't
> function correctly.

Right, I actually didn't get the remark right. It's the DOMID_SELF
check that's suspicious especially when tdom != current->domain,
not the tdom != fdom one.

Jan

[PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Jan Beulich]

This is together with its only caller, xenmem_add_to_physmap_one(). Move
the latter next to p2m_add_foreign(), allowing this one to become static
at the same time.

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

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -118,7 +118,6 @@
 #include <xen/vmap.h>
 #include <xen/xmalloc.h>
 #include <xen/efi.h>
-#include <xen/grant_table.h>
 #include <xen/hypercall.h>
 #include <xen/mm.h>
 #include <asm/paging.h>
@@ -142,10 +141,7 @@
 #include <asm/pci.h>
 #include <asm/guest.h>
 #include <asm/hvm/ioreq.h>
-
-#include <asm/hvm/grant_table.h>
 #include <asm/pv/domain.h>
-#include <asm/pv/grant_table.h>
 #include <asm/pv/mm.h>
 
 #ifdef CONFIG_PV
@@ -4591,114 +4587,6 @@ static int handle_iomem_range(unsigned l
     return err || s > e ? err : _handle_iomem_range(s, e, p);
 }
 
-int xenmem_add_to_physmap_one(
-    struct domain *d,
-    unsigned int space,
-    union add_to_physmap_extra extra,
-    unsigned long idx,
-    gfn_t gpfn)
-{
-    struct page_info *page = NULL;
-    unsigned long gfn = 0 /* gcc ... */, old_gpfn;
-    mfn_t prev_mfn;
-    int rc = 0;
-    mfn_t mfn = INVALID_MFN;
-    p2m_type_t p2mt;
-
-    switch ( space )
-    {
-        case XENMAPSPACE_shared_info:
-            if ( idx == 0 )
-                mfn = virt_to_mfn(d->shared_info);
-            break;
-        case XENMAPSPACE_grant_table:
-            rc = gnttab_map_frame(d, idx, gpfn, &mfn);
-            if ( rc )
-                return rc;
-            break;
-        case XENMAPSPACE_gmfn:
-        {
-            p2m_type_t p2mt;
-
-            gfn = idx;
-            mfn = get_gfn_unshare(d, gfn, &p2mt);
-            /* If the page is still shared, exit early */
-            if ( p2m_is_shared(p2mt) )
-            {
-                put_gfn(d, gfn);
-                return -ENOMEM;
-            }
-            page = get_page_from_mfn(mfn, d);
-            if ( unlikely(!page) )
-                mfn = INVALID_MFN;
-            break;
-        }
-        case XENMAPSPACE_gmfn_foreign:
-            return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid);
-        default:
-            break;
-    }
-
-    if ( mfn_eq(mfn, INVALID_MFN) )
-    {
-        rc = -EINVAL;
-        goto put_both;
-    }
-
-    /* Remove previously mapped page if it was present. */
-    prev_mfn = get_gfn(d, gfn_x(gpfn), &p2mt);
-    if ( mfn_valid(prev_mfn) )
-    {
-        if ( is_special_page(mfn_to_page(prev_mfn)) )
-            /* Special pages are simply unhooked from this phys slot. */
-            rc = guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K);
-        else if ( !mfn_eq(mfn, prev_mfn) )
-            /* Normal domain memory is freed, to avoid leaking memory. */
-            rc = guest_remove_page(d, gfn_x(gpfn));
-    }
-    /* In the XENMAPSPACE_gmfn case we still hold a ref on the old page. */
-    put_gfn(d, gfn_x(gpfn));
-
-    if ( rc )
-        goto put_both;
-
-    /* Unmap from old location, if any. */
-    old_gpfn = get_gpfn_from_mfn(mfn_x(mfn));
-    ASSERT(!SHARED_M2P(old_gpfn));
-    if ( space == XENMAPSPACE_gmfn && old_gpfn != gfn )
-    {
-        rc = -EXDEV;
-        goto put_both;
-    }
-    if ( old_gpfn != INVALID_M2P_ENTRY )
-        rc = guest_physmap_remove_page(d, _gfn(old_gpfn), mfn, PAGE_ORDER_4K);
-
-    /* Map at new location. */
-    if ( !rc )
-        rc = guest_physmap_add_page(d, gpfn, mfn, PAGE_ORDER_4K);
-
- put_both:
-    /*
-     * In the XENMAPSPACE_gmfn case, we took a ref of the gfn at the top.
-     * We also may need to transfer ownership of the page reference to our
-     * caller.
-     */
-    if ( space == XENMAPSPACE_gmfn )
-    {
-        put_gfn(d, gfn);
-        if ( !rc && extra.ppage )
-        {
-            *extra.ppage = page;
-            page = NULL;
-        }
-    }
-
-    if ( page )
-        put_page(page);
-
-    return rc;
-}
-
 int arch_acquire_resource(struct domain *d, unsigned int type,
                           unsigned int id, unsigned long frame,
                           unsigned int nr_frames, xen_pfn_t mfn_list[])
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -27,6 +27,7 @@
 #include <xen/mem_access.h>
 #include <xen/vm_event.h>
 #include <xen/event.h>
+#include <xen/grant_table.h>
 #include <xen/param.h>
 #include <public/vm_event.h>
 #include <asm/domain.h>
@@ -42,6 +43,10 @@
 
 #include "mm-locks.h"
 
+/* Override macro from asm/page.h to make work with mfn_t */
+#undef virt_to_mfn
+#define virt_to_mfn(v) _mfn(__virt_to_mfn(v))
+
 /* Turn on/off host superpage page table support for hap, default on. */
 bool_t __initdata opt_hap_1gb = 1, __initdata opt_hap_2mb = 1;
 boolean_param("hap_1gb", opt_hap_1gb);
@@ -2535,6 +2540,8 @@ out_p2m_audit:
 }
 #endif /* P2M_AUDIT */
 
+#ifdef CONFIG_HVM
+
 /*
  * Add frame from foreign domain to target domain's physmap. Similar to
  * XENMAPSPACE_gmfn but the frame is foreign being mapped into current,
@@ -2551,8 +2558,8 @@ out_p2m_audit:
  *
  * Returns: 0 ==> success
  */
-int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
-                    unsigned long gpfn, domid_t foreigndom)
+static int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
+                           unsigned long gpfn, domid_t foreigndom)
 {
     p2m_type_t p2mt, p2mt_prev;
     mfn_t prev_mfn, mfn;
@@ -2639,7 +2646,114 @@ int p2m_add_foreign(struct domain *tdom,
     return rc;
 }
 
-#ifdef CONFIG_HVM
+int xenmem_add_to_physmap_one(
+    struct domain *d,
+    unsigned int space,
+    union add_to_physmap_extra extra,
+    unsigned long idx,
+    gfn_t gpfn)
+{
+    struct page_info *page = NULL;
+    unsigned long gfn = 0 /* gcc ... */, old_gpfn;
+    mfn_t prev_mfn;
+    int rc = 0;
+    mfn_t mfn = INVALID_MFN;
+    p2m_type_t p2mt;
+
+    switch ( space )
+    {
+        case XENMAPSPACE_shared_info:
+            if ( idx == 0 )
+                mfn = virt_to_mfn(d->shared_info);
+            break;
+        case XENMAPSPACE_grant_table:
+            rc = gnttab_map_frame(d, idx, gpfn, &mfn);
+            if ( rc )
+                return rc;
+            break;
+        case XENMAPSPACE_gmfn:
+        {
+            p2m_type_t p2mt;
+
+            gfn = idx;
+            mfn = get_gfn_unshare(d, gfn, &p2mt);
+            /* If the page is still shared, exit early */
+            if ( p2m_is_shared(p2mt) )
+            {
+                put_gfn(d, gfn);
+                return -ENOMEM;
+            }
+            page = get_page_from_mfn(mfn, d);
+            if ( unlikely(!page) )
+                mfn = INVALID_MFN;
+            break;
+        }
+        case XENMAPSPACE_gmfn_foreign:
+            return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid);
+        default:
+            break;
+    }
+
+    if ( mfn_eq(mfn, INVALID_MFN) )
+    {
+        rc = -EINVAL;
+        goto put_both;
+    }
+
+    /* Remove previously mapped page if it was present. */
+    prev_mfn = get_gfn(d, gfn_x(gpfn), &p2mt);
+    if ( mfn_valid(prev_mfn) )
+    {
+        if ( is_special_page(mfn_to_page(prev_mfn)) )
+            /* Special pages are simply unhooked from this phys slot. */
+            rc = guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K);
+        else if ( !mfn_eq(mfn, prev_mfn) )
+            /* Normal domain memory is freed, to avoid leaking memory. */
+            rc = guest_remove_page(d, gfn_x(gpfn));
+    }
+    /* In the XENMAPSPACE_gmfn case we still hold a ref on the old page. */
+    put_gfn(d, gfn_x(gpfn));
+
+    if ( rc )
+        goto put_both;
+
+    /* Unmap from old location, if any. */
+    old_gpfn = get_gpfn_from_mfn(mfn_x(mfn));
+    ASSERT(!SHARED_M2P(old_gpfn));
+    if ( space == XENMAPSPACE_gmfn && old_gpfn != gfn )
+    {
+        rc = -EXDEV;
+        goto put_both;
+    }
+    if ( old_gpfn != INVALID_M2P_ENTRY )
+        rc = guest_physmap_remove_page(d, _gfn(old_gpfn), mfn, PAGE_ORDER_4K);
+
+    /* Map at new location. */
+    if ( !rc )
+        rc = guest_physmap_add_page(d, gpfn, mfn, PAGE_ORDER_4K);
+
+ put_both:
+    /*
+     * In the XENMAPSPACE_gmfn case, we took a ref of the gfn at the top.
+     * We also may need to transfer ownership of the page reference to our
+     * caller.
+     */
+    if ( space == XENMAPSPACE_gmfn )
+    {
+        put_gfn(d, gfn);
+        if ( !rc && extra.ppage )
+        {
+            *extra.ppage = page;
+            page = NULL;
+        }
+    }
+
+    if ( page )
+        put_page(page);
+
+    return rc;
+}
+
 /*
  * Set/clear the #VE suppress bit for a page.  Only available on VMX.
  */
@@ -2792,7 +2906,8 @@ int p2m_set_altp2m_view_visibility(struc
 
     return rc;
 }
-#endif
+
+#endif /* CONFIG_HVM */
 
 /*
  * Local variables:
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -661,10 +661,6 @@ int set_identity_p2m_entry(struct domain
                            p2m_access_t p2ma, unsigned int flag);
 int clear_identity_p2m_entry(struct domain *d, unsigned long gfn);
 
-/* Add foreign mapping to the guest's p2m table. */
-int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
-                    unsigned long gpfn, domid_t foreign_domid);
-
 /* 
  * Populate-on-demand
  */

Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Andrew Cooper]

On 15/12/2020 16:26, Jan Beulich wrote:
> This is together with its only caller, xenmem_add_to_physmap_one().

I can't parse this sentence.  Perhaps "... as is it's only caller," as a
follow-on from the subject sentence.

>  Move
> the latter next to p2m_add_foreign(), allowing this one to become static
> at the same time.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, although...

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2639,7 +2646,114 @@ int p2m_add_foreign(struct domain *tdom,
>      return rc;
>  }
>  
> -#ifdef CONFIG_HVM
> +int xenmem_add_to_physmap_one(
> +    struct domain *d,
> +    unsigned int space,
> +    union add_to_physmap_extra extra,
> +    unsigned long idx,
> +    gfn_t gpfn)
> +{
> +    struct page_info *page = NULL;
> +    unsigned long gfn = 0 /* gcc ... */, old_gpfn;
> +    mfn_t prev_mfn;
> +    int rc = 0;
> +    mfn_t mfn = INVALID_MFN;
> +    p2m_type_t p2mt;
> +
> +    switch ( space )
> +    {
> +        case XENMAPSPACE_shared_info:
> +            if ( idx == 0 )
> +                mfn = virt_to_mfn(d->shared_info);
> +            break;
> +        case XENMAPSPACE_grant_table:
> +            rc = gnttab_map_frame(d, idx, gpfn, &mfn);
> +            if ( rc )
> +                return rc;
> +            break;
> +        case XENMAPSPACE_gmfn:
> +        {
> +            p2m_type_t p2mt;
> +
> +            gfn = idx;
> +            mfn = get_gfn_unshare(d, gfn, &p2mt);
> +            /* If the page is still shared, exit early */
> +            if ( p2m_is_shared(p2mt) )
> +            {
> +                put_gfn(d, gfn);
> +                return -ENOMEM;
> +            }
> +            page = get_page_from_mfn(mfn, d);
> +            if ( unlikely(!page) )
> +                mfn = INVALID_MFN;
> +            break;
> +        }
> +        case XENMAPSPACE_gmfn_foreign:
> +            return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid);
> +        default:
> +            break;

... seeing as the function is moving wholesale, can we at least correct
the indention, to save yet another large churn in the future?  (If it
were me, I'd go as far as deleting the default case as well.)

~Andrew

Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Jan Beulich]

On 17.12.2020 20:18, Andrew Cooper wrote:
> On 15/12/2020 16:26, Jan Beulich wrote:
>> This is together with its only caller, xenmem_add_to_physmap_one().
> 
> I can't parse this sentence.  Perhaps "... as is it's only caller," as a
> follow-on from the subject sentence.

Yeah, changed along these lines.

>>  Move
>> the latter next to p2m_add_foreign(), allowing this one to become static
>> at the same time.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

> , although...
> 
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2639,7 +2646,114 @@ int p2m_add_foreign(struct domain *tdom,
>>      return rc;
>>  }
>>  
>> -#ifdef CONFIG_HVM
>> +int xenmem_add_to_physmap_one(
>> +    struct domain *d,
>> +    unsigned int space,
>> +    union add_to_physmap_extra extra,
>> +    unsigned long idx,
>> +    gfn_t gpfn)
>> +{
>> +    struct page_info *page = NULL;
>> +    unsigned long gfn = 0 /* gcc ... */, old_gpfn;
>> +    mfn_t prev_mfn;
>> +    int rc = 0;
>> +    mfn_t mfn = INVALID_MFN;
>> +    p2m_type_t p2mt;
>> +
>> +    switch ( space )
>> +    {
>> +        case XENMAPSPACE_shared_info:
>> +            if ( idx == 0 )
>> +                mfn = virt_to_mfn(d->shared_info);
>> +            break;
>> +        case XENMAPSPACE_grant_table:
>> +            rc = gnttab_map_frame(d, idx, gpfn, &mfn);
>> +            if ( rc )
>> +                return rc;
>> +            break;
>> +        case XENMAPSPACE_gmfn:
>> +        {
>> +            p2m_type_t p2mt;
>> +
>> +            gfn = idx;
>> +            mfn = get_gfn_unshare(d, gfn, &p2mt);
>> +            /* If the page is still shared, exit early */
>> +            if ( p2m_is_shared(p2mt) )
>> +            {
>> +                put_gfn(d, gfn);
>> +                return -ENOMEM;
>> +            }
>> +            page = get_page_from_mfn(mfn, d);
>> +            if ( unlikely(!page) )
>> +                mfn = INVALID_MFN;
>> +            break;
>> +        }
>> +        case XENMAPSPACE_gmfn_foreign:
>> +            return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid);
>> +        default:
>> +            break;
> 
> ... seeing as the function is moving wholesale, can we at least correct
> the indention, to save yet another large churn in the future?  (If it
> were me, I'd go as far as deleting the default case as well.)

Oh, indeed. I did look for obvious style issues but didn't spot this
(still quite obvious) one. I've done so and also added blank lines
between the case blocks.

Jan

Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Jan Beulich]

On 17.12.2020 20:18, Andrew Cooper wrote:
> On 15/12/2020 16:26, Jan Beulich wrote:
>> This is together with its only caller, xenmem_add_to_physmap_one().
> 
> I can't parse this sentence.  Perhaps "... as is it's only caller," as a
> follow-on from the subject sentence.
> 
>>  Move
>> the latter next to p2m_add_foreign(), allowing this one to become static
>> at the same time.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

So I had to ask Andrew to revert this (I was already at home when
noticing the breakage), as it turned out to break the shim build.
The problem is that xenmem_add_to_physmap() is non-static and
hence can't be eliminated altogether by the compiler when !HVM.
We could make the function conditionally static
"#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this
looks uglier to me than this extra hunk:

--- unstable.orig/xen/common/memory.c
+++ unstable/xen/common/memory.c
@@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain
     union add_to_physmap_extra extra = {};
     struct page_info *pages[16];
 
-    ASSERT(paging_mode_translate(d));
+    if ( !paging_mode_translate(d) )
+    {
+        ASSERT_UNREACHABLE();
+        return -EACCES;
+    }
 
     if ( xatp->space == XENMAPSPACE_gmfn_foreign )
         extra.foreign_domid = DOMID_INVALID;

Andrew, please let me know whether your ack stands with this (or
said alternative) added, or whether you'd prefer me to re-post.

Jan

Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Andrew Cooper]

On 21/12/2020 08:10, Jan Beulich wrote:
> On 17.12.2020 20:18, Andrew Cooper wrote:
>> On 15/12/2020 16:26, Jan Beulich wrote:
>>> This is together with its only caller, xenmem_add_to_physmap_one().
>> I can't parse this sentence.  Perhaps "... as is it's only caller," as a
>> follow-on from the subject sentence.
>>
>>>  Move
>>> the latter next to p2m_add_foreign(), allowing this one to become static
>>> at the same time.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> So I had to ask Andrew to revert this (I was already at home when
> noticing the breakage), as it turned out to break the shim build.
> The problem is that xenmem_add_to_physmap() is non-static and
> hence can't be eliminated altogether by the compiler when !HVM.
> We could make the function conditionally static
> "#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this
> looks uglier to me than this extra hunk:
>
> --- unstable.orig/xen/common/memory.c
> +++ unstable/xen/common/memory.c
> @@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain
>      union add_to_physmap_extra extra = {};
>      struct page_info *pages[16];
>  
> -    ASSERT(paging_mode_translate(d));
> +    if ( !paging_mode_translate(d) )
> +    {
> +        ASSERT_UNREACHABLE();
> +        return -EACCES;
> +    }
>  
>      if ( xatp->space == XENMAPSPACE_gmfn_foreign )
>          extra.foreign_domid = DOMID_INVALID;
>
> Andrew, please let me know whether your ack stands with this (or
> said alternative) added, or whether you'd prefer me to re-post.

Yeah, this is probably neater than the ifdefary.  My ack stands.

~Andrew

Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Oleksandr Tyshchenko]

[-- Attachment #1: Type: text/plain, Size: 3008 bytes --]

Hello all.

[Sorry for the possible format issues]

On Tue, Dec 22, 2020 at 12:41 PM Andrew Cooper <andrew.cooper3@citrix.com>
wrote:

> On 21/12/2020 08:10, Jan Beulich wrote:
> > On 17.12.2020 20:18, Andrew Cooper wrote:
> >> On 15/12/2020 16:26, Jan Beulich wrote:
> >>> This is together with its only caller, xenmem_add_to_physmap_one().
> >> I can't parse this sentence.  Perhaps "... as is it's only caller," as a
> >> follow-on from the subject sentence.
> >>
> >>>  Move
> >>> the latter next to p2m_add_foreign(), allowing this one to become
> static
> >>> at the same time.
> >>>
> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > So I had to ask Andrew to revert this (I was already at home when
> > noticing the breakage), as it turned out to break the shim build.
> > The problem is that xenmem_add_to_physmap() is non-static and
> > hence can't be eliminated altogether by the compiler when !HVM.
> > We could make the function conditionally static
> > "#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this
> > looks uglier to me than this extra hunk:
> >
> > --- unstable.orig/xen/common/memory.c
> > +++ unstable/xen/common/memory.c
> > @@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain
> >      union add_to_physmap_extra extra = {};
> >      struct page_info *pages[16];
> >
> > -    ASSERT(paging_mode_translate(d));
> > +    if ( !paging_mode_translate(d) )
> > +    {
> > +        ASSERT_UNREACHABLE();
> > +        return -EACCES;
> > +    }
> >
> >      if ( xatp->space == XENMAPSPACE_gmfn_foreign )
> >          extra.foreign_domid = DOMID_INVALID;
> >
> > Andrew, please let me know whether your ack stands with this (or
> > said alternative) added, or whether you'd prefer me to re-post.
>
> Yeah, this is probably neater than the ifdefary.  My ack stands.
>
> ~Andrew
>

I might miss something or did incorrect tests, but ...
... trying to build current staging
(7ba2ab495be54f608cb47440e1497b2795bd301a) for x86 (with # CONFIG_HVM is
not set) I got the following:

/media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:
undefined reference to `xenmem_add_to_physmap_one'
/media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:(.text+0x1e391):
relocation truncated to fit: R_X86_64_PC32 against undefined symbol
`xenmem_add_to_physmap_one'
ld:
/media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/.xen-syms.0:
hidden symbol `xenmem_add_to_physmap_one' isn't defined
ld: final link failed: Bad value

It is worth mentioning that I do not use pvshim_defconfig (I disable HVM
support via menuconfig manually before building).

-- 
Regards,

Oleksandr Tyshchenko

[-- Attachment #2: Type: text/html, Size: 4401 bytes --]

Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Jan Beulich]

On 04.01.2021 17:57, Oleksandr Tyshchenko wrote:
> Hello all.
> 
> [Sorry for the possible format issues]
> 
> On Tue, Dec 22, 2020 at 12:41 PM Andrew Cooper <andrew.cooper3@citrix.com>
> wrote:
> 
>> On 21/12/2020 08:10, Jan Beulich wrote:
>>> On 17.12.2020 20:18, Andrew Cooper wrote:
>>>> On 15/12/2020 16:26, Jan Beulich wrote:
>>>>> This is together with its only caller, xenmem_add_to_physmap_one().
>>>> I can't parse this sentence.  Perhaps "... as is it's only caller," as a
>>>> follow-on from the subject sentence.
>>>>
>>>>>  Move
>>>>> the latter next to p2m_add_foreign(), allowing this one to become
>> static
>>>>> at the same time.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> So I had to ask Andrew to revert this (I was already at home when
>>> noticing the breakage), as it turned out to break the shim build.
>>> The problem is that xenmem_add_to_physmap() is non-static and
>>> hence can't be eliminated altogether by the compiler when !HVM.
>>> We could make the function conditionally static
>>> "#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this
>>> looks uglier to me than this extra hunk:
>>>
>>> --- unstable.orig/xen/common/memory.c
>>> +++ unstable/xen/common/memory.c
>>> @@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain
>>>      union add_to_physmap_extra extra = {};
>>>      struct page_info *pages[16];
>>>
>>> -    ASSERT(paging_mode_translate(d));
>>> +    if ( !paging_mode_translate(d) )
>>> +    {
>>> +        ASSERT_UNREACHABLE();
>>> +        return -EACCES;
>>> +    }
>>>
>>>      if ( xatp->space == XENMAPSPACE_gmfn_foreign )
>>>          extra.foreign_domid = DOMID_INVALID;
>>>
>>> Andrew, please let me know whether your ack stands with this (or
>>> said alternative) added, or whether you'd prefer me to re-post.
>>
>> Yeah, this is probably neater than the ifdefary.  My ack stands.
>>
>> ~Andrew
>>
> 
> I might miss something or did incorrect tests, but ...
> ... trying to build current staging
> (7ba2ab495be54f608cb47440e1497b2795bd301a) for x86 (with # CONFIG_HVM is
> not set) I got the following:
> 
> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:
> undefined reference to `xenmem_add_to_physmap_one'
> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:(.text+0x1e391):
> relocation truncated to fit: R_X86_64_PC32 against undefined symbol
> `xenmem_add_to_physmap_one'
> ld:
> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/.xen-syms.0:
> hidden symbol `xenmem_add_to_physmap_one' isn't defined
> ld: final link failed: Bad value
> 
> It is worth mentioning that I do not use pvshim_defconfig (I disable HVM
> support via menuconfig manually before building).

The specific .config may matter. The specific compiler version may
also matter. Things work fine for me, both for the shim config and
a custom !HVM one, with gcc10.

Jan

Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Oleksandr]

On 05.01.21 10:48, Jan Beulich wrote:

Hi Jan


> On 04.01.2021 17:57, Oleksandr Tyshchenko wrote:
>> Hello all.
>>
>> [Sorry for the possible format issues]
>>
>> On Tue, Dec 22, 2020 at 12:41 PM Andrew Cooper <andrew.cooper3@citrix.com>
>> wrote:
>>
>>> On 21/12/2020 08:10, Jan Beulich wrote:
>>>> On 17.12.2020 20:18, Andrew Cooper wrote:
>>>>> On 15/12/2020 16:26, Jan Beulich wrote:
>>>>>> This is together with its only caller, xenmem_add_to_physmap_one().
>>>>> I can't parse this sentence.  Perhaps "... as is it's only caller," as a
>>>>> follow-on from the subject sentence.
>>>>>
>>>>>>   Move
>>>>>> the latter next to p2m_add_foreign(), allowing this one to become
>>> static
>>>>>> at the same time.
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> So I had to ask Andrew to revert this (I was already at home when
>>>> noticing the breakage), as it turned out to break the shim build.
>>>> The problem is that xenmem_add_to_physmap() is non-static and
>>>> hence can't be eliminated altogether by the compiler when !HVM.
>>>> We could make the function conditionally static
>>>> "#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this
>>>> looks uglier to me than this extra hunk:
>>>>
>>>> --- unstable.orig/xen/common/memory.c
>>>> +++ unstable/xen/common/memory.c
>>>> @@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain
>>>>       union add_to_physmap_extra extra = {};
>>>>       struct page_info *pages[16];
>>>>
>>>> -    ASSERT(paging_mode_translate(d));
>>>> +    if ( !paging_mode_translate(d) )
>>>> +    {
>>>> +        ASSERT_UNREACHABLE();
>>>> +        return -EACCES;
>>>> +    }
>>>>
>>>>       if ( xatp->space == XENMAPSPACE_gmfn_foreign )
>>>>           extra.foreign_domid = DOMID_INVALID;
>>>>
>>>> Andrew, please let me know whether your ack stands with this (or
>>>> said alternative) added, or whether you'd prefer me to re-post.
>>> Yeah, this is probably neater than the ifdefary.  My ack stands.
>>>
>>> ~Andrew
>>>
>> I might miss something or did incorrect tests, but ...
>> ... trying to build current staging
>> (7ba2ab495be54f608cb47440e1497b2795bd301a) for x86 (with # CONFIG_HVM is
>> not set) I got the following:
>>
>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:
>> undefined reference to `xenmem_add_to_physmap_one'
>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:(.text+0x1e391):
>> relocation truncated to fit: R_X86_64_PC32 against undefined symbol
>> `xenmem_add_to_physmap_one'
>> ld:
>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/.xen-syms.0:
>> hidden symbol `xenmem_add_to_physmap_one' isn't defined
>> ld: final link failed: Bad value
>>
>> It is worth mentioning that I do not use pvshim_defconfig (I disable HVM
>> support via menuconfig manually before building).
> The specific .config may matter. The specific compiler version may
> also matter. Things work fine for me, both for the shim config and
> a custom !HVM one, with gcc10.

ok, after updating my a little bit ancient compiler to the latest 
possible (?) on xenial gcc-9 the build issue had gone away. Sorry for 
the noise.



-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Jan Beulich]

On 08.01.2021 17:38, Oleksandr wrote:
> On 05.01.21 10:48, Jan Beulich wrote:
>> On 04.01.2021 17:57, Oleksandr Tyshchenko wrote:
>>> Hello all.
>>>
>>> [Sorry for the possible format issues]
>>>
>>> On Tue, Dec 22, 2020 at 12:41 PM Andrew Cooper <andrew.cooper3@citrix.com>
>>> wrote:
>>>
>>>> On 21/12/2020 08:10, Jan Beulich wrote:
>>>>> On 17.12.2020 20:18, Andrew Cooper wrote:
>>>>>> On 15/12/2020 16:26, Jan Beulich wrote:
>>>>>>> This is together with its only caller, xenmem_add_to_physmap_one().
>>>>>> I can't parse this sentence.  Perhaps "... as is it's only caller," as a
>>>>>> follow-on from the subject sentence.
>>>>>>
>>>>>>>   Move
>>>>>>> the latter next to p2m_add_foreign(), allowing this one to become
>>>> static
>>>>>>> at the same time.
>>>>>>>
>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> So I had to ask Andrew to revert this (I was already at home when
>>>>> noticing the breakage), as it turned out to break the shim build.
>>>>> The problem is that xenmem_add_to_physmap() is non-static and
>>>>> hence can't be eliminated altogether by the compiler when !HVM.
>>>>> We could make the function conditionally static
>>>>> "#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this
>>>>> looks uglier to me than this extra hunk:
>>>>>
>>>>> --- unstable.orig/xen/common/memory.c
>>>>> +++ unstable/xen/common/memory.c
>>>>> @@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain
>>>>>       union add_to_physmap_extra extra = {};
>>>>>       struct page_info *pages[16];
>>>>>
>>>>> -    ASSERT(paging_mode_translate(d));
>>>>> +    if ( !paging_mode_translate(d) )
>>>>> +    {
>>>>> +        ASSERT_UNREACHABLE();
>>>>> +        return -EACCES;
>>>>> +    }
>>>>>
>>>>>       if ( xatp->space == XENMAPSPACE_gmfn_foreign )
>>>>>           extra.foreign_domid = DOMID_INVALID;
>>>>>
>>>>> Andrew, please let me know whether your ack stands with this (or
>>>>> said alternative) added, or whether you'd prefer me to re-post.
>>>> Yeah, this is probably neater than the ifdefary.  My ack stands.
>>>>
>>>> ~Andrew
>>>>
>>> I might miss something or did incorrect tests, but ...
>>> ... trying to build current staging
>>> (7ba2ab495be54f608cb47440e1497b2795bd301a) for x86 (with # CONFIG_HVM is
>>> not set) I got the following:
>>>
>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:
>>> undefined reference to `xenmem_add_to_physmap_one'
>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:(.text+0x1e391):
>>> relocation truncated to fit: R_X86_64_PC32 against undefined symbol
>>> `xenmem_add_to_physmap_one'
>>> ld:
>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/.xen-syms.0:
>>> hidden symbol `xenmem_add_to_physmap_one' isn't defined
>>> ld: final link failed: Bad value
>>>
>>> It is worth mentioning that I do not use pvshim_defconfig (I disable HVM
>>> support via menuconfig manually before building).
>> The specific .config may matter. The specific compiler version may
>> also matter. Things work fine for me, both for the shim config and
>> a custom !HVM one, with gcc10.
> 
> ok, after updating my a little bit ancient compiler to the latest 
> possible (?) on xenial gcc-9 the build issue had gone away. Sorry for 
> the noise.

There's no reason to be sorry - we want Xen to build with a wide
range of compiler versions. It's just that if a build issue is
version dependent, it is often up to the person running into it
to determine how to address the issue (and submit a patch).

Jan

Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Oleksandr]

On 08.01.21 19:01, Jan Beulich wrote:

Hi Jan

> On 08.01.2021 17:38, Oleksandr wrote:
>> On 05.01.21 10:48, Jan Beulich wrote:
>>> On 04.01.2021 17:57, Oleksandr Tyshchenko wrote:
>>>> Hello all.
>>>>
>>>> [Sorry for the possible format issues]
>>>>
>>>> On Tue, Dec 22, 2020 at 12:41 PM Andrew Cooper <andrew.cooper3@citrix.com>
>>>> wrote:
>>>>
>>>>> On 21/12/2020 08:10, Jan Beulich wrote:
>>>>>> On 17.12.2020 20:18, Andrew Cooper wrote:
>>>>>>> On 15/12/2020 16:26, Jan Beulich wrote:
>>>>>>>> This is together with its only caller, xenmem_add_to_physmap_one().
>>>>>>> I can't parse this sentence.  Perhaps "... as is it's only caller," as a
>>>>>>> follow-on from the subject sentence.
>>>>>>>
>>>>>>>>    Move
>>>>>>>> the latter next to p2m_add_foreign(), allowing this one to become
>>>>> static
>>>>>>>> at the same time.
>>>>>>>>
>>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>> So I had to ask Andrew to revert this (I was already at home when
>>>>>> noticing the breakage), as it turned out to break the shim build.
>>>>>> The problem is that xenmem_add_to_physmap() is non-static and
>>>>>> hence can't be eliminated altogether by the compiler when !HVM.
>>>>>> We could make the function conditionally static
>>>>>> "#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this
>>>>>> looks uglier to me than this extra hunk:
>>>>>>
>>>>>> --- unstable.orig/xen/common/memory.c
>>>>>> +++ unstable/xen/common/memory.c
>>>>>> @@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain
>>>>>>        union add_to_physmap_extra extra = {};
>>>>>>        struct page_info *pages[16];
>>>>>>
>>>>>> -    ASSERT(paging_mode_translate(d));
>>>>>> +    if ( !paging_mode_translate(d) )
>>>>>> +    {
>>>>>> +        ASSERT_UNREACHABLE();
>>>>>> +        return -EACCES;
>>>>>> +    }
>>>>>>
>>>>>>        if ( xatp->space == XENMAPSPACE_gmfn_foreign )
>>>>>>            extra.foreign_domid = DOMID_INVALID;
>>>>>>
>>>>>> Andrew, please let me know whether your ack stands with this (or
>>>>>> said alternative) added, or whether you'd prefer me to re-post.
>>>>> Yeah, this is probably neater than the ifdefary.  My ack stands.
>>>>>
>>>>> ~Andrew
>>>>>
>>>> I might miss something or did incorrect tests, but ...
>>>> ... trying to build current staging
>>>> (7ba2ab495be54f608cb47440e1497b2795bd301a) for x86 (with # CONFIG_HVM is
>>>> not set) I got the following:
>>>>
>>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:
>>>> undefined reference to `xenmem_add_to_physmap_one'
>>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:(.text+0x1e391):
>>>> relocation truncated to fit: R_X86_64_PC32 against undefined symbol
>>>> `xenmem_add_to_physmap_one'
>>>> ld:
>>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/.xen-syms.0:
>>>> hidden symbol `xenmem_add_to_physmap_one' isn't defined
>>>> ld: final link failed: Bad value
>>>>
>>>> It is worth mentioning that I do not use pvshim_defconfig (I disable HVM
>>>> support via menuconfig manually before building).
>>> The specific .config may matter. The specific compiler version may
>>> also matter. Things work fine for me, both for the shim config and
>>> a custom !HVM one, with gcc10.
>> ok, after updating my a little bit ancient compiler to the latest
>> possible (?) on xenial gcc-9 the build issue had gone away. Sorry for
>> the noise.
> There's no reason to be sorry - we want Xen to build with a wide
> range of compiler versions. It's just that if a build issue is
> version dependent, it is often up to the person running into it
> to determine how to address the issue (and submit a patch).

ok, the issue was observed with gcc (Ubuntu 5.4.0-6ubuntu1~16.04.12) 
5.4.0 20160609

I think (but not 100% sure), to address the build issue something like 
the stub below could help:


diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 85a8df9..ed35352 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -609,9 +609,19 @@ union add_to_physmap_extra {
      domid_t foreign_domid;
  };

+#ifdef CONFIG_HVM
  int xenmem_add_to_physmap_one(struct domain *d, unsigned int space,
                                union add_to_physmap_extra extra,
                                unsigned long idx, gfn_t gfn);
+#else
+static inline int xenmem_add_to_physmap_one(struct domain *d,
+                                            unsigned int space,
+                                            union add_to_physmap_extra 
extra,
+                                            unsigned long idx, gfn_t gfn)
+{
+    return -EACCES;
+}
+#endif

  int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap 
*xatp,
                            unsigned int start);

-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Jan Beulich]

On 08.01.2021 18:37, Oleksandr wrote:
> 
> On 08.01.21 19:01, Jan Beulich wrote:
> 
> Hi Jan
> 
>> On 08.01.2021 17:38, Oleksandr wrote:
>>> On 05.01.21 10:48, Jan Beulich wrote:
>>>> On 04.01.2021 17:57, Oleksandr Tyshchenko wrote:
>>>>> Hello all.
>>>>>
>>>>> [Sorry for the possible format issues]
>>>>>
>>>>> On Tue, Dec 22, 2020 at 12:41 PM Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> wrote:
>>>>>
>>>>>> On 21/12/2020 08:10, Jan Beulich wrote:
>>>>>>> On 17.12.2020 20:18, Andrew Cooper wrote:
>>>>>>>> On 15/12/2020 16:26, Jan Beulich wrote:
>>>>>>>>> This is together with its only caller, xenmem_add_to_physmap_one().
>>>>>>>> I can't parse this sentence.  Perhaps "... as is it's only caller," as a
>>>>>>>> follow-on from the subject sentence.
>>>>>>>>
>>>>>>>>>    Move
>>>>>>>>> the latter next to p2m_add_foreign(), allowing this one to become
>>>>>> static
>>>>>>>>> at the same time.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>>> So I had to ask Andrew to revert this (I was already at home when
>>>>>>> noticing the breakage), as it turned out to break the shim build.
>>>>>>> The problem is that xenmem_add_to_physmap() is non-static and
>>>>>>> hence can't be eliminated altogether by the compiler when !HVM.
>>>>>>> We could make the function conditionally static
>>>>>>> "#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this
>>>>>>> looks uglier to me than this extra hunk:
>>>>>>>
>>>>>>> --- unstable.orig/xen/common/memory.c
>>>>>>> +++ unstable/xen/common/memory.c
>>>>>>> @@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain
>>>>>>>        union add_to_physmap_extra extra = {};
>>>>>>>        struct page_info *pages[16];
>>>>>>>
>>>>>>> -    ASSERT(paging_mode_translate(d));
>>>>>>> +    if ( !paging_mode_translate(d) )
>>>>>>> +    {
>>>>>>> +        ASSERT_UNREACHABLE();
>>>>>>> +        return -EACCES;
>>>>>>> +    }
>>>>>>>
>>>>>>>        if ( xatp->space == XENMAPSPACE_gmfn_foreign )
>>>>>>>            extra.foreign_domid = DOMID_INVALID;
>>>>>>>
>>>>>>> Andrew, please let me know whether your ack stands with this (or
>>>>>>> said alternative) added, or whether you'd prefer me to re-post.
>>>>>> Yeah, this is probably neater than the ifdefary.  My ack stands.
>>>>>>
>>>>>> ~Andrew
>>>>>>
>>>>> I might miss something or did incorrect tests, but ...
>>>>> ... trying to build current staging
>>>>> (7ba2ab495be54f608cb47440e1497b2795bd301a) for x86 (with # CONFIG_HVM is
>>>>> not set) I got the following:
>>>>>
>>>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:
>>>>> undefined reference to `xenmem_add_to_physmap_one'
>>>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:(.text+0x1e391):
>>>>> relocation truncated to fit: R_X86_64_PC32 against undefined symbol
>>>>> `xenmem_add_to_physmap_one'
>>>>> ld:
>>>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/.xen-syms.0:
>>>>> hidden symbol `xenmem_add_to_physmap_one' isn't defined
>>>>> ld: final link failed: Bad value
>>>>>
>>>>> It is worth mentioning that I do not use pvshim_defconfig (I disable HVM
>>>>> support via menuconfig manually before building).
>>>> The specific .config may matter. The specific compiler version may
>>>> also matter. Things work fine for me, both for the shim config and
>>>> a custom !HVM one, with gcc10.
>>> ok, after updating my a little bit ancient compiler to the latest
>>> possible (?) on xenial gcc-9 the build issue had gone away. Sorry for
>>> the noise.
>> There's no reason to be sorry - we want Xen to build with a wide
>> range of compiler versions. It's just that if a build issue is
>> version dependent, it is often up to the person running into it
>> to determine how to address the issue (and submit a patch).
> 
> ok, the issue was observed with gcc (Ubuntu 5.4.0-6ubuntu1~16.04.12) 
> 5.4.0 20160609
> 
> I think (but not 100% sure), to address the build issue something like 
> the stub below could help:

I'm sure this would help, but personally I'd prefer if we could limit
the number of such stubs and rely on the compiler DCE-ing the calls.
Hence it would be at least desirable (imo necessary) to understand
what prevents this to happen here, with this gcc version.

If you could also provide your exact .config, I could see whether I
can repro here with some of the gcc5 versions I have laying around.

Jan

Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Oleksandr]

[-- Attachment #1: Type: text/plain, Size: 4962 bytes --]


On 11.01.21 09:41, Jan Beulich wrote:

Hi Jan


> On 08.01.2021 18:37, Oleksandr wrote:
>> On 08.01.21 19:01, Jan Beulich wrote:
>>
>> Hi Jan
>>
>>> On 08.01.2021 17:38, Oleksandr wrote:
>>>> On 05.01.21 10:48, Jan Beulich wrote:
>>>>> On 04.01.2021 17:57, Oleksandr Tyshchenko wrote:
>>>>>> Hello all.
>>>>>>
>>>>>> [Sorry for the possible format issues]
>>>>>>
>>>>>> On Tue, Dec 22, 2020 at 12:41 PM Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>> wrote:
>>>>>>
>>>>>>> On 21/12/2020 08:10, Jan Beulich wrote:
>>>>>>>> On 17.12.2020 20:18, Andrew Cooper wrote:
>>>>>>>>> On 15/12/2020 16:26, Jan Beulich wrote:
>>>>>>>>>> This is together with its only caller, xenmem_add_to_physmap_one().
>>>>>>>>> I can't parse this sentence.  Perhaps "... as is it's only caller," as a
>>>>>>>>> follow-on from the subject sentence.
>>>>>>>>>
>>>>>>>>>>     Move
>>>>>>>>>> the latter next to p2m_add_foreign(), allowing this one to become
>>>>>>> static
>>>>>>>>>> at the same time.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>>>> So I had to ask Andrew to revert this (I was already at home when
>>>>>>>> noticing the breakage), as it turned out to break the shim build.
>>>>>>>> The problem is that xenmem_add_to_physmap() is non-static and
>>>>>>>> hence can't be eliminated altogether by the compiler when !HVM.
>>>>>>>> We could make the function conditionally static
>>>>>>>> "#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this
>>>>>>>> looks uglier to me than this extra hunk:
>>>>>>>>
>>>>>>>> --- unstable.orig/xen/common/memory.c
>>>>>>>> +++ unstable/xen/common/memory.c
>>>>>>>> @@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain
>>>>>>>>         union add_to_physmap_extra extra = {};
>>>>>>>>         struct page_info *pages[16];
>>>>>>>>
>>>>>>>> -    ASSERT(paging_mode_translate(d));
>>>>>>>> +    if ( !paging_mode_translate(d) )
>>>>>>>> +    {
>>>>>>>> +        ASSERT_UNREACHABLE();
>>>>>>>> +        return -EACCES;
>>>>>>>> +    }
>>>>>>>>
>>>>>>>>         if ( xatp->space == XENMAPSPACE_gmfn_foreign )
>>>>>>>>             extra.foreign_domid = DOMID_INVALID;
>>>>>>>>
>>>>>>>> Andrew, please let me know whether your ack stands with this (or
>>>>>>>> said alternative) added, or whether you'd prefer me to re-post.
>>>>>>> Yeah, this is probably neater than the ifdefary.  My ack stands.
>>>>>>>
>>>>>>> ~Andrew
>>>>>>>
>>>>>> I might miss something or did incorrect tests, but ...
>>>>>> ... trying to build current staging
>>>>>> (7ba2ab495be54f608cb47440e1497b2795bd301a) for x86 (with # CONFIG_HVM is
>>>>>> not set) I got the following:
>>>>>>
>>>>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:
>>>>>> undefined reference to `xenmem_add_to_physmap_one'
>>>>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:(.text+0x1e391):
>>>>>> relocation truncated to fit: R_X86_64_PC32 against undefined symbol
>>>>>> `xenmem_add_to_physmap_one'
>>>>>> ld:
>>>>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/.xen-syms.0:
>>>>>> hidden symbol `xenmem_add_to_physmap_one' isn't defined
>>>>>> ld: final link failed: Bad value
>>>>>>
>>>>>> It is worth mentioning that I do not use pvshim_defconfig (I disable HVM
>>>>>> support via menuconfig manually before building).
>>>>> The specific .config may matter. The specific compiler version may
>>>>> also matter. Things work fine for me, both for the shim config and
>>>>> a custom !HVM one, with gcc10.
>>>> ok, after updating my a little bit ancient compiler to the latest
>>>> possible (?) on xenial gcc-9 the build issue had gone away. Sorry for
>>>> the noise.
>>> There's no reason to be sorry - we want Xen to build with a wide
>>> range of compiler versions. It's just that if a build issue is
>>> version dependent, it is often up to the person running into it
>>> to determine how to address the issue (and submit a patch).
>> ok, the issue was observed with gcc (Ubuntu 5.4.0-6ubuntu1~16.04.12)
>> 5.4.0 20160609
>>
>> I think (but not 100% sure), to address the build issue something like
>> the stub below could help:
> I'm sure this would help, but personally I'd prefer if we could limit
> the number of such stubs and rely on the compiler DCE-ing the calls.
> Hence it would be at least desirable (imo necessary) to understand
> what prevents this to happen here, with this gcc version.

Sounds reasonable


>
> If you could also provide your exact .config, I could see whether I
> can repro here with some of the gcc5 versions I have laying around.

Please see attached

-- 
Regards,

Oleksandr Tyshchenko


[-- Attachment #2: .config --]
[-- Type: text/plain, Size: 2218 bytes --]

#
# Automatically generated file; DO NOT EDIT.
# Xen/x86 4.15-unstable Configuration
#
CONFIG_CC_IS_GCC=y
CONFIG_GCC_VERSION=50400
CONFIG_CLANG_VERSION=0
CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE=y
CONFIG_X86_64=y
CONFIG_X86=y
CONFIG_ARCH_DEFCONFIG="arch/x86/configs/x86_64_defconfig"
CONFIG_INDIRECT_THUNK=y

#
# Architecture Features
#
CONFIG_NR_CPUS=256
CONFIG_PV=y
CONFIG_PV32=y
CONFIG_PV_LINEAR_PT=y
# CONFIG_HVM is not set
CONFIG_SHADOW_PAGING=y
# CONFIG_BIGMEM is not set
CONFIG_TBOOT=y
CONFIG_XEN_ALIGN_DEFAULT=y
# CONFIG_XEN_ALIGN_2M is not set
# CONFIG_XEN_GUEST is not set
# CONFIG_HYPERV_GUEST is not set
# end of Architecture Features

#
# Common Features
#
CONFIG_COMPAT=y
CONFIG_CORE_PARKING=y
CONFIG_GRANT_TABLE=y
CONFIG_HAS_ALTERNATIVE=y
CONFIG_HAS_EX_TABLE=y
CONFIG_HAS_FAST_MULTIPLY=y
CONFIG_HAS_IOPORTS=y
CONFIG_HAS_KEXEC=y
CONFIG_HAS_MEM_PAGING=y
CONFIG_HAS_PDX=y
CONFIG_HAS_SCHED_GRANULARITY=y
CONFIG_HAS_UBSAN=y
CONFIG_MEM_ACCESS_ALWAYS_ON=y
CONFIG_MEM_ACCESS=y
CONFIG_NEEDS_LIBELF=y

#
# Speculative hardening
#
CONFIG_SPECULATIVE_HARDEN_ARRAY=y
CONFIG_SPECULATIVE_HARDEN_BRANCH=y
# end of Speculative hardening

CONFIG_HYPFS=y
CONFIG_HYPFS_CONFIG=y
CONFIG_KEXEC=y
CONFIG_XENOPROF=y
# CONFIG_XSM is not set
CONFIG_SCHED_CREDIT=y
CONFIG_SCHED_CREDIT2=y
CONFIG_SCHED_RTDS=y
CONFIG_SCHED_ARINC653=y
CONFIG_SCHED_NULL=y
CONFIG_SCHED_DEFAULT="credit2"
CONFIG_CRYPTO=y
CONFIG_LIVEPATCH=y
CONFIG_FAST_SYMBOL_LOOKUP=y
CONFIG_ENFORCE_UNIQUE_SYMBOLS=y
CONFIG_CMDLINE=""
CONFIG_DOM0_MEM=""
CONFIG_TRACEBUFFER=y
# end of Common Features

#
# Device Drivers
#
CONFIG_ACPI=y
CONFIG_ACPI_LEGACY_TABLES_LOOKUP=y
CONFIG_NUMA=y
CONFIG_HAS_NS16550=y
CONFIG_HAS_EHCI=y
CONFIG_HAS_CPUFREQ=y
CONFIG_HAS_PASSTHROUGH=y
CONFIG_HAS_PCI=y
CONFIG_VIDEO=y
CONFIG_VGA=y
# end of Device Drivers

# CONFIG_EXPERT is not set
CONFIG_ARCH_SUPPORTS_INT128=y

#
# Debugging Options
#
CONFIG_DEBUG=y
# CONFIG_CRASH_DEBUG is not set
CONFIG_GDBSX=y
CONFIG_DEBUG_INFO=y
CONFIG_FRAME_POINTER=y
# CONFIG_DEBUG_LOCK_PROFILE is not set
CONFIG_DEBUG_LOCKS=y
# CONFIG_PERF_COUNTERS is not set
CONFIG_VERBOSE_DEBUG=y
CONFIG_SCRUB_DEBUG=y
# CONFIG_UBSAN is not set
# CONFIG_DEBUG_TRACE is not set
CONFIG_XMEM_POOL_POISON=y
# end of Debugging Options

Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Jan Beulich]

On 11.01.2021 09:23, Oleksandr wrote:
> On 11.01.21 09:41, Jan Beulich wrote:
>> If you could also provide your exact .config, I could see whether I
>> can repro here with some of the gcc5 versions I have laying around.
> 
> Please see attached

Builds perfectly fine with 5.4.0 here.

Jan

Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Oleksandr]

On 12.01.21 13:58, Jan Beulich wrote:

Hi Jan.

> On 11.01.2021 09:23, Oleksandr wrote:
>> On 11.01.21 09:41, Jan Beulich wrote:
>>> If you could also provide your exact .config, I could see whether I
>>> can repro here with some of the gcc5 versions I have laying around.
>> Please see attached
> Builds perfectly fine with 5.4.0 here.

Thank you for testing.


I wonder whether I indeed missed something. I have switched to 5.4.0 
again (from 9.3.0) and rechecked, a build issue was still present.
I even downloaded 5.4.0 sources and built them to try to build Xen, and 
got the same effect.  What I noticed is that for non-debug builds the 
build issue wasn't present.
Then I decided to build today's staging 
(414be7b66349e7dca42bc1fd47c2b2f5b2d27432 xen/memory: Fix compat 
XENMEM_acquire_resource for size requests) instead of 9-day's old one when
I had initially reported about that build issue 
(7ba2ab495be54f608cb47440e1497b2795bd301a x86/p2m: Fix 
paging_gva_to_gfn() for nested virt). Today's staging builds perfectly 
fine with 5.4.0.
It seems that commit in the middle 
(994f6478a48a60e3b407c7defc2d36a80f880b04 xsm/dummy: harden against 
speculative abuse) indirectly fixes that weird build issue with 5.4.0...


-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Julien Grall]

Hi Jan & Oleksandr,

On 13/01/2021 15:06, Oleksandr wrote:
> 
> On 12.01.21 13:58, Jan Beulich wrote:
> 
> Hi Jan.
> 
>> On 11.01.2021 09:23, Oleksandr wrote:
>>> On 11.01.21 09:41, Jan Beulich wrote:
>>>> If you could also provide your exact .config, I could see whether I
>>>> can repro here with some of the gcc5 versions I have laying around.
>>> Please see attached
>> Builds perfectly fine with 5.4.0 here.
> 
> Thank you for testing.
> 
> 
> I wonder whether I indeed missed something. I have switched to 5.4.0 
> again (from 9.3.0) and rechecked, a build issue was still present.
> I even downloaded 5.4.0 sources and built them to try to build Xen, and 
> got the same effect.  What I noticed is that for non-debug builds the 
> build issue wasn't present.
> Then I decided to build today's staging 
> (414be7b66349e7dca42bc1fd47c2b2f5b2d27432 xen/memory: Fix compat 
> XENMEM_acquire_resource for size requests) instead of 9-day's old one when
> I had initially reported about that build issue 
> (7ba2ab495be54f608cb47440e1497b2795bd301a x86/p2m: Fix 
> paging_gva_to_gfn() for nested virt). Today's staging builds perfectly 
> fine with 5.4.0.
> It seems that commit in the middle 
> (994f6478a48a60e3b407c7defc2d36a80f880b04 xsm/dummy: harden against 
> speculative abuse) indirectly fixes that weird build issue with 5.4.0...

The gitlab CI reported a similar issue today (see [1]) when building 
with randconfig ([2]). This is happening on Debian sid with GCC 9.3.

Note that the default compiler on sid is GCC 10.2.1. So you will have to 
install the package gcc-9 and then use CC=gcc-9 make <...>.


 From a local repro, I get the following message:

ld: ld: prelink.o: in function `xenmem_add_to_physmap_batch':
/root/xen/xen/common/memory.c:942: undefined reference to 
`xenmem_add_to_physmap_one'
/root/xen/xen/common/memory.c:942:(.text+0x22145): relocation truncated 
to fit: R_X86_64_PLT32 against undefined symbol `xenmem_add_to_physmap_one'
prelink-efi.o: in function `xenmem_add_to_physmap_batch':
/root/xen/xen/common/memory.c:942: undefined reference to 
`xenmem_add_to_physmap_one'
make[2]: *** [Makefile:215: /root/xen/xen/xen.efi] Error 1
make[2]: *** Waiting for unfinished jobs....
ld: /root/xen/xen/.xen-syms.0: hidden symbol `xenmem_add_to_physmap_one' 
isn't defined
ld: final link failed: bad value


This points to the call in xenmem_add_to_physmap_batch(). I have played 
a bit with the .config options. I was able to get it built as soon as I 
disabled CONFIG_COVERAGE=y.

So maybe the optimizer is not clever enough on GCC 9 when building with 
coverage enabled?

With the diff below applied (borrowed from 
xenmem_add_to_physmap_batch()), I can build without tweaking the .config 
[1]:

diff --git a/xen/common/memory.c b/xen/common/memory.c
index ccb4d49fc6..5cfd36a53d 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -903,6 +903,12 @@ static int xenmem_add_to_physmap_batch(struct 
domain *d,
  {
      union add_to_physmap_extra extra = {};

+    if ( !paging_mode_translate(d) )
+    {
+        ASSERT_UNREACHABLE();
+        return -EACCES;
+    }
+
      if ( unlikely(xatpb->size < extent) )
          return -EILSEQ;

Cheers,

[1] https://gitlab.com/xen-project/xen/-/jobs/981624525
[2] https://pastebin.com/vTbQXXV9


https://gitlab.com/xen-project/xen/-/jobs/981624525

> 
> 

-- 
Julien Grall

Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Jan Beulich]

On 23.01.2021 14:22, Julien Grall wrote:
> On 13/01/2021 15:06, Oleksandr wrote:
>> On 12.01.21 13:58, Jan Beulich wrote:
>>> On 11.01.2021 09:23, Oleksandr wrote:
>>>> On 11.01.21 09:41, Jan Beulich wrote:
>>>>> If you could also provide your exact .config, I could see whether I
>>>>> can repro here with some of the gcc5 versions I have laying around.
>>>> Please see attached
>>> Builds perfectly fine with 5.4.0 here.
>>
>> Thank you for testing.
>>
>>
>> I wonder whether I indeed missed something. I have switched to 5.4.0 
>> again (from 9.3.0) and rechecked, a build issue was still present.
>> I even downloaded 5.4.0 sources and built them to try to build Xen, and 
>> got the same effect.  What I noticed is that for non-debug builds the 
>> build issue wasn't present.
>> Then I decided to build today's staging 
>> (414be7b66349e7dca42bc1fd47c2b2f5b2d27432 xen/memory: Fix compat 
>> XENMEM_acquire_resource for size requests) instead of 9-day's old one when
>> I had initially reported about that build issue 
>> (7ba2ab495be54f608cb47440e1497b2795bd301a x86/p2m: Fix 
>> paging_gva_to_gfn() for nested virt). Today's staging builds perfectly 
>> fine with 5.4.0.
>> It seems that commit in the middle 
>> (994f6478a48a60e3b407c7defc2d36a80f880b04 xsm/dummy: harden against 
>> speculative abuse) indirectly fixes that weird build issue with 5.4.0...
> 
> The gitlab CI reported a similar issue today (see [1]) when building 
> with randconfig ([2]). This is happening on Debian sid with GCC 9.3.
> 
> Note that the default compiler on sid is GCC 10.2.1. So you will have to 
> install the package gcc-9 and then use CC=gcc-9 make <...>.
> 
> 
>  From a local repro, I get the following message:
> 
> ld: ld: prelink.o: in function `xenmem_add_to_physmap_batch':
> /root/xen/xen/common/memory.c:942: undefined reference to 
> `xenmem_add_to_physmap_one'
> /root/xen/xen/common/memory.c:942:(.text+0x22145): relocation truncated 
> to fit: R_X86_64_PLT32 against undefined symbol `xenmem_add_to_physmap_one'
> prelink-efi.o: in function `xenmem_add_to_physmap_batch':
> /root/xen/xen/common/memory.c:942: undefined reference to 
> `xenmem_add_to_physmap_one'
> make[2]: *** [Makefile:215: /root/xen/xen/xen.efi] Error 1
> make[2]: *** Waiting for unfinished jobs....
> ld: /root/xen/xen/.xen-syms.0: hidden symbol `xenmem_add_to_physmap_one' 
> isn't defined
> ld: final link failed: bad value
> 
> 
> This points to the call in xenmem_add_to_physmap_batch(). I have played 
> a bit with the .config options. I was able to get it built as soon as I 
> disabled CONFIG_COVERAGE=y.
> 
> So maybe the optimizer is not clever enough on GCC 9 when building with 
> coverage enabled?
> 
> With the diff below applied (borrowed from 
> xenmem_add_to_physmap_batch()), I can build without tweaking the .config 
> [1]:
> 
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index ccb4d49fc6..5cfd36a53d 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -903,6 +903,12 @@ static int xenmem_add_to_physmap_batch(struct 
> domain *d,
>   {
>       union add_to_physmap_extra extra = {};
> 
> +    if ( !paging_mode_translate(d) )
> +    {
> +        ASSERT_UNREACHABLE();
> +        return -EACCES;
> +    }
> +
>       if ( unlikely(xatpb->size < extent) )
>           return -EILSEQ;

Interesting. So despite the function being static and
xatp_permission_check() already doing this check, the function
doesn't get squashed. Looking at my gcov build this might be
due to xatp_permission_check() not getting inlined in this
case, but I will need to try one with HVM=n to be certain. In
any event, I wouldn't mind the above as a workaround to the
issue, as long as its description makes clear this is a
workaround only.

Thanks for investigating!

Jan

Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Jan Beulich]

On 23.01.2021 14:22, Julien Grall wrote:
> On 13/01/2021 15:06, Oleksandr wrote:
>> On 12.01.21 13:58, Jan Beulich wrote:
>>> On 11.01.2021 09:23, Oleksandr wrote:
>>>> On 11.01.21 09:41, Jan Beulich wrote:
>>>>> If you could also provide your exact .config, I could see whether I
>>>>> can repro here with some of the gcc5 versions I have laying around.
>>>> Please see attached
>>> Builds perfectly fine with 5.4.0 here.
>>
>> Thank you for testing.
>>
>>
>> I wonder whether I indeed missed something. I have switched to 5.4.0 
>> again (from 9.3.0) and rechecked, a build issue was still present.
>> I even downloaded 5.4.0 sources and built them to try to build Xen, and 
>> got the same effect.  What I noticed is that for non-debug builds the 
>> build issue wasn't present.
>> Then I decided to build today's staging 
>> (414be7b66349e7dca42bc1fd47c2b2f5b2d27432 xen/memory: Fix compat 
>> XENMEM_acquire_resource for size requests) instead of 9-day's old one when
>> I had initially reported about that build issue 
>> (7ba2ab495be54f608cb47440e1497b2795bd301a x86/p2m: Fix 
>> paging_gva_to_gfn() for nested virt). Today's staging builds perfectly 
>> fine with 5.4.0.
>> It seems that commit in the middle 
>> (994f6478a48a60e3b407c7defc2d36a80f880b04 xsm/dummy: harden against 
>> speculative abuse) indirectly fixes that weird build issue with 5.4.0...
> 
> The gitlab CI reported a similar issue today (see [1]) when building 
> with randconfig ([2]). This is happening on Debian sid with GCC 9.3.
> 
> Note that the default compiler on sid is GCC 10.2.1. So you will have to 
> install the package gcc-9 and then use CC=gcc-9 make <...>.
> 
> 
>  From a local repro, I get the following message:
> 
> ld: ld: prelink.o: in function `xenmem_add_to_physmap_batch':
> /root/xen/xen/common/memory.c:942: undefined reference to 
> `xenmem_add_to_physmap_one'
> /root/xen/xen/common/memory.c:942:(.text+0x22145): relocation truncated 
> to fit: R_X86_64_PLT32 against undefined symbol `xenmem_add_to_physmap_one'
> prelink-efi.o: in function `xenmem_add_to_physmap_batch':
> /root/xen/xen/common/memory.c:942: undefined reference to 
> `xenmem_add_to_physmap_one'
> make[2]: *** [Makefile:215: /root/xen/xen/xen.efi] Error 1
> make[2]: *** Waiting for unfinished jobs....
> ld: /root/xen/xen/.xen-syms.0: hidden symbol `xenmem_add_to_physmap_one' 
> isn't defined
> ld: final link failed: bad value
> 
> 
> This points to the call in xenmem_add_to_physmap_batch(). I have played 
> a bit with the .config options. I was able to get it built as soon as I 
> disabled CONFIG_COVERAGE=y.
> 
> So maybe the optimizer is not clever enough on GCC 9 when building with 
> coverage enabled?

Possibly, albeit I can't repro this locally. Even with gcc9
the code gets collapsed sufficiently. I do notice though
that overall gcc10 does quite a bit better a job, so I could
see further factors potentially leading to what you did
observe, and then possibly independent of the specific gcc9
build in use.

Jan

[PATCH 3/6] x86/p2m: set_{foreign,mmio}_p2m_entry() are HVM-only

[Jan Beulich]

Extend a respective #ifdef from inside set_typed_p2m_entry() to around
all three functions. Add ASSERT_UNREACHABLE() to the latter one's safety
check path.

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

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1257,6 +1257,8 @@ int p2m_finish_type_change(struct domain
     return rc;
 }
 
+#ifdef CONFIG_HVM
+
 /*
  * Returns:
  *    0              for success
@@ -1277,7 +1279,10 @@ static int set_typed_p2m_entry(struct do
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
 
     if ( !paging_mode_translate(d) )
+    {
+        ASSERT_UNREACHABLE();
         return -EIO;
+    }
 
     gfn_lock(p2m, gfn, order);
     omfn = p2m->get_entry(p2m, gfn, &ot, &a, 0, &cur_order, NULL);
@@ -1308,7 +1313,6 @@ static int set_typed_p2m_entry(struct do
     if ( rc )
         gdprintk(XENLOG_ERR, "p2m_set_entry: %#lx:%u -> %d (0x%"PRI_mfn")\n",
                  gfn_l, order, rc, mfn_x(mfn));
-#ifdef CONFIG_HVM
     else if ( p2m_is_pod(ot) )
     {
         pod_lock(p2m);
@@ -1316,7 +1320,6 @@ static int set_typed_p2m_entry(struct do
         BUG_ON(p2m->pod.entry_count < 0);
         pod_unlock(p2m);
     }
-#endif
     gfn_unlock(p2m, gfn, order);
 
     return rc;
@@ -1341,6 +1344,8 @@ int set_mmio_p2m_entry(struct domain *d,
                                p2m_get_hostp2m(d)->default_access);
 }
 
+#endif /* CONFIG_HVM */
+
 int set_identity_p2m_entry(struct domain *d, unsigned long gfn_l,
                            p2m_access_t p2ma, unsigned int flag)
 {

Re: [PATCH 3/6] x86/p2m: set_{foreign,mmio}_p2m_entry() are HVM-only

[Andrew Cooper]

On 15/12/2020 16:26, Jan Beulich wrote:
> Extend a respective #ifdef from inside set_typed_p2m_entry() to around
> all three functions. Add ASSERT_UNREACHABLE() to the latter one's safety
> check path.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

As the code currently stands, yes.  However, I'm not sure I agree
conceptually.

The p2m APIs are either a common interface to use, or HVM-specific.

PV guests don't actually have a p2m, but some of the APIs are used from
common code (e.g. copy_to/from_guest()), and some p2m concepts are
special cased as identity for PV (technically paging_mode_translate()),
while other concepts, such as foreign/mmio, which do exist for both PV
and HVM guests, are handled with totally different API sets for PV and HVM.

This is a broken mess of an abstraction.  I suspect some of it has to do
with PV autotranslate mode in the past, but that doesn't alter the fact
that we have a totally undocumented and error prone set of APIs here.

Either P2M's should (fully) be the common abstraction (despite not being
a real object for PV guests), or they should should be a different set
of APIs which is the common abstraction, and P2Ms should move being
exclusively for HVM guests.

(It's also very obvious by all the CONFIG_X86 ifdefary that we've got
arch specifics in our common code, and that is another aspect of the API
mess which needs handling.)

I'm honestly not sure which of these would be better, but I'm fairly
sure that either would be better than what we've currently got.  I
certainly think it would be better to have a plan for improvement, to
guide patches like this.

~Andrew

Re: [PATCH 3/6] x86/p2m: set_{foreign,mmio}_p2m_entry() are HVM-only

[Jan Beulich]

On 17.12.2020 20:54, Andrew Cooper wrote:
> On 15/12/2020 16:26, Jan Beulich wrote:
>> Extend a respective #ifdef from inside set_typed_p2m_entry() to around
>> all three functions. Add ASSERT_UNREACHABLE() to the latter one's safety
>> check path.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> As the code currently stands, yes.  However, I'm not sure I agree
> conceptually.
> 
> The p2m APIs are either a common interface to use, or HVM-specific.
> 
> PV guests don't actually have a p2m, but some of the APIs are used from
> common code (e.g. copy_to/from_guest()), and some p2m concepts are
> special cased as identity for PV (technically paging_mode_translate()),
> while other concepts, such as foreign/mmio, which do exist for both PV
> and HVM guests, are handled with totally different API sets for PV and HVM.
> 
> This is a broken mess of an abstraction.  I suspect some of it has to do
> with PV autotranslate mode in the past, but that doesn't alter the fact
> that we have a totally undocumented and error prone set of APIs here.
> 
> Either P2M's should (fully) be the common abstraction (despite not being
> a real object for PV guests), or they should should be a different set
> of APIs which is the common abstraction, and P2Ms should move being
> exclusively for HVM guests.
> 
> (It's also very obvious by all the CONFIG_X86 ifdefary that we've got
> arch specifics in our common code, and that is another aspect of the API
> mess which needs handling.)
> 
> I'm honestly not sure which of these would be better, but I'm fairly
> sure that either would be better than what we've currently got.  I
> certainly think it would be better to have a plan for improvement, to
> guide patches like this.

Well, by the end of this series fairly large parts of p2m.c are inside
#ifdef CONFIG_HVM. I would have thought the route is clear - eventually
p2m.c should get built only when HVM is enabled. This change is simply
getting us one tiny step closer.

Otoh, when considering common code, hiding PV specifics inside the p2m
functions may turn out better, as else we may need another layer around
them (like effectively we already have with e.g.
guest_physmap_{add,remove}_page(), which I think would need to move out
of p2m.c if that was to become HVM-only as a whole) ...

Jan

[PATCH 4/6] x86/p2m: {,un}map_mmio_regions() are HVM-only

[Jan Beulich]

Mirror the "translated" check the functions do to do_domctl(), allowing
the calls to be DCEd by the compiler. Add ASSERT_UNREACHABLE() to the
original checks.

Also arrange for {set,clear}_mmio_p2m_entry() and
{set,clear}_identity_p2m_entry() to respectively live next to each
other, such that clear_mmio_p2m_entry() can also be covered by the
#ifdef already covering set_mmio_p2m_entry().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Arguably the original checks, returning success, could also be dropped
at this point.

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1344,52 +1344,6 @@ int set_mmio_p2m_entry(struct domain *d,
                                p2m_get_hostp2m(d)->default_access);
 }
 
-#endif /* CONFIG_HVM */
-
-int set_identity_p2m_entry(struct domain *d, unsigned long gfn_l,
-                           p2m_access_t p2ma, unsigned int flag)
-{
-    p2m_type_t p2mt;
-    p2m_access_t a;
-    gfn_t gfn = _gfn(gfn_l);
-    mfn_t mfn;
-    struct p2m_domain *p2m = p2m_get_hostp2m(d);
-    int ret;
-
-    if ( !paging_mode_translate(p2m->domain) )
-    {
-        if ( !is_iommu_enabled(d) )
-            return 0;
-        return iommu_legacy_map(d, _dfn(gfn_l), _mfn(gfn_l),
-                                1ul << PAGE_ORDER_4K,
-                                IOMMUF_readable | IOMMUF_writable);
-    }
-
-    gfn_lock(p2m, gfn, 0);
-
-    mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL, NULL);
-
-    if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm )
-        ret = p2m_set_entry(p2m, gfn, _mfn(gfn_l), PAGE_ORDER_4K,
-                            p2m_mmio_direct, p2ma);
-    else if ( mfn_x(mfn) == gfn_l && p2mt == p2m_mmio_direct && a == p2ma )
-        ret = 0;
-    else
-    {
-        if ( flag & XEN_DOMCTL_DEV_RDM_RELAXED )
-            ret = 0;
-        else
-            ret = -EBUSY;
-        printk(XENLOG_G_WARNING
-               "Cannot setup identity map d%d:%lx,"
-               " gfn already mapped to %lx.\n",
-               d->domain_id, gfn_l, mfn_x(mfn));
-    }
-
-    gfn_unlock(p2m, gfn, 0);
-    return ret;
-}
-
 /*
  * Returns:
  *    0        for success
@@ -1439,6 +1393,52 @@ int clear_mmio_p2m_entry(struct domain *
     return rc;
 }
 
+#endif /* CONFIG_HVM */
+
+int set_identity_p2m_entry(struct domain *d, unsigned long gfn_l,
+                           p2m_access_t p2ma, unsigned int flag)
+{
+    p2m_type_t p2mt;
+    p2m_access_t a;
+    gfn_t gfn = _gfn(gfn_l);
+    mfn_t mfn;
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    int ret;
+
+    if ( !paging_mode_translate(p2m->domain) )
+    {
+        if ( !is_iommu_enabled(d) )
+            return 0;
+        return iommu_legacy_map(d, _dfn(gfn_l), _mfn(gfn_l),
+                                1ul << PAGE_ORDER_4K,
+                                IOMMUF_readable | IOMMUF_writable);
+    }
+
+    gfn_lock(p2m, gfn, 0);
+
+    mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL, NULL);
+
+    if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm )
+        ret = p2m_set_entry(p2m, gfn, _mfn(gfn_l), PAGE_ORDER_4K,
+                            p2m_mmio_direct, p2ma);
+    else if ( mfn_x(mfn) == gfn_l && p2mt == p2m_mmio_direct && a == p2ma )
+        ret = 0;
+    else
+    {
+        if ( flag & XEN_DOMCTL_DEV_RDM_RELAXED )
+            ret = 0;
+        else
+            ret = -EBUSY;
+        printk(XENLOG_G_WARNING
+               "Cannot setup identity map d%d:%lx,"
+               " gfn already mapped to %lx.\n",
+               d->domain_id, gfn_l, mfn_x(mfn));
+    }
+
+    gfn_unlock(p2m, gfn, 0);
+    return ret;
+}
+
 int clear_identity_p2m_entry(struct domain *d, unsigned long gfn_l)
 {
     p2m_type_t p2mt;
@@ -1868,6 +1868,8 @@ void *map_domain_gfn(struct p2m_domain *
     return map_domain_page(*mfn);
 }
 
+#ifdef CONFIG_HVM
+
 static unsigned int mmio_order(const struct domain *d,
                                unsigned long start_fn, unsigned long nr)
 {
@@ -1908,7 +1910,10 @@ int map_mmio_regions(struct domain *d,
     unsigned int iter, order;
 
     if ( !paging_mode_translate(d) )
+    {
+        ASSERT_UNREACHABLE();
         return 0;
+    }
 
     for ( iter = i = 0; i < nr && iter < MAP_MMIO_MAX_ITER;
           i += 1UL << order, ++iter )
@@ -1940,7 +1945,10 @@ int unmap_mmio_regions(struct domain *d,
     unsigned int iter, order;
 
     if ( !paging_mode_translate(d) )
+    {
+        ASSERT_UNREACHABLE();
         return 0;
+    }
 
     for ( iter = i = 0; i < nr && iter < MAP_MMIO_MAX_ITER;
           i += 1UL << order, ++iter )
@@ -1962,8 +1970,6 @@ int unmap_mmio_regions(struct domain *d,
     return i == nr ? 0 : i ?: ret;
 }
 
-#ifdef CONFIG_HVM
-
 int altp2m_get_effective_entry(struct p2m_domain *ap2m, gfn_t gfn, mfn_t *mfn,
                                p2m_type_t *t, p2m_access_t *a,
                                bool prepopulate)
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -750,6 +750,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
         if ( ret )
             break;
 
+        if ( !paging_mode_translate(d) )
+            break;
+
         if ( add )
         {
             printk(XENLOG_G_DEBUG

[PATCH 5/6] x86/mm: the gva_to_gfn() hook is HVM-only

[Jan Beulich]

As is the adjacent ga_to_gfn() one as well as paging_gva_to_gfn().

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

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1772,7 +1772,6 @@ void np2m_schedule(int dir)
         p2m_unlock(p2m);
     }
 }
-#endif
 
 unsigned long paging_gva_to_gfn(struct vcpu *v,
                                 unsigned long va,
@@ -1820,6 +1819,8 @@ unsigned long paging_gva_to_gfn(struct v
     return hostmode->gva_to_gfn(v, hostp2m, va, pfec);
 }
 
+#endif /* CONFIG_HVM */
+
 /*
  * If the map is non-NULL, we leave this function having acquired an extra ref
  * on mfn_to_page(*mfn).  In all cases, *pfec contains appropriate
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -3414,6 +3414,7 @@ static bool sh_invlpg(struct vcpu *v, un
     return true;
 }
 
+#ifdef CONFIG_HVM
 
 static unsigned long
 sh_gva_to_gfn(struct vcpu *v, struct p2m_domain *p2m,
@@ -3447,6 +3448,7 @@ sh_gva_to_gfn(struct vcpu *v, struct p2m
     return gfn_x(gfn);
 }
 
+#endif /* CONFIG_HVM */
 
 static inline void
 sh_update_linear_entries(struct vcpu *v)
@@ -4571,7 +4573,9 @@ int sh_audit_l4_table(struct vcpu *v, mf
 const struct paging_mode sh_paging_mode = {
     .page_fault                    = sh_page_fault,
     .invlpg                        = sh_invlpg,
+#ifdef CONFIG_HVM
     .gva_to_gfn                    = sh_gva_to_gfn,
+#endif
     .update_cr3                    = sh_update_cr3,
     .update_paging_modes           = shadow_update_paging_modes,
     .flush_tlb                     = shadow_flush_tlb,
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -127,6 +127,7 @@ struct paging_mode {
                                             struct cpu_user_regs *regs);
     bool          (*invlpg                )(struct vcpu *v,
                                             unsigned long linear);
+#ifdef CONFIG_HVM
     unsigned long (*gva_to_gfn            )(struct vcpu *v,
                                             struct p2m_domain *p2m,
                                             unsigned long va,
@@ -136,6 +137,7 @@ struct paging_mode {
                                             unsigned long cr3,
                                             paddr_t ga, uint32_t *pfec,
                                             unsigned int *page_order);
+#endif
     void          (*update_cr3            )(struct vcpu *v, int do_locking,
                                             bool noflush);
     void          (*update_paging_modes   )(struct vcpu *v);
@@ -286,6 +288,8 @@ unsigned long paging_gva_to_gfn(struct v
                                 unsigned long va,
                                 uint32_t *pfec);
 
+#ifdef CONFIG_HVM
+
 /* Translate a guest address using a particular CR3 value.  This is used
  * to by nested HAP code, to walk the guest-supplied NPT tables as if
  * they were pagetables.
@@ -304,6 +308,8 @@ static inline unsigned long paging_ga_to
         page_order);
 }
 
+#endif /* CONFIG_HVM */
+
 /* Update all the things that are derived from the guest's CR3.
  * Called when the guest changes CR3; the caller can then use v->arch.cr3
  * as the value to load into the host CR3 to schedule this vcpu */

Re: [PATCH 5/6] x86/mm: the gva_to_gfn() hook is HVM-only

[Jan Beulich]

On 15.12.2020 17:27, Jan Beulich wrote:
> As is the adjacent ga_to_gfn() one as well as paging_gva_to_gfn().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1772,7 +1772,6 @@ void np2m_schedule(int dir)
>          p2m_unlock(p2m);
>      }
>  }
> -#endif
>  
>  unsigned long paging_gva_to_gfn(struct vcpu *v,
>                                  unsigned long va,
> @@ -1820,6 +1819,8 @@ unsigned long paging_gva_to_gfn(struct v
>      return hostmode->gva_to_gfn(v, hostp2m, va, pfec);
>  }
>  
> +#endif /* CONFIG_HVM */
> +
>  /*
>   * If the map is non-NULL, we leave this function having acquired an extra ref
>   * on mfn_to_page(*mfn).  In all cases, *pfec contains appropriate
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -3414,6 +3414,7 @@ static bool sh_invlpg(struct vcpu *v, un
>      return true;
>  }
>  
> +#ifdef CONFIG_HVM
>  
>  static unsigned long
>  sh_gva_to_gfn(struct vcpu *v, struct p2m_domain *p2m,
> @@ -3447,6 +3448,7 @@ sh_gva_to_gfn(struct vcpu *v, struct p2m
>      return gfn_x(gfn);
>  }
>  
> +#endif /* CONFIG_HVM */
>  
>  static inline void
>  sh_update_linear_entries(struct vcpu *v)
> @@ -4571,7 +4573,9 @@ int sh_audit_l4_table(struct vcpu *v, mf
>  const struct paging_mode sh_paging_mode = {
>      .page_fault                    = sh_page_fault,
>      .invlpg                        = sh_invlpg,
> +#ifdef CONFIG_HVM
>      .gva_to_gfn                    = sh_gva_to_gfn,
> +#endif
>      .update_cr3                    = sh_update_cr3,
>      .update_paging_modes           = shadow_update_paging_modes,
>      .flush_tlb                     = shadow_flush_tlb,

I've noticed (or really the compiler told me) I forgot to also
change none.c:

--- a/xen/arch/x86/mm/shadow/none.c
+++ b/xen/arch/x86/mm/shadow/none.c
@@ -43,12 +43,14 @@ static bool _invlpg(struct vcpu *v, unsi
     return true;
 }
 
+#ifdef CONFIG_HVM
 static unsigned long _gva_to_gfn(struct vcpu *v, struct p2m_domain *p2m,
                                  unsigned long va, uint32_t *pfec)
 {
     ASSERT_UNREACHABLE();
     return gfn_x(INVALID_GFN);
 }
+#endif
 
 static void _update_cr3(struct vcpu *v, int do_locking, bool noflush)
 {
@@ -63,7 +65,9 @@ static void _update_paging_modes(struct
 static const struct paging_mode sh_paging_none = {
     .page_fault                    = _page_fault,
     .invlpg                        = _invlpg,
+#ifdef CONFIG_HVM
     .gva_to_gfn                    = _gva_to_gfn,
+#endif
     .update_cr3                    = _update_cr3,
     .update_paging_modes           = _update_paging_modes,
 };

Jan

> --- a/xen/include/asm-x86/paging.h
> +++ b/xen/include/asm-x86/paging.h
> @@ -127,6 +127,7 @@ struct paging_mode {
>                                              struct cpu_user_regs *regs);
>      bool          (*invlpg                )(struct vcpu *v,
>                                              unsigned long linear);
> +#ifdef CONFIG_HVM
>      unsigned long (*gva_to_gfn            )(struct vcpu *v,
>                                              struct p2m_domain *p2m,
>                                              unsigned long va,
> @@ -136,6 +137,7 @@ struct paging_mode {
>                                              unsigned long cr3,
>                                              paddr_t ga, uint32_t *pfec,
>                                              unsigned int *page_order);
> +#endif
>      void          (*update_cr3            )(struct vcpu *v, int do_locking,
>                                              bool noflush);
>      void          (*update_paging_modes   )(struct vcpu *v);
> @@ -286,6 +288,8 @@ unsigned long paging_gva_to_gfn(struct v
>                                  unsigned long va,
>                                  uint32_t *pfec);
>  
> +#ifdef CONFIG_HVM
> +
>  /* Translate a guest address using a particular CR3 value.  This is used
>   * to by nested HAP code, to walk the guest-supplied NPT tables as if
>   * they were pagetables.
> @@ -304,6 +308,8 @@ static inline unsigned long paging_ga_to
>          page_order);
>  }
>  
> +#endif /* CONFIG_HVM */
> +
>  /* Update all the things that are derived from the guest's CR3.
>   * Called when the guest changes CR3; the caller can then use v->arch.cr3
>   * as the value to load into the host CR3 to schedule this vcpu */
> 
> 

[PATCH 6/6] x86/p2m: set_shared_p2m_entry() is MEM_SHARING-only

[Jan Beulich]

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

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1476,6 +1476,8 @@ int clear_identity_p2m_entry(struct doma
     return ret;
 }
 
+#ifdef CONFIG_MEM_SHARING
+
 /* Returns: 0 for success, -errno for failure */
 int set_shared_p2m_entry(struct domain *d, unsigned long gfn_l, mfn_t mfn)
 {
@@ -1514,7 +1516,10 @@ int set_shared_p2m_entry(struct domain *
     return rc;
 }
 
+#endif /* CONFIG_MEM_SHARING */
+
 #ifdef CONFIG_HVM
+
 static struct p2m_domain *
 p2m_getlru_nestedp2m(struct domain *d, struct p2m_domain *p2m)
 {

Re: [PATCH 6/6] x86/p2m: set_shared_p2m_entry() is MEM_SHARING-only

[Tamas K Lengyel]

On Tue, Dec 15, 2020 at 11:28 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Tamas K Lengyel <tamas@tklengyel.com>