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

[Janosch Frank]

On 3/13/20 9:21 AM, Christian Borntraeger wrote:
> 
> 
> 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.

The *_allowed() functions are all based on the machine and part of
s390-virtio-ccw.c so having one in kvm.c looks strange.

!protvirt_allowed could have any number of reasons in the future, I
introduced this patch to give a specific error message that can help the
user to chose the right options when looking for the error.

Other ideas or a revised one?

> 
>>
>> ---
>>  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);
>>
> 
>

signature.asc
Description: OpenPGP digital signature