Re: [Qemu-ppc] [PATCH 3/3] target/ppc: filter out non-zero PCR values when using TCG

[David Gibson]

On Wed, Jun 13, 2018 at 04:26:39PM +0200, Greg Kurz wrote:
> On Wed, 13 Jun 2018 22:05:02 +1000
> David Gibson <address@hidden> wrote:
> 
> > On Wed, Jun 13, 2018 at 10:19:15AM +0200, Greg Kurz wrote:
> > > On Wed, 13 Jun 2018 10:45:06 +1000
> > > David Gibson <address@hidden> wrote:
> > >   
> > > > On Tue, Jun 12, 2018 at 07:04:15PM +0200, Greg Kurz wrote:  
> > > > > Bits set in the PCR disable features of the processor. TCG currently
> > > > > doesn't implement that, ie, we always act like if PCR is all zeros.
> > > > > 
> > > > > But it is still possible for the PCR to have a non-null value. This 
> > > > > may
> > > > > confuse the guest.
> > > > > 
> > > > > There are three distinct cases:
> > > > > 
> > > > > 1) a powernv guest doing mtspr SPR_PCR
> > > > > 
> > > > > 2) reset of a pseries guest if the max-cpu-compat machine property is 
> > > > > set
> > > > > 
> > > > > 3) CAS of a pseries guest
> > > > > 
> > > > > This patch adds a ppc_store_pcr() helper that ensures we cannot put
> > > > > a non-null value in the PCR when using TCG. This helper also has
> > > > > error propagation support, so that each case listed above can be
> > > > > handled appropriately:
> > > > > 
> > > > > 1) since the powernv machine is mostly used for OpenPOWER FW devel,
> > > > >    we just print an error and let QEMU continue execution
> > > > > 
> > > > > 2) an error is printed and QEMU exits, ie, same behaviour as when
> > > > >    KVM doesn't support the requested compat mode
> > > > > 
> > > > > 3) an error is printed and QEMU reports H_HARDWARE to the guest
> > > > > 
> > > > > Signed-off-by: Greg Kurz <address@hidden>    
> > > > 
> > > > I'm not really convinced this is a good idea.  Printing a (non fatal)
> > > > error if the guest attempts to write a non-zero value to the PCR
> > > > should be ok.  However, you're generating a fatal error if the machine
> > > > tries to set the PCR in TCG mode.  That could easily happen using,
> > > > e.g. the cap-htm flag on a TCG guest.  That would take TCG from mostly
> > > > working, to refusing to run at all.
> > > >   
> > > 
> > > I'm confused... I don't see anything related to HTM in TCG. Also we have
> > > the following in cap_htm_apply():
> > > 
> > >     if (tcg_enabled()) {
> > >         error_setg(errp,
> > >                    "No Transactional Memory support in TCG, try 
> > > cap-htm=off");
> > > 
> > > I'm probably missing something... can you enlighten me ?  
> > 
> > Ok, so right now when cap-htm=off we don't actually enforce that, we
> > just don't advertise it to the guest.  We probably _should_ enforce
> > that, and the way we'd do it is to set the appropriate bit in the
> > PCR.  That'll do the right thing for KVM (well, once we update KVM to
> > actually pass on the PCR value) but would break TCG in conjunction
> > with your patch above.
> 
> Hmm... The granularity of the PCR bits is PowerISA versions, not individual
> facilities AFAIK... or am I missing something again ?

Huh.. so.  In the 3.0 ISA, there are only ISA version bits.  But in
the 2.07 ISA, there are ISA version bits *and* a bit to control HTM.

I'm not quite sure what to make of that.  I kind of love the fact that
they incompatibly change the compatibility register.

> Anyway, with the current code, if we start the guest with:
> 
> -machine accel=tcg,max-cpu-compat=power8 -cpu POWER9
> 
> We get this in the guest:
> 
> # grep ^cpu /proc/cpuinfo 
> cpu             : POWER8 (architected), altivec supported
> 
> This is the expected result of the CAS negotiation, but
> the guest can still execute PowerISA 3.0 instructions
> that should cause an invalid instruction exception.

Right, this is a bug, albeit not a very high priority one.

> I agree this shouldn't cause QEMU to exit, but I guess we should
> at least leave the PCR to all zeroes, so that the guest view
> is consistent with what TGC does (ie, raw mode). And maybe an
> error message to indicate that the PCR is ignored in TCG... but
> since that could be guest triggered, and the user cannot do anything
> about it, I'm wondering if this should rather be a trace actually.
> 
> Does it make sense for you ?

TBH, I don't care all that much.  There are a bunch of cases where TCG
doesn't behave quite right, most without warnings.  One more or less
doesn't make a huge difference.

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