[Qemu-devel] [PATCH v4 00/10] zpci detangling

[Qemu-devel] [PATCH v4 00/10] zpci detangling

[Cornelia Huck]

Just some minor changes from the last one. If nobody complains, I'll
queue to s390-next.

I'd be happy about some acks/r-bs for patches that don't have any yet,
though.

v3->v4:
- add "9pfs: fix dependencies"
- in "kvm: remove hard dependency on pci", do s/assert/g_assert/ and add
  return statements for builds with asserts off

Cornelia Huck (10):
  9pfs: fix dependencies
  kvm: remove hard dependency on pci
  s390x/pci: add stubs
  s390x: chsc nt2 events are pci-only
  s390x/pci: do not advertise pci on non-pci builds
  s390x/ccw: create s390 phb conditionally
  s390x/sclp: properly guard pci-specific functions
  s390x/pci: fence off instructions for non-pci
  s390x/kvm: msi route fixup for non-pci
  s390x: refine pci dependencies

 accel/kvm/kvm-all.c               |  6 ++--
 default-configs/s390x-softmmu.mak |  2 +-
 fsdev/Makefile.objs               |  9 ++---
 hw/9pfs/Makefile.objs             |  2 +-
 hw/Makefile.objs                  |  2 +-
 hw/pci/pci-stub.c                 | 15 ++++++++
 hw/pci/pci.c                      |  2 ++
 hw/s390x/Makefile.objs            |  3 +-
 hw/s390x/s390-pci-bus.c           |  4 +--
 hw/s390x/s390-pci-bus.h           |  4 +--
 hw/s390x/s390-pci-stub.c          | 74 +++++++++++++++++++++++++++++++++++++++
 hw/s390x/s390-virtio-ccw.c        | 14 ++++----
 hw/s390x/sclp.c                   | 19 +++++++---
 include/hw/pci/pci.h              |  2 ++
 target/s390x/ioinst.c             | 16 +++++++++
 target/s390x/kvm.c                | 64 +++++++++++++++++++++++++--------
 16 files changed, 197 insertions(+), 41 deletions(-)
 create mode 100644 hw/s390x/s390-pci-stub.c

-- 
2.13.5

[Qemu-devel] [PATCH v4 01/10] 9pfs: fix dependencies

[Cornelia Huck]

Nothing in fsdev/ or hw/9pfs/ depends on pci; it should rather depend
on CONFIG_VIRTFS and CONFIG_VIRTIO/CONFIG_XEN only.

Acked-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 fsdev/Makefile.objs   | 9 +++------
 hw/9pfs/Makefile.objs | 2 +-
 hw/Makefile.objs      | 2 +-
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs
index 659df6e187..fb38017c0b 100644
--- a/fsdev/Makefile.objs
+++ b/fsdev/Makefile.objs
@@ -1,10 +1,7 @@
-ifeq ($(CONFIG_VIRTIO)$(CONFIG_VIRTFS)$(CONFIG_PCI),yyy)
 # Lots of the fsdev/9pcode is pulled in by vl.c via qemu_fsdev_add.
-# only pull in the actual virtio-9p device if we also enabled virtio.
-common-obj-y = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o
-else
-common-obj-y = qemu-fsdev-dummy.o
-endif
+# only pull in the actual 9p backend if we also enabled virtio or xen.
+common-obj-$(call land,$(CONFIG_VIRTFS),$(call lor,$(CONFIG_VIRTIO),$(CONFIG_XEN))) = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o
+common-obj-$(call lnot,$(call land,$(CONFIG_VIRTFS),$(call lor,$(CONFIG_VIRTIO),$(CONFIG_XEN)))) = qemu-fsdev-dummy.o
 common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o
 
 # Toplevel always builds this; targets without virtio will put it in
diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
index cab5e942ed..fd90b62900 100644
--- a/hw/9pfs/Makefile.objs
+++ b/hw/9pfs/Makefile.objs
@@ -7,4 +7,4 @@ common-obj-$(CONFIG_OPEN_BY_HANDLE) +=  9p-handle.o
 common-obj-y += 9p-proxy.o
 common-obj-$(CONFIG_XEN) += xen-9p-backend.o
 
-obj-y += virtio-9p-device.o
+obj-$(CONFIG_VIRTIO) += virtio-9p-device.o
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index a2c61f6b09..cf4cb2010b 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -1,4 +1,4 @@
-devices-dirs-$(call land, $(CONFIG_VIRTIO),$(call land,$(CONFIG_VIRTFS),$(CONFIG_PCI))) += 9pfs/
+devices-dirs-$(call land,$(CONFIG_VIRTFS),$(call lor,$(CONFIG_VIRTIO),$(CONFIG_XEN))) += 9pfs/
 devices-dirs-$(CONFIG_SOFTMMU) += acpi/
 devices-dirs-$(CONFIG_SOFTMMU) += adc/
 devices-dirs-$(CONFIG_SOFTMMU) += audio/
-- 
2.13.5

[Qemu-devel] [PATCH v4 02/10] kvm: remove hard dependency on pci

[Cornelia Huck]

The msi routing code in kvm calls some pci functions: provide
some stubs to enable builds without pci.

Also, to make this more obvious, guard them via a pci_available boolean
(which also can be reused in other places).

Fixes: e1d4fb2de ("kvm-irqchip: x86: add msi route notify fn")
Fixes: 767a554a0 ("kvm-all: Pass requester ID to MSI routing functions")
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 accel/kvm/kvm-all.c  |  6 +++---
 hw/pci/pci-stub.c    | 15 +++++++++++++++
 hw/pci/pci.c         |  2 ++
 include/hw/pci/pci.h |  2 ++
 4 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 46ce479dc3..f85553a851 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -1248,7 +1248,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
     int virq;
     MSIMessage msg = {0, 0};
 
-    if (dev) {
+    if (pci_available && dev) {
         msg = pci_get_msi_message(dev, vector);
     }
 
@@ -1271,7 +1271,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
     kroute.u.msi.address_lo = (uint32_t)msg.address;
     kroute.u.msi.address_hi = msg.address >> 32;
     kroute.u.msi.data = le32_to_cpu(msg.data);
-    if (kvm_msi_devid_required()) {
+    if (pci_available && kvm_msi_devid_required()) {
         kroute.flags = KVM_MSI_VALID_DEVID;
         kroute.u.msi.devid = pci_requester_id(dev);
     }
@@ -1309,7 +1309,7 @@ int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg,
     kroute.u.msi.address_lo = (uint32_t)msg.address;
     kroute.u.msi.address_hi = msg.address >> 32;
     kroute.u.msi.data = le32_to_cpu(msg.data);
-    if (kvm_msi_devid_required()) {
+    if (pci_available && kvm_msi_devid_required()) {
         kroute.flags = KVM_MSI_VALID_DEVID;
         kroute.u.msi.devid = pci_requester_id(dev);
     }
diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c
index ecad664946..ace43821ca 100644
--- a/hw/pci/pci-stub.c
+++ b/hw/pci/pci-stub.c
@@ -23,10 +23,12 @@
 #include "monitor/monitor.h"
 #include "qapi/qmp/qerror.h"
 #include "hw/pci/pci.h"
+#include "hw/pci/msi.h"
 #include "qmp-commands.h"
 #include "hw/pci/msi.h"
 
 bool msi_nonbroken;
+bool pci_available;
 
 PciInfoList *qmp_query_pci(Error **errp)
 {
@@ -38,3 +40,16 @@ void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict)
 {
     monitor_printf(mon, "PCI devices not supported\n");
 }
+
+/* kvm-all wants this */
+MSIMessage pci_get_msi_message(PCIDevice *dev, int vector)
+{
+    g_assert(false);
+    return (MSIMessage){};
+}
+
+uint16_t pci_requester_id(PCIDevice *dev)
+{
+    g_assert(false);
+    return 0;
+}
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 258fbe51e2..26f346db2c 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -49,6 +49,8 @@
 # define PCI_DPRINTF(format, ...)       do { } while (0)
 #endif
 
+bool pci_available = true;
+
 static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
 static char *pcibus_get_dev_path(DeviceState *dev);
 static char *pcibus_get_fw_dev_path(DeviceState *dev);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index e598b095eb..8bb6449dd7 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -10,6 +10,8 @@
 
 #include "hw/pci/pcie.h"
 
+extern bool pci_available;
+
 /* PCI bus */
 
 #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))
-- 
2.13.5

[Qemu-devel] [PATCH v4 03/10] s390x/pci: add stubs

[Cornelia Huck]

Some non-pci code calls into zpci code. Provide some stubs for builds
without pci.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/Makefile.objs   |  3 +-
 hw/s390x/s390-pci-stub.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 1 deletion(-)
 create mode 100644 hw/s390x/s390-pci-stub.c

diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index b2aade2466..7ee19d3abc 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -11,7 +11,8 @@ obj-y += 3270-ccw.o
 obj-y += virtio-ccw.o
 obj-y += css-bridge.o
 obj-y += ccw-device.o
-obj-y += s390-pci-bus.o s390-pci-inst.o
+obj-$(CONFIG_PCI) += s390-pci-bus.o s390-pci-inst.o
+obj-$(call lnot,$(CONFIG_PCI)) += s390-pci-stub.o
 obj-y += s390-skeys.o
 obj-y += s390-stattrib.o
 obj-$(CONFIG_KVM) += s390-skeys-kvm.o
diff --git a/hw/s390x/s390-pci-stub.c b/hw/s390x/s390-pci-stub.c
new file mode 100644
index 0000000000..2e7e42a2af
--- /dev/null
+++ b/hw/s390x/s390-pci-stub.c
@@ -0,0 +1,74 @@
+/* stubs for non-pci builds */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "cpu.h"
+#include "s390-pci-inst.h"
+#include "s390-pci-bus.h"
+
+/* target/s390x/ioinst.c */
+int chsc_sei_nt2_get_event(void *res)
+{
+    return 1;
+}
+
+int chsc_sei_nt2_have_event(void)
+{
+    return 0;
+}
+
+/* hw/s390x/sclp.c */
+void s390_pci_sclp_configure(SCCB *sccb)
+{
+}
+
+void s390_pci_sclp_deconfigure(SCCB *sccb)
+{
+}
+
+/* target/s390x/kvm.c */
+int clp_service_call(S390CPU *cpu, uint8_t r2)
+{
+    return -1;
+}
+
+int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
+{
+    return -1;
+}
+
+int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
+{
+    return -1;
+}
+
+int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar)
+{
+    return -1;
+}
+
+int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
+{
+    return -1;
+}
+
+int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
+                        uint8_t ar)
+{
+    return -1;
+}
+
+int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar)
+{
+    return -1;
+}
+
+S390pciState *s390_get_phb(void)
+{
+    return NULL;
+}
+
+S390PCIBusDevice *s390_pci_find_dev_by_idx(S390pciState *s, uint32_t idx)
+{
+    return NULL;
+}
-- 
2.13.5

[Qemu-devel] [PATCH v4 04/10] s390x: chsc nt2 events are pci-only

[Cornelia Huck]

The nt2 event class is pci-only - don't look for events if pci is
not in the active cpu model.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/s390-pci-bus.c  |  4 ++--
 hw/s390x/s390-pci-bus.h  |  4 ++--
 hw/s390x/s390-pci-stub.c |  4 ++--
 target/s390x/ioinst.c    | 16 ++++++++++++++++
 4 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 61cfd2138f..c57f6ebae0 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -47,7 +47,7 @@ S390pciState *s390_get_phb(void)
     return phb;
 }
 
-int chsc_sei_nt2_get_event(void *res)
+int pci_chsc_sei_nt2_get_event(void *res)
 {
     ChscSeiNt2Res *nt2_res = (ChscSeiNt2Res *)res;
     PciCcdfAvail *accdf;
@@ -87,7 +87,7 @@ int chsc_sei_nt2_get_event(void *res)
     return rc;
 }
 
-int chsc_sei_nt2_have_event(void)
+int pci_chsc_sei_nt2_have_event(void)
 {
     S390pciState *s = s390_get_phb();
 
diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
index 67af2c12ff..5df6292509 100644
--- a/hw/s390x/s390-pci-bus.h
+++ b/hw/s390x/s390-pci-bus.h
@@ -319,8 +319,8 @@ typedef struct S390pciState {
 } S390pciState;
 
 S390pciState *s390_get_phb(void);
-int chsc_sei_nt2_get_event(void *res);
-int chsc_sei_nt2_have_event(void);
+int pci_chsc_sei_nt2_get_event(void *res);
+int pci_chsc_sei_nt2_have_event(void);
 void s390_pci_sclp_configure(SCCB *sccb);
 void s390_pci_sclp_deconfigure(SCCB *sccb);
 void s390_pci_iommu_enable(S390PCIIOMMU *iommu);
diff --git a/hw/s390x/s390-pci-stub.c b/hw/s390x/s390-pci-stub.c
index 2e7e42a2af..cc7278a865 100644
--- a/hw/s390x/s390-pci-stub.c
+++ b/hw/s390x/s390-pci-stub.c
@@ -7,12 +7,12 @@
 #include "s390-pci-bus.h"
 
 /* target/s390x/ioinst.c */
-int chsc_sei_nt2_get_event(void *res)
+int pci_chsc_sei_nt2_get_event(void *res)
 {
     return 1;
 }
 
-int chsc_sei_nt2_have_event(void)
+int pci_chsc_sei_nt2_have_event(void)
 {
     return 0;
 }
diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index 51fbea620d..3fa3301f50 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -599,6 +599,22 @@ static int chsc_sei_nt0_have_event(void)
     return 0;
 }
 
+static int chsc_sei_nt2_get_event(void *res)
+{
+    if (s390_has_feat(S390_FEAT_ZPCI)) {
+        return pci_chsc_sei_nt2_get_event(res);
+    }
+    return 1;
+}
+
+static int chsc_sei_nt2_have_event(void)
+{
+    if (s390_has_feat(S390_FEAT_ZPCI)) {
+        return pci_chsc_sei_nt2_have_event();
+    }
+    return 0;
+}
+
 #define CHSC_SEI_NT0    (1ULL << 63)
 #define CHSC_SEI_NT2    (1ULL << 61)
 static void ioinst_handle_chsc_sei(ChscReq *req, ChscResp *res)
-- 
2.13.5

[Qemu-devel] [PATCH v4 05/10] s390x/pci: do not advertise pci on non-pci builds

[Cornelia Huck]

Only set the zpci feature bit on builds that actually support pci.

Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 target/s390x/kvm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index c4c5791d27..bc62bba5b7 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2662,7 +2662,9 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
     }
 
     /* We emulate a zPCI bus and AEN, therefore we don't need HW support */
-    set_bit(S390_FEAT_ZPCI, model->features);
+    if (pci_available) {
+        set_bit(S390_FEAT_ZPCI, model->features);
+    }
     set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
 
     if (s390_known_cpu_type(cpu_type)) {
-- 
2.13.5

[Qemu-devel] [PATCH v4 06/10] s390x/ccw: create s390 phb conditionally

[Cornelia Huck]

Don't create the s390 pci host bridge if we do not provide the zpci
facility.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index d3d67b8d77..8331c0a275 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -118,12 +118,11 @@ static void ccw_init(MachineState *machine)
 {
     int ret;
     VirtualCssBus *css_bus;
-    DeviceState *dev;
 
     s390_sclp_init();
     s390_memory_init(machine->ram_size);
 
-    /* init CPUs */
+    /* init CPUs (incl. CPU model) early so s390_has_feature() works */
     s390_init_cpus(machine);
 
     s390_flic_init();
@@ -134,10 +133,13 @@ static void ccw_init(MachineState *machine)
                       machine->initrd_filename, "s390-ccw.img",
                       "s390-netboot.img", true);
 
-    dev = qdev_create(NULL, TYPE_S390_PCI_HOST_BRIDGE);
-    object_property_add_child(qdev_get_machine(), TYPE_S390_PCI_HOST_BRIDGE,
-                              OBJECT(dev), NULL);
-    qdev_init_nofail(dev);
+    if (s390_has_feat(S390_FEAT_ZPCI)) {
+        DeviceState *dev = qdev_create(NULL, TYPE_S390_PCI_HOST_BRIDGE);
+        object_property_add_child(qdev_get_machine(),
+                                  TYPE_S390_PCI_HOST_BRIDGE,
+                                  OBJECT(dev), NULL);
+        qdev_init_nofail(dev);
+    }
 
     /* register hypercalls */
     virtio_ccw_register_hcalls();
-- 
2.13.5

[Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Cornelia Huck]

If we do not provide zpci, pci reconfiguration via sclp is not available
either. Don't indicate it in the sclp facilities and return an invalid
command if the guest tries to issue pci configure/deconfigure.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/sclp.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 9253dbbc64..d0104cd784 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -59,6 +59,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     int rnsize, rnmax;
     int slots = MIN(machine->ram_slots, s390_get_memslot_count(kvm_state));
     IplParameterBlock *ipib = s390_ipl_get_iplb();
+    uint64_t sclp_facilities = SCLP_HAS_CPU_INFO;
 
     CPU_FOREACH(cpu) {
         cpu_count++;
@@ -79,8 +80,10 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 
     prepare_cpu_entries(sclp, read_info->entries, cpu_count);
 
-    read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
-                                        SCLP_HAS_PCI_RECONFIG);
+    if (s390_has_feat(S390_FEAT_ZPCI)) {
+        sclp_facilities |= SCLP_HAS_PCI_RECONFIG;
+    }
+    read_info->facilities = cpu_to_be64(sclp_facilities);
 
     /* Memory Hotplug is only supported for the ccw machine type */
     if (mhd) {
@@ -385,10 +388,18 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
         sclp_c->unassign_storage(sclp, sccb);
         break;
     case SCLP_CMDW_CONFIGURE_PCI:
-        s390_pci_sclp_configure(sccb);
+        if (s390_has_feat(S390_FEAT_ZPCI)) {
+            s390_pci_sclp_configure(sccb);
+        } else {
+            sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
+        }
         break;
     case SCLP_CMDW_DECONFIGURE_PCI:
-        s390_pci_sclp_deconfigure(sccb);
+        if (s390_has_feat(S390_FEAT_ZPCI)) {
+            s390_pci_sclp_deconfigure(sccb);
+        } else {
+            sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
+        }
         break;
     default:
         efc->command_handler(ef, sccb, code);
-- 
2.13.5

[Qemu-devel] [PATCH v4 08/10] s390x/pci: fence off instructions for non-pci

[Cornelia Huck]

If a guest running on a machine without zpci issues a pci instruction,
throw them an exception.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 target/s390x/kvm.c | 54 +++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 13 deletions(-)

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index bc62bba5b7..9de165d8b1 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1191,7 +1191,11 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run)
 {
     uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
 
-    return clp_service_call(cpu, r2);
+    if (s390_has_feat(S390_FEAT_ZPCI)) {
+        return clp_service_call(cpu, r2);
+    } else {
+        return -1;
+    }
 }
 
 static int kvm_pcilg_service_call(S390CPU *cpu, struct kvm_run *run)
@@ -1199,7 +1203,11 @@ static int kvm_pcilg_service_call(S390CPU *cpu, struct kvm_run *run)
     uint8_t r1 = (run->s390_sieic.ipb & 0x00f00000) >> 20;
     uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
 
-    return pcilg_service_call(cpu, r1, r2);
+    if (s390_has_feat(S390_FEAT_ZPCI)) {
+        return pcilg_service_call(cpu, r1, r2);
+    } else {
+        return -1;
+    }
 }
 
 static int kvm_pcistg_service_call(S390CPU *cpu, struct kvm_run *run)
@@ -1207,7 +1215,11 @@ static int kvm_pcistg_service_call(S390CPU *cpu, struct kvm_run *run)
     uint8_t r1 = (run->s390_sieic.ipb & 0x00f00000) >> 20;
     uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
 
-    return pcistg_service_call(cpu, r1, r2);
+    if (s390_has_feat(S390_FEAT_ZPCI)) {
+        return pcistg_service_call(cpu, r1, r2);
+    } else {
+        return -1;
+    }
 }
 
 static int kvm_stpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
@@ -1216,10 +1228,14 @@ static int kvm_stpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
     uint64_t fiba;
     uint8_t ar;
 
-    cpu_synchronize_state(CPU(cpu));
-    fiba = get_base_disp_rxy(cpu, run, &ar);
+    if (s390_has_feat(S390_FEAT_ZPCI)) {
+        cpu_synchronize_state(CPU(cpu));
+        fiba = get_base_disp_rxy(cpu, run, &ar);
 
-    return stpcifc_service_call(cpu, r1, fiba, ar);
+        return stpcifc_service_call(cpu, r1, fiba, ar);
+    } else {
+        return -1;
+    }
 }
 
 static int kvm_sic_service_call(S390CPU *cpu, struct kvm_run *run)
@@ -1247,7 +1263,11 @@ static int kvm_rpcit_service_call(S390CPU *cpu, struct kvm_run *run)
     uint8_t r1 = (run->s390_sieic.ipb & 0x00f00000) >> 20;
     uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
 
-    return rpcit_service_call(cpu, r1, r2);
+    if (s390_has_feat(S390_FEAT_ZPCI)) {
+        return rpcit_service_call(cpu, r1, r2);
+    } else {
+        return -1;
+    }
 }
 
 static int kvm_pcistb_service_call(S390CPU *cpu, struct kvm_run *run)
@@ -1257,10 +1277,14 @@ static int kvm_pcistb_service_call(S390CPU *cpu, struct kvm_run *run)
     uint64_t gaddr;
     uint8_t ar;
 
-    cpu_synchronize_state(CPU(cpu));
-    gaddr = get_base_disp_rsy(cpu, run, &ar);
+    if (s390_has_feat(S390_FEAT_ZPCI)) {
+        cpu_synchronize_state(CPU(cpu));
+        gaddr = get_base_disp_rsy(cpu, run, &ar);
 
-    return pcistb_service_call(cpu, r1, r3, gaddr, ar);
+        return pcistb_service_call(cpu, r1, r3, gaddr, ar);
+    } else {
+        return -1;
+    }
 }
 
 static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
@@ -1269,10 +1293,14 @@ static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
     uint64_t fiba;
     uint8_t ar;
 
-    cpu_synchronize_state(CPU(cpu));
-    fiba = get_base_disp_rxy(cpu, run, &ar);
+    if (s390_has_feat(S390_FEAT_ZPCI)) {
+        cpu_synchronize_state(CPU(cpu));
+        fiba = get_base_disp_rxy(cpu, run, &ar);
 
-    return mpcifc_service_call(cpu, r1, fiba, ar);
+        return mpcifc_service_call(cpu, r1, fiba, ar);
+    } else {
+        return -1;
+    }
 }
 
 static int handle_b9(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
-- 
2.13.5

[Qemu-devel] [PATCH v4 09/10] s390x/kvm: msi route fixup for non-pci

[Cornelia Huck]

If we don't provide pci, we cannot have a pci device for which we
have to translate to adapter routes: just return -ENODEV.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 target/s390x/kvm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 9de165d8b1..d8db1cbf6e 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2424,6 +2424,12 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
     uint32_t idx = data >> ZPCI_MSI_VEC_BITS;
     uint32_t vec = data & ZPCI_MSI_VEC_MASK;
 
+    if (!s390_has_feat(S390_FEAT_ZPCI)) {
+        /* How can we get here without pci enabled? */
+        g_assert(false);
+        return -ENODEV;
+    }
+
     pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
     if (!pbdev) {
         DPRINTF("add_msi_route no dev\n");
-- 
2.13.5

[Qemu-devel] [PATCH v4 10/10] s390x: refine pci dependencies

[Cornelia Huck]

VIRTIO_PCI should properly depend on CONFIG_PCI.
With this change, we can switch off pci for s390x by removing
'CONFIG_PCI=y' from the default config.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 default-configs/s390x-softmmu.mak | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
index 51191b77df..6ab2bc65ac 100644
--- a/default-configs/s390x-softmmu.mak
+++ b/default-configs/s390x-softmmu.mak
@@ -1,5 +1,5 @@
 CONFIG_PCI=y
-CONFIG_VIRTIO_PCI=y
+CONFIG_VIRTIO_PCI=$(CONFIG_PCI)
 CONFIG_VHOST_USER_SCSI=$(and $(CONFIG_VHOST_USER),$(CONFIG_LINUX))
 CONFIG_VIRTIO=y
 CONFIG_SCLPCONSOLE=y
-- 
2.13.5

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Halil Pasic]

On 08/21/2017 11:16 AM, Cornelia Huck wrote:
> If we do not provide zpci, pci reconfiguration via sclp is not available
> either. Don't indicate it in the sclp facilities and return an invalid
> command if the guest tries to issue pci configure/deconfigure.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/sclp.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 9253dbbc64..d0104cd784 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -59,6 +59,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>      int rnsize, rnmax;
>      int slots = MIN(machine->ram_slots, s390_get_memslot_count(kvm_state));
>      IplParameterBlock *ipib = s390_ipl_get_iplb();
> +    uint64_t sclp_facilities = SCLP_HAS_CPU_INFO;
> 
>      CPU_FOREACH(cpu) {
>          cpu_count++;
> @@ -79,8 +80,10 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
> 
>      prepare_cpu_entries(sclp, read_info->entries, cpu_count);
> 
> -    read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
> -                                        SCLP_HAS_PCI_RECONFIG);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        sclp_facilities |= SCLP_HAS_PCI_RECONFIG;
> +    }
> +    read_info->facilities = cpu_to_be64(sclp_facilities);
> 
>      /* Memory Hotplug is only supported for the ccw machine type */
>      if (mhd) {
> @@ -385,10 +388,18 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
>          sclp_c->unassign_storage(sclp, sccb);
>          break;
>      case SCLP_CMDW_CONFIGURE_PCI:
> -        s390_pci_sclp_configure(sccb);
> +        if (s390_has_feat(S390_FEAT_ZPCI)) {
> +            s390_pci_sclp_configure(sccb);
> +        } else {
> +            sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> +        }
>          break;
>      case SCLP_CMDW_DECONFIGURE_PCI:
> -        s390_pci_sclp_deconfigure(sccb);
> +        if (s390_has_feat(S390_FEAT_ZPCI)) {
> +            s390_pci_sclp_deconfigure(sccb);
> +        } else {
> +            sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> +        }
>          break;
>      default:
>          efc->command_handler(ef, sccb, code);
> 

LGTM

Re: [Qemu-devel] [PATCH v4 09/10] s390x/kvm: msi route fixup for non-pci

[Halil Pasic]

On 08/21/2017 11:16 AM, Cornelia Huck wrote:
> If we don't provide pci, we cannot have a pci device for which we
> have to translate to adapter routes: just return -ENODEV.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  target/s390x/kvm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 9de165d8b1..d8db1cbf6e 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2424,6 +2424,12 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
>      uint32_t idx = data >> ZPCI_MSI_VEC_BITS;
>      uint32_t vec = data & ZPCI_MSI_VEC_MASK;
> 
> +    if (!s390_has_feat(S390_FEAT_ZPCI)) {
> +        /* How can we get here without pci enabled? */
> +        g_assert(false);

You don't tell us about the g_assert in the commit message.
Do you expect G_DISABLE_ASSERT being defined for production 
builds. I've grepped for G_DISABLE_ASSERT and found nothing.

And why g_assert over assert (again no guidance in HACKING
mostly asking for my own learning)?

Other that that LGTM.


> +        return -ENODEV;
> +    }
> +
>      pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
>      if (!pbdev) {
>          DPRINTF("add_msi_route no dev\n");
> 

Re: [Qemu-devel] [PATCH v4 09/10] s390x/kvm: msi route fixup for non-pci

[Cornelia Huck]

On Mon, 21 Aug 2017 14:00:15 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 08/21/2017 11:16 AM, Cornelia Huck wrote:
> > If we don't provide pci, we cannot have a pci device for which we
> > have to translate to adapter routes: just return -ENODEV.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  target/s390x/kvm.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> > index 9de165d8b1..d8db1cbf6e 100644
> > --- a/target/s390x/kvm.c
> > +++ b/target/s390x/kvm.c
> > @@ -2424,6 +2424,12 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
> >      uint32_t idx = data >> ZPCI_MSI_VEC_BITS;
> >      uint32_t vec = data & ZPCI_MSI_VEC_MASK;
> > 
> > +    if (!s390_has_feat(S390_FEAT_ZPCI)) {
> > +        /* How can we get here without pci enabled? */
> > +        g_assert(false);  
> 
> You don't tell us about the g_assert in the commit message.
> Do you expect G_DISABLE_ASSERT being defined for production 
> builds. I've grepped for G_DISABLE_ASSERT and found nothing.

AFAIK this is set by distribution builds. I've also noticed that mingw
builds treat (g_)assert() as if code flow continues, but I don't know
whether asserts do anything there at all.

> 
> And why g_assert over assert (again no guidance in HACKING
> mostly asking for my own learning)?

I do recall a recent(ish) discussion, but not the details. Anyway,
using glib interfaces seems more consistent.

> 
> Other that that LGTM.

Thanks!

> 
> 
> > +        return -ENODEV;
> > +    }
> > +
> >      pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
> >      if (!pbdev) {
> >          DPRINTF("add_msi_route no dev\n");
> >   
> 

Re: [Qemu-devel] [PATCH v4 04/10] s390x: chsc nt2 events are pci-only

[Thomas Huth]

On 21.08.2017 11:16, Cornelia Huck wrote:
> The nt2 event class is pci-only - don't look for events if pci is
> not in the active cpu model.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/s390-pci-bus.c  |  4 ++--
>  hw/s390x/s390-pci-bus.h  |  4 ++--
>  hw/s390x/s390-pci-stub.c |  4 ++--
>  target/s390x/ioinst.c    | 16 ++++++++++++++++
>  4 files changed, 22 insertions(+), 6 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v4 05/10] s390x/pci: do not advertise pci on non-pci builds

[Thomas Huth]

On 21.08.2017 11:16, Cornelia Huck wrote:
> Only set the zpci feature bit on builds that actually support pci.
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  target/s390x/kvm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index c4c5791d27..bc62bba5b7 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2662,7 +2662,9 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>      }
>  
>      /* We emulate a zPCI bus and AEN, therefore we don't need HW support */
> -    set_bit(S390_FEAT_ZPCI, model->features);
> +    if (pci_available) {
> +        set_bit(S390_FEAT_ZPCI, model->features);
> +    }
>      set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
>  
>      if (s390_known_cpu_type(cpu_type)) {
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Cornelia Huck]

On Mon, 21 Aug 2017 13:41:53 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 08/21/2017 11:16 AM, Cornelia Huck wrote:
> > If we do not provide zpci, pci reconfiguration via sclp is not available
> > either. Don't indicate it in the sclp facilities and return an invalid
> > command if the guest tries to issue pci configure/deconfigure.
> > 
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  hw/s390x/sclp.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)

(...)

> LGTM

Is that an Acked-by:? Or a Reviewed-by:? :)

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Halil Pasic]

On 08/21/2017 03:16 PM, Cornelia Huck wrote:
> On Mon, 21 Aug 2017 13:41:53 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 08/21/2017 11:16 AM, Cornelia Huck wrote:
>>> If we do not provide zpci, pci reconfiguration via sclp is not available
>>> either. Don't indicate it in the sclp facilities and return an invalid
>>> command if the guest tries to issue pci configure/deconfigure.
>>>
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>  hw/s390x/sclp.c | 19 +++++++++++++++----
>>>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> (...)
> 
>> LGTM
> 
> Is that an Acked-by:? Or a Reviewed-by:? :)
> 

r-b

(Not sure what Acked-by from a role other that affected
maintainer means.)

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Cornelia Huck]

On Mon, 21 Aug 2017 15:32:23 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 08/21/2017 03:16 PM, Cornelia Huck wrote:
> > On Mon, 21 Aug 2017 13:41:53 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> On 08/21/2017 11:16 AM, Cornelia Huck wrote:  
> >>> If we do not provide zpci, pci reconfiguration via sclp is not available
> >>> either. Don't indicate it in the sclp facilities and return an invalid
> >>> command if the guest tries to issue pci configure/deconfigure.
> >>>
> >>> Reviewed-by: Thomas Huth <thuth@redhat.com>
> >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> >>> ---
> >>>  hw/s390x/sclp.c | 19 +++++++++++++++----
> >>>  1 file changed, 15 insertions(+), 4 deletions(-)  
> > 
> > (...)
> >   
> >> LGTM  
> > 
> > Is that an Acked-by:? Or a Reviewed-by:? :)
> >   
> 
> r-b

If you write that out in future, I can simply copy'n'paste it :)

> 
> (Not sure what Acked-by from a role other that affected
> maintainer means.)

I've always used it in the sense of "I don't know the gory details, and
I haven't tried to find them out, but it generally looks sane to me".

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Pierre Morel]

On 21/08/2017 11:16, Cornelia Huck wrote:
> If we do not provide zpci, pci reconfiguration via sclp is not available
> either. Don't indicate it in the sclp facilities and return an invalid
> command if the guest tries to issue pci configure/deconfigure.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>   hw/s390x/sclp.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 9253dbbc64..d0104cd784 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -59,6 +59,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>       int rnsize, rnmax;
>       int slots = MIN(machine->ram_slots, s390_get_memslot_count(kvm_state));
>       IplParameterBlock *ipib = s390_ipl_get_iplb();
> +    uint64_t sclp_facilities = SCLP_HAS_CPU_INFO;
> 
>       CPU_FOREACH(cpu) {
>           cpu_count++;
> @@ -79,8 +80,10 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
> 
>       prepare_cpu_entries(sclp, read_info->entries, cpu_count);
> 
> -    read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
> -                                        SCLP_HAS_PCI_RECONFIG);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        sclp_facilities |= SCLP_HAS_PCI_RECONFIG;
> +    }
> +    read_info->facilities = cpu_to_be64(sclp_facilities);
> 
>       /* Memory Hotplug is only supported for the ccw machine type */
>       if (mhd) {
> @@ -385,10 +388,18 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
>           sclp_c->unassign_storage(sclp, sccb);
>           break;
>       case SCLP_CMDW_CONFIGURE_PCI:
> -        s390_pci_sclp_configure(sccb);
> +        if (s390_has_feat(S390_FEAT_ZPCI)) {
> +            s390_pci_sclp_configure(sccb);
> +        } else {
> +            sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);

Hello Conny,

I find more logical if the response would be 0x06F0 : "Adapter type in 
SCCB not recognized"
Since we could have more than one adapter type... someday.
then the SCLP command would be valid but not the adapter type.

However, I will try to find a real hardware to test this since I noticed 
that my logic sometime... :) .

Another point is that don't you think that this test on S390_FEAT_ZPCI 
better belong to the dedicated PCI code inside of the 
s390_pci_sclp_configure().

best regards,

Pierre


> +        }
>           break;
>       case SCLP_CMDW_DECONFIGURE_PCI:
> -        s390_pci_sclp_deconfigure(sccb);
> +        if (s390_has_feat(S390_FEAT_ZPCI)) {
> +            s390_pci_sclp_deconfigure(sccb);
> +        } else {
> +            sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> +        }
>           break;
>       default:
>           efc->command_handler(ef, sccb, code);
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v4 09/10] s390x/kvm: msi route fixup for non-pci

[Halil Pasic]

On 08/21/2017 02:13 PM, Cornelia Huck wrote:
> On Mon, 21 Aug 2017 14:00:15 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 08/21/2017 11:16 AM, Cornelia Huck wrote:
>>> If we don't provide pci, we cannot have a pci device for which we
>>> have to translate to adapter routes: just return -ENODEV.
>>>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>  target/s390x/kvm.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>> index 9de165d8b1..d8db1cbf6e 100644
>>> --- a/target/s390x/kvm.c
>>> +++ b/target/s390x/kvm.c
>>> @@ -2424,6 +2424,12 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
>>>      uint32_t idx = data >> ZPCI_MSI_VEC_BITS;
>>>      uint32_t vec = data & ZPCI_MSI_VEC_MASK;
>>>
>>> +    if (!s390_has_feat(S390_FEAT_ZPCI)) {
>>> +        /* How can we get here without pci enabled? */
>>> +        g_assert(false);  
>>
>> You don't tell us about the g_assert in the commit message.
>> Do you expect G_DISABLE_ASSERT being defined for production 
>> builds. I've grepped for G_DISABLE_ASSERT and found nothing.
> 
> AFAIK this is set by distribution builds. I've also noticed that mingw
> builds treat (g_)assert() as if code flow continues, but I don't know
> whether asserts do anything there at all.
> 

After thinking some more,  I would prefer the the commit message being
modified so, that it's clear, what we really want is the assert; or this
assert being dropped or replaced with some kind of warning/tracing.

I assume no production QEMU should ever return -ENODEV here (and if it
does, it's due to an QEMU bug). So the assert is there to communicate
with the devel, and just in case if the client code fails to fulfill
their part of the contract.  In this case we shall fail fast and hard,
and no such QEMU should ship. The return -ENODEV is then 'just in case'
on the square -- a failsafe for the failsafe (which does make sense
to me).

On the contrary if we assume this is a legit error condition (and a part
of the contract) then according to HACKING 7.2 Handling errors we shall
not call exit() or abort() to handle an error that can be triggered by
the guest in particular and operation related errors in general. Makes
perfect sense to me.

Pierre helped me, kvm_arch_fixup_msi_route is guest triggered and also
considering this particular case killing off the QEMU, and the guest does
not seem like the lesser evil.

The situation is just complicated by the fact that there is code which
relies on assert(true) asserting for correctness (e.g. virtio goes so far
to make builds with normal asserts disabled fail). Thus for me it's hard
to assume that the assertion is guaranteed to be disabled in production.

But if it ain't disabled than it calls abort() which we should not
do (HACKING and IMHO common sense).

TL;DR

I'm for modifying the commit message so it tells us about
the assert.

>>
>> And why g_assert over assert (again no guidance in HACKING
>> mostly asking for my own learning)?
> 
> I do recall a recent(ish) discussion, but not the details. Anyway,
> using glib interfaces seems more consistent.

We can't live with NDEBUG is another reason for using g_assert (makes
my preferred solution work).

Regards,
Halil

[..]

Re: [Qemu-devel] [PATCH v4 09/10] s390x/kvm: msi route fixup for non-pci

[Thomas Huth]

On 21.08.2017 17:10, Halil Pasic wrote:
[...]
> The situation is just complicated by the fact that there is code which
> relies on assert(true) asserting for correctness (e.g. virtio goes so far
> to make builds with normal asserts disabled fail). Thus for me it's hard
> to assume that the assertion is guaranteed to be disabled in production.

FYI: https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg03608.html

 Thomas

Re: [Qemu-devel] [PATCH v4 09/10] s390x/kvm: msi route fixup for non-pci

[Halil Pasic]

On 08/21/2017 05:17 PM, Thomas Huth wrote:
> On 21.08.2017 17:10, Halil Pasic wrote:
> [...]
>> The situation is just complicated by the fact that there is code which
>> relies on assert(true) asserting for correctness (e.g. virtio goes so far
>> to make builds with normal asserts disabled fail). Thus for me it's hard
>> to assume that the assertion is guaranteed to be disabled in production.
> 
> FYI: https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg03608.html
> 
>  Thomas
> 

Thanks, I've missed that. With that assumed it becomes either
assert(false) or return -ENODEV but not both.

Regards,
Halil 

Re: [Qemu-devel] [PATCH v4 02/10] kvm: remove hard dependency on pci

[Pierre Morel]

On 21/08/2017 11:16, Cornelia Huck wrote:
> The msi routing code in kvm calls some pci functions: provide
> some stubs to enable builds without pci.
> 
> Also, to make this more obvious, guard them via a pci_available boolean
> (which also can be reused in other places).
> 
> Fixes: e1d4fb2de ("kvm-irqchip: x86: add msi route notify fn")
> Fixes: 767a554a0 ("kvm-all: Pass requester ID to MSI routing functions")
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>   accel/kvm/kvm-all.c  |  6 +++---
>   hw/pci/pci-stub.c    | 15 +++++++++++++++
>   hw/pci/pci.c         |  2 ++
>   include/hw/pci/pci.h |  2 ++
>   4 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 46ce479dc3..f85553a851 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1248,7 +1248,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
>       int virq;
>       MSIMessage msg = {0, 0};
> 
> -    if (dev) {
> +    if (pci_available && dev) {
>           msg = pci_get_msi_message(dev, vector);
>       }

Hi Conny,

I did not find a case where pci_available is false and dev is true.
but anyway, sure is sure.


> 
> @@ -1271,7 +1271,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
>       kroute.u.msi.address_lo = (uint32_t)msg.address;
>       kroute.u.msi.address_hi = msg.address >> 32;
>       kroute.u.msi.data = le32_to_cpu(msg.data);
> -    if (kvm_msi_devid_required()) {
> +    if (pci_available && kvm_msi_devid_required()) {
>           kroute.flags = KVM_MSI_VALID_DEVID;
>           kroute.u.msi.devid = pci_requester_id(dev);
>       }
> @@ -1309,7 +1309,7 @@ int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg,
>       kroute.u.msi.address_lo = (uint32_t)msg.address;
>       kroute.u.msi.address_hi = msg.address >> 32;
>       kroute.u.msi.data = le32_to_cpu(msg.data);
> -    if (kvm_msi_devid_required()) {
> +    if (pci_available && kvm_msi_devid_required()) {
>           kroute.flags = KVM_MSI_VALID_DEVID;
>           kroute.u.msi.devid = pci_requester_id(dev);
>       }
> diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c
> index ecad664946..ace43821ca 100644
> --- a/hw/pci/pci-stub.c
> +++ b/hw/pci/pci-stub.c
> @@ -23,10 +23,12 @@
>   #include "monitor/monitor.h"
>   #include "qapi/qmp/qerror.h"
>   #include "hw/pci/pci.h"
> +#include "hw/pci/msi.h"

I think you forgot that...

>   #include "qmp-commands.h"
>   #include "hw/pci/msi.h"

...you already have it included here. Didn't you ?

> 
>   bool msi_nonbroken;
> +bool pci_available;
> 
>   PciInfoList *qmp_query_pci(Error **errp)
>   {
> @@ -38,3 +40,16 @@ void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict)
>   {
>       monitor_printf(mon, "PCI devices not supported\n");
>   }
> +
> +/* kvm-all wants this */
> +MSIMessage pci_get_msi_message(PCIDevice *dev, int vector)
> +{
> +    g_assert(false);
> +    return (MSIMessage){};
> +}
> +
> +uint16_t pci_requester_id(PCIDevice *dev)
> +{
> +    g_assert(false);
> +    return 0;
> +}
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 258fbe51e2..26f346db2c 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -49,6 +49,8 @@
>   # define PCI_DPRINTF(format, ...)       do { } while (0)
>   #endif
> 
> +bool pci_available = true;
> +
>   static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
>   static char *pcibus_get_dev_path(DeviceState *dev);
>   static char *pcibus_get_fw_dev_path(DeviceState *dev);
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index e598b095eb..8bb6449dd7 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -10,6 +10,8 @@
> 
>   #include "hw/pci/pcie.h"
> 
> +extern bool pci_available;
> +
>   /* PCI bus */
> 
>   #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))
> 

otherwise LGTM

Regards,

Pierre


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Halil Pasic]

On 08/21/2017 04:58 PM, Pierre Morel wrote:
> On 21/08/2017 11:16, Cornelia Huck wrote:
>> If we do not provide zpci, pci reconfiguration via sclp is not available
>> either. Don't indicate it in the sclp facilities and return an invalid
>> command if the guest tries to issue pci configure/deconfigure.
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>   hw/s390x/sclp.c | 19 +++++++++++++++----
>>   1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 9253dbbc64..d0104cd784 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -59,6 +59,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>       int rnsize, rnmax;
>>       int slots = MIN(machine->ram_slots, s390_get_memslot_count(kvm_state));
>>       IplParameterBlock *ipib = s390_ipl_get_iplb();
>> +    uint64_t sclp_facilities = SCLP_HAS_CPU_INFO;
>>
>>       CPU_FOREACH(cpu) {
>>           cpu_count++;
>> @@ -79,8 +80,10 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>
>>       prepare_cpu_entries(sclp, read_info->entries, cpu_count);
>>
>> -    read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
>> -                                        SCLP_HAS_PCI_RECONFIG);
>> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
>> +        sclp_facilities |= SCLP_HAS_PCI_RECONFIG;
>> +    }
>> +    read_info->facilities = cpu_to_be64(sclp_facilities);
>>
>>       /* Memory Hotplug is only supported for the ccw machine type */
>>       if (mhd) {
>> @@ -385,10 +388,18 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
>>           sclp_c->unassign_storage(sclp, sccb);
>>           break;
>>       case SCLP_CMDW_CONFIGURE_PCI:
>> -        s390_pci_sclp_configure(sccb);
>> +        if (s390_has_feat(S390_FEAT_ZPCI)) {
>> +            s390_pci_sclp_configure(sccb);
>> +        } else {
>> +            sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> 
> Hello Conny,
> 
> I find more logical if the response would be 0x06F0 : "Adapter type in SCCB not recognized"
> Since we could have more than one adapter type... someday.
> then the SCLP command would be valid but not the adapter type.

I agree, 01F0 does not seem to be the correct answer, based on
the AR as it seems to be tied to the command code.

Verifying what the machine does would be great though -- frankly
I cant tell with absolute confidence what's the right thing to
do based on my understanding of the AR. 


> 
> However, I will try to find a real hardware to test this since I noticed that my logic sometime... :) .
> 
> Another point is that don't you think that this test on S390_FEAT_ZPCI better belong to the dedicated PCI code inside of the s390_pci_sclp_configure().
>

I'm fine either way. If I imagine having a lots of adapter types, then I
would expect a switch or a jumptable on the type before handling control
to the pci specific function. In this case statically not supported types
would probably get caught by the default branch of the switch and for a
jumptable it could even handle the dynamic case (based on the facilities)
trivially. In short both approaches can make sense.

Regards,
Halil

> best regards,
> 
> Pierre
> 
> 
>> +        }
>>           break;
>>       case SCLP_CMDW_DECONFIGURE_PCI:
>> -        s390_pci_sclp_deconfigure(sccb);
>> +        if (s390_has_feat(S390_FEAT_ZPCI)) {
>> +            s390_pci_sclp_deconfigure(sccb);
>> +        } else {
>> +            sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>> +        }
>>           break;
>>       default:
>>           efc->command_handler(ef, sccb, code);
>>
> 
> 

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Cornelia Huck]

On Mon, 21 Aug 2017 18:24:15 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 08/21/2017 04:58 PM, Pierre Morel wrote:
> > On 21/08/2017 11:16, Cornelia Huck wrote:  
> >> If we do not provide zpci, pci reconfiguration via sclp is not available
> >> either. Don't indicate it in the sclp facilities and return an invalid
> >> command if the guest tries to issue pci configure/deconfigure.
> >>
> >> Reviewed-by: Thomas Huth <thuth@redhat.com>
> >> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> >> ---
> >>   hw/s390x/sclp.c | 19 +++++++++++++++----
> >>   1 file changed, 15 insertions(+), 4 deletions(-)
> >>

> >> @@ -385,10 +388,18 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
> >>           sclp_c->unassign_storage(sclp, sccb);
> >>           break;
> >>       case SCLP_CMDW_CONFIGURE_PCI:
> >> -        s390_pci_sclp_configure(sccb);
> >> +        if (s390_has_feat(S390_FEAT_ZPCI)) {
> >> +            s390_pci_sclp_configure(sccb);
> >> +        } else {
> >> +            sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);  
> > 
> > Hello Conny,
> > 
> > I find more logical if the response would be 0x06F0 : "Adapter type in SCCB not recognized"
> > Since we could have more than one adapter type... someday.
> > then the SCLP command would be valid but not the adapter type.  
> 
> I agree, 01F0 does not seem to be the correct answer, based on
> the AR as it seems to be tied to the command code.

What I'm wondering here: SCLP_CMDW_CONFIGURE_PCI was introduced at some
point in time (probably when pci was first introduced). On older
machines, invalid command sounds like the most logical response. Do you
know whether there's some tie-in to the machine version?

> Verifying what the machine does would be great though -- frankly
> I cant tell with absolute confidence what's the right thing to
> do based on my understanding of the AR. 

Multiply this by not having access to the AR :)

> > However, I will try to find a real hardware to test this since I noticed that my logic sometime... :) .
> > 
> > Another point is that don't you think that this test on S390_FEAT_ZPCI better belong to the dedicated PCI code inside of the s390_pci_sclp_configure().
> >  
> 
> I'm fine either way. If I imagine having a lots of adapter types, then I
> would expect a switch or a jumptable on the type before handling control
> to the pci specific function. In this case statically not supported types
> would probably get caught by the default branch of the switch and for a
> jumptable it could even handle the dynamic case (based on the facilities)
> trivially. In short both approaches can make sense.

I'm also wondering at the naming (the command sounds very
pci-specific). I'd just stick with this approach (modulo a possible
change of the response code, for which I need to rely on you guys).

Re: [Qemu-devel] [PATCH v4 02/10] kvm: remove hard dependency on pci

[Cornelia Huck]

On Mon, 21 Aug 2017 18:02:12 +0200
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> On 21/08/2017 11:16, Cornelia Huck wrote:
> > The msi routing code in kvm calls some pci functions: provide
> > some stubs to enable builds without pci.
> > 
> > Also, to make this more obvious, guard them via a pci_available boolean
> > (which also can be reused in other places).
> > 
> > Fixes: e1d4fb2de ("kvm-irqchip: x86: add msi route notify fn")
> > Fixes: 767a554a0 ("kvm-all: Pass requester ID to MSI routing functions")
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >   accel/kvm/kvm-all.c  |  6 +++---
> >   hw/pci/pci-stub.c    | 15 +++++++++++++++
> >   hw/pci/pci.c         |  2 ++
> >   include/hw/pci/pci.h |  2 ++
> >   4 files changed, 22 insertions(+), 3 deletions(-)
> > 
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index 46ce479dc3..f85553a851 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -1248,7 +1248,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
> >       int virq;
> >       MSIMessage msg = {0, 0};
> > 
> > -    if (dev) {
> > +    if (pci_available && dev) {
> >           msg = pci_get_msi_message(dev, vector);
> >       }  
> 
> Hi Conny,
> 
> I did not find a case where pci_available is false and dev is true.
> but anyway, sure is sure.

It makes things more obvious, I think.

> > diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c
> > index ecad664946..ace43821ca 100644
> > --- a/hw/pci/pci-stub.c
> > +++ b/hw/pci/pci-stub.c
> > @@ -23,10 +23,12 @@
> >   #include "monitor/monitor.h"
> >   #include "qapi/qmp/qerror.h"
> >   #include "hw/pci/pci.h"
> > +#include "hw/pci/msi.h"  
> 
> I think you forgot that...
> 
> >   #include "qmp-commands.h"
> >   #include "hw/pci/msi.h"  
> 
> ...you already have it included here. Didn't you ?

Hum, once should really be enough.

> otherwise LGTM

Thanks. Can I translate that into a tag? :)

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Halil Pasic]

On 08/22/2017 10:39 AM, Cornelia Huck wrote:
>> I'm fine either way. If I imagine having a lots of adapter types, then I
>> would expect a switch or a jumptable on the type before handling control
>> to the pci specific function. In this case statically not supported types
>> would probably get caught by the default branch of the switch and for a
>> jumptable it could even handle the dynamic case (based on the facilities)
>> trivially. In short both approaches can make sense.
> I'm also wondering at the naming (the command sounds very
> pci-specific). I'd just stick with this approach (modulo a possible
> change of the response code, for which I need to rely on you guys).
> 


Well, the QEMU name of the command is misleading misleading. In the AR
it's called 'Configure I/O Adapter'. The PCI comes into the picture via
byte 8 of the SCCB, the so called adapter type. Valid values for the
adapter type are: 00-01 reserved; 02 PCI function; 03-FF reserved. So
at this point we only have PCI.

Regarding the code. I think we should wait for Pierre.

Regards,
Halil

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Cornelia Huck]

On Tue, 22 Aug 2017 11:20:51 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 08/22/2017 10:39 AM, Cornelia Huck wrote:
> >> I'm fine either way. If I imagine having a lots of adapter types, then I
> >> would expect a switch or a jumptable on the type before handling control
> >> to the pci specific function. In this case statically not supported types
> >> would probably get caught by the default branch of the switch and for a
> >> jumptable it could even handle the dynamic case (based on the facilities)
> >> trivially. In short both approaches can make sense.  
> > I'm also wondering at the naming (the command sounds very
> > pci-specific). I'd just stick with this approach (modulo a possible
> > change of the response code, for which I need to rely on you guys).
> >   
> 
> 
> Well, the QEMU name of the command is misleading misleading. In the AR
> it's called 'Configure I/O Adapter'. The PCI comes into the picture via
> byte 8 of the SCCB, the so called adapter type. Valid values for the
> adapter type are: 00-01 reserved; 02 PCI function; 03-FF reserved. So
> at this point we only have PCI.

OK, misleading naming combined with missing documentation leads to
confusion...

So:

- s/PCI/IOA/ for SCLP_CMDW_{CONFIGURE,DECONFIGURE}_PCI
- have a switch/case over byte 8 with only one case (pci)
- move the pci feature check into the pci code(? - not sure)

There's still the question of when this sclp command first became
available...

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Cornelia Huck]

On Tue, 22 Aug 2017 11:39:14 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 22 Aug 2017 11:20:51 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
> > On 08/22/2017 10:39 AM, Cornelia Huck wrote:  
> > >> I'm fine either way. If I imagine having a lots of adapter types, then I
> > >> would expect a switch or a jumptable on the type before handling control
> > >> to the pci specific function. In this case statically not supported types
> > >> would probably get caught by the default branch of the switch and for a
> > >> jumptable it could even handle the dynamic case (based on the facilities)
> > >> trivially. In short both approaches can make sense.    
> > > I'm also wondering at the naming (the command sounds very
> > > pci-specific). I'd just stick with this approach (modulo a possible
> > > change of the response code, for which I need to rely on you guys).
> > >     
> > 
> > 
> > Well, the QEMU name of the command is misleading misleading. In the AR
> > it's called 'Configure I/O Adapter'. The PCI comes into the picture via
> > byte 8 of the SCCB, the so called adapter type. Valid values for the
> > adapter type are: 00-01 reserved; 02 PCI function; 03-FF reserved. So
> > at this point we only have PCI.  
> 
> OK, misleading naming combined with missing documentation leads to
> confusion...
> 
> So:
> 
> - s/PCI/IOA/ for SCLP_CMDW_{CONFIGURE,DECONFIGURE}_PCI
> - have a switch/case over byte 8 with only one case (pci)

- switch to response code 0x06f0 instead of invalid command

> - move the pci feature check into the pci code(? - not sure)
> 
> There's still the question of when this sclp command first became
> available...

...because we probably want to indicate invalid command for older
machine types (or is there another facility bit?)

Another question: There's the sclp facilities bit SCLP_HAS_PCI_RECONFIG
- is that really pci, or I/O adapter as for the actual commands?

[The Linux kernel uses the _PCI naming scheme, so I can't derive
anything from that.]

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Halil Pasic]

On 08/22/2017 11:39 AM, Cornelia Huck wrote:
> On Tue, 22 Aug 2017 11:20:51 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 08/22/2017 10:39 AM, Cornelia Huck wrote:
>>>> I'm fine either way. If I imagine having a lots of adapter types, then I
>>>> would expect a switch or a jumptable on the type before handling control
>>>> to the pci specific function. In this case statically not supported types
>>>> would probably get caught by the default branch of the switch and for a
>>>> jumptable it could even handle the dynamic case (based on the facilities)
>>>> trivially. In short both approaches can make sense.  
>>> I'm also wondering at the naming (the command sounds very
>>> pci-specific). I'd just stick with this approach (modulo a possible
>>> change of the response code, for which I need to rely on you guys).
>>>   
>>
>>
>> Well, the QEMU name of the command is misleading misleading. In the AR
>> it's called 'Configure I/O Adapter'. The PCI comes into the picture via
>> byte 8 of the SCCB, the so called adapter type. Valid values for the
>> adapter type are: 00-01 reserved; 02 PCI function; 03-FF reserved. So
>> at this point we only have PCI.
> 
> OK, misleading naming combined with missing documentation leads to
> confusion...
> 
> So:
> 
> - s/PCI/IOA/ for SCLP_CMDW_{CONFIGURE,DECONFIGURE}_PCI
nod

> - have a switch/case over byte 8 with only one case (pci)

Let's say some kind of a check for bit 8 is a good idea.

> - move the pci feature check into the pci code(? - not sure)

Don't know. Architecturally I don't see any direct connection
between the pci feature and this command.

The availability of SCLP_CMDW_{,DE}CONFIGURE_IOA is indicated
by the result of the read scp info command read info in
ReadInfo.facilities. I think we should assume that the read scp
info command is always there.

I would appreciate someone with AR access double checking.

> 
> There's still the question of when this sclp command first became
> available...
> 

I would argue that it should not be important for AR
compliance. Currently it operates only on PCI so I doubt it
pre-dates PCI. But I don't think the current implementation
is buggy because it offers the sclp command regardless
of the zPCI facility.

I don't know where should I look for the historical details
which go beyond the AR.

Halil

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Halil Pasic]

On 08/22/2017 02:57 PM, Cornelia Huck wrote:
> On Tue, 22 Aug 2017 11:39:14 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Tue, 22 Aug 2017 11:20:51 +0200
>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>
>>> On 08/22/2017 10:39 AM, Cornelia Huck wrote:  
>>>>> I'm fine either way. If I imagine having a lots of adapter types, then I
>>>>> would expect a switch or a jumptable on the type before handling control
>>>>> to the pci specific function. In this case statically not supported types
>>>>> would probably get caught by the default branch of the switch and for a
>>>>> jumptable it could even handle the dynamic case (based on the facilities)
>>>>> trivially. In short both approaches can make sense.    
>>>> I'm also wondering at the naming (the command sounds very
>>>> pci-specific). I'd just stick with this approach (modulo a possible
>>>> change of the response code, for which I need to rely on you guys).
>>>>     
>>>
>>>
>>> Well, the QEMU name of the command is misleading misleading. In the AR
>>> it's called 'Configure I/O Adapter'. The PCI comes into the picture via
>>> byte 8 of the SCCB, the so called adapter type. Valid values for the
>>> adapter type are: 00-01 reserved; 02 PCI function; 03-FF reserved. So
>>> at this point we only have PCI.  
>>
>> OK, misleading naming combined with missing documentation leads to
>> confusion...
>>
>> So:
>>
>> - s/PCI/IOA/ for SCLP_CMDW_{CONFIGURE,DECONFIGURE}_PCI
>> - have a switch/case over byte 8 with only one case (pci)
> 
> - switch to response code 0x06f0 instead of invalid command
> 
>> - move the pci feature check into the pci code(? - not sure)
>>
>> There's still the question of when this sclp command first became
>> available...
> 
> ...because we probably want to indicate invalid command for older
> machine types (or is there another facility bit?)
> 
> Another question: There's the sclp facilities bit SCLP_HAS_PCI_RECONFIG
> - is that really pci, or I/O adapter as for the actual commands?
> 
> [The Linux kernel uses the _PCI naming scheme, so I can't derive
> anything from that.]
> 

Ah, we to each other almost simultaneously... See my other mail.

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Cornelia Huck]

On Tue, 22 Aug 2017 14:58:37 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 08/22/2017 11:39 AM, Cornelia Huck wrote:
> > On Tue, 22 Aug 2017 11:20:51 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> On 08/22/2017 10:39 AM, Cornelia Huck wrote:  
> >>>> I'm fine either way. If I imagine having a lots of adapter types, then I
> >>>> would expect a switch or a jumptable on the type before handling control
> >>>> to the pci specific function. In this case statically not supported types
> >>>> would probably get caught by the default branch of the switch and for a
> >>>> jumptable it could even handle the dynamic case (based on the facilities)
> >>>> trivially. In short both approaches can make sense.    
> >>> I'm also wondering at the naming (the command sounds very
> >>> pci-specific). I'd just stick with this approach (modulo a possible
> >>> change of the response code, for which I need to rely on you guys).
> >>>     
> >>
> >>
> >> Well, the QEMU name of the command is misleading misleading. In the AR
> >> it's called 'Configure I/O Adapter'. The PCI comes into the picture via
> >> byte 8 of the SCCB, the so called adapter type. Valid values for the
> >> adapter type are: 00-01 reserved; 02 PCI function; 03-FF reserved. So
> >> at this point we only have PCI.  
> > 
> > OK, misleading naming combined with missing documentation leads to
> > confusion...
> > 
> > So:
> > 
> > - s/PCI/IOA/ for SCLP_CMDW_{CONFIGURE,DECONFIGURE}_PCI  
> nod
> 
> > - have a switch/case over byte 8 with only one case (pci)  
> 
> Let's say some kind of a check for bit 8 is a good idea.

Yes.

> 
> > - move the pci feature check into the pci code(? - not sure)  
> 
> Don't know. Architecturally I don't see any direct connection
> between the pci feature and this command.

I'd either have the check in the pci case for the adapter type, or in
the called function. It's probably cleaner to do the check before
calling the pci function.

> 
> The availability of SCLP_CMDW_{,DE}CONFIGURE_IOA is indicated
> by the result of the read scp info command read info in
> ReadInfo.facilities. I think we should assume that the read scp
> info command is always there.

Sure. But see the question in my other mail regarding the sclp
facilities bit (does it cover pci or adapter reconfiguration?)

> 
> I would appreciate someone with AR access double checking.

I'd have hoped you had AR access :p

> 
> > 
> > There's still the question of when this sclp command first became
> > available...
> >   
> 
> I would argue that it should not be important for AR
> compliance. Currently it operates only on PCI so I doubt it
> pre-dates PCI. But I don't think the current implementation
> is buggy because it offers the sclp command regardless
> of the zPCI facility.

I'm just wondering if there's another facility we're missing. The zpci
facility might imply presence of adapter reconfiguration, but are there
other facilities implying that as well, or even a dedicated facility?

> 
> I don't know where should I look for the historical details
> which go beyond the AR.

If there is no independent facility, it is probably best to just always
provide the command, but fail for pci adapter type if the zpci facility
is off.

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Halil Pasic]

On 08/22/2017 03:24 PM, Cornelia Huck wrote:
> On Tue, 22 Aug 2017 14:58:37 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 08/22/2017 11:39 AM, Cornelia Huck wrote:
>>> On Tue, 22 Aug 2017 11:20:51 +0200
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>   
>>>> On 08/22/2017 10:39 AM, Cornelia Huck wrote:  
>>>>>> I'm fine either way. If I imagine having a lots of adapter types, then I
>>>>>> would expect a switch or a jumptable on the type before handling control
>>>>>> to the pci specific function. In this case statically not supported types
>>>>>> would probably get caught by the default branch of the switch and for a
>>>>>> jumptable it could even handle the dynamic case (based on the facilities)
>>>>>> trivially. In short both approaches can make sense.    
>>>>> I'm also wondering at the naming (the command sounds very
>>>>> pci-specific). I'd just stick with this approach (modulo a possible
>>>>> change of the response code, for which I need to rely on you guys).
>>>>>     
>>>>
>>>>
>>>> Well, the QEMU name of the command is misleading misleading. In the AR
>>>> it's called 'Configure I/O Adapter'. The PCI comes into the picture via
>>>> byte 8 of the SCCB, the so called adapter type. Valid values for the
>>>> adapter type are: 00-01 reserved; 02 PCI function; 03-FF reserved. So
>>>> at this point we only have PCI.  
>>>
>>> OK, misleading naming combined with missing documentation leads to
>>> confusion...
>>>
>>> So:
>>>
>>> - s/PCI/IOA/ for SCLP_CMDW_{CONFIGURE,DECONFIGURE}_PCI  
>> nod
>>
>>> - have a switch/case over byte 8 with only one case (pci)  
>>
>> Let's say some kind of a check for bit 8 is a good idea.
> 
> Yes.
> 
>>
>>> - move the pci feature check into the pci code(? - not sure)  
>>
>> Don't know. Architecturally I don't see any direct connection
>> between the pci feature and this command.
> 
> I'd either have the check in the pci case for the adapter type, or in
> the called function. It's probably cleaner to do the check before
> calling the pci function.
> 

nod

>>
>> The availability of SCLP_CMDW_{,DE}CONFIGURE_IOA is indicated
>> by the result of the read scp info command read info in
>> ReadInfo.facilities. I think we should assume that the read scp
>> info command is always there.
> 
> Sure. But see the question in my other mail regarding the sclp
> facilities bit (does it cover pci or adapter reconfiguration?)

It (SCLP_HAS_PCI_RECONFIG) exactly  covers adapter reconfiguration.
That's what I tried to say with the paragraph above.

> 
>>
>> I would appreciate someone with AR access double checking.
> 
> I'd have hoped you had AR access :p

Yes, my statements are based solely on my reading of the AR (me
still lacks druid-knowledge). What I've asked for is a second
opinion (because AR-s are a twisty maze).

> 
>>
>>>
>>> There's still the question of when this sclp command first became
>>> available...
>>>   
>>
>> I would argue that it should not be important for AR
>> compliance. Currently it operates only on PCI so I doubt it
>> pre-dates PCI. But I don't think the current implementation
>> is buggy because it offers the sclp command regardless
>> of the zPCI facility.
> 
> I'm just wondering if there's another facility we're missing. The zpci
> facility might imply presence of adapter reconfiguration, but are there
> other facilities implying that as well, or even a dedicated facility?

Yes. The SCLP facility with the facility code 33 (aka SCLP_HAS_PCI_RECONFIG).
It is the dedicated facility.

I don't think zPCI architecturally implies the presence of this SCLP
command. And logically I would say it's either the other way around
adapter reconfiguration implies zPCI (because otherwise adapter
reconfiguration would be completely useless) or bidirectional. 

> 
>>
>> I don't know where should I look for the historical details
>> which go beyond the AR.
> 
> If there is no independent facility, it is probably best to just always
> provide the command, but fail for pci adapter type if the zpci facility
> is off.

IMHO we should SCLP_RC_INVALID_SCLP_COMMAND iff we don't provide
SCLP_HAS_PCI_RECONFIG (which has bad name again s/PCI/IOA).

I don't know of any facility except basic SCLP on which 
SCLP_HAS_PCI_RECONFIG depends on. 

For me both presenting and not presenting SCLP_HAS_PCI_RECONFIG
to the guest (via read SCP info SCLP command) in the absence of
the zPCI feature makes sense. The late because of the likely historical
reasons, the former because I think the AR allows it and it gives us more
flexibility.

> 

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Cornelia Huck]

On Tue, 22 Aug 2017 15:24:34 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 22 Aug 2017 14:58:37 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
> > On 08/22/2017 11:39 AM, Cornelia Huck wrote:  
> > > On Tue, 22 Aug 2017 11:20:51 +0200
> > > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> > >> Well, the QEMU name of the command is misleading misleading. In the AR
> > >> it's called 'Configure I/O Adapter'. The PCI comes into the picture via
> > >> byte 8 of the SCCB, the so called adapter type. Valid values for the
> > >> adapter type are: 00-01 reserved; 02 PCI function; 03-FF reserved. So
> > >> at this point we only have PCI.   

OK, I need one more piece of information. 

We obviously need to check whether the sccb we got is long enough
before we try to access the command-specific field. How long is the
sccb supposed to be for configure I/O adapter? For pci, 16 bytes; in
general, I would guess that it needs to include at least atype and some
placeholder for the payload. What does the AR say?

Looking at the pci code, I also noted that it cheerfully uses the aid
field of the sccb before checking whether it is actually long enough...

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Cornelia Huck]

On Tue, 22 Aug 2017 15:54:32 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 08/22/2017 03:24 PM, Cornelia Huck wrote:
> > On Tue, 22 Aug 2017 14:58:37 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> >> The availability of SCLP_CMDW_{,DE}CONFIGURE_IOA is indicated
> >> by the result of the read scp info command read info in
> >> ReadInfo.facilities. I think we should assume that the read scp
> >> info command is always there.  
> > 
> > Sure. But see the question in my other mail regarding the sclp
> > facilities bit (does it cover pci or adapter reconfiguration?)  
> 
> It (SCLP_HAS_PCI_RECONFIG) exactly  covers adapter reconfiguration.
> That's what I tried to say with the paragraph above.

Sorry, I did not get that before. So we have another confusing name...

I'll just provide SCLP_HAS_PCI_RECONFIG unconditionally. Maybe
s/PCI/IOA/ here as well?

> 
> >   
> >>
> >> I would appreciate someone with AR access double checking.  
> > 
> > I'd have hoped you had AR access :p  
> 
> Yes, my statements are based solely on my reading of the AR (me
> still lacks druid-knowledge). What I've asked for is a second
> opinion (because AR-s are a twisty maze).

Be careful that you don't get eaten by a grue.

> 
> >   
> >>  
> >>>
> >>> There's still the question of when this sclp command first became
> >>> available...
> >>>     
> >>
> >> I would argue that it should not be important for AR
> >> compliance. Currently it operates only on PCI so I doubt it
> >> pre-dates PCI. But I don't think the current implementation
> >> is buggy because it offers the sclp command regardless
> >> of the zPCI facility.  
> > 
> > I'm just wondering if there's another facility we're missing. The zpci
> > facility might imply presence of adapter reconfiguration, but are there
> > other facilities implying that as well, or even a dedicated facility?  
> 
> Yes. The SCLP facility with the facility code 33 (aka SCLP_HAS_PCI_RECONFIG).
> It is the dedicated facility.

OK.

> 
> I don't think zPCI architecturally implies the presence of this SCLP
> command. And logically I would say it's either the other way around
> adapter reconfiguration implies zPCI (because otherwise adapter
> reconfiguration would be completely useless) or bidirectional. 

Not sure how useful pci would be without this. I'll just assume that we
have the facility, regardless whether pci is enabled for that
particular machine or not.

> 
> >   
> >>
> >> I don't know where should I look for the historical details
> >> which go beyond the AR.  
> > 
> > If there is no independent facility, it is probably best to just always
> > provide the command, but fail for pci adapter type if the zpci facility
> > is off.  
> 
> IMHO we should SCLP_RC_INVALID_SCLP_COMMAND iff we don't provide
> SCLP_HAS_PCI_RECONFIG (which has bad name again s/PCI/IOA).

Nod.

> 
> I don't know of any facility except basic SCLP on which 
> SCLP_HAS_PCI_RECONFIG depends on. 
> 
> For me both presenting and not presenting SCLP_HAS_PCI_RECONFIG
> to the guest (via read SCP info SCLP command) in the absence of
> the zPCI feature makes sense. The late because of the likely historical
> reasons, the former because I think the AR allows it and it gives us more
> flexibility.

I'll go with always presenting it. We'll just fail with invalid adapter
type for !pci.

Thanks for digging through the AR!

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Halil Pasic]

On 08/22/2017 04:06 PM, Cornelia Huck wrote:
> On Tue, 22 Aug 2017 15:24:34 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Tue, 22 Aug 2017 14:58:37 +0200
>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>
>>> On 08/22/2017 11:39 AM, Cornelia Huck wrote:  
>>>> On Tue, 22 Aug 2017 11:20:51 +0200
>>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>>>>> Well, the QEMU name of the command is misleading misleading. In the AR
>>>>> it's called 'Configure I/O Adapter'. The PCI comes into the picture via
>>>>> byte 8 of the SCCB, the so called adapter type. Valid values for the
>>>>> adapter type are: 00-01 reserved; 02 PCI function; 03-FF reserved. So
>>>>> at this point we only have PCI.   
> 
> OK, I need one more piece of information. 
> 
> We obviously need to check whether the sccb we got is long enough
> before we try to access the command-specific field. How long is the
> sccb supposed to be for configure I/O adapter? For pci, 16 bytes; in
> general, I would guess that it needs to include at least atype and some
> placeholder for the payload. What does the AR say?
>

The first 2 bytes of the SCCB designate it's length. For this particular
command it's at least 16 bytes (regardless of pci). The length is
marked as may be changed by the SCLP.
 
 
> Looking at the pci code, I also noted that it cheerfully uses the aid
> field of the sccb before checking whether it is actually long enough...
> 

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Cornelia Huck]

On Tue, 22 Aug 2017 16:27:38 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 08/22/2017 04:06 PM, Cornelia Huck wrote:
> > On Tue, 22 Aug 2017 15:24:34 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> >> On Tue, 22 Aug 2017 14:58:37 +0200
> >> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >>  
> >>> On 08/22/2017 11:39 AM, Cornelia Huck wrote:    
> >>>> On Tue, 22 Aug 2017 11:20:51 +0200
> >>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:  
> >   
> >>>>> Well, the QEMU name of the command is misleading misleading. In the AR
> >>>>> it's called 'Configure I/O Adapter'. The PCI comes into the picture via
> >>>>> byte 8 of the SCCB, the so called adapter type. Valid values for the
> >>>>> adapter type are: 00-01 reserved; 02 PCI function; 03-FF reserved. So
> >>>>> at this point we only have PCI.     
> > 
> > OK, I need one more piece of information. 
> > 
> > We obviously need to check whether the sccb we got is long enough
> > before we try to access the command-specific field. How long is the
> > sccb supposed to be for configure I/O adapter? For pci, 16 bytes; in
> > general, I would guess that it needs to include at least atype and some
> > placeholder for the payload. What does the AR say?
> >  
> 
> The first 2 bytes of the SCCB designate it's length. For this particular
> command it's at least 16 bytes (regardless of pci). The length is
> marked as may be changed by the SCLP.

Thanks for the info, this makes implementing it correctly much easier!

>  
>  
> > Looking at the pci code, I also noted that it cheerfully uses the aid
> > field of the sccb before checking whether it is actually long enough...
> >   
> 

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Halil Pasic]

On 08/22/2017 04:15 PM, Cornelia Huck wrote:
> On Tue, 22 Aug 2017 15:54:32 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 08/22/2017 03:24 PM, Cornelia Huck wrote:
>>> On Tue, 22 Aug 2017 14:58:37 +0200
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>>>> The availability of SCLP_CMDW_{,DE}CONFIGURE_IOA is indicated
>>>> by the result of the read scp info command read info in
>>>> ReadInfo.facilities. I think we should assume that the read scp
>>>> info command is always there.  
>>>
>>> Sure. But see the question in my other mail regarding the sclp
>>> facilities bit (does it cover pci or adapter reconfiguration?)  
>>
>> It (SCLP_HAS_PCI_RECONFIG) exactly  covers adapter reconfiguration.
>> That's what I tried to say with the paragraph above.
> 
> Sorry, I did not get that before. So we have another confusing name...
> 
> I'll just provide SCLP_HAS_PCI_RECONFIG unconditionally. Maybe
> s/PCI/IOA/ here as well?
> 

Yeah, I had the same idea a coupe of lines below.

>>
>>>   
>>>>
>>>> I would appreciate someone with AR access double checking.  
>>>
>>> I'd have hoped you had AR access :p  
>>
>> Yes, my statements are based solely on my reading of the AR (me
>> still lacks druid-knowledge). What I've asked for is a second
>> opinion (because AR-s are a twisty maze).
> 
> Be careful that you don't get eaten by a grue.
> 
>>
>>>   
>>>>  
>>>>>
>>>>> There's still the question of when this sclp command first became
>>>>> available...
>>>>>     
>>>>
>>>> I would argue that it should not be important for AR
>>>> compliance. Currently it operates only on PCI so I doubt it
>>>> pre-dates PCI. But I don't think the current implementation
>>>> is buggy because it offers the sclp command regardless
>>>> of the zPCI facility.  
>>>
>>> I'm just wondering if there's another facility we're missing. The zpci
>>> facility might imply presence of adapter reconfiguration, but are there
>>> other facilities implying that as well, or even a dedicated facility?  
>>
>> Yes. The SCLP facility with the facility code 33 (aka SCLP_HAS_PCI_RECONFIG).
>> It is the dedicated facility.
> 
> OK.
> 
>>
>> I don't think zPCI architecturally implies the presence of this SCLP
>> command. And logically I would say it's either the other way around
>> adapter reconfiguration implies zPCI (because otherwise adapter
>> reconfiguration would be completely useless) or bidirectional. 
> 
> Not sure how useful pci would be without this. I'll just assume that we
> have the facility, regardless whether pci is enabled for that
> particular machine or not.

I have no idea if there is another mechanism to put a pci adapter
into a configuration. If there isn't then we can agree on not too
useful.

> 
>>
>>>   
>>>>
>>>> I don't know where should I look for the historical details
>>>> which go beyond the AR.  
>>>
>>> If there is no independent facility, it is probably best to just always
>>> provide the command, but fail for pci adapter type if the zpci facility
>>> is off.  
>>
>> IMHO we should SCLP_RC_INVALID_SCLP_COMMAND iff we don't provide
>> SCLP_HAS_PCI_RECONFIG (which has bad name again s/PCI/IOA).
> 
> Nod.
> 
>>
>> I don't know of any facility except basic SCLP on which 
>> SCLP_HAS_PCI_RECONFIG depends on. 
>>
>> For me both presenting and not presenting SCLP_HAS_PCI_RECONFIG
>> to the guest (via read SCP info SCLP command) in the absence of
>> the zPCI feature makes sense. The late because of the likely historical
>> reasons, the former because I think the AR allows it and it gives us more
>> flexibility.
> 
> I'll go with always presenting it. We'll just fail with invalid adapter
> type for !pci.
> 
> Thanks for digging through the AR!
> 

You are welcome. I think we are in agreement. Looking forward to v2.

Halil 

Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Cornelia Huck]

On Tue, 22 Aug 2017 16:34:48 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> I think we are in agreement. Looking forward to v2.

FWIW, here's what I currently have. I'll test a bit before sending the
next version.

From 45e143bd820fe9c2f4e31dcc9feb5ad324d00077 Mon Sep 17 00:00:00 2001
From: Cornelia Huck <cohuck@redhat.com>
Date: Thu, 6 Jul 2017 17:13:14 +0200
Subject: [PATCH] s390x/sclp: properly guard pci-specific functions

If we do not provide zpci, pci reconfiguration via sclp is not available
either. I/O adapter configuration, however, should always be present.

Rename the values that refer to I/O adapter configuration (instead of only
pci) to make things clearer.

Move length checking of the sccb for I/O adapter configuration into the
common sclp code (out of the pci code). This also fixes an issue that
the pci code would refer to a field in the sccb before checking whether
it was actually long enough.

Check for the adapter type in the sccb and return unrecognized adapter
type if the guest tries to issue I/O adapter configure/deconfigure for
a type other than pci or for pci if the zpci facility is not provided.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/s390-pci-bus.c  | 14 ++------------
 hw/s390x/s390-pci-bus.h  |  8 --------
 hw/s390x/s390-pci-stub.c |  2 ++
 hw/s390x/sclp.c          | 39 ++++++++++++++++++++++++++++++++++-----
 include/hw/s390x/sclp.h  | 17 +++++++++++++----
 5 files changed, 51 insertions(+), 29 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index c57f6ebae0..0a31a4ae88 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -122,16 +122,11 @@ S390PCIBusDevice *s390_pci_find_dev_by_fid(S390pciState *s, uint32_t fid)
 
 void s390_pci_sclp_configure(SCCB *sccb)
 {
-    PciCfgSccb *psccb = (PciCfgSccb *)sccb;
+    IoaCfgSccb *psccb = (IoaCfgSccb *)sccb;
     S390PCIBusDevice *pbdev = s390_pci_find_dev_by_fid(s390_get_phb(),
                                                        be32_to_cpu(psccb->aid));
     uint16_t rc;
 
-    if (be16_to_cpu(sccb->h.length) < 16) {
-        rc = SCLP_RC_INSUFFICIENT_SCCB_LENGTH;
-        goto out;
-    }
-
     if (!pbdev) {
         DPRINTF("sclp config no dev found\n");
         rc = SCLP_RC_ADAPTER_ID_NOT_RECOGNIZED;
@@ -155,16 +150,11 @@ out:
 
 void s390_pci_sclp_deconfigure(SCCB *sccb)
 {
-    PciCfgSccb *psccb = (PciCfgSccb *)sccb;
+    IoaCfgSccb *psccb = (IoaCfgSccb *)sccb;
     S390PCIBusDevice *pbdev = s390_pci_find_dev_by_fid(s390_get_phb(),
                                                        be32_to_cpu(psccb->aid));
     uint16_t rc;
 
-    if (be16_to_cpu(sccb->h.length) < 16) {
-        rc = SCLP_RC_INSUFFICIENT_SCCB_LENGTH;
-        goto out;
-    }
-
     if (!pbdev) {
         DPRINTF("sclp deconfig no dev found\n");
         rc = SCLP_RC_ADAPTER_ID_NOT_RECOGNIZED;
diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
index 5df6292509..bd636abc28 100644
--- a/hw/s390x/s390-pci-bus.h
+++ b/hw/s390x/s390-pci-bus.h
@@ -244,14 +244,6 @@ typedef struct ChscSeiNt2Res {
     uint8_t ccdf[4016];
 } QEMU_PACKED ChscSeiNt2Res;
 
-typedef struct PciCfgSccb {
-    SCCBHeader header;
-    uint8_t atype;
-    uint8_t reserved1;
-    uint16_t reserved2;
-    uint32_t aid;
-} QEMU_PACKED PciCfgSccb;
-
 typedef struct S390MsixInfo {
     bool available;
     uint8_t table_bar;
diff --git a/hw/s390x/s390-pci-stub.c b/hw/s390x/s390-pci-stub.c
index cc7278a865..7a642d376c 100644
--- a/hw/s390x/s390-pci-stub.c
+++ b/hw/s390x/s390-pci-stub.c
@@ -20,10 +20,12 @@ int pci_chsc_sei_nt2_have_event(void)
 /* hw/s390x/sclp.c */
 void s390_pci_sclp_configure(SCCB *sccb)
 {
+    sccb->h.response_code = cpu_to_be16(SCLP_RC_ADAPTER_TYPE_NOT_RECOGNIZED);
 }
 
 void s390_pci_sclp_deconfigure(SCCB *sccb)
 {
+    sccb->h.response_code = cpu_to_be16(SCLP_RC_ADAPTER_TYPE_NOT_RECOGNIZED);
 }
 
 /* target/s390x/kvm.c */
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 9253dbbc64..7ae6a0e37a 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -80,7 +80,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     prepare_cpu_entries(sclp, read_info->entries, cpu_count);
 
     read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
-                                        SCLP_HAS_PCI_RECONFIG);
+                                        SCLP_HAS_IOA_RECONFIG);
 
     /* Memory Hotplug is only supported for the ccw machine type */
     if (mhd) {
@@ -354,6 +354,35 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
     sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
 }
 
+static void sclp_configure_io_adapter(SCLPDevice *sclp, SCCB *sccb,
+                                      bool configure)
+{
+    int rc;
+
+    if (be16_to_cpu(sccb->h.length) < 16) {
+        rc = SCLP_RC_INSUFFICIENT_SCCB_LENGTH;
+        goto out_err;
+    }
+
+    switch (((IoaCfgSccb *)sccb)->atype) {
+    case SCLP_RECONFIG_PCI_ATYPE:
+        if (s390_has_feat(S390_FEAT_ZPCI)) {
+            if (configure) {
+                s390_pci_sclp_configure(sccb);
+            } else {
+                s390_pci_sclp_deconfigure(sccb);
+            }
+            return;
+        }
+        /* fallthrough */
+    default:
+        rc = SCLP_RC_ADAPTER_TYPE_NOT_RECOGNIZED;
+    }
+
+ out_err:
+    sccb->h.response_code = cpu_to_be16(rc);
+}
+
 static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
 {
     SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
@@ -384,11 +413,11 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
     case SCLP_UNASSIGN_STORAGE:
         sclp_c->unassign_storage(sclp, sccb);
         break;
-    case SCLP_CMDW_CONFIGURE_PCI:
-        s390_pci_sclp_configure(sccb);
+    case SCLP_CMDW_CONFIGURE_IOA:
+        sclp_configure_io_adapter(sclp, sccb, true);
         break;
-    case SCLP_CMDW_DECONFIGURE_PCI:
-        s390_pci_sclp_deconfigure(sccb);
+    case SCLP_CMDW_DECONFIGURE_IOA:
+        sclp_configure_io_adapter(sclp, sccb, false);
         break;
     default:
         efc->command_handler(ef, sccb, code);
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index e71d526605..a72d096081 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -44,10 +44,10 @@
 #define SCLP_CMDW_DECONFIGURE_CPU               0x00100001
 
 /* SCLP PCI codes */
-#define SCLP_HAS_PCI_RECONFIG                   0x0000000040000000ULL
-#define SCLP_CMDW_CONFIGURE_PCI                 0x001a0001
-#define SCLP_CMDW_DECONFIGURE_PCI               0x001b0001
-#define SCLP_RECONFIG_PCI_ATPYE                 2
+#define SCLP_HAS_IOA_RECONFIG                   0x0000000040000000ULL
+#define SCLP_CMDW_CONFIGURE_IOA                 0x001a0001
+#define SCLP_CMDW_DECONFIGURE_IOA               0x001b0001
+#define SCLP_RECONFIG_PCI_ATYPE                 2
 
 /* SCLP response codes */
 #define SCLP_RC_NORMAL_READ_COMPLETION          0x0010
@@ -59,6 +59,7 @@
 #define SCLP_RC_INSUFFICIENT_SCCB_LENGTH        0x0300
 #define SCLP_RC_STANDBY_READ_COMPLETION         0x0410
 #define SCLP_RC_ADAPTER_IN_RESERVED_STATE       0x05f0
+#define SCLP_RC_ADAPTER_TYPE_NOT_RECOGNIZED     0x06f0
 #define SCLP_RC_ADAPTER_ID_NOT_RECOGNIZED       0x09f0
 #define SCLP_RC_INVALID_FUNCTION                0x40f0
 #define SCLP_RC_NO_EVENT_BUFFERS_STORED         0x60f0
@@ -167,6 +168,14 @@ typedef struct AssignStorage {
     uint16_t rn;
 } QEMU_PACKED AssignStorage;
 
+typedef struct IoaCfgSccb {
+    SCCBHeader header;
+    uint8_t atype;
+    uint8_t reserved1;
+    uint16_t reserved2;
+    uint32_t aid;
+} QEMU_PACKED IoaCfgSccb;
+
 typedef struct SCCB {
     SCCBHeader h;
     char data[SCCB_DATA_LEN];
-- 
2.13.5

Re: [Qemu-devel] [PATCH v4 09/10] s390x/kvm: msi route fixup for non-pci

[Cornelia Huck]

On Mon, 21 Aug 2017 17:30:58 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 08/21/2017 05:17 PM, Thomas Huth wrote:
> > On 21.08.2017 17:10, Halil Pasic wrote:
> > [...]  
> >> The situation is just complicated by the fact that there is code which
> >> relies on assert(true) asserting for correctness (e.g. virtio goes so far
> >> to make builds with normal asserts disabled fail). Thus for me it's hard
> >> to assume that the assertion is guaranteed to be disabled in production.  
> > 
> > FYI: https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg03608.html
> > 
> >  Thomas
> >   
> 
> Thanks, I've missed that. With that assumed it becomes either
> assert(false) or return -ENODEV but not both.
> 
> Regards,
> Halil 
> 

Thinking about this some more, this seems to be completely covered
within the next statement:

- For builds with pci completely disabled, we'll end up with NULL in
  both s390_get_phb() and s390_pci_find_dev_by_idx() and return -ENODEV.
- If only the zpci facility bit is not set, we'll hit the assert in
  s390_get_phb().

Without an error message, there does not really seem to be additional
value (other than failing explicitly), so I'll drop this patch.

(Yeah, deja vu...)

Re: [Qemu-devel] [PATCH v4 02/10] kvm: remove hard dependency on pci

[Pierre Morel]

On 22/08/2017 11:04, Cornelia Huck wrote:
> On Mon, 21 Aug 2017 18:02:12 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> On 21/08/2017 11:16, Cornelia Huck wrote:
>>> The msi routing code in kvm calls some pci functions: provide
>>> some stubs to enable builds without pci.
>>>
>>> Also, to make this more obvious, guard them via a pci_available boolean
>>> (which also can be reused in other places).
>>>
>>> Fixes: e1d4fb2de ("kvm-irqchip: x86: add msi route notify fn")
>>> Fixes: 767a554a0 ("kvm-all: Pass requester ID to MSI routing functions")
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>    accel/kvm/kvm-all.c  |  6 +++---
>>>    hw/pci/pci-stub.c    | 15 +++++++++++++++
>>>    hw/pci/pci.c         |  2 ++
>>>    include/hw/pci/pci.h |  2 ++
>>>    4 files changed, 22 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>> index 46ce479dc3..f85553a851 100644
>>> --- a/accel/kvm/kvm-all.c
>>> +++ b/accel/kvm/kvm-all.c
>>> @@ -1248,7 +1248,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
>>>        int virq;
>>>        MSIMessage msg = {0, 0};
>>>
>>> -    if (dev) {
>>> +    if (pci_available && dev) {
>>>            msg = pci_get_msi_message(dev, vector);
>>>        }
>>
>> Hi Conny,
>>
>> I did not find a case where pci_available is false and dev is true.
>> but anyway, sure is sure.
> 
> It makes things more obvious, I think.
> 
>>> diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c
>>> index ecad664946..ace43821ca 100644
>>> --- a/hw/pci/pci-stub.c
>>> +++ b/hw/pci/pci-stub.c
>>> @@ -23,10 +23,12 @@
>>>    #include "monitor/monitor.h"
>>>    #include "qapi/qmp/qerror.h"
>>>    #include "hw/pci/pci.h"
>>> +#include "hw/pci/msi.h"
>>
>> I think you forgot that...
>>
>>>    #include "qmp-commands.h"
>>>    #include "hw/pci/msi.h"
>>
>> ...you already have it included here. Didn't you ?
> 
> Hum, once should really be enough.
> 
>> otherwise LGTM
> 
> Thanks. Can I translate that into a tag? :)
> 

Yes,

Reviewed-by: Pierre Morel<pmorel@linux.vnet.ibm.com>




-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v4 08/10] s390x/pci: fence off instructions for non-pci

[Halil Pasic]

On 08/21/2017 11:16 AM, Cornelia Huck wrote:
> If a guest running on a machine without zpci issues a pci instruction,
> throw them an exception.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  target/s390x/kvm.c | 54 +++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 41 insertions(+), 13 deletions(-)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index bc62bba5b7..9de165d8b1 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1191,7 +1191,11 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run)
>  {
>      uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
> 
> -    return clp_service_call(cpu, r2);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        return clp_service_call(cpu, r2);
> +    } else {
> +        return -1;
> +    }

According to the AR the clp instruction ain't zPCI
only. OTOH if I read the AR correctly it might be
the only relevant one at the moment. To be more
precise, the CLP instruction is installed if one or
more of certain tree conditions are met. One of the
conditions is zPCI facility installed, and the other
two I don't really understand based on what I've
looked into.

>  }
> 
>  static int kvm_pcilg_service_call(S390CPU *cpu, struct kvm_run *run)
> @@ -1199,7 +1203,11 @@ static int kvm_pcilg_service_call(S390CPU *cpu, struct kvm_run *run)
>      uint8_t r1 = (run->s390_sieic.ipb & 0x00f00000) >> 20;
>      uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
> 
> -    return pcilg_service_call(cpu, r1, r2);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        return pcilg_service_call(cpu, r1, r2);
> +    } else {
> +        return -1;
> +    }
>  }
> 
>  static int kvm_pcistg_service_call(S390CPU *cpu, struct kvm_run *run)
> @@ -1207,7 +1215,11 @@ static int kvm_pcistg_service_call(S390CPU *cpu, struct kvm_run *run)
>      uint8_t r1 = (run->s390_sieic.ipb & 0x00f00000) >> 20;
>      uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
> 
> -    return pcistg_service_call(cpu, r1, r2);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        return pcistg_service_call(cpu, r1, r2);
> +    } else {
> +        return -1;
> +    }
>  }
> 
>  static int kvm_stpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
> @@ -1216,10 +1228,14 @@ static int kvm_stpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
>      uint64_t fiba;
>      uint8_t ar;
> 
> -    cpu_synchronize_state(CPU(cpu));
> -    fiba = get_base_disp_rxy(cpu, run, &ar);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        cpu_synchronize_state(CPU(cpu));
> +        fiba = get_base_disp_rxy(cpu, run, &ar);
> 
> -    return stpcifc_service_call(cpu, r1, fiba, ar);
> +        return stpcifc_service_call(cpu, r1, fiba, ar);
> +    } else {
> +        return -1;
> +    }
>  }
> 
>  static int kvm_sic_service_call(S390CPU *cpu, struct kvm_run *run)
> @@ -1247,7 +1263,11 @@ static int kvm_rpcit_service_call(S390CPU *cpu, struct kvm_run *run)
>      uint8_t r1 = (run->s390_sieic.ipb & 0x00f00000) >> 20;
>      uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
> 
> -    return rpcit_service_call(cpu, r1, r2);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        return rpcit_service_call(cpu, r1, r2);
> +    } else {
> +        return -1;
> +    }
>  }
> 
>  static int kvm_pcistb_service_call(S390CPU *cpu, struct kvm_run *run)
> @@ -1257,10 +1277,14 @@ static int kvm_pcistb_service_call(S390CPU *cpu, struct kvm_run *run)
>      uint64_t gaddr;
>      uint8_t ar;
> 
> -    cpu_synchronize_state(CPU(cpu));
> -    gaddr = get_base_disp_rsy(cpu, run, &ar);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        cpu_synchronize_state(CPU(cpu));
> +        gaddr = get_base_disp_rsy(cpu, run, &ar);
> 
> -    return pcistb_service_call(cpu, r1, r3, gaddr, ar);
> +        return pcistb_service_call(cpu, r1, r3, gaddr, ar);
> +    } else {
> +        return -1;
> +    }
>  }
> 
>  static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
> @@ -1269,10 +1293,14 @@ static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
>      uint64_t fiba;
>      uint8_t ar;
> 
> -    cpu_synchronize_state(CPU(cpu));
> -    fiba = get_base_disp_rxy(cpu, run, &ar);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        cpu_synchronize_state(CPU(cpu));
> +        fiba = get_base_disp_rxy(cpu, run, &ar);
> 
> -    return mpcifc_service_call(cpu, r1, fiba, ar);
> +        return mpcifc_service_call(cpu, r1, fiba, ar);
> +    } else {
> +        return -1;
> +    }

The rest is directly tied to the zPCI facility.

Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>

>  }
> 
>  static int handle_b9(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
> 

Re: [Qemu-devel] [PATCH v4 08/10] s390x/pci: fence off instructions for non-pci

[Cornelia Huck]

On Wed, 23 Aug 2017 16:10:55 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 08/21/2017 11:16 AM, Cornelia Huck wrote:
> > If a guest running on a machine without zpci issues a pci instruction,
> > throw them an exception.
> > 
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  target/s390x/kvm.c | 54 +++++++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 41 insertions(+), 13 deletions(-)
> > 
> > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> > index bc62bba5b7..9de165d8b1 100644
> > --- a/target/s390x/kvm.c
> > +++ b/target/s390x/kvm.c
> > @@ -1191,7 +1191,11 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run)
> >  {
> >      uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
> > 
> > -    return clp_service_call(cpu, r2);
> > +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> > +        return clp_service_call(cpu, r2);
> > +    } else {
> > +        return -1;
> > +    }  
> 
> According to the AR the clp instruction ain't zPCI
> only. OTOH if I read the AR correctly it might be
> the only relevant one at the moment. To be more
> precise, the CLP instruction is installed if one or
> more of certain tree conditions are met. One of the
> conditions is zPCI facility installed, and the other
> two I don't really understand based on what I've
> looked into.

Thx for the info. This sounds like it's ok to leave the check as-is.

> 
> >  }

(...)

> The rest is directly tied to the zPCI facility.
> 
> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>

Thanks!