| Message ID | 20221108152610.735205-1-nrb@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] s390/vfio-ap: GISA: sort out physical vs virtual pointers usage | expand |
On 11/8/22 16:26, Nico Boehr wrote: > Fix virtual vs physical address confusion (which currently are the same) > for the GISA when enabling the IRQ. > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> > --- > drivers/s390/crypto/vfio_ap_ops.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > index 0b4cc8c597ae..20859cabbced 100644 > --- a/drivers/s390/crypto/vfio_ap_ops.c > +++ b/drivers/s390/crypto/vfio_ap_ops.c > @@ -429,7 +429,7 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q, > > aqic_gisa.isc = nisc; > aqic_gisa.ir = 1; > - aqic_gisa.gisa = (uint64_t)gisa >> 4; > + aqic_gisa.gisa = (uint64_t)virt_to_phys(gisa) >> 4; I'd suggest doing s/uint64_t/u64/ or s/uint64_t/unsigned long/ but I'm wondering if (u32)(u64) would be more appropriate anyway. @halil @christian ? > > status = ap_aqic(q->apqn, aqic_gisa, h_nib); > switch (status.response_code) {
Quoting Janosch Frank (2022-11-15 09:56:52) > On 11/8/22 16:26, Nico Boehr wrote: > > Fix virtual vs physical address confusion (which currently are the same) > > for the GISA when enabling the IRQ. > > > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> > > --- > > drivers/s390/crypto/vfio_ap_ops.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > > index 0b4cc8c597ae..20859cabbced 100644 > > --- a/drivers/s390/crypto/vfio_ap_ops.c > > +++ b/drivers/s390/crypto/vfio_ap_ops.c > > @@ -429,7 +429,7 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q, > > > > aqic_gisa.isc = nisc; > > aqic_gisa.ir = 1; > > - aqic_gisa.gisa = (uint64_t)gisa >> 4; > > + aqic_gisa.gisa = (uint64_t)virt_to_phys(gisa) >> 4; > > I'd suggest doing s/uint64_t/u64/ or s/uint64_t/unsigned long/ but I'm > wondering if (u32)(u64) would be more appropriate anyway. The gisa origin is a unsigned int, hence you are right, uint64_t is odd. But since virt_to_phys() returns unsigned long, the cast to uint64_t is now useless. My suggestion is to remove the cast alltogether.
On Thu, 17 Nov 2022 09:50:14 +0100 Nico Boehr <nrb@linux.ibm.com> wrote: > Quoting Janosch Frank (2022-11-15 09:56:52) > > On 11/8/22 16:26, Nico Boehr wrote: > > > Fix virtual vs physical address confusion (which currently are the same) > > > for the GISA when enabling the IRQ. > > > > > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> > > > --- > > > drivers/s390/crypto/vfio_ap_ops.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > > > index 0b4cc8c597ae..20859cabbced 100644 > > > --- a/drivers/s390/crypto/vfio_ap_ops.c > > > +++ b/drivers/s390/crypto/vfio_ap_ops.c > > > @@ -429,7 +429,7 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q, > > > > > > aqic_gisa.isc = nisc; > > > aqic_gisa.ir = 1; > > > - aqic_gisa.gisa = (uint64_t)gisa >> 4; > > > + aqic_gisa.gisa = (uint64_t)virt_to_phys(gisa) >> 4; > > > > I'd suggest doing s/uint64_t/u64/ or s/uint64_t/unsigned long/ but I'm > > wondering if (u32)(u64) would be more appropriate anyway. > > The gisa origin is a unsigned int, hence you are right, uint64_t is odd. But since virt_to_phys() returns unsigned long, the cast to uint64_t is now useless. > > My suggestion is to remove the cast alltogether. I agree to remove it
On Thu, 17 Nov 2022 11:01:43 +0100 Claudio Imbrenda <imbrenda@linux.ibm.com> wrote: > On Thu, 17 Nov 2022 09:50:14 +0100 > Nico Boehr <nrb@linux.ibm.com> wrote: > > > Quoting Janosch Frank (2022-11-15 09:56:52) > > > On 11/8/22 16:26, Nico Boehr wrote: > > > > Fix virtual vs physical address confusion (which currently are the same) > > > > for the GISA when enabling the IRQ. > > > > > > > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> > > > > --- > > > > drivers/s390/crypto/vfio_ap_ops.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > > > > index 0b4cc8c597ae..20859cabbced 100644 > > > > --- a/drivers/s390/crypto/vfio_ap_ops.c > > > > +++ b/drivers/s390/crypto/vfio_ap_ops.c > > > > @@ -429,7 +429,7 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q, > > > > > > > > aqic_gisa.isc = nisc; > > > > aqic_gisa.ir = 1; > > > > - aqic_gisa.gisa = (uint64_t)gisa >> 4; > > > > + aqic_gisa.gisa = (uint64_t)virt_to_phys(gisa) >> 4; > > > > > > I'd suggest doing s/uint64_t/u64/ or s/uint64_t/unsigned long/ but I'm > > > wondering if (u32)(u64) would be more appropriate anyway. > > > > The gisa origin is a unsigned int, hence you are right, uint64_t is odd. The reason for the cast was that gisa is a pointer, but we needed to do integer arithmetic on the address of the object pointed to by the pointer. It happens so that the pointer must point to a piece of memory that is 31 bit addressable in host real address space, but for getting the address from a pointer, casting to the unsigned integral type with-wise corresponds to the pointer is IMHO sensible regardless of that information. >But since virt_to_phys() returns unsigned long, the cast to uint64_t is > now useless. > > > > My suggestion is to remove the cast alltogether. > > I agree to remove it Right: that cast makes no sense any more. And with that change: Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index 0b4cc8c597ae..20859cabbced 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -429,7 +429,7 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q, aqic_gisa.isc = nisc; aqic_gisa.ir = 1; - aqic_gisa.gisa = (uint64_t)gisa >> 4; + aqic_gisa.gisa = (uint64_t)virt_to_phys(gisa) >> 4; status = ap_aqic(q->apqn, aqic_gisa, h_nib); switch (status.response_code) {
Fix virtual vs physical address confusion (which currently are the same) for the GISA when enabling the IRQ. Signed-off-by: Nico Boehr <nrb@linux.ibm.com> --- drivers/s390/crypto/vfio_ap_ops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)