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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

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)