| Message ID | 20240116223157.73752-2-mjrosato@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x/pci: fix ISM reset | expand |
On Tue, 2024-01-16 at 17:31 -0500, Matthew Rosato wrote: > Use a flag to keep track of whether AIF is currently enabled. This > can be > used to avoid enabling/disabling AIF multiple times as well as to > determine > whether or not it should be disabled during reset processing. > > Fixes: d0bc7091c2 ("s390x/pci: enable adapter event notification for > interpreted devices") > Reported-by: Cédric Le Goater <clg@redhat.com> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > hw/s390x/s390-pci-kvm.c | 25 +++++++++++++++++++++++-- > include/hw/s390x/s390-pci-bus.h | 1 + > 2 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/hw/s390x/s390-pci-kvm.c b/hw/s390x/s390-pci-kvm.c > index ff41e4106d..f7e10cfa72 100644 > --- a/hw/s390x/s390-pci-kvm.c > +++ b/hw/s390x/s390-pci-kvm.c > @@ -27,6 +27,7 @@ bool s390_pci_kvm_interp_allowed(void) > > int s390_pci_kvm_aif_enable(S390PCIBusDevice *pbdev, ZpciFib *fib, > bool assist) > { > + int rc; > struct kvm_s390_zpci_op args = { > .fh = pbdev->fh, > .op = KVM_S390_ZPCIOP_REG_AEN, > @@ -38,15 +39,35 @@ int s390_pci_kvm_aif_enable(S390PCIBusDevice > *pbdev, ZpciFib *fib, bool assist) > .u.reg_aen.flags = (assist) ? 0 : > KVM_S390_ZPCIOP_REGAEN_HOST > }; > > - return kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args); > + if (pbdev->aif) { > + return -EINVAL; > + } > + > + rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args); > + if (rc == 0) { > + pbdev->aif = true; > + } > + > + return rc; > } > > int s390_pci_kvm_aif_disable(S390PCIBusDevice *pbdev) > { > + int rc; > + > struct kvm_s390_zpci_op args = { > .fh = pbdev->fh, > .op = KVM_S390_ZPCIOP_DEREG_AEN > }; > > - return kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args); > + if (!pbdev->aif) { > + return -EINVAL; > + } > + > + rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args); > + if (rc == 0) { > + pbev->aif = false; s/pbev/pbdev/ You fix this in patch 2. :) With that fixed: Reviewed-by: Eric Farman <farman@linux.ibm.com> > + } > + > + return rc; > } > diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390- > pci-bus.h > index b1bdbeaeb5..435e788867 100644 > --- a/include/hw/s390x/s390-pci-bus.h > +++ b/include/hw/s390x/s390-pci-bus.h > @@ -361,6 +361,7 @@ struct S390PCIBusDevice { > bool unplug_requested; > bool interp; > bool forwarding_assist; > + bool aif; > QTAILQ_ENTRY(S390PCIBusDevice) link; > }; >
On 1/16/24 23:31, Matthew Rosato wrote: > Use a flag to keep track of whether AIF is currently enabled. This can be > used to avoid enabling/disabling AIF multiple times as well as to determine > whether or not it should be disabled during reset processing. Why don't we disable AIF always at reset ? Doesn't KVM handle multiple calls to KVM_S390_ZPCIOP_DEREG_AEN cleanly ? Just asking, I am no expert there. Thanks, C. > Fixes: d0bc7091c2 ("s390x/pci: enable adapter event notification for interpreted devices") > Reported-by: Cédric Le Goater <clg@redhat.com> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > hw/s390x/s390-pci-kvm.c | 25 +++++++++++++++++++++++-- > include/hw/s390x/s390-pci-bus.h | 1 + > 2 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/hw/s390x/s390-pci-kvm.c b/hw/s390x/s390-pci-kvm.c > index ff41e4106d..f7e10cfa72 100644 > --- a/hw/s390x/s390-pci-kvm.c > +++ b/hw/s390x/s390-pci-kvm.c > @@ -27,6 +27,7 @@ bool s390_pci_kvm_interp_allowed(void) > > int s390_pci_kvm_aif_enable(S390PCIBusDevice *pbdev, ZpciFib *fib, bool assist) > { > + int rc; > struct kvm_s390_zpci_op args = { > .fh = pbdev->fh, > .op = KVM_S390_ZPCIOP_REG_AEN, > @@ -38,15 +39,35 @@ int s390_pci_kvm_aif_enable(S390PCIBusDevice *pbdev, ZpciFib *fib, bool assist) > .u.reg_aen.flags = (assist) ? 0 : KVM_S390_ZPCIOP_REGAEN_HOST > }; > > - return kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args); > + if (pbdev->aif) { > + return -EINVAL; > + } > + > + rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args); > + if (rc == 0) { > + pbdev->aif = true; > + } > + > + return rc; > } > > int s390_pci_kvm_aif_disable(S390PCIBusDevice *pbdev) > { > + int rc; > + > struct kvm_s390_zpci_op args = { > .fh = pbdev->fh, > .op = KVM_S390_ZPCIOP_DEREG_AEN > }; > > - return kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args); > + if (!pbdev->aif) { > + return -EINVAL; > + } > + > + rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args); > + if (rc == 0) { > + pbev->aif = false; > + } > + > + return rc; > } > diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h > index b1bdbeaeb5..435e788867 100644 > --- a/include/hw/s390x/s390-pci-bus.h > +++ b/include/hw/s390x/s390-pci-bus.h > @@ -361,6 +361,7 @@ struct S390PCIBusDevice { > bool unplug_requested; > bool interp; > bool forwarding_assist; > + bool aif; > QTAILQ_ENTRY(S390PCIBusDevice) link; > }; >
>> - return kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args); >> + if (!pbdev->aif) { >> + return -EINVAL; >> + } >> + >> + rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args); >> + if (rc == 0) { >> + pbev->aif = false; > > s/pbev/pbdev/ > > You fix this in patch 2. :) > Oops, thanks for catching that.
On 1/17/24 5:54 AM, Cédric Le Goater wrote: > On 1/16/24 23:31, Matthew Rosato wrote: >> Use a flag to keep track of whether AIF is currently enabled. This can be >> used to avoid enabling/disabling AIF multiple times as well as to determine >> whether or not it should be disabled during reset processing. > > Why don't we disable AIF always at reset ? Doesn't KVM handle multiple calls > to KVM_S390_ZPCIOP_DEREG_AEN cleanly ? Just asking, I am no expert there. > This may be some amount of defensive programming on my part :) Really, we're more concerned about enabling AIF twice without disabling AIF in between, and if we attempt to do so we should fail immediately rather than try messing with the hostdev. The kernel warning you were seeing was exactly because we got the guest ISC users count wrong due to a mismatch between the number of enables and disables; in a sense, that warning is the KVM cleanup handling things (by disabling the gisc that AIF was using but also spitting out the warning that we got the gisc usage count wrong). I suspect you're right in that there's room to improve the KVM code so that we can catch this earlier rather than continuing to increment/decrement the guest ISC count and waiting to catch it at the end of the VM lifecycle (the GISC count is per-VM, but we should only ever have 1 instance per-device but are allowing >1 per-device). I'll have a look at that separately from this series but IMHO QEMU should also try and 'behave' and be aware of what it previously enabled/disabled; if QEMU is trying to enable AIF multiple times then QEMU is doing something wrong.
diff --git a/hw/s390x/s390-pci-kvm.c b/hw/s390x/s390-pci-kvm.c index ff41e4106d..f7e10cfa72 100644 --- a/hw/s390x/s390-pci-kvm.c +++ b/hw/s390x/s390-pci-kvm.c @@ -27,6 +27,7 @@ bool s390_pci_kvm_interp_allowed(void) int s390_pci_kvm_aif_enable(S390PCIBusDevice *pbdev, ZpciFib *fib, bool assist) { + int rc; struct kvm_s390_zpci_op args = { .fh = pbdev->fh, .op = KVM_S390_ZPCIOP_REG_AEN, @@ -38,15 +39,35 @@ int s390_pci_kvm_aif_enable(S390PCIBusDevice *pbdev, ZpciFib *fib, bool assist) .u.reg_aen.flags = (assist) ? 0 : KVM_S390_ZPCIOP_REGAEN_HOST }; - return kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args); + if (pbdev->aif) { + return -EINVAL; + } + + rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args); + if (rc == 0) { + pbdev->aif = true; + } + + return rc; } int s390_pci_kvm_aif_disable(S390PCIBusDevice *pbdev) { + int rc; + struct kvm_s390_zpci_op args = { .fh = pbdev->fh, .op = KVM_S390_ZPCIOP_DEREG_AEN }; - return kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args); + if (!pbdev->aif) { + return -EINVAL; + } + + rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args); + if (rc == 0) { + pbev->aif = false; + } + + return rc; } diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h index b1bdbeaeb5..435e788867 100644 --- a/include/hw/s390x/s390-pci-bus.h +++ b/include/hw/s390x/s390-pci-bus.h @@ -361,6 +361,7 @@ struct S390PCIBusDevice { bool unplug_requested; bool interp; bool forwarding_assist; + bool aif; QTAILQ_ENTRY(S390PCIBusDevice) link; };
Use a flag to keep track of whether AIF is currently enabled. This can be used to avoid enabling/disabling AIF multiple times as well as to determine whether or not it should be disabled during reset processing. Fixes: d0bc7091c2 ("s390x/pci: enable adapter event notification for interpreted devices") Reported-by: Cédric Le Goater <clg@redhat.com> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> --- hw/s390x/s390-pci-kvm.c | 25 +++++++++++++++++++++++-- include/hw/s390x/s390-pci-bus.h | 1 + 2 files changed, 24 insertions(+), 2 deletions(-)