| Message ID | 5d5ec5fbd8787ed8f86bf84a5ac291d07a20b307.1671789736.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add minimal RISC-V Xen build and build testing | expand |
Hi Oleksii, + Anthony for the Makefile changes. On 23/12/2022 11:16, Oleksii Kurochko wrote: > The patch provides a minimal amount of changes to start > build and run minimal Xen binary at GitLab CI&CD that will > allow continuous checking of the build status of RISC-V Xen. > > RISC-V Xen can be built by the following instructions: > $ CONTAINER=riscv64 ./automation/scripts/containerize \ > make XEN_TARGET_ARCH=riscv64 tiny64_defconfig > $ CONTAINER=riscv64 ./automation/scripts/containerize \ > make XEN_TARGET_ARCH=riscv64 -C xen build > > RISC-V Xen can be run as: > $ qemu-system-riscv64 -M virt -smp 1 -nographic -m 2g \ > -kernel xen/xen > > To run in debug mode should be done the following instructions: > $ qemu-system-riscv64 -M virt -smp 1 -nographic -m 2g \ > -kernel xen/xen -s -S > # In separate terminal: > $ riscv64-buildroot-linux-gnu-gdb > $ target remote :1234 > $ add-symbol-file <xen_src>/xen/xen-syms 0x80200000 > $ hb *0x80200000 > $ c # it should stop at instruction j 0x80200000 <start> > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > xen/arch/riscv/Makefile | 30 +++++++++++++ > xen/arch/riscv/arch.mk | 10 +++++ > xen/arch/riscv/include/asm/config.h | 26 ++++++++++- > xen/arch/riscv/include/asm/types.h | 11 +++++ > xen/arch/riscv/riscv64/Makefile | 2 +- > xen/arch/riscv/riscv64/head.S | 2 +- > xen/arch/riscv/xen.lds.S | 69 +++++++++++++++++++++++++++++ > 7 files changed, 147 insertions(+), 3 deletions(-) > create mode 100644 xen/arch/riscv/include/asm/types.h > create mode 100644 xen/arch/riscv/xen.lds.S > > diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile > index 942e4ffbc1..893dd19ea6 100644 > --- a/xen/arch/riscv/Makefile > +++ b/xen/arch/riscv/Makefile > @@ -1,2 +1,32 @@ > +obj-$(CONFIG_RISCV_64) += riscv64/ > + > +$(TARGET): $(TARGET)-syms > + $(OBJCOPY) -O binary -S $< $@ > + > +$(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds > + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \ > + $(SYMBOLS_DUMMY_OBJ) -o $(@D)/.$(@F).0 > + $(NM) -pa --format=sysv $(@D)/.$(@F).0 \ > + | $(objtree)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).0.S > + $(MAKE) $(build)=$(@D) $(@D)/.$(@F).0.o > + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \ > + $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1 > + $(NM) -pa --format=sysv $(@D)/.$(@F).1 \ > + | $(objtree)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).1.S > + $(MAKE) $(build)=$(@D) $(@D)/.$(@F).1.o > + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) \ > + $(@D)/.$(@F).1.o -o $@ > + $(NM) -pa --format=sysv $(@D)/$(@F) \ > + | $(objtree)/tools/symbols --all-symbols --xensyms --sysv --sort \ > + >$(@D)/$(@F).map > + rm -f $(@D)/.$(@F).[0-9]* > + > +$(obj)/xen.lds: $(src)/xen.lds.S FORCE > + $(call if_changed_dep,cpp_lds_S) > + > +.PHONY: clean > +clean:: > + rm -f $(objtree)/.xen-syms.[0-9]* Any reason to not use the variable clean-files as this is done for the other architectures? > + > .PHONY: include > include: > diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk > index ae8fe9dec7..9292b72718 100644 > --- a/xen/arch/riscv/arch.mk > +++ b/xen/arch/riscv/arch.mk > @@ -11,3 +11,13 @@ riscv-march-$(CONFIG_RISCV_ISA_C) := $(riscv-march-y)c > # -mcmodel=medlow would force Xen into the lower half. > > CFLAGS += -march=$(riscv-march-y) -mstrict-align -mcmodel=medany > + > +# TODO: Drop override and SYMBOLS_DUMMY_OBJ when more > +# of the build is working > +override ALL_OBJS-y = arch/$(TARGET_ARCH)/built_in.o > +override ALL_LIBS-y = > +ifneq ($(wildcard $(objtree)/common/symbols-dummy.o),) > +SYMBOLS_DUMMY_OBJ=$(objtree)/common/symbols-dummy.o > +else > +SYMBOLS_DUMMY_OBJ= > +endif Why do you need the ifneq here? > diff --git a/xen/arch/riscv/include/asm/config.h b/xen/arch/riscv/include/asm/config.h > index e2ae21de61..756607a4a2 100644 > --- a/xen/arch/riscv/include/asm/config.h > +++ b/xen/arch/riscv/include/asm/config.h > @@ -28,7 +28,7 @@ > > /* Linkage for RISCV */ > #ifdef __ASSEMBLY__ > -#define ALIGN .align 2 > +#define ALIGN .align 4 Can you explain in the commit message why you need to change this? But ideally, this change should be part of a separate one. > > #define ENTRY(name) \ > .globl name; \ > @@ -36,6 +36,30 @@ > name: > #endif > > +/* > + * Definition of XEN_VIRT_START should look like: > + * define XEN_VIRT_START _AT(vaddr_t,0x00200000) > + * It requires including of additional headers which > + * will increase an amount of files unnecessary for > + * minimal RISC-V Xen build so set value of > + * XEN_VIRT_START explicitly. > + * > + * TODO: change it to _AT(vaddr_t,0x00200000) when > + * necessary header will be pushed. The address here doesn't match the one below. I know this is an example but this is fairly confusing. > + */ > +#define XEN_VIRT_START 0x80200000 I think you at least want to use _AT(unsigned long, ...) here to make clear this value should be interpreted as an unsigned value. You could even define vaddr_t now as you introduce a dummy types.h below. > +/* > + * PAGE_SIZE is defined in <{asm,xen}/page-*.h> but to > + * remove unnecessary headers for minimal > + * build headers it will be better to set PAGE_SIZE > + * explicitly. TBH, I think this is a shortcut that is unnecessary and will only introduce extra churn in the future (for instance, you will need to modify the include in xen.lds.S). But I am not the maintainer, so I will leave that decision to them whether this is acceptable. > + * > + * TODO: remove it when <asm/page-*.h> will be needed > + * defintion of PAGE_SIZE should be remove from s/defintion/definition/ > + * this header. > + */ > +#define PAGE_SIZE 4096 > + > #endif /* __RISCV_CONFIG_H__ */ > /* > * Local variables: > diff --git a/xen/arch/riscv/include/asm/types.h b/xen/arch/riscv/include/asm/types.h > new file mode 100644 > index 0000000000..afbca6b15c > --- /dev/null > +++ b/xen/arch/riscv/include/asm/types.h > @@ -0,0 +1,11 @@ > +#ifndef __TYPES_H__ > +#define __TYPES_H__ > + > +/* > + * > + * asm/types.h is required for xen-syms.S file which > + * is produced by tools/symbols. > + * > + */ > + > +#endif > diff --git a/xen/arch/riscv/riscv64/Makefile b/xen/arch/riscv/riscv64/Makefile > index 15a4a65f66..3340058c08 100644 > --- a/xen/arch/riscv/riscv64/Makefile > +++ b/xen/arch/riscv/riscv64/Makefile > @@ -1 +1 @@ > -extra-y += head.o > +obj-y += head.o This changes want to be explained. So does... > diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S > index 0dbc27ba75..0330b29c01 100644 > --- a/xen/arch/riscv/riscv64/head.S > +++ b/xen/arch/riscv/riscv64/head.S > @@ -1,6 +1,6 @@ > #include <asm/config.h> > > - .text > + .section .text.header, "ax", %progbits ... AFAICT this is to match the recent change in the build system. > > ENTRY(start) > j start > diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S > new file mode 100644 > index 0000000000..60628b3856 > --- /dev/null > +++ b/xen/arch/riscv/xen.lds.S > @@ -0,0 +1,69 @@ > +#include <xen/xen.lds.h> > + > +#undef ENTRY > +#undef ALIGN > + > +OUTPUT_ARCH(riscv) > +ENTRY(start) > + > +PHDRS > +{ > + text PT_LOAD ; > +#if defined(BUILD_ID) > + note PT_NOTE ; > +#endif > +} > + > +SECTIONS > +{ > + . = XEN_VIRT_START; > + _start = .; > + .text : { > + _stext = .; > + *(.text.header) > + *(.text) You are missing some sections here such as .text.cold, .text.unlikely... I understand they are probably not used yet. But it will avoid any nasty surprise if they are forgotten. > + *(.gnu.warning) > + . = ALIGN(POINTER_ALIGN); > + _etext = .; > + } :text > + > + . = ALIGN(PAGE_SIZE); > + .rodata : { > + _srodata = .; You introduce _srodata but I can't find the matching _erodata. > + *(.rodata) > + *(.rodata.*) > + *(.data.rel.ro) > + *(.data.rel.ro.*) > + } :text > + > +#if defined(BUILD_ID) > + . = ALIGN(4); > + .note.gnu.build-id : { > + __note_gnu_build_id_start = .; > + *(.note.gnu.build-id) > + __note_gnu_build_id_end = .; > + } :note :text > +#endif > + > + . = ALIGN(PAGE_SIZE); > + .data : { /* Data */ > + *(.data .data.*) It would be better if you introduce .data.read_mostly right now separately. You also want *.data.page_aligned in .data. Lastly you are missing CONSTRUCTORS > + } :text > + I am assuming you are going to add .init.* afterwards? > + . = ALIGN(PAGE_SIZE); > + .bss : { > + __bss_start = .; > + *(.bss .bss.*) > + . = ALIGN(POINTER_ALIGN); > + __bss_end = .; Same as .data, I would recommend to properly populate it. Cheers,
Hi Julien, Thanks for your comments. On Fri, 2022-12-23 at 13:50 +0000, Julien Grall wrote: > Hi Oleksii, > > + Anthony for the Makefile changes. > > On 23/12/2022 11:16, Oleksii Kurochko wrote: > > The patch provides a minimal amount of changes to start > > build and run minimal Xen binary at GitLab CI&CD that will > > allow continuous checking of the build status of RISC-V Xen. > > > > RISC-V Xen can be built by the following instructions: > > $ CONTAINER=riscv64 ./automation/scripts/containerize \ > > make XEN_TARGET_ARCH=riscv64 tiny64_defconfig > > $ CONTAINER=riscv64 ./automation/scripts/containerize \ > > make XEN_TARGET_ARCH=riscv64 -C xen build > > > > RISC-V Xen can be run as: > > $ qemu-system-riscv64 -M virt -smp 1 -nographic -m 2g \ > > -kernel xen/xen > > > > To run in debug mode should be done the following instructions: > > $ qemu-system-riscv64 -M virt -smp 1 -nographic -m 2g \ > > -kernel xen/xen -s -S > > # In separate terminal: > > $ riscv64-buildroot-linux-gnu-gdb > > $ target remote :1234 > > $ add-symbol-file <xen_src>/xen/xen-syms 0x80200000 > > $ hb *0x80200000 > > $ c # it should stop at instruction j 0x80200000 <start> > > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > --- > > xen/arch/riscv/Makefile | 30 +++++++++++++ > > xen/arch/riscv/arch.mk | 10 +++++ > > xen/arch/riscv/include/asm/config.h | 26 ++++++++++- > > xen/arch/riscv/include/asm/types.h | 11 +++++ > > xen/arch/riscv/riscv64/Makefile | 2 +- > > xen/arch/riscv/riscv64/head.S | 2 +- > > xen/arch/riscv/xen.lds.S | 69 > > +++++++++++++++++++++++++++++ > > 7 files changed, 147 insertions(+), 3 deletions(-) > > create mode 100644 xen/arch/riscv/include/asm/types.h > > create mode 100644 xen/arch/riscv/xen.lds.S > > > > diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile > > index 942e4ffbc1..893dd19ea6 100644 > > --- a/xen/arch/riscv/Makefile > > +++ b/xen/arch/riscv/Makefile > > @@ -1,2 +1,32 @@ > > +obj-$(CONFIG_RISCV_64) += riscv64/ > > + > > +$(TARGET): $(TARGET)-syms > > + $(OBJCOPY) -O binary -S $< $@ > > + > > +$(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds > > + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \ > > + $(SYMBOLS_DUMMY_OBJ) -o $(@D)/.$(@F).0 > > + $(NM) -pa --format=sysv $(@D)/.$(@F).0 \ > > + | $(objtree)/tools/symbols $(all_symbols) --sysv -- > > sort >$(@D)/.$(@F).0.S > > + $(MAKE) $(build)=$(@D) $(@D)/.$(@F).0.o > > + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \ > > + $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1 > > + $(NM) -pa --format=sysv $(@D)/.$(@F).1 \ > > + | $(objtree)/tools/symbols $(all_symbols) --sysv -- > > sort >$(@D)/.$(@F).1.S > > + $(MAKE) $(build)=$(@D) $(@D)/.$(@F).1.o > > + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< > > $(build_id_linker) \ > > + $(@D)/.$(@F).1.o -o $@ > > + $(NM) -pa --format=sysv $(@D)/$(@F) \ > > + | $(objtree)/tools/symbols --all-symbols --xensyms > > --sysv --sort \ > > + >$(@D)/$(@F).map > > + rm -f $(@D)/.$(@F).[0-9]* > > + > > +$(obj)/xen.lds: $(src)/xen.lds.S FORCE > > + $(call if_changed_dep,cpp_lds_S) > > + > > +.PHONY: clean > > +clean:: > > + rm -f $(objtree)/.xen-syms.[0-9]* > > Any reason to not use the variable clean-files as this is done for > the > other architectures? There is no reason not use the variable clean-files. I missed the vairable clean-files so the patch will be updated to use the variable clean-files instead of the variable clean. > > > + > > .PHONY: include > > include: > > diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk > > index ae8fe9dec7..9292b72718 100644 > > --- a/xen/arch/riscv/arch.mk > > +++ b/xen/arch/riscv/arch.mk > > @@ -11,3 +11,13 @@ riscv-march-$(CONFIG_RISCV_ISA_C) := > > $(riscv-march-y)c > > # -mcmodel=medlow would force Xen into the lower half. > > > > CFLAGS += -march=$(riscv-march-y) -mstrict-align -mcmodel=medany > > + > > +# TODO: Drop override and SYMBOLS_DUMMY_OBJ when more > > +# of the build is working > > +override ALL_OBJS-y = arch/$(TARGET_ARCH)/built_in.o > > +override ALL_LIBS-y = > > +ifneq ($(wildcard $(objtree)/common/symbols-dummy.o),) > > +SYMBOLS_DUMMY_OBJ=$(objtree)/common/symbols-dummy.o > > +else > > +SYMBOLS_DUMMY_OBJ= > > +endif > > Why do you need the ifneq here? The only reason for the ifneq here is to skip common stuff from the build of minimal RISC-V Xen binary as it requires pushing from the start a big amount of headers and function stubs which at least will lead to a huge patch and complicate a code review. It is possible to remove <common/symbols-dummy.o> from xen-syms target in <xen/arch/riscv/Makefile> but an intention here was to remain processing of xen-syms target similar to the original one and it is the second reason why the ifneq was introduced and added the comment "TODO: Drop ... SYMBOLS_DUMMY_OBJ when more of the build is working". > > > diff --git a/xen/arch/riscv/include/asm/config.h > > b/xen/arch/riscv/include/asm/config.h > > index e2ae21de61..756607a4a2 100644 > > --- a/xen/arch/riscv/include/asm/config.h > > +++ b/xen/arch/riscv/include/asm/config.h > > @@ -28,7 +28,7 @@ > > > > /* Linkage for RISCV */ > > #ifdef __ASSEMBLY__ > > -#define ALIGN .align 2 > > +#define ALIGN .align 4 > > Can you explain in the commit message why you need to change this? > But > ideally, this change should be part of a separate one. ALIGN was changed to equal the implementation-enforced instruction address alignment (4-bytes), in order to ensure that entry points are properly aligned. If to be honest I haven't verified that and took these changes from the initial patch series pushed by Bobby Eshleman. > > > > > #define ENTRY(name) \ > > .globl name; \ > > @@ -36,6 +36,30 @@ > > name: > > #endif > > > > +/* > > + * Definition of XEN_VIRT_START should look like: > > + * define XEN_VIRT_START _AT(vaddr_t,0x00200000) > > + * It requires including of additional headers which > > + * will increase an amount of files unnecessary for > > + * minimal RISC-V Xen build so set value of > > + * XEN_VIRT_START explicitly. > > + * > > + * TODO: change it to _AT(vaddr_t,0x00200000) when > > + * necessary header will be pushed. > > The address here doesn't match the one below. I know this is an > example > but this is fairly confusing. This was done only as an example. > > > + */ > > +#define XEN_VIRT_START 0x80200000 > > I think you at least want to use _AT(unsigned long, ...) here to make > clear this value should be interpreted as an unsigned value. > > You could even define vaddr_t now as you introduce a dummy types.h > below. It makes sense to push the definition of vaddr_t and use <xen/const.h> to be able to use _AT() macros. Probably it would be nice to introduce others from types.h from the start, wouldn't it? Or would it be better to leave the patch minimal and introduce only types necessary for vaddr_t? > > > +/* > > + * PAGE_SIZE is defined in <{asm,xen}/page-*.h> but to > > + * remove unnecessary headers for minimal > > + * build headers it will be better to set PAGE_SIZE > > + * explicitly. > > TBH, I think this is a shortcut that is unnecessary and will only > introduce extra churn in the future (for instance, you will need to > modify the include in xen.lds.S). > > But I am not the maintainer, so I will leave that decision to them > whether this is acceptable. I didn't get what you mean by a shortcut here. The idea was to introduce PAGE_SIZE in the config.h directly for now to keep build of RISC-V Xen minimal as much as possible otherwise it would be required to push dummy <asm/page-bits.h> to get only one needed definition of PAGE_SIZE to have buildable Xen. Thereby it was decided to define PAGE_SIZE directly in <asm/config.h> and remove it after all definitions from <{asm,xen}/page-*.h> will be needed. > > > + * > > + * TODO: remove it when <asm/page-*.h> will be needed > > + * defintion of PAGE_SIZE should be remove from > > s/defintion/definition/ Thanks for finding the typo. Will update it in the next version of the patch. > > > + * this header. > > + */ > > +#define PAGE_SIZE 4096 > + > > #endif /* __RISCV_CONFIG_H__ */ > > /* > > * Local variables: > > diff --git a/xen/arch/riscv/include/asm/types.h > > b/xen/arch/riscv/include/asm/types.h > > new file mode 100644 > > index 0000000000..afbca6b15c > > --- /dev/null > > +++ b/xen/arch/riscv/include/asm/types.h > > @@ -0,0 +1,11 @@ > > +#ifndef __TYPES_H__ > > +#define __TYPES_H__ > > + > > +/* > > + * > > + * asm/types.h is required for xen-syms.S file which > > + * is produced by tools/symbols. > > + * > > + */ > > + > > +#endif > > diff --git a/xen/arch/riscv/riscv64/Makefile > > b/xen/arch/riscv/riscv64/Makefile > > index 15a4a65f66..3340058c08 100644 > > --- a/xen/arch/riscv/riscv64/Makefile > > +++ b/xen/arch/riscv/riscv64/Makefile > > @@ -1 +1 @@ > > -extra-y += head.o > > +obj-y += head.o > > This changes want to be explained. So does... RISC-V 64 would be supported now and minimal build is built now only obj-y stuff. > > > diff --git a/xen/arch/riscv/riscv64/head.S > > b/xen/arch/riscv/riscv64/head.S > > index 0dbc27ba75..0330b29c01 100644 > > --- a/xen/arch/riscv/riscv64/head.S > > +++ b/xen/arch/riscv/riscv64/head.S > > @@ -1,6 +1,6 @@ > > #include <asm/config.h> > > > > - .text > > + .section .text.header, "ax", %progbits > > ... AFAICT this is to match the recent change in the build system. I am not sure that I get you here but it was done to make 'start' instructions to be the first instructions that will be executed and to make 'start' symbol to be the first in xen-syms.map file otherwise gdb shows that Xen starts from the wrong instructions, fails to find correct source file and goes crazy. > > > > > ENTRY(start) > > j start > > diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S > > new file mode 100644 > > index 0000000000..60628b3856 > > --- /dev/null > > +++ b/xen/arch/riscv/xen.lds.S > > @@ -0,0 +1,69 @@ > > +#include <xen/xen.lds.h> > > + > > +#undef ENTRY > > +#undef ALIGN > > + > > +OUTPUT_ARCH(riscv) > > +ENTRY(start) > > + > > +PHDRS > > +{ > > + text PT_LOAD ; > > +#if defined(BUILD_ID) > > + note PT_NOTE ; > > +#endif > > +} > > + > > +SECTIONS > > +{ > > + . = XEN_VIRT_START; > > + _start = .; > > + .text : { > > + _stext = .; > > + *(.text.header) > > + *(.text) > > You are missing some sections here such as .text.cold, > .text.unlikely... > > I understand they are probably not used yet. But it will avoid any > nasty > surprise if they are forgotten. They were skipped because they aren't use for now. Will add it in the next version of the patch. > > > + *(.gnu.warning) > > + . = ALIGN(POINTER_ALIGN); > > + _etext = .; > > + } :text > > + > > + . = ALIGN(PAGE_SIZE); > > + .rodata : { > > + _srodata = .; > > You introduce _srodata but I can't find the matching _erodata. My fault. Thanks. Will add it in the the next version of the patch. > > > + *(.rodata) > > + *(.rodata.*) > > + *(.data.rel.ro) > > + *(.data.rel.ro.*) > > + } :text > > + > > +#if defined(BUILD_ID) > > + . = ALIGN(4); > > + .note.gnu.build-id : { > > + __note_gnu_build_id_start = .; > > + *(.note.gnu.build-id) > > + __note_gnu_build_id_end = .; > > + } :note :text > > +#endif > > + > > + . = ALIGN(PAGE_SIZE); > > + .data : { /* Data */ > > + *(.data .data.*) > > It would be better if you introduce .data.read_mostly right now > separately. > > You also want *.data.page_aligned in .data. > > Lastly you are missing CONSTRUCTORS I will add offred sections and CONSTUCTORS in the next version of the patch > > > + } :text > > + > > I am assuming you are going to add .init.* afterwards? > > > + . = ALIGN(PAGE_SIZE); > > + .bss : { > > + __bss_start = .; > > + *(.bss .bss.*) > > + . = ALIGN(POINTER_ALIGN); > > + __bss_end = .; > > Same as .data, I would recommend to properly populate it. Do you mean to add .bss.stack_aligned, .bss.page_aligned, .bss.percpu*? One of the reasons they were skipped is they aren't used now and one more reason if to believe xen.lds.S file from ARM .bss.percpu.read_mostly should be aligned by SMP_CACHE_BYTES which requires dummy <asm/cache.h> (or not ?) which will increase the patch with unneeded stuff for now. > > Cheers, > Best regards, Oleksii
On Mon, Dec 26, 2022 at 9:14 PM Oleksii <oleksii.kurochko@gmail.com> wrote: > > Hi Julien, > > Thanks for your comments. > > On Fri, 2022-12-23 at 13:50 +0000, Julien Grall wrote: > > Hi Oleksii, > > > > + Anthony for the Makefile changes. > > > > On 23/12/2022 11:16, Oleksii Kurochko wrote: > > > The patch provides a minimal amount of changes to start > > > build and run minimal Xen binary at GitLab CI&CD that will > > > allow continuous checking of the build status of RISC-V Xen. > > > > > > RISC-V Xen can be built by the following instructions: > > > $ CONTAINER=riscv64 ./automation/scripts/containerize \ > > > make XEN_TARGET_ARCH=riscv64 tiny64_defconfig > > > $ CONTAINER=riscv64 ./automation/scripts/containerize \ > > > make XEN_TARGET_ARCH=riscv64 -C xen build > > > > > > RISC-V Xen can be run as: > > > $ qemu-system-riscv64 -M virt -smp 1 -nographic -m 2g \ > > > -kernel xen/xen > > > > > > To run in debug mode should be done the following instructions: > > > $ qemu-system-riscv64 -M virt -smp 1 -nographic -m 2g \ > > > -kernel xen/xen -s -S > > > # In separate terminal: > > > $ riscv64-buildroot-linux-gnu-gdb > > > $ target remote :1234 > > > $ add-symbol-file <xen_src>/xen/xen-syms 0x80200000 > > > $ hb *0x80200000 > > > $ c # it should stop at instruction j 0x80200000 <start> > > > > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > > --- > > > xen/arch/riscv/Makefile | 30 +++++++++++++ > > > xen/arch/riscv/arch.mk | 10 +++++ > > > xen/arch/riscv/include/asm/config.h | 26 ++++++++++- > > > xen/arch/riscv/include/asm/types.h | 11 +++++ > > > xen/arch/riscv/riscv64/Makefile | 2 +- > > > xen/arch/riscv/riscv64/head.S | 2 +- > > > xen/arch/riscv/xen.lds.S | 69 > > > +++++++++++++++++++++++++++++ > > > 7 files changed, 147 insertions(+), 3 deletions(-) > > > create mode 100644 xen/arch/riscv/include/asm/types.h > > > create mode 100644 xen/arch/riscv/xen.lds.S > > > > > > diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile > > > index 942e4ffbc1..893dd19ea6 100644 > > > --- a/xen/arch/riscv/Makefile > > > +++ b/xen/arch/riscv/Makefile > > > @@ -1,2 +1,32 @@ > > > +obj-$(CONFIG_RISCV_64) += riscv64/ > > > + > > > +$(TARGET): $(TARGET)-syms > > > + $(OBJCOPY) -O binary -S $< $@ > > > + > > > +$(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds > > > + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \ > > > + $(SYMBOLS_DUMMY_OBJ) -o $(@D)/.$(@F).0 > > > + $(NM) -pa --format=sysv $(@D)/.$(@F).0 \ > > > + | $(objtree)/tools/symbols $(all_symbols) --sysv -- > > > sort >$(@D)/.$(@F).0.S > > > + $(MAKE) $(build)=$(@D) $(@D)/.$(@F).0.o > > > + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \ > > > + $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1 > > > + $(NM) -pa --format=sysv $(@D)/.$(@F).1 \ > > > + | $(objtree)/tools/symbols $(all_symbols) --sysv -- > > > sort >$(@D)/.$(@F).1.S > > > + $(MAKE) $(build)=$(@D) $(@D)/.$(@F).1.o > > > + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< > > > $(build_id_linker) \ > > > + $(@D)/.$(@F).1.o -o $@ > > > + $(NM) -pa --format=sysv $(@D)/$(@F) \ > > > + | $(objtree)/tools/symbols --all-symbols --xensyms > > > --sysv --sort \ > > > + >$(@D)/$(@F).map > > > + rm -f $(@D)/.$(@F).[0-9]* > > > + > > > +$(obj)/xen.lds: $(src)/xen.lds.S FORCE > > > + $(call if_changed_dep,cpp_lds_S) > > > + > > > +.PHONY: clean > > > +clean:: > > > + rm -f $(objtree)/.xen-syms.[0-9]* > > > > Any reason to not use the variable clean-files as this is done for > > the > > other architectures? > > There is no reason not use the variable clean-files. I missed the > vairable clean-files so the patch will be updated to use the > variable clean-files instead of the variable clean. > > > > > > + > > > .PHONY: include > > > include: > > > diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk > > > index ae8fe9dec7..9292b72718 100644 > > > --- a/xen/arch/riscv/arch.mk > > > +++ b/xen/arch/riscv/arch.mk > > > @@ -11,3 +11,13 @@ riscv-march-$(CONFIG_RISCV_ISA_C) := > > > $(riscv-march-y)c > > > # -mcmodel=medlow would force Xen into the lower half. > > > > > > CFLAGS += -march=$(riscv-march-y) -mstrict-align -mcmodel=medany > > > + > > > +# TODO: Drop override and SYMBOLS_DUMMY_OBJ when more > > > +# of the build is working > > > +override ALL_OBJS-y = arch/$(TARGET_ARCH)/built_in.o > > > +override ALL_LIBS-y = > > > +ifneq ($(wildcard $(objtree)/common/symbols-dummy.o),) > > > +SYMBOLS_DUMMY_OBJ=$(objtree)/common/symbols-dummy.o > > > +else > > > +SYMBOLS_DUMMY_OBJ= > > > +endif > > > > Why do you need the ifneq here? > > The only reason for the ifneq here is to skip common > stuff from the build of minimal RISC-V Xen binary as it > requires pushing from the start a big amount of headers and function > stubs which at least will lead to a huge patch and complicate a code > review. > > It is possible to remove <common/symbols-dummy.o> from xen-syms > target in <xen/arch/riscv/Makefile> but an intention here was > to remain processing of xen-syms target similar to the original > one and it is the second reason why the ifneq was introduced and > added the comment "TODO: Drop ... SYMBOLS_DUMMY_OBJ when more > of the build is working". > > > > > > diff --git a/xen/arch/riscv/include/asm/config.h > > > b/xen/arch/riscv/include/asm/config.h > > > index e2ae21de61..756607a4a2 100644 > > > --- a/xen/arch/riscv/include/asm/config.h > > > +++ b/xen/arch/riscv/include/asm/config.h > > > @@ -28,7 +28,7 @@ > > > > > > /* Linkage for RISCV */ > > > #ifdef __ASSEMBLY__ > > > -#define ALIGN .align 2 > > > +#define ALIGN .align 4 > > > > Can you explain in the commit message why you need to change this? > > But > > ideally, this change should be part of a separate one. > > ALIGN was changed to equal the implementation-enforced instruction > address alignment (4-bytes), in order to ensure that entry points are > properly aligned. > If to be honest I haven't verified that and took these changes from > the initial patch series pushed by Bobby Eshleman. > > > > > > > > > #define ENTRY(name) \ > > > .globl name; \ > > > @@ -36,6 +36,30 @@ > > > name: > > > #endif > > > > > > +/* > > > + * Definition of XEN_VIRT_START should look like: > > > + * define XEN_VIRT_START _AT(vaddr_t,0x00200000) > > > + * It requires including of additional headers which > > > + * will increase an amount of files unnecessary for > > > + * minimal RISC-V Xen build so set value of > > > + * XEN_VIRT_START explicitly. > > > + * > > > + * TODO: change it to _AT(vaddr_t,0x00200000) when > > > + * necessary header will be pushed. > > > > The address here doesn't match the one below. I know this is an > > example > > but this is fairly confusing. > > This was done only as an example. > > > > > > + */ > > > +#define XEN_VIRT_START 0x80200000 > > > > I think you at least want to use _AT(unsigned long, ...) here to make > > clear this value should be interpreted as an unsigned value. > > > > You could even define vaddr_t now as you introduce a dummy types.h > > below. > > It makes sense to push the definition of vaddr_t and use <xen/const.h> > to be able to use _AT() macros. > > Probably it would be nice to introduce others from types.h from the > start, wouldn't it? Or would it be better to leave the patch minimal > and introduce only types necessary for vaddr_t? It would be best to add types.h early. I don't really see a reason not to > > > > > > +/* > > > + * PAGE_SIZE is defined in <{asm,xen}/page-*.h> but to > > > + * remove unnecessary headers for minimal > > > + * build headers it will be better to set PAGE_SIZE > > > + * explicitly. > > > > TBH, I think this is a shortcut that is unnecessary and will only > > introduce extra churn in the future (for instance, you will need to > > modify the include in xen.lds.S). > > > > But I am not the maintainer, so I will leave that decision to them > > whether this is acceptable. > > I didn't get what you mean by a shortcut here. > > The idea was to introduce PAGE_SIZE in the config.h directly for now > to keep build of RISC-V Xen minimal as much as possible otherwise > it would be required to push dummy <asm/page-bits.h> to get only one > needed definition of PAGE_SIZE to have buildable Xen. > Thereby it was decided to define PAGE_SIZE directly in <asm/config.h> > and remove it after all definitions from <{asm,xen}/page-*.h> will be > needed. > > > > > > + * > > > + * TODO: remove it when <asm/page-*.h> will be needed > > > + * defintion of PAGE_SIZE should be remove from > > > > s/defintion/definition/ > > Thanks for finding the typo. Will update it in the next version of > the patch. > > > > > > + * this header. > > > + */ > > > +#define PAGE_SIZE 4096 > + > > > #endif /* __RISCV_CONFIG_H__ */ > > > /* > > > * Local variables: > > > diff --git a/xen/arch/riscv/include/asm/types.h > > > b/xen/arch/riscv/include/asm/types.h > > > new file mode 100644 > > > index 0000000000..afbca6b15c > > > --- /dev/null > > > +++ b/xen/arch/riscv/include/asm/types.h > > > @@ -0,0 +1,11 @@ > > > +#ifndef __TYPES_H__ > > > +#define __TYPES_H__ > > > + > > > +/* > > > + * > > > + * asm/types.h is required for xen-syms.S file which > > > + * is produced by tools/symbols. > > > + * > > > + */ > > > + > > > +#endif > > > diff --git a/xen/arch/riscv/riscv64/Makefile > > > b/xen/arch/riscv/riscv64/Makefile > > > index 15a4a65f66..3340058c08 100644 > > > --- a/xen/arch/riscv/riscv64/Makefile > > > +++ b/xen/arch/riscv/riscv64/Makefile > > > @@ -1 +1 @@ > > > -extra-y += head.o > > > +obj-y += head.o > > > > This changes want to be explained. So does... > > RISC-V 64 would be supported now and minimal build is built > now only obj-y stuff. > > > > > > diff --git a/xen/arch/riscv/riscv64/head.S > > > b/xen/arch/riscv/riscv64/head.S > > > index 0dbc27ba75..0330b29c01 100644 > > > --- a/xen/arch/riscv/riscv64/head.S > > > +++ b/xen/arch/riscv/riscv64/head.S > > > @@ -1,6 +1,6 @@ > > > #include <asm/config.h> > > > > > > - .text > > > + .section .text.header, "ax", %progbits > > > > ... AFAICT this is to match the recent change in the build system. > > I am not sure that I get you here but it was done to make 'start' > instructions to be the first instructions that will be executed and > to make 'start' symbol to be the first in xen-syms.map file otherwise > gdb shows that Xen starts from the wrong instructions, fails to find > correct source file and goes crazy. > > > > > > > > > ENTRY(start) > > > j start > > > diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S > > > new file mode 100644 > > > index 0000000000..60628b3856 > > > --- /dev/null > > > +++ b/xen/arch/riscv/xen.lds.S > > > @@ -0,0 +1,69 @@ > > > +#include <xen/xen.lds.h> > > > + > > > +#undef ENTRY > > > +#undef ALIGN > > > + > > > +OUTPUT_ARCH(riscv) > > > +ENTRY(start) > > > + > > > +PHDRS > > > +{ > > > + text PT_LOAD ; > > > +#if defined(BUILD_ID) > > > + note PT_NOTE ; > > > +#endif > > > +} > > > + > > > +SECTIONS > > > +{ > > > + . = XEN_VIRT_START; > > > + _start = .; > > > + .text : { > > > + _stext = .; > > > + *(.text.header) > > > + *(.text) > > > > You are missing some sections here such as .text.cold, > > .text.unlikely... > > > > I understand they are probably not used yet. But it will avoid any > > nasty > > surprise if they are forgotten. > > They were skipped because they aren't use for now. Will add it in > the next version of the patch. > > > > > > + *(.gnu.warning) > > > + . = ALIGN(POINTER_ALIGN); > > > + _etext = .; > > > + } :text > > > + > > > + . = ALIGN(PAGE_SIZE); > > > + .rodata : { > > > + _srodata = .; > > > > You introduce _srodata but I can't find the matching _erodata. > > My fault. Thanks. Will add it in the the next version of the patch. > > > > > > + *(.rodata) > > > + *(.rodata.*) > > > + *(.data.rel.ro) > > > + *(.data.rel.ro.*) > > > + } :text > > > + > > > +#if defined(BUILD_ID) > > > + . = ALIGN(4); > > > + .note.gnu.build-id : { > > > + __note_gnu_build_id_start = .; > > > + *(.note.gnu.build-id) > > > + __note_gnu_build_id_end = .; > > > + } :note :text > > > +#endif > > > + > > > + . = ALIGN(PAGE_SIZE); > > > + .data : { /* Data */ > > > + *(.data .data.*) > > > > It would be better if you introduce .data.read_mostly right now > > separately. > > > > You also want *.data.page_aligned in .data. > > > > Lastly you are missing CONSTRUCTORS > > I will add offred sections and CONSTUCTORS in the next version of > the patch > > > > > > + } :text > > > + > > > > I am assuming you are going to add .init.* afterwards? > > > > > + . = ALIGN(PAGE_SIZE); > > > + .bss : { > > > + __bss_start = .; > > > + *(.bss .bss.*) > > > + . = ALIGN(POINTER_ALIGN); > > > + __bss_end = .; > > > > Same as .data, I would recommend to properly populate it. > > Do you mean to add .bss.stack_aligned, .bss.page_aligned, .bss.percpu*? > One of the reasons they were skipped is they aren't used now and one > more reason if to believe xen.lds.S file from ARM > .bss.percpu.read_mostly should be aligned by SMP_CACHE_BYTES which > requires dummy <asm/cache.h> (or not ?) which will increase the patch > with unneeded stuff for now. I think we should aim to get the linker file sorted right from the start. Otherwise someone is going to hit a nasty bug at some point in the future. Alistair > > > > > Cheers, > > > > Best regards, > Oleksii >
Alistair, Thanks for your comments! On Wed, 2022-12-28 at 14:51 +1000, Alistair Francis wrote: > On Mon, Dec 26, 2022 at 9:14 PM Oleksii <oleksii.kurochko@gmail.com> > wrote: > > > > Hi Julien, > > > > Thanks for your comments. > > > > On Fri, 2022-12-23 at 13:50 +0000, Julien Grall wrote: > > > Hi Oleksii, > > > > > > + Anthony for the Makefile changes. > > > > > > On 23/12/2022 11:16, Oleksii Kurochko wrote: > > > > The patch provides a minimal amount of changes to start > > > > build and run minimal Xen binary at GitLab CI&CD that will > > > > allow continuous checking of the build status of RISC-V Xen. > > > > > > > > RISC-V Xen can be built by the following instructions: > > > > $ CONTAINER=riscv64 ./automation/scripts/containerize \ > > > > make XEN_TARGET_ARCH=riscv64 tiny64_defconfig > > > > $ CONTAINER=riscv64 ./automation/scripts/containerize \ > > > > make XEN_TARGET_ARCH=riscv64 -C xen build > > > > > > > > RISC-V Xen can be run as: > > > > $ qemu-system-riscv64 -M virt -smp 1 -nographic -m 2g \ > > > > -kernel xen/xen > > > > > > > > To run in debug mode should be done the following instructions: > > > > $ qemu-system-riscv64 -M virt -smp 1 -nographic -m 2g \ > > > > -kernel xen/xen -s -S > > > > # In separate terminal: > > > > $ riscv64-buildroot-linux-gnu-gdb > > > > $ target remote :1234 > > > > $ add-symbol-file <xen_src>/xen/xen-syms 0x80200000 > > > > $ hb *0x80200000 > > > > $ c # it should stop at instruction j 0x80200000 <start> > > > > > > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > > > --- > > > > xen/arch/riscv/Makefile | 30 +++++++++++++ > > > > xen/arch/riscv/arch.mk | 10 +++++ > > > > xen/arch/riscv/include/asm/config.h | 26 ++++++++++- > > > > xen/arch/riscv/include/asm/types.h | 11 +++++ > > > > xen/arch/riscv/riscv64/Makefile | 2 +- > > > > xen/arch/riscv/riscv64/head.S | 2 +- > > > > xen/arch/riscv/xen.lds.S | 69 > > > > +++++++++++++++++++++++++++++ > > > > 7 files changed, 147 insertions(+), 3 deletions(-) > > > > create mode 100644 xen/arch/riscv/include/asm/types.h > > > > create mode 100644 xen/arch/riscv/xen.lds.S > > > > > > > > diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile > > > > index 942e4ffbc1..893dd19ea6 100644 > > > > --- a/xen/arch/riscv/Makefile > > > > +++ b/xen/arch/riscv/Makefile > > > > @@ -1,2 +1,32 @@ > > > > +obj-$(CONFIG_RISCV_64) += riscv64/ > > > > + > > > > +$(TARGET): $(TARGET)-syms > > > > + $(OBJCOPY) -O binary -S $< $@ > > > > + > > > > +$(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds > > > > + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \ > > > > + $(SYMBOLS_DUMMY_OBJ) -o $(@D)/.$(@F).0 > > > > + $(NM) -pa --format=sysv $(@D)/.$(@F).0 \ > > > > + | $(objtree)/tools/symbols $(all_symbols) -- > > > > sysv -- > > > > sort >$(@D)/.$(@F).0.S > > > > + $(MAKE) $(build)=$(@D) $(@D)/.$(@F).0.o > > > > + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \ > > > > + $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1 > > > > + $(NM) -pa --format=sysv $(@D)/.$(@F).1 \ > > > > + | $(objtree)/tools/symbols $(all_symbols) -- > > > > sysv -- > > > > sort >$(@D)/.$(@F).1.S > > > > + $(MAKE) $(build)=$(@D) $(@D)/.$(@F).1.o > > > > + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< > > > > $(build_id_linker) \ > > > > + $(@D)/.$(@F).1.o -o $@ > > > > + $(NM) -pa --format=sysv $(@D)/$(@F) \ > > > > + | $(objtree)/tools/symbols --all-symbols -- > > > > xensyms > > > > --sysv --sort \ > > > > + >$(@D)/$(@F).map > > > > + rm -f $(@D)/.$(@F).[0-9]* > > > > + > > > > +$(obj)/xen.lds: $(src)/xen.lds.S FORCE > > > > + $(call if_changed_dep,cpp_lds_S) > > > > + > > > > +.PHONY: clean > > > > +clean:: > > > > + rm -f $(objtree)/.xen-syms.[0-9]* > > > > > > Any reason to not use the variable clean-files as this is done > > > for > > > the > > > other architectures? > > > > There is no reason not use the variable clean-files. I missed the > > vairable clean-files so the patch will be updated to use the > > variable clean-files instead of the variable clean. > > > > > > > > > + > > > > .PHONY: include > > > > include: > > > > diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk > > > > index ae8fe9dec7..9292b72718 100644 > > > > --- a/xen/arch/riscv/arch.mk > > > > +++ b/xen/arch/riscv/arch.mk > > > > @@ -11,3 +11,13 @@ riscv-march-$(CONFIG_RISCV_ISA_C) := > > > > $(riscv-march-y)c > > > > # -mcmodel=medlow would force Xen into the lower half. > > > > > > > > CFLAGS += -march=$(riscv-march-y) -mstrict-align - > > > > mcmodel=medany > > > > + > > > > +# TODO: Drop override and SYMBOLS_DUMMY_OBJ when more > > > > +# of the build is working > > > > +override ALL_OBJS-y = arch/$(TARGET_ARCH)/built_in.o > > > > +override ALL_LIBS-y = > > > > +ifneq ($(wildcard $(objtree)/common/symbols-dummy.o),) > > > > +SYMBOLS_DUMMY_OBJ=$(objtree)/common/symbols-dummy.o > > > > +else > > > > +SYMBOLS_DUMMY_OBJ= > > > > +endif > > > > > > Why do you need the ifneq here? > > > > The only reason for the ifneq here is to skip common > > stuff from the build of minimal RISC-V Xen binary as it > > requires pushing from the start a big amount of headers and > > function > > stubs which at least will lead to a huge patch and complicate a > > code > > review. > > > > It is possible to remove <common/symbols-dummy.o> from xen-syms > > target in <xen/arch/riscv/Makefile> but an intention here was > > to remain processing of xen-syms target similar to the original > > one and it is the second reason why the ifneq was introduced and > > added the comment "TODO: Drop ... SYMBOLS_DUMMY_OBJ when more > > of the build is working". > > > > > > > > > diff --git a/xen/arch/riscv/include/asm/config.h > > > > b/xen/arch/riscv/include/asm/config.h > > > > index e2ae21de61..756607a4a2 100644 > > > > --- a/xen/arch/riscv/include/asm/config.h > > > > +++ b/xen/arch/riscv/include/asm/config.h > > > > @@ -28,7 +28,7 @@ > > > > > > > > /* Linkage for RISCV */ > > > > #ifdef __ASSEMBLY__ > > > > -#define ALIGN .align 2 > > > > +#define ALIGN .align 4 > > > > > > Can you explain in the commit message why you need to change > > > this? > > > But > > > ideally, this change should be part of a separate one. > > > > ALIGN was changed to equal the implementation-enforced instruction > > address alignment (4-bytes), in order to ensure that entry points > > are > > properly aligned. > > If to be honest I haven't verified that and took these changes from > > the initial patch series pushed by Bobby Eshleman. > > > > > > > > > > > > > #define ENTRY(name) \ > > > > .globl name; \ > > > > @@ -36,6 +36,30 @@ > > > > name: > > > > #endif > > > > > > > > +/* > > > > + * Definition of XEN_VIRT_START should look like: > > > > + * define XEN_VIRT_START _AT(vaddr_t,0x00200000) > > > > + * It requires including of additional headers which > > > > + * will increase an amount of files unnecessary for > > > > + * minimal RISC-V Xen build so set value of > > > > + * XEN_VIRT_START explicitly. > > > > + * > > > > + * TODO: change it to _AT(vaddr_t,0x00200000) when > > > > + * necessary header will be pushed. > > > > > > The address here doesn't match the one below. I know this is an > > > example > > > but this is fairly confusing. > > > > This was done only as an example. > > > > > > > > > + */ > > > > +#define XEN_VIRT_START 0x80200000 > > > > > > I think you at least want to use _AT(unsigned long, ...) here to > > > make > > > clear this value should be interpreted as an unsigned value. > > > > > > You could even define vaddr_t now as you introduce a dummy > > > types.h > > > below. > > > > It makes sense to push the definition of vaddr_t and use > > <xen/const.h> > > to be able to use _AT() macros. > > > > Probably it would be nice to introduce others from types.h from the > > start, wouldn't it? Or would it be better to leave the patch > > minimal > > and introduce only types necessary for vaddr_t? > > It would be best to add types.h early. I don't really see a reason > not to > > > > > > > > > > +/* > > > > + * PAGE_SIZE is defined in <{asm,xen}/page-*.h> but to > > > > + * remove unnecessary headers for minimal > > > > + * build headers it will be better to set PAGE_SIZE > > > > + * explicitly. > > > > > > TBH, I think this is a shortcut that is unnecessary and will only > > > introduce extra churn in the future (for instance, you will need > > > to > > > modify the include in xen.lds.S). > > > > > > But I am not the maintainer, so I will leave that decision to > > > them > > > whether this is acceptable. > > > > I didn't get what you mean by a shortcut here. > > > > The idea was to introduce PAGE_SIZE in the config.h directly for > > now > > to keep build of RISC-V Xen minimal as much as possible otherwise > > it would be required to push dummy <asm/page-bits.h> to get only > > one > > needed definition of PAGE_SIZE to have buildable Xen. > > Thereby it was decided to define PAGE_SIZE directly in > > <asm/config.h> > > and remove it after all definitions from <{asm,xen}/page-*.h> will > > be > > needed. > > > > > > > > > + * > > > > + * TODO: remove it when <asm/page-*.h> will be needed > > > > + * defintion of PAGE_SIZE should be remove from > > > > > > s/defintion/definition/ > > > > Thanks for finding the typo. Will update it in the next version of > > the patch. > > > > > > > > > + * this header. > > > > + */ > > > > +#define PAGE_SIZE 4096 > + > > > > #endif /* __RISCV_CONFIG_H__ */ > > > > /* > > > > * Local variables: > > > > diff --git a/xen/arch/riscv/include/asm/types.h > > > > b/xen/arch/riscv/include/asm/types.h > > > > new file mode 100644 > > > > index 0000000000..afbca6b15c > > > > --- /dev/null > > > > +++ b/xen/arch/riscv/include/asm/types.h > > > > @@ -0,0 +1,11 @@ > > > > +#ifndef __TYPES_H__ > > > > +#define __TYPES_H__ > > > > + > > > > +/* > > > > + * > > > > + * asm/types.h is required for xen-syms.S file which > > > > + * is produced by tools/symbols. > > > > + * > > > > + */ > > > > + > > > > +#endif > > > > diff --git a/xen/arch/riscv/riscv64/Makefile > > > > b/xen/arch/riscv/riscv64/Makefile > > > > index 15a4a65f66..3340058c08 100644 > > > > --- a/xen/arch/riscv/riscv64/Makefile > > > > +++ b/xen/arch/riscv/riscv64/Makefile > > > > @@ -1 +1 @@ > > > > -extra-y += head.o > > > > +obj-y += head.o > > > > > > This changes want to be explained. So does... > > > > RISC-V 64 would be supported now and minimal build is built > > now only obj-y stuff. > > > > > > > > > diff --git a/xen/arch/riscv/riscv64/head.S > > > > b/xen/arch/riscv/riscv64/head.S > > > > index 0dbc27ba75..0330b29c01 100644 > > > > --- a/xen/arch/riscv/riscv64/head.S > > > > +++ b/xen/arch/riscv/riscv64/head.S > > > > @@ -1,6 +1,6 @@ > > > > #include <asm/config.h> > > > > > > > > - .text > > > > + .section .text.header, "ax", %progbits > > > > > > ... AFAICT this is to match the recent change in the build > > > system. > > > > I am not sure that I get you here but it was done to make 'start' > > instructions to be the first instructions that will be executed and > > to make 'start' symbol to be the first in xen-syms.map file > > otherwise > > gdb shows that Xen starts from the wrong instructions, fails to > > find > > correct source file and goes crazy. > > > > > > > > > > > > > ENTRY(start) > > > > j start > > > > diff --git a/xen/arch/riscv/xen.lds.S > > > > b/xen/arch/riscv/xen.lds.S > > > > new file mode 100644 > > > > index 0000000000..60628b3856 > > > > --- /dev/null > > > > +++ b/xen/arch/riscv/xen.lds.S > > > > @@ -0,0 +1,69 @@ > > > > +#include <xen/xen.lds.h> > > > > + > > > > +#undef ENTRY > > > > +#undef ALIGN > > > > + > > > > +OUTPUT_ARCH(riscv) > > > > +ENTRY(start) > > > > + > > > > +PHDRS > > > > +{ > > > > + text PT_LOAD ; > > > > +#if defined(BUILD_ID) > > > > + note PT_NOTE ; > > > > +#endif > > > > +} > > > > + > > > > +SECTIONS > > > > +{ > > > > + . = XEN_VIRT_START; > > > > + _start = .; > > > > + .text : { > > > > + _stext = .; > > > > + *(.text.header) > > > > + *(.text) > > > > > > You are missing some sections here such as .text.cold, > > > .text.unlikely... > > > > > > I understand they are probably not used yet. But it will avoid > > > any > > > nasty > > > surprise if they are forgotten. > > > > They were skipped because they aren't use for now. Will add it in > > the next version of the patch. > > > > > > > > > + *(.gnu.warning) > > > > + . = ALIGN(POINTER_ALIGN); > > > > + _etext = .; > > > > + } :text > > > > + > > > > + . = ALIGN(PAGE_SIZE); > > > > + .rodata : { > > > > + _srodata = .; > > > > > > You introduce _srodata but I can't find the matching _erodata. > > > > My fault. Thanks. Will add it in the the next version of the patch. > > > > > > > > > + *(.rodata) > > > > + *(.rodata.*) > > > > + *(.data.rel.ro) > > > > + *(.data.rel.ro.*) > > > > + } :text > > > > + > > > > +#if defined(BUILD_ID) > > > > + . = ALIGN(4); > > > > + .note.gnu.build-id : { > > > > + __note_gnu_build_id_start = .; > > > > + *(.note.gnu.build-id) > > > > + __note_gnu_build_id_end = .; > > > > + } :note :text > > > > +#endif > > > > + > > > > + . = ALIGN(PAGE_SIZE); > > > > + .data : { /* Data */ > > > > + *(.data .data.*) > > > > > > It would be better if you introduce .data.read_mostly right now > > > separately. > > > > > > You also want *.data.page_aligned in .data. > > > > > > Lastly you are missing CONSTRUCTORS > > > > I will add offred sections and CONSTUCTORS in the next version of > > the patch > > > > > > > > > + } :text > > > > + > > > > > > I am assuming you are going to add .init.* afterwards? > > > > > > > + . = ALIGN(PAGE_SIZE); > > > > + .bss : { > > > > + __bss_start = .; > > > > + *(.bss .bss.*) > > > > + . = ALIGN(POINTER_ALIGN); > > > > + __bss_end = .; > > > > > > Same as .data, I would recommend to properly populate it. > > > > Do you mean to add .bss.stack_aligned, .bss.page_aligned, > > .bss.percpu*? > > One of the reasons they were skipped is they aren't used now and > > one > > more reason if to believe xen.lds.S file from ARM > > .bss.percpu.read_mostly should be aligned by SMP_CACHE_BYTES which > > requires dummy <asm/cache.h> (or not ?) which will increase the > > patch > > with unneeded stuff for now. > > I think we should aim to get the linker file sorted right from the > start. Otherwise someone is going to hit a nasty bug at some point in > the future. Probably I am not correct here but if I understand correcly the sections that are mentioned in riscv/xen.lds.S now they are "default" sections. All other sections such as *(.bss.percpu.*) *(.bss.*algined) etc are sections defined by user using __section directive or .section. Thereby it will be hard to get a nasty bug because if a section is needed and isn't defined then linker will produce an error about unkown section. Am i wrong? Best regards, Oleksii
On 23/12/2022 11:16 am, Oleksii Kurochko wrote: > diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile > index 942e4ffbc1..893dd19ea6 100644 > --- a/xen/arch/riscv/Makefile > +++ b/xen/arch/riscv/Makefile > @@ -1,2 +1,32 @@ > +obj-$(CONFIG_RISCV_64) += riscv64/ > + > +$(TARGET): $(TARGET)-syms > + $(OBJCOPY) -O binary -S $< $@ > + > +$(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds > + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \ > + $(SYMBOLS_DUMMY_OBJ) -o $(@D)/.$(@F).0 > + $(NM) -pa --format=sysv $(@D)/.$(@F).0 \ > + | $(objtree)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).0.S > + $(MAKE) $(build)=$(@D) $(@D)/.$(@F).0.o > + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \ > + $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1 > + $(NM) -pa --format=sysv $(@D)/.$(@F).1 \ > + | $(objtree)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).1.S > + $(MAKE) $(build)=$(@D) $(@D)/.$(@F).1.o > + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) \ > + $(@D)/.$(@F).1.o -o $@ The conditional emptying of SYMBOLS_DUMMY_OBJ in arch.mk will break this logic if it actually gets emptied, but you can drop almost all of it. This should be just $(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) -o $@ in the short term, I think. > + $(NM) -pa --format=sysv $(@D)/$(@F) \ > + | $(objtree)/tools/symbols --all-symbols --xensyms --sysv --sort \ > + >$(@D)/$(@F).map > + rm -f $(@D)/.$(@F).[0-9]* > + > +$(obj)/xen.lds: $(src)/xen.lds.S FORCE > + $(call if_changed_dep,cpp_lds_S) You've got a tab and some spaces here. It wants to be just one tab. > diff --git a/xen/arch/riscv/include/asm/config.h b/xen/arch/riscv/include/asm/config.h > index e2ae21de61..756607a4a2 100644 > --- a/xen/arch/riscv/include/asm/config.h > +++ b/xen/arch/riscv/include/asm/config.h > @@ -36,6 +36,30 @@ > name: > #endif > > +/* > + * Definition of XEN_VIRT_START should look like: > + * define XEN_VIRT_START _AT(vaddr_t,0x00200000) > + * It requires including of additional headers which > + * will increase an amount of files unnecessary for > + * minimal RISC-V Xen build so set value of > + * XEN_VIRT_START explicitly. > + * > + * TODO: change it to _AT(vaddr_t,0x00200000) when > + * necessary header will be pushed. > + */ > +#define XEN_VIRT_START 0x80200000 > +/* > + * PAGE_SIZE is defined in <{asm,xen}/page-*.h> but to > + * remove unnecessary headers for minimal > + * build headers it will be better to set PAGE_SIZE > + * explicitly. > + * > + * TODO: remove it when <asm/page-*.h> will be needed > + * defintion of PAGE_SIZE should be remove from > + * this header. > + */ > +#define PAGE_SIZE 4096 I've committed Alistair's patch which adds page-bits.h, so this section can go away. We still need XEN_VIRT_START, but we don't really need the commentary explaining that it's temporary - that is very clear from the subject of the patch. > diff --git a/xen/arch/riscv/include/asm/types.h b/xen/arch/riscv/include/asm/types.h > new file mode 100644 > index 0000000000..afbca6b15c > --- /dev/null > +++ b/xen/arch/riscv/include/asm/types.h > @@ -0,0 +1,11 @@ > +#ifndef __TYPES_H__ > +#define __TYPES_H__ > + > +/* > + * > + * asm/types.h is required for xen-syms.S file which > + * is produced by tools/symbols. > + * > + */ Again, no need for this comment. However, I think we want to rearrange the build system to have a final -I option for xen/include/stub/asm/ or so, so we can put some empty files there and avoid having architectures needing to provide empty files. If this file is needed, then it needs a more specific header guard than __TYPES_H__. That's the general guard for xen/types.h meaning that nothing inside this file will actually survive preprocessing. ~Andrew
On 28/12/2022 4:51 am, Alistair Francis wrote: > On Mon, Dec 26, 2022 at 9:14 PM Oleksii <oleksii.kurochko@gmail.com> wrote: >> On Fri, 2022-12-23 at 13:50 +0000, Julien Grall wrote: >>> On 23/12/2022 11:16, Oleksii Kurochko wrote: >>>> + . = ALIGN(PAGE_SIZE); >>>> + .bss : { >>>> + __bss_start = .; >>>> + *(.bss .bss.*) >>>> + . = ALIGN(POINTER_ALIGN); >>>> + __bss_end = .; >>> Same as .data, I would recommend to properly populate it. >> Do you mean to add .bss.stack_aligned, .bss.page_aligned, .bss.percpu*? >> One of the reasons they were skipped is they aren't used now and one >> more reason if to believe xen.lds.S file from ARM >> .bss.percpu.read_mostly should be aligned by SMP_CACHE_BYTES which >> requires dummy <asm/cache.h> (or not ?) which will increase the patch >> with unneeded stuff for now. > I think we should aim to get the linker file sorted right from the > start. Otherwise someone is going to hit a nasty bug at some point in > the future. What needs to happen is actually for Xen to start using a common linker script, rather than a per-arch linker script. The vast majority of the linker script is not architecture specific to begin with, and the rest is easy to parametrise. But in the short term, it's more important to get something working and properly into CI, rather than to block this change waiting for feature parity with a whole load of features not currently used. ~Andrew
On Mon, 26 Dec 2022 at 12:14, Oleksii <oleksii.kurochko@gmail.com> wrote: > > > > > +/* > > > + * PAGE_SIZE is defined in <{asm,xen}/page-*.h> but to > > > + * remove unnecessary headers for minimal > > > + * build headers it will be better to set PAGE_SIZE > > > + * explicitly. > > > > TBH, I think this is a shortcut that is unnecessary and will only > > introduce extra churn in the future (for instance, you will need to > > modify the include in xen.lds.S). > > > > But I am not the maintainer, so I will leave that decision to them > > whether this is acceptable. > > I didn't get what you mean by a shortcut here. config.h is automatically included everywhere. So anything defined in the header will also be available everywhere. Once you move the definition in a separate header, then you will have have to chase where the definition is used. An alternative would be to include the new header in config.h. However, this is not always wanted (we are trying to limit the scope of some definitions). So maybe you are saving a few minutes now. But you will spend a lot more to chase the places where the new header needs to be included. > > > > > + * > > > + * TODO: remove it when <asm/page-*.h> will be needed > > > + * defintion of PAGE_SIZE should be remove from > > > > s/defintion/definition/ > > Thanks for finding the typo. Will update it in the next version of > the patch. > > > > > > + * this header. > > > + */ > > > +#define PAGE_SIZE 4096 > + > > > #endif /* __RISCV_CONFIG_H__ */ > > > /* > > > * Local variables: > > > diff --git a/xen/arch/riscv/include/asm/types.h > > > b/xen/arch/riscv/include/asm/types.h > > > new file mode 100644 > > > index 0000000000..afbca6b15c > > > --- /dev/null > > > +++ b/xen/arch/riscv/include/asm/types.h > > > @@ -0,0 +1,11 @@ > > > +#ifndef __TYPES_H__ > > > +#define __TYPES_H__ > > > + > > > +/* > > > + * > > > + * asm/types.h is required for xen-syms.S file which > > > + * is produced by tools/symbols. > > > + * > > > + */ > > > + > > > +#endif > > > diff --git a/xen/arch/riscv/riscv64/Makefile > > > b/xen/arch/riscv/riscv64/Makefile > > > index 15a4a65f66..3340058c08 100644 > > > --- a/xen/arch/riscv/riscv64/Makefile > > > +++ b/xen/arch/riscv/riscv64/Makefile > > > @@ -1 +1 @@ > > > -extra-y += head.o > > > +obj-y += head.o > > > > This changes want to be explained. So does... > > RISC-V 64 would be supported now and minimal build is built > now only obj-y stuff. I am a bit confused. It thought what was checked in was meant to work. Did I misremembered? > > > > > > > diff --git a/xen/arch/riscv/riscv64/head.S > > > b/xen/arch/riscv/riscv64/head.S > > > index 0dbc27ba75..0330b29c01 100644 > > > --- a/xen/arch/riscv/riscv64/head.S > > > +++ b/xen/arch/riscv/riscv64/head.S > > > @@ -1,6 +1,6 @@ > > > #include <asm/config.h> > > > > > > - .text > > > + .section .text.header, "ax", %progbits > > > > ... AFAICT this is to match the recent change in the build system. > > I am not sure that I get you here but it was done to make 'start' > instructions to be the first instructions that will be executed and > to make 'start' symbol to be the first in xen-syms.map file otherwise > gdb shows that Xen starts from the wrong instructions, fails to find > correct source file and goes crazy. When the file head.S was originally created, there was no section .text.header. Instead head.S was included at the top with some different runes. > > > > > > + } :text > > > + > > > > I am assuming you are going to add .init.* afterwards? > > > > > + . = ALIGN(PAGE_SIZE); > > > + .bss : { > > > + __bss_start = .; > > > + *(.bss .bss.*) > > > + . = ALIGN(POINTER_ALIGN); > > > + __bss_end = .; > > > > Same as .data, I would recommend to properly populate it. > > Do you mean to add .bss.stack_aligned, .bss.page_aligned, .bss.percpu*? > One of the reasons they were skipped is they aren't used now and one > more reason if to believe xen.lds.S file from ARM > .bss.percpu.read_mostly should be aligned by SMP_CACHE_BYTES which > requires dummy <asm/cache.h> (or not ?) which will increase the patch > with unneeded stuff for now. I will answer your reply to Alistair here at the same time. The problem is .bss.* will include any section start with .bss.. IOW section like .bss.page_aligned will also be covered and therefore you will not get a linker failure. Instead , the linker will fold the section wherever it wants. Therefore, there is no guarantee, the content will be page aligned. When using the binary, you could end up with weird behavior that will be hard to debug. I understand you are trying to get a basic build. But, I feel the linker is not something you want to quickly rush. 1h may turn into hours lost in a few months because not everyone may remember that you didn’t special case .bss.page_aligned. Short of suggesting to have a common linker script, you could simply not use * (IOW explictly list the section). With that, you should get a linking warning/error if the section is not listed. Cheers,
On Wed, 2022-12-28 at 19:01 +0000, Andrew Cooper wrote: > On 28/12/2022 4:51 am, Alistair Francis wrote: > > On Mon, Dec 26, 2022 at 9:14 PM Oleksii > > <oleksii.kurochko@gmail.com> wrote: > > > On Fri, 2022-12-23 at 13:50 +0000, Julien Grall wrote: > > > > On 23/12/2022 11:16, Oleksii Kurochko wrote: > > > > > + . = ALIGN(PAGE_SIZE); > > > > > + .bss : { > > > > > + __bss_start = .; > > > > > + *(.bss .bss.*) > > > > > + . = ALIGN(POINTER_ALIGN); > > > > > + __bss_end = .; > > > > Same as .data, I would recommend to properly populate it. > > > Do you mean to add .bss.stack_aligned, .bss.page_aligned, > > > .bss.percpu*? > > > One of the reasons they were skipped is they aren't used now and > > > one > > > more reason if to believe xen.lds.S file from ARM > > > .bss.percpu.read_mostly should be aligned by SMP_CACHE_BYTES > > > which > > > requires dummy <asm/cache.h> (or not ?) which will increase the > > > patch > > > with unneeded stuff for now. > > I think we should aim to get the linker file sorted right from the > > start. Otherwise someone is going to hit a nasty bug at some point > > in > > the future. > > What needs to happen is actually for Xen to start using a common > linker > script, rather than a per-arch linker script. > Do you expect to see common linker script as a part of this patch series? > The vast majority of the linker script is not architecture specific > to > begin with, and the rest is easy to parametrise. > I reworked xen.lds.S file. I took xen.lds.S from ARM as a basis and remove all arch specific sections such as *(.proc.info), *(.ex_table), *(.ex_table.pre), *(.altinstructions), *(.teemediator.info), *(.adev.info), *(.dev.info), *(.arch.info), *(.bug_frames.*). So it would be possible to use xen.lds.S as a common linker script. > But in the short term, it's more important to get something working > and > properly into CI, rather than to block this change waiting for > feature > parity with a whole load of features not currently used. > > ~Andrew ~Oleksii
On Wed, 2022-12-28 at 22:22 +0000, Julien Grall wrote: > > > On Mon, 26 Dec 2022 at 12:14, Oleksii <oleksii.kurochko@gmail.com> > wrote: > > > > > > > +/* > > > > + * PAGE_SIZE is defined in <{asm,xen}/page-*.h> but to > > > > + * remove unnecessary headers for minimal > > > > + * build headers it will be better to set PAGE_SIZE > > > > + * explicitly. > > > > > > TBH, I think this is a shortcut that is unnecessary and will only > > > introduce extra churn in the future (for instance, you will need > > > to > > > modify the include in xen.lds.S). > > > > > > But I am not the maintainer, so I will leave that decision to > > > them > > > whether this is acceptable. > > > > I didn't get what you mean by a shortcut here. > > > > > config.h is automatically included everywhere. So anything defined in > the header will also be available everywhere. Once you move the > definition in a separate header, then you will have have to chase > where the definition is used. > > An alternative would be to include the new header in config.h. > However, this is not always wanted (we are trying to limit the scope > of some definitions). > > So maybe you are saving a few minutes now. But you will spend a lot > more to chase the places where the new header needs to be included. > Thanks for clarification. > > > > > > > > > + * > > > > + * TODO: remove it when <asm/page-*.h> will be needed > > > > + * defintion of PAGE_SIZE should be remove from > > > > > > s/defintion/definition/ > > > > Thanks for finding the typo. Will update it in the next version of > > the patch. > > > > > > > > > + * this header. > > > > + */ > > > > +#define PAGE_SIZE 4096 > + > > > > #endif /* __RISCV_CONFIG_H__ */ > > > > /* > > > > * Local variables: > > > > diff --git a/xen/arch/riscv/include/asm/types.h > > > > b/xen/arch/riscv/include/asm/types.h > > > > new file mode 100644 > > > > index 0000000000..afbca6b15c > > > > --- /dev/null > > > > +++ b/xen/arch/riscv/include/asm/types.h > > > > @@ -0,0 +1,11 @@ > > > > +#ifndef __TYPES_H__ > > > > +#define __TYPES_H__ > > > > + > > > > +/* > > > > + * > > > > + * asm/types.h is required for xen-syms.S file which > > > > + * is produced by tools/symbols. > > > > + * > > > > + */ > > > > + > > > > +#endif > > > > diff --git a/xen/arch/riscv/riscv64/Makefile > > > > b/xen/arch/riscv/riscv64/Makefile > > > > index 15a4a65f66..3340058c08 100644 > > > > --- a/xen/arch/riscv/riscv64/Makefile > > > > +++ b/xen/arch/riscv/riscv64/Makefile > > > > @@ -1 +1 @@ > > > > -extra-y += head.o > > > > +obj-y += head.o > > > > > > This changes want to be explained. So does... > > > > RISC-V 64 would be supported now and minimal build is built > > now only obj-y stuff. > > > > > I am a bit confused. It thought what was checked in was meant to > work. Did I misremembered? The current mainline Xen can only build head.o directly using the following command: make XEN_TARGET_ARCH=riscv64 CROSS_COMPILE=riscv64-linux-gnu- KBUILD_DEFCONFIG=tiny64_defconfig arch/riscv/riscv64/head.o Comments can be found in the commit: 2a04f396a34c5a43b9a09d72e8c4f > > > > > > > > > > > diff --git a/xen/arch/riscv/riscv64/head.S > > > > b/xen/arch/riscv/riscv64/head.S > > > > index 0dbc27ba75..0330b29c01 100644 > > > > --- a/xen/arch/riscv/riscv64/head.S > > > > +++ b/xen/arch/riscv/riscv64/head.S > > > > @@ -1,6 +1,6 @@ > > > > #include <asm/config.h> > > > > > > > > - .text > > > > + .section .text.header, "ax", %progbits > > > > > > ... AFAICT this is to match the recent change in the build > > > system. > > > > I am not sure that I get you here but it was done to make 'start' > > instructions to be the first instructions that will be executed and > > to make 'start' symbol to be the first in xen-syms.map file > > otherwise > > gdb shows that Xen starts from the wrong instructions, fails to > > find > > correct source file and goes crazy. > > > > > When the file head.S was originally created, there was no section > .text.header. Instead head.S was included at the top with some > different runes. > > > > > > > > > + } :text > > > > + > > > > > > I am assuming you are going to add .init.* afterwards? > > > > > > > + . = ALIGN(PAGE_SIZE); > > > > + .bss : { > > > > + __bss_start = .; > > > > + *(.bss .bss.*) > > > > + . = ALIGN(POINTER_ALIGN); > > > > + __bss_end = .; > > > > > > Same as .data, I would recommend to properly populate it. > > > > Do you mean to add .bss.stack_aligned, .bss.page_aligned, > > .bss.percpu*? > > One of the reasons they were skipped is they aren't used now and > > one > > more reason if to believe xen.lds.S file from ARM > > .bss.percpu.read_mostly should be aligned by SMP_CACHE_BYTES which > > requires dummy <asm/cache.h> (or not ?) which will increase the > > patch > > with unneeded stuff for now. > > > > > I will answer your reply to Alistair here at the same time. > > The problem is .bss.* will include any section start with .bss.. IOW > section like .bss.page_aligned will also be covered and therefore you > will not get a linker failure. > > Instead , the linker will fold the section wherever it wants. > Therefore, there is no guarantee, the content will be page aligned. > When using the binary, you could end up with weird behavior that will > be hard to debug. > > I understand you are trying to get a basic build. But, I feel the > linker is not something you want to quickly rush. 1h may turn into > hours lost in a few months because not everyone may remember that you > didn’t special case .bss.page_aligned. > > Short of suggesting to have a common linker script, you could simply > not use * (IOW explictly list the section). With that, you should > get a linking warning/error if the section is not listed. > Totally agree then. I missed that there is .bss.*. Actually I reworked a little bit xen.lds.S. As a basis I took xen.lds.S from ARM and removed all arch specific sections. So xen.lds.S contains stuff which isn't used for now (for example, *(.data.schedulers)) but will be used in future. Would it be better to go with the reworked linker script or remove unneeded sections for now and make it get a linking warning/error when sections will be used? > Cheers, BR, Oleksii
On Wed, 2022-12-28 at 18:56 +0000, Andrew Cooper wrote: > On 23/12/2022 11:16 am, Oleksii Kurochko wrote: > > diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile > > index 942e4ffbc1..893dd19ea6 100644 > > --- a/xen/arch/riscv/Makefile > > +++ b/xen/arch/riscv/Makefile > > @@ -1,2 +1,32 @@ > > +obj-$(CONFIG_RISCV_64) += riscv64/ > > + > > +$(TARGET): $(TARGET)-syms > > + $(OBJCOPY) -O binary -S $< $@ > > + > > +$(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds > > + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \ > > + $(SYMBOLS_DUMMY_OBJ) -o $(@D)/.$(@F).0 > > + $(NM) -pa --format=sysv $(@D)/.$(@F).0 \ > > + | $(objtree)/tools/symbols $(all_symbols) --sysv -- > > sort >$(@D)/.$(@F).0.S > > + $(MAKE) $(build)=$(@D) $(@D)/.$(@F).0.o > > + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \ > > + $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1 > > + $(NM) -pa --format=sysv $(@D)/.$(@F).1 \ > > + | $(objtree)/tools/symbols $(all_symbols) --sysv -- > > sort >$(@D)/.$(@F).1.S > > + $(MAKE) $(build)=$(@D) $(@D)/.$(@F).1.o > > + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< > > $(build_id_linker) \ > > + $(@D)/.$(@F).1.o -o $@ > > The conditional emptying of SYMBOLS_DUMMY_OBJ in arch.mk will break > this > logic if it actually gets emptied, but you can drop almost all of it. > > This should be just > > $(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds > $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) - > o $@ > > in the short term, I think. > Originally it was made in the same way but I decided to create addiotional variable SYMBOLS_DUMMY_OBJ. I will back the previous solution. > > + $(NM) -pa --format=sysv $(@D)/$(@F) \ > > + | $(objtree)/tools/symbols --all-symbols --xensyms > > --sysv --sort \ > > + >$(@D)/$(@F).map > > + rm -f $(@D)/.$(@F).[0-9]* > > + > > +$(obj)/xen.lds: $(src)/xen.lds.S FORCE > > + $(call if_changed_dep,cpp_lds_S) > > You've got a tab and some spaces here. It wants to be just one tab. > Thanks. Will re-check. > > diff --git a/xen/arch/riscv/include/asm/config.h > > b/xen/arch/riscv/include/asm/config.h > > index e2ae21de61..756607a4a2 100644 > > --- a/xen/arch/riscv/include/asm/config.h > > +++ b/xen/arch/riscv/include/asm/config.h > > @@ -36,6 +36,30 @@ > > name: > > #endif > > > > +/* > > + * Definition of XEN_VIRT_START should look like: > > + * define XEN_VIRT_START _AT(vaddr_t,0x00200000) > > + * It requires including of additional headers which > > + * will increase an amount of files unnecessary for > > + * minimal RISC-V Xen build so set value of > > + * XEN_VIRT_START explicitly. > > + * > > + * TODO: change it to _AT(vaddr_t,0x00200000) when > > + * necessary header will be pushed. > > + */ > > +#define XEN_VIRT_START 0x80200000 > > +/* > > + * PAGE_SIZE is defined in <{asm,xen}/page-*.h> but to > > + * remove unnecessary headers for minimal > > + * build headers it will be better to set PAGE_SIZE > > + * explicitly. > > + * > > + * TODO: remove it when <asm/page-*.h> will be needed > > + * defintion of PAGE_SIZE should be remove from > > + * this header. > > + */ > > +#define PAGE_SIZE 4096 > > I've committed Alistair's patch which adds page-bits.h, so this > section > can go away. > Nice. Thanks a lot. > > We still need XEN_VIRT_START, but we don't really need the commentary > explaining that it's temporary - that is very clear from the subject > of > the patch. > > > diff --git a/xen/arch/riscv/include/asm/types.h > > b/xen/arch/riscv/include/asm/types.h > > new file mode 100644 > > index 0000000000..afbca6b15c > > --- /dev/null > > +++ b/xen/arch/riscv/include/asm/types.h > > @@ -0,0 +1,11 @@ > > +#ifndef __TYPES_H__ > > +#define __TYPES_H__ > > + > > +/* > > + * > > + * asm/types.h is required for xen-syms.S file which > > + * is produced by tools/symbols. > > + * > > + */ > > Again, no need for this comment. > > However, I think we want to rearrange the build system to have a > final > -I option for xen/include/stub/asm/ or so, so we can put some empty > files there and avoid having architectures needing to provide empty > files. > Agree. And do you expect to see these changes as a part of this patch series or it someting that should be done in future? > If this file is needed, then it needs a more specific header guard > than > __TYPES_H__. That's the general guard for xen/types.h meaning that > nothing inside this file will actually survive preprocessing. > > ~Andrew
Hi, On 29/12/2022 08:45, Oleksii wrote: > Totally agree then. > I missed that there is .bss.*. > Actually I reworked a little bit xen.lds.S. As a basis I took xen.lds.S > from ARM and removed all arch specific sections. So xen.lds.S contains > stuff which isn't used for now (for example, *(.data.schedulers)) but > will be used in future. > Would it be better to go with the reworked linker script or remove > unneeded sections for now and make it get a linking warning/error when > sections will be used? I don't have a strong opinion either way. I would say what's the easiest for you. In the long term, we will want to have a common linker script. But that's a separate discussion and not a request for you to do it. Cheers,
diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile index 942e4ffbc1..893dd19ea6 100644 --- a/xen/arch/riscv/Makefile +++ b/xen/arch/riscv/Makefile @@ -1,2 +1,32 @@ +obj-$(CONFIG_RISCV_64) += riscv64/ + +$(TARGET): $(TARGET)-syms + $(OBJCOPY) -O binary -S $< $@ + +$(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \ + $(SYMBOLS_DUMMY_OBJ) -o $(@D)/.$(@F).0 + $(NM) -pa --format=sysv $(@D)/.$(@F).0 \ + | $(objtree)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).0.S + $(MAKE) $(build)=$(@D) $(@D)/.$(@F).0.o + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \ + $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1 + $(NM) -pa --format=sysv $(@D)/.$(@F).1 \ + | $(objtree)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).1.S + $(MAKE) $(build)=$(@D) $(@D)/.$(@F).1.o + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) \ + $(@D)/.$(@F).1.o -o $@ + $(NM) -pa --format=sysv $(@D)/$(@F) \ + | $(objtree)/tools/symbols --all-symbols --xensyms --sysv --sort \ + >$(@D)/$(@F).map + rm -f $(@D)/.$(@F).[0-9]* + +$(obj)/xen.lds: $(src)/xen.lds.S FORCE + $(call if_changed_dep,cpp_lds_S) + +.PHONY: clean +clean:: + rm -f $(objtree)/.xen-syms.[0-9]* + .PHONY: include include: diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk index ae8fe9dec7..9292b72718 100644 --- a/xen/arch/riscv/arch.mk +++ b/xen/arch/riscv/arch.mk @@ -11,3 +11,13 @@ riscv-march-$(CONFIG_RISCV_ISA_C) := $(riscv-march-y)c # -mcmodel=medlow would force Xen into the lower half. CFLAGS += -march=$(riscv-march-y) -mstrict-align -mcmodel=medany + +# TODO: Drop override and SYMBOLS_DUMMY_OBJ when more +# of the build is working +override ALL_OBJS-y = arch/$(TARGET_ARCH)/built_in.o +override ALL_LIBS-y = +ifneq ($(wildcard $(objtree)/common/symbols-dummy.o),) +SYMBOLS_DUMMY_OBJ=$(objtree)/common/symbols-dummy.o +else +SYMBOLS_DUMMY_OBJ= +endif diff --git a/xen/arch/riscv/include/asm/config.h b/xen/arch/riscv/include/asm/config.h index e2ae21de61..756607a4a2 100644 --- a/xen/arch/riscv/include/asm/config.h +++ b/xen/arch/riscv/include/asm/config.h @@ -28,7 +28,7 @@ /* Linkage for RISCV */ #ifdef __ASSEMBLY__ -#define ALIGN .align 2 +#define ALIGN .align 4 #define ENTRY(name) \ .globl name; \ @@ -36,6 +36,30 @@ name: #endif +/* + * Definition of XEN_VIRT_START should look like: + * define XEN_VIRT_START _AT(vaddr_t,0x00200000) + * It requires including of additional headers which + * will increase an amount of files unnecessary for + * minimal RISC-V Xen build so set value of + * XEN_VIRT_START explicitly. + * + * TODO: change it to _AT(vaddr_t,0x00200000) when + * necessary header will be pushed. + */ +#define XEN_VIRT_START 0x80200000 +/* + * PAGE_SIZE is defined in <{asm,xen}/page-*.h> but to + * remove unnecessary headers for minimal + * build headers it will be better to set PAGE_SIZE + * explicitly. + * + * TODO: remove it when <asm/page-*.h> will be needed + * defintion of PAGE_SIZE should be remove from + * this header. + */ +#define PAGE_SIZE 4096 + #endif /* __RISCV_CONFIG_H__ */ /* * Local variables: diff --git a/xen/arch/riscv/include/asm/types.h b/xen/arch/riscv/include/asm/types.h new file mode 100644 index 0000000000..afbca6b15c --- /dev/null +++ b/xen/arch/riscv/include/asm/types.h @@ -0,0 +1,11 @@ +#ifndef __TYPES_H__ +#define __TYPES_H__ + +/* + * + * asm/types.h is required for xen-syms.S file which + * is produced by tools/symbols. + * + */ + +#endif diff --git a/xen/arch/riscv/riscv64/Makefile b/xen/arch/riscv/riscv64/Makefile index 15a4a65f66..3340058c08 100644 --- a/xen/arch/riscv/riscv64/Makefile +++ b/xen/arch/riscv/riscv64/Makefile @@ -1 +1 @@ -extra-y += head.o +obj-y += head.o diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S index 0dbc27ba75..0330b29c01 100644 --- a/xen/arch/riscv/riscv64/head.S +++ b/xen/arch/riscv/riscv64/head.S @@ -1,6 +1,6 @@ #include <asm/config.h> - .text + .section .text.header, "ax", %progbits ENTRY(start) j start diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S new file mode 100644 index 0000000000..60628b3856 --- /dev/null +++ b/xen/arch/riscv/xen.lds.S @@ -0,0 +1,69 @@ +#include <xen/xen.lds.h> + +#undef ENTRY +#undef ALIGN + +OUTPUT_ARCH(riscv) +ENTRY(start) + +PHDRS +{ + text PT_LOAD ; +#if defined(BUILD_ID) + note PT_NOTE ; +#endif +} + +SECTIONS +{ + . = XEN_VIRT_START; + _start = .; + .text : { + _stext = .; + *(.text.header) + *(.text) + *(.gnu.warning) + . = ALIGN(POINTER_ALIGN); + _etext = .; + } :text + + . = ALIGN(PAGE_SIZE); + .rodata : { + _srodata = .; + *(.rodata) + *(.rodata.*) + *(.data.rel.ro) + *(.data.rel.ro.*) + } :text + +#if defined(BUILD_ID) + . = ALIGN(4); + .note.gnu.build-id : { + __note_gnu_build_id_start = .; + *(.note.gnu.build-id) + __note_gnu_build_id_end = .; + } :note :text +#endif + + . = ALIGN(PAGE_SIZE); + .data : { /* Data */ + *(.data .data.*) + } :text + + . = ALIGN(PAGE_SIZE); + .bss : { + __bss_start = .; + *(.bss .bss.*) + . = ALIGN(POINTER_ALIGN); + __bss_end = .; + } :text + _end = . ; + + DWARF2_DEBUG_SECTIONS + + DISCARD_SECTIONS + + STABS_DEBUG_SECTIONS + + ELF_DETAILS_SECTIONS +}
The patch provides a minimal amount of changes to start build and run minimal Xen binary at GitLab CI&CD that will allow continuous checking of the build status of RISC-V Xen. RISC-V Xen can be built by the following instructions: $ CONTAINER=riscv64 ./automation/scripts/containerize \ make XEN_TARGET_ARCH=riscv64 tiny64_defconfig $ CONTAINER=riscv64 ./automation/scripts/containerize \ make XEN_TARGET_ARCH=riscv64 -C xen build RISC-V Xen can be run as: $ qemu-system-riscv64 -M virt -smp 1 -nographic -m 2g \ -kernel xen/xen To run in debug mode should be done the following instructions: $ qemu-system-riscv64 -M virt -smp 1 -nographic -m 2g \ -kernel xen/xen -s -S # In separate terminal: $ riscv64-buildroot-linux-gnu-gdb $ target remote :1234 $ add-symbol-file <xen_src>/xen/xen-syms 0x80200000 $ hb *0x80200000 $ c # it should stop at instruction j 0x80200000 <start> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- xen/arch/riscv/Makefile | 30 +++++++++++++ xen/arch/riscv/arch.mk | 10 +++++ xen/arch/riscv/include/asm/config.h | 26 ++++++++++- xen/arch/riscv/include/asm/types.h | 11 +++++ xen/arch/riscv/riscv64/Makefile | 2 +- xen/arch/riscv/riscv64/head.S | 2 +- xen/arch/riscv/xen.lds.S | 69 +++++++++++++++++++++++++++++ 7 files changed, 147 insertions(+), 3 deletions(-) create mode 100644 xen/arch/riscv/include/asm/types.h create mode 100644 xen/arch/riscv/xen.lds.S