[XEN PATCH][for-4.19 v2 0/7] Fix or deviate various instances of missing declarations

[XEN PATCH][for-4.19 v2 0/7] Fix or deviate various instances of missing declarations

[Nicola Vetrini]

The patches in this series aim to fix or deviate various instances where a
function or variable do not have a declaration visible when such entity is
defined (in violation of MISRA C:2012 Rule 8.4).
An exception listed under docs/misra/rules.rst allows asm-only functions and
variables to be exempted, while the other instances are either changed
(e.g., making them static) or a missing header inclusion is added.

Some of the patches in this series are potential candidates for bug fixes, or
just general improvements that may be suited for inclusion in the next rc.

The deviation comment used by these patches is still SAF-x-safe, which is being
phased out in favour of some other name, as discussed here [1]. When such a name
has been determined, a new (trivial) version of this series can be produced,
the change could be made on commit or as part of the SAF rename troughout the
codebase.

Nicola Vetrini (7):
  xen: add declarations for variables where needed
  x86: add deviations for variables only used in asm code
  x86: add deviation comments for  asm-only functions
  x86/grant: switch included header to make declarations visible
  x86/vm_event: add missing include for hvm_vm_event_do_resume
  xen/console: make function static inline
  x86/mem_access: make function static

 xen/arch/arm/include/asm/setup.h           |  3 +++
 xen/arch/arm/include/asm/smp.h             |  3 +++
 xen/arch/arm/platform_hypercall.c          |  2 +-
 xen/arch/x86/cpu/mcheck/mce.c              |  6 +++---
 xen/arch/x86/hvm/grant_table.c             |  3 +--
 xen/arch/x86/hvm/svm/intr.c                |  1 +
 xen/arch/x86/hvm/svm/nestedsvm.c           |  1 +
 xen/arch/x86/hvm/svm/svm.c                 |  2 ++
 xen/arch/x86/hvm/vm_event.c                |  1 +
 xen/arch/x86/include/asm/asm_defns.h       |  1 +
 xen/arch/x86/include/asm/compat.h          |  1 +
 xen/arch/x86/include/asm/hvm/grant_table.h |  2 ++
 xen/arch/x86/include/asm/setup.h           |  1 +
 xen/arch/x86/irq.c                         |  2 +-
 xen/arch/x86/mm/mem_access.c               |  2 +-
 xen/arch/x86/setup.c                       |  1 +
 xen/arch/x86/traps.c                       |  1 +
 xen/arch/x86/x86_64/traps.c                |  1 +
 xen/common/symbols.c                       | 17 -----------------
 xen/include/xen/consoled.h                 |  2 +-
 xen/include/xen/symbols.h                  | 18 ++++++++++++++++++
 21 files changed, 45 insertions(+), 26 deletions(-)

--
2.34.1

[XEN PATCH v2 1/7] xen: add declarations for variables where needed

[Nicola Vetrini]

Some variables with external linkage used in C code do not have
a visible declaration where they are defined. Providing such
declaration also resolves violations of MISRA C:2012 Rule 8.4.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- make xenpf_lock static on ARM
- moved the declaration of cr4_pv32_mask to the appropriate file
---
 xen/arch/arm/include/asm/setup.h  |  3 +++
 xen/arch/arm/include/asm/smp.h    |  3 +++
 xen/arch/arm/platform_hypercall.c |  2 +-
 xen/arch/x86/cpu/mcheck/mce.c     |  6 +++---
 xen/arch/x86/include/asm/compat.h |  1 +
 xen/arch/x86/include/asm/setup.h  |  1 +
 xen/arch/x86/irq.c                |  2 +-
 xen/common/symbols.c              | 17 -----------------
 xen/include/xen/symbols.h         | 18 ++++++++++++++++++
 9 files changed, 31 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index b8866c20f462..8806a74b216d 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -183,9 +183,12 @@ int map_range_to_domain(const struct dt_device_node *dev,
 extern lpae_t boot_pgtable[XEN_PT_LPAE_ENTRIES];

 #ifdef CONFIG_ARM_64
+extern lpae_t boot_first[XEN_PT_LPAE_ENTRIES];
 extern lpae_t boot_first_id[XEN_PT_LPAE_ENTRIES];
 #endif
+extern lpae_t boot_second[XEN_PT_LPAE_ENTRIES];
 extern lpae_t boot_second_id[XEN_PT_LPAE_ENTRIES];
+extern lpae_t boot_third[XEN_PT_LPAE_ENTRIES * XEN_NR_ENTRIES(2)];
 extern lpae_t boot_third_id[XEN_PT_LPAE_ENTRIES];

 /* Find where Xen will be residing at runtime and return a PT entry */
diff --git a/xen/arch/arm/include/asm/smp.h b/xen/arch/arm/include/asm/smp.h
index 4fabdf5310d8..28bf24a01d95 100644
--- a/xen/arch/arm/include/asm/smp.h
+++ b/xen/arch/arm/include/asm/smp.h
@@ -6,6 +6,9 @@
 #include <asm/current.h>
 #endif

+extern struct init_info init_data;
+extern unsigned long smp_up_cpu;
+
 DECLARE_PER_CPU(cpumask_var_t, cpu_sibling_mask);
 DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask);

diff --git a/xen/arch/arm/platform_hypercall.c b/xen/arch/arm/platform_hypercall.c
index 743687a30390..fde4bc3e5809 100644
--- a/xen/arch/arm/platform_hypercall.c
+++ b/xen/arch/arm/platform_hypercall.c
@@ -17,7 +17,7 @@
 #include <asm/current.h>
 #include <asm/event.h>

-DEFINE_SPINLOCK(xenpf_lock);
+static DEFINE_SPINLOCK(xenpf_lock);

 long do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
 {
diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 6141b7eb9cf1..e855f958030d 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -1682,13 +1682,13 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc)
     return ret;
 }

-int mcinfo_dumpped;
+static int mcinfo_dumped;
 static int cf_check x86_mcinfo_dump_panic(mctelem_cookie_t mctc)
 {
     struct mc_info *mcip = mctelem_dataptr(mctc);

     x86_mcinfo_dump(mcip);
-    mcinfo_dumpped++;
+    mcinfo_dumped++;

     return 0;
 }
@@ -1702,7 +1702,7 @@ static void mc_panic_dump(void)
     for_each_online_cpu(cpu)
         mctelem_process_deferred(cpu, x86_mcinfo_dump_panic,
                                  mctelem_has_deferred_lmce(cpu));
-    dprintk(XENLOG_ERR, "End dump mc_info, %x mcinfo dumped\n", mcinfo_dumpped);
+    dprintk(XENLOG_ERR, "End dump mc_info, %x mcinfo dumped\n", mcinfo_dumped);
 }

 void mc_panic(const char *s)
diff --git a/xen/arch/x86/include/asm/compat.h b/xen/arch/x86/include/asm/compat.h
index 818cad87db67..6f84f1571666 100644
--- a/xen/arch/x86/include/asm/compat.h
+++ b/xen/arch/x86/include/asm/compat.h
@@ -13,6 +13,7 @@ typedef unsigned long full_ptr_t;

 struct domain;
 #ifdef CONFIG_PV32
+extern unsigned long cr4_pv32_mask;
 int switch_compat(struct domain *);
 #else
 #include <xen/errno.h>
diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index dfdd9e555149..6d22d3bd467a 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -13,6 +13,7 @@ extern char __2M_rwdata_start[], __2M_rwdata_end[];
 extern unsigned long xenheap_initial_phys_start;
 extern uint64_t boot_tsc_stamp;

+extern char cpu0_stack[STACK_SIZE];
 extern void *stack_start;

 void early_cpu_init(void);
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 6abfd8162120..604dba94b052 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -43,7 +43,7 @@ int __read_mostly opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_DEFAULT;
 static unsigned char __read_mostly irq_max_guests;
 integer_param("irq-max-guests", irq_max_guests);

-vmask_t global_used_vector_map;
+static vmask_t global_used_vector_map;

 struct irq_desc __read_mostly *irq_desc = NULL;

diff --git a/xen/common/symbols.c b/xen/common/symbols.c
index 691e61792506..7c3514c65f2e 100644
--- a/xen/common/symbols.c
+++ b/xen/common/symbols.c
@@ -21,23 +21,6 @@
 #include <xen/guest_access.h>
 #include <xen/errno.h>

-#ifdef SYMBOLS_ORIGIN
-extern const unsigned int symbols_offsets[];
-#define symbols_address(n) (SYMBOLS_ORIGIN + symbols_offsets[n])
-#else
-extern const unsigned long symbols_addresses[];
-#define symbols_address(n) symbols_addresses[n]
-#endif
-extern const unsigned int symbols_num_syms;
-extern const u8 symbols_names[];
-
-extern const struct symbol_offset symbols_sorted_offsets[];
-
-extern const u8 symbols_token_table[];
-extern const u16 symbols_token_index[];
-
-extern const unsigned int symbols_markers[];
-
 /* expand a compressed symbol data into the resulting uncompressed string,
    given the offset to where the symbol is in the compressed stream */
 static unsigned int symbols_expand_symbol(unsigned int off, char *result)
diff --git a/xen/include/xen/symbols.h b/xen/include/xen/symbols.h
index 20bbb28ef226..92540409265e 100644
--- a/xen/include/xen/symbols.h
+++ b/xen/include/xen/symbols.h
@@ -33,4 +33,22 @@ struct symbol_offset {
     uint32_t stream; /* .. in the compressed stream.*/
     uint32_t addr;   /* .. and in the fixed size address array. */
 };
+
+#ifdef SYMBOLS_ORIGIN
+extern const unsigned int symbols_offsets[];
+#define symbols_address(n) (SYMBOLS_ORIGIN + symbols_offsets[n])
+#else
+extern const unsigned long symbols_addresses[];
+#define symbols_address(n) symbols_addresses[n]
+#endif
+extern const unsigned int symbols_num_syms;
+extern const u8 symbols_names[];
+
+extern const struct symbol_offset symbols_sorted_offsets[];
+
+extern const u8 symbols_token_table[];
+extern const u16 symbols_token_index[];
+
+extern const unsigned int symbols_markers[];
+
 #endif /*_XEN_SYMBOLS_H*/
--
2.34.1

[XEN PATCH v2 2/7] x86: add deviations for variables only used in asm code

[Nicola Vetrini]

These variables are only used by asm code, and therefore
the lack of a declaration is justified by the corresponding
deviation comment.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/x86/include/asm/asm_defns.h | 1 +
 xen/arch/x86/setup.c                 | 1 +
 2 files changed, 2 insertions(+)

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..7e2979f419af 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -153,6 +153,7 @@ char __section(".init.bss.stack_aligned") __aligned(STACK_SIZE)
 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 };
--
2.34.1

[XEN PATCH v2 3/7] x86: add deviation comments for asm-only functions

[Nicola Vetrini]

As stated in rules.rst, functions used only in asm code
are allowed to have no prior declaration visible when being
defined, hence these functions are deviated.
This also fixes violations of MISRA C:2012 Rule 8.4.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/x86/hvm/svm/intr.c      | 1 +
 xen/arch/x86/hvm/svm/nestedsvm.c | 1 +
 xen/arch/x86/hvm/svm/svm.c       | 2 ++
 xen/arch/x86/traps.c             | 1 +
 xen/arch/x86/x86_64/traps.c      | 1 +
 5 files changed, 6 insertions(+)

diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
index 192e17ebbfbb..bd9dc560bbc6 100644
--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -123,6 +123,7 @@ static void svm_enable_intr_window(struct vcpu *v, struct hvm_intack intack)
         vmcb, general1_intercepts | GENERAL1_INTERCEPT_VINTR);
 }

+/* SAF-1-safe */
 void svm_intr_assist(void)
 {
     struct vcpu *v = current;
diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index a09b6abaaeaf..c80d59e0728e 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -1441,6 +1441,7 @@ nestedsvm_vcpu_vmexit(struct vcpu *v, struct cpu_user_regs *regs,
 }

 /* VCPU switch */
+/* SAF-1-safe */
 void nsvm_vcpu_switch(void)
 {
     struct cpu_user_regs *regs = guest_cpu_user_regs();
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index beb076ea8d62..b9fabd45a119 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1044,6 +1044,7 @@ static void noreturn cf_check svm_do_resume(void)
     reset_stack_and_jump(svm_asm_do_resume);
 }

+/* SAF-1-safe */
 void svm_vmenter_helper(void)
 {
     const struct cpu_user_regs *regs = guest_cpu_user_regs();
@@ -2574,6 +2575,7 @@ const struct hvm_function_table * __init start_svm(void)
     return &svm_function_table;
 }

+/* SAF-1-safe */
 void svm_vmexit_handler(void)
 {
     struct cpu_user_regs *regs = guest_cpu_user_regs();
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 0a005f088bca..f27ddb728b2c 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2260,6 +2260,7 @@ void asm_domain_crash_synchronous(unsigned long addr)
 }

 #ifdef CONFIG_DEBUG
+/* SAF-1-safe */
 void check_ist_exit(const struct cpu_user_regs *regs, bool ist_exit)
 {
     const unsigned int ist_mask =
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index e03e80813e36..5963d26d7848 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -266,6 +266,7 @@ void show_page_walk(unsigned long addr)
            l1_table_offset(addr), l1e_get_intpte(l1e), pfn);
 }

+/* SAF-1-safe */
 void do_double_fault(struct cpu_user_regs *regs)
 {
     unsigned int cpu;
--
2.34.1

[XEN PATCH][for-next v2 4/7] x86/grant: switch included header to make declarations visible

[Nicola Vetrini]

The declarations for {create,replace}_grant_p2m_mapping are
not visible when these functions are defined, therefore the right
header needs to be included to allow them to be visible.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/x86/hvm/grant_table.c             | 3 +--
 xen/arch/x86/include/asm/hvm/grant_table.h | 2 ++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/grant_table.c b/xen/arch/x86/hvm/grant_table.c
index 30d51d54a949..afe449d8882c 100644
--- a/xen/arch/x86/hvm/grant_table.c
+++ b/xen/arch/x86/hvm/grant_table.c
@@ -9,8 +9,7 @@
 
 #include <xen/types.h>
 
-#include <public/grant_table.h>
-
+#include <asm/hvm/grant_table.h>
 #include <asm/p2m.h>
 
 int create_grant_p2m_mapping(uint64_t addr, mfn_t frame,
diff --git a/xen/arch/x86/include/asm/hvm/grant_table.h b/xen/arch/x86/include/asm/hvm/grant_table.h
index 33c1da1a25f3..576aeb50adf4 100644
--- a/xen/arch/x86/include/asm/hvm/grant_table.h
+++ b/xen/arch/x86/include/asm/hvm/grant_table.h
@@ -10,6 +10,8 @@
 #ifndef __X86_HVM_GRANT_TABLE_H__
 #define __X86_HVM_GRANT_TABLE_H__
 
+#include <asm/paging.h>
+
 #ifdef CONFIG_HVM
 
 int create_grant_p2m_mapping(uint64_t addr, mfn_t frame,
-- 
2.34.1

[XEN PATCH][for-next v2 5/7] x86/vm_event: add missing include for hvm_vm_event_do_resume

[Nicola Vetrini]

The missing header makes the declaration visible when the function
is defined, thereby fixing a violation of MISRA C:2012 Rule 8.4.

Fixes: 1366a0e76db6 ("x86/vm_event: add hvm/vm_event.{h,c}")
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/hvm/vm_event.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/hvm/vm_event.c b/xen/arch/x86/hvm/vm_event.c
index 3b064bcfade5..c1af230e7aed 100644
--- a/xen/arch/x86/hvm/vm_event.c
+++ b/xen/arch/x86/hvm/vm_event.c
@@ -24,6 +24,7 @@
 #include <xen/vm_event.h>
 #include <asm/hvm/emulate.h>
 #include <asm/hvm/support.h>
+#include <asm/hvm/vm_event.h>
 #include <asm/vm_event.h>
 
 static void hvm_vm_event_set_registers(const struct vcpu *v)
-- 
2.34.1

[XEN PATCH][for-4.19 v2 6/7] xen/console: make function static inline

[Nicola Vetrini]

The definition of 'consoled_guest_tx' can be static inline,
thereby fixing a violation of MISRA C:2012 Rule 8.4.

Fixes: 5ef49f185c2d ("x86/pv-shim: shadow PV console's page for L2 DomU")
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/include/xen/consoled.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/xen/consoled.h b/xen/include/xen/consoled.h
index fd5d220a8aca..e943d8d48f7b 100644
--- a/xen/include/xen/consoled.h
+++ b/xen/include/xen/consoled.h
@@ -12,7 +12,7 @@ size_t consoled_guest_tx(char c);
 
 #else
 
-size_t consoled_guest_tx(char c) { return 0; }
+static inline size_t consoled_guest_tx(char c) { return 0; }
 
 #endif /* !CONFIG_PV_SHIM */
 #endif /* __XEN_CONSOLED_H__ */
-- 
2.34.1

[XEN PATCH][for-next v2 7/7] x86/mem_access: make function static

[Nicola Vetrini]

The function is used only within this file, and therefore can be static.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/mm/mem_access.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index c472fa1ee58b..78633d335d72 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -249,7 +249,7 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
     return (p2ma != p2m_access_n2rwx);
 }
 
-int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
+static int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
                               struct p2m_domain *ap2m, p2m_access_t a,
                               gfn_t gfn)
 {
-- 
2.34.1

Re: [XEN PATCH][for-next v2 5/7] x86/vm_event: add missing include for hvm_vm_event_do_resume

[Tamas K Lengyel]

On Mon, Oct 9, 2023 at 2:55 AM Nicola Vetrini
<nicola.vetrini@bugseng.com> wrote:
>
> The missing header makes the declaration visible when the function
> is defined, thereby fixing a violation of MISRA C:2012 Rule 8.4.
>
> Fixes: 1366a0e76db6 ("x86/vm_event: add hvm/vm_event.{h,c}")
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

Re: [XEN PATCH][for-next v2 7/7] x86/mem_access: make function static

[Tamas K Lengyel]

On Mon, Oct 9, 2023 at 2:55 AM Nicola Vetrini
<nicola.vetrini@bugseng.com> wrote:
>
> The function is used only within this file, and therefore can be static.
>
> No functional change.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

Re: [XEN PATCH v2 1/7] xen: add declarations for variables where needed

[Stefano Stabellini]

On Mon, 9 Oct 2023, Nicola Vetrini wrote:
> Some variables with external linkage used in C code do not have
> a visible declaration where they are defined. Providing such
> declaration also resolves violations of MISRA C:2012 Rule 8.4.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [XEN PATCH][for-next v2 7/7] x86/mem_access: make function static

[Jan Beulich]

On 09.10.2023 08:54, Nicola Vetrini wrote:
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -249,7 +249,7 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>      return (p2ma != p2m_access_n2rwx);
>  }
>  
> -int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
> +static int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>                                struct p2m_domain *ap2m, p2m_access_t a,
>                                gfn_t gfn)

As mentioned before, when adding static (or whatever else) like this,
the next line(s) need re-indenting.

Jan

Re: [XEN PATCH v2 1/7] xen: add declarations for variables where needed

[Jan Beulich]

On 09.10.2023 08:54, Nicola Vetrini wrote:
> Some variables with external linkage used in C code do not have
> a visible declaration where they are defined. Providing such
> declaration also resolves violations of MISRA C:2012 Rule 8.4.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

This is a mix of different approaches to the same underlying issue. I
think respectively splitting would have helped.

> --- a/xen/arch/x86/include/asm/compat.h
> +++ b/xen/arch/x86/include/asm/compat.h
> @@ -13,6 +13,7 @@ typedef unsigned long full_ptr_t;
> 
>  struct domain;
>  #ifdef CONFIG_PV32
> +extern unsigned long cr4_pv32_mask;

Why is this needed? Didn't we say declarations aren't needed when the
only consumer is assembly code? (I also wonder how this header is any
more "appropriate" - see the changelog entry - than about any other
one.)

> --- a/xen/arch/x86/include/asm/setup.h
> +++ b/xen/arch/x86/include/asm/setup.h
> @@ -13,6 +13,7 @@ extern char __2M_rwdata_start[], __2M_rwdata_end[];
>  extern unsigned long xenheap_initial_phys_start;
>  extern uint64_t boot_tsc_stamp;
> 
> +extern char cpu0_stack[STACK_SIZE];

Same question here.

> --- a/xen/common/symbols.c
> +++ b/xen/common/symbols.c
> @@ -21,23 +21,6 @@
>  #include <xen/guest_access.h>
>  #include <xen/errno.h>
> 
> -#ifdef SYMBOLS_ORIGIN
> -extern const unsigned int symbols_offsets[];
> -#define symbols_address(n) (SYMBOLS_ORIGIN + symbols_offsets[n])
> -#else
> -extern const unsigned long symbols_addresses[];
> -#define symbols_address(n) symbols_addresses[n]
> -#endif
> -extern const unsigned int symbols_num_syms;
> -extern const u8 symbols_names[];
> -
> -extern const struct symbol_offset symbols_sorted_offsets[];
> -
> -extern const u8 symbols_token_table[];
> -extern const u16 symbols_token_index[];
> -
> -extern const unsigned int symbols_markers[];
> -
>  /* expand a compressed symbol data into the resulting uncompressed string,
>     given the offset to where the symbol is in the compressed stream */
>  static unsigned int symbols_expand_symbol(unsigned int off, char *result)
> --- a/xen/include/xen/symbols.h
> +++ b/xen/include/xen/symbols.h
> @@ -33,4 +33,22 @@ struct symbol_offset {
>      uint32_t stream; /* .. in the compressed stream.*/
>      uint32_t addr;   /* .. and in the fixed size address array. */
>  };
> +
> +#ifdef SYMBOLS_ORIGIN
> +extern const unsigned int symbols_offsets[];
> +#define symbols_address(n) (SYMBOLS_ORIGIN + symbols_offsets[n])
> +#else
> +extern const unsigned long symbols_addresses[];
> +#define symbols_address(n) symbols_addresses[n]
> +#endif
> +extern const unsigned int symbols_num_syms;
> +extern const u8 symbols_names[];
> +
> +extern const struct symbol_offset symbols_sorted_offsets[];
> +
> +extern const u8 symbols_token_table[];
> +extern const u16 symbols_token_index[];
> +
> +extern const unsigned int symbols_markers[];
> +
>  #endif /*_XEN_SYMBOLS_H*/

This change is even less clear to me: The producer is assembly code,
and the consumer already had appropriate declarations. Why would we
want to increase the scope of their visibility?

Jan

Re: [XEN PATCH][for-4.19 v2 6/7] xen/console: make function static inline

[Jan Beulich]

On 09.10.2023 08:54, Nicola Vetrini wrote:
> --- a/xen/include/xen/consoled.h
> +++ b/xen/include/xen/consoled.h
> @@ -12,7 +12,7 @@ size_t consoled_guest_tx(char c);
>  
>  #else
>  
> -size_t consoled_guest_tx(char c) { return 0; }
> +static inline size_t consoled_guest_tx(char c) { return 0; }

Why inline? We do so in headers, but we generally avoid "inline" in
.c files.

Jan

Re: [XEN PATCH v2 2/7] x86: add deviations for variables only used in asm code

[Jan Beulich]

On 09.10.2023 08:54, Nicola Vetrini wrote:
> These variables are only used by asm code, and therefore
> the lack of a declaration is justified by the corresponding
> deviation comment.

Hmm, you say "declaration" here, but according to my understanding ...

> --- 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");

... this is a declaration, not a definition.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -153,6 +153,7 @@ char __section(".init.bss.stack_aligned") __aligned(STACK_SIZE)
>  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;

Imo such comments want folding; question is whether the tooling can
cope.

Jan

Re: [XEN PATCH v2 3/7] x86: add deviation comments for asm-only functions

[Jan Beulich]

On 09.10.2023 08:54, Nicola Vetrini wrote:
> As stated in rules.rst, functions used only in asm code
> are allowed to have no prior declaration visible when being
> defined, hence these functions are deviated.
> This also fixes violations of MISRA C:2012 Rule 8.4.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>  xen/arch/x86/hvm/svm/intr.c      | 1 +
>  xen/arch/x86/hvm/svm/nestedsvm.c | 1 +
>  xen/arch/x86/hvm/svm/svm.c       | 2 ++

Once again - why are you not also adjusting the respective VMX code?
Iirc it was agreed long ago that scans should be extended to cover as
much of the code base as possible.

Jan

Re: [XEN PATCH v2 1/7] xen: add declarations for variables where needed

[Nicola Vetrini]

On 16/10/2023 16:50, Jan Beulich wrote:
> On 09.10.2023 08:54, Nicola Vetrini wrote:
>> Some variables with external linkage used in C code do not have
>> a visible declaration where they are defined. Providing such
>> declaration also resolves violations of MISRA C:2012 Rule 8.4.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> This is a mix of different approaches to the same underlying issue. I
> think respectively splitting would have helped.
> 
>> --- a/xen/arch/x86/include/asm/compat.h
>> +++ b/xen/arch/x86/include/asm/compat.h
>> @@ -13,6 +13,7 @@ typedef unsigned long full_ptr_t;
>> 
>>  struct domain;
>>  #ifdef CONFIG_PV32
>> +extern unsigned long cr4_pv32_mask;
> 
> Why is this needed? Didn't we say declarations aren't needed when the
> only consumer is assembly code? (I also wonder how this header is any
> more "appropriate" - see the changelog entry - than about any other
> one.)
> 

It was pointed out to me [1] that compat.h might be more appropriate 
than setup.h
(probably because the asm code referencing it is under x86_64/compat).
Further, while it's true that this variable is used in asm, it's also 
used in x86/setup.c, hence
the need for a declaration.

>> --- a/xen/arch/x86/include/asm/setup.h
>> +++ b/xen/arch/x86/include/asm/setup.h
>> @@ -13,6 +13,7 @@ extern char __2M_rwdata_start[], __2M_rwdata_end[];
>>  extern unsigned long xenheap_initial_phys_start;
>>  extern uint64_t boot_tsc_stamp;
>> 
>> +extern char cpu0_stack[STACK_SIZE];
> 
> Same question here.
> 

This one is a bit more nuanced (I wouldn't oppose deviating this), but 
there is indeed one use.

>> --- a/xen/common/symbols.c
>> +++ b/xen/common/symbols.c
>> @@ -21,23 +21,6 @@
>>  #include <xen/guest_access.h>
>>  #include <xen/errno.h>
>> 
>> -#ifdef SYMBOLS_ORIGIN
>> -extern const unsigned int symbols_offsets[];
>> -#define symbols_address(n) (SYMBOLS_ORIGIN + symbols_offsets[n])
>> -#else
>> -extern const unsigned long symbols_addresses[];
>> -#define symbols_address(n) symbols_addresses[n]
>> -#endif
>> -extern const unsigned int symbols_num_syms;
>> -extern const u8 symbols_names[];
>> -
>> -extern const struct symbol_offset symbols_sorted_offsets[];
>> -
>> -extern const u8 symbols_token_table[];
>> -extern const u16 symbols_token_index[];
>> -
>> -extern const unsigned int symbols_markers[];
>> -
>>  /* expand a compressed symbol data into the resulting uncompressed 
>> string,
>>     given the offset to where the symbol is in the compressed stream 
>> */
>>  static unsigned int symbols_expand_symbol(unsigned int off, char 
>> *result)
>> --- a/xen/include/xen/symbols.h
>> +++ b/xen/include/xen/symbols.h
>> @@ -33,4 +33,22 @@ struct symbol_offset {
>>      uint32_t stream; /* .. in the compressed stream.*/
>>      uint32_t addr;   /* .. and in the fixed size address array. */
>>  };
>> +
>> +#ifdef SYMBOLS_ORIGIN
>> +extern const unsigned int symbols_offsets[];
>> +#define symbols_address(n) (SYMBOLS_ORIGIN + symbols_offsets[n])
>> +#else
>> +extern const unsigned long symbols_addresses[];
>> +#define symbols_address(n) symbols_addresses[n]
>> +#endif
>> +extern const unsigned int symbols_num_syms;
>> +extern const u8 symbols_names[];
>> +
>> +extern const struct symbol_offset symbols_sorted_offsets[];
>> +
>> +extern const u8 symbols_token_table[];
>> +extern const u16 symbols_token_index[];
>> +
>> +extern const unsigned int symbols_markers[];
>> +
>>  #endif /*_XEN_SYMBOLS_H*/
> 
> This change is even less clear to me: The producer is assembly code,
> and the consumer already had appropriate declarations. Why would we
> want to increase the scope of their visibility?
> 
> Jan

The missing decls are about common/symbols-dummy.c. Xen can choose that 
this file does
not need to conform (to this guideline or any guideline), in which case 
this change can be dropped.

[1] https://lore.kernel.org/xen-devel/ZRqkbeVUZbjizjNv@MacBookPdeRoger/

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH v2 3/7] x86: add deviation comments for asm-only functions

[Stefano Stabellini]

On Mon, 16 Oct 2023, Jan Beulich wrote:
> On 09.10.2023 08:54, Nicola Vetrini wrote:
> > As stated in rules.rst, functions used only in asm code
> > are allowed to have no prior declaration visible when being
> > defined, hence these functions are deviated.
> > This also fixes violations of MISRA C:2012 Rule 8.4.
> > 
> > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > ---
> >  xen/arch/x86/hvm/svm/intr.c      | 1 +
> >  xen/arch/x86/hvm/svm/nestedsvm.c | 1 +
> >  xen/arch/x86/hvm/svm/svm.c       | 2 ++
> 
> Once again - why are you not also adjusting the respective VMX code?
> Iirc it was agreed long ago that scans should be extended to cover as
> much of the code base as possible.


Let me summarize here our past discussions on the subject to make sure 
we are all aligned.

With my AMD hat on, of course we want to work with the upstream
community as much as possible and improve the overall codebase. But it
is not a goal for AMD to improve Intel-specific drivers (VMX and
others). Our safety configuration for Xen, including the public ECLAIR
instance currently sponsored by AMD, only includes SVM files, not VMX
files. MISRA compliance costs time and effort; this was done both
because of lack of interest in VMX and also as a cost saving measure.

Upon maintainer's request we can expand the scope of individual patches.
For example, AMD is not interested in ARM32 either, but in the past we
did address certain MISRA C issues on ARM32 too, if nothing else for
consistency of the code base. It comes down to a compromise what we
should do for consistency of the codebase versus addressing things that
makes sense for AMD business. For sure we could work on a few violations
affecting Intel drivers, but overall I don't think AMD could be asked to
make Intel drivers MISRA compliant.


In addition to the above, we also discussed during one of the past MISRA
C working group meetings to have larger-than-usual ECLAIR scans. ECLAIR
takes a couple of hours to run, so it is a good idea to restrict its
configuration in the usual runs. However, at least once a week maybe on
a Sunday, it would be good to run a comprehensive scan including
components that are not currently targeted for safety. This would help
us detect regressions and in general spot potential bugs.

As part of this larger-than-usual ECLAIR scan we could certainly
include Intel drivers as well as other things currently unsupported.


Now, concrete action items. Nicola, I think we should look into having a
larger-than-usual ECLAIR scan every week which includes all of Intel
files and in general as much as possible of the codebase.

Jan, for this specific patch, I don't think we have the scan including
Intel vmx files yet. Nicola please correct me if I am wrong. So Nicola
wouldn't be able to easily expand this patch to also cover Intel vmx
violations of this rule because we don't have the list of violations
affecting those files. 

Re: [XEN PATCH v2 1/7] xen: add declarations for variables where needed

[Jan Beulich]

On 16.10.2023 19:05, Nicola Vetrini wrote:
> On 16/10/2023 16:50, Jan Beulich wrote:
>> On 09.10.2023 08:54, Nicola Vetrini wrote:
>>> --- a/xen/arch/x86/include/asm/compat.h
>>> +++ b/xen/arch/x86/include/asm/compat.h
>>> @@ -13,6 +13,7 @@ typedef unsigned long full_ptr_t;
>>>
>>>  struct domain;
>>>  #ifdef CONFIG_PV32
>>> +extern unsigned long cr4_pv32_mask;
>>
>> Why is this needed? Didn't we say declarations aren't needed when the
>> only consumer is assembly code? (I also wonder how this header is any
>> more "appropriate" - see the changelog entry - than about any other
>> one.)
>>
> 
> It was pointed out to me [1] that compat.h might be more appropriate 
> than setup.h
> (probably because the asm code referencing it is under x86_64/compat).

Hmm. I agree setup.h isn't appropriate.

> Further, while it's true that this variable is used in asm, it's also 
> used in x86/setup.c, hence
> the need for a declaration.

But that's the file where the variable is defined. IOW no risk of
definition and (non-existing) declaration going out of sync.

>>> --- a/xen/arch/x86/include/asm/setup.h
>>> +++ b/xen/arch/x86/include/asm/setup.h
>>> @@ -13,6 +13,7 @@ extern char __2M_rwdata_start[], __2M_rwdata_end[];
>>>  extern unsigned long xenheap_initial_phys_start;
>>>  extern uint64_t boot_tsc_stamp;
>>>
>>> +extern char cpu0_stack[STACK_SIZE];
>>
>> Same question here.
>>
> 
> This one is a bit more nuanced (I wouldn't oppose deviating this), but 
> there is indeed one use.

Still same here then.

>>> --- a/xen/common/symbols.c
>>> +++ b/xen/common/symbols.c
>>> @@ -21,23 +21,6 @@
>>>  #include <xen/guest_access.h>
>>>  #include <xen/errno.h>
>>>
>>> -#ifdef SYMBOLS_ORIGIN
>>> -extern const unsigned int symbols_offsets[];
>>> -#define symbols_address(n) (SYMBOLS_ORIGIN + symbols_offsets[n])
>>> -#else
>>> -extern const unsigned long symbols_addresses[];
>>> -#define symbols_address(n) symbols_addresses[n]
>>> -#endif
>>> -extern const unsigned int symbols_num_syms;
>>> -extern const u8 symbols_names[];
>>> -
>>> -extern const struct symbol_offset symbols_sorted_offsets[];
>>> -
>>> -extern const u8 symbols_token_table[];
>>> -extern const u16 symbols_token_index[];
>>> -
>>> -extern const unsigned int symbols_markers[];
>>> -
>>>  /* expand a compressed symbol data into the resulting uncompressed 
>>> string,
>>>     given the offset to where the symbol is in the compressed stream 
>>> */
>>>  static unsigned int symbols_expand_symbol(unsigned int off, char 
>>> *result)
>>> --- a/xen/include/xen/symbols.h
>>> +++ b/xen/include/xen/symbols.h
>>> @@ -33,4 +33,22 @@ struct symbol_offset {
>>>      uint32_t stream; /* .. in the compressed stream.*/
>>>      uint32_t addr;   /* .. and in the fixed size address array. */
>>>  };
>>> +
>>> +#ifdef SYMBOLS_ORIGIN
>>> +extern const unsigned int symbols_offsets[];
>>> +#define symbols_address(n) (SYMBOLS_ORIGIN + symbols_offsets[n])
>>> +#else
>>> +extern const unsigned long symbols_addresses[];
>>> +#define symbols_address(n) symbols_addresses[n]
>>> +#endif
>>> +extern const unsigned int symbols_num_syms;
>>> +extern const u8 symbols_names[];
>>> +
>>> +extern const struct symbol_offset symbols_sorted_offsets[];
>>> +
>>> +extern const u8 symbols_token_table[];
>>> +extern const u16 symbols_token_index[];
>>> +
>>> +extern const unsigned int symbols_markers[];
>>> +
>>>  #endif /*_XEN_SYMBOLS_H*/
>>
>> This change is even less clear to me: The producer is assembly code,
>> and the consumer already had appropriate declarations. Why would we
>> want to increase the scope of their visibility?
> 
> The missing decls are about common/symbols-dummy.c. Xen can choose that 
> this file does
> not need to conform (to this guideline or any guideline), in which case 
> this change can be dropped.

Since symbols-dummy.c isn't used in the final binary, I'd prefer that.
Otherwise imo a new private header used by just the two files would want
introducing, to keep exposure limited.

Jan

Re: [XEN PATCH v2 3/7] x86: add deviation comments for asm-only functions

[Jan Beulich]

On 17.10.2023 00:34, Stefano Stabellini wrote:
> On Mon, 16 Oct 2023, Jan Beulich wrote:
>> On 09.10.2023 08:54, Nicola Vetrini wrote:
>>> As stated in rules.rst, functions used only in asm code
>>> are allowed to have no prior declaration visible when being
>>> defined, hence these functions are deviated.
>>> This also fixes violations of MISRA C:2012 Rule 8.4.
>>>
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>> ---
>>>  xen/arch/x86/hvm/svm/intr.c      | 1 +
>>>  xen/arch/x86/hvm/svm/nestedsvm.c | 1 +
>>>  xen/arch/x86/hvm/svm/svm.c       | 2 ++
>>
>> Once again - why are you not also adjusting the respective VMX code?
>> Iirc it was agreed long ago that scans should be extended to cover as
>> much of the code base as possible.
> 
> 
> Let me summarize here our past discussions on the subject to make sure 
> we are all aligned.
> 
> With my AMD hat on, of course we want to work with the upstream
> community as much as possible and improve the overall codebase. But it
> is not a goal for AMD to improve Intel-specific drivers (VMX and
> others). Our safety configuration for Xen, including the public ECLAIR
> instance currently sponsored by AMD, only includes SVM files, not VMX
> files. MISRA compliance costs time and effort; this was done both
> because of lack of interest in VMX and also as a cost saving measure.
> 
> Upon maintainer's request we can expand the scope of individual patches.
> For example, AMD is not interested in ARM32 either, but in the past we
> did address certain MISRA C issues on ARM32 too, if nothing else for
> consistency of the code base. It comes down to a compromise what we
> should do for consistency of the codebase versus addressing things that
> makes sense for AMD business. For sure we could work on a few violations
> affecting Intel drivers, but overall I don't think AMD could be asked to
> make Intel drivers MISRA compliant.
> 
> 
> In addition to the above, we also discussed during one of the past MISRA
> C working group meetings to have larger-than-usual ECLAIR scans. ECLAIR
> takes a couple of hours to run, so it is a good idea to restrict its
> configuration in the usual runs. However, at least once a week maybe on
> a Sunday, it would be good to run a comprehensive scan including
> components that are not currently targeted for safety. This would help
> us detect regressions and in general spot potential bugs.
> 
> As part of this larger-than-usual ECLAIR scan we could certainly
> include Intel drivers as well as other things currently unsupported.
> 
> 
> Now, concrete action items. Nicola, I think we should look into having a
> larger-than-usual ECLAIR scan every week which includes all of Intel
> files and in general as much as possible of the codebase.
> 
> Jan, for this specific patch, I don't think we have the scan including
> Intel vmx files yet. Nicola please correct me if I am wrong. So Nicola
> wouldn't be able to easily expand this patch to also cover Intel vmx
> violations of this rule because we don't have the list of violations
> affecting those files. 

I'm afraid I disagree: There are likely direct VMX counterparts to the SVM
items adjusted. No scan is needed to spot those. Anything VMX-only can be
left separate, sure.

Jan

Re: [XEN PATCH][for-next v2 7/7] x86/mem_access: make function static

[Nicola Vetrini]

On 16/10/2023 16:29, Jan Beulich wrote:
> On 09.10.2023 08:54, Nicola Vetrini wrote:
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -249,7 +249,7 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned 
>> long gla,
>>      return (p2ma != p2m_access_n2rwx);
>>  }
>> 
>> -int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain 
>> *hp2m,
>> +static int p2m_set_altp2m_mem_access(struct domain *d, struct 
>> p2m_domain *hp2m,
>>                                struct p2m_domain *ap2m, p2m_access_t 
>> a,
>>                                gfn_t gfn)
> 
> As mentioned before, when adding static (or whatever else) like this,
> the next line(s) need re-indenting.
> 
> Jan

Ok.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH][for-4.19 v2 6/7] xen/console: make function static inline

[Nicola Vetrini]

On 16/10/2023 16:52, Jan Beulich wrote:
> On 09.10.2023 08:54, Nicola Vetrini wrote:
>> --- a/xen/include/xen/consoled.h
>> +++ b/xen/include/xen/consoled.h
>> @@ -12,7 +12,7 @@ size_t consoled_guest_tx(char c);
>> 
>>  #else
>> 
>> -size_t consoled_guest_tx(char c) { return 0; }
>> +static inline size_t consoled_guest_tx(char c) { return 0; }
> 
> Why inline? We do so in headers, but we generally avoid "inline" in
> .c files.
> 
> Jan

Yes. The file modified is in fact an header.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH v2 3/7] x86: add deviation comments for asm-only functions

[Nicola Vetrini]

> Now, concrete action items. Nicola, I think we should look into having 
> a
> larger-than-usual ECLAIR scan every week which includes all of Intel
> files and in general as much as possible of the codebase.
> 

This can be arranged. I'll keep you posted about this.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH][for-4.19 v2 6/7] xen/console: make function static inline

[Jan Beulich]

On 17.10.2023 17:24, Nicola Vetrini wrote:
> On 16/10/2023 16:52, Jan Beulich wrote:
>> On 09.10.2023 08:54, Nicola Vetrini wrote:
>>> --- a/xen/include/xen/consoled.h
>>> +++ b/xen/include/xen/consoled.h
>>> @@ -12,7 +12,7 @@ size_t consoled_guest_tx(char c);
>>>
>>>  #else
>>>
>>> -size_t consoled_guest_tx(char c) { return 0; }
>>> +static inline size_t consoled_guest_tx(char c) { return 0; }
>>
>> Why inline? We do so in headers, but we generally avoid "inline" in
>> .c files.
> 
> Yes. The file modified is in fact an header.

Hmm, how did I not pay attention? Yet then a different question arises:
Without the "static inline" I'd expect this to result in a build error
from any two .c files including this header. Yet we aren't aware of
such a build issue, so I wonder whether the stub is needed in the first
place.

Jan

Re: [XEN PATCH][for-4.19 v2 6/7] xen/console: make function static inline

[Nicola Vetrini]

On 17/10/2023 18:26, Jan Beulich wrote:
> On 17.10.2023 17:24, Nicola Vetrini wrote:
>> On 16/10/2023 16:52, Jan Beulich wrote:
>>> On 09.10.2023 08:54, Nicola Vetrini wrote:
>>>> --- a/xen/include/xen/consoled.h
>>>> +++ b/xen/include/xen/consoled.h
>>>> @@ -12,7 +12,7 @@ size_t consoled_guest_tx(char c);
>>>> 
>>>>  #else
>>>> 
>>>> -size_t consoled_guest_tx(char c) { return 0; }
>>>> +static inline size_t consoled_guest_tx(char c) { return 0; }
>>> 
>>> Why inline? We do so in headers, but we generally avoid "inline" in
>>> .c files.
>> 
>> Yes. The file modified is in fact an header.
> 
> Hmm, how did I not pay attention? Yet then a different question arises:
> Without the "static inline" I'd expect this to result in a build error
> from any two .c files including this header. Yet we aren't aware of
> such a build issue, so I wonder whether the stub is needed in the first
> place.
> 
> Jan

This is a good observation. Right now I see only one caller, that is 
conditioned on
both CONFIG_X86 and pv_shim and pv_console, making this stub unused as 
far as I can tell.
It might indeed be a good idea to drop it.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH v2 1/7] xen: add declarations for variables where needed

[Nicola Vetrini]

On 17/10/2023 08:46, Jan Beulich wrote:
> On 16.10.2023 19:05, Nicola Vetrini wrote:
>> On 16/10/2023 16:50, Jan Beulich wrote:
>>> On 09.10.2023 08:54, Nicola Vetrini wrote:
>>>> --- a/xen/arch/x86/include/asm/compat.h
>>>> +++ b/xen/arch/x86/include/asm/compat.h
>>>> @@ -13,6 +13,7 @@ typedef unsigned long full_ptr_t;
>>>> 
>>>>  struct domain;
>>>>  #ifdef CONFIG_PV32
>>>> +extern unsigned long cr4_pv32_mask;
>>> 
>>> Why is this needed? Didn't we say declarations aren't needed when the
>>> only consumer is assembly code? (I also wonder how this header is any
>>> more "appropriate" - see the changelog entry - than about any other
>>> one.)
>>> 
>> 
>> It was pointed out to me [1] that compat.h might be more appropriate
>> than setup.h
>> (probably because the asm code referencing it is under x86_64/compat).
> 
> Hmm. I agree setup.h isn't appropriate.
> 
>> Further, while it's true that this variable is used in asm, it's also
>> used in x86/setup.c, hence
>> the need for a declaration.
> 
> But that's the file where the variable is defined. IOW no risk of
> definition and (non-existing) declaration going out of sync.
> 

This is an aspect specific to this variable, that unfortunately the rule 
does not
capture. I'll deviate this in the next version of this series.

>>>> --- a/xen/arch/x86/include/asm/setup.h
>>>> +++ b/xen/arch/x86/include/asm/setup.h
>>>> @@ -13,6 +13,7 @@ extern char __2M_rwdata_start[], 
>>>> __2M_rwdata_end[];
>>>>  extern unsigned long xenheap_initial_phys_start;
>>>>  extern uint64_t boot_tsc_stamp;
>>>> 
>>>> +extern char cpu0_stack[STACK_SIZE];
>>> 
>>> Same question here.
>>> 
>> 
>> This one is a bit more nuanced (I wouldn't oppose deviating this), but
>> there is indeed one use.
> 
> Still same here then.
> 

Same as above; it can be argued that there's no risk of anything going 
out of sync.

>>>> --- a/xen/common/symbols.c
>>>> +++ b/xen/common/symbols.c
>>>> @@ -21,23 +21,6 @@
>>>>  #include <xen/guest_access.h>
>>>>  #include <xen/errno.h>
>>>> 
>>>> -#ifdef SYMBOLS_ORIGIN
>>>> -extern const unsigned int symbols_offsets[];
>>>> -#define symbols_address(n) (SYMBOLS_ORIGIN + symbols_offsets[n])
>>>> -#else
>>>> -extern const unsigned long symbols_addresses[];
>>>> -#define symbols_address(n) symbols_addresses[n]
>>>> -#endif
>>>> -extern const unsigned int symbols_num_syms;
>>>> -extern const u8 symbols_names[];
>>>> -
>>>> -extern const struct symbol_offset symbols_sorted_offsets[];
>>>> -
>>>> -extern const u8 symbols_token_table[];
>>>> -extern const u16 symbols_token_index[];
>>>> -
>>>> -extern const unsigned int symbols_markers[];
>>>> -
>>>>  /* expand a compressed symbol data into the resulting uncompressed
>>>> string,
>>>>     given the offset to where the symbol is in the compressed stream
>>>> */
>>>>  static unsigned int symbols_expand_symbol(unsigned int off, char
>>>> *result)
>>>> --- a/xen/include/xen/symbols.h
>>>> +++ b/xen/include/xen/symbols.h
>>>> @@ -33,4 +33,22 @@ struct symbol_offset {
>>>>      uint32_t stream; /* .. in the compressed stream.*/
>>>>      uint32_t addr;   /* .. and in the fixed size address array. */
>>>>  };
>>>> +
>>>> +#ifdef SYMBOLS_ORIGIN
>>>> +extern const unsigned int symbols_offsets[];
>>>> +#define symbols_address(n) (SYMBOLS_ORIGIN + symbols_offsets[n])
>>>> +#else
>>>> +extern const unsigned long symbols_addresses[];
>>>> +#define symbols_address(n) symbols_addresses[n]
>>>> +#endif
>>>> +extern const unsigned int symbols_num_syms;
>>>> +extern const u8 symbols_names[];
>>>> +
>>>> +extern const struct symbol_offset symbols_sorted_offsets[];
>>>> +
>>>> +extern const u8 symbols_token_table[];
>>>> +extern const u16 symbols_token_index[];
>>>> +
>>>> +extern const unsigned int symbols_markers[];
>>>> +
>>>>  #endif /*_XEN_SYMBOLS_H*/
>>> 
>>> This change is even less clear to me: The producer is assembly code,
>>> and the consumer already had appropriate declarations. Why would we
>>> want to increase the scope of their visibility?
>> 
>> The missing decls are about common/symbols-dummy.c. Xen can choose 
>> that
>> this file does
>> not need to conform (to this guideline or any guideline), in which 
>> case
>> this change can be dropped.
> 
> Since symbols-dummy.c isn't used in the final binary, I'd prefer that.
> Otherwise imo a new private header used by just the two files would 
> want
> introducing, to keep exposure limited.
> 
> Jan

Ok

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH v2 2/7] x86: add deviations for variables only used in asm code

[Nicola Vetrini]

On 16/10/2023 16:58, Jan Beulich wrote:
> On 09.10.2023 08:54, Nicola Vetrini wrote:
>> These variables are only used by asm code, and therefore
>> the lack of a declaration is justified by the corresponding
>> deviation comment.
> 
> Hmm, you say "declaration" here, but according to my understanding ...
> 
>> --- 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");
> 
> ... this is a declaration, not a definition.
> 

It has automatic storage duration and it's not defined afterwards

>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -153,6 +153,7 @@ char __section(".init.bss.stack_aligned") 
>> __aligned(STACK_SIZE)
>>  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;
> 
> Imo such comments want folding; question is whether the tooling can
> cope.
> 

As far as I know, it can't fold /* comment SAF-x-safe */, but /* 
SAF-x-safe comment */,
though the latter should be a justification, which this comment is not

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH v2 2/7] x86: add deviations for variables only used in asm code

[Jan Beulich]

On 18.10.2023 16:28, Nicola Vetrini wrote:
> On 16/10/2023 16:58, Jan Beulich wrote:
>> On 09.10.2023 08:54, Nicola Vetrini wrote:
>>> These variables are only used by asm code, and therefore
>>> the lack of a declaration is justified by the corresponding
>>> deviation comment.
>>
>> Hmm, you say "declaration" here, but according to my understanding ...
>>
>>> --- 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");
>>
>> ... this is a declaration, not a definition.
>>
> 
> It has automatic storage duration and it's not defined afterwards

Mind me asking what makes you derive "automatic storage duration"?
I also don't see how "not defined afterwards" matters here. This is
a special construct, not covered by the C standard.

Jan

Re: [XEN PATCH v2 2/7] x86: add deviations for variables only used in asm code

[Nicola Vetrini]

On 18/10/2023 16:56, Jan Beulich wrote:
> On 18.10.2023 16:28, Nicola Vetrini wrote:
>> On 16/10/2023 16:58, Jan Beulich wrote:
>>> On 09.10.2023 08:54, Nicola Vetrini wrote:
>>>> These variables are only used by asm code, and therefore
>>>> the lack of a declaration is justified by the corresponding
>>>> deviation comment.
>>> 
>>> Hmm, you say "declaration" here, but according to my understanding 
>>> ...
>>> 
>>>> --- 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");
>>> 
>>> ... this is a declaration, not a definition.
>>> 
>> 
>> It has automatic storage duration and it's not defined afterwards
> 
> Mind me asking what makes you derive "automatic storage duration"?
> I also don't see how "not defined afterwards" matters here. This is
> a special construct, not covered by the C standard.
> 
> Jan

Oh, you're right. I was fooled by the fact that this is not a standard 
construct.
I see your point now.

Thanks,

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH v2 3/7] x86: add deviation comments for asm-only functions

[Stefano Stabellini]

On Tue, 17 Oct 2023, Jan Beulich wrote:
> > Jan, for this specific patch, I don't think we have the scan including
> > Intel vmx files yet. Nicola please correct me if I am wrong. So Nicola
> > wouldn't be able to easily expand this patch to also cover Intel vmx
> > violations of this rule because we don't have the list of violations
> > affecting those files. 
> 
> I'm afraid I disagree: There are likely direct VMX counterparts to the SVM
> items adjusted. No scan is needed to spot those. Anything VMX-only can be
> left separate, sure.

Nicola is new to the codebase, so let us help.

This patch adds deviations for these SVM functions:
- svm_intr_assist
- nsvm_vcpu_switch
- svm_vmenter_helper
- svm_vmexit_handler

I take these are the VMX equivalents:
- vmx_intr_assist
- nvmx_switch_guest
- vmx_vmenter_helper
- vmx_asm_vmexit_handler

Jan, did I miss anything?

Re: [XEN PATCH v2 3/7] x86: add deviation comments for asm-only functions

[Jan Beulich]

On 19.10.2023 02:07, Stefano Stabellini wrote:
> On Tue, 17 Oct 2023, Jan Beulich wrote:
>>> Jan, for this specific patch, I don't think we have the scan including
>>> Intel vmx files yet. Nicola please correct me if I am wrong. So Nicola
>>> wouldn't be able to easily expand this patch to also cover Intel vmx
>>> violations of this rule because we don't have the list of violations
>>> affecting those files. 
>>
>> I'm afraid I disagree: There are likely direct VMX counterparts to the SVM
>> items adjusted. No scan is needed to spot those. Anything VMX-only can be
>> left separate, sure.
> 
> Nicola is new to the codebase, so let us help.
> 
> This patch adds deviations for these SVM functions:
> - svm_intr_assist
> - nsvm_vcpu_switch
> - svm_vmenter_helper
> - svm_vmexit_handler
> 
> I take these are the VMX equivalents:
> - vmx_intr_assist
> - nvmx_switch_guest
> - vmx_vmenter_helper
> - vmx_asm_vmexit_handler
> 
> Jan, did I miss anything?

At the first glance - no, I don't think you do.

Jan

Re: [XEN PATCH v2 3/7] x86: add deviation comments for asm-only functions

[Nicola Vetrini]

>> - vmx_asm_vmexit_handler
>> 

Isn't this just a declaration:

void nocall vmx_asm_vmexit_handler(void);

while the function to be deviated is this:

void vmx_vmexit_handler(struct cpu_user_regs *regs)

Am I correct?

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH v2 3/7] x86: add deviation comments for asm-only functions

[Jan Beulich]

On 19.10.2023 10:04, Nicola Vetrini wrote:
> 
>>> - vmx_asm_vmexit_handler
>>>
> 
> Isn't this just a declaration:
> 
> void nocall vmx_asm_vmexit_handler(void);
> 
> while the function to be deviated is this:
> 
> void vmx_vmexit_handler(struct cpu_user_regs *regs)
> 
> Am I correct?

Looks like you are, yes.

Jan