* [PATCH v1] s390/vfio-ap: GISA: sort out physical vs virtual pointers usage
@ 2022-11-08 15:26 Nico Boehr
2022-11-15 8:56 ` Janosch Frank
0 siblings, 1 reply; 5+ messages in thread
From: Nico Boehr @ 2022-11-08 15:26 UTC (permalink / raw)
To: pasic, akrowiak, jjherne; +Cc: linux-s390, kvm, borntraeger, frankja, imbrenda
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;
status = ap_aqic(q->apqn, aqic_gisa, h_nib);
switch (status.response_code) {
--
2.37.3
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v1] s390/vfio-ap: GISA: sort out physical vs virtual pointers usage
2022-11-08 15:26 [PATCH v1] s390/vfio-ap: GISA: sort out physical vs virtual pointers usage Nico Boehr
@ 2022-11-15 8:56 ` Janosch Frank
2022-11-17 8:50 ` Nico Boehr
0 siblings, 1 reply; 5+ messages in thread
From: Janosch Frank @ 2022-11-15 8:56 UTC (permalink / raw)
To: Nico Boehr, pasic, akrowiak, jjherne
Cc: linux-s390, kvm, borntraeger, imbrenda
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) {
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v1] s390/vfio-ap: GISA: sort out physical vs virtual pointers usage
2022-11-15 8:56 ` Janosch Frank
@ 2022-11-17 8:50 ` Nico Boehr
2022-11-17 10:01 ` Claudio Imbrenda
0 siblings, 1 reply; 5+ messages in thread
From: Nico Boehr @ 2022-11-17 8:50 UTC (permalink / raw)
To: Janosch Frank, akrowiak, jjherne, pasic
Cc: linux-s390, kvm, borntraeger, imbrenda
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] s390/vfio-ap: GISA: sort out physical vs virtual pointers usage
2022-11-17 8:50 ` Nico Boehr
@ 2022-11-17 10:01 ` Claudio Imbrenda
2022-11-17 17:55 ` Halil Pasic
0 siblings, 1 reply; 5+ messages in thread
From: Claudio Imbrenda @ 2022-11-17 10:01 UTC (permalink / raw)
To: Nico Boehr
Cc: Janosch Frank, akrowiak, jjherne, pasic, linux-s390, kvm,
borntraeger
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] s390/vfio-ap: GISA: sort out physical vs virtual pointers usage
2022-11-17 10:01 ` Claudio Imbrenda
@ 2022-11-17 17:55 ` Halil Pasic
0 siblings, 0 replies; 5+ messages in thread
From: Halil Pasic @ 2022-11-17 17:55 UTC (permalink / raw)
To: Claudio Imbrenda
Cc: Nico Boehr, Janosch Frank, akrowiak, jjherne, linux-s390, kvm,
borntraeger, Halil Pasic
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-11-17 17:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-08 15:26 [PATCH v1] s390/vfio-ap: GISA: sort out physical vs virtual pointers usage Nico Boehr
2022-11-15 8:56 ` Janosch Frank
2022-11-17 8:50 ` Nico Boehr
2022-11-17 10:01 ` Claudio Imbrenda
2022-11-17 17:55 ` Halil Pasic
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox