Re: [PATCH v9 02/15] s390x: protvirt: Support unpack facility

[Christian Borntraeger]

On 11.03.20 14:21, Janosch Frank wrote:
[...]
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index b81942e1e6f9002e..98df89e62c25f583 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -27,6 +27,7 @@
>  #include "hw/s390x/vfio-ccw.h"
>  #include "hw/s390x/css.h"
>  #include "hw/s390x/ebcdic.h"
> +#include "hw/s390x/pv.h"
>  #include "ipl.h"
>  #include "qemu/error-report.h"
>  #include "qemu/config-file.h"
> @@ -566,12 +567,31 @@ void s390_ipl_update_diag308(IplParameterBlock *iplb)
>  {
>      S390IPLState *ipl = get_ipl_device();
>  
> -    ipl->iplb = *iplb;
> -    ipl->iplb_valid = true;
> +    /*
> +     * The IPLB set and retrieved by subcodes 8/9 is completely
> +     * separate from the one managed via subcodes 5/6.
> +     */
> +    if (iplb->pbt == S390_IPL_TYPE_PV) {
> +        ipl->iplb_pv = *iplb;
> +        ipl->iplb_valid_pv = true;
> +    } else {
> +        ipl->iplb = *iplb;
> +        ipl->iplb_valid = true;
> +    }

We call this for DIAG308_SET and DIAG308_PV_SET in diag.c (see below).
Doesnt this allow to set S390_IPL_TYPE_PV via subcode 5 and an CCW type
via subcode 8. It is certainly not an issue security-wise, but it seems to 
violate
the architecture.
Shouldnt we add a check in diag.c?

[...]
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
[..]
> @@ -93,6 +102,11 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, 
> uint64_t r3, uintptr_t ra)
>          return;
>      }
>  
> +    if (subcode >= DIAG308_PV_SET && !s390_has_feat(S390_FEAT_UNPACK)) {
> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> +        return;
> +    }
> +
>      switch (subcode) {
>      case DIAG308_RESET_MOD_CLR:
>          s390_ipl_reset_request(cs, S390_RESET_MODIFIED_CLEAR);
> @@ -105,6 +119,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:

somewhere here after we have loaded the block. 



Other than that this looks good.