| Message ID | 20170816234914.10494-1-bauerman@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
On 08/17/2017 05:19 AM, Thiago Jung Bauermann wrote: > LoPAPR says: > .../... > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> > --- > > This is used by patch 25 of Ram Pai's series which makes Linux support > storage keys: > > https://lists.ozlabs.org/pipermail/linuxppc-dev/2017-July/161230.html > > I will submit a patch for KVM so that it can pass the property along > to its guests in the next few days. > > core/init.c | 25 +++++++++++++++++++++++++ Thiago, Is there a specific reason to add this code under init.c? I think its better to add this code where we create /cpus node. (hdata/cpu-common.c). -Vasant
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. True. Though having it in every CPU node is kind of annoying. There's no way we're ever going to support a different number of keys on different CPUs in the system. We could put it under the /ibm,opal node, that would require checking there in the Linux code, but it should be ~= 2 extra lines so not a big deal. I dunno, maybe we should follow PAPR exactly, but it just seems silly to copy the property for every CPU in the system. Does skiboot want to reserve some keys for itself ? cheers
Vasant Hegde <hegdevasant@linux.vnet.ibm.com> writes: > On 08/17/2017 05:19 AM, Thiago Jung Bauermann wrote: >> LoPAPR says: >> > > .../... > >> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> >> --- >> >> This is used by patch 25 of Ram Pai's series which makes Linux support >> storage keys: >> >> https://lists.ozlabs.org/pipermail/linuxppc-dev/2017-July/161230.html >> >> I will submit a patch for KVM so that it can pass the property along >> to its guests in the next few days. >> >> core/init.c | 25 +++++++++++++++++++++++++ > > Thiago, > > Is there a specific reason to add this code under init.c? > > I think its better to add this code where we create /cpus node. > (hdata/cpu-common.c). That's where I put it at first, but then it only works on platforms where the device tree is contructed from hdata. It doesn't work on Mambo nor QEMU, for instance (I didn't test this option on the Firenze machine). Where it is now, it will work regardless where the device tree comes from.
Michael Ellerman <mpe@ellerman.id.au> 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. > > True. > > Though having it in every CPU node is kind of annoying. There's no way > we're ever going to support a different number of keys on different CPUs > in the system. > > We could put it under the /ibm,opal node, that would require checking > there in the Linux code, but it should be ~= 2 extra lines so not a big > deal. > > I dunno, maybe we should follow PAPR exactly, but it just seems silly to > copy the property for every CPU in the system. I don't mind putting it in /ibm,opal if that's what people prefer. I can also put it in /cpus. This way PowerVM could also adopt it if they find the pointless repetition annoying too. > Does skiboot want to reserve some keys for itself ? I don't know enough about skiboot to know how it could be useful, was hoping someone else would be able to answer it. But it doesn't have to be decided now since skiboot can just decrement the number of keys advertised in the property and both the host and guest OSes will know not to use the reserved keys.
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. > > Tested on QEMU POWER8 powernv model, Mambo P9 and POWER8 Firenze machine. > > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> 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.
diff --git a/core/init.c b/core/init.c index 61b531c77e39..405aa01af9d9 100644 --- a/core/init.c +++ b/core/init.c @@ -560,6 +560,29 @@ 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, *n; + + cpus = dt_find_by_path(dt_root, "/cpus"); + assert(cpus); + + if (proc_gen == proc_gen_unknown) + return; + + dt_for_each_child(cpus, n) { + /* There may be cache nodes in /cpus. */ + if (!dt_has_node_property(n, "device_type", "cpu") || + dt_has_node_property(n, "ibm,processor-storage-keys", NULL)) + continue; + + dt_add_property_cells(n, "ibm,processor-storage-keys", 32, + insn_keys); + } +} + static void dt_fixups(void) { struct dt_node *n; @@ -587,6 +610,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. Tested on QEMU POWER8 powernv model, Mambo P9 and POWER8 Firenze machine. Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> --- This is used by patch 25 of Ram Pai's series which makes Linux support storage keys: https://lists.ozlabs.org/pipermail/linuxppc-dev/2017-July/161230.html I will submit a patch for KVM so that it can pass the property along to its guests in the next few days. core/init.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)