[PATCH 0/5] x86: XSA-240 / -242 follow-up

[PATCH 0/5] x86: XSA-240 / -242 follow-up

[Jan Beulich]

These were held back due to the freeze.

1: make get_page_from_mfn() return struct page_info *
2: remove _PAGE_PSE check from get_page_from_l2e()
3: improve _put_page_type() readability
4: use switch() in _put_page_type()
5: make _get_page_type() a proper counterpart of _put_page_type() again

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/5] x86: make get_page_from_mfn() return struct page_info *

[Jan Beulich]

Almost all users of it want it, and it calculates it anyway.

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

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -704,7 +704,6 @@ get_##level##_linear_pagetable(
     level##_pgentry_t pde, unsigned long pde_pfn, struct domain *d)         \
 {                                                                           \
     unsigned long x, y;                                                     \
-    struct page_info *page;                                                 \
     unsigned long pfn;                                                      \
                                                                             \
     if ( !opt_pv_linear_pt )                                                \
@@ -723,14 +722,15 @@ get_##level##_linear_pagetable(
                                                                             \
     if ( (pfn = level##e_get_pfn(pde)) != pde_pfn )                         \
     {                                                                       \
-        struct page_info *ptpg = mfn_to_page(_mfn(pde_pfn));                \
+        struct page_info *page, *ptpg = mfn_to_page(_mfn(pde_pfn));         \
                                                                             \
         /* Make sure the page table belongs to the correct domain. */       \
         if ( unlikely(page_get_owner(ptpg) != d) )                          \
             return 0;                                                       \
                                                                             \
         /* Make sure the mapped frame belongs to the correct domain. */     \
-        if ( unlikely(!get_page_from_mfn(_mfn(pfn), d)) )                   \
+        page = get_page_from_mfn(_mfn(pfn), d);                             \
+        if ( unlikely(!page) )                                              \
             return 0;                                                       \
                                                                             \
         /*                                                                  \
@@ -740,7 +740,6 @@ get_##level##_linear_pagetable(
          * elsewhere.                                                       \
          * If so, atomically increment the count (checking for overflow).   \
          */                                                                 \
-        page = mfn_to_page(_mfn(pfn));                                      \
         if ( !inc_linear_entries(ptpg) )                                    \
         {                                                                   \
             put_page(page);                                                 \
@@ -3724,7 +3723,8 @@ long do_mmu_update(
                 xsm_checked = xsm_needed;
             }
 
-            if ( unlikely(!get_page_from_mfn(_mfn(mfn), pg_owner)) )
+            page = get_page_from_mfn(_mfn(mfn), pg_owner);
+            if ( unlikely(!page) )
             {
                 gdprintk(XENLOG_WARNING,
                          "Could not get page for mach->phys update\n");
@@ -3736,7 +3736,7 @@ long do_mmu_update(
 
             paging_mark_dirty(pg_owner, _mfn(mfn));
 
-            put_page(mfn_to_page(_mfn(mfn)));
+            put_page(page);
             break;
 
         default:
@@ -3921,10 +3921,10 @@ static int __do_update_va_mapping(
 
     rc = -EINVAL;
     pl1e = map_guest_l1e(va, &gl1mfn);
-    if ( unlikely(!pl1e || !get_page_from_mfn(gl1mfn, d)) )
+    gl1pg = pl1e ? get_page_from_mfn(gl1mfn, d) : NULL;
+    if ( unlikely(!gl1pg) )
         goto out;
 
-    gl1pg = mfn_to_page(gl1mfn);
     if ( !page_lock(gl1pg) )
     {
         put_page(gl1pg);
@@ -4120,10 +4120,10 @@ int xenmem_add_to_physmap_one(
                 put_gfn(d, gfn);
                 return -ENOMEM;
             }
-            if ( !get_page_from_mfn(_mfn(idx), d) )
-                break;
             mfn = _mfn(idx);
-            page = mfn_to_page(mfn);
+            page = get_page_from_mfn(mfn, d);
+            if ( unlikely(!page) )
+                mfn = INVALID_MFN;
             break;
         }
         case XENMAPSPACE_gmfn_foreign:
--- a/xen/arch/x86/pv/grant_table.c
+++ b/xen/arch/x86/pv/grant_table.c
@@ -80,7 +80,8 @@ int create_grant_pv_mapping(uint64_t add
 
         gl1mfn = _mfn(addr >> PAGE_SHIFT);
 
-        if ( !get_page_from_mfn(gl1mfn, currd) )
+        page = get_page_from_mfn(gl1mfn, currd);
+        if ( !page )
             goto out;
 
         pl1e = map_domain_page(gl1mfn) + (addr & ~PAGE_MASK);
@@ -101,11 +102,11 @@ int create_grant_pv_mapping(uint64_t add
             goto out;
         }
 
-        if ( !get_page_from_mfn(gl1mfn, currd) )
+        page = get_page_from_mfn(gl1mfn, currd);
+        if ( !page )
             goto out_unmap;
     }
 
-    page = mfn_to_page(gl1mfn);
     if ( !page_lock(page) )
         goto out_put;
 
@@ -159,10 +160,10 @@ static bool steal_linear_address(unsigne
         goto out;
     }
 
-    if ( !get_page_from_mfn(gl1mfn, currd) )
+    page = get_page_from_mfn(gl1mfn, currd);
+    if ( !page )
         goto out_unmap;
 
-    page = mfn_to_page(gl1mfn);
     if ( !page_lock(page) )
         goto out_put;
 
@@ -235,7 +236,8 @@ int replace_grant_pv_mapping(uint64_t ad
 
         gl1mfn = _mfn(addr >> PAGE_SHIFT);
 
-        if ( !get_page_from_mfn(gl1mfn, currd) )
+        page = get_page_from_mfn(gl1mfn, currd);
+        if ( !page )
             goto out;
 
         pl1e = map_domain_page(gl1mfn) + (addr & ~PAGE_MASK);
@@ -263,12 +265,11 @@ int replace_grant_pv_mapping(uint64_t ad
         if ( !pl1e )
             goto out;
 
-        if ( !get_page_from_mfn(gl1mfn, currd) )
+        page = get_page_from_mfn(gl1mfn, currd);
+        if ( !page )
             goto out_unmap;
     }
 
-    page = mfn_to_page(gl1mfn);
-
     if ( !page_lock(page) )
         goto out_put;
 
--- a/xen/arch/x86/pv/ro-page-fault.c
+++ b/xen/arch/x86/pv/ro-page-fault.c
@@ -253,10 +253,10 @@ static int ptwr_do_page_fault(struct x86
     struct page_info *page;
     int rc;
 
-    if ( !get_page_from_mfn(l1e_get_mfn(pte), current->domain) )
+    page = get_page_from_mfn(l1e_get_mfn(pte), current->domain);
+    if ( !page )
         return X86EMUL_UNHANDLEABLE;
 
-    page = l1e_get_page(pte);
     if ( !page_lock(page) )
     {
         put_page(page);
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -382,7 +382,7 @@ int  get_page_from_l1e(
     l1_pgentry_t l1e, struct domain *l1e_owner, struct domain *pg_owner);
 void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner);
 
-static inline bool get_page_from_mfn(mfn_t mfn, struct domain *d)
+static inline struct page_info *get_page_from_mfn(mfn_t mfn, struct domain *d)
 {
     struct page_info *page = __mfn_to_page(mfn_x(mfn));
 
@@ -390,10 +390,10 @@ static inline bool get_page_from_mfn(mfn
     {
         gdprintk(XENLOG_WARNING,
                  "Could not get page ref for mfn %"PRI_mfn"\n", mfn_x(mfn));
-        return false;
+        return NULL;
     }
 
-    return true;
+    return page;
 }
 
 static inline void put_page_and_type(struct page_info *page)



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/5] x86: remove _PAGE_PSE check from get_page_from_l2e()

[Jan Beulich]

With L2_DISALLOW_MASK containing _PAGE_PSE unconditionally as of commit
56fff3e5e9 ("x86: nuke PV superpage option and code") there's no point
anymore in separately checking for the bit.

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

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1106,15 +1106,10 @@ get_page_from_l2e(
         return -EINVAL;
     }
 
-    if ( !(l2e_get_flags(l2e) & _PAGE_PSE) )
-    {
-        rc = get_page_and_type_from_mfn(_mfn(mfn), PGT_l1_page_table, d, 0, 0);
-        if ( unlikely(rc == -EINVAL) && get_l2_linear_pagetable(l2e, pfn, d) )
-            rc = 0;
-        return rc;
-    }
-
-    return -EINVAL;
+    rc = get_page_and_type_from_mfn(_mfn(mfn), PGT_l1_page_table, d, 0, 0);
+    if ( unlikely(rc == -EINVAL) && get_l2_linear_pagetable(l2e, pfn, d) )
+        rc = 0;
+    return rc;
 }
 
 




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/5] x86: improve _put_page_type() readability

[Jan Beulich]

By limiting the scope of rc it is more obvious that failure can be
reported only if _put_final_page_type() failed.

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

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2452,7 +2452,6 @@ static int _put_page_type(struct page_in
                           struct page_info *ptpg)
 {
     unsigned long nx, x, y = page->u.inuse.type_info;
-    int rc = 0;
 
     for ( ; ; )
     {
@@ -2466,6 +2465,8 @@ static int _put_page_type(struct page_in
             if ( unlikely((nx & PGT_type_mask) <= PGT_l4_page_table) &&
                  likely(nx & (PGT_validated|PGT_partial)) )
             {
+                int rc;
+
                 /*
                  * Page-table pages must be unvalidated when count is zero. The
                  * 'free' is safe because the refcnt is non-zero and validated
@@ -2477,10 +2478,9 @@ static int _put_page_type(struct page_in
                     continue;
                 /* We cleared the 'valid bit' so we do the clean up. */
                 rc = _put_final_page_type(page, x, preemptible, ptpg);
-                ptpg = NULL;
                 if ( x & PGT_partial )
                     put_page(page);
-                break;
+                return rc;
             }
 
             if ( !ptpg || !PGT_type_equal(x, ptpg->u.inuse.type_info) )
@@ -2518,12 +2518,11 @@ static int _put_page_type(struct page_in
 
     if ( ptpg && PGT_type_equal(x, ptpg->u.inuse.type_info) )
     {
-        ASSERT(!rc);
         dec_linear_uses(page);
         dec_linear_entries(ptpg);
     }
 
-    return rc;
+    return 0;
 }
 
 




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 4/5] x86: use switch() in _put_page_type()

[Jan Beulich]

Use this to cheaply add another assertion.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: Would it perhaps be better to return after the assertion?

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2460,8 +2460,9 @@ static int _put_page_type(struct page_in
 
         ASSERT((x & PGT_count_mask) != 0);
 
-        if ( unlikely((nx & PGT_count_mask) == 0) )
+        switch ( nx & (PGT_locked | PGT_count_mask) )
         {
+        case 0:
             if ( unlikely((nx & PGT_type_mask) <= PGT_l4_page_table) &&
                  likely(nx & (PGT_validated|PGT_partial)) )
             {
@@ -2496,10 +2497,14 @@ static int _put_page_type(struct page_in
             }
             else
                 BUG_ON(!IS_ENABLED(CONFIG_PV_LINEAR_PT));
-        }
-        else if ( unlikely((nx & (PGT_locked | PGT_count_mask)) ==
-                           (PGT_locked | 1)) )
-        {
+
+            break;
+
+        case PGT_locked:
+            ASSERT_UNREACHABLE();
+            break;
+
+        case PGT_locked | 1:
             /*
              * We must not drop the second to last reference when the page is
              * locked, as page_unlock() doesn't do any cleanup of the type.




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 5/5] x86: make _get_page_type() a proper counterpart of _put_page_type() again

[Jan Beulich]

Drop one of the leading underscores and use bool for its "preemptible"
parameter.

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

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2531,8 +2531,8 @@ static int _put_page_type(struct page_in
 }
 
 
-static int __get_page_type(struct page_info *page, unsigned long type,
-                           int preemptible)
+static int _get_page_type(struct page_info *page, unsigned long type,
+                          bool preemptible)
 {
     unsigned long nx, x, y = page->u.inuse.type_info;
     int rc = 0, iommu_ret = 0;
@@ -2694,7 +2694,8 @@ void put_page_type(struct page_info *pag
 
 int get_page_type(struct page_info *page, unsigned long type)
 {
-    int rc = __get_page_type(page, type, 0);
+    int rc = _get_page_type(page, type, false);
+
     if ( likely(rc == 0) )
         return 1;
     ASSERT(rc != -EINTR && rc != -ERESTART);
@@ -2709,7 +2710,7 @@ int put_page_type_preemptible(struct pag
 int get_page_type_preemptible(struct page_info *page, unsigned long type)
 {
     ASSERT(!current->arch.old_guest_table);
-    return __get_page_type(page, type, 1);
+    return _get_page_type(page, type, true);
 }
 
 int put_old_guest_table(struct vcpu *v)




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/5] x86: make get_page_from_mfn() return struct page_info *

[Andrew Cooper]

On 04/12/17 10:44, Jan Beulich wrote:
> Almost all users of it want it, and it calculates it anyway.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/5] x86: remove _PAGE_PSE check from get_page_from_l2e()

[Andrew Cooper]

On 04/12/17 10:44, Jan Beulich wrote:
> With L2_DISALLOW_MASK containing _PAGE_PSE unconditionally as of commit
> 56fff3e5e9 ("x86: nuke PV superpage option and code") there's no point
> anymore in separately checking for the bit.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -1106,15 +1106,10 @@ get_page_from_l2e(
>          return -EINVAL;
>      }
>  
> -    if ( !(l2e_get_flags(l2e) & _PAGE_PSE) )
> -    {
> -        rc = get_page_and_type_from_mfn(_mfn(mfn), PGT_l1_page_table, d, 0, 0);
> -        if ( unlikely(rc == -EINVAL) && get_l2_linear_pagetable(l2e, pfn, d) )
> -            rc = 0;
> -        return rc;
> -    }
> -
> -    return -EINVAL;
> +    rc = get_page_and_type_from_mfn(_mfn(mfn), PGT_l1_page_table, d, 0, 0);
> +    if ( unlikely(rc == -EINVAL) && get_l2_linear_pagetable(l2e, pfn, d) )
> +        rc = 0;

Newline here.  Otherwise, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

> +    return rc;
>  }
>  
>  
>
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/5] x86: improve _put_page_type() readability

[Andrew Cooper]

On 04/12/17 10:45, Jan Beulich wrote:
> @@ -2477,10 +2478,9 @@ static int _put_page_type(struct page_in
>                      continue;
>                  /* We cleared the 'valid bit' so we do the clean up. */
>                  rc = _put_final_page_type(page, x, preemptible, ptpg);
> -                ptpg = NULL;
>                  if ( x & PGT_partial )
>                      put_page(page);
> -                break;

Newline here.  Otherwise, Reviewed-by: Andrew Cooper
<andrew.cooper3@citix.com>

> +                return rc;
>              }
>  
>              if ( !ptpg || !PGT_type_equal(x, ptpg->u.inuse.type_info) )
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/5] x86: use switch() in _put_page_type()

[Andrew Cooper]

On 04/12/17 10:46, Jan Beulich wrote:
> Use this to cheaply add another assertion.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TBD: Would it perhaps be better to return after the assertion?

Yes, otherwise we risk falling into an infinite continue loop.

With a suitable return value, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/5] x86: make _get_page_type() a proper counterpart of _put_page_type() again

[Andrew Cooper]

On 04/12/17 10:46, Jan Beulich wrote:
> @@ -2709,7 +2710,7 @@ int put_page_type_preemptible(struct pag
>  int get_page_type_preemptible(struct page_info *page, unsigned long type)
>  {
>      ASSERT(!current->arch.old_guest_table);

Newline.  Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> -    return __get_page_type(page, type, 1);
> +    return _get_page_type(page, type, true);
>  }
>  
>  int put_old_guest_table(struct vcpu *v)
>
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/5] x86: improve _put_page_type() readability

[Jan Beulich]

>>> On 04.12.17 at 16:28, <andrew.cooper3@citrix.com> wrote:
> On 04/12/17 10:45, Jan Beulich wrote:
>> @@ -2477,10 +2478,9 @@ static int _put_page_type(struct page_in
>>                      continue;
>>                  /* We cleared the 'valid bit' so we do the clean up. */
>>                  rc = _put_final_page_type(page, x, preemptible, ptpg);
>> -                ptpg = NULL;
>>                  if ( x & PGT_partial )
>>                      put_page(page);
>> -                break;
> 
> Newline here.

Hmm, I've added one because I don't really mind, but this isn't the
main return point of the function.

>  Otherwise, Reviewed-by: Andrew Cooper
> <andrew.cooper3@citix.com>

Thanks, Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/5] x86: use switch() in _put_page_type()

[Jan Beulich]

>>> On 04.12.17 at 16:40, <andrew.cooper3@citrix.com> wrote:
> On 04/12/17 10:46, Jan Beulich wrote:
>> Use this to cheaply add another assertion.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> TBD: Would it perhaps be better to return after the assertion?
> 
> Yes, otherwise we risk falling into an infinite continue loop.

I've used -EILSEQ, but no, there's no infinite loop potential here, as
there's a preemption check past the switch() statement (and there
was "break" rather than "continue" after the assertion). This and
the lack of reasonably suitable error code to return here was why
I didn't use "return" in the first version. Or did you mean "infinite
continuation loop" (affecting just the guest)?

> With a suitable return value, Reviewed-by: Andrew Cooper
> <andrew.cooper3@citrix.com>

Thanks.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel