Messages in this thread | | | Date | Tue, 8 Feb 2022 14:48:03 -0800 | From | Ira Weiny <> | Subject | Re: [PATCH V8 36/44] memremap_pages: Reserve a PKS PKey for eventual use by PMEM |
| |
On Sat, Feb 05, 2022 at 12:19:27AM -0800, Dan Williams wrote: > On Fri, Feb 4, 2022 at 9:40 PM Ira Weiny <ira.weiny@intel.com> wrote: > > > > On Fri, Feb 04, 2022 at 09:12:11AM -0800, Dan Williams wrote: > > > On Tue, Feb 1, 2022 at 10:35 AM Edgecombe, Rick P > > > <rick.p.edgecombe@intel.com> wrote: > > > > > > > > On Thu, 2022-01-27 at 09:54 -0800, ira.weiny@intel.com wrote: > > > > > enum pks_pkey_consumers { > > > > > - PKS_KEY_DEFAULT = 0, /* Must be 0 for default PTE > > > > > values */ > > > > > - PKS_KEY_TEST = 1, > > > > > - PKS_KEY_NR_CONSUMERS = 2, > > > > > + PKS_KEY_DEFAULT = 0, /* Must be 0 for default > > > > > PTE values */ > > > > > + PKS_KEY_TEST = 1, > > > > > + PKS_KEY_PGMAP_PROTECTION = 2, > > > > > + PKS_KEY_NR_CONSUMERS = 3, > > > > > }; > > > > > > > > The c spec says that any enum member that doesn't have an "=" will be > > > > one more than the previous member. As a consequence you can leave the > > > > "=" off PKS_KEY_NR_CONSUMERS and it will get auto adjusted when you add > > > > more like this. > > > > > > > > I know we've gone around and around on this, but why also specify the > > > > value for each key? They should auto increment and the first one is > > > > guaranteed to be zero. > > > > Because it was easier to ensure that the init value had all the defaults > > covered. > > > > > > > > > > Otherwise this doesn't use any of the features of "enum", it's just a > > > > verbose series of const int's. > > > > True but does this really matter? > > > > > > > > Going further, this can also build in support for dynamically (at > > > build time) freeing keys based on config, something like: > > > > > > enum { > > > #if IS_ENABLED(CONFIG_PKS_TEST) > > > PKS_KEY_TEST, > > > #endif > > > #if IS_ENABLED(CONFIG_DEVMAP_PROTECTION) > > > PKS_KEY_PGMAP_PROTECTION, > > > #endif > > > PKS_KEY_NR_CONSUMERS, > > > } > > > > This is all well and good until you get to the point of trying to define the > > initial MSR value. > > > > What Rick proposes without the Kconfig check is easier than this. But to do > > what both you and Rick suggest this is the best crap I've been able to come up > > with that actually works... > > > > > > /* pkeys_common.h */ > > #define PKR_AD_BIT 0x1u > > #define PKR_WD_BIT 0x2u > > #define PKR_BITS_PER_PKEY 2 > > > > #define PKR_PKEY_SHIFT(pkey) (pkey * PKR_BITS_PER_PKEY) > > > > #define PKR_KEY_INIT_RW(pkey) (0 << PKR_PKEY_SHIFT(pkey)) > > #define PKR_KEY_INIT_AD(pkey) (PKR_AD_BIT << PKR_PKEY_SHIFT(pkey)) > > #define PKR_KEY_INIT_WD(pkey) (PKR_WD_BIT << PKR_PKEY_SHIFT(pkey)) > > > > > > /* pks-keys.h */ > > #define PKR_KEY_MASK(pkey) (0xffffffff & ~((PKR_WD_BIT|PKR_AD_BIT) << PKR_PKEY_SHIFT(pkey))) > > > > enum pks_pkey_consumers { > > PKS_KEY_DEFAULT = 0, /* Must be 0 for default PTE values */ > > #if IS_ENABLED(CONFIG_PKS_TEST) > > PKS_KEY_TEST, > > #endif > > #if IS_ENABLED(CONFIG_DEVMAP_ACCESS_PROTECTION) > > PKS_KEY_PGMAP_PROTECTION, > > #endif > > PKS_KEY_NR_CONSUMERS > > }; > > > > #define PKS_DEFAULT_VALUE PKR_KEY_INIT_RW(PKS_KEY_DEFAULT) > > #define PKS_DEFAULT_MASK PKR_KEY_MASK(PKS_KEY_DEFAULT) > > > > #if IS_ENABLED(CONFIG_PKS_TEST) > > #define PKS_TEST_VALUE PKR_KEY_INIT_AD(PKS_KEY_TEST) > > #define PKS_TEST_MASK PKR_KEY_MASK(PKS_KEY_TEST) > > #else > > /* Just define another default value to fool the CPP */ > > #define PKS_TEST_VALUE PKR_KEY_INIT_RW(0) > > #define PKS_TEST_MASK PKR_KEY_MASK(0) > > #endif > > > > #if IS_ENABLED(CONFIG_DEVMAP_ACCESS_PROTECTION) > > #define PKS_PGMAP_VALUE PKR_KEY_INIT_AD(PKS_KEY_PGMAP_PROTECTION) > > #define PKS_PGMAP_MASK PKR_KEY_MASK(PKS_KEY_PGMAP_PROTECTION) > > #else > > /* Just define another default value to fool the CPP */ > > #define PKS_PGMAP_VALUE PKR_KEY_INIT_RW(0) > > #define PKS_PGMAP_MASK PKR_KEY_MASK(0) > > #endif > > > > #define PKS_INIT_VALUE ((0xFFFFFFFF & \ > > (PKS_DEFAULT_MASK & \ > > PKS_TEST_MASK & \ > > PKS_PGMAP_MASK \ > > )) | \ > > (PKS_DEFAULT_VALUE | \ > > PKS_TEST_VALUE | \ > > PKS_PGMAP_VALUE \ > > ) \ > > ) > > > > > > I find the above much harder to parse and of little value. I'm pretty sure > > that someone adding a key is much more likely to get the macro maze wrong. > > Reviewing a patch to add a key would be much more difficult as well, IMHO. > > > > I'm a bit tired of this back and forth trying to implement this for features > > which may never exist. In all my discussions I don't think we have reached > > more than 10 use cases in our wildest dreams and only 4 have even been > > attempted with real code including the PKS test. > > > > So I'm just a bit frustrated with the effort we have put in to try and do > > dynamic or even compile time dynamic keys. > > > > Anyway I'll think on it more. But I'm inclined to leave it alone right now. > > What I have is easy to review for correctness and only takes a bit of effort to > > actually use. > > Sorry, for the thrash Ira, I felt compelled to put some skin in the > game and came up with the below. Ignore the names they are just to > show the idea: > > #define KEYVAL(prev, config) (prev + __is_defined(config)) > #define KEYDEFAULT(key, default, config) ((default << (key * 2)) * > __is_defined(config)) > #define KEYX KEYVAL(0, ENABLE_X) > #define KEYY KEYVAL(KEYX, ENABLE_Y) > #define KEYZ KEYVAL(KEYY, ENABLE_Z) > #define KEYMAX KEYVAL(KEYZ, 1) > > #define KEY0_INIT 0x1 > #define KEYX_INIT KEYDEFAULT(KEYX, 2, ENABLE_X) > #define KEYY_INIT KEYDEFAULT(KEYY, 2, ENABLE_Y) > #define KEYZ_INIT KEYDEFAULT(KEYZ, 2, ENABLE_Z) > > #define ALL_AD 0x55555555 > #define DEFAULT (ALL_AD & (GENMASK(31, KEYMAX * 2))) | KEYX_INIT | > KEYY_INIT | KEYZ_INIT | KEY0_INIT > > The idea is that this relies on a defined key order, but still > dynamically assigns key values at compile time. It's not as simple as > an enum, but it still seems readable to me. > > The definition is done in 2 parts key slot numbers (KEYVAL()) and the > values they need to inject into the combined global init mask > (KEYDEFAULT()). The KEYVAL() section above defines the key order where > each key gets an incremented value in the order, but only if the > previous key was defined. KEYDEFAULT() defines the init value and > optionally zeros out the init value if the config is disabled. Finally > the DEFAULT mask is initialized to all 5s if there are zero keys > defined, but otherwise masks 2-bits per defined key + 1 and ORs in the > corresponding key init values. This uses some of the magic behind the > IS_ENABLED() macro to turn undefined symbols into 0s. > > It seems to work, for example if I just define KEYX and KEYZ, > > #define ENABLE_X 1 > #define ENABLE_Z 1 > > printf("X: %d:%#x Y: %d:%#x Z: %d:%#x MAX: %d DEFAULT: %#x\n", KEYX, > KEYX_INIT, KEYY, KEYY_INIT, KEYZ, KEYZ_INIT, KEYMAX, DEFAULT); > > # ./a.out > X: 1:0x8 Y: 1:0 Z: 2:0x20 MAX: 3 DEFAULT: 0x55555569
Yes this seems to work. I still think it is a bit clunky, but after documenting it I think it is clear enough. I also played with making it a bit more straight forward with the macro names.
Here is the doc:
/** * DOC: PKS_KEY_ALLOCATION * * Users reserve a key value in 5 steps. * 1) Use PKS_NEW_KEY to create a new key * 2) Ensure that the last key value is used in the PKS_NEW_KEY macro * 3) Adjust PKS_KEY_MAX to use the newly defined key value * 4) Use PKS_DECLARE_INIT_VALUE to define an initial value * 5) Add the new PKS default value to PKS_INIT_VALUE * * The PKS_NEW_KEY and PKS_DECLARE_INIT_VALUE macros require the Kconfig * option to be specified to automatically adjust the number of keys used. * * PKS_KEY_DEFAULT must remain 0 (prev = 0) key with a default of * PKS_DECLARE_DEFAULT_RW support non-pks protected pages. * * For example to configure a key for 'MY_FEATURE' with a default of Write * Disabled. * * .. code-block:: c * * #define PKS_KEY_DEFAULT PKS_NEW_KEY(0, 1) * * // 1) Add PKS_KEY_MY_FEATURE * // 2) Be sure to use the last defined key in the macro * #define PKS_KEY_MY_FEATURE PKS_NEW_KEY(PKS_KEY_DEFAULT, CONFIG_MY_FEATURE) * * // 3) Adjust PKS_KEY_MAX * #define PKS_KEY_MAX PKS_NEW_KEY(PKS_KEY_MY_FEATURE, 1) * * * #define PKS_KEY_DEFAULT_INIT PKS_DECLARE_INIT_VALUE(PKS_KEY_DEFAULT, RW, 1) * * // 4) Define initial value * #define PKS_KEY_MY_FEATURE_INIT PKS_DECLARE_INIT_VALUE(PKS_KEY_MY_FEATURE, WD, CONFIG_MY_FEATURE) * * // 5) Add initial value to PKS_INIT_VALUE * #define PKS_INIT_VALUE ((PKS_ALL_AD & (GENMASK(31, PKS_KEY_MAX * PKR_BITS_PER_PKEY))) | \ * PKS_KEY_DEFAULT_INIT | \ * PKS_KEY_MY_FEATURE_INIT | \ * ) */
Let me know if this is clear enough? Ira
| |