On Wed, 2011-11-09 at 08:14 +0000, annie.li@xxxxxxxxxx wrote:
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index 4f44b34..0d481a9 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -53,7 +53,7 @@
> /* External tools reserve first few grant table entries. */
> #define NR_RESERVED_ENTRIES 8
> #define GNTTAB_LIST_END 0xffffffff
> -#define GREFS_PER_GRANT_FRAME (PAGE_SIZE / sizeof(struct grant_entry))
> +#define GREFS_PER_GRANT_FRAME (PAGE_SIZE / sizeof(struct grant_entry_v1))
Does this need to become GREFS_V1_PER... or something?
>
> 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home