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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 10852 bytes --]

On Tue, Apr 02, 2019 at 12:28:07PM +0200, Greg Kurz wrote:
> On Mon, 1 Apr 2019 12:01:48 -0300
> "Maxiwell S. Garcia" <maxiwell@linux.ibm.com> wrote:
> 
> > Hi Greg,
> > 
> > Thanks for your review. I added some comments below...
> > 
> > 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" <maxiwell@linux.ibm.com> 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" <maxiwell@linux.ibm.com> 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:
> > > 
> > >        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:
> > > 
> > > [root@furax1 ~]# head -3 /proc/ppc64/lparcfg 
> > > lparcfg 1.9
> > > serial_number=IBM,032116A9A
> > > system_type=IBM,8247-22L
> > > 
> > > [root@furax1 ~]# echo $(cat /proc/device-tree/system-id)
> > > IBM,032116A9A
> > > [root@furax1 ~]# 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 :-\
> > > 
> > > 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"...
> > >   
> > 
> > Many important changes should be done to solve these inconsistencies.
> > So, I saw in the 'get-vpd' RTAS a manner to return host information
> > and that works with live-migration.
> > 
> 
> Yes it does indeed allow that, among other things. My concern is more that
> PAPR clearly indicates that the "system-id" and "model" in the root node
> are derived from the VPD, and this series is about to tie the VPD TM and
> VPD SE keywords to something else that isn't documented in PAPR...
> 
> The more I look at the "host-serial" and "host-model" properties, the more
> I have the impression that they serve the same purpose as "system-id" and
> "model" in PAPR (at least when peeking into the device tree of a PowerVM
> LPAR as shown somewhere ^^)... what about unifying these ? It would likely
> impact some guest software that use this to guess the platform type, like
> powerpc-utils for example:
> 
> 		} else if (strstr(line, "IBM pSeries (emulated by qemu)")) {
> 			rc = PLATFORM_POWERKVM_GUEST;
> 			break;
> 		} else if (strstr(line, "pSeries")) {
> 
> but this is fragile and should be improved anyway...
> 
> > >   
> > > > 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.
> > > 
> > > 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
> > > 
> > 
> > The 'ibm,update-properties' and 'ibm,update-nodes' RTAS report which DT
> > nodes was modified. So, to implement this approach, the QEMU should change
> > the DT nodes when the live-migration finish and some service in the guest
> > need to call these RTAS to discovery what nodes was changed. Is it the
> > right way?
> > 
> 
> If QEMU on the target is started with different "host-serial" and
> "host-model", the DT in QEMU already has the new value. No need to
> change anything. Appart from that, yes, QEMU should generate a PRRN
> event at post load to notify the guest, which in turns do the RTAS
> calls to get the updates.

Hrm.  The way the DT is handled between qemu, SLOF and the guest
would, I suspect, make ibm,update-properties a serious PITA to
implement.  So, I'm not super keen on that idea.

More generally, I'm not sure merging concepts from PAPR's guest-aware
migration model into qemu's guest-unaware migration model is a great
idea.

> > Assuming that in new pseries machine won't be possible choose
> > 'passthrough' options to 'host-serial' and 'host-model' (last patch of
> > dgibson about this), it's necessary get the host information and set it
> > as string in these options. So, if we use the same qemu options in a
> > live-migration scenario for src and dst (libvirt do that), these
> > properties will remain the same. Is this behavior expected?
> > 
> 
> The recent fixes around "host-serial" and "host-model" simply moved
> the decision to expose host data to the upper layer, ie. libvirt
> which should be involved in this discussion.

Right, that's deliberate.  Note that roughly-equivalent information on
x86 is currently supplied via the SMBIOS.  OpenStack Nova sets that,
rather than qemu, and I'd like to move towards a common configuration
model with x86, though it's a fairly long path to there.

OpenStack had an equivalent security problem to our one, which it
addressed by taking the host serial from /etc/machine-id if present
rather than the real host info.

> Cc'ing Andrea for expertise. Problem exposed below.
> 
> The pseries machine used to expose the content of the host's
> /proc/device-tree/system-id and /proc/device-tree/model in the guest
> DT. This led to a CVE and QEMU doesn't do that anymore for new machine
> types. Instead, two new properties where added to the pseries machine:
> 
> pseries-4.0.host-serial=string (Host serial number to advertise in guest device tree)
> pseries-4.0.host-model=string (Host model to advertise in guest device tree)
> 
> It is up to the caller to pass something... which may be anything,
> including something like $(cat /proc/device-tree/system-id) or
> randomly generated.
> 
> Is there a chance libvirt can be taught to pass a different string
> to the target QEMU in case of migration ?

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]