Re: [Xen-devel] [PATCH RFC] xen/gnttab: Avoid TLB flush if the grant mapped page was not accessed

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.07.14 at 14:54, <malcolm.crossley@xxxxxxxxxx> wrote:
> I believe Xen drops all attempts by guest to clear the accessed or dirty 
> bit's
> on the pte because Xen is the pte owner. This should prevent a malicous 
> guest
> from delibrately circumventing the TLB flush.

"believe" is clearly to weak here. This would need proving, and in fact
I don't think what you say is true: While Xen modifies the PTEs, it
doesn't own them (guests are relatively free in what to put there,
subject only to validation that they don't violate any isolation rules).
I.e. without you adding any code to prevent the accessed bit to be
cleared, there's not going to be any such guarantee. That would
entail having _PAGE_GNTTAB be non-zero also in non-debug builds,
resulting in put_page_from_l1e() crashing the guest if it tries to
drop such a PTE entry. Note that this alone wouldn't be enough:
mod_l1_entry() has a fast path bypassing the call to
put_page_from_l1e() - the l1e_has_changed() condition would need
amending for your purposes.

So I think what you want to do seems doable, but needs some more
care.

> @@ -4000,7 +4002,7 @@ int create_grant_host_mapping(uint64_t a
>          return create_grant_p2m_mapping(addr, frame, flags, cache_flags);
>  
>      grant_pte_flags =
> -        _PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_GNTTAB;
> +        _PAGE_PRESENT | _PAGE_GNTTAB;

So is there a reason to also drop the dirty bit here?

> @@ -81,6 +82,12 @@ struct gnttab_unmap_common {
>      struct domain *rd;
>  };
>  
> +#ifndef NDEBUG
> +atomic_t grant_unmap_tlb_flush_avoided;
> +atomic_t grant_unmap_tlb_flush_done;
> +atomic_t grant_unmap_operations;
> +#endif

These might even be useful to keep beyond the patch being RFC, but
then please static instead of having a "grant_" name prefix.

> @@ -924,7 +932,7 @@ static void
>      {
>          if ( (rc = replace_grant_host_mapping(op->host_addr,
>                                                op->frame, op->new_addr, 
> -                                              op->flags)) < 0 )
> +                                              op->flags, 
> &op->page_accessed)) < 0 )
>              goto unmap_out;
>  
>          ASSERT(act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask));

Considering the single use of replace_grant_host_mapping() being
this one, I think the interface would benefit from distinguishing flush
vs no-flush via zero/positive return values of the function. I that's
being disliked, the new parameter should in any event be "bool_t *".

> @@ -1082,20 +1090,25 @@ static long
>  gnttab_unmap_grant_ref(
>      XEN_GUEST_HANDLE_PARAM(gnttab_unmap_grant_ref_t) uop, unsigned int count)
>  {

Any reason you modify this one, but not gnttab_unmap_and_replace()?

Jan


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