Re: [PATCH v1] s390/vfio-ap: GISA: sort out physical vs virtual pointers usage

[Halil Pasic <pasic@linux.ibm.com>]

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>