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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a1fb26c0-14a4-0d8f-6f31-37cca112df14@linux.ibm.com>
Date:   Fri, 3 Dec 2021 16:23:54 +0100
From:   Laurent Dufour <ldufour@...ux.ibm.com>
To:     Michael Ellerman <michaele@....ibm.com>,
        linuxppc-dev@...ts.ozlabs.org
Cc:     linux-kernel@...r.kernel.org, Nathan Lynch <nathanl@...ux.ibm.com>
Subject: Re: [PATCH v2] powerpc/pseries: read the lpar name from the firmware

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