Re: [PATCH v20 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 07 August 2018 15:37
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel <xen-devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> Subject: RE: [PATCH v20 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
> 
> >>> On 07.08.18 at 16:14, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 07 August 2018 14:38
> >>
> >> >>> On 06.08.18 at 14:54, <paul.durrant@citrix.com> wrote:
> >> > --- a/xen/common/grant_table.c
> >> > +++ b/xen/common/grant_table.c
> >> > @@ -3860,6 +3860,38 @@ int mem_sharing_gref_to_gfn(struct
> >> grant_table *gt, grant_ref_t ref,
> >> >  }
> >> >  #endif
> >> >
> >> > +/* caller must hold read or write lock */
> >> > +static int gnttab_get_status_frame_mfn(struct domain *d,
> >> > +                                       unsigned long idx, mfn_t *mfn)
> >> > +{
> >> > +    struct grant_table *gt = d->grant_table;
> >> > +
> >> > +    if ( idx >= nr_status_frames(gt) )
> >> > +        return -EINVAL;
> >> > +
> >> > +    *mfn = _mfn(virt_to_mfn(gt->status[idx]));
> >> > +    return 0;
> >> > +}
> >> > +
> >> > +/* caller must hold write lock */
> >> > +static int gnttab_get_shared_frame_mfn(struct domain *d,
> >> > +                                       unsigned long idx, mfn_t *mfn)
> >> > +{
> >> > +    struct grant_table *gt = d->grant_table;
> >> > +
> >> > +    if ( gt->gt_version == 0 )
> >> > +        gt->gt_version = 1;
> >> > +
> >> > +    if ( (idx >= nr_grant_frames(gt)) && (idx < gt->max_grant_frames) )
> >> > +        gnttab_grow_table(d, idx + 1);
> >>
> >> I guess I had commented on this before, but it has been a while:
> >> While I realize that you're just moving code, I dislike the resulting
> >> asymmetry between the two functions: Why would the former
> >> not want to grow the grant table? And why would a version
> >> adjustment be needed (only) here (I've put "only" in parentheses
> >> because it's not obvious to me why this is needed)?
> >>
> >> And this asymmetry would surely be coming as surprise at least
> >> to callers of the new interface you add.
> >
> > I'm happy to have get_status_frame() grow the table too but it does mean
> a
> > change in behaviour to callers of gnttab_map_frame(). If that's not a
> problem
> > then I'll make the change.
> 
> I'd say it was a mistake there before. The (slight) difficulty (which
> probably made whoever wrote this originally ignore this aspect) is
> figuring out how much to grow the table.
> 

Yes, that's probably why it wasn't done.

> >> > +int gnttab_get_status_frame(struct domain *d, unsigned long idx,
> >> > +                            mfn_t *mfn)
> >> > +{
> >> > +    struct grant_table *gt = d->grant_table;
> >> > +    int rc;
> >> > +
> >> > +    grant_read_lock(gt);
> >> > +    rc = gnttab_get_status_frame_mfn(d, idx, mfn);
> >> > +    grant_read_unlock(gt);
> >> > +
> >> > +    return rc;
> >> > +}
> >>
> >> Along the lines of what gnttab_map_frame() does, I'd expect a
> >> check for gt_version to be 2 here. Or well, maybe that's implicit
> >> by there not being any status entries/pages for version 1 tables.
> >
> > Yes, that should be the case.
> >
> >> But it would become a requirement if gnttab_grow_table() was to
> >> be called from gnttab_get_status_frame_mfn().
> >>
> >
> > Yes, so making that change will certainly add complexity.
> 
> Hmm, yes - a single if().

Along with the calculation to work out how many frames to grow the grant table by to get to the necessary status frame. Not massive complexity... but more than there is now.

  Paul

> 
> Jan
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel