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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.06.15 at 15:02, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 22/06/15 13:55, Jan Beulich wrote:
>>>>> On 22.06.15 at 14:15, <andrew.cooper3@xxxxxxxxxx> 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).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel