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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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)