Re: [PATCH 2/2] scsi: Initial commit of VirtIO PCIe Endpoint Driver

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Sep 01, 2026 at 11:46:50AM +1000, alistair23@gmail.com wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> This patch adds a VirtIO SCSI endpoint build on top of the VirtIO PCIe
> endpoint. This is a similar approach to the NVMe PCIe Endpoint
> (drivers/nvme/target/pci-epf.c) but for SCSI.
> 
> This does end up being somewhat similar to the pci-epf.c code, but
> re-written for SCSI.
> 
> This approach allows a PCIe Endpoint device (tested on a
> radxa-rock5b) to setup what appears to be a SCSI device, using an
> existing SCSI backend (tested using scsi_debug).
> 
> At this point a host can connect over PCIe, ensure virtio_pci and
> virtio_scsi is loaded and on PCIe rescan will see a scsi device.
> 
> There are two main pain points with this approach though:
>  1. We have to use the Legacy SCSI VirtIO driver. This is because the
>     Raxda Rock5b (and AFAIK all PCIe Endpoint hardware) can't add
>     capabilities. So we can't advertise the VirtIO Common configuration
>     capability, which means we can't be a modern VirtIO SCSI device.
> 
>     This is unfortunate, but there doesn't seem to be any way around
>     this, at least with the current hardware.

This was discussed on the virtio list. The way around this is
to change virtio spec to allow common configuration
in memory. I don't think a patch like this was ever posted -
want to try?


....



> +
> +static int scsit_pci_epf_read_desc(struct scsit_pci_epf_vq *vq,
> +				   u16 idx, struct vring_desc *desc)
> +{
> +	void __iomem *p;
> +
> +	if (idx >= vq->depth)
> +		return -EINVAL;
> +
> +	p = vq->desc_map.virt_addr + (size_t)idx * sizeof(struct vring_desc);
> +	desc->addr = cpu_to_le64(readq(p + offsetof(struct vring_desc, addr)));
> +	desc->len = cpu_to_le32(readl(p + offsetof(struct vring_desc, len)));
> +	desc->flags = cpu_to_le16(readw(p + offsetof(struct vring_desc, flags)));
> +	desc->next = cpu_to_le16(readw(p + offsetof(struct vring_desc, next)));
> +
> +	return 0;
> +}
> +
> +static int scsit_pci_epf_read_indirect(struct scsit_pci_epf_ctrl *ctrl,
> +				       const struct vring_desc *parent,
> +				       struct vring_desc **out_descs,
> +				       unsigned int *out_n)
> +{
> +	struct vring_desc *descs;
> +	u32 len = le32_to_cpu(parent->len);
> +	unsigned int n;
> +	int ret;
> +
> +	if (len % sizeof(struct vring_desc) || !len)
> +		return -EINVAL;
> +
> +	n = len / sizeof(struct vring_desc);
> +	descs = kmalloc_array(n, sizeof(*descs), GFP_KERNEL);
> +	if (!descs)
> +		return -ENOMEM;
> +
> +	ret = scsit_pci_epf_transfer(ctrl, descs, le64_to_cpu(parent->addr),
> +				     len, DMA_FROM_DEVICE);
> +	if (ret) {
> +		kfree(descs);
> +		return ret;
> +	}
> +
> +	*out_descs = descs;
> +	*out_n = n;
> +	return 0;
> +}
> +
> +struct scsit_pci_epf_chain {
> +	struct vring_desc	*ro;
> +	unsigned int		nr_ro;
> +	u32			ro_len;
> +	struct vring_desc	*wo;
> +	unsigned int		nr_wo;
> +	u32			wo_len;
> +
> +	struct vring_desc	*indirect;
> +};
> +
> +static void scsit_pci_epf_chain_free(struct scsit_pci_epf_chain *c)
> +{
> +	kfree(c->ro);
> +	c->ro = NULL;
> +	kfree(c->wo);
> +	c->wo = NULL;
> +	kfree(c->indirect);
> +	c->indirect = NULL;
> +}
> +
> +static int scsit_pci_epf_walk_chain(struct scsit_pci_epf_ctrl *ctrl,
> +				    struct scsit_pci_epf_vq *vq,
> +				    u16 head_idx,
> +				    struct scsit_pci_epf_chain *out)
> +{
> +	struct vring_desc desc, *descs = NULL;
> +	unsigned int n = 0, i = 0, cap_ro = 8, cap_wo = 8;
> +	bool in_indirect = false;
> +	int ret;
> +
> +	memset(out, 0, sizeof(*out));
> +
> +	out->ro = kmalloc_array(cap_ro, sizeof(*out->ro), GFP_KERNEL);
> +	out->wo = kmalloc_array(cap_wo, sizeof(*out->wo), GFP_KERNEL);
> +	if (!out->ro || !out->wo) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	ret = scsit_pci_epf_read_desc(vq, head_idx, &desc);
> +	if (ret)
> +		goto err;
> +
> +	/* Reject obviously-invalid head descriptors */
> +	if (!le64_to_cpu(desc.addr) && !le32_to_cpu(desc.len) &&
> +	    !le16_to_cpu(desc.flags)) {
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	for (;;) {
> +		u16 flags = le16_to_cpu(desc.flags);
> +
> +		if (!in_indirect && (flags & VRING_DESC_F_INDIRECT)) {
> +			ret = scsit_pci_epf_read_indirect(ctrl, &desc,
> +							  &descs, &n);
> +			if (ret)
> +				goto err;
> +			out->indirect = descs;
> +			in_indirect = true;
> +			i = 0;
> +			desc = descs[0];
> +			continue;
> +		}
> +
> +		if (flags & VRING_DESC_F_WRITE) {
> +			if (out->nr_wo == cap_wo) {
> +				cap_wo *= 2;
> +				out->wo = krealloc_array(out->wo, cap_wo,
> +						sizeof(*out->wo), GFP_KERNEL);
> +				if (!out->wo) {
> +					ret = -ENOMEM;
> +					goto err;
> +				}
> +			}
> +			out->wo[out->nr_wo++] = desc;
> +			out->wo_len += le32_to_cpu(desc.len);
> +		} else {
> +			/* Read-only descriptors must precede write-only. */
> +			if (out->nr_wo) {
> +				ret = -EINVAL;
> +				goto err;
> +			}
> +			if (out->nr_ro == cap_ro) {
> +				cap_ro *= 2;
> +				out->ro = krealloc_array(out->ro, cap_ro,
> +						sizeof(*out->ro), GFP_KERNEL);
> +				if (!out->ro) {
> +					ret = -ENOMEM;
> +					goto err;
> +				}
> +			}
> +			out->ro[out->nr_ro++] = desc;
> +			out->ro_len += le32_to_cpu(desc.len);
> +		}
> +
> +		if (!(flags & VRING_DESC_F_NEXT))
> +			break;
> +
> +		if (in_indirect) {
> +			i = le16_to_cpu(desc.next);
> +			if (i >= n) {
> +				ret = -EINVAL;
> +				goto err;
> +			}
> +			desc = descs[i];
> +		} else {
> +			u16 next = le16_to_cpu(desc.next);
> +
> +			ret = scsit_pci_epf_read_desc(vq, next, &desc);
> +			if (ret)
> +				goto err;
> +		}
> +	}
> +
> +	return 0;
> +
> +err:
> +	scsit_pci_epf_chain_free(out);
> +	return ret;
> +}
> +





> +static int scsit_pci_epf_chain_to_segs(struct scsit_pci_epf_cmd *cmd,
> +				       const struct vring_desc *descs,
> +				       unsigned int n)
> +{
> +	struct scsit_pci_epf_segment *seg;
> +	unsigned int i;
> +	int ret;
> +
> +	if (n == 0)
> +		return 0;
> +
> +	if (n == 1) {
> +		cmd->nr_data_segs = 1;
> +		cmd->data_segs = &cmd->data_seg;
> +		seg = &cmd->data_segs[0];
> +		seg->pci_addr = le64_to_cpu(descs[0].addr);
> +		seg->length = le32_to_cpu(descs[0].len);
> +		return 0;
> +	}
> +
> +	ret = scsit_pci_epf_alloc_cmd_data_segs(cmd, n);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < n; i++) {
> +		seg = &cmd->data_segs[i];
> +		seg->pci_addr = le64_to_cpu(descs[i].addr);
> +		seg->length = le32_to_cpu(descs[i].len);
> +	}
> +
> +	return 0;
> +}
> +
> +static int scsit_pci_epf_alloc_cmd_data_buf(struct scsit_pci_epf_cmd *cmd)
> +{
> +	struct scsit_pci_epf_ctrl *ctrl = cmd->ctrl;
> +	struct scsit_pci_epf_segment *seg;
> +	struct scatterlist *sg;
> +	int ret, i;
> +
> +	if (cmd->se_cmd.data_length > ctrl->mdts)
> +		return -EINVAL;
> +
> +	if (cmd->nr_data_segs == 1) {
> +		sg_init_table(&cmd->data_sgl, 1);
> +		cmd->data_sgt.sgl = &cmd->data_sgl;
> +		cmd->data_sgt.nents = 1;
> +		cmd->data_sgt.orig_nents = 1;
> +	} else {
> +		ret = sg_alloc_table(&cmd->data_sgt, cmd->nr_data_segs,
> +				     GFP_KERNEL);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for_each_sgtable_sg(&cmd->data_sgt, sg, i) {
> +		seg = &cmd->data_segs[i];
> +		seg->buf = kmalloc(seg->length, GFP_KERNEL);
> +		if (!seg->buf)
> +			return -ENOMEM;
> +		sg_set_buf(sg, seg->buf, seg->length);
> +	}
> +
> +	return 0;
> +}
> +
> +static void scsit_pci_epf_post_used(struct scsit_pci_epf_vq *vq,
> +				    u16 head_idx, u32 len)
> +{
> +	void __iomem *ring = vq->used_map.virt_addr +
> +			     offsetof(struct vring_used, ring) +
> +			     (size_t)(vq->next_used_idx & (vq->depth - 1)) *
> +			     sizeof(struct vring_used_elem);
> +
> +	writel(head_idx, ring + offsetof(struct vring_used_elem, id));
> +	writel(len, ring + offsetof(struct vring_used_elem, len));
> +
> +	vq->next_used_idx++;
> +	writew(vq->next_used_idx,
> +	       vq->used_map.virt_addr + offsetof(struct vring_used, idx));
> +}



So this is a reimplementation of virtio, and a weird one, for example
features are legacy but format is LE like modern. Packed ring is
not implemented.  Uses strongly ordered ops for everything.

I don't have specific advice but there is some similarity here
to what Alex Graf is doing.


And I want to see a spec decumenting how all this is supposed
to work, please, and not just an ad hoc implementation.



-- 
MST