[v1] s390x/pci: fix ISM reset

[PATCH 2/3] s390x/pci: refresh fh before disabling aif

Posted by Matthew Rosato 10 months, 2 weeks ago

Typically we refresh the host fh during CLP enable, however it's possible
that the device goes through multiple reset events before the guest
performs another CLP enable.  Let's handle this for now by refreshing the
host handle from vfio before disabling aif.

Fixes: 03451953c7 ("s390x/pci: reset ISM passthrough devices on shutdown and system reset")
Reported-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 hw/s390x/s390-pci-kvm.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/s390-pci-kvm.c b/hw/s390x/s390-pci-kvm.c
index f7e10cfa72..9eef4fc3ec 100644
--- a/hw/s390x/s390-pci-kvm.c
+++ b/hw/s390x/s390-pci-kvm.c
@@ -18,6 +18,7 @@
 #include "hw/s390x/s390-pci-bus.h"
 #include "hw/s390x/s390-pci-kvm.h"
 #include "hw/s390x/s390-pci-inst.h"
+#include "hw/s390x/s390-pci-vfio.h"
 #include "cpu_models.h"
 
 bool s390_pci_kvm_interp_allowed(void)
@@ -64,9 +65,17 @@ int s390_pci_kvm_aif_disable(S390PCIBusDevice *pbdev)
         return -EINVAL;
     }
 
+    /*
+     * The device may have already been reset but we still want to relinquish
+     * the guest ISC, so always be sure to use an up-to-date host fh.
+     */
+    if (!s390_pci_get_host_fh(pbdev, &args.fh)) {
+        return -EPERM;
+    }
+
     rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
     if (rc == 0) {
-        pbev->aif = false;
+        pbdev->aif = false;
     }
 
     return rc;
-- 
2.43.0

Re: [PATCH 2/3] s390x/pci: refresh fh before disabling aif

Posted by Cédric Le Goater 10 months, 2 weeks ago

On 1/16/24 23:31, Matthew Rosato wrote:
> Typically we refresh the host fh during CLP enable, however it's possible
> that the device goes through multiple reset events before the guest
> performs another CLP enable.  Let's handle this for now by refreshing the
> host handle from vfio before disabling aif.
> 
> Fixes: 03451953c7 ("s390x/pci: reset ISM passthrough devices on shutdown and system reset")
> Reported-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>   hw/s390x/s390-pci-kvm.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-pci-kvm.c b/hw/s390x/s390-pci-kvm.c
> index f7e10cfa72..9eef4fc3ec 100644
> --- a/hw/s390x/s390-pci-kvm.c
> +++ b/hw/s390x/s390-pci-kvm.c
> @@ -18,6 +18,7 @@
>   #include "hw/s390x/s390-pci-bus.h"
>   #include "hw/s390x/s390-pci-kvm.h"
>   #include "hw/s390x/s390-pci-inst.h"
> +#include "hw/s390x/s390-pci-vfio.h"
>   #include "cpu_models.h"
>   
>   bool s390_pci_kvm_interp_allowed(void)
> @@ -64,9 +65,17 @@ int s390_pci_kvm_aif_disable(S390PCIBusDevice *pbdev)
>           return -EINVAL;
>       }
>   
> +    /*
> +     * The device may have already been reset but we still want to relinquish
> +     * the guest ISC, so always be sure to use an up-to-date host fh.
> +     */
> +    if (!s390_pci_get_host_fh(pbdev, &args.fh)) {
> +        return -EPERM;
> +    }

The callers of s390_pci_kvm_aif_disable() all test the original host
function with :

    if (pbdev->interp && (pbdev->fh & FH_MASK_ENABLE))

This change possibly fetches a new one. Shouldn't we move the test
also in s390_pci_kvm_aif_disable() ?


Thanks,

C.



> +
>       rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
>       if (rc == 0) {
> -        pbev->aif = false;
> +        pbdev->aif = false;
>       }
>   
>       return rc;

Re: [PATCH 2/3] s390x/pci: refresh fh before disabling aif

Posted by Matthew Rosato 10 months, 2 weeks ago

On 1/17/24 5:40 AM, Cédric Le Goater wrote:
> On 1/16/24 23:31, Matthew Rosato wrote:
>> Typically we refresh the host fh during CLP enable, however it's possible
>> that the device goes through multiple reset events before the guest
>> performs another CLP enable.  Let's handle this for now by refreshing the
>> host handle from vfio before disabling aif.
>>
>> Fixes: 03451953c7 ("s390x/pci: reset ISM passthrough devices on shutdown and system reset")
>> Reported-by: Cédric Le Goater <clg@redhat.com>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>> ---
>>   hw/s390x/s390-pci-kvm.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/s390-pci-kvm.c b/hw/s390x/s390-pci-kvm.c
>> index f7e10cfa72..9eef4fc3ec 100644
>> --- a/hw/s390x/s390-pci-kvm.c
>> +++ b/hw/s390x/s390-pci-kvm.c
>> @@ -18,6 +18,7 @@
>>   #include "hw/s390x/s390-pci-bus.h"
>>   #include "hw/s390x/s390-pci-kvm.h"
>>   #include "hw/s390x/s390-pci-inst.h"
>> +#include "hw/s390x/s390-pci-vfio.h"
>>   #include "cpu_models.h"
>>     bool s390_pci_kvm_interp_allowed(void)
>> @@ -64,9 +65,17 @@ int s390_pci_kvm_aif_disable(S390PCIBusDevice *pbdev)
>>           return -EINVAL;
>>       }
>>   +    /*
>> +     * The device may have already been reset but we still want to relinquish
>> +     * the guest ISC, so always be sure to use an up-to-date host fh.
>> +     */
>> +    if (!s390_pci_get_host_fh(pbdev, &args.fh)) {
>> +        return -EPERM;
>> +    }
> 
> The callers of s390_pci_kvm_aif_disable() all test the original host
> function with :
> 
>    if (pbdev->interp && (pbdev->fh & FH_MASK_ENABLE))
> 
> This change possibly fetches a new one. Shouldn't we move the test
> also in s390_pci_kvm_aif_disable() ?
> 
> 

Those codepaths are actually testing the enablement bit of the guest-visible function handle, not the host function handle.  Basically asking "was the guest using this device".  These handles (host and guest) are sync'd during guest CLP enable (necessary for interpretation to work) but will become disjoint once we reset the device until the next guest CLP ENABLE.  They can also become disjoint once the guest does a CLP DISABLE.
What this change is doing is basically making sure we disable AIF on the hostdev, but does NOT replace pbdev->fh.  The guest-visible function handle will get sync'd again when the guest does a new CLP ENABLE after reset.

Re: [PATCH 2/3] s390x/pci: refresh fh before disabling aif

Posted by Cédric Le Goater 10 months, 2 weeks ago

Hello Matthew,

On 1/16/24 23:31, Matthew Rosato wrote:
> Typically we refresh the host fh during CLP enable, however it's possible
> that the device goes through multiple reset events before the guest
> performs another CLP enable.  Let's handle this for now by refreshing the
> host handle from vfio before disabling aif.
> 
> Fixes: 03451953c7 ("s390x/pci: reset ISM passthrough devices on shutdown and system reset")
> Reported-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>   hw/s390x/s390-pci-kvm.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-pci-kvm.c b/hw/s390x/s390-pci-kvm.c
> index f7e10cfa72..9eef4fc3ec 100644
> --- a/hw/s390x/s390-pci-kvm.c
> +++ b/hw/s390x/s390-pci-kvm.c
> @@ -18,6 +18,7 @@
>   #include "hw/s390x/s390-pci-bus.h"
>   #include "hw/s390x/s390-pci-kvm.h"
>   #include "hw/s390x/s390-pci-inst.h"
> +#include "hw/s390x/s390-pci-vfio.h"
>   #include "cpu_models.h"
>   
>   bool s390_pci_kvm_interp_allowed(void)
> @@ -64,9 +65,17 @@ int s390_pci_kvm_aif_disable(S390PCIBusDevice *pbdev)
>           return -EINVAL;
>       }
>   
> +    /*
> +     * The device may have already been reset but we still want to relinquish
> +     * the guest ISC, so always be sure to use an up-to-date host fh.
> +     */
> +    if (!s390_pci_get_host_fh(pbdev, &args.fh)) {
> +        return -EPERM;
> +    }
> +
>       rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
>       if (rc == 0) {
> -        pbev->aif = false;
> +        pbdev->aif = false;
>       }

This belongs to patch 1.


Thanks,

C.


>   
>       return rc;

Re: [PATCH 2/3] s390x/pci: refresh fh before disabling aif

Posted by Matthew Rosato 10 months, 2 weeks ago

On 1/17/24 5:31 AM, Cédric Le Goater wrote:
> Hello Matthew,
> 
> On 1/16/24 23:31, Matthew Rosato wrote:
>> Typically we refresh the host fh during CLP enable, however it's possible
>> that the device goes through multiple reset events before the guest
>> performs another CLP enable.  Let's handle this for now by refreshing the
>> host handle from vfio before disabling aif.
>>
>> Fixes: 03451953c7 ("s390x/pci: reset ISM passthrough devices on shutdown and system reset")
>> Reported-by: Cédric Le Goater <clg@redhat.com>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>> ---
>>   hw/s390x/s390-pci-kvm.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/s390-pci-kvm.c b/hw/s390x/s390-pci-kvm.c
>> index f7e10cfa72..9eef4fc3ec 100644
>> --- a/hw/s390x/s390-pci-kvm.c
>> +++ b/hw/s390x/s390-pci-kvm.c
>> @@ -18,6 +18,7 @@
>>   #include "hw/s390x/s390-pci-bus.h"
>>   #include "hw/s390x/s390-pci-kvm.h"
>>   #include "hw/s390x/s390-pci-inst.h"
>> +#include "hw/s390x/s390-pci-vfio.h"
>>   #include "cpu_models.h"
>>     bool s390_pci_kvm_interp_allowed(void)
>> @@ -64,9 +65,17 @@ int s390_pci_kvm_aif_disable(S390PCIBusDevice *pbdev)
>>           return -EINVAL;
>>       }
>>   +    /*
>> +     * The device may have already been reset but we still want to relinquish
>> +     * the guest ISC, so always be sure to use an up-to-date host fh.
>> +     */
>> +    if (!s390_pci_get_host_fh(pbdev, &args.fh)) {
>> +        return -EPERM;
>> +    }
>> +
>>       rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
>>       if (rc == 0) {
>> -        pbev->aif = false;
>> +        pbdev->aif = false;
>>       }
> 
> This belongs to patch 1.
> 
> 

Thanks for pointing that out, will fix.

Re: [PATCH 2/3] s390x/pci: refresh fh before disabling aif

Posted by Eric Farman 10 months, 2 weeks ago

On Tue, 2024-01-16 at 17:31 -0500, Matthew Rosato wrote:
> Typically we refresh the host fh during CLP enable, however it's
> possible
> that the device goes through multiple reset events before the guest
> performs another CLP enable.  Let's handle this for now by refreshing
> the
> host handle from vfio before disabling aif.
> 
> Fixes: 03451953c7 ("s390x/pci: reset ISM passthrough devices on
> shutdown and system reset")
> Reported-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>  hw/s390x/s390-pci-kvm.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)

Reviewed-by: Eric Farman <farman@linux.ibm.com>