| Message ID | 20210804043231.2655537-4-ira.weiny@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PKS/PMEM: Add Stray Write Protection | expand |
On Tue, Aug 03 2021 at 21:32, ira weiny wrote: > @@ -200,16 +200,14 @@ __setup("init_pkru=", setup_init_pkru); > */ > u32 update_pkey_val(u32 pk_reg, int pkey, unsigned int flags) > { > - int pkey_shift = pkey * PKR_BITS_PER_PKEY; > - > /* Mask out old bit values */ > - pk_reg &= ~(((1 << PKR_BITS_PER_PKEY) - 1) << pkey_shift); > + pk_reg &= ~PKR_PKEY_MASK(pkey); > > /* Or in new values */ > if (flags & PKEY_DISABLE_ACCESS) > - pk_reg |= PKR_AD_BIT << pkey_shift; > + pk_reg |= PKR_AD_KEY(pkey); > if (flags & PKEY_DISABLE_WRITE) > - pk_reg |= PKR_WD_BIT << pkey_shift; > + pk_reg |= PKR_WD_KEY(pkey); I'm not seeing how this is improving that code. Quite the contrary. Thanks, tglx
On Thu, Nov 25 2021 at 15:25, Thomas Gleixner wrote: > On Tue, Aug 03 2021 at 21:32, ira weiny wrote: >> @@ -200,16 +200,14 @@ __setup("init_pkru=", setup_init_pkru); >> */ >> u32 update_pkey_val(u32 pk_reg, int pkey, unsigned int flags) >> { >> - int pkey_shift = pkey * PKR_BITS_PER_PKEY; >> - >> /* Mask out old bit values */ >> - pk_reg &= ~(((1 << PKR_BITS_PER_PKEY) - 1) << pkey_shift); >> + pk_reg &= ~PKR_PKEY_MASK(pkey); >> >> /* Or in new values */ >> if (flags & PKEY_DISABLE_ACCESS) >> - pk_reg |= PKR_AD_BIT << pkey_shift; >> + pk_reg |= PKR_AD_KEY(pkey); >> if (flags & PKEY_DISABLE_WRITE) >> - pk_reg |= PKR_WD_BIT << pkey_shift; >> + pk_reg |= PKR_WD_KEY(pkey); > > I'm not seeing how this is improving that code. Quite the contrary. Aside of that why are you ordering it the wrong way around, i.e. 1) implement turd 2) polish turd instead of implementing the required helpers first if they are really providing value. Thanks, tglx
On Thu, Nov 25, 2021 at 03:25:09PM +0100, Thomas Gleixner wrote: > On Tue, Aug 03 2021 at 21:32, ira weiny wrote: > > @@ -200,16 +200,14 @@ __setup("init_pkru=", setup_init_pkru); > > */ > > u32 update_pkey_val(u32 pk_reg, int pkey, unsigned int flags) > > { > > - int pkey_shift = pkey * PKR_BITS_PER_PKEY; > > - > > /* Mask out old bit values */ > > - pk_reg &= ~(((1 << PKR_BITS_PER_PKEY) - 1) << pkey_shift); > > + pk_reg &= ~PKR_PKEY_MASK(pkey); > > > > /* Or in new values */ > > if (flags & PKEY_DISABLE_ACCESS) > > - pk_reg |= PKR_AD_BIT << pkey_shift; > > + pk_reg |= PKR_AD_KEY(pkey); > > if (flags & PKEY_DISABLE_WRITE) > > - pk_reg |= PKR_WD_BIT << pkey_shift; > > + pk_reg |= PKR_WD_KEY(pkey); > > I'm not seeing how this is improving that code. Quite the contrary. Fair enough. Even more so when using the code you suggested for pkey_update_pkval(). In that case it boils down to: diff --git a/arch/x86/mm/pkeys.c b/arch/x86/mm/pkeys.c index eb6d6b872652..b7127329d115 100644 --- a/arch/x86/mm/pkeys.c +++ b/arch/x86/mm/pkeys.c @@ -198,7 +198,7 @@ __setup("init_pkru=", setup_init_pkru); */ u32 pkey_update_pkval(u32 pkval, int pkey, u32 accessbits) { - int shift = pkey * PKR_BITS_PER_PKEY; + int shift = PKR_PKEY_SHIFT(pkey); if (WARN_ON_ONCE(accessbits & ~PKEY_ACCESS_MASK)) accessbits &= PKEY_ACCESS_MASK; Better? As to the reason of why to put this patch after the other one. Why would I improve the old pre-refactoring code only to throw it away when moving it to pkey_update_pkval()? This reasoning is even stronger when pkey_update_pkval() is implemented. I agree with Dan regarding the macros though. I think they make it easier to see what is going on without dealing with masks and shifts directly. But I can remove this patch if you feel that strongly about it. Ira > > Thanks, > > tglx
Ira, On Tue, Dec 07 2021 at 16:51, Ira Weiny wrote: > On Thu, Nov 25, 2021 at 03:25:09PM +0100, Thomas Gleixner wrote: > > u32 pkey_update_pkval(u32 pkval, int pkey, u32 accessbits) > { > - int shift = pkey * PKR_BITS_PER_PKEY; > + int shift = PKR_PKEY_SHIFT(pkey); > > if (WARN_ON_ONCE(accessbits & ~PKEY_ACCESS_MASK)) > accessbits &= PKEY_ACCESS_MASK; > > Better? Let me postpone this question. > As to the reason of why to put this patch after the other one. Why would I > improve the old pre-refactoring code only to throw it away when moving it to > pkey_update_pkval()? This reasoning is even stronger when pkey_update_pkval() > is implemented. Which refactoring? We seem to have fundamentally definitions of that term. Let me illustrate why. The original version of this was: u32 get_new_pkr(u32 old_pkr, int pkey, unsigned long init_val) { int pkey_shift = (pkey * PKR_BITS_PER_PKEY); u32 new_pkr_bits = 0; /* Set the bits we need in the register: */ if (init_val & PKEY_DISABLE_ACCESS) new_pkr_bits |= PKR_AD_BIT; if (init_val & PKEY_DISABLE_WRITE) new_pkr_bits |= PKR_WD_BIT; /* Shift the bits in to the correct place: */ new_pkr_bits <<= pkey_shift; /* Mask off any old bits in place: */ old_pkr &= ~((PKR_AD_BIT | PKR_WD_BIT) << pkey_shift); /* Return the old part along with the new part: */ return old_pkr | new_pkr_bits; } IOW, mechanical Cut & Paste. Then PeterZ came along and suggested to improve it this way: u32 update_pkey_val(u32 pk_reg, int pkey, unsigned int flags) { int pkey_shift = pkey * PKR_BITS_PER_PKEY; /* Mask out old bit values */ pk_reg &= ~(((1 << PKR_BITS_PER_PKEY) - 1) << pkey_shift); /* Or in new values */ if (flags & PKEY_DISABLE_ACCESS) pk_reg |= PKR_AD_BIT << pkey_shift; if (flags & PKEY_DISABLE_WRITE) pk_reg |= PKR_WD_BIT << pkey_shift; return pk_reg; } which is already better. So you changed your approach from Cut & Paste to Copy & Paste. But neither Cut & Paste nor Copy & Paste match what refactoring is really about. Just throwing the term refactoring at it does not make it so. Refactoring is about improving the code in design and implementation. The keyword is: improving. There are obviously cases where you can take the code as is and split it out into a new helper function. You really have to look at it and answer the question whether it's good code or not, whether it could be written in better ways and with improved functionality. I could have given you this minimalistic one: u32 pkey_update_pkval(u32 pkval, int pkey, u32 accessbits) { int shift = pkey * PKR_BITS_PER_PKEY; pkval &= ~(PKEY_ACCESS_MASK << shift); return pkval | (accessbit & PKEY_ACCESS_MASK) << shift; } But I gave you this: u32 pkey_update_pkval(u32 pkval, int pkey, u32 accessbits) { int shift = pkey * PKR_BITS_PER_PKEY; if (WARN_ON_ONCE(accessbits & ~PKEY_ACCESS_MASK)) accessbits &= PKEY_ACCESS_MASK; pkval &= ~(PKEY_ACCESS_MASK << shift); return pkval | accessbit << shift; } This is what refactoring is about. See? > I agree with Dan regarding the macros though. I think they make it easier to > see what is going on without dealing with masks and shifts directly. But I can > remove this patch if you feel that strongly about it. I'm not against macros per se, but not everything is automatically better when it is hidden behind a macro. What I'm arguing against is the claim that macros are an improvement by definition. Especially when they are just blindly thrown into code which should not exist in the first place. Also versus ordering. What's wrong with doing it this way: 1) Define the macros first without changing the code 2) Implement pkey_update_pkval() in a sensible way and use the macros where appropriate. Thereby replacing the existing version in the other function. Which would end up in the obviously even simpler code: u32 pkey_update_pkval(u32 pkval, int pkey, u32 accessbits) { if (WARN_ON_ONCE(accessbits & ~PKEY_ACCESS_MASK)) accessbits &= PKEY_ACCESS_MASK; pkval &= ~PKR_PKEY_VALUE(pkey, PKEY_ACCESS_MASK); return pkval | PKR_PKEY_VALUE(pkey, accessbits); } That fits the goal of that macro exercise to make it easy to read and obvious what's going on, no? Instead of: > u32 pkey_update_pkval(u32 pkval, int pkey, u32 accessbits) > { > - int shift = pkey * PKR_BITS_PER_PKEY; > + int shift = PKR_PKEY_SHIFT(pkey); > > if (WARN_ON_ONCE(accessbits & ~PKEY_ACCESS_MASK)) > accessbits &= PKEY_ACCESS_MASK; > > pkval &= ~(PKEY_ACCESS_MASK << shift); > return pkval | accessbit << shift; > } > > Better? You surely can answer this question yourself, no? "By continuously improving the design of code, we make it easier and easier to work with. This is in sharp contrast to what typically happens: little refactoring and a great deal of attention paid to expediently adding new features. If you get into the hygienic habit of refactoring continuously, you'll find that it is easier to extend and maintain code." -- Joshua Kerievsky If you study that quote carefully, you surely can find our diverging approach to refactoring in it, no? Thanks, tglx
diff --git a/arch/x86/include/asm/pkeys_common.h b/arch/x86/include/asm/pkeys_common.h index f3277717faeb..8a3c6d2e6a8a 100644 --- a/arch/x86/include/asm/pkeys_common.h +++ b/arch/x86/include/asm/pkeys_common.h @@ -6,6 +6,10 @@ #define PKR_WD_BIT 0x2 #define PKR_BITS_PER_PKEY 2 -#define PKR_AD_KEY(pkey) (PKR_AD_BIT << ((pkey) * PKR_BITS_PER_PKEY)) +#define PKR_PKEY_SHIFT(pkey) (pkey * PKR_BITS_PER_PKEY) +#define PKR_PKEY_MASK(pkey) (((1 << PKR_BITS_PER_PKEY) - 1) << PKR_PKEY_SHIFT(pkey)) + +#define PKR_AD_KEY(pkey) (PKR_AD_BIT << PKR_PKEY_SHIFT(pkey)) +#define PKR_WD_KEY(pkey) (PKR_WD_BIT << PKR_PKEY_SHIFT(pkey)) #endif /*_ASM_X86_PKEYS_COMMON_H */ diff --git a/arch/x86/include/asm/pkru.h b/arch/x86/include/asm/pkru.h index a74325b0d1df..fb44ff542028 100644 --- a/arch/x86/include/asm/pkru.h +++ b/arch/x86/include/asm/pkru.h @@ -15,15 +15,13 @@ extern u32 init_pkru_value; static inline bool __pkru_allows_read(u32 pkru, u16 pkey) { - int pkru_pkey_bits = pkey * PKR_BITS_PER_PKEY; - return !(pkru & (PKR_AD_BIT << pkru_pkey_bits)); + return !(pkru & PKR_AD_KEY(pkey)); } static inline bool __pkru_allows_write(u32 pkru, u16 pkey) { - int pkru_pkey_bits = pkey * PKR_BITS_PER_PKEY; /* Access-disable disables writes too so check both bits here. */ - return !(pkru & ((PKR_AD_BIT|PKR_WD_BIT) << pkru_pkey_bits)); + return !(pkru & (PKR_AD_KEY(pkey) | PKR_WD_KEY(pkey))); } static inline u32 read_pkru(void) diff --git a/arch/x86/mm/pkeys.c b/arch/x86/mm/pkeys.c index ca2e20b18645..75437aa8fc56 100644 --- a/arch/x86/mm/pkeys.c +++ b/arch/x86/mm/pkeys.c @@ -200,16 +200,14 @@ __setup("init_pkru=", setup_init_pkru); */ u32 update_pkey_val(u32 pk_reg, int pkey, unsigned int flags) { - int pkey_shift = pkey * PKR_BITS_PER_PKEY; - /* Mask out old bit values */ - pk_reg &= ~(((1 << PKR_BITS_PER_PKEY) - 1) << pkey_shift); + pk_reg &= ~PKR_PKEY_MASK(pkey); /* Or in new values */ if (flags & PKEY_DISABLE_ACCESS) - pk_reg |= PKR_AD_BIT << pkey_shift; + pk_reg |= PKR_AD_KEY(pkey); if (flags & PKEY_DISABLE_WRITE) - pk_reg |= PKR_WD_BIT << pkey_shift; + pk_reg |= PKR_WD_KEY(pkey); return pk_reg; }