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

[David Gibson]

On Fri, Mar 29, 2019 at 01:29:51PM +0100, Greg Kurz wrote:
> On Thu, 28 Mar 2019 15:39:45 -0300
> "Maxiwell S. Garcia" <address@hidden> wrote:
> 
> > Hi,
> > 
> > On Thu, Mar 28, 2019 at 02:21:51PM +0100, Greg Kurz wrote:
> > > On Wed, 27 Mar 2019 17:41:00 -0300
> > > "Maxiwell S. Garcia" <address@hidden> wrote:
> > >   
> > > > Here are two patches to add a handler for ibm,get-vpd RTAS calls.
> > > > This RTAS exposes host information in case of set QEMU options
> > > > 'host-serial' and 'host-model' as 'passthrough'.
> > > > 
> > > > The patch 1 creates helper functions to get valid 'host-serial'
> > > > and 'host-model' parameters, guided by QEMU command line. These
> > > > parameters are useful to build the guest device tree and to return
> > > > get-vpd RTAS calls. The patch 2 adds the ibm,get-vpd itself.
> > > > 
> > > > Update v7:
> > > > * rtas_get_vpd_fields as a static array in spapr machine state
> > > > 
> > > > Maxiwell S. Garcia (2):
> > > >   spapr: helper functions to get valid host fields
> > > >   spapr-rtas: add ibm,get-vpd RTAS interface
> > > > 
> > > >  hw/ppc/spapr.c         | 48 +++++++++++----------
> > > >  hw/ppc/spapr_rtas.c    | 96 ++++++++++++++++++++++++++++++++++++++++++
> > > >  include/hw/ppc/spapr.h | 14 +++++-
> > > >  3 files changed, 135 insertions(+), 23 deletions(-)
> > > >   
> > > 
> > > Hi Maxiwell,
> > > 
> > > David sent a patch to rework how the host data is exposed to the guest.
> > > Especially, the special casing of the "none" and "passthrough" strings
> > > is no more... I'm afraid you'll have to rework your patches accordingly:
> > > code+changelog in patch 1 and at least changelog in patch 2.
> > > 
> > > Cheers,  
> > 
> > IIUC, the 'ibm,get-vpd' RTAS should return information about the
> > platform/cabinet. Thus, it's not necessary to add new nodes in the guest
> > device tree to export information like that.
> 
> I agree that these "host-model" and "host-serial" props, which aren't
> described anywhere and not used by either the linux kernel or the
> powerpc-utils, look like a QEMU-specific poor man's version of VPD.
> 
> Not quite sure why they were even created since this is the purpose
> of "system-id" and "model" as explained in PAPR, and supposedly
> exposed in /proc/ppc64/lparcfg according to the LPARCFG(5) manual
> page:

Yeah, I'm not sure why they were created either.  I rather suspect
nothing much is using them, and I'd kind of like to just kill them.
But Daniel Berrange (and maybe others) are paranoid about this
breaking things.

> 
>        serial_number
>        The serial number of the physical system in which the partition resides
> 
>        system_type
>        The  machine,type-model  of  the physical system in which the partition
>        resides
> 
> This is indeed what we get in a PowerVM LPAR running on a tuleta system:
> 
> address@hidden ~]# head -3 /proc/ppc64/lparcfg 
> lparcfg 1.9
> serial_number=IBM,032116A9A
> system_type=IBM,8247-22L
> 
> address@hidden ~]# echo $(cat /proc/device-tree/system-id)
> IBM,032116A9A
> address@hidden ~]# echo $(cat /proc/device-tree/model)
> IBM,8247-22L
> 
> But QEMU generates a hard coded "IBM pSeries (emulated by qemu)" model,
> which is clearly not PAPR compliant according to this requirement:
> 
>       R1–12.2–13. There must be a property, “model”, under the root node
>       in the format, “<vendor>,xxxx-yyy”, where <vendor> is replaced by
>       one to five letters representing the stock symbol of the company
>       (for example, for IBM: “IBM,xxxx-yyy”), and where xxxx-yyy is
>       derived from the VPD TM field (see Table 160‚ “LoPAPR VPD Fields‚”
>       on page 343) of the first or ‘marked’ processor enclosure.
> 
> And worse, it mixes "vm,uuid" which is QEMU specific concept to uniquely
> identify guests, with "system-id" which is about the host :-\

Ugh, right.  So, it's been this way for years, so it's clearly not
breaking things in practice.  Given that, it might be best to leave
it, even though it violates PAPR technically.

Frankly, providing information about the *guest* virtual model and
"serial number" =~= UUID makes more sense than providing information
about the host that might be security sensitive.

> All of this is very confusing and need to be sorted out before building
> anything on top of it. Especially since "model" and "system-id" are
> supposed to derive from VPD IIUC.
> 
> I guess that we should first decide what we really want to expose
> in "system-id" and "model": what we have now ? the same as in
> "host-serial" and "host-model", ie. user configurable ? Must we
> stay compatible with existing setups ? In any case, I'm afraid that
> we have to diverge from PAPR somehow, since it obviously doesn't
> care about the security concerns that motivated recent changes
> for "host-serial" and "host-model"...

We absolutely should not expose the real host information without the
user explicitly requesting it.

> > Since it's a POWER specific
> > functionality, may 'ibm,get-vpd' export host information if the
> > guest instance allows it? Or is it better return only the 'host-serial'
> > and 'host-model' content, like in the patch "spapr: Simplify handling
> > of host-serial and host-model values"?
> > 
> 
> I've spent some time reading PAPR on this topic and I'm not sure that
> "ibm,get-vpd" is the way to go for what you were trying to achieve
> initially (ie, obtain up-to-date host model and serial after migration).
> 
> Have you looked at RTAS "ibm,update-properties" ?
> 
>       7.4.8 ibm,update-properties RTAS Call
> 
>       This RTAS call is used to report updates to the properties changed
>       due to a massive platform reconfiguration such as when the partition
>       is migrated between machines.

Yeah.. the thing here is that partition migration kind of means
something different in PAPR than it does in the qemu world.  It uses a
guest-aware model of migration (which frankly I think is broken
verging on unworkable).  qemu and uses a guest-(mostly)-unaware
migration model.

> This explicitly covers updates to "system-id" and "model". Maybe it is
> time to do as Ben was suggesting a long time ago when "host-serial"
> and "host-model" were introduced [1]:
> 
>       On Tue, 2014-07-08 at 12:49 +0200, Alexander Graf wrote:
>       > Please be aware that all of the above is bogus when you start
>       > thinking 
>       > about live migration.
> 
>       What's probably where we need to start thinking about implementing
>       migration according to PAPR :-)
> 
>       IE. With pre and post-migration notifications to the guest including
>       device-tree updates.
> 
> 
> [1] https://patchwork.ozlabs.org/patch/367792/#813547
> 

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