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

["Yu, Zhang" <yu.c.zhang@linux.intel.com>]

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