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.hildebr...@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
