Re: [PATCH v2 6/8] ufs: Refactor common core out of the PCI implementation

Hi Mikail,

Thanks for reworking the UFS support in v2. The shared controller code
removes the duplication I pointed out in v1.

After considering the structure further, I would prefer to model UfsHc
as a plain controller core and have separate PCI and generic SysBus QOM
devices contain it, following the AHCI model. This keeps the PCI and
SysBus object layout and setup out of the shared UFS code. The generic
SysBus device can then be subclassed by Aspeed and other platforms.

I have prepared and posted a separate two-patch series implementing this
structure:

https://lore.kernel.org/qemu-devel/[email protected]/

Could you please rebase the UFS-related changes in patches 6-8 on top of
this series for v3? The Aspeed device can then derive from
TYPE_SYSBUS_UFS and provide only the AST2700-specific HCI version
setting.

Sorry for the extra churn.

Thanks,
Jeuk

On 7/31/2026 5:09 AM, Mikail Sadic wrote:
> In preparation for a sysbus UFS frontend (used by the Aspeed AST2700),
> separate the transport-independent UFS host-controller logic from the
> PCI-specific glue, following the pattern used by SDHCI and XHCI.
>
> UfsHc's PCIDevice parent is replaced by an anonymous union of a
> PCIDevice and a SysBusDevice, and the controller gains an explicit
> AddressSpace *dma_as used by ufs_addr_read()/ufs_addr_write() and the
> SG-list helpers instead of the PCI address space. Interrupt delivery
> goes through qemu_set_irq() in ufs_irq_check(). The realize path is
> split into a transport-agnostic ufs_realize_core() plus ufs_init_mmio(),
> and UfsBus gains an 'hc' back-pointer so ufs_lu_realize() no longer
> reaches through the PCI device. TYPE_UFS remains a PCI device, so
> '-device ufs' is unchanged and the PCI ufs-tests qtests still pass.
>
> Two small behaviour additions ride along on the core so that frontends
> with differing geometry and bus-scan behaviour can be modelled:
>
>    - ufs-lu gains configurable 'logical-block-size' property. It
>      defaults to UFS_BLOCK_SIZE, so the PCI device is unaffected.
>
>    - an unmapped LUN now answers INQUIRY as "not connected" rather than
>      failing the request, so that a host bus-scan does not observe
>      controller errors for absent LUNs.
>
> Signed-off-by: Mikail Sadic <[email protected]>
> ---
>   hw/ufs/ufs.h | 18 ++++++++++++++++-
>   hw/ufs/lu.c  | 50 +++++++++++++++++++++++++++++++++++++++++----
>   hw/ufs/ufs.c | 57 ++++++++++++++++++++++++++++++++++++----------------
>   3 files changed, 103 insertions(+), 22 deletions(-)
>
> diff --git a/hw/ufs/ufs.h b/hw/ufs/ufs.h
> index feb47f460d..3d4be8deb5 100644
> --- a/hw/ufs/ufs.h
> +++ b/hw/ufs/ufs.h
> @@ -12,7 +12,9 @@
>   #define HW_UFS_UFS_H
>   
>   #include "hw/pci/pci_device.h"
> +#include "hw/core/sysbus.h"
>   #include "hw/scsi/scsi.h"
> +#include "scsi/constants.h"
>   #include "block/ufs.h"
>   
>   #define UFS_MAX_LUS 32
> @@ -27,6 +29,7 @@ typedef struct UfsBusClass {
>   
>   typedef struct UfsBus {
>       BusState parent_bus;
> +    struct UfsHc *hc; /* host controller owning this bus (frontend-agnostic) */
>   } UfsBus;
>   
>   #define TYPE_UFS_BUS "ufs-bus"
> @@ -77,6 +80,7 @@ typedef UfsReqResult (*UfsScsiOp)(struct UfsLu *, UfsRequest *);
>   typedef struct UfsLu {
>       DeviceState qdev;
>       uint8_t lun;
> +    uint32_t logical_block_size;
>       UnitDescriptor unit_desc;
>       SCSIBus bus;
>       SCSIDevice *scsi_dev;
> @@ -141,7 +145,15 @@ typedef struct UfsWb {
>   } UfsWb;
>   
>   typedef struct UfsHc {
> -    PCIDevice parent_obj;
> +    /*
> +     * The controller can be instantiated either as a PCI function or as a
> +     * sysbus device.  Following the SDHCI model, both frontends share this
> +     * state structure via a union of the possible parent objects.
> +     */
> +    union {
> +        PCIDevice pci_dev;
> +        SysBusDevice sbdev;
> +    };
>       UfsBus bus;
>       MemoryRegion iomem;
>       UfsReg reg;
> @@ -162,6 +174,7 @@ typedef struct UfsHc {
>       Flags flags;
>   
>       qemu_irq irq;
> +    AddressSpace *dma_as;
>       QEMUBH *doorbell_bh;
>       QEMUBH *complete_bh;
>   
> @@ -302,4 +315,7 @@ void ufs_build_query_response(UfsRequest *req);
>   void ufs_complete_req(UfsRequest *req, UfsReqResult req_result);
>   void ufs_wb_update_avail_buffer(UfsHc *u);
>   void ufs_init_wlu(UfsLu *wlu, uint8_t wlun);
> +UfsReqResult ufs_emulate_absent_lun(UfsRequest *req);
> +void ufs_init_mmio(UfsHc *u);
> +bool ufs_realize_core(UfsHc *u, Error **errp);
>   #endif /* HW_UFS_UFS_H */
> diff --git a/hw/ufs/lu.c b/hw/ufs/lu.c
> index 13f4a90145..a0b5574668 100644
> --- a/hw/ufs/lu.c
> +++ b/hw/ufs/lu.c
> @@ -308,6 +308,44 @@ static int ufs_emulate_wlun_inquiry(UfsRequest *req, uint8_t *outbuf,
>       return SCSI_INQUIRY_LEN;
>   }
>   
> +UfsReqResult ufs_emulate_absent_lun(UfsRequest *req)
> +{
> +    QEMU_UNINITIALIZED uint8_t outbuf[SCSI_INQUIRY_LEN];
> +    uint8_t sense_buf[UFS_SENSE_SIZE];
> +    uint8_t scsi_status;
> +    int len = 0;
> +
> +    if (req->req_upiu.sc.cdb[0] == INQUIRY &&
> +        !(req->req_upiu.sc.cdb[1] & 0x1)) {
> +        /*
> +         * Standard INQUIRY to a logical unit that is not mapped: report it
> +         * as "not connected" (peripheral qualifier 0x3, device type 0x1f)
> +         * with GOOD status, like real hardware. Host-side bus scans then
> +         * skip the unit quietly instead of flagging a controller error.
> +         */
> +        memset(outbuf, 0, sizeof(outbuf));
> +        outbuf[0] = TYPE_NO_LUN;
> +        outbuf[3] = 0x2;
> +        outbuf[4] = SCSI_INQUIRY_LEN - 5;
> +        len = SCSI_INQUIRY_LEN;
> +        scsi_status = GOOD;
> +    } else {
> +        scsi_build_sense(sense_buf, SENSE_CODE(LUN_NOT_SUPPORTED));
> +        scsi_status = CHECK_CONDITION;
> +    }
> +
> +    len = MIN(len, (int)req->data_len);
> +    if (scsi_status == GOOD && len > 0 &&
> +        dma_buf_read(outbuf, len, NULL, req->sg, MEMTXATTRS_UNSPECIFIED) !=
> +            MEMTX_OK) {
> +        return UFS_REQUEST_FAIL;
> +    }
> +
> +    ufs_build_scsi_response_upiu(req, sense_buf, sizeof(sense_buf), len,
> +                                 scsi_status);
> +    return UFS_REQUEST_SUCCESS;
> +}
> +
>   static UfsReqResult ufs_emulate_scsi_cmd(UfsLu *lu, UfsRequest *req)
>   {
>       uint8_t lun = lu->lun;
> @@ -394,6 +432,8 @@ static UfsReqResult ufs_process_scsi_cmd(UfsLu *lu, UfsRequest *req)
>   static const Property ufs_lu_props[] = {
>       DEFINE_PROP_DRIVE("drive", UfsLu, conf.blk),
>       DEFINE_PROP_UINT8("lun", UfsLu, lun, 0),
> +    DEFINE_PROP_UINT32("logical-block-size", UfsLu, logical_block_size,
> +                       UFS_BLOCK_SIZE),
>   };
>   
>   static bool ufs_add_lu(UfsHc *u, UfsLu *lu, Error **errp)
> @@ -435,7 +475,7 @@ static void ufs_init_lu(UfsLu *lu)
>       lu->unit_desc.length = sizeof(UnitDescriptor);
>       lu->unit_desc.descriptor_idn = UFS_QUERY_DESC_IDN_UNIT;
>       lu->unit_desc.lu_enable = 0x01;
> -    lu->unit_desc.logical_block_size = UFS_BLOCK_SIZE_SHIFT;
> +    lu->unit_desc.logical_block_size = ctz32(lu->logical_block_size);
>       lu->unit_desc.unit_index = lu->lun;
>       lu->unit_desc.logical_block_count =
>           cpu_to_be64(brdv_len / (1 << lu->unit_desc.logical_block_size));
> @@ -475,8 +515,10 @@ static void ufs_init_scsi_device(UfsLu *lu, BlockBackend *blk, Error **errp)
>       scsi_dev = qdev_new("scsi-hd");
>       object_property_add_child(OBJECT(&lu->bus), "ufs-scsi", OBJECT(scsi_dev));
>   
> -    qdev_prop_set_uint32(scsi_dev, "physical_block_size", UFS_BLOCK_SIZE);
> -    qdev_prop_set_uint32(scsi_dev, "logical_block_size", UFS_BLOCK_SIZE);
> +    qdev_prop_set_uint32(scsi_dev, "physical_block_size",
> +                         lu->logical_block_size);
> +    qdev_prop_set_uint32(scsi_dev, "logical_block_size",
> +                         lu->logical_block_size);
>       qdev_prop_set_uint32(scsi_dev, "scsi-id", 0);
>       qdev_prop_set_uint32(scsi_dev, "lun", lu->lun);
>       if (!qdev_prop_set_drive_err(scsi_dev, "drive", blk, errp)) {
> @@ -497,7 +539,7 @@ static void ufs_lu_realize(DeviceState *dev, Error **errp)
>   {
>       UfsLu *lu = DO_UPCAST(UfsLu, qdev, dev);
>       BusState *s = qdev_get_parent_bus(dev);
> -    UfsHc *u = UFS(s->parent);
> +    UfsHc *u = UFS_BUS(s)->hc;
>       BlockBackend *blk = lu->conf.blk;
>   
>       if (!ufs_lu_check_constraints(lu, errp)) {
> diff --git a/hw/ufs/ufs.c b/hw/ufs/ufs.c
> index 464fd465b3..1489bd3321 100644
> --- a/hw/ufs/ufs.c
> +++ b/hw/ufs/ufs.c
> @@ -102,7 +102,8 @@ static MemTxResult ufs_addr_read(UfsHc *u, hwaddr addr, void *buf, int size)
>           return MEMTX_DECODE_ERROR;
>       }
>   
> -    return pci_dma_read(PCI_DEVICE(u), addr, buf, size);
> +    return dma_memory_read(u->dma_as, addr, buf, size,
> +                           MEMTXATTRS_UNSPECIFIED);
>   }
>   
>   static MemTxResult ufs_addr_write(UfsHc *u, hwaddr addr, const void *buf,
> @@ -117,7 +118,8 @@ static MemTxResult ufs_addr_write(UfsHc *u, hwaddr addr, const void *buf,
>           return MEMTX_DECODE_ERROR;
>       }
>   
> -    return pci_dma_write(PCI_DEVICE(u), addr, buf, size);
> +    return dma_memory_write(u->dma_as, addr, buf, size,
> +                            MEMTXATTRS_UNSPECIFIED);
>   }
>   
>   static inline hwaddr ufs_get_utrd_addr(UfsHc *u, uint32_t slot)
> @@ -222,7 +224,7 @@ static MemTxResult ufs_dma_read_prdt(UfsRequest *req)
>       }
>   
>       req->sg = g_malloc0(sizeof(QEMUSGList));
> -    pci_dma_sglist_init(req->sg, PCI_DEVICE(u), prdt_len);
> +    qemu_sglist_init(req->sg, DEVICE(u), prdt_len, u->dma_as);
>       req->data_len = 0;
>   
>       for (uint16_t i = 0; i < prdt_len; ++i) {
> @@ -317,14 +319,12 @@ static MemTxResult ufs_dma_write_upiu(UfsRequest *req)
>   
>   static void ufs_irq_check(UfsHc *u)
>   {
> -    PCIDevice *pci = PCI_DEVICE(u);
> -
>       if ((u->reg.is & UFS_INTR_MASK) & u->reg.ie) {
>           trace_ufs_irq_raise();
> -        pci_irq_assert(pci);
> +        qemu_set_irq(u->irq, 1);
>       } else {
>           trace_ufs_irq_lower();
> -        pci_irq_deassert(pci);
> +        qemu_set_irq(u->irq, 0);
>       }
>   }
>   
> @@ -1092,7 +1092,7 @@ static UfsReqResult ufs_exec_scsi_cmd(UfsRequest *req)
>   
>       if (!is_wlun(lun) && (lun >= UFS_MAX_LUS || u->lus[lun] == NULL)) {
>           trace_ufs_err_scsi_cmd_invalid_lun(lun);
> -        return UFS_REQUEST_FAIL;
> +        return ufs_emulate_absent_lun(req);
>       }
>   
>       switch (lun) {
> @@ -2488,6 +2488,12 @@ static bool ufs_check_constraints(UfsHc *u, Error **errp)
>       return true;
>   }
>   
> +void ufs_init_mmio(UfsHc *u)
> +{
> +    memory_region_init_io(&u->iomem, OBJECT(u), &ufs_mmio_ops, u, "ufs",
> +                          u->reg_size);
> +}
> +
>   static void ufs_init_pci(UfsHc *u, PCIDevice *pci_dev)
>   {
>       uint8_t *pci_conf = pci_dev->config;
> @@ -2495,8 +2501,7 @@ static void ufs_init_pci(UfsHc *u, PCIDevice *pci_dev)
>       pci_conf[PCI_INTERRUPT_PIN] = 1;
>       pci_config_set_prog_interface(pci_conf, 0x1);
>   
> -    memory_region_init_io(&u->iomem, OBJECT(u), &ufs_mmio_ops, u, "ufs",
> -                          u->reg_size);
> +    ufs_init_mmio(u);
>       pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &u->iomem);
>       u->irq = pci_allocate_irq(pci_dev);
>   }
> @@ -2689,25 +2694,43 @@ static void ufs_init_hc(UfsHc *u)
>       timer_mod(&u->idle_timer, now + UFS_IDLE_TIMER_TICK);
>   }
>   
> -static void ufs_realize(PCIDevice *pci_dev, Error **errp)
> +/*
> + * Frontend-agnostic realization: everything except the host-bus specific
> + * MMIO/IRQ/DMA plumbing, which the PCI or sysbus frontend sets up around
> + * this call.  The frontend must assign u->dma_as before calling.
> + */
> +bool ufs_realize_core(UfsHc *u, Error **errp)
>   {
> -    UfsHc *u = UFS(pci_dev);
> -
>       if (!ufs_check_constraints(u, errp)) {
> -        return;
> +        return false;
>       }
>   
> -    qbus_init(&u->bus, sizeof(UfsBus), TYPE_UFS_BUS, &pci_dev->qdev,
> -              u->parent_obj.qdev.id);
> +    qbus_init(&u->bus, sizeof(UfsBus), TYPE_UFS_BUS, DEVICE(u),
> +              DEVICE(u)->id);
> +    u->bus.hc = u;
>   
>       ufs_init_state(u);
>       ufs_init_hc(u);
> -    ufs_init_pci(u, pci_dev);
>   
>       ufs_init_wlu(&u->report_wlu, UFS_UPIU_REPORT_LUNS_WLUN);
>       ufs_init_wlu(&u->dev_wlu, UFS_UPIU_UFS_DEVICE_WLUN);
>       ufs_init_wlu(&u->boot_wlu, UFS_UPIU_BOOT_WLUN);
>       ufs_init_wlu(&u->rpmb_wlu, UFS_UPIU_RPMB_WLUN);
> +
> +    return true;
> +}
> +
> +static void ufs_realize(PCIDevice *pci_dev, Error **errp)
> +{
> +    UfsHc *u = UFS(pci_dev);
> +
> +    u->dma_as = pci_get_address_space(pci_dev);
> +
> +    if (!ufs_realize_core(u, errp)) {
> +        return;
> +    }
> +
> +    ufs_init_pci(u, pci_dev);
>   }
>   
>   static void ufs_exit(PCIDevice *pci_dev)