Message ID | 1cea913117f771a5f3b4404d7bfb7e1329f3f38e.1717008161.git.oleksii.kurochko@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | *Enable build of full Xen for RISC-V | expand |
On 29/05/2024 8:55 pm, Oleksii Kurochko wrote: > diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c > index 8285bcffef..bda35fc347 100644 > --- a/xen/arch/riscv/stubs.c > +++ b/xen/arch/riscv/stubs.c > @@ -24,12 +24,6 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_mask); > > nodemask_t __read_mostly node_online_map = { { [0] = 1UL } }; > > -/* > - * max_page is defined in page_alloc.c which isn't complied for now. > - * definition of max_page will be remove as soon as page_alloc is built. > - */ > -unsigned long __read_mostly max_page; > - > /* time.c */ > > unsigned long __ro_after_init cpu_khz; /* CPU clock frequency in kHz. */ > @@ -419,21 +413,3 @@ void __cpu_die(unsigned int cpu) > { > BUG_ON("unimplemented"); > } > - > -/* > - * The following functions are defined in common/irq.c, but common/irq.c isn't > - * built for now. These changes will be removed there when common/irq.c is > - * ready. > - */ > - > -void cf_check irq_actor_none(struct irq_desc *desc) > -{ > - BUG_ON("unimplemented"); > -} > - > -unsigned int cf_check irq_startup_none(struct irq_desc *desc) > -{ > - BUG_ON("unimplemented"); > - > - return 0; > -} All 3 of these are introduced in the previous patch and deleted again here. Looks like a rebasing accident. (This patch has to be the final one touching build related things, so I can't simply fix it on commit) ~Andrew
On Thu, 2024-05-30 at 17:48 +0100, Andrew Cooper wrote: > On 29/05/2024 8:55 pm, Oleksii Kurochko wrote: > > diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c > > index 8285bcffef..bda35fc347 100644 > > --- a/xen/arch/riscv/stubs.c > > +++ b/xen/arch/riscv/stubs.c > > @@ -24,12 +24,6 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, > > cpu_core_mask); > > > > nodemask_t __read_mostly node_online_map = { { [0] = 1UL } }; > > > > -/* > > - * max_page is defined in page_alloc.c which isn't complied for > > now. > > - * definition of max_page will be remove as soon as page_alloc is > > built. > > - */ > > -unsigned long __read_mostly max_page; > > - > > /* time.c */ > > > > unsigned long __ro_after_init cpu_khz; /* CPU clock frequency in > > kHz. */ > > @@ -419,21 +413,3 @@ void __cpu_die(unsigned int cpu) > > { > > BUG_ON("unimplemented"); > > } > > - > > -/* > > - * The following functions are defined in common/irq.c, but > > common/irq.c isn't > > - * built for now. These changes will be removed there when > > common/irq.c is > > - * ready. > > - */ > > - > > -void cf_check irq_actor_none(struct irq_desc *desc) > > -{ > > - BUG_ON("unimplemented"); > > -} > > - > > -unsigned int cf_check irq_startup_none(struct irq_desc *desc) > > -{ > > - BUG_ON("unimplemented"); > > - > > - return 0; > > -} > > All 3 of these are introduced in the previous patch and deleted again > here. Looks like a rebasing accident. Not really. This was done to avoid build failures for RISC-V. If the HEAD is on the previous patch where these changes are introduced and then we just drop them, it will lead to a linkage error because these functions are defined in common/irq.c, which isn't built for RISC-V if the HEAD is on the previous patch: /build/xen/arch/riscv/entry.S:86: undefined reference to `max_page' riscv64-linux-gnu-ld: prelink.o:(.rodata+0x8): undefined reference to `irq_startup_none' riscv64-linux-gnu-ld: prelink.o:(.rodata+0x10): undefined reference to `irq_actor_none' riscv64-linux-gnu-ld: prelink.o:(.rodata+0x18): undefined reference to `irq_actor_none' riscv64-linux-gnu-ld: prelink.o:(.rodata+0x20): undefined reference to `irq_actor_none' riscv64-linux-gnu-ld: xen-syms: hidden symbol `irq_actor_none' isn't defined That is why these stubs were introduced in the previous patch (because common/irq.c isn't built at that moment) and are removed in this patch (since at the moment of this patch, common/irq.c is now being built). ~ Oleksii > > (This patch has to be the final one touching build related things, so > I > can't simply fix it on commit) > > ~Andrew
On 30/05/2024 6:12 pm, Oleksii K. wrote: > On Thu, 2024-05-30 at 17:48 +0100, Andrew Cooper wrote: >> On 29/05/2024 8:55 pm, Oleksii Kurochko wrote: >>> diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c >>> index 8285bcffef..bda35fc347 100644 >>> --- a/xen/arch/riscv/stubs.c >>> +++ b/xen/arch/riscv/stubs.c >>> @@ -24,12 +24,6 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, >>> cpu_core_mask); >>> >>> nodemask_t __read_mostly node_online_map = { { [0] = 1UL } }; >>> >>> -/* >>> - * max_page is defined in page_alloc.c which isn't complied for >>> now. >>> - * definition of max_page will be remove as soon as page_alloc is >>> built. >>> - */ >>> -unsigned long __read_mostly max_page; >>> - >>> /* time.c */ >>> >>> unsigned long __ro_after_init cpu_khz; /* CPU clock frequency in >>> kHz. */ >>> @@ -419,21 +413,3 @@ void __cpu_die(unsigned int cpu) >>> { >>> BUG_ON("unimplemented"); >>> } >>> - >>> -/* >>> - * The following functions are defined in common/irq.c, but >>> common/irq.c isn't >>> - * built for now. These changes will be removed there when >>> common/irq.c is >>> - * ready. >>> - */ >>> - >>> -void cf_check irq_actor_none(struct irq_desc *desc) >>> -{ >>> - BUG_ON("unimplemented"); >>> -} >>> - >>> -unsigned int cf_check irq_startup_none(struct irq_desc *desc) >>> -{ >>> - BUG_ON("unimplemented"); >>> - >>> - return 0; >>> -} >> All 3 of these are introduced in the previous patch and deleted again >> here. Looks like a rebasing accident. > Not really. > > This was done to avoid build failures for RISC-V. If the HEAD is on the > previous patch where these changes are introduced and then we just drop > them, it will lead to a linkage error because these functions are > defined in common/irq.c, which isn't built for RISC-V if the HEAD is on > the previous patch: > /build/xen/arch/riscv/entry.S:86: undefined reference to `max_page' Nothing in the series touches entry.S, so I'm not sure what this is complaining about. The stub for get_upper_mfn_bound() references max_page, but it's only used in a SYSCTL so you can avoid the problem with a BUG_ON(). BTW, you also don't need a return after a BUG_ON(). __builtin_unreachable() takes care of everything properly for you. > riscv64-linux-gnu-ld: prelink.o:(.rodata+0x8): undefined reference to > `irq_startup_none' > riscv64-linux-gnu-ld: prelink.o:(.rodata+0x10): undefined reference to > `irq_actor_none' > riscv64-linux-gnu-ld: prelink.o:(.rodata+0x18): undefined reference to > `irq_actor_none' > riscv64-linux-gnu-ld: prelink.o:(.rodata+0x20): undefined reference to > `irq_actor_none' > riscv64-linux-gnu-ld: xen-syms: hidden symbol `irq_actor_none' isn't > defined > > That is why these stubs were introduced in the previous patch (because > common/irq.c isn't built at that moment) and are removed in this patch > (since at the moment of this patch, common/irq.c is now being built). These OTOH are a side effect of how no_irq_type works, which is horrifying, and not something I can unsee. I'll see about fixing it, because I really can't bare to leave it like this... ~Andrew
On Thu, 2024-05-30 at 18:45 +0100, Andrew Cooper wrote: > On 30/05/2024 6:12 pm, Oleksii K. wrote: > > On Thu, 2024-05-30 at 17:48 +0100, Andrew Cooper wrote: > > > On 29/05/2024 8:55 pm, Oleksii Kurochko wrote: > > > > diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c > > > > index 8285bcffef..bda35fc347 100644 > > > > --- a/xen/arch/riscv/stubs.c > > > > +++ b/xen/arch/riscv/stubs.c > > > > @@ -24,12 +24,6 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, > > > > cpu_core_mask); > > > > > > > > nodemask_t __read_mostly node_online_map = { { [0] = 1UL } }; > > > > > > > > -/* > > > > - * max_page is defined in page_alloc.c which isn't complied > > > > for > > > > now. > > > > - * definition of max_page will be remove as soon as page_alloc > > > > is > > > > built. > > > > - */ > > > > -unsigned long __read_mostly max_page; > > > > - > > > > /* time.c */ > > > > > > > > unsigned long __ro_after_init cpu_khz; /* CPU clock frequency > > > > in > > > > kHz. */ > > > > @@ -419,21 +413,3 @@ void __cpu_die(unsigned int cpu) > > > > { > > > > BUG_ON("unimplemented"); > > > > } > > > > - > > > > -/* > > > > - * The following functions are defined in common/irq.c, but > > > > common/irq.c isn't > > > > - * built for now. These changes will be removed there when > > > > common/irq.c is > > > > - * ready. > > > > - */ > > > > - > > > > -void cf_check irq_actor_none(struct irq_desc *desc) > > > > -{ > > > > - BUG_ON("unimplemented"); > > > > -} > > > > - > > > > -unsigned int cf_check irq_startup_none(struct irq_desc *desc) > > > > -{ > > > > - BUG_ON("unimplemented"); > > > > - > > > > - return 0; > > > > -} > > > All 3 of these are introduced in the previous patch and deleted > > > again > > > here. Looks like a rebasing accident. > > Not really. > > > > This was done to avoid build failures for RISC-V. If the HEAD is on > > the > > previous patch where these changes are introduced and then we just > > drop > > them, it will lead to a linkage error because these functions are > > defined in common/irq.c, which isn't built for RISC-V if the HEAD > > is on > > the previous patch: > > /build/xen/arch/riscv/entry.S:86: undefined reference to > > `max_page' > > Nothing in the series touches entry.S, so I'm not sure what this is > complaining about. > > The stub for get_upper_mfn_bound() references max_page, but it's only > used in a SYSCTL so you can avoid the problem with a BUG_ON(). I didn't get how I can use BUG_ON() with declaration of variable to avoid the compilation issue the undefined reference to max_page? > > BTW, you also don't need a return after a BUG_ON(). > __builtin_unreachable() takes care of everything properly for you. > > > > riscv64-linux-gnu-ld: prelink.o:(.rodata+0x8): undefined > > reference to > > `irq_startup_none' > > riscv64-linux-gnu-ld: prelink.o:(.rodata+0x10): undefined > > reference to > > `irq_actor_none' > > riscv64-linux-gnu-ld: prelink.o:(.rodata+0x18): undefined > > reference to > > `irq_actor_none' > > riscv64-linux-gnu-ld: prelink.o:(.rodata+0x20): undefined > > reference to > > `irq_actor_none' > > riscv64-linux-gnu-ld: xen-syms: hidden symbol `irq_actor_none' > > isn't > > defined > > > > That is why these stubs were introduced in the previous patch > > (because > > common/irq.c isn't built at that moment) and are removed in this > > patch > > (since at the moment of this patch, common/irq.c is now being > > built). > > These OTOH are a side effect of how no_irq_type works, which is > horrifying, and not something I can unsee. > > I'll see about fixing it, because I really can't bare to leave it > like > this... I am really sorry, but I don't understand should I deal with this somehow now or the provided changes are okay? ~ Oleksii
On 30/05/2024 7:27 pm, Oleksii K. wrote: > On Thu, 2024-05-30 at 18:45 +0100, Andrew Cooper wrote: >> On 30/05/2024 6:12 pm, Oleksii K. wrote: >>> On Thu, 2024-05-30 at 17:48 +0100, Andrew Cooper wrote: >>>> On 29/05/2024 8:55 pm, Oleksii Kurochko wrote: >>>>> diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c >>>>> index 8285bcffef..bda35fc347 100644 >>>>> --- a/xen/arch/riscv/stubs.c >>>>> +++ b/xen/arch/riscv/stubs.c >>>>> @@ -24,12 +24,6 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, >>>>> cpu_core_mask); >>>>> >>>>> nodemask_t __read_mostly node_online_map = { { [0] = 1UL } }; >>>>> >>>>> -/* >>>>> - * max_page is defined in page_alloc.c which isn't complied >>>>> for >>>>> now. >>>>> - * definition of max_page will be remove as soon as page_alloc >>>>> is >>>>> built. >>>>> - */ >>>>> -unsigned long __read_mostly max_page; >>>>> - >>>>> /* time.c */ >>>>> >>>>> unsigned long __ro_after_init cpu_khz; /* CPU clock frequency >>>>> in >>>>> kHz. */ >>>>> @@ -419,21 +413,3 @@ void __cpu_die(unsigned int cpu) >>>>> { >>>>> BUG_ON("unimplemented"); >>>>> } >>>>> - >>>>> -/* >>>>> - * The following functions are defined in common/irq.c, but >>>>> common/irq.c isn't >>>>> - * built for now. These changes will be removed there when >>>>> common/irq.c is >>>>> - * ready. >>>>> - */ >>>>> - >>>>> -void cf_check irq_actor_none(struct irq_desc *desc) >>>>> -{ >>>>> - BUG_ON("unimplemented"); >>>>> -} >>>>> - >>>>> -unsigned int cf_check irq_startup_none(struct irq_desc *desc) >>>>> -{ >>>>> - BUG_ON("unimplemented"); >>>>> - >>>>> - return 0; >>>>> -} >>>> All 3 of these are introduced in the previous patch and deleted >>>> again >>>> here. Looks like a rebasing accident. >>> Not really. >>> >>> This was done to avoid build failures for RISC-V. If the HEAD is on >>> the >>> previous patch where these changes are introduced and then we just >>> drop >>> them, it will lead to a linkage error because these functions are >>> defined in common/irq.c, which isn't built for RISC-V if the HEAD >>> is on >>> the previous patch: >>> /build/xen/arch/riscv/entry.S:86: undefined reference to >>> `max_page' >> Nothing in the series touches entry.S, so I'm not sure what this is >> complaining about. >> >> The stub for get_upper_mfn_bound() references max_page, but it's only >> used in a SYSCTL so you can avoid the problem with a BUG_ON(). > I didn't get how I can use BUG_ON() with declaration of variable to > avoid the compilation issue the undefined reference to max_page? You don't need to implement get_upper_mfn_bound() yet. If you implement it as: unsigned long get_upper_mfn_bound(void) { BUG_ON("unimplemented"); } in the previous patch, then you also don't have any problems with max_page between these two patches either. > >> BTW, you also don't need a return after a BUG_ON(). >> __builtin_unreachable() takes care of everything properly for you. >> >> >>> riscv64-linux-gnu-ld: prelink.o:(.rodata+0x8): undefined >>> reference to >>> `irq_startup_none' >>> riscv64-linux-gnu-ld: prelink.o:(.rodata+0x10): undefined >>> reference to >>> `irq_actor_none' >>> riscv64-linux-gnu-ld: prelink.o:(.rodata+0x18): undefined >>> reference to >>> `irq_actor_none' >>> riscv64-linux-gnu-ld: prelink.o:(.rodata+0x20): undefined >>> reference to >>> `irq_actor_none' >>> riscv64-linux-gnu-ld: xen-syms: hidden symbol `irq_actor_none' >>> isn't >>> defined >>> >>> That is why these stubs were introduced in the previous patch >>> (because >>> common/irq.c isn't built at that moment) and are removed in this >>> patch >>> (since at the moment of this patch, common/irq.c is now being >>> built). >> These OTOH are a side effect of how no_irq_type works, which is >> horrifying, and not something I can unsee. >> >> I'll see about fixing it, because I really can't bare to leave it >> like >> this... > I am really sorry, but I don't understand should I deal with this > somehow now or the provided changes are okay? I've emailed out a 2-patch series to unbreak this. ~Andrew
diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile index 60afbc0ad9..81b77b13d6 100644 --- a/xen/arch/riscv/Makefile +++ b/xen/arch/riscv/Makefile @@ -12,10 +12,24 @@ $(TARGET): $(TARGET)-syms $(OBJCOPY) -O binary -S $< $@ $(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds - $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) -o $@ + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \ + $(objtree)/common/symbols-dummy.o -o $(dot-target).0 + $(NM) -pa --format=sysv $(dot-target).0 \ + | $(objtree)/tools/symbols $(all_symbols) --sysv --sort \ + > $(dot-target).0.S + $(MAKE) $(build)=$(@D) $(dot-target).0.o + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \ + $(dot-target).0.o -o $(dot-target).1 + $(NM) -pa --format=sysv $(dot-target).1 \ + | $(objtree)/tools/symbols $(all_symbols) --sysv --sort \ + > $(dot-target).1.S + $(MAKE) $(build)=$(@D) $(dot-target).1.o + $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) \ + $(dot-target).1.o -o $@ $(NM) -pa --format=sysv $@ \ | $(objtree)/tools/symbols --all-symbols --xensyms --sysv --sort \ > $@.map + rm -f $(@D)/.$(@F).[0-9]* $(obj)/xen.lds: $(src)/xen.lds.S FORCE $(call if_changed_dep,cpp_lds_S) diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk index 8c071aff65..17827c302c 100644 --- a/xen/arch/riscv/arch.mk +++ b/xen/arch/riscv/arch.mk @@ -38,7 +38,3 @@ extensions := $(subst $(space),,$(extensions)) # -mcmodel=medlow would force Xen into the lower half. CFLAGS += $(riscv-generic-flags)$(extensions) -mstrict-align -mcmodel=medany - -# TODO: Drop override when more of the build is working -override ALL_OBJS-y = arch/$(SRCARCH)/built_in.o -override ALL_LIBS-y = diff --git a/xen/arch/riscv/early_printk.c b/xen/arch/riscv/early_printk.c index 60742a042d..610c814f54 100644 --- a/xen/arch/riscv/early_printk.c +++ b/xen/arch/riscv/early_printk.c @@ -40,171 +40,3 @@ void early_printk(const char *str) str++; } } - -/* - * The following #if 1 ... #endif should be removed after printk - * and related stuff are ready. - */ -#if 1 - -#include <xen/stdarg.h> -#include <xen/string.h> - -/** - * strlen - Find the length of a string - * @s: The string to be sized - */ -size_t (strlen)(const char * s) -{ - const char *sc; - - for (sc = s; *sc != '\0'; ++sc) - /* nothing */; - return sc - s; -} - -/** - * memcpy - Copy one area of memory to another - * @dest: Where to copy to - * @src: Where to copy from - * @count: The size of the area. - * - * You should not use this function to access IO space, use memcpy_toio() - * or memcpy_fromio() instead. - */ -void *(memcpy)(void *dest, const void *src, size_t count) -{ - char *tmp = (char *) dest, *s = (char *) src; - - while (count--) - *tmp++ = *s++; - - return dest; -} - -int vsnprintf(char* str, size_t size, const char* format, va_list args) -{ - size_t i = 0; /* Current position in the output string */ - size_t written = 0; /* Total number of characters written */ - char* dest = str; - - while ( format[i] != '\0' && written < size - 1 ) - { - if ( format[i] == '%' ) - { - i++; - - if ( format[i] == '\0' ) - break; - - if ( format[i] == '%' ) - { - if ( written < size - 1 ) - { - dest[written] = '%'; - written++; - } - i++; - continue; - } - - /* - * Handle format specifiers. - * For simplicity, only %s and %d are implemented here. - */ - - if ( format[i] == 's' ) - { - char* arg = va_arg(args, char*); - size_t arglen = strlen(arg); - - size_t remaining = size - written - 1; - - if ( arglen > remaining ) - arglen = remaining; - - memcpy(dest + written, arg, arglen); - - written += arglen; - i++; - } - else if ( format[i] == 'd' ) - { - int arg = va_arg(args, int); - - /* Convert the integer to string representation */ - char numstr[32]; /* Assumes a maximum of 32 digits */ - int numlen = 0; - int num = arg; - size_t remaining; - - if ( arg < 0 ) - { - if ( written < size - 1 ) - { - dest[written] = '-'; - written++; - } - - num = -arg; - } - - do - { - numstr[numlen] = '0' + num % 10; - num = num / 10; - numlen++; - } while ( num > 0 ); - - /* Reverse the string */ - for (int j = 0; j < numlen / 2; j++) - { - char tmp = numstr[j]; - numstr[j] = numstr[numlen - 1 - j]; - numstr[numlen - 1 - j] = tmp; - } - - remaining = size - written - 1; - - if ( numlen > remaining ) - numlen = remaining; - - memcpy(dest + written, numstr, numlen); - - written += numlen; - i++; - } - } - else - { - if ( written < size - 1 ) - { - dest[written] = format[i]; - written++; - } - i++; - } - } - - if ( size > 0 ) - dest[written] = '\0'; - - return written; -} - -void printk(const char *format, ...) -{ - static char buf[1024]; - - va_list args; - va_start(args, format); - - (void)vsnprintf(buf, sizeof(buf), format, args); - - early_printk(buf); - - va_end(args); -} - -#endif - diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c index 8285bcffef..bda35fc347 100644 --- a/xen/arch/riscv/stubs.c +++ b/xen/arch/riscv/stubs.c @@ -24,12 +24,6 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_mask); nodemask_t __read_mostly node_online_map = { { [0] = 1UL } }; -/* - * max_page is defined in page_alloc.c which isn't complied for now. - * definition of max_page will be remove as soon as page_alloc is built. - */ -unsigned long __read_mostly max_page; - /* time.c */ unsigned long __ro_after_init cpu_khz; /* CPU clock frequency in kHz. */ @@ -419,21 +413,3 @@ void __cpu_die(unsigned int cpu) { BUG_ON("unimplemented"); } - -/* - * The following functions are defined in common/irq.c, but common/irq.c isn't - * built for now. These changes will be removed there when common/irq.c is - * ready. - */ - -void cf_check irq_actor_none(struct irq_desc *desc) -{ - BUG_ON("unimplemented"); -} - -unsigned int cf_check irq_startup_none(struct irq_desc *desc) -{ - BUG_ON("unimplemented"); - - return 0; -}