Re: [Qemu-arm] [PATCH 16/16] hw/arm/mps2-tz: Create PL081s and MSCs

[Philippe Mathieu-Daudé]

On 08/09/2018 10:01 AM, Peter Maydell wrote:
> The AN505 FPGA image includes four PL081 DMA controllers, each
> of which is gated by a Master Security Controller that allows
> the guest to prevent a non-secure DMA controller from accessing
> memory that is used by secure guest code. Create and wire
> up these devices.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  hw/arm/mps2-tz.c | 101 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 94 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
> index 22180c56fb7..7d92bc5fe1c 100644
> --- a/hw/arm/mps2-tz.c
> +++ b/hw/arm/mps2-tz.c
> @@ -45,7 +45,9 @@
>  #include "hw/misc/mps2-scc.h"
>  #include "hw/misc/mps2-fpgaio.h"
>  #include "hw/misc/tz-mpc.h"
> +#include "hw/misc/tz-msc.h"
>  #include "hw/arm/iotkit.h"
> +#include "hw/dma/pl080.h"
>  #include "hw/devices.h"
>  #include "net/net.h"
>  #include "hw/core/split-irq.h"
> @@ -75,8 +77,9 @@ typedef struct {
>      UnimplementedDeviceState i2c[4];
>      UnimplementedDeviceState i2s_audio;
>      UnimplementedDeviceState gpio[4];
> -    UnimplementedDeviceState dma[4];
>      UnimplementedDeviceState gfx;
> +    PL080State dma[4];
> +    TZMSC msc[4];
>      CMSDKAPBUART uart[5];
>      SplitIRQ sec_resp_splitter;
>      qemu_or_irq uart_irq_orgate;
> @@ -273,6 +276,65 @@ static MemoryRegion *make_mpc(MPS2TZMachineState *mms, 
> void *opaque,
>      return sysbus_mmio_get_region(SYS_BUS_DEVICE(mpc), 0);
>  }
>  
> +static MemoryRegion *make_dma(MPS2TZMachineState *mms, void *opaque,
> +                              const char *name, hwaddr size)
> +{
> +    PL080State *dma = opaque;
> +    int i = dma - &mms->dma[0];

This line is not trivial to read. I wondered "isn't this ptrdiff_t? why
not divide by sizeof(dma)...

> +    SysBusDevice *s;
> +    char *mscname = g_strdup_printf("%s-msc", name);
> +    TZMSC *msc = &mms->msc[i];
> +    DeviceState *iotkitdev = DEVICE(&mms->iotkit);
> +    MemoryRegion *msc_upstream;
> +    MemoryRegion *msc_downstream;
> +
> +    /*
> +     * Each DMA device is a PL081 whose transaction master interface
> +     * is guarded by a Master Security Controller. The downstream end of
> +     * the MSC connects to the IoTKit AHB Slave Expansion port, so the
> +     * DMA devices can see all devices and memory that the CPU does.
> +     */
> +    init_sysbus_child(OBJECT(mms), mscname, msc, sizeof(mms->msc[0]),

sizeof(*msc) easier to read?

> +                      TYPE_TZ_MSC);
> +    msc_downstream = sysbus_mmio_get_region(SYS_BUS_DEVICE(&mms->iotkit), 0);
> +    object_property_set_link(OBJECT(msc), OBJECT(msc_downstream),
> +                             "downstream", &error_fatal);
> +    object_property_set_link(OBJECT(msc), OBJECT(mms),
> +                             "idau", &error_fatal);
> +    object_property_set_bool(OBJECT(msc), true, "realized", &error_fatal);
> +
> +    qdev_connect_gpio_out_named(DEVICE(msc), "irq", 0,
> +                                qdev_get_gpio_in_named(iotkitdev,
> +                                                       "mscexp_status", i));
> +    qdev_connect_gpio_out_named(iotkitdev, "mscexp_clear", i,
> +                                qdev_get_gpio_in_named(DEVICE(msc),
> +                                                       "irq_clear", 0));
> +    qdev_connect_gpio_out_named(iotkitdev, "mscexp_ns", i,
> +                                qdev_get_gpio_in_named(DEVICE(msc),
> +                                                       "cfg_nonsec", 0));
> +    qdev_connect_gpio_out(DEVICE(&mms->sec_resp_splitter),
> +                          ARRAY_SIZE(mms->ppc) + i,
> +                          qdev_get_gpio_in_named(DEVICE(msc),
> +                                                 "cfg_sec_resp", 0));
> +    msc_upstream = sysbus_mmio_get_region(SYS_BUS_DEVICE(msc), 0);
> +
> +    init_sysbus_child(OBJECT(mms), name, dma, sizeof(mms->dma[0]), 
> TYPE_PL081);
> +    object_property_set_link(OBJECT(dma), OBJECT(msc_upstream),
> +                             "downstream", &error_fatal);
> +    object_property_set_bool(OBJECT(dma), true, "realized", &error_fatal);
> +
> +    s = SYS_BUS_DEVICE(dma);
> +    /* Wire up DMACINTR, DMACINTERR, DMACINTTC */
> +    sysbus_connect_irq(s, 0, qdev_get_gpio_in_named(iotkitdev,
> +                                                    "EXP_IRQ", 58 + i * 3));
> +    sysbus_connect_irq(s, 1, qdev_get_gpio_in_named(iotkitdev,
> +                                                    "EXP_IRQ", 56 + i * 3));
> +    sysbus_connect_irq(s, 2, qdev_get_gpio_in_named(iotkitdev,
> +                                                    "EXP_IRQ", 57 + i * 3));
> +
> +    return sysbus_mmio_get_region(s, 0);
> +}
> +
>  static void mps2tz_common_init(MachineState *machine)
>  {
>      MPS2TZMachineState *mms = MPS2TZ_MACHINE(machine);
> @@ -299,13 +361,14 @@ static void mps2tz_common_init(MachineState *machine)
>                               &error_fatal);
>  
>      /* The sec_resp_cfg output from the IoTKit must be split into multiple
> -     * lines, one for each of the PPCs we create here.
> +     * lines, one for each of the PPCs we create here, plus one per MSC.
>       */
>      object_initialize(&mms->sec_resp_splitter, 
> sizeof(mms->sec_resp_splitter),
>                        TYPE_SPLIT_IRQ);
>      object_property_add_child(OBJECT(machine), "sec-resp-splitter",
>                                OBJECT(&mms->sec_resp_splitter), &error_abort);
> -    object_property_set_int(OBJECT(&mms->sec_resp_splitter), 5,
> +    object_property_set_int(OBJECT(&mms->sec_resp_splitter),
> +                            ARRAY_SIZE(mms->ppc) + ARRAY_SIZE(mms->msc),
>                              "num-lines", &error_fatal);
>      object_property_set_bool(OBJECT(&mms->sec_resp_splitter), true,
>                               "realized", &error_fatal);
> @@ -406,10 +469,10 @@ static void mps2tz_common_init(MachineState *machine)
>          }, {
>              .name = "ahb_ppcexp1",
>              .ports = {
> -                { "dma0", make_unimp_dev, &mms->dma[0], 0x40110000, 0x1000 },
> -                { "dma1", make_unimp_dev, &mms->dma[1], 0x40111000, 0x1000 },
> -                { "dma2", make_unimp_dev, &mms->dma[2], 0x40112000, 0x1000 },
> -                { "dma3", make_unimp_dev, &mms->dma[3], 0x40113000, 0x1000 },
> +                { "dma0", make_dma, &mms->dma[0], 0x40110000, 0x1000 },
> +                { "dma1", make_dma, &mms->dma[1], 0x40111000, 0x1000 },
> +                { "dma2", make_dma, &mms->dma[2], 0x40112000, 0x1000 },
> +                { "dma3", make_dma, &mms->dma[3], 0x40113000, 0x1000 },
>              },
>          },
>      };
> @@ -490,12 +553,32 @@ static void mps2tz_common_init(MachineState *machine)
>      armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename, 
> 0x400000);
>  }
>  
> +static void mps2_tz_idau_check(IDAUInterface *ii, uint32_t address,
> +                               int *iregion, bool *exempt, bool *ns, bool 
> *nsc)
> +{
> +    /*
> +     * The MPS2 TZ FPGA images have IDAUs in them which are connected to
> +     * the Master Security Controllers. Thes have the same logic as
> +     * is used by the IoTKit for the IDAU connected to the CPU, except
> +     * that MSCs don't care about the NSC attribute.
> +     */
> +    int region = extract32(address, 28, 4);
> +
> +    *ns = !(region & 1);
> +    *nsc = false;
> +    /* 0xe0000000..0xe00fffff and 0xf0000000..0xf00fffff are exempt */
> +    *exempt = (address & 0xeff00000) == 0xe0000000;
> +    *iregion = region;
> +}
> +
>  static void mps2tz_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> +    IDAUInterfaceClass *iic = IDAU_INTERFACE_CLASS(oc);
>  
>      mc->init = mps2tz_common_init;
>      mc->max_cpus = 1;
> +    iic->check = mps2_tz_idau_check;
>  }
>  
>  static void mps2tz_an505_class_init(ObjectClass *oc, void *data)
> @@ -516,6 +599,10 @@ static const TypeInfo mps2tz_info = {
>      .instance_size = sizeof(MPS2TZMachineState),
>      .class_size = sizeof(MPS2TZMachineClass),
>      .class_init = mps2tz_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_IDAU_INTERFACE },
> +        { }
> +    },
>  };
>  
>  static const TypeInfo mps2tz_an505_info = {
> 

mps2-tz is now a piece of art...

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>