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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5144 bytes --]

On Thu, Aug 03, 2017 at 07:28:06PM +0200, Greg Kurz wrote:
> On Thu, 13 Jul 2017 11:21:18 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > 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 <sjitindarsingh@gmail.com>  
> > > > > > 
> > > > > > 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.
> > 
> 
> Does it make sense at all to use compat mode with KVM_PR since it
> requires hypervisor privilege, that we're supposed not to have ?

Uh.. what?  Availability of the PCR is a question of the guest
environment, and PR (at least potentially) supports various different
guest environments, both with and without (apparent) hypervisor
capability.

> Shouldn't we check for kvm_get_one_reg(KVM_REG_PPC_ARCH_COMPAT) and
> don't try to do any compat stuff if it isn't supported ? This would
> include exiting QEMU if max-cpu-compat was passed on the cmdline.

Oh.. right.. that's actually what I meant by setting the PCR.  PCR
setting from the userspace side via PPC_ARCH_COMPAT, rather than PCR
setting from the guest side.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]