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

[David Gibson]

On Wed, Jul 12, 2017 at 04:45:17PM +1000, Suraj Jitindar Singh wrote:
> 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.

Right, that's the idea.

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

Yeah, as we discussed on IRC, I tend to agree.  I don't love the idea
of silently presenting something other than requested.  However, we
don't really have much choice and we do already have precedent.  PR
tries to match the CPU requested in the PVR, but won't always be able
to do so exactly (if the host CPU supports userspace instructions the
requested PVR doesn't).  This doesn't really change the situation,
except that we have a (PVR+PCR) combination instead of just a PVR that
we're trying, and not completely suceeding, in matching.

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