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

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

On 22/06/15 12:49, Jan Beulich wrote:
> - use 32-bit types where possible (produces slightly better code)
> - drop (now) unnecessary casts
> - avoid indirection where not needed
> - avoid duplicate log messages in vlapic_write()
> - minor other cleanup
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with two
suggestions.

>
> @@ -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 can't quite decide whether 0xff
will be interpreted as signed or unsigned, given the integer promotion
rules.)

>              break;
>  
>          case 2:
>              if ( alignment & 1 )
>                  goto unaligned_exit_and_crash;
> -            val = ((tmp & ~(0xffff << (8*alignment))) |
> -                   ((val & 0xffff) << (8*alignment)));
> +            val = ((tmp & ~(0xffff << alignment)) |
> +                   ((val & 0xffff) << alignment));
>              break;
>  
>          default:
> -            gdprintk(XENLOG_ERR, "Local APIC write with len = %lx, "
> -                     "should be 4 instead\n", len);
> +            gprintk(XENLOG_ERR, "LAPIC write with len %lx\n", len);
>              goto exit_and_crash;
>          }
> +
> +        gdprintk(XENLOG_INFO, "Notice: LAPIC write with len %lx\n", len);

Probably better using %lu.