> -----Original Message-----
> From: Jan Beulich [mailto:[email protected]]
> Sent: 07 August 2018 15:37
> To: Paul Durrant <[email protected]>
> Cc: Andrew Cooper <[email protected]>; George Dunlap
> <[email protected]>; Ian Jackson <[email protected]>; Wei Liu
> <[email protected]>; Stefano Stabellini <[email protected]>; xen-
> devel <[email protected]>; Konrad Rzeszutek Wilk
> <[email protected]>; Tim (Xen.org) <[email protected]>
> Subject: RE: [PATCH v20 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
>
> >>> On 07.08.18 at 16:14, <[email protected]> wrote:
> >> From: Jan Beulich [mailto:[email protected]]
> >> Sent: 07 August 2018 14:38
> >>
> >> >>> On 06.08.18 at 14:54, <[email protected]> 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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
