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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.08.14 at 13:02, <tiejun.chen@xxxxxxxxx> 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?

> +        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.

> --- 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?

> --- 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?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel