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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 9 Nov 2011 13:28:03 +0000
From:	Ian Campbell <Ian.Campbell@...rix.com>
To:	"annie.li@...cle.com" <annie.li@...cle.com>
CC:	"xen-devel@...ts.xensource.com" <xen-devel@...ts.xensource.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"konrad.wilk@...cle.com" <konrad.wilk@...cle.com>,
	"jeremy@...p.org" <jeremy@...p.org>,
	"kurt.hackel@...cle.com" <kurt.hackel@...cle.com>,
	Paul Durrant <Paul.Durrant@...rix.com>
Subject: Re: [Xen-devel] [PATCH 1/3] Introducing grant table V2 stucture

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/