Re: [Xen-devel] [PATCH v11 6/8] xen/arm: introduce GNTTABOP_cache_flush

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.10.14 at 18:40, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> Introduce a new hypercall to perform cache maintenance operation on
> behalf of the guest. The argument is a machine address and a size. The
> implementation checks that the memory range is owned by the guest or the
> guest has been granted access to it by another domain.
> 
> Introduce grant_map_exists: an internal grant table function to check
> whether an mfn has been granted to a given domain on a target grant
> table. Check hypercall_preempt_check() every 4096 iterations in the
> implementation of grant_map_exists.
> Use the top 20 bits of the GNTTABOP cmd encoding to save the last ref
> across the hypercall continuation.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
albeit the subject still says "arm" while there isn't anything ARM-specific
in here and ...

> +static int __gnttab_cache_flush(gnttab_cache_flush_t *cflush,
> +                                unsigned int *ref_count)
> +{
> +    struct domain *d, *owner;
> +    struct page_info *page;
> +    unsigned long mfn;
> +    void *v;
> +    int ret;
> +
> +    if ( (cflush->offset >= PAGE_SIZE) ||
> +         (cflush->length > PAGE_SIZE) ||
> +         (cflush->offset + cflush->length > PAGE_SIZE) )
> +        return -EINVAL;
> +
> +    if ( cflush->length == 0 || cflush->op == 0 )
> +        return 0;
> +
> +    /* currently unimplemented */
> +    if ( cflush->op & GNTTAB_CACHE_SOURCE_GREF )
> +        return -EOPNOTSUPP;
> +
> +    if ( !(cflush->op & (GNTTAB_CACHE_INVAL|GNTTAB_CACHE_CLEAN)) ||

... I think we should drop this because ...

> +         (cflush->op & ~(GNTTAB_CACHE_INVAL|GNTTAB_CACHE_CLEAN)) )
> +        return -EINVAL;
> +
> +    d = rcu_lock_current_domain();
> +    mfn = cflush->a.dev_bus_addr >> PAGE_SHIFT;
> +
> +    if ( !mfn_valid(mfn) )
> +    {
> +        rcu_unlock_domain(d);
> +        return -EINVAL;
> +    }
> +
> +    page = mfn_to_page(mfn);
> +    owner = page_get_owner_and_reference(page);
> +    if ( !owner )
> +    {
> +        rcu_unlock_domain(d);
> +        return -EPERM;
> +    }
> +
> +    if ( d != owner )
> +    {
> +        spin_lock(&owner->grant_table->lock);
> +
> +        ret = grant_map_exists(d, owner->grant_table, mfn, ref_count);
> +        if ( ret != 0 )
> +        {
> +            spin_unlock(&owner->grant_table->lock);
> +            rcu_unlock_domain(d);
> +            put_page(page);
> +            return ret;
> +        }
> +    }
> +
> +    v = map_domain_page(mfn);
> +    v += cflush->offset;
> +
> +    if ( (cflush->op & GNTTAB_CACHE_INVAL) && (cflush->op & 
> GNTTAB_CACHE_CLEAN) )
> +        ret = clean_and_invalidate_dcache_va_range(v, cflush->length);
> +    else if ( cflush->op & GNTTAB_CACHE_INVAL )
> +        ret = invalidate_dcache_va_range(v, cflush->length);
> +    else if ( cflush->op & GNTTAB_CACHE_CLEAN )
> +        ret = clean_dcache_va_range(v, cflush->length);
> +    else
> +        BUG();

... now that I think about it again it was a bad suggestion to BUG()
here - there's nothing wrong with the caller requesting an expensive
NOP.

Both could be fixed up while committing.

Jan


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