Re: [Xen-devel] [PATCH v2] xen/arm: p2m: Correctly flush TLB in create_p2m_entries

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-01-09 at 16:34 +0000, Julien Grall wrote:
> Except grant-table (I can't find {get,put}_page for grant-table code???),

I think they are in __gnttab_map_grant_ref, within __get_paged_frame or
through page_get_owner_and_reference.

and on unmap it is in__gnttab_unmap_common_complete.

It's a bit of a complex maze though so I'm not entirely sure, perhaps
Tim, Keir or Jan can confirm that a grant mapping always takes a
reference on the mapped page (it seems like PV x86 ought to be relying
on this for safety anyhow).

I think the flush in alloc_heap_pages would also serve as a backstop,
wouldn't it?

> all the callers are protected by a get_page before removing the page. So if 
> the
> another VCPU is trying to access to this page before the flush, it will just
> read/write the wrong page.
> 
> The downside of this patch is Xen flushes less TLBs. Instead of flushing all 
> TLBs
> on the current PCPU, Xen flushes TLBs for a specific VMID on every CPUs. This
> should be safe because create_p2m_entries only deal with a specific domain.
> 
> I don't think I forget case in this function. Let me know if it's the case.
> ---
>  xen/arch/arm/p2m.c |   24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 11f4714..ad6f76e 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -238,7 +238,7 @@ static int create_p2m_entries(struct domain *d,
>                       int mattr,
>                       p2m_type_t t)
>  {
> -    int rc, flush;
> +    int rc;
>      struct p2m_domain *p2m = &d->arch.p2m;
>      lpae_t *first = NULL, *second = NULL, *third = NULL;
>      paddr_t addr;
> @@ -246,10 +246,14 @@ static int create_p2m_entries(struct domain *d,
>                    cur_first_offset = ~0,
>                    cur_second_offset = ~0;
>      unsigned long count = 0;
> +    unsigned int flush = 0;
>      bool_t populate = (op == INSERT || op == ALLOCATE);
>  
>      spin_lock(&p2m->lock);
>  
> +    if ( d != current->domain )
> +        p2m_load_VTTBR(d);
> +
>      addr = start_gpaddr;
>      while ( addr < end_gpaddr )
>      {
> @@ -316,7 +320,7 @@ static int create_p2m_entries(struct domain *d,
>              cur_second_offset = second_table_offset(addr);
>          }
>  
> -        flush = third[third_table_offset(addr)].p2m.valid;
> +        flush |= third[third_table_offset(addr)].p2m.valid;
>  
>          /* Allocate a new RAM page and attach */
>          switch (op) {
> @@ -373,9 +377,6 @@ static int create_p2m_entries(struct domain *d,
>                  break;
>          }
>  
> -        if ( flush )
> -            flush_tlb_all_local();
> -
>          /* Preempt every 2MiB (mapped) or 32 MiB (unmapped) - arbitrary */
>          if ( op == RELINQUISH && count >= 0x2000 )
>          {
> @@ -392,6 +393,16 @@ static int create_p2m_entries(struct domain *d,
>          addr += PAGE_SIZE;
>      }
>  
> +    if ( flush )
> +    {
> +        /* At the beginning of the function, Xen is updating VTTBR
> +         * with the domain where the mappings are created. In this
> +         * case it's only necessary to flush TLBs on every CPUs with
> +         * the current VMID (our domain).
> +         */
> +        flush_tlb();
> +    }
> +
>      if ( op == ALLOCATE || op == INSERT )
>      {
>          unsigned long sgfn = paddr_to_pfn(start_gpaddr);
> @@ -409,6 +420,9 @@ out:
>      if (second) unmap_domain_page(second);
>      if (first) unmap_domain_page(first);
>  
> +    if ( d != current->domain )
> +        p2m_load_VTTBR(current->domain);
> +
>      spin_unlock(&p2m->lock);
>  
>      return rc;



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel