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

[Laurent Dufour]

On 03/12/2021, 13:28:10, Michael Ellerman wrote:
> Hi Laurent,
> 
> Just a few minor comments below ...

Thanks, Michael, for your _major_ comments.

I'll send a version 3 soon.

Laurent.

> 
> Laurent Dufour <ldufour@xxxxxxxxxxxxx> writes:
>> 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@xxxxxxxxxxxxx>
>> Signed-off-by: Laurent Dufour <ldufour@xxxxxxxxxxxxx>
>> ---
>> v2:
>>  address Nathan's comments.
>>  change title to partition_name aligning with existing partition_id
>> ---
>>  arch/powerpc/platforms/pseries/lparcfg.c | 53 ++++++++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/pseries/lparcfg.c b/arch/powerpc/platforms/pseries/lparcfg.c
>> index f71eac74ea92..0deca7b4cd81 100644
>> --- a/arch/powerpc/platforms/pseries/lparcfg.c
>> +++ b/arch/powerpc/platforms/pseries/lparcfg.c
>> @@ -311,6 +311,58 @@ 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 no maximum for the LPAR name, and defines that the maximum
>> + * length of the get-system-parameter's output buffer is 4000 plus 2 bytes for
>> + * the length. Limit LPAR's name size to 1024
>> + */
>> +#define SPLPAR_LPAR_NAME_MAXLEN	1026
> 
> Why not just allocate the full 4000? That's not much memory in the
> scheme of things, and allocating less risks us needing to update this in
> future. I realise that's a ridiculously long LPAR name, but still.
> 
>> +#define SPLPAR_LPAR_NAME_TOKEN	55
> 
> Can you add a comment saying where that value is defined, PAPR?
> 
>> +static void parse_lpar_name(struct seq_file *m)
> 
> Nitpick, but we're not really parsing it, we're just reading it from firmware.
> 
>> +{
>> +	int rc, len, token;
>> +	unsigned char *local_buffer;
>> +
>> +	token = rtas_token("ibm,get-system-parameter");
>> +	if (token == RTAS_UNKNOWN_SERVICE)
>> +		return;
>> +
>> +	local_buffer = kmalloc(SPLPAR_LPAR_NAME_MAXLEN, GFP_KERNEL);
>> +	if (!local_buffer)
>> +		return;
>> +
>> +	do {
>> +		spin_lock(&rtas_data_buf_lock);
>> +
>> +		memset(rtas_data_buf, 0, RTAS_DATA_BUF_SIZE);
>> +		rc = rtas_call(token, 3, 1, NULL, SPLPAR_LPAR_NAME_TOKEN,
>> +			       __pa(rtas_data_buf), RTAS_DATA_BUF_SIZE);
>> +		memcpy(local_buffer, rtas_data_buf, SPLPAR_LPAR_NAME_MAXLEN);
> 
> Would be nice to skip the memcpy() if rc is non-zero.
> 
>> +		spin_unlock(&rtas_data_buf_lock);
>> +	} while (rtas_busy_delay(rc));
>> +
>> +	if (rc != 0) {
>> +		pr_err_once(
>> +			"%s %s Error calling get-system-parameter (0x%x)\n",
>> +			__FILE__, __func__, rc);
>> +	} else {
>> +		local_buffer[SPLPAR_LPAR_NAME_MAXLEN - 1] = '\0';
>> +		len = local_buffer[0] * 256 + local_buffer[1];
> 
> This is kind of a hand rolled be16_to_cpu().
> 
> And we also have the + 2 to skip the length in several places.
> 
> I feel like the code would be cleaner if we had a type for the result
> buffer, eg something like:
> 
> union {
> 	char raw_buffer[1026];
>         struct {
>         	__be16 len;
>                 char data[1024];
>         };
> };
> 
>> +		/*
>> +		 * Forces an empty string in the weird case fw reports no data.
>> +		 */
>> +		if (!len)
>> +			local_buffer[2] = '\0';
>> +
>> +		seq_printf(m, "partition_name=%s\n", local_buffer + 2);
> 
> Then above you could use the data field rather than having to add the
> "2" in both places.
> 
>> +	}
>> +
>> +	kfree(local_buffer);
>> +}
>> +
>> +
>>  #define SPLPAR_CHARACTERISTICS_TOKEN 20
>>  #define SPLPAR_MAXLENGTH 1026*(sizeof(char))
>>  
>> @@ -496,6 +548,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 */
>> +		parse_lpar_name(m);
>>  		parse_system_parameter_string(m);
>>  		parse_ppp_data(m);
>>  		parse_mpp_data(m);
>> -- 
>> 2.34.1
> 
> 
> cheers
>