| Message ID | 20220114203145.242984-17-mjrosato@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: enable zPCI for interpretive execution | expand |
On 1/14/22 21:31, Matthew Rosato wrote: > In cases where interrupts are not forwarded to the guest via firmware, > KVM is responsible for ensuring delivery. When an interrupt presents > with the forwarding bit, we must process the forwarding tables until > all interrupts are delivered. > > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > arch/s390/include/asm/kvm_host.h | 1 + > arch/s390/include/asm/tpi.h | 13 ++++++ > arch/s390/kvm/interrupt.c | 76 +++++++++++++++++++++++++++++++- > arch/s390/kvm/kvm-s390.c | 3 +- > arch/s390/kvm/pci.h | 9 ++++ > 5 files changed, 100 insertions(+), 2 deletions(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index a604d51acfc8..3f147b8d050b 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -757,6 +757,7 @@ struct kvm_vm_stat { > u64 inject_pfault_done; > u64 inject_service_signal; > u64 inject_virtio; > + u64 aen_forward; > }; > > struct kvm_arch_memory_slot { > diff --git a/arch/s390/include/asm/tpi.h b/arch/s390/include/asm/tpi.h > index 1ac538b8cbf5..f76e5fdff23a 100644 > --- a/arch/s390/include/asm/tpi.h > +++ b/arch/s390/include/asm/tpi.h > @@ -19,6 +19,19 @@ struct tpi_info { > u32 :12; > } __packed __aligned(4); > > +/* I/O-Interruption Code as stored by TPI for an Adapter I/O */ > +struct tpi_adapter_info { > + u32 aism:8; > + u32 :22; > + u32 error:1; > + u32 forward:1; > + u32 reserved; > + u32 adapter_IO:1; > + u32 directed_irq:1; > + u32 isc:3; > + u32 :27; > +} __packed __aligned(4); > + > #endif /* __ASSEMBLY__ */ > > #endif /* _ASM_S390_TPI_H */ > diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c > index a591b8cd662f..07743c6a67c4 100644 > --- a/arch/s390/kvm/interrupt.c > +++ b/arch/s390/kvm/interrupt.c > @@ -3263,11 +3263,85 @@ int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc) > } > EXPORT_SYMBOL_GPL(kvm_s390_gisc_unregister); > > +static void aen_host_forward(unsigned long si) > +{ > + struct kvm_s390_gisa_interrupt *gi; > + struct zpci_gaite *gaite; > + struct kvm *kvm; > + > + gaite = (struct zpci_gaite *)aift->gait + > + (si * sizeof(struct zpci_gaite)); > + if (gaite->count == 0) > + return; > + if (gaite->aisb != 0) > + set_bit_inv(gaite->aisbo, (unsigned long *)gaite->aisb); > + > + kvm = kvm_s390_pci_si_to_kvm(aift, si); > + if (kvm == 0) > + return; > + gi = &kvm->arch.gisa_int; > + > + if (!(gi->origin->g1.simm & AIS_MODE_MASK(gaite->gisc)) || > + !(gi->origin->g1.nimm & AIS_MODE_MASK(gaite->gisc))) { > + gisa_set_ipm_gisc(gi->origin, gaite->gisc); > + if (hrtimer_active(&gi->timer)) > + hrtimer_cancel(&gi->timer); > + hrtimer_start(&gi->timer, 0, HRTIMER_MODE_REL); > + kvm->stat.aen_forward++; > + } > +} > + > +static void aen_process_gait(u8 isc) > +{ > + bool found = false, first = true; > + union zpci_sic_iib iib = {{0}}; > + unsigned long si, flags; > + > + spin_lock_irqsave(&aift->gait_lock, flags); > + > + if (!aift->gait) { > + spin_unlock_irqrestore(&aift->gait_lock, flags); > + return; > + } > + > + for (si = 0;;) { > + /* Scan adapter summary indicator bit vector */ > + si = airq_iv_scan(aift->sbv, si, airq_iv_end(aift->sbv)); > + if (si == -1UL) { > + if (first || found) { > + /* Reenable interrupts. */ > + if (zpci_set_irq_ctrl(SIC_IRQ_MODE_SINGLE, isc, > + &iib)) > + break; AFAIU this code is VFIO interpretation specific code and facility 12 is a precondition for it, so I think this break will never occur. If I am right we should not test the return value which will make the code clearer. > + first = found = false; > + } else { > + /* Interrupts on and all bits processed */ > + break; > + } May be add a comment: "rescan after re-enabling interrupts" > + found = false; > + si = 0; > + continue; > + } > + found = true; > + aen_host_forward(si); > + } > + > + spin_unlock_irqrestore(&aift->gait_lock, flags); > +} > + > static void gib_alert_irq_handler(struct airq_struct *airq, > struct tpi_info *tpi_info) > { > + struct tpi_adapter_info *info = (struct tpi_adapter_info *)tpi_info; > + > inc_irq_stat(IRQIO_GAL); > - process_gib_alert_list(); > + > + if (IS_ENABLED(CONFIG_PCI) && (info->forward || info->error)) { > + aen_process_gait(info->isc); > + if (info->aism != 0) > + process_gib_alert_list(); > + } else > + process_gib_alert_list(); NIT: I think we need braces around this statement > } > > static struct airq_struct gib_alert_irq = { > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 01dc3f6883d0..ab8b56deed11 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -65,7 +65,8 @@ const struct _kvm_stats_desc kvm_vm_stats_desc[] = { > STATS_DESC_COUNTER(VM, inject_float_mchk), > STATS_DESC_COUNTER(VM, inject_pfault_done), > STATS_DESC_COUNTER(VM, inject_service_signal), > - STATS_DESC_COUNTER(VM, inject_virtio) > + STATS_DESC_COUNTER(VM, inject_virtio), > + STATS_DESC_COUNTER(VM, aen_forward) > }; > > const struct kvm_stats_header kvm_vm_stats_header = { > diff --git a/arch/s390/kvm/pci.h b/arch/s390/kvm/pci.h > index b2000ed7b8c3..387b637863c9 100644 > --- a/arch/s390/kvm/pci.h > +++ b/arch/s390/kvm/pci.h > @@ -12,6 +12,7 @@ > > #include <linux/pci.h> > #include <linux/mutex.h> > +#include <linux/kvm_host.h> > #include <asm/airq.h> > #include <asm/kvm_pci.h> > > @@ -34,6 +35,14 @@ struct zpci_aift { > > extern struct zpci_aift *aift; > > +static inline struct kvm *kvm_s390_pci_si_to_kvm(struct zpci_aift *aift, > + unsigned long si) > +{ > + if (!IS_ENABLED(CONFIG_PCI) || aift->kzdev == 0 || aift->kzdev[si] == 0) Shouldn't it be better CONFIG_VFIO_PCI ? > + return 0; > + return aift->kzdev[si]->kvm; > +}; > + > int kvm_s390_pci_aen_init(u8 nisc); > void kvm_s390_pci_aen_exit(void); > >
On 1/17/22 12:38 PM, Pierre Morel wrote: > ... >> +static void aen_process_gait(u8 isc) >> +{ >> + bool found = false, first = true; >> + union zpci_sic_iib iib = {{0}}; >> + unsigned long si, flags; >> + >> + spin_lock_irqsave(&aift->gait_lock, flags); >> + >> + if (!aift->gait) { >> + spin_unlock_irqrestore(&aift->gait_lock, flags); >> + return; >> + } >> + >> + for (si = 0;;) { >> + /* Scan adapter summary indicator bit vector */ >> + si = airq_iv_scan(aift->sbv, si, airq_iv_end(aift->sbv)); >> + if (si == -1UL) { >> + if (first || found) { >> + /* Reenable interrupts. */ >> + if (zpci_set_irq_ctrl(SIC_IRQ_MODE_SINGLE, isc, >> + &iib)) >> + break; > > AFAIU this code is VFIO interpretation specific code and facility 12 is > a precondition for it, so I think this break will never occur. > If I am right we should not test the return value which will make the > code clearer. Yep, you are correct; we can just ignore the return value here. > >> + first = found = false; >> + } else { >> + /* Interrupts on and all bits processed */ >> + break; >> + } > > May be add a comment: "rescan after re-enabling interrupts" OK > >> + found = false; >> + si = 0; >> + continue; >> + } >> + found = true; >> + aen_host_forward(si); >> + } >> + >> + spin_unlock_irqrestore(&aift->gait_lock, flags); >> +} >> + >> static void gib_alert_irq_handler(struct airq_struct *airq, >> struct tpi_info *tpi_info) >> { >> + struct tpi_adapter_info *info = (struct tpi_adapter_info *)tpi_info; >> + >> inc_irq_stat(IRQIO_GAL); >> - process_gib_alert_list(); >> + >> + if (IS_ENABLED(CONFIG_PCI) && (info->forward || info->error)) { >> + aen_process_gait(info->isc); >> + if (info->aism != 0) >> + process_gib_alert_list(); >> + } else >> + process_gib_alert_list(); > > NIT: I think we need braces around this statement OK > >> } >> static struct airq_struct gib_alert_irq = { >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index 01dc3f6883d0..ab8b56deed11 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -65,7 +65,8 @@ const struct _kvm_stats_desc kvm_vm_stats_desc[] = { >> STATS_DESC_COUNTER(VM, inject_float_mchk), >> STATS_DESC_COUNTER(VM, inject_pfault_done), >> STATS_DESC_COUNTER(VM, inject_service_signal), >> - STATS_DESC_COUNTER(VM, inject_virtio) >> + STATS_DESC_COUNTER(VM, inject_virtio), >> + STATS_DESC_COUNTER(VM, aen_forward) >> }; >> const struct kvm_stats_header kvm_vm_stats_header = { >> diff --git a/arch/s390/kvm/pci.h b/arch/s390/kvm/pci.h >> index b2000ed7b8c3..387b637863c9 100644 >> --- a/arch/s390/kvm/pci.h >> +++ b/arch/s390/kvm/pci.h >> @@ -12,6 +12,7 @@ >> #include <linux/pci.h> >> #include <linux/mutex.h> >> +#include <linux/kvm_host.h> >> #include <asm/airq.h> >> #include <asm/kvm_pci.h> >> @@ -34,6 +35,14 @@ struct zpci_aift { >> extern struct zpci_aift *aift; >> +static inline struct kvm *kvm_s390_pci_si_to_kvm(struct zpci_aift *aift, >> + unsigned long si) >> +{ >> + if (!IS_ENABLED(CONFIG_PCI) || aift->kzdev == 0 || >> aift->kzdev[si] == 0) > > Shouldn't it be better CONFIG_VFIO_PCI ? While it's true that we can't be doing interpretation without CONFIG_VFIO_PCI=y|m, the reason I'm using CONFIG_PCI here and elsewhere in the code is because CONFIG_PCI is what is being used to determine whether or not we build arch/s390/kvm/pci.o in patch 14 (and thus whether or not the aift exists) -- And the reason we use this is because this is where the code dependencies exist (examples include ZPCI_NR_DEVICES, the AEN pieces that must be preserved over KVM module remove/insert in patch 15) If we for some reason have a case where CONFIG_KVM=y|m && CONFIG_PCI=y|m && CONFIG_VFIO_PCI=n, this will still work: aift and aift->kzdev will exist (kvm/pci.o is linked) but we will never actually drive this routine anyway because we'll never register a device for AEN forwarding without CONFIG_VFIO_PCI.
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h index a604d51acfc8..3f147b8d050b 100644 --- a/arch/s390/include/asm/kvm_host.h +++ b/arch/s390/include/asm/kvm_host.h @@ -757,6 +757,7 @@ struct kvm_vm_stat { u64 inject_pfault_done; u64 inject_service_signal; u64 inject_virtio; + u64 aen_forward; }; struct kvm_arch_memory_slot { diff --git a/arch/s390/include/asm/tpi.h b/arch/s390/include/asm/tpi.h index 1ac538b8cbf5..f76e5fdff23a 100644 --- a/arch/s390/include/asm/tpi.h +++ b/arch/s390/include/asm/tpi.h @@ -19,6 +19,19 @@ struct tpi_info { u32 :12; } __packed __aligned(4); +/* I/O-Interruption Code as stored by TPI for an Adapter I/O */ +struct tpi_adapter_info { + u32 aism:8; + u32 :22; + u32 error:1; + u32 forward:1; + u32 reserved; + u32 adapter_IO:1; + u32 directed_irq:1; + u32 isc:3; + u32 :27; +} __packed __aligned(4); + #endif /* __ASSEMBLY__ */ #endif /* _ASM_S390_TPI_H */ diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c index a591b8cd662f..07743c6a67c4 100644 --- a/arch/s390/kvm/interrupt.c +++ b/arch/s390/kvm/interrupt.c @@ -3263,11 +3263,85 @@ int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc) } EXPORT_SYMBOL_GPL(kvm_s390_gisc_unregister); +static void aen_host_forward(unsigned long si) +{ + struct kvm_s390_gisa_interrupt *gi; + struct zpci_gaite *gaite; + struct kvm *kvm; + + gaite = (struct zpci_gaite *)aift->gait + + (si * sizeof(struct zpci_gaite)); + if (gaite->count == 0) + return; + if (gaite->aisb != 0) + set_bit_inv(gaite->aisbo, (unsigned long *)gaite->aisb); + + kvm = kvm_s390_pci_si_to_kvm(aift, si); + if (kvm == 0) + return; + gi = &kvm->arch.gisa_int; + + if (!(gi->origin->g1.simm & AIS_MODE_MASK(gaite->gisc)) || + !(gi->origin->g1.nimm & AIS_MODE_MASK(gaite->gisc))) { + gisa_set_ipm_gisc(gi->origin, gaite->gisc); + if (hrtimer_active(&gi->timer)) + hrtimer_cancel(&gi->timer); + hrtimer_start(&gi->timer, 0, HRTIMER_MODE_REL); + kvm->stat.aen_forward++; + } +} + +static void aen_process_gait(u8 isc) +{ + bool found = false, first = true; + union zpci_sic_iib iib = {{0}}; + unsigned long si, flags; + + spin_lock_irqsave(&aift->gait_lock, flags); + + if (!aift->gait) { + spin_unlock_irqrestore(&aift->gait_lock, flags); + return; + } + + for (si = 0;;) { + /* Scan adapter summary indicator bit vector */ + si = airq_iv_scan(aift->sbv, si, airq_iv_end(aift->sbv)); + if (si == -1UL) { + if (first || found) { + /* Reenable interrupts. */ + if (zpci_set_irq_ctrl(SIC_IRQ_MODE_SINGLE, isc, + &iib)) + break; + first = found = false; + } else { + /* Interrupts on and all bits processed */ + break; + } + found = false; + si = 0; + continue; + } + found = true; + aen_host_forward(si); + } + + spin_unlock_irqrestore(&aift->gait_lock, flags); +} + static void gib_alert_irq_handler(struct airq_struct *airq, struct tpi_info *tpi_info) { + struct tpi_adapter_info *info = (struct tpi_adapter_info *)tpi_info; + inc_irq_stat(IRQIO_GAL); - process_gib_alert_list(); + + if (IS_ENABLED(CONFIG_PCI) && (info->forward || info->error)) { + aen_process_gait(info->isc); + if (info->aism != 0) + process_gib_alert_list(); + } else + process_gib_alert_list(); } static struct airq_struct gib_alert_irq = { diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index 01dc3f6883d0..ab8b56deed11 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -65,7 +65,8 @@ const struct _kvm_stats_desc kvm_vm_stats_desc[] = { STATS_DESC_COUNTER(VM, inject_float_mchk), STATS_DESC_COUNTER(VM, inject_pfault_done), STATS_DESC_COUNTER(VM, inject_service_signal), - STATS_DESC_COUNTER(VM, inject_virtio) + STATS_DESC_COUNTER(VM, inject_virtio), + STATS_DESC_COUNTER(VM, aen_forward) }; const struct kvm_stats_header kvm_vm_stats_header = { diff --git a/arch/s390/kvm/pci.h b/arch/s390/kvm/pci.h index b2000ed7b8c3..387b637863c9 100644 --- a/arch/s390/kvm/pci.h +++ b/arch/s390/kvm/pci.h @@ -12,6 +12,7 @@ #include <linux/pci.h> #include <linux/mutex.h> +#include <linux/kvm_host.h> #include <asm/airq.h> #include <asm/kvm_pci.h> @@ -34,6 +35,14 @@ struct zpci_aift { extern struct zpci_aift *aift; +static inline struct kvm *kvm_s390_pci_si_to_kvm(struct zpci_aift *aift, + unsigned long si) +{ + if (!IS_ENABLED(CONFIG_PCI) || aift->kzdev == 0 || aift->kzdev[si] == 0) + return 0; + return aift->kzdev[si]->kvm; +}; + int kvm_s390_pci_aen_init(u8 nisc); void kvm_s390_pci_aen_exit(void);
In cases where interrupts are not forwarded to the guest via firmware, KVM is responsible for ensuring delivery. When an interrupt presents with the forwarding bit, we must process the forwarding tables until all interrupts are delivered. Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> --- arch/s390/include/asm/kvm_host.h | 1 + arch/s390/include/asm/tpi.h | 13 ++++++ arch/s390/kvm/interrupt.c | 76 +++++++++++++++++++++++++++++++- arch/s390/kvm/kvm-s390.c | 3 +- arch/s390/kvm/pci.h | 9 ++++ 5 files changed, 100 insertions(+), 2 deletions(-)