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

["Yu, Zhang" <yu.c.zhang@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Yu Zhang <yu.c.zhang@xxxxxxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel