[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v20 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
>>> On 07.08.18 at 16:14, <Paul.Durrant@xxxxxxxxxx> wrote: >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx] >> Sent: 07 August 2018 14:38 >> >> >>> On 06.08.18 at 14:54, <paul.durrant@xxxxxxxxxx> 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. >> > +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(). Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |