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(+)
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");
+}
--
2.47.0On 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
© 2016 - 2024 Red Hat, Inc.