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

[annie li <annie.li@oracle.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4079 bytes --]



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.
>
>   

[-- Attachment #1.2: Type: text/html, Size: 5735 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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