Re: [PATCH v1 00/29] Introduce stub headers necessary for full Xen build

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Fri, 2023-09-22 at 09:00 +0300, Oleksii wrote:
> On Mon, 2023-09-18 at 14:38 +0200, Jan Beulich wrote:
> > On 18.09.2023 14:05, Oleksii wrote:
> > > On Mon, 2023-09-18 at 11:29 +0200, Jan Beulich wrote:
> > > > On 18.09.2023 10:51, Oleksii wrote:
> > > > > On Thu, 2023-09-14 at 17:08 +0200, Jan Beulich wrote:
> > > > > > On 14.09.2023 16:56, Oleksii Kurochko wrote:
> > > > > > > Based on two patch series [1] and [2], the idea of which
> > > > > > > is
> > > > > > > to
> > > > > > > provide minimal
> > > > > > > amount of things for a complete Xen build, a large amount
> > > > > > > of
> > > > > > > headers are the same
> > > > > > > or almost the same, so it makes sense to move them to
> > > > > > > asm-
> > > > > > > generic.
> > > > > > > 
> > > > > > > Also, providing such stub headers should help future
> > > > > > > architectures
> > > > > > > to add
> > > > > > > a full Xen build.
> > > > > > > 
> > > > > > > [1]
> > > > > > > https://lore.kernel.org/xen-devel/cover.1694543103.git.sanastasio@xxxxxxxxxxxxxxxxxxxxx/
> > > > > > > [2]
> > > > > > > https://lore.kernel.org/xen-devel/cover.1692181079.git.oleksii.kurochko@xxxxxxxxx/
> > > > > > > 
> > > > > > > Oleksii Kurochko (29):
> > > > > > >   xen/asm-generic: introduce stub header spinlock.h
> > > > > > 
> > > > > > At the example of this, personally I think this goes too
> > > > > > far.
> > > > > > Headers
> > > > > > in
> > > > > > asm-generic should be for the case where an arch elects to
> > > > > > not
> > > > > > implement
> > > > > > certain functionality. Clearly spinlocks are required
> > > > > > uniformly.
> > > > > It makes sense. Then I will back to the option [2] where I
> > > > > introduced
> > > > > all this headers as part of RISC-V architecture. 
> > > > 
> > > > You did see though that in a reply to my own mail I said I take
> > > > back
> > > > the
> > > > comment, at least as far as this header (and perhaps several
> > > > others)
> > > > are
> > > > concerned.
> > > > 
> > > I missed that comment on the patch about spinlock.
> > > 
> > > Well, then, I don't fully understand the criteria.
> > > 
> > > What about empty headers or temporary empty headers?
> > > 
> > > For example, asm/xenoprof.h is empty for all arches except x86,
> > > so
> > > it
> > > is a good candidate for asm-generic.
> > 
> > That's an example where I think it is wrong (or at least
> > unnecessary)
> > for
> > the xen/ header to include the asm/ one irrespective of the
> > controlling
> > CONFIG_* setting. From what I can tell common code would build fine
> > with
> > the #include moved; x86 code may require an adjustment or two. IOW
> > this
> > is a case where I think preferably presence of an arch header was
> > required only when XENOPROF can actually be yet to y in Kconfig.
> > 
> > > But asm/grant_table.h is empty for PPC and RISC-V for now but
> > > won't
> > > be
> > > empty in the future. Does it make sense to put them to asm-
> > > generic?
> > > The
> > > only benefit I see is that in future architecture if they follow
> > > the
> > > same way of adding support for the arch to Xen, they will face
> > > the
> > > same
> > > issue: building full Xen requires this empty header.
> > 
> > Here I can see different ways of looking at it. Personally I'd
> > prefer
> > stub headers to be used only if, for the foreseeable future, they
> > are
> > intended to remain in use. grant_table.h pretty clearly doesn't
> > fall
> > in
> > this category. (You may want to peek at what's being done on the
> > PPC
> > side. Nevertheless some of what's done there could likely benefit
> > from
> > what you're doing here.)
> > 
> > > So, should I wait for some time on other patches of the patch
> > > series?
> > 
> > Well, afaic I'd prefer if I got a chance to look over at least some
> > more
> > of the patches in this series. But you're of course free to submit
> > a
> > v2
> > at any time.
> I think that it will be better to wait for some time not to produce
> unnecessary patches.
Hmm... but my gitlab CI told me that there is an issue with riscv64
build I'll double-check.

~ Oleksii