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

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

On 2014/8/8 23:45, Jan Beulich wrote:
> > > > 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?
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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel