Re: [PATCH V3 19/23] xen/arm: io: Abstract sign-extension

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.12.2020 00:27, Oleksandr wrote:
> On 30.11.20 23:03, Volodymyr Babchuk wrote:
>> Oleksandr Tyshchenko writes:
>>> --- a/xen/include/asm-arm/traps.h
>>> +++ b/xen/include/asm-arm/traps.h
>>> @@ -83,6 +83,30 @@ static inline bool VABORT_GEN_BY_GUEST(const struct 
>>> cpu_user_regs *regs)
>>>           (unsigned long)abort_guest_exit_end == regs->pc;
>>>   }
>>>   
>>> +/* Check whether the sign extension is required and perform it */
>>> +static inline register_t sign_extend(const struct hsr_dabt dabt, 
>>> register_t r)
>>> +{
>>> +    uint8_t size = (1 << dabt.size) * 8;
>>> +
>>> +    /*
>>> +     * Sign extend if required.
>>> +     * Note that we expect the read handler to have zeroed the bits
>>> +     * outside the requested access size.
>>> +     */
>>> +    if ( dabt.sign && (r & (1UL << (size - 1))) )
>>> +    {
>>> +        /*
>>> +         * We are relying on register_t using the same as
>>> +         * an unsigned long in order to keep the 32-bit assembly
>>> +         * code smaller.
>>> +         */
>>> +        BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long));
>>> +        r |= (~0UL) << size;
>> If `size` is 64, you will get undefined behavior there.
> I think, we don't need to worry about undefined behavior here. Having 
> size=64 would be possible with doubleword (dabt.size=3). But if "r" 
> adjustment gets called (I mean Syndrome Sign Extend bit is set) then
> we deal with byte, halfword or word operations (dabt.size<3). Or I 
> missed something?

At which point please put in a respective ASSERT(), possibly amended
by a brief comment.

Jan