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

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

[Matthias Kaehlcke]

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)

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 arch/x86/kvm/mmu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index d634f0332c0f..a83817fdbd87 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4275,11 +4275,11 @@ static void update_permission_bitmask(struct kvm_vcpu *vcpu,
 		 */
 
 		/* Faults from writes to non-writable pages */
-		u8 wf = (pfec & PFERR_WRITE_MASK) ? ~w : 0;
+		u8 wf = (pfec & PFERR_WRITE_MASK) ? (u8)~w : 0;
 		/* Faults from user mode accesses to supervisor pages */
-		u8 uf = (pfec & PFERR_USER_MASK) ? ~u : 0;
+		u8 uf = (pfec & PFERR_USER_MASK) ? (u8)~u : 0;
 		/* Faults from fetches of non-executable pages*/
-		u8 ff = (pfec & PFERR_FETCH_MASK) ? ~x : 0;
+		u8 ff = (pfec & PFERR_FETCH_MASK) ? (u8)~x : 0;
 		/* Faults from kernel mode fetches of user pages */
 		u8 smepf = 0;
 		/* Faults from kernel mode accesses of user pages */
-- 
2.18.0.rc1.244.gcf134e6275-goog

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

[Nick Desaulniers]

On Fri, Jun 15, 2018 at 10:47 AM Matthias Kaehlcke <mka@chromium.org> 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)
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  arch/x86/kvm/mmu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index d634f0332c0f..a83817fdbd87 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -4275,11 +4275,11 @@ static void update_permission_bitmask(struct kvm_vcpu *vcpu,
>                  */
>
>                 /* Faults from writes to non-writable pages */
> -               u8 wf = (pfec & PFERR_WRITE_MASK) ? ~w : 0;
> +               u8 wf = (pfec & PFERR_WRITE_MASK) ? (u8)~w : 0;
>                 /* Faults from user mode accesses to supervisor pages */
> -               u8 uf = (pfec & PFERR_USER_MASK) ? ~u : 0;
> +               u8 uf = (pfec & PFERR_USER_MASK) ? (u8)~u : 0;
>                 /* Faults from fetches of non-executable pages*/
> -               u8 ff = (pfec & PFERR_FETCH_MASK) ? ~x : 0;
> +               u8 ff = (pfec & PFERR_FETCH_MASK) ? (u8)~x : 0;
>                 /* Faults from kernel mode fetches of user pages */
>                 u8 smepf = 0;
>                 /* Faults from kernel mode accesses of user pages */
> --
> 2.18.0.rc1.244.gcf134e6275-goog
>

This fixes -Woverflow warnings in gcc and -Wconstant-conversion
warnings in clang, without changing the disassembly.  Further
modification to mka's test case (to show no change in codegen):
https://godbolt.org/g/ynAoeJ

See also: https://wiki.sei.cmu.edu/confluence/display/c/EXP14-C.+Beware+of+integer+promotion+when+performing+bitwise+operations+on+integer+types+smaller+than+int

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
--
Thanks,
~Nick Desaulniers

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

[Joe Perches]

On Fri, 2018-06-15 at 11:04 -0700, Nick Desaulniers wrote:
> On Fri, Jun 15, 2018 at 10:47 AM Matthias Kaehlcke <mka@chromium.org> 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;


> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >  arch/x86/kvm/mmu.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> > index d634f0332c0f..a83817fdbd87 100644
> > --- a/arch/x86/kvm/mmu.c
> > +++ b/arch/x86/kvm/mmu.c
> > @@ -4275,11 +4275,11 @@ static void update_permission_bitmask(struct kvm_vcpu *vcpu,
> >                  */
> > 
> >                 /* Faults from writes to non-writable pages */
> > -               u8 wf = (pfec & PFERR_WRITE_MASK) ? ~w : 0;
> > +               u8 wf = (pfec & PFERR_WRITE_MASK) ? (u8)~w : 0;
> >                 /* Faults from user mode accesses to supervisor pages */
> > -               u8 uf = (pfec & PFERR_USER_MASK) ? ~u : 0;
> > +               u8 uf = (pfec & PFERR_USER_MASK) ? (u8)~u : 0;
> >                 /* Faults from fetches of non-executable pages*/
> > -               u8 ff = (pfec & PFERR_FETCH_MASK) ? ~x : 0;
> > +               u8 ff = (pfec & PFERR_FETCH_MASK) ? (u8)~x : 0;
> >                 /* Faults from kernel mode fetches of user pages */
> >                 u8 smepf = 0;
> >                 /* Faults from kernel mode accesses of user pages */
> > --
> > 2.18.0.rc1.244.gcf134e6275-goog
> > 
> 
> This fixes -Woverflow warnings in gcc and -Wconstant-conversion
> warnings in clang, without changing the disassembly.  Further
> modification to mka's test case (to show no change in codegen):
> https://godbolt.org/g/ynAoeJ
> 
> See also: https://wiki.sei.cmu.edu/confluence/display/c/EXP14-C.+Beware+of+integer+promotion+when+performing+bitwise+operations+on+integer+types+smaller+than+int
> 
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> --
> Thanks,
> ~Nick Desaulniers

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

[Matthias Kaehlcke]

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@chromium.org> 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.

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.

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@chromium.org> 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.

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

[Nick Desaulniers]

On Fri, Jun 15, 2018 at 11:40 AM Joe Perches <joe@perches.com> wrote:
>
> 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@chromium.org> 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.

Spurious warning today, actual bug tomorrow?  I prefer to not to
disable warnings wholesale.  They don't need to find actual bugs to be
useful.  Flagging code that can be further specified does not hurt.
Part of the effort to compile the kernel with different compilers is
to add warning coverage, not remove it.  That said, there may be
warnings that are never useful (or at least due to some invariant that
only affects the kernel).  I cant think of any off the top of my head,
but I'm also not sure this is one.
-- 
Thanks,
~Nick Desaulniers

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

[kbuild test robot]

Hi Matthias,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on kvm/linux-next]
[also build test WARNING on v4.17 next-20180615]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Matthias-Kaehlcke/kvm-x86-mmu-Add-cast-to-negated-bitmasks-in-update_permission_bitmask/20180616-015357
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   include/linux/seq_buf.h:71:16: sparse: expression using sizeof(void)
   include/linux/seq_buf.h:71:16: sparse: expression using sizeof(void)
   include/linux/seq_buf.h:71:16: sparse: expression using sizeof(void)
   include/linux/seq_buf.h:71:16: sparse: expression using sizeof(void)
   arch/x86/kvm/mmu.c:1123:21: sparse: expression using sizeof(void)
   arch/x86/kvm/mmu.c:1123:21: sparse: expression using sizeof(void)
   arch/x86/kvm/mmu.c:1769:37: sparse: expression using sizeof(void)
   arch/x86/kvm/mmu.c:1769:37: sparse: expression using sizeof(void)
   arch/x86/kvm/mmu.c:1770:35: sparse: expression using sizeof(void)
   arch/x86/kvm/mmu.c:1770:35: sparse: expression using sizeof(void)
   arch/x86/kvm/paging_tmpl.h:788:33: sparse: expression using sizeof(void)
   arch/x86/kvm/paging_tmpl.h:788:33: sparse: expression using sizeof(void)
   arch/x86/kvm/paging_tmpl.h:788:33: sparse: expression using sizeof(void)
   arch/x86/kvm/paging_tmpl.h:788:33: sparse: expression using sizeof(void)
   arch/x86/kvm/paging_tmpl.h:788:33: sparse: expression using sizeof(void)
   arch/x86/kvm/paging_tmpl.h:788:33: sparse: expression using sizeof(void)
   arch/x86/kvm/mmu.c:5168:33: sparse: expression using sizeof(void)
   arch/x86/kvm/mmu.c:5168:33: sparse: expression using sizeof(void)
   arch/x86/kvm/mmu.c:5169:31: sparse: expression using sizeof(void)
   arch/x86/kvm/mmu.c:5169:31: sparse: expression using sizeof(void)
   arch/x86/kvm/mmu.c:5548:24: sparse: expression using sizeof(void)
>> arch/x86/kvm/mmu.c:4280:57: sparse: cast truncates bits from constant value (ffffff33 becomes 33)
>> arch/x86/kvm/mmu.c:4282:56: sparse: cast truncates bits from constant value (ffffff0f becomes f)
>> arch/x86/kvm/mmu.c:4284:57: sparse: cast truncates bits from constant value (ffffff55 becomes 55)

vim +4280 arch/x86/kvm/mmu.c

  4247	
  4248	#define BYTE_MASK(access) \
  4249		((1 & (access) ? 2 : 0) | \
  4250		 (2 & (access) ? 4 : 0) | \
  4251		 (3 & (access) ? 8 : 0) | \
  4252		 (4 & (access) ? 16 : 0) | \
  4253		 (5 & (access) ? 32 : 0) | \
  4254		 (6 & (access) ? 64 : 0) | \
  4255		 (7 & (access) ? 128 : 0))
  4256	
  4257	
  4258	static void update_permission_bitmask(struct kvm_vcpu *vcpu,
  4259					      struct kvm_mmu *mmu, bool ept)
  4260	{
  4261		unsigned byte;
  4262	
  4263		const u8 x = BYTE_MASK(ACC_EXEC_MASK);
  4264		const u8 w = BYTE_MASK(ACC_WRITE_MASK);
  4265		const u8 u = BYTE_MASK(ACC_USER_MASK);
  4266	
  4267		bool cr4_smep = kvm_read_cr4_bits(vcpu, X86_CR4_SMEP) != 0;
  4268		bool cr4_smap = kvm_read_cr4_bits(vcpu, X86_CR4_SMAP) != 0;
  4269		bool cr0_wp = is_write_protection(vcpu);
  4270	
  4271		for (byte = 0; byte < ARRAY_SIZE(mmu->permissions); ++byte) {
  4272			unsigned pfec = byte << 1;
  4273	
  4274			/*
  4275			 * Each "*f" variable has a 1 bit for each UWX value
  4276			 * that causes a fault with the given PFEC.
  4277			 */
  4278	
  4279			/* Faults from writes to non-writable pages */
> 4280			u8 wf = (pfec & PFERR_WRITE_MASK) ? (u8)~w : 0;
  4281			/* Faults from user mode accesses to supervisor pages */
> 4282			u8 uf = (pfec & PFERR_USER_MASK) ? (u8)~u : 0;
  4283			/* Faults from fetches of non-executable pages*/
> 4284			u8 ff = (pfec & PFERR_FETCH_MASK) ? (u8)~x : 0;
  4285			/* Faults from kernel mode fetches of user pages */
  4286			u8 smepf = 0;
  4287			/* Faults from kernel mode accesses of user pages */
  4288			u8 smapf = 0;
  4289	
  4290			if (!ept) {
  4291				/* Faults from kernel mode accesses to user pages */
  4292				u8 kf = (pfec & PFERR_USER_MASK) ? 0 : u;
  4293	
  4294				/* Not really needed: !nx will cause pte.nx to fault */
  4295				if (!mmu->nx)
  4296					ff = 0;
  4297	
  4298				/* Allow supervisor writes if !cr0.wp */
  4299				if (!cr0_wp)
  4300					wf = (pfec & PFERR_USER_MASK) ? wf : 0;
  4301	
  4302				/* Disallow supervisor fetches of user code if cr4.smep */
  4303				if (cr4_smep)
  4304					smepf = (pfec & PFERR_FETCH_MASK) ? kf : 0;
  4305	
  4306				/*
  4307				 * SMAP:kernel-mode data accesses from user-mode
  4308				 * mappings should fault. A fault is considered
  4309				 * as a SMAP violation if all of the following
  4310				 * conditions are ture:
  4311				 *   - X86_CR4_SMAP is set in CR4
  4312				 *   - A user page is accessed
  4313				 *   - The access is not a fetch
  4314				 *   - Page fault in kernel mode
  4315				 *   - if CPL = 3 or X86_EFLAGS_AC is clear
  4316				 *
  4317				 * Here, we cover the first three conditions.
  4318				 * The fourth is computed dynamically in permission_fault();
  4319				 * PFERR_RSVD_MASK bit will be set in PFEC if the access is
  4320				 * *not* subject to SMAP restrictions.
  4321				 */
  4322				if (cr4_smap)
  4323					smapf = (pfec & (PFERR_RSVD_MASK|PFERR_FETCH_MASK)) ? 0 : kf;
  4324			}
  4325	
  4326			mmu->permissions[byte] = ff | uf | wf | smepf | smapf;
  4327		}
  4328	}
  4329	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

[Paolo Bonzini]

On 15/06/2018 20:45, Nick Desaulniers wrote:
>>
>>> 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.
> Spurious warning today, actual bug tomorrow?  I prefer to not to
> disable warnings wholesale.  They don't need to find actual bugs to be
> useful.  Flagging code that can be further specified does not hurt.
> Part of the effort to compile the kernel with different compilers is
> to add warning coverage, not remove it.  That said, there may be
> warnings that are never useful (or at least due to some invariant that
> only affects the kernel).  I cant think of any off the top of my head,
> but I'm also not sure this is one.

This one really makes the code uglier though, so I'm not really inclined
to applying the patch.

Paolo

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

[Nick Desaulniers]

On Tue, Jun 19, 2018 at 8:19 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 15/06/2018 20:45, Nick Desaulniers wrote:
> >>
> >>> 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.
> > Spurious warning today, actual bug tomorrow?  I prefer to not to
> > disable warnings wholesale.  They don't need to find actual bugs to be
> > useful.  Flagging code that can be further specified does not hurt.
> > Part of the effort to compile the kernel with different compilers is
> > to add warning coverage, not remove it.  That said, there may be
> > warnings that are never useful (or at least due to some invariant that
> > only affects the kernel).  I cant think of any off the top of my head,
> > but I'm also not sure this is one.
>
> This one really makes the code uglier though, so I'm not really inclined
> to applying the patch.

Note that of the three variables (w, u, x), only u is used later on.
What about declaring them as negated with the cast, that way there's
no cast in a ternary?

Ex:

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index d594690d8b95..53673ad4b295 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4261,8 +4261,9 @@ static void update_permission_bitmask(struct
kvm_vcpu *vcpu,
 {
        unsigned byte;

-       const u8 x = BYTE_MASK(ACC_EXEC_MASK);
-       const u8 w = BYTE_MASK(ACC_WRITE_MASK);
+       const u8 x_not = (u8)~BYTE_MASK(ACC_EXEC_MASK);
+       const u8 w_not = (u8)~BYTE_MASK(ACC_WRITE_MASK);
+       const u8 u_not = (u8)~BYTE_MASK(ACC_USER_MASK);
        const u8 u = BYTE_MASK(ACC_USER_MASK);

        bool cr4_smep = kvm_read_cr4_bits(vcpu, X86_CR4_SMEP) != 0;
@@ -4278,11 +4279,11 @@ static void update_permission_bitmask(struct
kvm_vcpu *vcpu,
                 */

                /* Faults from writes to non-writable pages */
-               u8 wf = (pfec & PFERR_WRITE_MASK) ? ~w : 0;
+               u8 wf = (pfec & PFERR_WRITE_MASK) ? w_not : 0;
                /* Faults from user mode accesses to supervisor pages */
-               u8 uf = (pfec & PFERR_USER_MASK) ? ~u : 0;
+               u8 uf = (pfec & PFERR_USER_MASK) ? u_not : 0;
                /* Faults from fetches of non-executable pages*/
-               u8 ff = (pfec & PFERR_FETCH_MASK) ? ~x : 0;
+               u8 ff = (pfec & PFERR_FETCH_MASK) ? x_not : 0;
                /* Faults from kernel mode fetches of user pages */
                u8 smepf = 0;
                /* Faults from kernel mode accesses of user pages */


Maybe you have a better naming scheme than *_not ? What do you think?
-- 
Thanks,
~Nick Desaulniers

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

[Paolo Bonzini]

On 19/06/2018 19:08, Nick Desaulniers wrote:
>> This one really makes the code uglier though, so I'm not really inclined
>> to applying the patch.
> Note that of the three variables (w, u, x), only u is used later on.
> What about declaring them as negated with the cast, that way there's
> no cast in a ternary?

I still find it inferior, but I guess it's at least acceptable.  I
prefer not_{x,w,u} though. :)

Paolo

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

[Joe Perches]

On Tue, 2018-06-19 at 10:08 -0700, Nick Desaulniers wrote:
> On Tue, Jun 19, 2018 at 8:19 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> > On 15/06/2018 20:45, Nick Desaulniers wrote:
> > > > 
> > > > > 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.
> > > 
> > > Spurious warning today, actual bug tomorrow?  I prefer to not to
> > > disable warnings wholesale.  They don't need to find actual bugs to be
> > > useful.  Flagging code that can be further specified does not hurt.
> > > Part of the effort to compile the kernel with different compilers is
> > > to add warning coverage, not remove it.  That said, there may be
> > > warnings that are never useful (or at least due to some invariant that
> > > only affects the kernel).  I cant think of any off the top of my head,
> > > but I'm also not sure this is one.
> > 
> > This one really makes the code uglier though, so I'm not really inclined
> > to applying the patch.
> 
> Note that of the three variables (w, u, x), only u is used later on.
> What about declaring them as negated with the cast, that way there's
> no cast in a ternary?

It'd be simpler to cast in the BYTE_MASK macro itself

Ex:
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index d594690d8b95..53673ad4b295 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -4261,8 +4261,9 @@ static void update_permission_bitmask(struct
> kvm_vcpu *vcpu,
>  {
>         unsigned byte;
> 
> -       const u8 x = BYTE_MASK(ACC_EXEC_MASK);
> -       const u8 w = BYTE_MASK(ACC_WRITE_MASK);
> +       const u8 x_not = (u8)~BYTE_MASK(ACC_EXEC_MASK);
> +       const u8 w_not = (u8)~BYTE_MASK(ACC_WRITE_MASK);
> +       const u8 u_not = (u8)~BYTE_MASK(ACC_USER_MASK);
>         const u8 u = BYTE_MASK(ACC_USER_MASK);
> 
> 
>         bool cr4_smep = kvm_read_cr4_bits(vcpu, X86_CR4_SMEP) != 0;
> @@ -4278,11 +4279,11 @@ static void update_permission_bitmask(struct
> kvm_vcpu *vcpu,
>                  */
> 
>                 /* Faults from writes to non-writable pages */
> -               u8 wf = (pfec & PFERR_WRITE_MASK) ? ~w : 0;
> +               u8 wf = (pfec & PFERR_WRITE_MASK) ? w_not : 0;
>                 /* Faults from user mode accesses to supervisor pages */
> -               u8 uf = (pfec & PFERR_USER_MASK) ? ~u : 0;
> +               u8 uf = (pfec & PFERR_USER_MASK) ? u_not : 0;
>                 /* Faults from fetches of non-executable pages*/
> -               u8 ff = (pfec & PFERR_FETCH_MASK) ? ~x : 0;
> +               u8 ff = (pfec & PFERR_FETCH_MASK) ? x_not : 0;
>                 /* Faults from kernel mode fetches of user pages */
>                 u8 smepf = 0;
>                 /* Faults from kernel mode accesses of user pages */
> 
> 
> Maybe you have a better naming scheme than *_not ? What do you think?

It'd be nicer to cast in the BYTE_MASK macro
and using "unsigned byte;" is misleading at best.

---
 arch/x86/kvm/mmu.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index d594690d8b95..201711aa99b9 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4246,15 +4246,14 @@ reset_ept_shadow_zero_bits_mask(struct kvm_vcpu *vcpu,
 				    boot_cpu_data.x86_phys_bits, execonly);
 }
 
-#define BYTE_MASK(access) \
-	((1 & (access) ? 2 : 0) | \
-	 (2 & (access) ? 4 : 0) | \
-	 (3 & (access) ? 8 : 0) | \
-	 (4 & (access) ? 16 : 0) | \
-	 (5 & (access) ? 32 : 0) | \
-	 (6 & (access) ? 64 : 0) | \
-	 (7 & (access) ? 128 : 0))
-
+#define BYTE_MASK(access)				\
+	((u8)(((access) & 1 ?   2 : 0) |		\
+	      ((access) & 2 ?   4 : 0) |		\
+	      ((access) & 3 ?   8 : 0) |		\
+	      ((access) & 4 ?  16 : 0) |		\
+	      ((access) & 5 ?  32 : 0) |		\
+	      ((access) & 6 ?  64 : 0) |		\
+	      ((access) & 7 ? 128 : 0)))
 
 static void update_permission_bitmask(struct kvm_vcpu *vcpu,
 				      struct kvm_mmu *mmu, bool ept)

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

[Paolo Bonzini]

On 19/06/2018 19:23, Joe Perches wrote:
> On Tue, 2018-06-19 at 10:08 -0700, Nick Desaulniers wrote:
>> On Tue, Jun 19, 2018 at 8:19 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>> On 15/06/2018 20:45, Nick Desaulniers wrote:
>>>>>
>>>>>> 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.
>>>>
>>>> Spurious warning today, actual bug tomorrow?  I prefer to not to
>>>> disable warnings wholesale.  They don't need to find actual bugs to be
>>>> useful.  Flagging code that can be further specified does not hurt.
>>>> Part of the effort to compile the kernel with different compilers is
>>>> to add warning coverage, not remove it.  That said, there may be
>>>> warnings that are never useful (or at least due to some invariant that
>>>> only affects the kernel).  I cant think of any off the top of my head,
>>>> but I'm also not sure this is one.
>>>
>>> This one really makes the code uglier though, so I'm not really inclined
>>> to applying the patch.
>>
>> Note that of the three variables (w, u, x), only u is used later on.
>> What about declaring them as negated with the cast, that way there's
>> no cast in a ternary?
> 
> It'd be simpler to cast in the BYTE_MASK macro itself

I don't think that would work, as the ~ would be done on a zero-extended
signed int.

Paolo

> Ex:
>>
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index d594690d8b95..53673ad4b295 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -4261,8 +4261,9 @@ static void update_permission_bitmask(struct
>> kvm_vcpu *vcpu,
>>  {
>>         unsigned byte;
>>
>> -       const u8 x = BYTE_MASK(ACC_EXEC_MASK);
>> -       const u8 w = BYTE_MASK(ACC_WRITE_MASK);
>> +       const u8 x_not = (u8)~BYTE_MASK(ACC_EXEC_MASK);
>> +       const u8 w_not = (u8)~BYTE_MASK(ACC_WRITE_MASK);
>> +       const u8 u_not = (u8)~BYTE_MASK(ACC_USER_MASK);
>>         const u8 u = BYTE_MASK(ACC_USER_MASK);
>>
>>
>>         bool cr4_smep = kvm_read_cr4_bits(vcpu, X86_CR4_SMEP) != 0;
>> @@ -4278,11 +4279,11 @@ static void update_permission_bitmask(struct
>> kvm_vcpu *vcpu,
>>                  */
>>
>>                 /* Faults from writes to non-writable pages */
>> -               u8 wf = (pfec & PFERR_WRITE_MASK) ? ~w : 0;
>> +               u8 wf = (pfec & PFERR_WRITE_MASK) ? w_not : 0;
>>                 /* Faults from user mode accesses to supervisor pages */
>> -               u8 uf = (pfec & PFERR_USER_MASK) ? ~u : 0;
>> +               u8 uf = (pfec & PFERR_USER_MASK) ? u_not : 0;
>>                 /* Faults from fetches of non-executable pages*/
>> -               u8 ff = (pfec & PFERR_FETCH_MASK) ? ~x : 0;
>> +               u8 ff = (pfec & PFERR_FETCH_MASK) ? x_not : 0;
>>                 /* Faults from kernel mode fetches of user pages */
>>                 u8 smepf = 0;
>>                 /* Faults from kernel mode accesses of user pages */
>>
>>
>> Maybe you have a better naming scheme than *_not ? What do you think?
> 
> It'd be nicer to cast in the BYTE_MASK macro
> and using "unsigned byte;" is misleading at best.
> 
> ---
>  arch/x86/kvm/mmu.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index d594690d8b95..201711aa99b9 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -4246,15 +4246,14 @@ reset_ept_shadow_zero_bits_mask(struct kvm_vcpu *vcpu,
>  				    boot_cpu_data.x86_phys_bits, execonly);
>  }
>  
> -#define BYTE_MASK(access) \
> -	((1 & (access) ? 2 : 0) | \
> -	 (2 & (access) ? 4 : 0) | \
> -	 (3 & (access) ? 8 : 0) | \
> -	 (4 & (access) ? 16 : 0) | \
> -	 (5 & (access) ? 32 : 0) | \
> -	 (6 & (access) ? 64 : 0) | \
> -	 (7 & (access) ? 128 : 0))
> -
> +#define BYTE_MASK(access)				\
> +	((u8)(((access) & 1 ?   2 : 0) |		\
> +	      ((access) & 2 ?   4 : 0) |		\
> +	      ((access) & 3 ?   8 : 0) |		\
> +	      ((access) & 4 ?  16 : 0) |		\
> +	      ((access) & 5 ?  32 : 0) |		\
> +	      ((access) & 6 ?  64 : 0) |		\
> +	      ((access) & 7 ? 128 : 0)))
>  
>  static void update_permission_bitmask(struct kvm_vcpu *vcpu,
>  				      struct kvm_mmu *mmu, bool ept)
> 

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

[Joe Perches]

On Tue, 2018-06-19 at 19:35 +0200, Paolo Bonzini wrote:
> On 19/06/2018 19:23, Joe Perches wrote:
> > On Tue, 2018-06-19 at 10:08 -0700, Nick Desaulniers wrote:
> > > On Tue, Jun 19, 2018 at 8:19 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > > 
> > > > On 15/06/2018 20:45, Nick Desaulniers wrote:
> > > > > > 
> > > > > > > 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.
> > > > > 
> > > > > Spurious warning today, actual bug tomorrow?  I prefer to not to
> > > > > disable warnings wholesale.  They don't need to find actual bugs to be
> > > > > useful.  Flagging code that can be further specified does not hurt.
> > > > > Part of the effort to compile the kernel with different compilers is
> > > > > to add warning coverage, not remove it.  That said, there may be
> > > > > warnings that are never useful (or at least due to some invariant that
> > > > > only affects the kernel).  I cant think of any off the top of my head,
> > > > > but I'm also not sure this is one.
> > > > 
> > > > This one really makes the code uglier though, so I'm not really inclined
> > > > to applying the patch.
> > > 
> > > Note that of the three variables (w, u, x), only u is used later on.
> > > What about declaring them as negated with the cast, that way there's
> > > no cast in a ternary?
> > 
> > It'd be simpler to cast in the BYTE_MASK macro itself
> 
> I don't think that would work, as the ~ would be done on a zero-extended
> signed int.

True, but the whole concept is dubious.
The implicit casts are all over the place,
not just in the file.

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

[Matthias Kaehlcke]

On Tue, Jun 19, 2018 at 11:07:47AM -0700, Joe Perches wrote:
> On Tue, 2018-06-19 at 19:35 +0200, Paolo Bonzini wrote:
> > On 19/06/2018 19:23, Joe Perches wrote:
> > > On Tue, 2018-06-19 at 10:08 -0700, Nick Desaulniers wrote:
> > > > On Tue, Jun 19, 2018 at 8:19 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > > > 
> > > > > On 15/06/2018 20:45, Nick Desaulniers wrote:
> > > > > > > 
> > > > > > > > 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.
> > > > > > 
> > > > > > Spurious warning today, actual bug tomorrow?  I prefer to not to
> > > > > > disable warnings wholesale.  They don't need to find actual bugs to be
> > > > > > useful.  Flagging code that can be further specified does not hurt.
> > > > > > Part of the effort to compile the kernel with different compilers is
> > > > > > to add warning coverage, not remove it.  That said, there may be
> > > > > > warnings that are never useful (or at least due to some invariant that
> > > > > > only affects the kernel).  I cant think of any off the top of my head,
> > > > > > but I'm also not sure this is one.
> > > > > 
> > > > > This one really makes the code uglier though, so I'm not really inclined
> > > > > to applying the patch.
> > > > 
> > > > Note that of the three variables (w, u, x), only u is used later on.
> > > > What about declaring them as negated with the cast, that way there's
> > > > no cast in a ternary?
> > > 
> > > It'd be simpler to cast in the BYTE_MASK macro itself
> > 
> > I don't think that would work, as the ~ would be done on a zero-extended
> > signed int.
> 
> True, but the whole concept is dubious.
> The implicit casts are all over the place,
> not just in the file.

Would that have been any different with the solution you proposed (if
it worked)?

Apparently both gcc and clang limit the warning to the potentially
more problematic case where a value with sign bit is promoted.

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

[Matthias Kaehlcke]

On Tue, Jun 19, 2018 at 07:13:41PM +0200, Paolo Bonzini wrote:
> On 19/06/2018 19:08, Nick Desaulniers wrote:
> >> This one really makes the code uglier though, so I'm not really inclined
> >> to applying the patch.
> > Note that of the three variables (w, u, x), only u is used later on.
> > What about declaring them as negated with the cast, that way there's
> > no cast in a ternary?
> 
> I still find it inferior, but I guess it's at least acceptable.  I
> prefer not_{x,w,u} though. :)

Thanks Nick and Paolo for the suggestions, I'll sent an updated
version soon.

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

[Joe Perches]

On Tue, 2018-06-19 at 11:36 -0700, Matthias Kaehlcke wrote:
> On Tue, Jun 19, 2018 at 11:07:47AM -0700, Joe Perches wrote:
> > On Tue, 2018-06-19 at 19:35 +0200, Paolo Bonzini wrote:
> > > On 19/06/2018 19:23, Joe Perches wrote:
> > > > On Tue, 2018-06-19 at 10:08 -0700, Nick Desaulniers wrote:
> > > > > On Tue, Jun 19, 2018 at 8:19 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > > > > 
> > > > > > On 15/06/2018 20:45, Nick Desaulniers wrote:
> > > > > > > > 
> > > > > > > > > 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.
> > > > > > > 
> > > > > > > Spurious warning today, actual bug tomorrow?  I prefer to not to
> > > > > > > disable warnings wholesale.  They don't need to find actual bugs to be
> > > > > > > useful.  Flagging code that can be further specified does not hurt.
> > > > > > > Part of the effort to compile the kernel with different compilers is
> > > > > > > to add warning coverage, not remove it.  That said, there may be
> > > > > > > warnings that are never useful (or at least due to some invariant that
> > > > > > > only affects the kernel).  I cant think of any off the top of my head,
> > > > > > > but I'm also not sure this is one.
> > > > > > 
> > > > > > This one really makes the code uglier though, so I'm not really inclined
> > > > > > to applying the patch.
> > > > > 
> > > > > Note that of the three variables (w, u, x), only u is used later on.
> > > > > What about declaring them as negated with the cast, that way there's
> > > > > no cast in a ternary?
> > > > 
> > > > It'd be simpler to cast in the BYTE_MASK macro itself
> > > 
> > > I don't think that would work, as the ~ would be done on a zero-extended
> > > signed int.
> > 
> > True, but the whole concept is dubious.
> > The implicit casts are all over the place,
> > not just in the file.
> 
> Would that have been any different with the solution you proposed (if
> it worked)?
> 
> Apparently both gcc and clang limit the warning to the potentially
> more problematic case where a value with sign bit is promoted.

I think the warning is exactly equivalent to -Wsign-conversion
and we don't normally compile the kernel with that either.

Trying to allow a "make W=3" to be compiler warning message free
is also silly.

I think it's better to make the warning emitted only at a W=3
level instead.

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

[Matthias Kaehlcke]

On Tue, Jun 19, 2018 at 12:11:27PM -0700, Joe Perches wrote:
> On Tue, 2018-06-19 at 11:36 -0700, Matthias Kaehlcke wrote:
> > On Tue, Jun 19, 2018 at 11:07:47AM -0700, Joe Perches wrote:
> > > On Tue, 2018-06-19 at 19:35 +0200, Paolo Bonzini wrote:
> > > > On 19/06/2018 19:23, Joe Perches wrote:
> > > > > On Tue, 2018-06-19 at 10:08 -0700, Nick Desaulniers wrote:
> > > > > > On Tue, Jun 19, 2018 at 8:19 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > > > > > 
> > > > > > > On 15/06/2018 20:45, Nick Desaulniers wrote:
> > > > > > > > > 
> > > > > > > > > > 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.
> > > > > > > > 
> > > > > > > > Spurious warning today, actual bug tomorrow?  I prefer to not to
> > > > > > > > disable warnings wholesale.  They don't need to find actual bugs to be
> > > > > > > > useful.  Flagging code that can be further specified does not hurt.
> > > > > > > > Part of the effort to compile the kernel with different compilers is
> > > > > > > > to add warning coverage, not remove it.  That said, there may be
> > > > > > > > warnings that are never useful (or at least due to some invariant that
> > > > > > > > only affects the kernel).  I cant think of any off the top of my head,
> > > > > > > > but I'm also not sure this is one.
> > > > > > > 
> > > > > > > This one really makes the code uglier though, so I'm not really inclined
> > > > > > > to applying the patch.
> > > > > > 
> > > > > > Note that of the three variables (w, u, x), only u is used later on.
> > > > > > What about declaring them as negated with the cast, that way there's
> > > > > > no cast in a ternary?
> > > > > 
> > > > > It'd be simpler to cast in the BYTE_MASK macro itself
> > > > 
> > > > I don't think that would work, as the ~ would be done on a zero-extended
> > > > signed int.
> > > 
> > > True, but the whole concept is dubious.
> > > The implicit casts are all over the place,
> > > not just in the file.
> > 
> > Would that have been any different with the solution you proposed (if
> > it worked)?
> > 
> > Apparently both gcc and clang limit the warning to the potentially
> > more problematic case where a value with sign bit is promoted.
> 
> I think the warning is exactly equivalent to -Wsign-conversion
> and we don't normally compile the kernel with that either.

I disagree with "exactly equivalent". With -Wsign-conversion a warning
is generated whenever a signed type is assigned to an unsigned
variable or viceversa.

-Wconstant-conversion is only issued when a *constant value* is assigned to
an incompatible type.

> Trying to allow a "make W=3" to be compiler warning message free
> is also silly.
> 
> I think it's better to make the warning emitted only at a W=3
> level instead.

Another difference with -Wsign-conversion is that enabling it would
probably result in thousands of warnings. Do you have evidence that
there is a significant number of spurious -Wconstant-conversion
warnings?

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

[Joe Perches]

On Tue, 2018-06-19 at 14:10 -0700, Matthias Kaehlcke wrote:
> On Tue, Jun 19, 2018 at 12:11:27PM -0700, Joe Perches wrote:
> > -Wconstant-conversion is only issued when a *constant value* is assigned to
> an incompatible type.
> 
> > Trying to allow a "make W=3" to be compiler warning message free
> > is also silly.
> > 
> > I think it's better to make the warning emitted only at a W=3
> > level instead.
> 
> Another difference with -Wsign-conversion is that enabling it would
> probably result in thousands of warnings. Do you have evidence that
> there is a significant number of spurious -Wconstant-conversion
> warnings?

Not my issue really.  You're advocating for
making the code more complex/ugly for a condition
where the result is identical.

Do you have evidence this is the only location in
an allyesconfig compilation?

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

[Matthias Kaehlcke]

On Tue, Jun 19, 2018 at 02:55:05PM -0700, Joe Perches wrote:
> On Tue, 2018-06-19 at 14:10 -0700, Matthias Kaehlcke wrote:
> > On Tue, Jun 19, 2018 at 12:11:27PM -0700, Joe Perches wrote:
> > > -Wconstant-conversion is only issued when a *constant value* is assigned to
> > an incompatible type.
> > 
> > > Trying to allow a "make W=3" to be compiler warning message free
> > > is also silly.
> > > 
> > > I think it's better to make the warning emitted only at a W=3
> > > level instead.
> > 
> > Another difference with -Wsign-conversion is that enabling it would
> > probably result in thousands of warnings. Do you have evidence that
> > there is a significant number of spurious -Wconstant-conversion
> > warnings?
> 
> Not my issue really.

Well, you advocate to disable a possibly useful warning globally ...

> You're advocating for making the code more complex/ugly for a
> condition where the result is identical.

My goal is no to make the code (slightly) more complex/ugly but have
the rest of the kernel benefit from a possibly useful warning.

> Do you have evidence this is the only location in
> an allyesconfig compilation?

I never claimed that this is the only location, but that it is not an
extremely noisy warning that can not be fixed without a major effort.

Since you asked:

v4.16 with an x86 allyesconfig and a few drivers disabled since they
don't build with clang (yet):

16 occurrences of -Wconstant-conversion, including the ones fixed by
this patch. Certainly no need for an endless churn of patches, and
given the limited number it also doesn't seem likely that new
instances will be added on a regular base.

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

[Joe Perches]

On Tue, 2018-06-19 at 16:45 -0700, Matthias Kaehlcke wrote:
> On Tue, Jun 19, 2018 at 02:55:05PM -0700, Joe Perches wrote:
> > Well, you advocate to disable a possibly useful warning globally ...
> > You're advocating for making the code more complex/ugly for a
> > condition where the result is identical.
> My goal is no to make the code (slightly) more complex/ugly but have
> the rest of the kernel benefit from a possibly useful warning.

For what case in the kernel is this warning useful?

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

[Matthias Kaehlcke]

On Tue, Jun 19, 2018 at 05:18:02PM -0700, Joe Perches wrote:
> On Tue, 2018-06-19 at 16:45 -0700, Matthias Kaehlcke wrote:
> > On Tue, Jun 19, 2018 at 02:55:05PM -0700, Joe Perches wrote:
> > > Well, you advocate to disable a possibly useful warning globally ...
> > > You're advocating for making the code more complex/ugly for a
> > > condition where the result is identical.
> > My goal is no to make the code (slightly) more complex/ugly but have
> > the rest of the kernel benefit from a possibly useful warning.
> 
> For what case in the kernel is this warning useful?

It's less about finding errors that are currently in the kernel (I
don't know if there are any) and more about preventing new ones from
creeping in.

Code similar to the example from the link shared by Nick could easily
be part of some kernel driver:

https://wiki.sei.cmu.edu/confluence/display/c/EXP14-C.+Beware+of+integer+promotion+when+performing+bitwise+operations+on+integer+types+smaller+than+int

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

[Paolo Bonzini]

On 20/06/2018 01:45, Matthias Kaehlcke wrote:
> 
> v4.16 with an x86 allyesconfig and a few drivers disabled since they
> don't build with clang (yet):
> 
> 16 occurrences of -Wconstant-conversion, including the ones fixed by
> this patch. Certainly no need for an endless churn of patches, and
> given the limited number it also doesn't seem likely that new
> instances will be added on a regular base.

Thanks for providing numbers!  The next question is: how many of these
16 occurrences are actual bugs?  This is what measures the usefulness of
the warning.

Paolo

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

[Matthias Kaehlcke]

On Wed, Jun 20, 2018 at 10:02:15AM +0200, Paolo Bonzini wrote:
> On 20/06/2018 01:45, Matthias Kaehlcke wrote:
> > 
> > v4.16 with an x86 allyesconfig and a few drivers disabled since they
> > don't build with clang (yet):
> > 
> > 16 occurrences of -Wconstant-conversion, including the ones fixed by
> > this patch. Certainly no need for an endless churn of patches, and
> > given the limited number it also doesn't seem likely that new
> > instances will be added on a regular base.
> 
> Thanks for providing numbers!  The next question is: how many of these
> 16 occurrences are actual bugs?

Probably none.

> This is what measures the usefulness of the warning.

I very much doubt that this is a useful metric given the limited
number of occurrences.

Let's assume that 1 out of 20 patches not doing the cast have an
actual problem. For 16 occurrences this means there's a 44% chance
that none of these instances is an error (18.5% for 1 out 10). This
is without taking into account that some of the instances are in the
same file and likey similar (as in the KVM case), and that most of the
code has undergone years of testing in the field.