On 24.08.2022 14:43, Bertrand Marquis wrote: > > >> On 24 Aug 2022, at 08:37, Jan Beulich <jbeul...@suse.com> wrote: >> >> On 23.08.2022 17:56, Bertrand Marquis wrote: >>>> On 23 Aug 2022, at 16:45, Jan Beulich <jbeul...@suse.com> wrote: >>>> On 23.08.2022 17:09, Bertrand Marquis wrote: >>>>> How about moving those to a xen-acpi.h header and include that one in >>>>> xen.h ? >>>> >>>> In principle okay, if there wasn't the need for HVM_MAX_VCPUS. With a >>>> suitable comment it may be okay to live there. I'd be curious what >>>> others think. >>> >>> The problem with this already exists in the current status as this is >>> defined in >>> hvm_info_table.h which is never included from arch-x86/xen.h >> >> You're referring to it being necessary to explicitly include both headers. >> That's not what I'm referring to, though: The tool imo shouldn't include >> hvm_info_table.h, and hence the HVM_MAX_VCPUS would need to move as well. > > Any suggestion where ?
Not really, no. That's why I said this is the one part where improvement is more difficult. Otoh hvm_info_table.h is self-contained right now and doesn't even produce potentially misleading struct layout for the one struct it declares. So perhaps not too bad if left alone. > The more I dig, the more I find that everything is including xen.h and going > round. > Arch-x86_*.h headers are including arch-x86/xen.h including xen.h Indeed, all quite odd. >>> Including hvm_info_table.h from xen-acpi.h could create include path issues. >> >> Include path issues? Both are / would be public headers. But as said, I >> don't think any new header introduced for the purpose at hand should >> include _any_ other public header. > > For now I can create a arch-x86/xen-acpi.h and move there the XEN_ACPI_* > definitions and include that one instead in mk_dsdt.h. > The change will be small and should not have much impact. > > Modifying HVM_MAX_VCPUS is not per say needed and I think would not be > enough to make the situation cleaner. > >> >>> But as those are used nowhere apart from mk_dsdt, I would probably skip the >>> include of xen-acpi.h from xen.h. >> >> Hmm, yes, that's reasonable I guess as far as XEN_ACPI_* go. Of course >> HVM_MAX_VCPUS is a different matter. >> >>> Any chance that those XEN_ACPI_ are needed by some external tools that >>> could get broken by this modification ? >> >> Requiring them to include another header is, I think, a tolerable form >> of breakage, the more that such breakage isn't very likely anyway. > > Then if you are ok with it, I will just submit the xen-acpi.h creation patch > and fix > mk_dsdt compilation for x86 on arm. > > The rest would require more thinking and I do not think it should be done now. > > Can you confirm you agree ? Almost - I don't like xen-acpi.h as the name of the new header. Just arch-x86/acpi.h would likely be too generic, so I'd like to suggest arch-x86/hvm-acpi.h or arch-x86/guest-acpi.h. I have a slight preference to the latter, because "hvm" also covers "pvh", yet PVH Dom0 is dealt with entirely differently ACPI-wise. Plus "guest" isn't misleading as to PV, because PV guests don't have ACPI anyway. Jan
