Re: [PATCH] x86/vLAPIC: adjust types in internal read/write handling

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 22/06/15 14:27, Jan Beulich wrote:
>>>> On 22.06.15 at 15:02, <andrew.cooper3@citrix.com> wrote:
>> On 22/06/15 13:55, Jan Beulich wrote:
>>>>>> On 22.06.15 at 14:15, <andrew.cooper3@citrix.com> wrote:
>>>> On 22/06/15 12:49, Jan Beulich wrote:
>>>>> @@ -847,47 +834,41 @@ static int vlapic_write(struct vcpu *v, 
>>>>>       * According to the IA32 Manual, all accesses should be 32 bits.
>>>>>       * Some OSes do 8- or 16-byte accesses, however.
>>>>>       */
>>>>> -    val = (uint32_t)val;
>>>>> -    if ( len != 4 )
>>>>> +    if ( unlikely(len != 4) )
>>>>>      {
>>>>> -        unsigned int tmp;
>>>>> -        unsigned char alignment;
>>>>> -
>>>>> -        gdprintk(XENLOG_INFO, "Notice: Local APIC write with len = 
>> %lx\n",len);
>>>>> -
>>>>> -        alignment = offset & 0x3;
>>>>> -        (void)vlapic_read_aligned(vlapic, offset & ~0x3, &tmp);
>>>>> +        unsigned int tmp = vlapic_read_aligned(vlapic, offset & ~3);
>>>>> +        unsigned char alignment = (offset & 3) * 8;
>>>>>  
>>>>>          switch ( len )
>>>>>          {
>>>>>          case 1:
>>>>> -            val = ((tmp & ~(0xff << (8*alignment))) |
>>>>> -                   ((val & 0xff) << (8*alignment)));
>>>>> +            val = ((tmp & ~(0xff << alignment)) |
>>>>> +                   ((val & 0xff) << alignment));
>>>> These should probably be explicitly unsigned constants, to avoid issues
>>>> with shifting a 1 into the sign bit.
>>> I don't see what harm the sign bit would do here - even if the shift
>>> operation is one on signed int, the & converts the operand to
>>> unsigned int anyway (and with them being the same size, the
>>> binary representation doesn't change).
>> The problem is with 0xff << 24, which where the sign bit will change
>> given the shift.
>>
>> If 0xff is interpreted as signed, then shifted, then promoted to
>> unsigned by the ~ operation, then the result is undefined behaviour
>> (altering the sign bit of a number with a shift).
> Okay, while I can buy that, I suppose we've got many more of these
> throughout the tree (and the compiler is treating them quite fine).

We likely have.  I try my best to fix them as I find them.

As for the compiler, it is probably easiest to ignore the potential
problem which ends up generating correct code (and is a legitimate
action to take with UB).  It is more likely to be a problem with extreme
levels of optimisations enabled, as the compiler tries harder and harder
to throw operations away.

~Andrew