| Message ID | 1528937115-10132-9-git-send-email-linuxram@us.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/13/2018 05:44 PM, Ram Pai wrote: > If the flag is 0, no bits will be set. Hence we cant expect > the resulting bitmap to have a higher value than what it > was earlier ... > if (flags) > - pkey_assert(read_pkey_reg() > orig_pkey_reg); > + pkey_assert(read_pkey_reg() >= orig_pkey_reg); > dprintf1("END<---%s(%d, 0x%x)\n", __func__, > pkey, flags); > } This is the kind of thing where I'd love to hear the motivation and background. This "disable a key that was already disabled" operation obviously doesn't happen today. What motivated you to change it now?
On Wed, Jun 20, 2018 at 07:47:02AM -0700, Dave Hansen wrote: > On 06/13/2018 05:44 PM, Ram Pai wrote: > > If the flag is 0, no bits will be set. Hence we cant expect > > the resulting bitmap to have a higher value than what it > > was earlier > ... > > if (flags) > > - pkey_assert(read_pkey_reg() > orig_pkey_reg); > > + pkey_assert(read_pkey_reg() >= orig_pkey_reg); > > dprintf1("END<---%s(%d, 0x%x)\n", __func__, > > pkey, flags); > > } > > This is the kind of thing where I'd love to hear the motivation and > background. This "disable a key that was already disabled" operation > obviously doesn't happen today. What motivated you to change it now? On powerpc, hardware supports READ_DISABLE and WRITE_DISABLE. ACCESS_DISABLE is basically READ_DISABLE|WRITE_DISABLE on powerpc. If access disable is called on a key followed by a write disable, the second operation becomes a nop. In such cases, read_pkey_reg() == orig_pkey_reg Hence the code above is modified to pkey_assert(read_pkey_reg() >= orig_pkey_reg);
On 07/17/2018 08:58 AM, Ram Pai wrote: > On Wed, Jun 20, 2018 at 07:47:02AM -0700, Dave Hansen wrote: >> On 06/13/2018 05:44 PM, Ram Pai wrote: >>> If the flag is 0, no bits will be set. Hence we cant expect >>> the resulting bitmap to have a higher value than what it >>> was earlier >> ... >>> if (flags) >>> - pkey_assert(read_pkey_reg() > orig_pkey_reg); >>> + pkey_assert(read_pkey_reg() >= orig_pkey_reg); >>> dprintf1("END<---%s(%d, 0x%x)\n", __func__, >>> pkey, flags); >>> } >> This is the kind of thing where I'd love to hear the motivation and >> background. This "disable a key that was already disabled" operation >> obviously doesn't happen today. What motivated you to change it now? > On powerpc, hardware supports READ_DISABLE and WRITE_DISABLE. > ACCESS_DISABLE is basically READ_DISABLE|WRITE_DISABLE on powerpc. > > If access disable is called on a key followed by a write disable, the > second operation becomes a nop. In such cases, > read_pkey_reg() == orig_pkey_reg > > Hence the code above is modified to > pkey_assert(read_pkey_reg() >= orig_pkey_reg); Makes sense. Do we have a comment for that now?
diff --git a/tools/testing/selftests/vm/protection_keys.c b/tools/testing/selftests/vm/protection_keys.c index 57340b3..5fcccdb 100644 --- a/tools/testing/selftests/vm/protection_keys.c +++ b/tools/testing/selftests/vm/protection_keys.c @@ -415,7 +415,7 @@ void pkey_disable_set(int pkey, int flags) dprintf1("%s(%d) pkey_reg: 0x"PKEY_REG_FMT"\n", __func__, pkey, read_pkey_reg()); if (flags) - pkey_assert(read_pkey_reg() > orig_pkey_reg); + pkey_assert(read_pkey_reg() >= orig_pkey_reg); dprintf1("END<---%s(%d, 0x%x)\n", __func__, pkey, flags); }
If the flag is 0, no bits will be set. Hence we cant expect the resulting bitmap to have a higher value than what it was earlier. cc: Dave Hansen <dave.hansen@intel.com> cc: Florian Weimer <fweimer@redhat.com> Signed-off-by: Ram Pai <linuxram@us.ibm.com> --- tools/testing/selftests/vm/protection_keys.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)