| Message ID | 20200227170048.236599-1-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Thu, Feb 27, 2020 at 12:00:48PM -0500, Peter Xu wrote: > +static inline void kvm_resample_fd_remove(int gsi) > +{ > + KVMResampleFd *rfd; > + > + QLIST_FOREACH(rfd, &kvm_resample_fd_list, node) { > + if (rfd->gsi == gsi) { > + QLIST_REMOVE(rfd, node); Oops, rfd is leaked... Will fix that in v2. > + break; > + } > + } > +}
Hi Peter, On 2/27/20 6:00 PM, Peter Xu wrote: > This is majorly only for X86 because that's the only one that supports > split irqchip for now. > > When the irqchip is split, we face a dilemma that KVM irqfd will be > enabled, however the slow irqchip is still running in the userspace. > It means that the resamplefd in the kernel irqfds won't take any > effect and it can miss to ack INTx interrupts on EOIs. Won't it always fail to ack INTx? With the above sentence I understand it can work sometimes? > > One example is split irqchip with VFIO INTx, which will break if we > use the VFIO INTx fast path. > > This patch can potentially supports the VFIO fast path again for INTx, > that the IRQ delivery will still use the fast path, while we don't > need to trap MMIOs in QEMU for the device to emulate the EIOs (see the > callers of vfio_eoi() hook). However the EOI of the INTx will still > need to be done from the userspace by caching all the resamplefds in > QEMU and kick properly for IOAPIC EOI broadcast. If I understand correctly this is a one way fast path? Fast path is on the trigger side only: VFIO -> KVM but not on the deactivation side, trapped by the userspace IOAPIC where you directly notify the UNMASK eventfd from userspace. Is that correct? > > When the userspace is responsible for the resamplefd kickup, don't > register it on the kvm_irqfd anymore, because on newer kernels (after > commit 654f1f13ea56, 5.2+) the KVM_IRQFD will fail if with both split > irqchip and resamplefd. This will make sure that the fast path will > work for all supported kernels. > > https://patchwork.kernel.org/patch/10738541/#22609933 > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > v1.1 changelog: > - when resamplefd is going to be kicked from userspace, don't register > it again in KVM_IRQFD. Tested against upstream kernel. > > accel/kvm/kvm-all.c | 74 ++++++++++++++++++++++++++++++++++++++++-- > accel/kvm/trace-events | 1 + > hw/intc/ioapic.c | 11 +++++-- > include/sysemu/kvm.h | 4 +++ > 4 files changed, 86 insertions(+), 4 deletions(-) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index d49b74512a..b766b6e93c 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -159,9 +159,62 @@ static const KVMCapabilityInfo kvm_required_capabilites[] = { > static NotifierList kvm_irqchip_change_notifiers = > NOTIFIER_LIST_INITIALIZER(kvm_irqchip_change_notifiers); > > +struct KVMResampleFd { > + int gsi; > + EventNotifier *resample_event; > + QLIST_ENTRY(KVMResampleFd) node; > +}; > +typedef struct KVMResampleFd KVMResampleFd; > + > +/* > + * Only used with split irqchip where we need to do the resample fd > + * kick for the kernel from userspace. > + */ > +static QLIST_HEAD(, KVMResampleFd) kvm_resample_fd_list = > + QLIST_HEAD_INITIALIZER(kvm_resample_fd_list); > + > #define kvm_slots_lock(kml) qemu_mutex_lock(&(kml)->slots_lock) > #define kvm_slots_unlock(kml) qemu_mutex_unlock(&(kml)->slots_lock) > > +static inline void kvm_resample_fd_remove(int gsi) > +{ > + KVMResampleFd *rfd; > + > + QLIST_FOREACH(rfd, &kvm_resample_fd_list, node) { > + if (rfd->gsi == gsi) { > + QLIST_REMOVE(rfd, node); > + break; > + } > + } > +} > + > +static inline void kvm_resample_fd_insert(int gsi, EventNotifier *event) > +{ > + KVMResampleFd *rfd = g_new0(KVMResampleFd, 1); > + > + rfd->gsi = gsi; > + rfd->resample_event = event; > + > + QLIST_INSERT_HEAD(&kvm_resample_fd_list, rfd, node); > +} > + > +void kvm_resample_fd_notify(int gsi) > +{ > + KVMResampleFd *rfd; > + > + if (!kvm_irqchip_is_split()) { > + return; > + } > + > + QLIST_FOREACH(rfd, &kvm_resample_fd_list, node) { > + if (rfd->gsi == gsi) { > + event_notifier_set(rfd->resample_event); > + trace_kvm_resample_fd_notify(gsi); > + break; > + } > + } > +} > + > int kvm_get_max_memslots(void) > { > KVMState *s = KVM_STATE(current_accel()); > @@ -1642,8 +1695,25 @@ static int kvm_irqchip_assign_irqfd(KVMState *s, EventNotifier *event, > }; > > if (rfd != -1) { > - irqfd.flags |= KVM_IRQFD_FLAG_RESAMPLE; > - irqfd.resamplefd = rfd; > + assert(assign); > + if (kvm_irqchip_is_split()) { > + /* > + * When the slow irqchip (e.g. IOAPIC) is in the > + * userspace, resamplefd will not work because the EOI of > + * the interrupt will be delivered to userspace instead, s/delivered to userspace/handled in userspace > + * the KVM resample fd kick is skipped. The userspace > + * needs to remember the resamplefd and kick it when we > + * receive EOI of this IRQ. Practically we now talk about a VFIO ACTION_UNMASK classical eventfd As such isn't it a bit weird to handle those normal UNMASK eventfds in the KVM code? > + */ > + kvm_resample_fd_insert(virq, resample); > + } else { > + irqfd.flags |= KVM_IRQFD_FLAG_RESAMPLE; > + irqfd.resamplefd = rfd; > + } > + } else if (!assign) { > + if (kvm_irqchip_is_split()) { > + kvm_resample_fd_remove(virq); > + } > } > > if (!kvm_irqfds_enabled()) { > diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events > index 4fb6e59d19..a68eb66534 100644 > --- a/accel/kvm/trace-events > +++ b/accel/kvm/trace-events > @@ -16,4 +16,5 @@ kvm_set_ioeventfd_mmio(int fd, uint64_t addr, uint32_t val, bool assign, uint32_ > kvm_set_ioeventfd_pio(int fd, uint16_t addr, uint32_t val, bool assign, uint32_t size, bool datamatch) "fd: %d @0x%x val=0x%x assign: %d size: %d match: %d" > kvm_set_user_memory(uint32_t slot, uint32_t flags, uint64_t guest_phys_addr, uint64_t memory_size, uint64_t userspace_addr, int ret) "Slot#%d flags=0x%x gpa=0x%"PRIx64 " size=0x%"PRIx64 " ua=0x%"PRIx64 " ret=%d" > kvm_clear_dirty_log(uint32_t slot, uint64_t start, uint32_t size) "slot#%"PRId32" start 0x%"PRIx64" size 0x%"PRIx32 > +kvm_resample_fd_notify(int gsi) "gsi %d" > > diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c > index 15747fe2c2..8c75465c62 100644 > --- a/hw/intc/ioapic.c > +++ b/hw/intc/ioapic.c > @@ -236,8 +236,15 @@ void ioapic_eoi_broadcast(int vector) > for (n = 0; n < IOAPIC_NUM_PINS; n++) { > entry = s->ioredtbl[n]; > > - if ((entry & IOAPIC_VECTOR_MASK) != vector || > - ((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1) != IOAPIC_TRIGGER_LEVEL) { > + if ((entry & IOAPIC_VECTOR_MASK) != vector) { > + continue; > + } > + > + /* Kick resamplefd if KVM is bypassed */ > + kvm_resample_fd_notify(n); KVM is bypassed on the deactivation path but still we call kvm_resample_fd_notify(). > + > + if (((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1) != > + IOAPIC_TRIGGER_LEVEL) { > continue; > } > > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > index 141342de98..b67552c047 100644 > --- a/include/sysemu/kvm.h > +++ b/include/sysemu/kvm.h > @@ -555,4 +555,8 @@ int kvm_set_one_reg(CPUState *cs, uint64_t id, void *source); > int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target); > struct ppc_radix_page_info *kvm_get_radix_page_info(void); > int kvm_get_max_memslots(void); > + > +/* Notify resamplefd for EOI of specific level triggered interrupts */ > +void kvm_resample_fd_notify(int gsi); > + > #endif > Thanks Eric
On Thu, Feb 27, 2020 at 06:42:09PM +0100, Auger Eric wrote: > Hi Peter, > > On 2/27/20 6:00 PM, Peter Xu wrote: > > This is majorly only for X86 because that's the only one that supports > > split irqchip for now. > > > > When the irqchip is split, we face a dilemma that KVM irqfd will be > > enabled, however the slow irqchip is still running in the userspace. > > It means that the resamplefd in the kernel irqfds won't take any > > effect and it can miss to ack INTx interrupts on EOIs. > Won't it always fail to ack INTx? With the above sentence I understand > it can work sometimes? I wanted to mean that it will fail. How about s/can/will/? Or even better wordings that you'd suggest? > > > > One example is split irqchip with VFIO INTx, which will break if we > > use the VFIO INTx fast path. > > > > This patch can potentially supports the VFIO fast path again for INTx, > > that the IRQ delivery will still use the fast path, while we don't > > need to trap MMIOs in QEMU for the device to emulate the EIOs (see the > > callers of vfio_eoi() hook). However the EOI of the INTx will still > > need to be done from the userspace by caching all the resamplefds in > > QEMU and kick properly for IOAPIC EOI broadcast. > If I understand correctly this is a one way fast path? Fast path is on > the trigger side only: VFIO -> KVM but not on the deactivation side, > trapped by the userspace IOAPIC where you directly notify the UNMASK > eventfd from userspace. Is that correct? Right, the injection is still using the whole fast path. However AFAIU even for the EOI path it should still be faster than the pure slow path of vfio INTx EIO. From what I got from reading the code, the slow path will conditionally unmap MMIO regions (with a timer to delay the recovery) so all MMIOs will be slowed down. For what this patch is doing, it will need to exit to userspace for sure for each EOI (after all IOAPIC is in userspace), however for the whole lifecycle of the device, the MMIO regions should always be mapped so no unwanted MMIO traps. > > > > When the userspace is responsible for the resamplefd kickup, don't > > register it on the kvm_irqfd anymore, because on newer kernels (after > > commit 654f1f13ea56, 5.2+) the KVM_IRQFD will fail if with both split > > irqchip and resamplefd. This will make sure that the fast path will > > work for all supported kernels. > > > > https://patchwork.kernel.org/patch/10738541/#22609933 > > > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > v1.1 changelog: > > - when resamplefd is going to be kicked from userspace, don't register > > it again in KVM_IRQFD. Tested against upstream kernel. > > > > accel/kvm/kvm-all.c | 74 ++++++++++++++++++++++++++++++++++++++++-- > > accel/kvm/trace-events | 1 + > > hw/intc/ioapic.c | 11 +++++-- > > include/sysemu/kvm.h | 4 +++ > > 4 files changed, 86 insertions(+), 4 deletions(-) > > > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > > index d49b74512a..b766b6e93c 100644 > > --- a/accel/kvm/kvm-all.c > > +++ b/accel/kvm/kvm-all.c > > @@ -159,9 +159,62 @@ static const KVMCapabilityInfo kvm_required_capabilites[] = { > > static NotifierList kvm_irqchip_change_notifiers = > > NOTIFIER_LIST_INITIALIZER(kvm_irqchip_change_notifiers); > > > > +struct KVMResampleFd { > > + int gsi; > > + EventNotifier *resample_event; > > + QLIST_ENTRY(KVMResampleFd) node; > > +}; > > +typedef struct KVMResampleFd KVMResampleFd; > > + > > +/* > > + * Only used with split irqchip where we need to do the resample fd > > + * kick for the kernel from userspace. > > + */ > > +static QLIST_HEAD(, KVMResampleFd) kvm_resample_fd_list = > > + QLIST_HEAD_INITIALIZER(kvm_resample_fd_list); > > + > > #define kvm_slots_lock(kml) qemu_mutex_lock(&(kml)->slots_lock) > > #define kvm_slots_unlock(kml) qemu_mutex_unlock(&(kml)->slots_lock) > > > > +static inline void kvm_resample_fd_remove(int gsi) > > +{ > > + KVMResampleFd *rfd; > > + > > + QLIST_FOREACH(rfd, &kvm_resample_fd_list, node) { > > + if (rfd->gsi == gsi) { > > + QLIST_REMOVE(rfd, node); > > + break; > > + } > > + } > > +} > > + > > +static inline void kvm_resample_fd_insert(int gsi, EventNotifier *event) > > +{ > > + KVMResampleFd *rfd = g_new0(KVMResampleFd, 1); > > + > > + rfd->gsi = gsi; > > + rfd->resample_event = event; > > + > > + QLIST_INSERT_HEAD(&kvm_resample_fd_list, rfd, node); > > +} > > + > > +void kvm_resample_fd_notify(int gsi) > > +{ > > + KVMResampleFd *rfd; > > + > > + if (!kvm_irqchip_is_split()) { > > + return; > > + } > > + > > + QLIST_FOREACH(rfd, &kvm_resample_fd_list, node) { > > + if (rfd->gsi == gsi) { > > + event_notifier_set(rfd->resample_event); > > + trace_kvm_resample_fd_notify(gsi); > > + break; > > + } > > + } > > +} > > + > > int kvm_get_max_memslots(void) > > { > > KVMState *s = KVM_STATE(current_accel()); > > @@ -1642,8 +1695,25 @@ static int kvm_irqchip_assign_irqfd(KVMState *s, EventNotifier *event, > > }; > > > > if (rfd != -1) { > > - irqfd.flags |= KVM_IRQFD_FLAG_RESAMPLE; > > - irqfd.resamplefd = rfd; > > + assert(assign); > > + if (kvm_irqchip_is_split()) { > > + /* > > + * When the slow irqchip (e.g. IOAPIC) is in the > > + * userspace, resamplefd will not work because the EOI of > > + * the interrupt will be delivered to userspace instead, > s/delivered to userspace/handled in userspace It will be delivered to userspace by KVM_EXIT_IOAPIC_EOI, so maybe... "delivered and handled"? > > + * the KVM resample fd kick is skipped. The userspace > > + * needs to remember the resamplefd and kick it when we > > + * receive EOI of this IRQ. > Practically we now talk about a VFIO ACTION_UNMASK classical eventfd > As such isn't it a bit weird to handle those normal UNMASK eventfds in > the KVM code? I'm not sure I completely get the question, but this should be something general to KVM resamplefd support. In other words, this should also fix other devices (besides VFIO) when they're using the KVM resamplefd, because IMHO it's the resamplefd and split irqchip which is really broken here. With that in mind, I think KVM should not need to even know what's behind the resamplefd (in VFIO's case, it's the UNMASK eventfd). It just needs to kick it when IOAPIC EOI comes for the specific IRQ. > > > > + */ > > + kvm_resample_fd_insert(virq, resample); > > + } else { > > + irqfd.flags |= KVM_IRQFD_FLAG_RESAMPLE; > > + irqfd.resamplefd = rfd; > > + } > > + } else if (!assign) { > > + if (kvm_irqchip_is_split()) { > > + kvm_resample_fd_remove(virq); > > + } > > } > > > > if (!kvm_irqfds_enabled()) { > > diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events > > index 4fb6e59d19..a68eb66534 100644 > > --- a/accel/kvm/trace-events > > +++ b/accel/kvm/trace-events > > @@ -16,4 +16,5 @@ kvm_set_ioeventfd_mmio(int fd, uint64_t addr, uint32_t val, bool assign, uint32_ > > kvm_set_ioeventfd_pio(int fd, uint16_t addr, uint32_t val, bool assign, uint32_t size, bool datamatch) "fd: %d @0x%x val=0x%x assign: %d size: %d match: %d" > > kvm_set_user_memory(uint32_t slot, uint32_t flags, uint64_t guest_phys_addr, uint64_t memory_size, uint64_t userspace_addr, int ret) "Slot#%d flags=0x%x gpa=0x%"PRIx64 " size=0x%"PRIx64 " ua=0x%"PRIx64 " ret=%d" > > kvm_clear_dirty_log(uint32_t slot, uint64_t start, uint32_t size) "slot#%"PRId32" start 0x%"PRIx64" size 0x%"PRIx32 > > +kvm_resample_fd_notify(int gsi) "gsi %d" > > > > diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c > > index 15747fe2c2..8c75465c62 100644 > > --- a/hw/intc/ioapic.c > > +++ b/hw/intc/ioapic.c > > @@ -236,8 +236,15 @@ void ioapic_eoi_broadcast(int vector) > > for (n = 0; n < IOAPIC_NUM_PINS; n++) { > > entry = s->ioredtbl[n]; > > > > - if ((entry & IOAPIC_VECTOR_MASK) != vector || > > - ((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1) != IOAPIC_TRIGGER_LEVEL) { > > + if ((entry & IOAPIC_VECTOR_MASK) != vector) { > > + continue; > > + } > > + > > + /* Kick resamplefd if KVM is bypassed */ > > + kvm_resample_fd_notify(n); > KVM is bypassed on the deactivation path but still we call > kvm_resample_fd_notify(). Yes I wanted to say that the kernel won't be able to kick the resamplefd. How about: When IOAPIC is in the userspace (while APIC is still in the kernel), we need to kick the resamplefd to deactivate the IRQ for KVM. Better suggestion on the wording is always welcomed. Thanks, > > + > > + if (((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1) != > > + IOAPIC_TRIGGER_LEVEL) { > > continue; > > } > > > > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > > index 141342de98..b67552c047 100644 > > --- a/include/sysemu/kvm.h > > +++ b/include/sysemu/kvm.h > > @@ -555,4 +555,8 @@ int kvm_set_one_reg(CPUState *cs, uint64_t id, void *source); > > int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target); > > struct ppc_radix_page_info *kvm_get_radix_page_info(void); > > int kvm_get_max_memslots(void); > > + > > +/* Notify resamplefd for EOI of specific level triggered interrupts */ > > +void kvm_resample_fd_notify(int gsi); > > + > > #endif > > > Thanks > > Eric >
Hi Peter, On 2/27/20 7:00 PM, Peter Xu wrote: > On Thu, Feb 27, 2020 at 06:42:09PM +0100, Auger Eric wrote: >> Hi Peter, >> >> On 2/27/20 6:00 PM, Peter Xu wrote: >>> This is majorly only for X86 because that's the only one that supports >>> split irqchip for now. >>> >>> When the irqchip is split, we face a dilemma that KVM irqfd will be >>> enabled, however the slow irqchip is still running in the userspace. >>> It means that the resamplefd in the kernel irqfds won't take any >>> effect and it can miss to ack INTx interrupts on EOIs. >> Won't it always fail to ack INTx? With the above sentence I understand >> it can work sometimes? > > I wanted to mean that it will fail. How about s/can/will/? Or even > better wordings that you'd suggest? yes: s/can/will > >>> >>> One example is split irqchip with VFIO INTx, which will break if we >>> use the VFIO INTx fast path. >>> >>> This patch can potentially supports the VFIO fast path again for INTx, >>> that the IRQ delivery will still use the fast path, while we don't >>> need to trap MMIOs in QEMU for the device to emulate the EIOs (see the >>> callers of vfio_eoi() hook). However the EOI of the INTx will still >>> need to be done from the userspace by caching all the resamplefds in >>> QEMU and kick properly for IOAPIC EOI broadcast. >> If I understand correctly this is a one way fast path? Fast path is on >> the trigger side only: VFIO -> KVM but not on the deactivation side, >> trapped by the userspace IOAPIC where you directly notify the UNMASK >> eventfd from userspace. Is that correct? > > Right, the injection is still using the whole fast path. However > AFAIU even for the EOI path it should still be faster than the pure > slow path of vfio INTx EIO. From what I got from reading the code, > the slow path will conditionally unmap MMIO regions (with a timer to > delay the recovery) so all MMIOs will be slowed down. For what this > patch is doing, it will need to exit to userspace for sure for each > EOI (after all IOAPIC is in userspace), however for the whole > lifecycle of the device, the MMIO regions should always be mapped so > no unwanted MMIO traps. Yes the EOI is trapped on IOAPIC side and not at the BAR level. So it should be more efficient and more precise. > >>> >>> When the userspace is responsible for the resamplefd kickup, don't >>> register it on the kvm_irqfd anymore, because on newer kernels (after >>> commit 654f1f13ea56, 5.2+) the KVM_IRQFD will fail if with both split >>> irqchip and resamplefd. This will make sure that the fast path will >>> work for all supported kernels. >>> >>> https://patchwork.kernel.org/patch/10738541/#22609933 >>> >>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >>> Signed-off-by: Peter Xu <peterx@redhat.com> >>> --- >>> v1.1 changelog: >>> - when resamplefd is going to be kicked from userspace, don't register >>> it again in KVM_IRQFD. Tested against upstream kernel. >>> >>> accel/kvm/kvm-all.c | 74 ++++++++++++++++++++++++++++++++++++++++-- >>> accel/kvm/trace-events | 1 + >>> hw/intc/ioapic.c | 11 +++++-- >>> include/sysemu/kvm.h | 4 +++ >>> 4 files changed, 86 insertions(+), 4 deletions(-) >>> >>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >>> index d49b74512a..b766b6e93c 100644 >>> --- a/accel/kvm/kvm-all.c >>> +++ b/accel/kvm/kvm-all.c >>> @@ -159,9 +159,62 @@ static const KVMCapabilityInfo kvm_required_capabilites[] = { >>> static NotifierList kvm_irqchip_change_notifiers = >>> NOTIFIER_LIST_INITIALIZER(kvm_irqchip_change_notifiers); >>> >>> +struct KVMResampleFd { >>> + int gsi; >>> + EventNotifier *resample_event; >>> + QLIST_ENTRY(KVMResampleFd) node; >>> +}; >>> +typedef struct KVMResampleFd KVMResampleFd; >>> + >>> +/* >>> + * Only used with split irqchip where we need to do the resample fd >>> + * kick for the kernel from userspace. >>> + */ >>> +static QLIST_HEAD(, KVMResampleFd) kvm_resample_fd_list = >>> + QLIST_HEAD_INITIALIZER(kvm_resample_fd_list); >>> + >>> #define kvm_slots_lock(kml) qemu_mutex_lock(&(kml)->slots_lock) >>> #define kvm_slots_unlock(kml) qemu_mutex_unlock(&(kml)->slots_lock) >>> >>> +static inline void kvm_resample_fd_remove(int gsi) >>> +{ >>> + KVMResampleFd *rfd; >>> + >>> + QLIST_FOREACH(rfd, &kvm_resample_fd_list, node) { >>> + if (rfd->gsi == gsi) { >>> + QLIST_REMOVE(rfd, node); >>> + break; >>> + } >>> + } >>> +} >>> + >>> +static inline void kvm_resample_fd_insert(int gsi, EventNotifier *event) >>> +{ >>> + KVMResampleFd *rfd = g_new0(KVMResampleFd, 1); >>> + >>> + rfd->gsi = gsi; >>> + rfd->resample_event = event; >>> + >>> + QLIST_INSERT_HEAD(&kvm_resample_fd_list, rfd, node); >>> +} >>> + >>> +void kvm_resample_fd_notify(int gsi) >>> +{ >>> + KVMResampleFd *rfd; >>> + >>> + if (!kvm_irqchip_is_split()) { >>> + return; >>> + } >>> + >>> + QLIST_FOREACH(rfd, &kvm_resample_fd_list, node) { >>> + if (rfd->gsi == gsi) { >>> + event_notifier_set(rfd->resample_event); >>> + trace_kvm_resample_fd_notify(gsi); >>> + break; >>> + } >>> + } >>> +} >>> + >>> int kvm_get_max_memslots(void) >>> { >>> KVMState *s = KVM_STATE(current_accel()); >>> @@ -1642,8 +1695,25 @@ static int kvm_irqchip_assign_irqfd(KVMState *s, EventNotifier *event, >>> }; >>> >>> if (rfd != -1) { >>> - irqfd.flags |= KVM_IRQFD_FLAG_RESAMPLE; >>> - irqfd.resamplefd = rfd; >>> + assert(assign); >>> + if (kvm_irqchip_is_split()) { >>> + /* >>> + * When the slow irqchip (e.g. IOAPIC) is in the >>> + * userspace, resamplefd will not work because the EOI of >>> + * the interrupt will be delivered to userspace instead, >> s/delivered to userspace/handled in userspace > > It will be delivered to userspace by KVM_EXIT_IOAPIC_EOI, so > maybe... "delivered and handled"? ah ok. TBH I don't really know how the split irqchip works and that may explain below misunderstandings. > >>> + * the KVM resample fd kick is skipped. The userspace >>> + * needs to remember the resamplefd and kick it when we >>> + * receive EOI of this IRQ. >> Practically we now talk about a VFIO ACTION_UNMASK classical eventfd >> As such isn't it a bit weird to handle those normal UNMASK eventfds in >> the KVM code? > > I'm not sure I completely get the question, but this should be > something general to KVM resamplefd support. In other words, this > should also fix other devices (besides VFIO) when they're using the > KVM resamplefd, because IMHO it's the resamplefd and split irqchip > which is really broken here. Here is my understanding (& memories): the KVM resamplefd is an eventfd you register to KVM so that KVM triggers the resamplefd when KVM traps the EOI. Here I understand this is the userspace IOAPIC that traps the EOI and not the in-kernel virtual interrupt controller. So I would have expected you just need to signal the VFIO UNMASK eventfd to re-enable the physical IRQ (which was automasked). This is no more a KVM resamplefd strictly speaking as KVM is not involved anymore in the deactivation process. > > With that in mind, I think KVM should not need to even know what's > behind the resamplefd (in VFIO's case, it's the UNMASK eventfd). It > just needs to kick it when IOAPIC EOI comes for the specific IRQ But above the userspace directly calls event_notifier_set(rfd->resample_event); This is not KVM anymore that "kicks it". Or maybe I miss something. So my comment was, why is it handled in the QEMU KVM layer? . > >> >> >>> + */ >>> + kvm_resample_fd_insert(virq, resample); >>> + } else { >>> + irqfd.flags |= KVM_IRQFD_FLAG_RESAMPLE; >>> + irqfd.resamplefd = rfd; >>> + } >>> + } else if (!assign) { >>> + if (kvm_irqchip_is_split()) { >>> + kvm_resample_fd_remove(virq); >>> + } >>> } >>> >>> if (!kvm_irqfds_enabled()) { >>> diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events >>> index 4fb6e59d19..a68eb66534 100644 >>> --- a/accel/kvm/trace-events >>> +++ b/accel/kvm/trace-events >>> @@ -16,4 +16,5 @@ kvm_set_ioeventfd_mmio(int fd, uint64_t addr, uint32_t val, bool assign, uint32_ >>> kvm_set_ioeventfd_pio(int fd, uint16_t addr, uint32_t val, bool assign, uint32_t size, bool datamatch) "fd: %d @0x%x val=0x%x assign: %d size: %d match: %d" >>> kvm_set_user_memory(uint32_t slot, uint32_t flags, uint64_t guest_phys_addr, uint64_t memory_size, uint64_t userspace_addr, int ret) "Slot#%d flags=0x%x gpa=0x%"PRIx64 " size=0x%"PRIx64 " ua=0x%"PRIx64 " ret=%d" >>> kvm_clear_dirty_log(uint32_t slot, uint64_t start, uint32_t size) "slot#%"PRId32" start 0x%"PRIx64" size 0x%"PRIx32 >>> +kvm_resample_fd_notify(int gsi) "gsi %d" >>> >>> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c >>> index 15747fe2c2..8c75465c62 100644 >>> --- a/hw/intc/ioapic.c >>> +++ b/hw/intc/ioapic.c >>> @@ -236,8 +236,15 @@ void ioapic_eoi_broadcast(int vector) >>> for (n = 0; n < IOAPIC_NUM_PINS; n++) { >>> entry = s->ioredtbl[n]; >>> >>> - if ((entry & IOAPIC_VECTOR_MASK) != vector || >>> - ((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1) != IOAPIC_TRIGGER_LEVEL) { >>> + if ((entry & IOAPIC_VECTOR_MASK) != vector) { >>> + continue; >>> + } >>> + >>> + /* Kick resamplefd if KVM is bypassed */ >>> + kvm_resample_fd_notify(n); >> KVM is bypassed on the deactivation path but still we call >> kvm_resample_fd_notify(). > > Yes I wanted to say that the kernel won't be able to kick the > resamplefd. How about: > > When IOAPIC is in the userspace (while APIC is still in the kernel), > we need to kick the resamplefd to deactivate the IRQ for KVM. This fd "just" aims at unmasking the IRQ at physical level (UNMASK VFIO event)? Does it perform anything related to the virtual interrupt controller? Thanks Eric > > Better suggestion on the wording is always welcomed. > > Thanks, > >>> + >>> + if (((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1) != >>> + IOAPIC_TRIGGER_LEVEL) { >>> continue; >>> } >>> >>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h >>> index 141342de98..b67552c047 100644 >>> --- a/include/sysemu/kvm.h >>> +++ b/include/sysemu/kvm.h >>> @@ -555,4 +555,8 @@ int kvm_set_one_reg(CPUState *cs, uint64_t id, void *source); >>> int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target); >>> struct ppc_radix_page_info *kvm_get_radix_page_info(void); >>> int kvm_get_max_memslots(void); >>> + >>> +/* Notify resamplefd for EOI of specific level triggered interrupts */ >>> +void kvm_resample_fd_notify(int gsi); >>> + >>> #endif >>> >> Thanks >> >> Eric >> >
On Thu, Feb 27, 2020 at 07:22:08PM +0100, Auger Eric wrote: > Hi Peter, > > On 2/27/20 7:00 PM, Peter Xu wrote: > > On Thu, Feb 27, 2020 at 06:42:09PM +0100, Auger Eric wrote: > >> Hi Peter, > >> > >> On 2/27/20 6:00 PM, Peter Xu wrote: > >>> This is majorly only for X86 because that's the only one that supports > >>> split irqchip for now. > >>> > >>> When the irqchip is split, we face a dilemma that KVM irqfd will be > >>> enabled, however the slow irqchip is still running in the userspace. > >>> It means that the resamplefd in the kernel irqfds won't take any > >>> effect and it can miss to ack INTx interrupts on EOIs. > >> Won't it always fail to ack INTx? With the above sentence I understand > >> it can work sometimes? > > > > I wanted to mean that it will fail. How about s/can/will/? Or even > > better wordings that you'd suggest? > yes: s/can/will > > > >>> > >>> One example is split irqchip with VFIO INTx, which will break if we > >>> use the VFIO INTx fast path. > >>> > >>> This patch can potentially supports the VFIO fast path again for INTx, > >>> that the IRQ delivery will still use the fast path, while we don't > >>> need to trap MMIOs in QEMU for the device to emulate the EIOs (see the > >>> callers of vfio_eoi() hook). However the EOI of the INTx will still > >>> need to be done from the userspace by caching all the resamplefds in > >>> QEMU and kick properly for IOAPIC EOI broadcast. > >> If I understand correctly this is a one way fast path? Fast path is on > >> the trigger side only: VFIO -> KVM but not on the deactivation side, > >> trapped by the userspace IOAPIC where you directly notify the UNMASK > >> eventfd from userspace. Is that correct? > > > > Right, the injection is still using the whole fast path. However > > AFAIU even for the EOI path it should still be faster than the pure > > slow path of vfio INTx EIO. From what I got from reading the code, > > the slow path will conditionally unmap MMIO regions (with a timer to > > delay the recovery) so all MMIOs will be slowed down. For what this > > patch is doing, it will need to exit to userspace for sure for each > > EOI (after all IOAPIC is in userspace), however for the whole > > lifecycle of the device, the MMIO regions should always be mapped so > > no unwanted MMIO traps. > Yes the EOI is trapped on IOAPIC side and not at the BAR level. So it > should be more efficient and more precise. Yes. > > > >>> > >>> When the userspace is responsible for the resamplefd kickup, don't > >>> register it on the kvm_irqfd anymore, because on newer kernels (after > >>> commit 654f1f13ea56, 5.2+) the KVM_IRQFD will fail if with both split > >>> irqchip and resamplefd. This will make sure that the fast path will > >>> work for all supported kernels. > >>> > >>> https://patchwork.kernel.org/patch/10738541/#22609933 > >>> > >>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > >>> Signed-off-by: Peter Xu <peterx@redhat.com> > >>> --- > >>> v1.1 changelog: > >>> - when resamplefd is going to be kicked from userspace, don't register > >>> it again in KVM_IRQFD. Tested against upstream kernel. > >>> > >>> accel/kvm/kvm-all.c | 74 ++++++++++++++++++++++++++++++++++++++++-- > >>> accel/kvm/trace-events | 1 + > >>> hw/intc/ioapic.c | 11 +++++-- > >>> include/sysemu/kvm.h | 4 +++ > >>> 4 files changed, 86 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > >>> index d49b74512a..b766b6e93c 100644 > >>> --- a/accel/kvm/kvm-all.c > >>> +++ b/accel/kvm/kvm-all.c > >>> @@ -159,9 +159,62 @@ static const KVMCapabilityInfo kvm_required_capabilites[] = { > >>> static NotifierList kvm_irqchip_change_notifiers = > >>> NOTIFIER_LIST_INITIALIZER(kvm_irqchip_change_notifiers); > >>> > >>> +struct KVMResampleFd { > >>> + int gsi; > >>> + EventNotifier *resample_event; > >>> + QLIST_ENTRY(KVMResampleFd) node; > >>> +}; > >>> +typedef struct KVMResampleFd KVMResampleFd; > >>> + > >>> +/* > >>> + * Only used with split irqchip where we need to do the resample fd > >>> + * kick for the kernel from userspace. > >>> + */ > >>> +static QLIST_HEAD(, KVMResampleFd) kvm_resample_fd_list = > >>> + QLIST_HEAD_INITIALIZER(kvm_resample_fd_list); > >>> + > >>> #define kvm_slots_lock(kml) qemu_mutex_lock(&(kml)->slots_lock) > >>> #define kvm_slots_unlock(kml) qemu_mutex_unlock(&(kml)->slots_lock) > >>> > >>> +static inline void kvm_resample_fd_remove(int gsi) > >>> +{ > >>> + KVMResampleFd *rfd; > >>> + > >>> + QLIST_FOREACH(rfd, &kvm_resample_fd_list, node) { > >>> + if (rfd->gsi == gsi) { > >>> + QLIST_REMOVE(rfd, node); > >>> + break; > >>> + } > >>> + } > >>> +} > >>> + > >>> +static inline void kvm_resample_fd_insert(int gsi, EventNotifier *event) > >>> +{ > >>> + KVMResampleFd *rfd = g_new0(KVMResampleFd, 1); > >>> + > >>> + rfd->gsi = gsi; > >>> + rfd->resample_event = event; > >>> + > >>> + QLIST_INSERT_HEAD(&kvm_resample_fd_list, rfd, node); > >>> +} > >>> + > >>> +void kvm_resample_fd_notify(int gsi) > >>> +{ > >>> + KVMResampleFd *rfd; > >>> + > >>> + if (!kvm_irqchip_is_split()) { > >>> + return; > >>> + } > >>> + > >>> + QLIST_FOREACH(rfd, &kvm_resample_fd_list, node) { > >>> + if (rfd->gsi == gsi) { > >>> + event_notifier_set(rfd->resample_event); > >>> + trace_kvm_resample_fd_notify(gsi); > >>> + break; > >>> + } > >>> + } > >>> +} > >>> + > >>> int kvm_get_max_memslots(void) > >>> { > >>> KVMState *s = KVM_STATE(current_accel()); > >>> @@ -1642,8 +1695,25 @@ static int kvm_irqchip_assign_irqfd(KVMState *s, EventNotifier *event, > >>> }; > >>> > >>> if (rfd != -1) { > >>> - irqfd.flags |= KVM_IRQFD_FLAG_RESAMPLE; > >>> - irqfd.resamplefd = rfd; > >>> + assert(assign); > >>> + if (kvm_irqchip_is_split()) { > >>> + /* > >>> + * When the slow irqchip (e.g. IOAPIC) is in the > >>> + * userspace, resamplefd will not work because the EOI of > >>> + * the interrupt will be delivered to userspace instead, > >> s/delivered to userspace/handled in userspace > > > > It will be delivered to userspace by KVM_EXIT_IOAPIC_EOI, so > > maybe... "delivered and handled"? > ah ok. TBH I don't really know how the split irqchip works and that may > explain below misunderstandings. > > > >>> + * the KVM resample fd kick is skipped. The userspace > >>> + * needs to remember the resamplefd and kick it when we > >>> + * receive EOI of this IRQ. > >> Practically we now talk about a VFIO ACTION_UNMASK classical eventfd > >> As such isn't it a bit weird to handle those normal UNMASK eventfds in > >> the KVM code? > > > > I'm not sure I completely get the question, but this should be > > something general to KVM resamplefd support. In other words, this > > should also fix other devices (besides VFIO) when they're using the > > KVM resamplefd, because IMHO it's the resamplefd and split irqchip > > which is really broken here. > Here is my understanding (& memories): the KVM resamplefd is an eventfd > you register to KVM so that KVM triggers the resamplefd when KVM traps > the EOI. Here I understand this is the userspace IOAPIC that traps the > EOI and not the in-kernel virtual interrupt controller. So I would have > expected you just need to signal the VFIO UNMASK eventfd to re-enable > the physical IRQ (which was automasked). This is no more a KVM > resamplefd strictly speaking as KVM is not involved anymore in the > deactivation process. Yes KVM kernel side should not be involed when we're using split irqchip in this case. However it should still belongs to the work of the userspace KVM module (kvm-all.c) so that it can still "mimic" the resamplefd feature that KVM_IRQFD provides. > > > > With that in mind, I think KVM should not need to even know what's > > behind the resamplefd (in VFIO's case, it's the UNMASK eventfd). It > > just needs to kick it when IOAPIC EOI comes for the specific IRQ > But above the userspace directly calls > event_notifier_set(rfd->resample_event); > > This is not KVM anymore that "kicks it". Or maybe I miss something. So > my comment was, why is it handled in the QEMU KVM layer? It's my fault to be unclear on using "KVM" above. I should really say it as kvm-all.c, say, the QEMU layer for the kernel KVM module. Indeed this problem is complicated... let me try to summarize. Firstly KVM split irqchip and resamplefd is not really going to work in the kernel (I think we just overlooked that when introducing the 2nd feature, no matter which one comes first), because the resample operation should be part of IOAPIC EOI, nevertheless when using split irqchip IOAPIC is in userspace. After we noticed this, Alex somewhere proposed to disable that in KVM, which is actually the 1st kernel patch (654f1f13ea56). We should (at the same time) propose patch 1 too in this series but I guess everybody just forgot this afterwards (Paolo actually proposed mostly the whole solution but I guess it got forgotten too)... About the fast path speedup: the main logic should be to mimic the same resamplefd feature as provided by KVM_IRQFD but this time only in the userspace. However now we're implementing the same logic only within userspace kvm-all.c, and the kernel KVM should be totally not aware of this. Doing that benefits us in that the KVM interface in QEMU does not need to be changed (majorly kvm_irqchip_assign_irqfd()). What we need to do is just to wire up the userspace IOAPIC with these resamplefds. And the idea is actually the same too - someone (VFIO) wants to have one fd (which is the resamplefd) kicked when EOI comes when requesting for a KVM irqfd, no matter who's going to kick it (kernel KVM or userspace). That's all. > . > > > >> > >> > >>> + */ > >>> + kvm_resample_fd_insert(virq, resample); > >>> + } else { > >>> + irqfd.flags |= KVM_IRQFD_FLAG_RESAMPLE; > >>> + irqfd.resamplefd = rfd; > >>> + } > >>> + } else if (!assign) { > >>> + if (kvm_irqchip_is_split()) { > >>> + kvm_resample_fd_remove(virq); > >>> + } > >>> } > >>> > >>> if (!kvm_irqfds_enabled()) { > >>> diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events > >>> index 4fb6e59d19..a68eb66534 100644 > >>> --- a/accel/kvm/trace-events > >>> +++ b/accel/kvm/trace-events > >>> @@ -16,4 +16,5 @@ kvm_set_ioeventfd_mmio(int fd, uint64_t addr, uint32_t val, bool assign, uint32_ > >>> kvm_set_ioeventfd_pio(int fd, uint16_t addr, uint32_t val, bool assign, uint32_t size, bool datamatch) "fd: %d @0x%x val=0x%x assign: %d size: %d match: %d" > >>> kvm_set_user_memory(uint32_t slot, uint32_t flags, uint64_t guest_phys_addr, uint64_t memory_size, uint64_t userspace_addr, int ret) "Slot#%d flags=0x%x gpa=0x%"PRIx64 " size=0x%"PRIx64 " ua=0x%"PRIx64 " ret=%d" > >>> kvm_clear_dirty_log(uint32_t slot, uint64_t start, uint32_t size) "slot#%"PRId32" start 0x%"PRIx64" size 0x%"PRIx32 > >>> +kvm_resample_fd_notify(int gsi) "gsi %d" > >>> > >>> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c > >>> index 15747fe2c2..8c75465c62 100644 > >>> --- a/hw/intc/ioapic.c > >>> +++ b/hw/intc/ioapic.c > >>> @@ -236,8 +236,15 @@ void ioapic_eoi_broadcast(int vector) > >>> for (n = 0; n < IOAPIC_NUM_PINS; n++) { > >>> entry = s->ioredtbl[n]; > >>> > >>> - if ((entry & IOAPIC_VECTOR_MASK) != vector || > >>> - ((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1) != IOAPIC_TRIGGER_LEVEL) { > >>> + if ((entry & IOAPIC_VECTOR_MASK) != vector) { > >>> + continue; > >>> + } > >>> + > >>> + /* Kick resamplefd if KVM is bypassed */ > >>> + kvm_resample_fd_notify(n); > >> KVM is bypassed on the deactivation path but still we call > >> kvm_resample_fd_notify(). > > > > Yes I wanted to say that the kernel won't be able to kick the > > resamplefd. How about: > > > > When IOAPIC is in the userspace (while APIC is still in the kernel), > > we need to kick the resamplefd to deactivate the IRQ for KVM. > This fd "just" aims at unmasking the IRQ at physical level (UNMASK VFIO > event)? Does it perform anything related to the virtual interrupt > controller? It should not. Thanks,
Hi Peter, On 2/27/20 8:19 PM, Peter Xu wrote: > On Thu, Feb 27, 2020 at 07:22:08PM +0100, Auger Eric wrote: >> Hi Peter, >> >> On 2/27/20 7:00 PM, Peter Xu wrote: >>> On Thu, Feb 27, 2020 at 06:42:09PM +0100, Auger Eric wrote: >>>> Hi Peter, >>>> >>>> On 2/27/20 6:00 PM, Peter Xu wrote: >>>>> This is majorly only for X86 because that's the only one that supports >>>>> split irqchip for now. >>>>> >>>>> When the irqchip is split, we face a dilemma that KVM irqfd will be >>>>> enabled, however the slow irqchip is still running in the userspace. >>>>> It means that the resamplefd in the kernel irqfds won't take any >>>>> effect and it can miss to ack INTx interrupts on EOIs. >>>> Won't it always fail to ack INTx? With the above sentence I understand >>>> it can work sometimes? >>> >>> I wanted to mean that it will fail. How about s/can/will/? Or even >>> better wordings that you'd suggest? >> yes: s/can/will >>> >>>>> >>>>> One example is split irqchip with VFIO INTx, which will break if we >>>>> use the VFIO INTx fast path. >>>>> >>>>> This patch can potentially supports the VFIO fast path again for INTx, >>>>> that the IRQ delivery will still use the fast path, while we don't >>>>> need to trap MMIOs in QEMU for the device to emulate the EIOs (see the >>>>> callers of vfio_eoi() hook). However the EOI of the INTx will still >>>>> need to be done from the userspace by caching all the resamplefds in >>>>> QEMU and kick properly for IOAPIC EOI broadcast. >>>> If I understand correctly this is a one way fast path? Fast path is on >>>> the trigger side only: VFIO -> KVM but not on the deactivation side, >>>> trapped by the userspace IOAPIC where you directly notify the UNMASK >>>> eventfd from userspace. Is that correct? >>> >>> Right, the injection is still using the whole fast path. However >>> AFAIU even for the EOI path it should still be faster than the pure >>> slow path of vfio INTx EIO. From what I got from reading the code, >>> the slow path will conditionally unmap MMIO regions (with a timer to >>> delay the recovery) so all MMIOs will be slowed down. For what this >>> patch is doing, it will need to exit to userspace for sure for each >>> EOI (after all IOAPIC is in userspace), however for the whole >>> lifecycle of the device, the MMIO regions should always be mapped so >>> no unwanted MMIO traps. >> Yes the EOI is trapped on IOAPIC side and not at the BAR level. So it >> should be more efficient and more precise. > > Yes. > >>> >>>>> >>>>> When the userspace is responsible for the resamplefd kickup, don't >>>>> register it on the kvm_irqfd anymore, because on newer kernels (after >>>>> commit 654f1f13ea56, 5.2+) the KVM_IRQFD will fail if with both split >>>>> irqchip and resamplefd. This will make sure that the fast path will >>>>> work for all supported kernels. >>>>> >>>>> https://patchwork.kernel.org/patch/10738541/#22609933 >>>>> >>>>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >>>>> Signed-off-by: Peter Xu <peterx@redhat.com> >>>>> --- >>>>> v1.1 changelog: >>>>> - when resamplefd is going to be kicked from userspace, don't register >>>>> it again in KVM_IRQFD. Tested against upstream kernel. >>>>> >>>>> accel/kvm/kvm-all.c | 74 ++++++++++++++++++++++++++++++++++++++++-- >>>>> accel/kvm/trace-events | 1 + >>>>> hw/intc/ioapic.c | 11 +++++-- >>>>> include/sysemu/kvm.h | 4 +++ >>>>> 4 files changed, 86 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >>>>> index d49b74512a..b766b6e93c 100644 >>>>> --- a/accel/kvm/kvm-all.c >>>>> +++ b/accel/kvm/kvm-all.c >>>>> @@ -159,9 +159,62 @@ static const KVMCapabilityInfo kvm_required_capabilites[] = { >>>>> static NotifierList kvm_irqchip_change_notifiers = >>>>> NOTIFIER_LIST_INITIALIZER(kvm_irqchip_change_notifiers); >>>>> >>>>> +struct KVMResampleFd { >>>>> + int gsi; >>>>> + EventNotifier *resample_event; >>>>> + QLIST_ENTRY(KVMResampleFd) node; >>>>> +}; >>>>> +typedef struct KVMResampleFd KVMResampleFd; >>>>> + >>>>> +/* >>>>> + * Only used with split irqchip where we need to do the resample fd >>>>> + * kick for the kernel from userspace. >>>>> + */ >>>>> +static QLIST_HEAD(, KVMResampleFd) kvm_resample_fd_list = >>>>> + QLIST_HEAD_INITIALIZER(kvm_resample_fd_list); >>>>> + >>>>> #define kvm_slots_lock(kml) qemu_mutex_lock(&(kml)->slots_lock) >>>>> #define kvm_slots_unlock(kml) qemu_mutex_unlock(&(kml)->slots_lock) >>>>> >>>>> +static inline void kvm_resample_fd_remove(int gsi) >>>>> +{ >>>>> + KVMResampleFd *rfd; >>>>> + >>>>> + QLIST_FOREACH(rfd, &kvm_resample_fd_list, node) { >>>>> + if (rfd->gsi == gsi) { >>>>> + QLIST_REMOVE(rfd, node); >>>>> + break; >>>>> + } >>>>> + } >>>>> +} >>>>> + >>>>> +static inline void kvm_resample_fd_insert(int gsi, EventNotifier *event) >>>>> +{ >>>>> + KVMResampleFd *rfd = g_new0(KVMResampleFd, 1); >>>>> + >>>>> + rfd->gsi = gsi; >>>>> + rfd->resample_event = event; >>>>> + >>>>> + QLIST_INSERT_HEAD(&kvm_resample_fd_list, rfd, node); >>>>> +} >>>>> + >>>>> +void kvm_resample_fd_notify(int gsi) >>>>> +{ >>>>> + KVMResampleFd *rfd; >>>>> + >>>>> + if (!kvm_irqchip_is_split()) { >>>>> + return; >>>>> + } >>>>> + >>>>> + QLIST_FOREACH(rfd, &kvm_resample_fd_list, node) { >>>>> + if (rfd->gsi == gsi) { >>>>> + event_notifier_set(rfd->resample_event); >>>>> + trace_kvm_resample_fd_notify(gsi); >>>>> + break; >>>>> + } >>>>> + } >>>>> +} >>>>> + >>>>> int kvm_get_max_memslots(void) >>>>> { >>>>> KVMState *s = KVM_STATE(current_accel()); >>>>> @@ -1642,8 +1695,25 @@ static int kvm_irqchip_assign_irqfd(KVMState *s, EventNotifier *event, >>>>> }; >>>>> >>>>> if (rfd != -1) { >>>>> - irqfd.flags |= KVM_IRQFD_FLAG_RESAMPLE; >>>>> - irqfd.resamplefd = rfd; >>>>> + assert(assign); >>>>> + if (kvm_irqchip_is_split()) { >>>>> + /* >>>>> + * When the slow irqchip (e.g. IOAPIC) is in the >>>>> + * userspace, resamplefd will not work because the EOI of >>>>> + * the interrupt will be delivered to userspace instead, >>>> s/delivered to userspace/handled in userspace >>> >>> It will be delivered to userspace by KVM_EXIT_IOAPIC_EOI, so >>> maybe... "delivered and handled"? >> ah ok. TBH I don't really know how the split irqchip works and that may >> explain below misunderstandings. >>> >>>>> + * the KVM resample fd kick is skipped. The userspace >>>>> + * needs to remember the resamplefd and kick it when we >>>>> + * receive EOI of this IRQ. >>>> Practically we now talk about a VFIO ACTION_UNMASK classical eventfd >>>> As such isn't it a bit weird to handle those normal UNMASK eventfds in >>>> the KVM code? >>> >>> I'm not sure I completely get the question, but this should be >>> something general to KVM resamplefd support. In other words, this >>> should also fix other devices (besides VFIO) when they're using the >>> KVM resamplefd, because IMHO it's the resamplefd and split irqchip >>> which is really broken here. >> Here is my understanding (& memories): the KVM resamplefd is an eventfd >> you register to KVM so that KVM triggers the resamplefd when KVM traps >> the EOI. Here I understand this is the userspace IOAPIC that traps the >> EOI and not the in-kernel virtual interrupt controller. So I would have >> expected you just need to signal the VFIO UNMASK eventfd to re-enable >> the physical IRQ (which was automasked). This is no more a KVM >> resamplefd strictly speaking as KVM is not involved anymore in the >> deactivation process. > > Yes KVM kernel side should not be involed when we're using split > irqchip in this case. However it should still belongs to the work of > the userspace KVM module (kvm-all.c) so that it can still "mimic" the > resamplefd feature that KVM_IRQFD provides. OK. So that what my actual question. Should this be handled by kvm-all.c? > >>> >>> With that in mind, I think KVM should not need to even know what's >>> behind the resamplefd (in VFIO's case, it's the UNMASK eventfd). It >>> just needs to kick it when IOAPIC EOI comes for the specific IRQ >> But above the userspace directly calls >> event_notifier_set(rfd->resample_event); >> >> This is not KVM anymore that "kicks it". Or maybe I miss something. So >> my comment was, why is it handled in the QEMU KVM layer? > > It's my fault to be unclear on using "KVM" above. I should really say > it as kvm-all.c, say, the QEMU layer for the kernel KVM module. > > Indeed this problem is complicated... let me try to summarize. > > Firstly KVM split irqchip and resamplefd is not really going to work > in the kernel (I think we just overlooked that when introducing the > 2nd feature, no matter which one comes first), because the resample > operation should be part of IOAPIC EOI, nevertheless when using split > irqchip IOAPIC is in userspace. > > After we noticed this, Alex somewhere proposed to disable that in KVM, > which is actually the 1st kernel patch (654f1f13ea56). > > We should (at the same time) propose patch 1 too in this series but I > guess everybody just forgot this afterwards (Paolo actually proposed > mostly the whole solution but I guess it got forgotten too)... > > About the fast path speedup: the main logic should be to mimic the > same resamplefd feature as provided by KVM_IRQFD but this time only in > the userspace. However now we're implementing the same logic only > within userspace kvm-all.c, and the kernel KVM should be totally not > aware of this. Doing that benefits us in that the KVM interface in > QEMU does not need to be changed (majorly kvm_irqchip_assign_irqfd()). > What we need to do is just to wire up the userspace IOAPIC with these > resamplefds. And the idea is actually the same too - someone (VFIO) > wants to have one fd (which is the resamplefd) kicked when EOI comes > when requesting for a KVM irqfd, no matter who's going to kick it > (kernel KVM or userspace). That's all. Yep I think it makes sense to accelerate the trigger path. And for the EOI path if you have means to trap this on the userspace irqchip it looks better than doing the map/unmap dance. So it looks a good iead to me. Now shall it be in kvm-all.c or elsewhere, to me it is not the most important, as long as we reach a consensus and the scheme gets documented somewhere. Thanks Eric > >> . >>> >>>> >>>> >>>>> + */ >>>>> + kvm_resample_fd_insert(virq, resample); >>>>> + } else { >>>>> + irqfd.flags |= KVM_IRQFD_FLAG_RESAMPLE; >>>>> + irqfd.resamplefd = rfd; >>>>> + } >>>>> + } else if (!assign) { >>>>> + if (kvm_irqchip_is_split()) { >>>>> + kvm_resample_fd_remove(virq); >>>>> + } >>>>> } >>>>> >>>>> if (!kvm_irqfds_enabled()) { >>>>> diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events >>>>> index 4fb6e59d19..a68eb66534 100644 >>>>> --- a/accel/kvm/trace-events >>>>> +++ b/accel/kvm/trace-events >>>>> @@ -16,4 +16,5 @@ kvm_set_ioeventfd_mmio(int fd, uint64_t addr, uint32_t val, bool assign, uint32_ >>>>> kvm_set_ioeventfd_pio(int fd, uint16_t addr, uint32_t val, bool assign, uint32_t size, bool datamatch) "fd: %d @0x%x val=0x%x assign: %d size: %d match: %d" >>>>> kvm_set_user_memory(uint32_t slot, uint32_t flags, uint64_t guest_phys_addr, uint64_t memory_size, uint64_t userspace_addr, int ret) "Slot#%d flags=0x%x gpa=0x%"PRIx64 " size=0x%"PRIx64 " ua=0x%"PRIx64 " ret=%d" >>>>> kvm_clear_dirty_log(uint32_t slot, uint64_t start, uint32_t size) "slot#%"PRId32" start 0x%"PRIx64" size 0x%"PRIx32 >>>>> +kvm_resample_fd_notify(int gsi) "gsi %d" >>>>> >>>>> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c >>>>> index 15747fe2c2..8c75465c62 100644 >>>>> --- a/hw/intc/ioapic.c >>>>> +++ b/hw/intc/ioapic.c >>>>> @@ -236,8 +236,15 @@ void ioapic_eoi_broadcast(int vector) >>>>> for (n = 0; n < IOAPIC_NUM_PINS; n++) { >>>>> entry = s->ioredtbl[n]; >>>>> >>>>> - if ((entry & IOAPIC_VECTOR_MASK) != vector || >>>>> - ((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1) != IOAPIC_TRIGGER_LEVEL) { >>>>> + if ((entry & IOAPIC_VECTOR_MASK) != vector) { >>>>> + continue; >>>>> + } >>>>> + >>>>> + /* Kick resamplefd if KVM is bypassed */ >>>>> + kvm_resample_fd_notify(n); >>>> KVM is bypassed on the deactivation path but still we call >>>> kvm_resample_fd_notify(). >>> >>> Yes I wanted to say that the kernel won't be able to kick the >>> resamplefd. How about: >>> >>> When IOAPIC is in the userspace (while APIC is still in the kernel), >>> we need to kick the resamplefd to deactivate the IRQ for KVM. >> This fd "just" aims at unmasking the IRQ at physical level (UNMASK VFIO >> event)? Does it perform anything related to the virtual interrupt >> controller? > > It should not. > > Thanks, >
On Thu, Feb 27, 2020 at 10:14:47PM +0100, Auger Eric wrote: > Hi Peter, Hi, Eric, [...] > >>>>> + * the KVM resample fd kick is skipped. The userspace > >>>>> + * needs to remember the resamplefd and kick it when we > >>>>> + * receive EOI of this IRQ. > >>>> Practically we now talk about a VFIO ACTION_UNMASK classical eventfd > >>>> As such isn't it a bit weird to handle those normal UNMASK eventfds in > >>>> the KVM code? > >>> > >>> I'm not sure I completely get the question, but this should be > >>> something general to KVM resamplefd support. In other words, this > >>> should also fix other devices (besides VFIO) when they're using the > >>> KVM resamplefd, because IMHO it's the resamplefd and split irqchip > >>> which is really broken here. > >> Here is my understanding (& memories): the KVM resamplefd is an eventfd > >> you register to KVM so that KVM triggers the resamplefd when KVM traps > >> the EOI. Here I understand this is the userspace IOAPIC that traps the > >> EOI and not the in-kernel virtual interrupt controller. So I would have > >> expected you just need to signal the VFIO UNMASK eventfd to re-enable > >> the physical IRQ (which was automasked). This is no more a KVM > >> resamplefd strictly speaking as KVM is not involved anymore in the > >> deactivation process. > > > > Yes KVM kernel side should not be involed when we're using split > > irqchip in this case. However it should still belongs to the work of > > the userspace KVM module (kvm-all.c) so that it can still "mimic" the > > resamplefd feature that KVM_IRQFD provides. > OK. So that what my actual question. Should this be handled by kvm-all.c? It should fix KVM split irqchip with resamplefd, so I think it's natural to do this in kvm-all.c (I'm a bit puzzled on where else we can put this... :). Or did I misunderstood your question? > > > >>> > >>> With that in mind, I think KVM should not need to even know what's > >>> behind the resamplefd (in VFIO's case, it's the UNMASK eventfd). It > >>> just needs to kick it when IOAPIC EOI comes for the specific IRQ > >> But above the userspace directly calls > >> event_notifier_set(rfd->resample_event); > >> > >> This is not KVM anymore that "kicks it". Or maybe I miss something. So > >> my comment was, why is it handled in the QEMU KVM layer? > > > > It's my fault to be unclear on using "KVM" above. I should really say > > it as kvm-all.c, say, the QEMU layer for the kernel KVM module. > > > > Indeed this problem is complicated... let me try to summarize. > > > > Firstly KVM split irqchip and resamplefd is not really going to work > > in the kernel (I think we just overlooked that when introducing the > > 2nd feature, no matter which one comes first), because the resample > > operation should be part of IOAPIC EOI, nevertheless when using split > > irqchip IOAPIC is in userspace. > > > > After we noticed this, Alex somewhere proposed to disable that in KVM, > > which is actually the 1st kernel patch (654f1f13ea56). > > > > We should (at the same time) propose patch 1 too in this series but I > > guess everybody just forgot this afterwards (Paolo actually proposed > > mostly the whole solution but I guess it got forgotten too)... > > > > About the fast path speedup: the main logic should be to mimic the > > same resamplefd feature as provided by KVM_IRQFD but this time only in > > the userspace. However now we're implementing the same logic only > > within userspace kvm-all.c, and the kernel KVM should be totally not > > aware of this. Doing that benefits us in that the KVM interface in > > QEMU does not need to be changed (majorly kvm_irqchip_assign_irqfd()). > > What we need to do is just to wire up the userspace IOAPIC with these > > resamplefds. And the idea is actually the same too - someone (VFIO) > > wants to have one fd (which is the resamplefd) kicked when EOI comes > > when requesting for a KVM irqfd, no matter who's going to kick it > > (kernel KVM or userspace). That's all. > > Yep I think it makes sense to accelerate the trigger path. And for the > EOI path if you have means to trap this on the userspace irqchip it > looks better than doing the map/unmap dance. So it looks a good iead to > me. Now shall it be in kvm-all.c or elsewhere, to me it is not the most > important, as long as we reach a consensus and the scheme gets > documented somewhere. Sure. For documentation: as mentioned above, I think the irqfd users will always use the interface just like before, and the resamplefd should work exactly like what KVM_IRQFD and kvm_irqchip_assign_irqfd() was offering before this patch too. IMO it'll just start to work even for split irqchips which was silently broken without being noticed. Thanks,
On 27/02/20 22:14, Auger Eric wrote: >> Yes KVM kernel side should not be involed when we're using split >> irqchip in this case. However it should still belongs to the work of >> the userspace KVM module (kvm-all.c) so that it can still "mimic" the >> resamplefd feature that KVM_IRQFD provides. > OK. So that what my actual question. Should this be handled by kvm-all.c? I think it should; kvm-all.c in this case is providing the API to enable irqfds (including resamplefds). You could have a generic file descriptor<->interrupt routing subsystem, but for now that only exists for KVM so that's where Peter's code need to go. Paolo
Hi Paolo, On 2/28/20 11:34 AM, Paolo Bonzini wrote: > On 27/02/20 22:14, Auger Eric wrote: >>> Yes KVM kernel side should not be involed when we're using split >>> irqchip in this case. However it should still belongs to the work of >>> the userspace KVM module (kvm-all.c) so that it can still "mimic" the >>> resamplefd feature that KVM_IRQFD provides. >> OK. So that what my actual question. Should this be handled by kvm-all.c? > > I think it should; kvm-all.c in this case is providing the API to enable > irqfds (including resamplefds). > > You could have a generic file descriptor<->interrupt routing subsystem, > but for now that only exists for KVM so that's where Peter's code need > to go. OK Thanks Eric > > Paolo >
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index d49b74512a..b766b6e93c 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -159,9 +159,62 @@ static const KVMCapabilityInfo kvm_required_capabilites[] = { static NotifierList kvm_irqchip_change_notifiers = NOTIFIER_LIST_INITIALIZER(kvm_irqchip_change_notifiers); +struct KVMResampleFd { + int gsi; + EventNotifier *resample_event; + QLIST_ENTRY(KVMResampleFd) node; +}; +typedef struct KVMResampleFd KVMResampleFd; + +/* + * Only used with split irqchip where we need to do the resample fd + * kick for the kernel from userspace. + */ +static QLIST_HEAD(, KVMResampleFd) kvm_resample_fd_list = + QLIST_HEAD_INITIALIZER(kvm_resample_fd_list); + #define kvm_slots_lock(kml) qemu_mutex_lock(&(kml)->slots_lock) #define kvm_slots_unlock(kml) qemu_mutex_unlock(&(kml)->slots_lock) +static inline void kvm_resample_fd_remove(int gsi) +{ + KVMResampleFd *rfd; + + QLIST_FOREACH(rfd, &kvm_resample_fd_list, node) { + if (rfd->gsi == gsi) { + QLIST_REMOVE(rfd, node); + break; + } + } +} + +static inline void kvm_resample_fd_insert(int gsi, EventNotifier *event) +{ + KVMResampleFd *rfd = g_new0(KVMResampleFd, 1); + + rfd->gsi = gsi; + rfd->resample_event = event; + + QLIST_INSERT_HEAD(&kvm_resample_fd_list, rfd, node); +} + +void kvm_resample_fd_notify(int gsi) +{ + KVMResampleFd *rfd; + + if (!kvm_irqchip_is_split()) { + return; + } + + QLIST_FOREACH(rfd, &kvm_resample_fd_list, node) { + if (rfd->gsi == gsi) { + event_notifier_set(rfd->resample_event); + trace_kvm_resample_fd_notify(gsi); + break; + } + } +} + int kvm_get_max_memslots(void) { KVMState *s = KVM_STATE(current_accel()); @@ -1642,8 +1695,25 @@ static int kvm_irqchip_assign_irqfd(KVMState *s, EventNotifier *event, }; if (rfd != -1) { - irqfd.flags |= KVM_IRQFD_FLAG_RESAMPLE; - irqfd.resamplefd = rfd; + assert(assign); + if (kvm_irqchip_is_split()) { + /* + * When the slow irqchip (e.g. IOAPIC) is in the + * userspace, resamplefd will not work because the EOI of + * the interrupt will be delivered to userspace instead, + * the KVM resample fd kick is skipped. The userspace + * needs to remember the resamplefd and kick it when we + * receive EOI of this IRQ. + */ + kvm_resample_fd_insert(virq, resample); + } else { + irqfd.flags |= KVM_IRQFD_FLAG_RESAMPLE; + irqfd.resamplefd = rfd; + } + } else if (!assign) { + if (kvm_irqchip_is_split()) { + kvm_resample_fd_remove(virq); + } } if (!kvm_irqfds_enabled()) { diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events index 4fb6e59d19..a68eb66534 100644 --- a/accel/kvm/trace-events +++ b/accel/kvm/trace-events @@ -16,4 +16,5 @@ kvm_set_ioeventfd_mmio(int fd, uint64_t addr, uint32_t val, bool assign, uint32_ kvm_set_ioeventfd_pio(int fd, uint16_t addr, uint32_t val, bool assign, uint32_t size, bool datamatch) "fd: %d @0x%x val=0x%x assign: %d size: %d match: %d" kvm_set_user_memory(uint32_t slot, uint32_t flags, uint64_t guest_phys_addr, uint64_t memory_size, uint64_t userspace_addr, int ret) "Slot#%d flags=0x%x gpa=0x%"PRIx64 " size=0x%"PRIx64 " ua=0x%"PRIx64 " ret=%d" kvm_clear_dirty_log(uint32_t slot, uint64_t start, uint32_t size) "slot#%"PRId32" start 0x%"PRIx64" size 0x%"PRIx32 +kvm_resample_fd_notify(int gsi) "gsi %d" diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c index 15747fe2c2..8c75465c62 100644 --- a/hw/intc/ioapic.c +++ b/hw/intc/ioapic.c @@ -236,8 +236,15 @@ void ioapic_eoi_broadcast(int vector) for (n = 0; n < IOAPIC_NUM_PINS; n++) { entry = s->ioredtbl[n]; - if ((entry & IOAPIC_VECTOR_MASK) != vector || - ((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1) != IOAPIC_TRIGGER_LEVEL) { + if ((entry & IOAPIC_VECTOR_MASK) != vector) { + continue; + } + + /* Kick resamplefd if KVM is bypassed */ + kvm_resample_fd_notify(n); + + if (((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1) != + IOAPIC_TRIGGER_LEVEL) { continue; } diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index 141342de98..b67552c047 100644 --- a/include/sysemu/kvm.h +++ b/include/sysemu/kvm.h @@ -555,4 +555,8 @@ int kvm_set_one_reg(CPUState *cs, uint64_t id, void *source); int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target); struct ppc_radix_page_info *kvm_get_radix_page_info(void); int kvm_get_max_memslots(void); + +/* Notify resamplefd for EOI of specific level triggered interrupts */ +void kvm_resample_fd_notify(int gsi); + #endif
This is majorly only for X86 because that's the only one that supports split irqchip for now. When the irqchip is split, we face a dilemma that KVM irqfd will be enabled, however the slow irqchip is still running in the userspace. It means that the resamplefd in the kernel irqfds won't take any effect and it can miss to ack INTx interrupts on EOIs. One example is split irqchip with VFIO INTx, which will break if we use the VFIO INTx fast path. This patch can potentially supports the VFIO fast path again for INTx, that the IRQ delivery will still use the fast path, while we don't need to trap MMIOs in QEMU for the device to emulate the EIOs (see the callers of vfio_eoi() hook). However the EOI of the INTx will still need to be done from the userspace by caching all the resamplefds in QEMU and kick properly for IOAPIC EOI broadcast. When the userspace is responsible for the resamplefd kickup, don't register it on the kvm_irqfd anymore, because on newer kernels (after commit 654f1f13ea56, 5.2+) the KVM_IRQFD will fail if with both split irqchip and resamplefd. This will make sure that the fast path will work for all supported kernels. https://patchwork.kernel.org/patch/10738541/#22609933 Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- v1.1 changelog: - when resamplefd is going to be kicked from userspace, don't register it again in KVM_IRQFD. Tested against upstream kernel. accel/kvm/kvm-all.c | 74 ++++++++++++++++++++++++++++++++++++++++-- accel/kvm/trace-events | 1 + hw/intc/ioapic.c | 11 +++++-- include/sysemu/kvm.h | 4 +++ 4 files changed, 86 insertions(+), 4 deletions(-)