| Message ID | 20170908223102.20470-1-bauerman@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] core/init: Add ibm, processor-storage-keys property to /cpus DT node. | expand |
On Fri, Sep 08, 2017 at 07:31:02PM -0300, Thiago Jung Bauermann wrote: > LoPAPR says: > > “ibm,processor-storage-keys” > > property name indicating the number of virtual storage keys supported > by the processor described by this node. > > prop-encoded-array: Consists of two cells encoded as with encode-int. > The first cell represents the number of virtual storage keys supported > for data accesses while the second cell represents the number of > virtual storage keys supported for instruction accesses. The cell value > of zero indicates that no storage keys are supported for the access > type. > > pHyp provides the property above but there's a bug in P8 firmware where the > second cell is zero even though POWER8 supports instruction access keys. > This bug will be fixed for P9. > > While this is a PAPR property, it's useful to have it in powernv as well > so that Linux has a uniform way of checking for the feature regardless of > the platform it's running on. > > On PAPR there is one property for each CPU node, but since it's highly > unlikely that different CPUs will support a different number of keys, we > put the property in the /cpus node instead. > > Tested on QEMU POWER8 powernv model and Mambo P9. > > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> > --- > core/init.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > Changes in v2: > - Put property in /cpus instead of in each CPU node. > > diff --git a/core/init.c b/core/init.c > index 8951e17b4c90..4a6f9155511d 100644 > --- a/core/init.c > +++ b/core/init.c > @@ -563,6 +563,22 @@ void __noreturn load_and_boot_kernel(bool is_reboot) > start_kernel(kernel_entry, fdt, mem_top); > } > > +static void storage_keys_fixup(void) > +{ > + /* P7 doesn't support instruction access keys. */ > + const u32 insn_keys = (proc_gen == proc_gen_p7) ? 0 : 32; theortically we should be checking for earlier gens too. right? It should be const u32 insn_keys = (proc_gen <= proc_gen_p7) ? 0 : 32; > + struct dt_node *cpus = dt_find_by_path(dt_root, "/cpus"); > + > + assert(cpus); > + > + if (proc_gen == proc_gen_unknown) > + return; > + else if (dt_has_node_property(cpus, "ibm,processor-storage-keys", NULL)) > + return; > + > + dt_add_property_cells(cpus, "ibm,processor-storage-keys", 32, insn_keys); > +} > + > static void dt_fixups(void) > { > struct dt_node *n; > @@ -590,6 +606,8 @@ static void dt_fixups(void) > if (!dt_has_node_property(n, "scom-controller", NULL)) > dt_add_property(n, "scom-controller", NULL, 0); > } > + > + storage_keys_fixup(); > } > > static void add_arch_vector(void)
Ram Pai <linuxram@us.ibm.com> writes: > On Fri, Sep 08, 2017 at 07:31:02PM -0300, Thiago Jung Bauermann wrote: >> LoPAPR says: >> >> “ibm,processor-storage-keys” >> >> property name indicating the number of virtual storage keys supported >> by the processor described by this node. >> >> prop-encoded-array: Consists of two cells encoded as with encode-int. >> The first cell represents the number of virtual storage keys supported >> for data accesses while the second cell represents the number of >> virtual storage keys supported for instruction accesses. The cell value >> of zero indicates that no storage keys are supported for the access >> type. >> >> pHyp provides the property above but there's a bug in P8 firmware where the >> second cell is zero even though POWER8 supports instruction access keys. >> This bug will be fixed for P9. >> >> While this is a PAPR property, it's useful to have it in powernv as well >> so that Linux has a uniform way of checking for the feature regardless of >> the platform it's running on. >> >> On PAPR there is one property for each CPU node, but since it's highly >> unlikely that different CPUs will support a different number of keys, we >> put the property in the /cpus node instead. >> >> Tested on QEMU POWER8 powernv model and Mambo P9. >> >> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> >> --- >> core/init.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> Changes in v2: >> - Put property in /cpus instead of in each CPU node. >> >> diff --git a/core/init.c b/core/init.c >> index 8951e17b4c90..4a6f9155511d 100644 >> --- a/core/init.c >> +++ b/core/init.c >> @@ -563,6 +563,22 @@ void __noreturn load_and_boot_kernel(bool is_reboot) >> start_kernel(kernel_entry, fdt, mem_top); >> } >> >> +static void storage_keys_fixup(void) >> +{ >> + /* P7 doesn't support instruction access keys. */ >> + const u32 insn_keys = (proc_gen == proc_gen_p7) ? 0 : 32; > > theortically we should be checking for earlier gens too. right? > It should be > const u32 insn_keys = (proc_gen <= proc_gen_p7) ? 0 : 32; Theoretically yes, but if you look at the definition of the proc_gen enum: /* Processor generation */ enum proc_gen { proc_gen_unknown, proc_gen_p7, /* P7 and P7+ */ proc_gen_p8, proc_gen_p9, }; extern enum proc_gen proc_gen; In practice skiboot doesn't support anything older than a P7. So proc_gen <= proc_gen_p7 really means proc_gen == proc_gen_unknown, and storage_keys_fixup returns early in that case already.
Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes: > Ram Pai <linuxram@us.ibm.com> writes: >> On Fri, Sep 08, 2017 at 07:31:02PM -0300, Thiago Jung Bauermann wrote: >>> diff --git a/core/init.c b/core/init.c >>> index 8951e17b4c90..4a6f9155511d 100644 >>> --- a/core/init.c >>> +++ b/core/init.c >>> @@ -563,6 +563,22 @@ void __noreturn load_and_boot_kernel(bool is_reboot) >>> start_kernel(kernel_entry, fdt, mem_top); >>> } >>> >>> +static void storage_keys_fixup(void) >>> +{ >>> + /* P7 doesn't support instruction access keys. */ >>> + const u32 insn_keys = (proc_gen == proc_gen_p7) ? 0 : 32; >> >> theortically we should be checking for earlier gens too. right? >> It should be >> const u32 insn_keys = (proc_gen <= proc_gen_p7) ? 0 : 32; > > Theoretically yes, but if you look at the definition of the proc_gen enum: > > /* Processor generation */ > enum proc_gen { > proc_gen_unknown, > proc_gen_p7, /* P7 and P7+ */ > proc_gen_p8, > proc_gen_p9, > }; > extern enum proc_gen proc_gen; > > In practice skiboot doesn't support anything older than a P7. Even P7 is not "supported", just "it might work if you're lucky" type of supported. So arguably we shouldn't even advertise keys there, because no one is likely to test it. But it's probably fine to do so, and if someone ever tries it they can report bugs then. cheers
Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes: > LoPAPR says: > > “ibm,processor-storage-keys” > > property name indicating the number of virtual storage keys supported > by the processor described by this node. > > prop-encoded-array: Consists of two cells encoded as with encode-int. > The first cell represents the number of virtual storage keys supported > for data accesses while the second cell represents the number of > virtual storage keys supported for instruction accesses. The cell value > of zero indicates that no storage keys are supported for the access > type. > > pHyp provides the property above but there's a bug in P8 firmware where the > second cell is zero even though POWER8 supports instruction access keys. > This bug will be fixed for P9. > > While this is a PAPR property, it's useful to have it in powernv as well > so that Linux has a uniform way of checking for the feature regardless of > the platform it's running on. > > On PAPR there is one property for each CPU node, but since it's highly > unlikely that different CPUs will support a different number of keys, we > put the property in the /cpus node instead. > > Tested on QEMU POWER8 powernv model and Mambo P9. > > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> > --- > core/init.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > Changes in v2: > - Put property in /cpus instead of in each CPU node. Ping?
Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes: > Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes: > >> LoPAPR says: >> >> “ibm,processor-storage-keys” >> >> property name indicating the number of virtual storage keys supported >> by the processor described by this node. >> >> prop-encoded-array: Consists of two cells encoded as with encode-int. >> The first cell represents the number of virtual storage keys supported >> for data accesses while the second cell represents the number of >> virtual storage keys supported for instruction accesses. The cell value >> of zero indicates that no storage keys are supported for the access >> type. >> >> pHyp provides the property above but there's a bug in P8 firmware where the >> second cell is zero even though POWER8 supports instruction access keys. >> This bug will be fixed for P9. >> >> While this is a PAPR property, it's useful to have it in powernv as well >> so that Linux has a uniform way of checking for the feature regardless of >> the platform it's running on. >> >> On PAPR there is one property for each CPU node, but since it's highly >> unlikely that different CPUs will support a different number of keys, we >> put the property in the /cpus node instead. >> >> Tested on QEMU POWER8 powernv model and Mambo P9. >> >> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> >> --- >> core/init.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> Changes in v2: >> - Put property in /cpus instead of in each CPU node. > > Ping? Ping? This patch still applies cleanly and works. The Linux kernel patches using this property are upstream now (as of v4.16-rc1) and we are still interested in having it on OpenPower machines. The only open issue is where to put the property. Currently, the Linux code only looks for it in nodes with device_type = "cpu". This patch puts it in /cpus to avoid repeating the property in each CPU node, but Michael Ellerman suggested putting it in the ibm,opal node instead. What do you think? -- Thiago Jung Bauermann IBM Linux Technology Center
On Fri, Feb 23, 2018 at 06:57:52PM -0300, Thiago Jung Bauermann wrote: > > Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes: > > > Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes: > > > >> LoPAPR says: > >> > >> “ibm,processor-storage-keys” > >> > >> property name indicating the number of virtual storage keys supported > >> by the processor described by this node. > >> > >> prop-encoded-array: Consists of two cells encoded as with encode-int. > >> The first cell represents the number of virtual storage keys supported > >> for data accesses while the second cell represents the number of > >> virtual storage keys supported for instruction accesses. The cell value > >> of zero indicates that no storage keys are supported for the access > >> type. > >> > >> pHyp provides the property above but there's a bug in P8 firmware where the > >> second cell is zero even though POWER8 supports instruction access keys. > >> This bug will be fixed for P9. > >> > >> While this is a PAPR property, it's useful to have it in powernv as well > >> so that Linux has a uniform way of checking for the feature regardless of > >> the platform it's running on. > >> > >> On PAPR there is one property for each CPU node, but since it's highly > >> unlikely that different CPUs will support a different number of keys, we > >> put the property in the /cpus node instead. > >> > >> Tested on QEMU POWER8 powernv model and Mambo P9. > >> > >> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> > >> --- > >> core/init.c | 18 ++++++++++++++++++ > >> 1 file changed, 18 insertions(+) > >> > >> Changes in v2: > >> - Put property in /cpus instead of in each CPU node. > > > > Ping? > > Ping? This patch still applies cleanly and works. > > The Linux kernel patches using this property are upstream now (as of > v4.16-rc1) and we are still interested in having it on OpenPower > machines. > "ibm,processor-storage-keys" > The only open issue is where to put the property. Currently, the Linux > code only looks for it in nodes with device_type = "cpu". This patch > puts it in /cpus to avoid repeating the property in each CPU node, but > Michael Ellerman suggested putting it in the ibm,opal node instead. If it moves to any other place, the upstream kernel code will have to be modified. Will have to standardize this before 4.16 is released. My preference is to keep it in the same place as what phyp does. RP
Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes: > Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes: >> Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes: >> >>> LoPAPR says: >>> >>> “ibm,processor-storage-keys” >>> >>> property name indicating the number of virtual storage keys supported >>> by the processor described by this node. >>> >>> prop-encoded-array: Consists of two cells encoded as with encode-int. >>> The first cell represents the number of virtual storage keys supported >>> for data accesses while the second cell represents the number of >>> virtual storage keys supported for instruction accesses. The cell value >>> of zero indicates that no storage keys are supported for the access >>> type. >>> >>> pHyp provides the property above but there's a bug in P8 firmware where the >>> second cell is zero even though POWER8 supports instruction access keys. >>> This bug will be fixed for P9. >>> >>> While this is a PAPR property, it's useful to have it in powernv as well >>> so that Linux has a uniform way of checking for the feature regardless of >>> the platform it's running on. >>> >>> On PAPR there is one property for each CPU node, but since it's highly >>> unlikely that different CPUs will support a different number of keys, we >>> put the property in the /cpus node instead. >>> >>> Tested on QEMU POWER8 powernv model and Mambo P9. >>> >>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> >>> --- >>> core/init.c | 18 ++++++++++++++++++ >>> 1 file changed, 18 insertions(+) >>> >>> Changes in v2: >>> - Put property in /cpus instead of in each CPU node. >> >> Ping? > > Ping? This patch still applies cleanly and works. > > The Linux kernel patches using this property are upstream now (as of > v4.16-rc1) and we are still interested in having it on OpenPower > machines. > > The only open issue is where to put the property. Currently, the Linux > code only looks for it in nodes with device_type = "cpu". This patch > puts it in /cpus to avoid repeating the property in each CPU node, but > Michael Ellerman suggested putting it in the ibm,opal node instead. Hmm did I, probably :) It's annoying the way these properties get multiplied for each CPU, but for now we should probably just stick with tradition and put it in each CPU node. Sorry to change my mind on that. Are you able to respin it with that change? cheers
Ram Pai <linuxram@us.ibm.com> writes: > If it moves to any other place, the upstream kernel code will have to be > modified. Will have to standardize this before 4.16 is released. > > My preference is to keep it in the same place as what phyp does. Michael Ellerman <mpe@ellerman.id.au> writes: >> The only open issue is where to put the property. Currently, the Linux >> code only looks for it in nodes with device_type = "cpu". This patch >> puts it in /cpus to avoid repeating the property in each CPU node, but >> Michael Ellerman suggested putting it in the ibm,opal node instead. > > Hmm did I, probably :) > > It's annoying the way these properties get multiplied for each CPU, but > for now we should probably just stick with tradition and put it in each > CPU node. I agree that it's better to do what pHyp does here. > Sorry to change my mind on that. Are you able to respin it with that > change? No problem. And no need to respin either. v1 of this patch still applies cleanly and still works, as well.
diff --git a/core/init.c b/core/init.c index 8951e17b4c90..4a6f9155511d 100644 --- a/core/init.c +++ b/core/init.c @@ -563,6 +563,22 @@ void __noreturn load_and_boot_kernel(bool is_reboot) start_kernel(kernel_entry, fdt, mem_top); } +static void storage_keys_fixup(void) +{ + /* P7 doesn't support instruction access keys. */ + const u32 insn_keys = (proc_gen == proc_gen_p7) ? 0 : 32; + struct dt_node *cpus = dt_find_by_path(dt_root, "/cpus"); + + assert(cpus); + + if (proc_gen == proc_gen_unknown) + return; + else if (dt_has_node_property(cpus, "ibm,processor-storage-keys", NULL)) + return; + + dt_add_property_cells(cpus, "ibm,processor-storage-keys", 32, insn_keys); +} + static void dt_fixups(void) { struct dt_node *n; @@ -590,6 +606,8 @@ static void dt_fixups(void) if (!dt_has_node_property(n, "scom-controller", NULL)) dt_add_property(n, "scom-controller", NULL, 0); } + + storage_keys_fixup(); } static void add_arch_vector(void)
LoPAPR says: “ibm,processor-storage-keys” property name indicating the number of virtual storage keys supported by the processor described by this node. prop-encoded-array: Consists of two cells encoded as with encode-int. The first cell represents the number of virtual storage keys supported for data accesses while the second cell represents the number of virtual storage keys supported for instruction accesses. The cell value of zero indicates that no storage keys are supported for the access type. pHyp provides the property above but there's a bug in P8 firmware where the second cell is zero even though POWER8 supports instruction access keys. This bug will be fixed for P9. While this is a PAPR property, it's useful to have it in powernv as well so that Linux has a uniform way of checking for the feature regardless of the platform it's running on. On PAPR there is one property for each CPU node, but since it's highly unlikely that different CPUs will support a different number of keys, we put the property in the /cpus node instead. Tested on QEMU POWER8 powernv model and Mambo P9. Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> --- core/init.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) Changes in v2: - Put property in /cpus instead of in each CPU node.