On 4/20/22 17:12, Matthew Rosato wrote:
On 4/19/22 3:47 PM, Pierre Morel wrote:
On 4/4/22 20:17, Matthew Rosato wrote:
If the appropriate CPU facilty is available as well as the necessary
ZPCI_OP ioctl, then the underlying KVM host will enable load/store
intepretation for any guest device without a SHM bit in the guest
function handle. For a device that will be using interpretation
support, ensure the guest function handle matches the host function
handle; this value is re-checked every time the guest issues a SET
PCI FN
to enable the guest device as it is the only opportunity to reflect
function handle changes.
By default, unless interpret=off is specified, interpretation support
will
always be assumed and exploited if the necessary ioctl and features are
available on the host kernel. When these are unavailable, we will
silently
revert to the interception model; this allows existing guest
configurations
to work unmodified on hosts with and without zPCI interpretation
support,
allowing QEMU to choose the best support model available.
Signed-off-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>
---
hw/s390x/meson.build | 1 +
hw/s390x/s390-pci-bus.c | 66 ++++++++++++++++++++++++++++++++-
hw/s390x/s390-pci-inst.c | 12 ++++++
hw/s390x/s390-pci-kvm.c | 21 +++++++++++
include/hw/s390x/s390-pci-bus.h | 1 +
include/hw/s390x/s390-pci-kvm.h | 24 ++++++++++++
target/s390x/kvm/kvm.c | 7 ++++
target/s390x/kvm/kvm_s390x.h | 1 +
8 files changed, 132 insertions(+), 1 deletion(-)
create mode 100644 hw/s390x/s390-pci-kvm.c
create mode 100644 include/hw/s390x/s390-pci-kvm.h
...snip...
if (s390_pci_msix_init(pbdev)) {
@@ -1360,6 +1423,7 @@ static Property s390_pci_device_properties[] = {
DEFINE_PROP_UINT16("uid", S390PCIBusDevice, uid, UID_UNDEFINED),
DEFINE_PROP_S390_PCI_FID("fid", S390PCIBusDevice, fid),
DEFINE_PROP_STRING("target", S390PCIBusDevice, target),
+ DEFINE_PROP_BOOL("interpret", S390PCIBusDevice, interp, true),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 6d400d4147..c898c8abe9 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -18,6 +18,8 @@
#include "sysemu/hw_accel.h"
#include "hw/s390x/s390-pci-inst.h"
#include "hw/s390x/s390-pci-bus.h"
+#include "hw/s390x/s390-pci-kvm.h"
+#include "hw/s390x/s390-pci-vfio.h"
#include "hw/s390x/tod.h"
#ifndef DEBUG_S390PCI_INST
@@ -246,6 +248,16 @@ int clp_service_call(S390CPU *cpu, uint8_t r2,
uintptr_t ra)
goto out;
}
+ /*
+ * Take this opportunity to make sure we still have an
accurate
+ * host fh. It's possible part of the handle changed
while the
+ * device was disabled to the guest (e.g. vfio hot reset
for
+ * ISM during plug)
+ */
+ if (pbdev->interp) {
+ /* Take this opportunity to make sure we are sync'd
with host */
+ s390_pci_get_host_fh(pbdev, &pbdev->fh);
Here we should check the return value and, AFAIU, assume that the device
disappear if it did return false.
+ }
pbdev->fh |= FH_MASK_ENABLE;
Are we sure here that the PCI device is always enabled?
Shouldn't we check?
I guess you mean the host device? Interesting thought.
So, to be clear, the idea on setting FH_MASK_ENABLE here is that we are
handling a guest CLP SET PCI FN enable so the guest fh should always
have FH_MASK_ENABLE set if we return CLP_RC_OK to the guest.
But for interpretation, if we find the host function is disabled, I
suppose we could return an error on the guest CLP (not sure which error
yet); otherwise, if we return the force-enabled handle and CLP_RC_OK as
we do here then the guest will just get errors attempting to use it.
hum, in this case can't we have a loop on
clp enable->error->clp disable->clp enable->error...
I think we should return an error if what the guest asked for could not
be done.
--
Pierre Morel
IBM Lab Boeblingen
