| Message ID | cover.1694702259.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce stub headers necessary for full Xen build | expand |
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@raptorengineering.com/ > [2] https://lore.kernel.org/xen-devel/cover.1692181079.git.oleksii.kurochko@gmail.com/ > > 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. Jan
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@raptorengineering.com/ > > [2] > > https://lore.kernel.org/xen-devel/cover.1692181079.git.oleksii.kurochko@gmail.com/ > > > > 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. ~ Oleksii
On Mon, 2023-09-18 at 11:51 +0300, 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@raptorengineering.com/ > > > [2] > > > https://lore.kernel.org/xen-devel/cover.1692181079.git.oleksii.kurochko@gmail.com/ > > > > > > 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. And I will review the current patch series probably it is still can be something moved to asm-generic. > > ~ Oleksii
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@raptorengineering.com/ >>> [2] >>> https://lore.kernel.org/xen-devel/cover.1692181079.git.oleksii.kurochko@gmail.com/ >>> >>> 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. Jan
Hi Jan, On 18/09/2023 10:29, 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@raptorengineering.com/ >>>> [2] >>>> https://lore.kernel.org/xen-devel/cover.1692181079.git.oleksii.kurochko@gmail.com/ >>>> >>>> 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, I can't find a reply to our own mail in my inbox. Do you have a message-id? ? at least as far as this header (and perhaps several others) are > concerned. Do you have a list where you think they should be kept? Or are you planning to answer to all you disagree/agree one by one? Cheers,
On 18.09.2023 11:32, Julien Grall wrote: > Hi Jan, > > On 18/09/2023 10:29, 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@raptorengineering.com/ >>>>> [2] >>>>> https://lore.kernel.org/xen-devel/cover.1692181079.git.oleksii.kurochko@gmail.com/ >>>>> >>>>> 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, > > I can't find a reply to our own mail in my inbox. Do you have a message-id? Oh, sorry, I said so in reply to 01/29. > ? at least as far as this header (and perhaps several others) are >> concerned. > > Do you have a list where you think they should be kept? Or are you > planning to answer to all you disagree/agree one by one? I think this can only be one-by-one. Jan
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@raptorengineering.com/ > > > > [2] > > > > https://lore.kernel.org/xen-devel/cover.1692181079.git.oleksii.kurochko@gmail.com/ > > > > > > > > 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. 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. So, should I wait for some time on other patches of the patch series? ~ Oleksii
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@raptorengineering.com/ >>>>> [2] >>>>> https://lore.kernel.org/xen-devel/cover.1692181079.git.oleksii.kurochko@gmail.com/ >>>>> >>>>> 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. Jan
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@raptorengineering.com/ > > > > > > [2] > > > > > > https://lore.kernel.org/xen-devel/cover.1692181079.git.oleksii.kurochko@gmail.com/ > > > > > > > > > > > > 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. ~ Oleksii
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@raptorengineering.com/ > > > > > > > [2] > > > > > > > https://lore.kernel.org/xen-devel/cover.1692181079.git.oleksii.kurochko@gmail.com/ > > > > > > > > > > > > > > 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