On Thu, 14 Mar 2019 13:29:49 -0300
"Maxiwell S. Garcia" <maxiw...@linux.ibm.com> wrote:
> This adds a handler for ibm,get-vpd RTAS calls, allowing pseries guest
> to collect host information. It is disabled by default to avoid unwanted
> information leakage. To enable it, use:
>
> -M pseries,host-serial={passthrough|string},host-model={passthrough|string}
>
> Only the SE and TM keywords are returned at the moment.
> SE for Machine or Cabinet Serial Number and
> TM for Machine Type and Model
>
> The SE and TM keywords are controlled by 'host-serial' and 'host-model'
> options, respectively. In the case of 'passthrough', the SE shows the
> host 'system-id' information and the TM shows the host 'model' information.
>
> Powerpc-utils tools can dispatch RTAS calls to retrieve host
> information using this ibm,get-vpd interface. The 'host-serial'
> and 'host-model' nodes of device-tree hold the same information but
> in a static manner, which is useless after a migration operation.
>
> Signed-off-by: Maxiwell S. Garcia <maxiw...@linux.ibm.com>
> ---
> hw/ppc/spapr_rtas.c | 99 ++++++++++++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr.h | 7 ++-
> 2 files changed, 105 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 24c45b12d4..8ab092c67b 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -287,6 +287,101 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU
> *cpu,
> rtas_st(rets, 0, ret);
> }
>
> +static inline int vpd_st(target_ulong addr, target_ulong len,PARAM
> + const void *val, uint16_t val_len)
I see no compelling reason to make this inline. Better to let the
compiler decide.
> +{
> + hwaddr phys = ppc64_phys_to_real(addr);
> +
> + if (len < val_len) {
> + return RTAS_OUT_PARAM_ERROR;
> + }
> +
> + cpu_physical_memory_write(phys, val, val_len);
> + return RTAS_OUT_SUCCESS;
> +}
> +
> +static inline void vpd_ret(target_ulong rets, const int status,
> + const int next_seq_number, const int
> bytes_returned)
Same here.
> +{
> + rtas_st(rets, 0, status);
> + rtas_st(rets, 1, next_seq_number);
> + rtas_st(rets, 2, bytes_returned);
> +}
> +
> +/* Maximum number of ibm,get-vpd fields supported */
> +#define RTAS_IBM_GET_VPD_MAX 2
> +
> +static void rtas_ibm_get_vpd_fields_register(SpaprMachineState *spapr)
> +{
> + int i = 0;
> + char *buf = NULL;
> +
> + spapr->rtas_get_vpd_fields = g_malloc0(sizeof(char *) *
> + RTAS_IBM_GET_VPD_MAX + 1);
As suggested in HACKING, better to use g_new0() here.
Also, this gets called during machine reset, ie, this would cause a
memory leak each time we do system_reset... you should do g_free()
before g_new0().
Thinking again, it is a bit suboptimal to free and re-allocate the same
fixed size array again and again, even if reset isn't a performance path.
It could be be a regular array under spapr I guess and you would only need
to memset(0) I guess...
David ? What was the motivation to ask Maxiwell to go for a dynamic
allocation ?
> +
> + buf = spapr_get_valid_host_serial(spapr);
> + if (buf) {
> + spapr->rtas_get_vpd_fields[i++] = g_strdup_printf("SE %s", buf);
> + g_free(buf);
> + }
> +
> + buf = spapr_get_valid_host_model(spapr);
> + if (buf) {
> + spapr->rtas_get_vpd_fields[i++] = g_strdup_printf("TM %s", buf);
> + g_free(buf);PARAM
> + }
> +}
> +
> +static void rtas_ibm_get_vpd(PowerPCCPU *cpu,
> + SpaprMachineState *spapr,
> + uint32_t token, uint32_t nargs,
> + target_ulong args,
> + uint32_t nret, target_ulong rets)
> +{
> + target_ulong loc_code_addr = rtas_ld(args, 0);
> + target_ulong work_area_addr = rtas_ld(args, 1);
> + target_ulong work_area_size = rtas_ld(args, 2);
> + target_ulong seq_number = rtas_ld(args, 3);
> + unsigned char loc_code = 0;
> + unsigned int next_seq_number = 1;
> + int status = RTAS_OUT_PARAM_ERROR;
> + int ret = RTAS_OUT_PARAM_ERROR;
> + char *vpd_field = NULL;
> + unsigned int vpd_field_len = 0;
It looks like loc_code, status, ret, vpd_field and vpd_field_len always
get set in the code and don't need default values...
> +
> + /* Specific Location Code is not supported and seq_number */
> + /* must be checked to avoid out of bound index error */
> + cpu_physical_memory_read(loc_code_addr, &loc_code, 1);
> + if ((loc_code != 0) || (seq_number <= 0) ||
> + (seq_number > RTAS_IBM_GET_VPD_MAX)) {
> + vpd_ret(rets, RTAS_OUT_PARAM_ERROR, 1, 0);
> + return;
> + }
> +
> + /* RTAS not authorized if no keywords have been registered */
> + if (!spapr->rtas_get_vpd_fields[seq_number - 1]) {
> + vpd_ret(rets, RTAS_OUT_NOT_AUTHORIZED, 1, 0);
> + return;
> + }
> +
> + vpd_field = spapr->rtas_get_vpd_fields[seq_number - 1];
> + vpd_field_len = strlen(vpd_field);
> + ret = vpd_st(work_area_addr, work_area_size,
> + vpd_field, vpd_field_len + 1);
> +
> + if (ret == 0) {
> + next_seq_number = seq_number + 1;
> + if (spapr->rtas_get_vpd_fields[next_seq_number - 1]) {
> + status = RTAS_IBM_GET_VPD_CONTINUE;
> + } else {
> + status = RTAS_OUT_SUCCESS;
> + next_seq_number = 1;
> + }
> + }
> +
> + vpd_ret(rets, status, next_seq_number, vpd_field_len);
> +}
> +
> static void rtas_ibm_os_term(PowerPCCPU *cpu,
> SpaprMachineState *spapr,
> uint32_t token, uint32_t nargs,
> @@ -464,6 +559,8 @@ void spapr_load_rtas(SpaprMachineState *spapr, void *fdt,
> hwaddr addr)
> fdt_strerror(ret));
> exit(1);
> }
> +
> + rtas_ibm_get_vpd_fields_register(spapr);
> }
>
> static void core_rtas_register_types(void)
> @@ -485,6 +582,8 @@ static void core_rtas_register_types(void)
> rtas_ibm_set_system_parameter);
> spapr_rtas_register(RTAS_IBM_OS_TERM, "ibm,os-term",
> rtas_ibm_os_term);
> + spapr_rtas_register(RTAS_IBM_GET_VPD, "ibm,get-vpd",
> + rtas_ibm_get_vpd);
> spapr_rtas_register(RTAS_SET_POWER_LEVEL, "set-power-level",
> rtas_set_power_level);
> spapr_rtas_register(RTAS_GET_POWER_LEVEL, "get-power-level",
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 5e0a1b2a8a..34bb835bd3 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -162,6 +162,7 @@ struct SpaprMachineState {
> struct PPCTimebase tb;
> bool has_graphics;
> uint32_t vsmt; /* Virtual SMT mode (KVM's "core stride") */
> + char **rtas_get_vpd_fields;
>
> Notifier epow_notifier;
> QTAILQ_HEAD(, SpaprEventLogEntry) pending_events;
> @@ -619,14 +620,18 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu,
> target_ulong opcode,
> #define RTAS_IBM_CREATE_PE_DMA_WINDOW (RTAS_TOKEN_BASE + 0x27)
> #define RTAS_IBM_REMOVE_PE_DMA_WINDOW (RTAS_TOKEN_BASE + 0x28)
> #define RTAS_IBM_RESET_PE_DMA_WINDOW (RTAS_TOKEN_BASE + 0x29)
> +#define RTAS_IBM_GET_VPD (RTAS_TOKEN_BASE + 0x2A)
>
> -#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2A)
> +#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2B)
>
> /* RTAS ibm,get-system-parameter token values */
> #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS 20
> #define RTAS_SYSPARM_DIAGNOSTICS_RUN_MODE 42
> #define RTAS_SYSPARM_UUID 48
>
> +/* RTAS ibm,get-vpd status value */
> +#define RTAS_IBM_GET_VPD_CONTINUE 1
> +
> /* RTAS indicator/sensor types
> *
> * as defined by PAPR+ 2.7 7.3.5.4, Table 41
