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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.10.2024 17:57, oleksii.kurochko@xxxxxxxxx wrote:
> 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? )

Yes, I think DCE is the explanation here. And that's what (imo) wants saying
in the description.

Jan