Skip to content

Commit

Permalink
x86/kvm: Use separate percpu variable to track the enabling of asyncpf
Browse files Browse the repository at this point in the history
Refer to commit fd10cde ("KVM paravirt: Add async PF initialization
to PV guest") and commit 344d958 ("KVM: Add PV MSR to enable
asynchronous page faults delivery"). It turns out that at the time when
asyncpf was introduced, the purpose was defining the shared PV data 'struct
kvm_vcpu_pv_apf_data' with the size of 64 bytes. However, it made a mistake
and defined the size to 68 bytes, which failed to make fit in a cache line
and made the code inconsistent with the documentation.

Below justification quoted from Sean[*]

  KVM (the host side) has *never* read kvm_vcpu_pv_apf_data.enabled, and
  the documentation clearly states that enabling is based solely on the
  bit in the synthetic MSR.

  So rather than update the documentation, fix the goof by removing the
  enabled filed and use the separate percpu variable instread.
  KVM-as-a-host obviously doesn't enforce anything or consume the size,
  and changing the header will only affect guests that are rebuilt against
  the new header, so there's no chance of ABI breakage between KVM and its
  guests. The only possible breakage is if some other hypervisor is
  emulating KVM's async #PF (LOL) and relies on the guest to set
  kvm_vcpu_pv_apf_data.enabled. But (a) I highly doubt such a hypervisor
  exists, (b) that would arguably be a violation of KVM's "spec", and
  (c) the worst case scenario is that the guest would simply lose async
  #PF functionality.

[*] https://lore.kernel.org/all/ZS7ERnnRqs8Fl0ZF@google.com/T/#u

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Link: https://lore.kernel.org/r/20231025055914.1201792-2-xiaoyao.li@intel.com
[sean: use true/false instead of 1/0 for booleans]
Signed-off-by: Sean Christopherson <seanjc@google.com>
  • Loading branch information
Xiaoyao Li authored and Sean Christopherson committed Feb 6, 2024
1 parent 41bccc9 commit ccb2280
Showing 3 changed files with 6 additions and 7 deletions.
1 change: 0 additions & 1 deletion Documentation/virt/kvm/x86/msr.rst
Original file line number Diff line number Diff line change
@@ -204,7 +204,6 @@ data:
__u32 token;

__u8 pad[56];
__u32 enabled;
};

Bits 5-4 of the MSR are reserved and should be zero. Bit 0 is set to 1
1 change: 0 additions & 1 deletion arch/x86/include/uapi/asm/kvm_para.h
Original file line number Diff line number Diff line change
@@ -142,7 +142,6 @@ struct kvm_vcpu_pv_apf_data {
__u32 token;

__u8 pad[56];
__u32 enabled;
};

#define KVM_PV_EOI_BIT 0
11 changes: 6 additions & 5 deletions arch/x86/kernel/kvm.c
Original file line number Diff line number Diff line change
@@ -65,6 +65,7 @@ static int __init parse_no_stealacc(char *arg)

early_param("no-steal-acc", parse_no_stealacc);

static DEFINE_PER_CPU_READ_MOSTLY(bool, async_pf_enabled);
static DEFINE_PER_CPU_DECRYPTED(struct kvm_vcpu_pv_apf_data, apf_reason) __aligned(64);
DEFINE_PER_CPU_DECRYPTED(struct kvm_steal_time, steal_time) __aligned(64) __visible;
static int has_steal_clock = 0;
@@ -244,7 +245,7 @@ noinstr u32 kvm_read_and_reset_apf_flags(void)
{
u32 flags = 0;

if (__this_cpu_read(apf_reason.enabled)) {
if (__this_cpu_read(async_pf_enabled)) {
flags = __this_cpu_read(apf_reason.flags);
__this_cpu_write(apf_reason.flags, 0);
}
@@ -295,7 +296,7 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_kvm_asyncpf_interrupt)

inc_irq_stat(irq_hv_callback_count);

if (__this_cpu_read(apf_reason.enabled)) {
if (__this_cpu_read(async_pf_enabled)) {
token = __this_cpu_read(apf_reason.token);
kvm_async_pf_task_wake(token);
__this_cpu_write(apf_reason.token, 0);
@@ -362,7 +363,7 @@ static void kvm_guest_cpu_init(void)
wrmsrl(MSR_KVM_ASYNC_PF_INT, HYPERVISOR_CALLBACK_VECTOR);

wrmsrl(MSR_KVM_ASYNC_PF_EN, pa);
__this_cpu_write(apf_reason.enabled, 1);
__this_cpu_write(async_pf_enabled, true);
pr_debug("setup async PF for cpu %d\n", smp_processor_id());
}

@@ -383,11 +384,11 @@ static void kvm_guest_cpu_init(void)

static void kvm_pv_disable_apf(void)
{
if (!__this_cpu_read(apf_reason.enabled))
if (!__this_cpu_read(async_pf_enabled))
return;

wrmsrl(MSR_KVM_ASYNC_PF_EN, 0);
__this_cpu_write(apf_reason.enabled, 0);
__this_cpu_write(async_pf_enabled, false);

pr_debug("disable async PF for cpu %d\n", smp_processor_id());
}

0 comments on commit ccb2280

Please sign in to comment.