[PATCH v3 02/14] pc-bios/s390-ccw: Remove redundant vring schid attribute

jrossi@linux.ibm.com posted 14 patches 1 week, 5 days ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, Jared Rossi <jrossi@linux.ibm.com>, Zhuoying Cai <zycai@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, Matthew Rosato <mjrosato@linux.ibm.com>, Richard Henderson <richard.henderson@linaro.org>, Ilya Leoshkevich <iii@linux.ibm.com>, David Hildenbrand <david@kernel.org>, John Snow <jsnow@redhat.com>, Fabiano Rosas <farosas@suse.de>, Laurent Vivier <lvivier@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
[PATCH v3 02/14] pc-bios/s390-ccw: Remove redundant vring schid attribute
Posted by jrossi@linux.ibm.com 1 week, 5 days ago
From: Jared Rossi <jrossi@linux.ibm.com>

The schid is already stored as an attribute of the VDev itself and any other
instances are copies of this same value.  To avoid CCW specific attributes in
the VRing let's just access the existing VDev schid attribute as needed.

Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
 pc-bios/s390-ccw/virtio-blkdev.c | 2 +-
 pc-bios/s390-ccw/virtio-net.c    | 2 +-
 pc-bios/s390-ccw/virtio.c        | 9 ++++-----
 pc-bios/s390-ccw/virtio.h        | 3 +--
 4 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
index 4b819dd80f..019c2718b1 100644
--- a/pc-bios/s390-ccw/virtio-blkdev.c
+++ b/pc-bios/s390-ccw/virtio-blkdev.c
@@ -42,7 +42,7 @@ static int virtio_blk_read_many(VDev *vdev, unsigned long sector, void *load_add
     /* Now we can tell the host to read */
     vring_wait_reply();
 
-    if (drain_irqs(vr->schid)) {
+    if (drain_irqs()) {
         /* Well, whatever status is supposed to contain... */
         status = 1;
     }
diff --git a/pc-bios/s390-ccw/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
index 301445bf97..7eb0850069 100644
--- a/pc-bios/s390-ccw/virtio-net.c
+++ b/pc-bios/s390-ccw/virtio-net.c
@@ -88,7 +88,7 @@ int send(int fd, const void *buf, int len, int flags)
     while (!vr_poll(txvq)) {
         yield();
     }
-    if (drain_irqs(txvq->schid)) {
+    if (drain_irqs()) {
         puts("send: drain irqs failed");
         return -1;
     }
diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index cd6c99c7e3..f384a990dc 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -72,14 +72,14 @@ static long virtio_notify(SubChannelId schid, int vq_idx, long cookie)
  *             Virtio functions                *
  ***********************************************/
 
-int drain_irqs(SubChannelId schid)
+int drain_irqs(void)
 {
     Irb irb = {};
     int r = 0;
 
     while (1) {
         /* FIXME: make use of TPI, for that enable subchannel and isc */
-        if (tsch(schid, &irb)) {
+        if (tsch(vdev.schid, &irb)) {
             /* Might want to differentiate error codes later on. */
             if (irb.scsw.cstat) {
                 r = -EIO;
@@ -134,7 +134,7 @@ static void vring_init(VRing *vr, VqInfo *info)
 
 bool vring_notify(VRing *vr)
 {
-    vr->cookie = virtio_notify(vr->schid, vr->id, vr->cookie);
+    vr->cookie = virtio_notify(vdev.schid, vr->id, vr->cookie);
     return vr->cookie >= 0;
 }
 
@@ -211,7 +211,7 @@ int virtio_run(VDev *vdev, int vqid, VirtioCmd *cmd)
     } while (cmd[i++].flags & VRING_DESC_F_NEXT);
 
     vring_wait_reply();
-    if (drain_irqs(vr->schid)) {
+    if (drain_irqs()) {
         return -1;
     }
     return 0;
@@ -316,7 +316,6 @@ int virtio_setup_ccw(VDev *vdev)
         }
         info.num = config.num;
         vring_init(&vdev->vrings[i], &info);
-        vdev->vrings[i].schid = vdev->schid;
         if (run_ccw(vdev, CCW_CMD_SET_VQ, &info, sizeof(info), false)) {
             puts("Cannot set VQ info");
             return -EIO;
diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
index 597bd42358..06ba4e45ac 100644
--- a/pc-bios/s390-ccw/virtio.h
+++ b/pc-bios/s390-ccw/virtio.h
@@ -103,7 +103,6 @@ struct VRing {
     VRingDesc *desc;
     VRingAvail *avail;
     VRingUsed *used;
-    SubChannelId schid;
     long cookie;
     int id;
 };
@@ -269,7 +268,7 @@ struct VirtioCmd {
 typedef struct VirtioCmd VirtioCmd;
 
 bool vring_notify(VRing *vr);
-int drain_irqs(SubChannelId schid);
+int drain_irqs(void);
 void vring_send_buf(VRing *vr, void *p, int len, int flags);
 int vr_poll(VRing *vr);
 int vring_wait_reply(void);
-- 
2.52.0
Re: [PATCH v3 02/14] pc-bios/s390-ccw: Remove redundant vring schid attribute
Posted by Farhan Ali 1 week, 3 days ago
On 1/27/2026 8:15 AM, jrossi@linux.ibm.com wrote:
> From: Jared Rossi<jrossi@linux.ibm.com>
>
> The schid is already stored as an attribute of the VDev itself and any other
> instances are copies of this same value.  To avoid CCW specific attributes in
> the VRing let's just access the existing VDev schid attribute as needed.
>
> Signed-off-by: Jared Rossi<jrossi@linux.ibm.com>
> ---
>   pc-bios/s390-ccw/virtio-blkdev.c | 2 +-
>   pc-bios/s390-ccw/virtio-net.c    | 2 +-
>   pc-bios/s390-ccw/virtio.c        | 9 ++++-----
>   pc-bios/s390-ccw/virtio.h        | 3 +--
>   4 files changed, 7 insertions(+), 9 deletions(-)

Reviewed-by: Farhan Ali<alifm@linux.ibm.com>
Re: [PATCH v3 02/14] pc-bios/s390-ccw: Remove redundant vring schid attribute
Posted by Thomas Huth 1 week, 3 days ago
On 27/01/2026 17.15, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> The schid is already stored as an attribute of the VDev itself and any other
> instances are copies of this same value.  To avoid CCW specific attributes in
> the VRing let's just access the existing VDev schid attribute as needed.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
>   pc-bios/s390-ccw/virtio-blkdev.c | 2 +-
>   pc-bios/s390-ccw/virtio-net.c    | 2 +-
>   pc-bios/s390-ccw/virtio.c        | 9 ++++-----
>   pc-bios/s390-ccw/virtio.h        | 3 +--
>   4 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
> index 4b819dd80f..019c2718b1 100644
> --- a/pc-bios/s390-ccw/virtio-blkdev.c
> +++ b/pc-bios/s390-ccw/virtio-blkdev.c
> @@ -42,7 +42,7 @@ static int virtio_blk_read_many(VDev *vdev, unsigned long sector, void *load_add
>       /* Now we can tell the host to read */
>       vring_wait_reply();
>   
> -    if (drain_irqs(vr->schid)) {
> +    if (drain_irqs()) {
>           /* Well, whatever status is supposed to contain... */
>           status = 1;
>       }
Just a thought: Maybe we should rather get rid of the global access to the 
"vdev" in the long run (in case we ever want to deal with multiple devices 
at once), so it might be nicer to add a "VDev *vdev" parameter to the 
drain_irqs() function now instead of making its parameter list "void". 
Anyway, it's just a thought, we can also still tackle this later if needed, 
so I'm also fine with this patch as it:

Reviewed-by: Thomas Huth <thuth@redhat.com>
Re: [PATCH v3 02/14] pc-bios/s390-ccw: Remove redundant vring schid attribute
Posted by Eric Farman 1 week, 3 days ago
On Thu, 2026-01-29 at 11:31 +0100, Thomas Huth wrote:
> On 27/01/2026 17.15, jrossi@linux.ibm.com wrote:
> > From: Jared Rossi <jrossi@linux.ibm.com>
> > 
> > The schid is already stored as an attribute of the VDev itself and any other
> > instances are copies of this same value.  To avoid CCW specific attributes in
> > the VRing let's just access the existing VDev schid attribute as needed.
> > 
> > Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> > ---
> >   pc-bios/s390-ccw/virtio-blkdev.c | 2 +-
> >   pc-bios/s390-ccw/virtio-net.c    | 2 +-
> >   pc-bios/s390-ccw/virtio.c        | 9 ++++-----
> >   pc-bios/s390-ccw/virtio.h        | 3 +--
> >   4 files changed, 7 insertions(+), 9 deletions(-)
> > 
> > diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
> > index 4b819dd80f..019c2718b1 100644
> > --- a/pc-bios/s390-ccw/virtio-blkdev.c
> > +++ b/pc-bios/s390-ccw/virtio-blkdev.c
> > @@ -42,7 +42,7 @@ static int virtio_blk_read_many(VDev *vdev, unsigned long sector, void *load_add
> >       /* Now we can tell the host to read */
> >       vring_wait_reply();
> >   
> > -    if (drain_irqs(vr->schid)) {
> > +    if (drain_irqs()) {
> >           /* Well, whatever status is supposed to contain... */
> >           status = 1;
> >       }
> Just a thought: Maybe we should rather get rid of the global access to the 
> "vdev" in the long run (in case we ever want to deal with multiple devices 
> at once), so it might be nicer to add a "VDev *vdev" parameter to the 
> drain_irqs() function now instead of making its parameter list "void". 
> Anyway, it's just a thought, we can also still tackle this later if needed, 
> so I'm also fine with this patch as it:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

I agree. Removing that global access to the vdev would be a good goal, since it's sometimes passed
around today and sometimes not, and this series highlights that oddity. But, that doesn't need to be
addressed at this particular moment. So...

Reviewed-by: Eric Farman <farman@linux.ibm.com>