Re: [Xen-devel] [PATCH v20 1/2] 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: 07 August 2018 14:38
> 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 <xen-devel@xxxxxxxxxxxxxxxxxxxx>;
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org)
> <tim@xxxxxxx>
> Subject: Re: [PATCH v20 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
> 
> >>> 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.

The version adjustment may be a hangover from the previous code base. Juergen's 
per-domain gnttab adjustments have probably rendered this unnecessary so I'll 
drop it as long as nothing breaks.

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

> > @@ -982,6 +983,54 @@ static long xatp_permission_check(struct domain
> *d, unsigned int space)
> >      return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> >  }
> >
> > +static int acquire_grant_table(struct domain *d, unsigned int id,
> > +                               unsigned long frame,
> > +                               unsigned int nr_frames,
> > +                               xen_pfn_t mfn_list[])
> > +{
> > +    unsigned int i = nr_frames;
> > +
> > +    /*
> > +     * FIXME: It is not currently safe to map grant status frames if they
> > +     *        will be inserted into the caller's P2M, because these
> > +     *        insertions are not yet properly reference counted.
> > +     *        This restriction can be removed when appropriate reference
> > +     *        counting is added.
> > +     */
> > +    if ( paging_mode_translate(current->domain) &&
> > +         (id == XENMEM_resource_grant_table_id_status) )
> > +        return -EOPNOTSUPP;
> 
> Would you mind reminding me why the P2M insertions are safe for
> the "ordinary" (shared) grant frames? I'm puzzled because P2M
> management in general lacks refcounting, yet I have the vague
> feeling that we had discussed this before and there was a reason.
> (If there is, perhaps slightly extending the comment above would
> be helpful.)

My memory is hazy too so I'll have to mine back down the email traffic. Without 
having done that I can only assume it is because there is the possibility of 
mapping the status frames and then setting the version back to 1 thus causing 
the status frames to evaporate whilst still being present in the P2M. This 
clearly won't happen to the shared frames.

> 
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -611,16 +611,21 @@ struct xen_mem_acquire_resource {
> >      uint16_t type;
> >
> >  #define XENMEM_resource_ioreq_server 0
> > +#define XENMEM_resource_grant_table 1
> >
> >      /*
> >       * IN - a type-specific resource identifier, which must be zero
> >       *      unless stated otherwise.
> >       *
> >       * type == XENMEM_resource_ioreq_server -> id == ioreq server id
> > +     * type == XENMEM_resource_grant_table -> id defined below
> >       */
> >      uint32_t id;
> > -    /*
> > -     * IN/OUT - As an IN parameter number of frames of the resource
> > +
> > +#define XENMEM_resource_grant_table_id_shared 0
> > +#define XENMEM_resource_grant_table_id_status 1
> > +
> > +    /* IN/OUT - As an IN parameter number of frames of the resource
> 
> Please don't break previously correct comment style here.
> 

Oh yes. Missed that bit of rebase breakage.

  Paul

> Jan


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