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

[Greg Kurz]

On Fri, 4 Aug 2017 12:35:30 +1000
David Gibson <address@hidden> wrote:

> On Thu, Aug 03, 2017 at 07:28:06PM +0200, Greg Kurz wrote:
> > On Thu, 13 Jul 2017 11:21:18 +1000
> > David Gibson <address@hidden> 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 <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.
> > >   
> > 
> > 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.
> 

I mean mtpcr/mfpcr only work when the CPU is in hypervisor state, and PR
is supposed to be *mostly* used nested, ie, without hypervisor privilege.
Thus, I don't see the point in implementing PPC_ARCH_COMPAT in PR... but
I'm probably missing something :)

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

pgpRKulSSjqc0.pgp
Description: OpenPGP digital signature