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

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

[Suraj Jitindar Singh]

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>

---

Based on: dwg/ppc-for-2.10

 target/ppc/compat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/compat.c b/target/ppc/compat.c
index f1b67fa..4482206 100644
--- a/target/ppc/compat.c
+++ b/target/ppc/compat.c
@@ -143,7 +143,7 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
     cpu->compat_pvr = compat_pvr;
     env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
 
-    if (kvm_enabled()) {
+    if (kvm_enabled() && compat_pvr) {
         int ret = kvmppc_set_compat(cpu, cpu->compat_pvr);
         if (ret < 0) {
             error_setg_errno(errp, -ret,
-- 
2.9.4

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

[David Gibson]

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

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

> 
> ---
> 
> Based on: dwg/ppc-for-2.10
> 
>  target/ppc/compat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/compat.c b/target/ppc/compat.c
> index f1b67fa..4482206 100644
> --- a/target/ppc/compat.c
> +++ b/target/ppc/compat.c
> @@ -143,7 +143,7 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
>      cpu->compat_pvr = compat_pvr;
>      env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
>  
> -    if (kvm_enabled()) {
> +    if (kvm_enabled() && compat_pvr) {
>          int ret = kvmppc_set_compat(cpu, cpu->compat_pvr);
>          if (ret < 0) {
>              error_setg_errno(errp, -ret,

-- 
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: 819 bytes --]

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

[Suraj Jitindar Singh]

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?

> 
> > 
> > ---
> > 
> > Based on: dwg/ppc-for-2.10
> > 
> >  target/ppc/compat.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/target/ppc/compat.c b/target/ppc/compat.c
> > index f1b67fa..4482206 100644
> > --- a/target/ppc/compat.c
> > +++ b/target/ppc/compat.c
> > @@ -143,7 +143,7 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t
> > compat_pvr, Error **errp)
> >      cpu->compat_pvr = compat_pvr;
> >      env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
> >  
> > -    if (kvm_enabled()) {
> > +    if (kvm_enabled() && compat_pvr) {
> >          int ret = kvmppc_set_compat(cpu, cpu->compat_pvr);
> >          if (ret < 0) {
> >              error_setg_errno(errp, -ret,
> 
> 

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

[David Gibson]

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

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.

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

-- 
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: 819 bytes --]

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

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH] target/ppc: Only set PCR in kvm if actually in a compat mode
Message-id: 20170629045940.11242-1-sjitindarsingh@gmail.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
fatal: Cannot update paths and switch to branch 'test' at the same time.
Did you intend to checkout 'origin/patchew/20170629045940.11242-1-sjitindarsingh@gmail.com' which can not be resolved as commit?
Traceback (most recent call last):
  File "/home/fam/bin/patchew", line 440, in test_one
    git_clone_repo(clone, r["repo"], r["head"], logf)
  File "/home/fam/bin/patchew", line 53, in git_clone_repo
    cwd=clone)
  File "/usr/lib64/python3.5/subprocess.py", line 271, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['git', 'checkout', 'origin/patchew/20170629045940.11242-1-sjitindarsingh@gmail.com', '-b', 'test']' returned non-zero exit status 128



---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

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

[Fam Zheng]

On Thu, 07/06 16:49, no-reply@patchew.org wrote:
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> fatal: Cannot update paths and switch to branch 'test' at the same time.
> Did you intend to checkout 'origin/patchew/20170629045940.11242-1-sjitindarsingh@gmail.com' which can not be resolved as commit?
> Traceback (most recent call last):
>   File "/home/fam/bin/patchew", line 440, in test_one
>     git_clone_repo(clone, r["repo"], r["head"], logf)
>   File "/home/fam/bin/patchew", line 53, in git_clone_repo
>     cwd=clone)
>   File "/usr/lib64/python3.5/subprocess.py", line 271, in check_call
>     raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['git', 'checkout', 'origin/patchew/20170629045940.11242-1-sjitindarsingh@gmail.com', '-b', 'test']' returned non-zero exit status 128


Ignore this please, patchew is recovering from a bad state.

Re: [Qemu-devel] [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 <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.

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

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

[David Gibson]

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

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.

-- 
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: 819 bytes --]

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

[Greg Kurz]

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

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 ?

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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[David Gibson]

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

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

[Greg Kurz]

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

On Fri, 4 Aug 2017 12:35:30 +1000
David Gibson <david@gibson.dropbear.id.au> 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 <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.
> 

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[David Gibson]

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

On Mon, Aug 07, 2017 at 11:00:18AM +0200, Greg Kurz wrote:
> On Fri, 4 Aug 2017 12:35:30 +1000
> David Gibson <david@gibson.dropbear.id.au> 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 <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.
> > 
> 
> 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

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

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

[Greg Kurz]

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

On Tue, 8 Aug 2017 16:21:06 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
[...]
> > > > 
> > > > 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.
> 

Thanks for the explanation!

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]