All of lore.kernel.org
 help / color / mirror / Atom feed
From: Marc Zyngier <maz@kernel.org>
To: Andrew Jones <drjones@redhat.com>
Cc: kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org
Subject: Re: [PATCH] arm64: KVM: Add XXX UAPI notes for swapped registers
Date: Mon, 20 Jan 2020 13:59:45 +0000	[thread overview]
Message-ID: <773360494fdacfbade4b1cc611bc0082@kernel.org> (raw)
In-Reply-To: <20200120135129.tgucvvwbeef2q3js@kamzik.brq.redhat.com>

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.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

WARNING: multiple messages have this Message-ID (diff)
From: Marc Zyngier <maz@kernel.org>
To: Andrew Jones <drjones@redhat.com>
Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH] arm64: KVM: Add XXX UAPI notes for swapped registers
Date: Mon, 20 Jan 2020 13:59:45 +0000	[thread overview]
Message-ID: <773360494fdacfbade4b1cc611bc0082@kernel.org> (raw)
In-Reply-To: <20200120135129.tgucvvwbeef2q3js@kamzik.brq.redhat.com>

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.
-- 
Jazz is not dead. It just smells funny...

  reply	other threads:[~2020-01-20 13:59 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-20 13:08 [PATCH] arm64: KVM: Add XXX UAPI notes for swapped registers Andrew Jones
2020-01-20 13:08 ` Andrew Jones
2020-01-20 13:26 ` Marc Zyngier
2020-01-20 13:26   ` Marc Zyngier
2020-01-20 13:51   ` Andrew Jones
2020-01-20 13:51     ` Andrew Jones
2020-01-20 13:59     ` Marc Zyngier [this message]
2020-01-20 13:59       ` Marc Zyngier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=773360494fdacfbade4b1cc611bc0082@kernel.org \
    --to=maz@kernel.org \
    --cc=drjones@redhat.com \
    --cc=kvm@vger.kernel.org \
    --cc=kvmarm@lists.cs.columbia.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.