Re: [Qemu-devel] [PATCH v2 5/9] hw: arm: Add SMMUv3 to virt platform, create DTS accordingly

[Auger Eric <eric.auger@redhat.com>]

Hi Prem,

> Default virt platform now creates SMMU device.
> Default config to build SMMU device along is in previous patches.
> 
> Signed-off-by: Prem Mallappa <prem.mallappa@broadcom.com>
> ---
>  hw/arm/virt.c         | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/smmu.h | 33 +++++++++++++++++++++++++++
>  include/hw/arm/virt.h |  2 ++
>  3 files changed, 97 insertions(+)
>  create mode 100644 include/hw/arm/smmu.h
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index c5c125e..f3c7891 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -57,6 +57,7 @@
>  #include "hw/smbios/smbios.h"
>  #include "qapi/visitor.h"
>  #include "standard-headers/linux/input.h"
> +#include "hw/arm/smmu.h"
>  
>  /* Number of external interrupt lines to configure the GIC with */
>  #define NUM_IRQS 256
> @@ -77,6 +78,7 @@ typedef struct VirtBoardInfo {
>      uint32_t gic_phandle;
>      uint32_t v2m_phandle;
>      bool using_psci;
> +    uint32_t smmu_phandle;
nit: would group the phandles together
>  } VirtBoardInfo;
>  
>  typedef struct {
> @@ -175,6 +177,7 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
>      [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
>      [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
> +    [VIRT_SMMU] =               { 0x09050000, 0x00020000 }, /* 128K, needed */
>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> @@ -195,9 +198,19 @@ static const int a15irqmap[] = {
>      [VIRT_SECURE_UART] = 8,
>      [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
>      [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
Currently don' we have NUM_GICV2M_SPIS = 64?
> +    [VIRT_SMMU] = 74,    /* ...to 74 + NUM_SMMU_IRQS - 1 */
>      [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */
>  };
>  
> +static const struct smmuirq {
> +    const char *name;
> +} smmuirqmap[NUM_SMMU_IRQS] = {
> +    [SMMU_IRQ_EVTQ] = {"eventq"},
> +    [SMMU_IRQ_PRIQ] = {"priq"},
> +    [SMMU_IRQ_CMD_SYNC] = {"cmdq-sync"},
> +    [SMMU_IRQ_GERROR] = {"gerror"},
> +};
> +
>  static VirtBoardInfo machines[] = {
>      {
>          .cpu_model = "cortex-a15",
> @@ -938,6 +951,50 @@ static void create_pcie_irq_map(const VirtBoardInfo *vbi, uint32_t gic_phandle,
>                             0x7           /* PCI irq */);
>  }
>  
> +static void alloc_smmu_phandle(VirtBoardInfo *vbi)
> +{
I would rather put that code in create_smmu as it is done in create_gic
for instance
> +    if (!vbi->smmu_phandle)
> +        vbi->smmu_phandle = qemu_fdt_alloc_phandle(vbi->fdt);
> +}
> +
> +static void create_smmu(VirtBoardInfo *vbi, qemu_irq *pic)
> +{
> +    int i;
> +    char *smmu;
> +    const char compat[] = "arm,smmu-v3";
> +    int irq =  vbi->irqmap[VIRT_SMMU];
> +    hwaddr base = vbi->memmap[VIRT_SMMU].base;
> +    hwaddr size = vbi->memmap[VIRT_SMMU].size;
> +    int type = GIC_FDT_IRQ_TYPE_SPI;
> +
> +    sysbus_create_varargs("smmuv3", base,
> +                          pic[irq],
> +                          pic[irq + 1],
> +                          pic[irq + 2],
> +                          pic[irq + 3],
> +                          NULL);
> +
> +    smmu = g_strdup_printf("/smmuv3@%" PRIx64, base);
> +    qemu_fdt_add_subnode(vbi->fdt, smmu);
> +    qemu_fdt_setprop(vbi->fdt, smmu, "compatible", compat, sizeof(compat));
> +    qemu_fdt_setprop_sized_cells(vbi->fdt, smmu, "reg", 2, base, 2, size);
> +
> +    for (i = 0; i < NUM_SMMU_IRQS; i++) {
> +        qemu_fdt_appendprop_cells(vbi->fdt, smmu, "interrupts",
> +                                  type, irq + i,
> +                                  GIC_FDT_IRQ_FLAGS_LEVEL_HI);
> +        qemu_fdt_appendprop_string(vbi->fdt, smmu, "interrupt-names",
> +                                   smmuirqmap[i].name);
so as I mentionned before I think we can manage with appendprop helpers
but let's see ...
> +    }
> +
> +    qemu_fdt_setprop_cell(vbi->fdt, smmu, "clocks", vbi->clock_phandle);
> +    qemu_fdt_setprop_cell(vbi->fdt, smmu, "#iommu-cells", 0);
> +    qemu_fdt_setprop_string(vbi->fdt, smmu, "clock-names", "apb_pclk");
> +
> +    qemu_fdt_setprop_cell(vbi->fdt, smmu, "phandle", vbi->smmu_phandle);
> +    g_free(smmu);
> +}
> +
>  static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic,
>                          bool use_highmem)
>  {
> @@ -1048,6 +1105,7 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic,
>      }
>  
>      qemu_fdt_setprop_cell(vbi->fdt, nodename, "#interrupt-cells", 1);
> +    qemu_fdt_setprop_cells(vbi->fdt, nodename, "iommus", vbi->smmu_phandle);
So if I understand correctly this connects the PCIe host controller to
the emulated smmu, always? If confirmed I suspect this is not what we
want. At leat we need a machine option. Or can't we target dynamic
instantiation using -device qemu option (as it is done on x86) using the
platform bus infra, as we do for VFIO platform devices? This currently
work with dt but maybe we can achieve ACPI table dynamic creation.

Thanks

Eric
>      create_pcie_irq_map(vbi, vbi->gic_phandle, irq, nodename);
>  
>      g_free(nodename);
> @@ -1332,8 +1390,12 @@ static void machvirt_init(MachineState *machine)
>  
>      create_rtc(vbi, pic);
>  
> +    alloc_smmu_phandle(vbi);
if you follow my comment this disappears

Thanks

Eric
> +
>      create_pcie(vbi, pic, vms->highmem);
>  
> +    create_smmu(vbi, pic);
> +
>      create_gpio(vbi, pic);
>  
>      /* Create mmio transports, so the user can create virtio backends
> diff --git a/include/hw/arm/smmu.h b/include/hw/arm/smmu.h
> new file mode 100644
> index 0000000..bbb5e5d
> --- /dev/null
> +++ b/include/hw/arm/smmu.h
> @@ -0,0 +1,33 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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, write to the Free Software
> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + *
> + * Copyright (C) 2014-2015 Broadcom Corporation
> + *
> + * Author: Prem Mallappa <pmallapp@broadcom.com>
> + *
> + */
> +#ifndef HW_ARM_SMMU_H
> +#define HW_ARM_SMMU_H
> +
> +#define TYPE_SMMU_DEV_BASE "smmu-base"
> +#define TYPE_SMMU_DEV      "smmuv3"
> +
> +typedef enum {
> +    SMMU_IRQ_GERROR,
> +    SMMU_IRQ_PRIQ,
> +    SMMU_IRQ_EVTQ,
> +    SMMU_IRQ_CMD_SYNC,
> +} SMMUIrq;
> +
> +#endif
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 9650193..0b7138b 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -35,6 +35,7 @@
>  
>  #define NUM_GICV2M_SPIS       64
>  #define NUM_VIRTIO_TRANSPORTS 32
> +#define NUM_SMMU_IRQS          4
>  
>  #define ARCH_TIMER_VIRT_IRQ   11
>  #define ARCH_TIMER_S_EL1_IRQ  13
> @@ -54,6 +55,7 @@ enum {
>      VIRT_GIC_V2M,
>      VIRT_GIC_ITS,
>      VIRT_GIC_REDIST,
> +    VIRT_SMMU,
>      VIRT_UART,
>      VIRT_MMIO,
>      VIRT_RTC,
>