Re: [PATCH 2/3] xen/grant_table: use the new GNTTABOP_unmap_and_replace hypercall

[Ian Campbell <ian.campbell@citrix.com>]

On Mon, 2013-07-22 at 11:38 +0100, Stefano Stabellini wrote:
> > >  
> > >  static union {
> > >  	struct grant_entry_v1 *v1;
> > > @@ -1238,6 +1240,12 @@ int gnttab_init(void)
> > >  	gnttab_free_count = nr_init_grefs - NR_RESERVED_ENTRIES;
> > >  	gnttab_free_head  = NR_RESERVED_ENTRIES;
> > >  
> > > +	ret = HYPERVISOR_grant_table_op(GNTTABOP_unmap_and_replace, NULL, 1);
> > > +	if (ret == -ENOSYS) {
> > > +		gnttabop_unmap_and_replace = GNTTABOP_unmap_and_replace_legacy;
> > > +		pr_warn("GNTTABOP_unmap_and_replace unavailable, falling back to the racy legacy version\n");
> > 
> > Perhaps WARN_ONCE?
> 
> It's already going through that function just once: the function is
> gnttab_init

Oh, so it is!

> 
> 
> > > +	}
> > > +
> > >  	printk("Grant table initialized\n");
> > >  	return 0;
> > >  
> > > diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> > > index 694dcaf..867d1a7 100644
> > > --- a/include/xen/grant_table.h
> > > +++ b/include/xen/grant_table.h
> > > @@ -179,6 +179,7 @@ int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
> > >  void arch_gnttab_unmap(void *shared, unsigned long nr_gframes);
> > >  
> > >  extern unsigned long xen_hvm_resume_frames;
> > > +extern int gnttabop_unmap_and_replace;
> > >  unsigned int gnttab_max_grant_frames(void);
> > >  
> > >  #define gnttab_map_vaddr(map) ((void *)(map.host_virt_addr))
> > > diff --git a/include/xen/interface/grant_table.h b/include/xen/interface/grant_table.h
> > > index e40fae9..2a2b74f 100644
> > > --- a/include/xen/interface/grant_table.h
> > > +++ b/include/xen/interface/grant_table.h
> > > @@ -408,15 +408,17 @@ DEFINE_GUEST_HANDLE_STRUCT(gnttab_query_size);
> > >  /*
> > >   * GNTTABOP_unmap_and_replace: Destroy one or more grant-reference mappings
> > >   * tracked by <handle> but atomically replace the page table entry with one
> > > - * pointing to the machine address under <new_addr>.  <new_addr> will be
> > > - * redirected to the null entry.
> > > + * pointing to the machine address under <new_addr>.
> > > + * GNTTABOP_unmap_and_replace_legacy also redirects <new_addr> to the
> > > + * null entry
> > 
> > I wonder if rather than _legacy (or _compat) if the old behaviour might
> > be well described as "unmap_and_swap", since I think that is what
> > clearing new_addr means?
> 
> Maybe GNTTABOP_unmap_and_swap is a better name. However it's still
> pretty far from what we need. Should I work-around the limitations of the
> current hypercall or implement a new one?
> I don't think that doing both is a great strategy in this case.

Not sure what you mean by both, whatever you do the old hypercall has to
keep working, it's used by classic-Xen netback.

BTW, the existing netback user is a fallback path which does:
      * allocate a new page
      * copy content of the granted page into it
      * unmap and replace to swap out the new page for the grant

Not sure if that makes the reason for zeroing new_addr make more
sense ;-)

Ian