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

[Greg Kurz]

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 ?

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.

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 ?

Cheers,

--
Greg

pgpvgO74Tax1o.pgp
Description: OpenPGP digital signature