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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.10.14 at 11:52, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> On Tue, 21 Oct 2014, Jan Beulich wrote:
>> >>> On 20.10.14 at 18:58, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
>> > @@ -490,6 +495,42 @@ 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,
>> > +                            grant_ref_t *ref_count)
>> > +{
>> > +    const struct active_grant_entry *act;
>> > +    grant_ref_t ref, max_iter;
>> > +    
>> > +    ASSERT(spin_is_locked(&rgt->lock));
>> > +
>> > +    max_iter = min(*ref_count + (1 << GNTTABOP_CONTINUATION_ARG_SHIFT), 
> nr_grant_entries(rgt));
>> 
>> Long line.
>> 
>> > @@ -2617,17 +2761,34 @@ do_grant_table_op(
>> >          }
>> >          break;
>> >      }
>> > +    case GNTTABOP_cache_flush:
>> > +    {
>> > +        grant_ref_t ref_count = opaque_in;
>> 
>> I can't see the need for this variable.
> 
> We use opaque_in's address below but opaque_in is an unsigned int, not
> grant_ref_t. It doesn't make sense to cast &opaque_in to (grant_ref_t*),
> because if they were different sizes, it couldn't really help.
> So I used a local variable.

If they're of different size there would end up being truncation on
either the opaque_in or opaque_out assignments. So using a
separate variable doesn't help you anyway. And there's no real
need for ref_count to be grant_ref_t (which isn't really meant to
be a type arithmetic can be performed on).

>> > --- a/xen/include/public/grant_table.h
>> > +++ b/xen/include/public/grant_table.h
>> > @@ -309,6 +309,8 @@ typedef uint16_t grant_status_t;
>> >  #define GNTTABOP_get_status_frames    9
>> >  #define GNTTABOP_get_version          10
>> >  #define GNTTABOP_swap_grant_ref         11
>> > +#define GNTTABOP_cache_flush            12
>> > +/* max GNTTABOP is 4095 */
>> 
>> This is an implementation detail, i.e. such a comment doesn't belong
>> in the public header.
> 
> xen/include/xen/grant_table.h?

That would disconnect it from the #define-s above. I don't see
much point in a comment like this. If too big a new GNTTABOP_
would get added, people would very quickly notice that it doesn't
work.

Jan


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