[PATCH 0/2] s390/vfio-ccw: addressing fixes

[PATCH 0/2] s390/vfio-ccw: addressing fixes

[Eric Farman]

Hi everyone,

The attached is a couple of small fixes for the addresses
created/used by vfio-ccw that are shared with hardware,
which I'm sure to lose sooner or later. Hopefully they
could get picked up before that happens.

Alexander Gordeev (1):
  vfio-ccw: sort out physical vs virtual pointers usage

Eric Farman (1):
  vfio/ccw: identify CCW data addresses as physical

 drivers/s390/cio/vfio_ccw_cp.c  | 4 ++--
 drivers/s390/cio/vfio_ccw_fsm.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.34.1

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

[Eric Farman]

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);
 	if (!orb) {
 		ret = -EIO;
 		goto out;
-- 
2.34.1

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

[Matthew Rosato]

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.

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

Either way:

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

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

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

[Eric Farman]

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;
> 

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

[Nico Boehr]

Quoting Eric Farman (2022-11-09 21:21:56)
> 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.

Maybe I don't get it, but I think the commit description is inaccurate. The PoP
says (p. 15-25):

> Bits 0-31 of word 0 are
> preserved unmodified in the subchannel until
> replaced by a subsequent START SUBCHANNEL or
> MODIFY SUBCHANNEL instruction. These bits are
> placed in word 1 of the interruption code when an I/O
> interruption occurs and when an interruption request
> is cleared by the execution of TEST PENDING
> INTERRUPTION.

So the hardware actually doesn't care what kind of address this is. Rather, the
CIO driver expects the intparam to be a physical address - probably so it fits
32 bits -, see do_cio_interrupt.

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

[Eric Farman]

On Thu, 2022-11-10 at 10:24 +0100, Nico Boehr wrote:
> Quoting Eric Farman (2022-11-09 21:21:56)
> > 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.
> 
> Maybe I don't get it, but I think the commit description is
> inaccurate. The PoP
> says (p. 15-25):
> 
> > Bits 0-31 of word 0 are
> > preserved unmodified in the subchannel until
> > replaced by a subsequent START SUBCHANNEL or
> > MODIFY SUBCHANNEL instruction. These bits are
> > placed in word 1 of the interruption code when an I/O
> > interruption occurs and when an interruption request
> > is cleared by the execution of TEST PENDING
> > INTERRUPTION.
> 
> So the hardware actually doesn't care what kind of address this is.
> Rather, the
> CIO driver expects the intparam to be a physical address - probably
> so it fits
> 32 bits -, see do_cio_interrupt.

Right, it doesn't even need to be an address; we could write 0xdeadbeef
if we wanted, so long as that could be decoded by the driver on the
interrupt side. I really just wanted to point out that it was sent to
the channel, not that the channel (or anything else on the hardware
side) used it. What about this?

   The ORB's interrupt parameter field is stored unmodified into the
   interruption code when an I/O interrupt occurs. As this reflects
   a real device, let's store the physical address of the subchannel
   struct so it can be used when processing an interrupt.

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

[Nico Boehr]

Quoting Eric Farman (2022-11-10 15:28:51)
[...]
> > So the hardware actually doesn't care what kind of address this is.
> > Rather, the
> > CIO driver expects the intparam to be a physical address - probably
> > so it fits
> > 32 bits -, see do_cio_interrupt.
> 
> Right, it doesn't even need to be an address; we could write 0xdeadbeef
> if we wanted, so long as that could be decoded by the driver on the
> interrupt side. I really just wanted to point out that it was sent to
> the channel, not that the channel (or anything else on the hardware
> side) used it. What about this?
> 
>    The ORB's interrupt parameter field is stored unmodified into the
>    interruption code when an I/O interrupt occurs. As this reflects
>    a real device, let's store the physical address of the subchannel
>    struct so it can be used when processing an interrupt.

Sounds good to me.

With this fixed:
Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

[PATCH 2/2] vfio/ccw: identify CCW data addresses as physical

[Eric Farman]

The CCW data address created by vfio-ccw is that of an IDAL
built by this code. Since this address is used by real hardware,
it should be a physical address rather than a virtual one.
Let's clarify it as such in the ORB.

Similarly, once the I/O has completed the memory for that IDAL
needs to be released, so convert the CCW data address back to
a virtual address so that kfree() can process it.

Note: this currently doesn't fix a real bug, since virtual
addresses are identical to physical ones.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 7b02e97f4b29..c0a09fa8991a 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -394,7 +394,7 @@ static void ccwchain_cda_free(struct ccwchain *chain, int idx)
 	if (ccw_is_tic(ccw))
 		return;
 
-	kfree((void *)(u64)ccw->cda);
+	kfree(phys_to_virt(ccw->cda));
 }
 
 /**
@@ -845,7 +845,7 @@ union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm)
 
 	chain = list_first_entry(&cp->ccwchain_list, struct ccwchain, next);
 	cpa = chain->ch_ccw;
-	orb->cmd.cpa = (__u32) __pa(cpa);
+	orb->cmd.cpa = (__u32)virt_to_phys(cpa);
 
 	return orb;
 }
-- 
2.34.1

Re: [PATCH 2/2] vfio/ccw: identify CCW data addresses as physical

[Nico Boehr]

Quoting Eric Farman (2022-11-09 21:21:57)
> The CCW data address created by vfio-ccw is that of an IDAL
> built by this code. Since this address is used by real hardware,
> it should be a physical address rather than a virtual one.
> Let's clarify it as such in the ORB.
> 
> Similarly, once the I/O has completed the memory for that IDAL
> needs to be released, so convert the CCW data address back to
> a virtual address so that kfree() can process it.
> 
> Note: this currently doesn't fix a real bug, since virtual
> addresses are identical to physical ones.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>