| Message ID | 6150a0a8c3d911c6c2ada23c0b9c8b35991bd235.camel@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] KVM: x86/xen: Inject vCPU upcall vector when local APIC is enabled | expand |
On Tue, Jan 16, 2024, David Woodhouse wrote: > From: David Woodhouse <dwmw@amazon.co.uk> > > Linux guests since commit b1c3497e604d ("x86/xen: Add support for > HVMOP_set_evtchn_upcall_vector") in v6.0 onwards will use the per-vCPU > upcall vector when it's advertised in the Xen CPUID leaves. > > This upcall is injected through the guest's local APIC as an MSI, unlike > the older system vector which was merely injected by the hypervisor any > time the CPU was able to receive an interrupt and the upcall_pending > flags is set in its vcpu_info. > > Effectively, that makes the per-CPU upcall edge triggered instead of > level triggered, which results in the upcall being lost if the MSI is > delivered when the local APIC is *disabled*. > > Xen checks the vcpu_info->evtchn_upcall_pending flag when the local APIC > for a vCPU is software enabled (in fact, on any write to the SPIV > register which doesn't disable the APIC). Do the same in KVM since KVM > doesn't provide a way for userspace to intervene and trap accesses to > the SPIV register of a local APIC emulated by KVM. > > Astute reviewers may note that kvm_xen_inject_vcpu_vector() function has > a WARN_ON_ONCE() in the case where kvm_irq_delivery_to_apic_fast() fails > and returns false. In the case where the MSI is not delivered due to the > local APIC being disabled, kvm_irq_delivery_to_apic_fast() still returns > true but the value in *r is zero. So the WARN_ON_ONCE() remains correct, > as that case should still never happen. > > Fixes: fde0451be8fb3 ("KVM: x86/xen: Support per-vCPU event channel upcall via local APIC") > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > Reviewed-by: Paul Durrant <paul@xen.org> > Cc: stable@vger.kernel.org > --- > v4: Reword commit message, > rename kvm_xen_enable_lapic() → kvm_xen_sw_enable_lapic(). > v3: Repost, add Cc:stable. > v2: Add Fixes: tag. > > arch/x86/kvm/lapic.c | 5 ++++- > arch/x86/kvm/xen.c | 2 +- > arch/x86/kvm/xen.h | 18 ++++++++++++++++++ > 3 files changed, 23 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 3242f3da2457..75bc7d3f0022 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -41,6 +41,7 @@ > #include "ioapic.h" > #include "trace.h" > #include "x86.h" > +#include "xen.h" > #include "cpuid.h" > #include "hyperv.h" > #include "smm.h" Patch is corrupt. git am /home/seanjc/patches/v4_20240116_dwmw2_kvm_x86_xen_inject_vcpu_upcall_vector_when_local_apic_is_enabled.mbx Applying: KVM: x86/xen: Inject vCPU upcall vector when local APIC is enabled error: corrupt patch at line 17 cat ~/patches/v4_20240116_dwmw2_kvm_x86_xen_inject_vcpu_upcall_vector_when_local_apic_is_enabled.mbx | patch -p 1 --merge patching file arch/x86/kvm/lapic.c patch: **** malformed patch at line 59: #include "ioapic.h" Based on what I see in a web view, I suspect something on your end is converting whitespace to fancy unicode equivalents. diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 3242f3da2457..75bc7d3f0022 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -41,6 +41,7 @@ =C2=A0#include "ioapic.h" =C2=A0#include "trace.h" =C2=A0#include "x86.h" +#include "xen.h" =C2=A0#include "cpuid.h" =C2=A0#include "hyperv.h" =C2=A0#include "smm.h"
On Tue, 2024-02-06 at 11:19 -0800, Sean Christopherson wrote: > > Patch is corrupt. > > git am /home/seanjc/patches/v4_20240116_dwmw2_kvm_x86_xen_inject_vcpu_upcall_vector_when_local_apic_is_enabled.mbx > Applying: KVM: x86/xen: Inject vCPU upcall vector when local APIC is enabled > error: corrupt patch at line 17 > > cat ~/patches/v4_20240116_dwmw2_kvm_x86_xen_inject_vcpu_upcall_vector_when_local_apic_is_enabled.mbx | patch -p 1 --merge > patching file arch/x86/kvm/lapic.c > patch: **** malformed patch at line 59: #include "ioapic.h" > > Based on what I see in a web view, I suspect something on your end is converting > whitespace to fancy unicode equivalents. > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 3242f3da2457..75bc7d3f0022 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -41,6 +41,7 @@ > =C2=A0#include "ioapic.h" > =C2=A0#include "trace.h" > =C2=A0#include "x86.h" That isn't Unicode. Well, it *is*, but it's the subset of Unicode which is also plain old legacy 8-bit ISO8859-1. For some reason, Evolution has converted those spaces to non-breaking spaces. I have no idea why it's suddenly started doing that; this is a Long Term Nosupport distro that $employer forces me to use, and it hasn't even been updated for over a year. The patch is in https://git.infradead.org/?p=users/dwmw2/linux.git;a=shortlog;h=refs/heads/xenfv where I'd gathered everything that was pending, but if you prefer I'll also resend it later when I deal with the locking thing we discussed a few minutes ago.
On Tue, Feb 06, 2024, David Woodhouse wrote: > On Tue, 2024-02-06 at 11:19 -0800, Sean Christopherson wrote: > > > > Patch is corrupt. > > > > git am /home/seanjc/patches/v4_20240116_dwmw2_kvm_x86_xen_inject_vcpu_upcall_vector_when_local_apic_is_enabled.mbx > > Applying: KVM: x86/xen: Inject vCPU upcall vector when local APIC is enabled > > error: corrupt patch at line 17 > > > > cat ~/patches/v4_20240116_dwmw2_kvm_x86_xen_inject_vcpu_upcall_vector_when_local_apic_is_enabled.mbx | patch -p 1 --merge > > patching file arch/x86/kvm/lapic.c > > patch: **** malformed patch at line 59: #include "ioapic.h" > > > > Based on what I see in a web view, I suspect something on your end is converting > > whitespace to fancy unicode equivalents. > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index 3242f3da2457..75bc7d3f0022 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -41,6 +41,7 @@ > > =C2=A0#include "ioapic.h" > > =C2=A0#include "trace.h" > > =C2=A0#include "x86.h" > > That isn't Unicode. Well, it *is*, but it's the subset of Unicode which > is also plain old legacy 8-bit ISO8859-1. For some reason, Evolution > has converted those spaces to non-breaking spaces. I have no idea why > it's suddenly started doing that; this is a Long Term Nosupport distro > that $employer forces me to use, and it hasn't even been updated for > over a year. Out of genuine curiosity, why not use `git send-email`? > The patch is in > https://git.infradead.org/?p=users/dwmw2/linux.git;a=shortlog;h=refs/heads/xenfv > where I'd gathered everything that was pending, but if you prefer I'll > also resend it later when I deal with the locking thing we discussed a > few minutes ago. I'd prefer a resend so that I generate a lore link to exactly what I applied. No rush on either patch, I'm going to be mostly offline from now-ish through tomorrow.
On Tue, 2024-02-06 at 11:49 -0800, Sean Christopherson wrote: > On Tue, Feb 06, 2024, David Woodhouse wrote: > > That isn't Unicode. Well, it *is*, but it's the subset of Unicode which > > is also plain old legacy 8-bit ISO8859-1. For some reason, Evolution > > has converted those spaces to non-breaking spaces. I have no idea why > > it's suddenly started doing that; this is a Long Term Nosupport distro > > that $employer forces me to use, and it hasn't even been updated for > > over a year. > > Out of genuine curiosity, why not use `git send-email`? I generally do for a series, but a single patch sometimes isn't even the latest or only thing in my working tree, and it's trivial enough to just dump it into the Drafts folder then edit it from there. And it's (almost) always worked in the past; as I said, I have no idea why it suddenly regressed. I do prefer to actually *read* the patch and the commit message one last time in the mailer before pressing 'send'. I do catch things that way. > > The patch is in > > https://git.infradead.org/?p=users/dwmw2/linux.git;a=shortlog;h=refs/heads/xenfv > > where I'd gathered everything that was pending, but if you prefer I'll > > also resend it later when I deal with the locking thing we discussed a > > few minutes ago. > > I'd prefer a resend so that I generate a lore link to exactly what I applied. > No rush on either patch, I'm going to be mostly offline from now-ish through > tomorrow. Ack. I'll send something based on what I find in your kvm-x86 tree. You have the first 19 of Paul's series, just not the __kvm_gpc_refresh() one at the end, right?
On 1/16/24 20:00, David Woodhouse wrote: > ... > Astute reviewers may note that kvm_xen_inject_vcpu_vector() function has > a WARN_ON_ONCE() in the case where kvm_irq_delivery_to_apic_fast() fails > and returns false. In the case where the MSI is not delivered due to the > local APIC being disabled, kvm_irq_delivery_to_apic_fast() still returns > true but the value in *r is zero. So the WARN_ON_ONCE() remains correct, > as that case should still never happen. I'm curious about that WARN_ON_ONCE(). It seems that a small modification to xen_shinfo_test is enough to trigger it. --- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c +++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c @@ -87,6 +87,8 @@ enum { #define EVTCHNSTAT_interdomain 2 +#define MAX_XAPIC_ID 0xff + struct evtchn_send { u32 port; }; @@ -425,6 +427,7 @@ static void *juggle_shinfo_state(void *arg) int main(int argc, char *argv[]) { + struct kvm_vcpu *vcpus[MAX_XAPIC_ID + 3]; struct timespec min_ts, max_ts, vm_ts; struct kvm_xen_hvm_attr evt_reset; struct kvm_vm *vm; @@ -445,7 +448,8 @@ int main(int argc, char *argv[]) clock_gettime(CLOCK_REALTIME, &min_ts); - vm = vm_create_with_one_vcpu(&vcpu, guest_code); + vm = vm_create_with_vcpus(ARRAY_SIZE(vcpus), guest_code, vcpus); + vcpu = vcpus[0]; /* Map a region for the shared_info page */ vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, @@ -516,6 +520,12 @@ int main(int argc, char *argv[]) }; vcpu_ioctl(vcpu, KVM_XEN_VCPU_SET_ATTR, &pvclock); + struct kvm_xen_hvm_attr ua = { + .type = KVM_XEN_VCPU_ATTR_TYPE_UPCALL_VECTOR, + .u.vector = EVTCHN_VECTOR, + }; + vcpu_ioctl(vcpu, KVM_XEN_VCPU_SET_ATTR, &ua); + struct kvm_xen_hvm_attr vec = { .type = KVM_XEN_ATTR_TYPE_UPCALL_VECTOR, .u.vector = EVTCHN_VECTOR, [ 28.669825] ------------[ cut here ]------------ [ 28.669831] WARNING: CPU: 5 PID: 1050 at arch/x86/kvm/xen.c:509 kvm_xen_inject_vcpu_vector.isra.0+0x50/0x60 [kvm] [ 28.669867] Modules linked in: 9p netfs qrtr sunrpc intel_rapl_msr intel_rapl_common kvm_intel kvm 9pnet_virtio 9pnet rapl pcspkr i2c_piix4 drm zram crct10dif_pclmul crc32_pclmul crc32c_intel ata_generic virtio_blk pata_acpi ghash_clmulni_intel serio_raw fuse qemu_fw_cfg virtio_console [ 28.669882] CPU: 5 PID: 1050 Comm: xen_shinfo_test Not tainted 6.8.0-rc2+ #6 [ 28.669884] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.3-1-1 04/01/2014 [ 28.669885] RIP: 0010:kvm_xen_inject_vcpu_vector.isra.0+0x50/0x60 [kvm] [ 28.669911] Code: 08 48 8d 54 24 08 48 c7 44 24 0c 00 00 00 00 c7 44 24 1c 00 00 00 00 c6 44 24 10 01 e8 99 6d fd ff 84 c0 74 05 48 83 c4 20 c3 <0f> 0b 48 83 c4 20 c3 66 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 55 [ 28.669912] RSP: 0018:ffffc90001323cb0 EFLAGS: 00010046 [ 28.669914] RAX: 0000000000000000 RBX: ffffc900036c1000 RCX: ffffc90001323c2c [ 28.669915] RDX: 0000000000000004 RSI: ffffffff82630bb0 RDI: ffffffff82667eb6 [ 28.669916] RBP: 0000000000000001 R08: ffffc90001323c70 R09: ffffc90001323c68 [ 28.669916] R10: 0000000000000001 R11: 0000000000000000 R12: ffff888107b43870 [ 28.669917] R13: ffffc900036cb278 R14: 0000000000000000 R15: ffff888107b427c0 [ 28.669918] FS: 00007f2033afc740(0000) GS:ffff88842fc80000(0000) knlGS:0000000000000000 [ 28.669919] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 28.669920] CR2: 0000000000000000 CR3: 00000001203d1000 CR4: 0000000000752ef0 [ 28.669922] PKRU: 55555554 [ 28.669923] Call Trace: [ 28.669924] <TASK> [ 28.669925] ? kvm_xen_inject_vcpu_vector.isra.0+0x50/0x60 [kvm] [ 28.669949] ? __warn+0x81/0x170 [ 28.669952] ? kvm_xen_inject_vcpu_vector.isra.0+0x50/0x60 [kvm] [ 28.669976] ? report_bug+0x189/0x1c0 [ 28.669979] ? handle_bug+0x38/0x70 [ 28.669981] ? exc_invalid_op+0x13/0x60 [ 28.669983] ? asm_exc_invalid_op+0x16/0x20 [ 28.669987] ? kvm_xen_inject_vcpu_vector.isra.0+0x50/0x60 [kvm] [ 28.670011] kvm_xen_set_evtchn_fast+0x40f/0x430 [kvm] [ 28.670037] irqfd_wakeup+0x160/0x270 [kvm] [ 28.670057] ? kvm_xen_vcpu_get_attr+0x210/0x210 [kvm] [ 28.670082] __wake_up_common+0x7f/0xb0 [ 28.670085] eventfd_write+0x9d/0x1e0 [ 28.670087] ? security_file_permission+0x2c/0x40 [ 28.670090] vfs_write+0xc1/0x500 [ 28.670092] ? do_syscall_64+0xa2/0x180 [ 28.670094] ? lockdep_hardirqs_on+0x7d/0x100 [ 28.670097] ksys_write+0x59/0xd0 [ 28.670099] do_syscall_64+0x95/0x180 [ 28.670101] ? do_syscall_64+0xa2/0x180 [ 28.670104] entry_SYSCALL_64_after_hwframe+0x46/0x4e [ 28.670106] RIP: 0033:0x7f2033c07c74 [ 28.670110] Code: c7 00 16 00 00 00 b8 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 f3 0f 1e fa 80 3d f5 76 0d 00 00 74 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 55 48 89 e5 48 83 ec 20 48 89 [ 28.670111] RSP: 002b:00007ffe79281ad8 EFLAGS: 00000202 ORIG_RAX: 0000000000000001 [ 28.670113] RAX: ffffffffffffffda RBX: 00007f2033069ff1 RCX: 00007f2033c07c74 [ 28.670113] RDX: 0000000000000008 RSI: 00007ffe79281ae8 RDI: 0000000000000109 [ 28.670114] RBP: 00007ffe79281af0 R08: 000000000041d22c R09: 00000000ffffffff [ 28.670115] R10: 00007f2033b09b78 R11: 0000000000000202 R12: 0000000000000002 [ 28.670116] R13: 00000000007e52a0 R14: 00007f2033068000 R15: 0000000000000000 [ 28.670120] </TASK> [ 28.670121] irq event stamp: 305006 [ 28.670122] hardirqs last enabled at (305005): [<ffffffff81eb9cf4>] do_syscall_64+0x54/0x180 [ 28.670124] hardirqs last disabled at (305006): [<ffffffff81eda4a2>] _raw_spin_lock_irq+0x52/0x60 [ 28.670125] softirqs last enabled at (305000): [<ffffffff81039cce>] fpu_swap_kvm_fpstate+0x7e/0x120 [ 28.670127] softirqs last disabled at (304998): [<ffffffff81039c7d>] fpu_swap_kvm_fpstate+0x2d/0x120 [ 28.670129] ---[ end trace 0000000000000000 ]--- As I understand, splat here is due to APIC map being gone (because of physical APIC ID aliasing?), but I'm not sure what is the expected behaviour. Thanks, Michal
On Wed, 2024-02-14 at 17:32 +0100, Michal Luczaj wrote: > As I understand, splat here is due to APIC map being gone (because of physical > APIC ID aliasing?), but I'm not sure what is the expected behaviour. Good catch, thank you. /* The fast version will always work for physical unicast */ WARN_ON_ONCE(!kvm_irq_delivery_to_apic_fast(v->kvm, NULL, &irq, &r, NULL)); So the comment is wrong, we *can't* rely on it being unicast. With APIC ID aliasing, it ends up being multicast. But I think we can just do this: --- a/arch/x86/kvm/xen.c +++ b/arch/x86/kvm/xen.c @@ -580,8 +580,7 @@ void kvm_xen_inject_vcpu_vector(struct kvm_vcpu *v) irq.delivery_mode = APIC_DM_FIXED; irq.level = 1; - /* The fast version will always work for physical unicast */ - WARN_ON_ONCE(!kvm_irq_delivery_to_apic_fast(v->kvm, NULL, &irq, &r, NULL)); + kvm_irq_delivery_to_apic(v->kvm, NULL, &irq, NULL); } /*
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 3242f3da2457..75bc7d3f0022 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -41,6 +41,7 @@ #include "ioapic.h" #include "trace.h" #include "x86.h" +#include "xen.h" #include "cpuid.h" #include "hyperv.h" #include "smm.h" @@ -499,8 +500,10 @@ static inline void apic_set_spiv(struct kvm_lapic *apic, u32 val) } /* Check if there are APF page ready requests pending */ - if (enabled) + if (enabled) { kvm_make_request(KVM_REQ_APF_READY, apic->vcpu); + kvm_xen_sw_enable_lapic(apic->vcpu); + } } static inline void kvm_apic_set_xapic_id(struct kvm_lapic *apic, u8 id) diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c index 8ef668922340..7c602037b596 100644 --- a/arch/x86/kvm/xen.c +++ b/arch/x86/kvm/xen.c @@ -568,7 +568,7 @@ void kvm_xen_update_runstate(struct kvm_vcpu *v, int state) kvm_xen_update_runstate_guest(v, state == RUNSTATE_runnable); } -static void kvm_xen_inject_vcpu_vector(struct kvm_vcpu *v) +void kvm_xen_inject_vcpu_vector(struct kvm_vcpu *v) { struct kvm_lapic_irq irq = { }; int r; diff --git a/arch/x86/kvm/xen.h b/arch/x86/kvm/xen.h index f8f1fe22d090..f5841d9000ae 100644 --- a/arch/x86/kvm/xen.h +++ b/arch/x86/kvm/xen.h @@ -18,6 +18,7 @@ extern struct static_key_false_deferred kvm_xen_enabled; int __kvm_xen_has_interrupt(struct kvm_vcpu *vcpu); void kvm_xen_inject_pending_events(struct kvm_vcpu *vcpu); +void kvm_xen_inject_vcpu_vector(struct kvm_vcpu *vcpu); int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data); int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data); int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data); @@ -36,6 +37,19 @@ int kvm_xen_setup_evtchn(struct kvm *kvm, const struct kvm_irq_routing_entry *ue); void kvm_xen_update_tsc_info(struct kvm_vcpu *vcpu); +static inline void kvm_xen_sw_enable_lapic(struct kvm_vcpu *vcpu) +{ + /* + * The local APIC is being enabled. If the per-vCPU upcall vector is + * set and the vCPU's evtchn_upcall_pending flag is set, inject the + * interrupt. + */ + if (static_branch_unlikely(&kvm_xen_enabled.key) && + vcpu->arch.xen.vcpu_info_cache.active && + vcpu->arch.xen.upcall_vector && __kvm_xen_has_interrupt(vcpu)) + kvm_xen_inject_vcpu_vector(vcpu); +} + static inline bool kvm_xen_msr_enabled(struct kvm *kvm) { return static_branch_unlikely(&kvm_xen_enabled.key) && @@ -101,6 +115,10 @@ static inline void kvm_xen_destroy_vcpu(struct kvm_vcpu *vcpu) { } +static inline void kvm_xen_sw_enable_lapic(struct kvm_vcpu *vcpu) +{ +} + static inline bool kvm_xen_msr_enabled(struct kvm *kvm) { return false;