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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 23 Oct 2014, Jan Beulich wrote:
> >>> 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.

Sure.
Given that the series already has all the required acks, are you up for
committing it yourself?
Do I need to do anything else?

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