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

[Julien Grall <julien@xxxxxxx>]

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