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

[Suraj Jitindar Singh]

On Mon, 2017-07-03 at 19:20 +1000, David Gibson wrote:
> 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.

I think this is the right approach. There is no point calling the ioctl
if nothing changed. Additionally this fixes KVM_PR in the interim
assuming no max-cpu-compat is specified on the command line.

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

This would be the ideal fix however I suggest the implementation of
that would be to simply ignore it- given the main use case of KVM_PR is
nested and that means we can't actually set the PCR since it's
hypervisor privileged.

> 


Suraj