|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH v1 1/4] arch/riscv: initial RISC-V support to build/run minimal Xen
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@xxxxxxxxx>
> > ---
> > 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |