[v3] xen/arm: physical timer improvements

[Xen-devel] [PATCH v3 2/2] xen/arm: sign extend writes to TimerValue

Posted by Jeff Kubascik 6 years, 2 months ago

Per the ARMv8 Reference Manual (ARM DDI 0487E.a), section D11.2.4
specifies that the values in the TimerValue view of the timers are
signed in standard two's complement form. When writing to the TimerValue
register, it should be signed extended as described by the equation

   CompareValue = (Counter[63:0] + SignExtend(TimerValue))[63:0]

Signed-off-by: Jeff Kubascik <jeff.kubascik@dornerworks.com>
---
 xen/arch/arm/vtimer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 21b98ec20a..872181d9b6 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -211,7 +211,7 @@ static bool vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r,
     }
     else
     {
-        v->arch.phys_timer.cval = cntpct + *r;
+        v->arch.phys_timer.cval = cntpct + (uint64_t)(int32_t)*r;
         if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
         {
             v->arch.phys_timer.ctl &= ~CNTx_CTL_PENDING;
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/2] xen/arm: sign extend writes to TimerValue

Posted by Julien Grall 6 years, 1 month ago

Hi Jeff,

On 11/12/2019 21:13, Jeff Kubascik wrote:
> Per the ARMv8 Reference Manual (ARM DDI 0487E.a), section D11.2.4
> specifies that the values in the TimerValue view of the timers are
> signed in standard two's complement form. When writing to the TimerValue

Do you mean CompareValue register instead of TimerValue register?

> register, it should be signed extended as described by the equation
> 
>     CompareValue = (Counter[63:0] + SignExtend(TimerValue))[63:0]
This explains the signed part, but it does not explain why the 32-bit 
case. So I would mention that TimerValue is a 32-bit signed integer.

Maybe saying "are 32-bit signed in standard ..."

> 
> Signed-off-by: Jeff Kubascik <jeff.kubascik@dornerworks.com>
> ---
>   xen/arch/arm/vtimer.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index 21b98ec20a..872181d9b6 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -211,7 +211,7 @@ static bool vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r,
>       }
>       else
>       {
> -        v->arch.phys_timer.cval = cntpct + *r;
> +        v->arch.phys_timer.cval = cntpct + (uint64_t)(int32_t)*r;
>           if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
>           {
>               v->arch.phys_timer.ctl &= ~CNTx_CTL_PENDING;
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/2] xen/arm: sign extend writes to TimerValue

Posted by Jeff Kubascik 6 years ago

On 12/18/2019 9:24 AM, Julien Grall wrote:
> Hi Jeff,
> 
> On 11/12/2019 21:13, Jeff Kubascik wrote:
>> Per the ARMv8 Reference Manual (ARM DDI 0487E.a), section D11.2.4
>> specifies that the values in the TimerValue view of the timers are
>> signed in standard two's complement form. When writing to the TimerValue
> 
> Do you mean CompareValue register instead of TimerValue register?

I'm fairly certain TimerValue register is correct. When the guest writes to the
TimerValue register, the equation below is used to convert it to a CompareValue
equivalent.

>> register, it should be signed extended as described by the equation
>>
>>     CompareValue = (Counter[63:0] + SignExtend(TimerValue))[63:0]
> This explains the signed part, but it does not explain why the 32-bit
> case. So I would mention that TimerValue is a 32-bit signed integer.
> 
> Maybe saying "are 32-bit signed in standard ..."

I pulled this equation directly from the ARMv8 Reference Manual - the manual
goes into detail about the sign extension. This is referenced earlier in the
commit message.

>>
>> Signed-off-by: Jeff Kubascik <jeff.kubascik@dornerworks.com>
>> ---
>>   xen/arch/arm/vtimer.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
>> index 21b98ec20a..872181d9b6 100644
>> --- a/xen/arch/arm/vtimer.c
>> +++ b/xen/arch/arm/vtimer.c
>> @@ -211,7 +211,7 @@ static bool vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r,
>>       }
>>       else
>>       {
>> -        v->arch.phys_timer.cval = cntpct + *r;
>> +        v->arch.phys_timer.cval = cntpct + (uint64_t)(int32_t)*r;
>>           if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
>>           {
>>               v->arch.phys_timer.ctl &= ~CNTx_CTL_PENDING;
>>
> 
> Cheers,
> 
> --
> Julien Grall
> 

Sincerely,
Jeff Kubascik

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/2] xen/arm: sign extend writes to TimerValue

Posted by Julien Grall 6 years ago

On 17/01/2020 21:29, Jeff Kubascik wrote:
> On 12/18/2019 9:24 AM, Julien Grall wrote:
>> Hi Jeff,
>>
>> On 11/12/2019 21:13, Jeff Kubascik wrote:
>>> Per the ARMv8 Reference Manual (ARM DDI 0487E.a), section D11.2.4
>>> specifies that the values in the TimerValue view of the timers are
>>> signed in standard two's complement form. When writing to the TimerValue
>>
>> Do you mean CompareValue register instead of TimerValue register?
> 
> I'm fairly certain TimerValue register is correct. When the guest writes to the
> TimerValue register, the equation below is used to convert it to a CompareValue
> equivalent.

I find the sentence quite confusing to read. It is not the write that 
needs to be signed extend, but the value used to compute CompareValue. 
So how about the following commit message:

"
xen/arm: Sign extend TimerValue when computing the CompareValue

Xen will only store the CompareValue as it can be derived from the 
TimerValue (ARM DDI 0487E.a section D11.2.4):

  CompareValue = (Counter[63:0] + SignExtend(TimerValue))[63:0]

While the TimerValue is a 32-bit signed value, our implementation 
assumed it is a 32-bit unsigned value.
"

> 
>>> register, it should be signed extended as described by the equation
>>>
>>>      CompareValue = (Counter[63:0] + SignExtend(TimerValue))[63:0]
>> This explains the signed part, but it does not explain why the 32-bit
>> case. So I would mention that TimerValue is a 32-bit signed integer.
>>
>> Maybe saying "are 32-bit signed in standard ..."
> 
> I pulled this equation directly from the ARMv8 Reference Manual - the manual
> goes into detail about the sign extension. This is referenced earlier in the
> commit message.

While I agree the commit message explain in details the sign extension, 
there is nothing in your commit message about the size of TimerValue 
(i.e 32-bit). If you say it is a 32-bit signed value, then it is much 
clearer to understand the cast you added below.

But please see above for a suggested commit message.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/2] xen/arm: sign extend writes to TimerValue

Posted by Jeff Kubascik 6 years ago

On 1/18/2020 6:49 AM, Julien Grall wrote:
> On 17/01/2020 21:29, Jeff Kubascik wrote:
>> On 12/18/2019 9:24 AM, Julien Grall wrote:
>>> Hi Jeff,
>>>
>>> On 11/12/2019 21:13, Jeff Kubascik wrote:
>>>> Per the ARMv8 Reference Manual (ARM DDI 0487E.a), section D11.2.4
>>>> specifies that the values in the TimerValue view of the timers are
>>>> signed in standard two's complement form. When writing to the TimerValue
>>>
>>> Do you mean CompareValue register instead of TimerValue register?
>>
>> I'm fairly certain TimerValue register is correct. When the guest writes to the
>> TimerValue register, the equation below is used to convert it to a CompareValue
>> equivalent.
> 
> I find the sentence quite confusing to read. It is not the write that
> needs to be signed extend, but the value used to compute CompareValue.
> So how about the following commit message:
> 
> "
> xen/arm: Sign extend TimerValue when computing the CompareValue
> 
> Xen will only store the CompareValue as it can be derived from the
> TimerValue (ARM DDI 0487E.a section D11.2.4):
> 
>   CompareValue = (Counter[63:0] + SignExtend(TimerValue))[63:0]
> 
> While the TimerValue is a 32-bit signed value, our implementation
> assumed it is a 32-bit unsigned value.
> "

I agree with this version, it is clearer and and simpler.

>>
>>>> register, it should be signed extended as described by the equation
>>>>
>>>>      CompareValue = (Counter[63:0] + SignExtend(TimerValue))[63:0]
>>> This explains the signed part, but it does not explain why the 32-bit
>>> case. So I would mention that TimerValue is a 32-bit signed integer.
>>>
>>> Maybe saying "are 32-bit signed in standard ..."
>>
>> I pulled this equation directly from the ARMv8 Reference Manual - the manual
>> goes into detail about the sign extension. This is referenced earlier in the
>> commit message.
> 
> While I agree the commit message explain in details the sign extension,
> there is nothing in your commit message about the size of TimerValue
> (i.e 32-bit). If you say it is a 32-bit signed value, then it is much
> clearer to understand the cast you added below.
> 
> But please see above for a suggested commit message.

I'll send out an updated patch set with the new commit message.

> Cheers,
> 
> --
> Julien Grall
> 

Sincerely,
Jeff Kubascik

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel