WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] RE: [PATCH 1/3] Introducing grant table V2 stucture

To: "annie.li@xxxxxxxxxx" <annie.li@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, "linux-kernel@xxxxxxxxxxxxxxx" <linux-kernel@xxxxxxxxxxxxxxx>, "konrad.wilk@xxxxxxxxxx" <konrad.wilk@xxxxxxxxxx>, "jeremy@xxxxxxxx" <jeremy@xxxxxxxx>
Subject: [Xen-devel] RE: [PATCH 1/3] Introducing grant table V2 stucture
From: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
Date: Wed, 9 Nov 2011 11:11:22 +0000
Accept-language: en-US
Acceptlanguage: en-US
Cc: "kurt.hackel@xxxxxxxxxx" <kurt.hackel@xxxxxxxxxx>
Delivery-date: Wed, 09 Nov 2011 03:11:52 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1320826490-29362-1-git-send-email-annie.li@xxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <4EBA35D3.3020506@xxxxxxxxxx> <1320826490-29362-1-git-send-email-annie.li@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
Thread-index: Acyet8ZYqfvoa7PvSFarsEFl6ZbbZQAFzQUg
Thread-topic: [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

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