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: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 08 August 2018 11:30
> 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 v21 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
> 
> >>> On 08.08.18 at 11:00, <paul.durrant@xxxxxxxxxx> wrote:
> > --- 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.*/
> > +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);
> > +}
> 
> Seeing no use of the grant table (it's not even passed in) I'm confused
> by the comment you add. I guess you've simply copied
> grant_to_status_frames()'es, which looks similarly wrong.
> 

Indeed.

> > @@ -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;
> 
> const?
> 

IIRC that didn't work because gnttab_grow_table() modifies the content.

> > +    ASSERT(gt->gt_version == 2);
> > +
> > +    if ( idx >= nr_status_frames(gt) )
> > +    {
> > +        unsigned long nr = status_to_grant_frames(idx + 1);
> > +
> > +        if ( nr <= gt->max_grant_frames )
> > +            gnttab_grow_table(d, nr);
> > +
> > +        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;
> 
> Again?
> 
> > @@ -3906,6 +3943,38 @@ int gnttab_map_frame(struct domain *d,
> unsigned long idx, gfn_t gfn,
> >      return rc;
> >  }
> >
> > +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);
> > +
> > +    if ( gt->gt_version == 0 )
> > +        gt->gt_version = 1;
> 
> Since you've moved this here instead of dropping it, what requirement
> have you found for this to be set (other than the ASSERT() you put in
> gnttab_get_shared_frame_mfn()?
> 

The code in patch #2 is executed before the grant table version is set. I could 
alternatively have libxl explicitly set the version to 1 before trying to seed 
the table.

> > @@ -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) &&
> > +            !is_hardware_domain(currd) )
> 
> Missing blank and as a result improper indentation. Also the U in
> "until" wants to be upper case I think.
> 

Ok.

> The cosmetic aspects could of course be taken care of while
> committing, but the version related question needs to be
> clarified first.
> 

It's ok, I'm happy to send v22. Just need to know whether you'd prefer explicit 
version setting in the toolstack.

  Paul

> Jan
> 


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