| Message ID | 20211207171109.22793-1-ldufour@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] powerpc/pseries: read the lpar name from the firmware | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
Happy New Year, Michael! Do you consider taking that patch soon? Thanks, Laurent. On 07/12/2021, 18:11:09, Laurent Dufour wrote: > 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> > --- > v4: > address Nathan's new comments limiting size of the buffer. > v3: > address Michael's comments. > v2: > address Nathan's comments. > change title to partition_name aligning with existing partition_id > --- > arch/powerpc/platforms/pseries/lparcfg.c | 54 ++++++++++++++++++++++++ > 1 file changed, 54 insertions(+) > > diff --git a/arch/powerpc/platforms/pseries/lparcfg.c b/arch/powerpc/platforms/pseries/lparcfg.c > index f71eac74ea92..058d9a5fe545 100644 > --- a/arch/powerpc/platforms/pseries/lparcfg.c > +++ b/arch/powerpc/platforms/pseries/lparcfg.c > @@ -311,6 +311,59 @@ 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, in section "7.3.16 System Parameters Option", the token 55 to > + * read the LPAR name, and the largest output data to 4000 + 2 bytes length. > + */ > +#define SPLPAR_LPAR_NAME_TOKEN 55 > +#define GET_SYS_PARM_BUF_SIZE 4002 > +#if GET_SYS_PARM_BUF_SIZE > RTAS_DATA_BUF_SIZE > +#error "GET_SYS_PARM_BUF_SIZE is larger than RTAS_DATA_BUF_SIZE" > +#endif > +static void read_lpar_name(struct seq_file *m) > +{ > + int rc, len, token; > + union { > + char raw_buffer[GET_SYS_PARM_BUF_SIZE]; > + struct { > + __be16 len; > + char name[GET_SYS_PARM_BUF_SIZE-2]; > + }; > + } *local_buffer; > + > + token = rtas_token("ibm,get-system-parameter"); > + if (token == RTAS_UNKNOWN_SERVICE) > + return; > + > + local_buffer = kmalloc(sizeof(*local_buffer), GFP_KERNEL); > + if (!local_buffer) > + return; > + > + do { > + spin_lock(&rtas_data_buf_lock); > + memset(rtas_data_buf, 0, sizeof(*local_buffer)); > + rc = rtas_call(token, 3, 1, NULL, SPLPAR_LPAR_NAME_TOKEN, > + __pa(rtas_data_buf), sizeof(*local_buffer)); > + if (!rc) > + memcpy(local_buffer->raw_buffer, rtas_data_buf, > + sizeof(local_buffer->raw_buffer)); > + spin_unlock(&rtas_data_buf_lock); > + } while (rtas_busy_delay(rc)); > + > + if (!rc) { > + /* Force end of string */ > + len = min((int) be16_to_cpu(local_buffer->len), > + (int) sizeof(local_buffer->name)-1); > + local_buffer->name[len] = '\0'; > + > + seq_printf(m, "partition_name=%s\n", local_buffer->name); > + } else > + pr_err_once("Error calling get-system-parameter (0x%x)\n", rc); > + > + kfree(local_buffer); > +} > + > + > #define SPLPAR_CHARACTERISTICS_TOKEN 20 > #define SPLPAR_MAXLENGTH 1026*(sizeof(char)) > > @@ -496,6 +549,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 */ > + read_lpar_name(m); > parse_system_parameter_string(m); > parse_ppp_data(m); > parse_mpp_data(m);
Laurent Dufour <ldufour@linux.ibm.com> writes: > On 07/12/2021, 18:11:09, Laurent Dufour wrote: >> 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. > > Do you consider taking that patch soon? This version prints an error on non-PowerVM guests the first time lparcfg is read. And I still contend that having this function fall back to reporting the partition name in the DT would provide a beneficial consistency in the user-facing API, allowing programs to avoid hypervisor-specific branches in their code. I don't understand the resistance I've encountered here. The fallback I'm suggesting (a root node property lookup) is certainly not more complex than the RTAS call sequence you've already implemented.
On 1/5/22 3:19 PM, Nathan Lynch wrote: > Laurent Dufour <ldufour@linux.ibm.com> writes: >> On 07/12/2021, 18:11:09, Laurent Dufour wrote: >>> 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. >> >> Do you consider taking that patch soon? > > This version prints an error on non-PowerVM guests the first time > lparcfg is read. I assume because QEMU doesn't implement the LPAR_NAME token for get_sysparm. > > And I still contend that having this function fall back to reporting the > partition name in the DT would provide a beneficial consistency in the > user-facing API, allowing programs to avoid hypervisor-specific branches > in their code. Agreed, if the get_sysparm fails just report the lpar-name from the device tree. I don't understand the resistance I've encountered here. > The fallback I'm suggesting (a root node property lookup) is certainly > not more complex than the RTAS call sequence you've already implemented. > Is there benefit of adding a partition_name field/value pair to lparcfg? The lparstat utility can just as easily make the get_sysparm call via librtas. Further, rtas_filters allows this particular RTAS call from userspace. -Tyrel
Tyrel Datwyler <tyreld@linux.ibm.com> writes: > On 1/5/22 3:19 PM, Nathan Lynch wrote: >> Laurent Dufour <ldufour@linux.ibm.com> writes: >>> On 07/12/2021, 18:11:09, Laurent Dufour wrote: >>>> 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. >>> >>> Do you consider taking that patch soon? >> >> This version prints an error on non-PowerVM guests the first time >> lparcfg is read. > > I assume because QEMU doesn't implement the LPAR_NAME token for > get_sysparm. Correct. >> And I still contend that having this function fall back to reporting the >> partition name in the DT would provide a beneficial consistency in the >> user-facing API, allowing programs to avoid hypervisor-specific branches >> in their code. > > Agreed, if the get_sysparm fails just report the lpar-name from the device tree. > >> I don't understand the resistance I've encountered here. >> The fallback I'm suggesting (a root node property lookup) is certainly >> not more complex than the RTAS call sequence you've already implemented. >> > > Is there benefit of adding a partition_name field/value pair to lparcfg? The > lparstat utility can just as easily make the get_sysparm call via librtas. > Further, rtas_filters allows this particular RTAS call from userspace. The RTAS syscall is root-only, but we want the partition name (whether supplied by RTAS or the device tree) to be available to unprivileged programs.
On 1/5/22 5:36 PM, Nathan Lynch wrote: > Tyrel Datwyler <tyreld@linux.ibm.com> writes: >> On 1/5/22 3:19 PM, Nathan Lynch wrote: >>> >> >> Is there benefit of adding a partition_name field/value pair to lparcfg? The >> lparstat utility can just as easily make the get_sysparm call via librtas. >> Further, rtas_filters allows this particular RTAS call from userspace. > > The RTAS syscall is root-only, but we want the partition name (whether > supplied by RTAS or the device tree) to be available to unprivileged > programs. > Ah, right. I recall this discussion now from previous iterations. -Tyrel
On 06/01/2022, 02:17:21, Tyrel Datwyler wrote: > On 1/5/22 3:19 PM, Nathan Lynch wrote: >> Laurent Dufour <ldufour@linux.ibm.com> writes: >>> On 07/12/2021, 18:11:09, Laurent Dufour wrote: >>>> 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. >>> >>> Do you consider taking that patch soon? >> >> This version prints an error on non-PowerVM guests the first time >> lparcfg is read. > > I assume because QEMU doesn't implement the LPAR_NAME token for get_sysparm. > >> >> And I still contend that having this function fall back to reporting the >> partition name in the DT would provide a beneficial consistency in the >> user-facing API, allowing programs to avoid hypervisor-specific branches >> in their code. > > Agreed, if the get_sysparm fails just report the lpar-name from the device tree. My aim is to not do in the kernel what can be easily done in user space but avoiding user space program hypervisor-specific branches is a good point. Note that if the RTAS call has been available to unprivileged user, all that stuff would have been made in user space, so hypervisor-specific... Anyway, I'll work on a new version fetching the DT value in the case the RTAS call is failing. Thanks, Laurent.
Laurent Dufour <ldufour@linux.ibm.com> writes: > Happy New Year, Michael! > > Do you consider taking that patch soon? I did but I was hoping you and Nathan could come to an agreement. Looks like you did while I was sleeping, perfect :) I'll pick up v5. cheers > On 07/12/2021, 18:11:09, Laurent Dufour wrote: >> 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> >> --- >> v4: >> address Nathan's new comments limiting size of the buffer. >> v3: >> address Michael's comments. >> v2: >> address Nathan's comments. >> change title to partition_name aligning with existing partition_id >> --- >> arch/powerpc/platforms/pseries/lparcfg.c | 54 ++++++++++++++++++++++++ >> 1 file changed, 54 insertions(+) >> >> diff --git a/arch/powerpc/platforms/pseries/lparcfg.c b/arch/powerpc/platforms/pseries/lparcfg.c >> index f71eac74ea92..058d9a5fe545 100644 >> --- a/arch/powerpc/platforms/pseries/lparcfg.c >> +++ b/arch/powerpc/platforms/pseries/lparcfg.c >> @@ -311,6 +311,59 @@ 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, in section "7.3.16 System Parameters Option", the token 55 to >> + * read the LPAR name, and the largest output data to 4000 + 2 bytes length. >> + */ >> +#define SPLPAR_LPAR_NAME_TOKEN 55 >> +#define GET_SYS_PARM_BUF_SIZE 4002 >> +#if GET_SYS_PARM_BUF_SIZE > RTAS_DATA_BUF_SIZE >> +#error "GET_SYS_PARM_BUF_SIZE is larger than RTAS_DATA_BUF_SIZE" >> +#endif >> +static void read_lpar_name(struct seq_file *m) >> +{ >> + int rc, len, token; >> + union { >> + char raw_buffer[GET_SYS_PARM_BUF_SIZE]; >> + struct { >> + __be16 len; >> + char name[GET_SYS_PARM_BUF_SIZE-2]; >> + }; >> + } *local_buffer; >> + >> + token = rtas_token("ibm,get-system-parameter"); >> + if (token == RTAS_UNKNOWN_SERVICE) >> + return; >> + >> + local_buffer = kmalloc(sizeof(*local_buffer), GFP_KERNEL); >> + if (!local_buffer) >> + return; >> + >> + do { >> + spin_lock(&rtas_data_buf_lock); >> + memset(rtas_data_buf, 0, sizeof(*local_buffer)); >> + rc = rtas_call(token, 3, 1, NULL, SPLPAR_LPAR_NAME_TOKEN, >> + __pa(rtas_data_buf), sizeof(*local_buffer)); >> + if (!rc) >> + memcpy(local_buffer->raw_buffer, rtas_data_buf, >> + sizeof(local_buffer->raw_buffer)); >> + spin_unlock(&rtas_data_buf_lock); >> + } while (rtas_busy_delay(rc)); >> + >> + if (!rc) { >> + /* Force end of string */ >> + len = min((int) be16_to_cpu(local_buffer->len), >> + (int) sizeof(local_buffer->name)-1); >> + local_buffer->name[len] = '\0'; >> + >> + seq_printf(m, "partition_name=%s\n", local_buffer->name); >> + } else >> + pr_err_once("Error calling get-system-parameter (0x%x)\n", rc); >> + >> + kfree(local_buffer); >> +} >> + >> + >> #define SPLPAR_CHARACTERISTICS_TOKEN 20 >> #define SPLPAR_MAXLENGTH 1026*(sizeof(char)) >> >> @@ -496,6 +549,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 */ >> + read_lpar_name(m); >> parse_system_parameter_string(m); >> parse_ppp_data(m); >> parse_mpp_data(m);
diff --git a/arch/powerpc/platforms/pseries/lparcfg.c b/arch/powerpc/platforms/pseries/lparcfg.c index f71eac74ea92..058d9a5fe545 100644 --- a/arch/powerpc/platforms/pseries/lparcfg.c +++ b/arch/powerpc/platforms/pseries/lparcfg.c @@ -311,6 +311,59 @@ 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, in section "7.3.16 System Parameters Option", the token 55 to + * read the LPAR name, and the largest output data to 4000 + 2 bytes length. + */ +#define SPLPAR_LPAR_NAME_TOKEN 55 +#define GET_SYS_PARM_BUF_SIZE 4002 +#if GET_SYS_PARM_BUF_SIZE > RTAS_DATA_BUF_SIZE +#error "GET_SYS_PARM_BUF_SIZE is larger than RTAS_DATA_BUF_SIZE" +#endif +static void read_lpar_name(struct seq_file *m) +{ + int rc, len, token; + union { + char raw_buffer[GET_SYS_PARM_BUF_SIZE]; + struct { + __be16 len; + char name[GET_SYS_PARM_BUF_SIZE-2]; + }; + } *local_buffer; + + token = rtas_token("ibm,get-system-parameter"); + if (token == RTAS_UNKNOWN_SERVICE) + return; + + local_buffer = kmalloc(sizeof(*local_buffer), GFP_KERNEL); + if (!local_buffer) + return; + + do { + spin_lock(&rtas_data_buf_lock); + memset(rtas_data_buf, 0, sizeof(*local_buffer)); + rc = rtas_call(token, 3, 1, NULL, SPLPAR_LPAR_NAME_TOKEN, + __pa(rtas_data_buf), sizeof(*local_buffer)); + if (!rc) + memcpy(local_buffer->raw_buffer, rtas_data_buf, + sizeof(local_buffer->raw_buffer)); + spin_unlock(&rtas_data_buf_lock); + } while (rtas_busy_delay(rc)); + + if (!rc) { + /* Force end of string */ + len = min((int) be16_to_cpu(local_buffer->len), + (int) sizeof(local_buffer->name)-1); + local_buffer->name[len] = '\0'; + + seq_printf(m, "partition_name=%s\n", local_buffer->name); + } else + pr_err_once("Error calling get-system-parameter (0x%x)\n", rc); + + kfree(local_buffer); +} + + #define SPLPAR_CHARACTERISTICS_TOKEN 20 #define SPLPAR_MAXLENGTH 1026*(sizeof(char)) @@ -496,6 +549,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 */ + read_lpar_name(m); parse_system_parameter_string(m); parse_ppp_data(m); parse_mpp_data(m);
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> --- v4: address Nathan's new comments limiting size of the buffer. v3: address Michael's comments. v2: address Nathan's comments. change title to partition_name aligning with existing partition_id --- arch/powerpc/platforms/pseries/lparcfg.c | 54 ++++++++++++++++++++++++ 1 file changed, 54 insertions(+)