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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.03.18 at 13:02, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 26 March 2018 13:55
>> 
>> >>> On 26.03.18 at 14:16, <JBeulich@xxxxxxxx> wrote:
>> >>>> On 22.03.18 at 12:55, <paul.durrant@xxxxxxxxxx> wrote:
>> >> --- a/xen/common/grant_table.c
>> >> +++ b/xen/common/grant_table.c
>> >> @@ -3863,6 +3863,35 @@ 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 ( (idx >= nr_grant_frames(gt)) && (idx < gt->max_grant_frames) )
>> >> +        gnttab_grow_table(d, idx + 1);
>> >> +
>> >> +    if ( idx >= nr_grant_frames(gt) )
>> >> +        return -EINVAL;
>> >> +
>> >> +    *mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
>> >> +    return 0;
>> >> +}
>> >
>> > I realize the anomaly was there already before, but imo it becomes
>> > more pronounced with the two functions differing in more than just
>> > the shared vs status naming (IOW I find it strange that one grows
>> > the grant table while the other doesn't). This extends to ...
>> >
>> >> +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
>> >> +                            mfn_t *mfn)
>> >> +{
>> >> +    struct grant_table *gt = d->grant_table;
>> >> +    int rc;
>> >> +
>> >> +    grant_write_lock(gt);
>> >> +    rc = gnttab_get_shared_frame_mfn(d, idx, mfn);
>> >> +    grant_write_unlock(gt);
>> >> +
>> >> +    return rc;
>> >> +}
>> >> +
>> >> +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;
>> >> +}
>> >
>> > ... these two acquiring the lock in different ways.
> 
> So, you want me to have gnttab_get_status_frame() grow the table 
> accordingly? I'd really rather not do that at v19 of the series when it's 
> never been part of the scope before.

Indeed, please only take this as a remark at this point in time.
It just so happens that when you look at something again after
a fair amount of time, you see things you haven't noticed before.

Jan


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