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

[Jeff Kubascik <jeff.kubascik@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel