Re: [PATCH v6 02/18] s390x: protvirt: Add diag308 subcodes 8 - 10

[Christian Borntraeger <borntraeger@de.ibm.com>]

On 04.03.20 12:42, Janosch Frank 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 <frankja@linux.ibm.com>
> ---
>  hw/s390x/ipl.c      | 47 ++++++++++++++++++++++++++++++++++++++++++---
>  hw/s390x/ipl.h      | 32 ++++++++++++++++++++++++++++++
>  target/s390x/diag.c | 26 ++++++++++++++++++++++---
>  3 files changed, 99 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 9c1ecd423c..80c6ab233a 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c

Can you update the copyright dates for files that you touch?

> @@ -538,15 +538,55 @@ static bool is_virtio_scsi_device(IplParameterBlock *iplb)
>      return is_virtio_ccw_device_of_type(iplb, VIRTIO_ID_SCSI);
>  }
>  
> +int s390_ipl_pv_check_components(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 &&

Why do we need this check? Isnt that already ensured by the for loop?

> +            ipib_pv->components[i].tweak_pref >
> +            ipib_pv->components[i + 1].tweak_pref) {

I think i+1 must be greater than i. So ">=" instead?

> +            return -EINVAL;
> +        }
> +    }
> +    return 0;
> +}
> +
>  void s390_ipl_update_diag308(IplParameterBlock *iplb)

maybe add a comment that explains that a guest can have 2 IPLBs. one for
the secure guest and one thsat we go back to when rebooting.
>  {
>      S390IPLState *ipl = get_ipl_device();
>  
> -    ipl->iplb = *iplb;
> -    ipl->iplb_valid = true;
> +    if (iplb->pbt == S390_IPL_TYPE_PV) {
> +        ipl->iplb_pv = *iplb;
> +        ipl->iplb_valid_pv = 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_pv) {
> +        return NULL;
> +    }
> +    return &ipl->iplb_pv;
> +}
> +
>  IplParameterBlock *s390_ipl_get_iplb(void)
>  {
>      S390IPLState *ipl = get_ipl_device();
> @@ -561,7 +601,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..04be63cee1 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[87];
> +    uint8_t  version;
> +    uint32_t reserved70;

Here you have 70 (the offset in hex I guess), I tßhink this is an odd name. In addition the reserved field 2 lines above has no address part in its
name.