[Qemu-devel] [PATCH v3 0/9] s390x: zPCI detangling

[Qemu-devel] [PATCH v3 0/9] s390x: zPCI detangling

[Cornelia Huck]

Next version, now without RFC. The goals are still the same:
- Being able to disable PCI support in a build completely.
- Properly fencing off PCI if the relevant facility bit is not provided.

Changes v2->v3:
- Only enable the zpci bit conditionally, keep the aen bit always on.
  Use a method that actually works...
- Some reordering in the msi route patch to keep the change small.
- Collected some r-bs.

Branch is still git://github.com/cohuck/qemu no-zpci-cpumodel

Likely 2.11 material, even though the 'fence off if facility bit is not
set' part could count as bugfixes. Thoughts?

Cornelia Huck (9):
  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

 default-configs/s390x-softmmu.mak |  2 +-
 hw/pci/pci-stub.c                 | 12 ++++++
 hw/s390x/Makefile.objs            |  3 +-
 hw/s390x/s390-pci-bus.c           |  9 ++++-
 hw/s390x/s390-pci-bus.h           |  5 ++-
 hw/s390x/s390-pci-stub.c          | 78 +++++++++++++++++++++++++++++++++++++++
 hw/s390x/s390-virtio-ccw.c        | 12 +++---
 hw/s390x/sclp.c                   | 19 ++++++++--
 target/s390x/ioinst.c             | 16 ++++++++
 target/s390x/kvm.c                | 61 +++++++++++++++++++++++-------
 10 files changed, 188 insertions(+), 29 deletions(-)
 create mode 100644 hw/s390x/s390-pci-stub.c

-- 
2.13.3

[Qemu-devel] [PATCH v3 1/9] 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.

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>
---
 hw/pci/pci-stub.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c
index ecad664946..bc228ce91e 100644
--- a/hw/pci/pci-stub.c
+++ b/hw/pci/pci-stub.c
@@ -23,6 +23,7 @@
 #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"
 
@@ -38,3 +39,14 @@ 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)
+{
+    assert(false);
+}
+
+uint16_t pci_requester_id(PCIDevice *dev)
+{
+    assert(false);
+}
-- 
2.13.3

[Qemu-devel] [PATCH v3 2/9] 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.3

[Qemu-devel] [PATCH v3 3/9] 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.3

[Qemu-devel] [PATCH v3 4/9] s390x/pci: do not advertise pci on non-pci builds

[Cornelia Huck]

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

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/s390-pci-bus.c  | 5 +++++
 hw/s390x/s390-pci-bus.h  | 1 +
 hw/s390x/s390-pci-stub.c | 4 ++++
 target/s390x/kvm.c       | 2 +-
 4 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index c57f6ebae0..7b30d4c7bd 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -34,6 +34,11 @@
         }                                                         \
     } while (0)
 
+void pci_enable_zpci_feature(S390CPUModel *model)
+{
+    set_bit(S390_FEAT_ZPCI, model->features);
+}
+
 S390pciState *s390_get_phb(void)
 {
     static S390pciState *phb;
diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
index 5df6292509..d8796536b0 100644
--- a/hw/s390x/s390-pci-bus.h
+++ b/hw/s390x/s390-pci-bus.h
@@ -333,4 +333,5 @@ S390PCIBusDevice *s390_pci_find_dev_by_fid(S390pciState *s, uint32_t fid);
 S390PCIBusDevice *s390_pci_find_next_avail_dev(S390pciState *s,
                                                S390PCIBusDevice *pbdev);
 
+void pci_enable_zpci_feature(S390CPUModel *model);
 #endif
diff --git a/hw/s390x/s390-pci-stub.c b/hw/s390x/s390-pci-stub.c
index cc7278a865..8ceaf482e7 100644
--- a/hw/s390x/s390-pci-stub.c
+++ b/hw/s390x/s390-pci-stub.c
@@ -72,3 +72,7 @@ S390PCIBusDevice *s390_pci_find_dev_by_idx(S390pciState *s, uint32_t idx)
 {
     return NULL;
 }
+
+void pci_enable_zpci_feature(S390CPUModel *model)
+{
+}
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index c4c5791d27..866ac3d414 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2662,7 +2662,7 @@ 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);
+    pci_enable_zpci_feature(model);
     set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
 
     if (s390_known_cpu_type(cpu_type)) {
-- 
2.13.3

[Qemu-devel] [PATCH v3 5/9] 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>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 1c7af39ce6..8be4a541c1 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -118,7 +118,6 @@ static void ccw_init(MachineState *machine)
 {
     int ret;
     VirtualCssBus *css_bus;
-    DeviceState *dev;
 
     s390_sclp_init();
     s390_memory_init(machine->ram_size);
@@ -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.3

[Qemu-devel] [PATCH v3 6/9] 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.3

[Qemu-devel] [PATCH v3 7/9] 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 866ac3d414..dc3f940b95 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.3

[Qemu-devel] [PATCH v3 8/9] 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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index dc3f940b95..fb3e21a3a4 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2424,6 +2424,11 @@ 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)) {
+        DPRINTF("fixup_msi_route on non-pci machine?!\n");
+        return -ENODEV;
+    }
+
     pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
     if (!pbdev) {
         DPRINTF("add_msi_route no dev\n");
-- 
2.13.3

[Qemu-devel] [PATCH v3 9/9] 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>
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 b227a36179..7adaa4a4fe 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=$(CONFIG_LINUX)
 CONFIG_VIRTIO=y
 CONFIG_SCLPCONSOLE=y
-- 
2.13.3

Re: [Qemu-devel] [PATCH v3 4/9] s390x/pci: do not advertise pci on non-pci builds

[Christian Borntraeger]

On 07/25/2017 05:33 PM, Cornelia Huck wrote:
> Only set the zpci feature bit on builds that actually support pci.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>

I like this variant.
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

> ---
>  hw/s390x/s390-pci-bus.c  | 5 +++++
>  hw/s390x/s390-pci-bus.h  | 1 +
>  hw/s390x/s390-pci-stub.c | 4 ++++
>  target/s390x/kvm.c       | 2 +-
>  4 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index c57f6ebae0..7b30d4c7bd 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -34,6 +34,11 @@
>          }                                                         \
>      } while (0)
> 
> +void pci_enable_zpci_feature(S390CPUModel *model)
> +{
> +    set_bit(S390_FEAT_ZPCI, model->features);
> +}
> +
>  S390pciState *s390_get_phb(void)
>  {
>      static S390pciState *phb;
> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
> index 5df6292509..d8796536b0 100644
> --- a/hw/s390x/s390-pci-bus.h
> +++ b/hw/s390x/s390-pci-bus.h
> @@ -333,4 +333,5 @@ S390PCIBusDevice *s390_pci_find_dev_by_fid(S390pciState *s, uint32_t fid);
>  S390PCIBusDevice *s390_pci_find_next_avail_dev(S390pciState *s,
>                                                 S390PCIBusDevice *pbdev);
> 
> +void pci_enable_zpci_feature(S390CPUModel *model);
>  #endif
> diff --git a/hw/s390x/s390-pci-stub.c b/hw/s390x/s390-pci-stub.c
> index cc7278a865..8ceaf482e7 100644
> --- a/hw/s390x/s390-pci-stub.c
> +++ b/hw/s390x/s390-pci-stub.c
> @@ -72,3 +72,7 @@ S390PCIBusDevice *s390_pci_find_dev_by_idx(S390pciState *s, uint32_t idx)
>  {
>      return NULL;
>  }
> +
> +void pci_enable_zpci_feature(S390CPUModel *model)
> +{
> +}
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index c4c5791d27..866ac3d414 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2662,7 +2662,7 @@ 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);
> +    pci_enable_zpci_feature(model);
>      set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
> 
>      if (s390_known_cpu_type(cpu_type)) {
> 

Re: [Qemu-devel] [PATCH v3 1/9] kvm: remove hard dependency on pci

[Thomas Huth]

On 25.07.2017 17:33, Cornelia Huck wrote:
> The msi routing code in kvm calls some pci functions: provide
> some stubs to enable builds without pci.
> 
> 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>
> ---
>  hw/pci/pci-stub.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c
> index ecad664946..bc228ce91e 100644
> --- a/hw/pci/pci-stub.c
> +++ b/hw/pci/pci-stub.c
> @@ -23,6 +23,7 @@
>  #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"
>  
> @@ -38,3 +39,14 @@ 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)
> +{
> +    assert(false);
> +}
> +
> +uint16_t pci_requester_id(PCIDevice *dev)
> +{
> +    assert(false);
> +}

Since this missed the 2.10 freeze anyway and we've got some more time to
discuss it: I still think it would be much cleaner to move the functions
from kvm-all.c that use these PCI functions into a separate file
kvm-pci.c instead and compile that only with CONFIG_PCI=y. That way we
likely also could prevent that more dependencies on CONFIG_PCI creep
into kvm-all.c again at later points in time.

I've now had a closer look, and it seems like the only affected
functions are kvm_irqchip_add_msi_route() and
kvm_irqchip_update_msi_route() ... and these seems only to be used in
code we would not care about on s390x with CONFIG_PCI=n. So could you
please have at least a try to move these two functions (and other
related msi functions) to a new file kvm-pci.c instead to see whether
that would work, too?

 Thanks,
  Thomas

Re: [Qemu-devel] [PATCH v3 3/9] s390x: chsc nt2 events are pci-only

[Thomas Huth]

On 25.07.2017 17:33, 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(-)
> 
> 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;
> +}

Not sure whether it is nicer/better, but you could also do the check
only once in ioinst_handle_chsc_sei() and then get along without these
wrapper functions?

 Thomas

Re: [Qemu-devel] [PATCH v3 4/9] s390x/pci: do not advertise pci on non-pci builds

[Thomas Huth]

On 25.07.2017 17:33, Cornelia Huck wrote:
> Only set the zpci feature bit on builds that actually support pci.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/s390-pci-bus.c  | 5 +++++
>  hw/s390x/s390-pci-bus.h  | 1 +
>  hw/s390x/s390-pci-stub.c | 4 ++++
>  target/s390x/kvm.c       | 2 +-
>  4 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index c57f6ebae0..7b30d4c7bd 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -34,6 +34,11 @@
>          }                                                         \
>      } while (0)
>  
> +void pci_enable_zpci_feature(S390CPUModel *model)
> +{
> +    set_bit(S390_FEAT_ZPCI, model->features);
> +}
> +
>  S390pciState *s390_get_phb(void)
>  {
>      static S390pciState *phb;
> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
> index 5df6292509..d8796536b0 100644
> --- a/hw/s390x/s390-pci-bus.h
> +++ b/hw/s390x/s390-pci-bus.h
> @@ -333,4 +333,5 @@ S390PCIBusDevice *s390_pci_find_dev_by_fid(S390pciState *s, uint32_t fid);
>  S390PCIBusDevice *s390_pci_find_next_avail_dev(S390pciState *s,
>                                                 S390PCIBusDevice *pbdev);
>  
> +void pci_enable_zpci_feature(S390CPUModel *model);
>  #endif
> diff --git a/hw/s390x/s390-pci-stub.c b/hw/s390x/s390-pci-stub.c
> index cc7278a865..8ceaf482e7 100644
> --- a/hw/s390x/s390-pci-stub.c
> +++ b/hw/s390x/s390-pci-stub.c
> @@ -72,3 +72,7 @@ S390PCIBusDevice *s390_pci_find_dev_by_idx(S390pciState *s, uint32_t idx)
>  {
>      return NULL;
>  }
> +
> +void pci_enable_zpci_feature(S390CPUModel *model)
> +{
> +}
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index c4c5791d27..866ac3d414 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2662,7 +2662,7 @@ 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);
> +    pci_enable_zpci_feature(model);
>      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 v3 8/9] s390x/kvm: msi route fixup for non-pci

[Thomas Huth]

On 25.07.2017 17:33, 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 | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index dc3f940b95..fb3e21a3a4 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2424,6 +2424,11 @@ 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)) {
> +        DPRINTF("fixup_msi_route on non-pci machine?!\n");
> +        return -ENODEV;
> +    }
> +
>      pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
>      if (!pbdev) {
>          DPRINTF("add_msi_route no dev\n");
> 

Is this additional check really needed here? I'd rather expect
s390_pci_find_dev_by_idx() to return NULL here already, so we should
already be fine, shouldn't we?

 Thomas

Re: [Qemu-devel] [PATCH v3 3/9] s390x: chsc nt2 events are pci-only

[Cornelia Huck]

On Wed, 26 Jul 2017 08:59:10 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 25.07.2017 17:33, 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(-)

> > 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;
> > +}  
> 
> Not sure whether it is nicer/better, but you could also do the check
> only once in ioinst_handle_chsc_sei() and then get along without these
> wrapper functions?

I prefer the wrapper functions, as it mirrors the nt0 wrappers and
nicely contains the fact that nt2 is pci specific.

Re: [Qemu-devel] [PATCH v3 8/9] s390x/kvm: msi route fixup for non-pci

[Cornelia Huck]

On Wed, 26 Jul 2017 09:09:06 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 25.07.2017 17:33, 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 | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> > index dc3f940b95..fb3e21a3a4 100644
> > --- a/target/s390x/kvm.c
> > +++ b/target/s390x/kvm.c
> > @@ -2424,6 +2424,11 @@ 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)) {
> > +        DPRINTF("fixup_msi_route on non-pci machine?!\n");
> > +        return -ENODEV;
> > +    }
> > +
> >      pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
> >      if (!pbdev) {
> >          DPRINTF("add_msi_route no dev\n");
> >   
> 
> Is this additional check really needed here? I'd rather expect
> s390_pci_find_dev_by_idx() to return NULL here already, so we should
> already be fine, shouldn't we?

Yes, the end result is the same, but (1) better safe than sorry and (2)
I can add a debug print here.

I had actually considered throwing an error here, as this function
really should not be called for !pci. Opinions?

Re: [Qemu-devel] [PATCH v3 8/9] s390x/kvm: msi route fixup for non-pci

[Thomas Huth]

On 26.07.2017 10:20, Cornelia Huck wrote:
> On Wed, 26 Jul 2017 09:09:06 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 25.07.2017 17:33, 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 | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>> index dc3f940b95..fb3e21a3a4 100644
>>> --- a/target/s390x/kvm.c
>>> +++ b/target/s390x/kvm.c
>>> @@ -2424,6 +2424,11 @@ 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)) {
>>> +        DPRINTF("fixup_msi_route on non-pci machine?!\n");
>>> +        return -ENODEV;
>>> +    }
>>> +
>>>      pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
>>>      if (!pbdev) {
>>>          DPRINTF("add_msi_route no dev\n");
>>>   
>>
>> Is this additional check really needed here? I'd rather expect
>> s390_pci_find_dev_by_idx() to return NULL here already, so we should
>> already be fine, shouldn't we?
> 
> Yes, the end result is the same, but (1) better safe than sorry and (2)
> I can add a debug print here.
> 
> I had actually considered throwing an error here, as this function
> really should not be called for !pci. Opinions?

At least the current DPRINTF will go unnoticed in 99% of all cases since
it is not compiled in by default. So I'd say either do a proper
error_report() or even g_assert() here, or simply drop the patch.

 Thomas

Re: [Qemu-devel] [PATCH v3 1/9] kvm: remove hard dependency on pci

[Cornelia Huck]

On Wed, 26 Jul 2017 08:52:44 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 25.07.2017 17:33, Cornelia Huck wrote:
> > The msi routing code in kvm calls some pci functions: provide
> > some stubs to enable builds without pci.
> > 
> > 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>
> > ---
> >  hw/pci/pci-stub.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c
> > index ecad664946..bc228ce91e 100644
> > --- a/hw/pci/pci-stub.c
> > +++ b/hw/pci/pci-stub.c
> > @@ -23,6 +23,7 @@
> >  #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"
> >  
> > @@ -38,3 +39,14 @@ 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)
> > +{
> > +    assert(false);
> > +}
> > +
> > +uint16_t pci_requester_id(PCIDevice *dev)
> > +{
> > +    assert(false);
> > +}  
> 
> Since this missed the 2.10 freeze anyway and we've got some more time to
> discuss it: I still think it would be much cleaner to move the functions
> from kvm-all.c that use these PCI functions into a separate file
> kvm-pci.c instead and compile that only with CONFIG_PCI=y. That way we
> likely also could prevent that more dependencies on CONFIG_PCI creep
> into kvm-all.c again at later points in time.
> 
> I've now had a closer look, and it seems like the only affected
> functions are kvm_irqchip_add_msi_route() and
> kvm_irqchip_update_msi_route() ... and these seems only to be used in
> code we would not care about on s390x with CONFIG_PCI=n. So could you
> please have at least a try to move these two functions (and other
> related msi functions) to a new file kvm-pci.c instead to see whether
> that would work, too?

I can try, as this missed the 2.10 boat anyway.

The code contains some parts that are not relevant in all cases (msi
routes if we don't use pci, adapter routes for anything not
s390x, ...), but I don't think trying to rip this out would be much of
an improvement. I'll try the minimal (well, not quite as minimal as
this patch) route instead.

Re: [Qemu-devel] [PATCH v3 8/9] s390x/kvm: msi route fixup for non-pci

[David Hildenbrand]

On 26.07.2017 10:25, Thomas Huth wrote:
> On 26.07.2017 10:20, Cornelia Huck wrote:
>> On Wed, 26 Jul 2017 09:09:06 +0200
>> Thomas Huth <thuth@redhat.com> wrote:
>>
>>> On 25.07.2017 17:33, 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 | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>>> index dc3f940b95..fb3e21a3a4 100644
>>>> --- a/target/s390x/kvm.c
>>>> +++ b/target/s390x/kvm.c
>>>> @@ -2424,6 +2424,11 @@ 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)) {
>>>> +        DPRINTF("fixup_msi_route on non-pci machine?!\n");
>>>> +        return -ENODEV;
>>>> +    }
>>>> +
>>>>      pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
>>>>      if (!pbdev) {
>>>>          DPRINTF("add_msi_route no dev\n");
>>>>   
>>>
>>> Is this additional check really needed here? I'd rather expect
>>> s390_pci_find_dev_by_idx() to return NULL here already, so we should
>>> already be fine, shouldn't we?
>>
>> Yes, the end result is the same, but (1) better safe than sorry and (2)
>> I can add a debug print here.
>>
>> I had actually considered throwing an error here, as this function
>> really should not be called for !pci. Opinions?
> 
> At least the current DPRINTF will go unnoticed in 99% of all cases since
> it is not compiled in by default. So I'd say either do a proper
> error_report() or even g_assert() here, or simply drop the patch.
> 
>  Thomas
> 

I'd vote for g_assert() or simply dropping it.

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v3 4/9] s390x/pci: do not advertise pci on non-pci builds

[Yi Min Zhao]

Good. This patch resolves the problem I mentioned in previous verion.

Thanks for your work.


在 2017/7/25 下午11:33, Cornelia Huck 写道:
> Only set the zpci feature bit on builds that actually support pci.
>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>   hw/s390x/s390-pci-bus.c  | 5 +++++
>   hw/s390x/s390-pci-bus.h  | 1 +
>   hw/s390x/s390-ci-stub.c | 4 ++++
>   target/s390x/kvm.c       | 2 +-
>   4 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index c57f6ebae0..7b30d4c7bd 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -34,6 +34,11 @@
>           }                                                         \
>       } while (0)
>
> +void pci_enable_zpci_feature(S390CPUModel *model)
> +{
> +    set_bit(S390_FEAT_ZPCI, model->features);
> +}
> +
>   S390pciState *s390_get_phb(void)
>   {
>       static S390pciState *phb;
> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
> index 5df6292509..d8796536b0 100644
> --- a/hw/s390x/s390-pci-bus.h
> +++ b/hw/s390x/s390-pci-bus.h
> @@ -333,4 +333,5 @@ S390PCIBusDevice *s390_pci_find_dev_by_fid(S390pciState *s, uint32_t fid);
>   S390PCIBusDevice *s390_pci_find_next_avail_dev(S390pciState *s,
>                                                  S390PCIBusDevice *pbdev);
>
> +void pci_enable_zpci_feature(S390CPUModel *model);
>   #endif
> diff --git a/hw/s390x/s390-pci-stub.c b/hw/s390x/s390-pci-stub.c
> index cc7278a865..8ceaf482e7 100644
> --- a/hw/s390x/s390-pci-stub.c
> +++ b/hw/s390x/s390-pci-stub.c
> @@ -72,3 +72,7 @@ S390PCIBusDevice *s390_pci_find_dev_by_idx(S390pciState *s, uint32_t idx)
>   {
>       return NULL;
>   }
> +
> +void pci_enable_zpci_feature(S390CPUModel *model)
> +{
> +}
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index c4c5791d27..866ac3d414 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2662,7 +2662,7 @@ 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);
> +    pci_enable_zpci_feature(model);
>       set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
>
>       if (s390_known_cpu_type(cpu_type)) {

Re: [Qemu-devel] [PATCH v3 0/9] s390x: zPCI detangling

[Christian Borntraeger]

On 07/25/2017 05:33 PM, Cornelia Huck wrote:
> Next version, now without RFC. The goals are still the same:
> - Being able to disable PCI support in a build completely.
> - Properly fencing off PCI if the relevant facility bit is not provided.
> 
> Changes v2->v3:
> - Only enable the zpci bit conditionally, keep the aen bit always on.
>   Use a method that actually works...
> - Some reordering in the msi route patch to keep the change small.
> - Collected some r-bs.
> 
> Branch is still git://github.com/cohuck/qemu no-zpci-cpumodel
> 
> Likely 2.11 material, even though the 'fence off if facility bit is not
> set' part could count as bugfixes. Thoughts?

In general, this series looks pretty good already, but I would prefer to
have it in 2.11. The PCI code now seems to work ok for passthrough devices
and this change would certainly require some retesting. 
I think it would stretch things too far to consider the fencing a 
must have bug fix.

> 
> Cornelia Huck (9):
>   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
> 
>  default-configs/s390x-softmmu.mak |  2 +-
>  hw/pci/pci-stub.c                 | 12 ++++++
>  hw/s390x/Makefile.objs            |  3 +-
>  hw/s390x/s390-pci-bus.c           |  9 ++++-
>  hw/s390x/s390-pci-bus.h           |  5 ++-
>  hw/s390x/s390-pci-stub.c          | 78 +++++++++++++++++++++++++++++++++++++++
>  hw/s390x/s390-virtio-ccw.c        | 12 +++---
>  hw/s390x/sclp.c                   | 19 ++++++++--
>  target/s390x/ioinst.c             | 16 ++++++++
>  target/s390x/kvm.c                | 61 +++++++++++++++++++++++-------
>  10 files changed, 188 insertions(+), 29 deletions(-)
>  create mode 100644 hw/s390x/s390-pci-stub.c
> 

Re: [Qemu-devel] [PATCH v3 4/9] s390x/pci: do not advertise pci on non-pci builds

[David Hildenbrand]

On 25.07.2017 17:33, Cornelia Huck wrote:
> Only set the zpci feature bit on builds that actually support pci.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/s390-pci-bus.c  | 5 +++++
>  hw/s390x/s390-pci-bus.h  | 1 +
>  hw/s390x/s390-pci-stub.c | 4 ++++
>  target/s390x/kvm.c       | 2 +-
>  4 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index c57f6ebae0..7b30d4c7bd 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -34,6 +34,11 @@
>          }                                                         \
>      } while (0)
>  
> +void pci_enable_zpci_feature(S390CPUModel *model)
> +{
> +    set_bit(S390_FEAT_ZPCI, model->features);
> +}
> +
>  S390pciState *s390_get_phb(void)
>  {
>      static S390pciState *phb;
> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
> index 5df6292509..d8796536b0 100644
> --- a/hw/s390x/s390-pci-bus.h
> +++ b/hw/s390x/s390-pci-bus.h
> @@ -333,4 +333,5 @@ S390PCIBusDevice *s390_pci_find_dev_by_fid(S390pciState *s, uint32_t fid);
>  S390PCIBusDevice *s390_pci_find_next_avail_dev(S390pciState *s,
>                                                 S390PCIBusDevice *pbdev);
>  
> +void pci_enable_zpci_feature(S390CPUModel *model);
>  #endif
> diff --git a/hw/s390x/s390-pci-stub.c b/hw/s390x/s390-pci-stub.c
> index cc7278a865..8ceaf482e7 100644
> --- a/hw/s390x/s390-pci-stub.c
> +++ b/hw/s390x/s390-pci-stub.c
> @@ -72,3 +72,7 @@ S390PCIBusDevice *s390_pci_find_dev_by_idx(S390pciState *s, uint32_t idx)
>  {
>      return NULL;
>  }
> +
> +void pci_enable_zpci_feature(S390CPUModel *model)
> +{
> +}
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index c4c5791d27..866ac3d414 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2662,7 +2662,7 @@ 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);
> +    pci_enable_zpci_feature(model);

While I see how this solves the problem, I don't really like it. If
there is a function "save_the_world()" I expect it to save the world in
all scenarios ;)

What about the same approach but rather

if(pci_configured())
	set_bit(S390_FEAT_ZPCI, model->features);

Or of course zpci_configured(), pci_zpci_bus_available() ...

>      set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
>  
>      if (s390_known_cpu_type(cpu_type)) {
> 


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v3 8/9] s390x/kvm: msi route fixup for non-pci

[Cornelia Huck]

On Wed, 26 Jul 2017 10:37:12 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 26.07.2017 10:25, Thomas Huth wrote:
> > On 26.07.2017 10:20, Cornelia Huck wrote:  
> >> On Wed, 26 Jul 2017 09:09:06 +0200
> >> Thomas Huth <thuth@redhat.com> wrote:
> >>  
> >>> On 25.07.2017 17:33, 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 | 5 +++++
> >>>>  1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> >>>> index dc3f940b95..fb3e21a3a4 100644
> >>>> --- a/target/s390x/kvm.c
> >>>> +++ b/target/s390x/kvm.c
> >>>> @@ -2424,6 +2424,11 @@ 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)) {
> >>>> +        DPRINTF("fixup_msi_route on non-pci machine?!\n");
> >>>> +        return -ENODEV;
> >>>> +    }
> >>>> +
> >>>>      pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
> >>>>      if (!pbdev) {
> >>>>          DPRINTF("add_msi_route no dev\n");
> >>>>     
> >>>
> >>> Is this additional check really needed here? I'd rather expect
> >>> s390_pci_find_dev_by_idx() to return NULL here already, so we should
> >>> already be fine, shouldn't we?  
> >>
> >> Yes, the end result is the same, but (1) better safe than sorry and (2)
> >> I can add a debug print here.
> >>
> >> I had actually considered throwing an error here, as this function
> >> really should not be called for !pci. Opinions?  
> > 
> > At least the current DPRINTF will go unnoticed in 99% of all cases since
> > it is not compiled in by default. So I'd say either do a proper
> > error_report() or even g_assert() here, or simply drop the patch.
> > 
> >  Thomas
> >   
> 
> I'd vote for g_assert() or simply dropping it.
> 

I don't like dropping the check. I'll go for g_assert().

Re: [Qemu-devel] [PATCH v3 4/9] s390x/pci: do not advertise pci on non-pci builds

[Cornelia Huck]

On Wed, 26 Jul 2017 11:28:17 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 25.07.2017 17:33, Cornelia Huck wrote:
> > Only set the zpci feature bit on builds that actually support pci.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  hw/s390x/s390-pci-bus.c  | 5 +++++
> >  hw/s390x/s390-pci-bus.h  | 1 +
> >  hw/s390x/s390-pci-stub.c | 4 ++++
> >  target/s390x/kvm.c       | 2 +-
> >  4 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > index c57f6ebae0..7b30d4c7bd 100644
> > --- a/hw/s390x/s390-pci-bus.c
> > +++ b/hw/s390x/s390-pci-bus.c
> > @@ -34,6 +34,11 @@
> >          }                                                         \
> >      } while (0)
> >  
> > +void pci_enable_zpci_feature(S390CPUModel *model)
> > +{
> > +    set_bit(S390_FEAT_ZPCI, model->features);
> > +}
> > +
> >  S390pciState *s390_get_phb(void)
> >  {
> >      static S390pciState *phb;
> > diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
> > index 5df6292509..d8796536b0 100644
> > --- a/hw/s390x/s390-pci-bus.h
> > +++ b/hw/s390x/s390-pci-bus.h
> > @@ -333,4 +333,5 @@ S390PCIBusDevice *s390_pci_find_dev_by_fid(S390pciState *s, uint32_t fid);
> >  S390PCIBusDevice *s390_pci_find_next_avail_dev(S390pciState *s,
> >                                                 S390PCIBusDevice *pbdev);
> >  
> > +void pci_enable_zpci_feature(S390CPUModel *model);
> >  #endif
> > diff --git a/hw/s390x/s390-pci-stub.c b/hw/s390x/s390-pci-stub.c
> > index cc7278a865..8ceaf482e7 100644
> > --- a/hw/s390x/s390-pci-stub.c
> > +++ b/hw/s390x/s390-pci-stub.c
> > @@ -72,3 +72,7 @@ S390PCIBusDevice *s390_pci_find_dev_by_idx(S390pciState *s, uint32_t idx)
> >  {
> >      return NULL;
> >  }
> > +
> > +void pci_enable_zpci_feature(S390CPUModel *model)
> > +{
> > +}
> > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> > index c4c5791d27..866ac3d414 100644
> > --- a/target/s390x/kvm.c
> > +++ b/target/s390x/kvm.c
> > @@ -2662,7 +2662,7 @@ 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);
> > +    pci_enable_zpci_feature(model);  
> 
> While I see how this solves the problem, I don't really like it. If
> there is a function "save_the_world()" I expect it to save the world in
> all scenarios ;)

What, you did not read the fine print? ;)

> 
> What about the same approach but rather
> 
> if(pci_configured())
> 	set_bit(S390_FEAT_ZPCI, model->features);
> 
> Or of course zpci_configured(), pci_zpci_bus_available() ...

I'm not sure that would help with readability. OTOH, it would be usable
to check for pci support in other places as well...

I can play with this a bit.

> 
> >      set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
> >  
> >      if (s390_known_cpu_type(cpu_type)) {
> >   
> 
> 

Re: [Qemu-devel] [PATCH v3 5/9] s390x/ccw: create s390 phb conditionally

[Christian Borntraeger]

On 07/25/2017 05:33 PM, Cornelia Huck wrote:
> Don't create the s390 pci host bridge if we do not provide the zpci
> facility.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>


> ---
>  hw/s390x/s390-virtio-ccw.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 1c7af39ce6..8be4a541c1 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -118,7 +118,6 @@ static void ccw_init(MachineState *machine)
>  {
>      int ret;
>      VirtualCssBus *css_bus;
> -    DeviceState *dev;
> 
>      s390_sclp_init();
>      s390_memory_init(machine->ram_size);
> @@ -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();
> 

Re: [Qemu-devel] [PATCH v3 4/9] s390x/pci: do not advertise pci on non-pci builds

[Thomas Huth]

On 26.07.2017 11:28, David Hildenbrand wrote:
> On 25.07.2017 17:33, Cornelia Huck wrote:
>> Only set the zpci feature bit on builds that actually support pci.
>>
>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>  hw/s390x/s390-pci-bus.c  | 5 +++++
>>  hw/s390x/s390-pci-bus.h  | 1 +
>>  hw/s390x/s390-pci-stub.c | 4 ++++
>>  target/s390x/kvm.c       | 2 +-
>>  4 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index c57f6ebae0..7b30d4c7bd 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -34,6 +34,11 @@
>>          }                                                         \
>>      } while (0)
>>  
>> +void pci_enable_zpci_feature(S390CPUModel *model)
>> +{
>> +    set_bit(S390_FEAT_ZPCI, model->features);
>> +}
>> +
>>  S390pciState *s390_get_phb(void)
>>  {
>>      static S390pciState *phb;
>> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
>> index 5df6292509..d8796536b0 100644
>> --- a/hw/s390x/s390-pci-bus.h
>> +++ b/hw/s390x/s390-pci-bus.h
>> @@ -333,4 +333,5 @@ S390PCIBusDevice *s390_pci_find_dev_by_fid(S390pciState *s, uint32_t fid);
>>  S390PCIBusDevice *s390_pci_find_next_avail_dev(S390pciState *s,
>>                                                 S390PCIBusDevice *pbdev);
>>  
>> +void pci_enable_zpci_feature(S390CPUModel *model);
>>  #endif
>> diff --git a/hw/s390x/s390-pci-stub.c b/hw/s390x/s390-pci-stub.c
>> index cc7278a865..8ceaf482e7 100644
>> --- a/hw/s390x/s390-pci-stub.c
>> +++ b/hw/s390x/s390-pci-stub.c
>> @@ -72,3 +72,7 @@ S390PCIBusDevice *s390_pci_find_dev_by_idx(S390pciState *s, uint32_t idx)
>>  {
>>      return NULL;
>>  }
>> +
>> +void pci_enable_zpci_feature(S390CPUModel *model)
>> +{
>> +}
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index c4c5791d27..866ac3d414 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -2662,7 +2662,7 @@ 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);
>> +    pci_enable_zpci_feature(model);
> 
> While I see how this solves the problem, I don't really like it. If
> there is a function "save_the_world()" I expect it to save the world in
> all scenarios ;)
> 
> What about the same approach but rather
> 
> if(pci_configured())
> 	set_bit(S390_FEAT_ZPCI, model->features);

Or maybe something like this (without introducing new functions):

    if (object_class_by_name(TYPE_S390_PCI_HOST_BRIDGE)) {
        set_bit(S390_FEAT_ZPCI, model->features);
    }

?

I haven't tested this, though, but I think it should work since the
S390_PCI classes are only compiled in if CONFIG_PCI=y.

 Thomas

Re: [Qemu-devel] [PATCH v3 1/9] kvm: remove hard dependency on pci

[Cornelia Huck]

On Wed, 26 Jul 2017 10:26:23 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 26 Jul 2017 08:52:44 +0200
> Thomas Huth <thuth@redhat.com> wrote:

> > Since this missed the 2.10 freeze anyway and we've got some more time to
> > discuss it: I still think it would be much cleaner to move the functions
> > from kvm-all.c that use these PCI functions into a separate file
> > kvm-pci.c instead and compile that only with CONFIG_PCI=y. That way we
> > likely also could prevent that more dependencies on CONFIG_PCI creep
> > into kvm-all.c again at later points in time.
> > 
> > I've now had a closer look, and it seems like the only affected
> > functions are kvm_irqchip_add_msi_route() and
> > kvm_irqchip_update_msi_route() ... and these seems only to be used in
> > code we would not care about on s390x with CONFIG_PCI=n. So could you
> > please have at least a try to move these two functions (and other
> > related msi functions) to a new file kvm-pci.c instead to see whether
> > that would work, too?  
> 
> I can try, as this missed the 2.10 boat anyway.
> 
> The code contains some parts that are not relevant in all cases (msi
> routes if we don't use pci, adapter routes for anything not
> s390x, ...), but I don't think trying to rip this out would be much of
> an improvement. I'll try the minimal (well, not quite as minimal as
> this patch) route instead.

I tried this; but it turned out to be one of those cases where you pull
on a stick and then find half a tree attached to it... Much of the code
is used by all routing variants, and I ended up exposing so many
functions that it was just ugly and unreadable.

The variant I'm now going with is keeping the original approach and
adding a pci_available variable, which I also use in the patch that
exposes the zpci facility patch only when pci is compiled in. I'll try
to post this after lunch.