[GIT PULL 0/1] KVM: s390: Fix for running nested under z/VM

[GIT PULL 0/1] KVM: s390: Fix for running nested under z/VM

[Christian Borntraeger]

Paolo,
a fix for kvm/master.

The following changes since commit 2a173ec993baa6a97e7b0fb89240200a88d90746:

  MAINTAINERS: add a reviewer for KVM/s390 (2020-04-20 11:24:00 +0200)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git  tags/kvm-s390-master-5.7-3

for you to fetch changes up to 5615e74f48dcc982655543e979b6c3f3f877e6f6:

  KVM: s390: Remove false WARN_ON_ONCE for the PQAP instruction (2020-05-05 11:15:05 +0200)

----------------------------------------------------------------
KVM: s390: Fix for running nested uner z/VM

There are circumstances when running nested under z/VM that would trigger a
WARN_ON_ONCE. Remove the WARN_ON_ONCE. Long term we certainly want to make this
code more robust and flexible, but just returning instead of WARNING makes
guest bootable again.

----------------------------------------------------------------
Christian Borntraeger (1):
      KVM: s390: Remove false WARN_ON_ONCE for the PQAP instruction

 arch/s390/kvm/priv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

[GIT PULL 1/1] KVM: s390: Remove false WARN_ON_ONCE for the PQAP instruction

[Christian Borntraeger]

In LPAR we will only get an intercept for FC==3 for the PQAP
instruction. Running nested under z/VM can result in other intercepts as
well as ECA_APIE is an effective bit: If one hypervisor layer has
turned this bit off, the end result will be that we will get intercepts for
all function codes. Usually the first one will be a query like PQAP(QCI).
So the WARN_ON_ONCE is not right. Let us simply remove it.

Cc: Pierre Morel <pmorel@linux.ibm.com>
Cc: Tony Krowiak <akrowiak@linux.ibm.com>
Cc: stable@vger.kernel.org # v5.3+
Fixes: e5282de93105 ("s390: ap: kvm: add PQAP interception for AQIC")
Link: https://lore.kernel.org/kvm/20200505083515.2720-1-borntraeger@de.ibm.com
Reported-by: Qian Cai <cailca@icloud.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/priv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index 69a824f9ef0b..893893642415 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -626,10 +626,12 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
 	 * available for the guest are AQIC and TAPQ with the t bit set
 	 * since we do not set IC.3 (FIII) we currently will only intercept
 	 * the AQIC function code.
+	 * Note: running nested under z/VM can result in intercepts for other
+	 * function codes, e.g. PQAP(QCI). We do not support this and bail out.
 	 */
 	reg0 = vcpu->run->s.regs.gprs[0];
 	fc = (reg0 >> 24) & 0xff;
-	if (WARN_ON_ONCE(fc != 0x03))
+	if (fc != 0x03)
 		return -EOPNOTSUPP;
 
 	/* PQAP instruction is allowed for guest kernel only */
-- 
2.25.4

Re: [GIT PULL 1/1] KVM: s390: Remove false WARN_ON_ONCE for the PQAP instruction

[Paolo Bonzini]

On 06/05/20 13:59, Christian Borntraeger wrote:
> Running nested under z/VM can result in other intercepts as
> well as ECA_APIE is an effective bit: If one hypervisor layer has
> turned this bit off, the end result will be that we will get intercepts for
> all function codes. Usually the first one will be a query like PQAP(QCI).
> So the WARN_ON_ONCE is not right. Let us simply remove it.

Possibly stupid question since I can only recognize some words here. :)
But anyway... shouldn't z/VM trap this intercept when the guest has
turned off the bit, and only reflect the SIE exit based on the function
code?

Thanks,

Paolo

Re: [GIT PULL 1/1] KVM: s390: Remove false WARN_ON_ONCE for the PQAP instruction

[Christian Borntraeger]

On 06.05.20 14:12, Paolo Bonzini wrote:
> On 06/05/20 13:59, Christian Borntraeger wrote:
>> Running nested under z/VM can result in other intercepts as
>> well as ECA_APIE is an effective bit: If one hypervisor layer has
>> turned this bit off, the end result will be that we will get intercepts for
>> all function codes. Usually the first one will be a query like PQAP(QCI).
>> So the WARN_ON_ONCE is not right. Let us simply remove it.
> 
> Possibly stupid question since I can only recognize some words here. :)
> But anyway... shouldn't z/VM trap this intercept when the guest has
> turned off the bit, and only reflect the SIE exit based on the function
> code?

So here is my theory.

the problem is that z/VM seems to have disabled that bit. The interpretion
only works when all layers have this bit set. (Its called an effective bit). 
So we have
LPAR -> ECA_APIE = 1
Z/VM -> ECA_APIE = 0
KVM  -> ECA_APIE = 1
nested KVM guest --> does during boot in ap_instructions_available PQAP with
FC==0 to test if crypto is available.

As the nested guest is in fact a shadow guest of z/VM
this will now intercept to z/VM, which will forward that to KVM.

As PQAP with FC==0 DOES work in the KVM, KVM believes that we have crypto
support and we will enable it for our guests. So all the early checks in
handle_pqap will succeed until we run in the check for fc!=3.

In the end this is obviously a case how this warning can trigger. So we better
remove it and rely on the error path to clean up.

Re: [GIT PULL 1/1] KVM: s390: Remove false WARN_ON_ONCE for the PQAP instruction

[Paolo Bonzini]

On 06/05/20 14:23, Christian Borntraeger wrote:
> 
> the problem is that z/VM seems to have disabled that bit. The interpretion
> only works when all layers have this bit set. (Its called an effective bit). 
> So we have
> LPAR -> ECA_APIE = 1
> Z/VM -> ECA_APIE = 0
> KVM  -> ECA_APIE = 1
> nested KVM guest --> does during boot in ap_instructions_available PQAP with
> FC==0 to test if crypto is available.
> 
> As the nested guest is in fact a shadow guest of z/VM
> this will now intercept to z/VM, which will forward that to KVM.

Right, and I'd understand, that since KVM has set ECA_APIE=1, z/VM
should either:

* not forward the intercept to KVM unless FC==3

* toggle ECA_APIE back to 1 while running the KVM nested guest.

Of course I have no idea if either of these choices is impossible, or
too expensive, but this is how we'd try to handle it for x86 features.

So it would be a z/VM bug, because it's not virtualizing ECA_APIE=1
correctly.  The next question is, if removing the WARN_ON is okay for
you, should KVM not bother setting ECA_APIE=1 so that you don't trigger
the z/VM bug at all?

Thanks,

Paolo

> 
> As PQAP with FC==0 DOES work in the KVM, KVM believes that we have crypto
> support and we will enable it for our guests. So all the early checks in
> handle_pqap will succeed until we run in the check for fc!=3.
> 
> In the end this is obviously a case how this warning can trigger. So we better
> remove it and rely on the error path to clean up.

Re: [GIT PULL 1/1] KVM: s390: Remove false WARN_ON_ONCE for the PQAP instruction

[David Hildenbrand]

On 06.05.20 15:04, Paolo Bonzini wrote:
> On 06/05/20 14:23, Christian Borntraeger wrote:
>>
>> the problem is that z/VM seems to have disabled that bit. The interpretion
>> only works when all layers have this bit set. (Its called an effective bit). 
>> So we have
>> LPAR -> ECA_APIE = 1
>> Z/VM -> ECA_APIE = 0
>> KVM  -> ECA_APIE = 1
>> nested KVM guest --> does during boot in ap_instructions_available PQAP with
>> FC==0 to test if crypto is available.
>>
>> As the nested guest is in fact a shadow guest of z/VM
>> this will now intercept to z/VM, which will forward that to KVM.
> 
> Right, and I'd understand, that since KVM has set ECA_APIE=1, z/VM
> should either:
> 
> * not forward the intercept to KVM unless FC==3
> 
> * toggle ECA_APIE back to 1 while running the KVM nested guest.
> 
> Of course I have no idea if either of these choices is impossible, or
> too expensive, but this is how we'd try to handle it for x86 features.
> 
> So it would be a z/VM bug, because it's not virtualizing ECA_APIE=1
> correctly.  The next question is, if removing the WARN_ON is okay for
> you, should KVM not bother setting ECA_APIE=1 so that you don't trigger
> the z/VM bug at all?
> 

IIRC, it's perfectly valid - according to the documentation - to ignore
ECA_APIE. That's the weird thing about effective controls. You don't
really know what you get in the end.

But, it's a long time since I had the chance to look at that
documentation ...

-- 
Thanks,

David / dhildenb

Re: [GIT PULL 1/1] KVM: s390: Remove false WARN_ON_ONCE for the PQAP instruction

[Paolo Bonzini]

On 06/05/20 15:09, David Hildenbrand wrote:
> IIRC, it's perfectly valid - according to the documentation - to ignore
> ECA_APIE. That's the weird thing about effective controls. You don't
> really know what you get in the end.

Ah okay, then yeah it could be considered a quality of implementation
issue but not a bug.  Thanks for the lecture! :)

Paolo

Re: [GIT PULL 1/1] KVM: s390: Remove false WARN_ON_ONCE for the PQAP instruction

[Christian Borntraeger]

On 06.05.20 15:04, Paolo Bonzini wrote:
> On 06/05/20 14:23, Christian Borntraeger wrote:
>>
>> the problem is that z/VM seems to have disabled that bit. The interpretion
>> only works when all layers have this bit set. (Its called an effective bit). 
>> So we have
>> LPAR -> ECA_APIE = 1
>> Z/VM -> ECA_APIE = 0
>> KVM  -> ECA_APIE = 1
>> nested KVM guest --> does during boot in ap_instructions_available PQAP with
>> FC==0 to test if crypto is available.
>>
>> As the nested guest is in fact a shadow guest of z/VM
>> this will now intercept to z/VM, which will forward that to KVM.
> 
> Right, and I'd understand, that since KVM has set ECA_APIE=1, z/VM
> should either:
> 
> * not forward the intercept to KVM unless FC==3
> 
> * toggle ECA_APIE back to 1 while running the KVM nested guest.
> 
> Of course I have no idea if either of these choices is impossible, or
> too expensive, but this is how we'd try to handle it for x86 features.
> 
> So it would be a z/VM bug, because it's not virtualizing ECA_APIE=1

It would be kind of a z/VM bug, but its ok according to the docs.

> correctly.  The next question is, if removing the WARN_ON is okay for
> you, should KVM not bother setting ECA_APIE=1 so that you don't trigger
> the z/VM bug at all?

We need ECA_APIE as this is the only way to provide hardware passthrough.
In the end running KVM under z/VM kind of works but it is not something that
I would consider supported. 

Re: [GIT PULL 0/1] KVM: s390: Fix for running nested under z/VM

[Paolo Bonzini]

On 06/05/20 13:59, Christian Borntraeger wrote:
> Paolo,
> a fix for kvm/master.
> 
> The following changes since commit 2a173ec993baa6a97e7b0fb89240200a88d90746:
> 
>   MAINTAINERS: add a reviewer for KVM/s390 (2020-04-20 11:24:00 +0200)
> 
> are available in the Git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git  tags/kvm-s390-master-5.7-3
> 
> for you to fetch changes up to 5615e74f48dcc982655543e979b6c3f3f877e6f6:
> 
>   KVM: s390: Remove false WARN_ON_ONCE for the PQAP instruction (2020-05-05 11:15:05 +0200)
> 
> ----------------------------------------------------------------
> KVM: s390: Fix for running nested uner z/VM
> 
> There are circumstances when running nested under z/VM that would trigger a
> WARN_ON_ONCE. Remove the WARN_ON_ONCE. Long term we certainly want to make this
> code more robust and flexible, but just returning instead of WARNING makes
> guest bootable again.
> 
> ----------------------------------------------------------------
> Christian Borntraeger (1):
>       KVM: s390: Remove false WARN_ON_ONCE for the PQAP instruction
> 
>  arch/s390/kvm/priv.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

Pulled, thanks.

Paolo