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

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

[Collin Walling]

Janosch, Thomas, Conny: I've removed your r-b's from patch #3 since I
added some g_mallocs in place and I'd like to make sure things are
done properly there (explained in changelog, but let me know if further
explanation is necessary).

Janosch, please let me know if the changes to #3 are safe under PV.

Thanks.

Changelog:

    v5

    • removed sccb_verify_length function
        - will simply use the length check code that was in place before

    • introduced a macro for calculating required SCCB length
        - takes a struct and max # of cpus as args

    • work_sccb size is now dynamically allocated based on the length
      provided by the guest kernel, instead of always using a static
      4K size
        - as such, the SCCB will have to be read twice:
            - first time to retrieve the header
            - second time with proper size after space for work_sccb 
              is allocated

    v4
    
    • added r-b's and ack's (thanks, everyone!)

    • renamed boundary and length function

    • updated header sync to reflect a change discussed in the respective
        KVM patches

    • s/data_len/offset_cpu

    • added /* fallthrough */ comment in boundary check

    v3

    • Device IOCTLs removed
        - diag 318 info is now communicated via sync_regs

    • Reset code removed
        - this is now handled in KVM
        - diag318_info is stored within the CPU reset portion of the
            S390CPUState

    • Various cleanups for ELS preliminary patches

    v2

    • QEMU now handles the instruction call
        - as such, the "enable diag 318" IOCTL has been removed

    • patch #1 now changes the read scp/cpu info functions to
      retrieve the machine state once
        - as such, I have not added any ack's or r-bs since this
          patch differs from the previous version

    • patch #3 introduces a new "get_read_scp_info_data_len"
      function in order clean-up the variable data length assignment
      in patch #7
        - a comment above this function should help clarify what's
          going on to make things a bit easier to read

    • other misc clean ups and fixes
        - s/diag318/diag_318 in order to keep the naming scheme
          consistent with Linux and other diag-related code
        - s/byte_134/fac134 to align naming scheme with Linux

-----------------------------------------------------------------------

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 (diag_318) enabling / migration handling

The diag 318 feature depends on els and KVM support.

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

Both features are made available starting with the zEC12-full model.

These patches are introduced together for two main reasons:
    - els allows diag 318 to exist while retaining the original 248 
        VCPU max
    - diag 318 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.

Collin L. Walling (8):
  s390/sclp: get machine once during read scp/cpu info
  s390/sclp: rework sclp boundary checks
  s390/sclp: read sccb from mem based on provided length
  s390/sclp: check sccb len before filling in data
  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: guest support for diagnose 0x318

 hw/s390x/event-facility.c           |   2 +-
 hw/s390x/sclp.c                     | 145 ++++++++++++++++++++--------
 include/hw/s390x/sclp.h             |   6 +-
 linux-headers/asm-s390/kvm.h        |   7 +-
 linux-headers/linux/kvm.h           |   1 +
 target/s390x/cpu.h                  |   2 +
 target/s390x/cpu_features.h         |   1 +
 target/s390x/cpu_features_def.h.inc |   4 +
 target/s390x/cpu_models.c           |   1 +
 target/s390x/gen-features.c         |   2 +
 target/s390x/kvm.c                  |  39 ++++++++
 target/s390x/machine.c              |  17 ++++
 12 files changed, 183 insertions(+), 44 deletions(-)

-- 
2.26.2

[PATCH v5 1/8] s390/sclp: get machine once during read scp/cpu info

[Collin Walling]

Functions within read scp/cpu info will need access to the machine
state. Let's make a call to retrieve the machine state once and
pass the appropriate data to the respective functions.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/sclp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index a0ce444b4b..28b973de8f 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -49,9 +49,8 @@ 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(MachineState *ms, CPUEntry *entry, int *count)
 {
-    MachineState *ms = MACHINE(qdev_get_machine());
     uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
     int i;
 
@@ -77,7 +76,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(machine, 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);
@@ -132,10 +131,11 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 /* Provide information about the CPU */
 static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
 {
+    MachineState *machine = MACHINE(qdev_get_machine());
     ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
     int cpu_count;
 
-    prepare_cpu_entries(sclp, cpu_info->entries, &cpu_count);
+    prepare_cpu_entries(machine, 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.26.2

[PATCH v5 2/8] s390/sclp: rework sclp boundary checks

[Collin Walling]

Rework the SCLP boundary check to account for different SCLP commands
(eventually) allowing different boundary sizes.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
Acked-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/sclp.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 28b973de8f..69a8724dc7 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -49,6 +49,18 @@ static inline bool sclp_command_code_valid(uint32_t code)
     return false;
 }
 
+static bool sccb_verify_boundary(uint64_t sccb_addr, uint16_t len)
+{
+    uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(len) - 1;
+    uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
+
+    if (sccb_max_addr < sccb_boundary) {
+        return true;
+    }
+
+    return false;
+}
+
 static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
 {
     uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
@@ -229,6 +241,11 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
         goto out_write;
     }
 
+    if (!sccb_verify_boundary(sccb, work_sccb.h.length)) {
+        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
+        goto out_write;
+    }
+
     sclp_c->execute(sclp, &work_sccb, code);
 out_write:
     s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
@@ -274,7 +291,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)) {
+    if (!sccb_verify_boundary(sccb, work_sccb.h.length)) {
         work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
         goto out_write;
     }
-- 
2.26.2

[PATCH v5 3/8] s390/sclp: read sccb from mem based on provided length

[Collin Walling]

The header contained within the SCCB passed to the SCLP service call
contains the actual length of the SCCB. Instead of allocating a static
4K size for the work sccb, let's allow for a variable size determined
by the value 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/event-facility.c |  2 +-
 hw/s390x/sclp.c           | 58 ++++++++++++++++++++++-----------------
 include/hw/s390x/sclp.h   |  2 +-
 3 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index 645b4080c5..ed92ce510d 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -213,7 +213,7 @@ static uint16_t handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb,
 
     event_buf = &red->ebh;
     event_buf->length = 0;
-    slen = sizeof(sccb->data);
+    slen = sccb_data_len(sccb);
 
     rc = SCLP_RC_NO_EVENT_BUFFERS_STORED;
 
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 69a8724dc7..cb8e2e8ec3 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -231,25 +231,30 @@ 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);
+    SCCBHeader header;
+    SCCB *work_sccb;
 
-    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
+    s390_cpu_pv_mem_read(env_archcpu(env), 0, &header, sizeof(SCCBHeader));
+
+    work_sccb = g_malloc0(header.length);
+    s390_cpu_pv_mem_read(env_archcpu(env), 0, work_sccb,
+                         be16_to_cpu(header.length));
 
     if (!sclp_command_code_valid(code)) {
-        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
+        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
         goto out_write;
     }
 
-    if (!sccb_verify_boundary(sccb, work_sccb.h.length)) {
-        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
+    if (!sccb_verify_boundary(sccb, work_sccb->h.length)) {
+        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
         goto out_write;
     }
 
-    sclp_c->execute(sclp, &work_sccb, code);
+    sclp_c->execute(sclp, work_sccb, code);
 out_write:
-    s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
-                          be16_to_cpu(work_sccb.h.length));
+    s390_cpu_pv_mem_write(env_archcpu(env), 0, work_sccb,
+                          be16_to_cpu(work_sccb->h.length));
+    g_free(work_sccb);
     sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
     return 0;
 }
@@ -258,9 +263,8 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
 {
     SCLPDevice *sclp = get_sclp_device();
     SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
-    SCCB work_sccb;
-
-    hwaddr sccb_len = sizeof(SCCB);
+    SCCBHeader header;
+    SCCB *work_sccb;
 
     /* first some basic checks on program checks */
     if (env->psw.mask & PSW_MASK_PSTATE) {
@@ -274,33 +278,37 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
         return -PGM_SPECIFICATION;
     }
 
+    /* the header contains the actual length of the sccb */
+    cpu_physical_memory_read(sccb, &header, sizeof(SCCBHeader));
+
+    /* Valid sccb sizes */
+    if (be16_to_cpu(header.length) < sizeof(SCCBHeader)) {
+        return -PGM_SPECIFICATION;
+    }
+
     /*
      * we want to work on a private copy of the sccb, to prevent guests
      * 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);
-
-    /* Valid sccb sizes */
-    if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
-        return -PGM_SPECIFICATION;
-    }
+    work_sccb = g_malloc0(header.length);
+    cpu_physical_memory_read(sccb, work_sccb, be16_to_cpu(header.length));
 
     if (!sclp_command_code_valid(code)) {
-        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
+        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
         goto out_write;
     }
 
-    if (!sccb_verify_boundary(sccb, work_sccb.h.length)) {
-        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
+    if (!sccb_verify_boundary(sccb, work_sccb->h.length)) {
+        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
         goto out_write;
     }
 
-    sclp_c->execute(sclp, &work_sccb, code);
+    sclp_c->execute(sclp, work_sccb, code);
 out_write:
-    cpu_physical_memory_write(sccb, &work_sccb,
-                              be16_to_cpu(work_sccb.h.length));
-
+    cpu_physical_memory_write(sccb, work_sccb,
+                              be16_to_cpu(work_sccb->h.length));
+    g_free(work_sccb);
     sclp_c->service_interrupt(sclp, sccb);
 
     return 0;
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index a87ed2a0ab..98c4727984 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -177,7 +177,7 @@ typedef struct IoaCfgSccb {
 
 typedef struct SCCB {
     SCCBHeader h;
-    char data[SCCB_DATA_LEN];
+    char data[];
  } QEMU_PACKED SCCB;
 
 #define TYPE_SCLP "sclp"
-- 
2.26.2

[PATCH v5 4/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.

While we're at it, let's cleanup the length check by placing the
calculation inside a macro.

Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length")
Signed-off-by: Collin Walling <walling@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/s390x/sclp.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index cb8e2e8ec3..12829bdd05 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -78,6 +78,8 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
     }
 }
 
+#define SCCB_REQ_LEN(s, max_cpus) (sizeof(s) + max_cpus * sizeof(CPUEntry))
+
 /* Provide information about the configuration, CPUs and storage */
 static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 {
@@ -86,6 +88,12 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     int cpu_count;
     int rnsize, rnmax;
     IplParameterBlock *ipib = s390_ipl_get_iplb();
+    int required_len = SCCB_REQ_LEN(ReadInfo, machine->possible_cpus->len);
+
+    if (be16_to_cpu(sccb->h.length) < required_len) {
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+        return;
+    }
 
     /* CPU information */
     prepare_cpu_entries(machine, read_info->entries, &cpu_count);
@@ -95,12 +103,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);
@@ -146,18 +148,18 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
     MachineState *machine = MACHINE(qdev_get_machine());
     ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
     int cpu_count;
+    int required_len = SCCB_REQ_LEN(ReadCpuInfo, machine->possible_cpus->len);
+
+    if (be16_to_cpu(sccb->h.length) < required_len) {
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+        return;
+    }
 
     prepare_cpu_entries(machine, 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));
-- 
2.26.2

[PATCH v5 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 the 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 12829bdd05..803fdd5ed4 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -89,6 +89,8 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     int rnsize, rnmax;
     IplParameterBlock *ipib = s390_ipl_get_iplb();
     int required_len = SCCB_REQ_LEN(ReadInfo, machine->possible_cpus->len);
+    int offset_cpu = offsetof(ReadInfo, entries);
+    CPUEntry *entries_start = (void *)sccb + offset_cpu;
 
     if (be16_to_cpu(sccb->h.length) < required_len) {
         sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
@@ -96,9 +98,9 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     }
 
     /* CPU information */
-    prepare_cpu_entries(machine, read_info->entries, &cpu_count);
+    prepare_cpu_entries(machine, 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.26.2

[PATCH v5 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
when this feature is disabled.

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 (such as during re-ipl).

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

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 803fdd5ed4..87d468087b 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -49,13 +49,30 @@ static inline bool sclp_command_code_valid(uint32_t code)
     return false;
 }
 
-static bool sccb_verify_boundary(uint64_t sccb_addr, uint16_t len)
+static bool sccb_verify_boundary(uint64_t sccb_addr, uint16_t len,
+                                 uint32_t code)
 {
     uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(len) - 1;
     uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
 
-    if (sccb_max_addr < sccb_boundary) {
-        return true;
+    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 Read SCP/CPU Info 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;
+        }
+        /* fallthrough */
+    default:
+        if (sccb_max_addr < sccb_boundary) {
+            return true;
+        }
     }
 
     return false;
@@ -80,6 +97,12 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
 
 #define SCCB_REQ_LEN(s, max_cpus) (sizeof(s) + max_cpus * sizeof(CPUEntry))
 
+static inline bool ext_len_sccb_supported(SCCBHeader header)
+{
+    return s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) &&
+           header.control_mask[2] & SCLP_VARIABLE_LENGTH_RESPONSE;
+}
+
 /* Provide information about the configuration, CPUs and storage */
 static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 {
@@ -89,10 +112,15 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     int rnsize, rnmax;
     IplParameterBlock *ipib = s390_ipl_get_iplb();
     int required_len = SCCB_REQ_LEN(ReadInfo, machine->possible_cpus->len);
-    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 (be16_to_cpu(sccb->h.length) < required_len) {
+        if (ext_len_sccb_supported(sccb->h)) {
+            sccb->h.length = cpu_to_be16(required_len);
+        }
         sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
         return;
     }
@@ -153,6 +181,9 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
     int required_len = SCCB_REQ_LEN(ReadCpuInfo, machine->possible_cpus->len);
 
     if (be16_to_cpu(sccb->h.length) < required_len) {
+        if (ext_len_sccb_supported(sccb->h)) {
+            sccb->h.length = cpu_to_be16(required_len);
+        }
         sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
         return;
     }
@@ -249,7 +280,7 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
         goto out_write;
     }
 
-    if (!sccb_verify_boundary(sccb, work_sccb->h.length)) {
+    if (!sccb_verify_boundary(sccb, work_sccb->h.length, code)) {
         work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
         goto out_write;
     }
@@ -303,7 +334,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
         goto out_write;
     }
 
-    if (!sccb_verify_boundary(sccb, work_sccb->h.length)) {
+    if (!sccb_verify_boundary(sccb, work_sccb->h.length, code)) {
         work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
         goto out_write;
     }
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index 98c4727984..141e57f765 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.h.inc b/target/s390x/cpu_features_def.h.inc
index 5942f81f16..1c04cc18f4 100644
--- a/target/s390x/cpu_features_def.h.inc
+++ b/target/s390x/cpu_features_def.h.inc
@@ -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 f2f75d2a57..a2d5ad78f6 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2456,6 +2456,14 @@ 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.
+     * For PV guests this is completely fenced by the Ultravisor, as Service
+     * Call error checking and STFLE interpretation are handled via SIE.
+     */
+    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.26.2

[PATCH v5 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 | 7 +++++--
 linux-headers/linux/kvm.h    | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
index 0138ccb0d8..f053b8304a 100644
--- a/linux-headers/asm-s390/kvm.h
+++ b/linux-headers/asm-s390/kvm.h
@@ -231,11 +231,13 @@ struct kvm_guest_debug_arch {
 #define KVM_SYNC_GSCB   (1UL << 9)
 #define KVM_SYNC_BPBC   (1UL << 10)
 #define KVM_SYNC_ETOKEN (1UL << 11)
+#define KVM_SYNC_DIAG318 (1UL << 12)
 
 #define KVM_SYNC_S390_VALID_FIELDS \
 	(KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
 	 KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
-	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
+	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN | \
+	 KVM_SYNC_DIAG318)
 
 /* length and alignment of the sdnx as a power of two */
 #define SDNXC 8
@@ -264,7 +266,8 @@ struct kvm_sync_regs {
 	__u8 reserved2 : 7;
 	__u8 padding1[51];	/* riccb needs to be 64byte aligned */
 	__u8 riccb[64];		/* runtime instrumentation controls block */
-	__u8 padding2[192];	/* sdnx needs to be 256byte aligned */
+	__u64 diag318;		/* diagnose 0x318 info */
+	__u8 padding2[184];	/* sdnx needs to be 256byte aligned */
 	union {
 		__u8 sdnx[SDNXL];  /* state description annex */
 		struct {
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index a28c366737..a789bfb5a2 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1031,6 +1031,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_PPC_SECURE_GUEST 181
 #define KVM_CAP_HALT_POLL 182
 #define KVM_CAP_ASYNC_PF_INT 183
+#define KVM_CAP_S390_DIAG318 184
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.26.2

[PATCH v5 8/8] s390: guest support for diagnose 0x318

[Collin Walling]

DIAGNOSE 0x318 (diag318) is an s390 instruction that allows the storage
of diagnostic information that is collected by the firmware in the case
of hardware/firmware service events.

QEMU handles the instruction by storing the info in the CPU state. A
subsequent register sync will communicate the data to the hypervisor.

QEMU handles the migration via a VM State Description.

This feature depends on the Extended-Length SCCB (els) feature. If
els is not present, then a warning will be printed and the SCLP bit
that allows the Linux kernel to execute the instruction will not be
set.

Availability of this instruction is determined by byte 134 (aka fac134)
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 may not be able to read information regarding all CPUs
unless the guest kernel supports an extended-length SCCB.

This feature is not supported in protected virtualization mode.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
Acked-by: Janosch Frank <frankja@linux.ibm.com>
---
 hw/s390x/sclp.c                     |  5 +++++
 include/hw/s390x/sclp.h             |  3 +++
 target/s390x/cpu.h                  |  2 ++
 target/s390x/cpu_features.h         |  1 +
 target/s390x/cpu_features_def.h.inc |  3 +++
 target/s390x/cpu_models.c           |  1 +
 target/s390x/gen-features.c         |  1 +
 target/s390x/kvm.c                  | 31 +++++++++++++++++++++++++++++
 target/s390x/machine.c              | 17 ++++++++++++++++
 9 files changed, 64 insertions(+)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 87d468087b..ad5d70e14d 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -139,6 +139,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_FAC134,
+                            &read_info->fac134);
+    }
+
     read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
                                         SCLP_HAS_IOA_RECONFIG);
 
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index 141e57f765..516f949109 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  fac134;
+    uint8_t  _reserved8[144 - 135];     /* 135-143 */
     struct CPUEntry entries[];
 } QEMU_PACKED ReadInfo;
 
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 035427521c..f875ebf0f4 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -112,6 +112,8 @@ struct CPUS390XState {
     uint16_t external_call_addr;
     DECLARE_BITMAP(emergency_signals, S390_MAX_CPUS);
 
+    uint64_t diag318_info;
+
     /* Fields up to this point are cleared by a CPU reset */
     struct {} end_reset_fields;
 
diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
index 2a29475493..ef52ffce83 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_FAC134,
     S390_FEAT_TYPE_SCLP_CPU,
     S390_FEAT_TYPE_MISC,
     S390_FEAT_TYPE_PLO,
diff --git a/target/s390x/cpu_features_def.h.inc b/target/s390x/cpu_features_def.h.inc
index 1c04cc18f4..f82b4b5ec1 100644
--- a/target/s390x/cpu_features_def.h.inc
+++ b/target/s390x/cpu_features_def.h.inc
@@ -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 Facilities byte 134 (bit numbers relative to byte-134) */
+DEF_FEAT(DIAG_318, "diag318", SCLP_FAC134, 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 c2af226174..9d615f13e7 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -824,6 +824,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_DIAG_318, S390_FEAT_EXTENDED_LENGTH_SCCB },
     };
     int i;
 
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 6857f657fb..a1f0a6f3c6 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_DIAG_318,
 };
 
 static uint16_t full_GEN12_GA2[] = {
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index a2d5ad78f6..b79feeba9f 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -105,6 +105,7 @@
 
 #define DIAG_TIMEREVENT                 0x288
 #define DIAG_IPL                        0x308
+#define DIAG_SET_CONTROL_PROGRAM_CODES  0x318
 #define DIAG_KVM_HYPERCALL              0x500
 #define DIAG_KVM_BREAKPOINT             0x501
 
@@ -602,6 +603,11 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_ETOKEN;
     }
 
+    if (can_sync_regs(cs, KVM_SYNC_DIAG318)) {
+        cs->kvm_run->s.regs.diag318 = env->diag318_info;
+        cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
+    }
+
     /* Finally the prefix */
     if (can_sync_regs(cs, KVM_SYNC_PREFIX)) {
         cs->kvm_run->s.regs.prefix = env->psa;
@@ -741,6 +747,10 @@ int kvm_arch_get_registers(CPUState *cs)
         }
     }
 
+    if (can_sync_regs(cs, KVM_SYNC_DIAG318)) {
+        env->diag318_info = cs->kvm_run->s.regs.diag318;
+    }
+
     return 0;
 }
 
@@ -1601,6 +1611,19 @@ static int handle_sw_breakpoint(S390CPU *cpu, struct kvm_run *run)
     return -ENOENT;
 }
 
+static void handle_diag_318(S390CPU *cpu, struct kvm_run *run)
+{
+    uint64_t reg = (run->s390_sieic.ipa & 0x00f0) >> 4;
+    uint64_t diag318_info = run->s.regs.gprs[reg];
+
+    cpu->env.diag318_info = diag318_info;
+
+    if (can_sync_regs(CPU(cpu), KVM_SYNC_DIAG318)) {
+        run->s.regs.diag318 = diag318_info;
+        run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
+    }
+}
+
 #define DIAG_KVM_CODE_MASK 0x000000000000ffff
 
 static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)
@@ -1620,6 +1643,9 @@ static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)
     case DIAG_IPL:
         kvm_handle_diag_308(cpu, run);
         break;
+    case DIAG_SET_CONTROL_PROGRAM_CODES:
+        handle_diag_318(cpu, run);
+        break;
     case DIAG_KVM_HYPERCALL:
         r = handle_hypercall(cpu, run);
         break;
@@ -2464,6 +2490,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
      */
     set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
 
+    /* DIAGNOSE 0x318 is not supported under protected virtualization */
+    if (!s390_is_pv() && kvm_check_extension(kvm_state, KVM_CAP_S390_DIAG318)) {
+        set_bit(S390_FEAT_DIAG_318, 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);
diff --git a/target/s390x/machine.c b/target/s390x/machine.c
index 549bb6c280..5b4e82f1ab 100644
--- a/target/s390x/machine.c
+++ b/target/s390x/machine.c
@@ -234,6 +234,22 @@ const VMStateDescription vmstate_etoken = {
     }
 };
 
+static bool diag318_needed(void *opaque)
+{
+    return s390_has_feat(S390_FEAT_DIAG_318);
+}
+
+const VMStateDescription vmstate_diag318 = {
+    .name = "cpu/diag318",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = diag318_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(env.diag318_info, S390CPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 const VMStateDescription vmstate_s390_cpu = {
     .name = "cpu",
     .post_load = cpu_post_load,
@@ -270,6 +286,7 @@ const VMStateDescription vmstate_s390_cpu = {
         &vmstate_gscb,
         &vmstate_bpbc,
         &vmstate_etoken,
+        &vmstate_diag318,
         NULL
     },
 };
-- 
2.26.2

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

[Cornelia Huck]

On Thu, 10 Sep 2020 05:36:54 -0400
Collin Walling <walling@linux.ibm.com> wrote:

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

This needs to be replaced by a real headers update now. I'll do that
myself, no need to repost.

Re: [PATCH v5 2/8] s390/sclp: rework sclp boundary checks

[Thomas Huth]

On 10/09/2020 11.36, Collin Walling wrote:
> Rework the SCLP boundary check to account for different SCLP commands
> (eventually) allowing different boundary sizes.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/sclp.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 28b973de8f..69a8724dc7 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -49,6 +49,18 @@ static inline bool sclp_command_code_valid(uint32_t code)
>      return false;
>  }
>  
> +static bool sccb_verify_boundary(uint64_t sccb_addr, uint16_t len)

Maybe it would be good to add a comment in front of the function to say
that len must be big endian?

 Thomas

> +{
> +    uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(len) - 1;
> +    uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
> +
> +    if (sccb_max_addr < sccb_boundary) {
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
>  static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>  {
>      uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
> @@ -229,6 +241,11 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>          goto out_write;
>      }
>  
> +    if (!sccb_verify_boundary(sccb, work_sccb.h.length)) {
> +        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> +        goto out_write;
> +    }
> +
>      sclp_c->execute(sclp, &work_sccb, code);
>  out_write:
>      s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
> @@ -274,7 +291,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)) {
> +    if (!sccb_verify_boundary(sccb, work_sccb.h.length)) {
>          work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>          goto out_write;
>      }
> 

Re: [PATCH v5 3/8] s390/sclp: read sccb from mem based on provided length

[Thomas Huth]

On 10/09/2020 11.36, Collin Walling wrote:
> The header contained within the SCCB passed to the SCLP service call
> contains the actual length of the SCCB. Instead of allocating a static
> 4K size for the work sccb, let's allow for a variable size determined
> by the value 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/event-facility.c |  2 +-
>  hw/s390x/sclp.c           | 58 ++++++++++++++++++++++-----------------
>  include/hw/s390x/sclp.h   |  2 +-
>  3 files changed, 35 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> index 645b4080c5..ed92ce510d 100644
> --- a/hw/s390x/event-facility.c
> +++ b/hw/s390x/event-facility.c
> @@ -213,7 +213,7 @@ static uint16_t handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb,
>  
>      event_buf = &red->ebh;
>      event_buf->length = 0;
> -    slen = sizeof(sccb->data);
> +    slen = sccb_data_len(sccb);
>  
>      rc = SCLP_RC_NO_EVENT_BUFFERS_STORED;
>  
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 69a8724dc7..cb8e2e8ec3 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -231,25 +231,30 @@ 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);
> +    SCCBHeader header;
> +    SCCB *work_sccb;

I'd maybe use "g_autofree SCCB *work_sccb = NULL" so you don't have to
worry about doing the g_free() later.

> -    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &header, sizeof(SCCBHeader));
> +
> +    work_sccb = g_malloc0(header.length);

Please use be16_to_cpu(header.length) here as well.

> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, work_sccb,
> +                         be16_to_cpu(header.length));
>  
>      if (!sclp_command_code_valid(code)) {
> -        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> +        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>          goto out_write;
>      }
>  
> -    if (!sccb_verify_boundary(sccb, work_sccb.h.length)) {
> -        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> +    if (!sccb_verify_boundary(sccb, work_sccb->h.length)) {
> +        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>          goto out_write;
>      }
>  
> -    sclp_c->execute(sclp, &work_sccb, code);
> +    sclp_c->execute(sclp, work_sccb, code);
>  out_write:
> -    s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
> -                          be16_to_cpu(work_sccb.h.length));
> +    s390_cpu_pv_mem_write(env_archcpu(env), 0, work_sccb,
> +                          be16_to_cpu(work_sccb->h.length));
> +    g_free(work_sccb);
>      sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
>      return 0;
>  }
> @@ -258,9 +263,8 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>  {
>      SCLPDevice *sclp = get_sclp_device();
>      SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
> -    SCCB work_sccb;
> -
> -    hwaddr sccb_len = sizeof(SCCB);
> +    SCCBHeader header;
> +    SCCB *work_sccb;

Maybe g_autofree again?

>      /* first some basic checks on program checks */
>      if (env->psw.mask & PSW_MASK_PSTATE) {
> @@ -274,33 +278,37 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>          return -PGM_SPECIFICATION;
>      }
>  
> +    /* the header contains the actual length of the sccb */
> +    cpu_physical_memory_read(sccb, &header, sizeof(SCCBHeader));
> +
> +    /* Valid sccb sizes */
> +    if (be16_to_cpu(header.length) < sizeof(SCCBHeader)) {
> +        return -PGM_SPECIFICATION;
> +    }
> +
>      /*
>       * we want to work on a private copy of the sccb, to prevent guests
>       * 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);
> -
> -    /* Valid sccb sizes */
> -    if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
> -        return -PGM_SPECIFICATION;
> -    }
> +    work_sccb = g_malloc0(header.length);

Needs be16_to_cpu again.

> +    cpu_physical_memory_read(sccb, work_sccb, be16_to_cpu(header.length));
>  
>      if (!sclp_command_code_valid(code)) {
> -        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> +        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>          goto out_write;
>      }
>  
> -    if (!sccb_verify_boundary(sccb, work_sccb.h.length)) {
> -        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> +    if (!sccb_verify_boundary(sccb, work_sccb->h.length)) {
> +        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>          goto out_write;
>      }
>  
> -    sclp_c->execute(sclp, &work_sccb, code);
> +    sclp_c->execute(sclp, work_sccb, code);
>  out_write:
> -    cpu_physical_memory_write(sccb, &work_sccb,
> -                              be16_to_cpu(work_sccb.h.length));
> -
> +    cpu_physical_memory_write(sccb, work_sccb,
> +                              be16_to_cpu(work_sccb->h.length));
> +    g_free(work_sccb);
>      sclp_c->service_interrupt(sclp, sccb);
>  
>      return 0;

 Thomas

Re: [PATCH v5 3/8] s390/sclp: read sccb from mem based on provided length

[Collin Walling]

On 9/10/20 1:50 PM, Thomas Huth wrote:
> On 10/09/2020 11.36, Collin Walling wrote:
>> The header contained within the SCCB passed to the SCLP service call
>> contains the actual length of the SCCB. Instead of allocating a static
>> 4K size for the work sccb, let's allow for a variable size determined
>> by the value 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/event-facility.c |  2 +-
>>  hw/s390x/sclp.c           | 58 ++++++++++++++++++++++-----------------
>>  include/hw/s390x/sclp.h   |  2 +-
>>  3 files changed, 35 insertions(+), 27 deletions(-)
>>
>> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
>> index 645b4080c5..ed92ce510d 100644
>> --- a/hw/s390x/event-facility.c
>> +++ b/hw/s390x/event-facility.c
>> @@ -213,7 +213,7 @@ static uint16_t handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb,
>>  
>>      event_buf = &red->ebh;
>>      event_buf->length = 0;
>> -    slen = sizeof(sccb->data);
>> +    slen = sccb_data_len(sccb);
>>  
>>      rc = SCLP_RC_NO_EVENT_BUFFERS_STORED;
>>  
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 69a8724dc7..cb8e2e8ec3 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -231,25 +231,30 @@ 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);
>> +    SCCBHeader header;
>> +    SCCB *work_sccb;
> 
> I'd maybe use "g_autofree SCCB *work_sccb = NULL" so you don't have to
> worry about doing the g_free() later.

Can do.

> 
>> -    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
>> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &header, sizeof(SCCBHeader));
>> +
>> +    work_sccb = g_malloc0(header.length);
> 
> Please use be16_to_cpu(header.length) here as well.

Good catch, thanks!

> 
>> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, work_sccb,
>> +                         be16_to_cpu(header.length));
>>  
>>      if (!sclp_command_code_valid(code)) {
>> -        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>> +        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>>          goto out_write;
>>      }
>>  
>> -    if (!sccb_verify_boundary(sccb, work_sccb.h.length)) {
>> -        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>> +    if (!sccb_verify_boundary(sccb, work_sccb->h.length)) {
>> +        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>>          goto out_write;
>>      }
>>  
>> -    sclp_c->execute(sclp, &work_sccb, code);
>> +    sclp_c->execute(sclp, work_sccb, code);
>>  out_write:
>> -    s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
>> -                          be16_to_cpu(work_sccb.h.length));
>> +    s390_cpu_pv_mem_write(env_archcpu(env), 0, work_sccb,
>> +                          be16_to_cpu(work_sccb->h.length));
>> +    g_free(work_sccb);
>>      sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
>>      return 0;
>>  }
>> @@ -258,9 +263,8 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>>  {
>>      SCLPDevice *sclp = get_sclp_device();
>>      SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
>> -    SCCB work_sccb;
>> -
>> -    hwaddr sccb_len = sizeof(SCCB);
>> +    SCCBHeader header;
>> +    SCCB *work_sccb;
> 
> Maybe g_autofree again?
> 
>>      /* first some basic checks on program checks */
>>      if (env->psw.mask & PSW_MASK_PSTATE) {
>> @@ -274,33 +278,37 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>>          return -PGM_SPECIFICATION;
>>      }
>>  
>> +    /* the header contains the actual length of the sccb */
>> +    cpu_physical_memory_read(sccb, &header, sizeof(SCCBHeader));
>> +
>> +    /* Valid sccb sizes */
>> +    if (be16_to_cpu(header.length) < sizeof(SCCBHeader)) {
>> +        return -PGM_SPECIFICATION;
>> +    }
>> +
>>      /*
>>       * we want to work on a private copy of the sccb, to prevent guests
>>       * 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);
>> -
>> -    /* Valid sccb sizes */
>> -    if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
>> -        return -PGM_SPECIFICATION;
>> -    }
>> +    work_sccb = g_malloc0(header.length);
> 
> Needs be16_to_cpu again.
> 
>> +    cpu_physical_memory_read(sccb, work_sccb, be16_to_cpu(header.length));
>>  
>>      if (!sclp_command_code_valid(code)) {
>> -        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>> +        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>>          goto out_write;
>>      }
>>  
>> -    if (!sccb_verify_boundary(sccb, work_sccb.h.length)) {
>> -        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>> +    if (!sccb_verify_boundary(sccb, work_sccb->h.length)) {
>> +        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>>          goto out_write;
>>      }
>>  
>> -    sclp_c->execute(sclp, &work_sccb, code);
>> +    sclp_c->execute(sclp, work_sccb, code);
>>  out_write:
>> -    cpu_physical_memory_write(sccb, &work_sccb,
>> -                              be16_to_cpu(work_sccb.h.length));
>> -
>> +    cpu_physical_memory_write(sccb, work_sccb,
>> +                              be16_to_cpu(work_sccb->h.length));
>> +    g_free(work_sccb);
>>      sclp_c->service_interrupt(sclp, sccb);
>>  
>>      return 0;
> 
>  Thomas
> 
> 

Thanks, Thomas!

-- 
Regards,
Collin

Stay safe and stay healthy

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

[Thomas Huth]

On 10/09/2020 11.36, Collin Walling wrote:
> 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 the 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(-)

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

Re: [PATCH v5 2/8] s390/sclp: rework sclp boundary checks

[Cornelia Huck]

On Thu, 10 Sep 2020 19:45:01 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 10/09/2020 11.36, Collin Walling wrote:
> > Rework the SCLP boundary check to account for different SCLP commands
> > (eventually) allowing different boundary sizes.
> > 
> > Signed-off-by: Collin Walling <walling@linux.ibm.com>
> > Acked-by: Janosch Frank <frankja@linux.ibm.com>
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  hw/s390x/sclp.c | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> > index 28b973de8f..69a8724dc7 100644
> > --- a/hw/s390x/sclp.c
> > +++ b/hw/s390x/sclp.c
> > @@ -49,6 +49,18 @@ static inline bool sclp_command_code_valid(uint32_t code)
> >      return false;
> >  }
> >  
> > +static bool sccb_verify_boundary(uint64_t sccb_addr, uint16_t len)  
> 
> Maybe it would be good to add a comment in front of the function to say
> that len must be big endian?

What about renaming it to sccb_h_len or so? That would make it more
clear that the parameter is not just some random length.

> 
>  Thomas
> 
> > +{
> > +    uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(len) - 1;
> > +    uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
> > +
> > +    if (sccb_max_addr < sccb_boundary) {
> > +        return true;
> > +    }
> > +
> > +    return false;
> > +}
> > +
> >  static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
> >  {
> >      uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
> > @@ -229,6 +241,11 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
> >          goto out_write;
> >      }
> >  
> > +    if (!sccb_verify_boundary(sccb, work_sccb.h.length)) {

...name inspired by the 'h' in here.

> > +        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> > +        goto out_write;
> > +    }
> > +
> >      sclp_c->execute(sclp, &work_sccb, code);
> >  out_write:
> >      s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
> > @@ -274,7 +291,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)) {
> > +    if (!sccb_verify_boundary(sccb, work_sccb.h.length)) {
> >          work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> >          goto out_write;
> >      }
> >   
> 

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

[Cornelia Huck]

On Thu, 10 Sep 2020 05:36:52 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> 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 the 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(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

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

[Cornelia Huck]

On Thu, 10 Sep 2020 05:36:53 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> 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
> when this feature is disabled.
> 
> 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 (such as during re-ipl).
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  hw/s390x/sclp.c                     | 43 +++++++++++++++++++++++++----
>  include/hw/s390x/sclp.h             |  1 +
>  target/s390x/cpu_features_def.h.inc |  1 +
>  target/s390x/gen-features.c         |  1 +
>  target/s390x/kvm.c                  |  8 ++++++
>  5 files changed, 48 insertions(+), 6 deletions(-)

Acked-by: Cornelia Huck <cohuck@redhat.com>

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

[Thomas Huth]

On 10/09/2020 11.36, Collin Walling wrote:
> 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
> when this feature is disabled.
> 
> 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 (such as during re-ipl).
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
[...]
>  /* Provide information about the configuration, CPUs and storage */
>  static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>  {
> @@ -89,10 +112,15 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>      int rnsize, rnmax;
>      IplParameterBlock *ipib = s390_ipl_get_iplb();
>      int required_len = SCCB_REQ_LEN(ReadInfo, machine->possible_cpus->len);
> -    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;

Sorry, but I'm having somewhat trouble to understand this...
What's the difference between offsetof(ReadInfo, entries) and
SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET ? Aren't both terms resulting in the
value 128 ?

 Thomas

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

[Thomas Huth]

On 11/09/2020 15.41, Thomas Huth wrote:
> On 10/09/2020 11.36, Collin Walling wrote:
>> 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
>> when this feature is disabled.
>>
>> 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 (such as during re-ipl).
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
> [...]
>>  /* Provide information about the configuration, CPUs and storage */
>>  static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>  {
>> @@ -89,10 +112,15 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>      int rnsize, rnmax;
>>      IplParameterBlock *ipib = s390_ipl_get_iplb();
>>      int required_len = SCCB_REQ_LEN(ReadInfo, machine->possible_cpus->len);
>> -    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;
> 
> Sorry, but I'm having somewhat trouble to understand this...
> What's the difference between offsetof(ReadInfo, entries) and
> SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET ? Aren't both terms resulting in the
> value 128 ?

Ah, well, the answer is clear after looking at patch 8/8 ... ReadInfo is
extended there, so offsetof(ReadInfo, entries) will result in a
different value.
Might have been better to move the above hunk into patch 8/8, but if you
want to keep it here, that's now ok for me, too.

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

Re: [PATCH v5 2/8] s390/sclp: rework sclp boundary checks

[Collin Walling]

On 9/11/20 6:24 AM, Cornelia Huck wrote:
> On Thu, 10 Sep 2020 19:45:01 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 10/09/2020 11.36, Collin Walling wrote:
>>> Rework the SCLP boundary check to account for different SCLP commands
>>> (eventually) allowing different boundary sizes.
>>>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>  hw/s390x/sclp.c | 19 ++++++++++++++++++-
>>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>>> index 28b973de8f..69a8724dc7 100644
>>> --- a/hw/s390x/sclp.c
>>> +++ b/hw/s390x/sclp.c
>>> @@ -49,6 +49,18 @@ static inline bool sclp_command_code_valid(uint32_t code)
>>>      return false;
>>>  }
>>>  
>>> +static bool sccb_verify_boundary(uint64_t sccb_addr, uint16_t len)  
>>
>> Maybe it would be good to add a comment in front of the function to say
>> that len must be big endian?
> 
> What about renaming it to sccb_h_len or so? That would make it more
> clear that the parameter is not just some random length.
> 

I think that makes sense.

>>
>>  Thomas
>>
>>> +{
>>> +    uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(len) - 1;
>>> +    uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
>>> +
>>> +    if (sccb_max_addr < sccb_boundary) {
>>> +        return true;
>>> +    }
>>> +
>>> +    return false;
>>> +}
>>> +
>>>  static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>>>  {
>>>      uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
>>> @@ -229,6 +241,11 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>>>          goto out_write;
>>>      }
>>>  
>>> +    if (!sccb_verify_boundary(sccb, work_sccb.h.length)) {
> 
> ...name inspired by the 'h' in here.
> 
>>> +        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>>> +        goto out_write;
>>> +    }
>>> +
>>>      sclp_c->execute(sclp, &work_sccb, code);
>>>  out_write:
>>>      s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
>>> @@ -274,7 +291,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)) {
>>> +    if (!sccb_verify_boundary(sccb, work_sccb.h.length)) {
>>>          work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>>>          goto out_write;
>>>      }
>>>   
>>
> 
> 


-- 
Regards,
Collin

Stay safe and stay healthy

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

[Collin Walling]

On 9/11/20 9:54 AM, Thomas Huth wrote:
> On 11/09/2020 15.41, Thomas Huth wrote:
>> On 10/09/2020 11.36, Collin Walling wrote:
>>> 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
>>> when this feature is disabled.
>>>
>>> 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 (such as during re-ipl).
>>>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>> ---
>> [...]
>>>  /* Provide information about the configuration, CPUs and storage */
>>>  static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>>  {
>>> @@ -89,10 +112,15 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>>      int rnsize, rnmax;
>>>      IplParameterBlock *ipib = s390_ipl_get_iplb();
>>>      int required_len = SCCB_REQ_LEN(ReadInfo, machine->possible_cpus->len);
>>> -    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;
>>
>> Sorry, but I'm having somewhat trouble to understand this...
>> What's the difference between offsetof(ReadInfo, entries) and
>> SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET ? Aren't both terms resulting in the
>> value 128 ?
> 
> Ah, well, the answer is clear after looking at patch 8/8 ... ReadInfo is
> extended there, so offsetof(ReadInfo, entries) will result in a
> different value.
> Might have been better to move the above hunk into patch 8/8, but if you
> want to keep it here, that's now ok for me, too.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> 

I see your point. In retrospect, it might've been better to include it
in patch 8/8 so it's more clear why these features are introduced within
the same patch set.

If there are any requests to change / fixup this patch in any other
regard, then I'll consider moving the offset_cpu calculation to 8/8.

Otherwise, I'll leave it here :)

Thanks!

-- 
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v5 8/8] s390: guest support for diagnose 0x318

[Thomas Huth]

On 10/09/2020 11.36, Collin Walling wrote:
> DIAGNOSE 0x318 (diag318) is an s390 instruction that allows the storage
> of diagnostic information that is collected by the firmware in the case
> of hardware/firmware service events.
> 
> QEMU handles the instruction by storing the info in the CPU state. A
> subsequent register sync will communicate the data to the hypervisor.
> 
> QEMU handles the migration via a VM State Description.
> 
> This feature depends on the Extended-Length SCCB (els) feature. If
> els is not present, then a warning will be printed and the SCLP bit
> that allows the Linux kernel to execute the instruction will not be
> set.
> 
> Availability of this instruction is determined by byte 134 (aka fac134)
> 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 may not be able to read information regarding all CPUs
> unless the guest kernel supports an extended-length SCCB.
> 
> This feature is not supported in protected virtualization mode.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  hw/s390x/sclp.c                     |  5 +++++
>  include/hw/s390x/sclp.h             |  3 +++
>  target/s390x/cpu.h                  |  2 ++
>  target/s390x/cpu_features.h         |  1 +
>  target/s390x/cpu_features_def.h.inc |  3 +++
>  target/s390x/cpu_models.c           |  1 +
>  target/s390x/gen-features.c         |  1 +
>  target/s390x/kvm.c                  | 31 +++++++++++++++++++++++++++++
>  target/s390x/machine.c              | 17 ++++++++++++++++
>  9 files changed, 64 insertions(+)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 87d468087b..ad5d70e14d 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -139,6 +139,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_FAC134,
> +                            &read_info->fac134);
> +    }

Wasn't this feature also possible if there are less than 240 CPUs? Or do
I mix that up with something else? ... well, maybe it's best anyway if
we only allow this when ELS is enabled.

>      read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
>                                          SCLP_HAS_IOA_RECONFIG);
>  
> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
> index 141e57f765..516f949109 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  fac134;
> +    uint8_t  _reserved8[144 - 135];     /* 135-143 */
>      struct CPUEntry entries[];

Could you please add a comment after entries[] like,

 /* only here with "ELS", it's at offset 128 otherwise */

?

>  } QEMU_PACKED ReadInfo;
>  
[...]
>  static uint16_t full_GEN12_GA2[] = {
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index a2d5ad78f6..b79feeba9f 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -105,6 +105,7 @@
>  
>  #define DIAG_TIMEREVENT                 0x288
>  #define DIAG_IPL                        0x308
> +#define DIAG_SET_CONTROL_PROGRAM_CODES  0x318
>  #define DIAG_KVM_HYPERCALL              0x500
>  #define DIAG_KVM_BREAKPOINT             0x501
>  
> @@ -602,6 +603,11 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>          cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_ETOKEN;
>      }
>  
> +    if (can_sync_regs(cs, KVM_SYNC_DIAG318)) {
> +        cs->kvm_run->s.regs.diag318 = env->diag318_info;
> +        cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
> +    }
> +
>      /* Finally the prefix */
>      if (can_sync_regs(cs, KVM_SYNC_PREFIX)) {
>          cs->kvm_run->s.regs.prefix = env->psa;
> @@ -741,6 +747,10 @@ int kvm_arch_get_registers(CPUState *cs)
>          }
>      }
>  
> +    if (can_sync_regs(cs, KVM_SYNC_DIAG318)) {
> +        env->diag318_info = cs->kvm_run->s.regs.diag318;
> +    }
> +
>      return 0;
>  }
>  
> @@ -1601,6 +1611,19 @@ static int handle_sw_breakpoint(S390CPU *cpu, struct kvm_run *run)
>      return -ENOENT;
>  }
>  
> +static void handle_diag_318(S390CPU *cpu, struct kvm_run *run)
> +{
> +    uint64_t reg = (run->s390_sieic.ipa & 0x00f0) >> 4;
> +    uint64_t diag318_info = run->s.regs.gprs[reg];
> +
> +    cpu->env.diag318_info = diag318_info;
> +
> +    if (can_sync_regs(CPU(cpu), KVM_SYNC_DIAG318)) {
> +        run->s.regs.diag318 = diag318_info;
> +        run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
> +    }
> +}

What's supposed to happen if a guest calls diag 318 but the
S390_FEAT_DIAG_318 has not been enabled? Shouldn't there be a program
interrupt in that case?

 Thomas

Re: [PATCH v5 8/8] s390: guest support for diagnose 0x318

[Thomas Huth]

On 11/09/2020 17.08, Thomas Huth wrote:
> On 10/09/2020 11.36, Collin Walling wrote:
>> DIAGNOSE 0x318 (diag318) is an s390 instruction that allows the storage
>> of diagnostic information that is collected by the firmware in the case
>> of hardware/firmware service events.
>>
>> QEMU handles the instruction by storing the info in the CPU state. A
>> subsequent register sync will communicate the data to the hypervisor.
>>
>> QEMU handles the migration via a VM State Description.
>>
>> This feature depends on the Extended-Length SCCB (els) feature. If
>> els is not present, then a warning will be printed and the SCLP bit
>> that allows the Linux kernel to execute the instruction will not be
>> set.
>>
>> Availability of this instruction is determined by byte 134 (aka fac134)
>> 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 may not be able to read information regarding all CPUs
>> unless the guest kernel supports an extended-length SCCB.
>>
>> This feature is not supported in protected virtualization mode.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  hw/s390x/sclp.c                     |  5 +++++
>>  include/hw/s390x/sclp.h             |  3 +++
>>  target/s390x/cpu.h                  |  2 ++
>>  target/s390x/cpu_features.h         |  1 +
>>  target/s390x/cpu_features_def.h.inc |  3 +++
>>  target/s390x/cpu_models.c           |  1 +
>>  target/s390x/gen-features.c         |  1 +
>>  target/s390x/kvm.c                  | 31 +++++++++++++++++++++++++++++
>>  target/s390x/machine.c              | 17 ++++++++++++++++
>>  9 files changed, 64 insertions(+)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 87d468087b..ad5d70e14d 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -139,6 +139,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_FAC134,
>> +                            &read_info->fac134);
>> +    }
> 
> Wasn't this feature also possible if there are less than 240 CPUs? Or do
> I mix that up with something else? ... well, maybe it's best anyway if
> we only allow this when ELS is enabled.

Hmmm, looking at the location of fac134 (i.e. offset 134) and the
previous location of the CPU entries (i.e. offset 128), I think I just
mixed this up with something different. So please never mind this question.

 Thomas

Re: [PATCH v5 3/8] s390/sclp: read sccb from mem based on provided length

[Collin Walling]

On 9/10/20 1:56 PM, Collin Walling wrote:
> On 9/10/20 1:50 PM, Thomas Huth wrote:
>> On 10/09/2020 11.36, Collin Walling wrote:
>>> The header contained within the SCCB passed to the SCLP service call
>>> contains the actual length of the SCCB. Instead of allocating a static
>>> 4K size for the work sccb, let's allow for a variable size determined
>>> by the value 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/event-facility.c |  2 +-
>>>  hw/s390x/sclp.c           | 58 ++++++++++++++++++++++-----------------
>>>  include/hw/s390x/sclp.h   |  2 +-
>>>  3 files changed, 35 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
>>> index 645b4080c5..ed92ce510d 100644
>>> --- a/hw/s390x/event-facility.c
>>> +++ b/hw/s390x/event-facility.c
>>> @@ -213,7 +213,7 @@ static uint16_t handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb,
>>>  
>>>      event_buf = &red->ebh;
>>>      event_buf->length = 0;
>>> -    slen = sizeof(sccb->data);
>>> +    slen = sccb_data_len(sccb);
>>>  
>>>      rc = SCLP_RC_NO_EVENT_BUFFERS_STORED;
>>>  
>>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>>> index 69a8724dc7..cb8e2e8ec3 100644
>>> --- a/hw/s390x/sclp.c
>>> +++ b/hw/s390x/sclp.c
>>> @@ -231,25 +231,30 @@ 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);
>>> +    SCCBHeader header;
>>> +    SCCB *work_sccb;
>>
>> I'd maybe use "g_autofree SCCB *work_sccb = NULL" so you don't have to
>> worry about doing the g_free() later.
> 
> Can do.
> 
>>
>>> -    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
>>> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &header, sizeof(SCCBHeader));
>>> +
>>> +    work_sccb = g_malloc0(header.length);
>>
>> Please use be16_to_cpu(header.length) here as well.
> 
> Good catch, thanks!
> 

Shouldn't the mallocs use cpu_to_be16 instead since the header length
was read in from a cpu?

[...]

-- 
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v5 3/8] s390/sclp: read sccb from mem based on provided length

[Thomas Huth]

On 11/09/2020 20.16, Collin Walling wrote:
> On 9/10/20 1:56 PM, Collin Walling wrote:
>> On 9/10/20 1:50 PM, Thomas Huth wrote:
>>> On 10/09/2020 11.36, Collin Walling wrote:
>>>> The header contained within the SCCB passed to the SCLP service call
>>>> contains the actual length of the SCCB. Instead of allocating a static
>>>> 4K size for the work sccb, let's allow for a variable size determined
>>>> by the value 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/event-facility.c |  2 +-
>>>>  hw/s390x/sclp.c           | 58 ++++++++++++++++++++++-----------------
>>>>  include/hw/s390x/sclp.h   |  2 +-
>>>>  3 files changed, 35 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
>>>> index 645b4080c5..ed92ce510d 100644
>>>> --- a/hw/s390x/event-facility.c
>>>> +++ b/hw/s390x/event-facility.c
>>>> @@ -213,7 +213,7 @@ static uint16_t handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb,
>>>>  
>>>>      event_buf = &red->ebh;
>>>>      event_buf->length = 0;
>>>> -    slen = sizeof(sccb->data);
>>>> +    slen = sccb_data_len(sccb);
>>>>  
>>>>      rc = SCLP_RC_NO_EVENT_BUFFERS_STORED;
>>>>  
>>>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>>>> index 69a8724dc7..cb8e2e8ec3 100644
>>>> --- a/hw/s390x/sclp.c
>>>> +++ b/hw/s390x/sclp.c
>>>> @@ -231,25 +231,30 @@ 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);
>>>> +    SCCBHeader header;
>>>> +    SCCB *work_sccb;
>>>
>>> I'd maybe use "g_autofree SCCB *work_sccb = NULL" so you don't have to
>>> worry about doing the g_free() later.
>>
>> Can do.
>>
>>>
>>>> -    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
>>>> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &header, sizeof(SCCBHeader));
>>>> +
>>>> +    work_sccb = g_malloc0(header.length);
>>>
>>> Please use be16_to_cpu(header.length) here as well.
>>
>> Good catch, thanks!
>>
> 
> Shouldn't the mallocs use cpu_to_be16 instead since the header length
> was read in from a cpu?

Now you confuse me ... s390x is big endian, so to get a usable value, we
have to convert big-endian to the host byte order, not the other way
round, don't we?

 Thomas

Re: [PATCH v5 3/8] s390/sclp: read sccb from mem based on provided length

[Collin Walling]

On 9/12/20 2:28 AM, Thomas Huth wrote:
> On 11/09/2020 20.16, Collin Walling wrote:
>> On 9/10/20 1:56 PM, Collin Walling wrote:
>>> On 9/10/20 1:50 PM, Thomas Huth wrote:
>>>> On 10/09/2020 11.36, Collin Walling wrote:
>>>>> The header contained within the SCCB passed to the SCLP service call
>>>>> contains the actual length of the SCCB. Instead of allocating a static
>>>>> 4K size for the work sccb, let's allow for a variable size determined
>>>>> by the value 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/event-facility.c |  2 +-
>>>>>  hw/s390x/sclp.c           | 58 ++++++++++++++++++++++-----------------
>>>>>  include/hw/s390x/sclp.h   |  2 +-
>>>>>  3 files changed, 35 insertions(+), 27 deletions(-)
>>>>>
>>>>> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
>>>>> index 645b4080c5..ed92ce510d 100644
>>>>> --- a/hw/s390x/event-facility.c
>>>>> +++ b/hw/s390x/event-facility.c
>>>>> @@ -213,7 +213,7 @@ static uint16_t handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb,
>>>>>  
>>>>>      event_buf = &red->ebh;
>>>>>      event_buf->length = 0;
>>>>> -    slen = sizeof(sccb->data);
>>>>> +    slen = sccb_data_len(sccb);
>>>>>  
>>>>>      rc = SCLP_RC_NO_EVENT_BUFFERS_STORED;
>>>>>  
>>>>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>>>>> index 69a8724dc7..cb8e2e8ec3 100644
>>>>> --- a/hw/s390x/sclp.c
>>>>> +++ b/hw/s390x/sclp.c
>>>>> @@ -231,25 +231,30 @@ 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);
>>>>> +    SCCBHeader header;
>>>>> +    SCCB *work_sccb;
>>>>
>>>> I'd maybe use "g_autofree SCCB *work_sccb = NULL" so you don't have to
>>>> worry about doing the g_free() later.
>>>
>>> Can do.
>>>
>>>>
>>>>> -    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
>>>>> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &header, sizeof(SCCBHeader));
>>>>> +
>>>>> +    work_sccb = g_malloc0(header.length);
>>>>
>>>> Please use be16_to_cpu(header.length) here as well.
>>>
>>> Good catch, thanks!
>>>
>>
>> Shouldn't the mallocs use cpu_to_be16 instead since the header length
>> was read in from a cpu?
> 
> Now you confuse me ... s390x is big endian, so to get a usable value, we
> have to convert big-endian to the host byte order, not the other way
> round, don't we?
> 
>  Thomas
> 
> 

Err, yes you're right.

-- 
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v5 8/8] s390: guest support for diagnose 0x318

[Collin Walling]

On 9/11/20 11:08 AM, Thomas Huth wrote:
> On 10/09/2020 11.36, Collin Walling wrote:
>> DIAGNOSE 0x318 (diag318) is an s390 instruction that allows the storage
>> of diagnostic information that is collected by the firmware in the case
>> of hardware/firmware service events.
>>
>> QEMU handles the instruction by storing the info in the CPU state. A
>> subsequent register sync will communicate the data to the hypervisor.
>>
>> QEMU handles the migration via a VM State Description.
>>
>> This feature depends on the Extended-Length SCCB (els) feature. If
>> els is not present, then a warning will be printed and the SCLP bit
>> that allows the Linux kernel to execute the instruction will not be
>> set.
>>
>> Availability of this instruction is determined by byte 134 (aka fac134)
>> 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 may not be able to read information regarding all CPUs
>> unless the guest kernel supports an extended-length SCCB.
>>
>> This feature is not supported in protected virtualization mode.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  hw/s390x/sclp.c                     |  5 +++++
>>  include/hw/s390x/sclp.h             |  3 +++
>>  target/s390x/cpu.h                  |  2 ++
>>  target/s390x/cpu_features.h         |  1 +
>>  target/s390x/cpu_features_def.h.inc |  3 +++
>>  target/s390x/cpu_models.c           |  1 +
>>  target/s390x/gen-features.c         |  1 +
>>  target/s390x/kvm.c                  | 31 +++++++++++++++++++++++++++++
>>  target/s390x/machine.c              | 17 ++++++++++++++++
>>  9 files changed, 64 insertions(+)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 87d468087b..ad5d70e14d 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -139,6 +139,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_FAC134,
>> +                            &read_info->fac134);
>> +    }
> 
> Wasn't this feature also possible if there are less than 240 CPUs? Or do
> I mix that up with something else? ... well, maybe it's best anyway if
> we only allow this when ELS is enabled.
> 
>>      read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
>>                                          SCLP_HAS_IOA_RECONFIG);
>>  
>> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
>> index 141e57f765..516f949109 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  fac134;
>> +    uint8_t  _reserved8[144 - 135];     /* 135-143 */
>>      struct CPUEntry entries[];
> 
> Could you please add a comment after entries[] like,
> 
>  /* only here with "ELS", it's at offset 128 otherwise */
> 
> ?
> 
>>  } QEMU_PACKED ReadInfo;
>>  
> [...]
>>  static uint16_t full_GEN12_GA2[] = {
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index a2d5ad78f6..b79feeba9f 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -105,6 +105,7 @@
>>  
>>  #define DIAG_TIMEREVENT                 0x288
>>  #define DIAG_IPL                        0x308
>> +#define DIAG_SET_CONTROL_PROGRAM_CODES  0x318
>>  #define DIAG_KVM_HYPERCALL              0x500
>>  #define DIAG_KVM_BREAKPOINT             0x501
>>  
>> @@ -602,6 +603,11 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>          cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_ETOKEN;
>>      }
>>  
>> +    if (can_sync_regs(cs, KVM_SYNC_DIAG318)) {
>> +        cs->kvm_run->s.regs.diag318 = env->diag318_info;
>> +        cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
>> +    }
>> +
>>      /* Finally the prefix */
>>      if (can_sync_regs(cs, KVM_SYNC_PREFIX)) {
>>          cs->kvm_run->s.regs.prefix = env->psa;
>> @@ -741,6 +747,10 @@ int kvm_arch_get_registers(CPUState *cs)
>>          }
>>      }
>>  
>> +    if (can_sync_regs(cs, KVM_SYNC_DIAG318)) {
>> +        env->diag318_info = cs->kvm_run->s.regs.diag318;
>> +    }
>> +
>>      return 0;
>>  }
>>  
>> @@ -1601,6 +1611,19 @@ static int handle_sw_breakpoint(S390CPU *cpu, struct kvm_run *run)
>>      return -ENOENT;
>>  }
>>  
>> +static void handle_diag_318(S390CPU *cpu, struct kvm_run *run)
>> +{
>> +    uint64_t reg = (run->s390_sieic.ipa & 0x00f0) >> 4;
>> +    uint64_t diag318_info = run->s.regs.gprs[reg];
>> +
>> +    cpu->env.diag318_info = diag318_info;
>> +
>> +    if (can_sync_regs(CPU(cpu), KVM_SYNC_DIAG318)) {
>> +        run->s.regs.diag318 = diag318_info;
>> +        run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
>> +    }
>> +}
> 
> What's supposed to happen if a guest calls diag 318 but the
> S390_FEAT_DIAG_318 has not been enabled? Shouldn't there be a program
> interrupt in that case?
> 
>  Thomas
> 
> 

Right... an edge case where a guest kernel tries to erroneously call
diag 318 without checking the SCLP bit first. I'll add this fence.

-- 
Regards,
Collin

Stay safe and stay healthy