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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.10.14 at 20:10, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> @@ -490,6 +495,43 @@ static int _set_status(unsigned gt_version,
>          return _set_status_v2(domid, readonly, mapflag, shah, act, status);
>  }
>  
> +static int grant_map_exists(const struct domain *ld,
> +                            struct grant_table *rgt,
> +                            unsigned long mfn,
> +                            unsigned int *ref_count)
> +{
> +    const struct active_grant_entry *act;
> +    grant_ref_t ref, max_iter;

These should now be unsigned int too.

> @@ -2488,16 +2530,119 @@ 
> gnttab_swap_grant_ref(XEN_GUEST_HANDLE_PARAM(gnttab_swap_grant_ref_t) uop,
>      return 0;
>  }
>  
> +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 = 0;

This initializer ought to be pointless, but isn't because...

> +
> +    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;


... the check that ->op has no other flags set than the three
supported ones got lost (at least I think it was there earlier on,
and it certainly needs to be there).

> +
> +    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();

> +
> +    if ( d != owner )
> +        spin_unlock(&owner->grant_table->lock);
> +    unmap_domain_page(v);
> +    put_page(page);
> +
> +    return ret;
> +}
> +
> +static long
> +gnttab_cache_flush(XEN_GUEST_HANDLE_PARAM(gnttab_cache_flush_t) uop,
> +                      unsigned int *ref_count,
> +                      unsigned int count)
> +{
> +    int ret = 0;

This initializer is pointless (and the variable could be moved into the
inner loop, with the result of __gnttab_cache_flush() being its
initializer).

> +    unsigned int i;
> +    gnttab_cache_flush_t op;
> +
> +    for ( i = 0; i < count; i++ )
> +    {
> +        if ( i && hypercall_preempt_check() )
> +            return i;
> +        if ( unlikely(__copy_from_guest(&op, uop, 1)) )
> +            return -EFAULT;
> +        do {
> +            ret = __gnttab_cache_flush(&op, ref_count);
> +            if ( ret < 0 )
> +                return ret;
> +            if ( ret > 0 && hypercall_preempt_check() )
> +                return i;
> +        } while ( ret > 0 );
> +        *ref_count = 0;
> +        guest_handle_add_offset(uop, 1);
> +    }
> +    return 0;
> +}
> +
> +
>  long

There are still two blank lines being added here instead of just one.

> @@ -2617,17 +2762,33 @@ do_grant_table_op(
>          }
>          break;
>      }
> +    case GNTTABOP_cache_flush:
> +    {
> +        XEN_GUEST_HANDLE_PARAM(gnttab_cache_flush_t) cflush =
> +            guest_handle_cast(uop, gnttab_cache_flush_t);

I think I overlooked this earlier - there ought to be a blank line here
(even if most/all other pre-existing cases have the same issue).

> +struct gnttab_cache_flush {
> +    union {
> +        /* dev_bus_addr must refer to a granted page and match a
> +         * previously returned dev_bus_addr by a GNTTABOP hypercall.
> +         * As a consequence dev_bus_addr has to be page aligned.
> +         */

As said before, I'm not sure the comment is that useful. But if you
add it, it ought to follow ./CODING_STYLE (of course the first
character in this specific case doesn't need to be upper case). Also
the trailing "... by a GNTTABOP hypercall" reads a little odd to me,
but maybe that's just because my English is lacking.

Jan

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