| Message ID | 20120921054434.GL15685@drongo (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 21.09.2012, at 07:44, Paul Mackerras wrote: > 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 <paulus@samba.org> > --- > 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(-) > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > index 76a07a6..e4a2067 100644 > --- a/Documentation/virtual/kvm/api.txt > +++ b/Documentation/virtual/kvm/api.txt > @@ -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 > > diff --git a/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/asm/kvm.h > index 3c14202..9557576 100644 > --- a/arch/powerpc/include/asm/kvm.h > +++ b/arch/powerpc/include/asm/kvm.h > @@ -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 */ > diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h > index 2c94cb3..6002b0a 100644 > --- a/arch/powerpc/include/asm/kvm_ppc.h > +++ b/arch/powerpc/include/asm/kvm_ppc.h > @@ -196,6 +196,11 @@ static inline u32 kvmppc_set_field(u64 inst, int msb, int lsb, int value) > return r; > } > > +union kvmppc_one_reg { > + u32 wval; > + u64 dval; Phew. Is this guaranteed to always pad on the right, rather than left? > +}; > + > 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); > diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c > index e946665..6d1306c 100644 > --- a/arch/powerpc/kvm/book3s.c > +++ b/arch/powerpc/kvm/book3s.c > @@ -485,6 +485,80 @@ int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu) > return -ENOTSUPP; > } > > +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 :). > + > +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); > + if (size > sizeof(val)) > + return -EINVAL; > + > + r = kvmppc_get_one_reg(vcpu, reg->id, &val); > + > + if (r == -EINVAL) { > + r = 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. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 21, 2012 at 11:15:51AM +0200, Alexander Graf wrote: > > On 21.09.2012, at 07:44, Paul Mackerras wrote: > > > +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. > > +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. > > + 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. > 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. > 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 :). 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. > 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. Paul. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21.09.2012, at 11:52, Paul Mackerras wrote: > On Fri, Sep 21, 2012 at 11:15:51AM +0200, Alexander Graf wrote: >> >> On 21.09.2012, at 07:44, Paul Mackerras wrote: >> >>> +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. > >>> +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. > >>> + 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. It's not really a problem, it's really about redundancy. We write the same thing twice case <id that encodes length>: val.<length dependent value> = foo; And every time we do constructs like that, someone will one day come in and do fun like case <id with length 8>: val.<length 4> = foo; So if we can get rid of that complete bug category from day 1, it makes me happy :). > >> 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. Imagine we get new awesome 128bit GPRs now. The lower 64bit are still the same as they always were. The new machine is also backwards compatible, so we could be running 64bit code. To transfer the bigger registers we could then do case KVM_REG_PPC_GPR1: case KVM_REG_PPC_GPR1_128: kvmppc_set_reg(id, val, foo); But the bit I care a lot more about is the redundancy as explained above. > >> 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 :). > > 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. What variable type would the 3rd argument be? Also, constant folding should still work with one_reg_size(), no? > >> 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. If unions are guaranteed to be laid out the way you describe above, that's perfectly fine for me. I just remember nightmares when I had to convert x86 incomplete unions (the 2 members were of different size) to ppc, where they started at the reverse end. A scheme that isn't tied to BE would maybe allow us to share code with ARM and x86. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 21, 2012 at 11:15:51AM +0200, Alexander Graf wrote: > 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; 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. Paul. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24.09.2012, at 14:16, Paul Mackerras wrote: > On Fri, Sep 21, 2012 at 11:15:51AM +0200, Alexander Graf wrote: > >> 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; > > 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. What a shame. Could you please repost a version that only handles 32/64 setting with the above helper then and leaves 128-bit the way they are implemented now? > 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. ... that way we can at least make the "common case" of 32-bit and 64-bit registers error proof and only have to worry more about double-checking the 128-bit ones, which are a lot less. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt index 76a07a6..e4a2067 100644 --- a/Documentation/virtual/kvm/api.txt +++ b/Documentation/virtual/kvm/api.txt @@ -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 diff --git a/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/asm/kvm.h index 3c14202..9557576 100644 --- a/arch/powerpc/include/asm/kvm.h +++ b/arch/powerpc/include/asm/kvm.h @@ -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 */ diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h index 2c94cb3..6002b0a 100644 --- a/arch/powerpc/include/asm/kvm_ppc.h +++ b/arch/powerpc/include/asm/kvm_ppc.h @@ -196,6 +196,11 @@ static inline u32 kvmppc_set_field(u64 inst, int msb, int lsb, int value) return r; } +union kvmppc_one_reg { + u32 wval; + u64 dval; +}; + 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); diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c index e946665..6d1306c 100644 --- a/arch/powerpc/kvm/book3s.c +++ b/arch/powerpc/kvm/book3s.c @@ -485,6 +485,80 @@ int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu) return -ENOTSUPP; } +static int one_reg_size(u64 id) +{ + id = (id & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT; + return 1 << id; +} + +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); + if (size > sizeof(val)) + return -EINVAL; + + r = kvmppc_get_one_reg(vcpu, reg->id, &val); + + if (r == -EINVAL) { + r = 0; + switch (reg->id) { + case KVM_REG_PPC_DAR: + val.dval = vcpu->arch.shared->dar; + break; + case KVM_REG_PPC_DSISR: + val.wval = vcpu->arch.shared->dsisr; + break; + default: + r = -EINVAL; + break; + } + } + if (r) + return r; + + if (copy_to_user((char __user *)(unsigned long)reg->addr, &val, size)) + r = -EFAULT; + + return r; +} + +int kvm_vcpu_ioctl_set_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); + if (size > sizeof(val)) + return -EINVAL; + + if (copy_from_user(&val, (char __user *)(unsigned long)reg->addr, size)) + return -EFAULT; + + r = kvmppc_set_one_reg(vcpu, reg->id, &val); + + if (r == -EINVAL) { + r = 0; + switch (reg->id) { + case KVM_REG_PPC_DAR: + vcpu->arch.shared->dar = val.dval; + break; + case KVM_REG_PPC_DSISR: + vcpu->arch.shared->dsisr = val.wval; + break; + default: + r = -EINVAL; + break; + } + } + + return r; +} + int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu, struct kvm_translation *tr) { diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c index f953f73..42e30cd 100644 --- a/arch/powerpc/kvm/book3s_hv.c +++ b/arch/powerpc/kvm/book3s_hv.c @@ -683,36 +683,88 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, return 0; } -int kvm_vcpu_ioctl_get_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 *val) { - int r = -EINVAL; + int r = 0; - switch (reg->id) { + switch (id) { case KVM_REG_PPC_HIOR: - r = put_user(0, (u64 __user *)reg->addr); + val->dval = 0; + break; + case KVM_REG_PPC_DABR: + val->dval = vcpu->arch.dabr; + break; + case KVM_REG_PPC_DSCR: + val->dval = vcpu->arch.dscr; + break; + case KVM_REG_PPC_PURR: + val->dval = vcpu->arch.purr; + break; + case KVM_REG_PPC_SPURR: + val->dval = vcpu->arch.spurr; + break; + case KVM_REG_PPC_AMR: + val->dval = vcpu->arch.amr; + break; + case KVM_REG_PPC_UAMOR: + val->dval = vcpu->arch.uamor; + break; + case KVM_REG_PPC_MMCR0 ... KVM_REG_PPC_MMCRA: + val->dval = vcpu->arch.mmcr[id - KVM_REG_PPC_MMCR0]; + break; + case KVM_REG_PPC_PMC1 ... KVM_REG_PPC_PMC8: + val->wval = vcpu->arch.pmc[id - KVM_REG_PPC_PMC1]; break; default: + r = -EINVAL; break; } return r; } -int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg) +int kvmppc_set_one_reg(struct kvm_vcpu *vcpu, u64 id, union kvmppc_one_reg *val) { - int r = -EINVAL; + int r = 0; - switch (reg->id) { + switch (id) { case KVM_REG_PPC_HIOR: - { - u64 hior; /* Only allow this to be set to zero */ - r = get_user(hior, (u64 __user *)reg->addr); - if (!r && (hior != 0)) + if (val->dval) r = -EINVAL; break; - } + case KVM_REG_PPC_DABR: + vcpu->arch.dabr = val->dval; + break; + case KVM_REG_PPC_DSCR: + vcpu->arch.dscr = val->dval; + break; + case KVM_REG_PPC_PURR: + vcpu->arch.purr = val->dval; + break; + case KVM_REG_PPC_SPURR: + vcpu->arch.spurr = val->dval; + break; + case KVM_REG_PPC_DAR: + vcpu->arch.shregs.dar = val->dval; + break; + case KVM_REG_PPC_DSISR: + vcpu->arch.shregs.dsisr = val->wval; + break; + case KVM_REG_PPC_AMR: + vcpu->arch.amr = val->dval; + break; + case KVM_REG_PPC_UAMOR: + vcpu->arch.uamor = val->dval; + break; + case KVM_REG_PPC_MMCR0 ... KVM_REG_PPC_MMCRA: + vcpu->arch.mmcr[id - KVM_REG_PPC_MMCR0] = val->dval; + break; + case KVM_REG_PPC_PMC1 ... KVM_REG_PPC_PMC8: + vcpu->arch.pmc[id - KVM_REG_PPC_PMC1] = val->wval; + break; default: + r = -EINVAL; break; } diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c index bf3ec5d..c1004d8 100644 --- a/arch/powerpc/kvm/book3s_pr.c +++ b/arch/powerpc/kvm/book3s_pr.c @@ -945,34 +945,33 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, return 0; } -int kvm_vcpu_ioctl_get_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 *val) { - int r = -EINVAL; + int r = 0; - switch (reg->id) { + switch (id) { case KVM_REG_PPC_HIOR: - r = copy_to_user((u64 __user *)(long)reg->addr, - &to_book3s(vcpu)->hior, sizeof(u64)); + val->dval = to_book3s(vcpu)->hior; break; default: + r = -EINVAL; break; } return r; } -int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg) +int kvmppc_set_one_reg(struct kvm_vcpu *vcpu, u64 id, union kvmppc_one_reg *val) { - int r = -EINVAL; + int r = 0; - switch (reg->id) { + switch (id) { case KVM_REG_PPC_HIOR: - r = copy_from_user(&to_book3s(vcpu)->hior, - (u64 __user *)(long)reg->addr, sizeof(u64)); - if (!r) - to_book3s(vcpu)->hior_explicit = true; + to_book3s(vcpu)->hior = val->dval; + to_book3s(vcpu)->hior_explicit = true; break; default: + r = -EINVAL; break; }
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 <paulus@samba.org> --- 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(-)