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

[David Gibson]

On Tue, Mar 12, 2019 at 11:46:29AM +0100, Greg Kurz wrote:
> 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 ?

Yeah, that should definitely be in the machine state.  I also don't
see any clear reason to keep the keyword and value separate.  IIUC
it's just a blob of data that gets sent to the guest.  We don't care
at that point that the first 2 characters are a keyword.

> > +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));

It would also be preferable to dynamically allocate this array under
the machine type.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature