Messages in this thread Patch in this message | | | From | Michael Ellerman <> | Subject | Re: [PATCH v2] powerpc/pseries: read the lpar name from the firmware | Date | Fri, 03 Dec 2021 23:28:10 +1100 |
| |
Hi Laurent,
Just a few minor comments below ...
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
| |