Re: [Xen-devel] [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.09.16 at 10:51, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> Changes since V1 / RFC:
>  - Renamed xc_set_mem_access_sparse() to xc_set_mem_access_multi(),
>    and XENMEM_access_op_set_access_sparse to
>    XENMEM_access_op_set_access_multi.
>  - Renamed the 'nr' parameter to 'size'.

Why?

>  - Wrapped long line in the implementation of xc_set_mem_access_multi().
>  - Factored out common code in _p2m_set_mem_access() (and modified
>    p2m_set_altp2m_mem_access() in the process, to take an unsigned
>    long argument instead of a gfn_t).
>  - Factored out xenmem_access_t to p2m_access_t conversion code in
>    p2m_xenmem_access_to_p2m_access().
>  - Added hypercall continuation support.
>  - Added compat translation support.
>  - No longer allocating buffers (now using copy_from_guest_offset()).
>  - Added support for setting an array of access restrictions, as
>    suggested by Tamas Lengyel.
>  - This patch incorporates Jan Beulich's "memory: fix compat handling
>    of XENMEM_access_op".

I'm not sure that's okay; I'd have preferred you make the other
patch a prereq (not the least because that one may want
backporting, while this one certainly won't). In no event should
such a bug fix go without mentioning in the commit message.

> @@ -1772,13 +1773,12 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned 
> long gla,
>  static inline
>  int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>                                struct p2m_domain *ap2m, p2m_access_t a,
> -                              gfn_t gfn)
> +                              unsigned long gfn_l)
>  {

I'm not really happy about such a step backwards.

> @@ -1811,21 +1811,39 @@ int p2m_set_altp2m_mem_access(struct domain *d, 
> struct p2m_domain *hp2m,
>                           (current->domain != d));
>  }
>  
> -/*
> - * Set access type for a region of gfns.
> - * If gfn == INVALID_GFN, sets the default access type.
> - */
> -long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
> -                        uint32_t start, uint32_t mask, xenmem_access_t 
> access,
> -                        unsigned int altp2m_idx)
> +static inline
> +int _p2m_set_mem_access(struct domain *d, struct p2m_domain *p2m,

Considering the file we're in, can't this just be set_mem_access()?

Also I'd leave the inlining decision completely to the compiler.

> +                        struct p2m_domain *ap2m, p2m_access_t a,
> +                        unsigned long gfn_l)
>  {
> -    struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL;
> -    p2m_access_t a, _a;
> -    p2m_type_t t;
> -    mfn_t mfn;
> -    unsigned long gfn_l;
> -    long rc = 0;
> +    int rc;
>  
> +    if ( ap2m )
> +    {
> +        rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, gfn_l);
> +        /* If the corresponding mfn is invalid we will want to just skip it 
> */
> +        if ( rc && rc != -ESRCH )
> +            return rc;

If you just converted -ESRCH to 0 here, you could then ...

> +    }
> +    else
> +    {
> +        mfn_t mfn;
> +        p2m_access_t _a;
> +        p2m_type_t t;
> +
> +        mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL);
> +        rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1);
> +        return rc;

... ditch this extra return path and ...

> +    }
> +
> +    return 0;

... have both cases come here.

> @@ -1851,17 +1897,8 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, 
> uint32_t nr,
>          ap2m = d->arch.altp2m_p2m[altp2m_idx];
>      }
>  
> -    switch ( access )
> -    {
> -    case 0 ... ARRAY_SIZE(memaccess) - 1:
> -        a = memaccess[access];
> -        break;
> -    case XENMEM_access_default:
> -        a = p2m->default_access;
> -        break;
> -    default:
> +    if ( p2m_xenmem_access_to_p2m_access(p2m, access, &a) )
>          return -EINVAL;

Either you make the helper function return bool, or you pass on
whatever value it returned instead of assuming it's -EINVAL.

> +long p2m_set_mem_access_multi(struct domain *d,
> +                              XEN_GUEST_HANDLE(uint64_t) pfn_list,
> +                              XEN_GUEST_HANDLE(uint8) access_list,
> +                              uint32_t size, uint32_t start, uint32_t mask,
> +                              unsigned int altp2m_idx)
> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL;
> +    long rc = 0;
> +
> +    /* altp2m view 0 is treated as the hostp2m */
> +    if ( altp2m_idx )
> +    {
> +        if ( altp2m_idx >= MAX_ALTP2M ||
> +             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
> +            return -EINVAL;
> +
> +        ap2m = d->arch.altp2m_p2m[altp2m_idx];
> +    }
> +
> +    p2m_lock(p2m);
> +    if ( ap2m )
> +        p2m_lock(ap2m);
> +
> +    while ( start < size )
> +    {
> +        p2m_access_t a;
> +
> +        uint8_t access;
> +        uint64_t gfn_l;

Stray blank line between declarations.

> @@ -320,6 +321,23 @@ int compat_memory_op(unsigned int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) compat)
>              break;
>          }
>  
> +        case XENMEM_access_op:
> +        {
> +#define XLAT_mem_access_op_HNDL_pfn_list(_d_, _s_) \
> +            guest_from_compat_handle((_d_)->pfn_list, (_s_)->pfn_list)
> +#define XLAT_mem_access_op_HNDL_access_list(_d_, _s_) \
> +            guest_from_compat_handle((_d_)->access_list, (_s_)->access_list)
> +
> +            XLAT_mem_access_op(nat.mao, &cmp.mao);
> +
> +#undef XLAT_mem_access_op_HNDL_pfn_list
> +#undef XLAT_mem_access_op_HNDL_access_list
> +
> +            return mem_access_memop(cmd,
> +                                    guest_handle_cast(compat,
> +                                                      xen_mem_access_op_t));
> +        }

You translate into native format, but then cast the compat handle
to its native counterpart type. Such casting is okay only when
accompanied by a respectively invoked CHECK_* macro. Please
follow the model other code here uses.

And reviewing this I notice that my earlier bug fix also is still not
really correct: It causes hypercall_xlat_continuation() to be
bypassed.

> @@ -452,6 +454,16 @@ struct xen_mem_access_op {
>       * ~0ull is used to set and get the default access for pages
>       */
>      uint64_aligned_t pfn;
> +    /*
> +     * List of pfns to set access for
> +     * Used only with XENMEM_access_op_set_access_multi
> +     */
> +    XEN_GUEST_HANDLE(uint64_t) pfn_list;

I'm not sure why we even have this kind of handle - please use
XEN_GUEST_HANDLE(uint64) instead.

> +    /*
> +     * Corresponding list of access settings for pfn_list
> +     * Used only with XENMEM_access_op_set_access_multi
> +     */
> +    XEN_GUEST_HANDLE(uint8) access_list;

And for both of them - I don't think the arrays are meant to be
used for output? In which case they should be handles of const
types.

Jan

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