RE: [PATCH v2 3/6] hw/arm/virt: Factor out common SMMUV3 dt bindings code

[Shameerali Kolothum Thodi via <qemu-devel@nongnu.org>]

> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: Monday, May 5, 2025 10:02 AM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
> qemu-devel@nongnu.org
> Cc: peter.maydell@linaro.org; jgg@nvidia.com; nicolinc@nvidia.com;
> ddutile@redhat.com; berrange@redhat.com; nathanc@nvidia.com;
> mochs@nvidia.com; smostafa@google.com; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v2 3/6] hw/arm/virt: Factor out common SMMUV3 dt
> bindings code
> 
> Hi Shameer,
> 
> On 5/2/25 12:27 PM, Shameer Kolothum wrote:
> > No functional changes intended. This will be useful when we
> > add support for user-creatable smmuv3 device.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  hw/arm/virt.c | 54 +++++++++++++++++++++++++++------------------------
> >  1 file changed, 29 insertions(+), 25 deletions(-)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index dd355f4454..464e84ae67 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1418,19 +1418,43 @@ static void create_pcie_irq_map(const
> MachineState *ms,
> >                             0x7           /* PCI irq */);
> >  }
> >
> > +static void create_smmuv3_dt_bindings(const VirtMachineState *vms,
> hwaddr base,
> > +                                      hwaddr size, int irq)
> > +{
> > +    char *node;
> > +    const char compat[] = "arm,smmu-v3";
> > +    const char irq_names[] = "eventq\0priq\0cmdq-sync\0gerror";
> > +    MachineState *ms = MACHINE(vms);
> > +
> > +    node = g_strdup_printf("/smmuv3@%" PRIx64, base);
> > +    qemu_fdt_add_subnode(ms->fdt, node);
> > +    qemu_fdt_setprop(ms->fdt, node, "compatible", compat,
> sizeof(compat));
> > +    qemu_fdt_setprop_sized_cells(ms->fdt, node, "reg", 2, base, 2, size);
> > +
> > +    qemu_fdt_setprop_cells(ms->fdt, node, "interrupts",
> > +            GIC_FDT_IRQ_TYPE_SPI, irq    , GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
> > +            GIC_FDT_IRQ_TYPE_SPI, irq + 1,
> GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
> > +            GIC_FDT_IRQ_TYPE_SPI, irq + 2,
> GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
> > +            GIC_FDT_IRQ_TYPE_SPI, irq + 3,
> GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
> > +
> > +    qemu_fdt_setprop(ms->fdt, node, "interrupt-names", irq_names,
> > +                     sizeof(irq_names));
> > +
> > +    qemu_fdt_setprop(ms->fdt, node, "dma-coherent", NULL, 0);
> > +    qemu_fdt_setprop_cell(ms->fdt, node, "#iommu-cells", 1);
> > +    qemu_fdt_setprop_cell(ms->fdt, node, "phandle", vms-
> >iommu_phandle);
> > +    g_free(node);
> > +}
> > +
> >  static void create_smmu(const VirtMachineState *vms,
> >                          PCIBus *bus)
> >  {
> >      VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> > -    char *node;
> > -    const char compat[] = "arm,smmu-v3";
> >      int irq =  vms->irqmap[VIRT_SMMU];
> >      int i;
> >      hwaddr base = vms->memmap[VIRT_SMMU].base;
> >      hwaddr size = vms->memmap[VIRT_SMMU].size;
> > -    const char irq_names[] = "eventq\0priq\0cmdq-sync\0gerror";
> >      DeviceState *dev;
> > -    MachineState *ms = MACHINE(vms);
> >
> >      if (vms->iommu != VIRT_IOMMU_SMMUV3 || !vms->iommu_phandle) {
> >          return;
> > @@ -1449,27 +1473,7 @@ static void create_smmu(const
> VirtMachineState *vms,
> >          sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
> >                             qdev_get_gpio_in(vms->gic, irq + i));
> >      }
> > -
> > -    node = g_strdup_printf("/smmuv3@%" PRIx64, base);
> > -    qemu_fdt_add_subnode(ms->fdt, node);
> > -    qemu_fdt_setprop(ms->fdt, node, "compatible", compat,
> sizeof(compat));
> > -    qemu_fdt_setprop_sized_cells(ms->fdt, node, "reg", 2, base, 2, size);
> > -
> > -    qemu_fdt_setprop_cells(ms->fdt, node, "interrupts",
> > -            GIC_FDT_IRQ_TYPE_SPI, irq    , GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
> > -            GIC_FDT_IRQ_TYPE_SPI, irq + 1, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
> > -            GIC_FDT_IRQ_TYPE_SPI, irq + 2, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
> > -            GIC_FDT_IRQ_TYPE_SPI, irq + 3,
> GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
> > -
> > -    qemu_fdt_setprop(ms->fdt, node, "interrupt-names", irq_names,
> > -                     sizeof(irq_names));
> > -
> > -    qemu_fdt_setprop(ms->fdt, node, "dma-coherent", NULL, 0);
> > -
> > -    qemu_fdt_setprop_cell(ms->fdt, node, "#iommu-cells", 1);
> > -
> > -    qemu_fdt_setprop_cell(ms->fdt, node, "phandle", vms-
> >iommu_phandle);
> > -    g_free(node);
> > +    create_smmuv3_dt_bindings(vms, base, size, irq);
> >  }
> >
> >  static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 
> nothing to do with that patch but I just noticed we omitted to support the
> 
> bypass_iommu=true along with DT mode. I don't see the iommu-map
> property set accordingly.
> 
> Something to further consolidate?

Yes. It looks like currently the virt SMMUv3 DT code doesn't take care of
bypass_iommu=true case. I will add that check.

Thanks,
Shameer