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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 12/23/19 2:04 PM, Alexandru Stefan ISAILA wrote:
> By default the sve bits are not set.
> This patch adds a new hypercall, xc_altp2m_set_supress_ve_multi(),
> to set a range of sve bits.
> The core function, p2m_set_suppress_ve_multi(), does not brake in case

*break

> of a error and it is doing a best effort for setting the bits in the
> given range. A check for continuation is made in order to have
> preemption on big ranges.

Weird English quirk: this should be "large".  ("Big" and "large" are
both adjectives, and "ranges" is a noun, so theoretically it should be
OK; but if you ask almost any native English speaker they'll say that
"big" sounds wrong in this case.  No real idea why.)

Both of these could be fixed on check-in.

> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 4fc919a9c5..de832dcc6d 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -3070,6 +3070,70 @@ 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_multi *sve)
> +{
> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *ap2m = NULL;
> +    struct p2m_domain *p2m = host_p2m;
> +    uint64_t start = sve->first_gfn;
> +    int rc = 0;
> +
> +    if ( sve->view > 0 )
> +    {
> +        if ( sve->view >= MAX_ALTP2M ||
> +             d->arch.altp2m_eptp[array_index_nospec(sve->view, MAX_ALTP2M)] 
> ==
> +             mfn_x(INVALID_MFN) )
> +            return -EINVAL;
> +
> +        p2m = ap2m = d->arch.altp2m_p2m[array_index_nospec(sve->view,
> +                                                           MAX_ALTP2M)];
> +    }
> +
> +    p2m_lock(host_p2m);
> +
> +    if ( ap2m )
> +        p2m_lock(ap2m);
> +
> +    while ( sve->last_gfn >= start )
> +    {
> +        p2m_access_t a;
> +        p2m_type_t t;
> +        mfn_t mfn;
> +        int err = 0;
> +
> +        if ( altp2m_get_effective_entry(p2m, _gfn(start), &mfn, &t, &a, 
> AP2MGET_query) )
> +            a = p2m->default_access;

So in the single-entry version, if altp2m_get_effective_entry() returns
an error, you pass that error up the stack; but in the multiple-entry
version, you ignore the error and simply set the access to
default_access?  I don't think that can be right.  If it is right, then
it definitely needs a comment.

This points out another issue: implementing this functionality twice
risks having this sort of drift between the single-entry version and the
multiple-entry version.  Would it make sense instead to implement the
single-entry version hypercall using p2m_set_suppress_ve_multi?

 -George

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