| Message ID | 1401453776-55285-34-git-send-email-agraf@suse.de |
|---|---|
| State | New, archived |
| Headers | show |
Il 30/05/2014 14:42, Alexander Graf ha scritto: > From: Paul Mackerras <paulus@samba.org> > > Commit b005255e12a3 ("KVM: PPC: Book3S HV: Context-switch new POWER8 > SPRs") added a definition of KVM_REG_PPC_WORT with the same register > number as the existing KVM_REG_PPC_VRSAVE (though in fact the > definitions are not identical because of the different register sizes.) > > For clarity, this moves KVM_REG_PPC_WORT to the next unused number, > and also adds it to api.txt. > > Signed-off-by: Paul Mackerras <paulus@samba.org> > Signed-off-by: Alexander Graf <agraf@suse.de> > --- > Documentation/virtual/kvm/api.txt | 1 + > arch/powerpc/include/uapi/asm/kvm.h | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > index 9a95770..6b30290 100644 > --- a/Documentation/virtual/kvm/api.txt > +++ b/Documentation/virtual/kvm/api.txt > @@ -1873,6 +1873,7 @@ registers, find a list below: > PPC | KVM_REG_PPC_PPR | 64 > PPC | KVM_REG_PPC_ARCH_COMPAT 32 > PPC | KVM_REG_PPC_DABRX | 32 > + PPC | KVM_REG_PPC_WORT | 64 > PPC | KVM_REG_PPC_TM_GPR0 | 64 > ... > PPC | KVM_REG_PPC_TM_GPR31 | 64 > diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h > index a6665be..2bc4a94 100644 > --- a/arch/powerpc/include/uapi/asm/kvm.h > +++ b/arch/powerpc/include/uapi/asm/kvm.h > @@ -545,7 +545,6 @@ struct kvm_get_htab_header { > #define KVM_REG_PPC_TCSCR (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb1) > #define KVM_REG_PPC_PID (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb2) > #define KVM_REG_PPC_ACOP (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb3) > -#define KVM_REG_PPC_WORT (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb4) > > #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) > @@ -555,6 +554,7 @@ struct kvm_get_htab_header { > #define KVM_REG_PPC_ARCH_COMPAT (KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xb7) > > #define KVM_REG_PPC_DABRX (KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xb8) > +#define KVM_REG_PPC_WORT (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb9) This is an ABI break, this symbol was added in 3.14. I think I should revert this. Can you convince me otherwise? Paolo -- 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
On 30.05.14 17:50, Paolo Bonzini wrote: > Il 30/05/2014 14:42, Alexander Graf ha scritto: >> From: Paul Mackerras <paulus@samba.org> >> >> Commit b005255e12a3 ("KVM: PPC: Book3S HV: Context-switch new POWER8 >> SPRs") added a definition of KVM_REG_PPC_WORT with the same register >> number as the existing KVM_REG_PPC_VRSAVE (though in fact the >> definitions are not identical because of the different register sizes.) >> >> For clarity, this moves KVM_REG_PPC_WORT to the next unused number, >> and also adds it to api.txt. >> >> Signed-off-by: Paul Mackerras <paulus@samba.org> >> Signed-off-by: Alexander Graf <agraf@suse.de> >> --- >> Documentation/virtual/kvm/api.txt | 1 + >> arch/powerpc/include/uapi/asm/kvm.h | 2 +- >> 2 files changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/virtual/kvm/api.txt >> b/Documentation/virtual/kvm/api.txt >> index 9a95770..6b30290 100644 >> --- a/Documentation/virtual/kvm/api.txt >> +++ b/Documentation/virtual/kvm/api.txt >> @@ -1873,6 +1873,7 @@ registers, find a list below: >> PPC | KVM_REG_PPC_PPR | 64 >> PPC | KVM_REG_PPC_ARCH_COMPAT 32 >> PPC | KVM_REG_PPC_DABRX | 32 >> + PPC | KVM_REG_PPC_WORT | 64 >> PPC | KVM_REG_PPC_TM_GPR0 | 64 >> ... >> PPC | KVM_REG_PPC_TM_GPR31 | 64 >> diff --git a/arch/powerpc/include/uapi/asm/kvm.h >> b/arch/powerpc/include/uapi/asm/kvm.h >> index a6665be..2bc4a94 100644 >> --- a/arch/powerpc/include/uapi/asm/kvm.h >> +++ b/arch/powerpc/include/uapi/asm/kvm.h >> @@ -545,7 +545,6 @@ struct kvm_get_htab_header { >> #define KVM_REG_PPC_TCSCR (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb1) >> #define KVM_REG_PPC_PID (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb2) >> #define KVM_REG_PPC_ACOP (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb3) >> -#define KVM_REG_PPC_WORT (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb4) >> >> #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) >> @@ -555,6 +554,7 @@ struct kvm_get_htab_header { >> #define KVM_REG_PPC_ARCH_COMPAT (KVM_REG_PPC | KVM_REG_SIZE_U32 | >> 0xb7) >> >> #define KVM_REG_PPC_DABRX (KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xb8) >> +#define KVM_REG_PPC_WORT (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb9) > > This is an ABI break, this symbol was added in 3.14. I think I should > revert this. Can you convince me otherwise? There's nothing bad happening with the change. Newer user space won't be able to read WORT on older kernels, but there were more things broken that just WORT for POWER8 support there ;). And user space build with new headers running on an old kernel won't find the register, which is OK. I couldn't find any combination where it's really a problem. 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
Il 30/05/2014 17:53, Alexander Graf ha scritto: >> This is an ABI break, this symbol was added in 3.14. I think I should >> revert this. Can you convince me otherwise? > > There's nothing bad happening with the change. Newer user space won't be > able to read WORT on older kernels, but there were more things broken > that just WORT for POWER8 support there ;). > > And user space build with new headers running on an old kernel won't > find the register, which is OK. Would new userspace with old kernel be able to detect that POWER8 support isn't quite complete? Paolo -- 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
On 30.05.14 17:55, Paolo Bonzini wrote: > Il 30/05/2014 17:53, Alexander Graf ha scritto: >>> This is an ABI break, this symbol was added in 3.14. I think I should >>> revert this. Can you convince me otherwise? >> >> There's nothing bad happening with the change. Newer user space won't be >> able to read WORT on older kernels, but there were more things broken >> that just WORT for POWER8 support there ;). >> >> And user space build with new headers running on an old kernel won't >> find the register, which is OK. > > Would new userspace with old kernel be able to detect that POWER8 > support isn't quite complete? It couldn't, no. It would try to run a guest - if it happens to work we're lucky ;). Even then the only thing that would remotely be affected by that one_reg rename is live migration (which just got a few more fixes in this pull request). 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
Il 30/05/2014 17:58, Alexander Graf ha scritto: >> >> Would new userspace with old kernel be able to detect that POWER8 >> support isn't quite complete? > > It couldn't, no. It would try to run a guest - if it happens to work > we're lucky ;). That's why I'm considering a revert. > Even then the only thing that would remotely be affected > by that one_reg rename is live migration (which just got a few more > fixes in this pull request). Doesn't "info cpus" also do get/set one_reg? What happens if it returns EINVAL? Also, reset should certainly try to write all registers, what happens if one is missed. Beyond the particular case of WORT, I'd just like to point out that uapi/ changes need even more scrutiny from maintainers than usual. I don't know exactly what checks Linus makes in my pull requests, but uapi/ is at the top of the list of things he might look at, right after the diffstat. :) Paolo -- 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
On 30.05.14 18:03, Paolo Bonzini wrote: > Il 30/05/2014 17:58, Alexander Graf ha scritto: >>> >>> Would new userspace with old kernel be able to detect that POWER8 >>> support isn't quite complete? >> >> It couldn't, no. It would try to run a guest - if it happens to work >> we're lucky ;). > > That's why I'm considering a revert. > >> Even then the only thing that would remotely be affected >> by that one_reg rename is live migration (which just got a few more >> fixes in this pull request). > > Doesn't "info cpus" also do get/set one_reg? Yeah, but WORT is not important enough to get listed. > What happens if it returns EINVAL? Also, reset should certainly try > to write all registers, what happens if one is missed. If it returns EINVAL we just ignore the register. > > Beyond the particular case of WORT, I'd just like to point out that > uapi/ changes need even more scrutiny from maintainers than usual. I > don't know exactly what checks Linus makes in my pull requests, but > uapi/ is at the top of the list of things he might look at, right > after the diffstat. :) Consider that ONE_REG as experimental flagged :). Really, I am as concerned as you are on ABI breakages, but in this case it's not worth it. I'm not even sure any guest uses WORT at all. Linux doesn't seem to. 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
Il 30/05/2014 18:08, Alexander Graf ha scritto: >> What happens if it returns EINVAL? Also, reset should certainly >> try to write all registers, what happens if one is missed. > > If it returns EINVAL we just ignore the register. I wonder if it's the right thing to do. You remember how you were bitten by less-than-sensible error handling in the x86 kvm_arch_put_registers. >> Beyond the particular case of WORT, I'd just like to point out that >> uapi/ changes need even more scrutiny from maintainers than usual. I >> don't know exactly what checks Linus makes in my pull requests, but >> uapi/ is at the top of the list of things he might look at, right >> after the diffstat. :) > > Consider that ONE_REG as experimental flagged :). Really, I am as > concerned as you are on ABI breakages, but in this case it's not worth > it. I'm not even sure any guest uses WORT at all. Linux doesn't seem to. Fair enough... for this time only!... Paolo -- 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
On 30.05.14 18:11, Paolo Bonzini wrote: > Il 30/05/2014 18:08, Alexander Graf ha scritto: >>> What happens if it returns EINVAL? Also, reset should certainly >>> try to write all registers, what happens if one is missed. >> >> If it returns EINVAL we just ignore the register. > > I wonder if it's the right thing to do. You remember how you were > bitten by less-than-sensible error handling in the x86 > kvm_arch_put_registers. Yeah, I think we'll have to do a flag day as of which we declare PPC as a first class supported citizen. At that point we'll have compat machine types and start to do more intense error checking. 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
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt index 9a95770..6b30290 100644 --- a/Documentation/virtual/kvm/api.txt +++ b/Documentation/virtual/kvm/api.txt @@ -1873,6 +1873,7 @@ registers, find a list below: PPC | KVM_REG_PPC_PPR | 64 PPC | KVM_REG_PPC_ARCH_COMPAT 32 PPC | KVM_REG_PPC_DABRX | 32 + PPC | KVM_REG_PPC_WORT | 64 PPC | KVM_REG_PPC_TM_GPR0 | 64 ... PPC | KVM_REG_PPC_TM_GPR31 | 64 diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h index a6665be..2bc4a94 100644 --- a/arch/powerpc/include/uapi/asm/kvm.h +++ b/arch/powerpc/include/uapi/asm/kvm.h @@ -545,7 +545,6 @@ struct kvm_get_htab_header { #define KVM_REG_PPC_TCSCR (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb1) #define KVM_REG_PPC_PID (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb2) #define KVM_REG_PPC_ACOP (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb3) -#define KVM_REG_PPC_WORT (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb4) #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) @@ -555,6 +554,7 @@ struct kvm_get_htab_header { #define KVM_REG_PPC_ARCH_COMPAT (KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xb7) #define KVM_REG_PPC_DABRX (KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xb8) +#define KVM_REG_PPC_WORT (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb9) /* Transactional Memory checkpointed state: * This is all GPRs, all VSX regs and a subset of SPRs