[PATCH v1 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[PATCH v1 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[Collin Walling]

This patch series introduces two features for an s390 KVM quest:
    - Extended-Length SCCB (els) for the Read SCP/CPU Info SCLP 
        commands
    - DIAGNOSE 0x318 (diag318) enabling / migration handling

The diag318 feature depends on els and KVM support.

The els feature is handled entirely with QEMU, and does not require 
KVM support.

These patches are introduced together for two main reasons:
    - els allows diag318 to exist while retaining the original 248 
        VCPU max
    - diag318 is presented to show how els is useful

Full els support is dependant on the Linux kernel, which must react
to the SCLP response code and set an appropriate-length SCCB. 

A user should take care when tuning the CPU model for a VM.
If a user defines a VM with els support and specifies 248 CPUs, but
the guest Linux kernel cannot react to the SCLP response code, then
the guest will crash immediately upon kernel startup.

Since it has been some time since the last review and a few things
have changed, I've removed all ack's and set this submission to v1.

Collin L. Walling (8):
  s390/sclp: remove SCLPDevice param from prepare_cpu_entries
  s390/sclp: check sccb len before filling in data
  s390/sclp: rework sclp boundary and length checks
  s390/sclp: read sccb from mem based on sccb length
  s390/sclp: use cpu offset to locate cpu entries
  s390/sclp: add extended-length sccb support for kvm guest
  s390/kvm: header sync for diag318
  s390: diagnose 318 info reset and migration support

 hw/s390x/s390-virtio-ccw.c          |  45 +++++++++++++
 hw/s390x/sclp.c                     | 101 +++++++++++++++++++++-------
 include/hw/s390x/s390-virtio-ccw.h  |   1 +
 include/hw/s390x/sclp.h             |   4 ++
 linux-headers/asm-s390/kvm.h        |   5 ++
 smp.max_cpus                        |   0
 target/s390x/cpu.c                  |  19 ++++++
 target/s390x/cpu.h                  |   4 ++
 target/s390x/cpu_features.h         |   1 +
 target/s390x/cpu_features_def.inc.h |   4 ++
 target/s390x/cpu_models.c           |   1 +
 target/s390x/gen-features.c         |   2 +
 target/s390x/kvm-stub.c             |  10 +++
 target/s390x/kvm.c                  |  52 ++++++++++++++
 target/s390x/kvm_s390x.h            |   3 +
 15 files changed, 229 insertions(+), 23 deletions(-)
 create mode 100644 smp.max_cpus

-- 
2.21.1

[PATCH v1 1/8] s390/sclp: remove SCLPDevice param from prepare_cpu_entries

[Collin Walling]

It was never used in this function, so let's remove it.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 hw/s390x/sclp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index ede056b3ef..156ffe3223 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -49,7 +49,7 @@ static inline bool sclp_command_code_valid(uint32_t code)
     return false;
 }
 
-static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int *count)
+static void prepare_cpu_entries(CPUEntry *entry, int *count)
 {
     MachineState *ms = MACHINE(qdev_get_machine());
     uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
@@ -77,7 +77,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     IplParameterBlock *ipib = s390_ipl_get_iplb();
 
     /* CPU information */
-    prepare_cpu_entries(sclp, read_info->entries, &cpu_count);
+    prepare_cpu_entries(read_info->entries, &cpu_count);
     read_info->entries_cpu = cpu_to_be16(cpu_count);
     read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
     read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
@@ -135,7 +135,7 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
     ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
     int cpu_count;
 
-    prepare_cpu_entries(sclp, cpu_info->entries, &cpu_count);
+    prepare_cpu_entries(cpu_info->entries, &cpu_count);
     cpu_info->nr_configured = cpu_to_be16(cpu_count);
     cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries));
     cpu_info->nr_standby = cpu_to_be16(0);
-- 
2.21.1

[PATCH v1 2/8] s390/sclp: check sccb len before filling in data

[Collin Walling]

The SCCB must be checked for a sufficient length before it is filled
with any data. If the length is insufficient, then the SCLP command
is suppressed and the proper response code is set in the SCCB header.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 hw/s390x/sclp.c | 22 ++++++++++------------
 smp.max_cpus    |  0
 2 files changed, 10 insertions(+), 12 deletions(-)
 create mode 100644 smp.max_cpus

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 156ffe3223..d08a291e40 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -76,6 +76,11 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     int rnsize, rnmax;
     IplParameterBlock *ipib = s390_ipl_get_iplb();
 
+    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+        return;
+    }
+
     /* CPU information */
     prepare_cpu_entries(read_info->entries, &cpu_count);
     read_info->entries_cpu = cpu_to_be16(cpu_count);
@@ -84,12 +89,6 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 
     read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
 
-    if (be16_to_cpu(sccb->h.length) <
-            (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {
-        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
-        return;
-    }
-
     /* Configuration Characteristic (Extension) */
     s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR,
                          read_info->conf_char);
@@ -135,17 +134,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
     ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
     int cpu_count;
 
+    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+        return;
+    }
+
     prepare_cpu_entries(cpu_info->entries, &cpu_count);
     cpu_info->nr_configured = cpu_to_be16(cpu_count);
     cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries));
     cpu_info->nr_standby = cpu_to_be16(0);
 
-    if (be16_to_cpu(sccb->h.length) <
-            (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
-        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
-        return;
-    }
-
     /* The standby offset is 16-byte for each CPU */
     cpu_info->offset_standby = cpu_to_be16(cpu_info->offset_configured
         + cpu_info->nr_configured*sizeof(CPUEntry));
diff --git a/smp.max_cpus b/smp.max_cpus
new file mode 100644
index 0000000000..e69de29bb2
-- 
2.21.1

[PATCH v1 3/8] s390/sclp: rework sclp boundary and length checks

[Collin Walling]

Let's factor out the SCLP boundary and length checks
into separate functions.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 hw/s390x/sclp.c | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index d08a291e40..470d5da7a2 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -49,6 +49,34 @@ static inline bool sclp_command_code_valid(uint32_t code)
     return false;
 }
 
+static bool check_sccb_boundary_valid(uint64_t sccb_addr, uint32_t code,
+                                      SCCB *sccb)
+{
+    uint64_t current_len = sccb_addr + be16_to_cpu(sccb->h.length);
+    uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
+
+    switch (code & SCLP_CMD_CODE_MASK) {
+    default:
+        if (current_len <= allowed_len) {
+            return true;
+        }
+    }
+    sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
+    return false;
+}
+
+static bool check_sufficient_sccb_len(SCCB *sccb, int size)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+    int required_len = size + ms->possible_cpus->len * sizeof(CPUEntry);
+
+    if (be16_to_cpu(sccb->h.length) < required_len) {
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+        return false;
+    }
+    return true;
+}
+
 static void prepare_cpu_entries(CPUEntry *entry, int *count)
 {
     MachineState *ms = MACHINE(qdev_get_machine());
@@ -76,8 +104,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     int rnsize, rnmax;
     IplParameterBlock *ipib = s390_ipl_get_iplb();
 
-    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {
-        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+    if (!check_sufficient_sccb_len(sccb, sizeof(ReadInfo))) {
         return;
     }
 
@@ -134,8 +161,7 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
     ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
     int cpu_count;
 
-    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
-        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+    if (!check_sufficient_sccb_len(sccb, sizeof(ReadCpuInfo))) {
         return;
     }
 
@@ -227,6 +253,10 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
         goto out_write;
     }
 
+    if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) {
+        goto out_write;
+    }
+
     sclp_c->execute(sclp, &work_sccb, code);
 out_write:
     s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
@@ -272,8 +302,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
         goto out_write;
     }
 
-    if ((sccb + be16_to_cpu(work_sccb.h.length)) > ((sccb & PAGE_MASK) + PAGE_SIZE)) {
-        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
+    if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) {
         goto out_write;
     }
 
-- 
2.21.1

[PATCH v1 4/8] s390/sclp: read sccb from mem based on sccb length

[Collin Walling]

The header of the SCCB contains the actual length of the
SCCB. Instead of using a static 4K size, let's allow
for a variable size determined by the value set in the
header. The proper checks are already in place to ensure
the SCCB length is sufficent to store a full response,
and that the length does not cross any explicitly-set
boundaries.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 hw/s390x/sclp.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 470d5da7a2..748d04a0e2 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -244,15 +244,16 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
     SCLPDevice *sclp = get_sclp_device();
     SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
     SCCB work_sccb;
-    hwaddr sccb_len = sizeof(SCCB);
 
-    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
+    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sizeof(SCCBHeader));
 
     if (!sclp_command_code_valid(code)) {
         work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
         goto out_write;
     }
 
+    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, work_sccb.h.length);
+
     if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) {
         goto out_write;
     }
@@ -271,8 +272,6 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
     SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
     SCCB work_sccb;
 
-    hwaddr sccb_len = sizeof(SCCB);
-
     /* first some basic checks on program checks */
     if (env->psw.mask & PSW_MASK_PSTATE) {
         return -PGM_PRIVILEGED;
@@ -290,13 +289,16 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
      * from playing dirty tricks by modifying the memory content after
      * the host has checked the values
      */
-    cpu_physical_memory_read(sccb, &work_sccb, sccb_len);
+    cpu_physical_memory_read(sccb, &work_sccb, sizeof(SCCBHeader));
 
     /* Valid sccb sizes */
     if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
         return -PGM_SPECIFICATION;
     }
 
+    /* the header contains the actual length of the sccb */
+    cpu_physical_memory_read(sccb, &work_sccb, work_sccb.h.length);
+
     if (!sclp_command_code_valid(code)) {
         work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
         goto out_write;
-- 
2.21.1

[PATCH v1 5/8] s390/sclp: use cpu offset to locate cpu entries

[Collin Walling]

The start of the CPU entry region in the Read SCP Info
response data is denoted by the offset_cpu field. As such,
QEMU needs to begin creating entries at this address.

This is in preparation for when Read SCP Info inevitably
introduces new bytes that push the start of its CPUEntry
field further away.

Read CPU Info is unlikely to ever change, so let's not
bother accounting for the offset there.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 hw/s390x/sclp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 748d04a0e2..c47bd3b5ab 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -103,15 +103,17 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     int cpu_count;
     int rnsize, rnmax;
     IplParameterBlock *ipib = s390_ipl_get_iplb();
+    int offset_cpu = offsetof(ReadInfo, entries);
+    CPUEntry *entries_start = (void *)sccb + offset_cpu;
 
     if (!check_sufficient_sccb_len(sccb, sizeof(ReadInfo))) {
         return;
     }
 
     /* CPU information */
-    prepare_cpu_entries(read_info->entries, &cpu_count);
+    prepare_cpu_entries(entries_start, &cpu_count);
     read_info->entries_cpu = cpu_to_be16(cpu_count);
-    read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
+    read_info->offset_cpu = cpu_to_be16(offset_cpu);
     read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
 
     read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
-- 
2.21.1

[PATCH v1 6/8] s390/sclp: add extended-length sccb support for kvm guest

[Collin Walling]

As more features and facilities are added to the Read SCP Info (RSCPI)
response, more space is required to store them. The space used to store
these new features intrudes on the space originally used to store CPU
entries. This means as more features and facilities are added to the
RSCPI response, less space can be used to store CPU entries.

With the Extended-Length SCCB (ELS) facility, a KVM guest can execute
the RSCPI command and determine if the SCCB is large enough to store a
complete reponse. If it is not large enough, then the required length
will be set in the SCCB header.

The caller of the SCLP command is responsible for creating a
large-enough SCCB to store a complete response. Proper checking should
be in place, and the caller should execute the command once-more with
the large-enough SCCB.

This facility also enables an extended SCCB for the Read CPU Info
(RCPUI) command.

When this facility is enabled, the boundary violation response cannot
be a result from the RSCPI, RSCPI Forced, or RCPUI commands.

In order to tolerate kernels that do not yet have full support for this
feature, a "fixed" offset to the start of the CPU Entries within the
Read SCP Info struct is set to allow for the original 248 max entries.

Additionally, this is introduced as a CPU feature to protect the guest
from migrating to a machine that does not support storing an extended
SCCB. This could otherwise hinder the VM from being able to read all
available CPU entries after migration.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 hw/s390x/sclp.c                     | 23 +++++++++++++++++++++--
 include/hw/s390x/sclp.h             |  1 +
 target/s390x/cpu_features_def.inc.h |  1 +
 target/s390x/gen-features.c         |  1 +
 target/s390x/kvm.c                  |  4 ++++
 5 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index c47bd3b5ab..b5c89760e7 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -56,6 +56,18 @@ static bool check_sccb_boundary_valid(uint64_t sccb_addr, uint32_t code,
     uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
 
     switch (code & SCLP_CMD_CODE_MASK) {
+    case SCLP_CMDW_READ_SCP_INFO:
+    case SCLP_CMDW_READ_SCP_INFO_FORCED:
+    case SCLP_CMDW_READ_CPU_INFO:
+        /*
+         * An extended-length SCCB is only allowed for RSCPI and RSCPU and is
+         * allowed to exceed the 4k boundary. The respective commands will
+         * set the length field to the required length if an insufficient
+         * SCCB length is provided.
+         */
+        if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
+            return true;
+        }
     default:
         if (current_len <= allowed_len) {
             return true;
@@ -72,6 +84,10 @@ static bool check_sufficient_sccb_len(SCCB *sccb, int size)
 
     if (be16_to_cpu(sccb->h.length) < required_len) {
         sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+        if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) &&
+            sccb->h.control_mask[2] & SCLP_VARIABLE_LENGTH_RESPONSE) {
+            sccb->h.length = required_len;
+        }
         return false;
     }
     return true;
@@ -103,10 +119,13 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     int cpu_count;
     int rnsize, rnmax;
     IplParameterBlock *ipib = s390_ipl_get_iplb();
-    int offset_cpu = offsetof(ReadInfo, entries);
+    int offset_cpu = s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) ?
+                     offsetof(ReadInfo, entries) :
+                     SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET;
     CPUEntry *entries_start = (void *)sccb + offset_cpu;
 
-    if (!check_sufficient_sccb_len(sccb, sizeof(ReadInfo))) {
+    if (!check_sufficient_sccb_len(sccb, offset_cpu)) {
+        warn_report("insufficient sccb size to store full read scp info response");
         return;
     }
 
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index 822eff4396..ef2d63eae9 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -110,6 +110,7 @@ typedef struct CPUEntry {
     uint8_t reserved1;
 } QEMU_PACKED CPUEntry;
 
+#define SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET     128
 typedef struct ReadInfo {
     SCCBHeader h;
     uint16_t rnmax;
diff --git a/target/s390x/cpu_features_def.inc.h b/target/s390x/cpu_features_def.inc.h
index 60db28351d..3548d65a69 100644
--- a/target/s390x/cpu_features_def.inc.h
+++ b/target/s390x/cpu_features_def.inc.h
@@ -97,6 +97,7 @@ DEF_FEAT(GUARDED_STORAGE, "gs", STFL, 133, "Guarded-storage facility")
 DEF_FEAT(VECTOR_PACKED_DECIMAL, "vxpd", STFL, 134, "Vector packed decimal facility")
 DEF_FEAT(VECTOR_ENH, "vxeh", STFL, 135, "Vector enhancements facility")
 DEF_FEAT(MULTIPLE_EPOCH, "mepoch", STFL, 139, "Multiple-epoch facility")
+DEF_FEAT(EXTENDED_LENGTH_SCCB, "els", STFL, 140, "Extended-length SCCB facility")
 DEF_FEAT(TEST_PENDING_EXT_INTERRUPTION, "tpei", STFL, 144, "Test-pending-external-interruption facility")
 DEF_FEAT(INSERT_REFERENCE_BITS_MULT, "irbm", STFL, 145, "Insert-reference-bits-multiple facility")
 DEF_FEAT(MSA_EXT_8, "msa8-base", STFL, 146, "Message-security-assist-extension-8 facility (excluding subfunctions)")
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 8ddeebc544..6857f657fb 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -522,6 +522,7 @@ static uint16_t full_GEN12_GA1[] = {
     S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL,
     S390_FEAT_AP_FACILITIES_TEST,
     S390_FEAT_AP,
+    S390_FEAT_EXTENDED_LENGTH_SCCB,
 };
 
 static uint16_t full_GEN12_GA2[] = {
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 69881a0da0..380fb81822 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2456,6 +2456,10 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
         KVM_S390_VM_CRYPTO_ENABLE_APIE)) {
         set_bit(S390_FEAT_AP, model->features);
     }
+
+    /* Extended-Length SCCB is handled entirely within QEMU */
+    set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
+
     /* strip of features that are not part of the maximum model */
     bitmap_and(model->features, model->features, model->def->full_feat,
                S390_FEAT_MAX);
-- 
2.21.1

[PATCH v1 7/8] s390/kvm: header sync for diag318

[Collin Walling]

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 linux-headers/asm-s390/kvm.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
index 0138ccb0d8..b661feafdc 100644
--- a/linux-headers/asm-s390/kvm.h
+++ b/linux-headers/asm-s390/kvm.h
@@ -74,6 +74,7 @@ struct kvm_s390_io_adapter_req {
 #define KVM_S390_VM_CRYPTO		2
 #define KVM_S390_VM_CPU_MODEL		3
 #define KVM_S390_VM_MIGRATION		4
+#define KVM_S390_VM_MISC		5
 
 /* kvm attributes for mem_ctrl */
 #define KVM_S390_VM_MEM_ENABLE_CMMA	0
@@ -171,6 +172,10 @@ struct kvm_s390_vm_cpu_subfunc {
 #define KVM_S390_VM_MIGRATION_START	1
 #define KVM_S390_VM_MIGRATION_STATUS	2
 
+/* kvm attributes for KVM_S390_VM_MISC */
+#define KVM_S390_VM_MISC_ENABLE_DIAG318		0
+#define KVM_S390_VM_MISC_DIAG318			1
+
 /* for KVM_GET_REGS and KVM_SET_REGS */
 struct kvm_regs {
 	/* general purpose regs for s390 */
-- 
2.21.1

[PATCH v1 8/8] s390: diagnose 318 info reset and migration support

[Collin Walling]

The DIAGNOSE 318 (diag318) instruction allows the guest to store
diagnostic data that is collected by the firmware in the case of
hardware/firmware service events. The instruction is invoked in the
Linux kernel and intercepted in KVM. QEMU needs to collect this data
for migration so that it is consistent on the destination machine.

Availability of this instruction is determined by byte 134, bit 0
of the SCLP Read Info block. This coincidentally expands into the
space used for CPU entries, which means VMs running with the diag318
capability will not be able to read information regarding all CPUs.

This feature depends on the Extended-Length SCCB (els) feature.

Diag318 is set to 0 during modified clear and load normal resets.

This feature is not supported in protected virtualization mode.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c          | 45 +++++++++++++++++++++++++++
 hw/s390x/sclp.c                     |  5 +++
 include/hw/s390x/s390-virtio-ccw.h  |  1 +
 include/hw/s390x/sclp.h             |  3 ++
 target/s390x/cpu.c                  | 19 ++++++++++++
 target/s390x/cpu.h                  |  4 +++
 target/s390x/cpu_features.h         |  1 +
 target/s390x/cpu_features_def.inc.h |  3 ++
 target/s390x/cpu_models.c           |  1 +
 target/s390x/gen-features.c         |  1 +
 target/s390x/kvm-stub.c             | 10 ++++++
 target/s390x/kvm.c                  | 48 +++++++++++++++++++++++++++++
 target/s390x/kvm_s390x.h            |  3 ++
 13 files changed, 144 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 45292fb5a8..dc4eb20ec8 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -242,6 +242,40 @@ static void s390_create_sclpconsole(const char *type, Chardev *chardev)
     qdev_init_nofail(dev);
 }
 
+static int diag318_post_load(void *opaque, int version_id)
+{
+    S390CcwMachineState *d = opaque;
+
+    s390_set_diag318_info(d->diag318_info);
+    return 0;
+}
+
+static int diag318_pre_save(void *opaque)
+{
+    S390CcwMachineState *d = opaque;
+
+    s390_get_diag318_info(&d->diag318_info);
+    return 0;
+}
+
+static bool diag318_needed(void *opaque)
+{
+    return s390_diag318_is_allowed();
+}
+
+const VMStateDescription vmstate_diag318 = {
+    .name = "vmstate_diag318",
+    .post_load = diag318_post_load,
+    .pre_save = diag318_pre_save,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = diag318_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(diag318_info, S390CcwMachineState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void ccw_init(MachineState *machine)
 {
     int ret;
@@ -299,6 +333,8 @@ static void ccw_init(MachineState *machine)
 
     /* init the TOD clock */
     s390_init_tod();
+
+    vmstate_register(NULL, 0, &vmstate_diag318, machine);
 }
 
 static void s390_cpu_plug(HotplugHandler *hotplug_dev,
@@ -404,6 +440,13 @@ static void s390_pv_prepare_reset(S390CcwMachineState *ms)
     s390_pv_perf_clear_reset();
 }
 
+static void s390_diag318_reset(void)
+{
+    if (s390_diag318_is_allowed()) {
+        s390_set_diag318_info(0);
+    }
+}
+
 static void s390_machine_reset(MachineState *machine)
 {
     S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
@@ -440,6 +483,7 @@ static void s390_machine_reset(MachineState *machine)
         subsystem_reset();
         s390_crypto_reset();
         s390_pv_prepare_reset(ms);
+        s390_diag318_reset();
         CPU_FOREACH(t) {
             run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
         }
@@ -452,6 +496,7 @@ static void s390_machine_reset(MachineState *machine)
          */
         subsystem_reset();
         s390_pv_prepare_reset(ms);
+        s390_diag318_reset();
         CPU_FOREACH(t) {
             if (t == cs) {
                 continue;
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index b5c89760e7..ae3de14e63 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -143,6 +143,11 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT,
                          read_info->conf_char_ext);
 
+    if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
+        s390_get_feat_block(S390_FEAT_TYPE_SCLP_BYTE_134,
+                            &read_info->byte_134);
+    }
+
     read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
                                         SCLP_HAS_IOA_RECONFIG);
 
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index cd1dccc6e3..1134da4d75 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -30,6 +30,7 @@ typedef struct S390CcwMachineState {
     bool dea_key_wrap;
     bool pv;
     uint8_t loadparm[8];
+    uint64_t diag318_info;
 } S390CcwMachineState;
 
 typedef struct S390CcwMachineClass {
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index ef2d63eae9..b42a6b6f5a 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -133,6 +133,9 @@ typedef struct ReadInfo {
     uint16_t highest_cpu;
     uint8_t  _reserved5[124 - 122];     /* 122-123 */
     uint32_t hmfai;
+    uint8_t  _reserved7[134 - 128];     /* 128-133 */
+    uint8_t  byte_134;
+    uint8_t  _reserved8[144 - 135];     /* 135-143 */
     struct CPUEntry entries[];
 } QEMU_PACKED ReadInfo;
 
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index f2ccf0a06a..47284a4a62 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -446,6 +446,25 @@ void s390_enable_css_support(S390CPU *cpu)
         kvm_s390_enable_css_support(cpu);
     }
 }
+
+void s390_get_diag318_info(uint64_t *info)
+{
+    if (kvm_enabled()) {
+        kvm_s390_get_diag318_info(info);
+    }
+}
+
+void s390_set_diag318_info(uint64_t info)
+{
+    if (kvm_enabled()) {
+        kvm_s390_set_diag318_info(info);
+    }
+}
+
+bool s390_diag318_is_allowed(void)
+{
+    return kvm_enabled() && kvm_s390_diag318_is_allowed();
+}
 #endif
 
 static gchar *s390_gdb_arch_name(CPUState *cs)
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 035427521c..db80c1f89f 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -769,6 +769,10 @@ void s390_cmma_reset(void);
 void s390_enable_css_support(S390CPU *cpu);
 int s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch_id,
                                 int vq, bool assign);
+void s390_get_diag318_info(uint64_t *info);
+void s390_set_diag318_info(uint64_t info);
+bool s390_diag318_is_allowed(void);
+
 #ifndef CONFIG_USER_ONLY
 unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu);
 #else
diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
index da695a8346..954544eed7 100644
--- a/target/s390x/cpu_features.h
+++ b/target/s390x/cpu_features.h
@@ -23,6 +23,7 @@ typedef enum {
     S390_FEAT_TYPE_STFL,
     S390_FEAT_TYPE_SCLP_CONF_CHAR,
     S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT,
+    S390_FEAT_TYPE_SCLP_BYTE_134,
     S390_FEAT_TYPE_SCLP_CPU,
     S390_FEAT_TYPE_MISC,
     S390_FEAT_TYPE_PLO,
diff --git a/target/s390x/cpu_features_def.inc.h b/target/s390x/cpu_features_def.inc.h
index 3548d65a69..01b877d180 100644
--- a/target/s390x/cpu_features_def.inc.h
+++ b/target/s390x/cpu_features_def.inc.h
@@ -122,6 +122,9 @@ DEF_FEAT(SIE_CMMA, "cmma", SCLP_CONF_CHAR_EXT, 1, "SIE: Collaborative-memory-man
 DEF_FEAT(SIE_PFMFI, "pfmfi", SCLP_CONF_CHAR_EXT, 9, "SIE: PFMF interpretation facility")
 DEF_FEAT(SIE_IBS, "ibs", SCLP_CONF_CHAR_EXT, 10, "SIE: Interlock-and-broadcast-suppression facility")
 
+/* Features exposed via SCLP SCCB Byte 134 (bit numbers relative to byte-134) */
+DEF_FEAT(DIAG318, "diag318", SCLP_BYTE_134, 0, "Control program name and version codes")
+
 /* Features exposed via SCLP CPU info. */
 DEF_FEAT(SIE_F2, "sief2", SCLP_CPU, 4, "SIE: interception format 2 (Virtual SIE)")
 DEF_FEAT(SIE_SKEY, "skey", SCLP_CPU, 5, "SIE: Storage-key facility")
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 7c32180269..c445aa08ee 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -827,6 +827,7 @@ static void check_consistency(const S390CPUModel *model)
         { S390_FEAT_PTFF_STOE, S390_FEAT_MULTIPLE_EPOCH },
         { S390_FEAT_PTFF_STOUE, S390_FEAT_MULTIPLE_EPOCH },
         { S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL, S390_FEAT_AP },
+        { S390_FEAT_DIAG318, S390_FEAT_EXTENDED_LENGTH_SCCB },
     };
     int i;
 
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 6857f657fb..df90f5a7e8 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -523,6 +523,7 @@ static uint16_t full_GEN12_GA1[] = {
     S390_FEAT_AP_FACILITIES_TEST,
     S390_FEAT_AP,
     S390_FEAT_EXTENDED_LENGTH_SCCB,
+    S390_FEAT_DIAG318,
 };
 
 static uint16_t full_GEN12_GA2[] = {
diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c
index aa185017a2..669dcbc80b 100644
--- a/target/s390x/kvm-stub.c
+++ b/target/s390x/kvm-stub.c
@@ -120,3 +120,13 @@ void kvm_s390_stop_interrupt(S390CPU *cpu)
 void kvm_s390_restart_interrupt(S390CPU *cpu)
 {
 }
+
+int kvm_s390_get_diag318_info(uint64_t *info)
+{
+    return 0;
+}
+
+int kvm_s390_set_diag318_info(uint64_t info)
+{
+    return 0;
+}
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 380fb81822..839564e83c 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -814,6 +814,44 @@ int kvm_s390_set_clock_ext(uint8_t tod_high, uint64_t tod_low)
     return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
 }
 
+int kvm_s390_get_diag318_info(uint64_t *info)
+{
+    struct kvm_device_attr attr = {
+        .group = KVM_S390_VM_MISC,
+        .attr = KVM_S390_VM_MISC_DIAG318,
+        .addr = (uint64_t)info,
+    };
+
+    return kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
+}
+
+int kvm_s390_set_diag318_info(uint64_t info)
+{
+    struct kvm_device_attr attr = {
+        .group = KVM_S390_VM_MISC,
+        .attr = KVM_S390_VM_MISC_DIAG318,
+        .addr = (uint64_t)&info,
+    };
+
+    return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
+}
+
+bool kvm_s390_diag318_is_allowed(void)
+{
+    return s390_has_feat(S390_FEAT_DIAG318) &&
+           s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB);
+}
+
+static int kvm_s390_enable_diag318(void)
+{
+    struct kvm_device_attr attr = {
+        .group = KVM_S390_VM_MISC,
+        .attr = KVM_S390_VM_MISC_ENABLE_DIAG318,
+    };
+
+    return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
+}
+
 /**
  * kvm_s390_mem_op:
  * @addr:      the logical start address in guest memory
@@ -2460,6 +2498,12 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
     /* Extended-Length SCCB is handled entirely within QEMU */
     set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
 
+    /* Allow diag318 iff KVM supported and not in PV mode */
+    if (!s390_is_pv() && kvm_vm_check_attr(kvm_state,
+        KVM_S390_VM_MISC, KVM_S390_VM_MISC_DIAG318)) {
+        set_bit(S390_FEAT_DIAG318, model->features);
+    }
+
     /* strip of features that are not part of the maximum model */
     bitmap_and(model->features, model->features, model->def->full_feat,
                S390_FEAT_MAX);
@@ -2528,6 +2572,10 @@ void kvm_s390_apply_cpu_model(const S390CPUModel *model, Error **errp)
     if (test_bit(S390_FEAT_AP, model->features)) {
         kvm_s390_configure_apie(true);
     }
+
+    if (kvm_s390_diag318_is_allowed()) {
+        kvm_s390_enable_diag318();
+    }
 }
 
 void kvm_s390_restart_interrupt(S390CPU *cpu)
diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
index 6ab17c81b7..d7666fbd55 100644
--- a/target/s390x/kvm_s390x.h
+++ b/target/s390x/kvm_s390x.h
@@ -32,6 +32,9 @@ int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_clock);
 int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_clock);
 int kvm_s390_set_clock(uint8_t tod_high, uint64_t tod_clock);
 int kvm_s390_set_clock_ext(uint8_t tod_high, uint64_t tod_clock);
+int kvm_s390_get_diag318_info(uint64_t *info);
+int kvm_s390_set_diag318_info(uint64_t info);
+bool kvm_s390_diag318_is_allowed(void);
 void kvm_s390_enable_css_support(S390CPU *cpu);
 int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
                                     int vq, bool assign);
-- 
2.21.1

Re: [PATCH v1 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200508230823.22956-1-walling@linux.ibm.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200508230823.22956-1-walling@linux.ibm.com
Subject: [PATCH v1 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
101f5c5 s390: diagnose 318 info reset and migration support
07dbec2 s390/kvm: header sync for diag318
9985457 s390/sclp: add extended-length sccb support for kvm guest
8d13a8a s390/sclp: use cpu offset to locate cpu entries
4d17516 s390/sclp: read sccb from mem based on sccb length
4a1b469 s390/sclp: rework sclp boundary and length checks
699f978 s390/sclp: check sccb len before filling in data
e4476cc s390/sclp: remove SCLPDevice param from prepare_cpu_entries

=== OUTPUT BEGIN ===
1/8 Checking commit e4476ccd2e2b (s390/sclp: remove SCLPDevice param from prepare_cpu_entries)
2/8 Checking commit 699f978f26e8 (s390/sclp: check sccb len before filling in data)
WARNING: line over 80 characters
#22: FILE: hw/s390x/sclp.c:79:
+    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {

ERROR: line over 90 characters
#47: FILE: hw/s390x/sclp.c:137:
+    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#67: 
new file mode 100644

total: 1 errors, 2 warnings, 45 lines checked

Patch 2/8 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/8 Checking commit 4a1b469bbbcb (s390/sclp: rework sclp boundary and length checks)
4/8 Checking commit 4d17516859c5 (s390/sclp: read sccb from mem based on sccb length)
5/8 Checking commit 8d13a8a15966 (s390/sclp: use cpu offset to locate cpu entries)
6/8 Checking commit 9985457a4f23 (s390/sclp: add extended-length sccb support for kvm guest)
WARNING: line over 80 characters
#87: FILE: hw/s390x/sclp.c:128:
+        warn_report("insufficient sccb size to store full read scp info response");

WARNING: line over 80 characters
#111: FILE: target/s390x/cpu_features_def.inc.h:100:
+DEF_FEAT(EXTENDED_LENGTH_SCCB, "els", STFL, 140, "Extended-length SCCB facility")

total: 0 errors, 2 warnings, 74 lines checked

Patch 6/8 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/8 Checking commit 07dbec216007 (s390/kvm: header sync for diag318)
8/8 Checking commit 101f5c56e483 (s390: diagnose 318 info reset and migration support)
WARNING: line over 80 characters
#219: FILE: target/s390x/cpu_features_def.inc.h:126:
+DEF_FEAT(DIAG318, "diag318", SCLP_BYTE_134, 0, "Control program name and version codes")

total: 0 errors, 1 warnings, 255 lines checked

Patch 8/8 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200508230823.22956-1-walling@linux.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v1 2/8] s390/sclp: check sccb len before filling in data

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 2938 bytes --]

On 5/9/20 1:08 AM, Collin Walling wrote:
> The SCCB must be checked for a sufficient length before it is filled
> with any data. If the length is insufficient, then the SCLP command
> is suppressed and the proper response code is set in the SCCB header.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>

Fixes tag?
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

> ---
>  hw/s390x/sclp.c | 22 ++++++++++------------
>  smp.max_cpus    |  0
>  2 files changed, 10 insertions(+), 12 deletions(-)
>  create mode 100644 smp.max_cpus
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 156ffe3223..d08a291e40 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -76,6 +76,11 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>      int rnsize, rnmax;
>      IplParameterBlock *ipib = s390_ipl_get_iplb();
>  
> +    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +        return;
> +    }
> +
>      /* CPU information */
>      prepare_cpu_entries(read_info->entries, &cpu_count);
>      read_info->entries_cpu = cpu_to_be16(cpu_count);
> @@ -84,12 +89,6 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>  
>      read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
>  
> -    if (be16_to_cpu(sccb->h.length) <
> -            (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {
> -        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> -        return;
> -    }
> -
>      /* Configuration Characteristic (Extension) */
>      s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR,
>                           read_info->conf_char);
> @@ -135,17 +134,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
>      ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
>      int cpu_count;
>  
> +    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +        return;
> +    }
> +
>      prepare_cpu_entries(cpu_info->entries, &cpu_count);
>      cpu_info->nr_configured = cpu_to_be16(cpu_count);
>      cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries));
>      cpu_info->nr_standby = cpu_to_be16(0);
>  
> -    if (be16_to_cpu(sccb->h.length) <
> -            (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
> -        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> -        return;
> -    }
> -
>      /* The standby offset is 16-byte for each CPU */
>      cpu_info->offset_standby = cpu_to_be16(cpu_info->offset_configured
>          + cpu_info->nr_configured*sizeof(CPUEntry));
> diff --git a/smp.max_cpus b/smp.max_cpus
> new file mode 100644
> index 0000000000..e69de29bb2
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 1/8] s390/sclp: remove SCLPDevice param from prepare_cpu_entries

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1810 bytes --]

On 5/9/20 1:08 AM, Collin Walling wrote:
> It was never used in this function, so let's remove it.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>

Acked-by: Janosch Frank <frankja@de.ibm.com>

> ---
>  hw/s390x/sclp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index ede056b3ef..156ffe3223 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -49,7 +49,7 @@ static inline bool sclp_command_code_valid(uint32_t code)
>      return false;
>  }
>  
> -static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int *count)
> +static void prepare_cpu_entries(CPUEntry *entry, int *count)
>  {
>      MachineState *ms = MACHINE(qdev_get_machine());
>      uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
> @@ -77,7 +77,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>      IplParameterBlock *ipib = s390_ipl_get_iplb();
>  
>      /* CPU information */
> -    prepare_cpu_entries(sclp, read_info->entries, &cpu_count);
> +    prepare_cpu_entries(read_info->entries, &cpu_count);
>      read_info->entries_cpu = cpu_to_be16(cpu_count);
>      read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
>      read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
> @@ -135,7 +135,7 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
>      ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
>      int cpu_count;
>  
> -    prepare_cpu_entries(sclp, cpu_info->entries, &cpu_count);
> +    prepare_cpu_entries(cpu_info->entries, &cpu_count);
>      cpu_info->nr_configured = cpu_to_be16(cpu_count);
>      cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries));
>      cpu_info->nr_standby = cpu_to_be16(0);
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 2/8] s390/sclp: check sccb len before filling in data

[David Hildenbrand]

On 11.05.20 16:36, Janosch Frank wrote:
> On 5/9/20 1:08 AM, Collin Walling wrote:
>> The SCCB must be checked for a sufficient length before it is filled
>> with any data. If the length is insufficient, then the SCLP command
>> is suppressed and the proper response code is set in the SCCB header.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> 
> Fixes tag?
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

This is not a fix AFAIKs.
sclp_service_call()/sclp_service_call_protected() always supplies a full
SCCB of exactly 4k size.


-- 
Thanks,

David / dhildenb

Re: [PATCH v1 2/8] s390/sclp: check sccb len before filling in data

[Collin Walling]

On 5/11/20 10:44 AM, David Hildenbrand wrote:
> On 11.05.20 16:36, Janosch Frank wrote:
>> On 5/9/20 1:08 AM, Collin Walling wrote:
>>> The SCCB must be checked for a sufficient length before it is filled
>>> with any data. If the length is insufficient, then the SCLP command
>>> is suppressed and the proper response code is set in the SCCB header.
>>>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>
>> Fixes tag?
>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> 
> This is not a fix AFAIKs.
> sclp_service_call()/sclp_service_call_protected() always supplies a full
> SCCB of exactly 4k size.
> 
> 

Right. This is more-or-less a preparation patch for the extended-length
SCCB stuff which allows an SCCB > 4k.

The Linux kernel always provides a 4k SSCB today, so this patch
more-or-less makes the code AR compliant.

-- 
--
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v1 2/8] s390/sclp: check sccb len before filling in data

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1047 bytes --]

On 5/11/20 4:44 PM, David Hildenbrand wrote:
> On 11.05.20 16:36, Janosch Frank wrote:
>> On 5/9/20 1:08 AM, Collin Walling wrote:
>>> The SCCB must be checked for a sufficient length before it is filled
>>> with any data. If the length is insufficient, then the SCLP command
>>> is suppressed and the proper response code is set in the SCCB header.
>>>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>
>> Fixes tag?
>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> 
> This is not a fix AFAIKs.
> sclp_service_call()/sclp_service_call_protected() always supplies a full
> SCCB of exactly 4k size.
> 

We don't check for QEMU's 4k buffer here, but for the length that was
specified by the guest.

It's valid for the guest to request cpu info and state that its buffer
is only 1k. We can't write everything in 1k if we have ~200 cpus, so
we'll report the insufficient length rc.

What he fixes here is the time of the length check, it should be done
before any changes are being done to the work_sccb.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 2/8] s390/sclp: check sccb len before filling in data

[David Hildenbrand]

On 11.05.20 16:50, Janosch Frank wrote:
> On 5/11/20 4:44 PM, David Hildenbrand wrote:
>> On 11.05.20 16:36, Janosch Frank wrote:
>>> On 5/9/20 1:08 AM, Collin Walling wrote:
>>>> The SCCB must be checked for a sufficient length before it is filled
>>>> with any data. If the length is insufficient, then the SCLP command
>>>> is suppressed and the proper response code is set in the SCCB header.
>>>>
>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>>
>>> Fixes tag?
>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>>
>> This is not a fix AFAIKs.
>> sclp_service_call()/sclp_service_call_protected() always supplies a full
>> SCCB of exactly 4k size.
>>
> 
> We don't check for QEMU's 4k buffer here, but for the length that was
> specified by the guest.
> 
> It's valid for the guest to request cpu info and state that its buffer
> is only 1k. We can't write everything in 1k if we have ~200 cpus, so
> we'll report the insufficient length rc.
> 
> What he fixes here is the time of the length check, it should be done
> before any changes are being done to the work_sccb.

I don't have access to the spec, especially, if the guest can expect
nothing else in the sccb to change in case we report an error code. So
whatever you tell me, I have to trust you :)


-- 
Thanks,

David / dhildenb

Re: [PATCH v1 1/8] s390/sclp: remove SCLPDevice param from prepare_cpu_entries

[David Hildenbrand]

On 09.05.20 01:08, Collin Walling wrote:
> It was never used in this function, so let's remove it.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  hw/s390x/sclp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index ede056b3ef..156ffe3223 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -49,7 +49,7 @@ static inline bool sclp_command_code_valid(uint32_t code)
>      return false;
>  }
>  
> -static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int *count)
> +static void prepare_cpu_entries(CPUEntry *entry, int *count)
>  {
>      MachineState *ms = MACHINE(qdev_get_machine());
>      uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
> @@ -77,7 +77,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>      IplParameterBlock *ipib = s390_ipl_get_iplb();
>  
>      /* CPU information */
> -    prepare_cpu_entries(sclp, read_info->entries, &cpu_count);
> +    prepare_cpu_entries(read_info->entries, &cpu_count);
>      read_info->entries_cpu = cpu_to_be16(cpu_count);
>      read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
>      read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
> @@ -135,7 +135,7 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
>      ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
>      int cpu_count;
>  
> -    prepare_cpu_entries(sclp, cpu_info->entries, &cpu_count);
> +    prepare_cpu_entries(cpu_info->entries, &cpu_count);
>      cpu_info->nr_configured = cpu_to_be16(cpu_count);
>      cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries));
>      cpu_info->nr_standby = cpu_to_be16(0);
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 3/8] s390/sclp: rework sclp boundary and length checks

[David Hildenbrand]

On 09.05.20 01:08, Collin Walling wrote:
> Let's factor out the SCLP boundary and length checks
> into separate functions.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  hw/s390x/sclp.c | 41 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index d08a291e40..470d5da7a2 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -49,6 +49,34 @@ static inline bool sclp_command_code_valid(uint32_t code)
>      return false;
>  }
>  
> +static bool check_sccb_boundary_valid(uint64_t sccb_addr, uint32_t code,
> +                                      SCCB *sccb)

I suggest naming this

"has_valid_sccb_boundary", then the true/false response is clearer.

> +{
> +    uint64_t current_len = sccb_addr + be16_to_cpu(sccb->h.length);
> +    uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
> +
> +    switch (code & SCLP_CMD_CODE_MASK) {
> +    default:
> +        if (current_len <= allowed_len) {
> +            return true;
> +        }
> +    }
> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> +    return false;
> +}
> +
> +static bool check_sufficient_sccb_len(SCCB *sccb, int size)

"has_sufficient_sccb_len" ?

> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    int required_len = size + ms->possible_cpus->len * sizeof(CPUEntry);

Rather pass in the number of cpus instead. Looking up the machine again
in here is ugly.

> +
> +    if (be16_to_cpu(sccb->h.length) < required_len) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +        return false;
> +    }
> +    return true;
> +}
> +
>  static void prepare_cpu_entries(CPUEntry *entry, int *count)
>  {
>      MachineState *ms = MACHINE(qdev_get_machine());
> @@ -76,8 +104,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>      int rnsize, rnmax;
>      IplParameterBlock *ipib = s390_ipl_get_iplb();
>  
> -    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {
> -        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +    if (!check_sufficient_sccb_len(sccb, sizeof(ReadInfo))) {
>          return;
>      }
>  
> @@ -134,8 +161,7 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
>      ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
>      int cpu_count;
>  
> -    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
> -        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +    if (!check_sufficient_sccb_len(sccb, sizeof(ReadCpuInfo))) {
>          return;
>      }
>  
> @@ -227,6 +253,10 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>          goto out_write;
>      }
>  
> +    if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) {
> +        goto out_write;
> +    }

This is not a "factor out". You're adding new code, this needs
justification in the patch description.

> +
>      sclp_c->execute(sclp, &work_sccb, code);
>  out_write:
>      s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
> @@ -272,8 +302,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>          goto out_write;
>      }
>  
> -    if ((sccb + be16_to_cpu(work_sccb.h.length)) > ((sccb & PAGE_MASK) + PAGE_SIZE)) {
> -        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> +    if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) {
>          goto out_write;
>      }
>  
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH v1 4/8] s390/sclp: read sccb from mem based on sccb length

[David Hildenbrand]

On 09.05.20 01:08, Collin Walling wrote:
> The header of the SCCB contains the actual length of the
> SCCB. Instead of using a static 4K size, let's allow
> for a variable size determined by the value set in the
> header. The proper checks are already in place to ensure
> the SCCB length is sufficent to store a full response,
> and that the length does not cross any explicitly-set
> boundaries.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  hw/s390x/sclp.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 470d5da7a2..748d04a0e2 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -244,15 +244,16 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>      SCLPDevice *sclp = get_sclp_device();
>      SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
>      SCCB work_sccb;
> -    hwaddr sccb_len = sizeof(SCCB);
>  
> -    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sizeof(SCCBHeader));
>  
>      if (!sclp_command_code_valid(code)) {
>          work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>          goto out_write;
>      }
>  
> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, work_sccb.h.length);

be16_to_cpu() necessary.

> +
>      if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) {
>          goto out_write;
>      }
> @@ -271,8 +272,6 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>      SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
>      SCCB work_sccb;
>  
> -    hwaddr sccb_len = sizeof(SCCB);
> -
>      /* first some basic checks on program checks */
>      if (env->psw.mask & PSW_MASK_PSTATE) {
>          return -PGM_PRIVILEGED;
> @@ -290,13 +289,16 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>       * from playing dirty tricks by modifying the memory content after
>       * the host has checked the values
>       */
> -    cpu_physical_memory_read(sccb, &work_sccb, sccb_len);
> +    cpu_physical_memory_read(sccb, &work_sccb, sizeof(SCCBHeader));
>  
>      /* Valid sccb sizes */
>      if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
>          return -PGM_SPECIFICATION;
>      }
>  
> +    /* the header contains the actual length of the sccb */
> +    cpu_physical_memory_read(sccb, &work_sccb, work_sccb.h.length);

be16_to_cpu() necessary.

> +
>      if (!sclp_command_code_valid(code)) {
>          work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>          goto out_write;
> 

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 4/8] s390/sclp: read sccb from mem based on sccb length

[Collin Walling]

On 5/12/20 3:26 AM, David Hildenbrand wrote:
> On 09.05.20 01:08, Collin Walling wrote:
>> The header of the SCCB contains the actual length of the
>> SCCB. Instead of using a static 4K size, let's allow
>> for a variable size determined by the value set in the
>> header. The proper checks are already in place to ensure
>> the SCCB length is sufficent to store a full response,
>> and that the length does not cross any explicitly-set
>> boundaries.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>   hw/s390x/sclp.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 470d5da7a2..748d04a0e2 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -244,15 +244,16 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>>       SCLPDevice *sclp = get_sclp_device();
>>       SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
>>       SCCB work_sccb;
>> -    hwaddr sccb_len = sizeof(SCCB);
>>   
>> -    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
>> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sizeof(SCCBHeader));
>>   
>>       if (!sclp_command_code_valid(code)) {
>>           work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>>           goto out_write;
>>       }
>>   
>> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, work_sccb.h.length);
> 
> be16_to_cpu() necessary.
> 
>> +
>>       if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) {
>>           goto out_write;
>>       }
>> @@ -271,8 +272,6 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>>       SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
>>       SCCB work_sccb;
>>   
>> -    hwaddr sccb_len = sizeof(SCCB);
>> -
>>       /* first some basic checks on program checks */
>>       if (env->psw.mask & PSW_MASK_PSTATE) {
>>           return -PGM_PRIVILEGED;
>> @@ -290,13 +289,16 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>>        * from playing dirty tricks by modifying the memory content after
>>        * the host has checked the values
>>        */
>> -    cpu_physical_memory_read(sccb, &work_sccb, sccb_len);
>> +    cpu_physical_memory_read(sccb, &work_sccb, sizeof(SCCBHeader));
>>   
>>       /* Valid sccb sizes */
>>       if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
>>           return -PGM_SPECIFICATION;
>>       }
>>   
>> +    /* the header contains the actual length of the sccb */
>> +    cpu_physical_memory_read(sccb, &work_sccb, work_sccb.h.length);
> 
> be16_to_cpu() necessary.
> 
>> +
>>       if (!sclp_command_code_valid(code)) {
>>           work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>>           goto out_write;
>>
> 

Thanks. Fixed up.

-- 
--
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v1 3/8] s390/sclp: rework sclp boundary and length checks

[Collin Walling]

On 5/12/20 3:21 AM, David Hildenbrand wrote:
> On 09.05.20 01:08, Collin Walling wrote:
>> Let's factor out the SCLP boundary and length checks
>> into separate functions.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>   hw/s390x/sclp.c | 41 +++++++++++++++++++++++++++++++++++------
>>   1 file changed, 35 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index d08a291e40..470d5da7a2 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -49,6 +49,34 @@ static inline bool sclp_command_code_valid(uint32_t code)
>>       return false;
>>   }
>>   
>> +static bool check_sccb_boundary_valid(uint64_t sccb_addr, uint32_t code,
>> +                                      SCCB *sccb)
> 
> I suggest naming this
> 
> "has_valid_sccb_boundary", then the true/false response is clearer.
> 
>> +{
>> +    uint64_t current_len = sccb_addr + be16_to_cpu(sccb->h.length);
>> +    uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
>> +
>> +    switch (code & SCLP_CMD_CODE_MASK) {
>> +    default:
>> +        if (current_len <= allowed_len) {
>> +            return true;
>> +        }
>> +    }
>> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>> +    return false;
>> +}
>> +
>> +static bool check_sufficient_sccb_len(SCCB *sccb, int size)
> 
> "has_sufficient_sccb_len" ?
> 
>> +{
>> +    MachineState *ms = MACHINE(qdev_get_machine());
>> +    int required_len = size + ms->possible_cpus->len * sizeof(CPUEntry);
> 
> Rather pass in the number of cpus instead. Looking up the machine again
> in here is ugly.

prepare_cpu_entries also looks up the machine again. Should I squeeze
in a cleanup where we pass the machine to that function too (perhaps
in the "remove SCLPDevice" patch)?

> 
>> +
>> +    if (be16_to_cpu(sccb->h.length) < required_len) {
>> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
>> +        return false;
>> +    }
>> +    return true;
>> +}
>> +
>>   static void prepare_cpu_entries(CPUEntry *entry, int *count)
>>   {
>>       MachineState *ms = MACHINE(qdev_get_machine());
>> @@ -76,8 +104,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>       int rnsize, rnmax;
>>       IplParameterBlock *ipib = s390_ipl_get_iplb();
>>   
>> -    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {
>> -        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
>> +    if (!check_sufficient_sccb_len(sccb, sizeof(ReadInfo))) {
>>           return;
>>       }
>>   
>> @@ -134,8 +161,7 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
>>       ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
>>       int cpu_count;
>>   
>> -    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
>> -        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
>> +    if (!check_sufficient_sccb_len(sccb, sizeof(ReadCpuInfo))) {
>>           return;
>>       }
>>   
>> @@ -227,6 +253,10 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>>           goto out_write;
>>       }
>>   
>> +    if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) {
>> +        goto out_write;
>> +    }
> 
> This is not a "factor out". You're adding new code, this needs
> justification in the patch description.

True. I'll fix up the message.

> 
>> +
>>       sclp_c->execute(sclp, &work_sccb, code);
>>   out_write:
>>       s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
>> @@ -272,8 +302,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>>           goto out_write;
>>       }
>>   
>> -    if ((sccb + be16_to_cpu(work_sccb.h.length)) > ((sccb & PAGE_MASK) + PAGE_SIZE)) {
>> -        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>> +    if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) {
>>           goto out_write;
>>       }
>>   
>>
> 
> 

Renamed functions. Thanks for the review!

-- 
--
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v1 2/8] s390/sclp: check sccb len before filling in data

[Cornelia Huck]

On Mon, 11 May 2020 17:02:06 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 11.05.20 16:50, Janosch Frank wrote:
> > On 5/11/20 4:44 PM, David Hildenbrand wrote:  
> >> On 11.05.20 16:36, Janosch Frank wrote:  
> >>> On 5/9/20 1:08 AM, Collin Walling wrote:  
> >>>> The SCCB must be checked for a sufficient length before it is filled
> >>>> with any data. If the length is insufficient, then the SCLP command
> >>>> is suppressed and the proper response code is set in the SCCB header.
> >>>>
> >>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>  
> >>>
> >>> Fixes tag?

Probably

Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length")

?

> >>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>  
> >>
> >> This is not a fix AFAIKs.
> >> sclp_service_call()/sclp_service_call_protected() always supplies a full
> >> SCCB of exactly 4k size.
> >>  
> > 
> > We don't check for QEMU's 4k buffer here, but for the length that was
> > specified by the guest.
> > 
> > It's valid for the guest to request cpu info and state that its buffer
> > is only 1k. We can't write everything in 1k if we have ~200 cpus, so
> > we'll report the insufficient length rc.
> > 
> > What he fixes here is the time of the length check, it should be done
> > before any changes are being done to the work_sccb.  
> 
> I don't have access to the spec, especially, if the guest can expect
> nothing else in the sccb to change in case we report an error code. So
> whatever you tell me, I have to trust you :)

Same here. Sounds plausible, but I have to trust the folks with the
documentation :)

Re: [PATCH v1 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[Cornelia Huck]

On Fri,  8 May 2020 19:08:15 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> Collin L. Walling (8):
>   s390/sclp: remove SCLPDevice param from prepare_cpu_entries

This looks like a simple cleanup...

>   s390/sclp: check sccb len before filling in data

...and that like a simple fix (for a problem that currently does not
trigger.) Would it help or hinder you if I went ahead and queued these
two patches already?

>   s390/sclp: rework sclp boundary and length checks
>   s390/sclp: read sccb from mem based on sccb length
>   s390/sclp: use cpu offset to locate cpu entries
>   s390/sclp: add extended-length sccb support for kvm guest
>   s390/kvm: header sync for diag318
>   s390: diagnose 318 info reset and migration support
> 
>  hw/s390x/s390-virtio-ccw.c          |  45 +++++++++++++
>  hw/s390x/sclp.c                     | 101 +++++++++++++++++++++-------
>  include/hw/s390x/s390-virtio-ccw.h  |   1 +
>  include/hw/s390x/sclp.h             |   4 ++
>  linux-headers/asm-s390/kvm.h        |   5 ++
>  smp.max_cpus                        |   0

Probably a stray file? (Should be stripped from patch 2.)

>  target/s390x/cpu.c                  |  19 ++++++
>  target/s390x/cpu.h                  |   4 ++
>  target/s390x/cpu_features.h         |   1 +
>  target/s390x/cpu_features_def.inc.h |   4 ++
>  target/s390x/cpu_models.c           |   1 +
>  target/s390x/gen-features.c         |   2 +
>  target/s390x/kvm-stub.c             |  10 +++
>  target/s390x/kvm.c                  |  52 ++++++++++++++
>  target/s390x/kvm_s390x.h            |   3 +
>  15 files changed, 229 insertions(+), 23 deletions(-)
>  create mode 100644 smp.max_cpus
> 

Re: [PATCH v1 2/8] s390/sclp: check sccb len before filling in data

[Collin Walling]

On 5/12/20 12:01 PM, Cornelia Huck wrote:
> On Mon, 11 May 2020 17:02:06 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 11.05.20 16:50, Janosch Frank wrote:
>>> On 5/11/20 4:44 PM, David Hildenbrand wrote:
>>>> On 11.05.20 16:36, Janosch Frank wrote:
>>>>> On 5/9/20 1:08 AM, Collin Walling wrote:
>>>>>> The SCCB must be checked for a sufficient length before it is filled
>>>>>> with any data. If the length is insufficient, then the SCLP command
>>>>>> is suppressed and the proper response code is set in the SCCB header.
>>>>>>
>>>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>>>>
>>>>> Fixes tag?
> 
> Probably
> 
> Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length")
> 
> ?
> 

Sounds reasonable. This patch doesn't fix any explicitly-known bugs 
AFAIK. The s390 Linux kernel is hard-coded to use a 4K size SCCB when
executing these commands.

I suppose this could introduce a bug if things change in the Linux 
kernel or if some other OS wants to use this command. That should be 
enough of a justification, right? (Just want to make sure I understand 
the use of the tag correctly).

>>>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>>>>
>>>> This is not a fix AFAIKs.
>>>> sclp_service_call()/sclp_service_call_protected() always supplies a full
>>>> SCCB of exactly 4k size.
>>>>   
>>>
>>> We don't check for QEMU's 4k buffer here, but for the length that was
>>> specified by the guest.
>>>
>>> It's valid for the guest to request cpu info and state that its buffer
>>> is only 1k. We can't write everything in 1k if we have ~200 cpus, so
>>> we'll report the insufficient length rc.
>>>
>>> What he fixes here is the time of the length check, it should be done
>>> before any changes are being done to the work_sccb.
>>
>> I don't have access to the spec, especially, if the guest can expect
>> nothing else in the sccb to change in case we report an error code. So
>> whatever you tell me, I have to trust you :)
> 
> Same here. Sounds plausible, but I have to trust the folks with the
> documentation :)
> 

Sorry I can't be of much help here :/

-- 
--
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v1 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[Collin Walling]

On 5/12/20 12:08 PM, Cornelia Huck wrote:
> On Fri,  8 May 2020 19:08:15 -0400
> Collin Walling <walling@linux.ibm.com> wrote:
> 
>> Collin L. Walling (8):
>>    s390/sclp: remove SCLPDevice param from prepare_cpu_entries
> 
> This looks like a simple cleanup...
> 
>>    s390/sclp: check sccb len before filling in data
> 
> ...and that like a simple fix (for a problem that currently does not
> trigger.) Would it help or hinder you if I went ahead and queued these
> two patches already?
> 

Let's wait for one more round if that's okay. I left a response to
David's feedback that may-or-may not add one more cleanup that can
be squeezed into patch 1 if it makes sense.

Thanks, though! :)

>>    s390/sclp: rework sclp boundary and length checks
>>    s390/sclp: read sccb from mem based on sccb length
>>    s390/sclp: use cpu offset to locate cpu entries
>>    s390/sclp: add extended-length sccb support for kvm guest
>>    s390/kvm: header sync for diag318
>>    s390: diagnose 318 info reset and migration support
>>
>>   hw/s390x/s390-virtio-ccw.c          |  45 +++++++++++++
>>   hw/s390x/sclp.c                     | 101 +++++++++++++++++++++-------
>>   include/hw/s390x/s390-virtio-ccw.h  |   1 +
>>   include/hw/s390x/sclp.h             |   4 ++
>>   linux-headers/asm-s390/kvm.h        |   5 ++
>>   smp.max_cpus                        |   0
> 
> Probably a stray file? (Should be stripped from patch 2.)

Uhhh not sure how that got there :) Probably silly editor doing silly 
things. I'll make sure next round has that stripped.

> 
>>   target/s390x/cpu.c                  |  19 ++++++
>>   target/s390x/cpu.h                  |   4 ++
>>   target/s390x/cpu_features.h         |   1 +
>>   target/s390x/cpu_features_def.inc.h |   4 ++
>>   target/s390x/cpu_models.c           |   1 +
>>   target/s390x/gen-features.c         |   2 +
>>   target/s390x/kvm-stub.c             |  10 +++
>>   target/s390x/kvm.c                  |  52 ++++++++++++++
>>   target/s390x/kvm_s390x.h            |   3 +
>>   15 files changed, 229 insertions(+), 23 deletions(-)
>>   create mode 100644 smp.max_cpus
>>
> 


-- 
--
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v1 2/8] s390/sclp: check sccb len before filling in data

[Cornelia Huck]

On Tue, 12 May 2020 12:16:45 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> On 5/12/20 12:01 PM, Cornelia Huck wrote:
> > On Mon, 11 May 2020 17:02:06 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 11.05.20 16:50, Janosch Frank wrote:  
> >>> On 5/11/20 4:44 PM, David Hildenbrand wrote:  
> >>>> On 11.05.20 16:36, Janosch Frank wrote:  
> >>>>> On 5/9/20 1:08 AM, Collin Walling wrote:  
> >>>>>> The SCCB must be checked for a sufficient length before it is filled
> >>>>>> with any data. If the length is insufficient, then the SCLP command
> >>>>>> is suppressed and the proper response code is set in the SCCB header.
> >>>>>>
> >>>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>  
> >>>>>
> >>>>> Fixes tag?  
> > 
> > Probably
> > 
> > Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length")
> > 
> > ?
> >   
> 
> Sounds reasonable. This patch doesn't fix any explicitly-known bugs 
> AFAIK. The s390 Linux kernel is hard-coded to use a 4K size SCCB when
> executing these commands.
> 
> I suppose this could introduce a bug if things change in the Linux 
> kernel or if some other OS wants to use this command. That should be 
> enough of a justification, right? (Just want to make sure I understand 
> the use of the tag correctly).

Yes; from the description of how this is supposed to work it fixes
architectural conformance, not a bug that is triggered by today's guest
systems.

[Usage of the Fixes: tag in QEMU is not quite as essential as in Linux,
as we don't do the numerous, big stable updates here; I don't think
this is stable material, but we can certainly record where this was
introduced.]

Re: [PATCH v1 2/8] s390/sclp: check sccb len before filling in data

[Collin Walling]

On 5/12/20 12:24 PM, Cornelia Huck wrote:
> On Tue, 12 May 2020 12:16:45 -0400
> Collin Walling <walling@linux.ibm.com> wrote:
> 
>> On 5/12/20 12:01 PM, Cornelia Huck wrote:
>>> On Mon, 11 May 2020 17:02:06 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>    
>>>> On 11.05.20 16:50, Janosch Frank wrote:
>>>>> On 5/11/20 4:44 PM, David Hildenbrand wrote:
>>>>>> On 11.05.20 16:36, Janosch Frank wrote:
>>>>>>> On 5/9/20 1:08 AM, Collin Walling wrote:
>>>>>>>> The SCCB must be checked for a sufficient length before it is filled
>>>>>>>> with any data. If the length is insufficient, then the SCLP command
>>>>>>>> is suppressed and the proper response code is set in the SCCB header.
>>>>>>>>
>>>>>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>>>>>>
>>>>>>> Fixes tag?
>>>
>>> Probably
>>>
>>> Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length")
>>>
>>> ?
>>>    
>>
>> Sounds reasonable. This patch doesn't fix any explicitly-known bugs
>> AFAIK. The s390 Linux kernel is hard-coded to use a 4K size SCCB when
>> executing these commands.
>>
>> I suppose this could introduce a bug if things change in the Linux
>> kernel or if some other OS wants to use this command. That should be
>> enough of a justification, right? (Just want to make sure I understand
>> the use of the tag correctly).
> 
> Yes; from the description of how this is supposed to work it fixes
> architectural conformance, not a bug that is triggered by today's guest
> systems.
> 
> [Usage of the Fixes: tag in QEMU is not quite as essential as in Linux,
> as we don't do the numerous, big stable updates here; I don't think
> this is stable material, but we can certainly record where this was
> introduced.]
> 
> 

Makes sense to me, thanks for the info and commit ID! I'll add it to the
message for next round.

-- 
--
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v1 3/8] s390/sclp: rework sclp boundary and length checks

[Cornelia Huck]

On Tue, 12 May 2020 10:55:56 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> On 5/12/20 3:21 AM, David Hildenbrand wrote:
> > On 09.05.20 01:08, Collin Walling wrote:  

> >> +static bool check_sufficient_sccb_len(SCCB *sccb, int size)  
> > 
> > "has_sufficient_sccb_len" ?
> >   
> >> +{
> >> +    MachineState *ms = MACHINE(qdev_get_machine());
> >> +    int required_len = size + ms->possible_cpus->len * sizeof(CPUEntry);  
> > 
> > Rather pass in the number of cpus instead. Looking up the machine again
> > in here is ugly.  
> 
> prepare_cpu_entries also looks up the machine again. Should I squeeze
> in a cleanup where we pass the machine to that function too (perhaps
> in the "remove SCLPDevice" patch)?

sclp_read_cpu_info() does not have the machine handy, so you'd need to
move machine lookup there; but I think it's worth getting rid of
duplicate lookups.

> 
> >   
> >> +
> >> +    if (be16_to_cpu(sccb->h.length) < required_len) {
> >> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> >> +        return false;
> >> +    }
> >> +    return true;
> >> +}
> >> +

Re: [PATCH v1 7/8] s390/kvm: header sync for diag318

[Cornelia Huck]

On Fri,  8 May 2020 19:08:22 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  linux-headers/asm-s390/kvm.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
> index 0138ccb0d8..b661feafdc 100644
> --- a/linux-headers/asm-s390/kvm.h
> +++ b/linux-headers/asm-s390/kvm.h
> @@ -74,6 +74,7 @@ struct kvm_s390_io_adapter_req {
>  #define KVM_S390_VM_CRYPTO		2
>  #define KVM_S390_VM_CPU_MODEL		3
>  #define KVM_S390_VM_MIGRATION		4
> +#define KVM_S390_VM_MISC		5
>  
>  /* kvm attributes for mem_ctrl */
>  #define KVM_S390_VM_MEM_ENABLE_CMMA	0
> @@ -171,6 +172,10 @@ struct kvm_s390_vm_cpu_subfunc {
>  #define KVM_S390_VM_MIGRATION_START	1
>  #define KVM_S390_VM_MIGRATION_STATUS	2
>  
> +/* kvm attributes for KVM_S390_VM_MISC */
> +#define KVM_S390_VM_MISC_ENABLE_DIAG318		0
> +#define KVM_S390_VM_MISC_DIAG318			1
> +
>  /* for KVM_GET_REGS and KVM_SET_REGS */
>  struct kvm_regs {
>  	/* general purpose regs for s390 */

Hm... remind me what the state of the kernel part is?

Re: [PATCH v1 2/8] s390/sclp: check sccb len before filling in data

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 2016 bytes --]

On 5/12/20 6:01 PM, Cornelia Huck wrote:
> On Mon, 11 May 2020 17:02:06 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 11.05.20 16:50, Janosch Frank wrote:
>>> On 5/11/20 4:44 PM, David Hildenbrand wrote:  
>>>> On 11.05.20 16:36, Janosch Frank wrote:  
>>>>> On 5/9/20 1:08 AM, Collin Walling wrote:  
>>>>>> The SCCB must be checked for a sufficient length before it is filled
>>>>>> with any data. If the length is insufficient, then the SCLP command
>>>>>> is suppressed and the proper response code is set in the SCCB header.
>>>>>>
>>>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>  
>>>>>
>>>>> Fixes tag?
> 
> Probably
> 
> Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length")
> 
> ?
> 
>>>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>  
>>>>
>>>> This is not a fix AFAIKs.
>>>> sclp_service_call()/sclp_service_call_protected() always supplies a full
>>>> SCCB of exactly 4k size.
>>>>  
>>>
>>> We don't check for QEMU's 4k buffer here, but for the length that was
>>> specified by the guest.
>>>
>>> It's valid for the guest to request cpu info and state that its buffer
>>> is only 1k. We can't write everything in 1k if we have ~200 cpus, so
>>> we'll report the insufficient length rc.
>>>
>>> What he fixes here is the time of the length check, it should be done
>>> before any changes are being done to the work_sccb.  
>>
>> I don't have access to the spec, especially, if the guest can expect
>> nothing else in the sccb to change in case we report an error code. So
>> whatever you tell me, I have to trust you :)
> 
> Same here. Sounds plausible, but I have to trust the folks with the
> documentation :)
> 

The AR states that:
* Command validity check (has prio over length, as length is dependent
on command)
* boundary (if extended-length is not available)
* Sufficient length check

are done before "any other command action is taken".
If a test fails the command is suppressed.





[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 2/8] s390/sclp: check sccb len before filling in data

[Cornelia Huck]

[-- Attachment #1: Type: text/plain, Size: 2252 bytes --]

On Wed, 13 May 2020 09:43:37 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 5/12/20 6:01 PM, Cornelia Huck wrote:
> > On Mon, 11 May 2020 17:02:06 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 11.05.20 16:50, Janosch Frank wrote:  
> >>> On 5/11/20 4:44 PM, David Hildenbrand wrote:    
> >>>> On 11.05.20 16:36, Janosch Frank wrote:    
> >>>>> On 5/9/20 1:08 AM, Collin Walling wrote:    
> >>>>>> The SCCB must be checked for a sufficient length before it is filled
> >>>>>> with any data. If the length is insufficient, then the SCLP command
> >>>>>> is suppressed and the proper response code is set in the SCCB header.
> >>>>>>
> >>>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>    
> >>>>>
> >>>>> Fixes tag?  
> > 
> > Probably
> > 
> > Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length")
> > 
> > ?
> >   
> >>>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>    
> >>>>
> >>>> This is not a fix AFAIKs.
> >>>> sclp_service_call()/sclp_service_call_protected() always supplies a full
> >>>> SCCB of exactly 4k size.
> >>>>    
> >>>
> >>> We don't check for QEMU's 4k buffer here, but for the length that was
> >>> specified by the guest.
> >>>
> >>> It's valid for the guest to request cpu info and state that its buffer
> >>> is only 1k. We can't write everything in 1k if we have ~200 cpus, so
> >>> we'll report the insufficient length rc.
> >>>
> >>> What he fixes here is the time of the length check, it should be done
> >>> before any changes are being done to the work_sccb.    
> >>
> >> I don't have access to the spec, especially, if the guest can expect
> >> nothing else in the sccb to change in case we report an error code. So
> >> whatever you tell me, I have to trust you :)  
> > 
> > Same here. Sounds plausible, but I have to trust the folks with the
> > documentation :)
> >   
> 
> The AR states that:
> * Command validity check (has prio over length, as length is dependent
> on command)
> * boundary (if extended-length is not available)
> * Sufficient length check
> 
> are done before "any other command action is taken".
> If a test fails the command is suppressed.

Thanks, makes sense.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 7/8] s390/kvm: header sync for diag318

[Collin Walling]

On 5/13/20 3:05 AM, Cornelia Huck wrote:
> On Fri,  8 May 2020 19:08:22 -0400
> Collin Walling <walling@linux.ibm.com> wrote:
> 
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>  linux-headers/asm-s390/kvm.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
>> index 0138ccb0d8..b661feafdc 100644
>> --- a/linux-headers/asm-s390/kvm.h
>> +++ b/linux-headers/asm-s390/kvm.h
>> @@ -74,6 +74,7 @@ struct kvm_s390_io_adapter_req {
>>  #define KVM_S390_VM_CRYPTO		2
>>  #define KVM_S390_VM_CPU_MODEL		3
>>  #define KVM_S390_VM_MIGRATION		4
>> +#define KVM_S390_VM_MISC		5
>>  
>>  /* kvm attributes for mem_ctrl */
>>  #define KVM_S390_VM_MEM_ENABLE_CMMA	0
>> @@ -171,6 +172,10 @@ struct kvm_s390_vm_cpu_subfunc {
>>  #define KVM_S390_VM_MIGRATION_START	1
>>  #define KVM_S390_VM_MIGRATION_STATUS	2
>>  
>> +/* kvm attributes for KVM_S390_VM_MISC */
>> +#define KVM_S390_VM_MISC_ENABLE_DIAG318		0
>> +#define KVM_S390_VM_MISC_DIAG318			1
>> +
>>  /* for KVM_GET_REGS and KVM_SET_REGS */
>>  struct kvm_regs {
>>  	/* general purpose regs for s390 */
> 
> Hm... remind me what the state of the kernel part is?
> 

Kernel code to execute the instruction is in place today. A kernel
running on real hardware (or at least LPAR) can successfully execute the
instruction if the hardware supports it.

The Linux commit is: 4ad78b8651aacf26b3ab6d1e784952eb70469c43

The KVM code is still under review, since most of it needs to align with
the QEMU changes as well. Guest kernels executing the instruction rely
on both QEMU and KVM support. KVM handles the execution and interception
of the instruction, while QEMU handles enabling the instruction (by
negotiating with KVM to allow it and then setting the appropriate SCLP
bit) and migration of the data.

The latest changes to the KVM code is a new IOCTL that allows QEMU to
negotiate with KVM to actually enable the diag318 feature. By default,
KVM treats the feature as "disabled" unless userspace explicitly asks
for it.

The KVM discussion is: https://www.spinics.net/lists/kvm/msg215758.html

-- 
--
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v1 2/8] s390/sclp: check sccb len before filling in data

[Collin Walling]

On 5/13/20 4:16 AM, Cornelia Huck wrote:
> On Wed, 13 May 2020 09:43:37 +0200
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On 5/12/20 6:01 PM, Cornelia Huck wrote:
>>> On Mon, 11 May 2020 17:02:06 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> On 11.05.20 16:50, Janosch Frank wrote:  
>>>>> On 5/11/20 4:44 PM, David Hildenbrand wrote:    
>>>>>> On 11.05.20 16:36, Janosch Frank wrote:    
>>>>>>> On 5/9/20 1:08 AM, Collin Walling wrote:    
>>>>>>>> The SCCB must be checked for a sufficient length before it is filled
>>>>>>>> with any data. If the length is insufficient, then the SCLP command
>>>>>>>> is suppressed and the proper response code is set in the SCCB header.
>>>>>>>>
>>>>>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>    
>>>>>>>
>>>>>>> Fixes tag?  
>>>
>>> Probably
>>>
>>> Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length")
>>>
>>> ?
>>>   
>>>>>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>    
>>>>>>
>>>>>> This is not a fix AFAIKs.
>>>>>> sclp_service_call()/sclp_service_call_protected() always supplies a full
>>>>>> SCCB of exactly 4k size.
>>>>>>    
>>>>>
>>>>> We don't check for QEMU's 4k buffer here, but for the length that was
>>>>> specified by the guest.
>>>>>
>>>>> It's valid for the guest to request cpu info and state that its buffer
>>>>> is only 1k. We can't write everything in 1k if we have ~200 cpus, so
>>>>> we'll report the insufficient length rc.
>>>>>
>>>>> What he fixes here is the time of the length check, it should be done
>>>>> before any changes are being done to the work_sccb.    
>>>>
>>>> I don't have access to the spec, especially, if the guest can expect
>>>> nothing else in the sccb to change in case we report an error code. So
>>>> whatever you tell me, I have to trust you :)  
>>>
>>> Same here. Sounds plausible, but I have to trust the folks with the
>>> documentation :)
>>>   
>>
>> The AR states that:
>> * Command validity check (has prio over length, as length is dependent
>> on command)
>> * boundary (if extended-length is not available)
>> * Sufficient length check
>>
>> are done before "any other command action is taken".
>> If a test fails the command is suppressed.
> 
> Thanks, makes sense.
> 

Thanks, Janosch! (I suppose I could've said the same as well. Sorry
about that).

-- 
--
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v1 3/8] s390/sclp: rework sclp boundary and length checks

[Collin Walling]

On 5/13/20 3:00 AM, Cornelia Huck wrote:
> On Tue, 12 May 2020 10:55:56 -0400
> Collin Walling <walling@linux.ibm.com> wrote:
> 
>> On 5/12/20 3:21 AM, David Hildenbrand wrote:
>>> On 09.05.20 01:08, Collin Walling wrote:  
> 
>>>> +static bool check_sufficient_sccb_len(SCCB *sccb, int size)  
>>>
>>> "has_sufficient_sccb_len" ?
>>>   
>>>> +{
>>>> +    MachineState *ms = MACHINE(qdev_get_machine());
>>>> +    int required_len = size + ms->possible_cpus->len * sizeof(CPUEntry);  
>>>
>>> Rather pass in the number of cpus instead. Looking up the machine again
>>> in here is ugly.  
>>
>> prepare_cpu_entries also looks up the machine again. Should I squeeze
>> in a cleanup where we pass the machine to that function too (perhaps
>> in the "remove SCLPDevice" patch)?
> 
> sclp_read_cpu_info() does not have the machine handy, so you'd need to
> move machine lookup there; but I think it's worth getting rid of
> duplicate lookups.
> 

Sounds good, then. I'll propose a change to patch 1 that removes the
unused SCLPDevice param and accepts a MachineState instead. Should make
things a bit cleaner :)

>>
>>>   
>>>> +
>>>> +    if (be16_to_cpu(sccb->h.length) < required_len) {
>>>> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
>>>> +        return false;
>>>> +    }
>>>> +    return true;
>>>> +}
>>>> +
> 
> 


-- 
--
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v1 2/8] s390/sclp: check sccb len before filling in data

[David Hildenbrand]

On 09.05.20 01:08, Collin Walling wrote:
> The SCCB must be checked for a sufficient length before it is filled
> with any data. If the length is insufficient, then the SCLP command
> is suppressed and the proper response code is set in the SCCB header.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  hw/s390x/sclp.c | 22 ++++++++++------------
>  smp.max_cpus    |  0
>  2 files changed, 10 insertions(+), 12 deletions(-)
>  create mode 100644 smp.max_cpus
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 156ffe3223..d08a291e40 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -76,6 +76,11 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>      int rnsize, rnmax;
>      IplParameterBlock *ipib = s390_ipl_get_iplb();
>  
> +    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +        return;
> +    }
> +

Lines too long.

Please run scripts/checkpatch.pl before submitting.


-- 
Thanks,

David / dhildenb