Re: [PATCH 06/12] xen: mark grant mapped pages as foreign

[Ian Campbell <Ian.Campbell@citrix.com>]

On Tue, 2015-01-06 at 18:57 +0000, David Vrabel wrote:
> From: Jenny Herbert <jennifer.herbert@citrix.com>
> 
> Use the "foreign" page flag to mark pages that have a grant map.  Use
> page->private to store information of the grant (the granting domain
> and the grant reference).
> 
> Signed-off-by: Jenny Herbert <jenny.herbert@citrix.com>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  arch/x86/xen/p2m.c        |   50 ++++++++++++++++++++++++++++++++++++++-------
>  include/xen/grant_table.h |   13 ++++++++++++
>  2 files changed, 56 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> index 0d70814..22624a3 100644
> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -648,6 +648,43 @@ bool set_phys_to_machine(unsigned long pfn, unsigned long mfn)
>  	return true;
>  }
>  
> +static int
> +init_page_grant_ref(struct page *p, domid_t domid, grant_ref_t grantref)

I'd be inclined to add "map" to the names somewhere, otherwise people
might thing they need to call this when allocating a grant in the f.e.
or other things.

> +{
> +#ifdef CONFIG_X86_64

Rather than suggesting to add CONFIG_ARM_64 here I'll suggest
BITS_PER_LONG >= 64.

> +	uint64_t gref;
> +	uint64_t* gref_p = &gref;
> +#else
> +	uint64_t* gref_p = kmalloc(sizeof(uint64_t), GFP_KERNEL);

Might this allocation be happening during e.g. swapping? I suppose it is
backend only, and swapping to a loopback vbd would be pretty mad. If you
can figure a reasonable use case for that you might want some extra GFP
flags?

Might this be hot enough to warrant using a specific kmem_cache?

> +	if (!gref)
> +		return -ENOMEM;
> +	uint64_t* gref = gref_p;
> +#endif
> +
> +	*gref_p = ((uint64_t) grantref << 32) | domid;
> +	p->private = gref;

There is a set_page_private() macro, which doesn't seem to do much but I
suppose you should use it (and page_private() for accessing, if you
don't already).

> @@ -182,4 +183,16 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
>  void gnttab_batch_map(struct gnttab_map_grant_ref *batch, unsigned count);
>  void gnttab_batch_copy(struct gnttab_copy *batch, unsigned count);
>  
> +static inline void
> +get_page_grant_ref(struct page *p, domid_t* domid, grant_ref_t* grantref)
> +{

BUG_ON(!PageBlah(p))?

> +#ifdef CONFIG_X86_64
> +	uint64_t gref = p->private;
> +#else
> +	uint64_t gref = *p->private;
> +#endif
> +	*domid = gref & 0xffff;
> +	*grantref = gref >> 32;
> +}
> +
>  #endif /* __ASM_GNTTAB_H__ */