[PATCH v1 1/1] KVM: s390: pci: fix virtual-physical confusion on module unload/load

[PATCH v1 1/1] KVM: s390: pci: fix virtual-physical confusion on module unload/load

[Nico Boehr]

When the kvm module is unloaded, zpci_setup_aipb() perists some data in the
zpci_aipb structure in s390 pci code. Note that this struct is also passed
to firmware in the zpci_set_irq_ctrl() call and thus the GAIT must be a
physical address.

On module re-insertion, the GAIT is restored from this structure in
zpci_reset_aipb(). But it is a physical address, hence this may cause
issues when the kvm module is unloaded and loaded again.

Fix virtual vs physical address confusion (which currently are the same) by
adding the necessary physical-to-virtual-conversion in zpci_reset_aipb().

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 arch/s390/kvm/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
index ec51e810e381..9adb4a4b2bba 100644
--- a/arch/s390/kvm/pci.c
+++ b/arch/s390/kvm/pci.c
@@ -112,7 +112,7 @@ static int zpci_reset_aipb(u8 nisc)
 		return -EINVAL;
 
 	aift->sbv = zpci_aif_sbv;
-	aift->gait = (struct zpci_gaite *)zpci_aipb->aipb.gait;
+	aift->gait = phys_to_virt(zpci_aipb->aipb.gait);
 
 	return 0;
 }
-- 
2.39.1

[PATCH v1 0/1] KVM: s390: pci: fix virtual-physical confusion on module unload/load

[Nico Boehr]

When the kvm module is unloaded, zpci_setup_aipb() perists some data in the
zpci_aipb structure in s390 pci code. Note that this struct is also passed
to firmware in the zpci_set_irq_ctrl() call and thus the GAIT must be a
physical address.

On module re-insertion, the GAIT is restored from this structure in
zpci_reset_aipb(). But it is a physical address, hence this may cause
issues when the kvm module is unloaded and loaded again.

Fix virtual vs physical address confusion (which currently are the same) by
adding the necessary physical-to-virtual-conversion in zpci_reset_aipb().

Nico Boehr (1):
  KVM: s390: pci: fix virtual-physical confusion on module unload/load

 arch/s390/kvm/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.39.1

Re: [PATCH v1 1/1] KVM: s390: pci: fix virtual-physical confusion on module unload/load

[Matthew Rosato]

On 2/22/23 10:55 AM, Nico Boehr wrote:
> When the kvm module is unloaded, zpci_setup_aipb() perists some data in the
> zpci_aipb structure in s390 pci code. Note that this struct is also passed
> to firmware in the zpci_set_irq_ctrl() call and thus the GAIT must be a
> physical address.
> 
> On module re-insertion, the GAIT is restored from this structure in
> zpci_reset_aipb(). But it is a physical address, hence this may cause
> issues when the kvm module is unloaded and loaded again.
> 
> Fix virtual vs physical address confusion (which currently are the same) by
> adding the necessary physical-to-virtual-conversion in zpci_reset_aipb().
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>

Yeah, that's right, in fact there is another address also stashed in the zpci_aipb which is also saved as physical addresses since, as you say, this structure is sent to firmware; the GAIT address just happens to be the one we care about at this spot, so I think it makes sense to leave zpci_aipb alone and just convert back to virt in this one place its needed.

Since we're looking at this bit of code, it's also worth noting that the other address restored here (aift->sbv) comes from zpci_aif_sbv which was instead stashed as a virtual address to begin with and that's why it doesn't need similar treatment.

Thanks Nico!

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

Re: [PATCH v1 1/1] KVM: s390: pci: fix virtual-physical confusion on module unload/load

[Alexander Gordeev]

On Wed, Feb 22, 2023 at 04:55:02PM +0100, Nico Boehr wrote:
> @@ -112,7 +112,7 @@ static int zpci_reset_aipb(u8 nisc)
>  		return -EINVAL;
>  
>  	aift->sbv = zpci_aif_sbv;
> -	aift->gait = (struct zpci_gaite *)zpci_aipb->aipb.gait;
> +	aift->gait = phys_to_virt(zpci_aipb->aipb.gait);
>  
>  	return 0;
>  }

With this change aift->gait would never be NULL. Does it work with line 125?

120 int kvm_s390_pci_aen_init(u8 nisc)
121 {
122         int rc = 0;
123 
124         /* If already enabled for AEN, bail out now */
125         if (aift->gait || aift->sbv)
126                 return -EPERM;

Re: [PATCH v1 1/1] KVM: s390: pci: fix virtual-physical confusion on module unload/load

[Matthew Rosato]

On 2/22/23 11:42 AM, Alexander Gordeev wrote:
> On Wed, Feb 22, 2023 at 04:55:02PM +0100, Nico Boehr wrote:
>> @@ -112,7 +112,7 @@ static int zpci_reset_aipb(u8 nisc)
>>  		return -EINVAL;
>>  
>>  	aift->sbv = zpci_aif_sbv;
>> -	aift->gait = (struct zpci_gaite *)zpci_aipb->aipb.gait;
>> +	aift->gait = phys_to_virt(zpci_aipb->aipb.gait);
>>  
>>  	return 0;
>>  }
> 
> With this change aift->gait would never be NULL. Does it work with line 125?

aift->gait will get set to NULL when kvm_s390_pci_aen_exit is called, which is called when the kvm module is unloaded.

Then kvm_s390_pci_aen_init is called again when kvm module is (re)loaded and is expected to set aift->gait, either for the first time or reset the values using what was stashed (or return on error).  kvm_s390_pci_aen_init should not be called more than once for the life of the kvm module, hence the check for aift->gait.

> 
> 120 int kvm_s390_pci_aen_init(u8 nisc)
> 121 {
> 122         int rc = 0;
> 123 
> 124         /* If already enabled for AEN, bail out now */
> 125         if (aift->gait || aift->sbv)
> 126                 return -EPERM;
>