* [PATCH] xen/x86: fix build with CONFIG_HVM=n and -Og
@ 2025-02-13 18:50 Stewart Hildebrand
2025-02-13 20:17 ` Oleksii Kurochko
2025-02-14 1:05 ` Andrew Cooper
0 siblings, 2 replies; 7+ messages in thread
From: Stewart Hildebrand @ 2025-02-13 18:50 UTC (permalink / raw)
To: xen-devel
Cc: Stewart Hildebrand, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Oleksii Kurochko
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(-)
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
--
2.48.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] xen/x86: fix build with CONFIG_HVM=n and -Og
2025-02-13 18:50 [PATCH] xen/x86: fix build with CONFIG_HVM=n and -Og Stewart Hildebrand
@ 2025-02-13 20:17 ` Oleksii Kurochko
2025-02-14 1:05 ` Andrew Cooper
1 sibling, 0 replies; 7+ messages in thread
From: Oleksii Kurochko @ 2025-02-13 20:17 UTC (permalink / raw)
To: Stewart Hildebrand, xen-devel
Cc: Jan Beulich, Andrew Cooper, Roger Pau Monné
[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]
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
[-- Attachment #2: Type: text/html, Size: 1970 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xen/x86: fix build with CONFIG_HVM=n and -Og
2025-02-13 18:50 [PATCH] xen/x86: fix build with CONFIG_HVM=n and -Og Stewart Hildebrand
2025-02-13 20:17 ` Oleksii Kurochko
@ 2025-02-14 1:05 ` Andrew Cooper
2025-02-14 8:25 ` Jan Beulich
2025-03-03 18:59 ` Stewart Hildebrand
1 sibling, 2 replies; 7+ messages in thread
From: Andrew Cooper @ 2025-02-14 1:05 UTC (permalink / raw)
To: Stewart Hildebrand, xen-devel
Cc: Jan Beulich, Roger Pau Monné, Oleksii Kurochko
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xen/x86: fix build with CONFIG_HVM=n and -Og
2025-02-14 1:05 ` Andrew Cooper
@ 2025-02-14 8:25 ` Jan Beulich
2025-03-03 21:18 ` Stewart Hildebrand
2025-03-03 18:59 ` Stewart Hildebrand
1 sibling, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2025-02-14 8:25 UTC (permalink / raw)
To: Andrew Cooper, Stewart Hildebrand
Cc: Roger Pau Monné, Oleksii Kurochko, xen-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xen/x86: fix build with CONFIG_HVM=n and -Og
2025-02-14 1:05 ` Andrew Cooper
2025-02-14 8:25 ` Jan Beulich
@ 2025-03-03 18:59 ` Stewart Hildebrand
2025-03-03 19:19 ` Andrew Cooper
1 sibling, 1 reply; 7+ messages in thread
From: Stewart Hildebrand @ 2025-03-03 18:59 UTC (permalink / raw)
To: Andrew Cooper, xen-devel
Cc: Jan Beulich, Roger Pau Monné, Oleksii Kurochko
On 2/13/25 20: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'
<snip>
> 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.
This is the only one that I'm aware of, and it only occurs when
CONFIG_HVM=n. I only happened to stumble upon this occurrence because
allyesconfig apparently results in CONFIG_HVM=n. I've been building Xen
with -Og in my development workflow for a couple of releases now.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xen/x86: fix build with CONFIG_HVM=n and -Og
2025-03-03 18:59 ` Stewart Hildebrand
@ 2025-03-03 19:19 ` Andrew Cooper
0 siblings, 0 replies; 7+ messages in thread
From: Andrew Cooper @ 2025-03-03 19:19 UTC (permalink / raw)
To: Stewart Hildebrand, xen-devel
Cc: Jan Beulich, Roger Pau Monné, Oleksii Kurochko
On 03/03/2025 6:59 pm, Stewart Hildebrand wrote:
> On 2/13/25 20: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'
> <snip>
>
>> 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.
> This is the only one that I'm aware of, and it only occurs when
> CONFIG_HVM=n. I only happened to stumble upon this occurrence because
> allyesconfig apparently results in CONFIG_HVM=n.
Yes. That's a know misbehaviour that doesn't have a resolution that
everyone is happy with.
> I've been building Xen
> with -Og in my development workflow for a couple of releases now.
We really ought to be using -Og rather than -O1 when available.
If you fix this bug, then change Xen's default, randconfig in CI will
keep it working.
~Andrew
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xen/x86: fix build with CONFIG_HVM=n and -Og
2025-02-14 8:25 ` Jan Beulich
@ 2025-03-03 21:18 ` Stewart Hildebrand
0 siblings, 0 replies; 7+ messages in thread
From: Stewart Hildebrand @ 2025-03-03 21:18 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper
Cc: Roger Pau Monné, Oleksii Kurochko, xen-devel
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@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.
I'll give this a try for v2
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-03-03 21:19 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-13 18:50 [PATCH] xen/x86: fix build with CONFIG_HVM=n and -Og Stewart Hildebrand
2025-02-13 20:17 ` Oleksii Kurochko
2025-02-14 1:05 ` Andrew Cooper
2025-02-14 8:25 ` Jan Beulich
2025-03-03 21:18 ` Stewart Hildebrand
2025-03-03 18:59 ` Stewart Hildebrand
2025-03-03 19:19 ` Andrew Cooper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.