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

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

[Collin Walling]

Changelog:

    v6

    • sccb_verify_boundary function:
        • s/len/sccb_len
        • removed the endian check/conversion of the sccb_len from within 
          this function (caller is now responsible)

    • proper endian conversion when using header length to malloc

    • use g_autofree for work_sccb

    • added r-b's and acks (thanks!)

    • added a feature-check fence within the diag_318_handler to ensure
        the handler does not complete without proper feature support
        • will throw a program exception if handler is invoked but
          feature is not enabled

    

    v5 (comment below pertains to version 5)

    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.

    • 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                     | 142 ++++++++++++++++++++--------
 include/hw/s390x/sclp.h             |  11 ++-
 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                  |  47 +++++++++
 target/s390x/machine.c              |  17 ++++
 12 files changed, 194 insertions(+), 43 deletions(-)

-- 
2.26.2

[PATCH v6 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 v6 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..a37cfbf534 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 sccb_len)
+{
+    uint64_t sccb_max_addr = sccb_addr + sccb_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, be16_to_cpu(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, be16_to_cpu(work_sccb.h.length))) {
         work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
         goto out_write;
     }
-- 
2.26.2

[PATCH v6 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           | 55 ++++++++++++++++++++++-----------------
 include/hw/s390x/sclp.h   |  2 +-
 3 files changed, 33 insertions(+), 26 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 a37cfbf534..4ae6fb400b 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -231,25 +231,29 @@ 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;
+    g_autofree SCCB *work_sccb = NULL;
 
-    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(be16_to_cpu(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, be16_to_cpu(work_sccb.h.length))) {
-        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
+    if (!sccb_verify_boundary(sccb, be16_to_cpu(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));
     sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
     return 0;
 }
@@ -258,9 +262,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;
+    g_autofree SCCB *work_sccb = NULL;
 
     /* first some basic checks on program checks */
     if (env->psw.mask & PSW_MASK_PSTATE) {
@@ -274,32 +277,36 @@ 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(be16_to_cpu(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, be16_to_cpu(work_sccb.h.length))) {
-        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
+    if (!sccb_verify_boundary(sccb, be16_to_cpu(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));
 
     sclp_c->service_interrupt(sclp, sccb);
 
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 v6 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 4ae6fb400b..0d54075309 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 v6 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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.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 0d54075309..1df67c99bf 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 v6 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>
Acked-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.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 1df67c99bf..caf40f41b6 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 sccb_len)
+static bool sccb_verify_boundary(uint64_t sccb_addr, uint16_t sccb_len,
+                                 uint32_t code)
 {
     uint64_t sccb_max_addr = sccb_addr + sccb_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, be16_to_cpu(work_sccb->h.length))) {
+    if (!sccb_verify_boundary(sccb, be16_to_cpu(work_sccb->h.length), code)) {
         work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
         goto out_write;
     }
@@ -302,7 +333,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
         goto out_write;
     }
 
-    if (!sccb_verify_boundary(sccb, be16_to_cpu(work_sccb->h.length))) {
+    if (!sccb_verify_boundary(sccb, be16_to_cpu(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 v6 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 v6 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             |  8 ++++++
 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                  | 39 +++++++++++++++++++++++++++++
 target/s390x/machine.c              | 17 +++++++++++++
 9 files changed, 77 insertions(+)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index caf40f41b6..00f1e4648d 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..c84eb8ac65 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -133,7 +133,15 @@ 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[];
+    /*
+     * When the Extended-Length SCCB (ELS) feature is enabled the
+     * start of the entries field begins at an offset denoted by the
+     * offset_cpu field, otherwise it's at an offset of 128.
+     */
 } QEMU_PACKED ReadInfo;
 
 typedef struct ReadCpuInfo {
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..f13eff688c 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,27 @@ 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];
+
+    /*
+     * DIAG 318 can only be enabled with KVM support. As such, let's
+     * ensure a guest cannot execute this instruction erroneously.
+     */
+    if (!s390_has_feat(S390_FEAT_DIAG_318)) {
+        kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
+    }
+
+    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 +1651,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 +2498,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 v6 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[no-reply]

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



Hi,

This series failed build test on FreeBSD host. Please find the details below.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
if qemu-system-x86_64 --help >/dev/null 2>&1; then
  QEMU=qemu-system-x86_64
elif /usr/libexec/qemu-kvm --help >/dev/null 2>&1; then
  QEMU=/usr/libexec/qemu-kvm
else
  exit 1
fi
make vm-build-freebsd J=21 QEMU=$QEMU
exit 0
=== TEST SCRIPT END ===




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

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

[Cornelia Huck]

On Tue, 15 Sep 2020 12:57:37 -0700 (PDT)
no-reply@patchew.org wrote:

> Patchew URL: https://patchew.org/QEMU/20200915194416.107460-1-walling@linux.ibm.com/
> 
> 
> 
> Hi,
> 
> This series failed build test on FreeBSD host. Please find the details below.
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> # Testing script will be invoked under the git checkout with
> # HEAD pointing to a commit that has the patches applied on top of "base"
> # branch
> if qemu-system-x86_64 --help >/dev/null 2>&1; then
>   QEMU=qemu-system-x86_64
> elif /usr/libexec/qemu-kvm --help >/dev/null 2>&1; then
>   QEMU=/usr/libexec/qemu-kvm
> else
>   exit 1
> fi
> make vm-build-freebsd J=21 QEMU=$QEMU
> exit 0
> === TEST SCRIPT END ===

"fatal: unable to write new index file"

Is patchew out of disk space?

[And it's a bit annoying that the actual error message has been snipped
from the email; is that intentional?]

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

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

[Thomas Huth]

On 15/09/2020 21.44, 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(-)

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

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

[Thomas Huth]

On 15/09/2020 21.44, 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           | 55 ++++++++++++++++++++++-----------------
>  include/hw/s390x/sclp.h   |  2 +-
>  3 files changed, 33 insertions(+), 26 deletions(-)

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

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

[Thomas Huth]

On 15/09/2020 21.44, 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             |  8 ++++++
>  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                  | 39 +++++++++++++++++++++++++++++
>  target/s390x/machine.c              | 17 +++++++++++++
>  9 files changed, 77 insertions(+)

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

(I hope David could still comment on the CPU model stuff ... it looks ok
to me here, but I got that wrong a couple of times in the past already...)

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

[David Hildenbrand]

On 16.09.20 10:11, Thomas Huth wrote:
> On 15/09/2020 21.44, 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             |  8 ++++++
>>  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                  | 39 +++++++++++++++++++++++++++++
>>  target/s390x/machine.c              | 17 +++++++++++++
>>  9 files changed, 77 insertions(+)
> 
> Acked-by: Thomas Huth <thuth@redhat.com>
> 
> (I hope David could still comment on the CPU model stuff ... it looks ok
> to me here, but I got that wrong a couple of times in the past already...)
> 

Had a skim over it and it looks good to me!

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

-- 
Thanks,

David / dhildenb

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

[Cornelia Huck]

On Tue, 15 Sep 2020 15:44:15 -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(-)

I've replaced this with an update against 5.9-rc5.

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

[Cornelia Huck]

On Tue, 15 Sep 2020 15:44:08 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> Changelog:
> 
>     v6
> 
>     • sccb_verify_boundary function:
>         • s/len/sccb_len
>         • removed the endian check/conversion of the sccb_len from within 
>           this function (caller is now responsible)
> 
>     • proper endian conversion when using header length to malloc
> 
>     • use g_autofree for work_sccb
> 
>     • added r-b's and acks (thanks!)
> 
>     • added a feature-check fence within the diag_318_handler to ensure
>         the handler does not complete without proper feature support
>         • will throw a program exception if handler is invoked but
>           feature is not enabled
> 
>     
> 
>     v5 (comment below pertains to version 5)
> 
>     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.
> 
>     • 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                     | 142 ++++++++++++++++++++--------
>  include/hw/s390x/sclp.h             |  11 ++-
>  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                  |  47 +++++++++
>  target/s390x/machine.c              |  17 ++++
>  12 files changed, 194 insertions(+), 43 deletions(-)
> 

Thanks, applied.

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

[Collin Walling]

On 9/16/20 11:52 AM, Cornelia Huck wrote:
> On Tue, 15 Sep 2020 15:44:15 -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(-)
> 
> I've replaced this with an update against 5.9-rc5.
> 
> 

Thanks, Conny

-- 
Regards,
Collin

Stay safe and stay healthy

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

[Collin Walling]

On 9/16/20 3:10 AM, Thomas Huth wrote:
> On 15/09/2020 21.44, 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(-)
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Thanks for the reviews and ack's

-- 
Regards,
Collin

Stay safe and stay healthy

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

[Collin Walling]

On 9/16/20 11:53 AM, Cornelia Huck wrote:

[...]

>>
> 
> Thanks, applied.
> 
> 

Thanks Conny.

Much appreciated for everyone's patience and review. The only thing I'd
like to hold out on for now is for someone to take a peek at patch #3
with respect to the protected virtualization stuff. I don't know too
much about it, honestly, and I want to ensure that dynamically
allocating memory for the SCCB makes sense there. The alternative would
be to allocate a static 4K for the work_sccb.

-- 
Regards,
Collin

Stay safe and stay healthy

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

[Collin Walling]

On 9/16/20 1:15 PM, Collin Walling wrote:
> On 9/16/20 11:53 AM, Cornelia Huck wrote:
> 
> [...]
> 
>>>
>>
>> Thanks, applied.
>>
>>
> 
> Thanks Conny.
> 
> Much appreciated for everyone's patience and review. The only thing I'd
> like to hold out on for now is for someone to take a peek at patch #3
> with respect to the protected virtualization stuff. I don't know too
> much about it, honestly, and I want to ensure that dynamically
> allocating memory for the SCCB makes sense there. The alternative would
> be to allocate a static 4K for the work_sccb.
> 

I had someone take a look at the patch for PV and was told everything
looks sane. Since the patches have already been applied, it seems like
it's too late to add a reviewed-by from someone?

Either way: thanks to everyone for the journey on getting these patches
through!

-- 
Regards,
Collin

Stay safe and stay healthy

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

[Cornelia Huck]

On Fri, 25 Sep 2020 11:13:49 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> On 9/16/20 1:15 PM, Collin Walling wrote:
> > On 9/16/20 11:53 AM, Cornelia Huck wrote:
> > 
> > [...]
> >   
> >>>  
> >>
> >> Thanks, applied.
> >>
> >>  
> > 
> > Thanks Conny.
> > 
> > Much appreciated for everyone's patience and review. The only thing I'd
> > like to hold out on for now is for someone to take a peek at patch #3
> > with respect to the protected virtualization stuff. I don't know too
> > much about it, honestly, and I want to ensure that dynamically
> > allocating memory for the SCCB makes sense there. The alternative would
> > be to allocate a static 4K for the work_sccb.
> >   
> 
> I had someone take a look at the patch for PV and was told everything
> looks sane. Since the patches have already been applied, it seems like
> it's too late to add a reviewed-by from someone?

Have the reviewer reply with their R-b, and I'll happily add it, as I
rebase s390-next before doing a pull req anyway :)

> 
> Either way: thanks to everyone for the journey on getting these patches
> through!
> 

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

[Claudio Imbrenda]

On Fri, 25 Sep 2020 17:18:55 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Fri, 25 Sep 2020 11:13:49 -0400
> Collin Walling <walling@linux.ibm.com> wrote:
> 
> > On 9/16/20 1:15 PM, Collin Walling wrote:  
> > > On 9/16/20 11:53 AM, Cornelia Huck wrote:
> > > 
> > > [...]
> > >     
> > >>>    
> > >>
> > >> Thanks, applied.
> > >>
> > >>    
> > > 
> > > Thanks Conny.
> > > 
> > > Much appreciated for everyone's patience and review. The only
> > > thing I'd like to hold out on for now is for someone to take a
> > > peek at patch #3 with respect to the protected virtualization
> > > stuff. I don't know too much about it, honestly, and I want to
> > > ensure that dynamically allocating memory for the SCCB makes
> > > sense there. The alternative would be to allocate a static 4K for
> > > the work_sccb. 
> > 
> > I had someone take a look at the patch for PV and was told
> > everything looks sane. Since the patches have already been applied,
> > it seems like it's too late to add a reviewed-by from someone?  
> 
> Have the reviewer reply with their R-b, and I'll happily add it, as I
> rebase s390-next before doing a pull req anyway :)

well it was me :)

you can add a 

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

for the first 6 patches, and an

Acked-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

for the last one


thanks!

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

[Cornelia Huck]

On Fri, 25 Sep 2020 17:32:05 +0200
Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:

> On Fri, 25 Sep 2020 17:18:55 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Fri, 25 Sep 2020 11:13:49 -0400
> > Collin Walling <walling@linux.ibm.com> wrote:
> >   
> > > On 9/16/20 1:15 PM, Collin Walling wrote:    
> > > > On 9/16/20 11:53 AM, Cornelia Huck wrote:
> > > > 
> > > > [...]
> > > >       
> > > >>>      
> > > >>
> > > >> Thanks, applied.
> > > >>
> > > >>      
> > > > 
> > > > Thanks Conny.
> > > > 
> > > > Much appreciated for everyone's patience and review. The only
> > > > thing I'd like to hold out on for now is for someone to take a
> > > > peek at patch #3 with respect to the protected virtualization
> > > > stuff. I don't know too much about it, honestly, and I want to
> > > > ensure that dynamically allocating memory for the SCCB makes
> > > > sense there. The alternative would be to allocate a static 4K for
> > > > the work_sccb.   
> > > 
> > > I had someone take a look at the patch for PV and was told
> > > everything looks sane. Since the patches have already been applied,
> > > it seems like it's too late to add a reviewed-by from someone?    
> > 
> > Have the reviewer reply with their R-b, and I'll happily add it, as I
> > rebase s390-next before doing a pull req anyway :)  
> 
> well it was me :)
> 
> you can add a 
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> 
> for the first 6 patches, and an
> 
> Acked-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> 
> for the last one
> 
> 
> thanks!
> 

Thanks, updated and pushed out.