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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 17 Oct 2014, Jan Beulich wrote:
> >>> On 17.10.14 at 17:31, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> > +static int grant_map_exists(const struct domain *ld,
> > +                            struct grant_table *rgt,
> > +                            unsigned long mfn,
> > +                            grant_ref_t *ref_count)
> > +{
> > +    const struct active_grant_entry *act;
> > +    grant_ref_t ref;
> > +
> > +    ASSERT(spin_is_locked(&rgt->lock));
> > +
> > +    for ( ref = *ref_count; ref < nr_grant_entries(rgt); ref++ )
> > +    {
> > +        if ( (ref % MAX_GRANT_ENTRIES_ITERATIONS) == 0 && ref != 
> > *ref_count )
> > +        {
> > +            *ref_count = ref;
> > +            return ref;
> > +        }
> > +
> > +        act = &active_entry(rgt, ref);
> > +
> > +        if ( !act->pin )
> > +            continue;
> > +
> > +        if ( act->domid != ld->domain_id )
> > +            continue;
> > +
> > +        if ( act->frame != mfn )
> > +            continue;
> > +
> > +        return 0;
> > +    }
> > +
> > +    return -1;
> 
> Return values need to be adjusted throughout the function: The
> -1 here gets passed up all the way, and hence will be seen as -EPERM
> by the caller of the hypercall, which clearly isn't right. Also returning
> ref (effectively a uint32_t) above doesn't fit with the function
> returning (signed) int. The caller doesn't care about the actual count,
> so just returning 1 in that case would seem fine.
> 
> > +#define OPAQUE_CONTINUATION_ARG_SHIFT 16
> > +#define CMD_MASK                      
> > ((1<<OPAQUE_CONTINUATION_ARG_SHIFT)-1)
> 
> Along the lines of MEMOP_CMD_MASK, GNTTABOP_CMD_MASK
> please. Also this needs to be moved and the compat wrapper
> needs to be updated to cope with this extension. And finally 16
> bits here and 11 bits in grant_map_exists() make 27, but you're
> nowhere enforcing this upper limit on grant entry count. Either
> do this, or adjust the value such that together with
> MAX_GRANT_ENTRIES_ITERATIONS you can encode to full
> value range.

All very good points. I'll make the changes.

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