| Message ID | 20170629045940.11242-1-sjitindarsingh@gmail.com |
|---|---|
| State | New |
| Headers | show |
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,
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, > >
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.
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
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.
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
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.
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.
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.
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. >
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. > >
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. > > > > > >
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,
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(-)