On Mon, 11 Mar 2019 19:57:09 -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}’ > Hmm... the quotation marks cause the line to exceed 80 characters in git log: spapr-rtas: add ibm, get-vpd RTAS interface 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. Maybe simply drop them. > 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 | 110 +++++++++++++++++++++++++++++++++++++++++ > include/hw/ppc/spapr.h | 12 ++++- > 2 files changed, 121 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index 7a2cb786a3..778abcef91 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -287,6 +287,112 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU > *cpu, > rtas_st(rets, 0, ret); > } > I find the following function is rather compact. > +static inline int vpd_st(target_ulong addr, target_ulong len, > + const void *val, uint16_t val_len) > +{ > + hwaddr phys = ppc64_phys_to_real(addr); Maybe add a newline here... > + if (len < val_len) { > + return RTAS_OUT_PARAM_ERROR; > + } ... and here. > + 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) > +{ > + rtas_st(rets, 0, status); > + rtas_st(rets, 1, next_seq_number); > + rtas_st(rets, 2, bytes_returned); > +} > + > +static struct { > + char *keyword; > + char *value; > +} rtas_get_vpd_fields[RTAS_IBM_GET_VPD_KEYWORDS_MAX + 1]; > + Yikes... a static ? Why not putting this under the machine state ? > +static void rtas_ibm_get_vpd_fields_register(sPAPRMachineState *sm) s/sm/spapr for consistency with the rest of the spapr code. > +{ > + int i = 0; > + char *buf = NULL; > + > + memset(rtas_get_vpd_fields, 0, sizeof(rtas_get_vpd_fields)); > + > + buf = spapr_get_valid_host_serial(sm); > + if (buf) { > + rtas_get_vpd_fields[i].keyword = g_strdup("SE"); > + rtas_get_vpd_fields[i++].value = g_strdup(buf); > + g_free(buf); > + } > + buf = spapr_get_valid_host_model(sm); > + if (buf) { > + rtas_get_vpd_fields[i].keyword = g_strdup("TM"); > + rtas_get_vpd_fields[i++].value = g_strdup(buf); > + g_free(buf); > + } > +} > + > +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; > + target_ulong work_area_addr; > + target_ulong work_area_size; > + target_ulong seq_number; > + unsigned char loc_code = 0; > + unsigned int next_seq_number = 1; > + int status = RTAS_IBM_GET_VPD_PARAMETER_ERROR; > + int ret = RTAS_OUT_PARAM_ERROR; > + char *vpd_field = NULL; > + unsigned int vpd_field_len = 0; > + > + /* RTAS not authorized if no keywords have been registered */ > + if (!rtas_get_vpd_fields[0].keyword) { > + vpd_ret(rets, RTAS_OUT_NOT_AUTHORIZED, 1, 0); > + return; > + } > + > + loc_code_addr = rtas_ld(args, 0); > + work_area_addr = rtas_ld(args, 1); > + work_area_size = rtas_ld(args, 2); > + seq_number = rtas_ld(args, 3); > + > + /* 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_KEYWORDS_MAX)) { > + vpd_ret(rets, RTAS_IBM_GET_VPD_PARAMETER_ERROR, 1, 0); > + return; > + } > + > + vpd_field = g_strdup_printf("%s %s", > + rtas_get_vpd_fields[seq_number - 1].keyword, > + rtas_get_vpd_fields[seq_number - 1].value); > + > + if (vpd_field) { g_strdup_print() always return non-NULL. > + 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 (rtas_get_vpd_fields[next_seq_number - 1].keyword) { > + status = RTAS_IBM_GET_VPD_CONTINUE; > + } else { > + status = RTAS_IBM_GET_VPD_SUCCESS; > + next_seq_number = 1; > + } > + } > + } > + > + vpd_ret(rets, status, next_seq_number, vpd_field_len); > + g_free(vpd_field); I personally prefer to g_free() things as early as possible, ie. just after the call vpd_st() above with the vpd_field check removed. > +} > + > static void rtas_ibm_os_term(PowerPCCPU *cpu, > sPAPRMachineState *spapr, > uint32_t token, uint32_t nargs, > @@ -464,6 +570,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 +593,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 f7ea99dc69..84e57c3f34 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -608,14 +608,24 @@ 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 values */ > +#define RTAS_IBM_GET_VPD_VPD_CHANGED -4 It is indeed described in PAPR but I don't see ^^ being used anywhere... > +#define RTAS_IBM_GET_VPD_PARAMETER_ERROR -3 > +#define RTAS_IBM_GET_VPD_HARDWARE_ERROR -1 > +#define RTAS_IBM_GET_VPD_SUCCESS 0 These are RTAS_OUT_PARAM_ERROR, RTAS_OUT_HW_ERROR and RTAS_OUT_SUCCESS. Why do you need these new definitions ? > +#define RTAS_IBM_GET_VPD_CONTINUE 1 > + This one makes sense :) > +#define RTAS_IBM_GET_VPD_KEYWORDS_MAX 2 > + This macro tells about the size of rtas_get_vpd_fields[], which is a QEMU internal. It would be better to define it just before the declaration of rtas_get_vpd_fields[] since it has nothing to do with the other values that come from the PAPR spec. > /* RTAS indicator/sensor types > * > * as defined by PAPR+ 2.7 7.3.5.4, Table 41