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
>