core/init: Add ibm, processor-storage-keys property to CPU DT node.

Message ID 20170816234914.10494-1-bauerman@linux.vnet.ibm.com
State New
Headers show

Commit Message

Thiago Jung Bauermann Aug. 16, 2017, 11:49 p.m.
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(+)

Comments

Vasant Hegde Aug. 17, 2017, 4:33 a.m. | #1
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
Michael Ellerman Aug. 17, 2017, 9:50 a.m. | #2
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
Thiago Jung Bauermann Aug. 17, 2017, 3:44 p.m. | #3
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.
Thiago Jung Bauermann Sept. 4, 2017, 6:51 p.m. | #4
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.

Patch

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)