Messages in this thread | | | Date | Tue, 18 Jan 2022 12:25:46 -0500 | Subject | Re: [PATCH v2 16/30] KVM: s390: pci: enable host forwarding of Adapter Event Notifications | From | Matthew Rosato <> |
| |
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.
| |