[PATCH v4] powerpc/pseries: read the lpar name from the firmware

[PATCH v4] powerpc/pseries: read the lpar name from the firmware

[Laurent Dufour]

The LPAR name may be changed after the LPAR has been started in the HMC.
In that case lparstat command is not reporting the updated value because it
reads it from the device tree which is read at boot time.

However this value could be read from RTAS.

Adding this value in the /proc/powerpc/lparcfg output allows to read the
updated value.

Cc: Nathan Lynch <nathanl@linux.ibm.com>
Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
---
v4:
 address Nathan's new comments limiting size of the buffer.
v3:
 address Michael's comments.
v2:
 address Nathan's comments.
 change title to partition_name aligning with existing partition_id
---
 arch/powerpc/platforms/pseries/lparcfg.c | 54 ++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/lparcfg.c b/arch/powerpc/platforms/pseries/lparcfg.c
index f71eac74ea92..058d9a5fe545 100644
--- a/arch/powerpc/platforms/pseries/lparcfg.c
+++ b/arch/powerpc/platforms/pseries/lparcfg.c
@@ -311,6 +311,59 @@ static void parse_mpp_x_data(struct seq_file *m)
 		seq_printf(m, "coalesce_pool_spurr=%ld\n", mpp_x_data.pool_spurr_cycles);
 }
 
+/*
+ * PAPR defines, in section "7.3.16 System Parameters Option", the token 55 to
+ * read the LPAR name, and the largest output data to 4000 + 2 bytes length.
+ */
+#define SPLPAR_LPAR_NAME_TOKEN	55
+#define GET_SYS_PARM_BUF_SIZE	4002
+#if GET_SYS_PARM_BUF_SIZE > RTAS_DATA_BUF_SIZE
+#error "GET_SYS_PARM_BUF_SIZE is larger than RTAS_DATA_BUF_SIZE"
+#endif
+static void read_lpar_name(struct seq_file *m)
+{
+	int rc, len, token;
+	union {
+		char raw_buffer[GET_SYS_PARM_BUF_SIZE];
+		struct {
+			__be16 len;
+			char name[GET_SYS_PARM_BUF_SIZE-2];
+		};
+	} *local_buffer;
+
+	token = rtas_token("ibm,get-system-parameter");
+	if (token == RTAS_UNKNOWN_SERVICE)
+		return;
+
+	local_buffer = kmalloc(sizeof(*local_buffer), GFP_KERNEL);
+	if (!local_buffer)
+		return;
+
+	do {
+		spin_lock(&rtas_data_buf_lock);
+		memset(rtas_data_buf, 0, sizeof(*local_buffer));
+		rc = rtas_call(token, 3, 1, NULL, SPLPAR_LPAR_NAME_TOKEN,
+			       __pa(rtas_data_buf), sizeof(*local_buffer));
+		if (!rc)
+			memcpy(local_buffer->raw_buffer, rtas_data_buf,
+			       sizeof(local_buffer->raw_buffer));
+		spin_unlock(&rtas_data_buf_lock);
+	} while (rtas_busy_delay(rc));
+
+	if (!rc) {
+		/* Force end of string */
+		len = min((int) be16_to_cpu(local_buffer->len),
+			  (int) sizeof(local_buffer->name)-1);
+		local_buffer->name[len] = '\0';
+
+		seq_printf(m, "partition_name=%s\n", local_buffer->name);
+	} else
+		pr_err_once("Error calling get-system-parameter (0x%x)\n", rc);
+
+	kfree(local_buffer);
+}
+
+
 #define SPLPAR_CHARACTERISTICS_TOKEN 20
 #define SPLPAR_MAXLENGTH 1026*(sizeof(char))
 
@@ -496,6 +549,7 @@ static int pseries_lparcfg_data(struct seq_file *m, void *v)
 
 	if (firmware_has_feature(FW_FEATURE_SPLPAR)) {
 		/* this call handles the ibm,get-system-parameter contents */
+		read_lpar_name(m);
 		parse_system_parameter_string(m);
 		parse_ppp_data(m);
 		parse_mpp_data(m);
-- 
2.34.1

Re: [PATCH v4] powerpc/pseries: read the lpar name from the firmware

[Laurent Dufour]

Happy New Year, Michael!

Do you consider taking that patch soon?

Thanks,
Laurent.

On 07/12/2021, 18:11:09, Laurent Dufour wrote:
> The LPAR name may be changed after the LPAR has been started in the HMC.
> In that case lparstat command is not reporting the updated value because it
> reads it from the device tree which is read at boot time.
> 
> However this value could be read from RTAS.
> 
> Adding this value in the /proc/powerpc/lparcfg output allows to read the
> updated value.
> 
> Cc: Nathan Lynch <nathanl@linux.ibm.com>
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
> v4:
>  address Nathan's new comments limiting size of the buffer.
> v3:
>  address Michael's comments.
> v2:
>  address Nathan's comments.
>  change title to partition_name aligning with existing partition_id
> ---
>  arch/powerpc/platforms/pseries/lparcfg.c | 54 ++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/arch/powerpc/platforms/pseries/lparcfg.c b/arch/powerpc/platforms/pseries/lparcfg.c
> index f71eac74ea92..058d9a5fe545 100644
> --- a/arch/powerpc/platforms/pseries/lparcfg.c
> +++ b/arch/powerpc/platforms/pseries/lparcfg.c
> @@ -311,6 +311,59 @@ static void parse_mpp_x_data(struct seq_file *m)
>  		seq_printf(m, "coalesce_pool_spurr=%ld\n", mpp_x_data.pool_spurr_cycles);
>  }
>  
> +/*
> + * PAPR defines, in section "7.3.16 System Parameters Option", the token 55 to
> + * read the LPAR name, and the largest output data to 4000 + 2 bytes length.
> + */
> +#define SPLPAR_LPAR_NAME_TOKEN	55
> +#define GET_SYS_PARM_BUF_SIZE	4002
> +#if GET_SYS_PARM_BUF_SIZE > RTAS_DATA_BUF_SIZE
> +#error "GET_SYS_PARM_BUF_SIZE is larger than RTAS_DATA_BUF_SIZE"
> +#endif
> +static void read_lpar_name(struct seq_file *m)
> +{
> +	int rc, len, token;
> +	union {
> +		char raw_buffer[GET_SYS_PARM_BUF_SIZE];
> +		struct {
> +			__be16 len;
> +			char name[GET_SYS_PARM_BUF_SIZE-2];
> +		};
> +	} *local_buffer;
> +
> +	token = rtas_token("ibm,get-system-parameter");
> +	if (token == RTAS_UNKNOWN_SERVICE)
> +		return;
> +
> +	local_buffer = kmalloc(sizeof(*local_buffer), GFP_KERNEL);
> +	if (!local_buffer)
> +		return;
> +
> +	do {
> +		spin_lock(&rtas_data_buf_lock);
> +		memset(rtas_data_buf, 0, sizeof(*local_buffer));
> +		rc = rtas_call(token, 3, 1, NULL, SPLPAR_LPAR_NAME_TOKEN,
> +			       __pa(rtas_data_buf), sizeof(*local_buffer));
> +		if (!rc)
> +			memcpy(local_buffer->raw_buffer, rtas_data_buf,
> +			       sizeof(local_buffer->raw_buffer));
> +		spin_unlock(&rtas_data_buf_lock);
> +	} while (rtas_busy_delay(rc));
> +
> +	if (!rc) {
> +		/* Force end of string */
> +		len = min((int) be16_to_cpu(local_buffer->len),
> +			  (int) sizeof(local_buffer->name)-1);
> +		local_buffer->name[len] = '\0';
> +
> +		seq_printf(m, "partition_name=%s\n", local_buffer->name);
> +	} else
> +		pr_err_once("Error calling get-system-parameter (0x%x)\n", rc);
> +
> +	kfree(local_buffer);
> +}
> +
> +
>  #define SPLPAR_CHARACTERISTICS_TOKEN 20
>  #define SPLPAR_MAXLENGTH 1026*(sizeof(char))
>  
> @@ -496,6 +549,7 @@ static int pseries_lparcfg_data(struct seq_file *m, void *v)
>  
>  	if (firmware_has_feature(FW_FEATURE_SPLPAR)) {
>  		/* this call handles the ibm,get-system-parameter contents */
> +		read_lpar_name(m);
>  		parse_system_parameter_string(m);
>  		parse_ppp_data(m);
>  		parse_mpp_data(m);

Re: [PATCH v4] powerpc/pseries: read the lpar name from the firmware

[Nathan Lynch]

Laurent Dufour <ldufour@linux.ibm.com> writes:
> On 07/12/2021, 18:11:09, Laurent Dufour wrote:
>> The LPAR name may be changed after the LPAR has been started in the HMC.
>> In that case lparstat command is not reporting the updated value because it
>> reads it from the device tree which is read at boot time.
>> 
>> However this value could be read from RTAS.
>> 
>> Adding this value in the /proc/powerpc/lparcfg output allows to read the
>> updated value.
>
> Do you consider taking that patch soon?

This version prints an error on non-PowerVM guests the first time
lparcfg is read.

And I still contend that having this function fall back to reporting the
partition name in the DT would provide a beneficial consistency in the
user-facing API, allowing programs to avoid hypervisor-specific branches
in their code. I don't understand the resistance I've encountered here.
The fallback I'm suggesting (a root node property lookup) is certainly
not more complex than the RTAS call sequence you've already implemented.

Re: [PATCH v4] powerpc/pseries: read the lpar name from the firmware

[Tyrel Datwyler]

On 1/5/22 3:19 PM, Nathan Lynch wrote:
> Laurent Dufour <ldufour@linux.ibm.com> writes:
>> On 07/12/2021, 18:11:09, Laurent Dufour wrote:
>>> The LPAR name may be changed after the LPAR has been started in the HMC.
>>> In that case lparstat command is not reporting the updated value because it
>>> reads it from the device tree which is read at boot time.
>>>
>>> However this value could be read from RTAS.
>>>
>>> Adding this value in the /proc/powerpc/lparcfg output allows to read the
>>> updated value.
>>
>> Do you consider taking that patch soon?
> 
> This version prints an error on non-PowerVM guests the first time
> lparcfg is read.

I assume because QEMU doesn't implement the LPAR_NAME token for get_sysparm.

> 
> And I still contend that having this function fall back to reporting the
> partition name in the DT would provide a beneficial consistency in the
> user-facing API, allowing programs to avoid hypervisor-specific branches
> in their code. 

Agreed, if the get_sysparm fails just report the lpar-name from the device tree.

I don't understand the resistance I've encountered here.
> The fallback I'm suggesting (a root node property lookup) is certainly
> not more complex than the RTAS call sequence you've already implemented.
> 

Is there benefit of adding a partition_name field/value pair to lparcfg? The
lparstat utility can just as easily make the get_sysparm call via librtas.
Further, rtas_filters allows this particular RTAS call from userspace.

-Tyrel

Re: [PATCH v4] powerpc/pseries: read the lpar name from the firmware

[Nathan Lynch]

Tyrel Datwyler <tyreld@linux.ibm.com> writes:
> On 1/5/22 3:19 PM, Nathan Lynch wrote:
>> Laurent Dufour <ldufour@linux.ibm.com> writes:
>>> On 07/12/2021, 18:11:09, Laurent Dufour wrote:
>>>> The LPAR name may be changed after the LPAR has been started in the HMC.
>>>> In that case lparstat command is not reporting the updated value because it
>>>> reads it from the device tree which is read at boot time.
>>>>
>>>> However this value could be read from RTAS.
>>>>
>>>> Adding this value in the /proc/powerpc/lparcfg output allows to read the
>>>> updated value.
>>>
>>> Do you consider taking that patch soon?
>> 
>> This version prints an error on non-PowerVM guests the first time
>> lparcfg is read.
>
> I assume because QEMU doesn't implement the LPAR_NAME token for
> get_sysparm.

Correct.


>> And I still contend that having this function fall back to reporting the
>> partition name in the DT would provide a beneficial consistency in the
>> user-facing API, allowing programs to avoid hypervisor-specific branches
>> in their code. 
>
> Agreed, if the get_sysparm fails just report the lpar-name from the device tree.
>
>> I don't understand the resistance I've encountered here.
>> The fallback I'm suggesting (a root node property lookup) is certainly
>> not more complex than the RTAS call sequence you've already implemented.
>> 
>
> Is there benefit of adding a partition_name field/value pair to lparcfg? The
> lparstat utility can just as easily make the get_sysparm call via librtas.
> Further, rtas_filters allows this particular RTAS call from userspace.

The RTAS syscall is root-only, but we want the partition name (whether
supplied by RTAS or the device tree) to be available to unprivileged
programs.

Re: [PATCH v4] powerpc/pseries: read the lpar name from the firmware

[Tyrel Datwyler]

On 1/5/22 5:36 PM, Nathan Lynch wrote:
> Tyrel Datwyler <tyreld@linux.ibm.com> writes:
>> On 1/5/22 3:19 PM, Nathan Lynch wrote:
>>>
>>
>> Is there benefit of adding a partition_name field/value pair to lparcfg? The
>> lparstat utility can just as easily make the get_sysparm call via librtas.
>> Further, rtas_filters allows this particular RTAS call from userspace.
> 
> The RTAS syscall is root-only, but we want the partition name (whether
> supplied by RTAS or the device tree) to be available to unprivileged
> programs.
> 

Ah, right. I recall this discussion now from previous iterations.

-Tyrel

Re: [PATCH v4] powerpc/pseries: read the lpar name from the firmware

[Laurent Dufour]

On 06/01/2022, 02:17:21, Tyrel Datwyler wrote:
> On 1/5/22 3:19 PM, Nathan Lynch wrote:
>> Laurent Dufour <ldufour@linux.ibm.com> writes:
>>> On 07/12/2021, 18:11:09, Laurent Dufour wrote:
>>>> The LPAR name may be changed after the LPAR has been started in the HMC.
>>>> In that case lparstat command is not reporting the updated value because it
>>>> reads it from the device tree which is read at boot time.
>>>>
>>>> However this value could be read from RTAS.
>>>>
>>>> Adding this value in the /proc/powerpc/lparcfg output allows to read the
>>>> updated value.
>>>
>>> Do you consider taking that patch soon?
>>
>> This version prints an error on non-PowerVM guests the first time
>> lparcfg is read.
> 
> I assume because QEMU doesn't implement the LPAR_NAME token for get_sysparm.
> 
>>
>> And I still contend that having this function fall back to reporting the
>> partition name in the DT would provide a beneficial consistency in the
>> user-facing API, allowing programs to avoid hypervisor-specific branches
>> in their code. 
> 
> Agreed, if the get_sysparm fails just report the lpar-name from the device tree.

My aim is to not do in the kernel what can be easily done in user space but
avoiding user space program hypervisor-specific branches is a good point.

Note that if the RTAS call has been available to unprivileged user, all
that stuff would have been made in user space, so hypervisor-specific...

Anyway, I'll work on a new version fetching the DT value in the case the
RTAS call is failing.

Thanks,
Laurent.

Re: [PATCH v4] powerpc/pseries: read the lpar name from the firmware

[Michael Ellerman]

Laurent Dufour <ldufour@linux.ibm.com> writes:
> Happy New Year, Michael!
>
> Do you consider taking that patch soon?

I did but I was hoping you and Nathan could come to an agreement.

Looks like you did while I was sleeping, perfect :)

I'll pick up v5.

cheers


> On 07/12/2021, 18:11:09, Laurent Dufour wrote:
>> The LPAR name may be changed after the LPAR has been started in the HMC.
>> In that case lparstat command is not reporting the updated value because it
>> reads it from the device tree which is read at boot time.
>> 
>> However this value could be read from RTAS.
>> 
>> Adding this value in the /proc/powerpc/lparcfg output allows to read the
>> updated value.
>> 
>> Cc: Nathan Lynch <nathanl@linux.ibm.com>
>> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
>> ---
>> v4:
>>  address Nathan's new comments limiting size of the buffer.
>> v3:
>>  address Michael's comments.
>> v2:
>>  address Nathan's comments.
>>  change title to partition_name aligning with existing partition_id
>> ---
>>  arch/powerpc/platforms/pseries/lparcfg.c | 54 ++++++++++++++++++++++++
>>  1 file changed, 54 insertions(+)
>> 
>> diff --git a/arch/powerpc/platforms/pseries/lparcfg.c b/arch/powerpc/platforms/pseries/lparcfg.c
>> index f71eac74ea92..058d9a5fe545 100644
>> --- a/arch/powerpc/platforms/pseries/lparcfg.c
>> +++ b/arch/powerpc/platforms/pseries/lparcfg.c
>> @@ -311,6 +311,59 @@ static void parse_mpp_x_data(struct seq_file *m)
>>  		seq_printf(m, "coalesce_pool_spurr=%ld\n", mpp_x_data.pool_spurr_cycles);
>>  }
>>  
>> +/*
>> + * PAPR defines, in section "7.3.16 System Parameters Option", the token 55 to
>> + * read the LPAR name, and the largest output data to 4000 + 2 bytes length.
>> + */
>> +#define SPLPAR_LPAR_NAME_TOKEN	55
>> +#define GET_SYS_PARM_BUF_SIZE	4002
>> +#if GET_SYS_PARM_BUF_SIZE > RTAS_DATA_BUF_SIZE
>> +#error "GET_SYS_PARM_BUF_SIZE is larger than RTAS_DATA_BUF_SIZE"
>> +#endif
>> +static void read_lpar_name(struct seq_file *m)
>> +{
>> +	int rc, len, token;
>> +	union {
>> +		char raw_buffer[GET_SYS_PARM_BUF_SIZE];
>> +		struct {
>> +			__be16 len;
>> +			char name[GET_SYS_PARM_BUF_SIZE-2];
>> +		};
>> +	} *local_buffer;
>> +
>> +	token = rtas_token("ibm,get-system-parameter");
>> +	if (token == RTAS_UNKNOWN_SERVICE)
>> +		return;
>> +
>> +	local_buffer = kmalloc(sizeof(*local_buffer), GFP_KERNEL);
>> +	if (!local_buffer)
>> +		return;
>> +
>> +	do {
>> +		spin_lock(&rtas_data_buf_lock);
>> +		memset(rtas_data_buf, 0, sizeof(*local_buffer));
>> +		rc = rtas_call(token, 3, 1, NULL, SPLPAR_LPAR_NAME_TOKEN,
>> +			       __pa(rtas_data_buf), sizeof(*local_buffer));
>> +		if (!rc)
>> +			memcpy(local_buffer->raw_buffer, rtas_data_buf,
>> +			       sizeof(local_buffer->raw_buffer));
>> +		spin_unlock(&rtas_data_buf_lock);
>> +	} while (rtas_busy_delay(rc));
>> +
>> +	if (!rc) {
>> +		/* Force end of string */
>> +		len = min((int) be16_to_cpu(local_buffer->len),
>> +			  (int) sizeof(local_buffer->name)-1);
>> +		local_buffer->name[len] = '\0';
>> +
>> +		seq_printf(m, "partition_name=%s\n", local_buffer->name);
>> +	} else
>> +		pr_err_once("Error calling get-system-parameter (0x%x)\n", rc);
>> +
>> +	kfree(local_buffer);
>> +}
>> +
>> +
>>  #define SPLPAR_CHARACTERISTICS_TOKEN 20
>>  #define SPLPAR_MAXLENGTH 1026*(sizeof(char))
>>  
>> @@ -496,6 +549,7 @@ static int pseries_lparcfg_data(struct seq_file *m, void *v)
>>  
>>  	if (firmware_has_feature(FW_FEATURE_SPLPAR)) {
>>  		/* this call handles the ibm,get-system-parameter contents */
>> +		read_lpar_name(m);
>>  		parse_system_parameter_string(m);
>>  		parse_ppp_data(m);
>>  		parse_mpp_data(m);