Xen devel - Dec 2011 - xen: patches for supporting sub-page and transitive grants

Hi

Following two patches introduce and implement interfaces for sub-page 
and transitive grants, and they are based on linux-next branch of 
linux/kernel/git/konrad/xen.git(3.2.0-rc2+). Sub-page and transitive 
grants are new types of grant table V2.

Descriptions for those patches:

Through sub-page grant table interfaces, a domain can grant another 
domain access to a range of bytes within a page, and Xen will then 
prevent the grantee domain accessing outside that range.  For obvious 
reasons, it isn''t possible to map these grant references, and domains 
are expected to use the grant copy hypercall instead.

Through transitive grant interfaces,  a domain can create grant 
reference which redirects to another grant reference, so that any 
attempt to access the first grant reference will be redirected to the 
second one.  This is used to implement receiver-side copy on 
inter-domain traffic: rather than copying the packet in dom0, dom0 
creates a transitive grant referencing the original transmit buffer, and 
passes that to the receiving domain.

Diff:

  drivers/xen/grant-table.c |   74 
+++++++++++++++++++++++++++++++++++++++++++++
  include/xen/grant_table.h |   19 +++++++++++
  2 files changed, 93 insertions(+), 0 deletions(-)

Shortlog:

Annie Li (2):
       xen/granttable: Support sub-page grants
       xen/granttable: Support transitive grants

Thanks
Annie

From: Annie Li <annie.li@oracle.com>

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

Signed-off-by: Annie Li <annie.li@oracle.com>
---
 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)
+{
+	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);
+	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;
+}
+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);
-- 
1.7.6.4

From: Annie Li <annie.li@oracle.com>

These allow a domain A which has been granted access on a page of domain
B''s
memory to issue domain C with a copy-grant on the same page.  This is useful
e.g. for forwarding packets between domains.

Signed-off-by: Annie Li <annie.li@oracle.com>
---
 drivers/xen/grant-table.c |   33 +++++++++++++++++++++++++++++++++
 include/xen/grant_table.h |    6 ++++++
 2 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 7a0f4d1..d64b7c5 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -296,6 +296,39 @@ void gnttab_grant_foreign_access_ref_subpage_v2(grant_ref_t
ref, domid_t domid,
 }
 EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_ref_subpage_v2);
 
+int gnttab_grant_foreign_access_trans_v2(domid_t domid, int flags,
+					 domid_t trans_domid,
+					 grant_ref_t trans_gref)
+{
+	int ref;
+
+	ref = get_free_entries(1);
+	if (unlikely(ref < 0))
+		return -ENOSPC;
+
+	gnttab_grant_foreign_access_ref_trans_v2(ref, domid, flags,
+						 trans_domid, trans_gref);
+
+	return ref;
+}
+EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_trans_v2);
+
+void gnttab_grant_foreign_access_ref_trans_v2(grant_ref_t ref, domid_t domid,
+					      int flags, domid_t trans_domid,
+					      grant_ref_t trans_gref)
+{
+	BUG_ON(flags & (GTF_accept_transfer | GTF_reading |
+			GTF_writing | GTF_sub_page));
+	BUG_ON(grant_table_version == 1);
+	gnttab_shared.v2[ref].transitive.trans_domid = trans_domid;
+	gnttab_shared.v2[ref].transitive.gref = trans_gref;
+	gnttab_shared.v2[ref].hdr.domid = domid;
+	wmb();
+	gnttab_shared.v2[ref].hdr.flags +				GTF_permit_access | GTF_transitive |
flags;
+}
+EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_ref_trans_v2);
+
 bool gnttab_subpage_trans_grants_available(void)
 {
 	return grant_table_version == 2;
diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index 7e43652..35a8c73 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -65,6 +65,9 @@ int gnttab_grant_foreign_access(domid_t domid, unsigned long
frame,
 int gnttab_grant_foreign_access_subpage_v2(domid_t domid, unsigned long frame,
 					   int flags, unsigned page_off,
 					   unsigned length);
+int gnttab_grant_foreign_access_trans_v2(domid_t domid, int flags,
+					 domid_t trans_domid,
+					 grant_ref_t trans_gref);
 
 /*
  * Are sub-page or transitive grants available on this version of Xen?  Returns
@@ -121,6 +124,9 @@ 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_access_ref_trans_v2(grant_ref_t ref, domid_t domid,
+					      int flags, domid_t trans_domid,
+					      grant_ref_t trans_gref);
 
 void gnttab_grant_foreign_transfer_ref(grant_ref_t, domid_t domid,
 				       unsigned long pfn);
-- 
1.7.6.4

On Tue, 2011-12-06 at 10:56 +0000, annie.li@oracle.com
wrote:
> From: Annie Li <annie.li@oracle.com>
> 
>     -- 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@oracle.com>
> ---
>  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);

On 12/06/2011 03:42 AM, Ian Campbell wrote:
>> +{
>> +	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?
Yeah, BUG_ON shouldn''t be used for API misuse unless there''s
absolutely
no other way to handle it.
>
>> +	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?
Well, in general, specifically testing for features rather than
interface versions is better.

    J

Thanks for your reviewing, Ian.
>>   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.
OK, the initial interface is without v2 suffixes. It was added in order 
to reminder user the interfaces are only available for grant table v2.
But I am fine to remove it, and following ops fn pointers are
better.
> 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.
Ok, it is good.
How about following implements?

gnttab_v1_ops = {
  ...
.access_subpage = NULL;
.access_ref_subpage = NULL;
.access_trans = NULL;
.access_ref_trans = NULL;
}

gnttab_v2_ops = {
  ...
.access_subpage = access_subpage_v2;
.access_ref_subpage = access_ref_subpage_v2;
.access_trans = access_trans_v2;
.access_ref_trans = access_ref_trans_v2;
}

gnttab_request_version()
{
.....
    if(v2)
       gnttab_interface = &gnttab_v2_ops;
    else
       gnttab_interface = &gnttab_v1_ops;
.....
}

int gnttab_grant_foreign_access_subpage()
{
       if(gnttab_interface->access_subpage != NULL)
             return gnttab_interface->access_subpage;
       return Esomething;
}

Same operations for access_ref_subpage, access_trans and access_ref_trans.

bool gnttab_subpage_available()
{
      return (gnttab_interface->access_subpage != NULL);
}

bool gnttab_subpage_available()
{
      return (gnttab_interface->access_trans != NULL);
}

Thanks
Annie

>> Returning -Esomething might be less drastic? ENOSYS perhaps?
> Yeah, BUG_ON shouldn''t be used for API misuse unless
there''s absolutely
> no other way to handle it.
OK, will change it, thanks.
>>> +	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?
> Well, in general, specifically testing for features rather than
> interface versions is better.
Ok, will split it into 2 functions and check the function pointer 
instead of grant table version, thanks.

Thanks
Annie

> -----Original Message-----
> From: ANNIE LI [mailto:annie.li@oracle.com]
> Sent: 07 December 2011 03:36
> To: Ian Campbell
> Cc: xen-devel@lists.xensource.com; linux-kernel@vger.kernel.org;
> konrad.wilk@oracle.com; jeremy@goop.org; kurt.hackel@oracle.com;
> Paul Durrant
> Subject: Re: [PATCH 1/2] xen/granttable: Support sub-page grants
> 
> Thanks for your reviewing, Ian.
> >>   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.
> OK, the initial interface is without v2 suffixes. It was added in
> order to reminder user the interfaces are only available for grant
> table v2.
> But I am fine to remove it, and following ops fn pointers are
> better.
> > 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.
> Ok, it is good.
> How about following implements?
> 
> gnttab_v1_ops = {
>   ...
> .access_subpage = NULL;
> .access_ref_subpage = NULL;
> .access_trans = NULL;
> .access_ref_trans = NULL;
> }
> 
> gnttab_v2_ops = {
>   ...
> .access_subpage = access_subpage_v2;
> .access_ref_subpage = access_ref_subpage_v2; .access_trans >
access_trans_v2; .access_ref_trans = access_ref_trans_v2; }
> 

Do you need ops for the ref and non-ref functions? I would have thought you
could just have the ref ones since the all the non-ref variants do is allocate
and then call the ref variant.

  Paul
> gnttab_request_version()
> {
> .....
>     if(v2)
>        gnttab_interface = &gnttab_v2_ops;
>     else
>        gnttab_interface = &gnttab_v1_ops; .....
> }
> 
> int gnttab_grant_foreign_access_subpage()
> {
>        if(gnttab_interface->access_subpage != NULL)
>              return gnttab_interface->access_subpage;
>        return Esomething;
> }
> 
> Same operations for access_ref_subpage, access_trans and
> access_ref_trans.
> 
> bool gnttab_subpage_available()
> {
>       return (gnttab_interface->access_subpage != NULL); }
> 
> bool gnttab_subpage_available()
> {
>       return (gnttab_interface->access_trans != NULL); }
> 
> Thanks
> Annie

On Wed, 2011-12-07 at 03:36 +0000, ANNIE LI wrote:
> Thanks for your reviewing, Ian.
> >>   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.
> OK, the initial interface is without v2 suffixes. It was added in order 
> to reminder user the interfaces are only available for grant table v2.
> But I am fine to remove it, and following ops fn pointers are better.
> > 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.
> Ok, it is good.
> How about following implements?
Looks to be along the right lines. Thanks.
> gnttab_v1_ops = {
>   ...
> .access_subpage = NULL;
> .access_ref_subpage = NULL;
> .access_trans = NULL;
> .access_ref_trans = NULL;
> }
I think you can omit these since NULL is the default but perhaps
explicitly listing them is useful in a self documenting type way.

[...]
> 
> Same operations for access_ref_subpage, access_trans and access_ref_trans.
> 
> bool gnttab_subpage_available()
> {
>       return (gnttab_interface->access_subpage != NULL);
> }
> 
> bool gnttab_subpage_available()
Typo:       ..trans..
> {
>       return (gnttab_interface->access_trans != NULL);
> }
Ian.
> 
> Thanks
> Annie

On Wed, 2011-12-07 at 08:59 +0000, Paul Durrant wrote:
> > gnttab_v2_ops = {
> >   ...
> > .access_subpage = access_subpage_v2;
> > .access_ref_subpage = access_ref_subpage_v2; .access_trans > >
access_trans_v2; .access_ref_trans = access_ref_trans_v2; }
> > 
> 
> 
> Do you need ops for the ref and non-ref functions? I would have
> thought you could just have the ref ones since the all the non-ref
> variants do is allocate and then call the ref variant.
Good point. It appears that all the existing ops are only present in the
ref form.

Ian.

On 2011-12-7 17:56, Ian Campbell wrote:
> On Wed, 2011-12-07 at 03:36 +0000, ANNIE LI wrote:
>> Thanks for your reviewing, Ian.
>>>>    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.
>> OK, the initial interface is without v2 suffixes. It was added in order
>> to reminder user the interfaces are only available for grant table v2.
>> But I am fine to remove it, and following ops fn pointers are better.
>>> 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.
>> Ok, it is good.
>> How about following implements?
> Looks to be along the right lines. Thanks.
>
>> gnttab_v1_ops = {
>>    ...
>> .access_subpage = NULL;
>> .access_ref_subpage = NULL;
>> .access_trans = NULL;
>> .access_ref_trans = NULL;
>> }
> I think you can omit these since NULL is the default but perhaps
> explicitly listing them is useful in a self documenting type way.
>
> [...]
OK, I can delete those.
>> Same operations for access_ref_subpage, access_trans and
access_ref_trans.
>>
>> bool gnttab_subpage_available()
>> {
>>        return (gnttab_interface->access_subpage != NULL);
>> }
>>
>> bool gnttab_subpage_available()
> Typo:       ..trans..
Thanks for pointing out this.

Thanks
Annie
>> {
>>        return (gnttab_interface->access_trans != NULL);
>> }
> Ian.
>
>> Thanks
>> Annie
>

On 2011-12-7 17:57, Ian Campbell wrote:
> On Wed, 2011-12-07 at 08:59 +0000, Paul Durrant wrote:
>>> gnttab_v2_ops = {
>>>    ...
>>> .access_subpage = access_subpage_v2;
>>> .access_ref_subpage = access_ref_subpage_v2; .access_trans
>>> access_trans_v2; .access_ref_trans = access_ref_trans_v2; }
>>>
>>
>> Do you need ops for the ref and non-ref functions? I would have
>> thought you could just have the ref ones since the all the non-ref
>> variants do is allocate and then call the ref variant.
> Good point. It appears that all the existing ops are only present in the
> ref form.
So I will add two elements: access_ref_subpage and access_ref_trans into 
gnttab_v2_ops.

Thanks
Annie
> Ian.
>
>

On Wed, 2011-12-07 at 10:27 +0000, ANNIE LI wrote:
> 
> On 2011-12-7 17:57, Ian Campbell wrote:
> > On Wed, 2011-12-07 at 08:59 +0000, Paul Durrant wrote:
> >>> gnttab_v2_ops = {
> >>>    ...
> >>> .access_subpage = access_subpage_v2;
> >>> .access_ref_subpage = access_ref_subpage_v2; .access_trans
> >>> access_trans_v2; .access_ref_trans = access_ref_trans_v2; }
> >>>
> >>
> >> Do you need ops for the ref and non-ref functions? I would have
> >> thought you could just have the ref ones since the all the non-ref
> >> variants do is allocate and then call the ref variant.
> > Good point. It appears that all the existing ops are only present in
the
> > ref form.
> So I will add two elements: access_ref_subpage and access_ref_trans into 
> gnttab_v2_ops.
The existing convention seems to be for _ref to be a suffix, although
it''s only actually used on the end_*_ref ones.
+       int (*end_foreign_access_ref)(grant_ref_t, int);
+       unsigned long (*end_foreign_transfer_ref)(grant_ref_t);

Also it occurs to me that access_* sounds like something which uses a
ref rather than something which sets one up. The existing hook to setup
a normal grant is called "update_entry". Perhaps
update_{subpage,transitive}_entry?

Ian.

> +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));
This is slightly changed from the initial code, welcome your suggestions.

Initial condition is:
BUG_ON(flags & (GTF_accept_transfer | GTF_reading | GTF_writing | 
GTF_sub_page | GTF_permit_access));

GTF_sub_page | GTF_permit_access will be set later in this function, so 
it is necessary to verify this flag here.
GTF_transitive and GTF_sub_page can not be enabled at same time, so 
GTF_transitive is checked here to avoid those two flags are both enabled.

Same code was changed in corresponding transitive function.

Thanks
Annie
> +	BUG_ON(grant_table_version == 1);
> +	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;
> +}
>

> The existing convention seems to be for _ref to be a suffix, although
> it''s only actually used on the end_*_ref ones.
> +       int (*end_foreign_access_ref)(grant_ref_t, int);
> +       unsigned long (*end_foreign_transfer_ref)(grant_ref_t);
>
> Also it occurs to me that access_* sounds like something which uses a
> ref rather than something which sets one up. The existing hook to setup
> a normal grant is called "update_entry". Perhaps
> update_{subpage,transitive}_entry?
Yes, you are right.
Just like the existing code:
gnttab_grant_foreign_access   VS   gnttab_grant_foreign_access_subpage
update_entry in gnttab_grant_foreign_access_ref  VS  
update_{subpage,transitive}_entry in 
gnttab_grant_foreign_access_{subpage,trans}_ref

Thanks
Annie

> > 
> > gnttab_v1_ops = {
> >   ...
> > .access_subpage = NULL;
> > .access_ref_subpage = NULL;
> > .access_trans = NULL;
> > .access_ref_trans = NULL;
> > }
You don''t really need that. If you just leave it empty, they are all
set
to NULL.