Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits

[Jan Beulich <jbeulich@suse.com>]

On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
> @@ -4681,7 +4682,7 @@ static int do_altp2m_op(
>          break;
>  
>      case HVMOP_altp2m_set_suppress_ve:
> -        if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
> +        if ( a.u.suppress_ve.pad1 )

Just because the field changes its name doesn't mean you can
drop the check. You even add a new field not used (yet) by
this sub-function, which then also would need checking here.

> @@ -4693,8 +4694,23 @@ static int do_altp2m_op(
>          }
>          break;
>  
> +    case HVMOP_altp2m_set_suppress_ve_multi:
> +        if ( a.u.suppress_ve.pad1 || !a.u.suppress_ve.nr )

A count of zero typically is taken as a no-op, not an error.

> +            rc = -EINVAL;
> +        else
> +        {
> +            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve);
> +
> +            if ( rc == -ERESTART )
> +                if ( __copy_field_to_guest(guest_handle_cast(arg,
> +                                           xen_hvm_altp2m_op_t),
> +                                           &a, u.suppress_ve.opaque) )
> +                    rc = -EFAULT;

If the operation is best effort, _some_ indication of failure should
still be handed back to the caller. Whether that's through the opaque
field or by some other means is secondary. If not via that field
(which would make the outer of the two if()-s disappear), please fold
the if()-s.

> +        }
> +        break;
> +
>      case HVMOP_altp2m_get_suppress_ve:
> -        if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
> +        if ( a.u.suppress_ve.pad1 )

See above.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -3054,6 +3054,64 @@ out:
>      return rc;
>  }
>  
> +/*
> + * Set/clear the #VE suppress bit for multiple pages.  Only available on VMX.
> + */
> +int p2m_set_suppress_ve_multi(struct domain *d,
> +                              struct xen_hvm_altp2m_suppress_ve* sve)

Misplaced *.

> +{
> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *ap2m = NULL;
> +    struct p2m_domain *p2m;
> +    uint64_t start = sve->opaque ?: sve->gfn;

According to this start (and hence ->opaque) are GFNs.

> +    int rc = 0;
> +
> +    if ( sve->view > 0 )
> +    {
> +        if ( sve->view >= MAX_ALTP2M ||
> +             d->arch.altp2m_eptp[sve->view] == mfn_x(INVALID_MFN) )
> +            return -EINVAL;
> +
> +        p2m = ap2m = d->arch.altp2m_p2m[sve->view];
> +    }
> +    else
> +        p2m = host_p2m;
> +
> +    p2m_lock(host_p2m);
> +
> +    if ( ap2m )
> +        p2m_lock(ap2m);
> +
> +
> +    while ( start < sve->nr )

According to this, start is an index. Which of the two do you
mean?

> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -42,8 +42,9 @@ struct xen_hvm_altp2m_suppress_ve {
>      uint16_t view;
>      uint8_t suppress_ve; /* Boolean type. */
>      uint8_t pad1;
> -    uint32_t pad2;
> +    uint32_t nr;
>      uint64_t gfn;
> +    uint64_t opaque;
>  };

How is this addition of a field going to work compatibly with old
and new callers on old and new hypervisors? Recall in particular
that these operations are (almost?) all potentially usable by the
guest itself.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel