WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH 1/3] Introducing grant table V2 stucture



On 2011-11-9 21:28, Ian Campbell wrote:
On Wed, 2011-11-09 at 08:14 +0000, annie.li@xxxxxxxxxx wrote:
  
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 4f44b34..0d481a9 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -53,7 +53,7 @@
 /* External tools reserve first few grant table entries. */
 #define NR_RESERVED_ENTRIES 8
 #define GNTTAB_LIST_END 0xffffffff
-#define GREFS_PER_GRANT_FRAME (PAGE_SIZE / sizeof(struct grant_entry))
+#define GREFS_PER_GRANT_FRAME (PAGE_SIZE / sizeof(struct grant_entry_v1))
    

Does this need to become GREFS_V1_PER... or something?
  
This is common definition for both V1 and V2, and is used by both v1 and v2 implementation. If it is changed to GREFS_V1_PER... or something with v1 tag in this patch, it is necessary to change it back in following "grant table v2 implementation" patch again. So I just keep it unchanged here.
  
 static grant_ref_t **gnttab_list;
 static unsigned int nr_grant_frames;
@@ -64,7 +64,72 @@ static DEFINE_SPINLOCK(gnttab_list_lock);
 unsigned long xen_hvm_resume_frames;
 EXPORT_SYMBOL_GPL(xen_hvm_resume_frames);

-static struct grant_entry *shared;
+static union {
+       struct grant_entry_v1 *v1;
+       void *ring_addr;
+} shared;
+
+/*
+ * This function is null for grant table v1, adding it here in order to keep
+ * consistent with *_v2 interface.
+ */
+static int gnttab_map_status_v1(unsigned int nr_gframes);
+/*
+ * This function is null for grant table v1, adding it here in order to keep
+ * consistent with *_v2 interface.
+ */
+static void gnttab_unmap_status_v1(void);
    

If you reorder the declarations of gnttab_request_version and
gnttab_(un)map_status_v1 below then you can avoid these forward
declarations.
  
Yes, you are right, thanks.
Also I don't think the comment really adds much once you have the empty
declaration underneath (like you do below). A simple "/* Nothing to do
for v1 */" in the implementation would be sufficient.
  
I agree.
  
+
+/*This is a structure of function pointers for grant table v1*/
    

and eventually v2, right? Actually I think the v1 is unnecessary. e.g.:
	/*This is a structure of function pointers for grant table*/

  
Thanks, will change it.

  
+static struct {
+       /*
+        * Mapping a list of frames for storing grant entry status, this
+        * mechanism can allow better synchronization using barriers. Input
+        * parameter is frame number, returning GNTST_okay means success and
+        * negative value means failure.
+        */
+       int (*_gnttab_map_status)(unsigned int);
    

I think you can drop the "_gnttab_" prefix from all of these, it'll be
clear from the gnttab->foo what the namespace is.
  
Ok, no problem.
[...]
  
+} gnttab_interface;
+
+static int grant_table_version;

 static struct gnttab_free_callback *gnttab_free_callback_list;

    

  
@@ -155,12 +229,10 @@ static void update_grant_entry(grant_ref_t ref, domid_t domid,
         *  3. Write memory barrier (WMB).
         *  4. Write ent->flags, inc. valid type.
         */
-       shared[ref].frame = frame;
-       shared[ref].domid = domid;
-       wmb();
-       shared[ref].flags = flags;
+       gnttab_interface._update_grant_entry(ref, domid, frame, flags);
 }

+
    

Please avoid unrelated whitespace changes.
  
Oh, it was ignored. Thanks, i will delete it.
  
 /*
  * Public grant-issuing interface functions
  */
@@ -187,31 +259,40 @@ int gnttab_grant_foreign_access(domid_t domid, unsigned long frame,
 }
 EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access);

-int gnttab_query_foreign_access(grant_ref_t ref)
+int gnttab_query_foreign_access_v1(grant_ref_t ref)
    

This and all the other similar functions can now be made static.
  
Ok, got it, thanks.

Thanks
Annie
Ian.

  
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
<Prev in Thread] Current Thread [Next in Thread>