On 18.10.2024 17:57, [email protected] wrote: > On Fri, 2024-10-18 at 15:27 +0200, Jan Beulich wrote: >> On 18.10.2024 15:10, [email protected] 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
