Re: [Qemu-devel] [PATCH 02/13] PPC: Fix lsxw bounds checks

[Thomas Huth]

On 03/11/15 20:21, Mark Cave-Ayland wrote:
> On 03/11/15 15:23, Thomas Huth wrote:
> 
>> On 23/10/15 15:56, Mark Cave-Ayland wrote:
>>> From: Alexander Graf <address@hidden>
>>>
>>> The lsxw instruction checks whether the desired string actually fits
>>> into all defined registers. Unfortunately it does the calculation wrong,
>>> resulting in illegal instruction traps for loads that really should fit.
>>
>> s/lsxw/lswx/ in the above text and in the title ... but I guess this
>> could also be done when this patch gets picked up.
>>
>>> Fix it up, making Mac OS happier.
>>>
>>> Signed-off-by: Alexander Graf <address@hidden>
>>> Signed-off-by: Mark Cave-Ayland <address@hidden>
>>> ---
>>>  target-ppc/mem_helper.c |    5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target-ppc/mem_helper.c b/target-ppc/mem_helper.c
>>> index 6d37dae..7e1f234 100644
>>> --- a/target-ppc/mem_helper.c
>>> +++ b/target-ppc/mem_helper.c
>>> @@ -100,8 +100,9 @@ void helper_lswx(CPUPPCState *env, target_ulong addr, 
>>> uint32_t reg,
>>>                   uint32_t ra, uint32_t rb)
>>>  {
>>>      if (likely(xer_bc != 0)) {
>>> -        if (unlikely((ra != 0 && reg < ra && (reg + xer_bc) > ra) ||
>>> -                     (reg < rb && (reg + xer_bc) > rb))) {
>>> +        int num_used_regs = (xer_bc + 3) / 4;
>>> +        if (unlikely((ra != 0 && reg < ra && (reg + num_used_regs) > ra) ||
>>> +                     (reg < rb && (reg + num_used_regs) > rb))) {
>>>              helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
>>>                                         POWERPC_EXCP_INVAL |
>>>                                         POWERPC_EXCP_INVAL_LSWX);
>>
>> The calculation of num_used_regs looks fine to me ... but is the
>> remaining part of the condition really right already?
>>
>> According to the PowerISA:
>>
>>  If RA or RB is in the range of registers to be loaded,
>>  including the case in which RA=0, the instruction is
>>  treated as if the instruction form were invalid. If RT=RA
>>  or RT=RB, the instruction form is invalid.
>>
>> So I wonder whether the check for "ra != 0" is really necessary here?
>> Also, shouldn't the code rather check for "reg <= ra" instead of "reg <
>> ra"? And "reg <= rb", too, of course?
>>
>> Also this code seems to completely ignore the case of the register
>> wrap-around, where the sequence of registers jumps back to r0 ...
>>
>> So I'm basically fine with the num_used_regs fix for now, but I think
>> this needs a big "FIXME" comment so that the remaining issues get
>> cleaned up later?
> 
> This was one of Alex's patches that was originally queued for ppc-next
> before being dropped for missing the SoB, so I was expecting review to
> find issues with other patches in the set rather than this one...
> 
> Having said that, I'm not sure whether this was deliberate for
> compatibility reasons or just an oversight. Unless David has any ideas
> it might be that we have to wait for Alex to return before this series
> can be included, but thanks for the review anyhow :)

Well, as I said, I'm basically fine with the patch, since it fixes one
bug and the fix itself also looks fine. It's just that the surrounding
code looks like it contains some more bugs... but these could also be
fixed with a later patch, I guess.

 Thomas