Re: [PATCH v9] s390x: protvirt: Fence huge pages

[Christian Borntraeger]

On 12.03.20 17:25, Janosch Frank wrote:
> Let's bail out of the protected transition if we detect that huge
> pages might be in use.
> 
> Signed-off-by: Janosch Frank <address@hidden>
> ---
> 
> I'd like to squash this into the unpack patch to give a proper error
> message if we try to transition into the protected mode while being
> backed by huge pages. 

Looks good.
But maybe we can do it better. Why not reverse the logic and
instead of having kvm_s390_get_hpage_1m, let us define an protvirt_allowed
that as of today only returns hugepages != 1:
Then we could (for kvm-stub.c) also say protvirt_allowed=false;
And if other reasons come along we can extend.

We could also keep this patch separate, does not really matter.

> 
> ---
>  hw/s390x/ipl.h             | 16 ++++++++++++++++
>  hw/s390x/s390-virtio-ccw.c |  1 -
>  target/s390x/diag.c        | 23 ++++++++---------------
>  target/s390x/kvm-stub.c    |  5 +++++
>  target/s390x/kvm.c         |  5 +++++
>  target/s390x/kvm_s390x.h   |  1 +
>  6 files changed, 35 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index af5bb130a6334821..95e3183c9cccf8b6 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -185,6 +185,22 @@ struct S390IPLState {
>  typedef struct S390IPLState S390IPLState;
>  QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, "alignment of iplb 
> wrong");
>  
> +#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      0x0902
> +#define DIAG_308_RC_INVAL_FOR_PV    0x0a02
> +
> +#define DIAG308_RESET_MOD_CLR       0
> +#define DIAG308_RESET_LOAD_NORM     1
> +#define DIAG308_LOAD_CLEAR          3
> +#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
> +
>  #define S390_IPL_TYPE_FCP 0x00
>  #define S390_IPL_TYPE_CCW 0x02
>  #define S390_IPL_TYPE_PV 0x05
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index ebdaaa3a001f6e8c..d32f35c7f47b9c1d 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -361,7 +361,6 @@ out_err:
>      return rc;
>  }
>  
> -#define DIAG_308_RC_INVAL_FOR_PV    0x0a02
>  static void s390_machine_inject_pv_error(CPUState *cs)
>  {
>      int r1 = (cs->kvm_run->s390_sieic.ipa & 0x00f0) >> 4;
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index b245e557037ded06..b1ca81633b83bbdc 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -21,6 +21,7 @@
>  #include "hw/s390x/ipl.h"
>  #include "hw/s390x/s390-virtio-ccw.h"
>  #include "hw/s390x/pv.h"
> +#include "kvm_s390x.h"
>  
>  int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
>  {
> @@ -50,21 +51,6 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, 
> uint64_t r3)
>      return diag288_class->handle_timer(diag288, func, timeout);
>  }
>  
> -#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      0x0902
> -
> -#define DIAG308_RESET_MOD_CLR       0
> -#define DIAG308_RESET_LOAD_NORM     1
> -#define DIAG308_LOAD_CLEAR          3
> -#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)
>  {
> @@ -166,6 +152,13 @@ out:
>              return;
>          }
>  
> +        if (kvm_s390_get_hpage_1m()) {
> +            error_report("Protected VMs can currently not be backed with "
> +                         "huge pages");
> +            env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
> +            return;
> +        }
> +
>          s390_ipl_reset_request(cs, S390_RESET_PV);
>          break;
>      default:
> diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c
> index c4cd497f850eb9c7..aa185017a2a886ca 100644
> --- a/target/s390x/kvm-stub.c
> +++ b/target/s390x/kvm-stub.c
> @@ -39,6 +39,11 @@ int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu)
>      return 0;
>  }
>  
> +int kvm_s390_get_hpage_1m(void)
> +{
> +    return 0;
> +}
> +
>  int kvm_s390_get_ri(void)
>  {
>      return 0;
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 1d6fd6a27b48e35f..c695941076b7aead 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -321,6 +321,11 @@ void kvm_s390_set_max_pagesize(uint64_t pagesize, Error 
> **errp)
>      cap_hpage_1m = 1;
>  }
>  
> +int kvm_s390_get_hpage_1m(void)
> +{
> +    return cap_hpage_1m;
> +}
> +
>  static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
> index 0b21789796d7c462..dea813f450153c34 100644
> --- a/target/s390x/kvm_s390x.h
> +++ b/target/s390x/kvm_s390x.h
> @@ -23,6 +23,7 @@ void kvm_s390_program_interrupt(S390CPU *cpu, uint16_t 
> code);
>  int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state);
>  void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu);
>  int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu);
> +int kvm_s390_get_hpage_1m(void);
>  int kvm_s390_get_ri(void);
>  int kvm_s390_get_gs(void);
>  int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_clock);
>