Re: [Xen-devel] [PATCH 1/2] xen/granttable: Support sub-page grants

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2011-12-06 at 10:56 +0000, annie.li@xxxxxxxxxx wrote:
> From: Annie Li <annie.li@xxxxxxxxxx>
> 
>     -- They can't be used to map the page (so can only be used in a 
> GNTTABOP_copy
>        hypercall).
>     -- It's possible to grant access with a finer granularity than whole 
> pages.
>     -- Xen guarantees that they can be revoked quickly (a normal map grant can
>        only be revoked with the cooperation of the domain which has been 
> granted
>        access).
> 

Almost all my comments will apply to the transitive grants patch too.

> Signed-off-by: Annie Li <annie.li@xxxxxxxxxx>
> ---
>  drivers/xen/grant-table.c |   41 +++++++++++++++++++++++++++++++++++++++++
>  include/xen/grant_table.h |   13 +++++++++++++
>  2 files changed, 54 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index bd325fd..7a0f4d1 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -261,6 +261,47 @@ int gnttab_grant_foreign_access(domid_t domid, unsigned 
> long frame,
>  }
>  EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access);
>  
> +int gnttab_grant_foreign_access_subpage_v2(domid_t domid, unsigned long 
> frame,
> +                                        int flags, unsigned page_off,
> +                                        unsigned length)

Please drop the v2 suffixes on the public functions.

Any reason not to route these via the ops table for consistency with all
the other ops? Then your availability check becomes a test for NULL fn
pointer rather than a specific version.

> +{
> +     int ref;
> +
> +     ref = get_free_entries(1);
> +     if (unlikely(ref < 0))
> +             return -ENOSPC;
> +
> +     gnttab_grant_foreign_access_ref_subpage_v2(ref, domid, frame, flags,
> +                                                page_off, length);
> +
> +     return ref;
> +}
> +EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_subpage_v2);
> +
> +void gnttab_grant_foreign_access_ref_subpage_v2(grant_ref_t ref, domid_t 
> domid,
> +                                             unsigned long frame, int flags,
> +                                             unsigned page_off,
> +                                             unsigned length)
> +{
> +     BUG_ON(flags & (GTF_accept_transfer | GTF_reading |
> +                     GTF_writing | GTF_transitive));
> +     BUG_ON(grant_table_version == 1);

Returning -Esomething might be less drastic? ENOSYS perhaps?

> +     gnttab_shared.v2[ref].sub_page.frame = frame;
> +     gnttab_shared.v2[ref].sub_page.page_off = page_off;
> +     gnttab_shared.v2[ref].sub_page.length = length;
> +     gnttab_shared.v2[ref].hdr.domid = domid;
> +     wmb();
> +     gnttab_shared.v2[ref].hdr.flags =
> +                             GTF_permit_access | GTF_sub_page | flags;
> +}
> +EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_ref_subpage_v2);
> +
> +bool gnttab_subpage_trans_grants_available(void)
> +{
> +     return grant_table_version == 2;
> +}

It's not clear this adds anything over and above letting the user query
the grant table version. It's hard to tell since there are no users
presented here though. Perhaps separate subpage and transitive functions
would be cleaner?

> +EXPORT_SYMBOL_GPL(gnttab_subpage_trans_grants_available);
> +
>  static int gnttab_query_foreign_access_v1(grant_ref_t ref)
>  {
>       return gnttab_shared.v1[ref].flags & (GTF_reading|GTF_writing);
> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> index fea4954..7e43652 100644
> --- a/include/xen/grant_table.h
> +++ b/include/xen/grant_table.h
> @@ -62,6 +62,15 @@ int gnttab_resume(void);
>  
>  int gnttab_grant_foreign_access(domid_t domid, unsigned long frame,
>                               int readonly);
> +int gnttab_grant_foreign_access_subpage_v2(domid_t domid, unsigned long 
> frame,
> +                                        int flags, unsigned page_off,
> +                                        unsigned length);

> +
> +/*
> + * Are sub-page or transitive grants available on this version of Xen?  
> Returns
> + * true if they are, and false if they're not.
> + */
> +bool gnttab_subpage_trans_grants_available(void);
>  
>  /*
>   * End access through the given grant reference, iff the grant entry is no
> @@ -108,6 +117,10 @@ void gnttab_cancel_free_callback(struct 
> gnttab_free_callback *callback);
>  
>  void gnttab_grant_foreign_access_ref(grant_ref_t ref, domid_t domid,
>                                    unsigned long frame, int readonly);
> +void gnttab_grant_foreign_access_ref_subpage_v2(grant_ref_t ref, domid_t 
> domid,
> +                                             unsigned long frame, int flags,
> +                                             unsigned page_off,
> +                                             unsigned length);
>  
>  void gnttab_grant_foreign_transfer_ref(grant_ref_t, domid_t domid,
>                                      unsigned long pfn);



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel