Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

["Jan Beulich" <JBeulich@suse.com>]

>>> On 26.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
> @@ -4842,6 +4843,54 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          return rc;
>      }
>  
> +    case XENMEM_reserved_device_memory_map:
> +    {
> +        struct xen_mem_reserved_device_memory_map map;
> +        XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
> +        XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t) buffer_param;
> +        unsigned int i = 0;
> +        static struct xen_mem_reserved_device_memory rmrr_map;
> +        struct acpi_rmrr_unit *rmrr;
> +
> +        if ( copy_from_guest(&map, arg, 1) )
> +            return -EFAULT;
> +
> +        if ( !acpi_rmrr_unit_entries )
> +                return -ENOENT;
> +
> +        if ( map.nr_entries < acpi_rmrr_unit_entries )
> +        {
> +            map.nr_entries = acpi_rmrr_unit_entries;
> +            if ( copy_to_guest(arg, &map, 1) )
> +                return -EFAULT;
> +            return -ENOBUFS;
> +        }
> +
> +        map.nr_entries = acpi_rmrr_unit_entries;
> +        buffer_param = guest_handle_cast(map.buffer,
> +                                         xen_mem_reserved_device_memory_t);
> +        buffer = guest_handle_from_param(buffer_param,
> +                                         xen_mem_reserved_device_memory_t);
> +        if ( !guest_handle_okay(buffer, map.nr_entries) )
> +            return -EFAULT;
> +
> +        list_for_each_entry( rmrr, &acpi_rmrr_units, list )
> +        {
> +            rmrr_map.start_pfn = rmrr->base_address >> PAGE_SHIFT;
> +            rmrr_map.nr_pages = PAGE_ALIGN(rmrr->end_address -
> +                                           rmrr->base_address) /
> +                                           PAGE_SIZE;
> +            if ( copy_to_guest_offset(buffer, i, &rmrr_map, 1) )
> +                return -EFAULT;
> +            i++;
> +        }

So as said before, a callback based mechanism will serve you quite
fine here. However, you're not obeying to the buffer size the
caller passed in (i.e. you may corrupt guest memory). And having
used guest_handle_okay() in the loop (which will get moved into
the callback function) you can then use __copy_to_guest_offset().

> +
> +        if ( copy_to_guest(arg, &map, 1) )
> +                return -EFAULT;

As much as, due to the earlier copy_from_guest() on the same
handle, you can use __copy_to_guest() here. For mechanical
things like these please consult other code - there are plenty of
useful examples throughout the tree.

> +struct xen_mem_reserved_device_memory_map {
> +    /*
> +     * On call the number of entries which can be stored in buffer. On
> +     * return the number of entries which have been stored in
> +     * buffer.
> +     */
> +    unsigned int nr_entries;
> +
> +    /*
> +     * Entries in the buffer are in the same format as
> +     * xen_mem_reserved_device_memory.
> +     */
> +    XEN_GUEST_HANDLE(void) buffer;

I think I had asked before that you use a properly typed handle here,
saving you from needing convoluted conversion code as you have
above.

Jan