Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 2/2] spapr-rtas: add ibm, get-vpd RTAS interface

[Greg Kurz]

On Mon, 11 Mar 2019 19:57:09 -0300
"Maxiwell S. Garcia" <address@hidden> 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 <address@hidden>
> ---
>  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