Linux Virtualization - Apr 2007 - [PATCH 5/6] i386 virtualization - Make generic set wrprotect a macro

* zach@vmware.com (zach@vmware.com) wrote:
> Make the generic version of ptep_set_wrprotect a macro.  This is good for
> code uniformity, and fixes the build for architectures which include
pgtable.h
> through headers into assembly code, but do not define a ptep_set_wrprotect
> function.
This one is unrelated to other descriptor related changes.  Why is it
included in this series?
> Signed-off-by: Zachary Amsden <zach@vmware.com>
> Index: linux-2.6.13/include/asm-generic/pgtable.h
> ==================================================================> ---
linux-2.6.13.orig/include/asm-generic/pgtable.h	2005-08-12 12:12:55.000000000
-0700
> +++ linux-2.6.13/include/asm-generic/pgtable.h	2005-08-15
13:54:42.000000000 -0700
> @@ -313,11 +313,12 @@
>  #endif
>  
>  #ifndef __HAVE_ARCH_PTEP_SET_WRPROTECT
> -static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long
address, pte_t *ptep)
> -{
> -	pte_t old_pte = *ptep;
> -	set_pte_at(mm, address, ptep, pte_wrprotect(old_pte));
> -}
> +#define ptep_set_wrprotect(__mm, __address, __ptep)			\
> +({									\
> +	pte_t __old_pte = *(__ptep);					\
> +	set_pte_at((__mm), (__address), (__ptep),			\
> +			pte_wrprotect(__old_pte));			\
> +})
>  #endif
I'm not sure I agree with this approach (although I understand the
motivation).  This should at least be a do {} while(0) type macro,
since it's not returning a value.

thanks,
-chris

Adrian Bunk wrote:
>On Mon, Aug 15, 2005 at 04:00:39PM -0700, zach@vmware.com wrote:
>
>  
>
>>Make the generic version of ptep_set_wrprotect a macro.  This is good
for
>>code uniformity, and fixes the build for architectures which include
pgtable.h
>>through headers into assembly code, but do not define a
ptep_set_wrprotect
>>function.
>>    
>>
>
>
>This against the kernel coding style.
>In fact, we are usually doing exactly the opposite. 
>
>What exactly is the technical problem this patch is trying to solve, IOW 
>which architectures are breaking for you?
>  
>
The generic pgtable.h include is special and apparently deliberately 
against kernel coding style.  Look at the rest of the file.  All 
"functions" here are purely macros, or encapsulated with:

#ifndef __ASSEMBLY__
static inline void foo()
#endif

This is because asm-generic/pgtable.h can get included in assembler 
files via a number of ways.

Now, if you have a header file that gets conditionally excluded based on 
#ifndef __ASSEMBLY__,  as asm-i386/pgtable.h does to 
pgtable-{2|3}level.h you must do one of the following:

1) move all  __HAVE_ARCH_PTEP_XXX definitions out of the !__ASSEMBLY__ 
clause
2) protect all inline assembler functions in asm-generic/pgtable.h with 
!__ASSEMBLY
3) use macros instead of inline functions in asm-generic

Having the ability to redefine page table accessors at the sub-arch 
level is necessary to have a paravirtualized sub-arch of i386.  My third 
attempt at this (the first was a horror unthinkable to even publish) is 
trying to make the code as clean and consistent as possible.  #1 above 
makes maintaing compile time PAE for i386 with a paravirtualized 
sub-arch cumbersome, since one must either isolate the __HAVE_ARCH _XXX 
defines from the XXX function definition itself, surround each 
individual function with !__ASSEMBLY__, or switch to macros instead of 
inline functions for include/asm-i386/pgtable-{2|3}level.h.  Ugly and 
difficult to maintain.

Thus, I chose the default convention of following the surrounding code.  
There are 6 C inline functions in the generic pgtable.h and 37 macros.  
Converting to and from macros and inline functions here is rather 
tedious and error prone, all of these functions are conditionally 
defined based on the architecture, and I don't want to risk introducing 
yet another regression for an architecture that I don't have a 
cross-compile set up for.

If you have a better approch, I'd be interested in hearing it.

Zach

On Mon, Aug 15, 2005 at 04:00:39PM -0700, zach@vmware.com wrote:
> Make the generic version of ptep_set_wrprotect a macro.  This is good for
> code uniformity, and fixes the build for architectures which include
pgtable.h
> through headers into assembly code, but do not define a ptep_set_wrprotect
> function.

This against the kernel coding style.
In fact, we are usually doing exactly the opposite. 

What exactly is the technical problem this patch is trying to solve, IOW 
which architectures are breaking for you?

> Signed-off-by: Zachary Amsden <zach@vmware.com>
> Index: linux-2.6.13/include/asm-generic/pgtable.h
> ==================================================================> ---
linux-2.6.13.orig/include/asm-generic/pgtable.h	2005-08-12 12:12:55.000000000
-0700
> +++ linux-2.6.13/include/asm-generic/pgtable.h	2005-08-15
13:54:42.000000000 -0700
> @@ -313,11 +313,12 @@
>  #endif
>  
>  #ifndef __HAVE_ARCH_PTEP_SET_WRPROTECT
> -static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long
address, pte_t *ptep)
> -{
> -	pte_t old_pte = *ptep;
> -	set_pte_at(mm, address, ptep, pte_wrprotect(old_pte));
> -}
> +#define ptep_set_wrprotect(__mm, __address, __ptep)			\
> +({									\
> +	pte_t __old_pte = *(__ptep);					\
> +	set_pte_at((__mm), (__address), (__ptep),			\
> +			pte_wrprotect(__old_pte));			\
> +})
>  #endif
>  
>  #ifndef __HAVE_ARCH_PTE_SAME

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed