| Message ID | 3dfd4bdf6584e73ecdbff5a66fdbaec68e31cc3c.1729068334.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Setup memory management | expand |
On 16.10.2024 11:15, Oleksii Kurochko wrote: > To avoid the following linkage fail the stub for share_xen_page_with_guest() > is introduced: What do you intend to express with "is introduced"? Is there a problem now? Would there be a problem with subsequent changes? I'm entirely fine with adding that stub, but the description should make clear why /when exactly it's needed. Jan > riscv64-linux-gnu-ld: prelink.o: in function `tasklet_kill': > /build/xen/common/tasklet.c:176: undefined reference to `share_xen_page_with_guest' > riscv64-linux-gnu-ld: ./.xen-syms.0: hidden symbol `share_xen_page_with_guest' isn't defined > riscv64-linux-gnu-ld: final link failed: bad value > > $ find . -name \*.o | while read F; do nm $F | grep share_xen_page_with_guest && echo $F; done > U share_xen_page_with_guest > ./xen/common/built_in.o > U share_xen_page_with_guest > ./xen/common/trace.o > U share_xen_page_with_guest > ./xen/prelink.o > > Despite the linker fingering tasklet.c (very likely a toolchain bug), > it's trace.o which has the undefined reference. > > Looking at trace.i, there is call of share_xen_page_with_guest() in > share_xen_page_with_privileged_guests() from asm/mm.h. > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > xen/arch/riscv/stubs.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c > index 5951b0ce91..c9a590b225 100644 > --- a/xen/arch/riscv/stubs.c > +++ b/xen/arch/riscv/stubs.c > @@ -2,7 +2,9 @@ > #include <xen/cpumask.h> > #include <xen/domain.h> > #include <xen/irq.h> > +#include <xen/mm.h> > #include <xen/nodemask.h> > +#include <xen/sched.h> > #include <xen/sections.h> > #include <xen/time.h> > #include <public/domctl.h> > @@ -409,3 +411,11 @@ unsigned long get_upper_mfn_bound(void) > { > BUG_ON("unimplemented"); > } > + > +/* mm.c */ > + > +void share_xen_page_with_guest(struct page_info *page, struct domain *d, > + enum XENSHARE_flags flags) > +{ > + BUG_ON("unimplemented"); > +}
On Thu, 2024-10-17 at 16:51 +0200, Jan Beulich wrote: > On 16.10.2024 11:15, Oleksii Kurochko wrote: > > To avoid the following linkage fail the stub for > > share_xen_page_with_guest() > > is introduced: > > What do you intend to express with "is introduced"? Is there a > problem now? > Would there be a problem with subsequent changes? I'm entirely fine > with > adding that stub, but the description should make clear why /when > exactly > it's needed. I mentioned that in the cover letter: ``` Also, after patch 3 ("xen/riscv: introduce setup_mm()") of this patch series, the linkage error mentioned in patch 1 ("xen/riscv: add stub for share_xen_page_with_guest()") began to occur, so patch 1 addresses this issue. ``` I thought it would be the better then just mention in the commit message that. Will it be fine to mention instead: ``` Introduction of setup memory management function will require share_xen_page_with_guest() defined, at least, as a stub otherwise the following linkage error will occur... ``` Perhaps it would be better just to merge these changes with patch 3 and add an explanation in patch 3 commit message. ~ Oleksii > > riscv64-linux-gnu-ld: prelink.o: in function `tasklet_kill': > > /build/xen/common/tasklet.c:176: undefined reference to > > `share_xen_page_with_guest' > > riscv64-linux-gnu-ld: ./.xen-syms.0: hidden symbol > > `share_xen_page_with_guest' isn't defined > > riscv64-linux-gnu-ld: final link failed: bad value > > > > $ find . -name \*.o | while read F; do nm $F | grep > > share_xen_page_with_guest && echo $F; done > > U share_xen_page_with_guest > > ./xen/common/built_in.o > > U share_xen_page_with_guest > > ./xen/common/trace.o > > U share_xen_page_with_guest > > ./xen/prelink.o > > > > Despite the linker fingering tasklet.c (very likely a toolchain > > bug), > > it's trace.o which has the undefined reference. > > > > Looking at trace.i, there is call of share_xen_page_with_guest() in > > share_xen_page_with_privileged_guests() from asm/mm.h. > > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > --- > > xen/arch/riscv/stubs.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c > > index 5951b0ce91..c9a590b225 100644 > > --- a/xen/arch/riscv/stubs.c > > +++ b/xen/arch/riscv/stubs.c > > @@ -2,7 +2,9 @@ > > #include <xen/cpumask.h> > > #include <xen/domain.h> > > #include <xen/irq.h> > > +#include <xen/mm.h> > > #include <xen/nodemask.h> > > +#include <xen/sched.h> > > #include <xen/sections.h> > > #include <xen/time.h> > > #include <public/domctl.h> > > @@ -409,3 +411,11 @@ unsigned long get_upper_mfn_bound(void) > > { > > BUG_ON("unimplemented"); > > } > > + > > +/* mm.c */ > > + > > +void share_xen_page_with_guest(struct page_info *page, struct > > domain *d, > > + enum XENSHARE_flags flags) > > +{ > > + BUG_ON("unimplemented"); > > +} >
On 18.10.2024 15:10, oleksii.kurochko@gmail.com wrote: > On Thu, 2024-10-17 at 16:51 +0200, Jan Beulich wrote: >> On 16.10.2024 11:15, Oleksii Kurochko wrote: >>> To avoid the following linkage fail the stub for >>> share_xen_page_with_guest() >>> is introduced: >> >> What do you intend to express with "is introduced"? Is there a >> problem now? >> Would there be a problem with subsequent changes? I'm entirely fine >> with >> adding that stub, but the description should make clear why /when >> exactly >> it's needed. > I mentioned that in the cover letter: > ``` > Also, after patch 3 ("xen/riscv: introduce setup_mm()") of this > patch series, > the linkage error mentioned in patch 1 ("xen/riscv: add stub for > share_xen_page_with_guest()") began to occur, so patch 1 addresses > this issue. > ``` > I thought it would be the better then just mention in the commit > message that. Mentioning in the cover letter is fine, but each patch needs to also be self-contained. > Will it be fine to mention instead: > ``` > Introduction of setup memory management function will require > share_xen_page_with_guest() defined, at least, as a stub otherwise > the following linkage error will occur... > ``` Quoting the linker error is imo of limited use. What such an explanation wants to say is why, all of the sudden, such an error occurs. After all you don't change anything directly related to common/trace.c. > Perhaps it would be better just to merge these changes with patch 3 and > add an explanation in patch 3 commit message. Perhaps, as you say. Jan
On Fri, 2024-10-18 at 15:27 +0200, Jan Beulich wrote: > On 18.10.2024 15:10, oleksii.kurochko@gmail.com wrote: > > On Thu, 2024-10-17 at 16:51 +0200, Jan Beulich wrote: > > > On 16.10.2024 11:15, Oleksii Kurochko wrote: > > > > To avoid the following linkage fail the stub for > > > > share_xen_page_with_guest() > > > > is introduced: > > > > > > What do you intend to express with "is introduced"? Is there a > > > problem now? > > > Would there be a problem with subsequent changes? I'm entirely > > > fine > > > with > > > adding that stub, but the description should make clear why /when > > > exactly > > > it's needed. > > I mentioned that in the cover letter: > > ``` > > Also, after patch 3 ("xen/riscv: introduce setup_mm()") of this > > patch series, > > the linkage error mentioned in patch 1 ("xen/riscv: add stub for > > share_xen_page_with_guest()") began to occur, so patch 1 > > addresses > > this issue. > > ``` > > I thought it would be the better then just mention in the commit > > message that. > > Mentioning in the cover letter is fine, but each patch needs to also > be self-contained. > > > Will it be fine to mention instead: > > ``` > > Introduction of setup memory management function will require > > share_xen_page_with_guest() defined, at least, as a stub > > otherwise > > the following linkage error will occur... > > ``` > > Quoting the linker error is imo of limited use. What such an > explanation > wants to say is why, all of the sudden, such an error occurs. After > all > you don't change anything directly related to common/trace.c. if maddr_to_virt() is defined as: static inline void *maddr_to_virt(paddr_t ma) { BUG_ON("unimplemented"); return NULL; // /* Offset in the direct map, accounting for pdx compression */ // unsigned long va_offset = maddr_to_directmapoff(ma); // ASSERT(va_offset < DIRECTMAP_SIZE); // return (void *)(DIRECTMAP_VIRT_START + va_offset); } Then no stub for share_xen_page_with_privileged_guests() is needed but it isn't clear at all why the definition of maddr_to_virt() affects linkage of share_xen_page_with_privileged_guests(). I tried to look what is the difference after preprocessing stage for common/trace.o and the only difference is in how maddr_to_virt() is implemented: 7574a7575,7577 > do { if (__builtin_expect(!!("unimplemented"),0)) do { do { ({ _Static_assert(!((30) >> ((31 - 24) + (31 - 24))), "!(" "(30) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)" ")"); }); ({ _Static_assert(!((2) >= 4), "!(" "(2) >= BUGFRAME_NR" ")"); }); asm volatile ( ".Lbug%=:""unimp""\n" " .pushsection .bug_frames.%""""[bf_type], \"a\", %%progbits\n" " .p2align 2\n" ".Lfrm%=:\n" " .long (.Lbug%= - .Lfrm%=) + %""""[bf_line_hi]\n" " .long (%""""[bf_ptr] - .Lfrm%=) + %""""[bf_line_lo]\n" " .if " "0" "\n" " .long 0, %""""[bf_msg] - .Lfrm%=\n" " .endif\n" " .popsection\n" :: [bf_type] "i" (2), [bf_ptr] "i" ("./arch/riscv/include/asm/mm.h"), [bf_msg] "i" (((void*)0)), [bf_line_lo] "i" (((30) & ((1 << (31 - 24)) - 1)) << 24), [bf_line_hi] "i" (((30) >> (31 - 24)) << 24) ); } while ( 0 ); __builtin_unreachable(); } while ( 0 ); } while (0); > return ((void*)0); > 7578d7580 < unsigned long va_offset = (ma); 7580d7581 < do { if ( __builtin_expect(!!(!(va_offset < ((((vaddr_t)(509)) << ((3 - 1) * (9) + 12)) - (((vaddr_t)(200)) << ((3 - 1) * (9) + 12))))),0) ) do { do { ({ _Static_assert(!((35) >> ((31 - 24) + (31 - 24))), "!(" "(35) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)" ")"); }); ({ _Static_assert(!((3) >= 4), "!(" "(3) >= BUGFRAME_NR" ")"); }); asm volatile ( ".Lbug%=:""unimp""\n" " .pushsection .bug_frames.%""""[bf_type], \"a\", %%progbits\n" " .p2align 2\n" ".Lfrm%=:\n" " .long (.Lbug%= - .Lfrm%=) + %""""[bf_line_hi]\n" " .long (%""""[bf_ptr] - .Lfrm%=) + %""""[bf_line_lo]\n" " .if " "1" "\n" " .long 0, %""""[bf_msg] - .Lfrm%=\n" " .endif\n" " .popsection\n" :: [bf_type] "i" (3), [bf_ptr] "i" ("./arch/riscv/include/asm/mm.h"), [bf_msg] "i" ("va_offset < DIRECTMAP_SIZE"), [bf_line_lo] "i" (((35) & ((1 << (31 - 24)) - 1)) << 24), [bf_line_hi] "i" (((35) >> (31 - 24)) << 24) ); } while ( 0 ); __builtin_unreachable(); } while ( 0 ); } while (0); 7582d7582 < return (void *)((((vaddr_t)(200)) << ((3 - 1) * (9) + 12)) + va_offset); Could it be that DCE likely happen when maddr_to_virt() is defined as stub and so code which call share_xen_page_with_privileged_guests() is just eliminated? ( but I am not sure that I know fast way to check that , do you have any pointers? ) ~ Oleksii
On 18.10.2024 17:57, oleksii.kurochko@gmail.com wrote: > On Fri, 2024-10-18 at 15:27 +0200, Jan Beulich wrote: >> On 18.10.2024 15:10, oleksii.kurochko@gmail.com wrote: >>> On Thu, 2024-10-17 at 16:51 +0200, Jan Beulich wrote: >>>> On 16.10.2024 11:15, Oleksii Kurochko wrote: >>>>> To avoid the following linkage fail the stub for >>>>> share_xen_page_with_guest() >>>>> is introduced: >>>> >>>> What do you intend to express with "is introduced"? Is there a >>>> problem now? >>>> Would there be a problem with subsequent changes? I'm entirely >>>> fine >>>> with >>>> adding that stub, but the description should make clear why /when >>>> exactly >>>> it's needed. >>> I mentioned that in the cover letter: >>> ``` >>> Also, after patch 3 ("xen/riscv: introduce setup_mm()") of this >>> patch series, >>> the linkage error mentioned in patch 1 ("xen/riscv: add stub for >>> share_xen_page_with_guest()") began to occur, so patch 1 >>> addresses >>> this issue. >>> ``` >>> I thought it would be the better then just mention in the commit >>> message that. >> >> Mentioning in the cover letter is fine, but each patch needs to also >> be self-contained. >> >>> Will it be fine to mention instead: >>> ``` >>> Introduction of setup memory management function will require >>> share_xen_page_with_guest() defined, at least, as a stub >>> otherwise >>> the following linkage error will occur... >>> ``` >> >> Quoting the linker error is imo of limited use. What such an >> explanation >> wants to say is why, all of the sudden, such an error occurs. After >> all >> you don't change anything directly related to common/trace.c. > if maddr_to_virt() is defined as: > static inline void *maddr_to_virt(paddr_t ma) > { > BUG_ON("unimplemented"); > return NULL; > // /* Offset in the direct map, accounting for pdx compression */ > // unsigned long va_offset = maddr_to_directmapoff(ma); > > // ASSERT(va_offset < DIRECTMAP_SIZE); > > // return (void *)(DIRECTMAP_VIRT_START + va_offset); > } > Then no stub for share_xen_page_with_privileged_guests() is needed but > it isn't clear at all why the definition of maddr_to_virt() affects > linkage of share_xen_page_with_privileged_guests(). > > I tried to look what is the difference after preprocessing stage for > common/trace.o and the only difference is in how maddr_to_virt() is > implemented: > 7574a7575,7577 > > do { if (__builtin_expect(!!("unimplemented"),0)) do { do { ({ > _Static_assert(!((30) >> ((31 - 24) + (31 - 24))), "!(" "(30) >> > (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)" ")"); }); ({ > _Static_assert(!((2) >= 4), "!(" "(2) >= BUGFRAME_NR" ")"); }); asm > volatile ( ".Lbug%=:""unimp""\n" " .pushsection > .bug_frames.%""""[bf_type], \"a\", %%progbits\n" " .p2align 2\n" > ".Lfrm%=:\n" " .long (.Lbug%= - .Lfrm%=) + %""""[bf_line_hi]\n" " > .long (%""""[bf_ptr] - .Lfrm%=) + %""""[bf_line_lo]\n" " .if " "0" > "\n" " .long 0, %""""[bf_msg] - .Lfrm%=\n" " .endif\n" " > .popsection\n" :: [bf_type] "i" (2), [bf_ptr] "i" > ("./arch/riscv/include/asm/mm.h"), [bf_msg] "i" (((void*)0)), > [bf_line_lo] "i" (((30) & ((1 << (31 - 24)) - 1)) << 24), > [bf_line_hi] "i" (((30) >> (31 - 24)) << 24) ); } while ( 0 ); > __builtin_unreachable(); } while ( 0 ); } while (0); > > return ((void*)0); > > > 7578d7580 > < unsigned long va_offset = (ma); > 7580d7581 > < do { if ( __builtin_expect(!!(!(va_offset < ((((vaddr_t)(509)) > << ((3 - 1) * (9) + 12)) - (((vaddr_t)(200)) << ((3 - 1) * (9) + > 12))))),0) ) do { do { ({ _Static_assert(!((35) >> ((31 - 24) + (31 > - 24))), "!(" "(35) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)" > ")"); }); ({ _Static_assert(!((3) >= 4), "!(" "(3) >= BUGFRAME_NR" > ")"); }); asm volatile ( ".Lbug%=:""unimp""\n" " .pushsection > .bug_frames.%""""[bf_type], \"a\", %%progbits\n" " .p2align 2\n" > ".Lfrm%=:\n" " .long (.Lbug%= - .Lfrm%=) + %""""[bf_line_hi]\n" " > .long (%""""[bf_ptr] - .Lfrm%=) + %""""[bf_line_lo]\n" " .if " "1" > "\n" " .long 0, %""""[bf_msg] - .Lfrm%=\n" " .endif\n" " > .popsection\n" :: [bf_type] "i" (3), [bf_ptr] "i" > ("./arch/riscv/include/asm/mm.h"), [bf_msg] "i" ("va_offset < > DIRECTMAP_SIZE"), [bf_line_lo] "i" (((35) & ((1 << (31 - 24)) - 1)) > << 24), [bf_line_hi] "i" (((35) >> (31 - 24)) << 24) ); } while ( 0 > ); __builtin_unreachable(); } while ( 0 ); } while (0); > 7582d7582 > < return (void *)((((vaddr_t)(200)) << ((3 - 1) * (9) + 12)) + > va_offset); > > Could it be that DCE likely happen when maddr_to_virt() is defined as > stub and so code which call share_xen_page_with_privileged_guests() is > just eliminated? ( but I am not sure that I know fast way to check that > , do you have any pointers? ) Yes, I think DCE is the explanation here. And that's what (imo) wants saying in the description. Jan
diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c index 5951b0ce91..c9a590b225 100644 --- a/xen/arch/riscv/stubs.c +++ b/xen/arch/riscv/stubs.c @@ -2,7 +2,9 @@ #include <xen/cpumask.h> #include <xen/domain.h> #include <xen/irq.h> +#include <xen/mm.h> #include <xen/nodemask.h> +#include <xen/sched.h> #include <xen/sections.h> #include <xen/time.h> #include <public/domctl.h> @@ -409,3 +411,11 @@ unsigned long get_upper_mfn_bound(void) { BUG_ON("unimplemented"); } + +/* mm.c */ + +void share_xen_page_with_guest(struct page_info *page, struct domain *d, + enum XENSHARE_flags flags) +{ + BUG_ON("unimplemented"); +}
To avoid the following linkage fail the stub for share_xen_page_with_guest() is introduced: riscv64-linux-gnu-ld: prelink.o: in function `tasklet_kill': /build/xen/common/tasklet.c:176: undefined reference to `share_xen_page_with_guest' riscv64-linux-gnu-ld: ./.xen-syms.0: hidden symbol `share_xen_page_with_guest' isn't defined riscv64-linux-gnu-ld: final link failed: bad value $ find . -name \*.o | while read F; do nm $F | grep share_xen_page_with_guest && echo $F; done U share_xen_page_with_guest ./xen/common/built_in.o U share_xen_page_with_guest ./xen/common/trace.o U share_xen_page_with_guest ./xen/prelink.o Despite the linker fingering tasklet.c (very likely a toolchain bug), it's trace.o which has the undefined reference. Looking at trace.i, there is call of share_xen_page_with_guest() in share_xen_page_with_privileged_guests() from asm/mm.h. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- xen/arch/riscv/stubs.c | 10 ++++++++++ 1 file changed, 10 insertions(+)