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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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