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

[David Gibson]

On Wed, Mar 20, 2019 at 09:46:42AM -0300, Maxiwell S. Garcia wrote:
> On Thu, Mar 14, 2019 at 06:26:02PM +0100, Greg Kurz wrote:
> > On Thu, 14 Mar 2019 13:29:49 -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}
> > > 
> > > 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    | 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 ?

Uh, I don't recall.  Possibly I misspoke - my main focus was having
the whole VPD pre-generated, rather than pre-generating an "index"
then generating the pieces at the time of the RTAS call, which is
unnecessarily complicated.

> Can I send a v7 patch creating the rtas_get_vpd_fields array as a regular 
> array
> under spapr, avoiding dynamic allocation?

Sure.  No rush - this won't go in until 4.1 anyway.

I have been meaning to have another look at this, as well as the
"host-serial" and "host-model" properties in terms of design.  I
gather x86 had a similar issue recently, and I'd like to make sure our
interface for the workaround is as close to their's as it reasonably
can be.

-- 
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