Re: [PATCH] xen/x86: fix build with CONFIG_HVM=n and -Og

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxx>
> 
> 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