| Message ID | 20230222155503.43399-1-nrb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: pci: fix virtual-physical confusion on module unload/load | expand |
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>
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;
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; >
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; }
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(-)