diff mbox series

[RFC,27/37] KVM: s390: protvirt: SIGP handling

Message ID 20191024114059.102802-28-frankja@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series KVM: s390: Add support for protected VMs | expand

Commit Message

Janosch Frank Oct. 24, 2019, 11:40 a.m. UTC 
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 arch/s390/kvm/intercept.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

David Hildenbrand Oct. 30, 2019, 6:29 p.m. UTC | #1 
On 24.10.19 13:40, Janosch Frank wrote:
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Can you add why this is necessary and how handle_stop() is intended to 
work in prot mode?

How is SIGP handled in general in prot mode? (which intercepts are 
handled by QEMU)
Would it be valid for user space to inject a STOP interrupt with "flags 
& KVM_S390_STOP_FLAG_STORE_STATUS" - I think not (legacy QEMU only)

I think we should rather disallow injecting such stop interrupts 
(KVM_S390_STOP_FLAG_STORE_STATUS) in prot mode in the first place. Also, 
we should disallow prot virt without user_sigp.

> ---
>   arch/s390/kvm/intercept.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> index 37cb62bc261b..a89738e4f761 100644
> --- a/arch/s390/kvm/intercept.c
> +++ b/arch/s390/kvm/intercept.c
> @@ -72,7 +72,8 @@ static int handle_stop(struct kvm_vcpu *vcpu)
>   	if (!stop_pending)
>   		return 0;
>   
> -	if (flags & KVM_S390_STOP_FLAG_STORE_STATUS) {
> +	if (flags & KVM_S390_STOP_FLAG_STORE_STATUS &&
> +	    !kvm_s390_pv_is_protected(vcpu->kvm)) {
>   		rc = kvm_s390_vcpu_store_status(vcpu,
>   						KVM_S390_STORE_STATUS_NOADDR);
>   		if (rc)
>
Thomas Huth Nov. 15, 2019, 11:15 a.m. UTC | #2 
On 24/10/2019 13.40, Janosch Frank wrote:
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  arch/s390/kvm/intercept.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> index 37cb62bc261b..a89738e4f761 100644
> --- a/arch/s390/kvm/intercept.c
> +++ b/arch/s390/kvm/intercept.c
> @@ -72,7 +72,8 @@ static int handle_stop(struct kvm_vcpu *vcpu)
>  	if (!stop_pending)
>  		return 0;
>  
> -	if (flags & KVM_S390_STOP_FLAG_STORE_STATUS) {
> +	if (flags & KVM_S390_STOP_FLAG_STORE_STATUS &&
> +	    !kvm_s390_pv_is_protected(vcpu->kvm)) {
>  		rc = kvm_s390_vcpu_store_status(vcpu,
>  						KVM_S390_STORE_STATUS_NOADDR);
>  		if (rc)

Can this still happen at all that we get here with
KVM_S390_STOP_FLAG_STORE_STATUS in the protected case? I'd rather expect
that SIGP is completely handled by the UV already, so userspace should
have no need to inject a SIGP_STOP anymore? Or did I get that wrong?

Anyway, I guess it can not hurt to add this check anyway, so:

Reviewed-by: Thomas Huth <thuth@redhat.com>
diff mbox series

Patch

diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index 37cb62bc261b..a89738e4f761 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -72,7 +72,8 @@  static int handle_stop(struct kvm_vcpu *vcpu)
 	if (!stop_pending)
 		return 0;
 
-	if (flags & KVM_S390_STOP_FLAG_STORE_STATUS) {
+	if (flags & KVM_S390_STOP_FLAG_STORE_STATUS &&
+	    !kvm_s390_pv_is_protected(vcpu->kvm)) {
 		rc = kvm_s390_vcpu_store_status(vcpu,
 						KVM_S390_STORE_STATUS_NOADDR);
 		if (rc)