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:43
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Wei Liu
> <wei.liu2@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 12:38, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 08 August 2018 11:30
> >>
> >> >>> On 08.08.18 at 11:00, <paul.durrant@xxxxxxxxxx> wrote:
> >> > @@ -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.
> 
> But you don't pass gt to the function:
> 
> >> > +    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);
> 
> ^^^

I know, I was just remembering that the compiler complained when I tried this 
before. My memory could be wrong so I'll try it 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.
> 
> But that's not my point. What's wrong with leaving it at zero?
> 

I'm not particularly happy calling gnttab_grow_table() with version left at 0 
but I can try it and see if it breaks.

  Paul

> Jan
> 


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