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

[Peter Maydell]

On 18 August 2018 at 02:09, Philippe Mathieu-Daudé <address@hidden> wrote:
> 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)...

It's following the same approach as the other existing
make_*() functions in this file, though.


>> +    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?

That would work and probably is better; again I was just following
the same thing I'd done in other make_*() functions earlier.

thanks
-- PMM