Re: [Xen-devel] [PATCH RFC] xen/grant_table: deprecate GNTTABOP_unmap_and_replace

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 22 Jul 2013, Stefano Stabellini wrote:
> On Sun, 21 Jul 2013, Ian Campbell wrote:
> > On Sun, 2013-07-21 at 18:34 +0100, Stefano Stabellini wrote:
> > > GNTTABOP_unmap_and_replace has two issues:
> > > - it unconditionally replaces the mapping passed in new_addr with 0;
> > > - it doesn't support GNTMAP_contains_pte mappings on x86, returning a
> > > general error instead of some forms of ENOSYS.
> > > 
> > > Deprecate GNTTABOP_unmap_and_replace and introduce a new
> > > GNTTABOP_unmap_and_replace (12) that returns GNTST_enosys for
> > > GNTMAP_contains_pte requests and doesn't zero the mapping at new_addr.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > > ---
> > >  xen/arch/x86/mm.c                |   12 +-----------
> > >  xen/include/public/grant_table.h |    7 ++++---
> > >  2 files changed, 5 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > > index 77dcafc..610fd09 100644
> > > --- a/xen/arch/x86/mm.c
> > > +++ b/xen/arch/x86/mm.c
> > > @@ -4064,7 +4064,7 @@ int replace_grant_host_mapping(
> > >              return destroy_grant_pte_mapping(addr, frame, curr->domain);
> > >          
> > >          MEM_LOG("Unsupported grant table operation");
> > > -        return GNTST_general_error;
> > > +        return GNTST_enosys;
> > >      }
> > >  
> > >      if ( !new_addr )
> > > @@ -4102,16 +4102,6 @@ int replace_grant_host_mapping(
> > >  
> > >      ol1e = *pl1e;
> > >  
> > > -    if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, l1e_empty(),
> > > -                                gl1mfn, curr, 0)) )
> > > -    {
> > > -        page_unlock(l1pg);
> > > -        put_page(l1pg);
> > > -        MEM_LOG("Cannot delete PTE entry at %p", (unsigned long *)pl1e);
> > > -        guest_unmap_l1e(curr, pl1e);
> > > -        return GNTST_general_error;
> > > -    }
> > 
> > Doesn't this break all existing users of the subop? I think you need to
> > stick a if (op == unmap_and_replace_legacy) in here and keep the code
> > for that case.
> 
> I don't think there are any existing users, but that's a fair point.
> 
> > > -
> > >      page_unlock(l1pg);
> > >      put_page(l1pg);
> > >      guest_unmap_l1e(curr, pl1e);
> > > diff --git a/xen/include/public/grant_table.h 
> > > b/xen/include/public/grant_table.h
> > > index b8a3d6c..ae841ae 100644
> > > --- a/xen/include/public/grant_table.h
> > > +++ b/xen/include/public/grant_table.h
> > > @@ -303,12 +303,13 @@ typedef uint16_t grant_status_t;
> > >  #define GNTTABOP_transfer             4
> > >  #define GNTTABOP_copy                 5
> > >  #define GNTTABOP_query_size           6
> > > -#define GNTTABOP_unmap_and_replace    7
> > > +#define GNTTABOP_unmap_and_replace_legacy    7
> > >  #if __XEN_INTERFACE_VERSION__ >= 0x0003020a
> > >  #define GNTTABOP_set_version          8
> > >  #define GNTTABOP_get_status_frames    9
> > >  #define GNTTABOP_get_version          10
> > >  #define GNTTABOP_swap_grant_ref        11
> > > +#define GNTTABOP_unmap_and_replace    12
> > >  #endif /* __XEN_INTERFACE_VERSION__ */
> > 
> > You need an ifdef here so that users specifying an old
> > __XEN_INTERFACE_VERSION__ get the old hypercall under the non-legacy
> > name.
> 
> I guess I need to bump the version too?

Thinking twice about this, even though bumping the interface version
would be easy enough, that would prevent any backports of the "fixed"
hypercalls to older hypervisors.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel