| Message ID | 20191024114059.102802-34-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: Add support for protected VMs | expand |
On 24/10/2019 13.40, Janosch Frank wrote: > With PV we need to do things for all reset types, not only initial... > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > arch/s390/kvm/kvm-s390.c | 53 ++++++++++++++++++++++++++++++++++++++++ > include/uapi/linux/kvm.h | 6 +++++ > 2 files changed, 59 insertions(+) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index d3fd3ad1d09b..d8ee3a98e961 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -3472,6 +3472,53 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu) > return 0; > } > > +static int kvm_arch_vcpu_ioctl_reset(struct kvm_vcpu *vcpu, > + unsigned long type) > +{ > + int rc; > + u32 ret; > + > + switch (type) { > + case KVM_S390_VCPU_RESET_NORMAL: > + /* > + * Only very little is reset, userspace handles the > + * non-protected case. > + */ > + rc = 0; > + if (kvm_s390_pv_handle_cpu(vcpu)) { > + rc = uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu), > + UVC_CMD_CPU_RESET, &ret); > + VCPU_EVENT(vcpu, 3, "PROTVIRT RESET NORMAL VCPU: cpu %d rc %x rrc %x", > + vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff); > + } > + break; > + case KVM_S390_VCPU_RESET_INITIAL: > + rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu); > + if (kvm_s390_pv_handle_cpu(vcpu)) { > + uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu), > + UVC_CMD_CPU_RESET_INITIAL, > + &ret); > + VCPU_EVENT(vcpu, 3, "PROTVIRT RESET INITIAL VCPU: cpu %d rc %x rrc %x", > + vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff); > + } > + break; > + case KVM_S390_VCPU_RESET_CLEAR: > + rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu); > + if (kvm_s390_pv_handle_cpu(vcpu)) { > + rc = uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu), > + UVC_CMD_CPU_RESET_CLEAR, &ret); > + VCPU_EVENT(vcpu, 3, "PROTVIRT RESET CLEAR VCPU: cpu %d rc %x rrc %x", > + vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff); > + } > + break; > + default: > + rc = -EINVAL; > + break; (nit: you could drop the "break;" here) > + } > + return rc; > +} > + > + > int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs) > { > vcpu_load(vcpu); > @@ -4633,8 +4680,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp, > break; > } > case KVM_S390_INITIAL_RESET: > + r = -EINVAL; > + if (kvm_s390_pv_is_protected(vcpu->kvm)) > + break; Wouldn't it be nicer to call kvm_arch_vcpu_ioctl_reset(vcpu, KVM_S390_VCPU_RESET_INITIAL) in this case instead? > r = kvm_arch_vcpu_ioctl_initial_reset(vcpu); > break; > + case KVM_S390_VCPU_RESET: > + r = kvm_arch_vcpu_ioctl_reset(vcpu, arg); > + break; > case KVM_SET_ONE_REG: > case KVM_GET_ONE_REG: { > struct kvm_one_reg reg; > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > index f75a051a7705..2846ed5e5dd9 100644 > --- a/include/uapi/linux/kvm.h > +++ b/include/uapi/linux/kvm.h > @@ -1496,6 +1496,12 @@ struct kvm_pv_cmd { > #define KVM_S390_PV_COMMAND _IOW(KVMIO, 0xc3, struct kvm_pv_cmd) > #define KVM_S390_PV_COMMAND_VCPU _IOW(KVMIO, 0xc4, struct kvm_pv_cmd) > > +#define KVM_S390_VCPU_RESET_NORMAL 0 > +#define KVM_S390_VCPU_RESET_INITIAL 1 > +#define KVM_S390_VCPU_RESET_CLEAR 2 > + > +#define KVM_S390_VCPU_RESET _IO(KVMIO, 0xd0) Why not 0xc5 ? Thomas
On 11/15/19 11:47 AM, Thomas Huth wrote: > On 24/10/2019 13.40, Janosch Frank wrote: >> With PV we need to do things for all reset types, not only initial... >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> arch/s390/kvm/kvm-s390.c | 53 ++++++++++++++++++++++++++++++++++++++++ >> include/uapi/linux/kvm.h | 6 +++++ >> 2 files changed, 59 insertions(+) >> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index d3fd3ad1d09b..d8ee3a98e961 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -3472,6 +3472,53 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu) >> return 0; >> } >> >> +static int kvm_arch_vcpu_ioctl_reset(struct kvm_vcpu *vcpu, >> + unsigned long type) >> +{ >> + int rc; >> + u32 ret; >> + >> + switch (type) { >> + case KVM_S390_VCPU_RESET_NORMAL: >> + /* >> + * Only very little is reset, userspace handles the >> + * non-protected case. >> + */ >> + rc = 0; >> + if (kvm_s390_pv_handle_cpu(vcpu)) { >> + rc = uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu), >> + UVC_CMD_CPU_RESET, &ret); >> + VCPU_EVENT(vcpu, 3, "PROTVIRT RESET NORMAL VCPU: cpu %d rc %x rrc %x", >> + vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff); >> + } >> + break; >> + case KVM_S390_VCPU_RESET_INITIAL: >> + rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu); >> + if (kvm_s390_pv_handle_cpu(vcpu)) { >> + uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu), >> + UVC_CMD_CPU_RESET_INITIAL, >> + &ret); >> + VCPU_EVENT(vcpu, 3, "PROTVIRT RESET INITIAL VCPU: cpu %d rc %x rrc %x", >> + vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff); >> + } >> + break; >> + case KVM_S390_VCPU_RESET_CLEAR: >> + rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu); >> + if (kvm_s390_pv_handle_cpu(vcpu)) { >> + rc = uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu), >> + UVC_CMD_CPU_RESET_CLEAR, &ret); >> + VCPU_EVENT(vcpu, 3, "PROTVIRT RESET CLEAR VCPU: cpu %d rc %x rrc %x", >> + vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff); >> + } >> + break; >> + default: >> + rc = -EINVAL; >> + break; > > (nit: you could drop the "break;" here) > >> + } >> + return rc; >> +} >> + >> + >> int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs) >> { >> vcpu_load(vcpu); >> @@ -4633,8 +4680,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp, >> break; >> } >> case KVM_S390_INITIAL_RESET: >> + r = -EINVAL; >> + if (kvm_s390_pv_is_protected(vcpu->kvm)) >> + break; > > Wouldn't it be nicer to call > > kvm_arch_vcpu_ioctl_reset(vcpu, KVM_S390_VCPU_RESET_INITIAL) > > in this case instead? How about: case KVM_S390_INITIAL_RESET: arg = KVM_S390_VCPU_RESET_INITIAL; case KVM_S390_VCPU_RESET: r = kvm_arch_vcpu_ioctl_reset(vcpu, arg); break; > >> r = kvm_arch_vcpu_ioctl_initial_reset(vcpu); >> break; >> + case KVM_S390_VCPU_RESET: >> + r = kvm_arch_vcpu_ioctl_reset(vcpu, arg); >> + break; >> case KVM_SET_ONE_REG: >> case KVM_GET_ONE_REG: { >> struct kvm_one_reg reg; >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >> index f75a051a7705..2846ed5e5dd9 100644 >> --- a/include/uapi/linux/kvm.h >> +++ b/include/uapi/linux/kvm.h >> @@ -1496,6 +1496,12 @@ struct kvm_pv_cmd { >> #define KVM_S390_PV_COMMAND _IOW(KVMIO, 0xc3, struct kvm_pv_cmd) >> #define KVM_S390_PV_COMMAND_VCPU _IOW(KVMIO, 0xc4, struct kvm_pv_cmd) >> >> +#define KVM_S390_VCPU_RESET_NORMAL 0 >> +#define KVM_S390_VCPU_RESET_INITIAL 1 >> +#define KVM_S390_VCPU_RESET_CLEAR 2 >> + >> +#define KVM_S390_VCPU_RESET _IO(KVMIO, 0xd0) > > Why not 0xc5 ? Fixed > > Thomas >
On 15/11/2019 14.06, Janosch Frank wrote: > On 11/15/19 11:47 AM, Thomas Huth wrote: >> On 24/10/2019 13.40, Janosch Frank wrote: >>> With PV we need to do things for all reset types, not only initial... >>> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>> --- >>> arch/s390/kvm/kvm-s390.c | 53 ++++++++++++++++++++++++++++++++++++++++ >>> include/uapi/linux/kvm.h | 6 +++++ >>> 2 files changed, 59 insertions(+) >>> >>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>> index d3fd3ad1d09b..d8ee3a98e961 100644 >>> --- a/arch/s390/kvm/kvm-s390.c >>> +++ b/arch/s390/kvm/kvm-s390.c >>> @@ -3472,6 +3472,53 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu) >>> return 0; >>> } >>> >>> +static int kvm_arch_vcpu_ioctl_reset(struct kvm_vcpu *vcpu, >>> + unsigned long type) >>> +{ >>> + int rc; >>> + u32 ret; >>> + >>> + switch (type) { >>> + case KVM_S390_VCPU_RESET_NORMAL: >>> + /* >>> + * Only very little is reset, userspace handles the >>> + * non-protected case. >>> + */ >>> + rc = 0; >>> + if (kvm_s390_pv_handle_cpu(vcpu)) { >>> + rc = uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu), >>> + UVC_CMD_CPU_RESET, &ret); >>> + VCPU_EVENT(vcpu, 3, "PROTVIRT RESET NORMAL VCPU: cpu %d rc %x rrc %x", >>> + vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff); >>> + } >>> + break; >>> + case KVM_S390_VCPU_RESET_INITIAL: >>> + rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu); >>> + if (kvm_s390_pv_handle_cpu(vcpu)) { >>> + uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu), >>> + UVC_CMD_CPU_RESET_INITIAL, >>> + &ret); >>> + VCPU_EVENT(vcpu, 3, "PROTVIRT RESET INITIAL VCPU: cpu %d rc %x rrc %x", >>> + vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff); >>> + } >>> + break; >>> + case KVM_S390_VCPU_RESET_CLEAR: >>> + rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu); >>> + if (kvm_s390_pv_handle_cpu(vcpu)) { >>> + rc = uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu), >>> + UVC_CMD_CPU_RESET_CLEAR, &ret); >>> + VCPU_EVENT(vcpu, 3, "PROTVIRT RESET CLEAR VCPU: cpu %d rc %x rrc %x", >>> + vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff); >>> + } >>> + break; >>> + default: >>> + rc = -EINVAL; >>> + break; >> >> (nit: you could drop the "break;" here) >> >>> + } >>> + return rc; >>> +} >>> + >>> + >>> int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs) >>> { >>> vcpu_load(vcpu); >>> @@ -4633,8 +4680,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp, >>> break; >>> } >>> case KVM_S390_INITIAL_RESET: >>> + r = -EINVAL; >>> + if (kvm_s390_pv_is_protected(vcpu->kvm)) >>> + break; >> >> Wouldn't it be nicer to call >> >> kvm_arch_vcpu_ioctl_reset(vcpu, KVM_S390_VCPU_RESET_INITIAL) >> >> in this case instead? > > How about: > case KVM_S390_INITIAL_RESET: > > > arg = KVM_S390_VCPU_RESET_INITIAL; > Add a "/* fallthrough */" comment here... > case KVM_S390_VCPU_RESET: > > > r = kvm_arch_vcpu_ioctl_reset(vcpu, arg); > > > break; ... then this looks good, yes! Thomas
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index d3fd3ad1d09b..d8ee3a98e961 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -3472,6 +3472,53 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu) return 0; } +static int kvm_arch_vcpu_ioctl_reset(struct kvm_vcpu *vcpu, + unsigned long type) +{ + int rc; + u32 ret; + + switch (type) { + case KVM_S390_VCPU_RESET_NORMAL: + /* + * Only very little is reset, userspace handles the + * non-protected case. + */ + rc = 0; + if (kvm_s390_pv_handle_cpu(vcpu)) { + rc = uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu), + UVC_CMD_CPU_RESET, &ret); + VCPU_EVENT(vcpu, 3, "PROTVIRT RESET NORMAL VCPU: cpu %d rc %x rrc %x", + vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff); + } + break; + case KVM_S390_VCPU_RESET_INITIAL: + rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu); + if (kvm_s390_pv_handle_cpu(vcpu)) { + uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu), + UVC_CMD_CPU_RESET_INITIAL, + &ret); + VCPU_EVENT(vcpu, 3, "PROTVIRT RESET INITIAL VCPU: cpu %d rc %x rrc %x", + vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff); + } + break; + case KVM_S390_VCPU_RESET_CLEAR: + rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu); + if (kvm_s390_pv_handle_cpu(vcpu)) { + rc = uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu), + UVC_CMD_CPU_RESET_CLEAR, &ret); + VCPU_EVENT(vcpu, 3, "PROTVIRT RESET CLEAR VCPU: cpu %d rc %x rrc %x", + vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff); + } + break; + default: + rc = -EINVAL; + break; + } + return rc; +} + + int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs) { vcpu_load(vcpu); @@ -4633,8 +4680,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp, break; } case KVM_S390_INITIAL_RESET: + r = -EINVAL; + if (kvm_s390_pv_is_protected(vcpu->kvm)) + break; r = kvm_arch_vcpu_ioctl_initial_reset(vcpu); break; + case KVM_S390_VCPU_RESET: + r = kvm_arch_vcpu_ioctl_reset(vcpu, arg); + break; case KVM_SET_ONE_REG: case KVM_GET_ONE_REG: { struct kvm_one_reg reg; diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index f75a051a7705..2846ed5e5dd9 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -1496,6 +1496,12 @@ struct kvm_pv_cmd { #define KVM_S390_PV_COMMAND _IOW(KVMIO, 0xc3, struct kvm_pv_cmd) #define KVM_S390_PV_COMMAND_VCPU _IOW(KVMIO, 0xc4, struct kvm_pv_cmd) +#define KVM_S390_VCPU_RESET_NORMAL 0 +#define KVM_S390_VCPU_RESET_INITIAL 1 +#define KVM_S390_VCPU_RESET_CLEAR 2 + +#define KVM_S390_VCPU_RESET _IO(KVMIO, 0xd0) + /* Secure Encrypted Virtualization command */ enum sev_cmd_id { /* Guest initialization commands */
With PV we need to do things for all reset types, not only initial... Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- arch/s390/kvm/kvm-s390.c | 53 ++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/kvm.h | 6 +++++ 2 files changed, 59 insertions(+)