Re: [PATCH v3 04/17] s390x: protvirt: Add diag308 subcodes 8 - 10

[Cornelia Huck]

On Fri, 14 Feb 2020 10:16:23 -0500
Janosch Frank <address@hidden> wrote:

> 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 <address@hidden>
> ---
>  hw/s390x/ipl.c      | 48 ++++++++++++++++++++++++++++++++++++++++++---
>  hw/s390x/ipl.h      | 31 +++++++++++++++++++++++++++++
>  target/s390x/diag.c | 27 ++++++++++++++++++++++---
>  3 files changed, 100 insertions(+), 6 deletions(-)

(...)

>  void s390_ipl_update_diag308(IplParameterBlock *iplb)
>  {
>      S390IPLState *ipl = get_ipl_device();
>  
> -    ipl->iplb = *iplb;
> -    ipl->iplb_valid = true;
> +    if (iplb->pbt == 5) {

Magic value; maybe introduce a #define or at least a comment?

> +        ipl->iplb_pbt5 = *iplb;
> +        ipl->iplb_valid_pbt5 = true;
> +    } else {
> +        ipl->iplb = *iplb;
> +        ipl->iplb_valid = true;
> +    }
>      ipl->netboot = is_virtio_net_device(iplb);
>  }

> @@ -133,6 +154,7 @@ struct S390IPLState {
>      /*< private >*/
>      DeviceState parent_obj;
>      IplParameterBlock iplb;
> +    IplParameterBlock iplb_pbt5;

Add /* for protected virtualization */ ?

Or, if this is not used for anything else, call it iplb_pv?

>      QemuIplParameters qipl;
>      uint64_t start_addr;
>      uint64_t compat_start_addr;

(...)

> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index b5aec06d6b..4ba6033609 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

DIAG_308_RC_INVAL_FOR_PV ?

>  
>  #define DIAG308_RESET_MOD_CLR       0
>  #define DIAG308_RESET_LOAD_NORM     1

(...)

> @@ -128,17 +135,31 @@ 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();
> +        }

iplb = (subcode == DIAG308_PV_STORE) ?
       s390_ipl_get_iplb_secure() : s390_ipl_get_iplb();

Matter of taste, I guess.

>          if (iplb) {
>              cpu_physical_memory_write(addr, iplb, be32_to_cpu(iplb->len));
>              env->regs[r1 + 1] = DIAG_308_RC_OK;
>          } else {
>              env->regs[r1 + 1] = DIAG_308_RC_NO_CONF;
>          }
> -        return;
> +        break;
> +    case DIAG308_PV_START:
> +        iplb = s390_ipl_get_iplb_secure();
> +        if (!iplb || !iplb_valid_pv(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;