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