Re: [PATCH 2/5] pc-bios/s390-ccw: Add per-queue notification offset for multi-queue virtio configurations

[Jared Rossi <jrossi@linux.ibm.com>]

On 5/6/26 2:20 PM, Zhuoying Cai wrote:
> On 5/4/26 6:16 PM, jrossi@linux.ibm.com wrote:
>> From: Jared Rossi <jrossi@linux.ibm.com>
>>
>> The initial support for virtio-blk-pci IPL devices used a single virt-queue, but
>> other device types require multiple queues, and for PCI device types this also
>> requires a per-queue notification offset.
>>
>> Add a PCI notify field to the VRing struct so that each queue has a unique
>> notify offset.  Also re-select the target queue before writing buffers to
>> ensure the proper queue is active.
>>
>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>> ---
>>   pc-bios/s390-ccw/virtio-pci.c | 36 ++++++++++++++++++++++-------------
>>   pc-bios/s390-ccw/virtio-pci.h |  3 ++-
>>   pc-bios/s390-ccw/virtio.c     |  4 +++-
>>   pc-bios/s390-ccw/virtio.h     |  1 +
>>   4 files changed, 29 insertions(+), 15 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/virtio-pci.c b/pc-bios/s390-ccw/virtio-pci.c
>> index 53bdb52e76..736869f4f5 100644
>> --- a/pc-bios/s390-ccw/virtio-pci.c
>> +++ b/pc-bios/s390-ccw/virtio-pci.c
>> @@ -74,10 +74,10 @@ int virtio_pci_reset(VDev *vdev)
>>       return 0;
>>   }
>>   
>> -long virtio_pci_notify(int vq_id)
>> +long virtio_pci_notify(VRing *vr)
>>   {
>> -    uint32_t offset = n_cap.off + notify_mult * q_notify_offset;
>> -    return vpci_bswap16_write(offset, n_cap.bar, (uint16_t) vq_id);
>> +    uint32_t offset = n_cap.off + notify_mult * vr->pci_notify;
>> +    return vpci_bswap16_write(offset, n_cap.bar, (uint16_t) vr->id);
>>   }
>>   
>>   /*
>> @@ -166,7 +166,7 @@ int vpci_read_flex(uint64_t offset, uint8_t pcias, void *buf, int len)
>>       return 0;
>>   }
>>   
>> -static int vpci_set_selected_vq(uint16_t queue_num)
>> +int vpci_set_selected_vq(uint16_t queue_num)
>>   {
>>       return vpci_bswap16_write(c_cap.off + VPCI_C_OFFSET_Q_SELECT, c_cap.bar, queue_num);
>>   }
>> @@ -332,7 +332,6 @@ int virtio_pci_setup(VDev *vdev)
>>       VRing *vr;
>>       int rc;
>>       uint8_t status;
>> -    uint16_t vq_size;
>>       int i = 0;
>>   
>>       vdev->guessed_disk_nature = VIRTIO_GDN_NONE;
>> @@ -380,28 +379,39 @@ int virtio_pci_setup(VDev *vdev)
>>           return -EIO;
>>       }
>>   
>> -    if (vpci_read_bswap16(VPCI_C_OFFSET_Q_SIZE, c_cap.bar, &vq_size)) {
>> -        puts("Failed to read virt-queue configuration");
>> -        return -EIO;
>> -    }
>> -
>>       /* Configure virt-queues for pci */
>>       for (i = 0; i < vdev->nr_vqs; i++) {
>> +        uint16_t vq_size;
>> +        uint16_t vq_notify;
>>           VqInfo info = {
>>               .queue = (unsigned long long) virtio_get_ring_area(i),
>>               .align = KVM_S390_VIRTIO_RING_ALIGN,
>>               .index = i,
>> -            .num = vq_size,
>> +            .num = 0,
>>           };
>>   
>>           vr = &vdev->vrings[i];
>> -        vring_init(vr, &info);
>>   
>> -        if (vpci_set_selected_vq(vr->id)) {
>> +        if (vpci_set_selected_vq(i)) {
>>               puts("Failed to set selected virt-queue");
>>               return -EIO;
>>           }
>>   
>> +        if (vpci_read_bswap16(VPCI_C_OFFSET_Q_SIZE, c_cap.bar, &vq_size)) {
> Small nit: c_cap.off + VPCI_C_OFFSET_Q_SIZE
Ah, this is actually not just a small nit!  Although the common capabilities
offset is 0 in the current QEMU implementation there is nothing 
preventing it
from being a different offset in the future.  So it must be included, 
otherwise
a non-zero value would break it.  Good catch.

>
>> +            puts("Failed to read virt-queue configuration");
>> +            return -EIO;
>> +        }
>> +
>> +        info.num = vq_size;
>> +
>> +        if (vpci_read_bswap16(c_cap.off + VPCI_C_OFFSET_Q_NOFF, c_cap.bar, &vq_notify)) {
>> +            puts("Failed to read virt-queue configuration");
>> +            return -EIO;
>> +        }
> Since the notification offset is now read per queue, should we remove
> the same read in virtio_pci_read_pci_cap_config() as well as the global
> q_notify_offset variable?
>
> if (rc || vpci_read_bswap16(c_cap.off + VPCI_C_OFFSET_Q_NOFF, c_cap.bar,
> &q_notify_offset))
Yes, that should be removed.  I will take care of it for V2. Thanks.

>
>> +
>> +        vr->pci_notify = vq_notify;
>> +        vring_init(vr, &info);
>> +
>>           rc = set_pci_vq_addr(VPCI_C_OFFSET_Q_DESCLO, vr->desc);
>>           rc |= set_pci_vq_addr(VPCI_C_OFFSET_Q_AVAILLO, vr->avail);
>>           rc |= set_pci_vq_addr(VPCI_C_OFFSET_Q_USEDLO, vr->used);
>> diff --git a/pc-bios/s390-ccw/virtio-pci.h b/pc-bios/s390-ccw/virtio-pci.h
>> index 90d07cb9a7..993fc285ac 100644
>> --- a/pc-bios/s390-ccw/virtio-pci.h
>> +++ b/pc-bios/s390-ccw/virtio-pci.h
>> @@ -62,9 +62,10 @@ struct VirtioPciCap {
>>   };
>>   typedef struct VirtioPciCap  VirtioPciCap;
>>   
>> +int vpci_set_selected_vq(uint16_t queue_num);
>>   void virtio_pci_id2type(VDev *vdev, uint16_t device_id);
>>   int virtio_pci_reset(VDev *vdev);
>> -long virtio_pci_notify(int vq_id);
>> +long virtio_pci_notify(VRing *vr);
>>   int virtio_pci_setup(VDev *vdev);
>>   int virtio_pci_setup_device(void);
>>   
>> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
>> index 30e6b2bc16..00850acc2f 100644
>> --- a/pc-bios/s390-ccw/virtio.c
>> +++ b/pc-bios/s390-ccw/virtio.c
>> @@ -114,7 +114,8 @@ bool vring_notify(VRing *vr)
>>           vr->cookie = virtio_ccw_notify(vdev.schid, vr->id, vr->cookie);
>>           break;
>>       case S390_IPL_TYPE_PCI:
>> -        vr->cookie = virtio_pci_notify(vr->id);
>> +        vr->cookie = virtio_pci_notify(vr);
>> +        break;
>>       default:
>>           return 1;
>>       }
>> @@ -154,6 +155,7 @@ static void vr_bswap_descriptor(VRingDesc *desc)
>>   void vring_send_buf(VRing *vr, void *p, int len, int flags)
>>   {
>>       if (!be_ipl()) {
>> +        vpci_set_selected_vq(vr->id);
>>           vr->avail->idx = bswap16(vr->avail->idx);
>>       }
>>   
>> diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
>> index d32a4830ca..75ae5bdbc2 100644
>> --- a/pc-bios/s390-ccw/virtio.h
>> +++ b/pc-bios/s390-ccw/virtio.h
>> @@ -107,6 +107,7 @@ struct VRing {
>>       VRingUsed *used;
>>       long cookie;
>>       int id;
>> +    uint16_t pci_notify;
>>   };
>>   typedef struct VRing VRing;
>>   

Regards,
Jared Rossi