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