diff mbox series

[4/4] x86/mm: More discriptive names for page de/validation functions

Message ID 20191212173203.1692762-5-george.dunlap@citrix.com (mailing list archive)
State Superseded
Headers show
Series Post-299 cleanups | expand

Commit Message

George Dunlap Dec. 12, 2019, 5:32 p.m. UTC 
The functions alloc_page_type(), alloc_lN_table(), free_page_type()
and free_lN_table() are confusingly named: nothing is being allocated
or freed.  Rather, the page being passed in is being either validated
or devalidated for use as the specific type.

Rename these functions to "validate" and "devalidate" respectively to
be more clear.

No functional change.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/domain.c    |  2 +-
 xen/arch/x86/mm.c        | 62 ++++++++++++++++++++--------------------
 xen/include/asm-x86/mm.h |  4 +--
 3 files changed, 34 insertions(+), 34 deletions(-)

Comments

Andrew Cooper Dec. 12, 2019, 7:22 p.m. UTC | #1 
On 12/12/2019 17:32, George Dunlap wrote:
> @@ -3016,7 +3016,7 @@ static int _get_page_type(struct page_info *page, unsigned long type,
>              page->partial_flags = 0;
>          }
>          page->linear_pt_count = 0;
> -        rc = alloc_page_type(page, type, preemptible);
> +        rc = validate_page_type(page, type, preemptible);
>      }
>  
>   out:

FYI this has a conflict vs XSA-309, and needs rebasing onto staging. 
Luckily, this one isn't hard to fix up.

~Andrew
Andrew Cooper Dec. 12, 2019, 8:07 p.m. UTC | #2 
On 12/12/2019 17:32, George Dunlap wrote:
> The functions alloc_page_type(), alloc_lN_table(), free_page_type()
> and free_lN_table() are confusingly named:

There is alloc_segdesc_page() which should be adjusted for consistency.

> nothing is being allocated or freed.

Well - strictly speaking the type reference is being obtained/dropped,
and this is a kind of alloc/free, although I agree that the names are
not great.

However, I'm not entirely sure that {de,}validate are great either,
because it isn't obviously tied to obtaining/dropping a type reference.

That said, I don't have a better suggestion right now.

~Andrew
Jan Beulich Dec. 13, 2019, 10:48 a.m. UTC | #3 
On 12.12.2019 21:07, Andrew Cooper wrote:
> On 12/12/2019 17:32, George Dunlap wrote:
>> The functions alloc_page_type(), alloc_lN_table(), free_page_type()
>> and free_lN_table() are confusingly named:
> 
> There is alloc_segdesc_page() which should be adjusted for consistency.
> 
>> nothing is being allocated or freed.
> 
> Well - strictly speaking the type reference is being obtained/dropped,
> and this is a kind of alloc/free, although I agree that the names are
> not great.
> 
> However, I'm not entirely sure that {de,}validate are great either,
> because it isn't obviously tied to obtaining/dropping a type reference.
> 
> That said, I don't have a better suggestion right now.

Following the wording of yours, how about {obtain,drop}_page_type()?

Jan
George Dunlap Dec. 13, 2019, noon UTC | #4 
> On Dec 13, 2019, at 10:48 AM, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 12.12.2019 21:07, Andrew Cooper wrote:
>> On 12/12/2019 17:32, George Dunlap wrote:
>>> The functions alloc_page_type(), alloc_lN_table(), free_page_type()
>>> and free_lN_table() are confusingly named:
>> 
>> There is alloc_segdesc_page() which should be adjusted for consistency.
>> 
>>> nothing is being allocated or freed.
>> 
>> Well - strictly speaking the type reference is being obtained/dropped,
>> and this is a kind of alloc/free, although I agree that the names are
>> not great.

On the contrary — the type reference was obtained / will be dropped in {get,put}_page_type(); but the page has not yet been validated to actually be used as that type / still holds references to other pages as though it were that type.  

>> 
>> However, I'm not entirely sure that {de,}validate are great either,
>> because it isn't obviously tied to obtaining/dropping a type reference.
>> 
>> That said, I don't have a better suggestion right now.
> 
> Following the wording of yours, how about {obtain,drop}_page_type()?

“Obtain” is literally a synonym for “get”; and there are many places in the code where we say thing like, “drop the type count” just before calling “put”.  

I agree “devalidate” looks a bit clunky, but all through the discussions on XSA-299, the word “de-validate” was used for the work that the “free" functions are doing — namely, dropping references to other pages such that the “validate” bit is clear.

I mean, we could do something like “bless” and “unbless”, but I hardly think that’s more clear.  “promote” and “demote”?

 -George
Jan Beulich Dec. 13, 2019, 12:25 p.m. UTC | #5 
On 13.12.2019 13:00, George Dunlap wrote:
> 
> 
>> On Dec 13, 2019, at 10:48 AM, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 12.12.2019 21:07, Andrew Cooper wrote:
>>> On 12/12/2019 17:32, George Dunlap wrote:
>>>> The functions alloc_page_type(), alloc_lN_table(), free_page_type()
>>>> and free_lN_table() are confusingly named:
>>>
>>> There is alloc_segdesc_page() which should be adjusted for consistency.
>>>
>>>> nothing is being allocated or freed.
>>>
>>> Well - strictly speaking the type reference is being obtained/dropped,
>>> and this is a kind of alloc/free, although I agree that the names are
>>> not great.
> 
> On the contrary — the type reference was obtained / will be dropped in {get,put}_page_type(); but the page has not yet been validated to actually be used as that type / still holds references to other pages as though it were that type.  
> 
>>>
>>> However, I'm not entirely sure that {de,}validate are great either,
>>> because it isn't obviously tied to obtaining/dropping a type reference.
>>>
>>> That said, I don't have a better suggestion right now.
>>
>> Following the wording of yours, how about {obtain,drop}_page_type()?
> 
> “Obtain” is literally a synonym for “get”; and there are many places in the code where we say thing like, “drop the type count” just before calling “put”.  
> 
> I agree “devalidate” looks a bit clunky, but all through the discussions on XSA-299, the word “de-validate” was used for the work that the “free" functions are doing — namely, dropping references to other pages such that the “validate” bit is clear.

True.

> I mean, we could do something like “bless” and “unbless”, but I hardly think that’s more clear.  “promote” and “demote”?

Why not. Perhaps with the trailing _type also dropped?

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index f5c0c378ef..ee20de6493 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2032,7 +2032,7 @@  static int relinquish_memory(
             if ( likely(y == x) )
             {
                 /* No need for atomic update of type_info here: noone else updates it. */
-                switch ( ret = free_page_type(page, x, 1) )
+                switch ( ret = devalidate_page_type(page, x, 1) )
                 {
                 case 0:
                     break;
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 54b4100d55..dc1463123c 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1366,7 +1366,7 @@  static int put_page_from_l4e(l4_pgentry_t l4e, unsigned long pfn,
     return put_pt_page(l4e_get_page(l4e), mfn_to_page(_mfn(pfn)), flags);
 }
 
-static int alloc_l1_table(struct page_info *page)
+static int validate_l1_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
     l1_pgentry_t  *pl1e;
@@ -1408,7 +1408,7 @@  static int alloc_l1_table(struct page_info *page)
 
  fail:
     gdprintk(XENLOG_WARNING,
-             "Failure %d in alloc_l1_table: slot %#x\n", ret, i);
+             "Failure %d in validate_l1_table: slot %#x\n", ret, i);
  out:
     while ( i-- > 0 )
         put_page_from_l1e(pl1e[i], d);
@@ -1441,7 +1441,7 @@  static int create_pae_xen_mappings(struct domain *d, l3_pgentry_t *pl3e)
      *  1. Cannot appear in slots != 3 because get_page_type() checks the
      *     PGT_pae_xen_l2 flag, which is asserted iff the L2 appears in slot 3
      *  2. Cannot appear in another page table's L3:
-     *     a. alloc_l3_table() calls this function and this check will fail
+     *     a. validate_l3_table() calls this function and this check will fail
      *     b. mod_l3_entry() disallows updates to slot 3 in an existing table
      */
     page = l3e_get_page(l3e3);
@@ -1457,7 +1457,7 @@  static int create_pae_xen_mappings(struct domain *d, l3_pgentry_t *pl3e)
     return 1;
 }
 
-static int alloc_l2_table(struct page_info *page, unsigned long type)
+static int validate_l2_table(struct page_info *page, unsigned long type)
 {
     struct domain *d = page_get_owner(page);
     unsigned long  l2mfn = mfn_x(page_to_mfn(page));
@@ -1469,8 +1469,8 @@  static int alloc_l2_table(struct page_info *page, unsigned long type)
     pl2e = map_domain_page(_mfn(l2mfn));
 
     /*
-     * NB that alloc_l2_table will never set partial_pte on an l2; but
-     * free_l2_table might if a linear_pagetable entry is interrupted
+     * NB that validate_l2_table will never set partial_pte on an l2; but
+     * devalidate_l2_table might if a linear_pagetable entry is interrupted
      * partway through de-validation.  In that circumstance,
      * get_page_from_l2e() will always return -EINVAL; and we must
      * retain the type ref by doing the normal partial_flags tracking.
@@ -1497,7 +1497,7 @@  static int alloc_l2_table(struct page_info *page, unsigned long type)
         /*
          * It shouldn't be possible for get_page_from_l2e to return
          * -ERESTART, since we never call this with PTF_preemptible.
-         * (alloc_l1_table may return -EINTR on an L1TF-vulnerable
+         * (validate_l1_table may return -EINTR on an L1TF-vulnerable
          * entry.)
          *
          * NB that while on a "clean" promotion, we can never get
@@ -1518,12 +1518,12 @@  static int alloc_l2_table(struct page_info *page, unsigned long type)
         else if ( rc < 0 && rc != -EINTR )
         {
             gdprintk(XENLOG_WARNING,
-                     "Failure %d in alloc_l2_table: slot %#x\n", rc, i);
+                     "Failure %d in validate_l2_table: slot %#x\n", rc, i);
             ASSERT(current->arch.old_guest_table == NULL);
             if ( i )
             {
                 /*
-                 * alloc_l1_table() doesn't set old_guest_table; it does
+                 * validate_l1_table() doesn't set old_guest_table; it does
                  * its own tear-down immediately on failure.  If it
                  * did we'd need to check it and set partial_flags as we
                  * do in alloc_l[34]_table().
@@ -1556,7 +1556,7 @@  static int alloc_l2_table(struct page_info *page, unsigned long type)
     return rc;
 }
 
-static int alloc_l3_table(struct page_info *page)
+static int validate_l3_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
     unsigned long  l3mfn = mfn_x(page_to_mfn(page));
@@ -1629,7 +1629,7 @@  static int alloc_l3_table(struct page_info *page)
     if ( rc < 0 && rc != -ERESTART && rc != -EINTR )
     {
         gdprintk(XENLOG_WARNING,
-                 "Failure %d in alloc_l3_table: slot %#x\n", rc, i);
+                 "Failure %d in validate_l3_table: slot %#x\n", rc, i);
         if ( i )
         {
             page->nr_validated_ptes = i;
@@ -1680,7 +1680,7 @@  void init_xen_pae_l2_slots(l2_pgentry_t *l2t, const struct domain *d)
  * Fill an L4 with Xen entries.
  *
  * This function must write all ROOT_PAGETABLE_PV_XEN_SLOTS, to clobber any
- * values a guest may have left there from alloc_l4_table().
+ * values a guest may have left there from validate_l4_table().
  *
  * l4t and l4mfn are mandatory, but l4mfn doesn't need to be the mfn under
  * *l4t.  All other parameters are optional and will either fill or zero the
@@ -1783,7 +1783,7 @@  void zap_ro_mpt(mfn_t mfn)
 }
 
 #ifdef CONFIG_PV
-static int alloc_l4_table(struct page_info *page)
+static int validate_l4_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
     unsigned long  l4mfn = mfn_x(page_to_mfn(page));
@@ -1822,7 +1822,7 @@  static int alloc_l4_table(struct page_info *page)
         {
             if ( rc != -EINTR )
                 gdprintk(XENLOG_WARNING,
-                         "Failure %d in alloc_l4_table: slot %#x\n", rc, i);
+                         "Failure %d in validate_l4_table: slot %#x\n", rc, i);
             if ( i )
             {
                 page->nr_validated_ptes = i;
@@ -1878,7 +1878,7 @@  static int alloc_l4_table(struct page_info *page)
     return rc;
 }
 
-static void free_l1_table(struct page_info *page)
+static void devalidate_l1_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
     l1_pgentry_t *pl1e;
@@ -1893,7 +1893,7 @@  static void free_l1_table(struct page_info *page)
 }
 
 
-static int free_l2_table(struct page_info *page)
+static int devalidate_l2_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
     unsigned long l2mfn = mfn_x(page_to_mfn(page));
@@ -1945,7 +1945,7 @@  static int free_l2_table(struct page_info *page)
     return rc;
 }
 
-static int free_l3_table(struct page_info *page)
+static int devalidate_l3_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
     unsigned long l3mfn = mfn_x(page_to_mfn(page));
@@ -1992,7 +1992,7 @@  static int free_l3_table(struct page_info *page)
     return rc > 0 ? 0 : rc;
 }
 
-static int free_l4_table(struct page_info *page)
+static int devalidate_l4_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
     unsigned long l4mfn = mfn_x(page_to_mfn(page));
@@ -2592,7 +2592,7 @@  static void get_page_light(struct page_info *page)
     while ( unlikely(y != x) );
 }
 
-static int alloc_page_type(struct page_info *page, unsigned long type,
+static int validate_page_type(struct page_info *page, unsigned long type,
                            int preemptible)
 {
 #ifdef CONFIG_PV
@@ -2606,25 +2606,25 @@  static int alloc_page_type(struct page_info *page, unsigned long type,
     switch ( type & PGT_type_mask )
     {
     case PGT_l1_page_table:
-        rc = alloc_l1_table(page);
+        rc = validate_l1_table(page);
         break;
     case PGT_l2_page_table:
         ASSERT(preemptible);
-        rc = alloc_l2_table(page, type);
+        rc = validate_l2_table(page, type);
         break;
     case PGT_l3_page_table:
         ASSERT(preemptible);
-        rc = alloc_l3_table(page);
+        rc = validate_l3_table(page);
         break;
     case PGT_l4_page_table:
         ASSERT(preemptible);
-        rc = alloc_l4_table(page);
+        rc = validate_l4_table(page);
         break;
     case PGT_seg_desc_page:
         rc = alloc_segdesc_page(page);
         break;
     default:
-        printk("Bad type in alloc_page_type %lx t=%" PRtype_info " c=%lx\n",
+        printk("Bad type in validate_page_type %lx t=%" PRtype_info " c=%lx\n",
                type, page->u.inuse.type_info,
                page->count_info);
         rc = -EINVAL;
@@ -2672,7 +2672,7 @@  static int alloc_page_type(struct page_info *page, unsigned long type,
 }
 
 
-int free_page_type(struct page_info *page, unsigned long type,
+int devalidate_page_type(struct page_info *page, unsigned long type,
                    int preemptible)
 {
 #ifdef CONFIG_PV
@@ -2700,20 +2700,20 @@  int free_page_type(struct page_info *page, unsigned long type,
     switch ( type & PGT_type_mask )
     {
     case PGT_l1_page_table:
-        free_l1_table(page);
+        devalidate_l1_table(page);
         rc = 0;
         break;
     case PGT_l2_page_table:
         ASSERT(preemptible);
-        rc = free_l2_table(page);
+        rc = devalidate_l2_table(page);
         break;
     case PGT_l3_page_table:
         ASSERT(preemptible);
-        rc = free_l3_table(page);
+        rc = devalidate_l3_table(page);
         break;
     case PGT_l4_page_table:
         ASSERT(preemptible);
-        rc = free_l4_table(page);
+        rc = devalidate_l4_table(page);
         break;
     default:
         gdprintk(XENLOG_WARNING, "type %" PRtype_info " mfn %" PRI_mfn "\n",
@@ -2733,7 +2733,7 @@  int free_page_type(struct page_info *page, unsigned long type,
 static int _put_final_page_type(struct page_info *page, unsigned long type,
                                 bool preemptible, struct page_info *ptpg)
 {
-    int rc = free_page_type(page, type, preemptible);
+    int rc = devalidate_page_type(page, type, preemptible);
 
     if ( ptpg && PGT_type_equal(type, ptpg->u.inuse.type_info) &&
          (type & PGT_validated) && rc != -EINTR )
@@ -3016,7 +3016,7 @@  static int _get_page_type(struct page_info *page, unsigned long type,
             page->partial_flags = 0;
         }
         page->linear_pt_count = 0;
-        rc = alloc_page_type(page, type, preemptible);
+        rc = validate_page_type(page, type, preemptible);
     }
 
  out:
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 320c6cd196..a837f69548 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -355,8 +355,8 @@  static inline void *__page_to_virt(const struct page_info *pg)
                     (PAGE_SIZE / (sizeof(*pg) & -sizeof(*pg))));
 }
 
-int free_page_type(struct page_info *page, unsigned long type,
-                   int preemptible);
+int devalidate_page_type(struct page_info *page, unsigned long type,
+                         int preemptible);
 
 void init_xen_pae_l2_slots(l2_pgentry_t *l2t, const struct domain *d);
 void init_xen_l4_slots(l4_pgentry_t *l4t, mfn_t l4mfn,