KVM: PPC: Book3S: Get/set guest SPRs using the GET/SET_ONE_REG interface

This enables userspace to get and set various SPRs (special-purpose registers) using the KVM_[GS]ET_ONE_REG ioctls. With this, userspace can get and set all the SPRs that are part of the guest state, either through the KVM_[GS]ET_REGS ioctls, the KVM_[GS]ET_SREGS ioctls, or the KVM_[GS]ET_ONE_REG ioctls.

The SPRs that are added here are:

- DABR:  Data address breakpoint register
- DSCR:  Data stream control register
- PURR:  Processor utilization of resources register

- SPURR: Scaled PURR
- DAR: Data address register
- DSISR: Data storage interrupt status register
- AMR: Authority mask register
- UAMOR: User authority mask override register
- MMCR0, MMCR1, MMCRA: Performance monitor unit control registers
- PMC1..PMC8: Performance monitor unit counter registers

In order to reduce code duplication between PR and HV KVM code, this moves the kvm_vcpu_ioctl_[gs]et_one_reg functions into book3s.c and centralizes the copying between user and kernel space there. The registers that are handled differently between PR and HV, and those that exist only in one flavor, are handled in kvmppc_[gs]et_one_reg() functions that are specific to each flavor.

Signed-off-by Paul Mackerras <[email protected]>

---
v3: handle DAR and DSISR, plus copy to/from userspace, in common code

Documentation/virtual/kvm/api.txt  |   19 +++++++++
arch/powerpc/include/asm/kvm.h     |   21 ++++++++++
arch/powerpc/include/asm/kvm_ppc.h |    7 ++++
arch/powerpc/kvm/book3s.c          |   74 +++++++++++++++++++++++++++++++++++
arch/powerpc/kvm/book3s_hv.c       |   76 ++++++++++++++++++++++++++++++------
arch/powerpc/kvm/book3s_pr.c       |   23 ++++++-----
6 files changed, 196 insertions(+), 24 deletions(-)

Documentation/virtual/kvm/api.txt +19 -0

index 76a07a6..e4a2067 100644
@@ -1740,6 +1740,25 @@ registers, find a list below:
  PPC   | KVM_REG_PPC_IAC4      | 64
  PPC   | KVM_REG_PPC_DAC1      | 64
  PPC   | KVM_REG_PPC_DAC2      | 64
+  PPC   | KVM_REG_PPC_DABR      | 64
+  PPC   | KVM_REG_PPC_DSCR      | 64
+  PPC   | KVM_REG_PPC_PURR      | 64
+  PPC   | KVM_REG_PPC_SPURR     | 64
+  PPC   | KVM_REG_PPC_DAR       | 64
+  PPC   | KVM_REG_PPC_DSISR	| 32
+  PPC   | KVM_REG_PPC_AMR       | 64
+  PPC   | KVM_REG_PPC_UAMOR     | 64
+  PPC   | KVM_REG_PPC_MMCR0     | 64
+  PPC   | KVM_REG_PPC_MMCR1     | 64
+  PPC   | KVM_REG_PPC_MMCRA     | 64
+  PPC   | KVM_REG_PPC_PMC1      | 32
+  PPC   | KVM_REG_PPC_PMC2      | 32
+  PPC   | KVM_REG_PPC_PMC3      | 32
+  PPC   | KVM_REG_PPC_PMC4      | 32
+  PPC   | KVM_REG_PPC_PMC5      | 32
+  PPC   | KVM_REG_PPC_PMC6      | 32
+  PPC   | KVM_REG_PPC_PMC7      | 32
+  PPC   | KVM_REG_PPC_PMC8      | 32
 

4.69 KVM_GET_ONE_REG

arch/powerpc/include/asm/kvm.h +21 -0

index 3c14202..9557576 100644
@@ -338,5 +338,26 @@ struct kvm_book3e_206_tlb_params {
 #define KVM_REG_PPC_IAC4	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x5)
 #define KVM_REG_PPC_DAC1	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x6)
 #define KVM_REG_PPC_DAC2	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x7)
+#define KVM_REG_PPC_DABR	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x8)
+#define KVM_REG_PPC_DSCR	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x9)
+#define KVM_REG_PPC_PURR	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xa)
+#define KVM_REG_PPC_SPURR	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb)
+#define KVM_REG_PPC_DAR		(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xc)
+#define KVM_REG_PPC_DSISR	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xd)
+#define KVM_REG_PPC_AMR		(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xe)
+#define KVM_REG_PPC_UAMOR	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xf)
+
+#define KVM_REG_PPC_MMCR0	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x10)
+#define KVM_REG_PPC_MMCR1	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x11)
+#define KVM_REG_PPC_MMCRA	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x12)
+
+#define KVM_REG_PPC_PMC1	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x18)
+#define KVM_REG_PPC_PMC2	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x19)
+#define KVM_REG_PPC_PMC3	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x1a)
+#define KVM_REG_PPC_PMC4	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x1b)
+#define KVM_REG_PPC_PMC5	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x1c)
+#define KVM_REG_PPC_PMC6	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x1d)
+#define KVM_REG_PPC_PMC7	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x1e)
+#define KVM_REG_PPC_PMC8	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x1f)
 
 #endif /* __LINUX_KVM_POWERPC_H */

arch/powerpc/include/asm/kvm_ppc.h +0 -0

index 2c94cb3..6002b0a 100644

arch/powerpc/include/asm/kvm_ppc.h +4 -0

+};
+
 void kvmppc_core_get_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs);
 int kvmppc_core_set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs);
 

@@ -204,6 +209,8 @@ int kvmppc_set_sregs_ivor(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs);
 
 int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg);
 int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg);
+int kvmppc_get_one_reg(struct kvm_vcpu *vcpu, u64 id, union kvmppc_one_reg *);
+int kvmppc_set_one_reg(struct kvm_vcpu *vcpu, u64 id, union kvmppc_one_reg *);
 
 int kvm_vcpu_get_vpa_info(struct kvm_vcpu *vcpu, struct kvm_ppc_vpa *vpa);
 int kvm_vcpu_set_vpa_info(struct kvm_vcpu *vcpu, struct kvm_ppc_vpa *vpa);

arch/powerpc/kvm/book3s.c +0 -0

index e946665..6d1306c 100644

arch/powerpc/kvm/book3s.c +8 -0

+
+int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
+{
+	int r;
+	union kvmppc_one_reg val;
+	int size;
+
+	size = one_reg_size(reg->id);

arch/powerpc/include/asm/kvm_ppc.h static inline u32 kvmppc_set_field(u64 inst, int msb, int lsb, int value) · line 201

Quoted 3 lines · Paul Mackerras

arch/powerpc/include/asm/kvm_ppc.h +3 -0

+union kvmppc_one_reg {
+	u32	wval;
+	u64	dval;

Phew. Is this guaranteed to always pad on the right, rather than left?

arch/powerpc/kvm/book3s.c int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu) · line 492

Quoted 5 lines

arch/powerpc/kvm/book3s.c +5 -0

+static int one_reg_size(u64 id)
+{
+	id = (id & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT;
+	return 1 << id;
+}

Rusty has a patch to export this as a generic function for all kvm targets. Check out

http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/97553

I actually like static inlines better, but we really should keep this code on top level :).

arch/powerpc/kvm/book3s.c int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu) · line 510

Quoted 3 lines

arch/powerpc/kvm/book3s.c +3 -0

+		switch (reg->id) {
+		case KVM_REG_PPC_DAR:
+			val.dval = vcpu->arch.shared->dar;

Hmm. I would like to keep the amount of places who need to manually know about the size of a register as low as possible. We do know the size of the register already on in the REG id and in sizeof(register), right?

I think there will be valid scenarios for them to be of different size. For example when a register suddenly increases in size. But the assignment should always happen on the size the register id tells us.

So how about something like

#define kvmppc_set_reg(id, val, reg) { \
switch (one_reg_size(id)) { \
case 4: val.wval = reg; break; \
case 8: val.dval = reg; break; \
default: BUG(); \
} \
}

case KVM_REG_PPC_DAR:
kvmppc_set_reg(reg->id, &val, vcpu->arch.shared->dar); break;

Maybe you can come up with something even easier though :).

Btw, with such a scheme in place, we wouldn't even need the union. We could just reserve a char array and cast it in the setter/getter functions.

Quoted 8 lines · Alexander Graf

arch/powerpc/include/asm/kvm_ppc.h static inline u32 kvmppc_set_field(u64 inst, int msb, int lsb, int value) · line 201

Quoted 3 lines · Paul Mackerras

arch/powerpc/include/asm/kvm_ppc.h +3 -0

+union kvmppc_one_reg {
+	u32	wval;
+	u64	dval;

Phew. Is this guaranteed to always pad on the right, rather than left?

Absolutely (for big-endian targets). A union is basically a struct with all the members at offset 0. So we read N bytes into the union and then access the N-byte member.

Quoted 11 lines

arch/powerpc/kvm/book3s.c int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu) · line 492

Quoted 5 lines

arch/powerpc/kvm/book3s.c +5 -0

+static int one_reg_size(u64 id)
+{
+	id = (id & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT;
+	return 1 << id;
+}

Rusty has a patch to export this as a generic function for all kvm targets. Check out

http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/97553

I actually like static inlines better, but we really should keep this code on top level :).

Once Rusty's patch goes in I'd be happy to use it.

Quoted 5 lines

arch/powerpc/kvm/book3s.c int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu) · line 510

Quoted 3 lines

arch/powerpc/kvm/book3s.c +3 -0

+		switch (reg->id) {
+		case KVM_REG_PPC_DAR:
+			val.dval = vcpu->arch.shared->dar;

Hmm. I would like to keep the amount of places who need to manually know about the size of a register as low as possible. We do know the size of the register already on in the REG id and in sizeof(register), right?

This code doesn't "manually" know about the size of the register. It could be that vcpu->arch.shared->dar is 4 bytes, or 2 bytes, instead of 8 bytes, and that wouldn't affect this code. All this code "knows" is that the definition of KVM_REG_PPC_DAR includes the "8 byte" encoding. And since that definition is -- or will be -- part of the userspace ABI, it's not something that can be changed later, so I don't see any problem with just using val.dval here.

Quoted 2 lines

I think there will be valid scenarios for them to be of different size. For example when a register suddenly increases in size. But the assignment should always happen on the size the register id tells us.

It doesn't matter if a register changes in size, that doesn't affect this code. We may want to define an extra KVM_REG_PPC_* symbol if it increases in size, but that would be a different symbol to the ones we already have. As I said, we couldn't just arbitrarily change the existing symbol because that would break existing userspace programs.

In other words the assignment already does happen on the size the register id tells us, with my code.

Quoted 11 lines

So how about something like

#define kvmppc_set_reg(id, val, reg) { \
switch (one_reg_size(id)) { \
case 4: val.wval = reg; break; \
case 8: val.dval = reg; break; \
default: BUG(); \
} \
}

Quoted 5 lines

case KVM_REG_PPC_DAR:
kvmppc_set_reg(reg->id, &val, vcpu->arch.shared->dar); break;

Maybe you can come up with something even easier though :).

Seems unnecessarily complex to me. If we did something like that we should do

kvmppc_set_reg(KVM_REG_PPC_DAR, &val, vcpu->arch.shared->dar);

and use a macro instead of one_reg_size() to give the compiler a better chance to do constant folding and only compile in the branch of the switch statement that we need.

Quoted 2 lines

Btw, with such a scheme in place, we wouldn't even need the union. We could just reserve a char array and cast it in the setter/getter functions.

I'm always wary of accessing a variable of one type as a different type using a cast on its address, because of the C99 type-punning rules. It's probably OK with a char array, but I think a union is cleaner, because you're explicitly stating that you want some storage that can be accessed as different types.

So how about something like

#define kvmppc_set_reg(id, val, reg) { \
switch (one_reg_size(id)) { \
case 4: val.wval = reg; break; \
case 8: val.dval = reg; break; \
default: BUG(); \
} \
}

case KVM_REG_PPC_DAR:
kvmppc_set_reg(reg->id, &val, vcpu->arch.shared->dar); break;

Quoted 10 lines · Alexander Graf

So how about something like

#define kvmppc_set_reg(id, val, reg) { \
switch (one_reg_size(id)) { \
case 4: val.wval = reg; break; \
case 8: val.dval = reg; break; \
default: BUG(); \
} \
}

Quoted 3 lines

case KVM_REG_PPC_DAR:
kvmppc_set_reg(reg->id, &val, vcpu->arch.shared->dar); break;

I tried that and it was fine for the wval vs. dval thing, and gcc even compiled it to the same number of instructions as what I had before. But it breaks down when you get to VMX and VSX -- firstly because there are two different types that are both 16 bytes, and secondly because when you do this:

#define kvmppc_get_reg(id, val, reg) { \
switch (one_reg_size(id)) { \
case 4: val.wval = reg; break; \
case 8: val.dval = reg; break; \
case 16: val.vval = reg; break; \
default: BUG(); \
} \
}

you get compile errors on things like:

kvmppc_set_reg(reg->id, val, vcpu->arch.shared->dar);

because val.vval is a vector128, and vcpu->arch.shared->dar is a u64, and you can't assign a u64 to a vector128 since vector128 is a struct. In other words, all of the cases of the switch have to satisfy the type rules even though only one of them will ever be used. Basically that trick will only work for integer types, and we don't have 128 bit integer support in gcc.

Given all that, I would like to see my patches go in while we continue to search for a way to avoid the potential mistakes you're talking about.