| Message ID | 20250313034524.3069690-33-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | accel/tcg, codebase: Build once patches | expand |
On 3/12/25 20:45, Richard Henderson wrote: > We were hiding a number of declarations from user-only, > although it hurts nothing to allow them. The inlines > for user-only are unused. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/hw/intc/armv7m_nvic.h | 14 -------------- > 1 file changed, 14 deletions(-) > > diff --git a/include/hw/intc/armv7m_nvic.h b/include/hw/intc/armv7m_nvic.h > index 89fe8aedaa..7b9964fe7e 100644 > --- a/include/hw/intc/armv7m_nvic.h > +++ b/include/hw/intc/armv7m_nvic.h > @@ -189,21 +189,7 @@ int armv7m_nvic_raw_execution_priority(NVICState *s); > * @secure: the security state to test > * This corresponds to the pseudocode IsReqExecPriNeg(). > */ > -#ifndef CONFIG_USER_ONLY > bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure); > -#else > -static inline bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure) > -{ > - return false; > -} > -#endif > -#ifndef CONFIG_USER_ONLY > bool armv7m_nvic_can_take_pending_exception(NVICState *s); > -#else > -static inline bool armv7m_nvic_can_take_pending_exception(NVICState *s) > -{ > - return true; > -} > -#endif > > #endif I'm a bit worried we might have regression when doing things this way. Maybe we could have a runtime check: config_user_mode() config_system_mode() And assert we are only in system.
On 3/13/25 14:00, Pierrick Bouvier wrote: >> diff --git a/include/hw/intc/armv7m_nvic.h b/include/hw/intc/armv7m_nvic.h >> index 89fe8aedaa..7b9964fe7e 100644 >> --- a/include/hw/intc/armv7m_nvic.h >> +++ b/include/hw/intc/armv7m_nvic.h >> @@ -189,21 +189,7 @@ int armv7m_nvic_raw_execution_priority(NVICState *s); >> * @secure: the security state to test >> * This corresponds to the pseudocode IsReqExecPriNeg(). >> */ >> -#ifndef CONFIG_USER_ONLY >> bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure); >> -#else >> -static inline bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure) >> -{ >> - return false; >> -} >> -#endif >> -#ifndef CONFIG_USER_ONLY >> bool armv7m_nvic_can_take_pending_exception(NVICState *s); >> -#else >> -static inline bool armv7m_nvic_can_take_pending_exception(NVICState *s) >> -{ >> - return true; >> -} >> -#endif >> #endif > > I'm a bit worried we might have regression when doing things this way. I expect link errors to diagnose errors. r~
On 3/14/25 11:13, Richard Henderson wrote: > On 3/13/25 14:00, Pierrick Bouvier wrote: >>> diff --git a/include/hw/intc/armv7m_nvic.h b/include/hw/intc/armv7m_nvic.h >>> index 89fe8aedaa..7b9964fe7e 100644 >>> --- a/include/hw/intc/armv7m_nvic.h >>> +++ b/include/hw/intc/armv7m_nvic.h >>> @@ -189,21 +189,7 @@ int armv7m_nvic_raw_execution_priority(NVICState *s); >>> * @secure: the security state to test >>> * This corresponds to the pseudocode IsReqExecPriNeg(). >>> */ >>> -#ifndef CONFIG_USER_ONLY >>> bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure); >>> -#else >>> -static inline bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure) >>> -{ >>> - return false; >>> -} >>> -#endif >>> -#ifndef CONFIG_USER_ONLY >>> bool armv7m_nvic_can_take_pending_exception(NVICState *s); >>> -#else >>> -static inline bool armv7m_nvic_can_take_pending_exception(NVICState *s) >>> -{ >>> - return true; >>> -} >>> -#endif >>> #endif >> >> I'm a bit worried we might have regression when doing things this way. > > I expect link errors to diagnose errors. > In this case, yes. More generally, for system vs user, it might be sufficient (even though I would prefer to be blindly prudent on this), but it might not protect all cases, with subtle configs or features enabled/disabled. With a runtime check, we could identify the missing calls "feature_enabled()". In this case, we would link, but could end up call_feature in some cases, when it should be hidden behind a "_enabled()" check. if (feature_enabled()) { call_feature(); } Stubs are really a dependency hack to palliate architecture problems. > > r~
On 3/14/25 11:36, Pierrick Bouvier wrote: > On 3/14/25 11:13, Richard Henderson wrote: >> On 3/13/25 14:00, Pierrick Bouvier wrote: >>>> diff --git a/include/hw/intc/armv7m_nvic.h b/include/hw/intc/armv7m_nvic.h >>>> index 89fe8aedaa..7b9964fe7e 100644 >>>> --- a/include/hw/intc/armv7m_nvic.h >>>> +++ b/include/hw/intc/armv7m_nvic.h >>>> @@ -189,21 +189,7 @@ int armv7m_nvic_raw_execution_priority(NVICState *s); >>>> * @secure: the security state to test >>>> * This corresponds to the pseudocode IsReqExecPriNeg(). >>>> */ >>>> -#ifndef CONFIG_USER_ONLY >>>> bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure); >>>> -#else >>>> -static inline bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure) >>>> -{ >>>> - return false; >>>> -} >>>> -#endif >>>> -#ifndef CONFIG_USER_ONLY >>>> bool armv7m_nvic_can_take_pending_exception(NVICState *s); >>>> -#else >>>> -static inline bool armv7m_nvic_can_take_pending_exception(NVICState *s) >>>> -{ >>>> - return true; >>>> -} >>>> -#endif >>>> #endif >>> >>> I'm a bit worried we might have regression when doing things this way. >> >> I expect link errors to diagnose errors. >> > > In this case, yes. > > More generally, for system vs user, it might be sufficient (even though I would prefer to > be blindly prudent on this), but it might not protect all cases, with subtle configs or > features enabled/disabled. > > With a runtime check, we could identify the missing calls "feature_enabled()". In this > case, we would link, but could end up call_feature in some cases, when it should be hidden > behind a "_enabled()" check. > > if (feature_enabled()) { > call_feature(); > } I'm not quite sure what you're arguing for here. A build-time error is vastly preferable to a run-time error. If it's a lesser used configuration or feature, a run-time error could lay dormant for years before a user encounters it. r~
On 3/14/25 13:03, Richard Henderson wrote: > On 3/14/25 11:36, Pierrick Bouvier wrote: >> On 3/14/25 11:13, Richard Henderson wrote: >>> On 3/13/25 14:00, Pierrick Bouvier wrote: >>>>> diff --git a/include/hw/intc/armv7m_nvic.h b/include/hw/intc/armv7m_nvic.h >>>>> index 89fe8aedaa..7b9964fe7e 100644 >>>>> --- a/include/hw/intc/armv7m_nvic.h >>>>> +++ b/include/hw/intc/armv7m_nvic.h >>>>> @@ -189,21 +189,7 @@ int armv7m_nvic_raw_execution_priority(NVICState *s); >>>>> * @secure: the security state to test >>>>> * This corresponds to the pseudocode IsReqExecPriNeg(). >>>>> */ >>>>> -#ifndef CONFIG_USER_ONLY >>>>> bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure); >>>>> -#else >>>>> -static inline bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure) >>>>> -{ >>>>> - return false; >>>>> -} >>>>> -#endif >>>>> -#ifndef CONFIG_USER_ONLY >>>>> bool armv7m_nvic_can_take_pending_exception(NVICState *s); >>>>> -#else >>>>> -static inline bool armv7m_nvic_can_take_pending_exception(NVICState *s) >>>>> -{ >>>>> - return true; >>>>> -} >>>>> -#endif >>>>> #endif >>>> >>>> I'm a bit worried we might have regression when doing things this way. >>> >>> I expect link errors to diagnose errors. >>> >> >> In this case, yes. >> >> More generally, for system vs user, it might be sufficient (even though I would prefer to >> be blindly prudent on this), but it might not protect all cases, with subtle configs or >> features enabled/disabled. >> >> With a runtime check, we could identify the missing calls "feature_enabled()". In this >> case, we would link, but could end up call_feature in some cases, when it should be hidden >> behind a "_enabled()" check. >> >> if (feature_enabled()) { >> call_feature(); >> } > > I'm not quite sure what you're arguing for here. > A build-time error is vastly preferable to a run-time error. > Even though this specific patch is safe (code calling those functions should be under system anyway, so it should not be linked in a user binary), I just wonder if we should not add explicit checks for this, for other kind of replacement we'll have to do. > If it's a lesser used configuration or feature, a run-time error could lay dormant for > years before a user encounters it. > Sure, but wouldn't it better to have an explicit assert, instead of observing a random behaviour? I'm just worried we end up calling something we should not (user vs system, or any other ifdef CONFIG/TARGET that might be hidden somewhere), and silently return a wrong value, which would not be covered by our test suite. > > r~
On 3/14/25 13:34, Pierrick Bouvier wrote: > On 3/14/25 13:03, Richard Henderson wrote: >> I'm not quite sure what you're arguing for here. >> A build-time error is vastly preferable to a run-time error. >> > > Even though this specific patch is safe (code calling those functions should be under > system anyway, so it should not be linked in a user binary), I just wonder if we should > not add explicit checks for this, for other kind of replacement we'll have to do. Any such runtime check should not be for "system" vs "user", but for "feature enabled". >> If it's a lesser used configuration or feature, a run-time error could lay dormant for >> years before a user encounters it. >> > > Sure, but wouldn't it better to have an explicit assert, instead of observing a random > behaviour? What random behaviour are you suggesting? > I'm just worried we end up calling something we should not (user vs system, or any other > ifdef CONFIG/TARGET that might be hidden somewhere), and silently return a wrong value, > which would not be covered by our test suite. Where is the wrong value going to be returned from, the stub? Yes, many stubs do abort, typically after the "enabled" stub returns false. It's still best if "feature enabled" is compile-time false when possible, such that everything after the feature check gets compiled out. At which point we don't need the stubs: they won't be reachable and errors are caught at build-time. r~
On 3/14/25 13:59, Richard Henderson wrote: > On 3/14/25 13:34, Pierrick Bouvier wrote: >> On 3/14/25 13:03, Richard Henderson wrote: >>> I'm not quite sure what you're arguing for here. >>> A build-time error is vastly preferable to a run-time error. >>> >> >> Even though this specific patch is safe (code calling those functions should be under >> system anyway, so it should not be linked in a user binary), I just wonder if we should >> not add explicit checks for this, for other kind of replacement we'll have to do. > > Any such runtime check should not be for "system" vs "user", but for "feature enabled". > From our build system point of view, I don't really see what is the difference. That's part of why I insisted previously on cleaning user vs system ifdefs at the same time we make files common, but the answer I had was "We don't need to do that now", so I stopped arguing. When building user vs system, we use ifdef to detect this case, and we select different implementations and include a specific set of source files, the same way we do for some features, or specific targets. So, why should it be treated differently, or later? >>> If it's a lesser used configuration or feature, a run-time error could lay dormant for >>> years before a user encounters it. >>> >> >> Sure, but wouldn't it better to have an explicit assert, instead of observing a random >> behaviour? > > What random behaviour are you suggesting? > In case we return a stub value by accident, when you expect to have a side effect done somewhere. And that it would not result in an immediate crash, but later at a random place. Maybe the case just does not exist, but my point was that it does not cost anything to add an assert just in case. >> I'm just worried we end up calling something we should not (user vs system, or any other >> ifdef CONFIG/TARGET that might be hidden somewhere), and silently return a wrong value, >> which would not be covered by our test suite. > > Where is the wrong value going to be returned from, the stub? > Yes, many stubs do abort, typically after the "enabled" stub returns false. > Many, but not all of them I guess. > It's still best if "feature enabled" is compile-time false when possible, such that > everything after the feature check gets compiled out. At which point we don't need the > stubs: they won't be reachable and errors are caught at build-time. > Sure, but as we saw, it's not always possible, because some calls are under: if (X_enabled()) {...do_X()...}, which requires to be able to link do_X even though it will be dead code at runtime. Maybe my concern is unfounded, but seeing some of the inline stubs and the ifdefs associated, I hope we won't miss a corner case. > > r~
diff --git a/include/hw/intc/armv7m_nvic.h b/include/hw/intc/armv7m_nvic.h index 89fe8aedaa..7b9964fe7e 100644 --- a/include/hw/intc/armv7m_nvic.h +++ b/include/hw/intc/armv7m_nvic.h @@ -189,21 +189,7 @@ int armv7m_nvic_raw_execution_priority(NVICState *s); * @secure: the security state to test * This corresponds to the pseudocode IsReqExecPriNeg(). */ -#ifndef CONFIG_USER_ONLY bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure); -#else -static inline bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure) -{ - return false; -} -#endif -#ifndef CONFIG_USER_ONLY bool armv7m_nvic_can_take_pending_exception(NVICState *s); -#else -static inline bool armv7m_nvic_can_take_pending_exception(NVICState *s) -{ - return true; -} -#endif #endif
We were hiding a number of declarations from user-only, although it hurts nothing to allow them. The inlines for user-only are unused. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- include/hw/intc/armv7m_nvic.h | 14 -------------- 1 file changed, 14 deletions(-)