Message-ID: <a1fb26c0-14a4-0d8f-6f31-37cca112df14@linux.ibm.com>
Date:   Fri, 3 Dec 2021 16:23:54 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.3.2
Subject: Re: [PATCH v2] powerpc/pseries: read the lpar name from the firmware
Content-Language: en-US
To:     Michael Ellerman <michaele@au1.ibm.com>,
        linuxppc-dev@lists.ozlabs.org
Cc:     linux-kernel@vger.kernel.org, Nathan Lynch <nathanl@linux.ibm.com>
References: <20211201190642.49897-1-ldufour@linux.ibm.com>
 <87y251ooth.fsf@mpe.ellerman.id.au>
From:   Laurent Dufour <ldufour@linux.ibm.com>
In-Reply-To: <87y251ooth.fsf@mpe.ellerman.id.au>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@linux.ibm.com> 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@linux.ibm.com>
>> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
>> ---
>> 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
>