Re: [PATCH 1/2] vfio-ccw: sort out physical vs virtual pointers usage

[Eric Farman <farman@linux.ibm.com>]

On Wed, 2022-11-09 at 17:20 -0500, Matthew Rosato wrote:
> On 11/9/22 3:21 PM, Eric Farman wrote:
> > From: Alexander Gordeev <agordeev@linux.ibm.com>
> > 
> > The ORB is a construct that is sent to the real hardware,
> > so should contain a physical address in its interrupt
> > parameter field. Let's clarify that.
> > 
> > Note: this currently doesn't fix a real bug, since virtual
> > addresses are identical to physical ones.
> > 
> > Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> > [EF: Updated commit message]
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >  drivers/s390/cio/vfio_ccw_fsm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_fsm.c
> > b/drivers/s390/cio/vfio_ccw_fsm.c
> > index a59c758869f8..0a5e8b4a6743 100644
> > --- a/drivers/s390/cio/vfio_ccw_fsm.c
> > +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> > @@ -29,7 +29,7 @@ static int fsm_io_helper(struct vfio_ccw_private
> > *private)
> >  
> >         spin_lock_irqsave(sch->lock, flags);
> >  
> > -       orb = cp_get_orb(&private->cp, (u32)(addr_t)sch, sch->lpm);
> > +       orb = cp_get_orb(&private->cp, (u32)virt_to_phys(sch), sch-
> > >lpm);
> 
> Nit: I think it would make more sense to do the virt_to_phys inside
> cp_get_orb at the time we place the address in the orb (since that's
> what gets sent to hardware), rather than requiring all callers of
> cp_get_orb to pass a physical address.  I realize there is only 1
> caller today.

Eh, maybe so. But that takes me into the 'what are we passing to this
routine and how can we simplify it' rabbit hole, and it stops becoming
a nit pretty quickly. I'd rather keep this patch as the simple change
described here. I have some more involved rework in the broader cp code
in the pipe, and can include your suggestion with that.

> 
> Nit++: Can we make the patch subjects match?  vfio/ccw or vfio-ccw

Heh, fair. "vfio/ccw" has been the style du jour of late.

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

Thanks!

> 
> >         if (!orb) {
> >                 ret = -EIO;
> >                 goto out;
>