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

[David Gibson]

On Mon, Aug 07, 2017 at 11:00:18AM +0200, Greg Kurz wrote:
> 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.

It tends to be used that way, but it doesn't have to be.

> Thus, I don't see the point in implementing PPC_ARCH_COMPAT in PR... but
> I'm probably missing something :)

Well, qemu expects to be able to set ARCH_COMPAT for a pseries guest,
if that guest is going into a compatibility mode (which it usually
does these days).  We don't want userspace to have to be constantly
checking which KVM implementation its working against, so it makes
sense for PR to implement the call, even if it's a no-op because it
can't really implement the PCR fully.

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

signature.asc
Description: PGP signature