From mboxrd@z Thu Jan 1 00:00:00 1970 From: annie li Subject: Re: [PATCH 1/3] Introducing grant table V2 stucture Date: Thu, 10 Nov 2011 00:08:06 +0800 Message-ID: <4EBAA566.80909@oracle.com> References: <4EBA35D3.3020506@oracle.com> <1320826490-29362-1-git-send-email-annie.li@oracle.com> <291EDFCB1E9E224A99088639C4762022B4543AB1BD@LONPMAILBOX01.citrite.net> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1887570781==" Return-path: In-Reply-To: <291EDFCB1E9E224A99088639C4762022B4543AB1BD@LONPMAILBOX01.citrite.net> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Paul Durrant Cc: "kurt.hackel@oracle.com" , "jeremy@goop.org" , "xen-devel@lists.xensource.com" , "linux-kernel@vger.kernel.org" , "konrad.wilk@oracle.com" List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --===============1887570781== Content-Type: multipart/alternative; boundary="------------010104080206070704040901" This is a multi-part message in MIME format. --------------010104080206070704040901 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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 > --------------010104080206070704040901 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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
--------------010104080206070704040901-- --===============1887570781== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --===============1887570781==--