Re: [PATCH v2] hw/scsi: Add SCSI tape device emulation

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Apr 16, 2026 at 09:35:40PM +0300, rft0 wrote:
> Add initial emulation of SCSI tape device supporting basic
> INQUIRY operation for now.
> 
> Signed-off-by: rft0 <rafettaskindev@gmail.com>
> ---
> Second patch:
>  - Add kconfig entry
>  - Validate vendor, product, version, serial lengths
> 
>  hw/scsi/Kconfig     |   5 +
>  hw/scsi/meson.build |   1 +
>  hw/scsi/scsi-tape.c | 349 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 355 insertions(+)
>  create mode 100644 hw/scsi/scsi-tape.c
> 
> diff --git a/hw/scsi/Kconfig b/hw/scsi/Kconfig
> index 5743ee9b4d..06e7cab178 100644
> --- a/hw/scsi/Kconfig
> +++ b/hw/scsi/Kconfig
> @@ -1,6 +1,11 @@
>  config SCSI
>      bool
>  
> +config SCSI_TAPE
> +    bool
> +    default y
> +    depends on SCSI
> +
>  config LSI_SCSI_PCI
>      bool
>      default y if PCI_DEVICES
> diff --git a/hw/scsi/meson.build b/hw/scsi/meson.build
> index 69fde0cf84..1c62115d8e 100644
> --- a/hw/scsi/meson.build
> +++ b/hw/scsi/meson.build
> @@ -8,6 +8,7 @@ scsi_ss.add(files(
>    'scsi-disk.c',
>    'scsi-generic.c',
>  ))
> +scsi_ss.add(when: 'CONFIG_SCSI_TAPE', if_true: files('scsi-tape.c'))
>  scsi_ss.add(when: 'CONFIG_ESP', if_true: files('esp.c'))
>  scsi_ss.add(when: 'CONFIG_ESP_PCI', if_true: files('esp-pci.c'))
>  scsi_ss.add(when: 'CONFIG_LSI_SCSI_PCI', if_true: files('lsi53c895a.c'))
> diff --git a/hw/scsi/scsi-tape.c b/hw/scsi/scsi-tape.c
> new file mode 100644
> index 0000000000..adbae9b4ad
> --- /dev/null
> +++ b/hw/scsi/scsi-tape.c
> @@ -0,0 +1,349 @@
> +/*
> + * SCSI Tape Device emulation
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * Written by Rafet Taskin <rafettaskindev@gmail.com>
> + *
> + * SCSI Tape device emulation.
> + *
> + */
> +
> +#include "qemu/hw-version.h"
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/module.h"
> +#include "qemu/memalign.h"
> +#include "qemu/cutils.h"
> +#include "hw/scsi/scsi.h"
> +#include "scsi/constants.h"
> +#include "system/block-backend.h"
> +#include "hw/block/block.h"
> +#include "hw/core/qdev-properties.h"
> +#include "hw/core/qdev-properties-system.h"
> +#include "qom/object.h"
> +
> +#define SCSI_MAX_INQUIRY_LEN    256
> +
> +#define MAX_SERIAL_LEN 36

This doesn't appear to be used - the value is just
hardcoded at time of use. Can we use that, and also
add constants for the max product, vendor and
version fields, since those magic constants are
used several times over


> +static int scsi_disk_emulate_vpd_page(SCSIRequest *req, uint8_t *outbuf)
> +{
> +    SCSITapeState *s = DO_UPCAST(SCSITapeState, qdev, req->dev);
> +    uint8_t page_code = req->cmd.buf[2];
> +
> +    outbuf[0] = TYPE_TAPE;
> +    outbuf[1] = page_code;
> +    outbuf[2] = 0x00;
> +    outbuf[3] = 0x00;
> +
> +    switch (page_code) {
> +    case 0x00: /* Supported VPD pages */
> +        outbuf[4] = 0x00;   /* page 0x00 (this page) */
> +        if (s->serial) {
> +            outbuf[5] = 0x80;   /* page 0x80 (serial number) */
> +            outbuf[3] = 2;      /* page data length */
> +            return 6;
> +        }
> +        outbuf[3] = 1;
> +        return 5;
> +
> +    case 0x80: /* Unit Serial Number */
> +        if (!s->serial) {
> +            return -1;   /* not supported, caller sends INVALID_FIELD */
> +        }
> +        {
> +            int l = strlen(s->serial);
> +            outbuf[3] = l;
> +            memcpy(&outbuf[4], s->serial, l);

strpadcpy is probably a better idea, as this leaves the tail of
the buffer uninitialized and does not include a NUL terminator.
Even if the caller has initialized the buffer with nuls, it is
more reassuring to reviewers if we explicitly pad here.

Use  MAX_SERIAL_LEN here too with strpadcpy

> +            return 4 + l;
> +        }
> +
> +    default:
> +        return -1;  /* unsupported VPD page */
> +    }
> +}
> +
> +static int scsi_tape_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
> +{
> +    SCSITapeState *s = DO_UPCAST(SCSITapeState, qdev, req->dev);
> +    int buflen;
> +
> +    if (req->cmd.buf[1] & 0x1) {
> +        return scsi_disk_emulate_vpd_page(req, outbuf);
> +    }
> +
> +    /* Standard INQUIRY, not a VPD request */
> +    if (req->cmd.buf[2] != 0) {
> +        return -1;
> +    }
> +
> +    /* PAGE_CODE == 0 */
> +    buflen = req->cmd.xfer;
> +    if (buflen > SCSI_MAX_INQUIRY_LEN) {
> +        buflen = SCSI_MAX_INQUIRY_LEN;
> +    }
> +
> +    outbuf[0] = TYPE_TAPE;      /* 0x01 = Tape */
> +    outbuf[1] = 0x80;           /* Always removable */
> +    outbuf[2] = 0x05;           /* SPC-3 */
> +    outbuf[3] = 0x02 | 0x10;    /* Format 2, HiSup */
> +
> +    if (buflen > 36) {
> +        outbuf[4] = buflen - 5;
> +    } else {
> +        outbuf[4] = 36 - 5;
> +    }
> +
> +    outbuf[7] = 0x10 | (req->bus->info->tcq ? 0x02 : 0);
> +
> +    strpadcpy((char *)&outbuf[16], 16, s->product, ' ');
> +    strpadcpy((char *)&outbuf[8], 8, s->vendor, ' ');

The 2nd arg here can use MAX_VENDOR_LEN / MAX_PRODUCT_LEN

> +
> +    memset(&outbuf[32], 0, 4);
> +    memcpy(&outbuf[32], s->version, MIN(4, strlen(s->version)));

Why not use strpadcpy like the two lines above. Also MAX_VERSION_LEN
constant would be wise.

> +
> +    return buflen;
> +}

> +static void scsi_tape_realize(SCSIDevice *dev, Error **errp)
> +{
> +    SCSITapeState *s = DO_UPCAST(SCSITapeState, qdev, dev);
> +
> +    dev->type = TYPE_TAPE;
> +
> +    if (!s->qdev.conf.blk) {
> +        error_setg(errp, "Drive property not set");
> +        return;
> +    }
> +
> +    if (!blk_is_inserted(s->qdev.conf.blk)) {
> +        error_setg(errp, "Device needs media, but drive is empty");
> +        return;
> +    }
> +
> +    if (!s->vendor) {
> +        s->vendor = g_strdup("QEMU");
> +    } else if (strlen(s->vendor) > 8) {
> +        error_setg(errp, "Vendor must be 8 characters at most");
> +        return;
> +    }

Get rid of the "else" here and in the checks below - we want
to sanity check that the static constant is not oversized too,
especially for QEMU_HW_VERSION. Also use the constants for max
length and include the string in the error message . IOW more
like this code:

    if (!s->vendor) {
        s->vendor = g_strdup("QEMU");
    }
    if (strlen(s->vendor) > MAX_VENDOR_LEN) {
        error_setg(errp, "Vendor '%s' must be %d characters at most",
	           s->vendor, MAX_VENDOR_LEN);
        return;
    }




> +    if (!s->product) {
> +        s->product = g_strdup("QEMU TAPE");
> +    } else if (strlen(s->product) > 16) {
> +        error_setg(errp, "Product must be 16 characters at most");
> +        return;
> +    }
> +
> +    if (!s->version) {
> +        s->version = g_strdup(QEMU_HW_VERSION);
> +    } else if (strlen(s->version) > 4) {
> +        error_setg(errp, "Version must be 4 characters at most");
> +        return;
> +    }
> +
> +    if (s->serial && strlen(s->serial) > 36) {
> +        error_setg(errp, "Serial must be 36 characters at most");
> +        return;
> +    }
> +}

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|