Messages in this thread | | | Date | Fri, 3 Dec 2021 16:23:54 +0100 | Subject | Re: [PATCH v2] powerpc/pseries: read the lpar name from the firmware | From | 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@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 >
| |