* [PATCH 1/3] Add new functions to get/set memory types.
@ 2016-03-16 12:21 Yu Zhang
2016-03-22 11:31 ` George Dunlap
0 siblings, 1 reply; 3+ messages in thread
From: Yu Zhang @ 2016-03-16 12:21 UTC (permalink / raw)
To: xen-devel
Cc: kevin.tian, keir, andrew.cooper3, Paul.Durrant, zhiyuan.lv, jbeulich
For clarity this patch breaks the code to set/get memory types out
of do_hvm_op() into dedicated functions: hvmop_set/get_mem_type().
Also, for clarity, checks for whether a memory type change is allowed
are broken out into a separate function called by hvmop_set_mem_type().
There is no intentional functional change in this patch.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
---
xen/arch/x86/hvm/hvm.c | 280 +++++++++++++++++++++++++++----------------------
1 file changed, 155 insertions(+), 125 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 255a1d6..3ccd33f 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -6557,6 +6557,57 @@ static int do_altp2m_op(
return rc;
}
+static int hvmop_get_mem_type(
+ XEN_GUEST_HANDLE_PARAM(xen_hvm_get_mem_type_t) arg)
+{
+ struct xen_hvm_get_mem_type a;
+ struct domain *d;
+ p2m_type_t t;
+ int rc;
+
+ if ( copy_from_guest(&a, arg, 1) )
+ return -EFAULT;
+
+ d = rcu_lock_domain_by_any_id(a.domid);
+ if ( d == NULL )
+ return -ESRCH;
+
+ rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_get_mem_type);
+ if ( rc )
+ goto out;
+
+ rc = -EINVAL;
+ if ( is_hvm_domain(d) )
+ {
+ /*
+ * Use get_gfn query as we are interested in the current
+ * type, not in allocating or unsharing. That'll happen
+ * on access.
+ */
+ get_gfn_query_unlocked(d, a.pfn, &t);
+ if ( p2m_is_mmio(t) )
+ a.mem_type = HVMMEM_mmio_dm;
+ else if ( t == p2m_mmio_write_dm )
+ a.mem_type = HVMMEM_mmio_write_dm;
+ else if ( p2m_is_readonly(t) )
+ a.mem_type = HVMMEM_ram_ro;
+ else if ( p2m_is_ram(t) )
+ a.mem_type = HVMMEM_ram_rw;
+ else if ( p2m_is_pod(t) )
+ a.mem_type = HVMMEM_ram_rw;
+ else if ( p2m_is_grant(t) )
+ a.mem_type = HVMMEM_ram_rw;
+ else
+ a.mem_type = HVMMEM_mmio_dm;
+ rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+ }
+
+ out:
+ rcu_unlock_domain(d);
+
+ return rc;
+}
+
/*
* Note that this value is effectively part of the ABI, even if we don't need
* to make it a formal part of it: A guest suspended for migration in the
@@ -6565,6 +6616,105 @@ static int do_altp2m_op(
*/
#define HVMOP_op_mask 0xff
+static bool_t hvm_allow_p2m_type_change(p2m_type_t old, p2m_type_t new)
+{
+ if ( p2m_is_ram(old) ||
+ (p2m_is_hole(old) && new == p2m_mmio_dm) ||
+ (old == p2m_mmio_write_dm && new == p2m_ram_rw) )
+ return 1;
+
+ return 0;
+}
+
+static int hvmop_set_mem_type(
+ XEN_GUEST_HANDLE_PARAM(xen_hvm_set_mem_type_t) arg,
+ unsigned long *iter)
+{
+ unsigned long start_iter = *iter;
+ struct xen_hvm_set_mem_type a;
+ struct domain *d;
+ int rc;
+
+ /* Interface types to internal p2m types */
+ static const p2m_type_t memtype[] = {
+ [HVMMEM_ram_rw] = p2m_ram_rw,
+ [HVMMEM_ram_ro] = p2m_ram_ro,
+ [HVMMEM_mmio_dm] = p2m_mmio_dm,
+ [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
+ };
+
+ if ( copy_from_guest(&a, arg, 1) )
+ return -EFAULT;
+
+ rc = rcu_lock_remote_domain_by_id(a.domid, &d);
+ if ( rc != 0 )
+ return rc;
+
+ rc = -EINVAL;
+ if ( !is_hvm_domain(d) )
+ goto out;
+
+ rc = xsm_hvm_control(XSM_DM_PRIV, d, HVMOP_set_mem_type);
+ if ( rc )
+ goto out;
+
+ rc = -EINVAL;
+ if ( a.nr < start_iter ||
+ ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+ ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
+ goto out;
+
+ if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
+ goto out;
+
+ while ( a.nr > start_iter )
+ {
+ unsigned long pfn = a.first_pfn + start_iter;
+ p2m_type_t t;
+
+ get_gfn_unshare(d, pfn, &t);
+ if ( p2m_is_paging(t) )
+ {
+ put_gfn(d, pfn);
+ p2m_mem_paging_populate(d, pfn);
+ rc = -EAGAIN;
+ goto out;
+ }
+ if ( p2m_is_shared(t) )
+ {
+ put_gfn(d, pfn);
+ rc = -EAGAIN;
+ goto out;
+ }
+ if ( !hvm_allow_p2m_type_change(t, memtype[a.hvmmem_type]) )
+ {
+ put_gfn(d, pfn);
+ goto out;
+ }
+
+ rc = p2m_change_type_one(d, pfn, t, memtype[a.hvmmem_type]);
+ put_gfn(d, pfn);
+
+ if ( rc )
+ goto out;
+
+ /* Check for continuation if it's not the last interation */
+ if ( a.nr > ++start_iter && !(start_iter & HVMOP_op_mask) &&
+ hypercall_preempt_check() )
+ {
+ rc = -ERESTART;
+ goto out;
+ }
+ }
+ rc = 0;
+
+ out:
+ rcu_unlock_domain(d);
+ *iter = start_iter;
+
+ return rc;
+}
+
long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
{
unsigned long start_iter, mask;
@@ -6754,135 +6904,15 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
}
case HVMOP_get_mem_type:
- {
- struct xen_hvm_get_mem_type a;
- struct domain *d;
- p2m_type_t t;
-
- if ( copy_from_guest(&a, arg, 1) )
- return -EFAULT;
-
- d = rcu_lock_domain_by_any_id(a.domid);
- if ( d == NULL )
- return -ESRCH;
-
- rc = xsm_hvm_param(XSM_TARGET, d, op);
- if ( unlikely(rc) )
- /* nothing */;
- else if ( likely(is_hvm_domain(d)) )
- {
- /* Use get_gfn query as we are interested in the current
- * type, not in allocating or unsharing. That'll happen
- * on access. */
- get_gfn_query_unlocked(d, a.pfn, &t);
- if ( p2m_is_mmio(t) )
- a.mem_type = HVMMEM_mmio_dm;
- else if ( t == p2m_mmio_write_dm )
- a.mem_type = HVMMEM_mmio_write_dm;
- else if ( p2m_is_readonly(t) )
- a.mem_type = HVMMEM_ram_ro;
- else if ( p2m_is_ram(t) )
- a.mem_type = HVMMEM_ram_rw;
- else if ( p2m_is_pod(t) )
- a.mem_type = HVMMEM_ram_rw;
- else if ( p2m_is_grant(t) )
- a.mem_type = HVMMEM_ram_rw;
- else
- a.mem_type = HVMMEM_mmio_dm;
- if ( __copy_to_guest(arg, &a, 1) )
- rc = -EFAULT;
- }
- else
- rc = -EINVAL;
-
- rcu_unlock_domain(d);
+ rc = hvmop_get_mem_type(
+ guest_handle_cast(arg, xen_hvm_get_mem_type_t));
break;
- }
case HVMOP_set_mem_type:
- {
- struct xen_hvm_set_mem_type a;
- struct domain *d;
-
- /* Interface types to internal p2m types */
- static const p2m_type_t memtype[] = {
- [HVMMEM_ram_rw] = p2m_ram_rw,
- [HVMMEM_ram_ro] = p2m_ram_ro,
- [HVMMEM_mmio_dm] = p2m_mmio_dm,
- [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
- };
-
- if ( copy_from_guest(&a, arg, 1) )
- return -EFAULT;
-
- rc = rcu_lock_remote_domain_by_id(a.domid, &d);
- if ( rc != 0 )
- return rc;
-
- rc = -EINVAL;
- if ( !is_hvm_domain(d) )
- goto setmemtype_fail;
-
- rc = xsm_hvm_control(XSM_DM_PRIV, d, op);
- if ( rc )
- goto setmemtype_fail;
-
- rc = -EINVAL;
- if ( a.nr < start_iter ||
- ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
- ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
- goto setmemtype_fail;
-
- if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
- goto setmemtype_fail;
-
- while ( a.nr > start_iter )
- {
- unsigned long pfn = a.first_pfn + start_iter;
- p2m_type_t t;
-
- get_gfn_unshare(d, pfn, &t);
- if ( p2m_is_paging(t) )
- {
- put_gfn(d, pfn);
- p2m_mem_paging_populate(d, pfn);
- rc = -EAGAIN;
- goto setmemtype_fail;
- }
- if ( p2m_is_shared(t) )
- {
- put_gfn(d, pfn);
- rc = -EAGAIN;
- goto setmemtype_fail;
- }
- if ( !p2m_is_ram(t) &&
- (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) &&
- (t != p2m_mmio_write_dm || a.hvmmem_type != HVMMEM_ram_rw) )
- {
- put_gfn(d, pfn);
- goto setmemtype_fail;
- }
-
- rc = p2m_change_type_one(d, pfn, t, memtype[a.hvmmem_type]);
- put_gfn(d, pfn);
- if ( rc )
- goto setmemtype_fail;
-
- /* Check for continuation if it's not the last interation */
- if ( a.nr > ++start_iter && !(start_iter & HVMOP_op_mask) &&
- hypercall_preempt_check() )
- {
- rc = -ERESTART;
- goto setmemtype_fail;
- }
- }
-
- rc = 0;
-
- setmemtype_fail:
- rcu_unlock_domain(d);
+ rc = hvmop_set_mem_type(
+ guest_handle_cast(arg, xen_hvm_set_mem_type_t),
+ &start_iter);
break;
- }
case HVMOP_pagetable_dying:
{
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/3] Add new functions to get/set memory types.
2016-03-16 12:21 [PATCH 1/3] Add new functions to get/set memory types Yu Zhang
@ 2016-03-22 11:31 ` George Dunlap
2016-03-23 6:30 ` Yu, Zhang
0 siblings, 1 reply; 3+ messages in thread
From: George Dunlap @ 2016-03-22 11:31 UTC (permalink / raw)
To: Yu Zhang
Cc: Tian, Kevin, Keir Fraser, Andrew Cooper, xen-devel, Paul Durrant,
Lv, Zhiyuan, Jan Beulich
On Wed, Mar 16, 2016 at 12:21 PM, Yu Zhang <yu.c.zhang@linux.intel.com> wrote:
> For clarity this patch breaks the code to set/get memory types out
> of do_hvm_op() into dedicated functions: hvmop_set/get_mem_type().
> Also, for clarity, checks for whether a memory type change is allowed
> are broken out into a separate function called by hvmop_set_mem_type().
>
> There is no intentional functional change in this patch.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> ---
> xen/arch/x86/hvm/hvm.c | 280 +++++++++++++++++++++++++++----------------------
> 1 file changed, 155 insertions(+), 125 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 255a1d6..3ccd33f 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -6557,6 +6557,57 @@ static int do_altp2m_op(
> return rc;
> }
>
> +static int hvmop_get_mem_type(
> + XEN_GUEST_HANDLE_PARAM(xen_hvm_get_mem_type_t) arg)
> +{
> + struct xen_hvm_get_mem_type a;
> + struct domain *d;
> + p2m_type_t t;
> + int rc;
> +
> + if ( copy_from_guest(&a, arg, 1) )
> + return -EFAULT;
> +
> + d = rcu_lock_domain_by_any_id(a.domid);
> + if ( d == NULL )
> + return -ESRCH;
> +
> + rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_get_mem_type);
> + if ( rc )
> + goto out;
> +
> + rc = -EINVAL;
> + if ( is_hvm_domain(d) )
> + {
> + /*
> + * Use get_gfn query as we are interested in the current
> + * type, not in allocating or unsharing. That'll happen
> + * on access.
> + */
> + get_gfn_query_unlocked(d, a.pfn, &t);
> + if ( p2m_is_mmio(t) )
> + a.mem_type = HVMMEM_mmio_dm;
> + else if ( t == p2m_mmio_write_dm )
> + a.mem_type = HVMMEM_mmio_write_dm;
> + else if ( p2m_is_readonly(t) )
> + a.mem_type = HVMMEM_ram_ro;
> + else if ( p2m_is_ram(t) )
> + a.mem_type = HVMMEM_ram_rw;
> + else if ( p2m_is_pod(t) )
> + a.mem_type = HVMMEM_ram_rw;
> + else if ( p2m_is_grant(t) )
> + a.mem_type = HVMMEM_ram_rw;
> + else
> + a.mem_type = HVMMEM_mmio_dm;
> + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
If you're going to be changing things rather than just pure code
motion, it would probably be better to make this follow the normal
"set rc / do something / goto out" pattern, rather than inventing a
new one:
rc = -EFAULT
if ( __copy_to_guest())
goto out;
rc = 0;
I suppose ideally this whole if() block would be rewritten this way as well:
rc = -EINVAL
if ( !is_hvm_domain() )
goto out;
[stuff currently in the if-block]
But that's probably too much for a patch we want to be mostly code-motion.
FWIW I've taken a close look and the functionality looks identical
before and after this patch.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/3] Add new functions to get/set memory types.
2016-03-22 11:31 ` George Dunlap
@ 2016-03-23 6:30 ` Yu, Zhang
0 siblings, 0 replies; 3+ messages in thread
From: Yu, Zhang @ 2016-03-23 6:30 UTC (permalink / raw)
To: George Dunlap
Cc: Tian, Kevin, Keir Fraser, Andrew Cooper, xen-devel, Paul Durrant,
Lv, Zhiyuan, Jan Beulich
Hi George,
Thanks a lot for your thorough review. :)
On 3/22/2016 7:31 PM, George Dunlap wrote:
> On Wed, Mar 16, 2016 at 12:21 PM, Yu Zhang <yu.c.zhang@linux.intel.com> wrote:
>> For clarity this patch breaks the code to set/get memory types out
>> of do_hvm_op() into dedicated functions: hvmop_set/get_mem_type().
>> Also, for clarity, checks for whether a memory type change is allowed
>> are broken out into a separate function called by hvmop_set_mem_type().
>>
>> There is no intentional functional change in this patch.
>>
>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>> ---
>> xen/arch/x86/hvm/hvm.c | 280 +++++++++++++++++++++++++++----------------------
>> 1 file changed, 155 insertions(+), 125 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 255a1d6..3ccd33f 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -6557,6 +6557,57 @@ static int do_altp2m_op(
>> return rc;
>> }
>>
>> +static int hvmop_get_mem_type(
>> + XEN_GUEST_HANDLE_PARAM(xen_hvm_get_mem_type_t) arg)
>> +{
>> + struct xen_hvm_get_mem_type a;
>> + struct domain *d;
>> + p2m_type_t t;
>> + int rc;
>> +
>> + if ( copy_from_guest(&a, arg, 1) )
>> + return -EFAULT;
>> +
>> + d = rcu_lock_domain_by_any_id(a.domid);
>> + if ( d == NULL )
>> + return -ESRCH;
>> +
>> + rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_get_mem_type);
>> + if ( rc )
>> + goto out;
>> +
>> + rc = -EINVAL;
>> + if ( is_hvm_domain(d) )
>> + {
>> + /*
>> + * Use get_gfn query as we are interested in the current
>> + * type, not in allocating or unsharing. That'll happen
>> + * on access.
>> + */
>> + get_gfn_query_unlocked(d, a.pfn, &t);
>> + if ( p2m_is_mmio(t) )
>> + a.mem_type = HVMMEM_mmio_dm;
>> + else if ( t == p2m_mmio_write_dm )
>> + a.mem_type = HVMMEM_mmio_write_dm;
>> + else if ( p2m_is_readonly(t) )
>> + a.mem_type = HVMMEM_ram_ro;
>> + else if ( p2m_is_ram(t) )
>> + a.mem_type = HVMMEM_ram_rw;
>> + else if ( p2m_is_pod(t) )
>> + a.mem_type = HVMMEM_ram_rw;
>> + else if ( p2m_is_grant(t) )
>> + a.mem_type = HVMMEM_ram_rw;
>> + else
>> + a.mem_type = HVMMEM_mmio_dm;
>> + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>
> If you're going to be changing things rather than just pure code
> motion, it would probably be better to make this follow the normal
> "set rc / do something / goto out" pattern, rather than inventing a
> new one:
>
> rc = -EFAULT
> if ( __copy_to_guest())
> goto out;
>
> rc = 0;
>
> I suppose ideally this whole if() block would be rewritten this way as well:
>
> rc = -EINVAL
> if ( !is_hvm_domain() )
> goto out;
>
> [stuff currently in the if-block]
>
> But that's probably too much for a patch we want to be mostly code-motion.
>
> FWIW I've taken a close look and the functionality looks identical
> before and after this patch.
Well, this patch is only code-motion. :)
The "set rc/ do sth. / goto out" pattern sounds reasonable, and I'd like
to take this change in next version.
>
> -George
>
B.R.
Yu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-03-23 6:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-16 12:21 [PATCH 1/3] Add new functions to get/set memory types Yu Zhang
2016-03-22 11:31 ` George Dunlap
2016-03-23 6:30 ` Yu, Zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).