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

[...]
>  
> +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

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
> [...]
> 
> 

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.

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

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
> 

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?