[v2] core/init: Add ibm, processor-storage-keys property to /cpus DT node.

Message ID 20170908223102.20470-1-bauerman@linux.vnet.ibm.com
State New
Headers show
Series
  • [v2] core/init: Add ibm, processor-storage-keys property to /cpus DT node.
Related show

Commit Message

Thiago Jung Bauermann Sept. 8, 2017, 10:31 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.

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.

Comments

Ram Pai Sept. 9, 2017, 12:11 a.m. | #1
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)
Thiago Jung Bauermann Sept. 9, 2017, 12:59 a.m. | #2
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.
Michael Ellerman Sept. 19, 2017, 9:30 a.m. | #3
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 Oct. 18, 2017, 2:09 a.m. | #4
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?

Patch

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)