All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] [RFC] x86/domctl: Fix getpageframeinfo* handling.
@ 2015-05-18 10:59 Andrew Cooper
  2015-05-18 11:43 ` Jan Beulich
  0 siblings, 1 reply; 4+ messages in thread
From: Andrew Cooper @ 2015-05-18 10:59 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Daniel De Graaf, Keir Fraser, Jan Beulich

In tree, there is one single caller of XEN_DOMCTL_getpageframeinfo3
(xc_get_pfn_type_batch()), and no callers of the older variants.

getpageframeinfo3 and getpageframeinfo2 are compatible if the parameter
contents are considered to be unsigned long; a compat guest calling
getpageframeinfo3 falls through into the getpageframeinfo2 handler.

However, getpageframeinfo3 and getpageframeinfo2 have different algorithms for
calculating the eventual frame type, which means that a toolstack will get
different answers depending on whether it is compat or not, which is a problem
for all possible uses.

Rewrite getpageframeinfo3 such that the code block can handle both regular and
compat guests, and use the original getpageframeinfo3 algorithm for frame
time, which is more complete.

Remove getpageframeinfo2 and getpageframeinfo1, as they are unused and
obsolete.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>

---
This is RFC because, while I like the consolidation of the compat and
non-compat codepaths, I am not totally happy with encouraging the general use
of raw_copy_to/from_guest().

Another option could be to introduce copy_ulong_to/from_guest(), but that does
not play nicely with the guest handle typing.
---
 xen/arch/x86/domctl.c               |  306 +++++++++--------------------------
 xen/include/public/domctl.h         |   27 +---
 xen/xsm/flask/hooks.c               |    2 -
 xen/xsm/flask/policy/access_vectors |    2 +-
 4 files changed, 78 insertions(+), 259 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 20cdccb..5c62849 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -91,237 +91,6 @@ long arch_do_domctl(
         break;
     }
 
-    case XEN_DOMCTL_getpageframeinfo:
-    {
-        struct page_info *page;
-        unsigned long mfn = domctl->u.getpageframeinfo.gmfn;
-
-        ret = -EINVAL;
-        if ( unlikely(!mfn_valid(mfn)) )
-            break;
-
-        page = mfn_to_page(mfn);
-
-        if ( likely(get_page(page, d)) )
-        {
-            ret = 0;
-
-            domctl->u.getpageframeinfo.type = XEN_DOMCTL_PFINFO_NOTAB;
-
-            if ( (page->u.inuse.type_info & PGT_count_mask) != 0 )
-            {
-                switch ( page->u.inuse.type_info & PGT_type_mask )
-                {
-                case PGT_l1_page_table:
-                    domctl->u.getpageframeinfo.type = XEN_DOMCTL_PFINFO_L1TAB;
-                    break;
-                case PGT_l2_page_table:
-                    domctl->u.getpageframeinfo.type = XEN_DOMCTL_PFINFO_L2TAB;
-                    break;
-                case PGT_l3_page_table:
-                    domctl->u.getpageframeinfo.type = XEN_DOMCTL_PFINFO_L3TAB;
-                    break;
-                case PGT_l4_page_table:
-                    domctl->u.getpageframeinfo.type = XEN_DOMCTL_PFINFO_L4TAB;
-                    break;
-                }
-            }
-
-            put_page(page);
-        }
-
-        copyback = 1;
-        break;
-    }
-
-    case XEN_DOMCTL_getpageframeinfo3:
-        if ( !has_32bit_shinfo(currd) )
-        {
-            unsigned int n, j;
-            unsigned int num = domctl->u.getpageframeinfo3.num;
-            struct page_info *page;
-            xen_pfn_t *arr;
-
-            if ( unlikely(num > 1024) ||
-                 unlikely(num != domctl->u.getpageframeinfo3.num) )
-            {
-                ret = -E2BIG;
-                break;
-            }
-
-            page = alloc_domheap_page(currd, MEMF_no_owner);
-            if ( !page )
-            {
-                ret = -ENOMEM;
-                break;
-            }
-            arr = __map_domain_page(page);
-
-            for ( n = ret = 0; n < num; )
-            {
-                unsigned int k = min_t(unsigned int, num - n,
-                                       PAGE_SIZE / sizeof(*arr));
-
-                if ( copy_from_guest_offset(arr,
-                                            domctl->u.getpageframeinfo3.array,
-                                            n, k) )
-                {
-                    ret = -EFAULT;
-                    break;
-                }
-
-                for ( j = 0; j < k; j++ )
-                {
-                    unsigned long type = 0;
-                    p2m_type_t t;
-
-                    page = get_page_from_gfn(d, arr[j], &t, P2M_ALLOC);
-
-                    if ( unlikely(!page) ||
-                         unlikely(is_xen_heap_page(page)) )
-                    {
-                        if ( p2m_is_broken(t) )
-                            type = XEN_DOMCTL_PFINFO_BROKEN;
-                        else
-                            type = XEN_DOMCTL_PFINFO_XTAB;
-                    }
-                    else
-                    {
-                        switch( page->u.inuse.type_info & PGT_type_mask )
-                        {
-                        case PGT_l1_page_table:
-                            type = XEN_DOMCTL_PFINFO_L1TAB;
-                            break;
-                        case PGT_l2_page_table:
-                            type = XEN_DOMCTL_PFINFO_L2TAB;
-                            break;
-                        case PGT_l3_page_table:
-                            type = XEN_DOMCTL_PFINFO_L3TAB;
-                            break;
-                        case PGT_l4_page_table:
-                            type = XEN_DOMCTL_PFINFO_L4TAB;
-                            break;
-                        }
-
-                        if ( page->u.inuse.type_info & PGT_pinned )
-                            type |= XEN_DOMCTL_PFINFO_LPINTAB;
-
-                        if ( page->count_info & PGC_broken )
-                            type = XEN_DOMCTL_PFINFO_BROKEN;
-                    }
-
-                    if ( page )
-                        put_page(page);
-                    arr[j] = type;
-                }
-
-                if ( copy_to_guest_offset(domctl->u.getpageframeinfo3.array,
-                                          n, arr, k) )
-                {
-                    ret = -EFAULT;
-                    break;
-                }
-
-                n += k;
-            }
-
-            page = mfn_to_page(domain_page_map_to_mfn(arr));
-            unmap_domain_page(arr);
-            free_domheap_page(page);
-
-            break;
-        }
-        /* fall thru */
-    case XEN_DOMCTL_getpageframeinfo2:
-    {
-        int n,j;
-        int num = domctl->u.getpageframeinfo2.num;
-        uint32_t *arr32;
-
-        if ( unlikely(num > 1024) )
-        {
-            ret = -E2BIG;
-            break;
-        }
-
-        arr32 = alloc_xenheap_page();
-        if ( !arr32 )
-        {
-            ret = -ENOMEM;
-            break;
-        }
-
-        ret = 0;
-        for ( n = 0; n < num; )
-        {
-            int k = PAGE_SIZE / 4;
-            if ( (num - n) < k )
-                k = num - n;
-
-            if ( copy_from_guest_offset(arr32,
-                                        domctl->u.getpageframeinfo2.array,
-                                        n, k) )
-            {
-                ret = -EFAULT;
-                break;
-            }
-
-            for ( j = 0; j < k; j++ )
-            {
-                struct page_info *page;
-                unsigned long gfn = arr32[j];
-
-                page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
-
-                if ( domctl->cmd == XEN_DOMCTL_getpageframeinfo3)
-                    arr32[j] = 0;
-
-                if ( unlikely(!page) ||
-                     unlikely(is_xen_heap_page(page)) )
-                    arr32[j] |= XEN_DOMCTL_PFINFO_XTAB;
-                else
-                {
-                    unsigned long type = 0;
-
-                    switch( page->u.inuse.type_info & PGT_type_mask )
-                    {
-                    case PGT_l1_page_table:
-                        type = XEN_DOMCTL_PFINFO_L1TAB;
-                        break;
-                    case PGT_l2_page_table:
-                        type = XEN_DOMCTL_PFINFO_L2TAB;
-                        break;
-                    case PGT_l3_page_table:
-                        type = XEN_DOMCTL_PFINFO_L3TAB;
-                        break;
-                    case PGT_l4_page_table:
-                        type = XEN_DOMCTL_PFINFO_L4TAB;
-                        break;
-                    }
-
-                    if ( page->u.inuse.type_info & PGT_pinned )
-                        type |= XEN_DOMCTL_PFINFO_LPINTAB;
-                    arr32[j] |= type;
-                }
-
-                if ( page )
-                    put_page(page);
-            }
-
-            if ( copy_to_guest_offset(domctl->u.getpageframeinfo2.array,
-                                      n, arr32, k) )
-            {
-                ret = -EFAULT;
-                break;
-            }
-
-            n += k;
-        }
-
-        free_xenheap_page(arr32);
-        break;
-    }
-
     case XEN_DOMCTL_getmemlist:
     {
         unsigned long max_pfns = domctl->u.getmemlist.max_pfns;
@@ -378,6 +147,81 @@ long arch_do_domctl(
         break;
     }
 
+    case XEN_DOMCTL_getpageframeinfo3:
+    {
+        unsigned int num = domctl->u.getpageframeinfo3.num;
+
+        /* Games to allow this code block to handle a compat guest. */
+        void * __user guest_handle = domctl->u.getpageframeinfo3.array.p;
+        unsigned int width = has_32bit_shinfo(currd) ? 32 : 64;
+
+        if ( unlikely(num > 1024) ||
+             unlikely(num != domctl->u.getpageframeinfo3.num) )
+        {
+            ret = -E2BIG;
+            break;
+        }
+
+        for ( i = 0; i < num; ++i )
+        {
+            unsigned long gfn = 0, type = 0;
+            struct page_info *page;
+            p2m_type_t t;
+
+            if ( raw_copy_from_guest(&gfn, guest_handle + (i * width), width) )
+            {
+                ret = -EFAULT;
+                break;
+            }
+
+            page = get_page_from_gfn(d, gfn, &t, P2M_ALLOC);
+
+            if ( unlikely(!page) ||
+                 unlikely(is_xen_heap_page(page)) )
+            {
+                if ( p2m_is_broken(t) )
+                    type = XEN_DOMCTL_PFINFO_BROKEN;
+                else
+                    type = XEN_DOMCTL_PFINFO_XTAB;
+            }
+            else
+            {
+                switch( page->u.inuse.type_info & PGT_type_mask )
+                {
+                case PGT_l1_page_table:
+                    type = XEN_DOMCTL_PFINFO_L1TAB;
+                    break;
+                case PGT_l2_page_table:
+                    type = XEN_DOMCTL_PFINFO_L2TAB;
+                    break;
+                case PGT_l3_page_table:
+                    type = XEN_DOMCTL_PFINFO_L3TAB;
+                    break;
+                case PGT_l4_page_table:
+                    type = XEN_DOMCTL_PFINFO_L4TAB;
+                    break;
+                }
+
+                if ( page->u.inuse.type_info & PGT_pinned )
+                    type |= XEN_DOMCTL_PFINFO_LPINTAB;
+
+                if ( page->count_info & PGC_broken )
+                    type = XEN_DOMCTL_PFINFO_BROKEN;
+            }
+
+            if ( page )
+                put_page(page);
+
+            if ( __raw_copy_to_guest(guest_handle + (i * width), &type, width) )
+            {
+                ret = -EFAULT;
+                break;
+            }
+        }
+
+        break;
+    }
+
     case XEN_DOMCTL_hypercall_init:
     {
         unsigned long gmfn = domctl->u.hypercall_init.gmfn;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 0c0ea4a..5d15f2a 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -149,27 +149,6 @@ struct xen_domctl_getmemlist {
 #define XEN_DOMCTL_PFINFO_BROKEN  (0xdU<<28) /* broken page */
 #define XEN_DOMCTL_PFINFO_LTAB_MASK (0xfU<<28)
 
-struct xen_domctl_getpageframeinfo {
-    /* IN variables. */
-    uint64_aligned_t gmfn; /* GMFN to query */
-    /* OUT variables. */
-    /* Is the page PINNED to a type? */
-    uint32_t type;         /* see above type defs */
-};
-typedef struct xen_domctl_getpageframeinfo xen_domctl_getpageframeinfo_t;
-DEFINE_XEN_GUEST_HANDLE(xen_domctl_getpageframeinfo_t);
-
-
-/* XEN_DOMCTL_getpageframeinfo2 */
-struct xen_domctl_getpageframeinfo2 {
-    /* IN variables. */
-    uint64_aligned_t num;
-    /* IN/OUT variables. */
-    XEN_GUEST_HANDLE_64(uint32) array;
-};
-typedef struct xen_domctl_getpageframeinfo2 xen_domctl_getpageframeinfo2_t;
-DEFINE_XEN_GUEST_HANDLE(xen_domctl_getpageframeinfo2_t);
-
 /* XEN_DOMCTL_getpageframeinfo3 */
 struct xen_domctl_getpageframeinfo3 {
     /* IN variables. */
@@ -1073,8 +1052,8 @@ struct xen_domctl {
 #define XEN_DOMCTL_unpausedomain                  4
 #define XEN_DOMCTL_getdomaininfo                  5
 #define XEN_DOMCTL_getmemlist                     6
-#define XEN_DOMCTL_getpageframeinfo               7
-#define XEN_DOMCTL_getpageframeinfo2              8
+/* #define XEN_DOMCTL_getpageframeinfo            7 Obsolete - use getpageframeinfo3 */
+/* #define XEN_DOMCTL_getpageframeinfo2           8 Obsolete - use getpageframeinfo3 */
 #define XEN_DOMCTL_setvcpuaffinity                9
 #define XEN_DOMCTL_shadow_op                     10
 #define XEN_DOMCTL_max_mem                       11
@@ -1150,8 +1129,6 @@ struct xen_domctl {
         struct xen_domctl_createdomain      createdomain;
         struct xen_domctl_getdomaininfo     getdomaininfo;
         struct xen_domctl_getmemlist        getmemlist;
-        struct xen_domctl_getpageframeinfo  getpageframeinfo;
-        struct xen_domctl_getpageframeinfo2 getpageframeinfo2;
         struct xen_domctl_getpageframeinfo3 getpageframeinfo3;
         struct xen_domctl_nodeaffinity      nodeaffinity;
         struct xen_domctl_vcpuaffinity      vcpuaffinity;
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 11b7453..6e37d29 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -644,8 +644,6 @@ static int flask_domctl(struct domain *d, int cmd)
     case XEN_DOMCTL_setdebugging:
         return current_has_perm(d, SECCLASS_DOMAIN, DOMAIN__SETDEBUGGING);
 
-    case XEN_DOMCTL_getpageframeinfo:
-    case XEN_DOMCTL_getpageframeinfo2:
     case XEN_DOMCTL_getpageframeinfo3:
         return current_has_perm(d, SECCLASS_MMU, MMU__PAGEINFO);
 
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index ea556df..68284d5 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -339,7 +339,7 @@ class mmu
 #  source = domain making the hypercall
 #  target = domain whose pages are being mapped
     map_write
-# XEN_DOMCTL_getpageframeinfo*
+# XEN_DOMCTL_getpageframeinfo3
     pageinfo
 # XEN_DOMCTL_getmemlist
     pagelist
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] [RFC] x86/domctl: Fix getpageframeinfo* handling.
  2015-05-18 10:59 [PATCH] [RFC] x86/domctl: Fix getpageframeinfo* handling Andrew Cooper
@ 2015-05-18 11:43 ` Jan Beulich
  2015-05-18 13:24   ` Andrew Cooper
  0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2015-05-18 11:43 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Daniel De Graaf, Keir Fraser, Xen-devel

>>> On 18.05.15 at 12:59, <andrew.cooper3@citrix.com> wrote:
> In tree, there is one single caller of XEN_DOMCTL_getpageframeinfo3
> (xc_get_pfn_type_batch()), and no callers of the older variants.
> 
> getpageframeinfo3 and getpageframeinfo2 are compatible if the parameter
> contents are considered to be unsigned long; a compat guest calling
> getpageframeinfo3 falls through into the getpageframeinfo2 handler.
> 
> However, getpageframeinfo3 and getpageframeinfo2 have different algorithms 
> for
> calculating the eventual frame type, which means that a toolstack will get
> different answers depending on whether it is compat or not, which is a 
> problem
> for all possible uses.

Is there any other difference besides the former being capable of
returning XEN_DOMCTL_PFINFO_BROKEN (which I would suppose to
have been a later addition that didn't get properly sync-ed to the
older handlers)?

> @@ -378,6 +147,81 @@ long arch_do_domctl(
>          break;
>      }
>  
> +    case XEN_DOMCTL_getpageframeinfo3:
> +    {
> +        unsigned int num = domctl->u.getpageframeinfo3.num;
> +
> +        /* Games to allow this code block to handle a compat guest. */
> +        void * __user guest_handle = domctl->u.getpageframeinfo3.array.p;

The __used belongs between "void" and "*".

Also the blank line above looks somewhat misplaced (I guess you
added it to kind of emphasize the comment).

> +        unsigned int width = has_32bit_shinfo(currd) ? 32 : 64;

These are bit counts, yet where you use the value you want byte
granularity.

> +        if ( unlikely(num > 1024) ||
> +             unlikely(num != domctl->u.getpageframeinfo3.num) )
> +        {
> +            ret = -E2BIG;
> +            break;
> +        }
> +
> +        for ( i = 0; i < num; ++i )
> +        {
> +            unsigned long gfn = 0, type = 0;

gfn's initializer looks pointless (and if anything it should be INVALID_MFN
or some such).

> +            struct page_info *page;
> +            p2m_type_t t;
> +
> +            if ( raw_copy_from_guest(&gfn, guest_handle + (i * width), width) )
> +            {
> +                ret = -EFAULT;
> +                break;
> +            }
> +
> +            page = get_page_from_gfn(d, gfn, &t, P2M_ALLOC);
> +
> +            if ( unlikely(!page) ||
> +                 unlikely(is_xen_heap_page(page)) )
> +            {
> +                if ( p2m_is_broken(t) )
> +                    type = XEN_DOMCTL_PFINFO_BROKEN;
> +                else
> +                    type = XEN_DOMCTL_PFINFO_XTAB;

Realizing that this was this way in the old code too, would you
nevertheless mind adding unlikely() and/or flip the if and else
branches?

Jan

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] [RFC] x86/domctl: Fix getpageframeinfo* handling.
  2015-05-18 11:43 ` Jan Beulich
@ 2015-05-18 13:24   ` Andrew Cooper
  2015-05-18 13:51     ` Jan Beulich
  0 siblings, 1 reply; 4+ messages in thread
From: Andrew Cooper @ 2015-05-18 13:24 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Daniel De Graaf, Keir Fraser, Xen-devel

On 18/05/15 12:43, Jan Beulich wrote:
>>>> On 18.05.15 at 12:59, <andrew.cooper3@citrix.com> wrote:
>> In tree, there is one single caller of XEN_DOMCTL_getpageframeinfo3
>> (xc_get_pfn_type_batch()), and no callers of the older variants.
>>
>> getpageframeinfo3 and getpageframeinfo2 are compatible if the parameter
>> contents are considered to be unsigned long; a compat guest calling
>> getpageframeinfo3 falls through into the getpageframeinfo2 handler.
>>
>> However, getpageframeinfo3 and getpageframeinfo2 have different algorithms 
>> for
>> calculating the eventual frame type, which means that a toolstack will get
>> different answers depending on whether it is compat or not, which is a 
>> problem
>> for all possible uses.
> Is there any other difference besides the former being capable of
> returning XEN_DOMCTL_PFINFO_BROKEN (which I would suppose to
> have been a later addition that didn't get properly sync-ed to the
> older handlers)?

That was the only difference I could spot, but any difference is a problem.

>
>> @@ -378,6 +147,81 @@ long arch_do_domctl(
>>          break;
>>      }
>>  
>> +    case XEN_DOMCTL_getpageframeinfo3:
>> +    {
>> +        unsigned int num = domctl->u.getpageframeinfo3.num;
>> +
>> +        /* Games to allow this code block to handle a compat guest. */
>> +        void * __user guest_handle = domctl->u.getpageframeinfo3.array.p;
> The __used belongs between "void" and "*".
>
> Also the blank line above looks somewhat misplaced (I guess you
> added it to kind of emphasize the comment).

I think it ended up like this more by accident, but the emphasis is
important.  I will shuffle the width up.

>
>> +        unsigned int width = has_32bit_shinfo(currd) ? 32 : 64;
> These are bit counts, yet where you use the value you want byte
> granularity.

So they are.  I am surprised that this didn't blow up.

> 32bit unsigned long into it
>> +        if ( unlikely(num > 1024) ||
>> +             unlikely(num != domctl->u.getpageframeinfo3.num) )
>> +        {
>> +            ret = -E2BIG;
>> +            break;
>> +        }
>> +
>> +        for ( i = 0; i < num; ++i )
>> +        {
>> +            unsigned long gfn = 0, type = 0;
> gfn's initializer looks pointless (and if anything it should be INVALID_MFN
> or some such).

It must absolutely be 0 for when we read a 32bit values into it,
although I realise I do need to extend ~0U to ~0UL for compat guests.

>
>> +            struct page_info *page;
>> +            p2m_type_t t;
>> +
>> +            if ( raw_copy_from_guest(&gfn, guest_handle + (i * width), width) )
>> +            {
>> +                ret = -EFAULT;
>> +                break;
>> +            }
>> +
>> +            page = get_page_from_gfn(d, gfn, &t, P2M_ALLOC);
>> +
>> +            if ( unlikely(!page) ||
>> +                 unlikely(is_xen_heap_page(page)) )
>> +            {
>> +                if ( p2m_is_broken(t) )
>> +                    type = XEN_DOMCTL_PFINFO_BROKEN;
>> +                else
>> +                    type = XEN_DOMCTL_PFINFO_XTAB;
> Realizing that this was this way in the old code too, would you
> nevertheless mind adding unlikely() and/or flip the if and else
> branches?

Certainly.

Does this mean that you are happy in principle with the raw_* use?

~Andrew

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] [RFC] x86/domctl: Fix getpageframeinfo* handling.
  2015-05-18 13:24   ` Andrew Cooper
@ 2015-05-18 13:51     ` Jan Beulich
  0 siblings, 0 replies; 4+ messages in thread
From: Jan Beulich @ 2015-05-18 13:51 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Daniel De Graaf, Keir Fraser, Xen-devel

>>> On 18.05.15 at 15:24, <andrew.cooper3@citrix.com> wrote:
> On 18/05/15 12:43, Jan Beulich wrote:
>>>>> On 18.05.15 at 12:59, <andrew.cooper3@citrix.com> wrote:
>>> +        if ( unlikely(num > 1024) ||
>>> +             unlikely(num != domctl->u.getpageframeinfo3.num) )
>>> +        {
>>> +            ret = -E2BIG;
>>> +            break;
>>> +        }
>>> +
>>> +        for ( i = 0; i < num; ++i )
>>> +        {
>>> +            unsigned long gfn = 0, type = 0;
>> gfn's initializer looks pointless (and if anything it should be INVALID_MFN
>> or some such).
> 
> It must absolutely be 0 for when we read a 32bit values into it,

Ah, of course!

> although I realise I do need to extend ~0U to ~0UL for compat guests.

Why is ~0 special here? It's not even valid input afaict, and hence
there's no point in massaging it in any way.

>>> +            struct page_info *page;
>>> +            p2m_type_t t;
>>> +
>>> +            if ( raw_copy_from_guest(&gfn, guest_handle + (i * width), width) )
>>> +            {
>>> +                ret = -EFAULT;
>>> +                break;
>>> +            }
>>> +
>>> +            page = get_page_from_gfn(d, gfn, &t, P2M_ALLOC);
>>> +
>>> +            if ( unlikely(!page) ||
>>> +                 unlikely(is_xen_heap_page(page)) )
>>> +            {
>>> +                if ( p2m_is_broken(t) )
>>> +                    type = XEN_DOMCTL_PFINFO_BROKEN;
>>> +                else
>>> +                    type = XEN_DOMCTL_PFINFO_XTAB;
>> Realizing that this was this way in the old code too, would you
>> nevertheless mind adding unlikely() and/or flip the if and else
>> branches?
> 
> Certainly.
> 
> Does this mean that you are happy in principle with the raw_* use?

I'm not particularly happy about it, but for the purpose of code
consolidation I think it is acceptable here.

Jan

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2015-05-18 13:51 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-18 10:59 [PATCH] [RFC] x86/domctl: Fix getpageframeinfo* handling Andrew Cooper
2015-05-18 11:43 ` Jan Beulich
2015-05-18 13:24   ` Andrew Cooper
2015-05-18 13:51     ` Jan Beulich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.