| Message ID | 20250213185055.711703-1-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/x86: fix build with CONFIG_HVM=n and -Og | expand |
On 2/13/25 7:50 PM, Stewart Hildebrand wrote: > When building with CONFIG_HVM=n and -Og, we encounter: > > prelink.o: in function `pit_set_gate': > xen/xen/arch/x86/emul-i8254.c:195: undefined reference to `destroy_periodic_time' > > Add an IS_ENABLED(CONFIG_HVM) check to assist with dead code > elimination. > > Fixes: 14f42af3f52d ("x86/vPIT: account for "counter stopped" time") > Signed-off-by: Stewart Hildebrand<stewart.hildebrand@amd.com> With proper review: Release-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com> ~ Oleksii > --- > xen/arch/x86/emul-i8254.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/x86/emul-i8254.c b/xen/arch/x86/emul-i8254.c > index 144aa168a3f0..7bc4b31b2894 100644 > --- a/xen/arch/x86/emul-i8254.c > +++ b/xen/arch/x86/emul-i8254.c > @@ -191,7 +191,7 @@ static void pit_set_gate(PITState *pit, int channel, int val) > case 3: > case 4: > /* Disable counting. */ > - if ( !channel ) > + if ( IS_ENABLED(CONFIG_HVM) && !channel ) > destroy_periodic_time(&pit->pt0); > pit->count_stop_time[channel] = get_guest_time(v); > break; > > base-commit: b5b2f9877a8777af6b78944407527e0a450389a2
On 13/02/2025 6:50 pm, Stewart Hildebrand wrote: > When building with CONFIG_HVM=n and -Og, we encounter: > > prelink.o: in function `pit_set_gate': > xen/xen/arch/x86/emul-i8254.c:195: undefined reference to `destroy_periodic_time' > > Add an IS_ENABLED(CONFIG_HVM) check to assist with dead code > elimination. > > Fixes: 14f42af3f52d ("x86/vPIT: account for "counter stopped" time") > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> While I appreciate the effort to get -Og working (I tried and gave up due to frustration), this is gnarly. PIT emulation is used by both PV and HVM guests. All other uses of {create,destroy}_periodic_time() are behind something that explicitly short-circuits in !HVM cases (usually an is_hvm_*() predicate). The PV path would normally passes 2 for the channel, which would normally get const-propagated and trigger DCE here. One option might be to make pit_set_gate() be __always_inline. It only has a single caller, and it's only because of -Og that it doesn't get inlined. Then again, this is arguably more subtle than the fix presented here. A preferable fix (but one that really won't get into 4.20 at this point) would be to genuinely compile pit->pt0 out in !HVM builds. That would save structure space, but would also force the use of full #ifdef-ary across this file. Is this the singular failure with -Og, or are there others? I never got it working, and there were quite a few failures that failed to get a resolution. ~Andrew
On 14.02.2025 02:05, Andrew Cooper wrote: > On 13/02/2025 6:50 pm, Stewart Hildebrand wrote: >> When building with CONFIG_HVM=n and -Og, we encounter: >> >> prelink.o: in function `pit_set_gate': >> xen/xen/arch/x86/emul-i8254.c:195: undefined reference to `destroy_periodic_time' >> >> Add an IS_ENABLED(CONFIG_HVM) check to assist with dead code >> elimination. >> >> Fixes: 14f42af3f52d ("x86/vPIT: account for "counter stopped" time") While I don't mind this as a tag, you could equally blame the commit having added support for EXTRA_CFLAGS_XEN_CORE, for not documenting restrictions. As Andrew says further down, it's deemed known that -Og doesn't work reliably. And who knows what other very special options would cause havoc. I'm inclined to go as far as saying that quite likely no Fixes: tag is appropriate here at all, as long as we have no way to use -Og without making use of EXTRA_CFLAGS_XEN_CORE (or hacking it in another way). People using EXTRA_CFLAGS_XEN_CORE are on their own anyway, because the documenting of restrictions mentioned above would be nice in theory, but is entirely impractical imo: We'd need to exhaustively test all options, and then document which ones we've found working (under what conditions). >> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> > > While I appreciate the effort to get -Og working (I tried and gave up > due to frustration), this is gnarly. > > PIT emulation is used by both PV and HVM guests. All other uses of > {create,destroy}_periodic_time() are behind something that explicitly > short-circuits in !HVM cases (usually an is_hvm_*() predicate). > > The PV path would normally passes 2 for the channel, which would > normally get const-propagated and trigger DCE here. > > One option might be to make pit_set_gate() be __always_inline. It only > has a single caller, and it's only because of -Og that it doesn't get > inlined. Then again, this is arguably more subtle than the fix > presented here. Making it always_inline on the basis that there's just a single caller would be equivalent to simply dropping the handling of channel 0 when the sole caller passes channel 2. I don't like either. Instead ... > A preferable fix (but one that really won't get into 4.20 at this point) > would be to genuinely compile pit->pt0 out in !HVM builds. That would > save structure space, but would also force the use of full #ifdef-ary > across this file. ... I was about to also suggest this approach. Jan
diff --git a/xen/arch/x86/emul-i8254.c b/xen/arch/x86/emul-i8254.c index 144aa168a3f0..7bc4b31b2894 100644 --- a/xen/arch/x86/emul-i8254.c +++ b/xen/arch/x86/emul-i8254.c @@ -191,7 +191,7 @@ static void pit_set_gate(PITState *pit, int channel, int val) case 3: case 4: /* Disable counting. */ - if ( !channel ) + if ( IS_ENABLED(CONFIG_HVM) && !channel ) destroy_periodic_time(&pit->pt0); pit->count_stop_time[channel] = get_guest_time(v); break;
When building with CONFIG_HVM=n and -Og, we encounter: prelink.o: in function `pit_set_gate': xen/xen/arch/x86/emul-i8254.c:195: undefined reference to `destroy_periodic_time' Add an IS_ENABLED(CONFIG_HVM) check to assist with dead code elimination. Fixes: 14f42af3f52d ("x86/vPIT: account for "counter stopped" time") Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> --- xen/arch/x86/emul-i8254.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: b5b2f9877a8777af6b78944407527e0a450389a2