Re: [RFC][v3][PATCH 4/6] xen:x86: add XENMEM_reserved_device_memory_map to expose RMRR

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 15/08/14 09:27, Tiejun Chen wrote:
> We should expose RMRR mapping to libxc, then setup_guest() can
> check if current MMIO is not covered by any RMRR mapping.
>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  xen/arch/x86/mm.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index d23cb3f..fb6e92f 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4769,6 +4769,38 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          return 0;
>      }
>  
> +    case XENMEM_reserved_device_memory_map:
> +    {
> +        struct xen_memory_map map;
> +        XEN_GUEST_HANDLE(e820entry_t) buffer;
> +        XEN_GUEST_HANDLE_PARAM(e820entry_t) buffer_param;
> +        unsigned int i;
> +
> +        if ( copy_from_guest(&map, arg, 1) )
> +            return -EFAULT;

This hypercall implementation is looking somewhat more plausible, but
still has some issues.

> +        if ( map.nr_entries < rmrr_maps.nr_map + 1 )
> +            return -EINVAL;

This causes a fencepost error, does it not?

Furthermore, the useful error would be to return -ENOBUFS and fill
arg.nr_entries with the rmrr_maps.nr_map so the caller can allocate an
appropriately sized buffer.


It is also very common with hypercalls like this to have allow a null
guest handle as an explicit request for size.

See XEN_DOMCTL_get_vcpu_msrs as an (admittedly more complicated) example.

> +
> +        buffer_param = guest_handle_cast(map.buffer, e820entry_t);
> +        buffer = guest_handle_from_param(buffer_param, e820entry_t);
> +        if ( !guest_handle_okay(buffer, map.nr_entries) )
> +            return -EFAULT;
> +
> +        /* Currently we just need to cover RMRR. */
> +        for ( i = 0; i < rmrr_maps.nr_map; ++i )
> +        {
> +            if ( __copy_to_guest_offset(buffer, i, rmrr_maps.map + i, 1) )
> +                return -EFAULT;

What is wrong with a single copy_to_guest_offset() ?

> +        }
> +
> +        map.nr_entries = rmrr_maps.nr_map;
> +
> +        if ( __copy_to_guest(arg, &map, 1) )
> +            return -EFAULT;
> +
> +        return 0;
> +    }
> +
>      case XENMEM_machphys_mapping:
>      {
>          struct xen_machphys_mapping mapping = {