Re: [PATCH v3 13/19] xen/arm: Move fixmap definitions in a separate header

[Julien Grall <julien@xxxxxxx>]

Hi,

On 06/04/2022 01:10, Stefano Stabellini wrote:
> On Tue, 5 Apr 2022, Julien Grall wrote:
> > On 05/04/2022 22:12, Stefano Stabellini wrote:
> > > > +/* Map a page in a fixmap entry */
> > > > +extern void set_fixmap(unsigned map, mfn_t mfn, unsigned attributes);
> > > > +/* Remove a mapping from a fixmap entry */
> > > > +extern void clear_fixmap(unsigned map);
> > > > +
> > > > +#endif /* __ASSEMBLY__ */
> > > > +
> > > > +#endif /* __ASM_FIXMAP_H */
> > >
> > > It is a good idea to create fixmap.h, but I think it should be acpi.h to
> > > include fixmap.h, not the other way around.
> > As I wrote in the commit message, one definition in fixmap.h rely on define
> > from acpi.h (i.e NUM_FIXMAP_ACPI_PAGES). So if we don't include it, then user
> > of FIXMAP_PMAP_BEGIN (see next patch) will requires to include acpi.h in order
> > to build.
> >
> > Re-ordering the values would not help because the problem would exactly be the
> > same but this time the acpi users would have to include pmap.h to define
> > NUM_FIX_PMAP.
> >
> > > The appended changes build correctly on top of this patch.
> > That's expected because all the users of FIXMAP_ACPI_END will be including
> > acpi.h. But after the next patch, we would need pmap.c to include acpi.h.
> >
> > I don't think this would be right (and quite likely you would ask why
> > this is done). Hence this approach.
>
> I premise that I see your point and I don't feel very strongly either
> way. In my opinion the fixmap is the low level "library" that others
> make use of, so it should be acpi.h and pmap.h (the clients of the
> library) that include fixmap.h and not the other way around.
That's one way to see it. The way I see it is each component decide how 
much entries they need. So I think it is better to move the definition 
to each components as they are best suited to decide the value.
> However, I won't insist if you don't like it. Rough
> patch below for reference.
I want to stay close to what x86 is doing to avoid any headers mess. 
This is what my patch is doing. Now, if x86 folks are happy to move the 
definition per-arch or in a xen/fixmap.h. Then I can look at it.
Andrew, Jan, Roger, Wei, what do you think?

Cheers,

--
Julien Grall