Re: [Qemu-ppc] [PATCH] target/ppc: Only set PCR in kvm if actually in a compat mode

[David Gibson]

On Mon, Jul 03, 2017 at 01:18:38PM +1000, Suraj Jitindar Singh wrote:
> On Fri, 2017-06-30 at 14:03 +1000, David Gibson wrote:
> > On Thu, Jun 29, 2017 at 02:59:39PM +1000, Suraj Jitindar Singh wrote:
> > > The Processor Compatibility Register (PCR) I used to set the
> > > compatibility mode of the processor using the SET_ONE_REG ioctl on
> > > KVM_REG_PPC_ARCH_COMPAT. Previously this was only called when a
> > > compat
> > > mode was actually in use, however a recent patch made it
> > > unconditional.
> > > Calling this in KVM_PR fails as there is no handler for that call
> > > and it
> > > is thus impossible to start a machine with KVM_PR.
> > > 
> > > Change ppc_set_compat() so that the ioctl is only actually called
> > > if a
> > > compat mode is in use. This means that a KVM_PR guest can boot.
> > > Additionally the current behaviour for KVM_HV is preserved where a
> > > compat
> > > mode of 0 set pcr and arch_compat in the vcore struct to zero, both
> > > of
> > > which are initialised to zero anyway.
> > > 
> > > Fixes: 37f516defa2e ("pseries: Reset CPU compatibility mode")
> > > 
> > > Signed-off-by: Suraj Jitindar Singh <address@hidden>
> > 
> > This doesn't seem quite right.  With this change, how would we ever
> > turn compatibility mode _off_ (which could happen on reset if nothing
> 
> Oh yeah, didn't really think about that.
> 
> > else).  Really we should add this pseudo-register to KVM PR, although
> > I'm fine with also having a qemu workaround to let it work with older
> > PR versions.
> 
> How do you feel about having a check and only calling the ioctl if the
> KVM in use is HV?

Don't really like it.  For one thing, we want to avoid explicitly
checking for KVM PR - we should check specific capabilities instead.
For another, it means on PR we're silently ignoring the compatibility
mode which isn't really right.

I think the right approach here is to only call the ioctl() if the
compatibility mode has actually changed.  That should make it work in
the cases the original patch did, which is.. actually very few, given
the new CAS logic.

Really the right fix is to implement the set compat mode ioctl() in
KVM PR.

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