| Message ID | 20191024114059.102802-37-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: > Code 5 for the set cpu state UV call tells the UV to load a PSW from > the SE header (first IPL) or from guest location 0x0 (diag 308 subcode > 0/1). Also it sets the cpu into operating state afterwards, so we can > start it. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > arch/s390/include/asm/uv.h | 1 + > arch/s390/kvm/kvm-s390.c | 4 ++++ > include/uapi/linux/kvm.h | 1 + > 3 files changed, 6 insertions(+) > > diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h > index 33b52ba306af..8d10ae731458 100644 > --- a/arch/s390/include/asm/uv.h > +++ b/arch/s390/include/asm/uv.h > @@ -163,6 +163,7 @@ struct uv_cb_unp { > #define PV_CPU_STATE_OPR 1 > #define PV_CPU_STATE_STP 2 > #define PV_CPU_STATE_CHKSTP 3 > +#define PV_CPU_STATE_OPR_LOAD 5 > > struct uv_cb_cpu_set_state { > struct uv_cb_header header; > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index cc5feb67f145..5cc9108c94e4 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -4652,6 +4652,10 @@ static int kvm_s390_handle_pv_vcpu(struct kvm_vcpu *vcpu, > r = kvm_s390_pv_destroy_cpu(vcpu); > break; > } > + case KVM_PV_VCPU_SET_IPL_PSW: { > + r = kvm_s390_pv_set_cpu_state(vcpu, PV_CPU_STATE_OPR_LOAD); > + break; > + } Nit: No need for the curly braces here. Thomas
On Thu, 24 Oct 2019 07:40:58 -0400 Janosch Frank <frankja@linux.ibm.com> wrote: > Code 5 for the set cpu state UV call tells the UV to load a PSW from > the SE header (first IPL) or from guest location 0x0 (diag 308 subcode > 0/1). Also it sets the cpu into operating state afterwards, so we can > start it. I'm a bit confused by the patch description: Does this mean that the UV does the transition to operating state? Does the hypervisor get a notification for that? > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > arch/s390/include/asm/uv.h | 1 + > arch/s390/kvm/kvm-s390.c | 4 ++++ > include/uapi/linux/kvm.h | 1 + > 3 files changed, 6 insertions(+) > > diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h > index 33b52ba306af..8d10ae731458 100644 > --- a/arch/s390/include/asm/uv.h > +++ b/arch/s390/include/asm/uv.h > @@ -163,6 +163,7 @@ struct uv_cb_unp { > #define PV_CPU_STATE_OPR 1 > #define PV_CPU_STATE_STP 2 > #define PV_CPU_STATE_CHKSTP 3 > +#define PV_CPU_STATE_OPR_LOAD 5 > > struct uv_cb_cpu_set_state { > struct uv_cb_header header; > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index cc5feb67f145..5cc9108c94e4 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -4652,6 +4652,10 @@ static int kvm_s390_handle_pv_vcpu(struct kvm_vcpu *vcpu, > r = kvm_s390_pv_destroy_cpu(vcpu); > break; > } > + case KVM_PV_VCPU_SET_IPL_PSW: { > + r = kvm_s390_pv_set_cpu_state(vcpu, PV_CPU_STATE_OPR_LOAD); > + break; > + } > default: > r = -ENOTTY; > } > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > index 2846ed5e5dd9..973007d27d55 100644 > --- a/include/uapi/linux/kvm.h > +++ b/include/uapi/linux/kvm.h > @@ -1483,6 +1483,7 @@ enum pv_cmd_id { > KVM_PV_VM_UNSHARE, > KVM_PV_VCPU_CREATE, > KVM_PV_VCPU_DESTROY, > + KVM_PV_VCPU_SET_IPL_PSW, > }; > > struct kvm_pv_cmd {
On 11/18/19 6:38 PM, Cornelia Huck wrote: > On Thu, 24 Oct 2019 07:40:58 -0400 > Janosch Frank <frankja@linux.ibm.com> wrote: > >> Code 5 for the set cpu state UV call tells the UV to load a PSW from >> the SE header (first IPL) or from guest location 0x0 (diag 308 subcode >> 0/1). Also it sets the cpu into operating state afterwards, so we can >> start it. > > I'm a bit confused by the patch description: Does this mean that the UV > does the transition to operating state? Does the hypervisor get a > notification for that? CMD 5 is defined as "load psw and set to operating". Currently QEMU will still go out and do a "set to operating" after the cmd 5 because our current infrastructure does it and it's basically a nop, so I didn't want to put in the effort to remove it. > >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> arch/s390/include/asm/uv.h | 1 + >> arch/s390/kvm/kvm-s390.c | 4 ++++ >> include/uapi/linux/kvm.h | 1 + >> 3 files changed, 6 insertions(+) >> >> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h >> index 33b52ba306af..8d10ae731458 100644 >> --- a/arch/s390/include/asm/uv.h >> +++ b/arch/s390/include/asm/uv.h >> @@ -163,6 +163,7 @@ struct uv_cb_unp { >> #define PV_CPU_STATE_OPR 1 >> #define PV_CPU_STATE_STP 2 >> #define PV_CPU_STATE_CHKSTP 3 >> +#define PV_CPU_STATE_OPR_LOAD 5 >> >> struct uv_cb_cpu_set_state { >> struct uv_cb_header header; >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index cc5feb67f145..5cc9108c94e4 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -4652,6 +4652,10 @@ static int kvm_s390_handle_pv_vcpu(struct kvm_vcpu *vcpu, >> r = kvm_s390_pv_destroy_cpu(vcpu); >> break; >> } >> + case KVM_PV_VCPU_SET_IPL_PSW: { >> + r = kvm_s390_pv_set_cpu_state(vcpu, PV_CPU_STATE_OPR_LOAD); >> + break; >> + } >> default: >> r = -ENOTTY; >> } >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >> index 2846ed5e5dd9..973007d27d55 100644 >> --- a/include/uapi/linux/kvm.h >> +++ b/include/uapi/linux/kvm.h >> @@ -1483,6 +1483,7 @@ enum pv_cmd_id { >> KVM_PV_VM_UNSHARE, >> KVM_PV_VCPU_CREATE, >> KVM_PV_VCPU_DESTROY, >> + KVM_PV_VCPU_SET_IPL_PSW, >> }; >> >> struct kvm_pv_cmd { >
On Tue, 19 Nov 2019 09:13:11 +0100 Janosch Frank <frankja@linux.ibm.com> wrote: > On 11/18/19 6:38 PM, Cornelia Huck wrote: > > On Thu, 24 Oct 2019 07:40:58 -0400 > > Janosch Frank <frankja@linux.ibm.com> wrote: > > > >> Code 5 for the set cpu state UV call tells the UV to load a PSW from > >> the SE header (first IPL) or from guest location 0x0 (diag 308 subcode > >> 0/1). Also it sets the cpu into operating state afterwards, so we can > >> start it. > > > > I'm a bit confused by the patch description: Does this mean that the UV > > does the transition to operating state? Does the hypervisor get a > > notification for that? > > CMD 5 is defined as "load psw and set to operating". > Currently QEMU will still go out and do a "set to operating" after the > cmd 5 because our current infrastructure does it and it's basically a > nop, so I didn't want to put in the effort to remove it. So, the "it" setting the cpu into operating state is QEMU, via the mpstate interface, which triggers that call? Or is that implicit, but it does not hurt to do it again (which would make more sense to me)? Assuming the latter, what about the following description: "KVM: s390: protvirt: support setting cpu state 5 Setting code 5 ("load psw and set to operating") in the set cpu state UV call tells the UV to load a PSW either from the SE header (first IPL) or from guest location 0x0 (diag 308 subcode 0/1). Subsequently, the cpu is set into operating state by the UV. Note that we can still instruct the UV to set the cpu into operating state explicitly afterwards." > > > > >> > >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > >> --- > >> arch/s390/include/asm/uv.h | 1 + > >> arch/s390/kvm/kvm-s390.c | 4 ++++ > >> include/uapi/linux/kvm.h | 1 + > >> 3 files changed, 6 insertions(+) > >> > >> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h > >> index 33b52ba306af..8d10ae731458 100644 > >> --- a/arch/s390/include/asm/uv.h > >> +++ b/arch/s390/include/asm/uv.h > >> @@ -163,6 +163,7 @@ struct uv_cb_unp { > >> #define PV_CPU_STATE_OPR 1 > >> #define PV_CPU_STATE_STP 2 > >> #define PV_CPU_STATE_CHKSTP 3 > >> +#define PV_CPU_STATE_OPR_LOAD 5 > >> > >> struct uv_cb_cpu_set_state { > >> struct uv_cb_header header; > >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > >> index cc5feb67f145..5cc9108c94e4 100644 > >> --- a/arch/s390/kvm/kvm-s390.c > >> +++ b/arch/s390/kvm/kvm-s390.c > >> @@ -4652,6 +4652,10 @@ static int kvm_s390_handle_pv_vcpu(struct kvm_vcpu *vcpu, > >> r = kvm_s390_pv_destroy_cpu(vcpu); > >> break; > >> } > >> + case KVM_PV_VCPU_SET_IPL_PSW: { > >> + r = kvm_s390_pv_set_cpu_state(vcpu, PV_CPU_STATE_OPR_LOAD); Also maybe add a comment here that setting into oper state (again) can be done separately? > >> + break; > >> + } > >> default: > >> r = -ENOTTY; > >> } > >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > >> index 2846ed5e5dd9..973007d27d55 100644 > >> --- a/include/uapi/linux/kvm.h > >> +++ b/include/uapi/linux/kvm.h > >> @@ -1483,6 +1483,7 @@ enum pv_cmd_id { > >> KVM_PV_VM_UNSHARE, > >> KVM_PV_VCPU_CREATE, > >> KVM_PV_VCPU_DESTROY, > >> + KVM_PV_VCPU_SET_IPL_PSW, > >> }; > >> > >> struct kvm_pv_cmd { > > > >
On 11/19/19 11:23 AM, Cornelia Huck wrote: > On Tue, 19 Nov 2019 09:13:11 +0100 > Janosch Frank <frankja@linux.ibm.com> wrote: > >> On 11/18/19 6:38 PM, Cornelia Huck wrote: >>> On Thu, 24 Oct 2019 07:40:58 -0400 >>> Janosch Frank <frankja@linux.ibm.com> wrote: >>> >>>> Code 5 for the set cpu state UV call tells the UV to load a PSW from >>>> the SE header (first IPL) or from guest location 0x0 (diag 308 subcode >>>> 0/1). Also it sets the cpu into operating state afterwards, so we can >>>> start it. >>> >>> I'm a bit confused by the patch description: Does this mean that the UV >>> does the transition to operating state? Does the hypervisor get a >>> notification for that? >> >> CMD 5 is defined as "load psw and set to operating". >> Currently QEMU will still go out and do a "set to operating" after the >> cmd 5 because our current infrastructure does it and it's basically a >> nop, so I didn't want to put in the effort to remove it. > > So, the "it" setting the cpu into operating state is QEMU, via the > mpstate interface, which triggers that call? Or is that implicit, but > it does not hurt to do it again (which would make more sense to me)? Qemu sets operating state via mpstate. I could fence that via env->pv checks but that would also mean more code and setting operating twice is just a nop. > > Assuming the latter, what about the following description: > > "KVM: s390: protvirt: support setting cpu state 5 > > Setting code 5 ("load psw and set to operating") in the set cpu state > UV call tells the UV to load a PSW either from the SE header (first > IPL) or from guest location 0x0 (diag 308 subcode 0/1). Subsequently, > the cpu is set into operating state by the UV. > > Note that we can still instruct the UV to set the cpu into operating > state explicitly afterwards." Sounds good > >> >>> >>>> >>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>> --- >>>> arch/s390/include/asm/uv.h | 1 + >>>> arch/s390/kvm/kvm-s390.c | 4 ++++ >>>> include/uapi/linux/kvm.h | 1 + >>>> 3 files changed, 6 insertions(+) >>>> >>>> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h >>>> index 33b52ba306af..8d10ae731458 100644 >>>> --- a/arch/s390/include/asm/uv.h >>>> +++ b/arch/s390/include/asm/uv.h >>>> @@ -163,6 +163,7 @@ struct uv_cb_unp { >>>> #define PV_CPU_STATE_OPR 1 >>>> #define PV_CPU_STATE_STP 2 >>>> #define PV_CPU_STATE_CHKSTP 3 >>>> +#define PV_CPU_STATE_OPR_LOAD 5 >>>> >>>> struct uv_cb_cpu_set_state { >>>> struct uv_cb_header header; >>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>>> index cc5feb67f145..5cc9108c94e4 100644 >>>> --- a/arch/s390/kvm/kvm-s390.c >>>> +++ b/arch/s390/kvm/kvm-s390.c >>>> @@ -4652,6 +4652,10 @@ static int kvm_s390_handle_pv_vcpu(struct kvm_vcpu *vcpu, >>>> r = kvm_s390_pv_destroy_cpu(vcpu); >>>> break; >>>> } >>>> + case KVM_PV_VCPU_SET_IPL_PSW: { >>>> + r = kvm_s390_pv_set_cpu_state(vcpu, PV_CPU_STATE_OPR_LOAD); > > Also maybe add a comment here that setting into oper state (again) can > be done separately? > >>>> + break; >>>> + } >>>> default: >>>> r = -ENOTTY; >>>> } >>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >>>> index 2846ed5e5dd9..973007d27d55 100644 >>>> --- a/include/uapi/linux/kvm.h >>>> +++ b/include/uapi/linux/kvm.h >>>> @@ -1483,6 +1483,7 @@ enum pv_cmd_id { >>>> KVM_PV_VM_UNSHARE, >>>> KVM_PV_VCPU_CREATE, >>>> KVM_PV_VCPU_DESTROY, >>>> + KVM_PV_VCPU_SET_IPL_PSW, >>>> }; >>>> >>>> struct kvm_pv_cmd { >>> >> >> >
diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h index 33b52ba306af..8d10ae731458 100644 --- a/arch/s390/include/asm/uv.h +++ b/arch/s390/include/asm/uv.h @@ -163,6 +163,7 @@ struct uv_cb_unp { #define PV_CPU_STATE_OPR 1 #define PV_CPU_STATE_STP 2 #define PV_CPU_STATE_CHKSTP 3 +#define PV_CPU_STATE_OPR_LOAD 5 struct uv_cb_cpu_set_state { struct uv_cb_header header; diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index cc5feb67f145..5cc9108c94e4 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -4652,6 +4652,10 @@ static int kvm_s390_handle_pv_vcpu(struct kvm_vcpu *vcpu, r = kvm_s390_pv_destroy_cpu(vcpu); break; } + case KVM_PV_VCPU_SET_IPL_PSW: { + r = kvm_s390_pv_set_cpu_state(vcpu, PV_CPU_STATE_OPR_LOAD); + break; + } default: r = -ENOTTY; } diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index 2846ed5e5dd9..973007d27d55 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -1483,6 +1483,7 @@ enum pv_cmd_id { KVM_PV_VM_UNSHARE, KVM_PV_VCPU_CREATE, KVM_PV_VCPU_DESTROY, + KVM_PV_VCPU_SET_IPL_PSW, }; struct kvm_pv_cmd {
Code 5 for the set cpu state UV call tells the UV to load a PSW from the SE header (first IPL) or from guest location 0x0 (diag 308 subcode 0/1). Also it sets the cpu into operating state afterwards, so we can start it. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- arch/s390/include/asm/uv.h | 1 + arch/s390/kvm/kvm-s390.c | 4 ++++ include/uapi/linux/kvm.h | 1 + 3 files changed, 6 insertions(+)