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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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?

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