x86: coco: mark cc_mask as __maybe_unused

[PATCH] x86: coco: mark cc_mask as __maybe_unused

Posted by Arnd Bergmann 11 months, 1 week ago

From: Arnd Bergmann <arnd@arndb.de>

When extra warnings are enabled, the cc_mask definition in asm/coco.h
causes a build failure with gcc:

arch/x86/include/asm/coco.h:28:18: error: 'cc_mask' defined but not used [-Werror=unused-const-variable=]
   28 | static const u64 cc_mask = 0;

Mark this one as __maybe_unused.

Fixes: a0a8d15a798b ("x86/tdx: Preserve shared bit on mprotect()")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/x86/include/asm/coco.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h
index aa6c8f8ca958..9e9204cfca6f 100644
--- a/arch/x86/include/asm/coco.h
+++ b/arch/x86/include/asm/coco.h
@@ -25,7 +25,7 @@ u64 cc_mkdec(u64 val);
 void cc_random_init(void);
 #else
 #define cc_vendor (CC_VENDOR_NONE)
-static const u64 cc_mask = 0;
+static const __maybe_unused u64 cc_mask = 0;
 
 static inline u64 cc_mkenc(u64 val)
 {
-- 
2.39.5

Re: [PATCH] x86: coco: mark cc_mask as __maybe_unused

Posted by Ingo Molnar 11 months, 1 week ago

* Arnd Bergmann <arnd@kernel.org> wrote:

> From: Arnd Bergmann <arnd@arndb.de>
> 
> When extra warnings are enabled, the cc_mask definition in asm/coco.h
> causes a build failure with gcc:
> 
> arch/x86/include/asm/coco.h:28:18: error: 'cc_mask' defined but not used [-Werror=unused-const-variable=]
>    28 | static const u64 cc_mask = 0;
> 
> Mark this one as __maybe_unused.
> 
> Fixes: a0a8d15a798b ("x86/tdx: Preserve shared bit on mprotect()")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  arch/x86/include/asm/coco.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h
> index aa6c8f8ca958..9e9204cfca6f 100644
> --- a/arch/x86/include/asm/coco.h
> +++ b/arch/x86/include/asm/coco.h
> @@ -25,7 +25,7 @@ u64 cc_mkdec(u64 val);
>  void cc_random_init(void);
>  #else
>  #define cc_vendor (CC_VENDOR_NONE)
> -static const u64 cc_mask = 0;
> +static const __maybe_unused u64 cc_mask = 0;

So I detest __maybe_unused with a vengeance: the 'maybe' unnecessarily 
inserts uncertainty & probability language into the text, while there's 
nothing uncertain about this interface or the code. Why cannot the 
compiler figure it out?

Anyway, I'd suggest we change direct usage of cc_mask to a 
get_cc_mask() inline function instead, this will resolve the warning, 
plus it avoids some messy looking variable shadowing in tdx.c AFAICS:

  arch/x86/coco/tdx/tdx.c:static void tdx_setup(u64 *cc_mask)

Thanks,

	Ingo

Re: [PATCH] x86: coco: mark cc_mask as __maybe_unused

Posted by Arnd Bergmann 11 months ago

On Wed, Mar 5, 2025, at 11:44, Ingo Molnar wrote:
> * Arnd Bergmann <arnd@kernel.org> wrote:
>
>> @@ -25,7 +25,7 @@ u64 cc_mkdec(u64 val);
>>  void cc_random_init(void);
>>  #else
>>  #define cc_vendor (CC_VENDOR_NONE)
>> -static const u64 cc_mask = 0;
>> +static const __maybe_unused u64 cc_mask = 0;
>
> So I detest __maybe_unused with a vengeance: the 'maybe' unnecessarily 
> inserts uncertainty & probability language into the text, while there's 
> nothing uncertain about this interface or the code. Why cannot the 
> compiler figure it out?

Right, I'm also trying to remove a lot of the __maybe_unused
annotations in places like power management and device_id tables
where we now have better solutions in place.

> Anyway, I'd suggest we change direct usage of cc_mask to a 
> get_cc_mask() inline function instead, this will resolve the warning, 
> plus it avoids some messy looking variable shadowing in tdx.c AFAICS:
>
>   arch/x86/coco/tdx/tdx.c:static void tdx_setup(u64 *cc_mask)

I've sent a v2 now.

      Arnd

Re: [PATCH] x86: coco: mark cc_mask as __maybe_unused

Posted by Borislav Petkov 11 months, 1 week ago

On Wed, Mar 05, 2025 at 11:44:34AM +0100, Ingo Molnar wrote:
> 
> * Arnd Bergmann <arnd@kernel.org> wrote:
> 
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > When extra warnings are enabled, the cc_mask definition in asm/coco.h
> > causes a build failure with gcc:
> > 
> > arch/x86/include/asm/coco.h:28:18: error: 'cc_mask' defined but not used [-Werror=unused-const-variable=]
> >    28 | static const u64 cc_mask = 0;
> > 
> > Mark this one as __maybe_unused.
> > 
> > Fixes: a0a8d15a798b ("x86/tdx: Preserve shared bit on mprotect()")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> >  arch/x86/include/asm/coco.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h
> > index aa6c8f8ca958..9e9204cfca6f 100644
> > --- a/arch/x86/include/asm/coco.h
> > +++ b/arch/x86/include/asm/coco.h
> > @@ -25,7 +25,7 @@ u64 cc_mkdec(u64 val);
> >  void cc_random_init(void);
> >  #else
> >  #define cc_vendor (CC_VENDOR_NONE)
> > -static const u64 cc_mask = 0;
> > +static const __maybe_unused u64 cc_mask = 0;
> 
> So I detest __maybe_unused with a vengeance: the 'maybe' unnecessarily 
> inserts uncertainty & probability language into the text, while there's 
> nothing uncertain about this interface or the code. Why cannot the 
> compiler figure it out?
> 
> Anyway, I'd suggest we change direct usage of cc_mask to a 
> get_cc_mask() inline function instead, this will resolve the warning, 
> plus it avoids some messy looking variable shadowing in tdx.c AFAICS:
> 
>   arch/x86/coco/tdx/tdx.c:static void tdx_setup(u64 *cc_mask)

Touch ~17 spots:

$ git grep -w cc_mask arch/x86 | wc -l
17

just because of some stupid gcc extra warning switch?

I say disable the gcc warning. Pfft.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86: coco: mark cc_mask as __maybe_unused

Posted by Borislav Petkov 11 months, 1 week ago

On Wed, Mar 05, 2025 at 11:17:00PM +0100, Borislav Petkov wrote:
> just because of some stupid gcc extra warning switch?

This warning has been kicked out into W1 once already for too many false
positives:

c9c6837d3931 ("kbuild: move -Wunused-const-variable to W=1 warning level")

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86: coco: mark cc_mask as __maybe_unused

Posted by Arnd Bergmann 11 months, 1 week ago

On Wed, Mar 5, 2025, at 23:20, Borislav Petkov wrote:
> On Wed, Mar 05, 2025 at 11:17:00PM +0100, Borislav Petkov wrote:
>> 
>> Touch ~17 spots:
>> 
>> $ git grep -w cc_mask arch/x86 | wc -l
>> 17
>> 
>> just because of some stupid gcc extra warning switch?

It's only one outside of CONFIG_ARCH_HAS_CC_PLATFORM, the
other ones always see the 'extern' declaration.

> This warning has been kicked out into W1 once already for too many false
> positives:
>
> c9c6837d3931 ("kbuild: move -Wunused-const-variable to W=1 warning level")

Yes, that was me. We have now come to the point where only about
a dozen instances are left and I resubmitted the remaining patches
for most of them.

There is a twist here: clang by default warns about unused const
variables in .c files but not in headers, while gcc doesn't
warn about them at all unless it's explictly enabled, and then
it warns about both of them. Newer gcc versions have a distinct
-Wunused-const-variable=1 for the clang behavior and
-Wunused-const-variable=2 that warns for both, so we could
reasonably decide to enable the =1 version by default and
leave the =2 version for W=2.

On the other hand, most of the users of 'static const' variables
in headers are rather dumb and should just be moved into the
file that uses them, or they can be replaced with a #define
or an enum.

In this case, the only user is a macro:
#define _PAGE_CC               (_AT(pteval_t, cc_mask))

so maybe '#define cc_mask 0' would be appropriate.

    Arnd

Re: [PATCH] x86: coco: mark cc_mask as __maybe_unused

Posted by Borislav Petkov 11 months, 1 week ago

On Wed, Mar 05, 2025 at 11:45:11PM +0100, Arnd Bergmann wrote:
> There is a twist here: clang by default warns about unused const
> variables in .c files but not in headers, while gcc doesn't

What is the point of this warning, do you know?

Someone defines a const, forgets to use it and? Oh big deal. This should be
a -Wunused anyway, no?

I must be missing something here...

> warn about them at all unless it's explictly enabled, and then
> it warns about both of them. Newer gcc versions have a distinct
> -Wunused-const-variable=1 for the clang behavior and
> -Wunused-const-variable=2 that warns for both, so we could
> reasonably decide to enable the =1 version by default and
> leave the =2 version for W=2.
> 
> On the other hand, most of the users of 'static const' variables
> in headers are rather dumb and should just be moved into the
> file that uses them, or they can be replaced with a #define
> or an enum.
> 
> In this case, the only user is a macro:
> #define _PAGE_CC               (_AT(pteval_t, cc_mask))
> 
> so maybe '#define cc_mask 0' would be appropriate.

Sounds a lot better to me.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86: coco: mark cc_mask as __maybe_unused

Posted by Arnd Bergmann 11 months ago

On Wed, Mar 5, 2025, at 23:50, Borislav Petkov wrote:
> On Wed, Mar 05, 2025 at 11:45:11PM +0100, Arnd Bergmann wrote:
>> There is a twist here: clang by default warns about unused const
>> variables in .c files but not in headers, while gcc doesn't
>
> What is the point of this warning, do you know?
>
> Someone defines a const, forgets to use it and? Oh big deal. This should be
> a -Wunused anyway, no?
>
> I must be missing something here...

We turned on -Wunused a while ago for default builds after all the
-Wunused-variable warnings got addressed, but instead turned off
-Wunused-const-variable and -Wunused-but-set-variable
unless W=1 is set while there are still existing warnings.

In my opinion, there is little difference between unused const and
non-const variables, the reason that gcc treats them differently
seems to be from common c++ coding style advocating for them to be
used in place of macros. This is the case here, but most of the
warnings it actually shows are for mistakes where some variable
is in the wrong #ifdef block or the only user got removed.

>> In this case, the only user is a macro:
>> #define _PAGE_CC               (_AT(pteval_t, cc_mask))
>> 
>> so maybe '#define cc_mask 0' would be appropriate.
>
> Sounds a lot better to me.

Too bad that did not work. This version is also a bit ugly:

diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index c90e9c51edb7..f31c1a31742d 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -179,7 +179,11 @@ enum page_cache_mode {
 };
 #endif
 
+#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
 #define _PAGE_CC               (_AT(pteval_t, cc_mask))
+#else
+#define _PAGE_CC               (_AT(pteval_t, 0))
+#endif
 #define _PAGE_ENC              (_AT(pteval_t, sme_me_mask))
 
 #define _PAGE_CACHE_MASK       (_PAGE_PWT | _PAGE_PCD | _PAGE_PAT)

so I'll just follow Ingo's earlier suggestion for the v2 patch.

       Arnd

Re: [PATCH] x86: coco: mark cc_mask as __maybe_unused

Posted by Huang, Kai 11 months, 1 week ago

> 
> > warn about them at all unless it's explictly enabled, and then
> > it warns about both of them. Newer gcc versions have a distinct
> > -Wunused-const-variable=1 for the clang behavior and
> > -Wunused-const-variable=2 that warns for both, so we could
> > reasonably decide to enable the =1 version by default and
> > leave the =2 version for W=2.
> > 
> > On the other hand, most of the users of 'static const' variables
> > in headers are rather dumb and should just be moved into the
> > file that uses them, or they can be replaced with a #define
> > or an enum.
> > 
> > In this case, the only user is a macro:
> > #define _PAGE_CC               (_AT(pteval_t, cc_mask))
> > 
> > so maybe '#define cc_mask 0' would be appropriate.
> 
> Sounds a lot better to me.
> 

I actually tried this with CONFIG_ARCH_HAS_CC_PLATFORM off yesterday but got
below error:

$ make -j$(nproc)
mkdir -p /work/enabling/src/linux/tools/objtool && make
O=/work/enabling/src/linux subdir=tools/objtool --no-print-directory -C objtool 
  CALL    scripts/checksyscalls.sh
  INSTALL libsubcmd_headers
  CC      drivers/char/tpm/tpm2-cmd.o
  CC      drivers/char/tpm/tpmrm-dev.o
  CC      drivers/char/tpm/tpm2-space.o
  CC      drivers/char/tpm/tpm-sysfs.o
In file included from ./arch/x86/include/asm/pgtable.h:23,
                 from ./arch/x86/include/asm/tlbflush.h:16,
                 from ./arch/x86/include/asm/uaccess.h:17,
                 from ./include/linux/uaccess.h:12,
                 from ./include/linux/sched/task.h:13,
                 from ./include/linux/sched/signal.h:9,
                 from ./include/linux/rcuwait.h:6,
                 from ./include/linux/percpu-rwsem.h:7,
                 from ./include/linux/fs.h:33,
                 from ./include/linux/tpm.h:23,
                 from drivers/char/tpm/tpm.h:27,
                 from drivers/char/tpm/tpm2-cmd.c:14:
drivers/char/tpm/tpm2-cmd.c: In function ‘tpm2_find_cc’:
./arch/x86/include/asm/coco.h:28:17: error: expected identifier or ‘(’ before
numeric constant
   28 | #define cc_mask 0
      |                 ^
drivers/char/tpm/tpm2-cmd.c:813:13: note: in expansion of macro ‘cc_mask’
  813 |         u32 cc_mask;
      |             ^~~~~~~
drivers/char/tpm/tpm2-cmd.c:816:17: error: lvalue required as left operand of
assignment
  816 |         cc_mask = 1 << TPM2_CC_ATTR_VENDOR | GENMASK(15, 0);
      |                 ^


Rename the local variable 'cc_mask' in drivers/char/tpm/tpm2-cmd.c fix the
build, but I think the real issue is the 'cc_mask' in asm/coco.h is too common
to be a global visible variable/macro.

Re: [PATCH] x86: coco: mark cc_mask as __maybe_unused

Posted by Kirill A. Shutemov 11 months, 1 week ago

On Tue, Mar 04, 2025 at 03:33:34PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> When extra warnings are enabled, the cc_mask definition in asm/coco.h
> causes a build failure with gcc:
> 
> arch/x86/include/asm/coco.h:28:18: error: 'cc_mask' defined but not used [-Werror=unused-const-variable=]
>    28 | static const u64 cc_mask = 0;
> 
> Mark this one as __maybe_unused.
> 
> Fixes: a0a8d15a798b ("x86/tdx: Preserve shared bit on mprotect()")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] x86: coco: mark cc_mask as __maybe_unused

Posted by Sathyanarayanan Kuppuswamy 11 months, 1 week ago

On 3/4/25 6:33 AM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> When extra warnings are enabled, the cc_mask definition in asm/coco.h
> causes a build failure with gcc:
>
> arch/x86/include/asm/coco.h:28:18: error: 'cc_mask' defined but not used [-Werror=unused-const-variable=]
>     28 | static const u64 cc_mask = 0;
>
> Mark this one as __maybe_unused.
>
> Fixes: a0a8d15a798b ("x86/tdx: Preserve shared bit on mprotect()")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
Looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan 
<sathyanarayanan.kuppuswamy@linux.intel.com>
>   arch/x86/include/asm/coco.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h
> index aa6c8f8ca958..9e9204cfca6f 100644
> --- a/arch/x86/include/asm/coco.h
> +++ b/arch/x86/include/asm/coco.h
> @@ -25,7 +25,7 @@ u64 cc_mkdec(u64 val);
>   void cc_random_init(void);
>   #else
>   #define cc_vendor (CC_VENDOR_NONE)
> -static const u64 cc_mask = 0;
> +static const __maybe_unused u64 cc_mask = 0;
>   
>   static inline u64 cc_mkenc(u64 val)
>   {

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer