[XEN PATCH v3 0/3] address some violations of MISRA C Rule 8.4

[XEN PATCH v3 0/3] address some violations of MISRA C Rule 8.4

[Nicola Vetrini]

MISRA C:2012 Rule 8.4 states:
"A compatible declaration shall be visible when an object or function with
external linkage is defined".

Changes from v1:
- Patch 1/7 has been committed;
- Patch 2/7 has been dropped, as the fix is already part of [1];
- Patch 3/7 drops the addition of asmlinkage on variables. Discussion on how to
  handle the existing uses can be part of a separate patch;
- Patch 4/7 is unchanged;
- Patch 5/7 has been committed;
- Patch 6/7 has been dropped
- Patch 7/7 has been revised to have a declaration for first_valid_mfn, rather
  than a deviation for the absence of a declaration
Changes from v2:
- Patch 3/3: remove redundant declarations

[1] https://lore.kernel.org/xen-devel/27dd8f40-1ea6-1e7e-49c2-31936a17e9d7@suse.com/


Nicola Vetrini (3):
  xen/x86: add missing instances of asmlinkage attributes
  x86/viridian: make build_assertions static
  xen/mm: add declaration for first_valid_mfn

 xen/arch/arm/include/asm/numa.h   | 8 ++++----
 xen/arch/ppc/include/asm/numa.h   | 7 +++----
 xen/arch/x86/efi/efi-boot.h       | 5 +++--
 xen/arch/x86/hvm/viridian/synic.c | 2 +-
 xen/arch/x86/smpboot.c            | 2 +-
 xen/include/xen/mm.h              | 2 ++
 6 files changed, 14 insertions(+), 12 deletions(-)

-- 
2.34.1

[XEN PATCH v3 1/3] xen/x86: add missing instances of asmlinkage attributes

[Nicola Vetrini]

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/efi/efi-boot.h | 5 +++--
 xen/arch/x86/smpboot.c      | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 86467da301e5..8ea64e31cdc2 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -808,8 +808,9 @@ static const char *__init get_option(const char *cmd, const char *opt)
     return o;
 }
 
-void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable,
-                           const char *cmdline)
+void asmlinkage __init efi_multiboot2(EFI_HANDLE ImageHandle,
+                                      EFI_SYSTEM_TABLE *SystemTable,
+                                      const char *cmdline)
 {
     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
     EFI_HANDLE gop_handle;
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 4c54ecbc91d7..8aa621533f3d 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -310,7 +310,7 @@ static void set_cpu_sibling_map(unsigned int cpu)
     }
 }
 
-void start_secondary(void *unused)
+void asmlinkage start_secondary(void *unused)
 {
     struct cpu_info *info = get_cpu_info();
 
-- 
2.34.1

Re: [XEN PATCH v3 1/3] xen/x86: add missing instances of asmlinkage attributes

[Stefano Stabellini]

On Mon, 11 Dec 2023, Nicola Vetrini wrote:
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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

Re: [XEN PATCH v3 1/3] xen/x86: add missing instances of asmlinkage attributes

[Jan Beulich]

On 12.12.2023 02:48, Stefano Stabellini wrote:
> On Mon, 11 Dec 2023, Nicola Vetrini wrote:
>> No functional change.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Acked-by: Jan Beulich <jbeulich@suse.com>

[XEN PATCH v3 2/3] x86/viridian: make build_assertions static

[Nicola Vetrini]

This is consistent with other instances of the same function
and also resolves a violation of MISRA C:2012 Rule 8.4.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/x86/hvm/viridian/synic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/viridian/synic.c b/xen/arch/x86/hvm/viridian/synic.c
index 8cf600cec68f..3375e55e95ca 100644
--- a/xen/arch/x86/hvm/viridian/synic.c
+++ b/xen/arch/x86/hvm/viridian/synic.c
@@ -18,7 +18,7 @@
 #include "private.h"
 
 
-void __init __maybe_unused build_assertions(void)
+static void __init __maybe_unused build_assertions(void)
 {
     BUILD_BUG_ON(sizeof(struct hv_message) != HV_MESSAGE_SIZE);
 }
-- 
2.34.1

Re: [XEN PATCH v3 2/3] x86/viridian: make build_assertions static

[Durrant, Paul]

On 11/12/2023 09:14, Nicola Vetrini wrote:
> This is consistent with other instances of the same function
> and also resolves a violation of MISRA C:2012 Rule 8.4.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>   xen/arch/x86/hvm/viridian/synic.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Acked-by: Paul Durrant <paul@xen.org>

[XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

[Nicola Vetrini]

Such declaration is needed because a compatible declaration
is not visible in xen/common/page_alloc.c, where the variable
is defined. That variable can't yet be static because of the lack of
support from ARM and PPC for NUMA.

On the occasion, use drop a use of u8.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Having this declaration essentially sidesteps the current impossibility
of having a static variable, as described in the comments in
ARM and PCC's asm/numa.h.

Changes in v3:
- Drop redundant declarations of first_valid_mfn in asm/numa.h for Arm and PPC.
---
 xen/arch/arm/include/asm/numa.h | 8 ++++----
 xen/arch/ppc/include/asm/numa.h | 7 +++----
 xen/include/xen/mm.h            | 2 ++
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/include/asm/numa.h b/xen/arch/arm/include/asm/numa.h
index e2bee2bd8223..44b689f67db8 100644
--- a/xen/arch/arm/include/asm/numa.h
+++ b/xen/arch/arm/include/asm/numa.h
@@ -2,8 +2,9 @@
 #define __ARCH_ARM_NUMA_H
 
 #include <xen/mm.h>
+#include <xen/types.h>
 
-typedef u8 nodeid_t;
+typedef uint8_t nodeid_t;
 
 #ifndef CONFIG_NUMA
 
@@ -12,10 +13,9 @@ typedef u8 nodeid_t;
 #define node_to_cpumask(node)   (cpu_online_map)
 
 /*
- * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
- * is required because the dummy helpers are using it.
+ * TODO: define here first_valid_mfn as static when NUMA is supported on Arm,
+ * this is required because the dummy helpers are using it.
  */
-extern mfn_t first_valid_mfn;
 
 /* XXX: implement NUMA support */
 #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
diff --git a/xen/arch/ppc/include/asm/numa.h b/xen/arch/ppc/include/asm/numa.h
index 7fdf66c3da74..152305ebe446 100644
--- a/xen/arch/ppc/include/asm/numa.h
+++ b/xen/arch/ppc/include/asm/numa.h
@@ -1,8 +1,8 @@
 #ifndef __ASM_PPC_NUMA_H__
 #define __ASM_PPC_NUMA_H__
 
-#include <xen/types.h>
 #include <xen/mm.h>
+#include <xen/types.h>
 
 typedef uint8_t nodeid_t;
 
@@ -11,10 +11,9 @@ typedef uint8_t nodeid_t;
 #define node_to_cpumask(node)   (cpu_online_map)
 
 /*
- * TODO: make first_valid_mfn static when NUMA is supported on PPC, this
- * is required because the dummy helpers are using it.
+ * TODO: define here first_valid_mfn as static when NUMA is supported on PPC,
+ * this is required because the dummy helpers are using it.
  */
-extern mfn_t first_valid_mfn;
 
 /* XXX: implement NUMA support */
 #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 3d9b2d05a5c8..a13a9a46ced7 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -118,6 +118,8 @@ int destroy_xen_mappings(unsigned long s, unsigned long e);
 /* Retrieve the MFN mapped by VA in Xen virtual address space. */
 mfn_t xen_map_to_mfn(unsigned long va);
 
+extern mfn_t first_valid_mfn;
+
 /*
  * Create only non-leaf page table entries for the
  * page range in Xen virtual address space.
-- 
2.34.1

Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

[Stefano Stabellini]

On Mon, 11 Dec 2023, Nicola Vetrini wrote:
> Such declaration is needed because a compatible declaration
> is not visible in xen/common/page_alloc.c, where the variable
> is defined. That variable can't yet be static because of the lack of
> support from ARM and PPC for NUMA.
> 
> On the occasion, use drop a use of u8.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> Having this declaration essentially sidesteps the current impossibility
> of having a static variable, as described in the comments in
> ARM and PCC's asm/numa.h.
> 
> Changes in v3:
> - Drop redundant declarations of first_valid_mfn in asm/numa.h for Arm and PPC.
> ---
>  xen/arch/arm/include/asm/numa.h | 8 ++++----
>  xen/arch/ppc/include/asm/numa.h | 7 +++----
>  xen/include/xen/mm.h            | 2 ++
>  3 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/numa.h b/xen/arch/arm/include/asm/numa.h
> index e2bee2bd8223..44b689f67db8 100644
> --- a/xen/arch/arm/include/asm/numa.h
> +++ b/xen/arch/arm/include/asm/numa.h
> @@ -2,8 +2,9 @@
>  #define __ARCH_ARM_NUMA_H
>  
>  #include <xen/mm.h>
> +#include <xen/types.h>

Why the addition of types.h? It doesn't seem to be necessary. Also on
PPC below doesn't seem to be necessary.

Everything else looks fine.



> -typedef u8 nodeid_t;
> +typedef uint8_t nodeid_t;
>  
>  #ifndef CONFIG_NUMA
>  
> @@ -12,10 +13,9 @@ typedef u8 nodeid_t;
>  #define node_to_cpumask(node)   (cpu_online_map)
>  
>  /*
> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
> - * is required because the dummy helpers are using it.
> + * TODO: define here first_valid_mfn as static when NUMA is supported on Arm,
> + * this is required because the dummy helpers are using it.
>   */
> -extern mfn_t first_valid_mfn;
>  
>  /* XXX: implement NUMA support */
>  #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
> diff --git a/xen/arch/ppc/include/asm/numa.h b/xen/arch/ppc/include/asm/numa.h
> index 7fdf66c3da74..152305ebe446 100644
> --- a/xen/arch/ppc/include/asm/numa.h
> +++ b/xen/arch/ppc/include/asm/numa.h
> @@ -1,8 +1,8 @@
>  #ifndef __ASM_PPC_NUMA_H__
>  #define __ASM_PPC_NUMA_H__
>  
> -#include <xen/types.h>
>  #include <xen/mm.h>
> +#include <xen/types.h>
>  
>  typedef uint8_t nodeid_t;
>  
> @@ -11,10 +11,9 @@ typedef uint8_t nodeid_t;
>  #define node_to_cpumask(node)   (cpu_online_map)
>  
>  /*
> - * TODO: make first_valid_mfn static when NUMA is supported on PPC, this
> - * is required because the dummy helpers are using it.
> + * TODO: define here first_valid_mfn as static when NUMA is supported on PPC,
> + * this is required because the dummy helpers are using it.
>   */
> -extern mfn_t first_valid_mfn;
>  
>  /* XXX: implement NUMA support */
>  #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 3d9b2d05a5c8..a13a9a46ced7 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -118,6 +118,8 @@ int destroy_xen_mappings(unsigned long s, unsigned long e);
>  /* Retrieve the MFN mapped by VA in Xen virtual address space. */
>  mfn_t xen_map_to_mfn(unsigned long va);
>  
> +extern mfn_t first_valid_mfn;
> +
>  /*
>   * Create only non-leaf page table entries for the
>   * page range in Xen virtual address space.
> -- 
> 2.34.1
> 

Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

[Jan Beulich]

On 11.12.2023 10:14, Nicola Vetrini wrote:
> --- a/xen/arch/arm/include/asm/numa.h
> +++ b/xen/arch/arm/include/asm/numa.h
> @@ -2,8 +2,9 @@
>  #define __ARCH_ARM_NUMA_H
>  
>  #include <xen/mm.h>

With this, ...

> +#include <xen/types.h>
>  
> -typedef u8 nodeid_t;
> +typedef uint8_t nodeid_t;
>  
>  #ifndef CONFIG_NUMA
>  
> @@ -12,10 +13,9 @@ typedef u8 nodeid_t;
>  #define node_to_cpumask(node)   (cpu_online_map)
>  
>  /*
> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
> - * is required because the dummy helpers are using it.
> + * TODO: define here first_valid_mfn as static when NUMA is supported on Arm,
> + * this is required because the dummy helpers are using it.
>   */
> -extern mfn_t first_valid_mfn;

... and this declaration moved to xen/mm.h, how is it going to be possible
to do as the adjusted comment says? The compiler will choke on the static
after having seen the extern.

I also firmly disagree with the entirely unrelated u8 -> uint8_t change
above. Such tidying can be done when somewhat related to what a patch is
doing anyway, but here there's (imo) not even a vague connection.

Jan

Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

[Stefano Stabellini]

On Wed, 13 Dec 2023, Jan Beulich wrote:
> On 11.12.2023 10:14, Nicola Vetrini wrote:
> > --- a/xen/arch/arm/include/asm/numa.h
> > +++ b/xen/arch/arm/include/asm/numa.h
> > @@ -2,8 +2,9 @@
> >  #define __ARCH_ARM_NUMA_H
> >  
> >  #include <xen/mm.h>
> 
> With this, ...
> 
> > +#include <xen/types.h>
> >  
> > -typedef u8 nodeid_t;
> > +typedef uint8_t nodeid_t;
> >  
> >  #ifndef CONFIG_NUMA
> >  
> > @@ -12,10 +13,9 @@ typedef u8 nodeid_t;
> >  #define node_to_cpumask(node)   (cpu_online_map)
> >  
> >  /*
> > - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
> > - * is required because the dummy helpers are using it.
> > + * TODO: define here first_valid_mfn as static when NUMA is supported on Arm,
> > + * this is required because the dummy helpers are using it.
> >   */
> > -extern mfn_t first_valid_mfn;
> 
> ... and this declaration moved to xen/mm.h, how is it going to be possible
> to do as the adjusted comment says? The compiler will choke on the static
> after having seen the extern.

Hi Jan,

Nicola was just following a review comment by Julien. NUMA has been
pending for a while and I wouldn't hold this patch back because of it.
I suggest we go with Julien's request (this version of the patch).

If you feel strongly about it, please suggest an alternative.


> I also firmly disagree with the entirely unrelated u8 -> uint8_t change
> above. Such tidying can be done when somewhat related to what a patch is
> doing anyway, but here there's (imo) not even a vague connection.

Can be removed on commit

Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

[Jan Beulich]

On 14.12.2023 03:05, Stefano Stabellini wrote:
> On Wed, 13 Dec 2023, Jan Beulich wrote:
>> On 11.12.2023 10:14, Nicola Vetrini wrote:
>>> --- a/xen/arch/arm/include/asm/numa.h
>>> +++ b/xen/arch/arm/include/asm/numa.h
>>> @@ -2,8 +2,9 @@
>>>  #define __ARCH_ARM_NUMA_H
>>>  
>>>  #include <xen/mm.h>
>>
>> With this, ...
>>
>>> +#include <xen/types.h>
>>>  
>>> -typedef u8 nodeid_t;
>>> +typedef uint8_t nodeid_t;
>>>  
>>>  #ifndef CONFIG_NUMA
>>>  
>>> @@ -12,10 +13,9 @@ typedef u8 nodeid_t;
>>>  #define node_to_cpumask(node)   (cpu_online_map)
>>>  
>>>  /*
>>> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
>>> - * is required because the dummy helpers are using it.
>>> + * TODO: define here first_valid_mfn as static when NUMA is supported on Arm,
>>> + * this is required because the dummy helpers are using it.
>>>   */
>>> -extern mfn_t first_valid_mfn;
>>
>> ... and this declaration moved to xen/mm.h, how is it going to be possible
>> to do as the adjusted comment says? The compiler will choke on the static
>> after having seen the extern.
> 
> Nicola was just following a review comment by Julien. NUMA has been
> pending for a while and I wouldn't hold this patch back because of it.
> I suggest we go with Julien's request (this version of the patch).
> 
> If you feel strongly about it, please suggest an alternative.

Leaving a TODO comment which cannot actually be carried out is just wrong
imo. And I consider in unfair to ask me to suggest an alternative. The
(imo obvious) alternative is to drop this patch, if no proper change can
be proposed. There's nothing wrong with leaving a violation in place,
when that violation is far from causing any kind of harm. The more that
the place is already accompanied by a (suitable afaict) comment.

Jan

Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

[Julien Grall]

Hi Jan,

On 14/12/2023 07:53, Jan Beulich wrote:
> On 14.12.2023 03:05, Stefano Stabellini wrote:
>> On Wed, 13 Dec 2023, Jan Beulich wrote:
>>> On 11.12.2023 10:14, Nicola Vetrini wrote:
>>>> --- a/xen/arch/arm/include/asm/numa.h
>>>> +++ b/xen/arch/arm/include/asm/numa.h
>>>> @@ -2,8 +2,9 @@
>>>>   #define __ARCH_ARM_NUMA_H
>>>>   
>>>>   #include <xen/mm.h>
>>>
>>> With this, ...
>>>
>>>> +#include <xen/types.h>
>>>>   
>>>> -typedef u8 nodeid_t;
>>>> +typedef uint8_t nodeid_t;
>>>>   
>>>>   #ifndef CONFIG_NUMA
>>>>   
>>>> @@ -12,10 +13,9 @@ typedef u8 nodeid_t;
>>>>   #define node_to_cpumask(node)   (cpu_online_map)
>>>>   
>>>>   /*
>>>> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
>>>> - * is required because the dummy helpers are using it.
>>>> + * TODO: define here first_valid_mfn as static when NUMA is supported on Arm,
>>>> + * this is required because the dummy helpers are using it.
>>>>    */
>>>> -extern mfn_t first_valid_mfn;
>>>
>>> ... and this declaration moved to xen/mm.h, how is it going to be possible
>>> to do as the adjusted comment says? The compiler will choke on the static
>>> after having seen the extern.
>>
>> Nicola was just following a review comment by Julien. NUMA has been
>> pending for a while and I wouldn't hold this patch back because of it.
>> I suggest we go with Julien's request (this version of the patch).
>>
>> If you feel strongly about it, please suggest an alternative.
> 
> Leaving a TODO comment which cannot actually be carried out is just wrong
> imo. And I consider in unfair to ask me to suggest an alternative. The
> (imo obvious) alternative is to drop this patch, if no proper change can
> be proposed. There's nothing wrong with leaving a violation in place,
> when that violation is far from causing any kind of harm. The more that
> the place is already accompanied by a (suitable afaict) comment.

Just to clarify, are you saying you would be happy if the proposal if 
the TODO is removed?

Cheers,

-- 
Julien Grall

Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

[Jan Beulich]

On 14.12.2023 09:32, Julien Grall wrote:
> Hi Jan,
> 
> On 14/12/2023 07:53, Jan Beulich wrote:
>> On 14.12.2023 03:05, Stefano Stabellini wrote:
>>> On Wed, 13 Dec 2023, Jan Beulich wrote:
>>>> On 11.12.2023 10:14, Nicola Vetrini wrote:
>>>>> --- a/xen/arch/arm/include/asm/numa.h
>>>>> +++ b/xen/arch/arm/include/asm/numa.h
>>>>> @@ -2,8 +2,9 @@
>>>>>   #define __ARCH_ARM_NUMA_H
>>>>>   
>>>>>   #include <xen/mm.h>
>>>>
>>>> With this, ...
>>>>
>>>>> +#include <xen/types.h>
>>>>>   
>>>>> -typedef u8 nodeid_t;
>>>>> +typedef uint8_t nodeid_t;
>>>>>   
>>>>>   #ifndef CONFIG_NUMA
>>>>>   
>>>>> @@ -12,10 +13,9 @@ typedef u8 nodeid_t;
>>>>>   #define node_to_cpumask(node)   (cpu_online_map)
>>>>>   
>>>>>   /*
>>>>> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
>>>>> - * is required because the dummy helpers are using it.
>>>>> + * TODO: define here first_valid_mfn as static when NUMA is supported on Arm,
>>>>> + * this is required because the dummy helpers are using it.
>>>>>    */
>>>>> -extern mfn_t first_valid_mfn;
>>>>
>>>> ... and this declaration moved to xen/mm.h, how is it going to be possible
>>>> to do as the adjusted comment says? The compiler will choke on the static
>>>> after having seen the extern.
>>>
>>> Nicola was just following a review comment by Julien. NUMA has been
>>> pending for a while and I wouldn't hold this patch back because of it.
>>> I suggest we go with Julien's request (this version of the patch).
>>>
>>> If you feel strongly about it, please suggest an alternative.
>>
>> Leaving a TODO comment which cannot actually be carried out is just wrong
>> imo. And I consider in unfair to ask me to suggest an alternative. The
>> (imo obvious) alternative is to drop this patch, if no proper change can
>> be proposed. There's nothing wrong with leaving a violation in place,
>> when that violation is far from causing any kind of harm. The more that
>> the place is already accompanied by a (suitable afaict) comment.
> 
> Just to clarify, are you saying you would be happy if the proposal if 
> the TODO is removed?

Removing the bad (new) TODO here is an option. But the previous TODO shouldn't
go away. However, you asking now required me to actually look into Stefano's
request to make an alternative proposal (I still don't see though why it
needed to be me to think about an appropriate solution; probably in the time
I've spent writing replies on this thread, I could have made the change myself).

First, Arm's and PPC's header have this in their !NUMA sections. Much like
Oleksii's putting in quite a bit of effort to reduce redundancy in order to
not further grow it with RISC-V, what's wrong with sorting the !NUMA case
properly as a first step? Move the entire !NUMA section either into xen/numa.h
(eliminating the need for arch-es not supporting NUMA to even have such a
header), or (if need be) to asm-generic/. Then, as a 2nd step (albeit that
could also be done on its own, just the result would be less neat imo), make
the variable in question here extern _only_ when !NUMA. This would then
address the original TODO, so that could then legitimately be dropped. This
would further be a good opportunity to adjust the already stale comment in
page_alloc.c (it's no longer applicable to Arm only).

Jan

Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

[George Dunlap]

On Thu, Dec 14, 2023 at 8:51 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 14.12.2023 09:32, Julien Grall wrote:
> > Hi Jan,
> >
> > On 14/12/2023 07:53, Jan Beulich wrote:
> >> On 14.12.2023 03:05, Stefano Stabellini wrote:
> >>> On Wed, 13 Dec 2023, Jan Beulich wrote:
> >>>> On 11.12.2023 10:14, Nicola Vetrini wrote:
> >>>>> --- a/xen/arch/arm/include/asm/numa.h
> >>>>> +++ b/xen/arch/arm/include/asm/numa.h
> >>>>> @@ -2,8 +2,9 @@
> >>>>>   #define __ARCH_ARM_NUMA_H
> >>>>>
> >>>>>   #include <xen/mm.h>
> >>>>
> >>>> With this, ...
> >>>>
> >>>>> +#include <xen/types.h>
> >>>>>
> >>>>> -typedef u8 nodeid_t;
> >>>>> +typedef uint8_t nodeid_t;
> >>>>>
> >>>>>   #ifndef CONFIG_NUMA
> >>>>>
> >>>>> @@ -12,10 +13,9 @@ typedef u8 nodeid_t;
> >>>>>   #define node_to_cpumask(node)   (cpu_online_map)
> >>>>>
> >>>>>   /*
> >>>>> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
> >>>>> - * is required because the dummy helpers are using it.
> >>>>> + * TODO: define here first_valid_mfn as static when NUMA is supported on Arm,
> >>>>> + * this is required because the dummy helpers are using it.
> >>>>>    */
> >>>>> -extern mfn_t first_valid_mfn;
> >>>>
> >>>> ... and this declaration moved to xen/mm.h, how is it going to be possible
> >>>> to do as the adjusted comment says? The compiler will choke on the static
> >>>> after having seen the extern.
> >>>
> >>> Nicola was just following a review comment by Julien. NUMA has been
> >>> pending for a while and I wouldn't hold this patch back because of it.
> >>> I suggest we go with Julien's request (this version of the patch).
> >>>
> >>> If you feel strongly about it, please suggest an alternative.
> >>
> >> Leaving a TODO comment which cannot actually be carried out is just wrong
> >> imo. And I consider in unfair to ask me to suggest an alternative. The
> >> (imo obvious) alternative is to drop this patch, if no proper change can
> >> be proposed. There's nothing wrong with leaving a violation in place,
> >> when that violation is far from causing any kind of harm. The more that
> >> the place is already accompanied by a (suitable afaict) comment.
> >
> > Just to clarify, are you saying you would be happy if the proposal if
> > the TODO is removed?
>
> Removing the bad (new) TODO here is an option. But the previous TODO shouldn't
> go away. However, you asking now required me to actually look into Stefano's
> request to make an alternative proposal (I still don't see though why it
> needed to be me to think about an appropriate solution;

In general, it is unreasonable to expect other people to come up with
suggestions to make you happy.  You're ultimately the only person who
knows what would make you satisfied.  You're also more senior and know
the code better, and thus in a better position to be able to come up
with ideas.  "What about X?" "No." "What about Y?" "No." "What about
Z?" "No." Contributors experience it as caustic behavior.

The only time it's acceptable is if you can specify, precisely and
reasonably completely, your criteria for acceptance, such that there's
a clear way forward.  In this case, for instance, it sounds like "Has
a TODO which isn't technically inaccurate" might be the criteria.

In which case, for instance, a solution might be along the lines of:
"TODO: When NUMA is supported on Arm we'll need to do something about
defining first_valid_mfn, which is used by the dummy helpers."  This
punts the actual solution down the road until we need it.

But I do think that it's fair to ask Julien to think about a suitable
wording, since the comment is in a sense to remind him (or other ARM
maintainers) what's needed, and since the eventual solution will be
something to do with the ARM code and architecture anyway.

 -George

>
> First, Arm's and PPC's header have this in their !NUMA sections. Much like
> Oleksii's putting in quite a bit of effort to reduce redundancy in order to
> not further grow it with RISC-V, what's wrong with sorting the !NUMA case
> properly as a first step? Move the entire !NUMA section either into xen/numa.h
> (eliminating the need for arch-es not supporting NUMA to even have such a
> header), or (if need be) to asm-generic/. Then, as a 2nd step (albeit that
> could also be done on its own, just the result would be less neat imo), make
> the variable in question here extern _only_ when !NUMA. This would then
> address the original TODO, so that could then legitimately be dropped. This
> would further be a good opportunity to adjust the already stale comment in
> page_alloc.c (it's no longer applicable to Arm only).
>
> Jan

Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

[Julien Grall]

Hi  George,

On 14/12/2023 14:15, George Dunlap wrote:
> But I do think that it's fair to ask Julien to think about a suitable
> wording, since the comment is in a sense to remind him (or other ARM
> maintainers) what's needed, and since the eventual solution will be
> something to do with the ARM code and architecture anyway.

The comment is for anyone using !NUMA (i.e. all architectures but x86) 
:). What about the following (this is Nicola's patch with the comments 
reworked):

diff --git a/xen/arch/arm/include/asm/numa.h 
b/xen/arch/arm/include/asm/numa.h
index e2bee2bd8223..4bf7c304ea3c 100644
--- a/xen/arch/arm/include/asm/numa.h
+++ b/xen/arch/arm/include/asm/numa.h
@@ -2,8 +2,9 @@
  #define __ARCH_ARM_NUMA_H

  #include <xen/mm.h>
+#include <xen/types.h>

-typedef u8 nodeid_t;
+typedef uint8_t nodeid_t;

  #ifndef CONFIG_NUMA

@@ -11,12 +12,6 @@ typedef u8 nodeid_t;
  #define cpu_to_node(cpu) 0
  #define node_to_cpumask(node)   (cpu_online_map)

-/*
- * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
- * is required because the dummy helpers are using it.
- */
-extern mfn_t first_valid_mfn;
-
  /* XXX: implement NUMA support */
  #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
  #define node_start_pfn(nid) (mfn_x(first_valid_mfn))
diff --git a/xen/arch/ppc/include/asm/numa.h 
b/xen/arch/ppc/include/asm/numa.h
index 7fdf66c3da74..888de2dbd1eb 100644
--- a/xen/arch/ppc/include/asm/numa.h
+++ b/xen/arch/ppc/include/asm/numa.h
@@ -1,8 +1,8 @@
  #ifndef __ASM_PPC_NUMA_H__
  #define __ASM_PPC_NUMA_H__

-#include <xen/types.h>
  #include <xen/mm.h>
+#include <xen/types.h>

  typedef uint8_t nodeid_t;

@@ -10,12 +10,6 @@ typedef uint8_t nodeid_t;
  #define cpu_to_node(cpu) 0
  #define node_to_cpumask(node)   (cpu_online_map)

-/*
- * TODO: make first_valid_mfn static when NUMA is supported on PPC, this
- * is required because the dummy helpers are using it.
- */
-extern mfn_t first_valid_mfn;
-
  /* XXX: implement NUMA support */
  #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
  #define node_start_pfn(nid) (mfn_x(first_valid_mfn))
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 9b5df74fddab..d874525916ea 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -255,8 +255,10 @@ static PAGE_LIST_HEAD(page_broken_list);
   */

  /*
- * first_valid_mfn is exported because it is use in ARM specific NUMA
- * helpers. See comment in arch/arm/include/asm/numa.h.
+ * first_valid_mfn is exported because it is used when !CONFIG_NUMA.
+ *
+ * TODO: Consider if we can conditionally export first_valid_mfn based
+ * on whether NUMA is selected.
   */
  mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER;

diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 3d9b2d05a5c8..a13a9a46ced7 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -118,6 +118,8 @@ int destroy_xen_mappings(unsigned long s, unsigned 
long e);
  /* Retrieve the MFN mapped by VA in Xen virtual address space. */
  mfn_t xen_map_to_mfn(unsigned long va);

+extern mfn_t first_valid_mfn;
+
  /*
   * Create only non-leaf page table entries for the
   * page range in Xen virtual address space.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

[Stefano Stabellini]

On Thu, 14 Dec 2023, Julien Grall wrote:
> Hi  George,
> 
> On 14/12/2023 14:15, George Dunlap wrote:
> > But I do think that it's fair to ask Julien to think about a suitable
> > wording, since the comment is in a sense to remind him (or other ARM
> > maintainers) what's needed, and since the eventual solution will be
> > something to do with the ARM code and architecture anyway.
> 
> The comment is for anyone using !NUMA (i.e. all architectures but x86) :).
> What about the following (this is Nicola's patch with the comments reworked):

The below looks good to me.



> diff --git a/xen/arch/arm/include/asm/numa.h b/xen/arch/arm/include/asm/numa.h
> index e2bee2bd8223..4bf7c304ea3c 100644
> --- a/xen/arch/arm/include/asm/numa.h
> +++ b/xen/arch/arm/include/asm/numa.h
> @@ -2,8 +2,9 @@
>  #define __ARCH_ARM_NUMA_H
> 
>  #include <xen/mm.h>
> +#include <xen/types.h>
> 
> -typedef u8 nodeid_t;
> +typedef uint8_t nodeid_t;
> 
>  #ifndef CONFIG_NUMA
> 
> @@ -11,12 +12,6 @@ typedef u8 nodeid_t;
>  #define cpu_to_node(cpu) 0
>  #define node_to_cpumask(node)   (cpu_online_map)
> 
> -/*
> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
> - * is required because the dummy helpers are using it.
> - */
> -extern mfn_t first_valid_mfn;
> -
>  /* XXX: implement NUMA support */
>  #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
>  #define node_start_pfn(nid) (mfn_x(first_valid_mfn))
> diff --git a/xen/arch/ppc/include/asm/numa.h b/xen/arch/ppc/include/asm/numa.h
> index 7fdf66c3da74..888de2dbd1eb 100644
> --- a/xen/arch/ppc/include/asm/numa.h
> +++ b/xen/arch/ppc/include/asm/numa.h
> @@ -1,8 +1,8 @@
>  #ifndef __ASM_PPC_NUMA_H__
>  #define __ASM_PPC_NUMA_H__
> 
> -#include <xen/types.h>
>  #include <xen/mm.h>
> +#include <xen/types.h>
> 
>  typedef uint8_t nodeid_t;
> 
> @@ -10,12 +10,6 @@ typedef uint8_t nodeid_t;
>  #define cpu_to_node(cpu) 0
>  #define node_to_cpumask(node)   (cpu_online_map)
> 
> -/*
> - * TODO: make first_valid_mfn static when NUMA is supported on PPC, this
> - * is required because the dummy helpers are using it.
> - */
> -extern mfn_t first_valid_mfn;
> -
>  /* XXX: implement NUMA support */
>  #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
>  #define node_start_pfn(nid) (mfn_x(first_valid_mfn))
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 9b5df74fddab..d874525916ea 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -255,8 +255,10 @@ static PAGE_LIST_HEAD(page_broken_list);
>   */
> 
>  /*
> - * first_valid_mfn is exported because it is use in ARM specific NUMA
> - * helpers. See comment in arch/arm/include/asm/numa.h.
> + * first_valid_mfn is exported because it is used when !CONFIG_NUMA.
> + *
> + * TODO: Consider if we can conditionally export first_valid_mfn based
> + * on whether NUMA is selected.
>   */
>  mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER;
> 
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 3d9b2d05a5c8..a13a9a46ced7 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -118,6 +118,8 @@ int destroy_xen_mappings(unsigned long s, unsigned long
> e);
>  /* Retrieve the MFN mapped by VA in Xen virtual address space. */
>  mfn_t xen_map_to_mfn(unsigned long va);
> 
> +extern mfn_t first_valid_mfn;
> +
>  /*
>   * Create only non-leaf page table entries for the
>   * page range in Xen virtual address space.
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

[Jan Beulich]

On 14.12.2023 20:10, Julien Grall wrote:
> On 14/12/2023 14:15, George Dunlap wrote:
>> But I do think that it's fair to ask Julien to think about a suitable
>> wording, since the comment is in a sense to remind him (or other ARM
>> maintainers) what's needed, and since the eventual solution will be
>> something to do with the ARM code and architecture anyway.
> 
> The comment is for anyone using !NUMA (i.e. all architectures but x86) 
> :). What about the following (this is Nicola's patch with the comments 
> reworked):

This clearly is better, yet then ...

> --- a/xen/arch/arm/include/asm/numa.h
> +++ b/xen/arch/arm/include/asm/numa.h
> @@ -2,8 +2,9 @@
>   #define __ARCH_ARM_NUMA_H
> 
>   #include <xen/mm.h>
> +#include <xen/types.h>
> 
> -typedef u8 nodeid_t;
> +typedef uint8_t nodeid_t;
> 
>   #ifndef CONFIG_NUMA
> 
> @@ -11,12 +12,6 @@ typedef u8 nodeid_t;
>   #define cpu_to_node(cpu) 0
>   #define node_to_cpumask(node)   (cpu_online_map)
> 
> -/*
> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
> - * is required because the dummy helpers are using it.
> - */
> -extern mfn_t first_valid_mfn;
> -
>   /* XXX: implement NUMA support */
>   #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
>   #define node_start_pfn(nid) (mfn_x(first_valid_mfn))
> diff --git a/xen/arch/ppc/include/asm/numa.h 
> b/xen/arch/ppc/include/asm/numa.h
> index 7fdf66c3da74..888de2dbd1eb 100644
> --- a/xen/arch/ppc/include/asm/numa.h
> +++ b/xen/arch/ppc/include/asm/numa.h
> @@ -1,8 +1,8 @@
>   #ifndef __ASM_PPC_NUMA_H__
>   #define __ASM_PPC_NUMA_H__
> 
> -#include <xen/types.h>
>   #include <xen/mm.h>
> +#include <xen/types.h>
> 
>   typedef uint8_t nodeid_t;
> 
> @@ -10,12 +10,6 @@ typedef uint8_t nodeid_t;
>   #define cpu_to_node(cpu) 0
>   #define node_to_cpumask(node)   (cpu_online_map)
> 
> -/*
> - * TODO: make first_valid_mfn static when NUMA is supported on PPC, this
> - * is required because the dummy helpers are using it.
> - */
> -extern mfn_t first_valid_mfn;
> -
>   /* XXX: implement NUMA support */
>   #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
>   #define node_start_pfn(nid) (mfn_x(first_valid_mfn))
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 9b5df74fddab..d874525916ea 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -255,8 +255,10 @@ static PAGE_LIST_HEAD(page_broken_list);
>    */
> 
>   /*
> - * first_valid_mfn is exported because it is use in ARM specific NUMA
> - * helpers. See comment in arch/arm/include/asm/numa.h.
> + * first_valid_mfn is exported because it is used when !CONFIG_NUMA.
> + *
> + * TODO: Consider if we can conditionally export first_valid_mfn based
> + * on whether NUMA is selected.
>    */
>   mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER;
> 
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 3d9b2d05a5c8..a13a9a46ced7 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -118,6 +118,8 @@ int destroy_xen_mappings(unsigned long s, unsigned 
> long e);
>   /* Retrieve the MFN mapped by VA in Xen virtual address space. */
>   mfn_t xen_map_to_mfn(unsigned long va);
> 
> +extern mfn_t first_valid_mfn;
> +
>   /*
>    * Create only non-leaf page table entries for the
>    * page range in Xen virtual address space.

... I still disagree with the placement here (should be xen/numa.h imo),
and I still don't see why we can't carry out the TODO right away, if we
have to touch all of this anyway. If it's really too much to ask from
the original contributor, I can certainly see about making a patch myself
(and I've now added this to my short-term TODO list).

Jan

Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

[Julien Grall]

Hi Jan,

On 15/12/2023 08:03, Jan Beulich wrote:
> On 14.12.2023 20:10, Julien Grall wrote:
>> On 14/12/2023 14:15, George Dunlap wrote:
>>> But I do think that it's fair to ask Julien to think about a suitable
>>> wording, since the comment is in a sense to remind him (or other ARM
>>> maintainers) what's needed, and since the eventual solution will be
>>> something to do with the ARM code and architecture anyway.
>>
>> The comment is for anyone using !NUMA (i.e. all architectures but x86)
>> :). What about the following (this is Nicola's patch with the comments
>> reworked):
> 
> This clearly is better, yet then ...
> 
>> --- a/xen/arch/arm/include/asm/numa.h
>> +++ b/xen/arch/arm/include/asm/numa.h
>> @@ -2,8 +2,9 @@
>>    #define __ARCH_ARM_NUMA_H
>>
>>    #include <xen/mm.h>
>> +#include <xen/types.h>
>>
>> -typedef u8 nodeid_t;
>> +typedef uint8_t nodeid_t;
>>
>>    #ifndef CONFIG_NUMA
>>
>> @@ -11,12 +12,6 @@ typedef u8 nodeid_t;
>>    #define cpu_to_node(cpu) 0
>>    #define node_to_cpumask(node)   (cpu_online_map)
>>
>> -/*
>> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
>> - * is required because the dummy helpers are using it.
>> - */
>> -extern mfn_t first_valid_mfn;
>> -
>>    /* XXX: implement NUMA support */
>>    #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
>>    #define node_start_pfn(nid) (mfn_x(first_valid_mfn))
>> diff --git a/xen/arch/ppc/include/asm/numa.h
>> b/xen/arch/ppc/include/asm/numa.h
>> index 7fdf66c3da74..888de2dbd1eb 100644
>> --- a/xen/arch/ppc/include/asm/numa.h
>> +++ b/xen/arch/ppc/include/asm/numa.h
>> @@ -1,8 +1,8 @@
>>    #ifndef __ASM_PPC_NUMA_H__
>>    #define __ASM_PPC_NUMA_H__
>>
>> -#include <xen/types.h>
>>    #include <xen/mm.h>
>> +#include <xen/types.h>
>>
>>    typedef uint8_t nodeid_t;
>>
>> @@ -10,12 +10,6 @@ typedef uint8_t nodeid_t;
>>    #define cpu_to_node(cpu) 0
>>    #define node_to_cpumask(node)   (cpu_online_map)
>>
>> -/*
>> - * TODO: make first_valid_mfn static when NUMA is supported on PPC, this
>> - * is required because the dummy helpers are using it.
>> - */
>> -extern mfn_t first_valid_mfn;
>> -
>>    /* XXX: implement NUMA support */
>>    #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
>>    #define node_start_pfn(nid) (mfn_x(first_valid_mfn))
>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>> index 9b5df74fddab..d874525916ea 100644
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -255,8 +255,10 @@ static PAGE_LIST_HEAD(page_broken_list);
>>     */
>>
>>    /*
>> - * first_valid_mfn is exported because it is use in ARM specific NUMA
>> - * helpers. See comment in arch/arm/include/asm/numa.h.
>> + * first_valid_mfn is exported because it is used when !CONFIG_NUMA.
>> + *
>> + * TODO: Consider if we can conditionally export first_valid_mfn based
>> + * on whether NUMA is selected.
>>     */
>>    mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER;
>>
>> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
>> index 3d9b2d05a5c8..a13a9a46ced7 100644
>> --- a/xen/include/xen/mm.h
>> +++ b/xen/include/xen/mm.h
>> @@ -118,6 +118,8 @@ int destroy_xen_mappings(unsigned long s, unsigned
>> long e);
>>    /* Retrieve the MFN mapped by VA in Xen virtual address space. */
>>    mfn_t xen_map_to_mfn(unsigned long va);
>>
>> +extern mfn_t first_valid_mfn;
>> +
>>    /*
>>     * Create only non-leaf page table entries for the
>>     * page range in Xen virtual address space.
> 
> ... I still disagree with the placement here (should be xen/numa.h imo),

I don't care too much about which header is used. So I am ok with numa.h 
if this is what you really want. However...

> and I still don't see why we can't carry out the TODO right away, if we
> have to touch all of this anyway. If it's really too much to ask from
> the original contributor, I can certainly see about making a patch myself

There are a few reasons I didn't update with that approach or want to 
ask Nicola for this:
   1. I assume this will be a macro and it is not clear to me whether 
Eclair would be able to detect the visibility change based on the config
   2. IMHO, this is obfuscating a bit more the code
   3. I don't see why we want to special case first_valid_mfn. I am sure 
we have other external variables in the same situation.

So if any solution needs to came up, then it needs to be generic, and I 
fear this is going to be yet another lengthy work which is not worth it 
for this case.

> (and I've now added this to my short-term TODO list).

Thanks. But as I wrote above, I am not sure I agree such proposal. Let 
see what the code looks like.

Cheers,


-- 
Julien Grall

Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

[Nicola Vetrini]

Hi Jan, Julien

>>> 
>>> +extern mfn_t first_valid_mfn;
>>> +
>>>    /*
>>>     * Create only non-leaf page table entries for the
>>>     * page range in Xen virtual address space.
>> 
>> ... I still disagree with the placement here (should be xen/numa.h 
>> imo),
> 
> I don't care too much about which header is used. So I am ok with 
> numa.h if this is what you really want. However...
> 
>> and I still don't see why we can't carry out the TODO right away, if 
>> we
>> have to touch all of this anyway. If it's really too much to ask from
>> the original contributor, I can certainly see about making a patch 
>> myself
> 
> There are a few reasons I didn't update with that approach or want to 
> ask Nicola for this:
>   1. I assume this will be a macro and it is not clear to me whether 
> Eclair would be able to detect the visibility change based on the 
> config
>   2. IMHO, this is obfuscating a bit more the code
>   3. I don't see why we want to special case first_valid_mfn. I am sure 
> we have other external variables in the same situation.
> 
> So if any solution needs to came up, then it needs to be generic, and I 
> fear this is going to be yet another lengthy work which is not worth it 
> for this case.
> 
>> (and I've now added this to my short-term TODO list).
> 
> Thanks. But as I wrote above, I am not sure I agree such proposal. Let 
> see what the code looks like.
> 

Given the controversy that arose on this patch, maybe it's best to drop 
it now from this series. If then it's agreed that as a temporary 
workaround you want to leave it as is and deviate, as I proposed 
elsewhere in the thread, feel free to reach out. Meanwhile, if patch 1 
looks ok I can mark this series as done and focus on the v2 for R2.1, 
which is almost ready.

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

Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

[Julien Grall]

Hi,

On 15/12/2023 09:59, Nicola Vetrini wrote:
>>>> +extern mfn_t first_valid_mfn;
>>>> +
>>>>    /*
>>>>     * Create only non-leaf page table entries for the
>>>>     * page range in Xen virtual address space.
>>>
>>> ... I still disagree with the placement here (should be xen/numa.h imo),
>>
>> I don't care too much about which header is used. So I am ok with 
>> numa.h if this is what you really want. However...
>>
>>> and I still don't see why we can't carry out the TODO right away, if we
>>> have to touch all of this anyway. If it's really too much to ask from
>>> the original contributor, I can certainly see about making a patch 
>>> myself
>>
>> There are a few reasons I didn't update with that approach or want to 
>> ask Nicola for this:
>>   1. I assume this will be a macro and it is not clear to me whether 
>> Eclair would be able to detect the visibility change based on the config
>>   2. IMHO, this is obfuscating a bit more the code
>>   3. I don't see why we want to special case first_valid_mfn. I am 
>> sure we have other external variables in the same situation.
>>
>> So if any solution needs to came up, then it needs to be generic, and 
>> I fear this is going to be yet another lengthy work which is not worth 
>> it for this case.
>>
>>> (and I've now added this to my short-term TODO list).
>>
>> Thanks. But as I wrote above, I am not sure I agree such proposal. Let 
>> see what the code looks like.
>>
> 
> Given the controversy that arose on this patch, maybe it's best to drop 
> it now from this series. If then it's agreed that as a temporary 
> workaround you want to leave it as is and deviate, as I proposed 
> elsewhere in the thread, feel free to reach out. Meanwhile, if patch 1 
> looks ok I can mark this series as done and focus on the v2 for R2.1, 
> which is almost ready.

I am still not in favor of deviation. We would need to create a new 
SAF-* (because none of the existing one are not correct) and it is not 
clear how you could argue this is a good deviation.

Just to be clear, this is not a Nack for a deviation. I find just silly 
we are trying to use deviation here just because there is a small roadblock.

To me the goal is to try to improve Xen, not trying to deviate 
everything because this is easier.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

[Stefano Stabellini]

On Fri, 15 Dec 2023, Jan Beulich wrote:
> On 14.12.2023 20:10, Julien Grall wrote:
> > On 14/12/2023 14:15, George Dunlap wrote:
> >> But I do think that it's fair to ask Julien to think about a suitable
> >> wording, since the comment is in a sense to remind him (or other ARM
> >> maintainers) what's needed, and since the eventual solution will be
> >> something to do with the ARM code and architecture anyway.
> > 
> > The comment is for anyone using !NUMA (i.e. all architectures but x86) 
> > :). What about the following (this is Nicola's patch with the comments 
> > reworked):
> 
> This clearly is better, yet then ...
> 
> > --- a/xen/arch/arm/include/asm/numa.h
> > +++ b/xen/arch/arm/include/asm/numa.h
> > @@ -2,8 +2,9 @@
> >   #define __ARCH_ARM_NUMA_H
> > 
> >   #include <xen/mm.h>
> > +#include <xen/types.h>
> > 
> > -typedef u8 nodeid_t;
> > +typedef uint8_t nodeid_t;
> > 
> >   #ifndef CONFIG_NUMA
> > 
> > @@ -11,12 +12,6 @@ typedef u8 nodeid_t;
> >   #define cpu_to_node(cpu) 0
> >   #define node_to_cpumask(node)   (cpu_online_map)
> > 
> > -/*
> > - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
> > - * is required because the dummy helpers are using it.
> > - */
> > -extern mfn_t first_valid_mfn;
> > -
> >   /* XXX: implement NUMA support */
> >   #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
> >   #define node_start_pfn(nid) (mfn_x(first_valid_mfn))
> > diff --git a/xen/arch/ppc/include/asm/numa.h 
> > b/xen/arch/ppc/include/asm/numa.h
> > index 7fdf66c3da74..888de2dbd1eb 100644
> > --- a/xen/arch/ppc/include/asm/numa.h
> > +++ b/xen/arch/ppc/include/asm/numa.h
> > @@ -1,8 +1,8 @@
> >   #ifndef __ASM_PPC_NUMA_H__
> >   #define __ASM_PPC_NUMA_H__
> > 
> > -#include <xen/types.h>
> >   #include <xen/mm.h>
> > +#include <xen/types.h>
> > 
> >   typedef uint8_t nodeid_t;
> > 
> > @@ -10,12 +10,6 @@ typedef uint8_t nodeid_t;
> >   #define cpu_to_node(cpu) 0
> >   #define node_to_cpumask(node)   (cpu_online_map)
> > 
> > -/*
> > - * TODO: make first_valid_mfn static when NUMA is supported on PPC, this
> > - * is required because the dummy helpers are using it.
> > - */
> > -extern mfn_t first_valid_mfn;
> > -
> >   /* XXX: implement NUMA support */
> >   #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
> >   #define node_start_pfn(nid) (mfn_x(first_valid_mfn))
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> > index 9b5df74fddab..d874525916ea 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -255,8 +255,10 @@ static PAGE_LIST_HEAD(page_broken_list);
> >    */
> > 
> >   /*
> > - * first_valid_mfn is exported because it is use in ARM specific NUMA
> > - * helpers. See comment in arch/arm/include/asm/numa.h.
> > + * first_valid_mfn is exported because it is used when !CONFIG_NUMA.
> > + *
> > + * TODO: Consider if we can conditionally export first_valid_mfn based
> > + * on whether NUMA is selected.
> >    */
> >   mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER;
> > 
> > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> > index 3d9b2d05a5c8..a13a9a46ced7 100644
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -118,6 +118,8 @@ int destroy_xen_mappings(unsigned long s, unsigned 
> > long e);
> >   /* Retrieve the MFN mapped by VA in Xen virtual address space. */
> >   mfn_t xen_map_to_mfn(unsigned long va);
> > 
> > +extern mfn_t first_valid_mfn;
> > +
> >   /*
> >    * Create only non-leaf page table entries for the
> >    * page range in Xen virtual address space.
> 
> ... I still disagree with the placement here (should be xen/numa.h imo),

This is where I would call it "good enough" and close it for now. I
think we should commit such a patch (Julien's patch with the placement
in xen/numa.h).

Anything else...


> and I still don't see why we can't carry out the TODO right away, if we
> have to touch all of this anyway. If it's really too much to ask from
> the original contributor, I can certainly see about making a patch myself
> (and I've now added this to my short-term TODO list).

... can still be done later whenever you get to it.

Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

[Nicola Vetrini]

On 2023-12-14 08:53, Jan Beulich wrote:
> On 14.12.2023 03:05, Stefano Stabellini wrote:
>> On Wed, 13 Dec 2023, Jan Beulich wrote:
>>> On 11.12.2023 10:14, Nicola Vetrini wrote:
>>>> --- a/xen/arch/arm/include/asm/numa.h
>>>> +++ b/xen/arch/arm/include/asm/numa.h
>>>> @@ -2,8 +2,9 @@
>>>>  #define __ARCH_ARM_NUMA_H
>>>> 
>>>>  #include <xen/mm.h>
>>> 
>>> With this, ...
>>> 
>>>> +#include <xen/types.h>
>>>> 
>>>> -typedef u8 nodeid_t;
>>>> +typedef uint8_t nodeid_t;
>>>> 
>>>>  #ifndef CONFIG_NUMA
>>>> 
>>>> @@ -12,10 +13,9 @@ typedef u8 nodeid_t;
>>>>  #define node_to_cpumask(node)   (cpu_online_map)
>>>> 
>>>>  /*
>>>> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, 
>>>> this
>>>> - * is required because the dummy helpers are using it.
>>>> + * TODO: define here first_valid_mfn as static when NUMA is 
>>>> supported on Arm,
>>>> + * this is required because the dummy helpers are using it.
>>>>   */
>>>> -extern mfn_t first_valid_mfn;
>>> 
>>> ... and this declaration moved to xen/mm.h, how is it going to be 
>>> possible
>>> to do as the adjusted comment says? The compiler will choke on the 
>>> static
>>> after having seen the extern.
>> 
>> Nicola was just following a review comment by Julien. NUMA has been
>> pending for a while and I wouldn't hold this patch back because of it.
>> I suggest we go with Julien's request (this version of the patch).
>> 
>> If you feel strongly about it, please suggest an alternative.
> 
> Leaving a TODO comment which cannot actually be carried out is just 
> wrong
> imo. And I consider in unfair to ask me to suggest an alternative. The
> (imo obvious) alternative is to drop this patch, if no proper change 
> can
> be proposed. There's nothing wrong with leaving a violation in place,
> when that violation is far from causing any kind of harm. The more that
> the place is already accompanied by a (suitable afaict) comment.
> 
> Jan

I have a proposal: deviate first_valid_mfn as a deliberate workaround to 
NUMA not being supported on ARM and PPC. This still supplies a 
justification and does not imply any other code change.

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