Re: [Xen-devel] [PATCH v4 5/7] xen/arm: introduce GNTTABOP_cache_flush

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.10.14 at 13:43, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -483,6 +483,34 @@ static int _set_status(unsigned gt_version,
>          return _set_status_v2(domid, readonly, mapflag, shah, act, status);
>  }
>  
> +static bool_t grant_map_exists(const struct domain *ld,
> +                        struct grant_table *rgt,
> +                        unsigned long mfn)
> +{
> +    const struct active_grant_entry *act;
> +    grant_ref_t ref;
> +
> +    ASSERT(spin_is_locked(&rgt->lock));
> +
> +    for ( ref = 0; ref != nr_grant_entries(rgt); ref++ )

While you added a way to restrict the time this loop takes, you
still don't enforce a sane upper bound on the value specified
for "gnttab_max_nr_frames=". Perhaps that could be done
together with the compatibility thing I mentioned in reply to one
of the earlier patches?

> +static int __gnttab_cache_flush(gnttab_cache_flush_t *cflush)
> +{
> +    struct domain *d, *owner;
> +    struct page_info *page;
> +    uint64_t mfn;

unsigned long

> +    void *v;
> +
> +    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;
> +
> +    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);
> +
> +        if ( !grant_map_exists(d, owner->grant_table, mfn) )
> +        {
> +            spin_unlock(&owner->grant_table->lock);
> +            rcu_unlock_domain(d);
> +            put_page(page);
> +            gdprintk(XENLOG_G_ERR, "mfn %"PRIx64" hasn't been granted by d%d 
> to d%d\n",
> +                    mfn, owner->domain_id, d->domain_id);

I'm afraid without a GPFN this message isn't going to be very useful
other than to indicate there is _some_ problem.

Jan


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