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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 2/14/25 03:25, Jan Beulich wrote:
> 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).

I'll drop the Fixes: tag

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

I'll give this a try for v2