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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 08 August 2018 11:11
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Jan Beulich <jbeulich@xxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Konrad
> Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>
> Subject: Re: [PATCH v21 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
> 
> On 08/08/18 10:00, Paul Durrant wrote:
> > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> > index d9ec711c73..8e623ea08e 100644
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -358,6 +358,12 @@ static inline unsigned int
> grant_to_status_frames(unsigned int grant_frames)
> >      return DIV_ROUND_UP(grant_frames * GRANT_PER_PAGE,
> GRANT_STATUS_PER_PAGE);
> >  }
> >
> > +/* Number of grant table entries. Caller must hold d's gr. table lock.*/
> 
> Really? this is some straight arithmetic on the integer parameter.
> 

Good point. I'd just adapted the comment from the reverse translation above it 
but the comment is indeed bogus.

> > +static inline unsigned int status_to_grant_frames(unsigned int
> status_frames)
> > +{
> > +    return DIV_ROUND_UP(status_frames * GRANT_STATUS_PER_PAGE,
> GRANT_PER_PAGE);
> > +}
> > +
> >  /* Check if the page has been paged out, or needs unsharing.
> >     If rc == GNTST_okay, *page contains the page struct with a ref taken.
> >     Caller must do put_page(*page).
> > @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct
> grant_table *gt, grant_ref_t ref,
> >  }
> >  #endif
> >
> > +/* caller must hold 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;
> > +
> > +    ASSERT(gt->gt_version == 2);
> > +
> > +    if ( idx >= nr_status_frames(gt) )
> > +    {
> > +        unsigned long nr = status_to_grant_frames(idx + 1);
> 
> Why the +1 ? Won't that cause a failure if you attempt to map the
> maximum valid index?

That's kind of the idea...

> 
> > +
> > +        if ( nr <= gt->max_grant_frames )
> > +            gnttab_grow_table(d, nr);
> 
> You want to capture the return value of grow_table(), which at least
> distinguishes between ENODEV and ENOMEM.
> 
> > +
> > +        if ( idx >= nr_status_frames(gt) )
> > +            return -EINVAL;
> 
> This can probably(?) be asserted if gnttab_grow_table() returns
> successfully.
> 

... which is why this is an if and not an ASSERT. I'm just following the 
analogy of the way the table is grown in gnttab_get_shared_frame_mfn() which is 
the way it was done before. If you'd rather I change things to use the return 
value from gnttab_grow_table() then I guess I could do that.

> > +    }
> > +
> > +    *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;
> > +
> > +    ASSERT(gt->gt_version != 0);
> > +
> > +    if ( (idx >= nr_grant_frames(gt)) && (idx < gt->max_grant_frames) )
> > +        gnttab_grow_table(d, idx + 1);
> 
> Similarly wrt rc.
> 
> > +
> > +    if ( idx >= nr_grant_frames(gt) )
> > +        return -EINVAL;
> > +
> > +    *mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
> > +    return 0;
> > +}
> > +
> >  int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> >                       mfn_t *mfn)
> >  {
> > diff --git a/xen/common/memory.c b/xen/common/memory.c
> > index e29d596727..427f65a5e1 100644
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -23,6 +23,7 @@
> >  #include <xen/numa.h>
> >  #include <xen/mem_access.h>
> >  #include <xen/trace.h>
> > +#include <xen/grant_table.h>
> >  #include <asm/current.h>
> >  #include <asm/hardirq.h>
> >  #include <asm/p2m.h>
> > @@ -982,6 +983,43 @@ 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;
> > +
> > +    /* Iterate backwards in case table needs to grow */
> > +    while ( i-- != 0 )
> > +    {
> > +        mfn_t mfn = INVALID_MFN;
> > +        int rc;
> > +
> > +        switch ( id )
> > +        {
> > +        case XENMEM_resource_grant_table_id_shared:
> > +            rc = gnttab_get_shared_frame(d, frame + i, &mfn);
> > +            break;
> > +
> > +        case XENMEM_resource_grant_table_id_status:
> > +            rc = gnttab_get_status_frame(d, frame + i, &mfn);
> > +            break;
> > +
> > +        default:
> > +            rc = -EINVAL;
> > +            break;
> > +        }
> > +
> > +        if ( rc )
> 
> Would it perhaps be prudent to have || mfn_eq(mfn, INVALID_MFN)
> here?  I
> don't think anything good will come of handing INVALID_MFN back to the
> caller.

Why? The value of mfn will be overwritten if rc == 0 and will be left as 
INVALID_MFN if rc != 0. I can ASSERT that if you'd like.

> 
> > +            return rc;
> > +
> > +        mfn_list[i] = mfn_x(mfn);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static int acquire_resource(
> >      XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
> >  {
> > @@ -1046,6 +1089,16 @@ static int acquire_resource(
> >          xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> >          unsigned int i;
> >
> > +        /*
> > +         * FIXME: until foreign pages inserted into the P2M are properly
> > +         *        reference counted, it is unsafe to allow mapping of
> > +         *        non-caller-owned resource pages unless the caller is
> > +         *        the hardware domain.
> > +         */
> > +        if (!(xmar.flags & XENMEM_rsrc_acq_caller_owned) &&
> 
> Space.

Oh yes.

> 
> > +            !is_hardware_domain(currd) )
> > +            return -EOPNOTSUPP;
> 
> EPERM perhaps?
> 

I debated that. I'm really not sure which one would be best.

  Paul

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