On 06.09.2024 17:59, Andrew Cooper wrote: > On 04/09/2024 3:56 pm, Frediano Ziglio wrote: >> This RFC series attempt to: >> - use more C code, that is replace some assembly code with C; >> - avoid some code duplication between C and assembly; >> - prevent some issues having relocations in C code. >> >> The idea is extending the current C to binary code conversion >> done for 32 bit C code called from head.S making sure relocations >> are safe and allowing external symbols usage from C code. >> >> Note that, as an addition, scripts generating code check for no >> data to allow code and data separation. >> >> More details of the implementation are in commit message 2/5, >> which is the largest patch. >> Patch 1/5 is to prepare code and avoid data. >> Patch 3/5 is an example of code reuse between 32 and 64 bit. >> Patch 4/5 is also another example of code reuse but is more hacky and >> dirty due to not being possible include use some headers. >> >> Code boot successfully using: >> - BIOS boot; >> - EFI boot with Grub2 and ELF file; >> - direct EFI boot without Grub. >> >> Suggestions/opinions are welcome. >> >> Code is currently based on "staging" branch, currently commit >> 6471badeeec92db1cb8155066551f7509cd82efd. > > I fully support taking logic out of asm and writing it in C, as well as > taking steps to dedup the EFI and non-EFI paths. A couple of > observations before diving into the details. > > The visibility pragmas mean that you've lost the `-include xen/config.h` > from the $(CC) invocation. We use this to get some Xen-wide settings > everywhere, which includes handling visibility. > > The symlinks for dual builds are going to cause problems for tarball > archives. Instead you can encode this with make rules. e.g. > > obj-y += foo32.o foo64.o > > %32.o: %.c > $(CC) -m32 ... > > %64.o: %.c > $(CC) -m64 ... > > will build two different .o's from the same .c. This is how XTF builds > different tests from the same source. > > > I'm on the fence with the ifdefary and bit suffixes. I don't think we > need the error case because x86_128 isn't coming along any time soon. > > For completeness, there's a trick used by the shadow code (see > SHADOW_INTERNAL_NAME()) which adds a suffix without local ifdefary. > It's nicer to read, but breaks grep/cscope/etc. I'm torn as to which is > the lesser evil.
While generally I prefer that approach shadow code takes, I think the #ifdef-ary is acceptably bounded here. What I'm more worried by are the fair number of #define-s for the 32-bit case of setting up page tables. Jan
