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

[annie li <annie.li@xxxxxxxxxx>]

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