Re: [PATCH v4 3/4] xen: implement guest_physmap_(un)pin_range

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

On Thu, 2013-08-15 at 12:18 +0100, Stefano Stabellini wrote:
> guest_physmap_pin_range pins a range of guest pages so that their p2m
> mappings won't be changed.
> guest_physmap_unpin_range unpins the previously pinned pages.
> The pinning is done using one of the spare bits in the p2m ptes.
> 
> Use the newly introduce p2m_walker to implement the two functions on
> ARM.
> Provide empty stubs for x86.
> 
> 
> Changes in v4:
> - use p2m_walker to implement guest_physmap_pin_range and
> guest_physmap_unpin_range;
> - return -EINVAL when the P2M_DMA_PIN check fails;
> - change the printk into a gdprintk;
> - add a comment on what type of page can be pinned.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/arm/p2m.c         |   45 ++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/mm.h   |    4 +++
>  xen/include/asm-arm/page.h |    7 +++++-
>  xen/include/asm-x86/p2m.h  |   12 +++++++++++
>  4 files changed, 67 insertions(+), 1 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index a77df9a..c185328 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -127,6 +127,43 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr)
>      return p2m.maddr;
>  }
>  
> +
> +static int _guest_physmap_pin_range(lpae_t *ptep, void *arg)

Single _ namespace is for the compiler. __ is, I think, legal in
"kernel" space but actually this should just be called pin_one_pte or
something like that.

> +{
> +    lpae_t pte = *ptep;
> +    if ( pte.p2m.avail & P2M_DMA_PIN )
> +        return -EINVAL;
> +    pte.p2m.avail |= P2M_DMA_PIN;

I think test_and_set_bit would be better here and you should operate on
ptep directly IMHO rather than changing a copy and writing that. Might
require a pte_modified macro with the flushing stuff from write_pte.

> +    write_pte(ptep, pte);
> +    return 0;
> +}
> +
> +int guest_physmap_pin_range(struct domain *d,
> +                            xen_pfn_t gpfn,
> +                            unsigned int order)
> +{
> +    return p2m_walker(d, gpfn << PAGE_SHIFT, order,
> +                      _guest_physmap_pin_range, NULL);
> +}
> +
> +static int _guest_physmap_unpin_range(lpae_t *ptep, void *arg)

unpin_on_pte or something.

> +{
> +    lpae_t pte = *ptep;
> +    if ( !pte.p2m.avail & P2M_DMA_PIN )
> +        return -EINVAL;
> +    pte.p2m.avail &= ~P2M_DMA_PIN;
> +    write_pte(ptep, pte);
> +    return 0;
> +}
> +
> +int guest_physmap_unpin_range(struct domain *d,
> +                              xen_pfn_t gpfn,
> +                              unsigned int order)
> +{
> +    return p2m_walker(d, gpfn << PAGE_SHIFT, order,
> +                      _guest_physmap_unpin_range, NULL);
> +}
> +
>  int guest_physmap_mark_populate_on_demand(struct domain *d,
>                                            unsigned long gfn,
>                                            unsigned int order)
> @@ -236,6 +273,14 @@ static int create_p2m_entries(struct domain *d,
>              cur_second_offset = second_table_offset(addr);
>          }
>  
> +        if ( third[third_table_offset(addr)].p2m.avail & P2M_DMA_PIN )

test_bit.

> +        {
> +            rc = -EINVAL;
> +            gdprintk(XENLOG_WARNING, "cannot change p2m mapping for paddr=%"PRIpaddr
> +                    " domid=%d, the page is pinned\n", addr, d->domain_id);
> +            goto out;
> +        }
> +
>          flush = third[third_table_offset(addr)].p2m.valid;
>  
>          /* Allocate a new RAM page and attach */
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index df6fea9..34167e6 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -317,6 +317,10 @@ void free_init_memory(void);
>  
>  int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn,
>                                            unsigned int order);
> +int guest_physmap_pin_range(struct domain *d, xen_pfn_t gpfn,
> +                            unsigned int order);
> +int guest_physmap_unpin_range(struct domain *d, xen_pfn_t gpfn,
> +                              unsigned int order);
>  
>  extern void put_page_type(struct page_info *page);
>  static inline void put_page_and_type(struct page_info *page)
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 41e9eff..c08cfca 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -153,11 +153,16 @@ typedef struct {
>      unsigned long hint:1;       /* In a block of 16 contiguous entries */
>      unsigned long sbz2:1;
>      unsigned long xn:1;         /* eXecute-Never */
> -    unsigned long avail:4;      /* Ignored by hardware */
> +    unsigned long avail:4;      /* Ignored by hardware, see below */
>  
>      unsigned long sbz1:5;
>  } __attribute__((__packed__)) lpae_p2m_t;
>  
> +/* Xen "avail" bits allocation in third level entries */
> +#define P2M_DMA_PIN     (1<<0)  /* The page has been "pinned": the hypervisor
> +                                   promises not to change the p2m mapping.
> +                                   Only normal r/w guest RAM can be pinned. */
> +
>  /*
>   * Walk is the common bits of p2m and pt entries which are needed to
>   * simply walk the table (e.g. for debug).
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 43583b2..b08a722 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -492,6 +492,18 @@ void guest_physmap_remove_page(struct domain *d,
>  /* Set a p2m range as populate-on-demand */
>  int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn,
>                                            unsigned int order);
> +static inline int guest_physmap_pin_range(struct domain *d,
> +                                          xen_pfn_t gpfn,
> +                                          unsigned int order)
> +{
> +    return -ENOSYS;
> +}
> +static inline int guest_physmap_unpin_range(struct domain *d,
> +                              xen_pfn_t gpfn,
> +                              unsigned int order)
> +{
> +    return -ENOSYS;
> +}
>  
>  /* Change types across all p2m entries in a domain */
>  void p2m_change_entry_type_global(struct domain *d,