Re: [Xen-devel] [PATCH v9 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of Jan
> Beulich
> Sent: 10 October 2017 11:26
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; KonradRzeszutek Wilk <konrad.wilk@xxxxxxxxxx>;
> George Dunlap <George.Dunlap@xxxxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Tim
> (Xen.org) <tim@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v9 10/11] common: add a new mappable
> resource type: XENMEM_resource_grant_table
> 
> >>> On 06.10.17 at 14:25, <paul.durrant@xxxxxxxxxx> wrote:
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -3756,14 +3756,13 @@ int mem_sharing_gref_to_gfn(struct
> grant_table *gt, grant_ref_t ref,
> >  }
> >  #endif
> >
> > -int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> > -                     mfn_t *mfn)
> > +/* Caller must hold write lock as version may change and table may grow
> */
> > +static int gnttab_get_frame_locked(struct domain *d, unsigned long idx,
> > +                                   mfn_t *mfn)
> 
> I don't think this helper function needs to be passed d; gt appears
> to suffice.

Alas it does not. It calls through to gnttab_grow_table() which requires the 
domain.

> 
> > @@ -3787,6 +3786,19 @@ int gnttab_map_frame(struct domain *d,
> unsigned long idx, gfn_t gfn,
> >              rc = -EINVAL;
> >      }
> >
> > +    return rc;
> > +}
> > +
> > +int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> > +                     mfn_t *mfn)
> > +{
> > +    struct grant_table *gt = d->grant_table;
> > +    int rc;
> > +
> > +    grant_write_lock(gt);
> > +
> > +    rc = gnttab_get_frame_locked(d, idx, mfn);
> > +
> >      if ( !rc )
> >          gnttab_set_frame_gfn(gt, idx, gfn);
> >
> > @@ -3795,6 +3807,18 @@ int gnttab_map_frame(struct domain *d,
> unsigned long idx, gfn_t gfn,
> >      return rc;
> >  }
> >
> > +int gnttab_get_frame(struct domain *d, unsigned long idx, mfn_t *mfn)
> 
> const struct domain * (I realize now that the same should have
> been done for gnttab_map_frame() when it was introduced;
> perhaps you could change that at the same time).

Again, the problem is that grow_table and functions it calls don't take a const 
pointer. I tried cascading the const through to the underlying function but the 
patch started to balloon so I think such work should be deferred.

> 
> > @@ -966,6 +967,30 @@ static long xatp_permission_check(struct domain
> *d,
> > unsigned int space)
> >  }
> >
> >  #ifdef CONFIG_X86
> > +static int acquire_grant_table(struct domain *d, unsigned int id,
> 
> This clearly isn't x86-specific code.

Ok.

> 
> > +                               unsigned long frame,
> > +                               unsigned long nr_frames,
> > +                               unsigned long mfn_list[])
> > +{
> > +    unsigned int i = nr_frames;
> 
> Silent truncation just like in the earlier patch.

Yes, nr_frames should be an unsigned int.

> 
> > +    if ( id != 0 )
> 
> Do we want a constant here again? Also acquiring the status page
> MFNs via separate ID would seem better than re-using
> XENMAPIDX_grant_table_status here, the more that this uses bit
> 31 while right now your interface structure field is still 64 bits wide.
> 

Ok, that's a better way to do it.

> > @@ -993,6 +1018,11 @@ static int acquire_resource(const
> xen_mem_acquire_resource_t *xmar)
> >                                           xmar->nr_frames, mfn_list);
> >          break;
> >
> > +    case XENMEM_resource_grant_table:
> > +        rc = acquire_grant_table(d, xmar->id, xmar->frame,
> > +                                 xmar->nr_frames, mfn_list);
> > +        break;
> 
> Is this really generally useful with mfn_list[] having just two entries?
> 

Good point. I'll increase the size of the array in this patch (to the default 
table size of 32... I think that's a reasonable value to choose).

> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel