linux-kernel - RE: [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 11:11:22 +0000
From:	Paul Durrant <Paul.Durrant@...rix.com>
To:	"annie.li@...cle.com" <annie.li@...cle.com>,
	"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>
CC:	"kurt.hackel@...cle.com" <kurt.hackel@...cle.com>
Subject: RE: [PATCH 1/3] Introducing grant table V2 stucture

Annie,

  Comments inline below...

> -----Original Message-----
[snip]
> -static struct grant_entry *shared;
> +static union {
> +	struct grant_entry_v1 *v1;
> +	void *ring_addr;
> +} shared;
> +

'ring_addr' seems like the wrong name here; how about 'raw'?

> +/*
> + * 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?

> +/*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?

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

> -	shared[ref].frame = frame;
> -	shared[ref].domid = domid;
> -	wmb();
> -	shared[ref].flags = flags;
> +	gnttab_interface._update_grant_entry(ref, domid, frame,
> flags);
>  }
[snip]

  Paul
--
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/