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.zh...@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.durr...@citrix.com> Signed-off-by: Yu Zhang <yu.c.zh...@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
