Re: [RFC][PATCH 2/5] xen:x86: introduce a new hypercall to get RMRR mappings

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2014/8/8 23:45, Jan Beulich wrote:
>>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>> +    case XENMEM_RMRR_memory_map:
>> +    {
>> +        struct memory_map_context ctxt;
>
> ???
>
>> +        XEN_GUEST_HANDLE(e820entry_t) buffer;
>> +        XEN_GUEST_HANDLE_PARAM(e820entry_t) buffer_param;
>> +        unsigned int i;
>> +
>> +        rc = xsm_machine_memory_map(XSM_PRIV);
>
> Are you sure? Can (and should) this really not be exposed to semi-
> privileged domains?

Will fixed.

>
>> +        if ( rc )
>> +            return rc;
>> +
>> +        if ( copy_from_guest(&ctxt.map, arg, 1) )
>> +            return -EFAULT;
>> +        if ( ctxt.map.nr_entries < rmrr_e820.nr_map + 1 )
>> +            return -EINVAL;
>
> So how would the caller know how many entries are needed?
>
>> +
>> +        buffer_param = guest_handle_cast(ctxt.map.buffer, e820entry_t);
>> +        buffer = guest_handle_from_param(buffer_param, e820entry_t);
>> +        if ( !guest_handle_okay(buffer, ctxt.map.nr_entries) )
>> +            return -EFAULT;
>> +
>> +        for ( i = 0, ctxt.n = 0, ctxt.s = 0; i < rmrr_e820.nr_map; ++i, ++ctxt.n )
>
> i and ctxt.n are redundant.
>
>> +        {
>> +            if ( __copy_to_guest_offset(buffer, ctxt.n, rmrr_e820.map + i, 1) )
>> +                return -EFAULT;
>> +        }
>> +
>> +        ctxt.map.nr_entries = ctxt.n;
>> +
>> +        if ( __copy_to_guest(arg, &ctxt.map, 1) )
>
> __copy_field_to_guest() if all you need to copy back is a single field.

I will try to address all comments in next revision.

>
>> --- a/xen/arch/x86/x86_64/compat/mm.c
>> +++ b/xen/arch/x86/x86_64/compat/mm.c
>> @@ -132,6 +132,14 @@ int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>           break;
>>       }
>>
>> +    case XENMEM_RMRR_memory_map:
>> +    {
>> +        if ( copy_to_guest(arg, &rmrr_e820, 1) )
>> +            return -EFAULT;
>> +
>> +        return 0;
>> +    }
>
> Pointless braces. And how come this is so much simpler than the
> native version?

Just hvmloader would walk this with a hypercall, and with a test I don't 
see any issue here.

If you think this is not correct, please comment this in next revision.

>
>> --- a/xen/include/public/memory.h
>> +++ b/xen/include/public/memory.h
>> @@ -523,7 +523,15 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
>>
>>   #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>>
>> -/* Next available subop number is 26 */
>> +/*
>> + * Returns the RMRR memory map as it was when the domain
>> + * was started.
>> + */
>> +#define XENMEM_RMRR_memory_map           26
>> +typedef struct e820map rmrr_e820_t;
>> +DEFINE_XEN_GUEST_HANDLE(rmrr_e820_t);
>
> Again just as a general remark: What in the world does the "e820"
> in here mean?

I will redefine a struct to represent this to avoid any confusion.

Thanks
Tiejun

>
> Jan
>
>