Re: [PATCH 1/3] Add new functions to get/set memory types.

[George Dunlap <dunlapg@umich.edu>]

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