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