| Message ID | 20120912001922.GG32642@bloggs.ozlabs.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12.09.2012, at 02:19, Paul Mackerras wrote: > Currently the KVM_GET_FPU and KVM_SET_FPU ioctls return an EOPNOTSUPP > error on powerpc. This implements them for Book 3S processors. Since > those processors have more than just the 32 basic floating-point > registers, this extends the kvm_fpu structure to have space for the > additional registers -- the 32 vector registers (128 bits each) for > VMX/Altivec and the 32 additional 64-bit registers that were added > on POWER7 for the vector-scalar extension (VSX). It also adds a > `valid' field, which is a bitmap indicating which elements contain > valid data. > > The layout of the floating-point register data in the vcpu struct is > mostly the same between different flavors of KVM on Book 3S processors, > but the set of supported registers may differ depending on what the > CPU hardware supports and how much is emulated. Therefore we have > a flavor-specific function to work out which set of registers to > supply for the get function. > > On POWER7 processors using the Book 3S HV flavor of KVM, we save the > standard floating-point registers together with their corresponding > VSX extension register in the vcpu->arch.vsr[] array, since each > pair can be loaded or stored with one instruction. This is different > to other flavors of KVM, and to other processors (i.e. PPC970) with > HV KVM, which store the standard FPRs in vcpu->arch.fpr[]. To cope > with this, we use the kvmppc_core_get_fpu_valid() and > kvmppc_core_set_fpu_valid() functions to sync between the arch.fpr[] > and arch.vsr[] arrays as needed. > > Signed-off-by: Paul Mackerras <paulus@samba.org> Any reason to not use ONE_REG here? 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 14, 2012 at 01:30:51AM +0200, Alexander Graf wrote: > > On 12.09.2012, at 02:19, Paul Mackerras wrote: > > > Currently the KVM_GET_FPU and KVM_SET_FPU ioctls return an EOPNOTSUPP > > error on powerpc. This implements them for Book 3S processors. Since > > those processors have more than just the 32 basic floating-point > > registers, this extends the kvm_fpu structure to have space for the > > additional registers -- the 32 vector registers (128 bits each) for > > VMX/Altivec and the 32 additional 64-bit registers that were added > > on POWER7 for the vector-scalar extension (VSX). It also adds a > > `valid' field, which is a bitmap indicating which elements contain > > valid data. > > > > The layout of the floating-point register data in the vcpu struct is > > mostly the same between different flavors of KVM on Book 3S processors, > > but the set of supported registers may differ depending on what the > > CPU hardware supports and how much is emulated. Therefore we have > > a flavor-specific function to work out which set of registers to > > supply for the get function. > > > > On POWER7 processors using the Book 3S HV flavor of KVM, we save the > > standard floating-point registers together with their corresponding > > VSX extension register in the vcpu->arch.vsr[] array, since each > > pair can be loaded or stored with one instruction. This is different > > to other flavors of KVM, and to other processors (i.e. PPC970) with > > HV KVM, which store the standard FPRs in vcpu->arch.fpr[]. To cope > > with this, we use the kvmppc_core_get_fpu_valid() and > > kvmppc_core_set_fpu_valid() functions to sync between the arch.fpr[] > > and arch.vsr[] arrays as needed. > > > > Signed-off-by: Paul Mackerras <paulus@samba.org> > > Any reason to not use ONE_REG here? Just consistency with x86 -- they have an xmm[][] field in their struct kvm_fpu which looks like it contains their vector state. 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 14.09.2012, at 01:58, Paul Mackerras wrote: > On Fri, Sep 14, 2012 at 01:30:51AM +0200, Alexander Graf wrote: >> >> On 12.09.2012, at 02:19, Paul Mackerras wrote: >> >>> Currently the KVM_GET_FPU and KVM_SET_FPU ioctls return an EOPNOTSUPP >>> error on powerpc. This implements them for Book 3S processors. Since >>> those processors have more than just the 32 basic floating-point >>> registers, this extends the kvm_fpu structure to have space for the >>> additional registers -- the 32 vector registers (128 bits each) for >>> VMX/Altivec and the 32 additional 64-bit registers that were added >>> on POWER7 for the vector-scalar extension (VSX). It also adds a >>> `valid' field, which is a bitmap indicating which elements contain >>> valid data. >>> >>> The layout of the floating-point register data in the vcpu struct is >>> mostly the same between different flavors of KVM on Book 3S processors, >>> but the set of supported registers may differ depending on what the >>> CPU hardware supports and how much is emulated. Therefore we have >>> a flavor-specific function to work out which set of registers to >>> supply for the get function. >>> >>> On POWER7 processors using the Book 3S HV flavor of KVM, we save the >>> standard floating-point registers together with their corresponding >>> VSX extension register in the vcpu->arch.vsr[] array, since each >>> pair can be loaded or stored with one instruction. This is different >>> to other flavors of KVM, and to other processors (i.e. PPC970) with >>> HV KVM, which store the standard FPRs in vcpu->arch.fpr[]. To cope >>> with this, we use the kvmppc_core_get_fpu_valid() and >>> kvmppc_core_set_fpu_valid() functions to sync between the arch.fpr[] >>> and arch.vsr[] arrays as needed. >>> >>> Signed-off-by: Paul Mackerras <paulus@samba.org> >> >> Any reason to not use ONE_REG here? > > Just consistency with x86 -- they have an xmm[][] field in their > struct kvm_fpu which looks like it contains their vector state. Yup, Considering how different the FPU state on differnet ppc cores is, I'd be more happy with shoving it into something that allows for more dynamic control. Otherwise we'd end up with yet another struct sregs that can contain SPE registers, altivec, and a dozen additions to it :). Please just use one_reg for all of the register synchronization you want to add, unless there's a compelling reason to do it differently. It will make our live a lot easier in the future. If we need to transfer too much data and actually run into performance trouble, we can always add a GET_MANY_REG ioctl. 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 14, 2012 at 02:03:15AM +0200, Alexander Graf wrote: > > Yup, Considering how different the FPU state on differnet ppc cores is, I'd be more happy with shoving it into something that allows for more dynamic control. Otherwise we'd end up with yet another struct sregs that can contain SPE registers, altivec, and a dozen additions to it :). > > Please just use one_reg for all of the register synchronization you want to add, unless there's a compelling reason to do it differently. It will make our live a lot easier in the future. If we need to transfer too much data and actually run into performance trouble, we can always add a GET_MANY_REG ioctl. It just seems perverse to ignore the existing interface that every other architecture uses, and instead do something unique that is actually slower, but whatever... 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 14.09.2012, at 03:36, Paul Mackerras wrote: > On Fri, Sep 14, 2012 at 02:03:15AM +0200, Alexander Graf wrote: >> >> Yup, Considering how different the FPU state on differnet ppc cores is, I'd be more happy with shoving it into something that allows for more dynamic control. Otherwise we'd end up with yet another struct sregs that can contain SPE registers, altivec, and a dozen additions to it :). >> >> Please just use one_reg for all of the register synchronization you want to add, unless there's a compelling reason to do it differently. It will make our live a lot easier in the future. If we need to transfer too much data and actually run into performance trouble, we can always add a GET_MANY_REG ioctl. > > It just seems perverse to ignore the existing interface that every > other architecture uses, and instead do something unique that is > actually slower, but whatever... We're slowly moving towards ONE_REG. ARM is already going full steam ahead and I'd like to have every new register in PPC be modeled with it as well. The old interface broke on us one time too often now :). As I said, if we run into performance problems, we will implement ways to improve performance. At the end of the day, x86 will be the odd one out. 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 14.09.2012, at 03:44, Alexander Graf wrote: > > On 14.09.2012, at 03:36, Paul Mackerras wrote: > >> On Fri, Sep 14, 2012 at 02:03:15AM +0200, Alexander Graf wrote: >>> >>> Yup, Considering how different the FPU state on differnet ppc cores is, I'd be more happy with shoving it into something that allows for more dynamic control. Otherwise we'd end up with yet another struct sregs that can contain SPE registers, altivec, and a dozen additions to it :). >>> >>> Please just use one_reg for all of the register synchronization you want to add, unless there's a compelling reason to do it differently. It will make our live a lot easier in the future. If we need to transfer too much data and actually run into performance trouble, we can always add a GET_MANY_REG ioctl. >> >> It just seems perverse to ignore the existing interface that every >> other architecture uses, and instead do something unique that is >> actually slower, but whatever... > > We're slowly moving towards ONE_REG. ARM is already going full steam ahead and I'd like to have every new register in PPC be modeled with it as well. The old interface broke on us one time too often now :). > > As I said, if we run into performance problems, we will implement ways to improve performance. At the end of the day, x86 will be the odd one out. (plus your patch breaks abi compatibility with old user space) 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 14, 2012 at 03:50:06AM +0200, Alexander Graf wrote:
> (plus your patch breaks abi compatibility with old user space)
Well, only to the extent that the old ioctl code would now return an
ENOTTY error rather than an EOPNOTSUPP error. Note that the ioctl
code has the struct size embedded, so it's not like a program compiled
against the old definition would suddenly get more data than it had
made room for.
But since you insist, I'll hack up the ONE_REG interface and see what
it looks like...
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 Fri, 2012-09-14 at 03:44 +0200, Alexander Graf wrote: > > We're slowly moving towards ONE_REG. ARM is already going full steam > ahead and I'd like to have every new register in PPC be modeled with > it as well. The old interface broke on us one time too often now :). > > As I said, if we run into performance problems, we will implement ways > to improve performance. At the end of the day, x86 will be the odd one > out. This is totally insane. In most cases, we care about the whole set of 32 registers. Doing 32 ioctl's for that is just plain stupid. There is nothing wrong in allowing the "one reg" interface as well, but it's totally ridiculous to *require* it. Ben. -- 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 19.09.2012, at 05:37, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > On Fri, 2012-09-14 at 03:44 +0200, Alexander Graf wrote: >> >> We're slowly moving towards ONE_REG. ARM is already going full steam >> ahead and I'd like to have every new register in PPC be modeled with >> it as well. The old interface broke on us one time too often now :). >> >> As I said, if we run into performance problems, we will implement ways >> to improve performance. At the end of the day, x86 will be the odd one >> out. > > This is totally insane. In most cases, we care about the whole set of 32 > registers. Doing 32 ioctl's for that is just plain stupid. No. In most cases we care about ~100 registers that we want to synchronize. Doing 10 ioctls for that is insane. I want to move to a model where we can have only a single ioctl for all of the 100 regs. But the first step towards that direction is to split them into individual ioctls with explicit IDs for each, so we can then introduce sn ioctl that can write an arbitrary number of regs. The ioctl to read/write an array of registers is dead simple. Just ask Rusty - he already has a prototype in a branch :). Also, don't only think about QEMU here. If you want to write a different user space for KVM, you might not want to keep a copy of the guest CPU's state in some struct. Instead, you could on every register access from user space simply call the one_reg API and leave all stare to kvm. Either way, we only synchronize these registers quite seldomly. We do it when you check out registers on the monitor, when we live migrate, or when we have some odd hypercall that requires knowledge of more guest state than it should (vmport). But this really is not a fast path today. Alex > > There is nothing wrong in allowing the "one reg" interface as well, but > it's totally ridiculous to *require* it. > > Ben. > > -- 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/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/asm/kvm.h index 9557576..5792024 100644 --- a/arch/powerpc/include/asm/kvm.h +++ b/arch/powerpc/include/asm/kvm.h @@ -261,9 +261,20 @@ struct kvm_sregs { }; struct kvm_fpu { + __u64 valid; + __u64 fpscr; __u64 fpr[32]; + __vector128 vr[32]; + __u32 _pad[3]; + __u32 vscr; + __u64 vsr[32]; }; +/* Values for kvm_fpu valid field */ +#define KVM_FPU_FPREGS 1 /* standard floating-point regs & FPSCR */ +#define KVM_FPU_VREGS 2 /* VMX/Altivec vector regs & VSCR */ +#define KVM_FPU_VSREGS 4 /* POWER7 vector/scalar additional regs */ + struct kvm_debug_exit_arch { }; diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h index c06a64b..5dccdc5 100644 --- a/arch/powerpc/include/asm/kvm_ppc.h +++ b/arch/powerpc/include/asm/kvm_ppc.h @@ -96,6 +96,8 @@ extern int kvmppc_core_vcpu_translate(struct kvm_vcpu *vcpu, extern void kvmppc_core_vcpu_load(struct kvm_vcpu *vcpu, int cpu); extern void kvmppc_core_vcpu_put(struct kvm_vcpu *vcpu); +extern u64 kvmppc_core_get_fpu_valid(struct kvm_vcpu *vcpu); +extern void kvmppc_core_set_fpu_valid(struct kvm_vcpu *vcpu, u64 valid); extern int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu); extern int kvmppc_core_pending_dec(struct kvm_vcpu *vcpu); diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c index e946665..c2278f6 100644 --- a/arch/powerpc/kvm/book3s.c +++ b/arch/powerpc/kvm/book3s.c @@ -477,12 +477,57 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs) int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu) { - return -ENOTSUPP; + u64 valid = kvmppc_core_get_fpu_valid(vcpu); + + /* avoid leaking information to userspace */ + memset(fpu, 0, sizeof(*fpu)); + + if (valid & KVM_FPU_FPREGS) { + memcpy(fpu->fpr, vcpu->arch.fpr, sizeof(fpu->fpr)); + fpu->fpscr = vcpu->arch.fpscr; + } +#ifdef CONFIG_ALTIVEC + if (valid & KVM_FPU_VREGS) { + memcpy(fpu->vr, vcpu->arch.vr, sizeof(fpu->vr)); + fpu->vscr = vcpu->arch.vscr.u[3]; + } +#endif +#ifdef CONFIG_VSX + if (valid & KVM_FPU_VSREGS) { + long i; + + for (i = 0; i < ARRAY_SIZE(fpu->vsr); ++i) + fpu->vsr[i] = vcpu->arch.vsr[2*i + 1]; + } +#endif + fpu->valid = valid; + return 0; } int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu) { - return -ENOTSUPP; + u64 valid = fpu->valid; + + if (valid & KVM_FPU_FPREGS) { + memcpy(vcpu->arch.fpr, fpu->fpr, sizeof(vcpu->arch.fpr)); + vcpu->arch.fpscr = fpu->fpscr; + } +#ifdef CONFIG_ALTIVEC + if (valid & KVM_FPU_VREGS) { + memcpy(vcpu->arch.vr, fpu->vr, sizeof(vcpu->arch.vr)); + vcpu->arch.vscr.u[3] = fpu->vscr; + } +#endif +#ifdef CONFIG_VSX + if (valid & KVM_FPU_VSREGS) { + long i; + + for (i = 0; i < ARRAY_SIZE(fpu->vsr); ++i) + vcpu->arch.vsr[2*i + 1] = fpu->vsr[i]; + } +#endif + kvmppc_core_set_fpu_valid(vcpu, valid); + return 0; } int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu, diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c index 7fe5c9a..2c2b6b9 100644 --- a/arch/powerpc/kvm/book3s_hv.c +++ b/arch/powerpc/kvm/book3s_hv.c @@ -677,6 +677,38 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg) return r; } +u64 kvmppc_core_get_fpu_valid(struct kvm_vcpu *vcpu) +{ + u64 valid = KVM_FPU_FPREGS; + +#ifdef CONFIG_ALTIVEC + if (cpu_has_feature(CPU_FTR_ALTIVEC)) + valid |= KVM_FPU_VREGS; +#endif +#ifdef CONFIG_VSX + if (cpu_has_feature(CPU_FTR_VSX)) { + long i; + + for (i = 0; i < ARRAY_SIZE(vcpu->arch.fpr); ++i) + vcpu->arch.fpr[i] = vcpu->arch.vsr[2*i]; + valid |= KVM_FPU_VSREGS; + } +#endif + return valid; +} + +void kvmppc_core_set_fpu_valid(struct kvm_vcpu *vcpu, u64 valid) +{ +#ifdef CONFIG_VSX + if ((valid & KVM_FPU_FPREGS) && cpu_has_feature(CPU_FTR_VSX)) { + long i; + + for (i = 0; i < ARRAY_SIZE(vcpu->arch.fpr); ++i) + vcpu->arch.vsr[2*i] = vcpu->arch.fpr[i]; + } +#endif +} + int kvmppc_core_check_processor_compat(void) { if (cpu_has_feature(CPU_FTR_HVMODE)) diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c index b3c584f..e3b81a2 100644 --- a/arch/powerpc/kvm/book3s_pr.c +++ b/arch/powerpc/kvm/book3s_pr.c @@ -978,6 +978,25 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg) return r; } +u64 kvmppc_core_get_fpu_valid(struct kvm_vcpu *vcpu) +{ + u64 valid = KVM_FPU_FPREGS; + +#ifdef CONFIG_ALTIVEC + if (cpu_has_feature(CPU_FTR_ALTIVEC)) + valid |= KVM_FPU_VREGS; +#endif +#ifdef CONFIG_VSX + if (cpu_has_feature(CPU_FTR_VSX)) + valid |= KVM_FPU_VSREGS; +#endif + return valid; +} + +void kvmppc_core_set_fpu_valid(struct kvm_vcpu *vcpu, u64 valid) +{ +} + int kvmppc_core_check_processor_compat(void) { return 0;
Currently the KVM_GET_FPU and KVM_SET_FPU ioctls return an EOPNOTSUPP error on powerpc. This implements them for Book 3S processors. Since those processors have more than just the 32 basic floating-point registers, this extends the kvm_fpu structure to have space for the additional registers -- the 32 vector registers (128 bits each) for VMX/Altivec and the 32 additional 64-bit registers that were added on POWER7 for the vector-scalar extension (VSX). It also adds a `valid' field, which is a bitmap indicating which elements contain valid data. The layout of the floating-point register data in the vcpu struct is mostly the same between different flavors of KVM on Book 3S processors, but the set of supported registers may differ depending on what the CPU hardware supports and how much is emulated. Therefore we have a flavor-specific function to work out which set of registers to supply for the get function. On POWER7 processors using the Book 3S HV flavor of KVM, we save the standard floating-point registers together with their corresponding VSX extension register in the vcpu->arch.vsr[] array, since each pair can be loaded or stored with one instruction. This is different to other flavors of KVM, and to other processors (i.e. PPC970) with HV KVM, which store the standard FPRs in vcpu->arch.fpr[]. To cope with this, we use the kvmppc_core_get_fpu_valid() and kvmppc_core_set_fpu_valid() functions to sync between the arch.fpr[] and arch.vsr[] arrays as needed. Signed-off-by: Paul Mackerras <paulus@samba.org> --- arch/powerpc/include/asm/kvm.h | 11 ++++++++ arch/powerpc/include/asm/kvm_ppc.h | 2 ++ arch/powerpc/kvm/book3s.c | 49 ++++++++++++++++++++++++++++++++++-- arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++++ arch/powerpc/kvm/book3s_pr.c | 19 ++++++++++++++ 5 files changed, 111 insertions(+), 2 deletions(-)