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

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Sun, 21 Jul 2013, Ian Campbell wrote:
> > diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> > index 04c1b2d..0f7d00b 100644
> > --- a/drivers/xen/grant-table.c
> > +++ b/drivers/xen/grant-table.c
> > @@ -65,6 +65,8 @@ static grant_ref_t gnttab_free_head;
> >  static DEFINE_SPINLOCK(gnttab_list_lock);
> >  unsigned long xen_hvm_resume_frames;
> >  EXPORT_SYMBOL_GPL(xen_hvm_resume_frames);
> > +int __read_mostly gnttabop_unmap_and_replace = GNTTABOP_unmap_and_replace; 
> > +EXPORT_SYMBOL_GPL(gnttabop_unmap_and_replace);
> 
> I can see this getting set but not used, does it really need to be
> exported?

I use it in the next patch


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


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