Patchwork [04/11] KVM: PPC: Book3S HV: Add GET/SET_ONE_REG interface for LPCR

login
register
mail settings
Submitter Paul Mackerras
Date Sept. 6, 2013, 3:21 a.m.
Message ID <20130906032107.GE29710@iris.ozlabs.ibm.com>
Download mbox | patch
Permalink /patch/273029/
State New
Headers show

Comments

Paul Mackerras - Sept. 6, 2013, 3:21 a.m.
This adds the ability for userspace to read and write the LPCR
(Logical Partitioning Control Register) value relating to a guest
via the GET/SET_ONE_REG interface.  There is only one LPCR value
for the guest, which can be accessed through any vcpu.  Userspace
can only modify the following fields of the LPCR value:

DPFD	Default prefetch depth
ILE	Interrupt little-endian
TC	Translation control (secondary HPT hash group search disable)

Signed-off-by: Paul Mackerras <paulus@samba.org>
---
 Documentation/virtual/kvm/api.txt   |  1 +
 arch/powerpc/include/asm/reg.h      |  2 ++
 arch/powerpc/include/uapi/asm/kvm.h |  1 +
 arch/powerpc/kvm/book3s_hv.c        | 21 +++++++++++++++++++++
 4 files changed, 25 insertions(+)
Alexander Graf - Sept. 13, 2013, 6:36 p.m.
On 05.09.2013, at 22:21, Paul Mackerras wrote:

> This adds the ability for userspace to read and write the LPCR
> (Logical Partitioning Control Register) value relating to a guest
> via the GET/SET_ONE_REG interface.  There is only one LPCR value
> for the guest, which can be accessed through any vcpu.  Userspace
> can only modify the following fields of the LPCR value:
> 
> DPFD	Default prefetch depth
> ILE	Interrupt little-endian
> TC	Translation control (secondary HPT hash group search disable)
> 
> Signed-off-by: Paul Mackerras <paulus@samba.org>

There are 3 things I dislike about this patch :)

  1) A vcpu one_reg should only change the state of the vcpu it's targeting. You want a vm wide thing.
  2) If anyone gets crazy enough to implement HV emulation in PR KVM this would overlap with the guest's guest LPCR, so we need to name it differently. It's really VM configuration, not a register. Maybe ENABLE_CAP is a better fit?
  3) Checkpatch fails:

WARNING: please, no space before tabs
#59: FILE: arch/powerpc/include/asm/reg.h:295:
+#define   LPCR_TC      ^I0x00000200^I/* Translation control */$


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Mackerras - Sept. 14, 2013, 2:21 a.m.
On Fri, Sep 13, 2013 at 01:36:25PM -0500, Alexander Graf wrote:
> 
> On 05.09.2013, at 22:21, Paul Mackerras wrote:
> 
> > This adds the ability for userspace to read and write the LPCR
> > (Logical Partitioning Control Register) value relating to a guest
> > via the GET/SET_ONE_REG interface.  There is only one LPCR value
> > for the guest, which can be accessed through any vcpu.  Userspace
> > can only modify the following fields of the LPCR value:
> > 
> > DPFD	Default prefetch depth
> > ILE	Interrupt little-endian
> > TC	Translation control (secondary HPT hash group search disable)
> > 
> > Signed-off-by: Paul Mackerras <paulus@samba.org>
> 
> There are 3 things I dislike about this patch :)
> 
>   1) A vcpu one_reg should only change the state of the vcpu it's targeting. You want a vm wide thing.

In hardware there is in fact an LPCR per hardware CPU thread, though
the architecture says that on threaded processors many of the fields
have to be the same on each thread, including the three fields
mentioned here.

>   2) If anyone gets crazy enough to implement HV emulation in PR KVM this would overlap with the guest's guest LPCR, so we need to name it differently. It's really VM configuration, not a register. Maybe ENABLE_CAP is a better fit?

If we were doing HV emulation in PR KVM then there would still only be
one LPCR per guest vcpu.  If there was a hypervisor running as the PR
guest, it would be its job to context switch the LPCR between its
guests.  So I don't see why there would be any need for the top-level
KVM to know about the guest's guest LPCR.

In that situation we would need a one_reg to get and set the guest's
LPCR, which would be per vcpu, and there would be no restriction on
which bits could be set by the host.

It sounds to me like the best option would be to keep an LPCR per vcpu
and use the one_reg interface to let the host get and set it.  The
actual LPCR applied when the guest runs would use some bits from the
per-vcpu value plus some bits set by the KVM host code (for the host's
protection).  How does that sound?

Alternatively I could define a new per-VM ioctl.  ENABLE_CAP doesn't
really help since it also is per-vcpu, not per-VM.

>   3) Checkpatch fails:
> 
> WARNING: please, no space before tabs
> #59: FILE: arch/powerpc/include/asm/reg.h:295:
> +#define   LPCR_TC      ^I0x00000200^I/* Translation control */$

Oops, will fix.

Paul.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - Sept. 14, 2013, 5:12 a.m.
Am 13.09.2013 um 21:21 schrieb Paul Mackerras <paulus@samba.org>:

> On Fri, Sep 13, 2013 at 01:36:25PM -0500, Alexander Graf wrote:
>> 
>> On 05.09.2013, at 22:21, Paul Mackerras wrote:
>> 
>>> This adds the ability for userspace to read and write the LPCR
>>> (Logical Partitioning Control Register) value relating to a guest
>>> via the GET/SET_ONE_REG interface.  There is only one LPCR value
>>> for the guest, which can be accessed through any vcpu.  Userspace
>>> can only modify the following fields of the LPCR value:
>>> 
>>> DPFD    Default prefetch depth
>>> ILE    Interrupt little-endian
>>> TC    Translation control (secondary HPT hash group search disable)
>>> 
>>> Signed-off-by: Paul Mackerras <paulus@samba.org>
>> 
>> There are 3 things I dislike about this patch :)
>> 
>>  1) A vcpu one_reg should only change the state of the vcpu it's targeting. You want a vm wide thing.
> 
> In hardware there is in fact an LPCR per hardware CPU thread, though
> the architecture says that on threaded processors many of the fields
> have to be the same on each thread, including the three fields
> mentioned here.

Ok. Is it mandatory to be identical per core? Or per vm?

> 
>>  2) If anyone gets crazy enough to implement HV emulation in PR KVM this would overlap with the guest's guest LPCR, so we need to name it differently. It's really VM configuration, not a register. Maybe ENABLE_CAP is a better fit?
> 
> If we were doing HV emulation in PR KVM then there would still only be
> one LPCR per guest vcpu.  If there was a hypervisor running as the PR
> guest, it would be its job to context switch the LPCR between its
> guests.  So I don't see why there would be any need for the top-level
> KVM to know about the guest's guest LPCR.
> 
> In that situation we would need a one_reg to get and set the guest's
> LPCR, which would be per vcpu, and there would be no restriction on
> which bits could be set by the host.

Thinking about this a bit more I think you might be correct.

> 
> It sounds to me like the best option would be to keep an LPCR per vcpu
> and use the one_reg interface to let the host get and set it.  The
> actual LPCR applied when the guest runs would use some bits from the
> per-vcpu value plus some bits set by the KVM host code (for the host's
> protection).  How does that sound?

Sounds similar to what you have, just that it's on vcpu level now :). I think that would work, yeah.

Alex

> 
> Alternatively I could define a new per-VM ioctl.  ENABLE_CAP doesn't
> really help since it also is per-vcpu, not per-VM.
> 
>>  3) Checkpatch fails:
>> 
>> WARNING: please, no space before tabs
>> #59: FILE: arch/powerpc/include/asm/reg.h:295:
>> +#define   LPCR_TC      ^I0x00000200^I/* Translation control */$
> 
> Oops, will fix.
> 
> Paul.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Mackerras - Sept. 14, 2013, 5:58 a.m.
On Sat, Sep 14, 2013 at 12:12:43AM -0500, Alexander Graf wrote:
> 
> 
> Am 13.09.2013 um 21:21 schrieb Paul Mackerras <paulus@samba.org>:
> 
> > In hardware there is in fact an LPCR per hardware CPU thread, though
> > the architecture says that on threaded processors many of the fields
> > have to be the same on each thread, including the three fields
> > mentioned here.
> 
> Ok. Is it mandatory to be identical per core? Or per vm?

Per core.

Paul.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - Sept. 14, 2013, 11:38 a.m.
Am 14.09.2013 um 00:58 schrieb Paul Mackerras <paulus@samba.org>:

> On Sat, Sep 14, 2013 at 12:12:43AM -0500, Alexander Graf wrote:
>> 
>> 
>> Am 13.09.2013 um 21:21 schrieb Paul Mackerras <paulus@samba.org>:
>> 
>>> In hardware there is in fact an LPCR per hardware CPU thread, though
>>> the architecture says that on threaded processors many of the fields
>>> have to be the same on each thread, including the three fields
>>> mentioned here.
>> 
>> Ok. Is it mandatory to be identical per core? Or per vm?
> 
> Per core.

Then put it into the vcore struct like you put everything else with core wide scope, no? :)

Alex

> 
> Paul.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index c36ff9af..1030ac9 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1835,6 +1835,7 @@  registers, find a list below:
   PPC   | KVM_REG_PPC_PID	| 64
   PPC   | KVM_REG_PPC_ACOP	| 64
   PPC   | KVM_REG_PPC_VRSAVE	| 32
+  PPC   | KVM_REG_PPC_LPCR	| 64
   PPC   | KVM_REG_PPC_TM_GPR0	| 64
           ...
   PPC   | KVM_REG_PPC_TM_GPR31	| 64
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 342e4ea..3fc0d06 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -275,6 +275,7 @@ 
 #define   LPCR_ISL	(1ul << (63-2))
 #define   LPCR_VC_SH	(63-2)
 #define   LPCR_DPFD_SH	(63-11)
+#define   LPCR_DPFD	(7ul << LPCR_DPFD_SH)
 #define   LPCR_VRMASD	(0x1ful << (63-16))
 #define   LPCR_VRMA_L	(1ul << (63-12))
 #define   LPCR_VRMA_LP0	(1ul << (63-15))
@@ -291,6 +292,7 @@ 
 #define     LPCR_PECE2	0x00001000	/* machine check etc can cause exit */
 #define   LPCR_MER	0x00000800	/* Mediated External Exception */
 #define   LPCR_MER_SH	11
+#define   LPCR_TC      	0x00000200	/* Translation control */
 #define   LPCR_LPES    0x0000000c
 #define   LPCR_LPES0   0x00000008      /* LPAR Env selector 0 */
 #define   LPCR_LPES1   0x00000004      /* LPAR Env selector 1 */
diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
index b98bf3f..e42127d 100644
--- a/arch/powerpc/include/uapi/asm/kvm.h
+++ b/arch/powerpc/include/uapi/asm/kvm.h
@@ -533,6 +533,7 @@  struct kvm_get_htab_header {
 #define KVM_REG_PPC_ACOP	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb3)
 
 #define KVM_REG_PPC_VRSAVE	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xb4)
+#define KVM_REG_PPC_LPCR	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xb5)
 
 /* Transactional Memory checkpointed state:
  * This is all GPRs, all VSX regs and a subset of SPRs
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index b930caf..9c878d7 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -714,6 +714,21 @@  int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+static void kvmppc_set_lpcr(struct kvm_vcpu *vcpu, u64 new_lpcr)
+{
+	struct kvm *kvm = vcpu->kvm;
+	u64 mask;
+
+	mutex_lock(&kvm->lock);
+	/*
+	 * Userspace can only modify DPFD (default prefetch depth),
+	 * ILE (interrupt little-endian) and TC (translation control).
+	 */
+	mask = LPCR_DPFD | LPCR_ILE | LPCR_TC;
+	kvm->arch.lpcr = (kvm->arch.lpcr & ~mask) | (new_lpcr & mask);
+	mutex_unlock(&kvm->lock);
+}
+
 int kvmppc_get_one_reg(struct kvm_vcpu *vcpu, u64 id, union kvmppc_one_reg *val)
 {
 	int r = 0;
@@ -796,6 +811,9 @@  int kvmppc_get_one_reg(struct kvm_vcpu *vcpu, u64 id, union kvmppc_one_reg *val)
 	case KVM_REG_PPC_TB_OFFSET:
 		*val = get_reg_val(id, vcpu->arch.vcore->tb_offset);
 		break;
+	case KVM_REG_PPC_LPCR:
+		*val = get_reg_val(id, vcpu->kvm->arch.lpcr);
+		break;
 	default:
 		r = -EINVAL;
 		break;
@@ -900,6 +918,9 @@  int kvmppc_set_one_reg(struct kvm_vcpu *vcpu, u64 id, union kvmppc_one_reg *val)
 		vcpu->arch.vcore->tb_offset =
 			ALIGN(set_reg_val(id, *val), 1UL << 24);
 		break;
+	case KVM_REG_PPC_LPCR:
+		kvmppc_set_lpcr(vcpu, set_reg_val(id, *val));
+		break;
 	default:
 		r = -EINVAL;
 		break;