thr3ads.net - Xen devel - [Xen-devel] patches of upstreaming grant table version 2 [Nov 2011]

If this information is useful, please help other people find it:
Share via:

ANNIE LI

2011-Nov-09 07:58 UTC

head link

[Xen-devel] patches of upstreaming grant table version 2

ANNIE LI

2011-Nov-09 08:12 UTC

head link

[Xen-devel] patches of upstreaming grant table version 2

The following patches introduce and implement grant table version 2, and
they are based on v3.1.0-rc9+.

Descriptions for those patches:

1. In those patches, the grant table code supports both grant table v1
and v2 version, v2 is an extension from v1. Grant table of guest domain
can be either v1 or v2 version, and every grant table entry on one guest
should be the same version.

2. Full page structure of grant table v2 play the same role as grant
table v1. Although full page structure is different from v1, grant table
2 is totally backwards compatible with v1. Grant table is shared between
guest and Xen, domu and dom0 all have their own grant table shared with
Xen, and their grant table version should be set before any grants are
activated. When domu grants an entry to dom0 to map a frame, following
are steps:
* domu introduces a grant entry by reference
* domu informs dom0 the gref
* dom0 sends hypercall to map frame through this reference, Xen copy
shared entry to active entry and update frame
* dom0 does its work and release the frame, Xen releases the entry.
* domu redo those steps for a new cycle.

Xen mapping process can be found in function __gnttab_map_grant_ref in
link:
http://xenbits.xen.org/hg/xen-4.1-testing.hg/file/81e39a4978ea/xen/common/grant_table.c#l2172

3. If dom0 supports grant table v2, guests run on it can either supports
v1 or v2. Xen is responsible to judge what version the guests are using.
This is implemented in link:
http://xenbits.xen.org/hg/xen-4.1-testing.hg/file/81e39a4978ea/xen/common/grant_table.c#l2172.
Key word is: rd->grant_table->gt_version.

4. Grant table2 is a precondition to netchannel2 mechanism. Netchannel2
imports a new device type: vif2. To support this type, we need new
netback2, netfront2 driver and modification in Xen. If all are ready,
two types of vif can be supported: vif and vif2. vif is for original
backend and frontend, vif2 is for netback2 and netfront2.

Thanks
Annie

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

annie.li@oracle.com

2011-Nov-09 08:14 UTC

head link

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

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

This patch introduces new structures of grant table V2, grant table V2 is an
extension from V1. Grant table is shared between guest and Xen, and Xen is
responsible to do corresponding work for grant operations, such as: figure
out guest''s grant table version, perform different actions based on
different grant table version, etc. Although full-page structure of V2
is different from V1, it play the same role as V1.

This patch also changes existing grant table V1 interfaces into **_V1 style in
order to keep consistence with **_V2 interfaces.

Signed-off-by: Annie Li <annie.li@oracle.com>
---
 arch/x86/xen/grant-table.c          |    7 +-
 drivers/xen/grant-table.c           |  181 ++++++++++++++++++++++++++++++-----
 include/xen/grant_table.h           |    4 +-
 include/xen/interface/grant_table.h |  167 +++++++++++++++++++++++++++++++-
 include/xen/interface/xen.h         |    2 +
 5 files changed, 326 insertions(+), 35 deletions(-)

diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c
index 49ba9b5..65ce508 100644
--- a/arch/x86/xen/grant-table.c
+++ b/arch/x86/xen/grant-table.c
@@ -64,10 +64,10 @@ static int unmap_pte_fn(pte_t *pte, struct page *pmd_page,
 
 int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
 			   unsigned long max_nr_gframes,
-			   struct grant_entry **__shared)
+			   void **__shared)
 {
 	int rc;
-	struct grant_entry *shared = *__shared;
+	void *shared = *__shared;
 
 	if (shared == NULL) {
 		struct vm_struct *area @@ -83,8 +83,7 @@ int arch_gnttab_map_shared(unsigned
long *frames, unsigned long nr_gframes,
 	return rc;
 }
 
-void arch_gnttab_unmap_shared(struct grant_entry *shared,
-			      unsigned long nr_gframes)
+void arch_gnttab_unmap_shared(void *shared, unsigned long nr_gframes)
 {
 	apply_to_page_range(&init_mm, (unsigned long)shared,
 			    PAGE_SIZE * nr_gframes, unmap_pte_fn, NULL);
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))
 
 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);
+
+/*This is a structure of function pointers for grant table v1*/
+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);
+	/*
+	 * Release a list of frames which are mapped in _gnttab_map_status for
+	 * grant entry status.
+	 */
+	void (*_gnttab_unmap_status)(void);
+	/*
+	 * Introducing a valid entry into the grant table, granting the frame
+	 * of this grant entry to domain for accessing, or transfering, or
+	 * transitively accessing. First input parameter is reference of this
+	 * introduced grant entry, second one is domid of granted domain, third
+	 * one is the frame to be granted, and the last one is status of the
+	 * grant entry to be updated.
+	 */
+	void (*_update_grant_entry)(grant_ref_t, domid_t,
+		unsigned long, unsigned);
+	/*
+	 * Stop granting a grant entry to domain for accessing. First input
+	 * parameter is reference of a grant entry whose grant access will be
+	 * stopped, second one is not in use now. If the grant entry is
+	 * currently mapped for reading or writing, just return failure(==0)
+	 * directly and don''t tear down the grant access. Otherwise, stop
grant
+	 * access for this entry and return success(==1).
+	 */
+	int (*_gnttab_end_foreign_access_ref)(grant_ref_t, int);
+	/*
+	 * Stop granting a grant entry to domain for transfer. If tranfer has
+	 * not started, just reclaim the grant entry and return failure(==0).
+	 * Otherwise, wait for the transfer to complete and then return the
+	 * frame.
+	 */
+	unsigned long (*_gnttab_end_foreign_transfer_ref)(grant_ref_t);
+	/*
+	 * Query the status of a grant entry. Input parameter is reference of
+	 * queried grant entry, return value is the status of queried entry.
+	 * Detailed status(writing/reading) can be gotten from the return value
+	 * by bit operations.
+	 */
+	int (*_gnttab_query_foreign_access)(grant_ref_t);
+} gnttab_interface;
+
+static int grant_table_version;
 
 static struct gnttab_free_callback *gnttab_free_callback_list;
 
@@ -142,6 +207,15 @@ static void put_free_entry(grant_ref_t ref)
 	spin_unlock_irqrestore(&gnttab_list_lock, flags);
 }
 
+static void update_grant_entry_v1(grant_ref_t ref, domid_t domid,
+				  unsigned long frame, unsigned flags)
+{
+	shared.v1[ref].frame = frame;
+	shared.v1[ref].domid = domid;
+	wmb();
+	shared.v1[ref].flags = flags;
+}
+
 static void update_grant_entry(grant_ref_t ref, domid_t domid,
 			       unsigned long frame, unsigned flags)
 {
@@ -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);
 }
 
+
 /*
  * 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)
 {
-	u16 nflags;
-
-	nflags = shared[ref].flags;
+	return shared.v1[ref].flags & (GTF_reading|GTF_writing);
+}
 
-	return (nflags & (GTF_reading|GTF_writing));
+int gnttab_query_foreign_access(grant_ref_t ref)
+{
+	return gnttab_interface._gnttab_query_foreign_access(ref);
 }
 EXPORT_SYMBOL_GPL(gnttab_query_foreign_access);
 
-int gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly)
+int gnttab_end_foreign_access_ref_v1(grant_ref_t ref, int readonly)
 {
 	u16 flags, nflags;
+	u16 *pflags;
 
-	nflags = shared[ref].flags;
+	pflags = &shared.v1[ref].flags;
+	nflags = *pflags;
 	do {
 		flags = nflags;
 		if (flags & (GTF_reading|GTF_writing)) {
 			printk(KERN_ALERT "WARNING: g.e. still in use!\n");
 			return 0;
 		}
-	} while ((nflags = sync_cmpxchg(&shared[ref].flags, flags, 0)) != flags);
+	} while ((nflags = sync_cmpxchg(&shared.v1[ref].flags, flags, 0))
+			!= flags);
 
 	return 1;
 }
+
+int gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly)
+{
+	return gnttab_interface._gnttab_end_foreign_access_ref(ref, readonly);
+}
 EXPORT_SYMBOL_GPL(gnttab_end_foreign_access_ref);
 
 void gnttab_end_foreign_access(grant_ref_t ref, int readonly,
@@ -250,33 +331,41 @@ void gnttab_grant_foreign_transfer_ref(grant_ref_t ref,
domid_t domid,
 }
 EXPORT_SYMBOL_GPL(gnttab_grant_foreign_transfer_ref);
 
-unsigned long gnttab_end_foreign_transfer_ref(grant_ref_t ref)
+unsigned long gnttab_end_foreign_transfer_ref_v1(grant_ref_t ref)
 {
 	unsigned long frame;
 	u16           flags;
+	u16          *pflags;
+
+	pflags = &shared.v1[ref].flags;
 
 	/*
 	 * If a transfer is not even yet started, try to reclaim the grant
 	 * reference and return failure (== 0).
 	 */
-	while (!((flags = shared[ref].flags) & GTF_transfer_committed)) {
-		if (sync_cmpxchg(&shared[ref].flags, flags, 0) == flags)
+	while (!((flags = *pflags) & GTF_transfer_committed)) {
+		if (sync_cmpxchg(pflags, flags, 0) == flags)
 			return 0;
 		cpu_relax();
 	}
 
 	/* If a transfer is in progress then wait until it is completed. */
 	while (!(flags & GTF_transfer_completed)) {
-		flags = shared[ref].flags;
+		flags = *pflags;
 		cpu_relax();
 	}
 
 	rmb();	/* Read the frame number /after/ reading completion status. */
-	frame = shared[ref].frame;
+	frame = shared.v1[ref].frame;
 	BUG_ON(frame == 0);
 
 	return frame;
 }
+
+unsigned long gnttab_end_foreign_transfer_ref(grant_ref_t ref)
+{
+	return gnttab_interface._gnttab_end_foreign_transfer_ref(ref);
+}
 EXPORT_SYMBOL_GPL(gnttab_end_foreign_transfer_ref);
 
 unsigned long gnttab_end_foreign_transfer(grant_ref_t ref)
@@ -520,6 +609,32 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref
*unmap_ops,
 }
 EXPORT_SYMBOL_GPL(gnttab_unmap_refs);
 
+static void gnttab_request_version(void)
+{
+	grant_table_version = 1;
+	gnttab_interface._gnttab_map_status = gnttab_map_status_v1;
+	gnttab_interface._gnttab_unmap_status = gnttab_unmap_status_v1;
+	gnttab_interface._update_grant_entry = update_grant_entry_v1;
+	gnttab_interface._gnttab_end_foreign_access_ref +				
gnttab_end_foreign_access_ref_v1;
+	gnttab_interface._gnttab_end_foreign_transfer_ref +				
gnttab_end_foreign_transfer_ref_v1;
+	gnttab_interface._gnttab_query_foreign_access +				
gnttab_query_foreign_access_v1;
+	printk(KERN_INFO "Grant tables using version %d layout.\n",
+		grant_table_version);
+}
+
+static int gnttab_map_status_v1(unsigned int nr_gframes)
+{
+	return 0;
+}
+
+static int gnttab_map_status(unsigned int nr_gframes)
+{
+	return gnttab_interface._gnttab_map_status(nr_gframes);
+}
+
 static int gnttab_map(unsigned int start_idx, unsigned int end_idx)
 {
 	struct gnttab_setup_table setup;
@@ -567,8 +682,14 @@ static int gnttab_map(unsigned int start_idx, unsigned int
end_idx)
 
 	BUG_ON(rc || setup.status);
 
+	rc = gnttab_map_status(nr_gframes);
+	if (rc < 0) {
+		kfree(frames);
+		return rc;
+	}
+
 	rc = arch_gnttab_map_shared(frames, nr_gframes, gnttab_max_grant_frames(),
-				    &shared);
+				    &shared.ring_addr);
 	BUG_ON(rc);
 
 	kfree(frames);
@@ -580,6 +701,7 @@ int gnttab_resume(void)
 {
 	unsigned int max_nr_gframes;
 
+	gnttab_request_version();
 	max_nr_gframes = gnttab_max_grant_frames();
 	if (max_nr_gframes < nr_grant_frames)
 		return -ENOSYS;
@@ -587,9 +709,10 @@ int gnttab_resume(void)
 	if (xen_pv_domain())
 		return gnttab_map(0, nr_grant_frames - 1);
 
-	if (!shared) {
-		shared = ioremap(xen_hvm_resume_frames, PAGE_SIZE * max_nr_gframes);
-		if (shared == NULL) {
+	if (!shared.ring_addr) {
+		shared.ring_addr = ioremap(xen_hvm_resume_frames,
+						PAGE_SIZE * max_nr_gframes);
+		if (shared.ring_addr == NULL) {
 			printk(KERN_WARNING
 					"Failed to ioremap gnttab share frames!");
 			return -ENOMEM;
@@ -601,9 +724,19 @@ int gnttab_resume(void)
 	return 0;
 }
 
+static void gnttab_unmap_status_v1(void)
+{
+}
+
+void gnttab_unmap_status(void)
+{
+	return gnttab_interface._gnttab_unmap_status();
+}
+
 int gnttab_suspend(void)
 {
-	arch_gnttab_unmap_shared(shared, nr_grant_frames);
+	arch_gnttab_unmap_shared(shared.ring_addr, nr_grant_frames);
+	gnttab_interface._gnttab_unmap_status();
 	return 0;
 }
 
diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index b1fab6b..405879d 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -146,8 +146,8 @@ gnttab_set_unmap_op(struct gnttab_unmap_grant_ref *unmap,
phys_addr_t addr,
 
 int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
 			   unsigned long max_nr_gframes,
-			   struct grant_entry **__shared);
-void arch_gnttab_unmap_shared(struct grant_entry *shared,
+			   void **__shared);
+void arch_gnttab_unmap_shared(void *shared,
 			      unsigned long nr_gframes);
 
 extern unsigned long xen_hvm_resume_frames;
diff --git a/include/xen/interface/grant_table.h
b/include/xen/interface/grant_table.h
index 39e5717..a17d844 100644
--- a/include/xen/interface/grant_table.h
+++ b/include/xen/interface/grant_table.h
@@ -85,12 +85,22 @@
  */
 
 /*
+ * Reference to a grant entry in a specified domain''s grant table.
+ */
+typedef uint32_t grant_ref_t;
+
+/*
  * A grant table comprises a packed array of grant entries in one or more
  * page frames shared between Xen and a guest.
  * [XEN]: This field is written by Xen and read by the sharing guest.
  * [GST]: This field is written by the guest and read by Xen.
  */
-struct grant_entry {
+
+/*
+ * Version 1 of the grant table entry structure is maintained purely
+ * for backwards compatibility.  New guests should use version 2.
+ */
+struct grant_entry_v1 {
     /* GTF_xxx: various type and flag information.  [XEN,GST] */
     uint16_t flags;
     /* The domain being granted foreign privileges. [GST] */
@@ -108,10 +118,13 @@ struct grant_entry {
  *  GTF_permit_access: Allow @domid to map/access @frame.
  *  GTF_accept_transfer: Allow @domid to transfer ownership of one page frame
  *                       to this guest. Xen writes the page number to @frame.
+ *  GTF_transitive: Allow @domid to transitively access a subrange of
+ *                  @trans_grant in @trans_domid.  No mappings are allowed.
  */
 #define GTF_invalid         (0U<<0)
 #define GTF_permit_access   (1U<<0)
 #define GTF_accept_transfer (2U<<0)
+#define GTF_transitive      (3U<<0)
 #define GTF_type_mask       (3U<<0)
 
 /*
@@ -119,6 +132,9 @@ struct grant_entry {
  *  GTF_readonly: Restrict @domid to read-only mappings and accesses. [GST]
  *  GTF_reading: Grant entry is currently mapped for reading by @domid. [XEN]
  *  GTF_writing: Grant entry is currently mapped for writing by @domid. [XEN]
+ *  GTF_sub_page: Grant access to only a subrange of the page.  @domid
+ *                will only be allowed to copy from the grant, and not
+ *                map it. [GST]
  */
 #define _GTF_readonly       (2)
 #define GTF_readonly        (1U<<_GTF_readonly)
@@ -126,6 +142,8 @@ struct grant_entry {
 #define GTF_reading         (1U<<_GTF_reading)
 #define _GTF_writing        (4)
 #define GTF_writing         (1U<<_GTF_writing)
+#define _GTF_sub_page       (8)
+#define GTF_sub_page        (1U<<_GTF_sub_page)
 
 /*
  * Subflags for GTF_accept_transfer:
@@ -142,15 +160,81 @@ struct grant_entry {
 #define _GTF_transfer_completed (3)
 #define GTF_transfer_completed  (1U<<_GTF_transfer_completed)
 
+/*
+ * Version 2 grant table entries.  These fulfil the same role as
+ * version 1 entries, but can represent more complicated operations.
+ * Any given domain will have either a version 1 or a version 2 table,
+ * and every entry in the table will be the same version.
+ *
+ * The interface by which domains use grant references does not depend
+ * on the grant table version in use by the other domain.
+ */
 
-/***********************************
- * GRANT TABLE QUERIES AND USES
+/*
+ * Version 1 and version 2 grant entries share a common prefix.  The
+ * fields of the prefix are documented as part of struct
+ * grant_entry_v1.
  */
+struct grant_entry_header {
+    uint16_t flags;
+    domid_t  domid;
+};
 
 /*
- * Reference to a grant entry in a specified domain''s grant table.
+ * Version 2 of the grant entry structure, here is an union because three
+ * different types are suppotted: full_page, sub_page and transitive.
+ */
+union grant_entry_v2 {
+    struct grant_entry_header hdr;
+
+    /*
+     * This member is used for V1-style full page grants, where either:
+     *
+     * -- hdr.type is GTF_accept_transfer, or
+     * -- hdr.type is GTF_permit_access and GTF_sub_page is not set.
+     *
+     * In that case, the frame field has the same semantics as the
+     * field of the same name in the V1 entry structure.
+     */
+    struct {
+	struct grant_entry_header hdr;
+	uint32_t pad0;
+	uint64_t frame;
+    } full_page;
+
+    /*
+     * If the grant type is GTF_grant_access and GTF_sub_page is set,
+     * @domid is allowed to access bytes [@page_off,@page_off+@length)
+     * in frame @frame.
+     */
+    struct {
+	struct grant_entry_header hdr;
+	uint16_t page_off;
+	uint16_t length;
+	uint64_t frame;
+    } sub_page;
+
+    /*
+     * If the grant is GTF_transitive, @domid is allowed to use the
+     * grant @gref in domain @trans_domid, as if it was the local
+     * domain.  Obviously, the transitive access must be compatible
+     * with the original grant.
+     */
+    struct {
+	struct grant_entry_header hdr;
+	domid_t trans_domid;
+	uint16_t pad0;
+	grant_ref_t gref;
+    } transitive;
+
+    uint32_t __spacer[4]; /* Pad to a power of two */
+};
+
+typedef uint16_t grant_status_t;
+
+/***********************************
+ * GRANT TABLE QUERIES AND USES
  */
-typedef uint32_t grant_ref_t;
 
 /*
  * Handle to track a mapping created via a grant reference.
@@ -322,6 +406,79 @@ struct gnttab_query_size {
 DEFINE_GUEST_HANDLE_STRUCT(gnttab_query_size);
 
 /*
+ * GNTTABOP_unmap_and_replace: Destroy one or more grant-reference mappings
+ * tracked by <handle> but atomically replace the page table entry with
one
+ * pointing to the machine address under <new_addr>.  <new_addr>
will be
+ * redirected to the null entry.
+ * NOTES:
+ *  1. The call may fail in an undefined manner if either mapping is not
+ *     tracked by <handle>.
+ *  2. After executing a batch of unmaps, it is guaranteed that no stale
+ *     mappings will remain in the device or host TLBs.
+ */
+#define GNTTABOP_unmap_and_replace    7
+struct gnttab_unmap_and_replace {
+    /* IN parameters. */
+    uint64_t host_addr;
+    uint64_t new_addr;
+    grant_handle_t handle;
+    /* OUT parameters. */
+    int16_t  status;              /* GNTST_* */
+};
+DEFINE_GUEST_HANDLE_STRUCT(gnttab_unmap_and_replace);
+
+/*
+ * GNTTABOP_set_version: Request a particular version of the grant
+ * table shared table structure.  This operation can only be performed
+ * once in any given domain.  It must be performed before any grants
+ * are activated; otherwise, the domain will be stuck with version 1.
+ * The only defined versions are 1 and 2.
+ */
+#define GNTTABOP_set_version          8
+struct gnttab_set_version {
+    /* IN parameters */
+    uint32_t version;
+};
+DEFINE_GUEST_HANDLE_STRUCT(gnttab_set_version);
+
+/*
+ * GNTTABOP_get_status_frames: Get the list of frames used to store grant
+ * status for <dom>. In grant format version 2, the status is separated
+ * from the other shared grant fields to allow more efficient synchronization
+ * using barriers instead of atomic cmpexch operations.
+ * <nr_frames> specify the size of vector <frame_list>.
+ * The frame addresses are returned in the <frame_list>.
+ * Only <nr_frames> addresses are returned, even if the table is larger.
+ * NOTES:
+ *  1. <dom> may be specified as DOMID_SELF.
+ *  2. Only a sufficiently-privileged domain may specify <dom> !=
DOMID_SELF.
+ */
+#define GNTTABOP_get_status_frames     9
+struct gnttab_get_status_frames {
+    /* IN parameters. */
+    uint32_t nr_frames;
+    domid_t  dom;
+    /* OUT parameters. */
+    int16_t  status;              /* GNTST_* */
+    GUEST_HANDLE(uint64_t) frame_list;
+};
+DEFINE_GUEST_HANDLE_STRUCT(gnttab_get_status_frames);
+
+/*
+ * GNTTABOP_get_version: Get the grant table version which is in
+ * effect for domain <dom>.
+ */
+#define GNTTABOP_get_version          10
+struct gnttab_get_version {
+    /* IN parameters */
+    domid_t dom;
+    uint16_t pad;
+    /* OUT parameters */
+    uint32_t version;
+};
+DEFINE_GUEST_HANDLE_STRUCT(gnttab_get_version);
+
+/*
  * Bitfield values for update_pin_status.flags.
  */
  /* Map the grant entry for access by I/O devices. */
diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
index 6acd9ce..9353bdf 100644
--- a/include/xen/interface/xen.h
+++ b/include/xen/interface/xen.h
@@ -522,6 +522,8 @@ struct tmem_op {
 	} u;
 };
 
+DEFINE_GUEST_HANDLE(uint64_t);
+
 #else /* __ASSEMBLY__ */
 
 /* In assembly code we cannot use C numeric constant suffixes. */
-- 
1.7.6


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

annie.li@oracle.com

2011-Nov-09 08:15 UTC

head link

[Xen-devel] [PATCH 2/3] Grant tables v2 implementation.

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

Receiver-side copying of packets is based on this implementation, it gives
better performance and better CPU accounting. It totally supports three types:
full-page, sub-page and transitive grants.

However this patch does not cover sub-page and transitive grants, it mainly
focus on Full-page part and implements grant table V2 interfaces corresponding
to what already exists in grant table V1, such as: grant table V2
initialization, mapping, releasing and exported interfaces.

Each guest can only supports one type of grant table type, every entry in grant
table should be the same version. It is necessary to set V1 or V2 version before
initializing the grant table.

Grant table exported interfaces of V2 are same with those of V1, Xen is
responsible to judge what grant table version guests are using in every grant
operation.

V2 fulfills the same role of V1, and it is totally backwards compitable with V1.
If dom0 support grant table V2, the guests runing on it can run with either V1
or V2.

Signed-off-by: Annie Li <annie.li@oracle.com>
---
 arch/x86/xen/grant-table.c |   34 ++++++++-
 drivers/xen/grant-table.c  |  194 +++++++++++++++++++++++++++++++++++++++++---
 include/xen/grant_table.h  |    6 +-
 3 files changed, 220 insertions(+), 14 deletions(-)

diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c
index 65ce508..3e9f936 100644
--- a/arch/x86/xen/grant-table.c
+++ b/arch/x86/xen/grant-table.c
@@ -54,6 +54,17 @@ static int map_pte_fn(pte_t *pte, struct page *pmd_page,
 	return 0;
 }
 
+/*32bits is not enough since Xen supports sparse physical memory*/
+static int map_pte_fn_status(pte_t *pte, struct page *pmd_page,
+			     unsigned long addr, void *data)
+{
+	uint64_t **frames = (uint64_t **)data;
+
+	set_pte_at(&init_mm, addr, pte, mfn_pte((*frames)[0], PAGE_KERNEL));
+	(*frames)++;
+	return 0;
+}
+
 static int unmap_pte_fn(pte_t *pte, struct page *pmd_page,
 			unsigned long addr, void *data)
 {
@@ -83,7 +94,28 @@ int arch_gnttab_map_shared(unsigned long *frames, unsigned
long nr_gframes,
 	return rc;
 }
 
-void arch_gnttab_unmap_shared(void *shared, unsigned long nr_gframes)
+int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
+			   unsigned long max_nr_gframes,
+			   grant_status_t **__shared)
+{
+	int rc;
+	grant_status_t *shared = *__shared;
+
+	if (shared == NULL) {
+		struct vm_struct *area +			xen_alloc_vm_area(PAGE_SIZE * max_nr_gframes);
+		BUG_ON(area == NULL);
+		shared = area->addr;
+		*__shared = shared;
+	}
+
+	rc = apply_to_page_range(&init_mm, (unsigned long)shared,
+				 PAGE_SIZE * nr_gframes,
+				 map_pte_fn_status, &frames);
+	return rc;
+}
+
+void arch_gnttab_unmap(void *shared, unsigned long nr_gframes)
 {
 	apply_to_page_range(&init_mm, (unsigned long)shared,
 			    PAGE_SIZE * nr_gframes, unmap_pte_fn, NULL);
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 0d481a9..a3294a26 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -44,6 +44,7 @@
 #include <xen/page.h>
 #include <xen/grant_table.h>
 #include <xen/interface/memory.h>
+#include <xen/hvc-console.h>
 #include <asm/xen/hypercall.h>
 
 #include <asm/pgtable.h>
@@ -53,7 +54,10 @@
 /* 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_v1))
+#define GREFS_PER_GRANT_FRAME \
+(grant_table_version == 1 ?                      \
+ (PAGE_SIZE / sizeof(struct grant_entry_v1)) :   \
+ (PAGE_SIZE / sizeof(union grant_entry_v2)))
 
 static grant_ref_t **gnttab_list;
 static unsigned int nr_grant_frames;
@@ -66,6 +70,7 @@ EXPORT_SYMBOL_GPL(xen_hvm_resume_frames);
 
 static union {
 	struct grant_entry_v1 *v1;
+	union grant_entry_v2 *v2;
 	void *ring_addr;
 } shared;
 
@@ -75,12 +80,25 @@ static union {
  */
 static int gnttab_map_status_v1(unsigned int nr_gframes);
 /*
+ * Mapping grant entry status into a list of frames, returning GNTST_okay means
+ * success and negative value means failure.
+ */
+static int gnttab_map_status_v2(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);
+/*
+ * Release a list of frames mapped in gnttab_map_status_** for grant entry
+ * status.
+ */
+static void gnttab_unmap_status_v2(void);
 
-/*This is a structure of function pointers for grant table v1*/
+/*
+ * This is a structure of function pointers which allows v1 and v2 to use
+ * identical interface.
+ */
 static struct {
 	/*
 	 * Mapping a list of frames for storing grant entry status, this
@@ -129,6 +147,9 @@ static struct {
 	int (*_gnttab_query_foreign_access)(grant_ref_t);
 } gnttab_interface;
 
+/*This reflects status of grant entries, so act as a global value*/
+static grant_status_t *grstatus;
+
 static int grant_table_version;
 
 static struct gnttab_free_callback *gnttab_free_callback_list;
@@ -136,6 +157,7 @@ static struct gnttab_free_callback
*gnttab_free_callback_list;
 static int gnttab_expand(unsigned int req_entries);
 
 #define RPP (PAGE_SIZE / sizeof(grant_ref_t))
+#define SPP (PAGE_SIZE / sizeof(grant_status_t))
 
 static inline grant_ref_t *__gnttab_entry(grant_ref_t entry)
 {
@@ -216,6 +238,15 @@ static void update_grant_entry_v1(grant_ref_t ref, domid_t
domid,
 	shared.v1[ref].flags = flags;
 }
 
+static void update_grant_entry_v2(grant_ref_t ref, domid_t domid,
+				  unsigned long frame, unsigned flags)
+{
+	shared.v2[ref].full_page.frame = frame;
+	shared.v2[ref].hdr.domid = domid;
+	wmb();
+	shared.v2[ref].hdr.flags = GTF_permit_access | flags;
+}
+
 static void update_grant_entry(grant_ref_t ref, domid_t domid,
 			       unsigned long frame, unsigned flags)
 {
@@ -264,6 +295,11 @@ int gnttab_query_foreign_access_v1(grant_ref_t ref)
 	return shared.v1[ref].flags & (GTF_reading|GTF_writing);
 }
 
+int gnttab_query_foreign_access_v2(grant_ref_t ref)
+{
+	return grstatus[ref] & (GTF_reading|GTF_writing);
+}
+
 int gnttab_query_foreign_access(grant_ref_t ref)
 {
 	return gnttab_interface._gnttab_query_foreign_access(ref);
@@ -289,6 +325,29 @@ int gnttab_end_foreign_access_ref_v1(grant_ref_t ref, int
readonly)
 	return 1;
 }
 
+int gnttab_end_foreign_access_ref_v2(grant_ref_t ref, int readonly)
+{
+	shared.v2[ref].hdr.flags = 0;
+	mb();
+	if (grstatus[ref] & (GTF_reading|GTF_writing)) {
+		return 0;
+	} else {
+		/* The read of grstatus needs to have acquire
+		semantics.  On x86, reads already have
+		that, and we just need to protect against
+		compiler reorderings.  On other
+		architectures we may need a full
+		barrier. */
+#ifdef CONFIG_X86
+		barrier();
+#else
+		mb();
+#endif
+		}
+
+	return 1;
+}
+
 int gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly)
 {
 	return gnttab_interface._gnttab_end_foreign_access_ref(ref, readonly);
@@ -362,6 +421,37 @@ unsigned long
gnttab_end_foreign_transfer_ref_v1(grant_ref_t ref)
 	return frame;
 }
 
+unsigned long gnttab_end_foreign_transfer_ref_v2(grant_ref_t ref)
+{
+	unsigned long frame;
+	u16           flags;
+	u16          *pflags;
+
+	pflags = &shared.v2[ref].hdr.flags;
+
+	/*
+	 * If a transfer is not even yet started, try to reclaim the grant
+	 * reference and return failure (== 0).
+	 */
+	while (!((flags = *pflags) & GTF_transfer_committed)) {
+		if (sync_cmpxchg(pflags, flags, 0) == flags)
+			return 0;
+		cpu_relax();
+	}
+
+	/* If a transfer is in progress then wait until it is completed. */
+	while (!(flags & GTF_transfer_completed)) {
+		flags = *pflags;
+		cpu_relax();
+	}
+
+	rmb();  /* Read the frame number /after/ reading completion status. */
+	frame = shared.v2[ref].full_page.frame;
+	BUG_ON(frame == 0);
+
+	return frame;
+}
+
 unsigned long gnttab_end_foreign_transfer_ref(grant_ref_t ref)
 {
 	return gnttab_interface._gnttab_end_foreign_transfer_ref(ref);
@@ -588,7 +678,7 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
 EXPORT_SYMBOL_GPL(gnttab_map_refs);
 
 int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
-		struct page **pages, unsigned int count)
+		      struct page **pages, unsigned int count)
 {
 	int i, ret;
 
@@ -609,18 +699,54 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref
*unmap_ops,
 }
 EXPORT_SYMBOL_GPL(gnttab_unmap_refs);
 
+static unsigned nr_status_frames(unsigned nr_grant_frames)
+{
+	return (nr_grant_frames * GREFS_PER_GRANT_FRAME + SPP - 1) / SPP;
+}
+
 static void gnttab_request_version(void)
 {
-	grant_table_version = 1;
-	gnttab_interface._gnttab_map_status = gnttab_map_status_v1;
-	gnttab_interface._gnttab_unmap_status = gnttab_unmap_status_v1;
-	gnttab_interface._update_grant_entry = update_grant_entry_v1;
-	gnttab_interface._gnttab_end_foreign_access_ref +	int rc;
+	struct gnttab_set_version gsv;
+	const char *str = "we need grant tables version 2, but only version 1 is
available\n";
+
+	gsv.version = 2;
+	rc = HYPERVISOR_grant_table_op(GNTTABOP_set_version, &gsv, 1);
+	if (rc == 0) {
+		grant_table_version = 2;
+		gnttab_interface._gnttab_map_status = gnttab_map_status_v2;
+		gnttab_interface._gnttab_unmap_status = gnttab_unmap_status_v2;
+		gnttab_interface._update_grant_entry = update_grant_entry_v2;
+		gnttab_interface._gnttab_end_foreign_access_ref +				
gnttab_end_foreign_access_ref_v2;
+		gnttab_interface._gnttab_end_foreign_transfer_ref +				
gnttab_end_foreign_transfer_ref_v2;
+		gnttab_interface._gnttab_query_foreign_access +				
gnttab_query_foreign_access_v2;
+	} else {
+		if (grant_table_version == 2) {
+			/*
+			 * If we''ve already used version 2 features,
+			 * but then suddenly discover that they''re not
+			 * available (e.g. migrating to an older
+			 * version of Xen), almost unbounded badness
+			 * can happen.
+			 */
+			xen_raw_printk(str);
+			panic(str);
+		}
+		grant_table_version = 1;
+		gnttab_interface._gnttab_map_status = gnttab_map_status_v1;
+		gnttab_interface._gnttab_unmap_status = gnttab_unmap_status_v1;
+		gnttab_interface._update_grant_entry = update_grant_entry_v1;
+		gnttab_interface._gnttab_end_foreign_access_ref  				
gnttab_end_foreign_access_ref_v1;
-	gnttab_interface._gnttab_end_foreign_transfer_ref +	
gnttab_interface._gnttab_end_foreign_transfer_ref  				
gnttab_end_foreign_transfer_ref_v1;
-	gnttab_interface._gnttab_query_foreign_access +	
gnttab_interface._gnttab_query_foreign_access  				
gnttab_query_foreign_access_v1;
+
+	}
 	printk(KERN_INFO "Grant tables using version %d layout.\n",
 		grant_table_version);
 }
@@ -630,6 +756,44 @@ static int gnttab_map_status_v1(unsigned int nr_gframes)
 	return 0;
 }
 
+static int gnttab_map_status_v2(unsigned int nr_gframes)
+{
+	uint64_t *sframes;
+	unsigned int nr_sframes;
+	struct gnttab_get_status_frames getframes;
+	int rc;
+
+	nr_sframes = nr_status_frames(nr_gframes);
+
+	/* No need for kzalloc as it is initialized in following hyercall
+	 * GNTTABOP_get_status_frames.
+	 */
+	sframes = kmalloc(nr_sframes  * sizeof(uint64_t), GFP_ATOMIC);
+	if (!sframes)
+		return -ENOMEM;
+
+	getframes.dom        = DOMID_SELF;
+	getframes.nr_frames  = nr_sframes;
+	set_xen_guest_handle(getframes.frame_list, sframes);
+
+	rc = HYPERVISOR_grant_table_op(GNTTABOP_get_status_frames,
+				       &getframes, 1);
+	if (rc == -ENOSYS) {
+		kfree(sframes);
+		return -ENOSYS;
+	}
+
+	BUG_ON(rc || getframes.status);
+
+	rc = arch_gnttab_map_status(sframes, nr_sframes,
+				    nr_status_frames(gnttab_max_grant_frames()),
+				    &grstatus);
+	BUG_ON(rc);
+	kfree(sframes);
+
+	return 0;
+}
+
 static int gnttab_map_status(unsigned int nr_gframes)
 {
 	return gnttab_interface._gnttab_map_status(nr_gframes);
@@ -666,6 +830,9 @@ static int gnttab_map(unsigned int start_idx, unsigned int
end_idx)
 		return rc;
 	}
 
+	/* No need for kzalloc as it is initialized in following hyercall
+	 * GNTTABOP_setup_table.
+	 */
 	frames = kmalloc(nr_gframes * sizeof(unsigned long), GFP_ATOMIC);
 	if (!frames)
 		return -ENOMEM;
@@ -728,6 +895,11 @@ static void gnttab_unmap_status_v1(void)
 {
 }
 
+static void gnttab_unmap_status_v2(void)
+{
+	arch_gnttab_unmap(grstatus, nr_status_frames(nr_grant_frames));
+}
+
 void gnttab_unmap_status(void)
 {
 	return gnttab_interface._gnttab_unmap_status();
@@ -735,7 +907,7 @@ void gnttab_unmap_status(void)
 
 int gnttab_suspend(void)
 {
-	arch_gnttab_unmap_shared(shared.ring_addr, nr_grant_frames);
+	arch_gnttab_unmap(shared.ring_addr, nr_grant_frames);
 	gnttab_interface._gnttab_unmap_status();
 	return 0;
 }
diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index 405879d..4568498 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -147,8 +147,10 @@ gnttab_set_unmap_op(struct gnttab_unmap_grant_ref *unmap,
phys_addr_t addr,
 int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
 			   unsigned long max_nr_gframes,
 			   void **__shared);
-void arch_gnttab_unmap_shared(void *shared,
-			      unsigned long nr_gframes);
+int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
+			   unsigned long max_nr_gframes,
+			   grant_status_t **__shared);
+void arch_gnttab_unmap(void *shared, unsigned long nr_gframes);
 
 extern unsigned long xen_hvm_resume_frames;
 unsigned int gnttab_max_grant_frames(void);
-- 
1.7.6


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

annie.li@oracle.com

2011-Nov-09 08:16 UTC

head link

[Xen-devel] [PATCH 3/3] code clean up

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

Signed-off-by: Annie Li <annie.li@oracle.com>
---
 drivers/xen/grant-table.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index a3294a26..458c00d 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -708,7 +708,8 @@ static void gnttab_request_version(void)
 {
 	int rc;
 	struct gnttab_set_version gsv;
-	const char *str = "we need grant tables version 2, but only version 1 is
available\n";
+	const char *str = "we need grant tables version 2, "
+					"but only version 1 is available\n";
 
 	gsv.version = 2;
 	rc = HYPERVISOR_grant_table_op(GNTTABOP_set_version, &gsv, 1);
-- 
1.7.6


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

ANNIE LI

2011-Nov-09 09:05 UTC

head link

Re: [Xen-devel] patches of upstreaming grant table version 2

Diff and shortlog:

  arch/x86/xen/grant-table.c          |   39 ++++-
  drivers/xen/grant-table.c           |  354 
++++++++++++++++++++++++++++++++---
  include/xen/grant_table.h           |    8 +-
  include/xen/interface/grant_table.h |  167 ++++++++++++++++-
  include/xen/interface/xen.h         |    2 +
  5 files changed, 534 insertions(+), 36 deletions(-)

Annie Li (3):
       Introducing grant table V2 stucture
       Grant tables v2 implementation.
       code clean up

Thanks
Annie

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

Paul Durrant

2011-Nov-09 11:11 UTC

head link

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

Annie,

  Comments inline below...
> -----Original Message-----
[snip]> -static struct grant_entry *shared;
> +static union {
> +	struct grant_entry_v1 *v1;
> +	void *ring_addr;
> +} shared;
> +
''ring_addr'' seems like the wrong name here; how about
''raw''?
> +/*
> + * 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);
> +
I don''t really like the idea of having null operations. How about
abstracting at the level of gnttab_map/unmap so that you can include the status
mapping for v2 but just do the arch_gnttab_map_shared for v1?
> +/*This is a structure of function pointers for grant table v1*/
> 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);
> +	/*
> +	 * Release a list of frames which are mapped in
> _gnttab_map_status for
> +	 * grant entry status.
> +	 */
> +	void (*_gnttab_unmap_status)(void);
> +	/*
> +	 * Introducing a valid entry into the grant table, granting
> the frame
> +	 * of this grant entry to domain for accessing, or
> transfering, or
> +	 * transitively accessing. First input parameter is reference
> of this
> +	 * introduced grant entry, second one is domid of granted
> domain, third
> +	 * one is the frame to be granted, and the last one is status
> of the
> +	 * grant entry to be updated.
> +	 */
> +	void (*_update_grant_entry)(grant_ref_t, domid_t,
> +		unsigned long, unsigned);
> +	/*
> +	 * Stop granting a grant entry to domain for accessing. First
> input
> +	 * parameter is reference of a grant entry whose grant access
> will be
> +	 * stopped, second one is not in use now. If the grant entry
> is
> +	 * currently mapped for reading or writing, just return
> failure(==0)
> +	 * directly and don''t tear down the grant access. Otherwise,
> stop grant
> +	 * access for this entry and return success(==1).
> +	 */
> +	int (*_gnttab_end_foreign_access_ref)(grant_ref_t, int);
> +	/*
> +	 * Stop granting a grant entry to domain for transfer. If
> tranfer has
> +	 * not started, just reclaim the grant entry and return
> failure(==0).
> +	 * Otherwise, wait for the transfer to complete and then
> return the
> +	 * frame.
> +	 */
> +	unsigned long
> (*_gnttab_end_foreign_transfer_ref)(grant_ref_t);
> +	/*
> +	 * Query the status of a grant entry. Input parameter is
> reference of
> +	 * queried grant entry, return value is the status of queried
> entry.
> +	 * Detailed status(writing/reading) can be gotten from the
> return value
> +	 * by bit operations.
> +	 */
> +	int (*_gnttab_query_foreign_access)(grant_ref_t);
> +} gnttab_interface;
> +
Why the leading ''_'' in the names?
> +static int grant_table_version;
> 
>  static struct gnttab_free_callback *gnttab_free_callback_list;
> 
> @@ -142,6 +207,15 @@ static void put_free_entry(grant_ref_t ref)
>  	spin_unlock_irqrestore(&gnttab_list_lock, flags);  }
> 
> +static void update_grant_entry_v1(grant_ref_t ref, domid_t domid,
> +				  unsigned long frame, unsigned flags) {
> +	shared.v1[ref].frame = frame;
> +	shared.v1[ref].domid = domid;
> +	wmb();
> +	shared.v1[ref].flags = flags;
> +}
> +
>  static void update_grant_entry(grant_ref_t ref, domid_t domid,
>  			       unsigned long frame, unsigned flags)  {
> @@ -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.
>  	 */
The comment above probably should be moved into the v1 function itself since the
v2 code differs.
> -	shared[ref].frame = frame;
> -	shared[ref].domid = domid;
> -	wmb();
> -	shared[ref].flags = flags;
> +	gnttab_interface._update_grant_entry(ref, domid, frame,
> flags);
>  }[snip]

  Paul

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

Paul Durrant

2011-Nov-09 11:15 UTC

head link

[Xen-devel] RE: [PATCH 3/3] code clean up

Annie,

  Why was this split out as a separate patch? Can it not be folded into patch 2
since the line it re-formats was introduced there?

  Paul
> -----Original Message-----
> From: annie.li@oracle.com [mailto:annie.li@oracle.com]
> Sent: 09 November 2011 08:16
> To: xen-devel@lists.xensource.com; linux-kernel@vger.kernel.org;
> konrad.wilk@oracle.com; jeremy@goop.org
> Cc: kurt.hackel@oracle.com; Paul Durrant; annie.li@oracle.com
> Subject: [PATCH 3/3] code clean up
> 
> From: Annie Li <annie.li@oracle.com>
> 
> Signed-off-by: Annie Li <annie.li@oracle.com>
> ---
>  drivers/xen/grant-table.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index a3294a26..458c00d 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -708,7 +708,8 @@ static void gnttab_request_version(void)  {
>  	int rc;
>  	struct gnttab_set_version gsv;
> -	const char *str = "we need grant tables version 2, but only
> version 1 is available\n";
> +	const char *str = "we need grant tables version 2, "
> +					"but only version 1 is
> available\n";
> 
>  	gsv.version = 2;
>  	rc = HYPERVISOR_grant_table_op(GNTTABOP_set_version, &gsv,
> 1);
> --
> 1.7.6

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

Ian Campbell

2011-Nov-09 12:18 UTC

head link

Re: [Xen-devel] RE: [PATCH 3/3] code clean up

On Wed, 2011-11-09 at 11:15 +0000, Paul Durrant wrote:> Annie,
> 
>   Why was this split out as a separate patch? Can it not be folded into
patch 2 since the line it re-formats was introduced there?
It is not generally necessary to split string constants in order to meet
the 80 column (soft) limit in Linux code. Some people frown on it
because it breaks grep''ability.

Ian.
> 
>   Paul
> 
> > -----Original Message-----
> > From: annie.li@oracle.com [mailto:annie.li@oracle.com]
> > Sent: 09 November 2011 08:16
> > To: xen-devel@lists.xensource.com; linux-kernel@vger.kernel.org;
> > konrad.wilk@oracle.com; jeremy@goop.org
> > Cc: kurt.hackel@oracle.com; Paul Durrant; annie.li@oracle.com
> > Subject: [PATCH 3/3] code clean up
> > 
> > From: Annie Li <annie.li@oracle.com>
> > 
> > Signed-off-by: Annie Li <annie.li@oracle.com>
> > ---
> >  drivers/xen/grant-table.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> > index a3294a26..458c00d 100644
> > --- a/drivers/xen/grant-table.c
> > +++ b/drivers/xen/grant-table.c
> > @@ -708,7 +708,8 @@ static void gnttab_request_version(void)  {
> >  	int rc;
> >  	struct gnttab_set_version gsv;
> > -	const char *str = "we need grant tables version 2, but only
> > version 1 is available\n";
> > +	const char *str = "we need grant tables version 2, "
> > +					"but only version 1 is
> > available\n";
> > 
> >  	gsv.version = 2;
> >  	rc = HYPERVISOR_grant_table_op(GNTTABOP_set_version, &gsv,
> > 1);
> > --
> > 1.7.6
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


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

Ian Campbell

2011-Nov-09 13:28 UTC

head link

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

On Wed, 2011-11-09 at 08:14 +0000, annie.li@oracle.com
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?
> 
>  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.

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.
> +
> +/*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*/
> +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.

[...]> +} 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.
>  /*
>   * 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.

Ian.


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

Ian Campbell

2011-Nov-09 13:32 UTC

head link

Re: [Xen-devel] [PATCH 2/3] Grant tables v2 implementation.

On Wed, 2011-11-09 at 08:15 +0000, annie.li@oracle.com
wrote:> From: Annie Li <annie.li@oracle.com>
> 
> Receiver-side copying of packets is based on this implementation, it gives
> better performance and better CPU accounting. It totally supports three
types:
> full-page, sub-page and transitive grants.
> 
> However this patch does not cover sub-page and transitive grants, it mainly
> focus on Full-page part and implements grant table V2 interfaces
corresponding
> to what already exists in grant table V1, such as: grant table V2
> initialization, mapping, releasing and exported interfaces.
> 
> Each guest can only supports one type of grant table type, every entry in
grant
> table should be the same version. It is necessary to set V1 or V2 version
before
> initializing the grant table.
> 
> Grant table exported interfaces of V2 are same with those of V1, Xen is
> responsible to judge what grant table version guests are using in every
grant
> operation.
> 
> V2 fulfills the same role of V1, and it is totally backwards compitable
with V1.
> If dom0 support grant table V2, the guests runing on it can run with either
V1
> or V2.
> 
> Signed-off-by: Annie Li <annie.li@oracle.com>
> ---
>  arch/x86/xen/grant-table.c |   34 ++++++++-
>  drivers/xen/grant-table.c  |  194
+++++++++++++++++++++++++++++++++++++++++---
>  include/xen/grant_table.h  |    6 +-
>  3 files changed, 220 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c
> index 65ce508..3e9f936 100644
> --- a/arch/x86/xen/grant-table.c
> +++ b/arch/x86/xen/grant-table.c
> @@ -54,6 +54,17 @@ static int map_pte_fn(pte_t *pte, struct page *pmd_page,
>         return 0;
>  }
> 
> +/*32bits is not enough since Xen supports sparse physical memory*/
What does this mean?
> +static int map_pte_fn_status(pte_t *pte, struct page *pmd_page,
> +                            unsigned long addr, void *data)
> +{
> +       uint64_t **frames = (uint64_t **)data;
> +
> +       set_pte_at(&init_mm, addr, pte, mfn_pte((*frames)[0],
PAGE_KERNEL));
> +       (*frames)++;
> +       return 0;
> +}
Isn''t this function identical to map_pte_fn?
> +
>  static int unmap_pte_fn(pte_t *pte, struct page *pmd_page,
>                         unsigned long addr, void *data)
>  {
> @@ -630,6 +756,44 @@ static int gnttab_map_status_v1(unsigned int
nr_gframes)
>         return 0;
>  }
> 
> +static int gnttab_map_status_v2(unsigned int nr_gframes)
> +{
> +       uint64_t *sframes;
> +       unsigned int nr_sframes;
> +       struct gnttab_get_status_frames getframes;
> +       int rc;
> +
> +       nr_sframes = nr_status_frames(nr_gframes);
> +
> +       /* No need for kzalloc as it is initialized in following hyercall
                                                                   hypercall
> +        * GNTTABOP_get_status_frames.
> +        */
> +       sframes = kmalloc(nr_sframes  * sizeof(uint64_t), GFP_ATOMIC);
> +       if (!sframes)
> +               return -ENOMEM;
> +
> +       getframes.dom        = DOMID_SELF;
> +       getframes.nr_frames  = nr_sframes;
> +       set_xen_guest_handle(getframes.frame_list, sframes);
> +
> +       rc = HYPERVISOR_grant_table_op(GNTTABOP_get_status_frames,
> +                                      &getframes, 1);
> +       if (rc == -ENOSYS) {
> +               kfree(sframes);
> +               return -ENOSYS;
> +       }
> +
> +       BUG_ON(rc || getframes.status);
> +
> +       rc = arch_gnttab_map_status(sframes, nr_sframes,
> +                                  
nr_status_frames(gnttab_max_grant_frames()),
> +                                   &grstatus);
> +       BUG_ON(rc);
> +       kfree(sframes);
> +
> +       return 0;
> +}
> +
>  static int gnttab_map_status(unsigned int nr_gframes)
>  {
>         return gnttab_interface._gnttab_map_status(nr_gframes);
> @@ -666,6 +830,9 @@ static int gnttab_map(unsigned int start_idx, unsigned
int end_idx)
>                 return rc;
>         }
> 
> +       /* No need for kzalloc as it is initialized in following hyercall
"hypercall" again

Ian.



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

Konrad Rzeszutek Wilk

2011-Nov-09 14:22 UTC

head link

[Xen-devel] Re: patches of upstreaming grant table version 2

On Wed, Nov 09, 2011 at 03:58:47PM +0800, ANNIE LI
wrote:> Hi
> 
> The following patches introduce and implement grant table version 2,
> and they are based on v3.1.0-rc9+.
Yeey! Thanks for posting them.> 
> Descriptions for those patches:
> 
> 1. In those patches, the grant table code supports both grant table
> v1 and v2 version, v2 is an extension from v1. Grant table of guest
> domain can be either v1 or v2 version, and every grant table entry
> on one guest should be the same version.
> 
> 2. Full page structure of grant table v2 play the same role as grant
> table v1. Although full page structure is different from v1, grant
> table 2 is totally backwards compatible with v1. Grant table is
> shared between guest and Xen, domu and dom0 all have their own grant
> table shared with Xen, and their grant table version should be set
> before any grants are activated. When domu grants an entry to dom0
> to map a frame, following are steps:
> * domu introduces a grant entry by reference
> * domu informs dom0 the gref
> * dom0 sends hypercall to map frame through this reference, Xen copy
> shared entry to active entry and update frame
> * dom0 does its work and release the frame, Xen releases the entry.
> * domu redo those steps for a new cycle.
> 
> Xen mapping process can be found in function __gnttab_map_grant_ref
> in link:
http://xenbits.xen.org/hg/xen-4.1-testing.hg/file/81e39a4978ea/xen/common/grant_table.c#l2172
> 
> 
> 3. If dom0 supports grant table v2, guests run on it can either
> supports v1 or v2. Xen is responsible to judge what version the
> guests are using. This is implemented in link:
http://xenbits.xen.org/hg/xen-4.1-testing.hg/file/81e39a4978ea/xen/common/grant_table.c#l2172.
> Key word is:  rd->grant_table->gt_version.
> 
> 4. Grant table2 is a precondition to netchannel2 mechanism.
Well, .. parts of netchannel2 that are going to be put in the netback
and netfront as ways of expanding the driver (add new features).

netchannel2 by itself is dead, and we can pick some of its grand ideas
from it and put them in netback/netfront.
> Netchannel2 imports a new device type: vif2. To support this type,
> we need new netback2, netfront2 driver and modification in Xen. If
> all are ready, two types of vif can be supported: vif and vif2. vif
> is for original backend and frontend, vif2 is for netback2 and
> netfront2.
> 
> Thanks
> Annie
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Konrad Rzeszutek Wilk

2011-Nov-09 14:49 UTC

head link

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

On Wed, Nov 09, 2011 at 11:11:22AM +0000, Paul Durrant
wrote:> Annie,
> 
>   Comments inline below...
> 
> > -----Original Message-----
> [snip]
> > -static struct grant_entry *shared;
> > +static union {
> > +	struct grant_entry_v1 *v1;
> > +	void *ring_addr;
> > +} shared;
> > +
> 
> ''ring_addr'' seems like the wrong name here; how about
''raw''?
Or ''ring''. I asked Annie to change it from
''raw'' to something else and the first
thing that came in my mind was ''ring_addr''. But this does not
point to a ring, so
the ''ring'' part is wrong.

Point here is to make it descriptive. ''raw'' does not carry
meaning of _what_
it is suppose to do. 

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

Paul Durrant

2011-Nov-09 14:56 UTC

head link

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

I''d be happy with ''addr'' or
''opaque'' or somesuch if ''raw'' is
distasteful.

  Paul
> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: 09 November 2011 14:49
> To: Paul Durrant
> Cc: annie.li@oracle.com; xen-devel@lists.xensource.com; linux-
> kernel@vger.kernel.org; jeremy@goop.org; kurt.hackel@oracle.com
> Subject: Re: [PATCH 1/3] Introducing grant table V2 stucture
> 
> On Wed, Nov 09, 2011 at 11:11:22AM +0000, Paul Durrant wrote:
> > Annie,
> >
> >   Comments inline below...
> >
> > > -----Original Message-----
> > [snip]
> > > -static struct grant_entry *shared;
> > > +static union {
> > > +	struct grant_entry_v1 *v1;
> > > +	void *ring_addr;
> > > +} shared;
> > > +
> >
> > ''ring_addr'' seems like the wrong name here; how
about ''raw''?
> 
> Or ''ring''. I asked Annie to change it from
''raw'' to something else
> and the first thing that came in my mind was ''ring_addr''.
But this
> does not point to a ring, so the ''ring'' part is wrong.
> 
> Point here is to make it descriptive. ''raw'' does not
carry meaning
> of _what_ it is suppose to do.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

annie li

2011-Nov-09 16:00 UTC

head link

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

Maybe addr or gnttab_addr?

Thanks
Annie

On 2011-11-9 22:56, Paul Durrant wrote:> I''d be happy with ''addr'' or
''opaque'' or somesuch if ''raw'' is
distasteful.
>
>   Paul
>
>   
>> -----Original Message-----
>> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
>> Sent: 09 November 2011 14:49
>> To: Paul Durrant
>> Cc: annie.li@oracle.com; xen-devel@lists.xensource.com; linux-
>> kernel@vger.kernel.org; jeremy@goop.org; kurt.hackel@oracle.com
>> Subject: Re: [PATCH 1/3] Introducing grant table V2 stucture
>>
>> On Wed, Nov 09, 2011 at 11:11:22AM +0000, Paul Durrant wrote:
>>     
>>> Annie,
>>>
>>>   Comments inline below...
>>>
>>>       
>>>> -----Original Message-----
>>>>         
>>> [snip]
>>>       
>>>> -static struct grant_entry *shared;
>>>> +static union {
>>>> +	struct grant_entry_v1 *v1;
>>>> +	void *ring_addr;
>>>> +} shared;
>>>> +
>>>>         
>>> ''ring_addr'' seems like the wrong name here; how
about ''raw''?
>>>       
>> Or ''ring''. I asked Annie to change it from
''raw'' to something else
>> and the first thing that came in my mind was
''ring_addr''. But this
>> does not point to a ring, so the ''ring'' part is
wrong.
>>
>> Point here is to make it descriptive. ''raw'' does not
carry meaning
>> of _what_ it is suppose to do.
>>     
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>   

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

annie li

2011-Nov-09 16:08 UTC

head link

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

Thanks, Paul. See following,

On 2011-11-9 19:11, Paul Durrant wrote:>  
>   
>> +/*
>> + * 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);
>> +
>>     
>
> I don''t really like the idea of having null operations. How about
abstracting at the level of gnttab_map/unmap so that you can include the status
mapping for v2 but just do the arch_gnttab_map_shared for v1?
>   I see. v2 function includes mapping and arch_gnttab_map_shared, v1 
function only include arch_gnttab_map_sh, right? This will lead to some 
code duplicated in two functions. >   
>> +/*This is a structure of function pointers for grant table v1*/
>> 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);
>> +	/*
>> +	 * Release a list of frames which are mapped in
>> _gnttab_map_status for
>> +	 * grant entry status.
>> +	 */
>> +	void (*_gnttab_unmap_status)(void);
>> +	/*
>> +	 * Introducing a valid entry into the grant table, granting
>> the frame
>> +	 * of this grant entry to domain for accessing, or
>> transfering, or
>> +	 * transitively accessing. First input parameter is reference
>> of this
>> +	 * introduced grant entry, second one is domid of granted
>> domain, third
>> +	 * one is the frame to be granted, and the last one is status
>> of the
>> +	 * grant entry to be updated.
>> +	 */
>> +	void (*_update_grant_entry)(grant_ref_t, domid_t,
>> +		unsigned long, unsigned);
>> +	/*
>> +	 * Stop granting a grant entry to domain for accessing. First
>> input
>> +	 * parameter is reference of a grant entry whose grant access
>> will be
>> +	 * stopped, second one is not in use now. If the grant entry
>> is
>> +	 * currently mapped for reading or writing, just return
>> failure(==0)
>> +	 * directly and don''t tear down the grant access. Otherwise,
>> stop grant
>> +	 * access for this entry and return success(==1).
>> +	 */
>> +	int (*_gnttab_end_foreign_access_ref)(grant_ref_t, int);
>> +	/*
>> +	 * Stop granting a grant entry to domain for transfer. If
>> tranfer has
>> +	 * not started, just reclaim the grant entry and return
>> failure(==0).
>> +	 * Otherwise, wait for the transfer to complete and then
>> return the
>> +	 * frame.
>> +	 */
>> +	unsigned long
>> (*_gnttab_end_foreign_transfer_ref)(grant_ref_t);
>> +	/*
>> +	 * Query the status of a grant entry. Input parameter is
>> reference of
>> +	 * queried grant entry, return value is the status of queried
>> entry.
>> +	 * Detailed status(writing/reading) can be gotten from the
>> return value
>> +	 * by bit operations.
>> +	 */
>> +	int (*_gnttab_query_foreign_access)(grant_ref_t);
>> +} gnttab_interface;
>> +
>>     
>
> Why the leading ''_'' in the names?
>   Just in order to differ those function pointers from the original
functions.>   
>> +static int grant_table_version;
>>
>>  static struct gnttab_free_callback *gnttab_free_callback_list;
>>
>> @@ -142,6 +207,15 @@ static void put_free_entry(grant_ref_t ref)
>>  	spin_unlock_irqrestore(&gnttab_list_lock, flags);  }
>>
>> +static void update_grant_entry_v1(grant_ref_t ref, domid_t domid,
>> +				  unsigned long frame, unsigned flags) {
>> +	shared.v1[ref].frame = frame;
>> +	shared.v1[ref].domid = domid;
>> +	wmb();
>> +	shared.v1[ref].flags = flags;
>> +}
>> +
>>  static void update_grant_entry(grant_ref_t ref, domid_t domid,
>>  			       unsigned long frame, unsigned flags)  {
>> @@ -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.
>>  	 */
>>     
>
> The comment above probably should be moved into the v1 function itself
since the v2 code differs.
>   Yes, you are right. Comments for v2 should be added too.

Thanks
Annie>   
>> -	shared[ref].frame = frame;
>> -	shared[ref].domid = domid;
>> -	wmb();
>> -	shared[ref].flags = flags;
>> +	gnttab_interface._update_grant_entry(ref, domid, frame,
>> flags);
>>  }
>>     
> [snip]
>
>   Paul
>   

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

annie li

2011-Nov-09 16:10 UTC

head link

Re: [Xen-devel] RE: [PATCH 3/3] code clean up

On 2011-11-9 20:18, Ian Campbell wrote:> On Wed, 2011-11-09 at 11:15 +0000, Paul Durrant wrote:
>   
>> Annie,
>>
>>   Why was this split out as a separate patch? Can it not be folded into
patch 2 since the line it re-formats was introduced there?
>>     
>
> It is not generally necessary to split string constants in order to meet
> the 80 column (soft) limit in Linux code. Some people frown on it
> because it breaks grep''ability.
>   If so, I''d like to revert the change.

Thanks
Annie> Ian.
>
>   
>>   Paul
>>
>>     
>>> -----Original Message-----
>>> From: annie.li@oracle.com [mailto:annie.li@oracle.com]
>>> Sent: 09 November 2011 08:16
>>> To: xen-devel@lists.xensource.com; linux-kernel@vger.kernel.org;
>>> konrad.wilk@oracle.com; jeremy@goop.org
>>> Cc: kurt.hackel@oracle.com; Paul Durrant; annie.li@oracle.com
>>> Subject: [PATCH 3/3] code clean up
>>>
>>> From: Annie Li <annie.li@oracle.com>
>>>
>>> Signed-off-by: Annie Li <annie.li@oracle.com>
>>> ---
>>>  drivers/xen/grant-table.c |    3 ++-
>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
>>> index a3294a26..458c00d 100644
>>> --- a/drivers/xen/grant-table.c
>>> +++ b/drivers/xen/grant-table.c
>>> @@ -708,7 +708,8 @@ static void gnttab_request_version(void)  {
>>>  	int rc;
>>>  	struct gnttab_set_version gsv;
>>> -	const char *str = "we need grant tables version 2, but only
>>> version 1 is available\n";
>>> +	const char *str = "we need grant tables version 2, "
>>> +					"but only version 1 is
>>> available\n";
>>>
>>>  	gsv.version = 2;
>>>  	rc = HYPERVISOR_grant_table_op(GNTTABOP_set_version, &gsv,
>>> 1);
>>> --
>>> 1.7.6
>>>       
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>>     
>
>
>   

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

Paul Durrant

2011-Nov-09 16:14 UTC

head link

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

Annie,

On 2011-11-9 19:11, Paul Durrant wrote: > I see. v2 function includes mapping and arch_gnttab_map_shared, v1 function
only include arch_gnttab_map_sh, right?
> This will lead to some code duplicated in two functions. 
My preference would be to have duplicated calls to arch_gnttab_map_shared(). I
think it''s more illustrative of the difference between v1 and v2 having
separate status pages.

  Paul

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

annie li

2011-Nov-09 16:19 UTC

head link

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@oracle.com 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@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2011-Nov-09 16:22 UTC

head link

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

On Wed, 2011-11-09 at 16:00 +0000, annie li wrote:> Maybe addr or gnttab_addr?
IMHO the gnttab_ bit belongs in either (or both) the struct name or the
variable names referencing the struct, rather than the fields
themselves. It should be obvious from the context that this member is
something to do with gnttab. e.g. I think gnttab_shared->addr would be
fine, as would shared->addr in the context of gnttab.c etc.

Ian.



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

annie li

2011-Nov-09 16:30 UTC

head link

Re: [Xen-devel] [PATCH 2/3] Grant tables v2 implementation.

ANNIE LI

2011-Nov-10 01:37 UTC

head link

[Xen-devel] Re: patches of upstreaming grant table version 2

> Well, .. parts of netchannel2 that are going to be put in the netback
> and netfront as ways of expanding the driver (add new features).
>
> netchannel2 by itself is dead, and we can pick some of its grand ideas
> from it and put them in netback/netfront.Yes, I should not mention netchannel2 here, will update the comments.

Thanks
Annie

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

ANNIE LI

2011-Nov-10 01:50 UTC

head link

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

On 2011-11-10 0:14, Paul Durrant wrote:> Annie,
>
> On 2011-11-9 19:11, Paul Durrant wrote:
>> I see. v2 function includes mapping and arch_gnttab_map_shared, v1
function only include arch_gnttab_map_sh, right?
>> This will lead to some code duplicated in two functions.
> My preference would be to have duplicated calls to
arch_gnttab_map_shared(). I think it''s more illustrative of the
difference between v1 and v2 having separate status pages.Ok, will do as you suggested, thanks.

Thanks
Annie

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

ANNIE LI

2011-Nov-10 01:56 UTC

head link

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

On 2011-11-10 0:22, Ian Campbell wrote:> IMHO the gnttab_ bit belongs in either (or both) the struct name or the
> variable names referencing the struct, rather than the fields
> themselves. It should be obvious from the context that this member is
> something to do with gnttab. e.g. I think gnttab_shared->addr would be
> fine, as would shared->addr in the context of gnttab.c etc.Right.
Thanks you all for the review, I will improve patches based on those 
suggestions and resend them.

Thanks
Annie

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

Xen devel - Nov 2011 - patches of upstreaming grant table version 2

[Xen-devel] patches of upstreaming grant table version 2

[Xen-devel] patches of upstreaming grant table version 2

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

[Xen-devel] [PATCH 2/3] Grant tables v2 implementation.

[Xen-devel] [PATCH 3/3] code clean up

Re: [Xen-devel] patches of upstreaming grant table version 2

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

[Xen-devel] RE: [PATCH 3/3] code clean up

Re: [Xen-devel] RE: [PATCH 3/3] code clean up

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

Re: [Xen-devel] [PATCH 2/3] Grant tables v2 implementation.

[Xen-devel] Re: patches of upstreaming grant table version 2

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

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

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

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

Re: [Xen-devel] RE: [PATCH 3/3] code clean up

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

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

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

Re: [Xen-devel] [PATCH 2/3] Grant tables v2 implementation.

[Xen-devel] Re: patches of upstreaming grant table version 2

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

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