Re: [Xen-devel] [PATCH v2] x86/HVM: Merge HVM and PVH hypercall tables

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.12.15 at 17:34, <boris.ostrovsky@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5188,12 +5188,21 @@ static hvm_hypercall_t *const 
> hvm_hypercall64_table[NR_hypercalls] = {
>      HYPERCALL(sysctl),
>      HYPERCALL(domctl),
>      HYPERCALL(tmem_op),
> +    HYPERCALL(platform_op),
> +    HYPERCALL(mmuext_op),
> +    HYPERCALL(xenpmu_op),
>      [ __HYPERVISOR_arch_1 ] = (hvm_hypercall_t *)paging_domctl_continuation
>  };
>  
>  #define COMPAT_CALL(x)                                        \
>      [ __HYPERVISOR_ ## x ] = (hvm_hypercall_t *) compat_ ## x
>  
> +extern int compat_mmuext_op(XEN_GUEST_HANDLE_PARAM(void) cmp_uops,
> +                            unsigned int count,
> +                            XEN_GUEST_HANDLE_PARAM(uint) pdone,
> +                            unsigned int foreigndom);
> +extern int compat_platform_op(XEN_GUEST_HANDLE_PARAM(void) u_xenpf_op);

While I realize that you only move the compat_mmuext_op()
declaration up, neither of the two belongs here. I have no idea why
I approved of you adding that declaration in this file in an earlier
change. Declarations have to be in a header included by the producer
and all consumers (and there are already a number of compat_ ones
in the headers iirc).

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3019,6 +3019,25 @@ long do_mmuext_op(
>              break;
>          }
>  
> +        if ( has_hvm_container_domain(d) )
> +        {
> +            switch ( op.cmd )
> +            {
> +            case MMUEXT_PIN_L1_TABLE:
> +            case MMUEXT_PIN_L2_TABLE:
> +            case MMUEXT_PIN_L3_TABLE:
> +            case MMUEXT_PIN_L4_TABLE:

If for nothing else then for consistency this should also include the
unpin counterpart.

> +                if ( is_control_domain(d) )
> +                    break;
> +                /* fallthrough */
> +            default:
> +                MEM_LOG("Invalid extended pt command %#x", op.cmd);
> +                okay = 0;

This is unnecessary due to ...

> +                rc = -ENOSYS;
> +                goto done;
> +            }
> +        }
> +
>          okay = 1;
>  
>          switch ( op.cmd )
> @@ -3448,6 +3467,7 @@ long do_mmuext_op(
>              break;
>          }
>  
> + done:
>          if ( unlikely(!okay) && !rc )
>              rc = -EINVAL;

... the conditional here.

Also I'd prefer if you used -EOPNOTSUPP instead of -ENOSYS.

Jan


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