Patchwork [PULL,33/41] KVM: PPC: Book3S: Move KVM_REG_PPC_WORT to an unused register number

login
register
mail settings
Submitter Alexander Graf
Date May 30, 2014, 12:42 p.m.
Message ID <1401453776-55285-34-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/354130/
State New
Headers show

Comments

Alexander Graf - May 30, 2014, 12:42 p.m.
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(-)
Paolo Bonzini - May 30, 2014, 3:50 p.m.
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
Alexander Graf - May 30, 2014, 3:53 p.m.
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
Paolo Bonzini - May 30, 2014, 3:55 p.m.
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
Alexander Graf - May 30, 2014, 3:58 p.m.
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
Paolo Bonzini - May 30, 2014, 4:03 p.m.
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
Alexander Graf - May 30, 2014, 4:08 p.m.
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
Paolo Bonzini - May 30, 2014, 4:11 p.m.
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
Alexander Graf - May 30, 2014, 4:14 p.m.
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

Patch

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