Re: [PATCH v7 11/15] x86/altp2m: define and implement alternate p2m HVMOP types.

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

>>> On 23.07.15 at 01:01, <edmund.h.white@intel.com> wrote:
> Signed-off-by: Ed White <edmund.h.white@intel.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>

And I have to withdraw this ack pending clarification of (and perhaps
adjustment to) the #VE info address interface.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -6138,6 +6138,140 @@ static int hvmop_get_param(
>      return rc;
>  }
>  
> +static int do_altp2m_op(
> +    XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> +    struct xen_hvm_altp2m_op a;
> +    struct domain *d = NULL;
> +    int rc = 0;
> +
> +    if ( !hvm_altp2m_supported() )
> +        return -EOPNOTSUPP;
> +
> +    if ( copy_from_guest(&a, arg, 1) )
> +        return -EFAULT;
> +
> +    if ( a.pad1 || a.pad2 ||
> +         (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) ||
> +         (a.cmd < HVMOP_altp2m_get_domain_state) ||
> +         (a.cmd > HVMOP_altp2m_change_gfn) )

I'm afraid such a change invalidates any earlier ack, even if ti is
correct. Instead of this, why don't you start numbering of the
sub-ops at zero? Or if you really have a reason to start at 1,
why not simply check a.cmd against zero (without using any
particular sub-op value)? And then it escapes me why this can't
be handled in a default case in the switch statement below
anyway.

Jan