Joao Martins <joao.m.martins@xxxxxxxxxx> writes: > On 30/04/2024 19:37, Ankur Arora wrote: >> From: Joao Martins <joao.m.martins@xxxxxxxxxx> >> >> Right now kvm_para_has_hint(KVM_HINTS_REALTIME) is x86 only. In >> pursuit of making cpuidle-haltpoll architecture independent, define >> arch_haltpoll_supported() which handles the architectural check for >> enabling haltpoll. >> >> Move the (kvm_para_available() && kvm_para_has_hint(KVM_HINTS_REALTIME)) >> check to the x86 specific arch_haltpoll_supported(). >> >> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx> >> Signed-off-by: Mihai Carabas <mihai.carabas@xxxxxxxxxx> >> Signed-off-by: Ankur Arora <ankur.a.arora@xxxxxxxxxx> >> >> --- >> Changelog: >> >> - s/arch_haltpoll_want/arch_haltpoll_supported/ > > > I am not sure it's correct to call supported() considering that it's supposed to > always supported (via WFE or cpu_relax()) and it's not exactly what it is doing. > The function you were changing is more about whether it's default enabled or > not. So I think the old name in v4 is more appropriate i.e. arch_haltpoll_want() > > Alternatively you could have it called arch_haltpoll_default_enabled() though > it's longer/verbose. So, I thought about it some and the driver loading decision tree should be: 1. bail out based on the value of boot_option_idle_override. 2. if arch_haltpoll_supported(), enable haltpoll 3. if cpuidle-haltpoll.force=1, enable haltpoll, Note: in the posted versions, cpuidle-haltpoll.force is allowed to override boot_option_idle_override, which is wrong. With that fixed the x86 check should be: bool arch_haltpoll_supported(void) { return kvm_para_available() && kvm_para_has_hint(KVM_HINTS_REALTIME); } and arm64: static inline bool arch_haltpoll_supported(void) { /* * Ensure the event stream is available to provide a terminating * condition to the WFE in the poll loop. */ return arch_timer_evtstrm_available(); } Now, both of these fit reasonably well with arch_haltpoll_supported(). My personal preference for that is because it seems to me that the architecture code should just deal with mechanism and not policy. However, as you imply arch_haltpoll_supported() is a more loaded name and given that the KVM side of arm64 haltpoll is not done yet, it's best to have a more neutral label like arch_haltpoll_want() or arch_haltpoll_do_enable(). > Though if you want a true supported() arch helper *I think* you need to make a > bigger change into introducing arch_haltpoll_supported() separate from > arch_haltpoll_want() where the former would ignore the .force=y modparam and > never be able to load if a given feature wasn't present e.g. prevent arm64 > haltpoll loading be conditioned to arch_timer_evtstrm_available() being present. > > Though I don't think that you want this AIUI Yeah I don't. I think the cpuidle-haltpoll.force=1, should be allowed to override arch_haltpoll_supported(), so long as smp_cond_load_relaxed() is well defined (as it is here). It shouldn't, however, override the user's choice of boot_option_idle_override. -- ankur