Re: [PATCH v2 04/15] hw/riscv: add riscv-iommu-pci device

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

On 4/29/24 04:21, Frank Chang wrote:
> Daniel Henrique Barboza <dbarboza@ventanamicro.com <mailto:dbarboza@ventanamicro.com>> 於 2024年3月8日 週五 上午12:04寫道：
>  >
>  > From: Tomasz Jeznach <tjeznach@rivosinc.com <mailto:tjeznach@rivosinc.com>>
>  >
>  > The RISC-V IOMMU can be modelled as a PCIe device following the
>  > guidelines of the RISC-V IOMMU spec, chapter 7.1, "Integrating an IOMMU
>  > as a PCIe device".
>  >
>  > Signed-off-by: Tomasz Jeznach <tjeznach@rivosinc.com <mailto:tjeznach@rivosinc.com>>
>  > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com <mailto:dbarboza@ventanamicro.com>>
>  > ---
>  >  hw/riscv/meson.build       |   2 +-
>  >  hw/riscv/riscv-iommu-pci.c | 173 +++++++++++++++++++++++++++++++++++++
>  >  2 files changed, 174 insertions(+), 1 deletion(-)
>  >  create mode 100644 hw/riscv/riscv-iommu-pci.c
>  >
>  > diff --git a/hw/riscv/meson.build b/hw/riscv/meson.build
>  > index ba9eebd605..4674cec6c4 100644
>  > --- a/hw/riscv/meson.build
>  > +++ b/hw/riscv/meson.build
>  > @@ -10,6 +10,6 @@ riscv_ss.add(when: 'CONFIG_SIFIVE_U', if_true: files('sifive_u.c'))
>  >  riscv_ss.add(when: 'CONFIG_SPIKE', if_true: files('spike.c'))
>  >  riscv_ss.add(when: 'CONFIG_MICROCHIP_PFSOC', if_true: files('microchip_pfsoc.c'))
>  >  riscv_ss.add(when: 'CONFIG_ACPI', if_true: files('virt-acpi-build.c'))
>  > -riscv_ss.add(when: 'CONFIG_RISCV_IOMMU', if_true: files('riscv-iommu.c'))
>  > +riscv_ss.add(when: 'CONFIG_RISCV_IOMMU', if_true: files('riscv-iommu.c', 'riscv-iommu-pci.c'))
>  >
>  >  hw_arch += {'riscv': riscv_ss}
>  > diff --git a/hw/riscv/riscv-iommu-pci.c b/hw/riscv/riscv-iommu-pci.c
>  > new file mode 100644
>  > index 0000000000..4eb1057210
>  > --- /dev/null
>  > +++ b/hw/riscv/riscv-iommu-pci.c
>  > @@ -0,0 +1,173 @@
>  > +/*
>  > + * QEMU emulation of an RISC-V IOMMU (Ziommu)
>  > + *
>  > + * Copyright (C) 2022-2023 Rivos Inc.
>  > + *
>  > + * This program is free software; you can redistribute it and/or modify
>  > + * it under the terms of the GNU General Public License as published by
>  > + * the Free Software Foundation; either version 2 of the License.
>  > + *
>  > + * This program is distributed in the hope that it will be useful,
>  > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>  > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>  > + * GNU General Public License for more details.
>  > + *
>  > + * You should have received a copy of the GNU General Public License along
>  > + * with this program; if not, see <http://www.gnu.org/licenses/ <http://www.gnu.org/licenses/>>.
>  > + */
>  > +
>  > +#include "qemu/osdep.h"
>  > +#include "hw/pci/msi.h"
>  > +#include "hw/pci/msix.h"
>  > +#include "hw/pci/pci_bus.h"
>  > +#include "hw/qdev-properties.h"
>  > +#include "hw/riscv/riscv_hart.h"
>  > +#include "migration/vmstate.h"
>  > +#include "qapi/error.h"
>  > +#include "qemu/error-report.h"
>  > +#include "qemu/host-utils.h"
>  > +#include "qom/object.h"
>  > +
>  > +#include "cpu_bits.h"
>  > +#include "riscv-iommu.h"
>  > +#include "riscv-iommu-bits.h"
>  > +
>  > +#ifndef PCI_VENDOR_ID_RIVOS
>  > +#define PCI_VENDOR_ID_RIVOS           0x1efd
>  > +#endif
>  > +
>  > +#ifndef PCI_DEVICE_ID_RIVOS_IOMMU
>  > +#define PCI_DEVICE_ID_RIVOS_IOMMU     0xedf1
>  > +#endif
>  > +
>  > +/* RISC-V IOMMU PCI Device Emulation */
>  > +
>  > +typedef struct RISCVIOMMUStatePci {
>  > +    PCIDevice        pci;     /* Parent PCIe device state */
>  > +    MemoryRegion     bar0;    /* PCI BAR (including MSI-x config) */
>  > +    RISCVIOMMUState  iommu;   /* common IOMMU state */
>  > +} RISCVIOMMUStatePci;
>  > +
>  > +/* interrupt delivery callback */
>  > +static void riscv_iommu_pci_notify(RISCVIOMMUState *iommu, unsigned vector)
>  > +{
>  > +    RISCVIOMMUStatePci *s = container_of(iommu, RISCVIOMMUStatePci, iommu);
>  > +
>  > +    if (msix_enabled(&(s->pci))) {
>  > +        msix_notify(&(s->pci), vector);
>  > +    }
>  > +}
>  > +
>  > +static void riscv_iommu_pci_realize(PCIDevice *dev, Error **errp)
>  > +{
>  > +    RISCVIOMMUStatePci *s = DO_UPCAST(RISCVIOMMUStatePci, pci, dev);
>  > +    RISCVIOMMUState *iommu = &s->iommu;
>  > +    Error *err = NULL;
>  > +
>  > +    /* Set device id for trace / debug */
>  > +    DEVICE(iommu)->id = g_strdup_printf("%02x:%02x.%01x",
>  > +        pci_dev_bus_num(dev), PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
> 
> pci_dev_bus_num() calls pci_bus_num(),
> and pci_bus_num() is assigned to pcibus_num(),
> which returns bus->parent_dev->config[PCI_SECONDARY_BUS]
> However, PCI bus number is not initialized by SW when IOMMU is initialized.
> So pci_bus_num() will always return 0, IIRC.
> Same issue as pci_bus_num() above.
> 
>  > +    qdev_realize(DEVICE(iommu), NULL, errp);
>  > +
>  > +    memory_region_init(&s->bar0, OBJECT(s), "riscv-iommu-bar0",
>  > +        QEMU_ALIGN_UP(memory_region_size(&iommu->regs_mr), TARGET_PAGE_SIZE));
>  > +    memory_region_add_subregion(&s->bar0, 0, &iommu->regs_mr);
>  > +
>  > +    pcie_endpoint_cap_init(dev, 0);
>  > +
>  > +    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
>  > +                     PCI_BASE_ADDRESS_MEM_TYPE_64, &s->bar0);
>  > +
>  > +    int ret = msix_init(dev, RISCV_IOMMU_INTR_COUNT,
>  > +                        &s->bar0, 0, RISCV_IOMMU_REG_MSI_CONFIG,
>  > +                        &s->bar0, 0, RISCV_IOMMU_REG_MSI_CONFIG + 256, 0, &err);
>  > +
>  > +    if (ret == -ENOTSUP) {
>  > +        /*
>  > +         * MSI-x is not supported by the platform.
>  > +         * Driver should use timer/polling based notification handlers.
>  > +         */
>  > +        warn_report_err(err);
>  > +    } else if (ret < 0) {
>  > +        error_propagate(errp, err);
>  > +        return;
>  > +    } else {
>  > +        /* mark all allocated MSIx vectors as used. */
>  > +        msix_vector_use(dev, RISCV_IOMMU_INTR_CQ);
>  > +        msix_vector_use(dev, RISCV_IOMMU_INTR_FQ);
>  > +        msix_vector_use(dev, RISCV_IOMMU_INTR_PM);
>  > +        msix_vector_use(dev, RISCV_IOMMU_INTR_PQ);
>  > +        iommu->notify = riscv_iommu_pci_notify;
>  > +    }
>  > +
>  > +    PCIBus *bus = pci_device_root_bus(dev);
>  > +    if (!bus) {
>  > +        error_setg(errp, "can't find PCIe root port for %02x:%02x.%x",
>  > +            pci_bus_num(pci_get_bus(dev)), PCI_SLOT(dev->devfn),
> 
> Same issue to pci_dev_bus_num() above.
> 
>  > +            PCI_FUNC(dev->devfn));
>  > +        return;
>  > +    }
>  > +
>  > +    riscv_iommu_pci_setup_iommu(iommu, bus, errp);
>  > +}
>  > +
>  > +static void riscv_iommu_pci_exit(PCIDevice *pci_dev)
>  > +{
>  > +    pci_setup_iommu(pci_device_root_bus(pci_dev), NULL, NULL);
>  > +}
>  > +
>  > +static const VMStateDescription riscv_iommu_vmstate = {
>  > +    .name = "riscv-iommu",
>  > +    .unmigratable = 1
>  > +};
>  > +
>  > +static void riscv_iommu_pci_init(Object *obj)
>  > +{
>  > +    RISCVIOMMUStatePci *s = RISCV_IOMMU_PCI(obj);
>  > +    RISCVIOMMUState *iommu = &s->iommu;
>  > +
>  > +    object_initialize_child(obj, "iommu", iommu, TYPE_RISCV_IOMMU);
>  > +    qdev_alias_all_properties(DEVICE(iommu), obj);
>  > +}
>  > +
>  > +static Property riscv_iommu_pci_properties[] = {
>  > +    DEFINE_PROP_END_OF_LIST(),
>  > +};
> 
> Do we need to assign the empty properties?
> 
>  > +
>  > +static void riscv_iommu_pci_class_init(ObjectClass *klass, void *data)
>  > +{
>  > +    DeviceClass *dc = DEVICE_CLASS(klass);
>  > +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  > +
>  > +    k->realize = riscv_iommu_pci_realize;
>  > +    k->exit = riscv_iommu_pci_exit;
>  > +    k->vendor_id = PCI_VENDOR_ID_RIVOS;
>  > +    k->device_id = PCI_DEVICE_ID_RIVOS_IOMMU;
> 
> I know RIVOS originally modeled this IOMMU,
> but we (SiFive) also have our IOMMU based on RISC-V IOMMU:
> https://open-src-soc.org/2022-05/media/slides/RISC-V-International-Day-2022-05-05-14h10-Perinne-Peresse.pdf <https://open-src-soc.org/2022-05/media/slides/RISC-V-International-Day-2022-05-05-14h10-Perinne-Peresse.pdf>
> Do we have the guidelines on how to extend the vendor IOMMU?

We'll use a generic PCI ID for the QEMU IOMMU model. Drew is giving a
hand looking into it. We'll either send a separated patch to add
the new PCI ID or fold the patch into this series.

As for extend the generic IOMMU, you can use this base implementation
(that doesn't have anything specific to Rivos, it's a generic spec
implementation) to implement the Si-Five device if you want. It
woud a device like:

>  > +static const TypeInfo riscv_sifive_iommu_pci = {
>  > +    .name = TYPE_RISCV_SIFIVE_IOMMU_PCI,
>  > +    .parent = TYPE_RISCV_IOMMU_PCI,

Same thing with the base emulation code. You can use it as a base and then
add the logic that is exclusive to Si-Five on top of it.


Thanks,

Daniel



> 
>  > +    k->revision = 0;
>  > +    k->class_id = 0x0806;
> 
> We should add
> #define PCI_CLASS_SYSTEM_IOMMU 0x0806
> instead of the hard-coded value.
> 
> P.S. AMD's IOMMU also uses hard-coded value 0x0806 in: hw/i386/amd_iommu.c.
> 
>  > +    dc->desc = "RISCV-IOMMU DMA Remapping device";
>  > +    dc->vmsd = &riscv_iommu_vmstate;
>  > +    dc->hotpluggable = false;
>  > +    dc->user_creatable = true;
>  > +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>  > +    device_class_set_props(dc, riscv_iommu_pci_properties);
>  > +}
>  > +
>  > +static const TypeInfo riscv_iommu_pci = {
>  > +    .name = TYPE_RISCV_IOMMU_PCI,
>  > +    .parent = TYPE_PCI_DEVICE,
>  > +    .class_init = riscv_iommu_pci_class_init,
>  > +    .instance_init = riscv_iommu_pci_init,
>  > +    .instance_size = sizeof(RISCVIOMMUStatePci),
>  > +    .interfaces = (InterfaceInfo[]) {
>  > +        { INTERFACE_PCIE_DEVICE },
>  > +        { },
>  > +    },
>  > +};
>  > +
>  > +static void riscv_iommu_register_pci_types(void)
>  > +{
>  > +    type_register_static(&riscv_iommu_pci);
>  > +}
>  > +
>  > +type_init(riscv_iommu_register_pci_types);
>  > --
>  > 2.43.2
>  >
>  >