| Message ID | 1574935984-16910-2-git-send-email-pmorel@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: protvirt: SCLP interpretation | expand |
On 28/11/2019 11.13, Pierre Morel wrote: > The SCLP protection handle some of the exceptions due to > mis-constructions of the SCLP Control Block (SCCB) by the guest and > provides notifications to the host when something gets wrong. > We currently do not handle these exceptions, letting all the work to the > firmware therefor, we only need to inject an external interrupt to the > guest. > > When the SCCB is correct, the S390x virtualisation protection copies > the SCLP Control Block (SCCB) from the guest inside the kernel to avoid > opening a direct access to the guest memory. > When accessing the kernel memory with standard s390_cpu_virt_mem_* > functions the host opens access to the SCCB shadow at address 0. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > hw/s390x/sclp.c | 18 +++++++++++++ > include/hw/s390x/sclp.h | 2 ++ > target/s390x/kvm.c | 56 ++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 75 insertions(+), 1 deletion(-) > > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c > index f57ce7b739..02e4e0146f 100644 > --- a/hw/s390x/sclp.c > +++ b/hw/s390x/sclp.c > @@ -193,6 +193,24 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code) > } > } > > +int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, > + uint32_t code) > +{ > + SCLPDevice *sclp = get_sclp_device(); > + SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp); > + SCCB work_sccb; > + hwaddr sccb_len = sizeof(SCCB); > + > + /* Protected guest SCCB is always seen at address 0 */ Well, as far as I've understood it, the address is rather ignored (and you can only specify an offset into the 4k page)? > + s390_cpu_virt_mem_read(env_archcpu(env), 0, 0, &work_sccb, sccb_len); > + sclp_c->execute(sclp, &work_sccb, code); > + s390_cpu_virt_mem_write(env_archcpu(env), 0, 0, &work_sccb, > + be16_to_cpu(work_sccb.h.length)); > + > + sclp_c->service_interrupt(sclp, (uint64_t)&work_sccb); > + return 0; > +} > + > int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code) > { > SCLPDevice *sclp = get_sclp_device(); > diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h > index c54413b78c..c0a3faa37d 100644 > --- a/include/hw/s390x/sclp.h > +++ b/include/hw/s390x/sclp.h > @@ -217,5 +217,7 @@ void s390_sclp_init(void); > void sclp_service_interrupt(uint32_t sccb); > void raise_irq_cpu_hotplug(void); > int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); > +int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, > + uint32_t code); > > #endif > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index 0c9d14b4b1..559f470f51 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -1170,7 +1170,14 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run, > sccb = env->regs[ipbh0 & 0xf]; > code = env->regs[(ipbh0 & 0xf0) >> 4]; > > - r = sclp_service_call(env, sccb, code); > + switch (run->s390_sieic.icptcode) { > + case ICPT_PV_INSTR: > + r = sclp_service_call_protected(env, sccb, code); > + break; > + default: > + r = sclp_service_call(env, sccb, code); > + break; > + } Why not simply if (run->s390_sieic.icptcode == ICPT_PV_INSTR) { r = sclp_service_call_protected(env, sccb, code); } else { r = sclp_service_call(env, sccb, code); } ... that's way short and easier to read. Or do you expect other icptcodes in the near future? > if (r < 0) { > kvm_s390_program_interrupt(cpu, -r); > } else { > @@ -1575,6 +1582,47 @@ static int kvm_s390_handle_sigp(S390CPU *cpu, uint8_t ipa1, uint32_t ipb) > return 0; > } > > +static int handle_secure_notification(S390CPU *cpu, struct kvm_run *run) > +{ > + unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00); > + uint8_t ipa1 = run->s390_sieic.ipa & 0x00ff; > + > + switch (ipa0) { > + case IPA0_SIGP: /* We get the notification that the guest stop */ > + kvm_s390_handle_sigp(cpu, ipa1, run->s390_sieic.ipb); > + break; > + case IPA0_B2: /* We accept but do nothing for B2 notifications */ > + break; > + default: /* We do not expect other instruction's notification */ > + kvm_s390_program_interrupt(cpu, PGM_OPERATION); Maybe add a tracepoint or qemu_log_mask(LOG_UNIMP, ...) or CPU_LOG_INT here, so we can spot this condition more easily? > + break; > + } > + return 0; > +} > + > +static int handle_secure_instruction(S390CPU *cpu, struct kvm_run *run) > +{ > + unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00); > + uint8_t ipa1 = run->s390_sieic.ipa & 0x00ff; > + int r = -1; > + > + switch (ipa0) { > + case IPA0_B2: > + r = handle_b2(cpu, run, ipa1); > + break; > + case IPA0_DIAG: > + r = handle_diag(cpu, run, run->s390_sieic.ipb); > + break; > + } > + > + if (r < 0) { > + r = 0; > + kvm_s390_program_interrupt(cpu, PGM_OPERATION); > + } > + > + return r; > +} > + > static int handle_instruction(S390CPU *cpu, struct kvm_run *run) > { > unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00); > @@ -1665,6 +1713,12 @@ static int handle_intercept(S390CPU *cpu) > DPRINTF("intercept: 0x%x (at 0x%lx)\n", icpt_code, > (long)cs->kvm_run->psw_addr); > switch (icpt_code) { > + case ICPT_PV_INSTR_NOT: > + r = handle_secure_notification(cpu, run); > + break; > + case ICPT_PV_INSTR: > + r = handle_secure_instruction(cpu, run); > + break; > case ICPT_INSTRUCTION: > r = handle_instruction(cpu, run); > break; > Thomas
On 2019-11-28 13:10, Thomas Huth wrote: > On 28/11/2019 11.13, Pierre Morel wrote: >> The SCLP protection handle some of the exceptions due to >> mis-constructions of the SCLP Control Block (SCCB) by the guest and >> provides notifications to the host when something gets wrong. >> We currently do not handle these exceptions, letting all the work to the >> firmware therefor, we only need to inject an external interrupt to the >> guest. >> >> When the SCCB is correct, the S390x virtualisation protection copies >> the SCLP Control Block (SCCB) from the guest inside the kernel to avoid >> opening a direct access to the guest memory. >> When accessing the kernel memory with standard s390_cpu_virt_mem_* >> functions the host opens access to the SCCB shadow at address 0. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> hw/s390x/sclp.c | 18 +++++++++++++ >> include/hw/s390x/sclp.h | 2 ++ >> target/s390x/kvm.c | 56 ++++++++++++++++++++++++++++++++++++++++- >> 3 files changed, 75 insertions(+), 1 deletion(-) >> >> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >> index f57ce7b739..02e4e0146f 100644 >> --- a/hw/s390x/sclp.c >> +++ b/hw/s390x/sclp.c >> @@ -193,6 +193,24 @@ static void sclp_execute(SCLPDevice *sclp, SCCB >> *sccb, uint32_t code) >> } >> } >> +int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, >> + uint32_t code) >> +{ >> + SCLPDevice *sclp = get_sclp_device(); >> + SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp); >> + SCCB work_sccb; >> + hwaddr sccb_len = sizeof(SCCB); >> + >> + /* Protected guest SCCB is always seen at address 0 */ > > Well, as far as I've understood it, the address is rather ignored (and > you can only specify an offset into the 4k page)? You can see it like this, then the offset is 0. However we give here an address as argument. > >> + s390_cpu_virt_mem_read(env_archcpu(env), 0, 0, &work_sccb, sccb_len); >> + sclp_c->execute(sclp, &work_sccb, code); >> + s390_cpu_virt_mem_write(env_archcpu(env), 0, 0, &work_sccb, >> + be16_to_cpu(work_sccb.h.length)); >> + >> + sclp_c->service_interrupt(sclp, (uint64_t)&work_sccb); >> + return 0; >> +} >> + >> int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t >> code) >> { >> SCLPDevice *sclp = get_sclp_device(); >> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h >> index c54413b78c..c0a3faa37d 100644 >> --- a/include/hw/s390x/sclp.h >> +++ b/include/hw/s390x/sclp.h >> @@ -217,5 +217,7 @@ void s390_sclp_init(void); >> void sclp_service_interrupt(uint32_t sccb); >> void raise_irq_cpu_hotplug(void); >> int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t >> code); >> +int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, >> + uint32_t code); >> #endif >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >> index 0c9d14b4b1..559f470f51 100644 >> --- a/target/s390x/kvm.c >> +++ b/target/s390x/kvm.c >> @@ -1170,7 +1170,14 @@ static int kvm_sclp_service_call(S390CPU *cpu, >> struct kvm_run *run, >> sccb = env->regs[ipbh0 & 0xf]; >> code = env->regs[(ipbh0 & 0xf0) >> 4]; >> - r = sclp_service_call(env, sccb, code); >> + switch (run->s390_sieic.icptcode) { >> + case ICPT_PV_INSTR: >> + r = sclp_service_call_protected(env, sccb, code); >> + break; >> + default: >> + r = sclp_service_call(env, sccb, code); >> + break; >> + } > > Why not simply > > if (run->s390_sieic.icptcode == ICPT_PV_INSTR) { > r = sclp_service_call_protected(env, sccb, code); > } else { > r = sclp_service_call(env, sccb, code); > } > > ... that's way short and easier to read. Or do you expect other > icptcodes in the near future? No you are right, it is better, I just like switches :) > >> if (r < 0) { >> kvm_s390_program_interrupt(cpu, -r); >> } else { >> @@ -1575,6 +1582,47 @@ static int kvm_s390_handle_sigp(S390CPU *cpu, >> uint8_t ipa1, uint32_t ipb) >> return 0; >> } >> +static int handle_secure_notification(S390CPU *cpu, struct kvm_run >> *run) >> +{ >> + unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00); >> + uint8_t ipa1 = run->s390_sieic.ipa & 0x00ff; >> + >> + switch (ipa0) { >> + case IPA0_SIGP: /* We get the notification that the guest stop */ >> + kvm_s390_handle_sigp(cpu, ipa1, run->s390_sieic.ipb); >> + break; >> + case IPA0_B2: /* We accept but do nothing for B2 notifications */ >> + break; >> + default: /* We do not expect other instruction's notification */ >> + kvm_s390_program_interrupt(cpu, PGM_OPERATION); > > Maybe add a tracepoint or qemu_log_mask(LOG_UNIMP, ...) or CPU_LOG_INT > here, so we can spot this condition more easily? > >> + break; >> + } >> + return 0; >> +} >> + >> +static int handle_secure_instruction(S390CPU *cpu, struct kvm_run *run) >> +{ >> + unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00); >> + uint8_t ipa1 = run->s390_sieic.ipa & 0x00ff; >> + int r = -1; >> + >> + switch (ipa0) { >> + case IPA0_B2: >> + r = handle_b2(cpu, run, ipa1); >> + break; >> + case IPA0_DIAG: >> + r = handle_diag(cpu, run, run->s390_sieic.ipb); >> + break; >> + } >> + >> + if (r < 0) { >> + r = 0; >> + kvm_s390_program_interrupt(cpu, PGM_OPERATION); >> + } >> + >> + return r; >> +} >> + >> static int handle_instruction(S390CPU *cpu, struct kvm_run *run) >> { >> unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00); >> @@ -1665,6 +1713,12 @@ static int handle_intercept(S390CPU *cpu) >> DPRINTF("intercept: 0x%x (at 0x%lx)\n", icpt_code, >> (long)cs->kvm_run->psw_addr); >> switch (icpt_code) { >> + case ICPT_PV_INSTR_NOT: >> + r = handle_secure_notification(cpu, run); >> + break; >> + case ICPT_PV_INSTR: >> + r = handle_secure_instruction(cpu, run); >> + break; >> case ICPT_INSTRUCTION: >> r = handle_instruction(cpu, run); >> break; >> > > Thomas > >
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c index f57ce7b739..02e4e0146f 100644 --- a/hw/s390x/sclp.c +++ b/hw/s390x/sclp.c @@ -193,6 +193,24 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code) } } +int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, + uint32_t code) +{ + SCLPDevice *sclp = get_sclp_device(); + SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp); + SCCB work_sccb; + hwaddr sccb_len = sizeof(SCCB); + + /* Protected guest SCCB is always seen at address 0 */ + s390_cpu_virt_mem_read(env_archcpu(env), 0, 0, &work_sccb, sccb_len); + sclp_c->execute(sclp, &work_sccb, code); + s390_cpu_virt_mem_write(env_archcpu(env), 0, 0, &work_sccb, + be16_to_cpu(work_sccb.h.length)); + + sclp_c->service_interrupt(sclp, (uint64_t)&work_sccb); + return 0; +} + int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code) { SCLPDevice *sclp = get_sclp_device(); diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h index c54413b78c..c0a3faa37d 100644 --- a/include/hw/s390x/sclp.h +++ b/include/hw/s390x/sclp.h @@ -217,5 +217,7 @@ void s390_sclp_init(void); void sclp_service_interrupt(uint32_t sccb); void raise_irq_cpu_hotplug(void); int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); +int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, + uint32_t code); #endif diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index 0c9d14b4b1..559f470f51 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -1170,7 +1170,14 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run, sccb = env->regs[ipbh0 & 0xf]; code = env->regs[(ipbh0 & 0xf0) >> 4]; - r = sclp_service_call(env, sccb, code); + switch (run->s390_sieic.icptcode) { + case ICPT_PV_INSTR: + r = sclp_service_call_protected(env, sccb, code); + break; + default: + r = sclp_service_call(env, sccb, code); + break; + } if (r < 0) { kvm_s390_program_interrupt(cpu, -r); } else { @@ -1575,6 +1582,47 @@ static int kvm_s390_handle_sigp(S390CPU *cpu, uint8_t ipa1, uint32_t ipb) return 0; } +static int handle_secure_notification(S390CPU *cpu, struct kvm_run *run) +{ + unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00); + uint8_t ipa1 = run->s390_sieic.ipa & 0x00ff; + + switch (ipa0) { + case IPA0_SIGP: /* We get the notification that the guest stop */ + kvm_s390_handle_sigp(cpu, ipa1, run->s390_sieic.ipb); + break; + case IPA0_B2: /* We accept but do nothing for B2 notifications */ + break; + default: /* We do not expect other instruction's notification */ + kvm_s390_program_interrupt(cpu, PGM_OPERATION); + break; + } + return 0; +} + +static int handle_secure_instruction(S390CPU *cpu, struct kvm_run *run) +{ + unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00); + uint8_t ipa1 = run->s390_sieic.ipa & 0x00ff; + int r = -1; + + switch (ipa0) { + case IPA0_B2: + r = handle_b2(cpu, run, ipa1); + break; + case IPA0_DIAG: + r = handle_diag(cpu, run, run->s390_sieic.ipb); + break; + } + + if (r < 0) { + r = 0; + kvm_s390_program_interrupt(cpu, PGM_OPERATION); + } + + return r; +} + static int handle_instruction(S390CPU *cpu, struct kvm_run *run) { unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00); @@ -1665,6 +1713,12 @@ static int handle_intercept(S390CPU *cpu) DPRINTF("intercept: 0x%x (at 0x%lx)\n", icpt_code, (long)cs->kvm_run->psw_addr); switch (icpt_code) { + case ICPT_PV_INSTR_NOT: + r = handle_secure_notification(cpu, run); + break; + case ICPT_PV_INSTR: + r = handle_secure_instruction(cpu, run); + break; case ICPT_INSTRUCTION: r = handle_instruction(cpu, run); break;
The SCLP protection handle some of the exceptions due to mis-constructions of the SCLP Control Block (SCCB) by the guest and provides notifications to the host when something gets wrong. We currently do not handle these exceptions, letting all the work to the firmware therefor, we only need to inject an external interrupt to the guest. When the SCCB is correct, the S390x virtualisation protection copies the SCLP Control Block (SCCB) from the guest inside the kernel to avoid opening a direct access to the guest memory. When accessing the kernel memory with standard s390_cpu_virt_mem_* functions the host opens access to the SCCB shadow at address 0. Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> --- hw/s390x/sclp.c | 18 +++++++++++++ include/hw/s390x/sclp.h | 2 ++ target/s390x/kvm.c | 56 ++++++++++++++++++++++++++++++++++++++++- 3 files changed, 75 insertions(+), 1 deletion(-)