[PATCH v2 00/13] s390x: Protected Virtualization support

[PATCH v2 00/13] s390x: Protected Virtualization support

[Janosch Frank]

Most of the QEMU changes for PV are related to the new IPL type with
subcodes 8 - 10 and the execution of the necessary Ultravisor calls to
IPL secure guests. Note that we can only boot into secure mode from
normal mode, i.e. stfle 161 is not active in secure mode.

The other changes related to data gathering for emulation and
disabling addressing checks in secure mode, as well as CPU resets.

https://github.com/frankjaa/qemu/tree/protvirt

v2:
	* Split out cleanups
	* Internal PV state tracking
	* Review feedback


Janosch Frank (13):
  s390x: protvirt: Add diag308 subcodes 8 - 10
  Header sync protvirt
  s390x: protvirt: Support unpack facility
  s390x: protvirt: Handle diag 308 subcodes 0,1,3,4
  s390x: protvirt: Add pv state to cpu env
  s390x: protvirt: KVM intercept changes
  s390x: protvirt: SCLP interpretation
  s390x: protvirt: Add new VCPU reset functions
  s390x: Exit on vcpu reset error
  s390x: protvirt: Set guest IPL PSW
  s390x: protvirt: Move diag 308 data over SIDAD
  s390x: protvirt: Disable address checks for PV guest IO emulation
  s390x: protvirt: Handle SIGP store status correctly

 hw/s390x/Makefile.objs              |   1 +
 hw/s390x/ipl.c                      |  81 +++++++++++++++++-
 hw/s390x/ipl.h                      |  35 ++++++++
 hw/s390x/pv.c                       | 123 ++++++++++++++++++++++++++++
 hw/s390x/pv.h                       |  27 ++++++
 hw/s390x/s390-virtio-ccw.c          |  59 ++++++++++++-
 hw/s390x/sclp.c                     |  17 ++++
 include/hw/s390x/sclp.h             |   2 +
 linux-headers/linux/kvm.h           |  43 ++++++++++
 target/s390x/cpu.c                  |  23 +++++-
 target/s390x/cpu.h                  |   1 +
 target/s390x/cpu_features_def.inc.h |   1 +
 target/s390x/diag.c                 |  56 +++++++++++--
 target/s390x/helper.c               |   4 +
 target/s390x/ioinst.c               |  26 ++++--
 target/s390x/kvm-stub.c             |  10 ++-
 target/s390x/kvm.c                  |  51 ++++++++++--
 target/s390x/kvm_s390x.h            |   4 +-
 target/s390x/sigp.c                 |   1 +
 19 files changed, 536 insertions(+), 29 deletions(-)
 create mode 100644 hw/s390x/pv.c
 create mode 100644 hw/s390x/pv.h

-- 
2.20.1

[PATCH v2 01/13] s390x: protvirt: Add diag308 subcodes 8 - 10

[Janosch Frank]

For diag308 subcodes 8 - 10 we have a new ipib of type 5. The ipib
holds the address and length of the secure execution header, as well
as a list of guest components.

Each component is a block of memory, for example kernel or initrd,
which needs to be decrypted by the Ultravisor in order to run a
protected VM. The secure execution header instructs the Ultravisor on
how to handle the protected VM and its components.

Subcodes 8 and 9 are similiar to 5 and 6 and subcode 10 will finally
start the protected guest.

Subcodes 8-10 are not valid in protected mode, we have to do a subcode
3 and then the 8 and 10 combination for a protected reboot.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 hw/s390x/ipl.c      | 48 ++++++++++++++++++++++++++++++++++++++++++---
 hw/s390x/ipl.h      | 33 +++++++++++++++++++++++++++++++
 target/s390x/diag.c | 26 ++++++++++++++++++++++--
 3 files changed, 102 insertions(+), 5 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index ca544d64c5..a077926f36 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -529,15 +529,56 @@ static bool is_virtio_scsi_device(IplParameterBlock *iplb)
     return is_virtio_ccw_device_of_type(iplb, VIRTIO_ID_SCSI);
 }
 
+int s390_ipl_pv_check_comp(IplParameterBlock *iplb)
+{
+    int i;
+    IPLBlockPV *ipib_pv = &iplb->pv;
+
+    if (ipib_pv->num_comp == 0) {
+        return -EINVAL;
+    }
+
+    for (i = 0; i < ipib_pv->num_comp; i++) {
+
+        /* Addr must be 4k aligned */
+        if (ipib_pv->components[i].addr & ~TARGET_PAGE_MASK) {
+            return -EINVAL;
+        }
+
+        /* Tweak prefix is monotonously increasing with each component */
+        if (i < ipib_pv->num_comp - 1 &&
+            ipib_pv->components[i].tweak_pref >
+            ipib_pv->components[i + 1].tweak_pref) {
+            return -EINVAL;
+        }
+    }
+    return 1;
+}
+
 void s390_ipl_update_diag308(IplParameterBlock *iplb)
 {
     S390IPLState *ipl = get_ipl_device();
 
-    ipl->iplb = *iplb;
-    ipl->iplb_valid = true;
+    if (iplb->pbt == 5) {
+        ipl->iplb_pbt5 = *iplb;
+        ipl->iplb_valid_pbt5 = true;
+    } else {
+        ipl->iplb = *iplb;
+        ipl->iplb_valid = true;
+    }
     ipl->netboot = is_virtio_net_device(iplb);
 }
 
+IplParameterBlock *s390_ipl_get_iplb_secure(void)
+{
+    S390IPLState *ipl = get_ipl_device();
+
+    if (!ipl->iplb_valid_pbt5) {
+        return NULL;
+    }
+    return &ipl->iplb_pbt5;
+}
+
 IplParameterBlock *s390_ipl_get_iplb(void)
 {
     S390IPLState *ipl = get_ipl_device();
@@ -552,7 +593,8 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type)
 {
     S390IPLState *ipl = get_ipl_device();
 
-    if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL) {
+    if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL ||
+        reset_type == S390_RESET_PV) {
         /* use CPU 0 for full resets */
         ipl->reset_cpu_index = 0;
     } else {
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index d4813105db..7b8a493509 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -15,6 +15,24 @@
 #include "cpu.h"
 #include "hw/qdev-core.h"
 
+struct IPLBlockPVComp {
+    uint64_t tweak_pref;
+    uint64_t addr;
+    uint64_t size;
+} QEMU_PACKED;
+typedef struct IPLBlockPVComp IPLBlockPVComp;
+
+struct IPLBlockPV {
+    uint8_t  reserved[84];
+    uint8_t  reserved67[3];
+    uint8_t  version;
+    uint32_t num_comp;
+    uint64_t pv_header_addr;
+    uint64_t pv_header_len;
+    struct IPLBlockPVComp components[];
+} QEMU_PACKED;
+typedef struct IPLBlockPV IPLBlockPV;
+
 struct IplBlockCcw {
     uint8_t  reserved0[85];
     uint8_t  ssid;
@@ -71,6 +89,7 @@ union IplParameterBlock {
         union {
             IplBlockCcw ccw;
             IplBlockFcp fcp;
+            IPLBlockPV pv;
             IplBlockQemuScsi scsi;
         };
     } QEMU_PACKED;
@@ -84,9 +103,11 @@ union IplParameterBlock {
 typedef union IplParameterBlock IplParameterBlock;
 
 int s390_ipl_set_loadparm(uint8_t *loadparm);
+int s390_ipl_pv_check_comp(IplParameterBlock *iplb);
 void s390_ipl_update_diag308(IplParameterBlock *iplb);
 void s390_ipl_prepare_cpu(S390CPU *cpu);
 IplParameterBlock *s390_ipl_get_iplb(void);
+IplParameterBlock *s390_ipl_get_iplb_secure(void);
 
 enum s390_reset {
     /* default is a reset not triggered by a CPU e.g. issued by QMP */
@@ -94,6 +115,7 @@ enum s390_reset {
     S390_RESET_REIPL,
     S390_RESET_MODIFIED_CLEAR,
     S390_RESET_LOAD_NORMAL,
+    S390_RESET_PV,
 };
 void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type);
 void s390_ipl_get_reset_request(CPUState **cs, enum s390_reset *reset_type);
@@ -133,6 +155,7 @@ struct S390IPLState {
     /*< private >*/
     DeviceState parent_obj;
     IplParameterBlock iplb;
+    IplParameterBlock iplb_pbt5;
     QemuIplParameters qipl;
     uint64_t start_addr;
     uint64_t compat_start_addr;
@@ -140,6 +163,7 @@ struct S390IPLState {
     uint64_t compat_bios_start_addr;
     bool enforce_bios;
     bool iplb_valid;
+    bool iplb_valid_pbt5;
     bool netboot;
     /* reset related properties don't have to be migrated or reset */
     enum s390_reset reset_type;
@@ -161,9 +185,11 @@ QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, "alignment of iplb wrong");
 
 #define S390_IPL_TYPE_FCP 0x00
 #define S390_IPL_TYPE_CCW 0x02
+#define S390_IPL_TYPE_PV 0x05
 #define S390_IPL_TYPE_QEMU_SCSI 0xff
 
 #define S390_IPLB_HEADER_LEN 8
+#define S390_IPLB_MIN_PV_LEN 148
 #define S390_IPLB_MIN_CCW_LEN 200
 #define S390_IPLB_MIN_FCP_LEN 384
 #define S390_IPLB_MIN_QEMU_SCSI_LEN 200
@@ -185,4 +211,11 @@ static inline bool iplb_valid_fcp(IplParameterBlock *iplb)
            iplb->pbt == S390_IPL_TYPE_FCP;
 }
 
+static inline bool iplb_valid_se(IplParameterBlock *iplb)
+{
+    return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_PV_LEN &&
+           iplb->pbt == S390_IPL_TYPE_PV;
+}
+
+
 #endif
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index b5aec06d6b..112a6c92e0 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -52,6 +52,8 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
 #define DIAG_308_RC_OK              0x0001
 #define DIAG_308_RC_NO_CONF         0x0102
 #define DIAG_308_RC_INVALID         0x0402
+#define DIAG_308_RC_NO_PV_CONF      0x0a02
+#define DIAG_308_RC_INV_FOR_PV      0x0b02
 
 #define DIAG308_RESET_MOD_CLR       0
 #define DIAG308_RESET_LOAD_NORM     1
@@ -59,6 +61,9 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
 #define DIAG308_LOAD_NORMAL_DUMP    4
 #define DIAG308_SET                 5
 #define DIAG308_STORE               6
+#define DIAG308_PV_SET              8
+#define DIAG308_PV_STORE            9
+#define DIAG308_PV_START            10
 
 static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
                               uintptr_t ra, bool write)
@@ -105,6 +110,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
         s390_ipl_reset_request(cs, S390_RESET_REIPL);
         break;
     case DIAG308_SET:
+    case DIAG308_PV_SET:
         if (diag308_parm_check(env, r1, addr, ra, false)) {
             return;
         }
@@ -117,7 +123,8 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
 
         cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
 
-        if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb)) {
+        if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb) &&
+            !(iplb_valid_se(iplb) && s390_ipl_pv_check_comp(iplb) >= 0)) {
             env->regs[r1 + 1] = DIAG_308_RC_INVALID;
             goto out;
         }
@@ -128,10 +135,15 @@ out:
         g_free(iplb);
         return;
     case DIAG308_STORE:
+    case DIAG308_PV_STORE:
         if (diag308_parm_check(env, r1, addr, ra, true)) {
             return;
         }
-        iplb = s390_ipl_get_iplb();
+        if (subcode == DIAG308_PV_STORE) {
+            iplb = s390_ipl_get_iplb_secure();
+        } else {
+            iplb = s390_ipl_get_iplb();
+        }
         if (iplb) {
             cpu_physical_memory_write(addr, iplb, be32_to_cpu(iplb->len));
             env->regs[r1 + 1] = DIAG_308_RC_OK;
@@ -139,6 +151,16 @@ out:
             env->regs[r1 + 1] = DIAG_308_RC_NO_CONF;
         }
         return;
+        break;
+    case DIAG308_PV_START:
+        iplb = s390_ipl_get_iplb_secure();
+        if (!iplb_valid_se(iplb)) {
+            env->regs[r1 + 1] = DIAG_308_RC_NO_PV_CONF;
+            return;
+        }
+
+        s390_ipl_reset_request(cs, S390_RESET_PV);
+        break;
     default:
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);
         break;
-- 
2.20.1

Re: [PATCH v2 01/13] s390x: protvirt: Add diag308 subcodes 8 - 10

[David Hildenbrand]

[...]
>  
> +struct IPLBlockPVComp {
> +    uint64_t tweak_pref;
> +    uint64_t addr;
> +    uint64_t size;
> +} QEMU_PACKED;

QEMU_PACKED  should not be needed.

> +typedef struct IPLBlockPVComp IPLBlockPVComp;
> +
> +struct IPLBlockPV {
> +    uint8_t  reserved[84];

"reserved0"

> +    uint8_t  reserved67[3];

Where does that magic 67 come from? (84 dec is 54 hex)

> +    uint8_t  version;

So, to this point we spent 88 bytes == 11 * uint64_t.

> +    uint32_t num_comp;

... so after this, all uint64_t (and components) are mis-aligned by
32bit - is that correct?

> +    uint64_t pv_header_addr;
> +    uint64_t pv_header_len;
> +    struct IPLBlockPVComp components[];
> +} QEMU_PACKED;
> +typedef struct IPLBlockPV IPLBlockPV;
> +
>  struct IplBlockCcw {
>      uint8_t  reserved0[85];
>      uint8_t  ssid;
> @@ -71,6 +89,7 @@ union IplParameterBlock {
>          union {
>              IplBlockCcw ccw;
>              IplBlockFcp fcp;
> +            IPLBlockPV pv;
>              IplBlockQemuScsi scsi;
>          };
>      } QEMU_PACKED;
> @@ -84,9 +103,11 @@ union IplParameterBlock {
>  typedef union IplParameterBlock IplParameterBlock;
>  
>  int s390_ipl_set_loadparm(uint8_t *loadparm);
> +int s390_ipl_pv_check_comp(IplParameterBlock *iplb);
>  void s390_ipl_update_diag308(IplParameterBlock *iplb);
>  void s390_ipl_prepare_cpu(S390CPU *cpu);
>  IplParameterBlock *s390_ipl_get_iplb(void);
> +IplParameterBlock *s390_ipl_get_iplb_secure(void);
>  
>  enum s390_reset {
>      /* default is a reset not triggered by a CPU e.g. issued by QMP */
> @@ -94,6 +115,7 @@ enum s390_reset {
>      S390_RESET_REIPL,
>      S390_RESET_MODIFIED_CLEAR,
>      S390_RESET_LOAD_NORMAL,
> +    S390_RESET_PV,

I do wonder if that should be called S390_RESET_PV_START

>  };
>  void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type);
>  void s390_ipl_get_reset_request(CPUState **cs, enum s390_reset *reset_type);
> @@ -133,6 +155,7 @@ struct S390IPLState {
>      /*< private >*/
>      DeviceState parent_obj;
>      IplParameterBlock iplb;
> +    IplParameterBlock iplb_pbt5;
>      QemuIplParameters qipl;
>      uint64_t start_addr;
>      uint64_t compat_start_addr;
> @@ -140,6 +163,7 @@ struct S390IPLState {
>      uint64_t compat_bios_start_addr;
>      bool enforce_bios;
>      bool iplb_valid;
> +    bool iplb_valid_pbt5;
>      bool netboot;
>      /* reset related properties don't have to be migrated or reset */
>      enum s390_reset reset_type;
> @@ -161,9 +185,11 @@ QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, "alignment of iplb wrong");
>  
>  #define S390_IPL_TYPE_FCP 0x00
>  #define S390_IPL_TYPE_CCW 0x02
> +#define S390_IPL_TYPE_PV 0x05
>  #define S390_IPL_TYPE_QEMU_SCSI 0xff
>  
>  #define S390_IPLB_HEADER_LEN 8
> +#define S390_IPLB_MIN_PV_LEN 148
>  #define S390_IPLB_MIN_CCW_LEN 200
>  #define S390_IPLB_MIN_FCP_LEN 384
>  #define S390_IPLB_MIN_QEMU_SCSI_LEN 200
> @@ -185,4 +211,11 @@ static inline bool iplb_valid_fcp(IplParameterBlock *iplb)
>             iplb->pbt == S390_IPL_TYPE_FCP;
>  }
>  
> +static inline bool iplb_valid_se(IplParameterBlock *iplb)

s/_se/_pv/ ? Or was that intended?

> +{
> +    return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_PV_LEN &&
> +           iplb->pbt == S390_IPL_TYPE_PV;
> +}
> +
> +

Maybe drop one empty line

>  #endif
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index b5aec06d6b..112a6c92e0 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
[...]


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 01/13] s390x: protvirt: Add diag308 subcodes 8 - 10

[Janosch Frank]

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

On 11/29/19 11:09 AM, David Hildenbrand wrote:
> [...]
>>  
>> +struct IPLBlockPVComp {
>> +    uint64_t tweak_pref;
>> +    uint64_t addr;
>> +    uint64_t size;
>> +} QEMU_PACKED;
> 
> QEMU_PACKED  should not be needed.
> 
>> +typedef struct IPLBlockPVComp IPLBlockPVComp;
>> +
>> +struct IPLBlockPV {
>> +    uint8_t  reserved[84];
> 
> "reserved0"
> 
>> +    uint8_t  reserved67[3];
> 
> Where does that magic 67 come from? (84 dec is 54 hex)
> 
>> +    uint8_t  version;
> 
> So, to this point we spent 88 bytes == 11 * uint64_t.

I'll have a look

> 
>> +    uint32_t num_comp;
> 
> ... so after this, all uint64_t (and components) are mis-aligned by
> 32bit - is that correct?
> 
>> +    uint64_t pv_header_addr;
>> +    uint64_t pv_header_len;
>> +    struct IPLBlockPVComp components[];
>> +} QEMU_PACKED;
>> +typedef struct IPLBlockPV IPLBlockPV;
>> +
>>  struct IplBlockCcw {
>>      uint8_t  reserved0[85];
>>      uint8_t  ssid;
>> @@ -71,6 +89,7 @@ union IplParameterBlock {
>>          union {
>>              IplBlockCcw ccw;
>>              IplBlockFcp fcp;
>> +            IPLBlockPV pv;
>>              IplBlockQemuScsi scsi;
>>          };
>>      } QEMU_PACKED;
>> @@ -84,9 +103,11 @@ union IplParameterBlock {
>>  typedef union IplParameterBlock IplParameterBlock;
>>  
>>  int s390_ipl_set_loadparm(uint8_t *loadparm);
>> +int s390_ipl_pv_check_comp(IplParameterBlock *iplb);
>>  void s390_ipl_update_diag308(IplParameterBlock *iplb);
>>  void s390_ipl_prepare_cpu(S390CPU *cpu);
>>  IplParameterBlock *s390_ipl_get_iplb(void);
>> +IplParameterBlock *s390_ipl_get_iplb_secure(void);
>>  
>>  enum s390_reset {
>>      /* default is a reset not triggered by a CPU e.g. issued by QMP */
>> @@ -94,6 +115,7 @@ enum s390_reset {
>>      S390_RESET_REIPL,
>>      S390_RESET_MODIFIED_CLEAR,
>>      S390_RESET_LOAD_NORMAL,
>> +    S390_RESET_PV,
> 
> I do wonder if that should be called S390_RESET_PV_START

I have no strong feelings for the name, whatever floats you boat(s)

> 
>>  };
>>  void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type);
>>  void s390_ipl_get_reset_request(CPUState **cs, enum s390_reset *reset_type);
>> @@ -133,6 +155,7 @@ struct S390IPLState {
>>      /*< private >*/
>>      DeviceState parent_obj;
>>      IplParameterBlock iplb;
>> +    IplParameterBlock iplb_pbt5;
>>      QemuIplParameters qipl;
>>      uint64_t start_addr;
>>      uint64_t compat_start_addr;
>> @@ -140,6 +163,7 @@ struct S390IPLState {
>>      uint64_t compat_bios_start_addr;
>>      bool enforce_bios;
>>      bool iplb_valid;
>> +    bool iplb_valid_pbt5;
>>      bool netboot;
>>      /* reset related properties don't have to be migrated or reset */
>>      enum s390_reset reset_type;
>> @@ -161,9 +185,11 @@ QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, "alignment of iplb wrong");
>>  
>>  #define S390_IPL_TYPE_FCP 0x00
>>  #define S390_IPL_TYPE_CCW 0x02
>> +#define S390_IPL_TYPE_PV 0x05
>>  #define S390_IPL_TYPE_QEMU_SCSI 0xff
>>  
>>  #define S390_IPLB_HEADER_LEN 8
>> +#define S390_IPLB_MIN_PV_LEN 148
>>  #define S390_IPLB_MIN_CCW_LEN 200
>>  #define S390_IPLB_MIN_FCP_LEN 384
>>  #define S390_IPLB_MIN_QEMU_SCSI_LEN 200
>> @@ -185,4 +211,11 @@ static inline bool iplb_valid_fcp(IplParameterBlock *iplb)
>>             iplb->pbt == S390_IPL_TYPE_FCP;
>>  }
>>  
>> +static inline bool iplb_valid_se(IplParameterBlock *iplb)
> 
> s/_se/_pv/ ? Or was that intended?

Not intended, the rename in the middle of the project took some tolls.

> 
>> +{
>> +    return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_PV_LEN &&
>> +           iplb->pbt == S390_IPL_TYPE_PV;
>> +}
>> +
>> +
> 
> Maybe drop one empty line

Sure

> 
>>  #endif
>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>> index b5aec06d6b..112a6c92e0 100644
>> --- a/target/s390x/diag.c
>> +++ b/target/s390x/diag.c
> [...]
> 
> 



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

Re: [PATCH v2 01/13] s390x: protvirt: Add diag308 subcodes 8 - 10

[Cornelia Huck]

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

On Fri, 29 Nov 2019 12:18:56 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 11/29/19 11:09 AM, David Hildenbrand wrote:

> >> @@ -94,6 +115,7 @@ enum s390_reset {
> >>      S390_RESET_REIPL,
> >>      S390_RESET_MODIFIED_CLEAR,
> >>      S390_RESET_LOAD_NORMAL,
> >> +    S390_RESET_PV,  
> > 
> > I do wonder if that should be called S390_RESET_PV_START  
> 
> I have no strong feelings for the name, whatever floats you boat(s)

PVY_MC_PVFACE?

But seriously speaking, I'd also prefer S390_RESET_PV_START.

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

Re: [PATCH v2 01/13] s390x: protvirt: Add diag308 subcodes 8 - 10

[Thomas Huth]

On 29/11/2019 10.47, Janosch Frank wrote:
[...]
> Subcodes 8-10 are not valid in protected mode, we have to do a subcode
> 3 and then the 8 and 10 combination for a protected reboot.

So if 8-10 are not valid in protected mode...

> @@ -59,6 +61,9 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
>  #define DIAG308_LOAD_NORMAL_DUMP    4
>  #define DIAG308_SET                 5
>  #define DIAG308_STORE               6
> +#define DIAG308_PV_SET              8
> +#define DIAG308_PV_STORE            9
> +#define DIAG308_PV_START            10
>  
>  static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
>                                uintptr_t ra, bool write)
> @@ -105,6 +110,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>          s390_ipl_reset_request(cs, S390_RESET_REIPL);
>          break;
>      case DIAG308_SET:
> +    case DIAG308_PV_SET:

... should you maybe add a check here (and the other cases) to make sure
that the guest is currently not running in PV mode? Or is this taken
care of by the Ultravisor already?

>          if (diag308_parm_check(env, r1, addr, ra, false)) {
>              return;
>          }
> @@ -117,7 +123,8 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>  
>          cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
>  
> -        if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb)) {
> +        if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb) &&
> +            !(iplb_valid_se(iplb) && s390_ipl_pv_check_comp(iplb) >= 0)) {
>              env->regs[r1 + 1] = DIAG_308_RC_INVALID;
>              goto out;
>          }
> @@ -128,10 +135,15 @@ out:
>          g_free(iplb);
>          return;
>      case DIAG308_STORE:
> +    case DIAG308_PV_STORE:
>          if (diag308_parm_check(env, r1, addr, ra, true)) {
>              return;
>          }
> -        iplb = s390_ipl_get_iplb();
> +        if (subcode == DIAG308_PV_STORE) {
> +            iplb = s390_ipl_get_iplb_secure();
> +        } else {
> +            iplb = s390_ipl_get_iplb();
> +        }
>          if (iplb) {
>              cpu_physical_memory_write(addr, iplb, be32_to_cpu(iplb->len));
>              env->regs[r1 + 1] = DIAG_308_RC_OK;
> @@ -139,6 +151,16 @@ out:
>              env->regs[r1 + 1] = DIAG_308_RC_NO_CONF;
>          }
>          return;
> +        break;

Please remove the break. Or the return. But let's not do both.

> +    case DIAG308_PV_START:
> +        iplb = s390_ipl_get_iplb_secure();
> +        if (!iplb_valid_se(iplb)) {
> +            env->regs[r1 + 1] = DIAG_308_RC_NO_PV_CONF;
> +            return;
> +        }
> +
> +        s390_ipl_reset_request(cs, S390_RESET_PV);
> +        break;
>      default:
>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>          break;
> 

 Thomas

Re: [PATCH v2 01/13] s390x: protvirt: Add diag308 subcodes 8 - 10

[Janosch Frank]

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

On 11/29/19 1:40 PM, Thomas Huth wrote:
> On 29/11/2019 10.47, Janosch Frank wrote:
> [...]
>> Subcodes 8-10 are not valid in protected mode, we have to do a subcode
>> 3 and then the 8 and 10 combination for a protected reboot.
> 
> So if 8-10 are not valid in protected mode...
> 
>> @@ -59,6 +61,9 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
>>  #define DIAG308_LOAD_NORMAL_DUMP    4
>>  #define DIAG308_SET                 5
>>  #define DIAG308_STORE               6
>> +#define DIAG308_PV_SET              8
>> +#define DIAG308_PV_STORE            9
>> +#define DIAG308_PV_START            10
>>  
>>  static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
>>                                uintptr_t ra, bool write)
>> @@ -105,6 +110,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>>          s390_ipl_reset_request(cs, S390_RESET_REIPL);
>>          break;
>>      case DIAG308_SET:
>> +    case DIAG308_PV_SET:
> 
> ... should you maybe add a check here (and the other cases) to make sure
> that the guest is currently not running in PV mode? Or is this taken
> care of by the Ultravisor already?

The Ultravisor takes care of that.

> 
>>          if (diag308_parm_check(env, r1, addr, ra, false)) {
>>              return;
>>          }
>> @@ -117,7 +123,8 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>>  
>>          cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
>>  
>> -        if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb)) {
>> +        if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb) &&
>> +            !(iplb_valid_se(iplb) && s390_ipl_pv_check_comp(iplb) >= 0)) {
>>              env->regs[r1 + 1] = DIAG_308_RC_INVALID;
>>              goto out;
>>          }
>> @@ -128,10 +135,15 @@ out:
>>          g_free(iplb);
>>          return;
>>      case DIAG308_STORE:
>> +    case DIAG308_PV_STORE:
>>          if (diag308_parm_check(env, r1, addr, ra, true)) {
>>              return;
>>          }
>> -        iplb = s390_ipl_get_iplb();
>> +        if (subcode == DIAG308_PV_STORE) {
>> +            iplb = s390_ipl_get_iplb_secure();
>> +        } else {
>> +            iplb = s390_ipl_get_iplb();
>> +        }
>>          if (iplb) {
>>              cpu_physical_memory_write(addr, iplb, be32_to_cpu(iplb->len));
>>              env->regs[r1 + 1] = DIAG_308_RC_OK;
>> @@ -139,6 +151,16 @@ out:
>>              env->regs[r1 + 1] = DIAG_308_RC_NO_CONF;
>>          }
>>          return;
>> +        break;
> 
> Please remove the break. Or the return. But let's not do both.

Right, I forgot to remove that...

> 
>> +    case DIAG308_PV_START:
>> +        iplb = s390_ipl_get_iplb_secure();
>> +        if (!iplb_valid_se(iplb)) {
>> +            env->regs[r1 + 1] = DIAG_308_RC_NO_PV_CONF;
>> +            return;
>> +        }
>> +
>> +        s390_ipl_reset_request(cs, S390_RESET_PV);
>> +        break;
>>      default:
>>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>>          break;
>>
> 
>  Thomas
> 



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

Re: [PATCH v2 01/13] s390x: protvirt: Add diag308 subcodes 8 - 10

[Cornelia Huck]

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

On Fri, 29 Nov 2019 15:08:58 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 11/29/19 1:40 PM, Thomas Huth wrote:
> > On 29/11/2019 10.47, Janosch Frank wrote:
> > [...]  
> >> Subcodes 8-10 are not valid in protected mode, we have to do a subcode
> >> 3 and then the 8 and 10 combination for a protected reboot.  
> > 
> > So if 8-10 are not valid in protected mode...
> >   
> >> @@ -59,6 +61,9 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
> >>  #define DIAG308_LOAD_NORMAL_DUMP    4
> >>  #define DIAG308_SET                 5
> >>  #define DIAG308_STORE               6
> >> +#define DIAG308_PV_SET              8
> >> +#define DIAG308_PV_STORE            9
> >> +#define DIAG308_PV_START            10
> >>  
> >>  static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
> >>                                uintptr_t ra, bool write)
> >> @@ -105,6 +110,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
> >>          s390_ipl_reset_request(cs, S390_RESET_REIPL);
> >>          break;
> >>      case DIAG308_SET:
> >> +    case DIAG308_PV_SET:  
> > 
> > ... should you maybe add a check here (and the other cases) to make sure
> > that the guest is currently not running in PV mode? Or is this taken
> > care of by the Ultravisor already?  
> 
> The Ultravisor takes care of that.

I'm wondering whether we should add some asserts. If the uv is broken,
we're hosed anyway; but it might make the code flow more obvious?

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

[PATCH v2 02/13] Header sync protvirt

[Janosch Frank]

Let's sync all the protvirt header changes

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 linux-headers/linux/kvm.h | 42 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 3d9b18f7f8..4448d59960 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1000,6 +1000,8 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_PMU_EVENT_FILTER 173
 #define KVM_CAP_ARM_IRQ_LINE_LAYOUT_2 174
 #define KVM_CAP_HYPERV_DIRECT_TLBFLUSH 175
+#define KVM_CAP_S390_PROTECTED 180
+#define KVM_CAP_S390_VCPU_RESETS 181
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -1461,6 +1463,46 @@ struct kvm_enc_region {
 /* Available with KVM_CAP_ARM_SVE */
 #define KVM_ARM_VCPU_FINALIZE	  _IOW(KVMIO,  0xc2, int)
 
+struct kvm_s390_pv_sec_parm {
+	__u64	origin;
+	__u64	length;
+};
+
+struct kvm_s390_pv_unp {
+	__u64 addr;
+	__u64 size;
+	__u64 tweak;
+};
+
+enum pv_cmd_id {
+	KVM_PV_VM_CREATE,
+	KVM_PV_VM_DESTROY,
+	KVM_PV_VM_SET_SEC_PARMS,
+	KVM_PV_VM_UNPACK,
+	KVM_PV_VM_VERIFY,
+	KVM_PV_VM_PERF_CLEAR_RESET,
+	KVM_PV_VM_UNSHARE,
+	KVM_PV_VCPU_CREATE,
+	KVM_PV_VCPU_DESTROY,
+};
+
+struct kvm_pv_cmd {
+	__u32	cmd;
+	__u16	rc;
+	__u16	rrc;
+	__u64	data;
+};
+
+/* Available with KVM_CAP_S390_PROTECTED */
+#define KVM_S390_PV_COMMAND		_IOW(KVMIO, 0xc3, struct kvm_pv_cmd)
+#define KVM_S390_PV_COMMAND_VCPU	_IOW(KVMIO, 0xc4, struct kvm_pv_cmd)
+
+#define KVM_S390_VCPU_RESET_NORMAL	0
+#define KVM_S390_VCPU_RESET_INITIAL	1
+#define KVM_S390_VCPU_RESET_CLEAR	2
+
+#define KVM_S390_VCPU_RESET    _IO(KVMIO,   0xc5)
+
 /* Secure Encrypted Virtualization command */
 enum sev_cmd_id {
 	/* Guest initialization commands */
-- 
2.20.1

[PATCH v2 03/13] s390x: protvirt: Support unpack facility

[Janosch Frank]

When a guest has saved a ipib of type 5 and call diagnose308 with
subcode 10, we have to setup the protected processing environment via
Ultravisor calls. The calls are done by KVM and are exposed via an API.

The following steps are necessary:
1. Create a VM (register it with the Ultravisor)
2. Create secure CPUs for all of our current cpus
3. Forward the secure header to the Ultravisor (has all information on
how to decrypt the image and VM information)
4. Protect image pages from the host and decrypt them
5. Verify the image integrity

Only after step 5 a protected VM is allowed to run.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 hw/s390x/Makefile.objs              |   1 +
 hw/s390x/ipl.c                      |  33 ++++++++
 hw/s390x/ipl.h                      |   2 +
 hw/s390x/pv.c                       | 118 ++++++++++++++++++++++++++++
 hw/s390x/pv.h                       |  26 ++++++
 hw/s390x/s390-virtio-ccw.c          |  26 ++++++
 target/s390x/cpu_features_def.inc.h |   1 +
 7 files changed, 207 insertions(+)
 create mode 100644 hw/s390x/pv.c
 create mode 100644 hw/s390x/pv.h

diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index e02ed80b68..a46a1c7894 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -31,6 +31,7 @@ obj-y += tod-qemu.o
 obj-$(CONFIG_KVM) += tod-kvm.o
 obj-$(CONFIG_KVM) += s390-skeys-kvm.o
 obj-$(CONFIG_KVM) += s390-stattrib-kvm.o
+obj-$(CONFIG_KVM) += pv.o
 obj-y += s390-ccw.o
 obj-y += ap-device.o
 obj-y += ap-bridge.o
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index a077926f36..50501fcd27 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -33,6 +33,7 @@
 #include "qemu/cutils.h"
 #include "qemu/option.h"
 #include "exec/exec-all.h"
+#include "pv.h"
 
 #define KERN_IMAGE_START                0x010000UL
 #define LINUX_MAGIC_ADDR                0x010008UL
@@ -668,6 +669,38 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
     cpu_physical_memory_unmap(addr, len, 1, len);
 }
 
+int s390_ipl_prepare_pv_header(void)
+{
+    int rc;
+    IplParameterBlock *iplb = s390_ipl_get_iplb_secure();
+    IPLBlockPV *ipib_pv = &iplb->pv;
+    void *hdr = g_malloc(ipib_pv->pv_header_len);
+
+    cpu_physical_memory_read(ipib_pv->pv_header_addr, hdr,
+                             ipib_pv->pv_header_len);
+    rc = s390_pv_set_sec_parms((uint64_t)hdr,
+                               ipib_pv->pv_header_len);
+    g_free(hdr);
+    return rc;
+}
+
+int s390_ipl_pv_unpack(void)
+{
+    int i, rc;
+    IplParameterBlock *iplb = s390_ipl_get_iplb_secure();
+    IPLBlockPV *ipib_pv = &iplb->pv;
+
+    for (i = 0; i < ipib_pv->num_comp; i++) {
+        rc = s390_pv_unpack(ipib_pv->components[i].addr,
+                            TARGET_PAGE_ALIGN(ipib_pv->components[i].size),
+                            ipib_pv->components[i].tweak_pref);
+        if (rc) {
+            return rc;
+        }
+    }
+    return rc;
+}
+
 void s390_ipl_prepare_cpu(S390CPU *cpu)
 {
     S390IPLState *ipl = get_ipl_device();
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index 7b8a493509..e848602c16 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -105,6 +105,8 @@ typedef union IplParameterBlock IplParameterBlock;
 int s390_ipl_set_loadparm(uint8_t *loadparm);
 int s390_ipl_pv_check_comp(IplParameterBlock *iplb);
 void s390_ipl_update_diag308(IplParameterBlock *iplb);
+int s390_ipl_prepare_pv_header(void);
+int s390_ipl_pv_unpack(void);
 void s390_ipl_prepare_cpu(S390CPU *cpu);
 IplParameterBlock *s390_ipl_get_iplb(void);
 IplParameterBlock *s390_ipl_get_iplb_secure(void);
diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
new file mode 100644
index 0000000000..0218070322
--- /dev/null
+++ b/hw/s390x/pv.c
@@ -0,0 +1,118 @@
+/*
+ * Secure execution functions
+ *
+ * Copyright IBM Corp. 2019
+ * Author(s):
+ *  Janosch Frank <frankja@linux.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#include "qemu/osdep.h"
+#include <sys/ioctl.h>
+
+#include <linux/kvm.h>
+
+#include "qemu/error-report.h"
+#include "sysemu/kvm.h"
+#include "pv.h"
+
+static int s390_pv_cmd(uint32_t cmd, void *data)
+{
+    int rc;
+    struct kvm_pv_cmd pv_cmd = {
+        .cmd = cmd,
+        .data = (uint64_t)data,
+    };
+
+    rc = kvm_vm_ioctl(kvm_state, KVM_S390_PV_COMMAND, &pv_cmd);
+    if (rc) {
+        error_report("KVM PV command failed cmd: %d rc: %d", cmd, rc);
+        exit(1);
+    }
+    return rc;
+}
+
+static int s390_pv_cmd_vcpu(CPUState *cs, uint32_t cmd, void *data)
+{
+    int rc;
+    struct kvm_pv_cmd pv_cmd = {
+        .cmd = cmd,
+        .data = (uint64_t)data,
+    };
+
+    rc = kvm_vcpu_ioctl(cs, KVM_S390_PV_COMMAND_VCPU, &pv_cmd);
+    if (rc) {
+        error_report("KVM PV VCPU command failed cmd: %d rc: %d", cmd, rc);
+        exit(1);
+    }
+    return rc;
+}
+
+int s390_pv_vm_create(void)
+{
+    return s390_pv_cmd(KVM_PV_VM_CREATE, NULL);
+}
+
+int s390_pv_vm_destroy(void)
+{
+    return s390_pv_cmd(KVM_PV_VM_DESTROY, NULL);
+}
+
+int s390_pv_vcpu_create(CPUState *cs)
+{
+    return s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_CREATE, NULL);
+}
+
+int s390_pv_vcpu_destroy(CPUState *cs)
+{
+    S390CPU *cpu = S390_CPU(cs);
+    CPUS390XState *env = &cpu->env;
+    int rc;
+
+    rc = s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_DESTROY, NULL);
+    if (!rc) {
+        env->pv = false;
+    }
+    return rc;
+}
+
+int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
+{
+    struct kvm_s390_pv_sec_parm args = {
+        .origin = origin,
+        .length = length,
+    };
+
+    return s390_pv_cmd(KVM_PV_VM_SET_SEC_PARMS, &args);
+}
+
+/*
+ * Called for each component in the SE type IPL parameter block 0.
+ */
+int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
+{
+    struct kvm_s390_pv_unp args = {
+        .addr = addr,
+        .size = size,
+        .tweak = tweak,
+    };
+
+    return s390_pv_cmd(KVM_PV_VM_UNPACK, &args);
+}
+
+int s390_pv_perf_clear_reset(void)
+{
+    return s390_pv_cmd(KVM_PV_VM_PERF_CLEAR_RESET, NULL);
+}
+
+int s390_pv_verify(void)
+{
+    return s390_pv_cmd(KVM_PV_VM_VERIFY, NULL);
+}
+
+int s390_pv_unshare(void)
+{
+    return s390_pv_cmd(KVM_PV_VM_UNSHARE, NULL);
+}
diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
new file mode 100644
index 0000000000..eb074e4bc9
--- /dev/null
+++ b/hw/s390x/pv.h
@@ -0,0 +1,26 @@
+/*
+ * Protected Virtualization header
+ *
+ * Copyright IBM Corp. 2019
+ * Author(s):
+ *  Janosch Frank <frankja@linux.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#ifndef HW_S390_PV_H
+#define HW_S390_PV_H
+
+int s390_pv_vm_create(void);
+int s390_pv_vm_destroy(void);
+int s390_pv_vcpu_destroy(CPUState *cs);
+int s390_pv_vcpu_create(CPUState *cs);
+int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
+int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
+int s390_pv_perf_clear_reset(void);
+int s390_pv_verify(void);
+int s390_pv_unshare(void);
+
+#endif /* HW_S390_PV_H */
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index c1d1440272..f9481ccace 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -41,6 +41,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/s390x/tod.h"
 #include "sysemu/sysemu.h"
+#include "hw/s390x/pv.h"
 
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 {
@@ -322,6 +323,7 @@ static void s390_machine_reset(MachineState *machine)
 {
     enum s390_reset reset_type;
     CPUState *cs, *t;
+    S390CPU *cpu;
 
     /* get the reset parameters, reset them once done */
     s390_ipl_get_reset_request(&cs, &reset_type);
@@ -329,6 +331,8 @@ static void s390_machine_reset(MachineState *machine)
     /* all CPUs are paused and synchronized at this point */
     s390_cmma_reset();
 
+    cpu = S390_CPU(cs);
+
     switch (reset_type) {
     case S390_RESET_EXTERNAL:
     case S390_RESET_REIPL:
@@ -357,6 +361,28 @@ static void s390_machine_reset(MachineState *machine)
         run_on_cpu(cs, s390_do_cpu_initial_reset, RUN_ON_CPU_NULL);
         run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
         break;
+    case S390_RESET_PV: /* Subcode 10 */
+        subsystem_reset();
+        s390_crypto_reset();
+
+        CPU_FOREACH(t) {
+            run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
+        }
+
+        /* Create SE VM */
+        s390_pv_vm_create();
+        CPU_FOREACH(t) {
+            s390_pv_vcpu_create(t);
+        }
+
+        /* Set SE header and unpack */
+        s390_ipl_prepare_pv_header();
+        /* Decrypt image */
+        s390_ipl_pv_unpack();
+        /* Verify integrity */
+        s390_pv_verify();
+        s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
+        break;
     default:
         g_assert_not_reached();
     }
diff --git a/target/s390x/cpu_features_def.inc.h b/target/s390x/cpu_features_def.inc.h
index 31dff0d84e..60db28351d 100644
--- a/target/s390x/cpu_features_def.inc.h
+++ b/target/s390x/cpu_features_def.inc.h
@@ -107,6 +107,7 @@ DEF_FEAT(DEFLATE_BASE, "deflate-base", STFL, 151, "Deflate-conversion facility (
 DEF_FEAT(VECTOR_PACKED_DECIMAL_ENH, "vxpdeh", STFL, 152, "Vector-Packed-Decimal-Enhancement Facility")
 DEF_FEAT(MSA_EXT_9, "msa9-base", STFL, 155, "Message-security-assist-extension-9 facility (excluding subfunctions)")
 DEF_FEAT(ETOKEN, "etoken", STFL, 156, "Etoken facility")
+DEF_FEAT(UNPACK, "unpack", STFL, 161, "Unpack facility")
 
 /* Features exposed via SCLP SCCB Byte 80 - 98  (bit numbers relative to byte-80) */
 DEF_FEAT(SIE_GSLS, "gsls", SCLP_CONF_CHAR, 40, "SIE: Guest-storage-limit-suppression facility")
-- 
2.20.1

Re: [PATCH v2 03/13] s390x: protvirt: Support unpack facility

[David Hildenbrand]

On 29.11.19 10:47, Janosch Frank wrote:
> When a guest has saved a ipib of type 5 and call diagnose308 with
> subcode 10, we have to setup the protected processing environment via
> Ultravisor calls. The calls are done by KVM and are exposed via an API.
> 
> The following steps are necessary:
> 1. Create a VM (register it with the Ultravisor)
> 2. Create secure CPUs for all of our current cpus

I do wonder why KVM can't handle that when switching to the encrypted
VM. Any specific reason QEMU has to be involved?

I would have guessed s390_pv_vm_create() can handle that internally. KVM
knows all the VCPUs.

[...]
>      switch (reset_type) {
>      case S390_RESET_EXTERNAL:
>      case S390_RESET_REIPL:
> @@ -357,6 +361,28 @@ static void s390_machine_reset(MachineState *machine)
>          run_on_cpu(cs, s390_do_cpu_initial_reset, RUN_ON_CPU_NULL);
>          run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>          break;
> +    case S390_RESET_PV: /* Subcode 10 */
> +        subsystem_reset();
> +        s390_crypto_reset();
> +
> +        CPU_FOREACH(t) {
> +            run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
> +        }
> +
> +        /* Create SE VM */
> +        s390_pv_vm_create();
> +        CPU_FOREACH(t) {
> +            s390_pv_vcpu_create(t);
> +        }

So, on any other reboot, the VM/CPUs won't get cleaned up?

(is this really a "create" or rather a "s390_pv_vm_enable()").

The "create" terminology somehow sounds wrong to me ...

> +
> +        /* Set SE header and unpack */
> +        s390_ipl_prepare_pv_header();
> +        /* Decrypt image */
> +        s390_ipl_pv_unpack();
> +        /* Verify integrity */
> +        s390_pv_verify();
> +        s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
> +        break;
>      default:
>          g_assert_not_reached();
>      }
> diff --git a/target/s390x/cpu_features_def.inc.h b/target/s390x/cpu_features_def.inc.h
> index 31dff0d84e..60db28351d 100644
> --- a/target/s390x/cpu_features_def.inc.h
> +++ b/target/s390x/cpu_features_def.inc.h
> @@ -107,6 +107,7 @@ DEF_FEAT(DEFLATE_BASE, "deflate-base", STFL, 151, "Deflate-conversion facility (
>  DEF_FEAT(VECTOR_PACKED_DECIMAL_ENH, "vxpdeh", STFL, 152, "Vector-Packed-Decimal-Enhancement Facility")
>  DEF_FEAT(MSA_EXT_9, "msa9-base", STFL, 155, "Message-security-assist-extension-9 facility (excluding subfunctions)")
>  DEF_FEAT(ETOKEN, "etoken", STFL, 156, "Etoken facility")
> +DEF_FEAT(UNPACK, "unpack", STFL, 161, "Unpack facility")
>  
>  /* Features exposed via SCLP SCCB Byte 80 - 98  (bit numbers relative to byte-80) */
>  DEF_FEAT(SIE_GSLS, "gsls", SCLP_CONF_CHAR, 40, "SIE: Guest-storage-limit-suppression facility")
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 03/13] s390x: protvirt: Support unpack facility

[Thomas Huth]

On 29/11/2019 10.47, Janosch Frank wrote:
> When a guest has saved a ipib of type 5 and call diagnose308 with
> subcode 10, we have to setup the protected processing environment via
> Ultravisor calls. The calls are done by KVM and are exposed via an API.
> 
> The following steps are necessary:
> 1. Create a VM (register it with the Ultravisor)
> 2. Create secure CPUs for all of our current cpus
> 3. Forward the secure header to the Ultravisor (has all information on
> how to decrypt the image and VM information)
> 4. Protect image pages from the host and decrypt them
> 5. Verify the image integrity
> 
> Only after step 5 a protected VM is allowed to run.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
[...]
> +++ b/hw/s390x/pv.c
> @@ -0,0 +1,118 @@
> +/*
> + * Secure execution functions
> + *
> + * Copyright IBM Corp. 2019
> + * Author(s):
> + *  Janosch Frank <frankja@linux.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +#include "qemu/osdep.h"
> +#include <sys/ioctl.h>
> +
> +#include <linux/kvm.h>
> +
> +#include "qemu/error-report.h"
> +#include "sysemu/kvm.h"
> +#include "pv.h"
> +
> +static int s390_pv_cmd(uint32_t cmd, void *data)
> +{
> +    int rc;
> +    struct kvm_pv_cmd pv_cmd = {
> +        .cmd = cmd,
> +        .data = (uint64_t)data,
> +    };
> +
> +    rc = kvm_vm_ioctl(kvm_state, KVM_S390_PV_COMMAND, &pv_cmd);
> +    if (rc) {
> +        error_report("KVM PV command failed cmd: %d rc: %d", cmd, rc);
> +        exit(1);
> +    }
> +    return rc;
> +}
> +
> +static int s390_pv_cmd_vcpu(CPUState *cs, uint32_t cmd, void *data)
> +{
> +    int rc;
> +    struct kvm_pv_cmd pv_cmd = {
> +        .cmd = cmd,
> +        .data = (uint64_t)data,
> +    };
> +
> +    rc = kvm_vcpu_ioctl(cs, KVM_S390_PV_COMMAND_VCPU, &pv_cmd);
> +    if (rc) {
> +        error_report("KVM PV VCPU command failed cmd: %d rc: %d", cmd, rc);
> +        exit(1);
> +    }
> +    return rc;
> +}
> +
> +int s390_pv_vm_create(void)
> +{
> +    return s390_pv_cmd(KVM_PV_VM_CREATE, NULL);
> +}
> +
> +int s390_pv_vm_destroy(void)
> +{
> +    return s390_pv_cmd(KVM_PV_VM_DESTROY, NULL);
> +}
> +
> +int s390_pv_vcpu_create(CPUState *cs)
> +{
> +    return s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_CREATE, NULL);
> +}
> +
> +int s390_pv_vcpu_destroy(CPUState *cs)
> +{
> +    S390CPU *cpu = S390_CPU(cs);
> +    CPUS390XState *env = &cpu->env;
> +    int rc;
> +
> +    rc = s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_DESTROY, NULL);
> +    if (!rc) {
> +        env->pv = false;
> +    }
> +    return rc;
> +}
> +
> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
> +{
> +    struct kvm_s390_pv_sec_parm args = {
> +        .origin = origin,
> +        .length = length,
> +    };
> +
> +    return s390_pv_cmd(KVM_PV_VM_SET_SEC_PARMS, &args);
> +}
> +
> +/*
> + * Called for each component in the SE type IPL parameter block 0.
> + */
> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
> +{
> +    struct kvm_s390_pv_unp args = {
> +        .addr = addr,
> +        .size = size,
> +        .tweak = tweak,
> +    };
> +
> +    return s390_pv_cmd(KVM_PV_VM_UNPACK, &args);
> +}
> +
> +int s390_pv_perf_clear_reset(void)
> +{
> +    return s390_pv_cmd(KVM_PV_VM_PERF_CLEAR_RESET, NULL);
> +}
> +
> +int s390_pv_verify(void)
> +{
> +    return s390_pv_cmd(KVM_PV_VM_VERIFY, NULL);
> +}
> +
> +int s390_pv_unshare(void)
> +{
> +    return s390_pv_cmd(KVM_PV_VM_UNSHARE, NULL);
> +}
> diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
> new file mode 100644
> index 0000000000..eb074e4bc9
> --- /dev/null
> +++ b/hw/s390x/pv.h
> @@ -0,0 +1,26 @@
> +/*
> + * Protected Virtualization header
> + *
> + * Copyright IBM Corp. 2019
> + * Author(s):
> + *  Janosch Frank <frankja@linux.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#ifndef HW_S390_PV_H
> +#define HW_S390_PV_H
> +
> +int s390_pv_vm_create(void);
> +int s390_pv_vm_destroy(void);
> +int s390_pv_vcpu_destroy(CPUState *cs);
> +int s390_pv_vcpu_create(CPUState *cs);
> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
> +int s390_pv_perf_clear_reset(void);
> +int s390_pv_verify(void);
> +int s390_pv_unshare(void);

I still think you should make all those functions returning "void"
instead of "int" - since errors results in an exit() in s390_pv_cmd()
and s390_pv_cmd_vcpu() anyway...

> +
> +#endif /* HW_S390_PV_H */
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index c1d1440272..f9481ccace 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -41,6 +41,7 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/s390x/tod.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/s390x/pv.h"
>  
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>  {
> @@ -322,6 +323,7 @@ static void s390_machine_reset(MachineState *machine)
>  {
>      enum s390_reset reset_type;
>      CPUState *cs, *t;
> +    S390CPU *cpu;
>  
>      /* get the reset parameters, reset them once done */
>      s390_ipl_get_reset_request(&cs, &reset_type);
> @@ -329,6 +331,8 @@ static void s390_machine_reset(MachineState *machine)
>      /* all CPUs are paused and synchronized at this point */
>      s390_cmma_reset();
>  
> +    cpu = S390_CPU(cs);
> +
>      switch (reset_type) {
>      case S390_RESET_EXTERNAL:
>      case S390_RESET_REIPL:
> @@ -357,6 +361,28 @@ static void s390_machine_reset(MachineState *machine)
>          run_on_cpu(cs, s390_do_cpu_initial_reset, RUN_ON_CPU_NULL);
>          run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>          break;
> +    case S390_RESET_PV: /* Subcode 10 */
> +        subsystem_reset();
> +        s390_crypto_reset();
> +
> +        CPU_FOREACH(t) {
> +            run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
> +        }
> +
> +        /* Create SE VM */
> +        s390_pv_vm_create();
> +        CPU_FOREACH(t) {
> +            s390_pv_vcpu_create(t);
> +        }
> +
> +        /* Set SE header and unpack */
> +        s390_ipl_prepare_pv_header();
> +        /* Decrypt image */
> +        s390_ipl_pv_unpack();
> +        /* Verify integrity */
> +        s390_pv_verify();
> +        s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
> +        break;

... and here you completely ignore the return codes of all the new
functions.

 Thomas

Re: [PATCH v2 03/13] s390x: protvirt: Support unpack facility

[Janosch Frank]

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

On 12/4/19 11:48 AM, Thomas Huth wrote:
> On 29/11/2019 10.47, Janosch Frank wrote:
>> When a guest has saved a ipib of type 5 and call diagnose308 with
>> subcode 10, we have to setup the protected processing environment via
>> Ultravisor calls. The calls are done by KVM and are exposed via an API.
>>
>> The following steps are necessary:
>> 1. Create a VM (register it with the Ultravisor)
>> 2. Create secure CPUs for all of our current cpus
>> 3. Forward the secure header to the Ultravisor (has all information on
>> how to decrypt the image and VM information)
>> 4. Protect image pages from the host and decrypt them
>> 5. Verify the image integrity
>>
>> Only after step 5 a protected VM is allowed to run.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
> [...]
>> +++ b/hw/s390x/pv.c
>> @@ -0,0 +1,118 @@
>> +/*
>> + * Secure execution functions
>> + *
>> + * Copyright IBM Corp. 2019
>> + * Author(s):
>> + *  Janosch Frank <frankja@linux.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>> + * your option) any later version. See the COPYING file in the top-level
>> + * directory.
>> + */
>> +#include "qemu/osdep.h"
>> +#include <sys/ioctl.h>
>> +
>> +#include <linux/kvm.h>
>> +
>> +#include "qemu/error-report.h"
>> +#include "sysemu/kvm.h"
>> +#include "pv.h"
>> +
>> +static int s390_pv_cmd(uint32_t cmd, void *data)
>> +{
>> +    int rc;
>> +    struct kvm_pv_cmd pv_cmd = {
>> +        .cmd = cmd,
>> +        .data = (uint64_t)data,
>> +    };
>> +
>> +    rc = kvm_vm_ioctl(kvm_state, KVM_S390_PV_COMMAND, &pv_cmd);
>> +    if (rc) {
>> +        error_report("KVM PV command failed cmd: %d rc: %d", cmd, rc);
>> +        exit(1);
>> +    }
>> +    return rc;
>> +}
>> +
>> +static int s390_pv_cmd_vcpu(CPUState *cs, uint32_t cmd, void *data)
>> +{
>> +    int rc;
>> +    struct kvm_pv_cmd pv_cmd = {
>> +        .cmd = cmd,
>> +        .data = (uint64_t)data,
>> +    };
>> +
>> +    rc = kvm_vcpu_ioctl(cs, KVM_S390_PV_COMMAND_VCPU, &pv_cmd);
>> +    if (rc) {
>> +        error_report("KVM PV VCPU command failed cmd: %d rc: %d", cmd, rc);
>> +        exit(1);
>> +    }
>> +    return rc;
>> +}
>> +
>> +int s390_pv_vm_create(void)
>> +{
>> +    return s390_pv_cmd(KVM_PV_VM_CREATE, NULL);
>> +}
>> +
>> +int s390_pv_vm_destroy(void)
>> +{
>> +    return s390_pv_cmd(KVM_PV_VM_DESTROY, NULL);
>> +}
>> +
>> +int s390_pv_vcpu_create(CPUState *cs)
>> +{
>> +    return s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_CREATE, NULL);
>> +}
>> +
>> +int s390_pv_vcpu_destroy(CPUState *cs)
>> +{
>> +    S390CPU *cpu = S390_CPU(cs);
>> +    CPUS390XState *env = &cpu->env;
>> +    int rc;
>> +
>> +    rc = s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_DESTROY, NULL);
>> +    if (!rc) {
>> +        env->pv = false;
>> +    }
>> +    return rc;
>> +}
>> +
>> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
>> +{
>> +    struct kvm_s390_pv_sec_parm args = {
>> +        .origin = origin,
>> +        .length = length,
>> +    };
>> +
>> +    return s390_pv_cmd(KVM_PV_VM_SET_SEC_PARMS, &args);
>> +}
>> +
>> +/*
>> + * Called for each component in the SE type IPL parameter block 0.
>> + */
>> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
>> +{
>> +    struct kvm_s390_pv_unp args = {
>> +        .addr = addr,
>> +        .size = size,
>> +        .tweak = tweak,
>> +    };
>> +
>> +    return s390_pv_cmd(KVM_PV_VM_UNPACK, &args);
>> +}
>> +
>> +int s390_pv_perf_clear_reset(void)
>> +{
>> +    return s390_pv_cmd(KVM_PV_VM_PERF_CLEAR_RESET, NULL);
>> +}
>> +
>> +int s390_pv_verify(void)
>> +{
>> +    return s390_pv_cmd(KVM_PV_VM_VERIFY, NULL);
>> +}
>> +
>> +int s390_pv_unshare(void)
>> +{
>> +    return s390_pv_cmd(KVM_PV_VM_UNSHARE, NULL);
>> +}
>> diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
>> new file mode 100644
>> index 0000000000..eb074e4bc9
>> --- /dev/null
>> +++ b/hw/s390x/pv.h
>> @@ -0,0 +1,26 @@
>> +/*
>> + * Protected Virtualization header
>> + *
>> + * Copyright IBM Corp. 2019
>> + * Author(s):
>> + *  Janosch Frank <frankja@linux.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>> + * your option) any later version. See the COPYING file in the top-level
>> + * directory.
>> + */
>> +
>> +#ifndef HW_S390_PV_H
>> +#define HW_S390_PV_H
>> +
>> +int s390_pv_vm_create(void);
>> +int s390_pv_vm_destroy(void);
>> +int s390_pv_vcpu_destroy(CPUState *cs);
>> +int s390_pv_vcpu_create(CPUState *cs);
>> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
>> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
>> +int s390_pv_perf_clear_reset(void);
>> +int s390_pv_verify(void);
>> +int s390_pv_unshare(void);
> 
> I still think you should make all those functions returning "void"
> instead of "int" - since errors results in an exit() in s390_pv_cmd()
> and s390_pv_cmd_vcpu() anyway...

Hey Thomas,

I have requested an error code for diag 308 subcode 10 that tells the
VM, that we didn't succeed starting a secure guest. Once that is in
place, I'll need to check the return codes.

Although I'm a bit unsure how I should hook that up because of the
diag308/reset handler split.



> 
>> +
>> +#endif /* HW_S390_PV_H */
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index c1d1440272..f9481ccace 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -41,6 +41,7 @@
>>  #include "hw/qdev-properties.h"
>>  #include "hw/s390x/tod.h"
>>  #include "sysemu/sysemu.h"
>> +#include "hw/s390x/pv.h"
>>  
>>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>>  {
>> @@ -322,6 +323,7 @@ static void s390_machine_reset(MachineState *machine)
>>  {
>>      enum s390_reset reset_type;
>>      CPUState *cs, *t;
>> +    S390CPU *cpu;
>>  
>>      /* get the reset parameters, reset them once done */
>>      s390_ipl_get_reset_request(&cs, &reset_type);
>> @@ -329,6 +331,8 @@ static void s390_machine_reset(MachineState *machine)
>>      /* all CPUs are paused and synchronized at this point */
>>      s390_cmma_reset();
>>  
>> +    cpu = S390_CPU(cs);
>> +
>>      switch (reset_type) {
>>      case S390_RESET_EXTERNAL:
>>      case S390_RESET_REIPL:
>> @@ -357,6 +361,28 @@ static void s390_machine_reset(MachineState *machine)
>>          run_on_cpu(cs, s390_do_cpu_initial_reset, RUN_ON_CPU_NULL);
>>          run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>>          break;
>> +    case S390_RESET_PV: /* Subcode 10 */
>> +        subsystem_reset();
>> +        s390_crypto_reset();
>> +
>> +        CPU_FOREACH(t) {
>> +            run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
>> +        }
>> +
>> +        /* Create SE VM */
>> +        s390_pv_vm_create();
>> +        CPU_FOREACH(t) {
>> +            s390_pv_vcpu_create(t);
>> +        }
>> +
>> +        /* Set SE header and unpack */
>> +        s390_ipl_prepare_pv_header();
>> +        /* Decrypt image */
>> +        s390_ipl_pv_unpack();
>> +        /* Verify integrity */
>> +        s390_pv_verify();
>> +        s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>> +        break;
> 
> ... and here you completely ignore the return codes of all the new
> functions.
> 
>  Thomas
> 



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

Re: [PATCH v2 03/13] s390x: protvirt: Support unpack facility

[Thomas Huth]

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

On 04/12/2019 12.32, Janosch Frank wrote:
> On 12/4/19 11:48 AM, Thomas Huth wrote:
>> On 29/11/2019 10.47, Janosch Frank wrote:
>>> When a guest has saved a ipib of type 5 and call diagnose308 with
>>> subcode 10, we have to setup the protected processing environment via
>>> Ultravisor calls. The calls are done by KVM and are exposed via an API.
>>>
>>> The following steps are necessary:
>>> 1. Create a VM (register it with the Ultravisor)
>>> 2. Create secure CPUs for all of our current cpus
>>> 3. Forward the secure header to the Ultravisor (has all information on
>>> how to decrypt the image and VM information)
>>> 4. Protect image pages from the host and decrypt them
>>> 5. Verify the image integrity
>>>
>>> Only after step 5 a protected VM is allowed to run.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>> [...]
>>> +++ b/hw/s390x/pv.c
>>> @@ -0,0 +1,118 @@
>>> +/*
>>> + * Secure execution functions
>>> + *
>>> + * Copyright IBM Corp. 2019
>>> + * Author(s):
>>> + *  Janosch Frank <frankja@linux.ibm.com>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>>> + * your option) any later version. See the COPYING file in the top-level
>>> + * directory.
>>> + */
>>> +#include "qemu/osdep.h"
>>> +#include <sys/ioctl.h>
>>> +
>>> +#include <linux/kvm.h>
>>> +
>>> +#include "qemu/error-report.h"
>>> +#include "sysemu/kvm.h"
>>> +#include "pv.h"
>>> +
>>> +static int s390_pv_cmd(uint32_t cmd, void *data)
>>> +{
>>> +    int rc;
>>> +    struct kvm_pv_cmd pv_cmd = {
>>> +        .cmd = cmd,
>>> +        .data = (uint64_t)data,
>>> +    };
>>> +
>>> +    rc = kvm_vm_ioctl(kvm_state, KVM_S390_PV_COMMAND, &pv_cmd);
>>> +    if (rc) {
>>> +        error_report("KVM PV command failed cmd: %d rc: %d", cmd, rc);
>>> +        exit(1);
>>> +    }
>>> +    return rc;
>>> +}
>>> +
>>> +static int s390_pv_cmd_vcpu(CPUState *cs, uint32_t cmd, void *data)
>>> +{
>>> +    int rc;
>>> +    struct kvm_pv_cmd pv_cmd = {
>>> +        .cmd = cmd,
>>> +        .data = (uint64_t)data,
>>> +    };
>>> +
>>> +    rc = kvm_vcpu_ioctl(cs, KVM_S390_PV_COMMAND_VCPU, &pv_cmd);
>>> +    if (rc) {
>>> +        error_report("KVM PV VCPU command failed cmd: %d rc: %d", cmd, rc);
>>> +        exit(1);
>>> +    }
>>> +    return rc;
>>> +}
>>> +
>>> +int s390_pv_vm_create(void)
>>> +{
>>> +    return s390_pv_cmd(KVM_PV_VM_CREATE, NULL);
>>> +}
>>> +
>>> +int s390_pv_vm_destroy(void)
>>> +{
>>> +    return s390_pv_cmd(KVM_PV_VM_DESTROY, NULL);
>>> +}
>>> +
>>> +int s390_pv_vcpu_create(CPUState *cs)
>>> +{
>>> +    return s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_CREATE, NULL);
>>> +}
>>> +
>>> +int s390_pv_vcpu_destroy(CPUState *cs)
>>> +{
>>> +    S390CPU *cpu = S390_CPU(cs);
>>> +    CPUS390XState *env = &cpu->env;
>>> +    int rc;
>>> +
>>> +    rc = s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_DESTROY, NULL);
>>> +    if (!rc) {
>>> +        env->pv = false;
>>> +    }
>>> +    return rc;
>>> +}
>>> +
>>> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
>>> +{
>>> +    struct kvm_s390_pv_sec_parm args = {
>>> +        .origin = origin,
>>> +        .length = length,
>>> +    };
>>> +
>>> +    return s390_pv_cmd(KVM_PV_VM_SET_SEC_PARMS, &args);
>>> +}
>>> +
>>> +/*
>>> + * Called for each component in the SE type IPL parameter block 0.
>>> + */
>>> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
>>> +{
>>> +    struct kvm_s390_pv_unp args = {
>>> +        .addr = addr,
>>> +        .size = size,
>>> +        .tweak = tweak,
>>> +    };
>>> +
>>> +    return s390_pv_cmd(KVM_PV_VM_UNPACK, &args);
>>> +}
>>> +
>>> +int s390_pv_perf_clear_reset(void)
>>> +{
>>> +    return s390_pv_cmd(KVM_PV_VM_PERF_CLEAR_RESET, NULL);
>>> +}
>>> +
>>> +int s390_pv_verify(void)
>>> +{
>>> +    return s390_pv_cmd(KVM_PV_VM_VERIFY, NULL);
>>> +}
>>> +
>>> +int s390_pv_unshare(void)
>>> +{
>>> +    return s390_pv_cmd(KVM_PV_VM_UNSHARE, NULL);
>>> +}
>>> diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
>>> new file mode 100644
>>> index 0000000000..eb074e4bc9
>>> --- /dev/null
>>> +++ b/hw/s390x/pv.h
>>> @@ -0,0 +1,26 @@
>>> +/*
>>> + * Protected Virtualization header
>>> + *
>>> + * Copyright IBM Corp. 2019
>>> + * Author(s):
>>> + *  Janosch Frank <frankja@linux.ibm.com>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>>> + * your option) any later version. See the COPYING file in the top-level
>>> + * directory.
>>> + */
>>> +
>>> +#ifndef HW_S390_PV_H
>>> +#define HW_S390_PV_H
>>> +
>>> +int s390_pv_vm_create(void);
>>> +int s390_pv_vm_destroy(void);
>>> +int s390_pv_vcpu_destroy(CPUState *cs);
>>> +int s390_pv_vcpu_create(CPUState *cs);
>>> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
>>> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
>>> +int s390_pv_perf_clear_reset(void);
>>> +int s390_pv_verify(void);
>>> +int s390_pv_unshare(void);
>>
>> I still think you should make all those functions returning "void"
>> instead of "int" - since errors results in an exit() in s390_pv_cmd()
>> and s390_pv_cmd_vcpu() anyway...
> 
> Hey Thomas,
> 
> I have requested an error code for diag 308 subcode 10 that tells the
> VM, that we didn't succeed starting a secure guest. Once that is in
> place, I'll need to check the return codes.

Ok, but then the exit()s need to go away, I assume?

 Thomas


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

Re: [PATCH v2 03/13] s390x: protvirt: Support unpack facility

[Janosch Frank]

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

On 12/4/19 12:34 PM, Thomas Huth wrote:
> On 04/12/2019 12.32, Janosch Frank wrote:
>> On 12/4/19 11:48 AM, Thomas Huth wrote:
>>> On 29/11/2019 10.47, Janosch Frank wrote:
>>>> When a guest has saved a ipib of type 5 and call diagnose308 with
>>>> subcode 10, we have to setup the protected processing environment via
>>>> Ultravisor calls. The calls are done by KVM and are exposed via an API.
>>>>
>>>> The following steps are necessary:
>>>> 1. Create a VM (register it with the Ultravisor)
>>>> 2. Create secure CPUs for all of our current cpus
>>>> 3. Forward the secure header to the Ultravisor (has all information on
>>>> how to decrypt the image and VM information)
>>>> 4. Protect image pages from the host and decrypt them
>>>> 5. Verify the image integrity
>>>>
>>>> Only after step 5 a protected VM is allowed to run.
>>>>
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>> [...]
>>>> +++ b/hw/s390x/pv.c
>>>> @@ -0,0 +1,118 @@
>>>> +/*
>>>> + * Secure execution functions
>>>> + *
>>>> + * Copyright IBM Corp. 2019
>>>> + * Author(s):
>>>> + *  Janosch Frank <frankja@linux.ibm.com>
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>>>> + * your option) any later version. See the COPYING file in the top-level
>>>> + * directory.
>>>> + */
>>>> +#include "qemu/osdep.h"
>>>> +#include <sys/ioctl.h>
>>>> +
>>>> +#include <linux/kvm.h>
>>>> +
>>>> +#include "qemu/error-report.h"
>>>> +#include "sysemu/kvm.h"
>>>> +#include "pv.h"
>>>> +
>>>> +static int s390_pv_cmd(uint32_t cmd, void *data)
>>>> +{
>>>> +    int rc;
>>>> +    struct kvm_pv_cmd pv_cmd = {
>>>> +        .cmd = cmd,
>>>> +        .data = (uint64_t)data,
>>>> +    };
>>>> +
>>>> +    rc = kvm_vm_ioctl(kvm_state, KVM_S390_PV_COMMAND, &pv_cmd);
>>>> +    if (rc) {
>>>> +        error_report("KVM PV command failed cmd: %d rc: %d", cmd, rc);
>>>> +        exit(1);
>>>> +    }
>>>> +    return rc;
>>>> +}
>>>> +
>>>> +static int s390_pv_cmd_vcpu(CPUState *cs, uint32_t cmd, void *data)
>>>> +{
>>>> +    int rc;
>>>> +    struct kvm_pv_cmd pv_cmd = {
>>>> +        .cmd = cmd,
>>>> +        .data = (uint64_t)data,
>>>> +    };
>>>> +
>>>> +    rc = kvm_vcpu_ioctl(cs, KVM_S390_PV_COMMAND_VCPU, &pv_cmd);
>>>> +    if (rc) {
>>>> +        error_report("KVM PV VCPU command failed cmd: %d rc: %d", cmd, rc);
>>>> +        exit(1);
>>>> +    }
>>>> +    return rc;
>>>> +}
>>>> +
>>>> +int s390_pv_vm_create(void)
>>>> +{
>>>> +    return s390_pv_cmd(KVM_PV_VM_CREATE, NULL);
>>>> +}
>>>> +
>>>> +int s390_pv_vm_destroy(void)
>>>> +{
>>>> +    return s390_pv_cmd(KVM_PV_VM_DESTROY, NULL);
>>>> +}
>>>> +
>>>> +int s390_pv_vcpu_create(CPUState *cs)
>>>> +{
>>>> +    return s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_CREATE, NULL);
>>>> +}
>>>> +
>>>> +int s390_pv_vcpu_destroy(CPUState *cs)
>>>> +{
>>>> +    S390CPU *cpu = S390_CPU(cs);
>>>> +    CPUS390XState *env = &cpu->env;
>>>> +    int rc;
>>>> +
>>>> +    rc = s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_DESTROY, NULL);
>>>> +    if (!rc) {
>>>> +        env->pv = false;
>>>> +    }
>>>> +    return rc;
>>>> +}
>>>> +
>>>> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
>>>> +{
>>>> +    struct kvm_s390_pv_sec_parm args = {
>>>> +        .origin = origin,
>>>> +        .length = length,
>>>> +    };
>>>> +
>>>> +    return s390_pv_cmd(KVM_PV_VM_SET_SEC_PARMS, &args);
>>>> +}
>>>> +
>>>> +/*
>>>> + * Called for each component in the SE type IPL parameter block 0.
>>>> + */
>>>> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
>>>> +{
>>>> +    struct kvm_s390_pv_unp args = {
>>>> +        .addr = addr,
>>>> +        .size = size,
>>>> +        .tweak = tweak,
>>>> +    };
>>>> +
>>>> +    return s390_pv_cmd(KVM_PV_VM_UNPACK, &args);
>>>> +}
>>>> +
>>>> +int s390_pv_perf_clear_reset(void)
>>>> +{
>>>> +    return s390_pv_cmd(KVM_PV_VM_PERF_CLEAR_RESET, NULL);
>>>> +}
>>>> +
>>>> +int s390_pv_verify(void)
>>>> +{
>>>> +    return s390_pv_cmd(KVM_PV_VM_VERIFY, NULL);
>>>> +}
>>>> +
>>>> +int s390_pv_unshare(void)
>>>> +{
>>>> +    return s390_pv_cmd(KVM_PV_VM_UNSHARE, NULL);
>>>> +}
>>>> diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
>>>> new file mode 100644
>>>> index 0000000000..eb074e4bc9
>>>> --- /dev/null
>>>> +++ b/hw/s390x/pv.h
>>>> @@ -0,0 +1,26 @@
>>>> +/*
>>>> + * Protected Virtualization header
>>>> + *
>>>> + * Copyright IBM Corp. 2019
>>>> + * Author(s):
>>>> + *  Janosch Frank <frankja@linux.ibm.com>
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>>>> + * your option) any later version. See the COPYING file in the top-level
>>>> + * directory.
>>>> + */
>>>> +
>>>> +#ifndef HW_S390_PV_H
>>>> +#define HW_S390_PV_H
>>>> +
>>>> +int s390_pv_vm_create(void);
>>>> +int s390_pv_vm_destroy(void);
>>>> +int s390_pv_vcpu_destroy(CPUState *cs);
>>>> +int s390_pv_vcpu_create(CPUState *cs);
>>>> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
>>>> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
>>>> +int s390_pv_perf_clear_reset(void);
>>>> +int s390_pv_verify(void);
>>>> +int s390_pv_unshare(void);
>>>
>>> I still think you should make all those functions returning "void"
>>> instead of "int" - since errors results in an exit() in s390_pv_cmd()
>>> and s390_pv_cmd_vcpu() anyway...
>>
>> Hey Thomas,
>>
>> I have requested an error code for diag 308 subcode 10 that tells the
>> VM, that we didn't succeed starting a secure guest. Once that is in
>> place, I'll need to check the return codes.
> 
> Ok, but then the exit()s need to go away, I assume?

That would be necessary to wire the rc up, yes.

> 
>  Thomas
> 



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

[PATCH v2 04/13] s390x: protvirt: Handle diag 308 subcodes 0,1,3,4

[Janosch Frank]

Now that we know the protection state off the cpus, we can start
handling all diag 308 subcodes in the protected state.

For subcodes 0 and 1 we need to unshare all pages before continuing,
so the guest doesn't accidentally expose data when dumping.

For subcode 3/4 we tear down the protected VM and reboot into
unprotected mode. We do not provide a secure reboot.

Before we can do the unshare calls, we need to mark all cpus as
stopped.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 31 ++++++++++++++++++++++++++++---
 target/s390x/diag.c        |  4 ++++
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index f9481ccace..e2a302398d 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -319,11 +319,26 @@ static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg)
     s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
 }
 
+static void s390_pv_prepare_reset(CPUS390XState *env)
+{
+    CPUState *cs;
+
+    if (!env->pv) {
+        return;
+    }
+    CPU_FOREACH(cs) {
+        s390_cpu_set_state(S390_CPU_STATE_STOPPED, S390_CPU(cs));
+    }
+    s390_pv_unshare();
+    s390_pv_perf_clear_reset();
+}
+
 static void s390_machine_reset(MachineState *machine)
 {
     enum s390_reset reset_type;
     CPUState *cs, *t;
     S390CPU *cpu;
+    CPUS390XState *env;
 
     /* get the reset parameters, reset them once done */
     s390_ipl_get_reset_request(&cs, &reset_type);
@@ -332,10 +347,18 @@ static void s390_machine_reset(MachineState *machine)
     s390_cmma_reset();
 
     cpu = S390_CPU(cs);
+    env = &cpu->env;
 
     switch (reset_type) {
     case S390_RESET_EXTERNAL:
     case S390_RESET_REIPL:
+        if (env->pv) {
+            CPU_FOREACH(t) {
+                s390_pv_vcpu_destroy(t);
+            }
+            s390_pv_vm_destroy();
+        }
+
         qemu_devices_reset();
         s390_crypto_reset();
 
@@ -343,21 +366,23 @@ static void s390_machine_reset(MachineState *machine)
         run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
         break;
     case S390_RESET_MODIFIED_CLEAR:
+        subsystem_reset();
+        s390_crypto_reset();
+        s390_pv_prepare_reset(env);
         CPU_FOREACH(t) {
             run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
         }
-        subsystem_reset();
-        s390_crypto_reset();
         run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
         break;
     case S390_RESET_LOAD_NORMAL:
+        subsystem_reset();
+        s390_pv_prepare_reset(env);
         CPU_FOREACH(t) {
             if (t == cs) {
                 continue;
             }
             run_on_cpu(t, s390_do_cpu_reset, RUN_ON_CPU_NULL);
         }
-        subsystem_reset();
         run_on_cpu(cs, s390_do_cpu_initial_reset, RUN_ON_CPU_NULL);
         run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
         break;
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index 112a6c92e0..5489fc721a 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -68,6 +68,10 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
 static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
                               uintptr_t ra, bool write)
 {
+    /* Handled by the Ultravisor */
+    if (env->pv) {
+        return 0;
+    }
     if ((r1 & 1) || (addr & ~TARGET_PAGE_MASK)) {
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);
         return -1;
-- 
2.20.1

Re: [PATCH v2 04/13] s390x: protvirt: Handle diag 308 subcodes 0,1,3,4

[David Hildenbrand]

On 29.11.19 10:48, Janosch Frank wrote:
> Now that we know the protection state off the cpus, we can start
> handling all diag 308 subcodes in the protected state.
> 
> For subcodes 0 and 1 we need to unshare all pages before continuing,
> so the guest doesn't accidentally expose data when dumping.
> 
> For subcode 3/4 we tear down the protected VM and reboot into
> unprotected mode. We do not provide a secure reboot.
> 
> Before we can do the unshare calls, we need to mark all cpus as
> stopped.

This patch should be squashed into the previous one IMHO ...

> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 31 ++++++++++++++++++++++++++++---
>  target/s390x/diag.c        |  4 ++++
>  2 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index f9481ccace..e2a302398d 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -319,11 +319,26 @@ static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg)
>      s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>  }
>  
> +static void s390_pv_prepare_reset(CPUS390XState *env)
> +{
> +    CPUState *cs;
> +
> +    if (!env->pv) {
> +        return;
> +    }

I'd suggest doing that check in the callers. (just like e.g., in "case
S390_RESET_REIPL" already)

> +    CPU_FOREACH(cs) {
> +        s390_cpu_set_state(S390_CPU_STATE_STOPPED, S390_CPU(cs));
> +    }
> +    s390_pv_unshare();
> +    s390_pv_perf_clear_reset();
> +}
> +
>  static void s390_machine_reset(MachineState *machine)
>  {
>      enum s390_reset reset_type;
>      CPUState *cs, *t;
>      S390CPU *cpu;
> +    CPUS390XState *env;
>  
>      /* get the reset parameters, reset them once done */
>      s390_ipl_get_reset_request(&cs, &reset_type);
> @@ -332,10 +347,18 @@ static void s390_machine_reset(MachineState *machine)
>      s390_cmma_reset();
>  
>      cpu = S390_CPU(cs);
> +    env = &cpu->env;
>  
>      switch (reset_type) {
>      case S390_RESET_EXTERNAL:
>      case S390_RESET_REIPL:
> +        if (env->pv) {
> +            CPU_FOREACH(t) {
> +                s390_pv_vcpu_destroy(t);
> +            }
> +            s390_pv_vm_destroy();
> +        }
> +
>          qemu_devices_reset();
>          s390_crypto_reset();
>  
> @@ -343,21 +366,23 @@ static void s390_machine_reset(MachineState *machine)
>          run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
>          break;
>      case S390_RESET_MODIFIED_CLEAR:
> +        subsystem_reset();
> +        s390_crypto_reset();
> +        s390_pv_prepare_reset(env);
>          CPU_FOREACH(t) {
>              run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
>          }
> -        subsystem_reset();
> -        s390_crypto_reset();
>          run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>          break;
>      case S390_RESET_LOAD_NORMAL:
> +        subsystem_reset();
> +        s390_pv_prepare_reset(env);
>          CPU_FOREACH(t) {
>              if (t == cs) {
>                  continue;
>              }
>              run_on_cpu(t, s390_do_cpu_reset, RUN_ON_CPU_NULL);
>          }
> -        subsystem_reset();

I do wonder if changing the orders here is okay ... why do you have to
move subsystem_reset()?

-- 
Thanks,

David / dhildenb

[PATCH v2 05/13] s390x: protvirt: Add pv state to cpu env

[Janosch Frank]

We need to know if we run in pv state or not when emulating
instructions.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 2 ++
 target/s390x/cpu.h         | 1 +
 2 files changed, 3 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index e2a302398d..6fcd695b81 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -357,6 +357,7 @@ static void s390_machine_reset(MachineState *machine)
                 s390_pv_vcpu_destroy(t);
             }
             s390_pv_vm_destroy();
+            env->pv = false;
         }
 
         qemu_devices_reset();
@@ -406,6 +407,7 @@ static void s390_machine_reset(MachineState *machine)
         s390_ipl_pv_unpack();
         /* Verify integrity */
         s390_pv_verify();
+        env->pv = true;
         s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
         break;
     default:
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index d2af13b345..43e6c286d2 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -116,6 +116,7 @@ struct CPUS390XState {
 
     /* Fields up to this point are cleared by a CPU reset */
     struct {} end_reset_fields;
+    bool pv; /* protected virtualization */
 
 #if !defined(CONFIG_USER_ONLY)
     uint32_t core_id; /* PoP "CPU address", same as cpu_index */
-- 
2.20.1

Re: [PATCH v2 05/13] s390x: protvirt: Add pv state to cpu env

[David Hildenbrand]

On 29.11.19 10:48, Janosch Frank wrote:
> We need to know if we run in pv state or not when emulating
> instructions.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 2 ++
>  target/s390x/cpu.h         | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index e2a302398d..6fcd695b81 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -357,6 +357,7 @@ static void s390_machine_reset(MachineState *machine)
>                  s390_pv_vcpu_destroy(t);
>              }
>              s390_pv_vm_destroy();
> +            env->pv = false;
>          }
>  
>          qemu_devices_reset();
> @@ -406,6 +407,7 @@ static void s390_machine_reset(MachineState *machine)
>          s390_ipl_pv_unpack();
>          /* Verify integrity */
>          s390_pv_verify();
> +        env->pv = true;
>          s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>          break;
>      default:
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index d2af13b345..43e6c286d2 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -116,6 +116,7 @@ struct CPUS390XState {
>  
>      /* Fields up to this point are cleared by a CPU reset */
>      struct {} end_reset_fields;
> +    bool pv; /* protected virtualization */

so ... the preceding patches fail to compile?

Please properly squash that ...

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 05/13] s390x: protvirt: Add pv state to cpu env

[Janosch Frank]

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

On 11/29/19 11:30 AM, David Hildenbrand wrote:
> On 29.11.19 10:48, Janosch Frank wrote:
>> We need to know if we run in pv state or not when emulating
>> instructions.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  hw/s390x/s390-virtio-ccw.c | 2 ++
>>  target/s390x/cpu.h         | 1 +
>>  2 files changed, 3 insertions(+)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index e2a302398d..6fcd695b81 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -357,6 +357,7 @@ static void s390_machine_reset(MachineState *machine)
>>                  s390_pv_vcpu_destroy(t);
>>              }
>>              s390_pv_vm_destroy();
>> +            env->pv = false;
>>          }
>>  
>>          qemu_devices_reset();
>> @@ -406,6 +407,7 @@ static void s390_machine_reset(MachineState *machine)
>>          s390_ipl_pv_unpack();
>>          /* Verify integrity */
>>          s390_pv_verify();
>> +        env->pv = true;
>>          s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>>          break;
>>      default:
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index d2af13b345..43e6c286d2 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -116,6 +116,7 @@ struct CPUS390XState {
>>  
>>      /* Fields up to this point are cleared by a CPU reset */
>>      struct {} end_reset_fields;
>> +    bool pv; /* protected virtualization */
> 
> so ... the preceding patches fail to compile?
> 
> Please properly squash that ...
> 

Sure, will do.


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

Re: [PATCH v2 05/13] s390x: protvirt: Add pv state to cpu env

[Janosch Frank]

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

On 11/29/19 11:30 AM, David Hildenbrand wrote:
> On 29.11.19 10:48, Janosch Frank wrote:
>> We need to know if we run in pv state or not when emulating
>> instructions.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  hw/s390x/s390-virtio-ccw.c | 2 ++
>>  target/s390x/cpu.h         | 1 +
>>  2 files changed, 3 insertions(+)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index e2a302398d..6fcd695b81 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -357,6 +357,7 @@ static void s390_machine_reset(MachineState *machine)
>>                  s390_pv_vcpu_destroy(t);
>>              }
>>              s390_pv_vm_destroy();
>> +            env->pv = false;
>>          }
>>  
>>          qemu_devices_reset();
>> @@ -406,6 +407,7 @@ static void s390_machine_reset(MachineState *machine)
>>          s390_ipl_pv_unpack();
>>          /* Verify integrity */
>>          s390_pv_verify();
>> +        env->pv = true;
>>          s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>>          break;
>>      default:
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index d2af13b345..43e6c286d2 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -116,6 +116,7 @@ struct CPUS390XState {
>>  
>>      /* Fields up to this point are cleared by a CPU reset */
>>      struct {} end_reset_fields;
>> +    bool pv; /* protected virtualization */
> 
> so ... the preceding patches fail to compile?
> 
> Please properly squash that ...
> 

As it turns out, this patch doesn't really support multiple cpus, since
we only mark the ipl cpu as protected. We also need to mark all others
(we can do that in the create cpu call) and have a machine flag, so we
also mark hotplug cpus.

I need to think about that a bit more.


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

Re: [PATCH v2 05/13] s390x: protvirt: Add pv state to cpu env

[David Hildenbrand]

On 06.12.19 10:50, Janosch Frank wrote:
> On 11/29/19 11:30 AM, David Hildenbrand wrote:
>> On 29.11.19 10:48, Janosch Frank wrote:
>>> We need to know if we run in pv state or not when emulating
>>> instructions.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>  hw/s390x/s390-virtio-ccw.c | 2 ++
>>>  target/s390x/cpu.h         | 1 +
>>>  2 files changed, 3 insertions(+)
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index e2a302398d..6fcd695b81 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -357,6 +357,7 @@ static void s390_machine_reset(MachineState *machine)
>>>                  s390_pv_vcpu_destroy(t);
>>>              }
>>>              s390_pv_vm_destroy();
>>> +            env->pv = false;
>>>          }
>>>  
>>>          qemu_devices_reset();
>>> @@ -406,6 +407,7 @@ static void s390_machine_reset(MachineState *machine)
>>>          s390_ipl_pv_unpack();
>>>          /* Verify integrity */
>>>          s390_pv_verify();
>>> +        env->pv = true;
>>>          s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>>>          break;
>>>      default:
>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>>> index d2af13b345..43e6c286d2 100644
>>> --- a/target/s390x/cpu.h
>>> +++ b/target/s390x/cpu.h
>>> @@ -116,6 +116,7 @@ struct CPUS390XState {
>>>  
>>>      /* Fields up to this point are cleared by a CPU reset */
>>>      struct {} end_reset_fields;
>>> +    bool pv; /* protected virtualization */
>>
>> so ... the preceding patches fail to compile?
>>
>> Please properly squash that ...
>>
> 
> As it turns out, this patch doesn't really support multiple cpus, since
> we only mark the ipl cpu as protected. We also need to mark all others
> (we can do that in the create cpu call) and have a machine flag, so we
> also mark hotplug cpus.

Setting a machine state flag when enabling/disabling prot feels right.
But not sure about the performance implications when always having to
look up the machine state ... maybe you have to cache it in all CPUs, or
somewhere else ("global variable").

> 
> I need to think about that a bit more.
> 


-- 
Thanks,

David / dhildenb

[PATCH v2 06/13] s390x: protvirt: KVM intercept changes

[Janosch Frank]

Secure guests no longer intercept with code 4 for an instruction
interception. Instead they have codes 104 and 108 for secure
instruction interception and secure instruction notification
respectively.

The 104 mirrors the 4 interception.

The 108 is a notification interception to let KVM and QEMU know that
something changed and we need to update tracking information or
perform specific tasks. It's currently taken for the following
instructions:

* stpx (To inform about the changed prefix location)
* sclp (On incorrect SCCB values, so we can inject a IRQ)
* sigp (All but "stop and store status")
* diag308 (Subcodes 0/1)

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/kvm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index ad6e38c876..3d9c44ba9d 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -115,6 +115,8 @@
 #define ICPT_CPU_STOP                   0x28
 #define ICPT_OPEREXC                    0x2c
 #define ICPT_IO                         0x40
+#define ICPT_PV_INSTR                   0x68
+#define ICPT_PV_INSTR_NOTIFICATION      0x6c
 
 #define NR_LOCAL_IRQS 32
 /*
@@ -151,6 +153,7 @@ static int cap_s390_irq;
 static int cap_ri;
 static int cap_gs;
 static int cap_hpage_1m;
+static int cap_protvirt;
 
 static int active_cmma;
 
@@ -342,6 +345,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
     cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF);
     cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
     cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
+    cap_protvirt = kvm_check_extension(s, KVM_CAP_S390_PROTECTED);
 
     if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
         || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
@@ -1664,6 +1668,8 @@ static int handle_intercept(S390CPU *cpu)
             (long)cs->kvm_run->psw_addr);
     switch (icpt_code) {
         case ICPT_INSTRUCTION:
+        case ICPT_PV_INSTR:
+        case ICPT_PV_INSTR_NOTIFICATION:
             r = handle_instruction(cpu, run);
             break;
         case ICPT_PROGRAM:
-- 
2.20.1

Re: [PATCH v2 06/13] s390x: protvirt: KVM intercept changes

[David Hildenbrand]

On 29.11.19 10:48, Janosch Frank wrote:
> Secure guests no longer intercept with code 4 for an instruction
> interception. Instead they have codes 104 and 108 for secure
> instruction interception and secure instruction notification
> respectively.
> 
> The 104 mirrors the 4 interception.
> 
> The 108 is a notification interception to let KVM and QEMU know that
> something changed and we need to update tracking information or
> perform specific tasks. It's currently taken for the following
> instructions:
> 
> * stpx (To inform about the changed prefix location)
> * sclp (On incorrect SCCB values, so we can inject a IRQ)
> * sigp (All but "stop and store status")
> * diag308 (Subcodes 0/1)
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/kvm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index ad6e38c876..3d9c44ba9d 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -115,6 +115,8 @@
>  #define ICPT_CPU_STOP                   0x28
>  #define ICPT_OPEREXC                    0x2c
>  #define ICPT_IO                         0x40
> +#define ICPT_PV_INSTR                   0x68
> +#define ICPT_PV_INSTR_NOTIFICATION      0x6c
>  
>  #define NR_LOCAL_IRQS 32
>  /*
> @@ -151,6 +153,7 @@ static int cap_s390_irq;
>  static int cap_ri;
>  static int cap_gs;
>  static int cap_hpage_1m;
> +static int cap_protvirt;
>  
>  static int active_cmma;
>  
> @@ -342,6 +345,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF);
>      cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
>      cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
> +    cap_protvirt = kvm_check_extension(s, KVM_CAP_S390_PROTECTED);
>  

Introduced but not used. This has to be moved to the place where it is
actually needed ...

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 06/13] s390x: protvirt: KVM intercept changes

[Cornelia Huck]

On Fri, 29 Nov 2019 04:48:02 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Secure guests no longer intercept with code 4 for an instruction
> interception. Instead they have codes 104 and 108 for secure
> instruction interception and secure instruction notification
> respectively.
> 
> The 104 mirrors the 4 interception.
> 
> The 108 is a notification interception to let KVM and QEMU know that
> something changed and we need to update tracking information or
> perform specific tasks. It's currently taken for the following
> instructions:
> 
> * stpx (To inform about the changed prefix location)
> * sclp (On incorrect SCCB values, so we can inject a IRQ)
> * sigp (All but "stop and store status")
> * diag308 (Subcodes 0/1)
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/kvm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index ad6e38c876..3d9c44ba9d 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -115,6 +115,8 @@
>  #define ICPT_CPU_STOP                   0x28
>  #define ICPT_OPEREXC                    0x2c
>  #define ICPT_IO                         0x40
> +#define ICPT_PV_INSTR                   0x68
> +#define ICPT_PV_INSTR_NOTIFICATION      0x6c
>  
>  #define NR_LOCAL_IRQS 32
>  /*
> @@ -151,6 +153,7 @@ static int cap_s390_irq;
>  static int cap_ri;
>  static int cap_gs;
>  static int cap_hpage_1m;
> +static int cap_protvirt;
>  
>  static int active_cmma;
>  
> @@ -342,6 +345,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF);
>      cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
>      cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
> +    cap_protvirt = kvm_check_extension(s, KVM_CAP_S390_PROTECTED);
>  
>      if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>          || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
> @@ -1664,6 +1668,8 @@ static int handle_intercept(S390CPU *cpu)
>              (long)cs->kvm_run->psw_addr);
>      switch (icpt_code) {
>          case ICPT_INSTRUCTION:
> +        case ICPT_PV_INSTR:
> +        case ICPT_PV_INSTR_NOTIFICATION:
>              r = handle_instruction(cpu, run);

I'm still a bit uneasy about going through the same path for both 104
and 108. How does the handler figure out whether it should emulate an
instruction, or just process a notification? Is it guaranteed that a
given instruction is always showing up as either a 104 or a 108, so
that the handler can check the pv state?

[Even if that works, it still feels a bit unclean to me.]

>              break;
>          case ICPT_PROGRAM:

Re: [PATCH v2 06/13] s390x: protvirt: KVM intercept changes

[Janosch Frank]

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

On 12/5/19 6:15 PM, Cornelia Huck wrote:
> On Fri, 29 Nov 2019 04:48:02 -0500
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Secure guests no longer intercept with code 4 for an instruction
>> interception. Instead they have codes 104 and 108 for secure
>> instruction interception and secure instruction notification
>> respectively.
>>
>> The 104 mirrors the 4 interception.
>>
>> The 108 is a notification interception to let KVM and QEMU know that
>> something changed and we need to update tracking information or
>> perform specific tasks. It's currently taken for the following
>> instructions:
>>
>> * stpx (To inform about the changed prefix location)
>> * sclp (On incorrect SCCB values, so we can inject a IRQ)
>> * sigp (All but "stop and store status")
>> * diag308 (Subcodes 0/1)
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  target/s390x/kvm.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index ad6e38c876..3d9c44ba9d 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -115,6 +115,8 @@
>>  #define ICPT_CPU_STOP                   0x28
>>  #define ICPT_OPEREXC                    0x2c
>>  #define ICPT_IO                         0x40
>> +#define ICPT_PV_INSTR                   0x68
>> +#define ICPT_PV_INSTR_NOTIFICATION      0x6c
>>  
>>  #define NR_LOCAL_IRQS 32
>>  /*
>> @@ -151,6 +153,7 @@ static int cap_s390_irq;
>>  static int cap_ri;
>>  static int cap_gs;
>>  static int cap_hpage_1m;
>> +static int cap_protvirt;
>>  
>>  static int active_cmma;
>>  
>> @@ -342,6 +345,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>      cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF);
>>      cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
>>      cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
>> +    cap_protvirt = kvm_check_extension(s, KVM_CAP_S390_PROTECTED);
>>  
>>      if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>>          || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
>> @@ -1664,6 +1668,8 @@ static int handle_intercept(S390CPU *cpu)
>>              (long)cs->kvm_run->psw_addr);
>>      switch (icpt_code) {
>>          case ICPT_INSTRUCTION:
>> +        case ICPT_PV_INSTR:
>> +        case ICPT_PV_INSTR_NOTIFICATION:
>>              r = handle_instruction(cpu, run);
> 
> I'm still a bit uneasy about going through the same path for both 104
> and 108. How does the handler figure out whether it should emulate an
> instruction, or just process a notification? Is it guaranteed that a
> given instruction is always showing up as either a 104 or a 108, so
> that the handler can check the pv state?

diag 308 subcode 0/1 are 108, but all other subcodes are defined as a
104 (if they are an exit at all)...

> 
> [Even if that works, it still feels a bit unclean to me.]
> 
>>              break;
>>          case ICPT_PROGRAM:
> 
> 



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

Re: [PATCH v2 06/13] s390x: protvirt: KVM intercept changes

[Cornelia Huck]

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

On Thu, 5 Dec 2019 18:34:32 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 12/5/19 6:15 PM, Cornelia Huck wrote:
> > On Fri, 29 Nov 2019 04:48:02 -0500
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> Secure guests no longer intercept with code 4 for an instruction
> >> interception. Instead they have codes 104 and 108 for secure
> >> instruction interception and secure instruction notification
> >> respectively.
> >>
> >> The 104 mirrors the 4 interception.
> >>
> >> The 108 is a notification interception to let KVM and QEMU know that
> >> something changed and we need to update tracking information or
> >> perform specific tasks. It's currently taken for the following
> >> instructions:
> >>
> >> * stpx (To inform about the changed prefix location)
> >> * sclp (On incorrect SCCB values, so we can inject a IRQ)
> >> * sigp (All but "stop and store status")
> >> * diag308 (Subcodes 0/1)
> >>
> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >> ---
> >>  target/s390x/kvm.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>

> >> @@ -1664,6 +1668,8 @@ static int handle_intercept(S390CPU *cpu)
> >>              (long)cs->kvm_run->psw_addr);
> >>      switch (icpt_code) {
> >>          case ICPT_INSTRUCTION:
> >> +        case ICPT_PV_INSTR:
> >> +        case ICPT_PV_INSTR_NOTIFICATION:
> >>              r = handle_instruction(cpu, run);  
> > 
> > I'm still a bit uneasy about going through the same path for both 104
> > and 108. How does the handler figure out whether it should emulate an
> > instruction, or just process a notification? Is it guaranteed that a
> > given instruction is always showing up as either a 104 or a 108, so
> > that the handler can check the pv state?  
> 
> diag 308 subcode 0/1 are 108, but all other subcodes are defined as a
> 104 (if they are an exit at all)...

I think that's a reason to really split 108 from 4/104, or at least add
an parameter...

> 
> > 
> > [Even if that works, it still feels a bit unclean to me.]
> >   
> >>              break;
> >>          case ICPT_PROGRAM:  
> > 
> >   
> 
> 


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

Re: [PATCH v2 06/13] s390x: protvirt: KVM intercept changes

[Janosch Frank]

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

On 12/5/19 6:46 PM, Cornelia Huck wrote:
> On Thu, 5 Dec 2019 18:34:32 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On 12/5/19 6:15 PM, Cornelia Huck wrote:
>>> On Fri, 29 Nov 2019 04:48:02 -0500
>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>   
>>>> Secure guests no longer intercept with code 4 for an instruction
>>>> interception. Instead they have codes 104 and 108 for secure
>>>> instruction interception and secure instruction notification
>>>> respectively.
>>>>
>>>> The 104 mirrors the 4 interception.
>>>>
>>>> The 108 is a notification interception to let KVM and QEMU know that
>>>> something changed and we need to update tracking information or
>>>> perform specific tasks. It's currently taken for the following
>>>> instructions:
>>>>
>>>> * stpx (To inform about the changed prefix location)
>>>> * sclp (On incorrect SCCB values, so we can inject a IRQ)
>>>> * sigp (All but "stop and store status")
>>>> * diag308 (Subcodes 0/1)
>>>>
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>>>  target/s390x/kvm.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
> 
>>>> @@ -1664,6 +1668,8 @@ static int handle_intercept(S390CPU *cpu)
>>>>              (long)cs->kvm_run->psw_addr);
>>>>      switch (icpt_code) {
>>>>          case ICPT_INSTRUCTION:
>>>> +        case ICPT_PV_INSTR:
>>>> +        case ICPT_PV_INSTR_NOTIFICATION:
>>>>              r = handle_instruction(cpu, run);  
>>>
>>> I'm still a bit uneasy about going through the same path for both 104
>>> and 108. How does the handler figure out whether it should emulate an
>>> instruction, or just process a notification? Is it guaranteed that a
>>> given instruction is always showing up as either a 104 or a 108, so
>>> that the handler can check the pv state?  
>>
>> diag 308 subcode 0/1 are 108, but all other subcodes are defined as a
>> 104 (if they are an exit at all)...
> 
> I think that's a reason to really split 108 from 4/104, or at least add
> an parameter...

And still call the diag 308 handler or have separate handlers?

> 
>>
>>>
>>> [Even if that works, it still feels a bit unclean to me.]
>>>   
>>>>              break;
>>>>          case ICPT_PROGRAM:  
>>>
>>>   
>>
>>
> 



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

Re: [PATCH v2 06/13] s390x: protvirt: KVM intercept changes

[Cornelia Huck]

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

On Fri, 6 Dec 2019 08:44:52 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 12/5/19 6:46 PM, Cornelia Huck wrote:
> > On Thu, 5 Dec 2019 18:34:32 +0100
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> On 12/5/19 6:15 PM, Cornelia Huck wrote:  
> >>> On Fri, 29 Nov 2019 04:48:02 -0500
> >>> Janosch Frank <frankja@linux.ibm.com> wrote:
> >>>     
> >>>> Secure guests no longer intercept with code 4 for an instruction
> >>>> interception. Instead they have codes 104 and 108 for secure
> >>>> instruction interception and secure instruction notification
> >>>> respectively.
> >>>>
> >>>> The 104 mirrors the 4 interception.
> >>>>
> >>>> The 108 is a notification interception to let KVM and QEMU know that
> >>>> something changed and we need to update tracking information or
> >>>> perform specific tasks. It's currently taken for the following
> >>>> instructions:
> >>>>
> >>>> * stpx (To inform about the changed prefix location)
> >>>> * sclp (On incorrect SCCB values, so we can inject a IRQ)
> >>>> * sigp (All but "stop and store status")
> >>>> * diag308 (Subcodes 0/1)
> >>>>
> >>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >>>> ---
> >>>>  target/s390x/kvm.c | 6 ++++++
> >>>>  1 file changed, 6 insertions(+)
> >>>>  
> >   
> >>>> @@ -1664,6 +1668,8 @@ static int handle_intercept(S390CPU *cpu)
> >>>>              (long)cs->kvm_run->psw_addr);
> >>>>      switch (icpt_code) {
> >>>>          case ICPT_INSTRUCTION:
> >>>> +        case ICPT_PV_INSTR:
> >>>> +        case ICPT_PV_INSTR_NOTIFICATION:
> >>>>              r = handle_instruction(cpu, run);    
> >>>
> >>> I'm still a bit uneasy about going through the same path for both 104
> >>> and 108. How does the handler figure out whether it should emulate an
> >>> instruction, or just process a notification? Is it guaranteed that a
> >>> given instruction is always showing up as either a 104 or a 108, so
> >>> that the handler can check the pv state?    
> >>
> >> diag 308 subcode 0/1 are 108, but all other subcodes are defined as a
> >> 104 (if they are an exit at all)...  
> > 
> > I think that's a reason to really split 108 from 4/104, or at least add
> > an parameter...  
> 
> And still call the diag 308 handler or have separate handlers?

I'd probably split it into a "normal" one and one for pv special
handling... does that make sense?

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

Re: [PATCH v2 06/13] s390x: protvirt: KVM intercept changes

[Janosch Frank]

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

On 12/6/19 9:29 AM, Cornelia Huck wrote:
> On Fri, 6 Dec 2019 08:44:52 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On 12/5/19 6:46 PM, Cornelia Huck wrote:
>>> On Thu, 5 Dec 2019 18:34:32 +0100
>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>   
>>>> On 12/5/19 6:15 PM, Cornelia Huck wrote:  
>>>>> On Fri, 29 Nov 2019 04:48:02 -0500
>>>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>>>     
>>>>>> Secure guests no longer intercept with code 4 for an instruction
>>>>>> interception. Instead they have codes 104 and 108 for secure
>>>>>> instruction interception and secure instruction notification
>>>>>> respectively.
>>>>>>
>>>>>> The 104 mirrors the 4 interception.
>>>>>>
>>>>>> The 108 is a notification interception to let KVM and QEMU know that
>>>>>> something changed and we need to update tracking information or
>>>>>> perform specific tasks. It's currently taken for the following
>>>>>> instructions:
>>>>>>
>>>>>> * stpx (To inform about the changed prefix location)
>>>>>> * sclp (On incorrect SCCB values, so we can inject a IRQ)
>>>>>> * sigp (All but "stop and store status")
>>>>>> * diag308 (Subcodes 0/1)
>>>>>>
>>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>>> ---
>>>>>>  target/s390x/kvm.c | 6 ++++++
>>>>>>  1 file changed, 6 insertions(+)
>>>>>>  
>>>   
>>>>>> @@ -1664,6 +1668,8 @@ static int handle_intercept(S390CPU *cpu)
>>>>>>              (long)cs->kvm_run->psw_addr);
>>>>>>      switch (icpt_code) {
>>>>>>          case ICPT_INSTRUCTION:
>>>>>> +        case ICPT_PV_INSTR:
>>>>>> +        case ICPT_PV_INSTR_NOTIFICATION:
>>>>>>              r = handle_instruction(cpu, run);    
>>>>>
>>>>> I'm still a bit uneasy about going through the same path for both 104
>>>>> and 108. How does the handler figure out whether it should emulate an
>>>>> instruction, or just process a notification? Is it guaranteed that a
>>>>> given instruction is always showing up as either a 104 or a 108, so
>>>>> that the handler can check the pv state?    
>>>>
>>>> diag 308 subcode 0/1 are 108, but all other subcodes are defined as a
>>>> 104 (if they are an exit at all)...  
>>>
>>> I think that's a reason to really split 108 from 4/104, or at least add
>>> an parameter...  
>>
>> And still call the diag 308 handler or have separate handlers?
> 
> I'd probably split it into a "normal" one and one for pv special
> handling... does that make sense?
> 
IMHO: not really
We still need to do ipa/ipb parsing for both paths, which will result in
code duplication. Looking at diag308 subcode 4, we would have a code 4
one which just does the device resets and reboots and one which does all
that, plus the teardown of the protected guest.

I tried to inline as much as possible to have as little changes as
possible. Notable exception is sclp, which has more checks than
emulation code...



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

Re: [PATCH v2 06/13] s390x: protvirt: KVM intercept changes

[Cornelia Huck]

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

On Fri, 6 Dec 2019 09:45:41 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 12/6/19 9:29 AM, Cornelia Huck wrote:
> > On Fri, 6 Dec 2019 08:44:52 +0100
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> On 12/5/19 6:46 PM, Cornelia Huck wrote:  
> >>> On Thu, 5 Dec 2019 18:34:32 +0100
> >>> Janosch Frank <frankja@linux.ibm.com> wrote:
> >>>     
> >>>> On 12/5/19 6:15 PM, Cornelia Huck wrote:    
> >>>>> On Fri, 29 Nov 2019 04:48:02 -0500
> >>>>> Janosch Frank <frankja@linux.ibm.com> wrote:
> >>>>>       
> >>>>>> Secure guests no longer intercept with code 4 for an instruction
> >>>>>> interception. Instead they have codes 104 and 108 for secure
> >>>>>> instruction interception and secure instruction notification
> >>>>>> respectively.
> >>>>>>
> >>>>>> The 104 mirrors the 4 interception.
> >>>>>>
> >>>>>> The 108 is a notification interception to let KVM and QEMU know that
> >>>>>> something changed and we need to update tracking information or
> >>>>>> perform specific tasks. It's currently taken for the following
> >>>>>> instructions:
> >>>>>>
> >>>>>> * stpx (To inform about the changed prefix location)
> >>>>>> * sclp (On incorrect SCCB values, so we can inject a IRQ)
> >>>>>> * sigp (All but "stop and store status")
> >>>>>> * diag308 (Subcodes 0/1)
> >>>>>>
> >>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >>>>>> ---
> >>>>>>  target/s390x/kvm.c | 6 ++++++
> >>>>>>  1 file changed, 6 insertions(+)
> >>>>>>    
> >>>     
> >>>>>> @@ -1664,6 +1668,8 @@ static int handle_intercept(S390CPU *cpu)
> >>>>>>              (long)cs->kvm_run->psw_addr);
> >>>>>>      switch (icpt_code) {
> >>>>>>          case ICPT_INSTRUCTION:
> >>>>>> +        case ICPT_PV_INSTR:
> >>>>>> +        case ICPT_PV_INSTR_NOTIFICATION:
> >>>>>>              r = handle_instruction(cpu, run);      
> >>>>>
> >>>>> I'm still a bit uneasy about going through the same path for both 104
> >>>>> and 108. How does the handler figure out whether it should emulate an
> >>>>> instruction, or just process a notification? Is it guaranteed that a
> >>>>> given instruction is always showing up as either a 104 or a 108, so
> >>>>> that the handler can check the pv state?      
> >>>>
> >>>> diag 308 subcode 0/1 are 108, but all other subcodes are defined as a
> >>>> 104 (if they are an exit at all)...    
> >>>
> >>> I think that's a reason to really split 108 from 4/104, or at least add
> >>> an parameter...    
> >>
> >> And still call the diag 308 handler or have separate handlers?  
> > 
> > I'd probably split it into a "normal" one and one for pv special
> > handling... does that make sense?
> >   
> IMHO: not really
> We still need to do ipa/ipb parsing for both paths, which will result in
> code duplication. Looking at diag308 subcode 4, we would have a code 4
> one which just does the device resets and reboots and one which does all
> that, plus the teardown of the protected guest.
> 
> I tried to inline as much as possible to have as little changes as
> possible. Notable exception is sclp, which has more checks than
> emulation code...

Fair enough.

But taking a step back: What's the purpose of the new exits, then?
IIUC, we have the following cases:

- code 4: normal guest, nothing special
- code 104: protected guest, emulate the instruction
- code 108: protected guest, notification for the instruction

The backend code can figure out what to do simply by checking whether
the guest is protected or not (as whatever needs to be done is simply
determined by that anyway).

Are we overlooking something? Or is the information contained in the
different exits simply redundant?

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

Re: [PATCH v2 06/13] s390x: protvirt: KVM intercept changes

[Janosch Frank]

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

On 12/6/19 10:08 AM, Cornelia Huck wrote:
> On Fri, 6 Dec 2019 09:45:41 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On 12/6/19 9:29 AM, Cornelia Huck wrote:
>>> On Fri, 6 Dec 2019 08:44:52 +0100
>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>   
>>>> On 12/5/19 6:46 PM, Cornelia Huck wrote:  
>>>>> On Thu, 5 Dec 2019 18:34:32 +0100
>>>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>>>     
>>>>>> On 12/5/19 6:15 PM, Cornelia Huck wrote:    
>>>>>>> On Fri, 29 Nov 2019 04:48:02 -0500
>>>>>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>>>>>       
>>>>>>>> Secure guests no longer intercept with code 4 for an instruction
>>>>>>>> interception. Instead they have codes 104 and 108 for secure
>>>>>>>> instruction interception and secure instruction notification
>>>>>>>> respectively.
>>>>>>>>
>>>>>>>> The 104 mirrors the 4 interception.
>>>>>>>>
>>>>>>>> The 108 is a notification interception to let KVM and QEMU know that
>>>>>>>> something changed and we need to update tracking information or
>>>>>>>> perform specific tasks. It's currently taken for the following
>>>>>>>> instructions:
>>>>>>>>
>>>>>>>> * stpx (To inform about the changed prefix location)
>>>>>>>> * sclp (On incorrect SCCB values, so we can inject a IRQ)
>>>>>>>> * sigp (All but "stop and store status")
>>>>>>>> * diag308 (Subcodes 0/1)
>>>>>>>>
>>>>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>>>>> ---
>>>>>>>>  target/s390x/kvm.c | 6 ++++++
>>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>>    
>>>>>     
>>>>>>>> @@ -1664,6 +1668,8 @@ static int handle_intercept(S390CPU *cpu)
>>>>>>>>              (long)cs->kvm_run->psw_addr);
>>>>>>>>      switch (icpt_code) {
>>>>>>>>          case ICPT_INSTRUCTION:
>>>>>>>> +        case ICPT_PV_INSTR:
>>>>>>>> +        case ICPT_PV_INSTR_NOTIFICATION:
>>>>>>>>              r = handle_instruction(cpu, run);      
>>>>>>>
>>>>>>> I'm still a bit uneasy about going through the same path for both 104
>>>>>>> and 108. How does the handler figure out whether it should emulate an
>>>>>>> instruction, or just process a notification? Is it guaranteed that a
>>>>>>> given instruction is always showing up as either a 104 or a 108, so
>>>>>>> that the handler can check the pv state?      
>>>>>>
>>>>>> diag 308 subcode 0/1 are 108, but all other subcodes are defined as a
>>>>>> 104 (if they are an exit at all)...    
>>>>>
>>>>> I think that's a reason to really split 108 from 4/104, or at least add
>>>>> an parameter...    
>>>>
>>>> And still call the diag 308 handler or have separate handlers?  
>>>
>>> I'd probably split it into a "normal" one and one for pv special
>>> handling... does that make sense?
>>>   
>> IMHO: not really
>> We still need to do ipa/ipb parsing for both paths, which will result in
>> code duplication. Looking at diag308 subcode 4, we would have a code 4
>> one which just does the device resets and reboots and one which does all
>> that, plus the teardown of the protected guest.
>>
>> I tried to inline as much as possible to have as little changes as
>> possible. Notable exception is sclp, which has more checks than
>> emulation code...
> 
> Fair enough.
> 
> But taking a step back: What's the purpose of the new exits, then?
> IIUC, we have the following cases:
> 
> - code 4: normal guest, nothing special
> - code 104: protected guest, emulate the instruction
> - code 108: protected guest, notification for the instruction
> 
> The backend code can figure out what to do simply by checking whether
> the guest is protected or not (as whatever needs to be done is simply
> determined by that anyway).
> 
> Are we overlooking something? Or is the information contained in the
> different exits simply redundant?

The difference is in the entry after the exit:

On a 104 we have a "continuation", i.e. the data that's a result of the
emulation by KVM/QEMU is used to complete the instruction. Copying the
sccb from the satellite block into guest2 memory, etc.

For a 108 we don't have any special handling (except for maybe state
checking) and just continue with the next instruction.



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

[PATCH v2 07/13] s390x: protvirt: SCLP interpretation

[Janosch Frank]

SCLP for a protected guest is done over the SIDAD, so we need to use
the s390_cpu_virt_mem_* functions to access the SIDAD instead of guest
memory when reading/writing SCBs.

To not confuse the sclp emulation, we set 0x4000 as the SCCB address,
since the function that injects the sclp external interrupt would
reject a zero sccb address.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 hw/s390x/sclp.c         | 17 +++++++++++++++++
 include/hw/s390x/sclp.h |  2 ++
 target/s390x/kvm.c      |  5 +++++
 3 files changed, 24 insertions(+)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index f57ce7b739..ca71ace664 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -193,6 +193,23 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
     }
 }
 
+#define SCLP_PV_DUMMY_ADDR 0x4000
+int sclp_service_call_protected(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);
+
+    s390_cpu_virt_mem_read(env_archcpu(env), 0, 0, &work_sccb, sccb_len);
+    sclp_c->execute(sclp, &work_sccb, code);
+    s390_cpu_virt_mem_write(env_archcpu(env), 0, 0, &work_sccb,
+                            be16_to_cpu(work_sccb.h.length));
+    sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
+    return 0;
+}
+
 int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
 {
     SCLPDevice *sclp = get_sclp_device();
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index c54413b78c..c0a3faa37d 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -217,5 +217,7 @@ void s390_sclp_init(void);
 void sclp_service_interrupt(uint32_t sccb);
 void raise_irq_cpu_hotplug(void);
 int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
+int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
+                                uint32_t code);
 
 #endif
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 3d9c44ba9d..b802d02ff5 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1174,6 +1174,11 @@ static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
     sccb = env->regs[ipbh0 & 0xf];
     code = env->regs[(ipbh0 & 0xf0) >> 4];
 
+    if (run->s390_sieic.icptcode == ICPT_PV_INSTR) {
+        sclp_service_call_protected(env, sccb, code);
+        return;
+    }
+
     r = sclp_service_call(env, sccb, code);
     if (r < 0) {
         kvm_s390_program_interrupt(cpu, -r);
-- 
2.20.1

Re: [PATCH v2 07/13] s390x: protvirt: SCLP interpretation

[David Hildenbrand]

On 29.11.19 10:48, Janosch Frank wrote:
> SCLP for a protected guest is done over the SIDAD, so we need to use
> the s390_cpu_virt_mem_* functions to access the SIDAD instead of guest
> memory when reading/writing SCBs.

... Can you elaborate a bit more how that is going to be used? Did you
hack in special memory access to something called "SIDAD" via
s390_cpu_virt_mem_*?

I'd suggest a different access path ... especially because

a) it's confusing
b) it's unclear how exceptions apply

...

> 
> To not confuse the sclp emulation, we set 0x4000 as the SCCB address,
> since the function that injects the sclp external interrupt would
> reject a zero sccb address.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  hw/s390x/sclp.c         | 17 +++++++++++++++++
>  include/hw/s390x/sclp.h |  2 ++
>  target/s390x/kvm.c      |  5 +++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index f57ce7b739..ca71ace664 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -193,6 +193,23 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
>      }
>  }
>  
> +#define SCLP_PV_DUMMY_ADDR 0x4000
> +int sclp_service_call_protected(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);
> +
> +    s390_cpu_virt_mem_read(env_archcpu(env), 0, 0, &work_sccb, sccb_len);
> +    sclp_c->execute(sclp, &work_sccb, code);
> +    s390_cpu_virt_mem_write(env_archcpu(env), 0, 0, &work_sccb,
> +                            be16_to_cpu(work_sccb.h.length));

this access itself without handling exceptions looks dangerous as it is
completely unclear what's happening here.

> +    sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
> +    return 0;
> +}
> +
>  int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>  {
>      SCLPDevice *sclp = get_sclp_device();
> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
> index c54413b78c..c0a3faa37d 100644
> --- a/include/hw/s390x/sclp.h
> +++ b/include/hw/s390x/sclp.h
> @@ -217,5 +217,7 @@ void s390_sclp_init(void);
>  void sclp_service_interrupt(uint32_t sccb);
>  void raise_irq_cpu_hotplug(void);
>  int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
> +int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
> +                                uint32_t code);
>  
>  #endif
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 3d9c44ba9d..b802d02ff5 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1174,6 +1174,11 @@ static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
>      sccb = env->regs[ipbh0 & 0xf];
>      code = env->regs[(ipbh0 & 0xf0) >> 4];
>  
> +    if (run->s390_sieic.icptcode == ICPT_PV_INSTR) {

isn't checking against env->pv easier and cleaner?


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 07/13] s390x: protvirt: SCLP interpretation

[Janosch Frank]

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

On 11/29/19 11:43 AM, David Hildenbrand wrote:
> On 29.11.19 10:48, Janosch Frank wrote:
>> SCLP for a protected guest is done over the SIDAD, so we need to use
>> the s390_cpu_virt_mem_* functions to access the SIDAD instead of guest
>> memory when reading/writing SCBs.
> 
> ... Can you elaborate a bit more how that is going to be used? Did you
> hack in special memory access to something called "SIDAD" via
> s390_cpu_virt_mem_*?

For secure guests we can't ever access virtual guest memory, since we
have no access to the guest translation tables.

Hence we have the satellite block (SIDA) as a bounce buffer. SIE will
bounce referenced blocks of data (like the SCCB) over the SIDA.

The virt_mem functions go through the KVM mem op API. A KVM patch
reroutes mem op access to the SIDA. The alternative would be to map the
SIDA into vcpu_run.

> 
> I'd suggest a different access path ... especially because
> 
> a) it's confusing

Granted, there's a lot of inherent knowledge behind these patches.
And looking at my past answers to the KVM intercept patch I already
forgot lots of it.

> b) it's unclear how exceptions apply

There are no PGM exceptions, as they are pre-checked and reported by
SIE. There are however errors that the mem op API can return.

> 
> ...
> 
>>
>> To not confuse the sclp emulation, we set 0x4000 as the SCCB address,
>> since the function that injects the sclp external interrupt would
>> reject a zero sccb address.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  hw/s390x/sclp.c         | 17 +++++++++++++++++
>>  include/hw/s390x/sclp.h |  2 ++
>>  target/s390x/kvm.c      |  5 +++++
>>  3 files changed, 24 insertions(+)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index f57ce7b739..ca71ace664 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -193,6 +193,23 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
>>      }
>>  }
>>  
>> +#define SCLP_PV_DUMMY_ADDR 0x4000
>> +int sclp_service_call_protected(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);
>> +
>> +    s390_cpu_virt_mem_read(env_archcpu(env), 0, 0, &work_sccb, sccb_len);
>> +    sclp_c->execute(sclp, &work_sccb, code);
>> +    s390_cpu_virt_mem_write(env_archcpu(env), 0, 0, &work_sccb,
>> +                            be16_to_cpu(work_sccb.h.length));
> 
> this access itself without handling exceptions looks dangerous as it is
> completely unclear what's happening here.

See above

> 
>> +    sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
>> +    return 0;
>> +}
>> +
>>  int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>>  {
>>      SCLPDevice *sclp = get_sclp_device();
>> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
>> index c54413b78c..c0a3faa37d 100644
>> --- a/include/hw/s390x/sclp.h
>> +++ b/include/hw/s390x/sclp.h
>> @@ -217,5 +217,7 @@ void s390_sclp_init(void);
>>  void sclp_service_interrupt(uint32_t sccb);
>>  void raise_irq_cpu_hotplug(void);
>>  int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
>> +int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>> +                                uint32_t code);
>>  
>>  #endif
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index 3d9c44ba9d..b802d02ff5 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -1174,6 +1174,11 @@ static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
>>      sccb = env->regs[ipbh0 & 0xf];
>>      code = env->regs[(ipbh0 & 0xf0) >> 4];
>>  
>> +    if (run->s390_sieic.icptcode == ICPT_PV_INSTR) {
> 
> isn't checking against env->pv easier and cleaner?

Hmm, I dislike checking a global state for a CPU icpt.

> 
> 



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

Re: [PATCH v2 07/13] s390x: protvirt: SCLP interpretation

[David Hildenbrand]

On 29.11.19 12:15, Janosch Frank wrote:
> On 11/29/19 11:43 AM, David Hildenbrand wrote:
>> On 29.11.19 10:48, Janosch Frank wrote:
>>> SCLP for a protected guest is done over the SIDAD, so we need to use
>>> the s390_cpu_virt_mem_* functions to access the SIDAD instead of guest
>>> memory when reading/writing SCBs.
>>
>> ... Can you elaborate a bit more how that is going to be used? Did you
>> hack in special memory access to something called "SIDAD" via
>> s390_cpu_virt_mem_*?
> 
> For secure guests we can't ever access virtual guest memory, since we
> have no access to the guest translation tables.
> 
> Hence we have the satellite block (SIDA) as a bounce buffer. SIE will
> bounce referenced blocks of data (like the SCCB) over the SIDA.
> 
> The virt_mem functions go through the KVM mem op API. A KVM patch
> reroutes mem op access to the SIDA. The alternative would be to map the
> SIDA into vcpu_run.
> 

I'd prefer *anything* over going via  s390_cpu_virt_mem_*, because as
you say "For secure guests we can't ever access virtual guest memory".
Introduce a new interface or go via vcpu_run. IMHO that's much cleaner.



-- 
Thanks,

David / dhildenb

[PATCH v2 08/13] s390x: protvirt: Add new VCPU reset functions

[Janosch Frank]

CPU resets for protected guests need to be done via Ultravisor
calls. Hence we need a way to issue these calls for each reset.

As we formerly had only one reset function and it was called for
initial, as well as for the clear reset, we now need a new interface.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/cpu.c       | 14 ++++++++++++--
 target/s390x/kvm-stub.c  | 10 +++++++++-
 target/s390x/kvm.c       | 38 ++++++++++++++++++++++++++++++++------
 target/s390x/kvm_s390x.h |  4 +++-
 4 files changed, 56 insertions(+), 10 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index bd39cb54b7..52fefa1586 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -131,8 +131,18 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
     }
 
     /* Reset state inside the kernel that we cannot access yet from QEMU. */
-    if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) {
-        kvm_s390_reset_vcpu(cpu);
+    if (kvm_enabled()) {
+        switch (type) {
+        case S390_CPU_RESET_CLEAR:
+            kvm_s390_reset_vcpu_clear(cpu);
+            break;
+        case S390_CPU_RESET_INITIAL:
+            kvm_s390_reset_vcpu_initial(cpu);
+            break;
+        case S390_CPU_RESET_NORMAL:
+            kvm_s390_reset_vcpu_normal(cpu);
+            break;
+        }
     }
 }
 
diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c
index 5152e2bdf1..c4cd497f85 100644
--- a/target/s390x/kvm-stub.c
+++ b/target/s390x/kvm-stub.c
@@ -83,7 +83,15 @@ void kvm_s390_cmma_reset(void)
 {
 }
 
-void kvm_s390_reset_vcpu(S390CPU *cpu)
+void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
+{
+}
+
+void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
+{
+}
+
+void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
 {
 }
 
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index b802d02ff5..5b1ed3acb4 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -154,6 +154,7 @@ static int cap_ri;
 static int cap_gs;
 static int cap_hpage_1m;
 static int cap_protvirt;
+static int cap_vcpu_resets;
 
 static int active_cmma;
 
@@ -346,6 +347,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
     cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
     cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
     cap_protvirt = kvm_check_extension(s, KVM_CAP_S390_PROTECTED);
+    cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
 
     if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
         || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
@@ -407,20 +409,44 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
     return 0;
 }
 
-void kvm_s390_reset_vcpu(S390CPU *cpu)
+static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
 {
     CPUState *cs = CPU(cpu);
 
-    /* The initial reset call is needed here to reset in-kernel
-     * vcpu data that we can't access directly from QEMU
-     * (i.e. with older kernels which don't support sync_regs/ONE_REG).
-     * Before this ioctl cpu_synchronize_state() is called in common kvm
-     * code (kvm-all) */
+    /*
+     * The reset call is needed here to reset in-kernel vcpu data that
+     * we can't access directly from QEMU (i.e. with older kernels
+     * which don't support sync_regs/ONE_REG).  Before this ioctl
+     * cpu_synchronize_state() is called in common kvm code
+     * (kvm-all).
+     */
+    if (cap_vcpu_resets) {
+        if (kvm_vcpu_ioctl(cs, KVM_S390_VCPU_RESET, type)) {
+            error_report("CPU reset type %ld failed on CPU %i",
+                         type, cs->cpu_index);
+        }
+        return;
+    }
     if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
         error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
     }
 }
 
+void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
+{
+    kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_INITIAL);
+}
+
+void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
+{
+    kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_CLEAR);
+}
+
+void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
+{
+    kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_NORMAL);
+}
+
 static int can_sync_regs(CPUState *cs, int regs)
 {
     return cap_sync_regs && (cs->kvm_run->kvm_valid_regs & regs) == regs;
diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
index caf985955b..0b21789796 100644
--- a/target/s390x/kvm_s390x.h
+++ b/target/s390x/kvm_s390x.h
@@ -34,7 +34,9 @@ int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
                                     int vq, bool assign);
 int kvm_s390_cmma_active(void);
 void kvm_s390_cmma_reset(void);
-void kvm_s390_reset_vcpu(S390CPU *cpu);
+void kvm_s390_reset_vcpu_clear(S390CPU *cpu);
+void kvm_s390_reset_vcpu_normal(S390CPU *cpu);
+void kvm_s390_reset_vcpu_initial(S390CPU *cpu);
 int kvm_s390_set_mem_limit(uint64_t new_limit, uint64_t *hw_limit);
 void kvm_s390_set_max_pagesize(uint64_t pagesize, Error **errp);
 void kvm_s390_crypto_reset(void);
-- 
2.20.1

Re: [PATCH v2 08/13] s390x: protvirt: Add new VCPU reset functions

[David Hildenbrand]

On 29.11.19 10:48, Janosch Frank wrote:
> CPU resets for protected guests need to be done via Ultravisor
> calls. Hence we need a way to issue these calls for each reset.
> 
> As we formerly had only one reset function and it was called for
> initial, as well as for the clear reset, we now need a new interface.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/cpu.c       | 14 ++++++++++++--
>  target/s390x/kvm-stub.c  | 10 +++++++++-
>  target/s390x/kvm.c       | 38 ++++++++++++++++++++++++++++++++------
>  target/s390x/kvm_s390x.h |  4 +++-
>  4 files changed, 56 insertions(+), 10 deletions(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index bd39cb54b7..52fefa1586 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -131,8 +131,18 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
>      }
>  
>      /* Reset state inside the kernel that we cannot access yet from QEMU. */
> -    if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) {
> -        kvm_s390_reset_vcpu(cpu);
> +    if (kvm_enabled()) {
> +        switch (type) {
> +        case S390_CPU_RESET_CLEAR:
> +            kvm_s390_reset_vcpu_clear(cpu);
> +            break;
> +        case S390_CPU_RESET_INITIAL:
> +            kvm_s390_reset_vcpu_initial(cpu);
> +            break;
> +        case S390_CPU_RESET_NORMAL:
> +            kvm_s390_reset_vcpu_normal(cpu);
> +            break;
> +        }

I would have assumed you only have to do that for pv? For ordinary
guests we can avoid unnecessary ioctls IMHO.

>      }
>  }
>  
> diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c
> index 5152e2bdf1..c4cd497f85 100644
> --- a/target/s390x/kvm-stub.c
> +++ b/target/s390x/kvm-stub.c
> @@ -83,7 +83,15 @@ void kvm_s390_cmma_reset(void)
>  {
>  }
>  
> -void kvm_s390_reset_vcpu(S390CPU *cpu)
> +void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
> +{
> +}
> +
> +void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
> +{
> +}
> +
> +void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
>  {
>  }
>  
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index b802d02ff5..5b1ed3acb4 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -154,6 +154,7 @@ static int cap_ri;
>  static int cap_gs;
>  static int cap_hpage_1m;
>  static int cap_protvirt;
> +static int cap_vcpu_resets;
>  
>  static int active_cmma;
>  
> @@ -346,6 +347,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
>      cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
>      cap_protvirt = kvm_check_extension(s, KVM_CAP_S390_PROTECTED);
> +    cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
>  
>      if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>          || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
> @@ -407,20 +409,44 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
>      return 0;
>  }
>  
> -void kvm_s390_reset_vcpu(S390CPU *cpu)
> +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
>  {
>      CPUState *cs = CPU(cpu);
>  
> -    /* The initial reset call is needed here to reset in-kernel
> -     * vcpu data that we can't access directly from QEMU
> -     * (i.e. with older kernels which don't support sync_regs/ONE_REG).
> -     * Before this ioctl cpu_synchronize_state() is called in common kvm
> -     * code (kvm-all) */
> +    /*
> +     * The reset call is needed here to reset in-kernel vcpu data that
> +     * we can't access directly from QEMU (i.e. with older kernels
> +     * which don't support sync_regs/ONE_REG).  Before this ioctl
> +     * cpu_synchronize_state() is called in common kvm code
> +     * (kvm-all).
> +     */
> +    if (cap_vcpu_resets) {
> +        if (kvm_vcpu_ioctl(cs, KVM_S390_VCPU_RESET, type)) {
> +            error_report("CPU reset type %ld failed on CPU %i",
> +                         type, cs->cpu_index);
> +        }
> +        return;> +    }

This is broken for S390_CPU_RESET_NORMAL where we don't do a
KVM_S390_INITIAL_RESET for !pv ...

Can't we limit that new handling to pv only and keep it out of this path?


[...]


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 08/13] s390x: protvirt: Add new VCPU reset functions

[Janosch Frank]

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

On 11/29/19 11:47 AM, David Hildenbrand wrote:
> On 29.11.19 10:48, Janosch Frank wrote:
>> CPU resets for protected guests need to be done via Ultravisor
>> calls. Hence we need a way to issue these calls for each reset.
>>
>> As we formerly had only one reset function and it was called for
>> initial, as well as for the clear reset, we now need a new interface.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  target/s390x/cpu.c       | 14 ++++++++++++--
>>  target/s390x/kvm-stub.c  | 10 +++++++++-
>>  target/s390x/kvm.c       | 38 ++++++++++++++++++++++++++++++++------
>>  target/s390x/kvm_s390x.h |  4 +++-
>>  4 files changed, 56 insertions(+), 10 deletions(-)
>>
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index bd39cb54b7..52fefa1586 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -131,8 +131,18 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
>>      }
>>  
>>      /* Reset state inside the kernel that we cannot access yet from QEMU. */
>> -    if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) {
>> -        kvm_s390_reset_vcpu(cpu);
>> +    if (kvm_enabled()) {
>> +        switch (type) {
>> +        case S390_CPU_RESET_CLEAR:
>> +            kvm_s390_reset_vcpu_clear(cpu);
>> +            break;
>> +        case S390_CPU_RESET_INITIAL:
>> +            kvm_s390_reset_vcpu_initial(cpu);
>> +            break;
>> +        case S390_CPU_RESET_NORMAL:
>> +            kvm_s390_reset_vcpu_normal(cpu);
>> +            break;
>> +        }
> 
> I would have assumed you only have to do that for pv? For ordinary
> guests we can avoid unnecessary ioctls IMHO.

Remember the reset normal IRQ fix in KVM?
Without it we're not architecture compliant.

> 
>>      }
>>  }
>>  
>> diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c
>> index 5152e2bdf1..c4cd497f85 100644
>> --- a/target/s390x/kvm-stub.c
>> +++ b/target/s390x/kvm-stub.c
>> @@ -83,7 +83,15 @@ void kvm_s390_cmma_reset(void)
>>  {
>>  }
>>  
>> -void kvm_s390_reset_vcpu(S390CPU *cpu)
>> +void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
>> +{
>> +}
>> +
>> +void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
>> +{
>> +}
>> +
>> +void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
>>  {
>>  }
>>  
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index b802d02ff5..5b1ed3acb4 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -154,6 +154,7 @@ static int cap_ri;
>>  static int cap_gs;
>>  static int cap_hpage_1m;
>>  static int cap_protvirt;
>> +static int cap_vcpu_resets;
>>  
>>  static int active_cmma;
>>  
>> @@ -346,6 +347,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>      cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
>>      cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
>>      cap_protvirt = kvm_check_extension(s, KVM_CAP_S390_PROTECTED);
>> +    cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
>>  
>>      if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>>          || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
>> @@ -407,20 +409,44 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
>>      return 0;
>>  }
>>  
>> -void kvm_s390_reset_vcpu(S390CPU *cpu)
>> +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
>>  {
>>      CPUState *cs = CPU(cpu);
>>  
>> -    /* The initial reset call is needed here to reset in-kernel
>> -     * vcpu data that we can't access directly from QEMU
>> -     * (i.e. with older kernels which don't support sync_regs/ONE_REG).
>> -     * Before this ioctl cpu_synchronize_state() is called in common kvm
>> -     * code (kvm-all) */
>> +    /*
>> +     * The reset call is needed here to reset in-kernel vcpu data that
>> +     * we can't access directly from QEMU (i.e. with older kernels
>> +     * which don't support sync_regs/ONE_REG).  Before this ioctl
>> +     * cpu_synchronize_state() is called in common kvm code
>> +     * (kvm-all).
>> +     */
>> +    if (cap_vcpu_resets) {
>> +        if (kvm_vcpu_ioctl(cs, KVM_S390_VCPU_RESET, type)) {
>> +            error_report("CPU reset type %ld failed on CPU %i",
>> +                         type, cs->cpu_index);
>> +        }
>> +        return;> +    }
> 
> This is broken for S390_CPU_RESET_NORMAL where we don't do a
> KVM_S390_INITIAL_RESET for !pv ...
> 
> Can't we limit that new handling to pv only and keep it out of this path?

Look above

> 
> 
> [...]
> 
> 



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

Re: [PATCH v2 08/13] s390x: protvirt: Add new VCPU reset functions

[David Hildenbrand]

On 29.11.19 12:21, Janosch Frank wrote:
> On 11/29/19 11:47 AM, David Hildenbrand wrote:
>> On 29.11.19 10:48, Janosch Frank wrote:
>>> CPU resets for protected guests need to be done via Ultravisor
>>> calls. Hence we need a way to issue these calls for each reset.
>>>
>>> As we formerly had only one reset function and it was called for
>>> initial, as well as for the clear reset, we now need a new interface.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>  target/s390x/cpu.c       | 14 ++++++++++++--
>>>  target/s390x/kvm-stub.c  | 10 +++++++++-
>>>  target/s390x/kvm.c       | 38 ++++++++++++++++++++++++++++++++------
>>>  target/s390x/kvm_s390x.h |  4 +++-
>>>  4 files changed, 56 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>>> index bd39cb54b7..52fefa1586 100644
>>> --- a/target/s390x/cpu.c
>>> +++ b/target/s390x/cpu.c
>>> @@ -131,8 +131,18 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
>>>      }
>>>  
>>>      /* Reset state inside the kernel that we cannot access yet from QEMU. */
>>> -    if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) {
>>> -        kvm_s390_reset_vcpu(cpu);
>>> +    if (kvm_enabled()) {
>>> +        switch (type) {
>>> +        case S390_CPU_RESET_CLEAR:
>>> +            kvm_s390_reset_vcpu_clear(cpu);
>>> +            break;
>>> +        case S390_CPU_RESET_INITIAL:
>>> +            kvm_s390_reset_vcpu_initial(cpu);
>>> +            break;
>>> +        case S390_CPU_RESET_NORMAL:
>>> +            kvm_s390_reset_vcpu_normal(cpu);
>>> +            break;
>>> +        }
>>
>> I would have assumed you only have to do that for pv? For ordinary
>> guests we can avoid unnecessary ioctls IMHO.
> 
> Remember the reset normal IRQ fix in KVM?
> Without it we're not architecture compliant.

Please spell that out somewhere. People reviewing this have no idea
what's going on under the hood :)


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 08/13] s390x: protvirt: Add new VCPU reset functions

[Thomas Huth]

On 29/11/2019 10.48, Janosch Frank wrote:
> CPU resets for protected guests need to be done via Ultravisor
> calls. Hence we need a way to issue these calls for each reset.
> 
> As we formerly had only one reset function and it was called for
> initial, as well as for the clear reset, we now need a new interface.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
[...]
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index b802d02ff5..5b1ed3acb4 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -154,6 +154,7 @@ static int cap_ri;
>  static int cap_gs;
>  static int cap_hpage_1m;
>  static int cap_protvirt;
> +static int cap_vcpu_resets;
>  
>  static int active_cmma;
>  
> @@ -346,6 +347,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
>      cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
>      cap_protvirt = kvm_check_extension(s, KVM_CAP_S390_PROTECTED);
> +    cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
>  
>      if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>          || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
> @@ -407,20 +409,44 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
>      return 0;
>  }
>  
> -void kvm_s390_reset_vcpu(S390CPU *cpu)
> +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
>  {
>      CPUState *cs = CPU(cpu);
>  
> -    /* The initial reset call is needed here to reset in-kernel
> -     * vcpu data that we can't access directly from QEMU
> -     * (i.e. with older kernels which don't support sync_regs/ONE_REG).
> -     * Before this ioctl cpu_synchronize_state() is called in common kvm
> -     * code (kvm-all) */
> +    /*
> +     * The reset call is needed here to reset in-kernel vcpu data that
> +     * we can't access directly from QEMU (i.e. with older kernels
> +     * which don't support sync_regs/ONE_REG).  Before this ioctl
> +     * cpu_synchronize_state() is called in common kvm code
> +     * (kvm-all).
> +     */
> +    if (cap_vcpu_resets) {
> +        if (kvm_vcpu_ioctl(cs, KVM_S390_VCPU_RESET, type)) {
> +            error_report("CPU reset type %ld failed on CPU %i",
> +                         type, cs->cpu_index);
> +        }
> +        return;
> +    }
>      if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
>          error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
>      }

Don't you want to check for type != KVM_S390_VCPU_RESET_NORMAL before
doing the INITIAL_RESET here?

>  }
>  
> +void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
> +{
> +    kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_INITIAL);
> +}
> +
> +void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
> +{
> +    kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_CLEAR);
> +}
> +
> +void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
> +{
> +    kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_NORMAL);
> +}

... otherwise this might reset more things than expected?

Or do I miss something here?

 Thomas

Re: [PATCH v2 08/13] s390x: protvirt: Add new VCPU reset functions

[Janosch Frank]

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

On 12/4/19 12:58 PM, Thomas Huth wrote:
> On 29/11/2019 10.48, Janosch Frank wrote:
>> CPU resets for protected guests need to be done via Ultravisor
>> calls. Hence we need a way to issue these calls for each reset.
>>
>> As we formerly had only one reset function and it was called for
>> initial, as well as for the clear reset, we now need a new interface.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
> [...]
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index b802d02ff5..5b1ed3acb4 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -154,6 +154,7 @@ static int cap_ri;
>>  static int cap_gs;
>>  static int cap_hpage_1m;
>>  static int cap_protvirt;
>> +static int cap_vcpu_resets;
>>  
>>  static int active_cmma;
>>  
>> @@ -346,6 +347,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>      cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
>>      cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
>>      cap_protvirt = kvm_check_extension(s, KVM_CAP_S390_PROTECTED);
>> +    cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
>>  
>>      if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>>          || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
>> @@ -407,20 +409,44 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
>>      return 0;
>>  }
>>  
>> -void kvm_s390_reset_vcpu(S390CPU *cpu)
>> +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
>>  {
>>      CPUState *cs = CPU(cpu);
>>  
>> -    /* The initial reset call is needed here to reset in-kernel
>> -     * vcpu data that we can't access directly from QEMU
>> -     * (i.e. with older kernels which don't support sync_regs/ONE_REG).
>> -     * Before this ioctl cpu_synchronize_state() is called in common kvm
>> -     * code (kvm-all) */
>> +    /*
>> +     * The reset call is needed here to reset in-kernel vcpu data that
>> +     * we can't access directly from QEMU (i.e. with older kernels
>> +     * which don't support sync_regs/ONE_REG).  Before this ioctl
>> +     * cpu_synchronize_state() is called in common kvm code
>> +     * (kvm-all).
>> +     */
>> +    if (cap_vcpu_resets) {
>> +        if (kvm_vcpu_ioctl(cs, KVM_S390_VCPU_RESET, type)) {
>> +            error_report("CPU reset type %ld failed on CPU %i",
>> +                         type, cs->cpu_index);
>> +        }
>> +        return;
>> +    }
>>      if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
>>          error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
>>      }
> 
> Don't you want to check for type != KVM_S390_VCPU_RESET_NORMAL before
> doing the INITIAL_RESET here?
> 
>>  }
>>  
>> +void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
>> +{
>> +    kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_INITIAL);
>> +}
>> +
>> +void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
>> +{
>> +    kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_CLEAR);
>> +}
>> +
>> +void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
>> +{
>> +    kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_NORMAL);
>> +}
> 
> ... otherwise this might reset more things than expected?
> 
> Or do I miss something here?

Hey Thomas I split out this patch into the "architectural compliance"
patch series. Have a look into there, this is an old version.

> 
>  Thomas
> 



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

[PATCH v2 09/13] s390x: Exit on vcpu reset error

[Janosch Frank]

If a vcpu is not properly reset it might be better to just end the VM.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/kvm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 5b1ed3acb4..15b041a601 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -424,11 +424,13 @@ static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
         if (kvm_vcpu_ioctl(cs, KVM_S390_VCPU_RESET, type)) {
             error_report("CPU reset type %ld failed on CPU %i",
                          type, cs->cpu_index);
+            exit(1);
         }
         return;
     }
     if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
         error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
+        exit(1);
     }
 }
 
-- 
2.20.1

[PATCH v2 10/13] s390x: protvirt: Set guest IPL PSW

[Janosch Frank]

Handling of CPU reset and setting of the IPL psw from guest storage at
offset 0 is done by a Ultravisor call. Let's only fetch it if
necessary.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/s390x/pv.c              | 5 +++++
 hw/s390x/pv.h              | 1 +
 hw/s390x/s390-virtio-ccw.c | 2 +-
 linux-headers/linux/kvm.h  | 1 +
 target/s390x/cpu.c         | 9 ++++++++-
 5 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
index 0218070322..106252833f 100644
--- a/hw/s390x/pv.c
+++ b/hw/s390x/pv.c
@@ -88,6 +88,11 @@ int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
     return s390_pv_cmd(KVM_PV_VM_SET_SEC_PARMS, &args);
 }
 
+int s390_pv_set_ipl_psw(CPUState *cs)
+{
+    return s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_SET_IPL_PSW, NULL);
+}
+
 /*
  * Called for each component in the SE type IPL parameter block 0.
  */
diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
index eb074e4bc9..e670c67270 100644
--- a/hw/s390x/pv.h
+++ b/hw/s390x/pv.h
@@ -18,6 +18,7 @@ int s390_pv_vm_destroy(void);
 int s390_pv_vcpu_destroy(CPUState *cs);
 int s390_pv_vcpu_create(CPUState *cs);
 int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
+int s390_pv_set_ipl_psw(CPUState *cs);
 int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
 int s390_pv_perf_clear_reset(void);
 int s390_pv_verify(void);
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 6fcd695b81..1133de9423 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -408,7 +408,7 @@ static void s390_machine_reset(MachineState *machine)
         /* Verify integrity */
         s390_pv_verify();
         env->pv = true;
-        s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
+        run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
         break;
     default:
         g_assert_not_reached();
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 4448d59960..7c6118c703 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1484,6 +1484,7 @@ enum pv_cmd_id {
 	KVM_PV_VM_UNSHARE,
 	KVM_PV_VCPU_CREATE,
 	KVM_PV_VCPU_DESTROY,
+	KVM_PV_VCPU_SET_IPL_PSW,
 };
 
 struct kvm_pv_cmd {
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 52fefa1586..8c673dab2c 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -37,6 +37,7 @@
 #include "sysemu/hw_accel.h"
 #include "hw/qdev-properties.h"
 #ifndef CONFIG_USER_ONLY
+#include "hw/s390x/pv.h"
 #include "hw/boards.h"
 #include "sysemu/arch_init.h"
 #include "sysemu/sysemu.h"
@@ -76,7 +77,13 @@ static bool s390_cpu_has_work(CPUState *cs)
 static void s390_cpu_load_normal(CPUState *s)
 {
     S390CPU *cpu = S390_CPU(s);
-    cpu->env.psw.addr = ldl_phys(s->as, 4) & PSW_MASK_ESA_ADDR;
+    CPUS390XState *env = &cpu->env;
+
+    if (!env->pv) {
+        cpu->env.psw.addr = ldl_phys(s->as, 4) & PSW_MASK_ESA_ADDR;
+    } else {
+        s390_pv_set_ipl_psw(s);
+    }
     cpu->env.psw.mask = PSW_MASK_32 | PSW_MASK_64;
     s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
 }
-- 
2.20.1

Re: [PATCH v2 10/13] s390x: protvirt: Set guest IPL PSW

[David Hildenbrand]

On 29.11.19 10:48, Janosch Frank wrote:
> Handling of CPU reset and setting of the IPL psw from guest storage at
> offset 0 is done by a Ultravisor call. Let's only fetch it if
> necessary.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/s390x/pv.c              | 5 +++++
>  hw/s390x/pv.h              | 1 +
>  hw/s390x/s390-virtio-ccw.c | 2 +-
>  linux-headers/linux/kvm.h  | 1 +
>  target/s390x/cpu.c         | 9 ++++++++-
>  5 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> index 0218070322..106252833f 100644
> --- a/hw/s390x/pv.c
> +++ b/hw/s390x/pv.c
> @@ -88,6 +88,11 @@ int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
>      return s390_pv_cmd(KVM_PV_VM_SET_SEC_PARMS, &args);
>  }
>  
> +int s390_pv_set_ipl_psw(CPUState *cs)
> +{
> +    return s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_SET_IPL_PSW, NULL);
> +}
> +
>  /*
>   * Called for each component in the SE type IPL parameter block 0.
>   */
> diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
> index eb074e4bc9..e670c67270 100644
> --- a/hw/s390x/pv.h
> +++ b/hw/s390x/pv.h
> @@ -18,6 +18,7 @@ int s390_pv_vm_destroy(void);
>  int s390_pv_vcpu_destroy(CPUState *cs);
>  int s390_pv_vcpu_create(CPUState *cs);
>  int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
> +int s390_pv_set_ipl_psw(CPUState *cs);
>  int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
>  int s390_pv_perf_clear_reset(void);
>  int s390_pv_verify(void);
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 6fcd695b81..1133de9423 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -408,7 +408,7 @@ static void s390_machine_reset(MachineState *machine)
>          /* Verify integrity */
>          s390_pv_verify();
>          env->pv = true;
> -        s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
> +        run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>          break;
>      default:
>          g_assert_not_reached();
> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> index 4448d59960..7c6118c703 100644
> --- a/linux-headers/linux/kvm.h
> +++ b/linux-headers/linux/kvm.h
> @@ -1484,6 +1484,7 @@ enum pv_cmd_id {
>  	KVM_PV_VM_UNSHARE,
>  	KVM_PV_VCPU_CREATE,
>  	KVM_PV_VCPU_DESTROY,
> +	KVM_PV_VCPU_SET_IPL_PSW,
>  };
>  
>  struct kvm_pv_cmd {
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 52fefa1586..8c673dab2c 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -37,6 +37,7 @@
>  #include "sysemu/hw_accel.h"
>  #include "hw/qdev-properties.h"
>  #ifndef CONFIG_USER_ONLY
> +#include "hw/s390x/pv.h"
>  #include "hw/boards.h"
>  #include "sysemu/arch_init.h"
>  #include "sysemu/sysemu.h"
> @@ -76,7 +77,13 @@ static bool s390_cpu_has_work(CPUState *cs)
>  static void s390_cpu_load_normal(CPUState *s)
>  {
>      S390CPU *cpu = S390_CPU(s);
> -    cpu->env.psw.addr = ldl_phys(s->as, 4) & PSW_MASK_ESA_ADDR;
> +    CPUS390XState *env = &cpu->env;
> +
> +    if (!env->pv) {
> +        cpu->env.psw.addr = ldl_phys(s->as, 4) & PSW_MASK_ESA_ADDR;
> +    } else {
> +        s390_pv_set_ipl_psw(s);
> +    }
>      cpu->env.psw.mask = PSW_MASK_32 | PSW_MASK_64;

Isn't that dead code for pv? AFAIKS, the psw is not synced back ...
maybe we should.

>      s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>  }
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 10/13] s390x: protvirt: Set guest IPL PSW

[David Hildenbrand]

>      S390CPU *cpu = S390_CPU(s);
> -    cpu->env.psw.addr = ldl_phys(s->as, 4) & PSW_MASK_ESA_ADDR;
> +    CPUS390XState *env = &cpu->env;
> +
> +    if (!env->pv) {
> +        cpu->env.psw.addr = ldl_phys(s->as, 4) & PSW_MASK_ESA_ADDR;
> +    } else {
> +        s390_pv_set_ipl_psw(s);

Oh, and you ignore any errors you get here ... not sure if that is
intended (error and exit?)

> +    }
>      cpu->env.psw.mask = PSW_MASK_32 | PSW_MASK_64;
>      s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>  }
> 


-- 
Thanks,

David / dhildenb

[PATCH v2 11/13] s390x: protvirt: Move diag 308 data over SIDAD

[Janosch Frank]

For protected guests the IPIB is written/read to/from the satellite
block, so we need to make those accesses virtual to make them go
through KVM mem ops.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/diag.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index 5489fc721a..6d78759151 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -88,6 +88,7 @@ static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
 void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
 {
     CPUState *cs = env_cpu(env);
+    S390CPU *cpu = S390_CPU(cs);
     uint64_t addr =  env->regs[r1];
     uint64_t subcode = env->regs[r3];
     IplParameterBlock *iplb;
@@ -118,14 +119,27 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
         if (diag308_parm_check(env, r1, addr, ra, false)) {
             return;
         }
+
         iplb = g_new0(IplParameterBlock, 1);
-        cpu_physical_memory_read(addr, iplb, sizeof(iplb->len));
+        if (!env->pv) {
+            cpu_physical_memory_read(addr, iplb, sizeof(iplb->len));
+        } else {
+            s390_cpu_virt_mem_read(cpu, 0, 0, iplb, sizeof(iplb->len));
+            s390_cpu_virt_mem_handle_exc(cpu, ra);
+        }
+
         if (!iplb_valid_len(iplb)) {
             env->regs[r1 + 1] = DIAG_308_RC_INVALID;
             goto out;
         }
 
-        cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
+        if (!env->pv) {
+            cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
+        } else {
+            s390_cpu_virt_mem_read(cpu, 0, 0, iplb, be32_to_cpu(iplb->len));
+            s390_cpu_virt_mem_handle_exc(cpu, ra);
+        }
+
 
         if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb) &&
             !(iplb_valid_se(iplb) && s390_ipl_pv_check_comp(iplb) >= 0)) {
@@ -149,7 +163,13 @@ out:
             iplb = s390_ipl_get_iplb();
         }
         if (iplb) {
-            cpu_physical_memory_write(addr, iplb, be32_to_cpu(iplb->len));
+            if (!env->pv) {
+                cpu_physical_memory_write(addr, iplb, be32_to_cpu(iplb->len));
+            } else {
+                s390_cpu_virt_mem_write(cpu, 0, 0, iplb,
+                                        be32_to_cpu(iplb->len));
+                s390_cpu_virt_mem_handle_exc(cpu, ra);
+            }
             env->regs[r1 + 1] = DIAG_308_RC_OK;
         } else {
             env->regs[r1 + 1] = DIAG_308_RC_NO_CONF;
-- 
2.20.1

Re: [PATCH v2 11/13] s390x: protvirt: Move diag 308 data over SIDAD

[David Hildenbrand]

On 29.11.19 10:48, Janosch Frank wrote:
> For protected guests the IPIB is written/read to/from the satellite
> block, so we need to make those accesses virtual to make them go
> through KVM mem ops.

same comment regarding virt mem access. IMHO, the KVM mem ops should
return a hard error in case we're in pv mode.

> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/diag.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index 5489fc721a..6d78759151 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -88,6 +88,7 @@ static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
>  void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>  {
>      CPUState *cs = env_cpu(env);
> +    S390CPU *cpu = S390_CPU(cs);
>      uint64_t addr =  env->regs[r1];
>      uint64_t subcode = env->regs[r3];
>      IplParameterBlock *iplb;
> @@ -118,14 +119,27 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>          if (diag308_parm_check(env, r1, addr, ra, false)) {
>              return;
>          }
> +
>          iplb = g_new0(IplParameterBlock, 1);
> -        cpu_physical_memory_read(addr, iplb, sizeof(iplb->len));
> +        if (!env->pv) {
> +            cpu_physical_memory_read(addr, iplb, sizeof(iplb->len));
> +        } else {
> +            s390_cpu_virt_mem_read(cpu, 0, 0, iplb, sizeof(iplb->len));
> +            s390_cpu_virt_mem_handle_exc(cpu, ra);
> +        }
> +
>          if (!iplb_valid_len(iplb)) {
>              env->regs[r1 + 1] = DIAG_308_RC_INVALID;
>              goto out;
>          }
>  
> -        cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
> +        if (!env->pv) {
> +            cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
> +        } else {
> +            s390_cpu_virt_mem_read(cpu, 0, 0, iplb, be32_to_cpu(iplb->len));
> +            s390_cpu_virt_mem_handle_exc(cpu, ra);
> +        }
> +
>  
>          if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb) &&
>              !(iplb_valid_se(iplb) && s390_ipl_pv_check_comp(iplb) >= 0)) {
> @@ -149,7 +163,13 @@ out:
>              iplb = s390_ipl_get_iplb();
>          }
>          if (iplb) {
> -            cpu_physical_memory_write(addr, iplb, be32_to_cpu(iplb->len));
> +            if (!env->pv) {
> +                cpu_physical_memory_write(addr, iplb, be32_to_cpu(iplb->len));
> +            } else {
> +                s390_cpu_virt_mem_write(cpu, 0, 0, iplb,
> +                                        be32_to_cpu(iplb->len));
> +                s390_cpu_virt_mem_handle_exc(cpu, ra);

... exactly due to this handling where we actually can't have
exceptions, I don't like reusing this infrastructure/interface.

-- 
Thanks,

David / dhildenb

[PATCH v2 12/13] s390x: protvirt: Disable address checks for PV guest IO emulation

[Janosch Frank]

IO instruction data is routed through SIDAD for protected guests, so
adresses do not need to be checked, as this is kernel memory.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/ioinst.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index c437a1d8c6..e4102430aa 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -17,6 +17,16 @@
 #include "trace.h"
 #include "hw/s390x/s390-pci-bus.h"
 
+static uint64_t get_address_from_regs(CPUS390XState *env, uint32_t ipb,
+                                      uint8_t *ar)
+{
+    if (env->pv) {
+        *ar = 0;
+        return 0;
+    }
+    return decode_basedisp_s(env, ipb, ar);
+}
+
 int ioinst_disassemble_sch_ident(uint32_t value, int *m, int *cssid, int *ssid,
                                  int *schid)
 {
@@ -114,7 +124,7 @@ void ioinst_handle_msch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra)
     CPUS390XState *env = &cpu->env;
     uint8_t ar;
 
-    addr = decode_basedisp_s(env, ipb, &ar);
+    addr = get_address_from_regs(env, ipb, &ar);
     if (addr & 3) {
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);
         return;
@@ -171,7 +181,7 @@ void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra)
     CPUS390XState *env = &cpu->env;
     uint8_t ar;
 
-    addr = decode_basedisp_s(env, ipb, &ar);
+    addr = get_address_from_regs(env, ipb, &ar);
     if (addr & 3) {
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);
         return;
@@ -203,7 +213,7 @@ void ioinst_handle_stcrw(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
     CPUS390XState *env = &cpu->env;
     uint8_t ar;
 
-    addr = decode_basedisp_s(env, ipb, &ar);
+    addr = get_address_from_regs(env, ipb, &ar);
     if (addr & 3) {
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);
         return;
@@ -234,7 +244,7 @@ void ioinst_handle_stsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb,
     CPUS390XState *env = &cpu->env;
     uint8_t ar;
 
-    addr = decode_basedisp_s(env, ipb, &ar);
+    addr = get_address_from_regs(env, ipb, &ar);
     if (addr & 3) {
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);
         return;
@@ -303,7 +313,7 @@ int ioinst_handle_tsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra)
         return -EIO;
     }
     trace_ioinst_sch_id("tsch", cssid, ssid, schid);
-    addr = decode_basedisp_s(env, ipb, &ar);
+    addr = get_address_from_regs(env, ipb, &ar);
     if (addr & 3) {
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);
         return -EIO;
@@ -601,7 +611,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
 {
     ChscReq *req;
     ChscResp *res;
-    uint64_t addr;
+    uint64_t addr = 0;
     int reg;
     uint16_t len;
     uint16_t command;
@@ -610,7 +620,9 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
 
     trace_ioinst("chsc");
     reg = (ipb >> 20) & 0x00f;
-    addr = env->regs[reg];
+    if (!env->pv) {
+        addr = env->regs[reg];
+    }
     /* Page boundary? */
     if (addr & 0xfff) {
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);
-- 
2.20.1

Re: [PATCH v2 12/13] s390x: protvirt: Disable address checks for PV guest IO emulation

[David Hildenbrand]

On 29.11.19 10:48, Janosch Frank wrote:
> IO instruction data is routed through SIDAD for protected guests, so
> adresses do not need to be checked, as this is kernel memory.

"do not need" - is it actually evil to check? I would have assumed the
kernel checks and these checks are just superfluous but not dangerous?

IOW, some overhead we can ignore for now easily.

Same comment regarding doing the SIDAD access differently ...

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 12/13] s390x: protvirt: Disable address checks for PV guest IO emulation

[Thomas Huth]

On 29/11/2019 10.48, Janosch Frank wrote:
> IO instruction data is routed through SIDAD for protected guests, so
> adresses do not need to be checked, as this is kernel memory.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/ioinst.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)

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

Re: [PATCH v2 12/13] s390x: protvirt: Disable address checks for PV guest IO emulation

[Cornelia Huck]

On Fri, 29 Nov 2019 04:48:08 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> IO instruction data is routed through SIDAD for protected guests, so
> adresses do not need to be checked, as this is kernel memory.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/ioinst.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index c437a1d8c6..e4102430aa 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -17,6 +17,16 @@
>  #include "trace.h"
>  #include "hw/s390x/s390-pci-bus.h"
>  
> +static uint64_t get_address_from_regs(CPUS390XState *env, uint32_t ipb,
> +                                      uint8_t *ar)

Would like to keep the ioinst_* pattern here (even though this is an
internal function). ioinst_decode_addr()?

> +{
> +    if (env->pv) {
> +        *ar = 0;
> +        return 0;
> +    }
> +    return decode_basedisp_s(env, ipb, ar);
> +}
> +
>  int ioinst_disassemble_sch_ident(uint32_t value, int *m, int *cssid, int *ssid,
>                                   int *schid)
>  {

(...)

> @@ -601,7 +611,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
>  {
>      ChscReq *req;
>      ChscResp *res;
> -    uint64_t addr;
> +    uint64_t addr = 0;
>      int reg;
>      uint16_t len;
>      uint16_t command;
> @@ -610,7 +620,9 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
>  
>      trace_ioinst("chsc");
>      reg = (ipb >> 20) & 0x00f;
> -    addr = env->regs[reg];
> +    if (!env->pv) {
> +        addr = env->regs[reg];
> +    }

addr = env->pv ? 0 : env->regs[reg];

?

>      /* Page boundary? */
>      if (addr & 0xfff) {
>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);

[PATCH v2 13/13] s390x: protvirt: Handle SIGP store status correctly

[Janosch Frank]

Status storing is obviously not done by qemu anymore.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/helper.c | 4 ++++
 target/s390x/sigp.c   | 1 +
 2 files changed, 5 insertions(+)

diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index a3a49164e4..3800c4b395 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -246,6 +246,10 @@ int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch)
     hwaddr len = sizeof(*sa);
     int i;
 
+    if (cpu->env.pv) {
+        return 0;
+    }
+
     sa = cpu_physical_memory_map(addr, &len, 1);
     if (!sa) {
         return -EFAULT;
diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
index 727875bb4a..2007946299 100644
--- a/target/s390x/sigp.c
+++ b/target/s390x/sigp.c
@@ -497,6 +497,7 @@ void do_stop_interrupt(CPUS390XState *env)
     if (s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu) == 0) {
         qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
     }
+    /* Storing will occur on next SIE entry for fmt 4 */
     if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
         s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
     }
-- 
2.20.1

Re: [PATCH v2 13/13] s390x: protvirt: Handle SIGP store status correctly

[Thomas Huth]

On 29/11/2019 10.48, Janosch Frank wrote:
> Status storing is obviously not done by qemu anymore.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/helper.c | 4 ++++
>  target/s390x/sigp.c   | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> index a3a49164e4..3800c4b395 100644
> --- a/target/s390x/helper.c
> +++ b/target/s390x/helper.c
> @@ -246,6 +246,10 @@ int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch)
>      hwaddr len = sizeof(*sa);
>      int i;
>  
> +    if (cpu->env.pv) {
> +        return 0;
> +    }
> +
>      sa = cpu_physical_memory_map(addr, &len, 1);
>      if (!sa) {
>          return -EFAULT;
> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
> index 727875bb4a..2007946299 100644
> --- a/target/s390x/sigp.c
> +++ b/target/s390x/sigp.c
> @@ -497,6 +497,7 @@ void do_stop_interrupt(CPUS390XState *env)
>      if (s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu) == 0) {
>          qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>      }
> +    /* Storing will occur on next SIE entry for fmt 4 */
>      if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
>          s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
>      }
> 

I'd suggest to move the comment to the if-statement in helper.c, too.

 Thomas

Re: [PATCH v2 13/13] s390x: protvirt: Handle SIGP store status correctly

[David Hildenbrand]

On 29.11.19 12:04, Thomas Huth wrote:
> On 29/11/2019 10.48, Janosch Frank wrote:
>> Status storing is obviously not done by qemu anymore.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  target/s390x/helper.c | 4 ++++
>>  target/s390x/sigp.c   | 1 +
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
>> index a3a49164e4..3800c4b395 100644
>> --- a/target/s390x/helper.c
>> +++ b/target/s390x/helper.c
>> @@ -246,6 +246,10 @@ int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch)
>>      hwaddr len = sizeof(*sa);
>>      int i;
>>  
>> +    if (cpu->env.pv) {
>> +        return 0;
>> +    }
>> +
>>      sa = cpu_physical_memory_map(addr, &len, 1);
>>      if (!sa) {
>>          return -EFAULT;
>> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
>> index 727875bb4a..2007946299 100644
>> --- a/target/s390x/sigp.c
>> +++ b/target/s390x/sigp.c
>> @@ -497,6 +497,7 @@ void do_stop_interrupt(CPUS390XState *env)
>>      if (s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu) == 0) {
>>          qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>>      }
>> +    /* Storing will occur on next SIE entry for fmt 4 */
>>      if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
>>          s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
>>      }
>>
> 
> I'd suggest to move the comment to the if-statement in helper.c, too.
> 

+1

>  Thomas
> 


-- 
Thanks,

David / dhildenb