Re: [PATCH] kvm: x86: mmu: Add cast to negated bitmasks in update_permission_bitmask()

[Joe Perches]

On Fri, 2018-06-15 at 11:29 -0700, Matthias Kaehlcke wrote:
> On Fri, Jun 15, 2018 at 11:18:12AM -0700, Joe Perches wrote:
> > On Fri, 2018-06-15 at 11:04 -0700, Nick Desaulniers wrote:
> > > On Fri, Jun 15, 2018 at 10:47 AM Matthias Kaehlcke <mka@xxxxxxxxxxxx> wrote:
> > > > 
> > > > update_permission_bitmask() negates u8 bitmask values and assigns them
> > > > to variables of type u8. Since the MSB is set in the bitmask values the
> > > > compiler expands the negated values to int, which then are assigned to
> > > > u8 variables. Cast the negated values back to u8.
> > > > 
> > > > This fixes several warnings like this when building with clang:
> > > > 
> > > > arch/x86/kvm/mmu.c:4266:39: error: implicit conversion from 'int' to 'u8'
> > > >   (aka 'unsigned char') changes value from -205 to 51 [-Werror,
> > > >   -Wconstant-conversion]
> > > >     u8 wf = (pfec & PFERR_WRITE_MASK) ? ~w : 0;
> > > >        ~~                               ^~
> > > > 
> > > > (gcc also raises a warning (see https://godbolt.org/g/6JWfWk), however it
> > > > doesn't seem to be universally enabled)
> > 
> > Perhaps it's better to turn off the warning.
> > There are more of these in the kernel too.
> > 
> > At least:
> > drivers/regulator/max8660.c:	u8 mask = (rdev_get_id(rdev) == MAX8660_V3) ? ~1 : ~4;
> > drivers/regulator/max8660.c:	u8 mask = (rdev_get_id(rdev) == MAX8660_V6) ? ~2 : ~4;
> > fs/ext4/resize.c:	__u16 uninit_mask = (flexbg_size > 1) ? ~EXT4_BG_BLOCK_UNINIT : ~0;
> 
> In my experience neither clang nor gcc should promote these values to
> int, since the MSB/sign bit is not set.

Well, that is what the c90 standard requires.

6.5.3.3 Unary arithmetic operators
Constraints
4 The result of the ~ operator is the bitwise complement of its (promoted) operand (that is,
each bit in the result is set if and only if the corresponding bit in the converted operand is
not set). The integer promotions are performed on the operand, and the result has the
promoted type. If the promoted type is an unsigned type, the expression ~E is equivalent
to the maximum value representable in that type minus E.

> In any case I think it it preferable to fix the code over disabling
> the warning, unless the warning is bogus or there are just too many
> occurrences.

Maybe.