| Message ID | 20200311132151.172389-9-frankja@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: Protected Virtualization support | expand |
> + * We only need the address to have something valid for the > + * service_interrupt call. > + */ > +#define SCLP_PV_DUMMY_ADDR 0x4000 > +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); > + > + /* > + * Only a very limited amount of calls is permitted by the s/amount/number/ ?
On 3/11/20 2:24 PM, David Hildenbrand wrote: > >> + * We only need the address to have something valid for the >> + * service_interrupt call. >> + */ >> +#define SCLP_PV_DUMMY_ADDR 0x4000 >> +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); >> + >> + /* >> + * Only a very limited amount of calls is permitted by the > > s/amount/number/ ? > > Ack
On Wed, 11 Mar 2020 09:21:44 -0400 Janosch Frank <frankja@linux.ibm.com> wrote: > SCLP for a protected guest is done over the SIDAD, so we need to use > the s390_cpu_pv_mem_* functions to access the SIDAD instead of guest > memory when reading/writing SCBs. > > To not confuse the sclp emulation, we set 0x4000 as the SCCB address, > since the function that injects the sclp external interrupt would > reject a zero sccb address. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > Reviewed-by: David Hildenbrand <david@redhat.com> > --- > hw/s390x/sclp.c | 30 ++++++++++++++++++++++++++++++ > include/hw/s390x/sclp.h | 2 ++ > target/s390x/kvm.c | 24 +++++++++++++++++++----- > 3 files changed, 51 insertions(+), 5 deletions(-) > > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c > index af0bfbc2eca74767..5f3aa30d6283dce5 100644 > --- a/hw/s390x/sclp.c > +++ b/hw/s390x/sclp.c > @@ -193,6 +193,36 @@ static void sclp_execute(SCLPDevice *sclp, SCCB > *sccb, uint32_t code) } > } > > +/* > + * We only need the address to have something valid for the > + * service_interrupt call. > + */ > +#define SCLP_PV_DUMMY_ADDR 0x4000 > +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); > + > + /* > + * Only a very limited amount of calls is permitted by the > + * Ultravisor and we support all of them, so we don't check for > + * them. All other specification exceptions are also interpreted > + * by the Ultravisor and hence never cause an exit we need to > + * handle. > + * > + * Setting the CC is also done by the Ultravisor. > + */ > + s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len); > + sclp_c->execute(sclp, &work_sccb, code); > + s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb, > + be16_to_cpu(work_sccb.h.length)); > + sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR); > + 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 c54413b78cf01b27..c0a3faa37d730453 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 8085d5030e7c6454..ff6027036ec2f14a 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -1227,12 +1227,26 @@ static void 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); > - if (r < 0) { > - kvm_s390_program_interrupt(cpu, -r); > - return; > + switch (run->s390_sieic.icptcode) { > + case ICPT_PV_INSTR_NOTIFICATION: > + g_assert(s390_is_pv()); > + /* The notification intercepts are currently handled by KVM > */ > + error_report("unexpected SCLP PV notification"); > + exit(1); > + break; > + case ICPT_PV_INSTR: > + g_assert(s390_is_pv()); > + sclp_service_call_protected(env, sccb, code); > + break; > + case ICPT_INSTRUCTION: > + g_assert(!s390_is_pv()); > + r = sclp_service_call(env, sccb, code); > + if (r < 0) { > + kvm_s390_program_interrupt(cpu, -r); > + return; > + } > + setcc(cpu, r); > } > - setcc(cpu, r); > } > > static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1) Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
On 11.03.20 14:21, Janosch Frank wrote: > SCLP for a protected guest is done over the SIDAD, so we need to use > the s390_cpu_pv_mem_* functions to access the SIDAD instead of guest > memory when reading/writing SCBs. > > To not confuse the sclp emulation, we set 0x4000 as the SCCB address, > since the function that injects the sclp external interrupt would > reject a zero sccb address. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > Reviewed-by: David Hildenbrand <david@redhat.com> > --- > hw/s390x/sclp.c | 30 ++++++++++++++++++++++++++++++ > include/hw/s390x/sclp.h | 2 ++ > target/s390x/kvm.c | 24 +++++++++++++++++++----- > 3 files changed, 51 insertions(+), 5 deletions(-) > > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c > index af0bfbc2eca74767..5f3aa30d6283dce5 100644 > --- a/hw/s390x/sclp.c > +++ b/hw/s390x/sclp.c > @@ -193,6 +193,36 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code) > } > } > > +/* > + * We only need the address to have something valid for the > + * service_interrupt call. > + */ > +#define SCLP_PV_DUMMY_ADDR 0x4000 > +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); > + > + /* > + * Only a very limited amount of calls is permitted by the > + * Ultravisor and we support all of them, so we don't check for > + * them. All other specification exceptions are also interpreted > + * by the Ultravisor and hence never cause an exit we need to > + * handle. > + * > + * Setting the CC is also done by the Ultravisor. > + */ This is fine for the current architecture which specifies a list of sclp commands that are passed through (and this is fine). Question is still if we replace this comment with an assertion that this is the case? Or maybe even really do the same as sclp_service_call and return 0x1f0 for unknown commands? Anyway, whatever you decide. Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > + s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len); > + sclp_c->execute(sclp, &work_sccb, code); > + s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb, > + be16_to_cpu(work_sccb.h.length)); > + sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR); > + 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 c54413b78cf01b27..c0a3faa37d730453 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 8085d5030e7c6454..ff6027036ec2f14a 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -1227,12 +1227,26 @@ static void 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); > - if (r < 0) { > - kvm_s390_program_interrupt(cpu, -r); > - return; > + switch (run->s390_sieic.icptcode) { > + case ICPT_PV_INSTR_NOTIFICATION: > + g_assert(s390_is_pv()); > + /* The notification intercepts are currently handled by KVM */ > + error_report("unexpected SCLP PV notification"); > + exit(1); > + break; > + case ICPT_PV_INSTR: > + g_assert(s390_is_pv()); > + sclp_service_call_protected(env, sccb, code); > + break; > + case ICPT_INSTRUCTION: > + g_assert(!s390_is_pv()); > + r = sclp_service_call(env, sccb, code); > + if (r < 0) { > + kvm_s390_program_interrupt(cpu, -r); > + return; > + } > + setcc(cpu, r); > } > - setcc(cpu, r); > } > > static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1) >
On Fri, 13 Mar 2020 14:14:35 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 11.03.20 14:21, Janosch Frank wrote: > > SCLP for a protected guest is done over the SIDAD, so we need to use > > the s390_cpu_pv_mem_* functions to access the SIDAD instead of guest > > memory when reading/writing SCBs. > > > > To not confuse the sclp emulation, we set 0x4000 as the SCCB address, > > since the function that injects the sclp external interrupt would > > reject a zero sccb address. > > > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > > Reviewed-by: David Hildenbrand <david@redhat.com> > > --- > > hw/s390x/sclp.c | 30 ++++++++++++++++++++++++++++++ > > include/hw/s390x/sclp.h | 2 ++ > > target/s390x/kvm.c | 24 +++++++++++++++++++----- > > 3 files changed, 51 insertions(+), 5 deletions(-) > > +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); > > + > > + /* > > + * Only a very limited amount of calls is permitted by the > > + * Ultravisor and we support all of them, so we don't check for > > + * them. All other specification exceptions are also interpreted > > + * by the Ultravisor and hence never cause an exit we need to > > + * handle. > > + * > > + * Setting the CC is also done by the Ultravisor. > > + */ > > This is fine for the current architecture which specifies a list of sclp > commands that are passed through (and this is fine). Question is still if > we replace this comment with an assertion that this is the case? > Or maybe even really do the same as sclp_service_call and return 0x1f0 for > unknown commands? That would be a case of older QEMU on newer hardware, right? Signaling that the command is unsupported seems the most reasonable to me (depending on what the architecture allows.) > > Anyway, whatever you decide. > > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > > > + s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len); > > + sclp_c->execute(sclp, &work_sccb, code); > > + s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb, > > + be16_to_cpu(work_sccb.h.length)); > > + sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR); > > + return 0; > > +} > > +
On 3/17/20 12:05 PM, Cornelia Huck wrote: > On Fri, 13 Mar 2020 14:14:35 +0100 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> On 11.03.20 14:21, Janosch Frank wrote: >>> SCLP for a protected guest is done over the SIDAD, so we need to use >>> the s390_cpu_pv_mem_* functions to access the SIDAD instead of guest >>> memory when reading/writing SCBs. >>> >>> To not confuse the sclp emulation, we set 0x4000 as the SCCB address, >>> since the function that injects the sclp external interrupt would >>> reject a zero sccb address. >>> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>> Reviewed-by: David Hildenbrand <david@redhat.com> >>> --- >>> hw/s390x/sclp.c | 30 ++++++++++++++++++++++++++++++ >>> include/hw/s390x/sclp.h | 2 ++ >>> target/s390x/kvm.c | 24 +++++++++++++++++++----- >>> 3 files changed, 51 insertions(+), 5 deletions(-) > >>> +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); >>> + >>> + /* >>> + * Only a very limited amount of calls is permitted by the >>> + * Ultravisor and we support all of them, so we don't check for >>> + * them. All other specification exceptions are also interpreted >>> + * by the Ultravisor and hence never cause an exit we need to >>> + * handle. >>> + * >>> + * Setting the CC is also done by the Ultravisor. >>> + */ >> >> This is fine for the current architecture which specifies a list of sclp >> commands that are passed through (and this is fine). Question is still if >> we replace this comment with an assertion that this is the case? >> Or maybe even really do the same as sclp_service_call and return 0x1f0 for >> unknown commands? > > That would be a case of older QEMU on newer hardware, right? Signaling > that the command is unsupported seems the most reasonable to me > (depending on what the architecture allows.) Question is if we want to check for the non-pv codes as the hardware will currently only allow a smaller subset anyway. Then if the IO codes are passed through by SIE we would support them right away. > >> >> Anyway, whatever you decide. >> >> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> >> >>> + s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len); >>> + sclp_c->execute(sclp, &work_sccb, code); >>> + s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb, >>> + be16_to_cpu(work_sccb.h.length)); >>> + sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR); >>> + return 0; >>> +} >>> + > >
On Tue, 17 Mar 2020 12:54:54 +0100 Janosch Frank <frankja@linux.ibm.com> wrote: > On 3/17/20 12:05 PM, Cornelia Huck wrote: > > On Fri, 13 Mar 2020 14:14:35 +0100 > > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > >> On 11.03.20 14:21, Janosch Frank wrote: > >>> SCLP for a protected guest is done over the SIDAD, so we need to use > >>> the s390_cpu_pv_mem_* functions to access the SIDAD instead of guest > >>> memory when reading/writing SCBs. > >>> > >>> To not confuse the sclp emulation, we set 0x4000 as the SCCB address, > >>> since the function that injects the sclp external interrupt would > >>> reject a zero sccb address. > >>> > >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > >>> Reviewed-by: David Hildenbrand <david@redhat.com> > >>> --- > >>> hw/s390x/sclp.c | 30 ++++++++++++++++++++++++++++++ > >>> include/hw/s390x/sclp.h | 2 ++ > >>> target/s390x/kvm.c | 24 +++++++++++++++++++----- > >>> 3 files changed, 51 insertions(+), 5 deletions(-) > > > >>> +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); > >>> + > >>> + /* > >>> + * Only a very limited amount of calls is permitted by the > >>> + * Ultravisor and we support all of them, so we don't check for > >>> + * them. All other specification exceptions are also interpreted > >>> + * by the Ultravisor and hence never cause an exit we need to > >>> + * handle. > >>> + * > >>> + * Setting the CC is also done by the Ultravisor. > >>> + */ > >> > >> This is fine for the current architecture which specifies a list of sclp > >> commands that are passed through (and this is fine). Question is still if > >> we replace this comment with an assertion that this is the case? > >> Or maybe even really do the same as sclp_service_call and return 0x1f0 for > >> unknown commands? > > > > That would be a case of older QEMU on newer hardware, right? Signaling > > that the command is unsupported seems the most reasonable to me > > (depending on what the architecture allows.) > > Question is if we want to check for the non-pv codes as the hardware > will currently only allow a smaller subset anyway. Then if the IO codes > are passed through by SIE we would support them right away. Depending on if the passed-through codes would work without any further changes, I guess (which seems likely?) You probably have a better idea about that :) > > > > >> > >> Anyway, whatever you decide. > >> > >> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > >> > >>> + s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len); > >>> + sclp_c->execute(sclp, &work_sccb, code); > >>> + s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb, > >>> + be16_to_cpu(work_sccb.h.length)); > >>> + sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR); > >>> + return 0; > >>> +} > >>> + > > > > > >
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c index af0bfbc2eca74767..5f3aa30d6283dce5 100644 --- a/hw/s390x/sclp.c +++ b/hw/s390x/sclp.c @@ -193,6 +193,36 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code) } } +/* + * We only need the address to have something valid for the + * service_interrupt call. + */ +#define SCLP_PV_DUMMY_ADDR 0x4000 +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); + + /* + * Only a very limited amount of calls is permitted by the + * Ultravisor and we support all of them, so we don't check for + * them. All other specification exceptions are also interpreted + * by the Ultravisor and hence never cause an exit we need to + * handle. + * + * Setting the CC is also done by the Ultravisor. + */ + s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len); + sclp_c->execute(sclp, &work_sccb, code); + s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb, + be16_to_cpu(work_sccb.h.length)); + sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR); + 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 c54413b78cf01b27..c0a3faa37d730453 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 8085d5030e7c6454..ff6027036ec2f14a 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -1227,12 +1227,26 @@ static void 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); - if (r < 0) { - kvm_s390_program_interrupt(cpu, -r); - return; + switch (run->s390_sieic.icptcode) { + case ICPT_PV_INSTR_NOTIFICATION: + g_assert(s390_is_pv()); + /* The notification intercepts are currently handled by KVM */ + error_report("unexpected SCLP PV notification"); + exit(1); + break; + case ICPT_PV_INSTR: + g_assert(s390_is_pv()); + sclp_service_call_protected(env, sccb, code); + break; + case ICPT_INSTRUCTION: + g_assert(!s390_is_pv()); + r = sclp_service_call(env, sccb, code); + if (r < 0) { + kvm_s390_program_interrupt(cpu, -r); + return; + } + setcc(cpu, r); } - setcc(cpu, r); } static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)