On 18.07.2025 10:19, Naveen N Rao wrote:
On Mon, Jul 07, 2025 at 04:21:53PM -0700, Sean Christopherson wrote:
On Fri, Jun 27, 2025, Naveen N Rao wrote:
Back when I implemented this, I just wanted to be a bit safer, a bit more explicit that
this uses an undocumented feature.
It doesn't matter much though.
I don't see any reason to do major surgery, just give "avic" auto -1/0/1 behavior:
I am wary of breaking existing users/deployments on Zen4/Zen5 enabling
AVIC by specifying avic=on, or avic=true today. That's primarily the
reason I chose not to change 'avic' into an integer. Also, post module
load, sysfs reports the value for 'avic' as a 'Y' or 'N' today. So if
there are scripts relying on that, those will break if we change 'avic'
into an integer.
That's easy enough to handle, e.g. see nx_huge_pages_ops for a very similar case
where KVM has "auto" behavior (and a "never" case too), but otherwise treats the
param like a bool.
Nice! Looks like I can re-use existing callbacks for this too:
static const struct kernel_param_ops avic_ops = {
.flags = KERNEL_PARAM_OPS_FL_NOARG,
.set = param_set_bint,
.get = param_get_bool,
};
/* enable/disable AVIC (-1 = auto) */
int avic = -1;
module_param_cb(avic, &avic_ops, &avic, 0444);
__MODULE_PARM_TYPE(avic, "bool");
For Zen1/Zen2, as I mentioned, it is unlikely that anyone today is
enabling AVIC and expecting it to work since the workaround is only just
hitting upstream. So, I'm hoping requiring force_avic=1 should be ok
with the taint removed.
But if that's the motivation, changing the semantics of force_avic doesn't make
any sense. Once the workaround lands, the only reason for force_avic to exist
is to allow forcing KVM to enable AVIC even when it's not supported.
Indeed.
Longer term, once we get wider testing with the workaround on Zen1/Zen2,
we can consider relaxing the need for force_avic, at which point AVIC
can be default enabled
I don't see why the default value for "avic" needs to be tied to force_avic.
If we're not confident that AVIC is 100% functional and a net positive for the
vast majority of setups/workloads on Zen1/Zen2, then simply leave "avic" off by
default for those CPUs. If we ever want to enable AVIC by default across the
board, we can simply change the default value of "avic".
But to be honest, I don't see any reason to bother trying to enable AVIC by default
for Zen1/Zen2. There's a very real risk that doing so would regress existing users
that have been running setups for ~6 years, and we can't fudge around AVIC being
hidden on Zen3 (and the IOMMU not supporting it at all), i.e. enabling AVIC by
default only for Zen4+ provides a cleaner story for end users.
Works for me. I completely agree with that.
Some Zen3 platforms (at least Ryzen SKUs) *do* enumerate AVIC in CPUID:
$ cat /proc/cpuinfo | grep -E '(model[[:space:]]{2,})|family|step' | head -n3
cpu family : 25
model : 33
stepping : 2
$ cat /proc/cpuinfo | grep avic | head -n1
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov
pat pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext fxsr_opt pdpe1gb
rdtscp lm constant_tsc rep_good nopl xtopology nonstop_tsc cpuid extd_apicid
aperfmperf rapl pni pclmulqdq monitor ssse3 fma cx16 sse4_1 sse4_2 x2apic movbe
popcnt aes xsave avx f16c rdrand lahf_lm cmp_legacy svm extapic cr8_legacy
abm sse4a misalignsse 3dnowprefetch osvw ibs skinit wdt tce topoext perfctr_core
perfctr_nb bpext perfctr_llc mwaitx cpb cat_l3 cdp_l3 hw_pstate ssbd mba ibrs
ibpb stibp vmmcall fsgsbase bmi1 avx2 smep bmi2 erms invpcid cqm rdt_a rdseed
adx smap clflushopt clwb sha_ni xsaveopt xsavec xgetbv1 xsaves cqm_llc
cqm_occup_llc cqm_mbm_total cqm_mbm_local user_shstk clzero irperf xsaveerptr
rdpru wbnoinvd arat npt lbrv svm_lock nrip_save tsc_scale vmcb_clean flushbyasid
decodeassists pausefilter pfthreshold -> avic <-
v_vmsave_vmload vgif v_spec_ctrl umip pku ospke vaes vpclmulqdq rdpid overflow_recov
succor smca fsrm debug_swap
$ cat /sys/module/kvm_amd/parameters/force_avic
N
$ dmesg | grep AVIC
kvm_amd: AVIC enabled
As you can see, currently AVIC works there even without force_avic=1 so why
now hide it behind that parameter if errata #1235 is supposedly not present
on Zen3?
Also, this platform is apparently confident enough that the AVIC silicon is
working correctly there to expose it in CPUID - maybe because that's CPU
stepping 2 instead of the initial 0?
Thanks,
Naveen
Thanks,
Maciej
