Re: [Qemu-arm] Problems adding TMS570LC43 - Cortex-R5f

[Peter Maydell]

On Thu, 17 Jan 2019 at 17:20, João Gaspar
<address@hidden> wrote:
>
> Hi!
> I'm new at QEMU so I have some questions.

Hi; I'll see if I can answer some of them. Note that creating
a new machine model isn't a trivial task. You should think of
it as about the same amount of effort and difficulty as doing
a port of Linux to a new piece of hardware.

> I'm trying to create a new support machine to QEMU, the TMS570LC43 board, 
> this board use the Cortex-R5f microcontroller.
> At this moment when I do the "qemu-system-arm -M help" my machine is showed 
> with the name "cortex-r5".

This isn't really a good name for the board, because
"cortex-r5" is a CPU, not a board. You should call it
"TMS570LC43x" or something similar, because that's what
you're modelling here.

> The probleme start when I try to instaciate the machine with the comand 
> "qemu-system-arm --machine cortex-r5 -cpu cortex-r5f", after that I received 
> the message "segmentation fault (core dump)".
>
> Does anyone have an possible cause to my problem or an tip?

This means your code has a bug in it :-)

You need to run QEMU under a debugger and look at the back trace
to find out where it's crashing and why.

> I'm gonna let the code too, because probably I'm doing aniything wrong!

I have a few general notes below, which aren't necessarily
where your bug is (though there is one part that looks
like it probably is).

> //RTI -> Real Time Interruption
> //static const uint32_t RTI_addr = 0xFFFFFC00;
>
> //SPI
> static const uint32_t spi_addr[CR5_NUM_SPI] = {0xFFF7F400, 0xFFF7F600, 
> 0xFFF7F800, 0xFFF7FA00, 0xFFF7FC00};
> //I2C
> static const uint32_t i2c_addr[CR5_NUM_I2C] = {0xFFF7D400, 0xFFF7D500};
> //CAN
> static const uint32_t can_addr[CR5_NUM_CAN] = {0XFFF7DC00, 0XFFF7DE00, 
> 0X0FFF7E00, 0X0FFF7E200};
> //ADC
> static const uint32_t adc_addr[CR5_NUM_ADC] = {0xFFF7C000, 0xFFF7C200};
> //GIO
> static const uint32_t gio_addr[CR5_NUM_GIO] = {0xFFF7BC00};
>
> /*IRQ
> Interrupt Request Assignement
> http://www.ti.com/lit/ds/spns195c/spns195c.pdf
> Page: 118
> */
> //irq RTI
> /* static const int rti_cmp_irq0 = 2;
> static const int rti_cmp_irq1 = 3;
> static const int rti_cmp_irq2 = 4;
> static const int rti_cmp_irq3 = 5;
> static const int rti_oflw_irq0 = 6;
> static const int rti_oflw_irq1 = 7;
> static const int rti_timebase_irq = 8; */

These are weird. You'll notice that they don't follow the
style the rest of QEMU uses, which is mostly #defines for
constants. In general, you should follow some existing
(and ideally recently added) code that does the sort of
thing you're trying to do.


> //irq SPI
> static const int spi_l0_irq[CR5_NUM_SPI] = {12, 17, 37, 49, 53};
> static const int spi_l1_irq[CR5_NUM_SPI] = {26, 30, 38, 54, 56};
> //irq I2C
> static const int i2c_irq[CR5_NUM_I2C] = {66, 114};
> //irc CAN
> static const int can_l0_irq[CR5_NUM_CAN] = {16, 35, 45, 113};
> static const int can_l1_irq[CR5_NUM_CAN] = {29, 42, 55, 117};
> static const int can_if3_irq[CR5_NUM_CAN] = {44, 46, 60, 120};
> //irq ADC
> static const int adc_swgroup1_irq[CR5_NUM_ADC] = {15, 51};
> static const int adc_swgroup2_irq[CR5_NUM_ADC] = {28, 57};
> static const int adc_eventgroup_irq[CR5_NUM_ADC] = {14, 50};
> static const int adc_magnitudecmp_irq[CR5_NUM_ADC] = {31, 59};
> //irq GIO
> static const int gio_high_irq[CR5_NUM_GIO] = {9};
> static const int gio_low_irq[CR5_NUM_GIO] = {23};
>
>
> static void cortexR5_initf(MachineState *mc)
> {
>     CORTEXR5State *s = g_new0(CORTEXR5State, 1);

This isn't right -- you already have a state struct,
that's what the mc pointer is. You can add space for
anything you want at the machine level by subclassing
MachineState, and then casting the pointer here to
your subclass. hw/arm/aspeed.c (among others) has an
example of that.

>
>     DeviceState *dev;
>     SysBusDevice *busdev;
>     Error *err = NULL;
>
>     int i;
>     ARMCPU *cpu;
>
>
>     MemoryRegion *system_memory = get_system_memory();
>     MemoryRegion *ram = g_new(MemoryRegion, 1);
>     MemoryRegion *flash = g_new(MemoryRegion, 1);
>     MemoryRegion *flash_alias = g_new(MemoryRegion, 1);
>
>
>
>     //s->cr5_cpu = ARM_CPU(object_new(mc->cpu_type));
>     cpu = ARM_CPU(object_new(mc->cpu_type));

You don't want to create the CPU at the top level "machine"
level of your code. (It can be made to work, but it's structured
all wrong.) What you want is to model the hardware:

 * have a model of the SoC (which is the chip on your test
   board which includes the CPU and typically various other devices
   such as UARTs, GPIOs, etc etc)
 * have a model of the board, which creates the SoC, and
   anything else that is on the board but not part of the
   SoC (usually including the main system memory)
 * devices that are part of the SoC go in their own files
   in hw/<whatever>/

You can see examples of this in
hw/arm/aspeed.c (board model) and hw/arm/aspeed_soc.c (SoC model).


>     //object_initialize_child(obj, "cpu", &s->cr5_cpu, sizeof(s->cr5_cpu), 
> ARM_CPU_TYPE_NAME("cortex-r5f"), &error_abort, NULL);
>     object_property_set_bool(OBJECT(cpu), true, "realized", &error_fatal);
>
>     memory_region_init_ram(flash, NULL, "CORTEXR5.flash", FLASH_SIZE, 
> &error_fatal);
>
>     memory_region_init_alias(flash_alias, NULL, "CORTEXR5.flash.alias", 
> flash, 0, FLASH_SIZE);
>
>     memory_region_set_readonly(flash, true);
>     memory_region_set_readonly(flash_alias, true);
>
>     memory_region_add_subregion(system_memory, FLASH_BASE_ADDRESS, flash);
>     memory_region_add_subregion(system_memory, 0, flash_alias);
>
>     memory_region_init_ram(ram, NULL, "CORTEXR5.ram", SRAM_SIZE, 
> &error_fatal);
>     memory_region_add_subregion(system_memory, RAM_BASE_ADDRESS, ram);
>
>     //SPI
>     for (i = 0; i < CR5_NUM_SPI; i++)
>     {
>         dev = DEVICE(&(s->spi[i]));
>         object_property_set_bool(OBJECT(&s->spi[i]), true, "realized", &err);

You never created this device, so attempting to 'realize' it is going
to break badly (and is probably where your segfault is coming from).

QEMU's device object model has a sort of multiple-phase creation:
 * "init" -- usually either by calling 'object_initialize()' or
   'object_initialize_child()' on some piece of already-allocated
   memory, or by calling 'object_new()' or 'qdev_create()' to
   do 'allocate and init'.
 * set properties on the device to configure it if necessary
 * "realize" (either by setting the 'realized' property, or by
   calling qdev_init_nofail())
 * then you can map its MMIO regions and connect its IRQ lines

There are two styles of creating devices that you can find in
QEMU code:
 * the older style uses qdev_* functions, which are usually
   more compact in terms of numbers of lines of source code.
   These families of functions create devices in newly allocated
   memory, so a "container" device like an SoC ends up with a
   state struct full of pointers to other devices.
 * a newer style tends to embed the sub-devices into the state
   struct of the container device, and thus uses object_initialize_child()
   and similar object_* functions rather than the qdev functions.
   The SoC object usually does the "init" part of the creation
   of its subdevices in its own "init" method, and the "realize"
   part in its own "realize".

Part of your problem here seems to be that you've taken code from
an SoC object which uses the latter style and put it into a
machine function where you haven't set up a proper subclass
of MachineState, and also have only taken the 'realize' part
and not the 'init' part. (Machines don't have split 'init' and
'realize' methods, so the machine init function needs to do
both things.)

>         if (err != NULL)
>         {
>             //error_propagate(errp, err);
>             return;
>         }
>         busdev = SYS_BUS_DEVICE(dev);
>         sysbus_mmio_map(busdev, 0, spi_addr[i]);
>         sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(DEVICE(dev), 
> spi_l0_irq[i]));
>         sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(DEVICE(dev), 
> spi_l1_irq[i]));
>     }


>
> static void cortexR5_machine_init(MachineClass *mc){
>    //mc->name = "Cortex-R5f";
>     mc->desc = "Based on TMS570LC43x";
>     mc->init =  cortexR5_initf;
>     mc->max_cpus = 1;
>     mc->ignore_memory_transaction_failures = true;

This flag should never be set for new board models -- it is
only present for handling legacy old boards where we don't
know what guest code we might break by setting it to false.
For new boards you can create the right devices for the guest
code you care about to run. (There is an "unimplemented-device"
in hw/misc which can be used to stub out areas of memory where
there are devices in hardware which you haven't written yet.)

PS: if you run scripts/checkpatch.pl it will let you know about
various minor coding style nits like brace placement and
comment style that you might like to fix.

thanks
-- PMM