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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 26 March 2018 13:55
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>; Ian
> Jackson <Ian.Jackson@xxxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; Tim (Xen.org)
> <tim@xxxxxxx>
> Subject: Re: [PATCH v18 10/11] common: add a new mappable resource
> type: XENMEM_resource_grant_table
> 
> >>> 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.

> >
> > And then I'm completely missing the interaction with
> > gnttab_unpopulate_status_frames(). While this might not be a
> > practical problem at this point in time, we're liable to forget to
> > address this later on if there's no stop gap measure. A PV guest
> > mapping the obtained MFNs is going to be fine, but a HVM/PVH
> > one isn't, since neither x86 nor ARM refcount pages inserted into
> > or removed from a domain's p2m. I therefore think you need to
> > add a is_hvm_domain() check to acquire_grant_table(), with a
> > suitable fixme comment attached to it.
> 
> Or perhaps better paging_mode_translate(current->domain).
> 

Ok. I'll add the safety check and comment.

  Paul

> Jan


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