Re: [PATCH v4 11/15] pc-bios/s390-ccw: Introduce virtio-pci functions

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

On Tue, 2026-03-03 at 21:59 -0500, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> Define common functionality for interacting with virtio-pci devices.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/Makefile     |   2 +-
>  pc-bios/s390-ccw/virtio-pci.c | 167 ++++++++++++++++++++++++++++++++++
>  pc-bios/s390-ccw/virtio-pci.h |  80 ++++++++++++++++
>  pc-bios/s390-ccw/virtio.h     |   5 +
>  4 files changed, 253 insertions(+), 1 deletion(-)
>  create mode 100644 pc-bios/s390-ccw/virtio-pci.c
>  create mode 100644 pc-bios/s390-ccw/virtio-pci.h
> 

...snip...
(Only comment I had in here was the s/0x1402/0x1042/ that Thomas pointed out.)


> diff --git a/pc-bios/s390-ccw/virtio-pci.h b/pc-bios/s390-ccw/virtio-pci.h
> new file mode 100644
> index 0000000000..54c524f698
> --- /dev/null
> +++ b/pc-bios/s390-ccw/virtio-pci.h
> @@ -0,0 +1,80 @@
> +/*
> + * Definitions for virtio-pci
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Jared Rossi <jrossi@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef VIRTIO_PCI_H
> +#define VIRTIO_PCI_H
> +
> +/* Common configuration */
> +#define VPCI_CAP_COMMON_CFG          1
> +/* Notifications */
> +#define VPCI_CAP_NOTIFY_CFG          2
> +/* ISR access */
> +#define VPCI_CAP_ISR_CFG             3
> +/* Device specific configuration */
> +#define VPCI_CAP_DEVICE_CFG          4
> +/* PCI configuration access */
> +#define VPCI_CAP_PCI_CFG             5
> +/* Additional shared memory capability */
> +#define VPCI_CAP_SHARED_MEMORY_CFG   8
> +/* PCI vendor data configuration */
> +#define VPCI_CAP_VENDOR_CFG          9
> +
> +/* Offsets within capability header */
> +#define VPCI_CAP_VNDR        0
> +#define VPCI_CAP_NEXT        1
> +#define VPCI_CAP_LEN         2
> +#define VPCI_CAP_CFG_TYPE    3
> +#define VPCI_CAP_BAR         4
> +#define VPCI_CAP_OFFSET      8
> +#define VPCI_CAP_LENGTH      12
> +
> +#define VPCI_N_CAP_MULT 16 /* Notify multiplier, VPCI_CAP_NOTIFY_CFG only */
> +
> +/* Common Area Offsets for virtio-pci queue */
> +#define VPCI_C_OFFSET_DFSELECT      0
> +#define VPCI_C_OFFSET_DF            4
> +#define VPCI_C_OFFSET_GFSELECT      8
> +#define VPCI_C_OFFSET_GF            12
> +#define VPCI_C_COMMON_NUMQ          18
> +#define VPCI_C_OFFSET_STATUS        20
> +#define VPCI_C_OFFSET_Q_SELECT      22
> +#define VPCI_C_OFFSET_Q_SIZE        24
> +#define VPCI_C_OFFSET_Q_ENABLE      28
> +#define VPCI_C_OFFSET_Q_NOFF        30
> +#define VPCI_C_OFFSET_Q_DESCLO      32
> +#define VPCI_C_OFFSET_Q_DESCHI      36
> +#define VPCI_C_OFFSET_Q_AVAILLO     40
> +#define VPCI_C_OFFSET_Q_AVAILHI     44
> +#define VPCI_C_OFFSET_Q_USEDLO      48
> +#define VPCI_C_OFFSET_Q_USEDHI      52
> +
> +#define VIRTIO_F_VERSION_1          1   /* Feature bit 32 */
> +
> +struct VirtioPciCap {
> +    uint8_t bar;     /* Which PCIAS it's in */
> +    uint32_t off;    /* Offset within bar */
> +};
> +typedef struct VirtioPciCap  VirtioPciCap;
> +
> +void virtio_pci_id2type(VDev *vdev, uint16_t device_id);
> +int virtio_pci_reset(VDev *vdev);
> +long virtio_pci_notify(int vq_id);
> +
> +int vpci_read_flex(uint64_t offset, uint8_t pcias, void *buf, int len);
> +int vpci_read_bswap64(uint64_t offset, uint8_t pcias, uint64_t *buf);
> +int vpci_read_bswap32(uint64_t offset, uint8_t pcias, uint32_t *buf);
> +int vpci_read_bswap16(uint64_t offset, uint8_t pcias, uint16_t *buf);
> +int vpci_read_byte(uint64_t offset, uint8_t pcias, uint8_t *buf);
> +
> +int vpci_bswap64_write(uint64_t offset, uint8_t pcias, uint64_t data);
> +int vpci_bswap32_write(uint64_t offset, uint8_t pcias, uint32_t data);
> +int vpci_bswap16_write(uint64_t offset, uint8_t pcias, uint16_t data);
> +int vpci_write_byte(uint64_t offset, uint8_t pcias, uint8_t data);

I know it's pedantic, but since the reads are vpci_read_bXXXX while the writes are vcpi_bXXXX_write
(except the single byte case) could we make them consistent, please? (Soft preference for
vpci_read/write_bXXXX, but I don't much care.)

> +
> +#endif
> diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
> index c3cb5a6ee3..0e7dbdb64c 100644
> --- a/pc-bios/s390-ccw/virtio.h
> +++ b/pc-bios/s390-ccw/virtio.h
> @@ -18,6 +18,10 @@
>  #define VIRTIO_CONFIG_S_DRIVER          2
>  /* Driver has used its parts of the config, and is happy */
>  #define VIRTIO_CONFIG_S_DRIVER_OK       4
> +/* Feature negotiation complete */
> +#define VIRTIO_CONFIG_S_FEATURES_OK     8
> +/* Clear status byte */
> +#define VIRTIO_CONFIG_S_RESET           0x00

Heh, in v3 I wondered if you could add the new status bit(s) here. Buuuut... :)

Virtio section 2.1 defines "NEEDS_RESET" as 64 (0x40). Of course it says that the status field
starts as zero and is reinitialized to zero BY a reset, which is what you end up doing. Its
placement here between FEATURES_OK and FAILED with their spec-accurate definitions, but with a
different definition had me puzzled. Maybe move it to the top of this list, and rename it to
..._S_INIT or ..._S_ZERO or something? (Or don't define it and just set it to zero in the code.)

>  /* We've given up on this device. */
>  #define VIRTIO_CONFIG_S_FAILED          0x80
>  
> @@ -255,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];
>  };