The inhibition status of APICv can currently be checked using the
'kvm_apicv_inhibit_changed' tracepoint, but this is not accessible if
tracefs is not available (e.g. kernel lockdown, non-root user). Export
inhibition status as a binary stat that can be monitored from userspace
without elevated privileges.
Signed-off-by: Alejandro Jimenez <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 1 +
arch/x86/kvm/x86.c | 10 +++++++++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index ad5319a503f0..9b960a523715 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1524,6 +1524,7 @@ struct kvm_vm_stat {
u64 nx_lpage_splits;
u64 max_mmu_page_hash_collisions;
u64 max_mmu_rmap_size;
+ u64 apicv_inhibited;
};
struct kvm_vcpu_stat {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b66c45e7f6f8..f7f598f066e7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -255,7 +255,8 @@ const struct _kvm_stats_desc kvm_vm_stats_desc[] = {
STATS_DESC_ICOUNTER(VM, pages_1g),
STATS_DESC_ICOUNTER(VM, nx_lpage_splits),
STATS_DESC_PCOUNTER(VM, max_mmu_rmap_size),
- STATS_DESC_PCOUNTER(VM, max_mmu_page_hash_collisions)
+ STATS_DESC_PCOUNTER(VM, max_mmu_page_hash_collisions),
+ STATS_DESC_IBOOLEAN(VM, apicv_inhibited)
};
const struct kvm_stats_header kvm_vm_stats_header = {
@@ -10588,6 +10589,13 @@ void __kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
*/
kvm_make_all_cpus_request(kvm, KVM_REQ_APICV_UPDATE);
kvm->arch.apicv_inhibit_reasons = new;
+
+ /*
+ * Update inhibition statistic only when toggling APICv
+ * activation status.
+ */
+ kvm->stat.apicv_inhibited = !!new;
+
if (new) {
unsigned long gfn = gpa_to_gfn(APIC_DEFAULT_PHYS_BASE);
int idx = srcu_read_lock(&kvm->srcu);
--
2.39.3
Hi
On 2/15/2024 11:01 PM, Alejandro Jimenez wrote:
> The inhibition status of APICv can currently be checked using the
> 'kvm_apicv_inhibit_changed' tracepoint, but this is not accessible if
> tracefs is not available (e.g. kernel lockdown, non-root user). Export
> inhibition status as a binary stat that can be monitored from userspace
> without elevated privileges.
>
> Signed-off-by: Alejandro Jimenez <[email protected]>
> ---
> arch/x86/include/asm/kvm_host.h | 1 +
> arch/x86/kvm/x86.c | 10 +++++++++-
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index ad5319a503f0..9b960a523715 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1524,6 +1524,7 @@ struct kvm_vm_stat {
> u64 nx_lpage_splits;
> u64 max_mmu_page_hash_collisions;
> u64 max_mmu_rmap_size;
> + u64 apicv_inhibited;
> };
>
> struct kvm_vcpu_stat {
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b66c45e7f6f8..f7f598f066e7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -255,7 +255,8 @@ const struct _kvm_stats_desc kvm_vm_stats_desc[] = {
> STATS_DESC_ICOUNTER(VM, pages_1g),
> STATS_DESC_ICOUNTER(VM, nx_lpage_splits),
> STATS_DESC_PCOUNTER(VM, max_mmu_rmap_size),
> - STATS_DESC_PCOUNTER(VM, max_mmu_page_hash_collisions)
> + STATS_DESC_PCOUNTER(VM, max_mmu_page_hash_collisions),
> + STATS_DESC_IBOOLEAN(VM, apicv_inhibited)
One use for this stats would be to help:
1. Determine if vm and/or vcpu is inhibited.
2. Determine the inhibit reason.
Therefore can we use STATS_DESC_ICOUNTER() instead of
STATS_DESC_IBOOLEAN(), and show the inhibit flag from
vm_reason = struct kvm->arch.apicv_inhibit_reasons
vcpu_reason = static_call(kvm_x86_vcpu_get_apicv_inhibit_reasons)(vcpu)
See kvm_vcpu_apicv_activated().
Thanks,
Suravee
> };
>
> const struct kvm_stats_header kvm_vm_stats_header = {
> @@ -10588,6 +10589,13 @@ void __kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
> */
> kvm_make_all_cpus_request(kvm, KVM_REQ_APICV_UPDATE);
> kvm->arch.apicv_inhibit_reasons = new;
> +
> + /*
> + * Update inhibition statistic only when toggling APICv
> + * activation status.
> + */
> + kvm->stat.apicv_inhibited = !!new;
> +
> if (new) {
> unsigned long gfn = gpa_to_gfn(APIC_DEFAULT_PHYS_BASE);
> int idx = srcu_read_lock(&kvm->srcu);
On Thu, Feb 15, 2024, Alejandro Jimenez wrote:
> The inhibition status of APICv can currently be checked using the
> 'kvm_apicv_inhibit_changed' tracepoint, but this is not accessible if
> tracefs is not available (e.g. kernel lockdown, non-root user). Export
> inhibition status as a binary stat that can be monitored from userspace
> without elevated privileges.
>
> Signed-off-by: Alejandro Jimenez <[email protected]>
> ---
> arch/x86/include/asm/kvm_host.h | 1 +
> arch/x86/kvm/x86.c | 10 +++++++++-
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index ad5319a503f0..9b960a523715 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1524,6 +1524,7 @@ struct kvm_vm_stat {
> u64 nx_lpage_splits;
> u64 max_mmu_page_hash_collisions;
> u64 max_mmu_rmap_size;
> + u64 apicv_inhibited;
Tracking the negative is odd, i.e. if we add a stat, KVM should probably track
if APICv is fully enabled, not if it's inhibited.
This also should be a boolean, not a u64. Precisely enumerating _why_ APICv is
inhibited is firmly in debug territory, i.e. not in scope for "official" stats.
Oh, and this should be a per-vCPU stat, not a VM-wide stat.
As for whether or not we should add a stat for this, I'm leaning towards "yes".
APICv can have such a profound impact on performance (and functionality) that
definitively knowing that it's enabled seems justified.
On Tue, Apr 16, 2024 at 8:19 PM Sean Christopherson <[email protected]> wrote:
>
> > + u64 apicv_inhibited;
>
> Tracking the negative is odd, i.e. if we add a stat, KVM should probably track
> if APICv is fully enabled, not if it's inhibited.
>
> This also should be a boolean, not a u64. Precisely enumerating _why_ APICv is
> inhibited is firmly in debug territory, i.e. not in scope for "official" stats.
It is a boolean, but stats are all u64 in the file and the contents of
the file map the stats struct directly. See for example the existing
'u64 guest_mode".
Paolo
On 4/16/24 14:19, Sean Christopherson wrote:
> On Thu, Feb 15, 2024, Alejandro Jimenez wrote:
>> The inhibition status of APICv can currently be checked using the
>> 'kvm_apicv_inhibit_changed' tracepoint, but this is not accessible if
>> tracefs is not available (e.g. kernel lockdown, non-root user). Export
>> inhibition status as a binary stat that can be monitored from userspace
>> without elevated privileges.
>>
>> Signed-off-by: Alejandro Jimenez <[email protected]>
>> ---
>> arch/x86/include/asm/kvm_host.h | 1 +
>> arch/x86/kvm/x86.c | 10 +++++++++-
>> 2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index ad5319a503f0..9b960a523715 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1524,6 +1524,7 @@ struct kvm_vm_stat {
>> u64 nx_lpage_splits;
>> u64 max_mmu_page_hash_collisions;
>> u64 max_mmu_rmap_size;
>> + u64 apicv_inhibited;
I was about to send v2 based on the earlier feedback. I think the changes would
partially address your comments, but there are still wrinkles. In short, I ended
up with:
per-vCPU:
apicv_active (bool) --> tracks vcpu apic.apicv_active
per-VM:
apicv_inhibited (u64) --> exposes kvm apicv_inhibit_reasons
>
> Tracking the negative is odd, i.e. if we add a stat, KVM should probably track
> if APICv is fully enabled, not if it's inhibited>
> This also should be a boolean, not a u64. Precisely enumerating _why_ APICv is
> inhibited is firmly in debug territory, i.e. not in scope for "official" stats.
From that perspective I am perhaps stretching the stats official purpose,
by exposing "too much info", showing _why_ APICv is inhibited (i.e. new
VM-wide apicv_inhibited).
It is true that I am approaching this with a "debugging" bias, but _if_ we do
expose a stat related to inhibition state, I don't think it would be overloading
its meaning to encode relevant inhibit reason information on it.
It would be need to be documented, of course, which is something I can do once
we reach an agreement.
>
> Oh, and this should be a per-vCPU stat, not a VM-wide stat.
>
> As for whether or not we should add a stat for this, I'm leaning towards "yes".
> APICv can have such a profound impact on performance (and functionality) that
> definitively knowing that it's enabled seems justified.
The new per-vCPU apicv_active stat fills this role.
I see you don't agree with a separate stat exposing VM-wide inhibits due to scope
and ABI restrictions mentioned here and in the cover letter reply. I follow the
argument, but it also seems like we'd be handicapping this interface by not
providing inhibit reasons along with it, since they are essential in determining
whether or not AVIC in particular is working (again my debugging bias). I am aware
this is a slight overloading (hopefully not abuse) of the stats purpose, and so
it might not be acceptable.
Alejandro