Re: [PATCH v1 1/5] xen/riscv: add stub for share_xen_page_with_guest()

[oleksii.kurochko@xxxxxxxxx]

On Fri, 2024-10-18 at 15:27 +0200, Jan Beulich wrote:
> On 18.10.2024 15:10, oleksii.kurochko@xxxxxxxxx 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