Re: [PATCH 5/7] pc-bios/s390-ccw: Add support for virtio-blk-pci IPL

[Thomas Huth <thuth@redhat.com>]

On 20/10/2025 18.20, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> Enable virt-queue PCI configuration and add routines for virtio-blk-pci devices.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
...
> diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
> index 1c1017a0db..4e4a7280b6 100644
> --- a/pc-bios/s390-ccw/virtio.h
> +++ b/pc-bios/s390-ccw/virtio.h
> @@ -259,6 +259,7 @@ struct VDev {
>       uint8_t scsi_dev_heads;
>       bool scsi_device_selected;
>       ScsiDevice selected_scsi_device;
> +    uint32_t pci_fh;
>       uint32_t max_transfer;
>       uint32_t guest_features[2];
>   };
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 7299b8911f..69e7d39862 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -15,8 +15,10 @@
>   #include "s390-arch.h"
>   #include "s390-ccw.h"
>   #include "cio.h"
> +#include "clp.h"
>   #include "virtio.h"
>   #include "virtio-scsi.h"
> +#include "virtio-pci.h"
>   #include "dasd-ipl.h"
>   
>   SubChannelId blk_schid = { .one = 1 };
> @@ -150,6 +152,20 @@ static bool find_subch(int dev_no)
>       return false;
>   }
>   
> +static bool find_fid(uint32_t fid) {

Put the curly bracket into the next line, please.

> +    ClpFhListEntry entry;
> +    VDev *vdev = virtio_get_device();
> +
> +    if (find_pci_function(fid, &entry)) {
> +        return false;
> +    }
> +
> +    vdev->pci_fh = entry.fh;
> +    virtio_pci_id2type(vdev, entry.device_id);
> +
> +    return (vdev->type != 0);

You could drop the braces here.

> +}
> +
...
> diff --git a/pc-bios/s390-ccw/virtio-pci.c b/pc-bios/s390-ccw/virtio-pci.c
> new file mode 100644
> index 0000000000..b6cb4a661a
> --- /dev/null
> +++ b/pc-bios/s390-ccw/virtio-pci.c
> @@ -0,0 +1,357 @@
> +/*
> + * Functionality for virtio-pci
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Jared Rossi <jrossi@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "clp.h"
> +#include "pci.h"
> +#include "helper.h"
> +#include "s390-ccw.h"
> +#include "virtio.h"
> +#include "bswap.h"
> +#include "virtio-pci.h"
> +#include "s390-time.h"
> +#include <stdio.h>
> +
> +/* Variable offsets used for reads/writes to modern memory region BAR 4 */
> +uint32_t common_offset;
> +uint32_t device_offset;
> +uint32_t notify_offset;
> +uint32_t notify_mult;
> +uint16_t q_notify_offset;
> +
> +static int virtio_pci_set_status(VDev *vdev, uint8_t status)
> +{
> +    uint64_t status64 = status;
> +
> +    return pci_write(vdev->pci_fh, VPCI_C_OFFSET_STATUS, status64, 1);
> +}
> +
> +static int virtio_pci_get_status(VDev *vdev, uint8_t *status)
> +{
> +    uint64_t status64;
> +    int rc;
> +
> +    rc = pci_read(vdev->pci_fh, VPCI_C_OFFSET_STATUS, 4, &status64, 1);
> +    if (rc) {
> +        puts("Failed to read virtio-pci status");
> +        return rc;
> +    }
> +
> +    *status = (uint8_t) status64;

Ok, so after reading this code, I realized that pci_read is definitely only 
supposed to work on 64-bit values.

Could you please change the "buf" parameter of pci_read() from "void *" to 
"uint64_t *", otherwise this is super-confusing.

> +    return 0;
> +}
> +
> +static int virtio_pci_get_hfeatures(VDev *vdev, uint64_t *features)
> +{
> +    uint64_t feat0, feat1;
> +    uint32_t selector;
> +    int rc;
> +
> +    selector = bswap32(0);
> +    rc = pci_write(vdev->pci_fh, VPCI_C_OFFSET_DFSELECT, selector, 4);
> +    rc |= pci_read(vdev->pci_fh, VPCI_C_OFFSET_DF, 4, &feat0, 4);
> +    feat0 = bswap32(feat0);
 > +> +    selector = bswap32(1);
> +    rc |= pci_write(vdev->pci_fh, VPCI_C_OFFSET_DFSELECT, selector, 4);
> +    rc |= pci_read(vdev->pci_fh, VPCI_C_OFFSET_DF, 4, &feat1, 4);
> +    feat1 = bswap32(feat1);
> +> +    *features = feat1 << 32;
> +    *features |= feat0;
> +
> +    return rc;
> +}
> +
> +static int virtio_pci_set_gfeatures(VDev *vdev)
> +{
> +    uint64_t feats;
> +    uint32_t selector;
> +    int rc;
> +
> +    selector = bswap32(0);
> +    rc = pci_write(vdev->pci_fh, VPCI_C_OFFSET_GFSELECT, selector, 4);
> +
> +    feats = bswap32((uint64_t)vdev->guest_features[1]);

The cast to 64-bit does not make sense here.

> +    rc |= pci_write(vdev->pci_fh, VPCI_C_OFFSET_GF, feats, 4);
> +
> +    selector = bswap32(1);
> +    rc |= pci_write(vdev->pci_fh, VPCI_C_OFFSET_GFSELECT, selector, 4);
> +
> +    feats = bswap32((uint64_t)vdev->guest_features[0]);

dito

> +    rc |= pci_write(vdev->pci_fh, VPCI_C_OFFSET_GF, feats, 4);
> +
> +    return rc;
> +}
> +
> +static int virtio_pci_get_blk_config(VDev *vdev)
> +{
> +    return pci_read(vdev->pci_fh, device_offset, 4, (uint64_t *)&vdev->config.blk,
> +                    sizeof(VirtioBlkConfig));
> +

Please remove the empty line.

> +}
> +
> +int virtio_pci_set_selected_vq(VDev *vdev, uint16_t queue_num)
> +{
> +    uint16_t le_queue_num;
> +
> +    le_queue_num = bswap16(queue_num);
> +    return pci_write(vdev->pci_fh, VPCI_C_OFFSET_Q_SELECT, (uint64_t)le_queue_num, 2);

The "(uint64_t)" is not required here, I think.

> +}
> +
> +int virtio_pci_set_queue_size(VDev *vdev, uint16_t queue_size)
> +{
> +    uint16_t le_queue_size;
> +
> +    le_queue_size = bswap16(queue_size);
> +    return pci_write(vdev->pci_fh, VPCI_C_OFFSET_Q_SIZE, (uint64_t)le_queue_size, 2);

dito

> +}
> +
> +static int virtio_pci_set_queue_enable(VDev *vdev, uint16_t enabled)
> +{
> +    uint16_t le_enabled;
> +
> +    le_enabled = bswap16(enabled);
> +    return pci_write(vdev->pci_fh, VPCI_C_OFFSET_Q_ENABLE, (uint64_t)le_enabled, 2);

dito

> +}
> +
> +static int set_pci_vq_addr(VDev *vdev, void* addr, uint64_t config_offset_lo)
> +{
> +    uint32_t le_lo, le_hi;
> +    uint32_t tmp;
> +    int rc;
> +
> +    tmp = (uint32_t)(((uint64_t)addr) >> 32);
> +    le_hi = bswap32(tmp);
> +
> +    tmp = (uint32_t)((uint64_t)addr & 0xFFFFFFFF);
> +    le_lo = bswap32(tmp);
> +
> +    rc =  pci_write(vdev->pci_fh, config_offset_lo, (uint64_t)le_lo, 4);
> +    rc |=  pci_write(vdev->pci_fh, config_offset_lo + 4, (uint64_t)le_hi, 4);

Wouldn't it be possible bswap64() the value and write all 8 bytes at once?

> +    return rc;
> +}
> +
> +/* virtio spec v1.1 para 4.1.2.1 */
> +void virtio_pci_id2type(VDev *vdev, uint16_t device_id)
> +{
> +    switch(device_id) {
> +    case 0x1001:
> +        vdev->type = VIRTIO_ID_BLOCK;
> +        break;
> +    case 0x1000: /* Other valid but currently unsupported virtio device types */
> +    case 0x1004:
> +    default:
> +        vdev->type = 0;
> +    }
> +}
> +
> +/*
> + * Read PCI configuration space to find the offset of the Common, Device, and
> + * Notification memory regions within the modern memory space.
> + * Returns 0 if success, 1 if a capability could not be located, or a
> + * negative RC if the configuration read failed.
> + */
> +static int virtio_pci_read_pci_cap_config(VDev *vdev)
> +{
> +    uint8_t pos;
> +    uint64_t data;
> +
> +    /* Common cabilities */
> +    pos = find_cap_pos(vdev->pci_fh, VIRTIO_PCI_CAP_COMMON_CFG);
> +    if (!pos) {
> +        puts("Failed to locate PCI common configuration");
> +        return 1;
> +    }
> +    if (pci_read(vdev->pci_fh, pos + VIRTIO_PCI_CAP_OFFSET, 15, &data, 4)) {
> +        return -EIO;
> +    }
> +    common_offset = bswap32(data);
> +
> +    /* Device cabilities */
> +    pos = find_cap_pos(vdev->pci_fh, VIRTIO_PCI_CAP_DEVICE_CFG);
> +    if (!pos) {
> +        puts("Failed to locate PCI device configuration");
> +        return 1;
> +    }
> +    if (pci_read(vdev->pci_fh, pos + VIRTIO_PCI_CAP_OFFSET, 15, &data, 4)) {
> +        return -EIO;
> +    }
> +    device_offset = bswap32(data);
> +
> +    /* Notification cabilities */
> +    pos = find_cap_pos(vdev->pci_fh, VIRTIO_PCI_CAP_NOTIFY_CFG);
> +    if (!pos) {
> +        puts("Failed to locate PCI notification configuration");
> +        return 1;
> +    }
> +    if (pci_read(vdev->pci_fh, pos + VIRTIO_PCI_CAP_OFFSET, 15, &data, 4)) {
> +        return -EIO;
> +    }
> +    notify_offset = bswap32(data);
> +
> +    if (pci_read(vdev->pci_fh, pos + VIRTIO_PCI_NOTIFY_CAP_MULT, 15, &data, 4)) {
> +        return -EIO;
> +    }
> +    notify_mult = bswap32(data);
> +
> +    if (pci_read(vdev->pci_fh, device_offset + VPCI_C_OFFSET_Q_NOFF, 4, &data, 2)) {
> +        return -EIO;
> +    }
> +    q_notify_offset = bswap16(data);

After reading all this code, I wonder whether it would make sense to have 
proper pci_read16(), pci_read32() and pci_read64() functions that do also 
the byte-swapping for you and operate on the right size of variable size?

> +    return 0;
> +}
> +
> +int virtio_pci_reset(VDev *vdev)
> +{
> +    int rc;
> +    uint8_t status = VPCI_S_RESET;
> +
> +    rc = virtio_pci_set_status(vdev, status);
> +    rc |= virtio_pci_get_status(vdev, &status);
> +
> +    if (rc || status) {
> +        puts("Failed to reset virtio-pci device");
> +        return 1;

Maybe nicer to return a negative error code (-1 if nothing else fits)?

> +    }
> +
> +    return 0;
> +}


  Thomas