Re: [mtosatti-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org: [patch 3/4] paravirt: set_access_flags/set_wrprotect should use paravirt interface]

[Jeremy Fitzhardinge <jeremy-TSDbQ3PG+2Y@public.gmane.org>]

Marcelo Tosatti wrote:
> Forgot to copy you... Ideally all pte updates should be done via the
> paravirt interface.
>   

Hm, are you sure?

> +static inline void pte_clear_bit(unsigned int bit, pte_t *ptep)
> +{
> +	pte_t pte = *ptep;
> +	clear_bit(bit, (unsigned long *)&pte.pte);
> +	set_pte(ptep, pte);
> +}
>   

This is not a safe transformation.  This will lose hardware A/D bit 
changes if the pte is part of an active pagetable.  Aside from that, 
clear_bit is atomic and so is the wrong thing to use on a local 
variable; a plain bitmask would do the job.

clear_bit on a PTE is fine, since everyone has to support some level of 
trap'n'emulate on pte-level entries (ptep_get_and_clear is very hard to 
implement otherwise).  Shadow pagetable implementations will do this 
naturally, and Xen must do it to allow atomic updates on ptes (also 
because it makes late-pinning/early-unpinning a performance win).

> +
>  static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
>  			      pte_t *ptep, pte_t pte)
>  {
> Index: linux-2.6-x86-kvm/include/asm-x86/pgtable.h
> ===================================================================
> --- linux-2.6-x86-kvm.orig/include/asm-x86/pgtable.h
> +++ linux-2.6-x86-kvm/include/asm-x86/pgtable.h
> @@ -227,6 +227,8 @@ void native_pagetable_setup_done(pgd_t *
>  #define pte_update(mm, addr, ptep)              do { } while (0)
>  #define pte_update_defer(mm, addr, ptep)        do { } while (0)
>  
> +#define pte_clear_bit(bit, ptep)	clear_bit(bit, (unsigned long *)&ptep->pte)
> +
>  static inline void paravirt_pagetable_setup_start(pgd_t *base)
>  {
>  	native_pagetable_setup_start(base);
> @@ -302,7 +304,7 @@ static inline void native_set_pte_at(str
>  ({									\
>  	int __changed = !pte_same(*(ptep), entry);			\
>  	if (__changed && dirty) {					\
> -		*ptep = entry;						\
> +		set_pte(ptep, entry);					\
>   

This change is OK, but doesn't really do anything.  All hypervisors 
allow direct access to ptes.

>  		pte_update_defer((vma)->vm_mm, (address), (ptep));	\
>  		flush_tlb_page(vma, address);				\
>  	}								\
> @@ -357,7 +359,7 @@ static inline pte_t ptep_get_and_clear_f
>  #define __HAVE_ARCH_PTEP_SET_WRPROTECT
>  static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
>  {
> -	clear_bit(_PAGE_BIT_RW, (unsigned long *)&ptep->pte);
> +	pte_clear_bit(_PAGE_BIT_RW, ptep);
>   

This is only valid if ptep_set_wrprotect is never used on active ptes, 
which I don't think is the case.

    J

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/