[Qemu-devel] [PATCH RFC 0/7] s390x: zPCI detangling

[Qemu-devel] [PATCH RFC 0/7] s390x: zPCI detangling

[Cornelia Huck]

Current support of zPCI in qemu is still a bit sketchy. For example,
we're not actually advertising the pci cpu facilities to the guest,
and therefore a Linux guest will not even try to use any pci devices.
We're also still missing a proper SIC implementation.

This can lead to a situation where pci devices are configured for an
instance but the guest user is not able to make proper use of them.
In order to avoid such surprises, it makes sense to be able to configure
out pci support for s390x so that no pci devices show up in the list
of available devices.

This RFC patch set (also available at github.com/cohuck/qemu no-zpci) tries
to clean up some of the s390x pci intertwinings so that we can do a non-pci
build by simply removing 'CONFIG_PCI=y' from the default config. It also
removes a dependency on pci by kvm code, which is probably unintended (it
seems all kvm platforms also support pci).

Patch set is RFC as:
- I'm not sure whether my fencing is architecturally correct, as I don't
  have access to the respective documentation,
- I don't really like the #ifdeffery too much.

Cornelia Huck (7):
  kvm: remove hard dependency on pci
  s390x: chsc nt2 events are pci-only
  s390x/sclp: properly guard pci-specific functions
  s390x/ccw: create s390 phb conditionally
  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            |  2 +-
 hw/s390x/s390-pci-bus.c           |  4 ++--
 hw/s390x/s390-pci-bus.h           |  4 ++--
 hw/s390x/s390-virtio-ccw.c        |  9 +++++----
 hw/s390x/sclp.c                   | 17 ++++++++++++++---
 target/s390x/ioinst.c             | 18 ++++++++++++++++++
 target/s390x/kvm.c                | 28 ++++++++++++++++++++++++++++
 9 files changed, 83 insertions(+), 13 deletions(-)

-- 
2.13.0

[Qemu-devel] [PATCH RFC 1/7] 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 36d2c430c5..ec12962d73 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"
 
 PciInfoList *qmp_query_pci(Error **errp)
@@ -35,3 +36,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.0

[Qemu-devel] [PATCH RFC 2/7] s390x: chsc nt2 events are pci-only

[Cornelia Huck]

The nt2 event class is pci-only and therefore implemented in
the s390x pci code. Properly stub it out for non-pci builds.

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

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 5651483781..eee9a04eac 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 cf142a3e68..29d3da10f6 100644
--- a/hw/s390x/s390-pci-bus.h
+++ b/hw/s390x/s390-pci-bus.h
@@ -318,8 +318,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/target/s390x/ioinst.c b/target/s390x/ioinst.c
index d5e6b8066b..a552f53167 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -599,6 +599,24 @@ static int chsc_sei_nt0_have_event(void)
     return 0;
 }
 
+static int chsc_sei_nt2_get_event(void *res)
+{
+#ifdef CONFIG_PCI
+    return pci_chsc_sei_nt2_get_event(res);
+#else
+    return 1;
+#endif
+}
+
+static int chsc_sei_nt2_have_event(void)
+{
+#ifdef CONFIG_PCI
+    return pci_chsc_sei_nt2_have_event();
+#else
+    return 0;
+#endif
+}
+
 #define CHSC_SEI_NT0    (1ULL << 63)
 #define CHSC_SEI_NT2    (1ULL << 61)
 static void ioinst_handle_chsc_sei(ChscReq *req, ChscResp *res)
-- 
2.13.0

[Qemu-devel] [PATCH RFC 3/7] s390x/sclp: properly guard pci-specific functions

[Cornelia Huck]

For non-pci builds, pci reconfiguration via sclp is not available.
Don't indicate it in the sclp facilities and return an invalid
command if the guest tries to issue pci configure/deconfigure.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/sclp.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 83d6023894..d20baa3bbd 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);
+#ifdef CONFIG_PCI
+    sclp_facilities |= SCLP_HAS_PCI_RECONFIG;
+#endif
+    read_info->facilities = cpu_to_be64(sclp_facilities);
 
     /* Memory Hotplug is only supported for the ccw machine type */
     if (mhd) {
@@ -386,10 +389,18 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
         sclp_c->unassign_storage(sclp, sccb);
         break;
     case SCLP_CMDW_CONFIGURE_PCI:
+#ifdef CONFIG_PCI
         s390_pci_sclp_configure(sccb);
+#else
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
+#endif
         break;
     case SCLP_CMDW_DECONFIGURE_PCI:
-        s390_pci_sclp_deconfigure(sccb);
+#ifdef CONFIG_PCI
+        sclp_c->pci_sclp_deconfigure(sccb);
+#else
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
+#endif
         break;
     default:
         efc->command_handler(ef, sccb, code);
-- 
2.13.0

[Qemu-devel] [PATCH RFC 4/7] s390x/ccw: create s390 phb conditionally

[Cornelia Huck]

Don't create the s390 pci host bridge for non-pci builds.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 41ca6668e2..563f7702a1 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -114,7 +114,6 @@ static void ccw_init(MachineState *machine)
 {
     int ret;
     VirtualCssBus *css_bus;
-    DeviceState *dev;
 
     s390_sclp_init();
     s390_memory_init(machine->ram_size);
@@ -126,12 +125,14 @@ static void ccw_init(MachineState *machine)
     s390_init_ipl_dev(machine->kernel_filename, machine->kernel_cmdline,
                       machine->initrd_filename, "s390-ccw.img",
                       "s390-netboot.img", true);
-
-    dev = qdev_create(NULL, TYPE_S390_PCI_HOST_BRIDGE);
+#ifdef CONFIG_PCI
+    {
+    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);
-
+    }
+#endif
     /* register hypercalls */
     virtio_ccw_register_hcalls();
 
-- 
2.13.0

[Qemu-devel] [PATCH RFC 5/7] s390x/pci: fence off instructions for non-pci

[Cornelia Huck]

If a guest running on a non-pci build issues a pci instruction,
throw them an exception.

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

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index a3d00196f4..c5c7c27a21 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1160,6 +1160,9 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run)
 {
     uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
 
+#ifndef CONFIG_PCI
+    return -1;
+#endif
     return clp_service_call(cpu, r2);
 }
 
@@ -1168,6 +1171,9 @@ 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;
 
+#ifndef CONFIG_PCI
+    return -1;
+#endif
     return pcilg_service_call(cpu, r1, r2);
 }
 
@@ -1176,6 +1182,9 @@ 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;
 
+#ifndef CONFIG_PCI
+    return -1;
+#endif
     return pcistg_service_call(cpu, r1, r2);
 }
 
@@ -1188,11 +1197,17 @@ static int kvm_stpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
     cpu_synchronize_state(CPU(cpu));
     fiba = get_base_disp_rxy(cpu, run, &ar);
 
+#ifndef CONFIG_PCI
+    return -1;
+#endif
     return stpcifc_service_call(cpu, r1, fiba, ar);
 }
 
 static int kvm_sic_service_call(S390CPU *cpu, struct kvm_run *run)
 {
+#ifndef CONFIG_PCI
+    return -1;
+#endif
     /* NOOP */
     return 0;
 }
@@ -1202,6 +1217,9 @@ 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;
 
+#ifndef CONFIG_PCI
+    return -1;
+#endif
     return rpcit_service_call(cpu, r1, r2);
 }
 
@@ -1212,6 +1230,9 @@ static int kvm_pcistb_service_call(S390CPU *cpu, struct kvm_run *run)
     uint64_t gaddr;
     uint8_t ar;
 
+#ifndef CONFIG_PCI
+    return -1;
+#endif
     cpu_synchronize_state(CPU(cpu));
     gaddr = get_base_disp_rsy(cpu, run, &ar);
 
@@ -1224,6 +1245,9 @@ static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
     uint64_t fiba;
     uint8_t ar;
 
+#ifndef CONFIG_PCI
+    return -1;
+#endif
     cpu_synchronize_state(CPU(cpu));
     fiba = get_base_disp_rxy(cpu, run, &ar);
 
-- 
2.13.0

[Qemu-devel] [PATCH RFC 6/7] s390x/kvm: msi route fixup for non-pci

[Cornelia Huck]

On non-pci builds, 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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index c5c7c27a21..911b6da3f0 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2351,6 +2351,7 @@ int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu)
 int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
                              uint64_t address, uint32_t data, PCIDevice *dev)
 {
+#ifdef CONFIG_PCI
     S390PCIBusDevice *pbdev;
     uint32_t idx = data >> ZPCI_MSI_VEC_BITS;
     uint32_t vec = data & ZPCI_MSI_VEC_MASK;
@@ -2371,6 +2372,9 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
     route->u.adapter.ind_offset = pbdev->routes.adapter.ind_offset;
     route->u.adapter.adapter_id = pbdev->routes.adapter.adapter_id;
     return 0;
+#else
+    return -ENODEV;
+#endif
 }
 
 int kvm_arch_add_msi_route_post(struct kvm_irq_routing_entry *route,
-- 
2.13.0

[Qemu-devel] [PATCH RFC 7/7] s390x: refine pci dependencies

[Cornelia Huck]

We have some code that 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.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 default-configs/s390x-softmmu.mak | 2 +-
 hw/s390x/Makefile.objs            | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

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
diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index a8e5575a8a..7040a2c7f2 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -11,7 +11,7 @@ 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-y += s390-skeys.o
 obj-$(CONFIG_KVM) += s390-skeys-kvm.o
 obj-y += s390-ccw.o
-- 
2.13.0

Re: [Qemu-devel] [PATCH RFC 5/7] s390x/pci: fence off instructions for non-pci

[Christian Borntraeger]

On 07/07/2017 02:21 PM, Cornelia Huck wrote:
> If a guest running on a non-pci build issues a pci instruction,
> throw them an exception.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  target/s390x/kvm.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index a3d00196f4..c5c7c27a21 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1160,6 +1160,9 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run)
>  {
>      uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
> 
> +#ifndef CONFIG_PCI
> +    return -1;
> +#endif

Instead of this ifdefing, can you use the cpu model to decide if the instruction
should be available? We need to do this anyway for proper handling.

You can then fence off the PCI bits in the CPU model for
CONFIG_PCI == off.

Re: [Qemu-devel] [PATCH RFC 5/7] s390x/pci: fence off instructions for non-pci

[Thomas Huth]

On 07.07.2017 14:21, Cornelia Huck wrote:
> If a guest running on a non-pci build issues a pci instruction,
> throw them an exception.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  target/s390x/kvm.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index a3d00196f4..c5c7c27a21 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1160,6 +1160,9 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run)
>  {
>      uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
>  
> +#ifndef CONFIG_PCI
> +    return -1;
> +#endif
>      return clp_service_call(cpu, r2);
>  }
>  
> @@ -1168,6 +1171,9 @@ 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;
>  
> +#ifndef CONFIG_PCI
> +    return -1;
> +#endif
>      return pcilg_service_call(cpu, r1, r2);
>  }

pcilg_service_call() seems to be defined in s390-pci-inst.c ... which
you later remove from the !CONFIG_PCI builds... so I wonder why this
still compiles ... I guess GCC is smart enough to optimize it away.
Anyway, to be on the safe side (and to be able to compile with -O0), you
should maybe rather do this instead:

#ifndef CONFIG_PCI
    return -1;
#else
    return pcilg_service_call(cpu, r1, r2);
#endif

?

 Thomas

Re: [Qemu-devel] [PATCH RFC 2/7] s390x: chsc nt2 events are pci-only

[Christian Borntraeger]

On 07/07/2017 02:21 PM, Cornelia Huck wrote:
> The nt2 event class is pci-only and therefore implemented in
> the s390x pci code. Properly stub it out for non-pci builds.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/s390-pci-bus.c |  4 ++--
>  hw/s390x/s390-pci-bus.h |  4 ++--
>  target/s390x/ioinst.c   | 18 ++++++++++++++++++
>  3 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 5651483781..eee9a04eac 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 cf142a3e68..29d3da10f6 100644
> --- a/hw/s390x/s390-pci-bus.h
> +++ b/hw/s390x/s390-pci-bus.h
> @@ -318,8 +318,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/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index d5e6b8066b..a552f53167 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -599,6 +599,24 @@ static int chsc_sei_nt0_have_event(void)
>      return 0;
>  }
> 
> +static int chsc_sei_nt2_get_event(void *res)
> +{
> +#ifdef CONFIG_PCI
> +    return pci_chsc_sei_nt2_get_event(res);
> +#else
> +    return 1;
> +#endif
> +}
> +
> +static int chsc_sei_nt2_have_event(void)
> +{
> +#ifdef CONFIG_PCI
> +    return pci_chsc_sei_nt2_have_event();
> +#else
> +    return 0;
> +#endif

I think this is similar to the io instructions. We should use the CPU
model to decide what to return. This will then cover the 3 cases
- CONFIG_PCI off
- CONFIG_PCI on, but PCI disabled via cpu model
- CONFIG_PCI on, and PCI enabled via cpu model

no?


> +}
> +
>  #define CHSC_SEI_NT0    (1ULL << 63)
>  #define CHSC_SEI_NT2    (1ULL << 61)
>  static void ioinst_handle_chsc_sei(ChscReq *req, ChscResp *res)
> 

Re: [Qemu-devel] [PATCH RFC 5/7] s390x/pci: fence off instructions for non-pci

[Cornelia Huck]

On Fri, 7 Jul 2017 14:55:23 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 07/07/2017 02:21 PM, Cornelia Huck wrote:
> > If a guest running on a non-pci build issues a pci instruction,
> > throw them an exception.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  target/s390x/kvm.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> > index a3d00196f4..c5c7c27a21 100644
> > --- a/target/s390x/kvm.c
> > +++ b/target/s390x/kvm.c
> > @@ -1160,6 +1160,9 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run)
> >  {
> >      uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
> > 
> > +#ifndef CONFIG_PCI
> > +    return -1;
> > +#endif  
> 
> Instead of this ifdefing, can you use the cpu model to decide if the instruction
> should be available? We need to do this anyway for proper handling.
> 
> You can then fence off the PCI bits in the CPU model for
> CONFIG_PCI == off.

Sounds like a good idea, I'll give it a try.

We'll probably also want to fence off the sclp facility bit via that
mechanism.

Re: [Qemu-devel] [PATCH RFC 5/7] s390x/pci: fence off instructions for non-pci

[Cornelia Huck]

On Fri, 7 Jul 2017 15:00:20 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 07.07.2017 14:21, Cornelia Huck wrote:
> > If a guest running on a non-pci build issues a pci instruction,
> > throw them an exception.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  target/s390x/kvm.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> > index a3d00196f4..c5c7c27a21 100644
> > --- a/target/s390x/kvm.c
> > +++ b/target/s390x/kvm.c
> > @@ -1160,6 +1160,9 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run)
> >  {
> >      uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
> >  
> > +#ifndef CONFIG_PCI
> > +    return -1;
> > +#endif
> >      return clp_service_call(cpu, r2);
> >  }
> >  
> > @@ -1168,6 +1171,9 @@ 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;
> >  
> > +#ifndef CONFIG_PCI
> > +    return -1;
> > +#endif
> >      return pcilg_service_call(cpu, r1, r2);
> >  }  
> 
> pcilg_service_call() seems to be defined in s390-pci-inst.c ... which
> you later remove from the !CONFIG_PCI builds... so I wonder why this
> still compiles ... I guess GCC is smart enough to optimize it away.

Yes, as the second return is not reachable anymore.

> Anyway, to be on the safe side (and to be able to compile with -O0), you
> should maybe rather do this instead:
> 
> #ifndef CONFIG_PCI
>     return -1;
> #else
>     return pcilg_service_call(cpu, r1, r2);
> #endif
> 
> ?

I'll try Christian's suggestion to use the cpu model first. However,
we'll need some ifdeffery somewhere... maybe we should introduce a
zpci-stub.c for the referenced functions.

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

[Thomas Huth]

On 07.07.2017 14:21, 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 36d2c430c5..ec12962d73 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"
>  
>  PciInfoList *qmp_query_pci(Error **errp)
> @@ -35,3 +36,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);
> +}

Do you need a dummy return statement in here in case somebody ever tries
to compile the code with NDEBUG ? Or maybe rather use abort() instead?

 Thomas

Re: [Qemu-devel] [PATCH RFC 2/7] s390x: chsc nt2 events are pci-only

[Cornelia Huck]

On Fri, 7 Jul 2017 15:01:59 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 07/07/2017 02:21 PM, Cornelia Huck wrote:
> > The nt2 event class is pci-only and therefore implemented in
> > the s390x pci code. Properly stub it out for non-pci builds.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  hw/s390x/s390-pci-bus.c |  4 ++--
> >  hw/s390x/s390-pci-bus.h |  4 ++--
> >  target/s390x/ioinst.c   | 18 ++++++++++++++++++
> >  3 files changed, 22 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > index 5651483781..eee9a04eac 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 cf142a3e68..29d3da10f6 100644
> > --- a/hw/s390x/s390-pci-bus.h
> > +++ b/hw/s390x/s390-pci-bus.h
> > @@ -318,8 +318,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/target/s390x/ioinst.c b/target/s390x/ioinst.c
> > index d5e6b8066b..a552f53167 100644
> > --- a/target/s390x/ioinst.c
> > +++ b/target/s390x/ioinst.c
> > @@ -599,6 +599,24 @@ static int chsc_sei_nt0_have_event(void)
> >      return 0;
> >  }
> > 
> > +static int chsc_sei_nt2_get_event(void *res)
> > +{
> > +#ifdef CONFIG_PCI
> > +    return pci_chsc_sei_nt2_get_event(res);
> > +#else
> > +    return 1;
> > +#endif
> > +}
> > +
> > +static int chsc_sei_nt2_have_event(void)
> > +{
> > +#ifdef CONFIG_PCI
> > +    return pci_chsc_sei_nt2_have_event();
> > +#else
> > +    return 0;
> > +#endif  
> 
> I think this is similar to the io instructions. We should use the CPU
> model to decide what to return. This will then cover the 3 cases
> - CONFIG_PCI off
> - CONFIG_PCI on, but PCI disabled via cpu model
> - CONFIG_PCI on, and PCI enabled via cpu model
> 
> no?

Using the cpu model sounds good, but I think we'll really need a stub
file for non-pci...

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

[Cornelia Huck]

On Fri, 7 Jul 2017 15:11:15 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 07.07.2017 14:21, 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 36d2c430c5..ec12962d73 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"
> >  
> >  PciInfoList *qmp_query_pci(Error **errp)
> > @@ -35,3 +36,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);
> > +}  
> 
> Do you need a dummy return statement in here in case somebody ever tries
> to compile the code with NDEBUG ? Or maybe rather use abort() instead?

Right, I did not consider that.

Probably assert() + return. Should Never Happen(tm), but I don't really
want qemu to abort() on an unsuspecting user.

Re: [Qemu-devel] [PATCH RFC 5/7] s390x/pci: fence off instructions for non-pci

[Cornelia Huck]

On Fri, 7 Jul 2017 15:04:52 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Fri, 7 Jul 2017 14:55:23 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
> > On 07/07/2017 02:21 PM, Cornelia Huck wrote:  
> > > If a guest running on a non-pci build issues a pci instruction,
> > > throw them an exception.
> > > 
> > > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > > ---
> > >  target/s390x/kvm.c | 24 ++++++++++++++++++++++++
> > >  1 file changed, 24 insertions(+)
> > > 
> > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> > > index a3d00196f4..c5c7c27a21 100644
> > > --- a/target/s390x/kvm.c
> > > +++ b/target/s390x/kvm.c
> > > @@ -1160,6 +1160,9 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run)
> > >  {
> > >      uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
> > > 
> > > +#ifndef CONFIG_PCI
> > > +    return -1;
> > > +#endif    
> > 
> > Instead of this ifdefing, can you use the cpu model to decide if the instruction
> > should be available? We need to do this anyway for proper handling.
> > 
> > You can then fence off the PCI bits in the CPU model for
> > CONFIG_PCI == off.  
> 
> Sounds like a good idea, I'll give it a try.
> 
> We'll probably also want to fence off the sclp facility bit via that
> mechanism.

Slight problem here... we don't have the relevant facilities defined
yet, and they are not in the POP (other than "Assigned to IBM internal
use").

While I'm pretty sure that the magic number is 69 (judging from the
Linux code), I think they should be introduced in a patch by someone
who has access to the documentation including the proper names.

Re: [Qemu-devel] [PATCH RFC 5/7] s390x/pci: fence off instructions for non-pci

[Christian Borntraeger]

On 07/10/2017 01:04 PM, Cornelia Huck wrote:
> On Fri, 7 Jul 2017 15:04:52 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Fri, 7 Jul 2017 14:55:23 +0200
>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>
>>> On 07/07/2017 02:21 PM, Cornelia Huck wrote:  
>>>> If a guest running on a non-pci build issues a pci instruction,
>>>> throw them an exception.
>>>>
>>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>>> ---
>>>>  target/s390x/kvm.c | 24 ++++++++++++++++++++++++
>>>>  1 file changed, 24 insertions(+)
>>>>
>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>>> index a3d00196f4..c5c7c27a21 100644
>>>> --- a/target/s390x/kvm.c
>>>> +++ b/target/s390x/kvm.c
>>>> @@ -1160,6 +1160,9 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run)
>>>>  {
>>>>      uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
>>>>
>>>> +#ifndef CONFIG_PCI
>>>> +    return -1;
>>>> +#endif    
>>>
>>> Instead of this ifdefing, can you use the cpu model to decide if the instruction
>>> should be available? We need to do this anyway for proper handling.
>>>
>>> You can then fence off the PCI bits in the CPU model for
>>> CONFIG_PCI == off.  
>>
>> Sounds like a good idea, I'll give it a try.
>>
>> We'll probably also want to fence off the sclp facility bit via that
>> mechanism.
> 
> Slight problem here... we don't have the relevant facilities defined
> yet, and they are not in the POP (other than "Assigned to IBM internal
> use").
> 
> While I'm pretty sure that the magic number is 69 (judging from the
> Linux code), I think they should be introduced in a patch by someone
> who has access to the documentation including the proper names.

I will try to get some patches out for PCI in the next days that will contain
the PCI related facilities.

Re: [Qemu-devel] [PATCH RFC 5/7] s390x/pci: fence off instructions for non-pci

[Cornelia Huck]

On Mon, 10 Jul 2017 14:41:47 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 07/10/2017 01:04 PM, Cornelia Huck wrote:
> > On Fri, 7 Jul 2017 15:04:52 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> >> On Fri, 7 Jul 2017 14:55:23 +0200
> >> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >>  
> >>> On 07/07/2017 02:21 PM, Cornelia Huck wrote:    
> >>>> If a guest running on a non-pci build issues a pci instruction,
> >>>> throw them an exception.
> >>>>
> >>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> >>>> ---
> >>>>  target/s390x/kvm.c | 24 ++++++++++++++++++++++++
> >>>>  1 file changed, 24 insertions(+)
> >>>>
> >>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> >>>> index a3d00196f4..c5c7c27a21 100644
> >>>> --- a/target/s390x/kvm.c
> >>>> +++ b/target/s390x/kvm.c
> >>>> @@ -1160,6 +1160,9 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run)
> >>>>  {
> >>>>      uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
> >>>>
> >>>> +#ifndef CONFIG_PCI
> >>>> +    return -1;
> >>>> +#endif      
> >>>
> >>> Instead of this ifdefing, can you use the cpu model to decide if the instruction
> >>> should be available? We need to do this anyway for proper handling.
> >>>
> >>> You can then fence off the PCI bits in the CPU model for
> >>> CONFIG_PCI == off.    
> >>
> >> Sounds like a good idea, I'll give it a try.
> >>
> >> We'll probably also want to fence off the sclp facility bit via that
> >> mechanism.  
> > 
> > Slight problem here... we don't have the relevant facilities defined
> > yet, and they are not in the POP (other than "Assigned to IBM internal
> > use").
> > 
> > While I'm pretty sure that the magic number is 69 (judging from the
> > Linux code), I think they should be introduced in a patch by someone
> > who has access to the documentation including the proper names.  
> 
> I will try to get some patches out for PCI in the next days that will contain
> the PCI related facilities.
> 

Cool, thanks!