| Message ID | 67ec8b468d6048f7f91590b59430df275b2f5870.1698053876.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix or deviate various instances of missing declarations | expand |
On 23.10.2023 11:56, Nicola Vetrini wrote: > --- a/xen/arch/x86/include/asm/asm_defns.h > +++ b/xen/arch/x86/include/asm/asm_defns.h > @@ -31,6 +31,7 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, " > * gets set up by the containing function. > */ > #ifdef CONFIG_FRAME_POINTER > +/* SAF-1-safe */ > register unsigned long current_stack_pointer asm("rsp"); > # define ASM_CALL_CONSTRAINT , "+r" (current_stack_pointer) > #else SAF-1-safe is about symbols "used only by asm modules". This doesn't apply to the declaration here. > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -75,6 +75,7 @@ static bool __initdata opt_invpcid = true; > boolean_param("invpcid", opt_invpcid); > bool __read_mostly use_invpcid; > > +/* SAF-1-safe Only used in asm code and within this source file */ > unsigned long __read_mostly cr4_pv32_mask; > > /* **** Linux config option: propagated to domain0. */ > @@ -147,12 +148,13 @@ cpumask_t __read_mostly cpu_present_map; > unsigned long __read_mostly xen_phys_start; > > char __section(".init.bss.stack_aligned") __aligned(STACK_SIZE) > - cpu0_stack[STACK_SIZE]; > + cpu0_stack[STACK_SIZE]; /* SAF-1-safe Only used in asm code and below */ Wasn't it that such comments need to live on the earlier line? Jan
On 23.10.2023 11:56, Nicola Vetrini wrote: > To avoid a violation of MISRA C:2012 Rule 8.4, as permitted > by docs/misra/rules.rst. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > Changes in v3: > - Edited commit message > - Add two new variables Oh, also: The R-b dates back to v1. Imo any addition invalidates prior R-b, unless indicated otherwise by the person offering it (which isn't the case here afaict). Jan
On 24/10/2023 09:32, Jan Beulich wrote: > On 23.10.2023 11:56, Nicola Vetrini wrote: >> --- a/xen/arch/x86/include/asm/asm_defns.h >> +++ b/xen/arch/x86/include/asm/asm_defns.h >> @@ -31,6 +31,7 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, " >> * gets set up by the containing function. >> */ >> #ifdef CONFIG_FRAME_POINTER >> +/* SAF-1-safe */ >> register unsigned long current_stack_pointer asm("rsp"); >> # define ASM_CALL_CONSTRAINT , "+r" (current_stack_pointer) >> #else > > SAF-1-safe is about symbols "used only by asm modules". This doesn't > apply > to the declaration here. > The wording could change to "asm code" if that is deemed clearer. >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -75,6 +75,7 @@ static bool __initdata opt_invpcid = true; >> boolean_param("invpcid", opt_invpcid); >> bool __read_mostly use_invpcid; >> >> +/* SAF-1-safe Only used in asm code and within this source file */ >> unsigned long __read_mostly cr4_pv32_mask; >> >> /* **** Linux config option: propagated to domain0. */ >> @@ -147,12 +148,13 @@ cpumask_t __read_mostly cpu_present_map; >> unsigned long __read_mostly xen_phys_start; >> >> char __section(".init.bss.stack_aligned") __aligned(STACK_SIZE) >> - cpu0_stack[STACK_SIZE]; >> + cpu0_stack[STACK_SIZE]; /* SAF-1-safe Only used in asm code and >> below */ > > Wasn't it that such comments need to live on the earlier line? > > Jan On the same line is fine as well. I personally found it less clear putting that in the line above.
On 24.10.2023 09:58, Nicola Vetrini wrote: > On 24/10/2023 09:32, Jan Beulich wrote: >> On 23.10.2023 11:56, Nicola Vetrini wrote: >>> --- a/xen/arch/x86/include/asm/asm_defns.h >>> +++ b/xen/arch/x86/include/asm/asm_defns.h >>> @@ -31,6 +31,7 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, " >>> * gets set up by the containing function. >>> */ >>> #ifdef CONFIG_FRAME_POINTER >>> +/* SAF-1-safe */ >>> register unsigned long current_stack_pointer asm("rsp"); >>> # define ASM_CALL_CONSTRAINT , "+r" (current_stack_pointer) >>> #else >> >> SAF-1-safe is about symbols "used only by asm modules". This doesn't >> apply >> to the declaration here. >> > > The wording could change to "asm code" if that is deemed clearer. Question is what would be meant by "asm code"; "asm modules" is quite clear. >>> --- a/xen/arch/x86/setup.c >>> +++ b/xen/arch/x86/setup.c >>> @@ -75,6 +75,7 @@ static bool __initdata opt_invpcid = true; >>> boolean_param("invpcid", opt_invpcid); >>> bool __read_mostly use_invpcid; >>> >>> +/* SAF-1-safe Only used in asm code and within this source file */ >>> unsigned long __read_mostly cr4_pv32_mask; >>> >>> /* **** Linux config option: propagated to domain0. */ >>> @@ -147,12 +148,13 @@ cpumask_t __read_mostly cpu_present_map; >>> unsigned long __read_mostly xen_phys_start; >>> >>> char __section(".init.bss.stack_aligned") __aligned(STACK_SIZE) >>> - cpu0_stack[STACK_SIZE]; >>> + cpu0_stack[STACK_SIZE]; /* SAF-1-safe Only used in asm code and >>> below */ >> >> Wasn't it that such comments need to live on the earlier line? > > On the same line is fine as well. I personally found it less clear > putting that in the > line above. But please recall that these comments are intended to cover other scanners as well. Iirc only Eclair accepts comments on the same line. Nevertheless I realize that putting the comment on the earlier line is problematic (and maybe also scanner dependent) when that ends up in the middle of a declaration / definition. Jan
On 24/10/2023 10:12, Jan Beulich wrote: > On 24.10.2023 09:58, Nicola Vetrini wrote: >> On 24/10/2023 09:32, Jan Beulich wrote: >>> On 23.10.2023 11:56, Nicola Vetrini wrote: >>>> --- a/xen/arch/x86/include/asm/asm_defns.h >>>> +++ b/xen/arch/x86/include/asm/asm_defns.h >>>> @@ -31,6 +31,7 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, " >>>> * gets set up by the containing function. >>>> */ >>>> #ifdef CONFIG_FRAME_POINTER >>>> +/* SAF-1-safe */ >>>> register unsigned long current_stack_pointer asm("rsp"); >>>> # define ASM_CALL_CONSTRAINT , "+r" (current_stack_pointer) >>>> #else >>> >>> SAF-1-safe is about symbols "used only by asm modules". This doesn't >>> apply >>> to the declaration here. >>> >> >> The wording could change to "asm code" if that is deemed clearer. > > Question is what would be meant by "asm code"; "asm modules" is quite > clear. > Well, I don't know. It's up to the community to decide that. It can be an ad-hoc justification, but I don't see much value in doing so. What do you propose to get this patch approved (at least on your account)?. >>>> --- a/xen/arch/x86/setup.c >>>> +++ b/xen/arch/x86/setup.c >>>> @@ -75,6 +75,7 @@ static bool __initdata opt_invpcid = true; >>>> boolean_param("invpcid", opt_invpcid); >>>> bool __read_mostly use_invpcid; >>>> >>>> +/* SAF-1-safe Only used in asm code and within this source file */ >>>> unsigned long __read_mostly cr4_pv32_mask; >>>> >>>> /* **** Linux config option: propagated to domain0. */ >>>> @@ -147,12 +148,13 @@ cpumask_t __read_mostly cpu_present_map; >>>> unsigned long __read_mostly xen_phys_start; >>>> >>>> char __section(".init.bss.stack_aligned") __aligned(STACK_SIZE) >>>> - cpu0_stack[STACK_SIZE]; >>>> + cpu0_stack[STACK_SIZE]; /* SAF-1-safe Only used in asm code and >>>> below */ >>> >>> Wasn't it that such comments need to live on the earlier line? >> >> On the same line is fine as well. I personally found it less clear >> putting that in the >> line above. > > But please recall that these comments are intended to cover other > scanners as well. Iirc only Eclair accepts comments on the same line. > Nevertheless I realize that putting the comment on the earlier line > is problematic (and maybe also scanner dependent) when that ends up > in the middle of a declaration / definition. > > Jan
On 24.10.2023 15:40, Nicola Vetrini wrote: > On 24/10/2023 10:12, Jan Beulich wrote: >> On 24.10.2023 09:58, Nicola Vetrini wrote: >>> On 24/10/2023 09:32, Jan Beulich wrote: >>>> On 23.10.2023 11:56, Nicola Vetrini wrote: >>>>> --- a/xen/arch/x86/include/asm/asm_defns.h >>>>> +++ b/xen/arch/x86/include/asm/asm_defns.h >>>>> @@ -31,6 +31,7 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, " >>>>> * gets set up by the containing function. >>>>> */ >>>>> #ifdef CONFIG_FRAME_POINTER >>>>> +/* SAF-1-safe */ >>>>> register unsigned long current_stack_pointer asm("rsp"); >>>>> # define ASM_CALL_CONSTRAINT , "+r" (current_stack_pointer) >>>>> #else >>>> >>>> SAF-1-safe is about symbols "used only by asm modules". This doesn't >>>> apply >>>> to the declaration here. >>>> >>> >>> The wording could change to "asm code" if that is deemed clearer. >> >> Question is what would be meant by "asm code"; "asm modules" is quite >> clear. >> > > Well, I don't know. It's up to the community to decide that. It can be > an ad-hoc > justification, but I don't see much value in doing so. What do you > propose to get this patch > approved (at least on your account)?. Drop this change and have Eclair recognize that what we're talking about here is just a declaration, not a definition. Jan
diff --git a/xen/arch/x86/include/asm/asm_defns.h b/xen/arch/x86/include/asm/asm_defns.h index baaaccb26e17..a2516de7749b 100644 --- a/xen/arch/x86/include/asm/asm_defns.h +++ b/xen/arch/x86/include/asm/asm_defns.h @@ -31,6 +31,7 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, " * gets set up by the containing function. */ #ifdef CONFIG_FRAME_POINTER +/* SAF-1-safe */ register unsigned long current_stack_pointer asm("rsp"); # define ASM_CALL_CONSTRAINT , "+r" (current_stack_pointer) #else diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 08ba1f95d635..4480f08de718 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -75,6 +75,7 @@ static bool __initdata opt_invpcid = true; boolean_param("invpcid", opt_invpcid); bool __read_mostly use_invpcid; +/* SAF-1-safe Only used in asm code and within this source file */ unsigned long __read_mostly cr4_pv32_mask; /* **** Linux config option: propagated to domain0. */ @@ -147,12 +148,13 @@ cpumask_t __read_mostly cpu_present_map; unsigned long __read_mostly xen_phys_start; char __section(".init.bss.stack_aligned") __aligned(STACK_SIZE) - cpu0_stack[STACK_SIZE]; + cpu0_stack[STACK_SIZE]; /* SAF-1-safe Only used in asm code and below */ /* Used by the BSP/AP paths to find the higher half stack mapping to use. */ void *stack_start = cpu0_stack + STACK_SIZE - sizeof(struct cpu_info); /* Used by the boot asm to stash the relocated multiboot info pointer. */ +/* SAF-1-safe */ unsigned int __initdata multiboot_ptr; struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 0, 0, -1 };