| Message ID | 20200120130825.28838-1-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: KVM: Add XXX UAPI notes for swapped registers | expand |
Hi Andrew, Many thanks for this. Comments below. On 2020-01-20 13:08, Andrew Jones wrote: > Two UAPI system register IDs do not derive their values from the > ARM system register encodings. This is because their values were > accidentally swapped. As the IDs are API, they cannot be changed. > Add XXX notes to point them out. > > Suggested-by: Marc Zyngier <maz@kernel.org> > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > Documentation/virt/kvm/api.txt | 8 ++++++++ > arch/arm64/include/uapi/asm/kvm.h | 11 +++++++++-- > 2 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/Documentation/virt/kvm/api.txt > b/Documentation/virt/kvm/api.txt > index ebb37b34dcfc..11556fc457c3 100644 > --- a/Documentation/virt/kvm/api.txt > +++ b/Documentation/virt/kvm/api.txt > @@ -2196,6 +2196,14 @@ arm64 CCSIDR registers are demultiplexed by > CSSELR value: > arm64 system registers have the following id bit patterns: > 0x6030 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3> > > +XXX: Two system register IDs do not follow the specified pattern. > These > + are KVM_REG_ARM_TIMER_CVAL and KVM_REG_ARM_TIMER_CNT, which map > to > + system registers CNTV_CVAL_EL0 and CNTVCT_EL0 respectively. > These > + two had their values accidentally swapped, which means TIMER_CVAL > is > + derived from the register encoding for CNTVCT_EL0 and TIMER_CNT > is > + derived from the register encoding for CNTV_CVAL_EL0. As this is > + API, it must remain this way. Is 'XXX' an establiched way of documenting this kind of misfeature? I couldn't find any other occurrence in Documentation, but I haven't searched very hard. If nobody has a better idea, I'll queue it as is. Thanks, M.
On Mon, Jan 20, 2020 at 01:26:32PM +0000, Marc Zyngier wrote: > Hi Andrew, > > Many thanks for this. Comments below. > > On 2020-01-20 13:08, Andrew Jones wrote: > > Two UAPI system register IDs do not derive their values from the > > ARM system register encodings. This is because their values were > > accidentally swapped. As the IDs are API, they cannot be changed. > > Add XXX notes to point them out. > > > > Suggested-by: Marc Zyngier <maz@kernel.org> > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > Documentation/virt/kvm/api.txt | 8 ++++++++ > > arch/arm64/include/uapi/asm/kvm.h | 11 +++++++++-- > > 2 files changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/virt/kvm/api.txt > > b/Documentation/virt/kvm/api.txt > > index ebb37b34dcfc..11556fc457c3 100644 > > --- a/Documentation/virt/kvm/api.txt > > +++ b/Documentation/virt/kvm/api.txt > > @@ -2196,6 +2196,14 @@ arm64 CCSIDR registers are demultiplexed by > > CSSELR value: > > arm64 system registers have the following id bit patterns: > > 0x6030 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3> > > > > +XXX: Two system register IDs do not follow the specified pattern. > > These > > + are KVM_REG_ARM_TIMER_CVAL and KVM_REG_ARM_TIMER_CNT, which map to > > + system registers CNTV_CVAL_EL0 and CNTVCT_EL0 respectively. These > > + two had their values accidentally swapped, which means TIMER_CVAL > > is > > + derived from the register encoding for CNTVCT_EL0 and TIMER_CNT is > > + derived from the register encoding for CNTV_CVAL_EL0. As this is > > + API, it must remain this way. > > Is 'XXX' an establiched way of documenting this kind of misfeature? > I couldn't find any other occurrence in Documentation, but I haven't > searched very hard. I didn't find anything claiming it was the standard way of doing it. I also considered using 'NOTE', but I was afraid it wouldn't stand out as a "problem" enough. And, even though 'BUG' would certainly stand out, I felt it implied we should be posting a fix. Anyway, I'd be happy to change it to whatever the consensus is. Thanks, drew > > If nobody has a better idea, I'll queue it as is. > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny... >
On 2020-01-20 13:51, Andrew Jones wrote: > On Mon, Jan 20, 2020 at 01:26:32PM +0000, Marc Zyngier wrote: >> Hi Andrew, >> >> Many thanks for this. Comments below. >> >> On 2020-01-20 13:08, Andrew Jones wrote: >> > Two UAPI system register IDs do not derive their values from the >> > ARM system register encodings. This is because their values were >> > accidentally swapped. As the IDs are API, they cannot be changed. >> > Add XXX notes to point them out. >> > >> > Suggested-by: Marc Zyngier <maz@kernel.org> >> > Signed-off-by: Andrew Jones <drjones@redhat.com> >> > --- >> > Documentation/virt/kvm/api.txt | 8 ++++++++ >> > arch/arm64/include/uapi/asm/kvm.h | 11 +++++++++-- >> > 2 files changed, 17 insertions(+), 2 deletions(-) >> > >> > diff --git a/Documentation/virt/kvm/api.txt >> > b/Documentation/virt/kvm/api.txt >> > index ebb37b34dcfc..11556fc457c3 100644 >> > --- a/Documentation/virt/kvm/api.txt >> > +++ b/Documentation/virt/kvm/api.txt >> > @@ -2196,6 +2196,14 @@ arm64 CCSIDR registers are demultiplexed by >> > CSSELR value: >> > arm64 system registers have the following id bit patterns: >> > 0x6030 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3> >> > >> > +XXX: Two system register IDs do not follow the specified pattern. >> > These >> > + are KVM_REG_ARM_TIMER_CVAL and KVM_REG_ARM_TIMER_CNT, which map to >> > + system registers CNTV_CVAL_EL0 and CNTVCT_EL0 respectively. These >> > + two had their values accidentally swapped, which means TIMER_CVAL >> > is >> > + derived from the register encoding for CNTVCT_EL0 and TIMER_CNT is >> > + derived from the register encoding for CNTV_CVAL_EL0. As this is >> > + API, it must remain this way. >> >> Is 'XXX' an establiched way of documenting this kind of misfeature? >> I couldn't find any other occurrence in Documentation, but I haven't >> searched very hard. > > I didn't find anything claiming it was the standard way of doing it. I > also considered using 'NOTE', but I was afraid it wouldn't stand out > as a "problem" enough. And, even though 'BUG' would certainly stand > out, > I felt it implied we should be posting a fix. > > Anyway, I'd be happy to change it to whatever the consensus is. My personal preference would be for a big fat "*WARNING*" (with an optional <blink> attribute), and a link to a picture of me wearing a brown paper bag on my head. But maybe "WARNING" is enough. Dunno. Anyway, if nobody comes up with a better idea by tomorrow, I'll take it as is. Thanks, M.
diff --git a/Documentation/virt/kvm/api.txt b/Documentation/virt/kvm/api.txt index ebb37b34dcfc..11556fc457c3 100644 --- a/Documentation/virt/kvm/api.txt +++ b/Documentation/virt/kvm/api.txt @@ -2196,6 +2196,14 @@ arm64 CCSIDR registers are demultiplexed by CSSELR value: arm64 system registers have the following id bit patterns: 0x6030 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3> +XXX: Two system register IDs do not follow the specified pattern. These + are KVM_REG_ARM_TIMER_CVAL and KVM_REG_ARM_TIMER_CNT, which map to + system registers CNTV_CVAL_EL0 and CNTVCT_EL0 respectively. These + two had their values accidentally swapped, which means TIMER_CVAL is + derived from the register encoding for CNTVCT_EL0 and TIMER_CNT is + derived from the register encoding for CNTV_CVAL_EL0. As this is + API, it must remain this way. + arm64 firmware pseudo-registers have the following bit pattern: 0x6030 0000 0014 <regno:16> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h index 820e5751ada7..c72387e48851 100644 --- a/arch/arm64/include/uapi/asm/kvm.h +++ b/arch/arm64/include/uapi/asm/kvm.h @@ -220,10 +220,17 @@ struct kvm_vcpu_events { #define KVM_REG_ARM_PTIMER_CVAL ARM64_SYS_REG(3, 3, 14, 2, 2) #define KVM_REG_ARM_PTIMER_CNT ARM64_SYS_REG(3, 3, 14, 0, 1) -/* EL0 Virtual Timer Registers */ +/* + * EL0 Virtual Timer Registers + * + * XXX: KVM_REG_ARM_TIMER_CVAL and KVM_REG_ARM_TIMER_CNT are not defined + * with the appropriate register encodings. Their values have been + * accidentally swapped. As this is set API, the definitions here + * must be used, rather than ones derived from the encodings. + */ #define KVM_REG_ARM_TIMER_CTL ARM64_SYS_REG(3, 3, 14, 3, 1) -#define KVM_REG_ARM_TIMER_CNT ARM64_SYS_REG(3, 3, 14, 3, 2) #define KVM_REG_ARM_TIMER_CVAL ARM64_SYS_REG(3, 3, 14, 0, 2) +#define KVM_REG_ARM_TIMER_CNT ARM64_SYS_REG(3, 3, 14, 3, 2) /* KVM-as-firmware specific pseudo-registers */ #define KVM_REG_ARM_FW (0x0014 << KVM_REG_ARM_COPROC_SHIFT)
Two UAPI system register IDs do not derive their values from the ARM system register encodings. This is because their values were accidentally swapped. As the IDs are API, they cannot be changed. Add XXX notes to point them out. Suggested-by: Marc Zyngier <maz@kernel.org> Signed-off-by: Andrew Jones <drjones@redhat.com> --- Documentation/virt/kvm/api.txt | 8 ++++++++ arch/arm64/include/uapi/asm/kvm.h | 11 +++++++++-- 2 files changed, 17 insertions(+), 2 deletions(-)